Scrape an HTML table with python I think I'm on the right track, but ANY suggestions or critques are welcome.  The program just scrapes an HTML table and prints it to stdout.

import sys
from urllib2 import urlopen, URLError
from argparse import ArgumentParser
from bs4 import BeautifulSoup

def parse_arguments():
    """ Process command line arguments """
    parser = ArgumentParser(description='Grabs tables from html')
    parser.add_argument('-u', '--url', help='url to grab from',
                        required=True)
    args = parser.parse_args()
    return args

def parse_rows(rows):
    """ Get data from rows """
    results = []
    for row in rows:
        table_headers = row.find_all('th')
        if table_headers:
            results.append([headers.get_text() for headers in table_headers])

        table_data = row.find_all('td')
        if table_data:
            results.append([data.get_text() for data in table_data])
    return results

def main():
    # Get arguments
    args = parse_arguments()
    if args.url:
        url = args.url

    # Make soup
    try:
        resp = urlopen(url)
    except URLError as e:
        print 'An error occured fetching %s \n %s' % (url, e.reason)
        return 1
    soup = BeautifulSoup(resp.read())

    # Get table
    try:
        table = soup.find('table')
    except AttributeError as e:
        print 'No tables found, exiting'
        return 1

    # Get rows
    try:
        rows = table.find_all('tr')
    except AttributeError as e:
        print 'No table rows found, exiting'
        return 1

    # Get data
    table_data = parse_rows(rows)

    # Print data
    for i in table_data:
       print '\t'.join(i)

if __name__ == '__main__':
    status = main()
    sys.exit(status)

In general, your code is very well written. PEP8 compliance is very good, names make sense, you have docstrings, you use if __name__ == '__main__:.

Here's a few comments on what I would do differently:

# Get table
try:
    table = soup.find('table')
except AttributeError as e:
    print 'No tables found, exiting'
    return 1

# Get rows
try:
    rows = table.find_all('tr')
except AttributeError as e:
    print 'No table rows found, exiting'
    return 1

This is essentially the same code duplicated twice. The fact that you differentiate between not finding a table and not finding rows within it is nice, but ultimately useless, as a table with nothing in it might as well be no table at all.

Therefore, I propose changing it to detect a valid table, that is one with rows. This tidies up the code a bit like so:

try:
    table = soup.find('table')
    rows = table.find_all('tr')
except AttributeError as e:
    raise ValueError("No valid table found")

You'll notice I eliminated the print and the return 1. Errors should be errors, they shouldn't magically become print statements. The only exception to this is if this is supposed to be for end users, then you will want to retain the print. Secondly, sys.exit() is unnecessary as the script will end automatically when it reaches the end of the file. Therefore, all the return 1's are unnecessary and are extra fluff.

Lastly, let's talk about your comments. Most of them are useless.

# Make soup

Is that really the best description? You are retrieving a url and reading the HTML. THAT should be your comment.

# Get table

# Get rows

As I said, I think these two can be wonderfully merged. Also, I'm pretty sure the names are more than sufficient to understand that you are retrieving the table and the rows of the table.

# Get data
table_data = parse_rows(rows)

Hmm... what else could table_data be? This is a comment that seems pretty useless and adds un-needed fluff.

# Print data

This one I actually think should be more descriptive, such as:

# Print table data

As indicated in the comments, it is suggested that you remove all of your comments, as they are largely plain fluff considering your naming is so well done.

Another last thing to note is that

if args.url:

Is a completely useless check. It is guaranteed that args.url will always exist as per how the argparse module functions. If you made the url an optional parameter, that would be a different story, but then your program would be useless.


Are these C# and Python code snippets functionally identical? I have a bit of Python source code that I want to recreate in C#. The code is about reading and decrypting a binary file. I have tested the function on an existing file and it runs without errors; while the resulting string is not garbled or anything, it does not appear to be useful to me.

But that is outside the scope of this question.  I only want to know if I have translated the function correctly to C# so it does the same as in Python.

The Python code:

    filename = os.path.basename(path_and_filename)
    key = 0
    for char in filename:
        key = key + ord(char)
    f = open(path_and_filename, 'rb')
    results = ''
    tick = 1
    while True:
        byte = f.read(2)
        if byte:
            if tick:
                key = key + 2
            else:
                key = key - 2
            tick = not tick
            res = struct.unpack('h', byte)[0] ^ key
            results = results + chr(res)
            continue
        break
    f.close()
    return results

path_and_filename is an absolute path to the encrypted file.

Here is the C# code I wrote:

        string path = _path;
        string fileName = _fileName;
        char[] chars = fileName.ToCharArray();
        int key = 0;
        foreach (char c in chars)
        {
            key += c;
        }
        StringBuilder builder = new StringBuilder();

        using (FileStream file = new FileStream(path, FileMode.Open))
        {
            bool tick = true;

            while (true)
            {
                int i = file.ReadByte();
                if (i == -1)
                    break;
                if (tick)
                    key += 2;
                else
                    key -= 2;
                tick = !tick;

                //The next 2 lines are for parsing the short, equivalent(?) to struct.unpack('h', byte)
                i <<= 8;
                i += file.ReadByte();

                i ^= key;
                byte b = (byte)i;
                char c = (char)b;
                builder.Append(c);
            }
        }
        string result = builder.ToString();

Never mind the dirtiness of the code. Will those 2 snippets give the same output to a certain input?

This section seems wrong:

//The next 2 lines are for parsing the short, equivalent(?) to struct.unpack('h', byte)
i <<= 8;
i += file.ReadByte();

you're shifting the value in i 8 bits left and then reading another byte. I'm guessing because the original code read it in 2 bytes at a time. If your file isn't the right length then this will break it here. what about:

i <<= 8;
int secondByte = file.ReadByte()
if(secondByte != -1)
{
   i += secondByte ;
}

Whats with this? It is casting an int to a byte to a char and it is truncating.

byte b = (byte)i;
char c = (char)b;

so you're better off ignoring the first byte and instead everything after tick = !tick;
could be:

i = file.ReadByte();

char c = (char)(i ^ (key & 255));
builder.Append(c);

In Python chr() will throw an exception if the value is greater than 255 anyway.

And IMHO it is a bad idea to roll your own encryption/decryption like this. Use a trusted known solution instead.

Question: I only want to know if I have translated the function correctly to C# so it does the same as in Python

Answer Its more of a transliteration rather than a translation but they should give the same output. If I were to re-write the python into c# it would look different as it seems inefficient to me.


Design of Experiments data structure with C++17 I've defined a data structure which represents a doe_model, providing two maps with a configuration_id as key and a configuration_model as value. The configuration_model is an unordered_map which has a knob_name as key and a knob_value as value. There are things I don't like very much about this but I can't get a grasp on how I should change them. For instance: using the next and get_next() trick I'm forced to add a copy constructor and an assignment operator in order to get rid of the undefined behavior I would get by shallow copying the next iterator member. But I don't know if there's a better way to achieve what I need, which is: since I have a thread pool which concurrently access the doe data structure, I have to provide them the next configuration to explore in a smart way. So if I'm reaching the end of the configurations (and there are still some to use) I have to return them from the begin, until I've exhausted all of them.
Here's the code:
using configuration_model = std::unordered_map<std::string, std::string>;

struct doe_model {

  doe_model() {}
  doe_model(const doe_model& doe)
  {
    required_explorations = doe.required_explorations;
    number_of_explorations = doe.number_of_explorations;
    next = required_explorations.end();
  }

  inline doe_model& operator=(const doe_model& ldoe)
  {
    required_explorations = ldoe.required_explorations;
    number_of_explorations = ldoe.number_of_explorations;
    next = required_explorations.end();
    return *this;
  }

  inline bool add_config(const std::string &config_id,
                         const configuration_model &config,
                         const int required_number_of_observations) {
    bool assignment_took_place =
        !required_explorations.insert_or_assign(config_id, config).second ||
        !number_of_explorations
             .insert_or_assign(config_id, required_number_of_observations)
             .second;
    next = required_explorations.end();
    return assignment_took_place;
  }

  inline void update_config(const std::string &config_id) {
    number_of_explorations.at(config_id)--;

    // remove the configuration in case we exausted all the explorations
    if (number_of_explorations.at(config_id) <= 0)
      remove_config(config_id);
  }

  inline void remove_config(const std::string &config_id) {
    required_explorations.erase(config_id);
    number_of_explorations.erase(config_id);
  }

  // this method returns the next configuration to explore
  // NOTE: the caller MUST check the pointer first.
  inline std::map<std::string, configuration_model>::iterator get_next() {
    // we may have an empty map or one with only a single configuration left
    if (required_explorations.empty() || next == required_explorations.end()) {
      next = required_explorations.begin();
      return next;
    }

    next++;

    return next;
  }

  // key is the configuration_id
  std::map<std::string, configuration_model> required_explorations;
  std::map<std::string, configuration_model>::iterator next;
  std::map<std::string, int> number_of_explorations;
};

Because you're defining your member functions within the class declaration, you don't need to use the inline keyword. All member functions declared in the class declaration are implicitly inline (including the default and copy constructors) are inline.
In your copy constructor, you can make use of member initializers to construct the copied maps directly, rather than default construct and assign:
doe_model(const doe_model& doe): required_explorations(doe.required_explorations),
    next(required_explorations.end()), number_of_explorations(doe.number_of_explorations)
}

There are a few places where you're using two statements but they can be combined.
In update_config, you can decrement in the test:
if (--number_of_explorations.at(config_id) <= 0)

In get_next, you can combine the iterator increment and return statement:
return ++next;

This also uses the preincrement on the iterator, as it avoids creating a copy of the original iterator that is returned by the postincrement version, then immediately discarded.
In add_config, you have the potential of adding a config with a higher number of required explorations than are provided if that config already exists (if required_explorations is assigned to, no change is made to number_of_explorations). This may or may not be a problem.
You should consider adding a move constructor and a move assignment operator, which can avoid creating copies.


Encapsulating Complex Calculation in JavaScript This is code for performing a calculation that should be repeated with different levels and temperatures. I am concerned about the organization of this code as I feel it may contain too many helper functions and seems a little unnecessarily complicated. Only the getCharge() method needs to be exposed to the browser.

There is sample usage of this code after the initial definition.

var hprVessel = function (level, temp) {
/**
* @params {Number} level - Percentage load for HPR
* @params {Number} temp - temperature of fluid
*/
    var liquidCoefficients = [
        83.4485407734612,
        -0.111449280945489,
        0.000100022560084721,
        -0.00000206240663899928
    ];

    var vaporCoefficients = [
        0.895329190066827,
        0.016635773286873,
        -0.0000894924196507015,
        0.00000206296636296276
    ];

    var vessel = {
        // Vessel-specific Params
        diameter: 1.9583 * level,
        height: 0.2937 * level,
        length: 15.9583 * level,

        // Calculations
        getLiquidVol: function () {
            var r = this.diameter/2;
            var sectionArea = Math.pow(r, 2) * Math.acos((r - this.height) / r);
            var triangleArea = Math.sqrt(Math.pow(r, 2) - Math.pow((r-this.height), 2))*(r-this.height);

            return (this.length * sectionArea * triangleArea);
        },
        getVesselVol: function () {
            return (Math.PI * Math.pow(this.diameter, 2) * this.length);
        },
        getVaporVol: function () {
            return (vessel.getVesselVol() - vessel.getLiquidVol());
        },
        getLiquidCharge: function (temp) {
            return (vessel.getLiquidVol() * vessel.calcDensity(liquidCoefficients, temp));
        },
        getVaporCharge: function (temp) {
            return (vessel.getVaporVol() * vessel.calcDensity(vaporCoefficients, temp));
        },
        chargeHPR: function (temp) {
            return (1000 + vessel.getLiquidCharge(temp) + vessel.getVaporCharge(temp));
        },

        // Helper functions
        calcDensity: function (regressionCoefs, temp) {
            var liqDensityArray = regressionCoefs.map(function (coef) {
                return coef * Math.pow(temp, regressionCoefs.indexOf(coef));
            });

            return vessel.sum(liqDensityArray);
        },
        sum: function (arr) {
            return arr.reduce( function (prev, current) {
                return (prev + current);
            }
        }
    };

    return {
        getCharge: function() {
            return vessel.chargeHPR(temp);
        }
    };
}

var vessel = hprVessel(1, -30);
var result = vessel.getCharge();

diameter: 1.9583 * level,
height: 0.2937 * level,
length: 15.9583 * level,

It might be wise to actually put these numbers in a constant and naming them appropriately. That way, in the long run, you won't forget what these values are.

return (/* stuff */);

I notice you also use parens in your returns. Unless they're a personal preference, they're actually optional (What do you think this is? PHP? :P)

Now this code appears to benefit more using prototypal inheritance rather than a factory pattern. That's because every time you call hprVessel, it builds an object of functions. With prototypal inheritance, your instance will only hold instance properties, aka "vessel-specific data". The functions will be shared across your vessel instances via the prototype.

Here's a short example:

function HPRVessel(level, temp){

  // Instance data, aka vessel-specific data
  this.temp = temp;
  this.level = level;
  this.diameter = HPRVessel.SOME_CONSTANT * level,
  this.height = HPRVessel.ANOTHER_COSNTANT * level,
  this.length = HPRVessel.YET_ANOTHER_CONSTANT * level,

}

// pseudo-static properties
// Here we append general HPRVessel constants
HPRVessel.liquidCoefficients = [
  83.4485407734612,
  -0.111449280945489,
  0.000100022560084721,
  -0.00000206240663899928
];

HPRVessel.SOME_CONSTANT = 1.9583
HPRVessel.ANOTHER_COSNTANT = 0.2937
HPRVessel.YET_ANOTHER_CONSTANT = 15.9583

// Here, we define shared stuff. Best practice is to just put methods on it.
HPRVessel.prototype.getCharge = function(){
  // Anything on the instance is accessible via `this`
  return this._chargeHPR(this.temp);
};

// Since there's no "private", we use the convention of prefixing `_` to tell
// curious people not to call these methods directly
HPRVessel.prototype._chargeHPR= function(){
  return (1000 + this._getLiquidCharge(this.temp) + this._getVaporCharge(this.temp));
};

// Usage:
var vessel = new HPRVessel(1, -30);
var result = vessel.getCharge();


Date picker consisting of three component pickers I made a UI picker tool, like a "rolling select" to pick vales (demo here).

I made first a independent module with a ES6 Class, basically:

class ComponentPicker {

    constructor(target, data){
        // init stuff
    }

  deployHTML(target, data) {...}
    getValue(){...}
    setValue(val){...}
    onChange(callback){...}
}

Then I added a new layer to it. A Date picker that needs 3 component pickers. I thought about extending the class ComponentPicker but I didn't find a way to call 3 times super, it would look like

class DatePicker extends ComponentPicker{
  constructor(length) {
      this.pickers = data.map(component=>{
            return super(target, component); // <-- weird and wrong

        });
  }

So I ended up doing a "good old" ES5- style of class like:

function DatePicker (target, data) {
    this.wrapper = document.createElement('div');
    this.wrapper.className = 'date-picker';
    target.appendChild(this.wrapper);
    this.pickers = data.map(component=>{
        return new ComponentPicker(target, component);
    });
    return this;
}
DatePicker.prototype.getValue = function(){
    return this.pickers.map(picker=>{
        return picker.getValue();
    });
}
DatePicker.prototype.setValue = function(values){
    // etc

and calling it with new DatePicker();

It works, but is there another way to reuse/extend the first class into another one that needs to interact with 3 of the original?

So if I understand correctly, you want to create an instance of DatePicker which by default has 3 instances of ComponentPicker in an array called pickers? Then the following should work:

class DatePicker extends ComponentPicker{
  constructor(length) {
    this.pickers = data.map(component => new ComponentPicker(target, component));
  }


The optimized way to find the top k leading candidates from a unsorted hash map in python I need to write a method that will accept a timestamp and takes an array of votes, I need to return the k leading candidates with that timestamp. I came up with the following solution,

Here is the input to the methods:

votes = [{'candidate':'a', 'timestamp':2},{'candidate':'c', 'timestamp': 5},{'candidate':'c', 'timestamp': 12}]

timestamp = 5

k = 5

And the method to solve the problem,

def leading_candidates(votes, timestamp,k):

    candidates = {}
    leading_candidates = []

    for vote in votes:
        if vote['timestamp'] <= timestamp:
            if vote['candidate'] not in candidates:
                candidates[vote['candidate']] = 1
            else:
                candidates[vote['candidate']] += 1

    sorted_votes = sorted(candidates.values())[:k]

    for candidate in candidates:
        if candidates[candidate] in sorted_votes:
            leading_candidates.append(candidate)

    return leading_candidates

print(leading_candidates(votes, timestamp, 2))

As you can see the second solution has a time complexity of \$O(k\,n)\$ where k is the time it takes to find the index in the leading candidates sorted array, In the worst case, it can be \$O(n^2)\$ and because of sorting it may be at least \$O(n\,\log n)\$.

Is there any way we can make it work with \$O(n)\$?

So, you want to count something and afterwards get the top k? That sounds like a job for collections.Counter!

from collections import Counter

def leading_candidates(votes, timestamp, k):
    vote_counts = Counter(vote['candidate']
                          for vote in votes
                          if vote['timestamp'] <= timestamp)
    return [candidate[0] for candidate in vote_counts.most_common(k)]

if __name__ == "__main__":
    print(leading_candidates(votes, timestamp, 2))

This way you don't need to special case a candidate not yet having received a vote (something you could have also done with a collections.defaultdict(int)). And it is \$\mathcal{O}(n)\$.

Also note that if k is large, the line if candidates[candidate] in sorted_votes will become slow, as it is a linear scan. At the same time, you can iterate over the keys and values of a dictionary at the same time with candidates.items(), so you don't need to do candidates[candidate].

Python has an official style-guide, PEP8, which recommends using spaces after commas, which you forgot to do before k in the function signature.

You should always guard your code with an if __name__ == "__main__": guard to allow importing from the script without running it.


Transform poor CSS into a more beatiful form in Dart I have this script that opens a file containing (possibly) bad CSS, lexes the content of it and creates a new file with the same code in a more pleasant way to look at. It formats it and sorts the properties alphabetically.

I would love to hear some very critical feedback on coding style, best practises and how I can make it cleaner.

Some concerns I have right now are the global variables (like i and iSort), the hard coded file path and the use of while loops.

I am trying to make it as easy as possible for others to read, but to me it seems as if passing around file[i] and other variables makes it more cluttered than it should be.

The exception handling seems unnecessary, but without it, the index overflows at some point and the program execution just stops. Is there a way I can avoid it in this case?

import 'dart:io';
import "dart:core";

class Token {
  String identifier;
  String value;
  Token(this.identifier, this.value);
}

List<Token> tokens = [];

void main() {
  String rootPath = "C:/Users/User/Desktop/Datein/AlphaCSS/bin/";
  String originalFilePath = "samplecss.css";

  new File(rootPath + originalFilePath).readAsString().then((String file) {
    List<Token> tokens = getTokensFromFile(file);
    List<Token> sortedTokens = getSortedTokens(tokens);
    new File(rootPath + "newcss.txt").writeAsStringSync(reconstructCSS(sortedTokens));
  });
}

int i = 0;

List<Token> getTokensFromFile(file){
  String value = "";
  while (i < file.length-1){
    value = "";
    if (isValidStart(file[i])){
      value = getTokenContent(file);
      addTokenBasedOnType(file[i], value);
    }
    else if (isEndOfSelector(file[i])){
      addTokenBasedOnType(file[i], "");
    }
    i++;
  }
  return tokens;
}

bool isValidStart(String char){
  RegExp regex = new RegExp(r"[a-zA-Z0-9#.]");
  return (regex.hasMatch(char)) ? true : false;
}

bool isEndOfSelector(String char){
  return (char == "}") ? true : false;
}

String getTokenContent(String file){
  String content = "";
  while (file[i] != "{" && file[i] != ";"){
    content += file[i];
    i++;
  }
  return content;
}

void addTokenBasedOnType(String char, String value){
  if (char == "{"){
    tokens.add(new Token("SELECTOR", value));
  }
  else if (char == "}"){
    tokens.add(new Token("END", ""));
  }
  else {
    tokens.add(new Token("PROPERTY", value));
  }
}

int iSort = 0;

List<Token> getSortedTokens(tokens){

  List<Token> sortedTokens = [];
  try {
    while (iSort < tokens.length){
      if (tokens[iSort].identifier == "PROPERTY"){
        List<Token> toSort = getAllPropertiesInSelector();
        iSort--;
        toSort.sort((current, next) => current.value.compareTo(next.value));
        for (var elem in toSort){
          sortedTokens.add(elem);
        }
      }
      else {
        sortedTokens.add(tokens[iSort]);
      }
      iSort++;
    }
  }
  catch(ex) { }
  return sortedTokens;
}

List<Token> getAllPropertiesInSelector(){
  List<Token> properties = [];
  try {
    while (tokens[iSort].identifier == "PROPERTY"){
      properties.add(tokens[iSort]);
      iSort++;
    }
  }
  catch(ex) { };
  return properties;
}

String reconstructCSS(List<Token> sortedTokens) {
  String finalString = "";

  for (var elem in sortedTokens){
    if (elem.identifier == "SELECTOR"){
      finalString += "${elem.value}{\n";
    }
    else if (elem.identifier == "END"){
      finalString += "}\n\n";
    }
    else if (elem.identifier == "PROPERTY"){
      finalString += "  ${elem.value};\n";
    }
  }
  return finalString;
}

Sorting the properties alphabetically does not yield "the same code". Properties that appear later in a rule override properties that occur earlier in the same rule. Mozilla Developer Network gives an example of a tricky edge case:

  A value which is not specified is set to its initial value. That sounds anecdotal, but it really means that it overrides previously set values. Therefore:

background-color: red;
background: url(images/bg.gif) no-repeat top right;

  will not set the color of the background to red but to background-color's default, transparent, as the second rule has precedence.

If you reverse the properties in that example, you would get a different result.


Swap even and odd bits of unsigned 8-bit integer The task:

  Given an unsigned 8-bit integer, swap its even and odd bits. The 1st
  and 2nd bit should be swapped, the 3rd and 4th bit should be swapped,
  and so on.

  For example, 10101010 should be 01010101. 11100010 should be 11010001.

  Bonus: Can you do this in one line?

My solutions:

// imperative:
function swap8BitInteger(bytes){
  let copiedBytes = '';
  for(let i = 0; i < bytes.length; i+=2) {
    copiedBytes+= `${bytes[i+1]}${bytes[i]}`;
  }
  return copiedBytes;
}

console.log(swap8BitInteger('11100010'));

// I don't know wether this qualifies as a one-liner
const swap8BitInteger2 = bytes => bytes.split('').reduce((acc, bit, idx) => {acc[idx % 2 === 0 ? idx + 1 : idx - 1] = bit;return acc;}, []).join('');

console.log(swap8BitInteger2('11100010'));

// Not sure wether this is still readable
const swap8BitInteger3 = bytes => bytes.split('').reduce((acc, _, idx) => idx % 2 ? acc : acc + bytes[idx + 1] + bytes[idx], '');

console.log(swap8BitInteger3('11100010'));

Would be interested to see an even shorter (and/or more readable) solution.

OMDG arrays to manipulate bits
Bit manipulation is the most basic type of operation a computer can do. These days it can mess with 64 bit in one go. JS number (as signed int 32) is limited to 32 (to keep us in our place)
BTW JS has 8, 16, 32bit signed and unsigned arrays.
In JS you can write binary numbers with the 0b prefix. Eg 0b111 is 7
To swap even odd, you shift all bits to the left << 1 (same as * 2) and mask out & 0b10101010 the odd bits. The for the even you shift all bits to the right >> 1 similar to /2, mask out the even bits & 0b101010101 and add or or the result of the previous shift.
Example show two ways of doing the same thing.

const swapEvenOdd = char => ((char << 1) & 0b10101010) | ((char >> 1) & 0b1010101);

const swapEvenOdd2 = char => ((char * 2) & 170) + ((char / 2) & 85);

console.log(swapEvenOdd(0b01100110).toString(2).padStart(8,"0"))
console.log(swapEvenOdd2(0b01100110).toString(2).padStart(8,"0"))


Collection to hold maximum number of running Tasks I need a collection/bag (something) that will hold some maximum number of running Task objects. Adding a new running Task to the collection should block the calling thread (there are many threads trying to add a task so it should be thread safe) until there is an available slot for the new Task to be added if the maximum number of running tasks is reached. This is what I have so far and it is working fine.

Can you find any problems with this code? Or is there some built-in class that can handle this kind of task?

public class ConcurrentTaskLimiter
{
    public int MaxWorkingTasks { get; }
    private readonly Task[] _tasks;
    private readonly bool[] _finished;

    public ConcurrentTaskLimiter(int maxWorkingTasks)
    {
        MaxWorkingTasks = maxWorkingTasks;
        if ((1 <= maxWorkingTasks) == false)
            throw new ArgumentOutOfRangeException(nameof(maxWorkingTasks), maxWorkingTasks, "Must be >= 1");
        _tasks = new Task[maxWorkingTasks];
        _finished = new bool[maxWorkingTasks];

        for (int i = 0; i < MaxWorkingTasks; i++)
        {
            _tasks[i] = Task.FromResult(0); // use this as finished tasks
            _finished[i] = true;
        }
    }

    public void BlockAdd(Task t)
    {
        if (t == null)
            throw new ArgumentNullException(nameof(t));

        if (t.Status == TaskStatus.Canceled
            || t.Status == TaskStatus.Faulted
            || t.Status == TaskStatus.RanToCompletion)
            return;

        lock (this)
        {
            int i;
            while (true)
            {
                for (i = 0; i < MaxWorkingTasks; i++)
                {
                    if (_finished[i])
                    {
                        _tasks[i] = t;
                        _finished[i] = false;
                        return;
                    }
                }
                i = Task.WaitAny(_tasks);
                _finished[i] = true;
            }
        }
    }
}

What you have described here is a Producer- Consumer relationship. .Net has a BlockingCollection that performs exactly what you are trying to create here

public BlockingCollection(
    IProducerConsumerCollection<T> collection,
    int boundedCapacity
)

  You could use a ConcurrentBag as the underlying store or any ConcurrentCollection of your choice and the boundedCapacitycan be the maximum number of Tasks that can be added to the collection. An example usage is

BlockingCollection<int> messages = new BlockingCollection<int>(new ConcurrentBag<int>(), 10);


Recursively traverse directory tree and print all files I want to build a small console program, that for given directory, will print (for now) all files inside that tree.
Here is an F# script that I came up with:
open System.IO

let getDirObjects (dir :DirectoryInfo) =
    let dirs = dir.GetDirectories()
    let files = dir.GetFiles()
    dirs,files

let rec traverse allFiles dir =
    let dirs,files = getDirObjects dir
    let allFiles' = Seq.append allFiles files
    let allFiles'' = dirs |> Seq.map (traverse allFiles') |> Seq.concat
    Seq.append allFiles' allFiles''

let printFileInfo (fileInfo:FileInfo) =
    printfn "%A" fileInfo.FullName

let printAllFilesFromDir path =
    let dirInfo = DirectoryInfo path
    let allFiles = traverse Seq.empty dirInfo
    allFiles |> Seq.iter printFileInfo

Changelog:

 #1 changed List to Seq to be more generic, no need to convert Arrays returned by GetDirectories and GetFiles to List

I've been reading about F# for past 2 weeks or so. This is actually first toy program that I wrote :)
One thing I don't like is getDirObjects function, that returns a tuple DirectoryInfo * FileInfo. Maybe this could be done in a more fancy way, with pattern matching or sth?
Update
Here is much shorter version, whith same output.
It's get the job done, but my first intention was to do it in an F#, functional way. Please comment on the code above.
open System.IO

let printAllFilesFromDir path =
    Directory.EnumerateFiles(path,"*",SearchOption.AllDirectories)
        |> Seq.iter (printfn "%A")

printAllFilesFromDir "D:\_FTP"

let allFiles' = Seq.append allFiles files
let allFiles'' = dirs |> Seq.map (traverse allFiles') |> Seq.concat

Just because you can use ' to differentiate variables does not mean that you should. Unless it's very clear what the different variables mean, you should use separate name for each of them.

dirs |> Seq.map (traverse allFiles') |> Seq.concat

You can simplify Seq.map followed by Seq.concat to just Seq.collect:

dirs |> Seq.collect (traverse allFiles')

let rec traverse allFiles dir =
    let dirs,files = getDirObjects dir
    let allFiles' = Seq.append allFiles files
    let allFiles'' = dirs |> Seq.map (traverse allFiles') |> Seq.concat
    Seq.append allFiles' allFiles''

This code is wrong. You pass files from the parent directory to each child directory and then return the parent directory files from each child directory and concat them.

For example, if you had two subdirectories, dir1 and dir2 and two files, file-in-root and dir1/file-in-dir1, then your code outputs:

file-in-root
file-in-root
file-in-root
dir2/file-in-dir2

Notice how file-in-root is output three times: once for the root directory, and also once for each subdirectory.

The solution here is not to pass allFiles to traverse:

let rec traverse dir =
    let dirs,files = getDirObjects dir
    let filesInDirs = dirs |> Seq.collect traverse
    Seq.append files filesInDirs

let dirs = dir.GetDirectories()
let files = dir.GetFiles()

Since you're using lazy seqs extensively, it would make sense to make dirs and files lazy seqs too, by using seq-returning Enumerate* methods instead of array-returning Get* methods. This way, if you enumerate the resulting seq only partially, it's going to be more efficient.

let dirs = dir.EnumerateDirectories()
let files = dir.EnumerateFiles()


Temperature value converter I'm trying to improve my programming skills (not just using one language). I don't have a group of other programmers to bounce ideas off other than on the web - so please forgive me if I have posted this in the wrong place.

I'd like to improve my coding in this example. Here's my Python sketch that takes a number and converts it to either Fahrenheit or Celsius. It takes a number to convert with checks in place in the calc function, incase no text is entered (I could also check whether the values entered are computable but I've not done this). It uses type casting to convert the text entry string to an int and if you tick the checkbox it will calculate Fahrenheit. It uses one class to define the whole GUI layout as this could be a sub window in a larger GUI i.e. this could be a class among three or more as part of a larger GUI.

I'd appreciate your feedback about the code and what could be done to make it more efficient e.g. layout, code structure and general feedback. I'd appreciate it.

from Tkinter import Tk, Label, Button, Checkbutton, Entry, W,LEFT, RIGHT, CENTER
import Tkinter as tk

class MyFirstGUI:
    def __init__(self, master):
        self.master = master
        master.title("Celsius <-> Farenheit")

        self.var = tk.IntVar()

        self.label = Label(master, anchor=CENTER, text="Temperature Conversion")
        self.label.grid(columnspan=3, rowspan=2, sticky=W)

        #self.labelInput = Label(master, text="Enter temperature", borderwidth=2, relief="solid")
        #self.labelInput.grid(row=2, column=1)

        self.textEntry = Entry(master, text="enter temperature")
        self.textEntry.grid(row=2, column=1)

        self.calculate_button = Button(master, text="Convert", command=self.calc)
        self.calculate_button.grid(row=2, column=2)
        #self.labelInput.pack(side=LEFT)

        self.converstion_type = Checkbutton(master, text="Farenheit",  variable=var)
        self.converstion_type.grid(row=3, column=1)
        #self.labelInput.pack(side=RIGHT)

        self.output = Label(master, text="...")
        self.output.grid(row=3, column=3)

        self.outputInput = Label(master, text="Converted Temperature", borderwidth=2, relief="solid")
        #self.outputInput.grid(row=1, column=2)
        #self.outputInput.pack(side=RIGHT)

    def calc(self):
        entry = self.textEntry.get()
        if(entry != "" and var.get()==False):
            #convert celsius to celsius
            val =  5*(int(entry)-32)/9
            self.output.config(text=str(val))

        elif(entry != "" and var.get()==True):
            #convert celsius to farenheit
            val =  9*(int(entry)/5) + 32
            self.output.config(text=str(val))

root = Tk()
var = tk.IntVar()
my_gui = MyFirstGUI(root)
root.mainloop()

Don't import tkinter twice

You are both importing individual parts of tkinter as well as the module as a whole. Don't do both. Stick with importing the whole library and then using the library as a prefix to the objects. This makes your code more self-documenting.

For example:

import Tkinter as tk
...
self.label = tk.Label(...)
self.textEntry = Entry(...)

...

Separate widget creation from widget layout

Instead of creating a widget, then calling grid, then creating a widget, calling grid, etc, create your widgets in one group and lay them out in another.

self.label = Label(master, anchor=CENTER, text="Temperature Conversion")
self.textEntry = Entry(master, text="enter temperature")
self.calculate_button = Button(master, text="Convert", command=self.calc)
self.converstion_type = Checkbutton(master, text="Farenheit",  variable=var)
self.output = Label(master, text="...")
self.outputInput = Label(master, text="Converted Temperature", borderwidth=2, relief="solid")

self.label.grid(columnspan=3, rowspan=2, sticky=W)
self.textEntry.grid(row=2, column=1)
self.calculate_button.grid(row=2, column=2)
self.converstion_type.grid(row=3, column=1)
self.output.grid(row=3, column=3)
self.outputInput.grid(row=1, column=2)

In my experience, this makes it much easier to visualize the GUI structure in the code, and also makes it easier to change the layout.

If you have multiple sections in your code (eg: a navigation area, a main area, a toolbar, etc.) you should group them by section.

Hide the main code

Even though this is a complete program rather than a reusable library, it's good to get into the habit of moving executable code to a section that won't get run when you import the file.

For example:

if __name__ == "__main__":
    root = Tk()
    var = tk.IntVar()
    my_gui = MyFirstGUI(root)
    root.mainloop()

Some people take the extra step of moving that code to a main function placed at the top of the file:

def main():
    root = Tk()
    var = tk.IntVar()
    my_gui = MyFirstGUI(root)
    root.mainloop()
...
if __name__ == "__main__":
    main()

The choice is simply one of readability. Some people like having their main logic at the top of the file.

Even if you never plan to make this a reusable library, by hiding the main logic like this you can create test cases that import the file and test individual pieces of the code. While that's overkill for this small program, it's a good habit to get into.

For more information see What does if__name__ == "main" do?


Short script to hash files in a directory What improvements could be made to this script and can you explain why you would make those changes?

import hashlib
import os
import time
time = time.strftime("%Y%m%d-%H%M%S")
Masterhash = open("Filehash" + time, "a")
#get a list of files from your directory
for filename in os.listdir('.'):
    #ignore directory names
    if not os.path.isdir(filename):
        #open the file for readying
        primingfiletohash = open(filename, "r")
        #read contents of file
        filetohash = primingfiletohash.read()
        #close the file
        primingfiletohash.close()
        #use hashlib to generate an md5 hash of the file contents
        Filehash = hashlib.md5(filetohash)
        Filehash = Filehash.hexdigest()
        #write file names and hashes to a file
        Masterhash.write(filename + "\t")
        Masterhash.write(Filehash + "\n")
        #close the file
Masterhash.close()

First of all, I'd make a variable which will keep the current path. For that, I'd use os.getcwd(...):

current_path = getcwd()

From the docs:

  Return a string representing the current working directory.

After this, I'd normalize the path, so that your script will be able to run on multiple OSs:

current_path = normpath(getcwd())

From the docs:

  Normalize a pathname by collapsing redundant separators and up-level
  references so that A//B, A/B/, A/./B and A/foo/../B all become A/B.
  This string manipulation may change the meaning of a path that
  contains symbolic links. On Windows, it converts forward slashes to
  backward slashes.

Then, I'd get a list of all the files in the directory using listdir(...):

only_files = [join(current_path, f) for f in listdir(current_path) if isfile(join(current_path, f))]

After getting the files, I'd wrap everything within a function:

def get_files():
    current_path = normpath(getcwd())
    return [join(current_path, f) for f in listdir(current_path) if isfile(join(current_path, f))]

Then, I'd make another function which will return a list of hashes for all the files in the list:

def get_hashes():
    files = get_files()
    list_of_hashes = []
    for each_file in files:
        hash_md5 = hashlib.md5()
        with open(each_file, "rb") as f:
            for chunk in iter(lambda: f.read(4096), b""):
                hash_md5.update(chunk)
        list_of_hashes.append('Filename: {}\tHash: {}\n'.format(basename(each_file), hash_md5.hexdigest()))
    return list_of_hashes

After this, I'd just write this list of hashes in the desired file:

def write_hashes():
    hashes = get_hashes()
    with open('list_of_hashes.txt', 'w') as f:
        for md5_hash in hashes:
            f.write(md5_hash)

Finally, the code would look like this:

from os import listdir, getcwd
from os.path import isfile, join, normpath, basename
import hashlib

def get_files():
    current_path = normpath(getcwd())
    return [join(current_path, f) for f in listdir(current_path) if isfile(join(current_path, f))]

def get_hashes():
    files = get_files()
    list_of_hashes = []
    for each_file in files:
        hash_md5 = hashlib.md5()
        with open(each_file, "rb") as f:
            for chunk in iter(lambda: f.read(4096), b""):
                hash_md5.update(chunk)
        list_of_hashes.append('Filename: {}\tHash: {}\n'.format(basename(each_file), hash_md5.hexdigest()))
    return list_of_hashes

def write_hashes():
    hashes = get_hashes()
    with open('list_of_hashes.txt', 'w') as f:
        for md5_hash in hashes:
            f.write(md5_hash)

if __name__ == '__main__':
    write_hashes()

Now, why I'd do it this way:

Compare your code with this new one, and tell me which one is more readable.
You should read PEP8, which is the standard style guide for python.
You should use with() when opening a file as this will make sure the file will be closed when needed.
I've used for chunk in iter(lambda: f.read(4096), b"") which is a better approach for hashing large files (sometimes you won't be able to fit the whole file in memory. In that case, you'll have to read chunks of 4096 bytes sequentially).
I've split the logic of your main program into multiple functions which I'll let you document using docstrings:

E.g:

def get_files():
    """Returns a list of all files in the current directory"""
    # ...

You can see that I also added if __name__ == '__main__'. By doing the main check, you can have that code only execute when you want to run the module as a program and not have it execute when someone just wants to import your module and call your functions themselves.

To be considered:

MD5 is known broken and (IMHO) should come with scary deprecation warnings and removed from the library, so you should actually do the hashing thing using sha256 from the the same module:

Remember:

Each statement that you write is important, and if changing the name of a local variable / method / anything with a descriptive name makes your code a lot clearer and more readable then do it!


Fixing Archive Annoyances with Ruby I have recently become annoyed enough with something about the linux command line experience to take action.

I welcome all comments, but specific things I'd like input on are:

Is there something already available that does this?
How would you do the same thing in Perl or Python?
How would you go about reducing code duplication here? (The file-types hash looks similar enough in both places that it feels like I should be able to abstract it)
In unpack, is there a better way of determining file-type?
Can you think of any situations that would cause pack to blow up (and any ways of solving those situations)? Just failing to do anything wouldn't be too bad, but (for example) compressing random other files is something I'd like to avoid.

Take 4:

unpack:

#!/usr/bin/ruby

archive_types = {
  "tar"     => ["tar", "-xvf"],
  "tar.gz"  => ["tar", "-zxvf"],
  "tgz"     => ["tar", "-zxvf"],
  "tar.bz2" => ["tar", "-jxvf"],
  "rar"     => ["unrar", "x"],
  "zip"     => ["unzip"]
}

ARGV.each do |target|
  file_type = target.match(/\.([^\W0-9]{3}?(\.\w+)?)$/)[1]
  if archive_types[file_type]
    args = archive_types[file_type].push target
    system(*args)
  else
    puts "Dunno how to deal with '#{file_type}' files"
  end
end

pack (with added --exclude options for my ease of use):

#!/usr/bin/ruby

require 'optparse'
require 'pp'
require 'fileutils'

archive_types = {
  "tar"     => ["tar", "-cvf"],
  "tar.gz"  => ["tar", "-zcvf"],
  "tgz"     => ["tar", "-zcvf"],
  "tar.bz2" => ["tar", "-jcvf"],
  "zip"     => ["zip"]
}

########## parsing inputs
options = { :type => "tar", :excluded => [".git", ".gitignore", "*~"] }
optparse = OptionParser.new do|opts|
  opts.on("-e", "--exclude a,b,c", Array,
          "Specify things to ignore. Defaults to [#{options[:excluded].join ", "}]") do |e|
    options[:excluded] = e
  end
  opts.on("-t", "--type FILE-TYPE",
          "Specify archive type to make. Defaults to '#{options[:type]}'. Supported types: #{archive_types.keys.join ", "}") do |t|
    options[:type] = t
  end
end
optparse.parse!
##########

ARGV.each do |target|
  if not archive_types[options[:type]]
    puts "Supported types are #{archive_types.keys.join ", "}"
    exit
  elsif options[:type] == "zip"
    exclude = options[:excluded].map{|d| ["-x", d]}.flatten
  else
    exclude = options[:excluded].map{|d| ["--exclude", d]}.flatten
  end
  args = archive_types[options[:type]] +
         [target + "." + options[:type], target] +
         exclude
  system(*args)
end

Both your pack and unpack scripts will fail if any of the targets contain spaces (or other shell meta-characters) in their name. Generally it's almost always a bad idea to insert arguments into commands using string interpolation.

I assume you're using backticks rather than system because you don't want the output of the invoked commands to go to the screen. In that case you can use IO.popen, which since ruby 1.9 can take an array as its first argument.

As a trivial sidenote: There doesn't seem to be any point in passing the -v flag to tar if you're not going to print its output, so I'd remove it.


Phone Number Extracting using RegEx And HtmlAgilityPack I've written this whole code to extract cell numbers from a website. It is extracting numbers perfectly but very slowly, and it's also hanging my Form while Extracting.

Imports HtmlAgilityPack
Imports System.Text.RegularExpressions
Public Class Extractor

Shared doc As New HtmlAgilityPack.HtmlDocument()

Public Shared Function ScrapLinks(TextBox1 As TextBox, ListBox1 As ListBox, lbllinks As Label)
Dim hw As New HtmlWeb()
Try
    doc = hw.Load(TextBox1.Text)
    doc.LoadHtml(doc.DocumentNode.SelectSingleNode("//*[@id='ad_list']").InnerHtml())

    For Each link As HtmlNode In doc.DocumentNode.SelectNodes("//a[@href]")

        Dim hrefValue As String = link.GetAttributeValue("href", String.Empty)

        If hrefValue.Contains("/detail/") Then
            If Not ListBox1.Items.Contains(hrefValue) Then
                ListBox1.Items.Add(hrefValue)
            End If
        End If
    Next

Catch ex As Exception
    MsgBox("Error " + ex.Message)

End Try
Return Nothing

End Function

Public Shared Function Scrapnums(lstbox As ListBox,lstnum As ListBox)
Try

    Dim hw As New HtmlWeb()
    doc = hw.Load(lstbox.SelectedItem)

    Dim data = doc.DocumentNode.SelectSingleNode("//*[@class='det_ad f_left']").InnerText

    Dim m As Match = Regex.Match(data, "(\+92|0092)-?\d{3}-?\d{7}|\d{11}|\d{4}-\d{7}")

    If Not lstnum.Items.Contains(m.Value) Then

        lstnum.Items.Add(m.Value)

    End If

Catch ex As Exception

End Try
Return Nothing

End Function

End Class

This code has a major design flaw.

Always keep your user interface (UI) separated from your data access layer (DAL).

ScrapLinks

Should be named ScrapeLinks.
If a Function doesn't return anything, then it should be a Sub. In this case, we still want it to be a function.
No need to have a shared HtmlDocument as this is created by the HtmlWeb.
In every iteration where the conditions are met  you add a new value to the ListBox item collection. This will result in a bunch of windows messages being sent. I guess the most costly is WM_PAINT. Scrape the links first, then compare the result with current links. Use the AddRange method to insert new items.
A Try-Catch block, whose only purpose is to display an error, should never be used outside the UI layer.

Public Shared Function ScrapeLinks(url As String) As IEnumerable(Of String)

    If (url Is Nothing) Then Throw New ArgumentNullException("url")

    Dim web As New HtmlWeb()
    Dim document As HtmlDocument = web.Load(url)

    If (Not document.DocumentNode Is Nothing) Then

        Dim rootNode As HtmlNode = document.DocumentNode.SelectSingleNode("//*[@id='ad_list']")

        If (Not rootNode Is Nothing) Then

            document.LoadHtml(rootNode.InnerHtml())

            If (Not document.DocumentNode Is Nothing) Then

                Dim childNodes As HtmlNodeCollection = document.DocumentNode.SelectNodes("//a[@href]")

                If (Not childNodes Is Nothing) Then

                    Return (
                        From childNode As HtmlNode
                        In childNodes
                        Let href As String = childNode.GetAttributeValue("href", String.Empty)
                        Where href.Contains("/detail/")
                        Select href
                    )

                End If

            End If

        End If

    End If

    Return Nothing

End Function

Scrapnums

(Same as above)
Should be named ScrapeNumber as it only returns one number.

Public Shared Function ScrapeNumber(url As String) As String

    If (url Is Nothing) Then Throw New ArgumentNullException("url")

    Dim web As New HtmlWeb()
    Dim document As HtmlDocument = web.Load(url)
    Dim root As HtmlNode = document.DocumentNode.SelectSingleNode("//*[@class='det_ad f_left']")

    If (Not root Is Nothing) Then

        Dim regexMatch As Match = Regex.Match(root.InnerText, "(\+92|0092)-?\d{3}-?\d{7}|\d{11}|\d{4}-\d{7}")

        If (Not regexMatch Is Nothing) Then
            Return regexMatch.Value
        End If

    End If

    Return Nothing

End Function


Time limit exceeded in Heap I wrote a Heap, but the judge system reports "time limit exceeded" in some tests.  I use a sift_down to build, so I don't know why.

Maybe I can improve the build?  Or do I have a bad sift_down?

  Input:

  n commands

  Commands:

  0 xxx, where xxx is a number to add to heap
  1 - extract maximum from heap

#include <iostream>
using namespace std;
int j = 0;

int sift_down(long long int *a, int i, int n){
    int l = 2 * i;
    int r = 2 * i + 1;
    int largest = i;
    if (l <= n && a[l] > a[i])
        largest = l;
    else
        largest = i;
    if (r <= n && a[r] > a[largest])
        largest = r;
    if (largest != i){
        long long int x = a[i];
        a[i] = a[largest];
        a[largest] = x;
        sift_down(a, largest, n);
    }
    return largest;
}

void sift_up(long long int *a, int i){
    long long x = a[i];
    while (i > 1 && a[i / 2] < x){
        a[i] = a[i / 2];
        i /= 2;
    }
    a[i] = x;
 //   cout << i << endl;
}

void Extract_Max(long long int *a, int *n){
    long long int z = a[1];
    a[1] = a[*n];
    (*n)--;
    sift_down(a, j = 1, *n);
    cout << z << endl;
}

int main(){
    int n = 0;
    int k = 0;
    int l = 0; //len
    int command = 0;
    cin >> k;

    long long int *a = new long long int[k + 1]; // n <= k
    for (int i = 0; i < k; i++){
        cin >> command;
        if (command == 0){
            cin >> a[++n];
        }
        else {
                for(int i = n - 1; i >= 1; i--)
                    sift_down(a, i, n);
                Extract_Max(a, &n);

        }
    }

    delete []a;

    return 0;
}

Some generic comments about your code, not related to your problem.

using namespace std;

There are some developers which consider importing a whole namespace (overwriting some standard functions with imported ones) as very bad practice. I'm not qualified enough to make comments on that, but it's something you should keep in mind.

You're inconsistent about your usage of letter casing.

int sift_down( ...
void Extract_Max( ...

That's one of the things you should fix first, there is no standard for C/C++ when it comes to casing, but at least be consistent within your source code. And even better, have a look at what everyone else in your environment does and copy that.

int sift_down(long long int *a, int i, int n){
    int l = 2 * i;
    int r = 2 * i + 1;

To abuse a very popular movie quote:

  I find your lack of properly named variables disturbing.

There is absolutely no valid reason to use single letter variable names in production code, with the only exception of dimensions (x, y, z) and loop variables (i, j, k).

You might also notice that, normally, an integer with the name i is only used inside loops. Seeing it as a parameter to a function is...disturbing.

The lack of comments is also problematic, especially when it comes to rather complex implementations.


Generate sample coordinates inside a Polygon I have a Polygon named as poly. I attempted to randomly select 5 coordinate points that lies inside the polygon.

import numpy as np
from shapely.geometry import Polygon, Point

poly = Polygon([(141.4378366,-25.95915986), (165.4279876,-29.43400298), (163.1382942,-47.65345814), (133.1675418,-42.99807751)])

minx, miny, maxx, maxy = poly.bounds

longs = np.arange(minx, maxx, 0.002); lats = np.arange(miny, maxy, 0.002)
longs = np.tile(longs,3).ravel(); lats = np.repeat(lats,3).ravel()
coords = np.array([(x,y) for x,y in zip(longs,lats)])

points = [Point(xy) for xy in coords]

check = [xy.within(poly) for xy in points]
pointsInside = coords[check]

ranIdx = np.random.choice(len(pointsInside),5,replace=False)
result = pointsInside[ranIdx]

print result

I think my code is ineffective. Are there any ideas for a straight and elegant implementation?

Style Nitpicks

Put spaces after commas / Keep Lines within 120 Chars

poly = Polygon([(141.4378366,-25.95915986), (165.4279876,-29.43400298), (163.1382942,-47.65345814), (133.1675418,-42.99807751)])

becomes

poly = Polygon([(141.4378366, -25.95915986), (165.4279876, -29.43400298), (163.1382942, -47.65345814),
                (133.1675418, -42.99807751)])

Use Pythonic Underscores

minx, miny, maxx, maxy = poly.bounds

becomes

min_x, min_y, max_x, max_y = poly.bounds

Eschew Semicolons (Personal Opinion)

You can use semicolons like this:

longs = np.arange(minx, maxx, 0.002); lats = np.arange(miny, maxy, 0.002)

But personally, I just wouldn't in Python. Eww.

An Alternative Algorithm

EDIT: Having read @MrE's and @MartinR's comments, I now propose this rejection sampling method. Although, this could miss frequently in a polygon with a large bounding box relative to its area; .e.g. an 8-point Christmas star with a small inner circle.

def random_points_within(poly, num_points):
    min_x, min_y, max_x, max_y = poly.bounds

    points = []

    while len(points) < num_points:
        random_point = Point([random.uniform(min_x, max_x), random.uniform(min_y, max_y)])
        if (random_point.within(poly)):
            points.append(random_point)

    return points

Another alternative that never misses, but may not be well distributed.

This was my original idea, but it didn't look so good in the morning.

Firstly, work out how to select one point within a polygon.

def random_point_within(poly):
    min_x, min_y, max_x, max_y = poly.bounds

    x = random.uniform(min_x, max_x)
    x_line = LineString([(x, min_y), (x, max_y)])
    x_line_intercept_min, x_line_intercept_max = x_line.intersection(poly).xy[1].tolist()
    y = random.uniform(x_line_intercept_min, x_line_intercept_max)

    return Point([x, y])

Then simply call that in a list comprehension to generate however many points you desire.

points = [random_point_within(poly) for i in range(5)]
checks = [point.within(poly) for point in points]

My approach is to select x randomly within the polygon, then constrain y.

This approach doesn't require NumPy and always produces a point within the polygon.


Customize data types in a generated model with hibernate I have a Spring/Hibernate application with the following domain class (irrelevant code stripped for brevity):

@Entity
@Data
public class Program implements Serializable {

    @Id
    @GeneratedValue(strategy = GenerationType.AUTO)
    private Long tid;

    @Column
    @Convert(converter = ProgramStatusConverter.class)
    private ProgramStatus status;

    @Column
    @Length(max=64)
    private String containerId;

    @Column
    private String sourceCode;

    @Column
    private String compilationOutput;

    @Column
    @OneToMany(fetch=FetchType.LAZY, mappedBy="program")
    private Set<Execution> executions;
}

The problem is that the database schema (currently an in-memory h2 database) is generated when I startup the application. It works fine in almost all cases, but the "compilationOutput" and "sourceCode" columns are generated as VARCHAR(255), which is too short.

The solution I used was simply to add @Length(max=10000) on the two columns, but I don't like the solution for two reasons:

It's a semantic annotation and I'm using it to solve a purely technical issue. I have absolutely no reason to limit these fields to 10000 characters.
What if I get a sourceCode that's longer than 10000 characters ? This solution is not a real solution.

Is there a better way ?

It's not unreasonable that a maximum length should be specified for a field that is backed by a VARCHAR. In some frameworks, for example Django (Python), a max_length parameter is mandatory to create a CharField, but optional for a TextField. This makes sense: how else can the framework know the size of the column. A VARCHAR column needs a max size.

Make the maximum size a conscious design decision when using a relational database as storage. If the data is text and you need to support arbitrary length, then in the database it should be stored as a TEXT field instead of a VARCHAR. For that, use @Column(columnDefinition="text") or @Type(type="text"), as @ferada said in a comment,
and as discussed in this related post.

You remarked in a comment that H2 used the type VARCHAR(2147483647).
That's an irrelevant implementation detail of how H2 supports TEXT type.
It's indeed a bit funny.


Convert string to fishy and fishy to string Inspired by this question.

Here's the code:

class FishyError(Exception):
    __module__ = Exception.__module__

def string_to_fishy(string: str) -> str:
    """
    Converts a string to fishy code

    Examples:
        Hello World!
        ><72> ><101> ><108> ><108> ><111> ><32> ><87> ><111> ><114> ><108> ><100> ><33>

        007 James bond
        ><48> ><48> ><55> ><32> ><74> ><97> ><109> ><101> ><115> ><32> ><98> ><111> ><110> ><100>

    """

    l = []

    for char in string:
        l.append(f'><{ord(char)}>')

    return ' '.join(l)

def fishy_to_string(fishy: str) -> str:
    """
    Converts a fishy code to string

    Examples:
        ><72> ><101> ><108> ><108> ><111> ><32> ><87> ><111> ><114> ><108> ><100> ><33>
        Hello World!

        ><48> ><48> ><55> ><32> ><74> ><97> ><109> ><101> ><115> ><32> ><98> ><111> ><110> ><100>
        007 James bond

    """

    if not fishy:
        return ''

    fishy_chars = fishy.split(' ')

    string = ''

    for char in fishy_chars:
        if not char:
            raise FishyError('Invalid syntax: Extra whitespace')

        if not (char.startswith('><') and char.endswith('>')):
            raise FishyError('Invalid syntax: ' + char)

        try:
            value = int(char[2:-1])
            string += chr(value)

        except ValueError:
            raise FishyError('Invalid syntax: ' + char)

    return string

while True:
    print(fishy_to_string(input()))

This works well, but doesn't look that great. Any ideas?

I'm not entirely sure why you are not satisfied with your code as it looks quite good. Maybe too many blank lines that seem to stretch the code length. There are also a few things on the Python side that could be changed to reduce the amount of source lines needed for the task:

string_to_fishy

string_to_fishy can be rewritten using a list comprehension:

def string_to_fishy(string: str) -> str:
    """..."""
    return ' '.join(f'><{ord(char)}>' for char in string)

fishy_to_string

The inverse function could use a similar technique in order to avoid repeated creations of new strings in string += chr(value):

def fishy_to_string(fishy: str) -> str:
    """
    Converts a fishy code to string

    Examples:
        ><72> ><101> ><108> ><108> ><111> ><32> ><87> ><111> ><114> ><108> ><100> ><33>
        Hello World!

        ><48> ><48> ><55> ><32> ><74> ><97> ><109> ><101> ><115> ><32> ><98> ><111> ><110> ><100>
        007 James bond
    """
    if not fishy:
        return ''

    def parse_fish(token):
        if not token:
            raise FishyError('Invalid syntax: Extra whitespace')

        if not (token.startswith('><') and token.endswith('>')):
            raise FishyError('Invalid syntax: ' + token)

        try:
            value = int(token[2:-1])
            string += chr(value)
        except ValueError:
            raise FishyError('Input cannot be parsed as character: ' + token)

    return ''.join(parse_fish(token) for token in fishy.split(' '))

I also allowed myself to change the variable name char to token, because I find it more appropriate in that context, and also slightly reworded the last error message to make it more expressive.


Mini RPG equipment code what do you think about this code? It's upgraded version of:
Classes representing items in an RPG game

using System;
using System.Collections.Generic;

public class Item
{
    public string Name { get; protected set; }
    public int Cost { get; protected set; }

    public override string ToString()
    {
        return "Name: " + Name + "\nCost: " + Cost;
    }

    public Item(string name, int cost)
    {
        Name = name;
        Cost = cost;
    }
}

public abstract class SpecialItem : Item
{
    public virtual int GetSpecialProperty { get; }
    public override string ToString() { return base.ToString() + "\nDamages/ArmorPts/HungryRestorePts: " + GetSpecialProperty; }
    public SpecialItem(string name, int cost) : base(name, cost) { }
}

public abstract class ElementalsItem : SpecialItem
{
    public int Fire  { get; protected set; }
    public int Water { get; protected set; }
    public int Earth { get; protected set; }
    public int Wind  { get; protected set; }
    public override string ToString()
    {
        return base.ToString()
            + "\nFire Damages/Resistance: " + Fire
            + "\nWater Damages/Resistance: " + Water
            + "\nEarth Damages/Resistance: " + Earth
            + "\nWind Damages/Resistance: " + Wind;
    }
    protected ElementalsItem(string name,
                             int cost,
                             int fire,
                             int water,
                             int earth,
                             int wind)
                             : base(name, cost)
    {
        Fire  = fire;
        Water = water;
        Earth = earth;
        Wind  = wind;
    }
}

public class Weapon : ElementalsItem
{
    public int Damages { get; private set; }
    public override int GetSpecialProperty { get { return Damages; }}
    public override string ToString() { return base.ToString(); }

    public Weapon(string name,
                  int cost,
                  int damages,
                  int fireDamages  = 0,
                  int waterDamages = 0,
                  int earthDamages = 0,
                  int windDamages  = 0)
                  : base(name, cost, fireDamages, waterDamages, earthDamages, windDamages)

    {
        Damages = damages;
    }
}

public class Armor : ElementalsItem
{
    public int ArmorPts { get; private set; }

    public override int GetSpecialProperty { get { return ArmorPts; }}
    public override string ToString() { return base.ToString(); }

    public Armor(string name,
                 int cost,
                 int armorPts,
                 int fireResistance  = 0,
                 int waterResistance = 0,
                 int earthResistance = 0,
                 int windResistance  = 0)
                 : base(name, cost, fireResistance, waterResistance, earthResistance, windResistance)
    {
        ArmorPts = armorPts;
    }
}

public class Food : SpecialItem
{
    public int HungryRestorePts { get; private set; }
    public override int GetSpecialProperty { get { return HungryRestorePts; }}
    public override string ToString() { return base.ToString(); }

    public Food(string name, int cost, int hungryRestorePts) : base(name, cost)
    {
        HungryRestorePts = hungryRestorePts;
    }
}

class Equipment
{
    public static void Test()
    {
        List<Item> items = new List<Item>();
        items.AddRange(new Item[] { new Item("Stone", 1),
                                    new Armor("Steel Helmet", 80, 10),
                                    new Weapon("Great Sword", 120, 80),
                                    new Armor("Storm Cuirass", 1000, 120, 0, 0, 0, 60),
                                    new Weapon("Fire Spear", 1400, 60, 0, 80, 0, 0),
                                    new Food("Apple", 5, 10) });
        foreach(Item i in items)
        {
            Console.WriteLine(i.ToString());
            Console.WriteLine();
        }
    }
}

Edge cases are where such class hierarchies often fall short.

An offhand short sword that adds to defense?
A shield that does damage when you bash with it?
A magic potion that not only makes you less hungry but also adds
armor when you drink it?

How will those fit into your current hierarchy? One way to avoid those problems is to use aggregation instead of inheritance and treat each game object as a collection of arbitrary properties, that can be added to object or removed from it. You might want to look at how, for example, things are done in Unity, to get a better understanding of how things are actually designed in modern engines.

To be clear, there is nothing wrong with having game object hierarchies in small projects. It is the easiest approach and sometimes it is enough, especially if all you want is to write some code. I just wanted to give you some perspective.

EDIT
Since this question is getting a lot of attention I though I might as well update my answer with a simplified example.

First, you have to create some "components". For the sake of this example, you can think of components as properties that define the item and make it different and special. What is a fire sword? It is an item that:

people in the village call "Lightbringer"
requires some "sword" skill to use
looks like a big burning knife
can do slicing damage to things
but does fire damage too
it weights 8 kg
it costs 1000g and can be bought or sold
etc...

Those are the aspects of a fire sword that make it what it is. And every one of those aspects can be defined as separate component.

abstract class ItemComponent
{
    //What goes into base class entirely depends
    //on how your game engine is implemented.
    //I left it out because it is irrelevant
    //in context of this example
}

class Valuable : ItemComponent
{
    public int Cost { get; set; }

    public override string ToString()
    {
        return "It is a valuable item, that can be bought for " + Cost + "g.";
    }
}

class PhysicalDamage : ItemComponent
{
    public int Slashing { get; set; }
    public int Bashing { get; set; }
    public int Piercing { get; set; }

    public override string ToString()
    {
        return String.Format("It does {0}/{1}/{2} physical damage.", Slashing, Bashing, Piercing);
    }
}

class ElementalDamage : ItemComponent
{
    public int Fire { get; set; }
    public int Water { get; set; }
    public int Earth { get; set; }
    public int Wind { get; set; }

    public override string ToString()
    {
        return String.Format("It does {0}/{1}/{2}/{3} elemental damage.", Fire, Water, Earth, Wind);
    }
}

//etc...

And this is a typical game object that theoretically can represent anything from a stone on the ground to a bullet. It's just a collection of arbitrary components.

class Item
{
    public Item(string name)
    {
        Name = name;
    }
    //Name can be a component too!
    public string Name { get; private set; }

    public Item AddComponent(ItemComponent component)
    {
        _components.Add(component);
        return this;
    }

    public T GetComponent<T>()
    {
        return (T)_components.OfType<T>().FristOrDefault();
    }

    public override string ToString()
    {
        var sb = new StringBuilder();
        sb.AppendLine(Name);
        _components.ForEach(c => sb.AppendLine(c.ToString()));
        return sb.ToString();
    }

    private List<ItemComponent> _components = new List<ItemComponent>();
}

Then to create a fire sword all you need to do is create an item with all the necessary aspects:

var fireSword = new Item("Lightbringer")
                   .AddComponent(new Valuable { Cost = 1000 })
                   .AddComponent(new PhysicalDamage { Slashing = 10 })
                   .AddComponent(new ElementalDamage { Fire = 10 })
//Maybe it also makes you weak to water attacks?
//No problem, just define and add new component
                   .AddComponent(new ElementalResistance { Water = -10 };
Console.WriteLine(fireSword.ToString())

This allows you to do all sorts of neat stuff later on. For example, to calculate total resistances of a player, you can call GetComponent<ElementalResistance>() for every equipped item and sum the result.


Jest / Enzyme test for a React component to calculate average data speed I'm trying to master testing.  I struggle, conceptually, as to what should be tested.

Is this Jest / Enzyme test sufficient for this react component?
Is is too much?
Are there better practices that I should be implementing?

Here is my component.

import React from 'react';
import PropTypes from 'prop-types';
import styled from 'styled-components';
import {AverageText} from './index';

const Row = styled.div`
  display: flex;
  justify-content: center;
`;

const AvgSpeed = ({tests}) => {
  const downloadSpeeds = extractSpeeds(tests, 'download');
  const uploadSpeeds = extractSpeeds(tests, 'upload');
  const downloadAverage = getAverage(downloadSpeeds);
  const uploadAverage = getAverage(uploadSpeeds);
  return (
    <Row>
      <AverageText label="download" value={downloadAverage} />
      <AverageText label="upload" value={uploadAverage} />
    </Row>
  );
};

AvgSpeed.propTypes = {
  tests: PropTypes.object,
};

//returns average from array of numbers fixed at 1
function getAverage(array) {
  return (
    array.reduce((a, b) => {
      return a + b;
    }) / array.length
  ).toFixed(1);
}

//extracts float value from object property string value
function extractSpeeds(obj, property) {
  return Object.keys(obj).map(item => parseFloat(obj[item][property]));
}

//export to test
export {getAverage, extractSpeeds};

export default AvgSpeed;

My test

import React from 'react';
import AvgSpeed, {getAverage, extractSpeeds} from './AvgSpeed.js';
import {shallow, mount} from 'enzyme';
import renderer from 'react-test-renderer';

describe('AvgSpeed Component', () => {
  const inputObj = {
    1: {
      download: '22 mbs',
      upload: '9 mbs',
    },
    2: {
      download: '24 mbs',
      upload: '18 mbs',
    },
  };

  it('matches the snapshot', () => {
    const tree = renderer.create(<AvgSpeed tests={inputObj} />).toJSON();
    expect(tree).toMatchSnapshot();
  });

  describe('getAverage function', () => {
    it('returns the average from an array (fixed at 1)', () => {
      let input = [1, 2, 3];
      let output = '2.0';

      expect(getAverage(input)).toBe(output);

      //2,3,4
      input = input.map(n => (n += 1));
      output = '3.0';
      expect(getAverage(input)).toBe(output);

      //4,5,6
      input = input.map(n => (n += 2));
      output = '5.0';
    });
  });

  describe('extractSpeeds function', () => {
    it('return an array of float numbers', () => {
      const downloadIn = 'download';
      const uploadIn = 'upload';

      const downloadOut = [22, 24];
      const uploadOut = [9, 18];

      expect(extractSpeeds(inputObj, downloadIn)).toEqual(downloadOut);
      expect(extractSpeeds(inputObj, uploadIn)).toEqual(uploadOut);
    });
  });
});

There are two distinct parts in your test:

Testing the rendering of AvgSpeed component.
Testing the logic of getAverage() and extractSpeeds().

This separation between components and logic is good. I would suggest separating them even more by moving the getAverage() and extractSpeeds() functions to separate file, which you can then test completely independently from the component.

The component rendering test however is dependent on the logic. That's not good:

You're re-testing the logic through the component test, which is duplication.
When there's a change in logic, the component test will also break, resulting in more work maintaining the tests.

So your component test really is more of an integration test, not a pure unit test. There are basically two ways to improve on that:

Change the component. Instead of the component importing these functions and calling them, perhaps you could perform the calculation outside of the component and just pass in download and upload props.
Change the test. Instead of letting the component call actual getAverage() and extractSpeeds() you could replace them with mock implementations. Though, the less mocking you need, the better. So, you could consider combining the two functions into one, which will be easier to mock out.

Usually the first approach is better and requires no extra tooling. If you decide to go the mocking route, my personal choice is the rewire library, of which there exist various versions for various environments (e.g. for Babel).

Regarding specific tests

Avoid temporary variables when you can write the values inline. Instead of:

expect(extractSpeeds(inputObj, downloadIn)).toEqual(downloadOut);

prefer:

expect(extractSpeeds(inputObj, "download")).toEqual([22, 24]);

Avoid logic in tests. Logic is error-prone. While hard-coded values are a big no-no in normal code, in test code they're usually OK. Instead of:

input = [1, 2, 3];
input = input.map(n => (n += 1));

just hard-code it:

input = [2, 3, 4];

Try to write one assertion (expect) per it() block. This way you can give a more descriptive name for each scenario you want to test.

Don't simply repeat the same test multiple times. For example, you're testing the getAverage with [1,2,3], [2,3,4], [3,4,5] - it's essentially the same test: three sequential numbers. Try to come up with more fundamentally different scenarios:

different lengths of array.
positive/negative numbers, fractional numbers, zero.
array of similar numbers.

Think of the edge cases:

What if there is no data? (e.g. empty object or array)
What kind of input data would be problematic? (e.g. negative numbers)


Putting in order my two packages and possibly merging them I am making an application which fetches tweets for a specified amount of time, then inserts those tweets in a database and when the user presses another button the top n words and the hashtags will be shown.

This is my Twitter package:

[Package]Twitter
     >[Class]TwitterTools.java

This is my Analyzing package:

[Package]Analyzing
         >[Class]WordCounting.java

This is the TwitterTools class:

public class TwitterTools {

    public static List<Status> search(Query query) {
    }

    public static void filterTweetsBasedOnCity(List<Status> tweets, final String city) {
    }

    public static Query queryMaker(final String keywords, final Date since,
            final Date until, final int count) {
    }
}

search - returns a list of status based on a query
filterTweetsBasedOnCity - deletes status from a list if they were not made in a certain city
queryMaker - makes a query based on the parameters

This is the WordCounting class:

public class WordCounting {

    public static String getHtmlTable(final List<String[]> words, final List<String[]> hashtags) {
    }

    public static Stream<Map.Entry<String, Long>> getTopWords(final int topX, final Stream<String> words) {
    }

    public static String listToHtmlTable(List<Map.Entry<String, Long>> topEntries, final String title) {
    }
}

getHtmlTable - returns the html table of the top X words
getTopWords - returns a stream of the top words
listToHtmlTable - converts a list to an html table

My question is how should I arrange these two packages. Should I merge them since they have only one class each? Should I split them even more by having some of the functions in another class?

The ideal package structure is one which indicates usage. Classes such as these which exclusively contain static methods are definitely utility classes, which I usually put in a util subpackage of your main application.

From what it sounds like, you don't have a main package at the moment which ideally would be your website (in reverse domain order), e.g. com.stackexchange.codereview so that the more specific elements are later, then on top of that you'd have the name of your application which may make it something like com.stackexchange.codereview.tweettrends.

This would be the core of your program, and from here you could then add on your extras.

The Twitter package in particular doesn't sound helpful: your entire program seems to relate to Twitter so it doesn't really convey what that part of the program does. Frankly, I'd just put all those classes under com.stackexchange.codereview.tweettrends.util and add in more sub-packages if you add more classes.

And generally speaking, in Java packages should be exclusively lowercase.


Future in f# is this an unavoidable use of a mutable variable? I have this type:

type Future<'a> () =
    let mutable _value: 'a option = None

    member x.Resolve value =
        if _value.IsSome then failwith "can only resolve once"
        _value <- Some value

    member x.IsResolved with get () = _value.IsSome

    member x.Value
        with get () =
            match _value with
            | Some v -> v
            | None -> failwith "hasn't resolved yet"

And I need something like this because I have something along the lines of:

let testHttpGet response =
    let urlFuture = Future<string>()

    let get url =
        urlFuture.Resolve url
        response

    (get, urlFuture)

Call that function and set the return value of the returned function

let get, urlFuture = testHttpGet "resp"

Call the returned func, thus resolving the Future

let response = get "http://test"

In the end we have the result of the function

response |> should equal "resp"

And we have the value with which the func was called

urlFuture.IsResolved |> should be True
urlFuture.Value |> should equal "http://test"

To me this looks like a sound approach but I also have a feeling that something like it must exist in f# allready

Here's where I use them.

Here is a fiddle.

This looks pretty close to a Clojure Promise. On .NET, the similar type is (very Microsoftish) called TaskCompletionSource<TResult>.

However, you should consider if F# Lazy Computations isn't what you need.


Read message protocol socket c My code recovers all sent messages (ending with "\r\n") in function read_message. I would like to improve this code, make it more efficient? If it's possible, I am open to all reviews!

Code

static char     *read_message(char *data)
{
    static char     *buffer = NULL;
    char            *message = NULL;
    char            *ptr;
    size_t          size;

    if (buffer)
    {
        size = strlen(buffer) + strlen(data) + 1;
        if (!(buffer = realloc(buffer, size)))
            return (NULL);
        strcat(buffer, data);
    }
    else {
        if (!(buffer = strdup(data)))
            return (NULL);
    }
    if ((ptr = strstr(buffer, "\r\n")))
    {
        size = ((unsigned int)ptr - (unsigned int)buffer) + 1;
        if (!(message = (char*)malloc(sizeof(char) * size)))
            return (NULL);
        strncpy(message, buffer, size);
        ptr += 2;
        if (!(ptr = strdup(ptr)))
            return (NULL);
        free(buffer);
        buffer = ptr;
    }
    return (message);
}

static t_message    *parse_message(char *buffer)
{
    int     id;

    id = atoi(buffer);
    printf("L'id du message est %d\n", id);
    return (NULL);
}

static void     handle_message(void)
{
    printf("OK\n");
}

void            listen_client(t_args *args)
{
    ssize_t     count;
    char        buffer[BUFF_SIZE + 1] = { 0 };
    char        *msg_str;
    t_message   *msg;

    while ((count = recv(args->client->s, buffer, BUFF_SIZE, 0)))
    {
        buffer[count] = '\0';
        if (!(msg_str = read_message(buffer)))
            continue ;
        if (!(msg = parse_message(msg_str)))
            continue ;
        handle_message();
    }
    printf("Fin de connexion\n");
}

Portability

First this program could be made more portable. There are 2 portability issues here

If it was ported to a system that did not define end of line as "\r\n" this program might not work. This problem could be solved by including the proper header files or using

#define EndOfLine "\r\n"

and using

    ptr = strstr(buffer, EndOfLine)

The second portability issue is the use of strdup(). It is not implemented on all systems. This Stackoverflow question discusses the use of strdup.

Consistency in Coding Style

There is one place in the code where the indentation is inconsistent:

    if (buffer)
    {
        size = strlen(buffer) + strlen(data) + 1;
        if (!(buffer = realloc(buffer, size)))
            return (NULL);
        strcat(buffer, data);
    }
    else {
        if (!(buffer = strdup(data)))
            return (NULL);
    }

The use of braces "{" and "}" should be consistent through the entire program.
Everywhere except in this code the brace starts on a new line.

It would be very nice if the headers were included for compilation and
testing. Without the headers or the calling functions it is difficult to
compile or test this code.

Complexity

It might be better if the function read_message(char *data) was separated
into multiple functions to reduce the complexity. All of the functions might
be more readable if there was some virtical spacing between blocks of code.

Possible Bug

The function parse_message(char *buffer) always returns NULL, in your
testing did the function handle_message() always get called?


Getting a value from a database table dynamically I am trying to create a forum using php in a MVC architecture.
This is my database setup:

Table: forum_categories

`forum_categories` (
`cat_id` INT(8) NOT NULL AUTO_INCREMENT,
`cat_title` VARCHAR(255) NOT NULL,
`cat_desc` TEXT NOT NULL,
PRIMARY KEY (`cat_id`),
UNIQUE KEY (`cat_title`)

Table: forum_topics

`forum_topics` (
`topic_id` INT(8) NOT NULL AUTO_INCREMENT,
`cat_id` INT(8) NOT NULL COMMENT 'foreign key with forum_categories table',
`user_id` INT(11) NOT NULL COMMENT 'foreign key with users table',
`topic_title` VARCHAR(255) NOT NULL,
`topic_desc` TEXT NOT NULL,
`topic_date` DATETIME DEFAULT NULL,
PRIMARY KEY (`topic_id`),
FOREIGN KEY (`cat_id`) REFERENCES forum_categories (`cat_id`) ON DELETE CASCADE ON UPDATE CASCADE

Example of the functionality, I would like to achieve:

  Category 1 has cat_id = 1
  Category 2 has cat_id = 2

  Topic 1 has cat_id = 1
  Topic 2 has cat_id = 2

Now when category 1 is selected I just want topic 1 to show.
If category 2 is selected I just want topic 2 to show.

The following code does that:

Controller

 /**
 * Show all the topics in the chosen category
 */
public function showForumTopicsCat1()
{
    $topic_model = $this->loadModel('Forum');
    $this->view->forum_topics = $topic_model->getForumTopics(1);
    $this->view->render('forum/viewTopics');
}

public function showForumTopicsCat2()
{
    $topic_model = $this->loadModel('Forum');
    $this->view->forum_topics = $topic_model->getForumTopics(2);
    $this->view->render('forum/viewTopics');
}

public function showForumTopicsCat3()
{
    $topic_model = $this->loadModel('Forum');
    $this->view->forum_topics = $topic_model->getForumTopics(3);
    $this->view->render('forum/viewTopics');
}

public function showForumTopicsCat4()
{
    $topic_model = $this->loadModel('Forum');
    $this->view->forum_topics = $topic_model->getForumTopics(4);
    $this->view->render('forum/viewTopics');
}

Model

/**
 * Gets an array that contains all the forum topics in the database.
 * Each array element is an object, containing a specific topic's data.
 * @return array All the forum topics
 */
public function getForumTopics($cat_id)
{
    $sql = 'SELECT * FROM forum_categories fc JOIN forum_topics ft ON fc.cat_id = ft.cat_id WHERE fc.cat_id = :cat_id ORDER BY ft.topic_date DESC';
    $query = $this->db->prepare($sql);
    $query->execute(array(':cat_id' => $cat_id));

    return $query->fetchAll();
}

View

<?php
    if ($this->forum_categories) {
            echo 'Category1';
            echo 'Category2';
            echo 'Category3';
            echo 'Category4';
    } else {
        echo '
There are no categories.
';
    }
?>

My Concern: Is there anyway to this more DRY and more reusable?

you could do something like that :

public function showForumTopicsByCat($catId)
{
    $topic_model = $this->loadModel('Forum');
    $this->view->forum_topics = $topic_model->getForumTopics($catId);
    $this->view->render('forum/viewTopics');
}

and on view

<?php
if ($this->forum_categories) {
   for($i=o; $i < $numberCat; $i++){
       echo 'Category1';
    }
   } else {
    echo '
There are no categories.
';
}
?>


basic javascript to advanced javascript I have following code for a content scroller. This has been implemented with basic jquery. Iam trying to convert the same using advanced javascript concepts. Guide me to implement the same.

Working Code :

$(document).ready(function() {

var currentPosition = 0,
    slideWidth = 112,
    slides = $('#scroller ul li'),
    numberOfSlides = slides.length;
    scrollerContainer = $('#scroller ul');

$(scrollerContainer).css('width', slideWidth * numberOfSlides);

manageControls(currentPosition);

$('.control').click(function(){
    currentPosition = ($(this).attr('id')=='rightControl') ? currentPosition+1 : currentPosition-1;
    manageControls(currentPosition);
    $(scrollerContainer).animate({
        'marginLeft' : slideWidth*(-(currentPosition*3))
    });
});

function manageControls(position){
    if(position==0){ $('#leftControl').hide() }
    else{ $('#leftControl').show() }

    if((position*3)==numberOfSlides-1 || (position*3)==numberOfSlides-3 || (position*3)==numberOfSlides-2) { $('#rightControl').hide() }
    else{ $('#rightControl').show() }
}
});

Advanced Code Structure : Initial understanding

if(!scroller) { var scroller = {}; }

scroller =
{
    videos : {

        moveThumbnails : function() {

        },
        gotonextThumbnails : function() {
            var obj = this;

        },
        gotopreviousThumbnails : function() {
            var obj = this;

        },
        init : function() {
            var obj = this;

        }
    },
    init : function() {
        var obj = this;
        this.videos.init();
    }
}

Working example : http://jsfiddle.net/ylokesh/9EyEu/

I can't post a full answer about the "advanced JavaScript" part of your question, but here's what I would change in the actual code:

Use jQuery.fn.find method

You could optimize this $('#scroller ul li') with this $('#scroller').find('ul li'). When using a selector like #someidname, jQuery can use directly the native method document.getElementById. When using  #someidname something, then jQuery has to parse the selector, and only after this it can say "oh there's an id string in there !".
Don't recreate jQuery objects

When doing var scrollerContainer = $('#scroller ul');,  scrollerContainer becomes a jQuery object. Then, you don't need to do $(scrollerContainer).someMethodName() but you could directly write scrollerContainer.someMethodName().
Always use triple equals sign

You should (almost) always prefer === over ==. The double equal sign may have some unexpected behavior such as "\t \n" == 0 which evaluate to true.
Don't use jQuery when it's not needed

$(this).attr('id') could be simplified in this.id. This way, you don't construct a new jQuery object (this can be costly when done in loops for example), and the id property is supported in a cross-browser way so you don't have to worry about it.
Cache your DOM selections

$('#leftControl') and $('#rightControl') should be cached in variables in the same way as $('#scroller ul') is. We should avoid doing the same DOM query wherever it's possible.
Prefer jQuery.fn.on or jQuery.fn.bind methods over traditional ones

Because:

they are faster: internally $('whatever').click(fn) calls $('whatever').on('click', fn), so you can use it directly and avoid a function call,
they are more flexible: you can bind the same function to different events ($('whatever').on('click mouseenter mouseleave', fn)), you can namespace your events ($('whatever').on('click.myPlugin', fn)) which IMO would be useful in your case (your doing some sort of namespacing)

Simplify (Math) expressions

IMO, the code slideWidth*(-(currentPosition*3)) is not really readable. Why using such parenthesis ? And what about this inner - sign ?

What about writing it this way -slideWidth*currentPosition*3 ?

Also, the part (position*3)==numberOfSlides-1 || (position*3)==numberOfSlides-3 || (position*3)==numberOfSlides-2 can be simplified. Have a look (shorter variable names for the example):

(p*3)==n-1 || (p*3)==n-3 || (p*3)==n-2
// which is equivalent to
1==n-p*3 || 3==n-p*3 || 2==n-p*3
// which is equivalent to
1 <= n-p*3 && n-p*3 <= 3
// and also equivalent to
(n-p*3-1) in [0,0,0]

so you end up with this:

if( (numberOfSlides-position*3-1) in [0,0,0] ) {
    ...
}

That (n-p*3-1) in [0,0,0] part is quite interesting. When doing a in b you'll search if the array b has the index a. This only happens in our case because b is an array. So we use a array b = [0,0,0] which has 3 elements. 3 comes from the expression n-p*3, this is the maximum X of the three n-p*X forms seen above. Then, since [0,0,0] has 3 elements, its index are 0, 1, and 2. So the test if the expression n-p*3 equals to one of 1, 2, or 3, we have to subtract -1.

To explain it on another way, we can say that 0 in [0,0,0], 1 in [0,0,0], and 2 in [0,0,0] will both be true (because we're using the array indexes and not its values), and all others numerical values will evaluate the expression to false. Since what you want you is to check if n-p*3 is of one 1, 2 or 3, if we remove 1 to this expression, we retrieve our values 0, 1, and 2 from above which will evaluate the expression to true.

If you think this one is too tricky (and to be honest, it is !), you can do the following, which avoid code repetition and where the intent is explicit and readable (I would recommend this technique over the index i [0,0,0] one which is implicit and less readable IMO)

var temp = numberOfSlides - position * 3;
if( 1 <= temp && temp <= 3 ) {
    ...
}

Hope that helps :)


Binding click event through elements I need to know whether I am doing the best way of binding click event through elements in jQuery. I need to avoid duplicate code in my function (since lot of duplicate code is repeated in all functions).

How can I clean this up and do it in a better way?

 $(".nna-icon-excel").click(function (e) {
                var $this = $(this);
                $this.parent().attr("href", "?excel=1")
                var href = $this.parent().attr("href");
                if (activeTab.toLowerCase() == "table") {
                    action = "expand";
                }
                else {
                    action = "expandheatmap";
                }
                $this.parent().attr("href", href + "&from=" + action + "&expand=" + Expand + "");
            });
            $(".nna-icon-pdf").click(function (e) {
                var $this = $(this);
                $this.parent().attr("href", "?savehtml=1&openpdf=1&orientation=@ViewBag.PdfOrientation")
                var href = $this.parent().attr("href");
                if (activeTab.toLowerCase() == "table") {
                    action = "expand";
                }
                else {
                    action = "expandheatmap";
                }
                $this.parent().attr("href", href + "&from=" + action + "&tabactive=" + action + "&expand=" + Expand + "");
            });
            $(".gridoptions-anchor").click(function (e) {
                var $this = $(this);
                $this.attr("href", "/home/xx/Index" + "/?columnchooser=1");
                var href = $(this).attr("href");
                var action = "";
                if (activeTab.toLowerCase() == "table") {
                    action = "expand";
                }
                else {
                    action = "expandheatmap";
                }

              $this.attr("href", href  + "&from=" +action + "&expand=" + Expand+ "");

            });

You are right, the code could be tigher

This

var action = "";
if  {
    action = "expand";
}
else {
    action = "expandheatmap";
}

could be reduced with a ternary operation to:

var action = (activeTab.toLowerCase() == 'table') ? 'expand' : 'expandheatmap`;

Since you repeat this in every handle, you should consider building a function for this, in case the logic ever changes.
I am a bit confused by your code, you seem to build the href, get the href, modify that string and then set the href again ? Why ? The following should work in my mind ( I took the last click handler ) :

$(".gridoptions-anchor").click(function (e) {
    var href = "/home/xx/Index" + "/?columnchooser=1"
    var action = (activeTab.toLowerCase() == 'table') ? 'expand' : 'expandheatmap`;
    $(this).attr("href", href  + "&from=" +action + "&expand=" + Expand + "");
});

This brings me to my next point; activeTab, action, and Expand are not defined with var in this code
Finally you could consider a dedicated URL building function to have the string concatenation in one place:

function buildURL( href, action, expand, addTabActive )
{
  var tabActiveParameter = addTabActive ? ( '&tabactive=' + action ) : '';
  return href + "&from=" + action + "&expand=" + expand + tabActiveParameter;
}

Which brings me to my last point if tabactive shows the action, then activetab would be a better name.