Parsing argument list specifying a player to be added I want to start off by saying sorry, I wasn't able to search for this problem as I couldn't exactly describe it myself.

I have a series of if-statements to account for each scenario. As you can see in the mammoth block of code below, I want arguments to come in any order. How, if possible, can I shorten this?

if(args[1].startsWith("u:") && args[2].startsWith("r:") && args[3].startsWith("l:")){ // u / r / l
    Player p = player.getServer().getPlayer(args[1].split(":")[1]);
    String r = args[2].split(":")[1];
    String l = args[3].split(":")[1];
    Config.addPlayer(p, r, l, player);
    return true;
}else if(args[1].startsWith("r:") && args[2].startsWith("l:") && args[3].startsWith("u:")){ // r / l / u
    Player p = player.getServer().getPlayer(args[3].split(":")[1]);
    String r = args[1].split(":")[1];
    String l = args[2].split(":")[1];
    Config.addPlayer(p, r, l, player);
    return true;
}else if(args[1].startsWith("l:") && args[2].startsWith("u:") && args[3].startsWith("r:")){ // l / u / r
    Player p = player.getServer().getPlayer(args[2].split(":")[1]);
    String r = args[3].split(":")[1];
    String l = args[1].split(":")[1];
    Config.addPlayer(p, r, l, player);
    return true;
}else if(args[1].startsWith("u") && args[2].startsWith("l:") && args[3].startsWith("r:")){ // u / l / r
    Player p = player.getServer().getPlayer(args[1].split(":")[1]);
    String r = args[3].split(":")[1];
    String l = args[2].split(":")[1];
    Config.addPlayer(p, r, l, player);
    return true;
}else if(args[1].startsWith("l:") && args[2].startsWith("r:") && args[3].startsWith("u:")){ // l / r / u
    Player p = player.getServer().getPlayer(args[2].split(":")[1]);
    String r = args[2].split(":")[1];
    String l = args[1].split(":")[1];
    Config.addPlayer(p, r, l, player);
    return true;
}else if(args[1].startsWith("r:") && args[2].startsWith("u:") && args[3].startsWith("l:")){ // r / u / l
    Player p = player.getServer().getPlayer(args[2].split(":")[1]);
    String r = args[1].split(":")[1];
    String l = args[3].split(":")[1];
    Config.addPlayer(p, r, l, player);
    return true;
}

First of all, don't ever write code like this... you know it can be done better, so don't waste your time.

Moreover, you can be sure, you'll make some errors there, e.g., this looks wrong:

Player p = player.getServer().getPlayer(args[2].split(":")[1]);
String r = args[2].split(":")[1];

as you're using args[2] twice.

It's actually very trivial:

String[] inputs = {args[1], args[2], args[3]);
Arrays.sort(inputs);
if (inputs[0].startsWith("l:") && inputs[1].startsWith("r:") && inputs[2].startsWith("u:")) {
    Player p = player.getServer().getPlayer(inputs[2].split(":")[1]);
    String r = inputs[1].split(":")[1];
    String l = inputs[0].split(":")[1];
    Config.addPlayer(p, r, l, player);
    return true;
} else {
    return false;
}

A possibly even simpler alternative would be to go through the args and always fill the appropriate data slot (p, r, and l). After processing all three args, all slots must be filled, otherwise the input is wrong.

There's much more to say... e.g., your repetitive

args[2].split(":")[1]

should be replaced by a method. You also most probably don't want to allow args like l:12:34:567 (or you don't want to lose the part after the second colon), so don't use split.

Actually, I guess, you need some command line parser... there are at least 10 such things in Java freely available.


Python BMI Calculator I wrote a BMI Calculator. I tested the code and everything seems to be working fine. I would like to check if there is any room for improvement.

class bmi:
    def __init__(self):
        to_start = input("Y to start N to exit: ")
        if to_start in ("Y","y"):
            found = False
            while not found:
                weight_input = float(input("Please enter your weight(kg): "))
                if weight_input < 0:
                    print("Invalid Value")
                else:
                    weight_confirm = input("Y to confirm. N to exit: ")
                    if weight_confirm in ("Y","y"):
                        while not found:
                            height_input = float(input("Please enter your height(m): "))
                            if height_input < 0:
                                print("Invalid Value")
                            else:
                                BMI = round(weight_input/(height_input*height_input),2)
                                print ("Your BMI is: ",BMI)
                                if BMI < 18.5:
                                    print("Under-Weight")
                                    found = True
                                elif 18.5 <= BMI <= 24.99:
                                    print("Healthy Weight")
                                    found = True
                                elif 25.0 <= BMI <= 29.99:
                                    print("Over-weight")
                                    found = True
                                elif BMI > 30:
                                    print("Obese")
                                    found = True
                    else:
                        print("Re-enter")
                        found = False
        else:
            exit()

if __name__ == '__main__':
    a = bmi()

Introduction
After reflecting and reading other answers, I found that Adam Smith is right: we should get rid of the class. For this problem I think that it's better to structure the program as a group of functions. This is my new answer.
Style
Variable names should be lowercase, so your variable BMI should be renamed bmi.
Modularity
According to Wikipedia, "In software design, modularity refers to a logical partitioning of the "software design" that allows complex software to be manageable for the purpose of implementation and maintenance. The logic of partitioning may be based on related functions, implementation considerations, data links, or other criteria." So you should divide your code into smaller units, for this purpose we will divide your code into a group of functions.
This is better, because it's a bad practice, IMHO, to write all your code as a single whole without dividing it into units. This makes your code reusable, readable, and maintainable.
Documentation
It's a good practice, IMHO, to document your code, this makes it more readable and easier to use. For this purpose we will use docstrings to document the functions that we'll define. Docstings are indented and contained into triple quotes, they describe what the function does, its arguments, the type of its arguments, its return value and the type of the return value (when used with functions). Docstrings are the right way to document functions, not comments, because if you need help about using the function, you can use help(name here) to display the docstring and get help, which you can't do with comments.
Dividing the Code into Functions
To achieve modularity, we'll divide the code into functions. We'll define:

a boolen-valued function (a function that returns only True or False) that asks whether the user wants to start. We will rename to_start to answer. Using upper method, we'll make the input uppercase, so we can compare it to 'Y' and 'N' easily if the user entered 'y' or 'n' instead.

a function that handles both weight and height.

a function that takes calculates the BMI and returns it.

a function that returns the weight status.

a function called main that contains all the code that will be executed. We'll use it as the body of if __name__ == '__main__' by calling it.

Return Value
Instead of making our new functions print the output, we'll make them return it. You can read about return values here (this is a link to chapter in Think Python 2e, I recommend that you read the whole book). Just printing the output gets the job done but makes functions less reusable. Suppose that you want to store the BMIs associated with users in a textfile, what will you do? Sure you can make other functions to get this done, but if you make your functions return the result you can reuse many cases.
Code after Refactoring
def body_mass_index(weight, height):
    """Returns the BMI based on weight and height.

    weight: float or int

    height: float or int

    Returns: float or int
    """
    return round(weight / height ** 2 , 2)

def weight_category(bmi):
    """Returns the weight category of the user based on their BMI.

    Returns: str
    """
    if bmi < 18.5:
        return 'underweight'

    elif 18.5 <= bmi <= 24.99:
        return 'healthy weight'

    elif 25.0 <= bmi <= 29.99:
        return 'overweight'

    elif bmi > 30:
        return 'obese'

def wants_to_start():
    """Asks whether the user wants to start.

    Returns: bool
    """
    while True:
        answer = input("Y to start N to exit: ").upper()

        if answer == 'Y':
            return True
        elif answer == 'N':
            return False

        print('Please try again.\n')

def get_user_data():
    """Asks for the user's weight and height.

    Rerurns: tuple of 2 floats or integers
    """
    while True:
        try:
            weight = float(input('Enter your weight in kilograms: '))
            height = float(input('Enter your height in meters: '))

            if 0 < weight and 0 < height:
                return weight, height
            else:
                raise ValueError()

        except ValueError:
            print('Invalid input for weight or height')
            continue

def main():
    if wants_to_start():
        weight, height = get_user_data()
        bmi = body_mass_index(weight, height)
        category = weight_category(bmi)
        print('Your BMI is: ', bmi)
        print('Your are ', category, '.')

    else:
        quit()

if __name__ == '__main__':
    main()

Notes
The function get_user_data is adopted from Lukasz Salitra's function _get_user_info in his answer to the OP's previous question. You can find the question here.
I'm no expert on BMI, so I've no idea if the program does what it's supposed to do properly. I focused on making the code more reusable, readable, and well-structured, according to what I know as a beginner.


Well, fine for good signals. Good signals means low noise level, absense of features like very slow volume change rates, etc.
Otherwise, you need things like FIR filter(s) to average and diffifentiate signal and use an interpolation to increase precision of the detection.


Loop on maps to find duplicate My code does the following:

There are two  Maps with properties. I need to check if properties with the same key and value already exist, and if they do, throw an exception.

    Map<String, Object> existingProperties = requires.get(requiresIndex).getProperties();
    for (Map.Entry<String, Object> property : properties.entrySet()) {
        if (existingProperties.containsKey(property.getKey())){
            if (existingProperties.get(property.getKey()) instanceof String && property.getValue() instanceof String ){
                String existingValue = (String) existingProperties.get(property.getKey());
                String newValue = (String) property.getValue();
                if (existingValue.equals(newValue)){
                    throw new Exception("Property " + property.getKey() + " is existsing with value " + newValue + "in requires " + requiresName);
                }
            }
        }
    }

We are working on Java 8. Is there a better/shorter way to write it?

Sharon Ben Asher has a good answer using streams. I'd like to add some suggestions for the original code, since beginners are often not familiar with streams in Java.

You have some redundancy in your code, meaning code that is executed multiple times, which can hurt readability, decrease performance and introduce bugs. To tackle the issue, you can save results from method calls in variables and just access the variables multiple times. This also allows you to give the result a new name, and although it might make the code slightly longer in terms of lines of code, it can also make it a lot cleaner, and the individual lines shorter.

The most important example is calling property.getKey() and property.getValue() multiple times. Instead, do that at the beginning of the loop, and save the results in variables.

for (Map.Entry<String, Object> property : properties.entrySet()) {
    String key = property.getKey();
    Object value = property.getValue();

Then you access them in your code, which makes it easier to see where the same values are used. You are using the same method call to access another element of a different map, so again you can use a variable to save the result and make the code clearer and shorter:

    if (existingProperties.containsKey(key)) {
        Object existingObject = existingProperties.get(key);
        if (existingObject instanceof String && value instanceof String) {
            String existingValue = (String) existingObject;
            String newValue = (String) value;

We can improve readability by formatting very long lines (like that long string concatenation) on 2 or 3 lines, introducing newlines where we can devide it into logical parts:

            if (existingValue.equals(newValue)) {
                throw new Exception("Property " + key +
                                    " is existing with value " + newValue +
                                    " in requires " + requiresName);
            }

By the way, there seems to have been a space missing at the start of the string "in requires ", and a typo in " is existsing with value ".


Counting subscribers with matching ZIP code, interests, and sex I am trying to cycle through a few of these blocks. They basically narrow down a number of people that fulfill a bunch of attributes.

I apologize if this seems really messy, but my database is really taking a toll processing this, and I know there's a better way. I'm just lost on strategy right now.

def count_of_distribution
  #beginning with an array..
  array_of_users = []

  # any matching zip codes? ..
  # zip_codes
  @zip_codes = self.distributions.map(&:zip_code).compact
  unless @zip_codes.nil? || @zip_codes.empty?
    @matched_zips = CardSignup.all.map(&:zip_code) & @zip_codes
    @matched_zips.each do |mz|
      CardSignup.find(:all, :conditions => ["zip_code = ?", mz]).each do |cs|
        array_of_users << cs.id
      end
    end
  end

  # any matching interests?..
  # interest
  @topics = self.distributions.map(&:me_topic).compact
  unless  @topics.nil? || @topics.empty?
    @matched_topics = MeTopic.all.map(&:name) & @topics
    @matched_topics.each do |mt|
      MeTopic.find(:all, :conditions => ["name = ?", mt]).each do |mt2|
        mt2.users.each do |u|
          array_of_users << u.card_signup.id if u.card_signup
        end
      end
    end
  end

  # any matching sexes?..
  # sex
  @sexes = self.distributions.map(&:sex).compact
  unless @sexes.nil? || @sexes.empty?
    @matched_sexes = CardSignup.all.map(&:sex) & @sexes
    @matched_sexes.each do |ms|
      CardSignup.find(:all, :conditions => ["sex = ?", ms]).each do |cs|
        array_of_users << cs.id
      end
    end
  end

  total_number = array_of_users.compact.uniq

  return total_number
end

The first thing I notice is that you're calling map.{}.compact when you probably could be calling select{}, that is, you want to select from self.distribution. So that would replace 6 method calls with 3.

If you use select, it always returns an array and never a nil, so instead of unless X.nil or X.empty, you could use if X.any?, which would replace 6 method calls with three.

If you aren't using @matched_zips outside of this context, it makes no sense to assign a variable here. Instead of assigning, you could just use (CardSignup.all.map(&:zip_code) & @zip_codes).each do, which would spare you three variable assignments.


Efficient numpy cosine distance calculation I want to calculate the nearest cosine neighbors of a vector using the rows of a matrix, and have been testing the performance of a few Python functions for doing this.

def cos_loop_spatial(matrix, vector):
    """
    Calculating pairwise cosine distance using a common for loop with the numpy cosine function.
    """
    neighbors = []
    for row in range(matrix.shape[0]):
        neighbors.append(scipy.spatial.distance.cosine(vector, matrix[row,:]))
    return neighbors

def cos_loop(matrix, vector):
    """
    Calculating pairwise cosine distance using a common for loop with manually calculated cosine value.
    """
    neighbors = []
    for row in range(matrix.shape[0]):
        vector_norm = np.linalg.norm(vector)
        row_norm = np.linalg.norm(matrix[row,:])
        cos_val = vector.dot(matrix[row,:]) / (vector_norm * row_norm)
        neighbors.append(cos_val)
    return neighbors

def cos_matrix_multiplication(matrix, vector):
    """
    Calculating pairwise cosine distance using matrix vector multiplication.
    """
    dotted = matrix.dot(vector)
    matrix_norms = np.linalg.norm(matrix, axis=1)
    vector_norm = np.linalg.norm(vector)
    matrix_vector_norms = np.multiply(matrix_norms, vector_norm)
    neighbors = np.divide(dotted, matrix_vector_norms)
    return neighbors

cos_functions = [cos_loop_spatial, cos_loop, cos_matrix_multiplication]

# Test performance and plot the best results of each function
mat = np.random.randn(1000,1000)
vec = np.random.randn(1000)
cos_performance = {}
for func in cos_functions:
    func_performance = %timeit -o func(mat, vec)
    cos_performance[func.__name__] = func_performance.best

pd.Series(cos_performance).plot(kind='bar')

The cos_matrix_multiplication function is clearly the fastest of these, but I'm wondering if you have suggestions of further efficiency improvements for matrix vector cosine distance calculations.

Your code does not run: there are missing import statements:

import numpy as np
import scipy.spatial.distance

Your algorithms compute different results, so some of them must be wrong!

>>> vector = np.array([1, 2, 3, 4, 5])
>>> matrix = np.array([[7, 5, 8, 1, 9], [6, 6, 4, 0, 8]])
>>> cos_matrix_multiplication(matrix, vector)
array([ 0.81818182,  0.76558762])
>>> cos_loop(matrix, vector)
[0.81818181818181823, 0.76558761870466252]
>>> cos_loop_spatial(matrix, vector)
[0.18181818181818177, 0.23441238129533748]

In cos_matrix_multiplication and cos_loop, you've forgotten to subtract the distance from 1. In what follows I have fixed these bugs.
Your graph. (i) It has a very poor "data-ink ratio": just three data points! (ii) We can't actually read off the data points in any case, only estimate them based on the length of the bars; (iii) The labels are hard to read because they are written vertically; (iv) I can't copy and paste the labels, let alone the data. A better way to present this kind of information is in a table, like the one I give below.
You hint in the text that your real problem is to do with finding nearest neighbours. It's always best to ask about your real problem rather than some proxy for it. We might be able to suggest a solution that involves a different approach. (For example, if you were using Euclidean distance rather than cosine distance, it might make sense to use scipy.spatial.KDTree. But we can't help you unless you tell us what you're really trying to do.)
Scipy includes a function scipy.spatial.distance.cdist specifically for computing pairwise distances. In your case you could call it like this:

def cos_cdist(matrix, vector):
    """
    Compute the cosine distances between each row of matrix and vector.
    """
    v = vector.reshape(1, -1)
    return scipy.spatial.distance.cdist(matrix, v, 'cosine').reshape(-1)

You don't give us your test case, so I can't confirm your findings or compare them against my own implementation. For example, how many dimensions do your points typically have? I used the following test case:

import timeit

def test():
    algorithms = (cos_loop_spatial, cos_loop, cos_matrix_multiplication,
                  cos_cdist)
    vector = np.random.rand(10)
    matrix = np.random.rand(100000, 10)
    for algorithm in algorithms:
        t = timeit.timeit(lambda:algorithm(matrix, vector), number=1)
        print("{:30s} {:f}".format(algorithm.__name__, t))

Which produces the following results:

cos_loop_spatial               8.231966
cos_loop                       7.212096
cos_matrix_multiplication      0.106005
cos_cdist                      0.019018

So scipy.spatial.distance.cdist is about five times as fast (on this test case) as cos_matrix_multiplication.


Calculate the intersections of opening hours I have a solution but it feels bloated and unclean. How can I clean it up? The second for loop especially does not feel right.

I'm trying to calculate the opening hours but where we only use the highest opening time and the lowest closing time in a time-block. I will show you an example to clarify it.

If we have two time-blocks with

10:00-12:00 and 10:30-12:30

The result for the opening hour is going to be

10:30-12:00

Now imagine we have an array for a specific day:

[
    {
        "opens": "10:00",
        "closes": "12:00",
        "weekday": 5
    },
    {
        "opens": "16:00",
        "closes": "19:30",
        "weekday": 5
    },
    {
        "opens": "11:00",
        "closes": "12:30",
        "weekday": 5
    },
    {
        "opens": "17:00",
        "closes": "18:30",
        "weekday": 5
    }
]

Our opening hour should be for this day:

[
    {
        "opens": "11:00",
        "closes": "12:00"
    },
    {
        "opens": "17:00",
        "closes": "18:30"
    }
]

Now to my approach:

First I'm going to through the whole opening hour array for a specific day:

this.filteredTimes.forEach((time, index, array) => {...});

Inside the for loop, I'm saving the opens and closes time in MomentJS:

let opens = this.momentTime(time.opens); //saving it to moment
let closes = this.momentTime(time.closes); //moment(time, 'HH:mm')

After that, I'm again going through the whole array in another for loop:

for(let i = 0; i < array.length; i++){...} //array === this.filteredTimes

Inside the loop, I'm saving the opens and closes times for each time block again, but now I'm comparing it to the opens and closes time from the first for loop.

let arrayOpens = this.momentTime(array[i].opens);
let arrayCloses = this.momentTime(array[i].closes);

if(arrayOpens.isBetween(opens, closes)){
   opens = arrayOpens.isAfter(opens) ? arrayOpens : opens;
}

If the arrayOpens is between the saved time block, we can save a new opens time if our condition is true.

After that, we are going to save the new opens and closes time and we try to avoid duplication:

let hour = {
    opens: opens.format('HH:mm'),
    closes: closes.format('HH:mm')
};

if(! openingHour.find(element => element.opens == hour.opens && element.closes == hour.closes)){
    openingHour.push(hour);
}

The whole code is here to test (or on JsFiddle):

moment().format();

let filteredTimes = [
 {
    "opens": "10:00",
    "closes": "12:00",
    "weekday": 5
  },
  {
    "opens": "16:00",
    "closes": "19:30",
    "weekday": 5
  },
  {
    "opens": "11:00",
    "closes": "12:30",
    "weekday": 5
  },
  {
    "opens": "17:00",
    "closes": "18:30",
    "weekday": 5
  }
];

let openingHour = [];

calculate();

function calculate(){

  filteredTimes.forEach((time, index, array) => {
    let opens = momentTime(time.opens);
    let closes = momentTime(time.closes);

    for(let i = 0; i < array.length; i++){
      let arrayOpens = momentTime(array[i].opens);
      let arrayCloses = momentTime(array[i].closes);

      if(arrayOpens.isBetween(opens, closes)){
        opens  = arrayOpens.isAfter(opens) ? arrayOpens : opens;
      }
      if(arrayCloses.isBetween(opens, closes)){
        closes  = arrayCloses.isBefore(closes) ? arrayCloses : closes;
      }
    }

    let hour = {
      opens: opens.format('HH:mm'),
      closes: closes.format('HH:mm')
    };

    if(! openingHour.find(element => element.opens == hour.opens && element.closes == hour.closes)){

      openingHour.push(hour);
    }

  });

  console.log(openingHour);
  return openingHour;
}

function momentTime(time){
 return moment(time, 'HH:mm');
}
<script src="https://cdnjs.cloudflare.com/ajax/libs/moment.js/2.22.2/moment.min.js"></script>

calculate is not reusable. It's hardcoded to always use filteredTimes. It's also putting its results to an array on the outside. Strive for pure functions as much as possible, like math equations. It keeps them reusable, testable and deterministic.
Your variable names are vague. A good sign that you're not naming things well enough is when you have to explain to another developer what it is (like what you're doing in the comments). Never be afraid to name things verbosely.
You might not need moment.js. What you're doing with moment.js is just intermediary conversion of formats - an unnecessary procedure. When operating with dates and times, it's best to have your data in a format that doesn't need intermediate conversions.

Now simply put, what you're essentially looking for "windows" where the given times overlap. Here's a modified version of your code. The modifications include:

The times are all numbers. You don't really need to have the HH:mm format in this operation. If you strip off the : from the time, they're effectively numbers from 0000 to 2359` (with the last 40 mins in each hour missing). This eliminates the need for moment.js.
Nested array.forEach(), because for loops are also clunky (you have to keep indexes, do length comparisons, etc.). Plus you're using array.forEach() already anyways.
I stored results in an object keyed by the combination of from and to. This way, the lookup doesn't require a very clunky array.find(). Converting it to an array is as simple as iterating over the keys and mapping them to the values.
computeWindows is now a pure-ish function. You can now call it anytime, anywhere with any input, and always get a deterministic-ish output (Object.keys() do not guarantee the order of keys, so while the items may be the same, the order may be different by browser).

const ranges = [{
  from: 1000,
  to: 1100
}, {
  from: 1015,
  to: 1115
}, {
  from: 1030,
  to: 1130
}, {
  from: 1045,
  to: 1145
}, {
  from: 1100,
  to: 1200
}]

const isBetween = (value, start, end) => value > start && value < end

const computeWindows = ranges => {
  const index = {}

  ranges.forEach(({ from: rFrom, to: rTo }) => {
    ranges.forEach(({ from: cFrom, to: cTo }) => {
      // The actual logic
      rFrom = isBetween(cFrom, rFrom, rTo) && cFrom > rFrom ? cFrom : rFrom
      rTo = isBetween(cTo, rFrom, rTo) && cTo < rTo ? cTo : rTo
    })

    // Construct the key and value
    const key = `${rFrom}-${rTo}`
    const value = { from: rFrom, to: rTo }

    // Use existing value of key or add new value to key.
    index[key] = index[key] || value
  })

  // Convert results into an array of ranges.
  return Object.keys(index).map(k => index[k])
}

console.log(computeWindows(ranges))


Determining directions for a 2D boad My original code is:

let (upPosition, downPosition, leftPosition, rightPosition) =
    match myMove.MyDirection with
    | Direction.Up -> (GoUp y, y, GoLeft x, GoRight x)
    | Direction.Down-> (y, GoDown y, GoLeft x, GoRight x)
    | Direction.Left -> (GoUp y, GoDown y, GoLeft x, x)
    | Direction.Right -> (GoUp y, GoDown y, x, GoRight x)
    | _ -> (y, y, x, x)

Basically, there is an object on the 2D board. myMove.MyDirection dictates its next move, and x,y are its target position.

Once the object gets to x,y, it would cause some impacts on its front, left, right neighbours from its local rotational space. GoUp, ... , GoRight determines how far those neighbours could be.

So, ultimately, the code returns a "+" (cross) shape centred at x,y, so the minimum variable to describe this shape is (x, y, upPosition, downPosition, leftPosition, rightPosition).

The best solution I can think of to the previous code is the following. However, I feel there is more can be done to make it better. Is there any map function for tuple and is it costly?

let Reverse = function
    | Direction.Up -> Direction.Down
    | Direction.Down -> Direction.Up
    | Direction.Left -> Direction.Right
    | Direction.Right -> Direction.Left
    | _ -> raise (ArgumentException("Not Vaild Move!"))

let Go myDirection filter pos =
    match myDirection, filter with
    | a,b when a=b -> pos
    | Direction.Up , _  -> GoUp pos
    | Direction.Down , _ -> GoDown pos
    | Direction.Left , _ -> GoLeft pos
    | Direction.Right , _ -> GoRight pos
    | _ -> pos

let (upPosition, downPosition, leftPosition, rightPosition) =
    let rev = Reverse myMove.MyDirection
    (Go Direction.Up rev y, Go Direction.Down rev y, Go Direction.Left rev x, Go Direction.Right rev x)

It's tough to review only part of the code, because there are often small improvements which could be made but they depend on knowing the structure of the entire program -- for example, why are you using a tuple to pass around the positions ((upPosition, downPosition, leftPosition, rightPosition))? You'd likely be better off using a record type to encapsulate those values, since you'd only have to pass one variable around (the record) and it also facilitates structural matching which'll allow you to clean up some of your other functions.

The Direction enum would be much better as a discriminated union type (see the code below). Even though enums can define certain values (e.g., Up, Down, Left, Right) they can also take on any integer value by casting the value to the enum type. Discriminated union types also allow you to define a fixed set of values, but the F# type system ensures that your variables can have only the defined values -- which in turn, keeps you from having to use the wildcard pattern (_) to check for error conditions in your match statements.

Here's my version of your code -- it's largely the same, but I've changed Direction into a union type and moved the Reverse function into a new Direction module.

type Direction =
    | Up
    | Down
    | Left
    | Right

[<RequireQualifiedAccess; CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]
module Direction =
    /// Reverses a specified direction.
    [<CompiledName("Reverse")>]
    let rev = function
        | Direction.Up ->
            Direction.Down
        | Direction.Down ->
            Direction.Up
        | Direction.Left ->
            Direction.Right
        | Direction.Right ->
            Direction.Left

[<CompiledName("Go")>]
let go myDirection filter pos =
    match myDirection with
    | x when x = filter ->
        pos
    | Direction.Up ->
        GoUp pos
    | Direction.Down ->
        GoDown pos
    | Direction.Left ->
        GoLeft pos
    | Direction.Right ->
        GoRight pos

let (upPosition, downPosition, leftPosition, rightPosition) =
    match myMove.MyDirection with
    | Direction.Up ->
        (GoUp y, y, GoLeft x, GoRight x)
    | Direction.Down->
        (y, GoDown y, GoLeft x, GoRight x)
    | Direction.Left ->
        (GoUp y, GoDown y, GoLeft x, x)
    | Direction.Right ->
        (GoUp y, GoDown y, x, GoRight x)

let (upPosition, downPosition, leftPosition, rightPosition) =
    let rev = Direction.rev myMove.MyDirection
    (go Direction.Up rev y, go Direction.Down rev y, go Direction.Left rev x, go Direction.Right rev x)

EDIT: Answers to your questions:

Normally, you can't have a module and a type with the same name within a certain namespace, but the [<CompilationRepresentation(...)>] attribute I used adds "Module" to the name of the compiled module type. So in this case, the Direction module would be compiled to a type called DirectionModule.
You could make rev a member function of the Direction type. However, in F# it's easier (in general) to consume module-based functions instead of type members because it works much better with F#'s type inference mechanism. In our example, if you had an instance of Direction you wouldn't be able to call myDirection.Reverse() unless you declared myDirection as a variable of type Direction (or the F# compiler was able to figure it out another way); if you call the module-based function (Direction.rev myDirection) F# can easily infer the type of the myDirection variable so you don't need any explicit type annotations.

If you're consuming this code from C#, then I'd write the code like this to get the best of both worlds:

type Direction =
    | Up
    | Down
    | Left
    | Right

    member this.Reverse () =
        match this with
        | Direction.Up ->
            Direction.Down
        | Direction.Down ->
            Direction.Up
        | Direction.Left ->
            Direction.Right
        | Direction.Right ->
            Direction.Left

[<RequireQualifiedAccess; CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]
module internal Direction =
    /// Reverses a specified direction.
    [<CompiledName("Reverse")>]
    let inline rev (dir : Direction) =
        dir.Reverse ()

In this version of the code, I've defined Reverse as a member on the direction type (making it easy to consume from C#), but I also have the module-based function which is easier to consume from F#. Note the module-based function is now marked inline; as it only calls the Reverse() member it makes sense for the F# compiler to emit code which directly calls Reverse(). Another way to think about this is that within this version of the code, the module-based rev function is only useful for type-inference purposes (but it is useful if you're using it from within F#).
[<CompiledName(...)>] is an attribute specific to F#; when you use it on a function, the F# compiler will compile the function so it has the name you've specified in the attribute. This is primarily used when you want to consume some F# code from C#; you can define your F# functions with idiomatic F# names (e.g., rev), but by applying this attribute the methods in the compiled assembly can have idiomatic C# names (e.g., Reverse) to make them easier to consume.


Bouncy castle 128bit AES/CBC/PKCS7 in java I have been trying to do some encryption-decryption using bouncy castle and with the available docs and samples on the internet I have managed to prepare the below. Though this works without throwing any exception, i feel something fishy. Can somebody tell me if i have done it right or is there any thing that i should do :)

 private static void Test()
  {
  try
  {
    String keyString = "Olmy9iqs1LwWblwe";
    String input = "teststring";
    byte[] inputBytes = input.getBytes();
    String xiv = "1234567891234567";
    byte[] iv = xiv.getBytes("UTF-8");
    int length;
    //Set up
    AESEngine engine = new AESEngine();
    CBCBlockCipher blockCipher = new CBCBlockCipher(engine);
    PaddedBufferedBlockCipher cipher = new PaddedBufferedBlockCipher(blockCipher);
    KeyParameter keyParam = new KeyParameter(keyString.getBytes());
    ParametersWithIV keyParamWithIV = new ParametersWithIV(keyParam, iv, 0, 16);

    // Encrypt
    cipher.init(true, keyParamWithIV);
    byte[] outputBytes = new byte[cipher.getOutputSize(inputBytes.length)];
     length = cipher.processBytes(inputBytes,0,inputBytes.length, outputBytes, 0);
    cipher.doFinal(outputBytes, length);
    String encryptedInput = new String(Base64.encode(outputBytes));
    System.out.println("Encrypted String:" +encryptedInput);

    //Decrypt
    cipher.init(false, keyParamWithIV);
    byte[] out2 = Base64.decode(encryptedInput);
    byte[] comparisonBytes = new byte[cipher.getOutputSize(out2.length)];
    length = cipher.processBytes(out2, 0, out2.length, comparisonBytes, 0);
    cipher.doFinal(comparisonBytes, length); //Do the final block
    String s2 = new String(comparisonBytes);
    System.out.println("Decrypted String:"+s2);
}
  catch (Exception e)
  {
    e.printStackTrace();
  }
}

Some observations:

It is best to always specify the encoding when converting from a string to a byte array:
input.getBytes()     -> input.getBytes("UTF-8")
keyString.getBytes() -> keyString.getBytes("UTF-8")
The IV should be an array of random bytes unique for each encryption, usually created from a CSPRNG. Then prefix the encrypted data with the IV for use during decryption, the IV does not need to be secret.
When using a password it is necessary to derive the key from it with a function such as PBKDF2.


Allocating matrices for in-place modification This code seems to be working. I am allocating matrices on the stack and passing them to functions to modify in place. Is this a standard practice, or am I doing it wrong?

Header:

#ifndef _MATRIX_H
#define _MATRIX_H

typedef enum {MT_OK, MT_INVAL} MT_RESULT;

typedef float mat4[4][4];
typedef float vec4[4];

MT_RESULT mat4multvec4(vec4 target, mat4 m, vec4 v);
MT_RESULT zero4(mat4 target);
MT_RESULT id4(mat4 target);
MT_RESULT translate(mat4 target, float x, float y, float z);
MT_RESULT scale(mat4 target, float x, float y, float z);
MT_RESULT perspective(mat4 target, float fov, float aspectratio, float nearclip, float farclip);

#endif

Implementation:

#include <stdlib.h>
#include <stdio.h>
#include <string.h> /* memcpy */
#include <math.h>

#include "matrix.h"

#undef PI
#define PI 3.14159265

/*
Multiply 4x4 matrix with 1x4 vector
returns MT_OK
*/
MT_RESULT mat4multvec4(vec4 target, mat4 m, vec4 v)
{
    int i, j, sum;

    for(i = 0; i < 4; i++) {
        sum = 0;
        for(j=0; j<4; j++) {
            sum = sum + m[i][j] * v[j];
        }
        target[i] = sum;
    }

    return MT_OK;
}

/*
make a 4x4 zeros matrix
returns MT_OK
*/
MT_RESULT zero4(mat4 target)
{
    memset(target, 0, sizeof(float) * 16);
    return MT_OK;
}

/*
make a 4x4 identity matrix
returns MT_OK
*/
MT_RESULT id4(mat4 target)
{
    int i;
    zero4(target);

    for(i = 0; i < 4; i++)
                target[i][i] = 1;

    return MT_OK;
}

/*
Create 4x4 translation matrix for opengl
return MT_OK
*/
MT_RESULT translate(mat4 target, float x, float y, float z)
{
    id4(target);

    target[0][3] = x;
    target[1][3] = y;
    target[2][3] = z;

    return MT_OK;
}

/*
Create 4x4 scale matrix for opengl
return MT_OK
*/
MT_RESULT scale(mat4 target, float x, float y, float z)
{
    zero4(target);

    target[0][0] = x;
    target[1][1] = y;
    target[2][2] = z;
    target[3][3] = 1;

    return MT_OK;
}

/*
mat4 perspective(float fov, float aspectratio, float nearclip, float farclip)
{
    mat4 m = new_mat4();
    float frustumdepth = farclip - nearclip;
    float inversefrustumdepth = 1 / frustumdepth;

    m[]
}*/

void main(void)
{
    int i;
    mat4 myMatrix;
    vec4 myVector = {10.0f, 10.0f, 10.0f, 1.0f};
    vec4 myTVector;

    translate(myMatrix, 10.0f, 0.0f, 0.0f);

    mat4multvec4(myTVector, myMatrix, myVector);

    printf("Original Vector\n");
    for(i=0;i<4;i++) {
        printf(" %f", myVector[i]);
    }

    printf("\nTransformed Vector\n");
    for(i=0;i<4;i++) {
        printf(" %f", myTVector[i]);
    }
}

Answering some of your questions...

  Is passing arrays to functions memory safe?

No, not really, but the C language offers no other approach, so you just have to be extra careful, since neither the compiler nor the runtime will alert you if you try to access invalid or out-of-bounds memory.

  I am allocating matrices on stack and passing them to function to modify in place. Is this a standard practice?

It can be, yes. If you prefer to have them modified in place, it is fine. Some libraries prefer to have separate input and output parameters, and that tends to produce cleaner code, but sometimes it's simpler to just modify in place. E.g.: It would be annoying to have, for instance, a "make identity" function in a matrix library take two parameters, like: matrix_identity(const mat* in, mat* out). Much more convenient to just write:

matrix_identity(&matrix);

However, for arithmetical operations it would be more natural to have inputs and outputs:

mat a = { ... };
mat b = { ... };
mat result;
matrix_multiply(&result, &a, &b); // result = a * b

As to allocating on the stack, leave that up to the user. Your code should only be concerned with pointers to objects, how that's allocated is up to the caller. Leave the option for both, since stack variables are cheap, but sometimes one might have to use the global heap to extend lifetime.

Other details about your code:

The error codes you're returning in each function are irrelevant, since every function always returns the same MT_OK, which means they could very well be void functions. You don't have to make all functions return an error code for the sake of uniformity. Save the error codes for functions that can actually fail for some reason.
Since C doesn't have any construct for scoping names (like modules/namespaces), it is common practice to prefix your function names with a pseudo namespace, to reduce the chance of collisions. You could maybe prefix your function names with mat4_, e.g.: mat4_ translate(), mat4_identity(), mat4_mult_vec4() etcetera.
Don't redefine PI risking using less precision than the actual supported. Most math.h implementations will have M_PI defined, which you can use. You may still provide a fallback if necessary, but default to M_PI:

#ifndef M_PI
  #define M_PI 3.14159265f
#endif // M_PI

Which matrix layout are you using? Column major, I suspect? State that very clearly somewhere, like in a documentation comment in the header file, to leave no room for confusion.


Selecting variable amount of vector elements in C++ Suppose we are given a C++ vector. We want to specify a variable amount of indices and select elements from a vector being indexed. I have two implementation: (A) one relies on C++11 initializer lists, and the second one (B) on va_list and  macro mess. Arrangement B is however funkier to type because it requires no typing the braces comprising the initializer list.

So my main question is: which one should an adult C++ programmer use, if any?

coderodde.h:

#ifndef CODERODDE_H
#define CODERODDE_H

#include <cstdarg>
#include <iostream>
#include <vector>

#define NUM_ARGS(...) (sizeof((size_t[]){__VA_ARGS__}) / sizeof(size_t))

namespace coderodde {

    template<class T>
    std::vector<T> select(const std::vector<T>& vec,
                          const std::vector<size_t>& indices)
    {
        std::vector<T> ret;

        for (auto index : indices) {
            ret.push_back(vec.at(index));
        }

        return ret;
    }

    template<class T>
    std::vector<T> select(const std::vector<T>& vec, const size_t len, ...)
    {
        std::va_list ap;
        std::vector<T> ret;
        va_start(ap, len);

        for (size_t i = 0; i < len; ++i)
        {
            ret.push_back(vec.at(va_arg(ap, size_t)));
        }

        va_end(ap);
        return ret;
    }
}

#define SELECT(vec, ...) (select(vec, NUM_ARGS(__VA_ARGS__), __VA_ARGS__))

#endif // CODERODDE_H

The test driver main.cpp:

#include <iostream>
#include <string>
#include <vector>
#include "coderodde.h"

using std::cout;
using std::endl;
using std::string;
using std::vector;
using coderodde::select;

void test_smart(const vector<string>& input)
{
    const size_t N = input.size();

    for (size_t ca = 0; ca < N; ++ca)
    {
        for (size_t cb = 0; cb < N; ++cb)
        {
            for (size_t cc = 0; cc < N; ++cc)
            {
                for (auto s : select(input, { ca, cb, cc }))
                {
                    cout << s;
                }

                cout << endl;
            }
        }
    }

}

void test_stupid(const vector<string>& input)
{
    const size_t N = input.size();

    for (size_t ca = 0; ca < N; ++ca)
    {
        for (size_t cb = 0; cb < N; ++cb)
        {
            for (size_t cc = 0; cc < N; ++cc)
            {
                for (auto s : SELECT(input, ca, cb, cc))
                {
                    cout << s;
                }

                cout << endl;
            }
        }
    }
}

int main(int argc, char** argv)
{
    std::vector<std::string> string_vec {"A", "B", "C", "D"};
    cout << "Smart:" << endl;
    test_smart(string_vec);
    cout << "Stupid:" << endl;
    test_stupid(string_vec);
    return 0;
}

You can avoid problems related to variadic macros and problems related to C-style variadic functions altogether by creating a function that takes an std::initializer_list directly:

template<class T>
std::vector<T> select(const std::vector<T>& vec, std::initializer_list<std::size_t> indices)
{
    std::vector<T> ret;
    for (std::size_t ind: indices)
    {
        ret.push_back(vec.at(ind));
    }
    return ret;
}

Moreover, it is the easiest to read of the three implementations.


Get unique entries from array For a website I'm working on, I had to get all the unique entries in an array and count their occurrence. It is possible a certain entry is only found once, but it can also be found 20 times. So I designed the following bit of code:

for ($i = 0; $i < count($nodes); $i++)
{
    for ($j = 0; $j < count($nodes[$i]); $j++)
    {
        if (!array_key_exists($nodes[$i][$j], $uniekenodes))
        {
            $uniekenodes[$nodes[$i][$j]] = 1;
        }
        else
        {
            $uniekenodes[$nodes[$i][$j]] += 1;
        }
    }
}

The $nodes array contains the the entries returned from the database. And the $uniekenodes array contains the unique entries and how many times they occured in the $nodes array.

This is my first php script (on a drupal webpage by the way) and as such I don't know that much about php. I'm pretty confident there is probably a more a efficient way to do this, using php-specific functions. Any and all tips are welcome!

EDIT: I might have to clarify the structure of the arrays:

$nodes has two dimensions. The first dimension is just a key for the second dimension. This one contains an array of drupal nodes for each key.

$uniekenodes uses the nodes from $nodes as a key and the value is how many times the node occured in $nodes

EDIT 2:

I printed the arrays, as requested by Boris Guéry:

Array
(
[0] => Array //Each of these contains node id's returned by a query
    (
        [0] => 12
        [1] => 11
        [2] => 10
        [3] => 9
        [4] => 8
        [5] => 7
    )

[1] => Array
    (
        [0] => 10
        [1] => 9
        [2] => 8
        [3] => 7
    )

[2] => Array
    (
    )

[3] => Array
    (
        [0] => 11
        [1] => 10
        [2] => 9
        [3] => 8
        [4] => 7
       )
)

    Array //This one uses the node ids from the previous array as keys, the values are the number of occurences.
    (
        [12] => 1
        [11] => 2
        [10] => 3
        [9] => 3
        [8] => 3
        [7] => 3
    )

I think the associative array approach is good. You could probably get away with:

for ($i = 0; $i < count($nodes); $i++)
  for ($j = 0; $j < count($nodes[$i]); $j++)
    $uniekenodes[$nodes[$i][$j]]++;

As incrementing NULL values results in 1.


MCRYPT - are there any flaws or areas for improvement in this class? I am working on a class for encryption to use on my site. I have read through many examples of these functions and would just like to clarify a few points I have read and check if this code is worthy. It is for storing sensitive information in a database. I am aware that making this method secure does not mean that the information is 100% secure, I simply need an appraisal of the code.

<?php

class Encryption
{
    private $eMethod = MCRYPT_RIJNDAEL_128;
    private $siteKey = "01234567890123456789012345678901"; # strlen = 32
    private $ivSize;
    private $iv;

    public function __construct()
    {
        $this->ivSize     = mcrypt_get_iv_size( $this->eMethod, MCRYPT_MODE_CBC );
        $this->iv         = mcrypt_create_iv( $this->ivSize, MCRYPT_RAND );
    }

    public function encrypt( $data, $salt )
    {
        $encryptedMessage = mcrypt_encrypt( $this->eMethod, $this->siteKey, $data, MCRYPT_MODE_CBC, $this->iv );
        $encryptedSalt    = mcrypt_encrypt( $this->eMethod, $this->siteKey, $salt, MCRYPT_MODE_CBC, $this->iv );
        $saltIdentifier   = str_pad( strlen($encryptedSalt), 6, '0', STR_PAD_LEFT );

        $newData          = $encryptedSalt.($this->iv).$encryptedMessage.$saltIdentifier;
        return($newData);
    }

    public function decrypt( $data )
    {
        $thisSaltID       = substr( $data, -6 );
        $saltID           = ltrim($thisSaltID,'0');
        $thisSalt         = substr( $data, 0, $saltID );
        $goTo             = $saltID + ($this->ivSize);
        $thisIv           = substr( $data, $saltID, ($this->ivSize) );
        $thisHash         = substr( $data, $goTo, -6 );

        $decryptedMessage = mcrypt_decrypt( $this->eMethod, $this->siteKey, $thisHash, MCRYPT_MODE_CBC, $thisIv );
        return($decryptedMessage);
    }
}

?>

When encrypting, it takes the data and a salt, encrypts both and then combines them with the iv and the padded strlen of the salt as per this line:

$newData = $encryptedSalt.($this->iv).$encryptedMessage.$saltIdentifier;

Then I know which order things appear and can pull it apart accordingly.

Is this salt unnecessary or poorly implemented?
When creating an object and encrypting a string, I pad the string to a length of multiple 16, and generate a random alphanumeric string of length 16 for the salt. Is it unnecessary to make sure the inputs are formatted in this way?
Should the site key be random every time (or static as it is now)? And are there advisements on its length?
Is using RIJNDAEL_128 with a 256-bit key the equivalent of using AES-256?
Is this code okay to use?/area of criticism

I'm no security expert, however I think answering this would be fun!

I'll start off by doing my best answering your four questions:

The salt is unnecessary.

  a salt is random data that is used as an additional input to a one-way
  function... The primary function of salts is to defend against
  dictionary attacks versus a list of password hashes and against
  pre-computed rainbow table attacks.

These first few lines from the Wikipedia page for salts
basically sum it up. Because we're working with encryption
(opposed to say, hashing), the private key and the IV are our protection, not a salt. Reiterated over here on our Security Stack Exchange site.

This section elaborates on why it's necessary for hashing to
have salts:

  Without a salt, a successful SQL injection attack may yield easily
  crackable passwords.

Well, we just found out that having a salt infused really isn't very
beneficial. So if we eliminate the salt, then all we have left in
the encrypted output is the actual encryption, and the IV. Because
the IV size won't be changing in this example, it's A-OK to strip
the IV from the string using the IV size, and then the left over is
the message encrypted.
No, it cannot be unique. How would you decrypt it later on if the data left the class?

I'm not sure how you're creating the key, but I suggest something
such as to get a truly random one:

pack("H*", bin2hex(openssl_random_pseudo_bytes(32)));

Yes. Rijndael has three members in it's family: Rijndael-128,
Rijndael-192, and Rijndael-256 (there're more, but only these three are AES standard). However, each has a block size of
128 bits. So therefore Rijndael-256 with a (the only option) block
size of 128 would be the same as saying AES-256.

  The AES algorithm is capable of using cryptographic keys of 128, 192,
  and 256 bits to encrypt and decrypt data in blocks of 128 bits.

Via NIST
Depends. Where do you want to use it? In production, I'd say pick up a framework which already has this topic highly covered and critiqued. It also depends on what you're encrypting.

Other comments

private $eMethod = MCRYPT_RIJNDAEL_128;

never changes, so you could turn this into a constant.

return($newData);

It's up to you, but it's more common to leave the parentheses off.

Your whitespace and vertical alignment can have negative effects on readability. Of course, it's up to you, but you may want to check out some other coding styles.


Indentation on a long (ish) linq chain I have some code that loops through directories named for years (e.g. /2011/, /2012/ etc) which each contain files whose names end with dates in the "yyyyMMdd" format, and builds a list of all the dates in all the directories. But what is the best way to indent this, nothing I've tried is readable:

files = files.Union(
    Directory.GetFiles(year)
             .Select(d => DateTime.ParseExact(
      Path.GetFileNameWithoutExtension(d)
          .Substring(
         Path.GetFileNameWithoutExtension(d)
             .Length - 8),
     "yyyyMMdd",
     null))).ToList();

or

files = files.Union(Directory.GetFiles(year)
                             .Select(d => DateTime.ParseExact(
                                                              Path.GetFileNameWithoutExtension(d).Substring(
                                                                                Path.GetFileNameWithoutExtension(d).Length - 8),
                                                              "yyyyMMdd",
                                                              null)
                                     )
                    ).ToList();

or

files = files.Union(Directory.GetFiles(year)
                             .Select(d => DateTime.ParseExact(
                                                              Path.GetFileNameWithoutExtension(d).Substring(
                                                                                Path.GetFileNameWithoutExtension(d).Length - 8),
                                                              "yyyyMMdd",
                                                              null))).ToList();

or

files = files.Union(Directory.GetFiles(year).Select(d => DateTime.ParseExact(
                    Path.GetFileNameWithoutExtension(d).Substring(
                    Path.GetFileNameWithoutExtension(d).Length - 8),
                    "yyyyMMdd", null))).ToList();

I would consider refactoring the .Select call to do this:

files = files.Union(Directory.GetFiles(year).Select(d => GetTimeForFile(d))).ToList();

Where GetTimeForFile is a method with the following content:

DateTime GetTimeForFile(String fileName)
{
    var noExtName = Path.GetFileNameWithoutExtension(fileName);
    return DateTime.ParseExact(noExtName.Substring(noExtName.Length - 8),
     "yyyyMMdd", null);
}

Either way, no matter how you indent it now, the method extraction of GetTimeForFile makes it significantly more readable.


Usage of a C library in a C++ project (std::string char array conversion) Abstract

I have little C/C++ skills.  I've learned programming mainly with Java, and so I chose C++ (targeting C++11) as the language to learn better. In my current C++ project, I have to use the discount C library for its Markdown functions.

To explain briefly my code: the member variable std::string markdown_ is the Markdown I want to "compile" to HTML. To do so, I have to use the following functions from discount:

MMIOT *mkd_string(char *string, int size, int flags);
int mkd_compile(MMIOT *document, int flags);
int mkd_document(MMIOT *document, char **doc);
mkd_css and mkd_toc are alike mkd_document

Questions

As you see, I need to copy the markdown_ string and make it a char*. To do so, I used a std::vector as suggested in Converting an std::string into a char array, but I would like to know if doing it this way is appropriate in my case, or if there are other ways to achieve that.
MMIOT *mkd_string(char *string, int size, int flags); is defined in the main page to return null on error. Is it correct to use nullptr like I do in the following code to check the return value?
Do you have suggestions to improve this code, or does it looks good?

Code extract

const std::string Markdown::error_msg_mkd_string = "libmarkdown failed to "
    "parse the following Markdown:\n";

void Markdown::compile()
{
    MMIOT* doc;
    int size;
    char *cstr_buff;
    int flags = MKD_TOC|MKD_SAFELINK|MKD_EXTRA_FOOTNOTE;

    std::vector<char> buff(markdown_.cbegin(), markdown_.cend());
    doc = mkd_string(&buff[0], buff.size(), flags);

    if(doc == nullptr)
        throw std::runtime_error(
            std::string(error_msg_mkd_string)
                .append(&buff[0], buff.size())
            );

    mkd_compile(doc, flags);

    // It is not a bad practice to reuse 'size' and 'cstr_buff' like in the
    // following code, or is it?
    size = mkd_css(doc, &cstr_buff);
    css_.assign(cstr_buff, size);

    size = mkd_document(doc, &cstr_buff);
    html_.assign(cstr_buff, size);

    size = mkd_toc(doc, &cstr_buff);
    toc_.assign(cstr_buff, size);

    mkd_cleanup(doc);
}

If mkd_string accepts a const char*, then you are better off using a std::string and its method c_str() to pass the value to the function. Otherwise your solution is correct.
Yes. See this answer for the why. Or build this code (tested with g++ -std=c++0x, gcc version 4.6):

#include <iostream>
int main()
{
    if (NULL == nullptr)
        std::cout << "NULL == nullptr\n";
}

Declare the variable immediately before using it, possibly also defining it. This reduces its scope to the minimum, helps the reader/maintainer of the code understanding what the variable is for, and avoids terrible errors deriving from forgetting to initialize variables. (This should also provide you with some guideline for the question about re-using the same variable twice in the same block.)


Concise Proof of the Monty Hall Problem in R This is an attempt at a concise proof of the Monty Hall problem in probability and statistics in R.

For those unfamiliar, the scenario is this:

There exists a contestant on a game show. This contestant chooses between three doors (A, B, and C), behind one of which is a new car.
The host (Monty Hall) reveals what is behind one of the other two doors.
The contestant is offered to either stick with their same door, or switch.
The question is: which choice offers the highest probability of success (winning the car)?

To complete this problem, there is some necessary background:

The probability that the car is behind any door before anything happens is uniformly distributed.
The host will never reveal the car on the first opening of a door (this is what offsets the probability).

Assume (arbitrarily) that the contestant chooses door A. If the car is behind door A, the host will open door B or C with equal (50/50) probability - this is intuitive. If the car is behind door B however, the host must open door C to prevent revealing the car, thus the probability of opening door B is zero. This foreknowledge of the location of the car is what skews the problem.

The result is that the contestant ends up with a 66% chance of winning by switching their chosen door, and a 33% if they stick with their current choice. In the code below, it is compulsory for the contestant to switch - this was to improve computational efficiency:

doors <- c("A", "B", "C")
wins <- 0
n <- 10000

for (i in 1:n) {
  # Choose which door the car is behind
  car <- sample(doors)[1]

  # The contestant chooses a door
  contestant <- sample(doors)[1]

  # Monty opens a door that the car is not behind
  monty <- sample(doors[which((doors != car) & (doors != contestant))])[1]

  # Force the contestant to switch
  contestant <- sample(doors[which((doors != monty) & (doors != contestant))])[1]

  # Count the wins
  if(contestant == car){wins <- wins+1}
}

(wins/n)

The fundamental ideas of your code are solid, however there are a couple of areas that could be improved. For starters, when you call sample and you only want one number returned, you can make use of the size argument.  For vectors of only size 3, as in your case, there isn't much difference:

library(microbenchmark)
microbenchmark(sample(doors, size = 1),
               sample(doors)[1], times = 10^4, unit = "relative")
Unit: relative
                   expr      min      lq     mean   median       uq      max neval
sample(doors, size = 1) 1.000000 1.00000 1.000000 1.000000 1.000000 1.000000 10000
       sample(doors)[1] 1.104667 1.09216 1.111044 1.086809 1.082041 1.072216 10000

However, as the size of your input vector increases, there is a noticeable difference in methods.

doorsHuges <- rep(doors, 10^6)
length(doorsHuges)
[1] 3000000

microbenchmark(sample(doorsHuges, size = 1),
               sample(doorsHuges)[1], unit = "relative")
Unit: relative
                        expr      min       lq     mean  median       uq     max neval
sample(doorsHuges, size = 1)     1.00     1.00     1.00    1.00    1.000    1.00   100
       sample(doorsHuges)[1] 65693.19 31524.98 10609.52 9126.07 6347.235 4338.52   100

Next, we can greatly improve our efficiency by taking advantage of the fact that sample can perform many replications in one go by making use of the replace argument. For example, instead of using a for loop and generating a single scenario, we can do the following:

sample(doors, n, replace = TRUE)

The gains in efficiency are tremendous:

microbenchmark(forLoop = for(i in 1:1000){sample(doors, 1)},
               usingRep = sample(doors, 1000, replace = TRUE), unit = "relative")
Unit: relative
    expr      min       lq     mean   median       uq      max neval
 forLoop 211.0778 204.1896 127.7005 199.9415 191.6994 185.1182   100
usingRep   1.0000   1.0000   1.0000   1.0000   1.0000   1.0000   100

Now, let's look at our data types.  Since we are mainly concerned with probabilities, the actual values of our main vector doesn't matter.  Since this is the case, we should look to use integers when we can.  We can achieve this by using seq_along(doors) or simply length(doors). Observe:

intDoors <- seq_along(doors)
microbenchmark(intDoors == 2, doors == "B", times = 10^4, unit = "relative")
Unit: relative
         expr      min       lq    mean   median       uq      max neval
intDoors == 2 1.000000 1.000000 1.00000 1.000000 1.000000 1.000000 10000
 doors == "B" 1.449541 1.422819 1.35682 1.396226 1.390533 1.053855 10000

While we are on integers, which is an awesome function which is very efficient and very intuitive. However, when we are indexing, it is completely unnecessary.  We can use logical subsetting instead. It is cleaner and just as efficient. Observe:

 mySamp <- sample(10^6, 10^6)
 boolTest <- rep(FALSE, 10^6)
 boolTest[mySamp] <- TRUE
 testIndex <- 1:10^6

 microbenchmark(testIndex[which(boolTest)], testIndex[boolTest], unit = "relative")
 Unit: relative
                       expr      min       lq     mean  median       uq       max neval
 testIndex[which(boolTest)] 1.533577 1.350712 1.185725 1.36155 1.386107 0.1012212   100
        testIndex[boolTest] 1.000000 1.000000 1.000000 1.00000 1.000000 1.0000000   100

In order to resolve the monty vector and the second iteration of contestant, we can implement vapply along with length to avoid calls to sample where the length is 1.

monty <- vapply(1:numReps, function(x) {
                mySet <- intDoors[intDoors != car[x] & intDoors != contestant[x]]
                if (length(mySet) > 1) sample(mySet, 1) else mySet}, 1L)

And finally, instead of incrementing win, we can get it all at once using sum(contestant == car).  This takes advantage of vectorization and the internal coercion from logicals to integers. Putting it all together, we get:

funImproved <- function(numReps) {
    car <- sample(intDoors, numReps, replace = TRUE)
    contestant <- sample(intDoors, numReps, replace = TRUE)

    monty <- vapply(1:numReps, function(x) {
                    mySet <- intDoors[intDoors != car[x] & intDoors != contestant[x]]
                    if (length(mySet) > 1) sample(mySet, 1) else mySet}, 1L)

    contestant <- vapply(1:numReps, function(x) {
                        mySet <- intDoors[intDoors != monty[x] & intDoors != contestant[x]]
                        if (length(mySet) > 1) sample(mySet, 1) else mySet}, 1L)

    wins <- sum(contestant == car)
    (wins/numReps)
}

The OP's function is:

funOP <- function(numReps) {
    wins <- 0

    for (i in 1:numReps) {
        # Choose which door the car is behind
        car <- sample(doors)[1]

        # The contestant chooses a door
        contestant <- sample(doors)[1]

        # Monty opens a door that the car is not behind
        monty <- sample(doors[which((doors != car) & (doors != contestant))])[1]

        # Force the contestant to switch
        contestant <- sample(doors[which((doors != monty) & (doors != contestant))])[1]

        # Count the wins
        if(contestant == car){wins <- wins+1}
    }

    (wins/numReps)
}

And the final comparison sees an improvement of about 4.5x.

microbenchmark(funOP(n), funImproved(n),
               times = 50, unit = "relative")
Unit: relative
          expr      min       lq    mean   median       uq      max neval
      funOP(n) 5.060335 4.518107 4.54938 4.539024 4.183145 7.831481    50
funImproved(n) 1.000000 1.000000 1.00000 1.000000 1.000000 1.000000    50

The results are very similar as well:

## the average difference over 100 trials when n = 10^5
mean(replicate(100, funOP(10^5) - funImproved(10^5)))
[1] -0.0001978

As a final note, when one is performing any sort of procedure that calls for verification, it is a good idea to use set.seed so you can predictably verify your results. For example:

sapply(1:100, function(x) {
        set.seed(42)
        funImproved(10^3)
    })
 [1] 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668
[16] 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668
[31] 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668
[46] 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668
[61] 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668
[76] 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668
[91] 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668 0.668


Sudoku Puzzle Generator I've written a Sudoku puzzle generator. It currently runs through each line of the 9x9 grid and places numbers randomly if they're valid. It loops over all the numbers from 1-9 and then if it finds itself trapped in a corner with no valid answers it throws out the whole board and restarts.

I tested it by making 1000 puzzles and it took just under 100 seconds, so a single puzzle takes 0.1. In practical terms that's almost negligible but still seems wasteful to ditch the processing up to that point (as it takes, on average, hundreds of attempts to find a valid puzzle). I'm maybe just being impractical to want a more intelligent solution so I thought I'd ask how it looks to people on here and if anyone has suggestions on improving it.

import random
numbers = [1,2,3,4,5,6,7,8,9]

def makeBoard():
    board = None
    while board is None:
        board = attemptBoard()
    return board

def attemptBoard():
    board = [[None for _ in range(9)] for _ in range(9)]
    for i in range(9):
        for j in range(9):
            checking = numbers[:]
            random.shuffle(checking)
            x = -1
            loopStart = 0
            while board[i][j] is None:
                x += 1
                if x == 9:
                    #No number is valid in this cell, start over
                    return None
                checkMe = [checking[x],True]
                if checkMe in board[i]:
                    #If it's already in this row
                    continue
                checkis = False
                for checkRow in board:
                    if checkRow[j] == checkMe:
                        #If it's already in this column
                        checkis = True
                if checkis: continue
                #Check if the number is elsewhere in this 3x3 grid based on where this is in the 3x3 grid
                if i % 3 == 1:
                    if   j % 3 == 0 and checkMe in (board[i-1][j+1],board[i-1][j+2]): continue
                    elif j % 3 == 1 and checkMe in (board[i-1][j-1],board[i-1][j+1]): continue
                    elif j % 3 == 2 and checkMe in (board[i-1][j-1],board[i-1][j-2]): continue
                elif i % 3 == 2:
                    if   j % 3 == 0 and checkMe in (board[i-1][j+1],board[i-1][j+2],board[i-2][j+1],board[i-2][j+2]): continue
                    elif j % 3 == 1 and checkMe in (board[i-1][j-1],board[i-1][j+1],board[i-2][j-1],board[i-2][j+1]): continue
                    elif j % 3 == 2 and checkMe in (board[i-1][j-1],board[i-1][j-2],board[i-2][j-1],board[i-2][j-2]): continue
                #If we've reached here, the number is valid.
                board[i][j] = checkMe
    return board

def printBoard(board):
    spacer = "++---+---+---++---+---+---++---+---+---++"
    print (spacer.replace('-','='))
    for i,line in enumerate(board):
        print ("|| {} | {} | {} || {} | {} | {} || {} | {} | {} ||".format(
                    line[0][0] if line[0][1] else ' ',
                    line[1][0] if line[1][1] else ' ',
                    line[2][0] if line[2][1] else ' ',
                    line[3][0] if line[3][1] else ' ',
                    line[4][0] if line[4][1] else ' ',
                    line[5][0] if line[5][1] else ' ',
                    line[6][0] if line[6][1] else ' ',
                    line[7][0] if line[7][1] else ' ',
                    line[8][0] if line[8][1] else ' ',))
        if (i+1) % 3 == 0: print(spacer.replace('-','='))
        else: print(spacer)

1. Review

There are no docstrings. What do these functions do?
The Python style guide says, "limit all lines to a maximum of 79 characters." If the code followed this recommendation, then we wouldn't have to scroll it horizontally to read it here.
The board is not represented consistently. Looking at printBoard, it seems that each cell is represented by a list [a, b] where b is False if the cell is empty, and True if it contains the number a. But the initialization of the board in attemptBoard looks like this:

board = [[None for _ in range(9)] for _ in range(9)]

which represents empty cells as None, so that if I try to print this board, I get:

TypeError: 'NoneType' object is not subscriptable

I would recommend using a consistent board representation. In this case I think it makes more sense to use None for an empty cell and a number for a full cell (rather than a list). That's because (i) None and small numbers don't need any memory allocation, whereas a list needs to be allocated; (ii) testing a None or a number is quicker than testing a list.
In printBoard you have very repetitive code:

print ("|| {} | {} | {} || {} | {} | {} || {} | {} | {} ||".format(
            line[0][0] if line[0][1] else ' ',
            line[1][0] if line[1][1] else ' ',
            line[2][0] if line[2][1] else ' ',
            line[3][0] if line[3][1] else ' ',
            line[4][0] if line[4][1] else ' ',
            line[5][0] if line[5][1] else ' ',
            line[6][0] if line[6][1] else ' ',
            line[7][0] if line[7][1] else ' ',
            line[8][0] if line[8][1] else ' ',))

This can be rewritten using a loop:

print("|| {} | {} | {} || {} | {} | {} || {} | {} | {} ||"
      .format(*(number if full else ' ' for number, full in line)))

or, after simplifying the board representation as recommended above:

print("|| {} | {} | {} || {} | {} | {} || {} | {} | {} ||"
      .format(*(cell or ' ' for cell in line)))

The nested loops:

for i in range(9):
    for j in range(9):

can be combined into one using itertools.product:

for i, j in itertools.product(range(9), repeat=2):

The variable loopStart is never used.
Instead of this complex while loop:

x = -1
while board[i][j] is None:
    x += 1
    if x == 9:
        #No number is valid in this cell, start over
        return None
    checkMe = [checking[x],True]

    # ... loop body here ...

    #If we've reached here, the number is valid.
    board[i][j] = checkMe

write a for loop with an else:

for x in checking:
    # ... loop body here ...

    # If we've reached here, the number is valid.
    board[i][j] = x
    break
else:
    # No number is valid in this cell, start over.
    return None

The column check:

checkis = False
for checkRow in board:
    if checkRow[j] == checkMe:
        #If it's already in this column
        checkis = True
if checkis: continue

can be simplified using the built-in function any:

if any(row[j] == checkMe for row in board): continue

The code for checking against other cells in the 3×3 block is very repetitive:

if i % 3 == 1:
    if   j % 3 == 0 and checkMe in (board[i-1][j+1],board[i-1][j+2]): continue
    elif j % 3 == 1 and checkMe in (board[i-1][j-1],board[i-1][j+1]): continue
    elif j % 3 == 2 and checkMe in (board[i-1][j-1],board[i-1][j-2]): continue
elif i % 3 == 2:
    if   j % 3 == 0 and checkMe in (board[i-1][j+1],board[i-1][j+2],board[i-2][j+1],board[i-2][j+2]): continue
    elif j % 3 == 1 and checkMe in (board[i-1][j-1],board[i-1][j+1],board[i-2][j-1],board[i-2][j+1]): continue
    elif j % 3 == 2 and checkMe in (board[i-1][j-1],board[i-1][j-2],board[i-2][j-1],board[i-2][j-2]): continue

The reason you go to this trouble is to avoid testing against board[i-1][j] and board[i-2][j], which you know would be useless, because you already tested these cells when you checked the column. But in fact that's a false economy. You avoid an unnecessary test, but at the cost of a lot of extra code. It turns out to be just as fast, but a lot simpler, to test all the entries in previous rows of the block, like this:

i0, j0 = i - i % 3, j - j % 3 # origin of 3x3 block
if any(x in row[j0:j0+3] for row in board[i0:i]):
    continue

The code only works for 9×9 Sudoku grids made up of 3×3 blocks. But there's nothing special about the numbers 3 and 9 here: the algorithm would be essentially the same for 2 and 4, or 4 and 16. So why not make the code general?

2. Revised code

This isn't any faster than the original code, but it's a lot shorter and simpler, which makes it a better place to start when speeding it up:

import itertools
import random

def attempt_board(m=3):
    """Make one attempt to generate a filled m**2 x m**2 Sudoku board,
    returning the board if successful, or None if not.

    """
    n = m**2
    numbers = list(range(1, n + 1))
    board = [[None for _ in range(n)] for _ in range(n)]
    for i, j in itertools.product(range(n), repeat=2):
        i0, j0 = i - i % m, j - j % m # origin of mxm block
        random.shuffle(numbers)
        for x in numbers:
            if (x not in board[i]                     # row
                and all(row[j] != x for row in board) # column
                and all(x not in row[j0:j0+m]         # block
                        for row in board[i0:i])):
                board[i][j] = x
                break
        else:
            # No number is valid in this cell.
            return None
    return board

3. Backtracking

If attempt_board finds that there are no valid numbers for some cell, then it throws away all its work and starts all over again from the beginning. But all that work is not necessarily invalid: most likely the mistake was made only in the last few steps, and so if the algorithm were to go back a little bit and try some different choices, then it would find a solution. This approach is known as backtracking.

Backtracking is easily implemented by using recursion:

def make_board(m=3):
    """Return a random filled m**2 x m**2 Sudoku board."""
    n = m**2
    board = [[None for _ in range(n)] for _ in range(n)]

    def search(c=0):
        "Recursively search for a solution starting at position c."
        i, j = divmod(c, n)
        i0, j0 = i - i % m, j - j % m # Origin of mxm block
        numbers = list(range(1, n + 1))
        random.shuffle(numbers)
        for x in numbers:
            if (x not in board[i]                     # row
                and all(row[j] != x for row in board) # column
                and all(x not in row[j0:j0+m]         # block
                        for row in board[i0:i])):
                board[i][j] = x
                if c + 1 >= n**2 or search(c + 1):
                    return board
        else:
            # No number is valid in this cell: backtrack and try again.
            board[i][j] = None
            return None

    return search()

I find that this is about 60 times faster than the original code.

4. Algorithm X

There's a reformulation of Sudoku in terms of the "exact cover" problem, and this can be solved using Donald Knuth's "Algorithm X". See this blog post of mine for a detailed explaination of the how this algorithm can be used to solve Sudoku, and see this post on Code Review for an implementation in Python.


Winforms tools for selection of items using ComboBox - binding datasource This is mi UserControl containing ComboBox and few buttons for selecting and manipulating collection of items which implement simple interface IHasHeader (just header with name and ID).

It works now but I feel that I use some not ideal hacks, especially in the area of binding of ComboBox's datasource. I'll be glad for any review notes.

public partial class TemplateSelector : UserControl
{
    /// <summary>
    /// Type of access right
    /// </summary>
    public enum TemplateType
    {
        Private,
        Public
    }

    private BindingSource bindSrc = new BindingSource();
    private IHasHeader SelectedItem;
    private bool doHandleSelection = true;

    // delegates

    public delegate void ItemSelectedHandler(IHasHeader item);
    public delegate void NewTemplateSelectionHandler(string name, bool isPublic);
    public delegate void NameSelectionHandler(string name);

    // events

    public event ItemSelectedHandler TemplateSelectionChanged = delegate { };
    public event NameSelectionHandler TemplateDeleting = delegate { };
    public event NewTemplateSelectionHandler CreatingNewTemplate = delegate { };
    public event Action RefreshingTemplatesList = delegate { };
    public event Action ClearingTemplate = delegate { };

    public TemplateType SelectedTemplateType { get; set; }

    public IEnumerable<IHasHeader> TemplatesDatasource
    {
        set
        {
            doHandleSelection = false;
            bindSrc.DataSource = value;
            cboxTemplates.DataSource = bindSrc;
            cboxTemplates.SelectedIndex = -1;
            doHandleSelection = true;
        }
    }

    public TemplateSelector()
    {
        InitializeComponent();
        cboxTemplates.SelectedIndexChanged += cboxTemplates_SelectedIndexChanged;
    }

    /// <summary>
    /// Select given item
    /// </summary>
    /// <param name="item"></param>
    public void SelectItem(IHasHeader item)
    {
        for (int i = 0; i < cboxTemplates.Items.Count; ++i)
        {
            if ((cboxTemplates.Items[i] as IHasHeader).Header.ID == item.Header.ID)
            {
                SelectedItem = item;
                cboxTemplates.SelectedIndex = i;
                return;
            }
        }
    }

    /// <summary>
    /// Clear selection of combobox
    /// </summary>
    public void ClearSelection()
    {
        cboxTemplates.SelectedIndex = -1;
        SelectedItem = null;
    }

    /// <summary>
    /// Add item to combobox datasource
    /// </summary>
    /// <param name="item"></param>
    public void AddItem(IHasHeader item)
    {
        var newSource = (bindSrc.DataSource as IEnumerable<IHasHeader>).ToList();
        newSource.Add(item);
        TemplatesDatasource = newSource;
    }

    private void btnDelete_Click(object sender, EventArgs e)
    {
        if (cboxTemplates.SelectedItem is IHasHeader)
        {
            IHasHeader selected = cboxTemplates.SelectedItem as IHasHeader;
            TemplateDeleting(selected.Header.Name);
            bindSrc.ResetBindings(false);
        }
        ClearSelection();
        ClearingTemplate();
    }

    private void btnNew_Click(object sender, EventArgs e)
    {
        CreatingNewTemplate(TextBoxNewTemplateNew.Text, RadioButtonMeasureQuantitySaveSettingsPublic.Checked);
        bindSrc.ResetBindings(false);
    }

    private void cboxTemplates_SelectedIndexChanged(object sender, EventArgs e)
    {
        if (cboxTemplates.SelectedItem is IHasHeader && doHandleSelection)
        {
            IHasHeader selected = cboxTemplates.SelectedItem as IHasHeader;
            TemplateSelectionChanged(selected);
        }
    }

    private void btnRefresh_Click(object sender, EventArgs e)
    {
        RefreshingTemplatesList();
        bindSrc.ResetBindings(false);
        cboxTemplates.SelectedItem = SelectedItem;
    }

    private void btnClear_Click(object sender, EventArgs e)
    {
        ClearSelection();
        ClearingTemplate();
    }
}

Style & Conventions

Enums and delegates shouldn't be nested in a class.
The default event delegate is the EventHandler. If you want to create a custom event handler then its signature should be

delegate void ItemSelectedHandler(Object sender, EventArgs e);

but if you need custom event args then you should create a new class derived from EventArgs and with the properties that you need:

class SelectEventArgs : EventArgs
{
    ..ctor
    public IHasHeader item { get; }
}

then you can use the generic event-handler:

public event EventHandler<SelectEventArgs> TemplateSelectionChanged = delegate { };

You shouldn't use any other pattern for events like

public event Action RefreshingTemplatesList = delegate { };

If you want to follow the event style and conventions then stick to the EventHandler and event args.

Binding

public IEnumerable<IHasHeader> TemplatesDatasource
{
  set
  {
      doHandleSelection = false;
      bindSrc.DataSource = value;
      cboxTemplates.DataSource = bindSrc;
      cboxTemplates.SelectedIndex = -1;
      doHandleSelection = true;
  }
}

here you could write

 bindSrc.DataSource = value.ToList();

instead

bindSrc.DataSource = value;

so that you can later reuse this list for adding more items:

public void AddItem(IHasHeader item)
{
    var headers = bindSrc.DataSource as List<IHasHeader>;
    headers.Add(item);
    bindSrc.ResetBindings(false);
}

Don't abbreviate the names like bindSrc. Use full names and preferably meaninigful ones like headerSource or headerBindingSource. bindSrc doesn't say anything about what the binding source holds.


NodeJS Promises VS async library I am currently working on a NodeJS server, and am switching from callbacks to promises.

Here's an example of my code:

 var record = req.body.record;
var schoolId = req.body.schoolId;
var promise = BusController.addBus(record,schoolId);
promise.then(function(){
    return res.json({status:"success", message : "bus successfully created"});
}).catch(function(error){
        if(error){
            console.log(error);
            res.json({status:"error",message:"error while creating bus"});
            return;
        }

The bus controller has the following in it:

addBus : function(record,schoold){
    return new Promise(function(resolve,reject){
        var bus_number = record.bus_number;

        var newBus = new Bus({
            language: language ? language : "",
            bus_number: bus_number  ? bus_number : "",
            bus_driver: bus_driver  ? bus_driver : {},
            bus_details: bus_details ,
            trips: trips ? trips : [],
            school_id: schoold,
            location: location ? location : {}
        });
        newBus.save().then(function(bus){
            var busCreation = {};
            busCreation.name=  "باص رقم"+ ' ' +bus_number;
            busCreation.type= "3";
            busCreation.refid= bus._id;
            busCreation.email= record.email;
            busCreation.phone_number=record.phone_number;
            busCreation.school_id = schoold;
            console.log(busCreation);
            var promise = UserController.addUser(busCreation);
            promise.then(function(){
                resolve();
            })
        }).catch(function(error){
            if (error) {
                reject(error);
            }
        });
    });
}

This my current logic. Is this a good practice? Or should I switch to async library?

That bus controller is too complicated; in general you should not be using new Promise(fn) unless you are wrapping a callback, and what you're doing is better done by returning a promise from the .then(). (When you return a value from myPromise.then() it gets passed into Promise.resolve(p), which if p is not a promise creates a new one which immediately resolves with the value you created; or if p is a promise, just returns that promise.)

In addition I wouldn't mix camelCase and underscore_names; it leads to bigger confusions across the codebase. In addition the form a ? a : b is equivalent to the shorter a || b. I also would in general add a debugging string to any console.log so that while you're in development you can better drill down to exactly what you're looking for.

With those changes the bus controller simplifies to just:

addBus: function(record, schoold) {
    return new Bus({
        language: language || "",
        bus_number: record.bus_number || "",
        bus_driver: bus_driver  || {},
        bus_details: bus_details,
        trips: trips || [],
        school_id: schoold,
        location: location || {}
    }).save().then(function (bus) {
        var bus_creation_user = {
            name: "باص رقم" + ' ' + record.bus_number,
            type: "3",
            refid: bus._id,
            email: record.email,
            phone_number: record.phone_number,
            school_id: schoold
        };
        console.log("bus_creation_user", bus_creation_user);
        return UserController.addUser(bus_creation_user);
    });
});

The code for using it seems mostly fine but has some code smells. For example what is the if doing in your .catch(err => { if (err) { ... } })? Are you legitimately throwing non-Error objects like null and undefined and false? (In JS you can do this but it is widely considered to be a bad idea.) Instead you can just write,

BusController.addBus(record, schoolId).then(function(){
    res.json({status:"success", message: "bus successfully created"});
}).catch(function(error){
    console.log(error);
    res.json({status:"error", message: "error while creating bus"});
});


Checking a collection of Ints I'm learning a little Scala by writing a little card game. What I want to do here is check that the Traversable[Team] supplied has the same number of team members for each team.

How can I clean this up?

val teamSizes = teams.map(_.members.size)
require(teamSizes.foldLeft((true, teamSizes.head)) {
  (tuple, lastSize) =>
    val (b, size) = tuple
    (b && size == lastSize, lastSize)
}._1)

An alternative approach is:

require (teamSizes.min == teamSizes.max)

but ỳour forall-solution expresses better the idea, that all members share the same size.

And without measuring it, or trying to investigate my assumption, I guess that my approach traverses the collection twice, which could be a drawback for bigger collections or if performed million of times. So less so in this example.

But maybe it is possible to model the constraint into your design, so that all teams get the right size from the beginning?


Maintaining the Single Responsibility Principle with Project Euler #2 This is my solution to Project Euler problem 2:

  By considering the terms in the Fibonacci sequence whose values do not exceed four million, find the sum of the even-valued terms.

The class fibonacci_Even_Sum_Calculator does two things:

calculate the sum of the even-valued Fibonacci numbers
cache the results in a dictionary

Is this a sign that this code violates the Single Responsibility Principle?

class fibonacci_Even_Sum_Calculator:

        def __init__(self):
                self.cached_result = dict()

        def F(self, n):

                if n == 0:
                        return 0
                elif n == 1:
                        return 1
                else:
                        if(self.cached_result.has_key(n)):
                                return self.cached_result[n]
                        else:
                                result = self.F(n-1) + self.F(n-2)
                                self.cached_result[n] = result
                                return result

        def is_even(self, number):
                return number % 2 == 0

        def get_result(self,limit_value):

                sequence = 2
                sum = 0

                while True:

                        result = self.F(sequence)

                        if result > limit_value:
                                break

                        if self.is_even(result) :
                                sum = sum + result

                        sequence = sequence + 1

                return sum

fibonacci_calculator = fibonacci_Even_Sum_Calculator()
result = fibonacci_calculator.get_result(4000000)
print result

I think you are violating SRP, but not for the reason you think you are. Memoization is fine, it's definitely one approach to calculating the fibonacci values, but you have one class that is doing BOTH calculating the Fibonacci values AND summing the even ones.

If you are really worried about SRP, I would have a generator to calculate the Fibonacci numbers, and then an external function to sum them:

def sum_evens(limit):
    sum = 0
    for f in fibonacci_numbers(): # left as an exercise
        if f > limit:
            break
        elif f % 2 == 0:
            sum += f
    return sum

Otherwise, minor notes, your class is named fibonacci_Even_Sum_Calculator. The most accepted convention for class names is CamelCase, so FibonacciEvenSumCalculator is strongly preferred. Similarly, function names should be lowercase, and f is an insufficient name for code that is "get the nth Fibonacci number".


Checking if two floating point numbers are equal Is this the best way to check if two floating point numbers are equal, or close to being equal?

template <class T>
bool IsEqual(T rhs, T lhs)
{
    T diff = std::abs(lhs - rhs);

    T epsilon = std::numeric_limits<T>::epsilon( ) * std::max(std::abs(rhs), std::abs(lhs));

    return diff <= epsilon ;
}

Seems unlikely.  epsilon from std::numeric_limits is the smallest increment representable by the type (around the value 1).

You want to check for something "close to" equal, but you don't say what close to means for you. Assuming it to be a few multiples of epsilon, the following check would seem reasonable:

const int FEW = 10;
T diff = std::abs(lhs - rhs);
T epsilon = std::numeric_limits<T>::epsilon();

return diff < (epsilon * FEW);


Writing FizzBuzz like this makes me feel like a small child For the sake of full disclosure, this was written with bananas, not apples.  The doctor may not be kept away.
With AppleScript highlighting:

For Copy&Pasting:
activate application "TextEdit"
tell application "System Events"
    repeat until first window of application "TextEdit" exists
        tell application "TextEdit" to make new document at the front
        delay 1.0E-3
    end repeat
    repeat until process "TextEdit" is frontmost
        set frontmost of process "TextEdit" to true
        delay 1.0E-3
    end repeat
    set focused of first window of process "TextEdit" to true

    repeat with i from 1 to 100
        if i mod 15 is 0 then
            keystroke "FizzBuzz"
            keystroke return
        else if i mod 3 is 0 then
            keystroke "Fizz"
            keystroke return
        else if i mod 5 is 0 then
            keystroke "Buzz"
            keystroke return
        else
            keystroke i
            keystroke return
        end if
    end repeat
end tell

I borrowed the logic for appropriately waiting for "TextEdit" to be ready to receive text from this excellent Code Review answer.
I feel like I want to write functions... but I don't know what's available in AppleScript for modularizing code... err script... I'm just getting my toes wet here.

It may be fun watching it type out FizzBuzz, but interacting with the UI directly has its pitfalls. For example, if the user focuses a different application while your script is running, you will now be typing in the wrong place; and if you refocus the application, that could be annoying.

Fortunately, there is a better way. Rather than activating the application, waiting until a window appears, and making the window frontmost, you could just…

tell application "TextEdit" to set text of first document to "Hello, world!"

This is much less problematic. The only issue is if TextEdit is already open, in which case it will overwrite the existing document. In that case, you could use

tell application "TextEdit" to set text of (make new document) to "Hello, world!"

Either way, you’re not interacting directly with the UI any more, which eliminates a lot of potential problems.

The actual meat of the FizzBuzz implementation (barring the use of keystroke) looks fine, although you might consider something like keystroke "FizzBuzz" & return rather than keystroke "FizzBuzz" and separately keystroke return.


Better way to write generic method to convert List> to T[][] I was working through getting myself past the creation of a generic method, that converts a List<T> or List<List<T>>, to T[] or T[][] respectively. Well, let's just consider a maximum dimension of 2 for now.

List<T> to T[] is quite straight-forward:

public static <T> T[] listToArray(List<T> list, Class<T> clazz) {

    /**** Null Checks removed for brevity ****/

    T[] arr = (T[]) Array.newInstance(clazz, list.size());

    for (int i = 0; i < list.size(); ++i) {
        arr[i] = list.get(i);
    }
    return arr;
}

Now I can call this method as:

List<String> list = new ArrayList<>(Arrays.asList("rohit", "jain"));
String[] arr = listToArray(list, String.class);

List<List<T>> to T[][] is what gives problems:

Now, here's the problem. Consider the code:

public static <T> T[][] multiListToArray(List<List<T>> listOfList, Class<T> clazz) {

    // Here I don't know what size to give for 2nd dimension
 // T[][] arr = (T[][]) Array.newInstance(clazz, listOfList.size(), ?);

    // So, I tried this. But this of course is not type safe
 // T[][] arr = (T[][]) new Object[listOfList.size()][];

    /* Only alternative I had was to iterate over the listOfList to get the
       maximum out of all the column sizes, which I can give as 2nd
       dimension later on.
    */
    int maxCol = 0;

    for (List<T> row: listOfList) {
        if (row.size() > maxCol) {
            maxCol = row.size();
        }
    }

    // Now I can pass `maxCol` as 2nd dimension
    T[][] arr = (T[][]) Array.newInstance(clazz, listOfList.size(), maxCol);

    for (int i = 0; i < listOfList.size(); ++i) {
        List<T> row = listOfList.get(i);
        arr[i] = listOfList.get(i).toArray((T[])Array.newInstance(clazz, row.size()));
    }

    return arr;
}

Now, this is what I want to avoid - double iteration of List<List<T>> to be able to create the array. Is there any possibility?

Since you are creating new instance of arrays in your second loop, you don't need to know the size of the "inner list". Your "outer array" has to be as big as the "outer list". In a second phase, you instanciate the "inner arrays".

There's a simple way do this:

public <T> T[][] multiListToArray(final List<List<T>> listOfList, final Class<T> classz) {
    final T[][] array = (T[][]) Array.newInstance(classz, listOfList.size(), 0);

    for (int i = 0; i < listOfList.size(); i++) {
        array[i] = listOfList.get(i).toArray((T[]) Array.newInstance(classz, listOfList.get(i).size()));
    }

    return array;
}

More over, as you can see in the code above, you can provide a 0-length array to toArray(T[]) method.

This piece of code:

final List<List<String>> test = new ArrayList<List<String>>();
test.add(new ArrayList<String>(Arrays.asList("a", "b", "c")));
test.add(new ArrayList<String>(Arrays.asList("d", "e", "f", "g")));
test.add(new ArrayList<String>(Arrays.asList("h", "i")));

for (final String[] innerArray : multiListToArray(test, String.class)) {
    System.out.println(Arrays.toString(innerArray));
}

Will produce this output:

  [a, b, c]
  [d, e, f, g]
  [h, i]

EDIT: There is an Ideone example.

EDIT 2: Code and Ideone updated to comply with OP needs.