Password strength checker I would like to receive feedback about my code.  Is there any better way to shorten the code or is it fine?

Demo

var pass_strength;
function IsEnoughLength(str,length){
    if ((str == null) || isNaN(length))
        return false;
    else if (str.length < length)
        return false;
    return true;
}
function HasMixedCase(passwd){
    if(passwd.match(/([a-z].*[A-Z])|([A-Z].*[a-z])/))
        if ( passwd.split(/[a-z]/).length >= 3 && passwd.split(/[A-Z]/).length >= 3)
            return true;
        else
            return false;
    else
        return false;
}
function HasNumeral(passwd){
    if(passwd.match(/[0-9]/))
        return true;
    else
        return false;
}
function HasSpecialChars(passwd){
    if(passwd.match(/.[!,@,#,$,%,^,&,*,?,_,~]/))
        return true;
    else
        return false;
}
function CheckPasswordStrength(pwd)
{
    if (IsEnoughLength(pwd,14) && HasMixedCase(pwd) && HasNumeral(pwd) && HasSpecialChars(pwd))
        pass_strength = "Very strong";
    else if (IsEnoughLength(pwd,10) && HasMixedCase(pwd) && HasNumeral(pwd) && HasSpecialChars(pwd))
        pass_strength = "Strong";
    else if (IsEnoughLength(pwd,10) && HasMixedCase(pwd) && HasNumeral(pwd))
        pass_strength = "Medium";
    else
        pass_strength = "Weak";
    document.getElementById('pwd_strength').innerHTML = "Password strength: " + pass_strength;
}

You can replace all of the

if(x.match(...))
    return true;
else
    return false;

with:

return x.match(...) !== null;

or if you're content with returning a truthy/falsy value instead of proper booleans you can even simplify it to:

return x.match(...)

IsEnoughLength is ungrammatical. I'd use IsLongEnough
You're mixing the password strength estimation with presentation logic. You should have one function which returns a value indicating the strength of the password and another which manipulates the DOM.
Use CSS classes to format the text instead of <b> and <font style=
Personally I wouldn't bother with argument checking in such a simple internal function. But if you do, throw an exception on programmer error, don't just return false.

if ((str == null) || isNaN(length))
    return false;

should be something like

if (str == null)
    throw new Error("str must not be null");
if(isNaN(length))
    throw new Error("length must be a number");

(or a fancier exception class if you want)


Applying a 'mean filter' I am just trying to apply a "mean filter" with C#. The working code below gets window size (3,5,7,9,...) from textBoxWinSize and applies the mean filter.

I am new and I want your comments and suggestions about my code so I can improve myself.

Can I make it faster? How? Is there any unnecessary or absurd part in the code? Can it be improved in any other way?

I do not care about border pixels for now.

private void buttonApplyFilter_Click(object sender, EventArgs e)
{
    int R, G, B,aveR,aveG,aveB;
    double sumR,sumG,sumB;
    Color pixel_value;
    Bitmap bmp = new Bitmap(bmpOrig);
    int WinSize = Int32.Parse(textBoxWinSize.Text);
    int col,row,n,m;
    for (col = (WinSize-1); col < (bmp.Width-WinSize); col++)
        for (row = (WinSize-1); row < (bmp.Height-WinSize); row++)
        {
            sumB = 0;
            sumG = 0;
            sumR = 0;
            for (m=-(WinSize-1)/2; m<=(WinSize-1)/2;m++)
                for (n=-(WinSize-1)/2;n<=(WinSize-1)/2;n++)
                {
                    pixel_value = bmp.GetPixel(col+m, row+n);
                    R = (pixel_value.R);
                    G =(pixel_value.G);
                    B = (pixel_value.B);
                    sumR = sumR + R;
                    sumG = sumG + G;
                    sumB = sumB + B;
                }
            aveR = (int)(sumR / (WinSize * WinSize));
            aveG = (int)( sumG / (WinSize * WinSize));
            aveB = (int)( sumB / (WinSize * WinSize));
            bmp.SetPixel(col, row, Color.FromArgb(aveR, aveG, aveB));
        }
    pictureBox1.Image = bmp;

}

First up, I think you're doing too much in the button click handler. The logic for applying the mean filter should be moved to its own method, something like

private static Bitmap ApplyMeanFilter(Bitmap input, int windowSize)

Now it can be re-used and unit tested more easily.

I think this line is wrong

pixel_value = bmp.GetPixel(col+m, row+n)

in that bmp should be bmpOrig. The reasoning is that you're changing pixels in bmp as you go, which will have an effect on pixels that are processed later. That is, they won't be assigned the mean of their original surrounding pixels.

Assuming the bmp should be bmpOrig, we can optimise this quite a bit.

Say that instead of a bitmap, we have a 2D array of ints.

input

\begin{array}{c | c c c c}
  & 0 & 1 & 2 & 3 \\
\hline
0 & 1 & 2 & 3 & -1 \\
1 & 4 & 1 & 6 & -3 \\
2 & 4 & 5 & 9 & -3 \\
3 & 4 & -2 & 6 & -3
\end{array}

We can compute another 2D array such that the \$(i, j)\$-th entry is the sum of entries in the submatrix \$(0, 0), (i, j)\$:

var sums = new int[height, width];
for (var i = 0; i < height; i++)
{
    var sum = 0;
    for (var j = 0; j < width; j++)
    {
        sum += input[i, j];
        sums[i, j] = i > 0 ? sums[i - 1, j] + sum : sum;
    }
}

sums

\begin{array}{c | c c c c}
  & 0 & 1 & 2 & 3 \\
\hline
0 & 1 & 3 & 6 & 5 \\
1 & 5 & 8 & 17 & 13 \\
2 & 9 & 17 & 35 & 28 \\
3 & 13 & 19 & 43 & 33
\end{array}

Now suppose we want to find the sum of the values in the 3x3 square around entry \$(i, j) = (2, 2)\$:

\begin{array}{c | c c c c}
  & 0 & 1 & 2 & 3 \\
\hline
0 & 1 & 2 & 3 & -1 \\
1 & 4 & 1 & 6 & -3 \\
2 & 4 & 5 & \textbf{9} & -3 \\
3 & 4 & -2 & 6 & -3
\end{array}

We can do this quickly using the matrix we just computed. The answer is given by

  sums[3, 3] - sums[3, 0] - sums[0, 3] + sums[0, 0]
= 33 - 13 - 5 + 1
= 16

In general, if halfWindow = (windowSize - 1) / 2 then the sum of values in the windowSize * windowSize square around entry \$(i, j)\$ is given by

sums[i + halfWindow,     j + halfWindow] -
sums[i - halfWindow - 1, j + halfWindow] -
sums[i + halfWindow,     j - halfWindow - 1] +
sums[i - halfWindow - 1, j - halfWindow - 1]

The beauty is, computing the sum of the surrounding windowSize * windowSize values this way is independent of windowSize.

Getting back to the original problem of bitmaps, let's suppose we have a struct that will let us sum up the RGB values of the pixels. I named it Clr because I'm not very imaginative today; I'm sure you can think of a better name. Let's also assume we have operators +, - and / defined in the obvious way.

Putting it all together would look something like this

var sums = new Clr[input.Height, input.Width];
for (var i = 0; i < input.Height; i++)
{
    var sum = new Clr();
    for (var j = 0; j < input.Width; j++)
    {
        sum += Clr.FromColor(input.GetPixel(j, i));
        sums[i, j] = i > 0 ? sums[i - 1, j] + sum : sum;
    }
}

var output = new Bitmap(input);
var windowArea = windowSize * windowSize;
var halfWindow = (windowSize - 1) / 2;
for (var i = windowSize - 1; i < input.Height - windowSize; i++)
{
    for (var j = windowSize - 1; j < input.Width - windowSize; j++)
    {
        var clr = sums[i + halfWindow,     j + halfWindow] -
                  sums[i - halfWindow - 1, j + halfWindow] -
                  sums[i + halfWindow,     j - halfWindow - 1] +
                  sums[i - halfWindow - 1, j - halfWindow - 1];

        output.SetPixel(j, i, (clr / windowArea).ToColor());
    }
}

return output;

Running this on a 256x256 image, I got the following timings

windowSize       this               original
 3          00:00:00.0910750    00:00:00.4720078
 5          00:00:00.0899366    00:00:01.2674758
 7          00:00:00.0853111    00:00:02.2569419
 9          00:00:00.0938911    00:00:03.4408123
11          00:00:00.0840854    00:00:04.9614386

Other things to try that might improve performance:

Use a Clr[][] instead of a Clr[,]. See Why are multi-dimensional
arrays in .NET slower than normal
arrays?
Doing the above will let you move sums[i + halfWindow] and sums[i - halfWindow - 1] into variables outside the inner loop.
Compute the first row of sums separately to remove the conditional in this line sums[i, j] = i > 0 ? sums[i - 1, j] + sum : sum;


Project Euler # 22: Names scores Problem 22, here, asks the following:

  Using names.txt (right click and 'Save Link/Target As...'), a 46K text file containing over five-thousand first names, begin by sorting it into alphabetical order. Then working out the alphabetical value for each name, multiply this value by its alphabetical position in the list to obtain a name score.

  For example, when the list is sorted into alphabetical order, COLIN, which is worth 3 + 15 + 12 + 9 + 14 = 53, is the 938th name in the list. So, COLIN would obtain a score of 938 × 53 = 49714.

  What is the total of all the name scores in the file?

My solution is as follows:

import re
import string

def create_names_list(file_name):
    #create a names list parsing
    names = open(file_name).read()
    names = re.findall(r'"(.*?)"', names)
    names.sort()
    return names

def name_weights(file_name):
    #fetch name-list
    names = create_names_list(file_name)

    #create a letters dictionary e.g. {'A' : 1, 'B' : 2, ... 'Z' : 26}
    letters = string.ascii_uppercase
    letters_map_reversed = dict(enumerate(letters))
    letters_map = {value: key+1 for key, value in letters_map_reversed.iteritems()}

    # sum all letters using letter score * index
    return sum(letters_map[char]*(names.index(name)+1) for name in names for char in name)

  %timeit name_weights('names.txt')

  1 loops, best of 3: 1.18 s per loop

I noticed in the thread for the solutions that many python solutions appeared to be in the 8-12ms range, 10x faster than my solution. As a result I was concerned with the performance of my solution.

Two things I've done to attempt to tweak my solution are changing the letters_map[char] portion to just be ord(char)-64 which I noticed a few people were using. It seems like a clever, certainly shorter, way of getting the number value that I hadn't thought of. It didn't alter performance though, which makes me think my problem is with the final expression that uses a nested for loop to multiply all the characters in all the words by the given weight and letter_mapping number. It seems as though other solutions made similar use of nested for loops, but I noticed that nobody had used index() in order to get the numeric index of the name in the list. I wonder if that is what is causing performance, but I suspect it may also just be my misunderstanding of how to structure these nested for loops optimally (for instance I could have used enumerate() on names in addition to on letters in order to get the index in a dictionary which would forgo the need to use index().

Aside: I've intentionally used regular expressions here instead of .strip() and .replace() in order to just get an idea for what regular expressions are and how they work (in addition to perhaps there being non-zero improvements because RE is implemented in C right?) The file parsing itself doesn't really effect performance I've noticed, but any input on what is standard python convention and when to use strip() and built-in string methods vs. python's re module is helpful there is very welcome.

General code criticism as well as specific syntax very welcome.

Here's your problem:

return sum(letters_map[char]*(names.index(name)+1) for name in names for char in name)
                              ^^^^^^^^^^^^^^^^^^^

So as we walk down names, we calculate the value of the current name... and then start over and search for it from scratch to determine the index. That extra lookup turns our algorithm from \$O(n)\$ to \$O(n^2)\$!! And string search is expensive - for the 1000th name, we have to first check 999 other strings that are definitely going to be wrong. We don't need to search for name in names to find the index. Not only that, but then we re-search the index for every char in name! (Maybe Python is smart enough to cache this, maybe not). So really this is more like \$O(n^3)\$...

We can just keep track of the index as we go. That's what enumerate() is for:

return sum(letters_map[char] * idx
           for idx, name in enumerate(names, start=1)
           for char in name)

That one change takes your runtime over 10 iterations from 13.31s to 0.07s. Behold the power of not redoing a bunch of work.

Everything else is basically fine. Creating letters_map could be abridged to just:

letters_map = {letter: idx
               for idx, letter in enumerate(string.ascii_uppercase, start=1)}

That doesn't affect performance, just makes it a little easier to see what letters_map actually is. It turns out that dictionary lookup is fater than doing the name lookup on ord() within the loop is, so this is pretty speedy.

It's also typical to do file operations with a context manager. That is:

with open(file_name) as f:
    return sorted(re.findall(r'"(.*?)"', f.read()))

That is marginally slower, but more Pythonic in my opinion.


Top 2 most common elements inside array in Python I got this interview problem from leetcode for top k frequent elements, https://leetcode.com/problems/top-k-frequent-elements/description/ But I decide to simplify the problem just most common elements to make it simpler to solve.  If the 3 item has the same frequentcy, then it's okay to return all 3.

"""Given an array, return an array of the top 2 most elements. // Input: [1, 2, 3, 4, 5, 3, 4] | Output: [3, 4] // Input: [2, 2, 3, 2, 4, 3, 4] | Output: [2,4]"""

import collections

def top2Frequent(nums):
    """
    :type nums: List[int]
    :type k: int
    :rtype: List[int]
    """
    if len(nums) < 2:
        return []
    freq = {}
    for num in nums:
        if num in freq:
            freq[num] = freq[num] + 1
        else:
            freq[num] = 1

    bucket = collections.defaultdict(list)
    for key in freq:
        f = freq[key]
        bucket[f].append(key)
    res = []
    count = len(nums)  # the upper limit for res
    while len(res) < 2:
        if bucket[count]:
            res += bucket[count]
        count -= 1
    return res

nums = [1, 4, 3, 4, 5, 3, 4]  # [4,3]
solution = top2Frequent(nums)
print(solution) # [4,3]

https://gist.github.com/Jeffchiucp/2e733e57476bd697bc430cdf48f6e180

I am also tried to solve this problem without using any python library built in collections.

If you're okay with using the built-in collections module, I'd suggest the following. collections.Counter is ideally suited for this and will make the problem trivial.

from collections import Counter

def top_k(numbers, k=2):
    """The counter.most_common([k]) method works
    in the following way:
    >>> Counter('abracadabra').most_common(3)
    [('a', 5), ('r', 2), ('b', 2)]
    """

    c = Counter(numbers)
    most_common = [key for key, val in c.most_common(k)]

    return most_common

Without using the built-in collections module, the following will work:

def top_k_nobuiltin(numbers, k=2):
    # The first part is identical to your implementation and works just
    # fine
    counter = {}
    for number in numbers:
        if number in counter:
            counter[number] += 1
        else:
            counter[number] = 1

    # The line below creates an iterator that will generate the
    # sequence [(4, 3), (3, 2), (5, 1), (1, 1)], i.e.
    # count the occurrence for each value.
    # In particular, sorted() with key=lambda kv: kv[1] will turn
    # a dictionary into a list of tuples, sorted by the second item
    # of each tuple

    sorted_by_value = reversed(sorted(counter.items(), key=lambda kv: kv[1]))

    top_vals = [item[0] for item in sorted_by_value][:k]

    return top_vals

For both cases, you'll get the desired result:

def main():
    nums = [1, 4, 3, 4, 5, 3, 4]  # [4,3]

    print(top_k(nums))  # [4,3]
    print(top_k_nobuiltin(nums))  # [4,3]

if __name__ == '__main__':
    main()


Accessing parent isolated scope in directive I was looking for a way to markup parts of my app as restricted to only certain roles, on an element basis.

This works quite well, using an attribute directive, where I specify the allowed roles and disable/hide the corresponding element; e.g.

<input require-role="admin" />

In order to make the app more maintainable I thought about inheriting the the required role attribute, e.g.

<form>

        <input required-role/>
        <input required-role/>
        <input required-role/>
        <input required-role/>
        <input />

            <input required-role/>

        <input required-role/>
        <input required-role/>
        <input required-role/>
        <input required-role="Admin"/>

    <button required-role="Admin">Save</button>
    <button required-role="User">Send to review</button>
</form>

What I came up with is the following directive, that recursively searches in the isolatedScope of the parent element until it eventually finds the requiredRole property:

var app = angular.module('plunker', []);

app.controller('MainCtrl', function($scope){
  $scope.name = 'World';
});
app.directive('requireRole', function(){
  var getRequiredRoleFromParent = function(element){
    var par = element.parent();
    if (!par) return;
    var is = par.isolateScope();
    if (is && is.requireRole) return is.requireRole;
    return getRequiredRoleFromParent(par);
  };
  return {
    restrict: 'A',
    scope   : {
      requireRole: '@'
    },
    link    : function(scope, element){
      var labelClass = "label-success";
      if (!scope.requireRole) {
        scope.requireRole = getRequiredRoleFromParent(element);
        labelClass = "label-info";
      }
      element.prepend('' + scope.requireRole + '');
    }
  }
});

I am happy with the solution I came up with, yet my experience developing directives is limited; maybe there are better ways to achieve the same behavior?

Plunkr can be found here

In your HTML

  <form>

          <input required-role/>
          <input required-role/>
          <input required-role/>
          <input required-role/>
          <input />

              <input required-role/>

          <input required-role/>
          <input required-role/>
          <input required-role/>
          <input required-role="Admin"/>

      <button required-role="Admin">Save</button>
      <button required-role="User">Send to review</button>
  </form>

You create a div just to inherit from it, this is bad practice, unless there is more than one element inside that div that is going to inherit from the div

in the second group of code you actually do it the right way, not sure why you were inconsistent with the first block.

you should write it like this

<form>

        <input required-role/>
        <input required-role/>
        <input required-role/>
        <input required-role/>
        <input />
        <input required-role="User"/>

        <input required-role/>
        <input required-role/>
        <input required-role/>
        <input required-role="Admin"/>

    <button required-role="Admin">Save</button>
    <button required-role="User">Send to review</button>
</form>

I also assume that this is Example HTML as you don't give any of those input tags ID's  which makes them unusable.

In your Javascript

  var getRequiredRoleFromParent = function(element){
      var par = element.parent();
      if (!par) return;
      var is = par.isolateScope();
      if (is && is.requireRole) return is.requireRole;
      return getRequiredRoleFromParent(par);
  };

You could take the negative check out of there, it doesn't make sense that way, instead perform a positive check, if it's true then you can perform what is after that.

The "after that" stuff should be changed to a ternary statement, it looks cleaner to me and it is equivalent.  It is slightly nested but it shows the flow of the code brightly.

var getRequiredRoleFromParent = function(element){
    var par = element.parent();
    if (par) {
        var is = par.isolateScope();
        return (is && is.requireRole)
            ? is.requireRole
            : getRequiredRoleFromParent(par);
    }
};

The Recursion of this function looks good, I don't see an issue with the recursive nature of this function.


Populating a vector with alternating numbers This code inserts the number 4 into a vector's even indexes.

vector<double> vecCoeffs;
    // Put the coefficient 4 into each even index and 2 for each odd index
    for(int i = 0; i <= 10; i++){
        vecCoeffs[i];
        if(i % 2 == 0 ){
            vecCoeffs.push_back(4);
        }else{
            vecCoeffs.push_back(2);
        }
        // Starting and ending with coefficient 1
        vecCoeffs[0] = 1;
        vecCoeffs[10] = 1;

If you know how large the vector needs to be, the specify the size in the constructor so that it doesn't need to guess.

vecCoeffs[i]; is a useless statement.

It would be clearer to pull the assignment of starting and ending coefficients out of the loop, and to avoid assigning vecCoeffs[10] twice.  You could assign the starting and ending coefficients in the same statement.

The if-else would be better as a ternary conditional.

vector<double> vecCoeffs(11);
// Starting and ending coefficients
vecCoeffs[0] = vecCoeffs[vecCoeffs.size() - 1] = 1;
// Put the coefficient 2 into each odd index and 4 into each even index
for (int i = 1; i < vecCoeffs.size() - 1; i++) {
    vecCoeffs[i] = (i % 2 ? 2 : 4);
}

But the modulo operator is relatively slow.  You would be better off with two loops…

vector<double> vecCoeffs(11);

// Starting and ending coefficients
vecCoeffs[0] = vecCoeffs[vecCoeffs.size() - 1] = 1;

for (int i = 1; i < vecCoeffs.size() - 1; i += 2) {
    vecCoeffs[i] = 2;                // Odd coefficients
}
for (int i = 2; i < vecCoeffs.size() - 1; i += 2) {
    vecCoeffs[i] = 4;                // Even coefficients
}

… or maybe just one, but getting the termination correct is trickier.  This version might eliminate an instruction or two from the loop, but I don't recommend it.

vector<double> vecCoeffs(11);
for (int i = 0; i < vecCoeffs.size() - 1; i += 2) {
    vecCoeffs[i] = 4;                // Even coefficients
    vecCoeffs[i + 1] = 2;            // Odd coefficients
}
// Starting and ending coefficients
vecCoeffs[0] = vecCoeffs[vecCoeffs.size() - 1] = 1;


Student marks report I want to display a table similar to this:

This table is generated from three tables depending on the student ID:

I don't want the table to depend on fixed values. For example, I should be able to add a subject or exam in the database and see the changes reflected in the table without touching the code. Any suggestions?

<?

$con = mysql_connect("localhost","root","123");
if (!$con)
{
die('Could not connect: ' . mysql_error());
 }
mysql_select_db("test", $con);

////////////////////////

///////////////// Select the exams and put it in array
$ex = mysql_query("SELECT * FROM exams");
$dc=1;
while ($rowex= mysql_fetch_array($ex)){
$exn[$dc]=$rowex['Exam_Title'];
$exid[$dc]=$rowex['Exam_ID'];
$dc++;
}

/////////////////////////// Select the subjects and put it in array
$sj = mysql_query("SELECT * FROM subjects");
$dsj=1;
while ($rowsj= mysql_fetch_array($sj)){
$sjn[$dsj]=$rowsj['Subj_Title'];
$sjid[$dsj]=$rowsj['Subj_ID'];
$dsj++;
}

////////////////Select the student marks and put it in array with subject id and exam id

$result = mysql_query("SELECT * FROM stu_marks");
while ($row= mysql_fetch_array($result)){
$arr[$row['Subj_ID']][$row['Exam_ID']]=$row['Grade'];

}
/////////////////////// count the exams and the subjects to draw the table
$exc=count($exn);
 $sjc=count($sjn);
?>

   <?
///////// display subjects in table rows
for ($d=0;$d<=$sjc;$d++){

if ($d==0){
echo '-	';
}else{
echo '
'.$sjn[$d].'	';
}

///////// display exams in table head tds

for ($p=1;$p<=$exc;$p++){
if ($d==0){
 echo ' '.$exn[$p].'	';
}else{
?>

<?=$arr[$sjid[$d]][$exid[$p]]?>

 <?}
  }
  }?>

////////////////Select the student marks and put it in array with subject id and exam id

$result = mysql_query("SELECT * FROM stu_marks");
while ($row= mysql_fetch_array($result)){
$arr[$row['Subj_ID']][$row['Exam_ID']]=$row['Grade'];
}

I don't fully understand why you are pulling data out of an array to put it back into an array.  You should consider writing better, more specific sql queries so that the result of the individual SQl query is as close to what you want your table to look like as possible.  From there, you can then loop through the already made (via mysql_fetch_array) result and reduce the amount of redundancy.

Also, use PDO, don't use or die (probably not ready for these two yet) and for the love of god, NAME YOUR VARIABLES DESCRIPTIVELY.  I can barely tell whats going on and it's not in any way complicated.


CSS Coding Style - Floating and text alignment I'm not sure what is the best way to handle css when I have two uses of a class where the difference is one floats left and the other right.
An example:

img.screenshot{
width:100px;
height:110px;
padding:5px;
}

Do I do this:

img.screenshot.floatLeft{
float:left
}
img.screenshot.floatRight{
float:right;
}

Or this:

.floatLeft{float:left;}
.floatRight{float:right;}

Or:

...
...

Or something else?

The one that makes the most sense to me is the .float... class because it can be used my many elements, but it seems a bit pointless having a class that just wraps a single tag like that.
I have the same question regarding text alignment as well.

I don't have much experience with CSS & HTML.

CSS, like HTML itself, is essentially an interpreted computer program. That means, among other things, that while img.screenshot.floatLeft is more specific (it targets fewer elements), it's also more costly to evaluate. You can think of this in plain English: is it faster to say "if this is floatLeft" or "is this an img and is this screenshot and is this floatLeft"?

Clearly, the former is shorter, and if you have many images in a document, you might get a fairly modest boost in loading time. You also want to minimize the total number of rules you use to achieve the desired effects.

Inlining styles is generally considered a bad idea, because it associates content with layout, which was the original problem CSS was trying to solve. Before CSS, designers had to use many semantically meaningless elements for the sake of presentation, and inline styles are basically no different.

Given the previous paragraph, this also means that you should avoid wrapping elements for the sake of presentation. It's almost never necessary to have more than a few levels of nesting in a modern browser with reasonable CSS support. If you must do so, consider including aria attributes to improve accessibility.

Pretty much the only time you should inline is by script, where the values are not known ahead of time and must be calculated. I find myself using this most frequently to position dynamic elements. Even then, most of the styles (position, display, color, etc) still go in a CSS file, and I only specify left, right, top, or bottom attributes dynamically.

So, in summary, avoid elements without semantic meaning, make selectors as simple as possible, and inline styles only when necessary.


Logs 'Fizz' for multiples of three, 'Buzz', for multiples of 5, and 'Fizz Buzz' for multiples of 3 & 5 I'm new to coding and have been directed here from Stack Overflow. As I'm new to coding, I'm not entirely sure if my program works completely, but I think it does. I'm looking for suggestions on how to improve my code, particularly the format, as well as the code itself please.
Rather than output FizzBuzz for the first 100 numbers, i.e., 1 - 100, I wanted to write a program that could output FizzBuzz for any positive whole number range.
const fizzBuzz = (num) => {
  let fizzBuzzNumbers = [];

  const populateArray = num => {
    for (let i = 1; i < num + 1; i++) {
      fizzBuzzNumbers.push(i);
    }
  };

  populateArray(num)

  for (let i = 0; i < fizzBuzzNumbers.length; i++) {
    if (fizzBuzzNumbers[i] % 3 === 0 && fizzBuzzNumbers[i] % 5 === 0) {
      console.log('FizzBuzz');
    } else if (fizzBuzzNumbers[i] % 3 === 0 && fizzBuzzNumbers[i] % 5 !== 0) {
      console.log('Fizz');
    } else if (fizzBuzzNumbers[i] % 3 !== 0 && fizzBuzzNumbers[i] % 5 === 0) {
      console.log('Buzz');
    } else {
      console.log(fizzBuzzNumbers[i]);
    }
  }
};

Any feedback or suggestions will be greatly appreciated.

A short review;

You don't need the array, you could just use i
Part of the challenge is to realize that you only have to check for 3 and 5
i is a good short variable, I would go for num -> n or even count
I only use fat arrow syntax => for inline functions
Function names ideally follow <verb>Thing syntax so fizzBuzz -> logFizzBuzz

This is my counter proposal;

function logFizzBuzz(count){

  for (let i = 1; i <= count; i++) {
    let out = '';
    if (i % 3 === 0){
      out += 'Fizz';
    }
    if (i % 5 === 0){
      out += 'Buzz';
    }
    console.log(out || i);
  }
};

logFizzBuzz(45);


Applying an id attribute based on a variable's value I have the following code that checks the value of a variable and then applies the correct id attribute accordingly. The value of menuLink will have the same value of one of the parts of .mainmenu a href path name.

Is there a way to simplify this so it can search the href path of .mainmenu a instead of explicitly stating each one?

if (menuLink == "whatsnew") {
    $('.mainmenu a:eq(1)').attr('id', 'active').addClass("disable");
} else if (menuLink == "products") {
    $('.mainmenu a:eq(2)').attr('id', 'active').addClass("disable");
} else if (menuLink == "contact") {
    $('.mainmenu a:eq(3)').attr('id', 'active').addClass("disable");
} etc.....

Don't change IDs. IDs are supposed to identify a specific element, regardless of any state. "Active" should be a class.

The .mainmenu class should probably also be an ID. It's the "main menu" - that's its identity. Classes are for when you have multiple elements that are "of the same class", i.e. they share some functionality or styling. (If you have multiple things called "main menu", you have other problems.)

Using an active class will also let you use the IDs to find the link you want make active.

$("#mainmenu a.active").removeClass("active")
$("#" + menulink).addClass("active");

Basically: Find the currently active link, and remove the "active" class from it. Then find the link with the ID you're looking for, and set that to active by adding the class.


A simple animation in pygame I'm following a tutorial into pygame. I realized the example code was written for the sake of being easy to understand. I wondered if I could 'improve' the code, with a focus on keeping the exact same functionality. I tried, but I'm not really sure if this is an improvement.

This is the original code from the instructions:

import pygame, sys
from pygame.locals import *

pygame.init()

FPS = 30 # frames per second setting
fpsClock = pygame.time.Clock()

# set up the window
DISPLAYSURF = pygame.display.set_mode((400, 300), 0, 32)
pygame.display.set_caption('Animation')

WHITE = (255, 255, 255)
catImg = pygame.image.load('cat.png') # hosted at https://inventwithpython.com/cat.png
catx = 10
caty = 10
direction = 'right'

while True: # the main game loop
    DISPLAYSURF.fill(WHITE)

    if direction == 'right':
        catx += 5
        if catx == 280:
            direction = 'down'
    elif direction == 'down':
        caty += 5
        if caty == 220:
            direction = 'left'
    elif direction == 'left':
        catx -= 5
        if catx == 10:
            direction = 'up'
    elif direction == 'up':
        caty -= 5
        if caty == 10:
            direction = 'right'

    DISPLAYSURF.blit(catImg, (catx, caty))

    for event in pygame.event.get():
        if event.type == QUIT:
            pygame.quit()
            sys.exit()

    pygame.display.update()
    fpsClock.tick(FPS)

This is my 'improved' code. Two questions. In general, how can my version be improved and why is that improvement better then my code? And, how could the original be improved?

import pygame, sys
from pygame.locals import *

pygame.init()

FPS = 30
fps_clock = pygame.time.Clock()

window_size = (400,300)
DISPLAYSURF = pygame.display.set_mode(window_size,0,32)
pygame.display.set_caption("Animation test!")

WHITE = (255,255,255)
cat = pygame.image.load('assests/cat.png') # hosted at https://inventwithpython.com/cat.png
direction = 'right'
x_,y_ = 10,10

def listenToQuit():
    for event in pygame.event.get():
        if event.type == QUIT:
            pygame.quit()
            sys.exit()

def detectCollision(cat,x,y,window_size):
    return cat.get_size()[0] + x >= window_size[0] or \
        cat.get_size()[1] + y  >= window_size[1] or \
            x <= 0 or y <= 0

def getDirection(direction):
    directions = ['right','down','left','up']
    idx = directions.index(direction)
    idx = 0 if idx + 1 == len(directions) else idx + 1
    return directions[idx]

def getMovement(x,y,direction):
    if direction == 'right':
        return x+5,y
    elif direction == 'down':
        return x , y+5
    elif direction == 'left':
        return x - 5 , y
    elif direction == 'up':
        return x , y - 5

i = 0
while 1:
    DISPLAYSURF.fill(WHITE)

    x,y = getMovement(x_,y_,direction)
    while detectCollision(cat,x,y,window_size):  # if there would be a collision, change direction
        direction = getDirection(direction)
        x,y = getMovement(x_,y_,direction) # re-calculate movement, now avoiding collision

    x_,y_ = x,y

    DISPLAYSURF.blit(cat,(x,y))
    listenToQuit()

    pygame.display.update()
    fps_clock.tick(FPS)

I specifically chose not to make the animated object a class. I will try to do so as my next exercise towards myself.

Yours is definitely an improvement, but there is still room for more improvement:

PEP8

The official Python style guide will tell you:

two newlines between every function (i.e. before detectCollision, etc.)
function and variable names in snake_case, i.e. detect_collision
avoid line continuation: this

    return cat.get_size()[0] + x >= window_size[0] or \
        cat.get_size()[1] + y  >= window_size[1] or \
            x <= 0 or y <= 0

should become

return (
    cat.get_size()[0] + x >= window_size[0]
    or cat.get_size()[1] + y  >= window_size[1]
    or x <= 0
    or y <= 0
)

spaces after the commas in ['right','down','left','up']
remove the spaces before the commas here:

        return x , y+5
    elif direction == 'left':
        return x - 5 , y
    elif direction == 'up':
        return x , y - 5

Main function

Move everything starting with i = 0 into a main function that is called with a name guard:

if __name__ == '__main__':
    main()

Constant arrangement

There's a big crazy mix of constants and variables in global scope. I consider these constants that should be grouped together:

FPS = 30  # frames per second setting
WINDOW_SIZE = (400,300)
WHITE = (255, 255, 255)

Nothing else in that region should be in global scope.

Enums

Represent direction as an enum.Enum with four entries, rather than a stringly-typed variable.

Hard exit

Don't exit from listen_to_quit - just break out of the loop.

Better yet - rearrange your code so that you don't need a forever loop:

while not should_quit():
    pygame.display.update()
    fps_clock.tick(FPS)

    DISPLAYSURF.fill(WHITE)

    x,y = getMovement(x_,y_,direction)
    while detectCollision(cat,x,y,window_size):  # if there would be a collision, change direction
        direction = getDirection(direction)
        x,y = getMovement(x_,y_,direction) # re-calculate movement, now avoiding collision

    x_,y_ = x,y

    DISPLAYSURF.blit(cat,(x,y))


Function to determine the appropriate dimensions for a close button How can I reduce the cyclomatic complexity of this CloseButtonDimensions function?

export function isMobile():boolean {
    return (/Android|webOS|iPhone|iPad|iPod|BlackBerry|iemobile|Windows Phone/i.test(navigator.userAgent) );
}

get CloseButtonDimensions(): Size {
    const IS_MOBILE: Boolean = isMobile();

    let mediaType:String     = this.options.tag.media_type;
    let adWidth:number       = this.options.tag.size.width;
    let adHeight:number      = this.options.tag.size.height;

    let closeButtonHeight:number;
    let closeButtonWidth:number;

    if( (mediaType == "VIDEO") && !IS_MOBILE && ((adWidth >= 640 && adHeight >= 480) || (adWidth >= 480 && adHeight >= 360)) ) {
        closeButtonWidth  = 15;
        closeButtonHeight = 15;
    } else if( ((mediaType == "VIDEO")  && (adWidth >= 300 && adHeight >= 250)) ||
               ((mediaType == "BANNER") && (adWidth == 300 && adHeight == 250)) ||
               ((mediaType == "BANNER") && !IS_MOBILE && (adWidth == 468 && adHeight == 60)) ) {
        closeButtonWidth  = 11;
        closeButtonHeight = 11;
    } else if( (mediaType == "VIDEO")  &&  IS_MOBILE && (adWidth >= 320 && adHeight >= 240) ||
               (mediaType == "BANNER") && !IS_MOBILE && (adWidth == 728 && adHeight == 90) ) {
        closeButtonWidth  = 13;
        closeButtonHeight = 13;
    } else if((mediaType == "BANNER") && !IS_MOBILE && (adWidth == 468 && adHeight == 60)) {
        closeButtonWidth  = 13;
        closeButtonHeight = 13;
    } else if( (mediaType == "BANNER") && IS_MOBILE && ((adWidth == 320 && adHeight == 50) || (adWidth == 300 && adHeight == 50)) ) {
        closeButtonWidth  = 8;
        closeButtonHeight = 8;
    }

    return {
        width: closeButtonWidth,
        height: closeButtonHeight
    }
}

mediaType that I can get in the function are VIDEO and BANNER.

Ad dimensions (widthxheight) for video are (these can be greater than or equal to):

Desktop: "640x480", "480x360", "300x250"
Mobile:  "300x250", "320x240"

Ad dimensions (widthxheight) for banner are (these will be exact not greater nor equal):

Desktop: "728x90", "468x60", "300x250"
Mobile:  "300x250", "320x50", "300x50"

IS_MOBILE will be true/false depending on the device.

Questions:

How can I approach such complex if else statements while coding?
How can I make code more readable especially for junior developers or when I make commits to repos so others can understand better?

First, a few general notes about types.

Don't use String and Boolean. Use string and boolean instead. See the Typescript Handbook
If you know what mediaType must be, type it to require this.

type Media = 'VIDEO' | 'BANNER';

let m1: Media = 'VIDEO'; // OK
let m2: Media = 'video'; // Error: Type '"video"' is not assignable to type 'Media'.

Now on to your function. I believe your biggest problem here is that you are trying to do too much in a single function. As it appears you are using this in a class which can have two different types -- VIDEO and BANNER. You likely have other similar methods where the behavior is slightly different for each case. An abstract parent class for common functionality with a separate class for BANNER and VIDEO elements might a good idea.

To avoid assuming too much, I have left all of the logic in a single method for now, though splitting this up would greatly help with the method length.

The way I would do this is to avoid if statements as much as possible to make it simpler to extend the configuration in the future. With your current approach, what happens if the customer says they want to add a video ad with the dimensions of 300x300 on desktop? With this in mind, here is the approach I would take. It is a bit longer than your approach, but the cyclomatic complexity is 6, and adding further configurations does not increase the complexity. Splitting this up into two separate methods getCloseButtonBannerDims and getCloseButtonVideoDims is also a trivial change to shorten the method.

get CloseButtonDimensions(): Size {
    const IS_MOBILE = isMobile();

    let mediaType: Media = this.options.tag.media_type;
    let adWidth: number = this.options.tag.size.width;
    let adHeight: number = this.options.tag.size.height;

    // Sizes must be listed in decreasing order for this to work
    let sizes: Array<Size & {adWidth: number, adHeight: number}>;

    if (mediaType == "VIDEO") {
        if (!IS_MOBILE) {
            // Desktop:- "640x480", "480x360", "300x250"
            sizes = [
                { adWidth: 640, adHeight: 480, width: 15, height: 15 },
                { adWidth: 480, adHeight: 360, width: 15, height: 15 },
                { adWidth: 300, adHeight: 250, width: 11, height: 11 },
            ];
        } else {
            // Mobile:-  "300x250", "320x240"
            sizes = [
                { adWidth: 300, adHeight: 250, width: 11, height: 11 },
                { adWidth: 320, adHeight: 240, width: 13, height: 13 },
            ];
        }

        // Find the first ad where both the width and the height are
        // Greater than or equal to the expected size
        let {width, height} = sizes.find((size) => adWidth >= size.adWidth && adHeight >= size.adHeight);

        return { width, height };
    }

    // We know mediaType must be BANNER as we handled all VIDEO types above
    if (!IS_MOBILE) {
        // Desktop:- "728x90", "468x60", "300x250"
        sizes = [
            { adWidth: 728, adHeight: 90, width: 13, height: 13 },
            { adWidth: 468, adHeight: 60, width: 11, height: 11 },
            { adWidth: 300, adHeight: 250, width: 11, height: 11 },
        ]
    } else {
        // Mobile:-  "300x250", "320x50", "300x50"
        sizes = [
            { adWidth: 300, adHeight: 250, width: 11, height: 11 },
            { adWidth: 320, adHeight: 50, width: 8, height: 8 },
            { adWidth: 300, adHeight: 50, width: 8, height: 8 },
        ];
    }

    let {width, height} = sizes.find((size) => adWidth == size.adWidth && adHeight == size.adHeight);

    return { width, height };
}

For your more general questions, if you have a chain of if statements where all (or even several) check the same thing (!IS_MOBILE and mediaType == "BANNER"), it is probably a good idea to pull that check out and test it by itself before checking the other conditions (dimensions in this case). It might even be worth pulling all of the checks after that condition into their own method. The guiding principle here is to avoid doing more than one thing at once.

As for making code more readable for junior developers, trying to avoid doing more than one thing at once will help a lot. Try to make it so that when you go back in 6 months and revisit the code, there is a clear path through it. The fewer things you have to remember to understand a line of code, or a method, the better.


Structuring Test Data I'm just getting started with F# and was writing some unit tests for some functionality I had written. In the interest of brevity, I decided to create some lists of the different test cases and just run through them all in one test instead of creating separate test methods for each one (I'm not focusing on that design decision here, just pointing it out as otherwise my question won't make much sense).

So here's my code:

// simple helper to cut down on repeated code
let testRunner cases func =
    cases |> List.map func

// isNumeric is the function under test, its functionality should be obvious
[<Test>]
let ``IsNumericTest``() =
    let testCases = ["123"; "Foo"; "123Foo123"]
    let expectedResults = [true; false; false]
    let results = testRunner testCases isNumeric
    results |> should equal expectedResults

I started thinking about creating a simple type to encapsulate the test case input and the expected result, something like this:

type TestCase = { Input : string; ExpectedResult : bool}

type TestCases = TestCase list

let testCases = [{Input="123"; ExpectedResult=true};{Input="Foo"; ExpectedResult=false}; {Input="123Foo123"; ExpectedResult=false}]

Then I struggled to think of how to split each of those down to use in my testRunner method until I came up with this:

let results = testRunner (testCases |> List.map (fun (T) -> T.Input)) isNumeric
results |> should equal (testCases |> List.map (fun (T) -> T.ExpectedResult))

But that just looks ugly. Is there a better way to split out the Input value for my testRunner method? I am fine with changing it, but figured that while my question is specific to a unit testing scenario, the lessons I should learn would be widely applicable.

Since you're on NUnit, why don't you just use its built-in TestCase attribute? Your test would then look like this:

[<TestCase("123", true)>]
[<TestCase("Foo", false)>]
[<TestCase("123Foo123", false)>]
let ``IsNumericTest``(candidate : string, expected : bool) =
    let actual = isNumeric candidate
    actual |> should equal expected

xUnit.net (a better unit testing framework for .NET) has a similar feature.


Sorting texts by similarity in Clojure This is my first attempt with a lisp language. I'd like your review to address the following points, already listed in order of relevance:

Is my code making good use of standard library?
Is my code using the right data structures?
Is my code well formated and organized?
Is my code computationally efficient?

Code:

(ns clojure-first-try.core)

(use '[clojure.string :only [split]])
(use '[clojure.set :only [intersection]])

(defn sum [arr]
  (reduce + 0.0 arr))

(defn squares [arr]
  (map #(* % %) arr))

(defn words-frequency [text]
  (as-> text $
    (split $ #"\s+")
    (filter (complement empty?) $)
    (frequencies $)))

(defn cos-numerator [map-1 map-2]
  (def common-keys (intersection (set (keys map-1)) (set (keys map-2))))
  (sum (map #(* (map-1 %) (map-2 %)) common-keys)))

(defn cos-denominator [map-1 map-2]
  (def p-1 (sum (squares (vals map-1))))
  (def p-2 (sum (squares (vals map-2))))
  (* (Math/sqrt p-1) (Math/sqrt p-2)))

(defn cos-similarity [freq-1 freq-2]
  (def a (cos-numerator freq-1 freq-2))
  (def b (cos-denominator freq-1 freq-2))
  (if (= b 0.0) 0.0 (/ a b)))

(defn sort-by-similarity [base options]
  (def base-freq (words-frequency base))
  (def sorter (comp (partial cos-similarity base-freq) words-frequency))
  (sort-by sorter > options))

Tests:

(ns clojure-first-try.core-test
  (:require [clojure.test :refer :all]
            [clojure-first-try.core :refer :all]))

(defn close-enough? [a b]
  (def delta (- a b))
  (def abs-delta (max delta (- delta)))
  (> 0.0001 abs-delta))

(deftest words-frequency-test
  (testing "Counting the frequency of words in a text with actual words"
    (def expected {"a" 3, "bb" 2, "ccc" 4})
    (def input "ccc ccc a bb a bb a ccc ccc")
    (def result (words-frequency input))
    (is (= expected result)))
  (testing "Counting the frequency of words in an empty text"
    (def expected {})
    (def input "")
    (def result (words-frequency input))
    (is (= expected result))))

(deftest cos-similarity-test
  (testing "Getting the similarity of two equal texts"
    (def expected 1.0)
    (def input-1 {:a 1 :b 2})
    (def input-2 {:b 2 :a 1})
    (def result (cos-similarity input-1 input-2))
    (is (close-enough? expected result)))
  (testing "Getting the similarity of two totally different texts"
    (def expected 0.0)
    (def input-1 {:a 1 :b 2})
    (def input-2 {:c 2 :d 1})
    (def result (cos-similarity input-1 input-2))
    (is (close-enough? expected result)))
  (testing "Getting the similarity of two half equal texts"
    (def expected 0.5)
    (def input-1 {:a 2 :b 2})
    (def input-2 {:b 2 :d 2})
    (def result (cos-similarity input-1 input-2))
    (is (close-enough? expected result))))

(deftest sort-by-similarity-test
  (testing "Sorting regular texts"
    (def expected ["a b c" "a b d" "d e f"])
    (def input-1 "a b c")
    (def input-2 ["d e f" "a b c" "a b d"])
    (def result (sort-by-similarity input-1 input-2))
    (is (= expected result)))
  (testing "Sorting empty list of options")
    (def expected [])
    (def input-1 "a b c")
    (def input-2 [])
    (def result (sort-by-similarity input-1 input-2))
    (is (= expected result)))

All tests passing:

lein test clojure-first-try.core-test

Ran 3 tests containing 7 assertions.
0 failures, 0 errors.

First, the most serious problem with your code:

Never, ever, ever use def inside of defn, unless you really know what you're doing. def creates global variables that last even once the function has returned:

(defn func []
  (def x 1))

(func)
(println x) ; Prints 1! Ouch!

The only time it's really ever appropriate to use def locally is when you intend to create globals. This is an extremely rare case though.

Instead, you should be using let. For example:

(defn cos-denominator [map-1 map-2]
  (let [p-1 (sum (squares (vals map-1)))
        p-2 (sum (squares (vals map-2)))]
    (* (Math/sqrt p-1) (Math/sqrt p-2))))

let creates a limited scope for the vals that it defines. Once you leave the let, they go out of scope.

That's the only really outright wrong thing I see in your code. The rest of my comments will be best practice related.

use, require and related functions shouldn't really be used directly. It's much cleaner to have them as part of the ns macro:

(ns clojure-first-try.core
  (:use [clojure.string :only [split]]
        [clojure.set :only [intersection]]))

Your use of $ as an identifier is bizarre. I had to double take there. I would recommend using something a little more descriptive. If you can't think of a good identifier to use, I think even a single letter would be better than a odd symbol like $.

(reduce + 0.0 arr))

is more typically written as

(apply + arr)

Your ways not wrong, the latter is just generally considered more idiomatic. This works because + has a var-arg overload that is basically an explicit reduction.

Also, you don't need to specify 0.0 as the starting accumulator of the reduction. + handles that for you.

(reduce + []) ; 0

(filter (complement empty?) arr)

can be written as

(remove empty? arr)

remove adds complement for you! Here's the core definition (basically):

(defn remove [pred coll] ; Abridged defintion
   (filter (complement pred) coll))

It reads much nicer without the explicit call to complement.

(= b 0.0)

can also be written as just:

(zero? b)

Although the gains aren't huge. I find it's better when in an already busy line, like when checking for factors of a number. I find:

(zero? (rem a b))

to be neater than

(= 0 (rem a b))

For cos-numerator:

(intersection (set (keys map-1)) (set (keys map-2)))

is really inefficient. You iterate both maps just to put them in a set to test for intersections. Without doing more timing, it's unclear what the complexity of keys is. If it's O(n), this iterates each map twice, then again as a set. It's much more concise, and significantly faster to just use filter:

(filter map-1 (keys map-2))

Maps return nil on a bad lookup, so they can be used as predicates to check for membership.

I also think this could be cleaned up with ->>. I changed it to:

(defn cos-numerator [map-1 map-2]
  (->> (filter map-1 (keys map-2))
       (map #(* (map-1 %) (map-2 %)))
       (sum)))

squares could also be written as

(map * arr arr))

You're going to have a little bit of duplication somewhere. It just depends what you want it to look like. I find this a little neater.

I'm a big fan of local anonymous functions to clean up code. cos-denominator has you doing the exact same thing to both maps, then taking their product.

I'd make the common transformation a local function, then use that instead:

(defn cos-denominator [map-1 map-2]
  (let [proc #(-> % (vals) (squares) (sum) (Math/sqrt))]
    (* (proc map-1) (proc map-2))))

That has much less duplication. You could even go a little overboard and get rid of the duplicate calls to proc:

(defn cos-denominator2 [map-1 map-2]
  (let [proc #(-> % (vals) (squares) (sum) (Math/sqrt))]
    (apply * (map proc [map-1 map-2]))))

Which could also be made a little neater with ->>:

(defn cos-denominator3 [map-1 map-2]
  (let [proc #(-> % (vals) (squares) (sum) (Math/sqrt))]
    (->> [map-1 map-2]
         (map proc)
         (apply *))))

After the above changes, and a few other touchups that I thought made it neater, this is what I ended up with:

(ns clojure-first-try.core
  (:use [clojure.string :only [split]]
        [clojure.set :only [intersection]]
        [clojure.test :refer :all])
  (:require [criterium.core :as c]))

(defn sum [arr]
  (apply + arr))

(defn squares [arr]
  (map * arr arr))

(defn words-frequency [text]
  (as-> text t
        (split t #"\s+")
        (filter (complement empty?) t)
        (frequencies t)))

(defn cos-numerator [map-1 map-2]
  (->> (filter map-1 (keys map-2))
       (map #(* (map-1 %) (map-2 %)))
       (sum)))

(defn cos-denominator [map-1 map-2]
  (let [proc #(-> % (vals) (squares) (sum) (Math/sqrt))]
    (->> [map-1 map-2]
         (map proc)
         (apply *))))

(defn cos-similarity [freq-1 freq-2]
  (let [a (cos-numerator freq-1 freq-2)
        b (cos-denominator freq-1 freq-2)]
    (if (zero? b)
      0.0
      (/ a b))))

(defn sort-by-similarity [base options]
  (let [base-freq (words-frequency base)
        sorter (comp #(cos-similarity base-freq %) words-frequency)]
    (sort-by sorter > options)))


Handling with the 'error' parameter inside a callback function in node.js I have multiple controllers and every controller has this error handler:

if (error) {
  res.render('error', {
     error: error
  });
}

Sometimes I have more than one:

exports.update = function(req, res, next) {
    MyModel.findById(req.params.id, function(error, event) {
        if (error) {
            res.render('error', {
                error: error
            });
        }
        else {
            event['active'] === false ? event['active'] = true : event['active'] = false;

            event.update({
                'active': event['active']
            }, function(error) {
                if (error) {
                    res.render('error', {
                        error: error
                    });
                }
                else {
                    res.end('Success!');
                }
            });
        }
    });
};

I have more than 30 controllers and this seems like an inefficient way of handling.

If all of your functions adhere to this pattern you can wrap them in,
you guessed it, another function:

defaultError = function(handler) {
    return function(error) {
        if (error) {
            res.render('error', {
                error: error
            });
        }
        else {
            return handler.apply(null, Array.prototype.slice.call(arguments, 1));
        }
    };
};

This uses the
special arguments object,
which has to be converted to an array before you can slice off the error object, and apply on the passed-in function.

Now the previously repetitive code gets a bit shorter:

exports.update = function(req, res, next) {
    MyModel.findById(req.params.id, defaultError(function(event) {
        event['active'] === false ? event['active'] = true : event['active'] = false;

        event.update({
            'active': event['active']
        }, defaultError(function() {
            res.end('Success!');
        }));
    }));
};

Now obviously the defaultError can also customize the rendered error,
so as long as error is in a fixed position in the call, this pattern
can be used to build a small control flow abstraction.

The cost here is a possible speed penalty due to the manipulation of arguments and the apply call.

If you had promises a generic error handler would be equally
viable, but it looks like that is not an option.


Google's Python exercise about lists very different from the given solution I found some Python exercises that were made by Google in their Python classes and decided to spend some time with them.
Given the following description:

E. Given two lists sorted in increasing order, create and return a merged
ist of all the elements in sorted order. You may modify the passed in lists.
deally, the solution should work in "linear" time, making a single
pass of both lists.

So, knowing that comparing two characters is \$O(1)\$, Python's sorted() function in this situation is \$O(n \log{n})\$ and thinking that merging two lists into a new one is \$O(k)\$ where \$k\$ the number of elements in list1 + list2, which is quite "linear" in the sense of what was asked, I did...
def linear_merge(list1, list2):
  return sorted(list1 + list2)

However, when looking at the problem's solution, I found it to be somewhat different:
def linear_merge(list1, list2):
  result = []
  # Look at the two lists so long as both are non-empty.
  # Take whichever element [0] is smaller.
  while len(list1) and len(list2):
    if list1[0] < list2[0]:
      result.append(list1.pop(0))
    else:
      result.append(list2.pop(0))

  # Now tack on what's left
  result.extend(list1)
  result.extend(list2)
  return result

which is followed by the following comment:

Note: the solution above is kind of cute, but unforunately list.pop(0)
is not constant time with the standard Python list implementation, so
the above is not strictly linear time.
An alternate approach uses pop(-1) to remove the endmost elements
from each list, building a solution list which is backwards.
Then use reversed() to put the result back in the correct order. That
solution works in linear time, but is more ugly.

This confused me a little bit, since my solution looks... better in general (code and complexity, given the last comment paragraph).
Are any of my assumptions about my version of the code wrong?
Keep in mind that I wrote this version using Python3, when instead google's python classes uses Python2. I'm not really sure, but this may have something to do with it.
Here is the rest of the related source to give a full example:
def test(got, expected):
  if got == expected:
    prefix = ' OK '
  else:
    prefix = '  X '
  print('{} got: {} expected: {}'.format(prefix, repr(got), repr(expected)))

# Calls the above functions with interesting inputs.
def main():
  print('linear_merge')
  test(linear_merge(['aa', 'xx', 'zz'], ['bb', 'cc']),
       ['aa', 'bb', 'cc', 'xx', 'zz'])
  test(linear_merge(['aa', 'xx'], ['bb', 'cc', 'zz']),
       ['aa', 'bb', 'cc', 'xx', 'zz'])
  test(linear_merge(['aa', 'aa'], ['aa', 'bb', 'bb']),
       ['aa', 'aa', 'aa', 'bb', 'bb'])

if __name__ == '__main__':
  main()

In a nutshell, yours is better, for two reasons.

First, Python isn't designed for speed. It's decently fast, but the goal is code like yours: so clear, concise, obvious, and readable that anyone can glance at it and immediately see what it does. You can then spend the rest of the project's development time working on the difficult problems (like attending meetings).

Second, the "answer" code doesn't really answer the exercise, as it notes. It looks right in theory, but popping elements from the beginning of a list is not the most performant operation in most languages, including Python. It would be a more reasonable solution with a linked list, which is a type that was probably omitted from Python precisely because its utility is mostly limited to fixing the micro-optimizations in bloated code. This is like optimizing a recursive function by making it tail-recursive, and then admitting that it doesn't make any difference because Python doesn't have tail call optimization.

If you were using this code in a real program and determined through actual testing that this linear_merge function was taking too much time due to the extra sorting, you might then be justified in optimizing it.

For fun, here's something with indexing instead of pop():

def linear_merge(list1, list2):
    result = []
    c1 = c2 = 0
    while c1<len(list1) and c2<len(list2):
        if list1[c1] <= list2[c2]:
            result.append(list1[c1])
            c1 += 1
        else:
            result.append(list2[c2])
            c2 += 1
    result.extend(list2[c1:])
    result.extend(list1[c2:])
    return result

This might be faster due to not pop()ing items from the beginning of each list, but it also might be slower (or possibly more memory-intensive) due to having to slice a list at the end. I leave it as an exercise to you to time these approaches... but remember that the most important time to conserve is usually your own, not your computer's.


Detect file type using file signatures My program has to detect the real file type of a given file using signatures. For now I'm just checking JPG, but I want to add more.

Dim files() As String = IO.Directory.GetFiles(pictures)
Dim file_data As Byte()

Dim jpg_file_extension() As Byte = {&HFF, &HD8, &HFF}
Dim office_file_extension() As Byte = {&H50, &H4B, &H3, &H4, &H14, &H0, &H6, &H0}

Dim check As Integer = 0

For Each file As String In files
    file_data = IO.File.ReadAllBytes(file)
    If file_data.Length > 2 Then
        For i = 0 To jpg_file_extension.Length - 1
            If file_data(i) = jpg_file_extension(i) Then
                check += 1
            Else
                check = 0
                Exit For
            End If
        Next
        If (check.ToString.Length = jpg_file_extension.Length - 1) Then
            MsgBox(file.Split("\").Last & ": its jpg")
        End If
     End If
Next

The code looks a bit messy right now and It's only checking one file type, my questions are:

How can I improve this code, make it cleaner and efficient.
Is there a way to implement this code in such a way that I can have a function, give it the file data and check if the signature is "whitelisted"?

Your first step is to wind your thinking back a few steps and re-approach your code with a fresh line of thinking. Looking at your code, you say "to detect the real file type of a given file" but you have written code to detect a JPEG(*) file.

There is a subtlety here, but once you have mastered that you can approach complex problems with more confidence. The subtlety is you want a generic approach, but your thinking at the moment is constrained to and focussed on a particular example - your solution is tailored to that example. More specifically, your current code answers the question "Is this a JPEG file?", you want your solution to answer the question "What is the file type of this file?".

Signatures

You define your signatures early. This is a good approach because it lends itself to a future implementation where you can import a tailored list of signatures.

However, you are currently using separate arrays to store the signature data. The use of multiple arrays is going to be inefficient for any improvements or even for checking multiple files/filetypes.

The use of static arrays implies looping through all arrays. In a small implementation this is not that noticeable, but if you have a hundred arrays with a size ranging from 3 to 15 bytes, you will start to notice a performance hit. Basically, you will be continuing to check arrays that you have already eliminated as being relevant to your quest.

A suggested way to improve the performance initially is to put the signatures in a collection (e.g. List(Of OrderedList(Of Byte))). This way, once you eliminate a signature you can remove it from the collection, thus quickly removing the unnecessary checks with a commensurate improvement in performance.

The use of the inner collection removes the need to check array lengths, but having a List(Of Array) could also work.

Looping

You manually loop through your array. This is always a simple first approach and reflects the basic solution to identifying a signature. Your code is set up to first loop through the first signature and I assume you were thinking of duplicating this kind of loop for the other signatures.

Sitting here, I can think of two simple approaches:

Looping through the file bytes individually, removing signatures from the collection as they fail
Looping through the signatures and doing an array check against the first x bytes of each file

Intuitively, I think the second option will be less efficient but I could be wrong.
Some example code (not guaranteed to be compilable):

For Each file As String In files
    file_data = IO.File.ReadAllBytes(file)
    For signatureIterator = MasterSignatureList.Count - 1 to 0 Step -1
    ' Declare and implement as required
    ' Used a For loop going backwards because in this example we are going to remove elements from the collection
        signature = MasterSignatureList(signatureIterator) ' the shorter text makes my example easier to read.
        If file_data.Length < signature.Length then
            MasterSignatureList.Remove signatureIterator
        Else
            If Not CheckArrayIsSame(file_data.Resize(signature.length), signature) then
            ' Some function to check arrays are the same will be required
            ' The native .Resize actually changes the original array, so you should make a copy before running .Resize. I was being lazy.
                MasterSignatureList.Remove signatureIterator
            End If
        End if
    Next signatureIterator
' **** do something here with the remaining signatures as these are the valid ones for that particular file!
Next file

And an example for the first option

For Each file As String In files
    file_data = IO.File.ReadAllBytes(file)
    For each signature in MasterSignatureList
        if filedata.Length < signature.Length Then MasterSignatureList.Remove signature ' Obviously wrong
    Next signature
    For signatureIterator = 0 to file_data.Length ' we should exit the loop before getting to the end of most files!
        signatureCheck = false
        For each signature in MasterSignatureList
            If signatureIterator <  signature.Length Then ' retains signatures that have already passed
                signatureCheck = true ' still some signatures to check
                If file_data(signatureIterator) <> signature(signatureIterator) Then
                    MasterSignatureList.Remove signature ' signature does not match
                End if
            End if
        Next signature
        If MasterSignatureList.Empty or Not signatureCheck then Exit For ' exit if nothing left to check
    Next signatureIterator
' **** do something here with the remaining signatures as these are the valid ones for that particular file!
Next file

In both of those examples, the signatures remaining in the signature list are the potential file types. In these examples, the possibility of multiple signatures passing is allowed - how you handle that is up to your programming logic.

As already noted - I have not tested the above code, so also check for the dreaded Jedi array error condition (off-by-1) in my iterations.

(*) The correct nomenclature is JPEG, the file extension in traditional 8.3 style is ".jpg". Why this is so, I leave up to your own research.


Image downloader class I'm a novice regarding both C++ and Qt. This class is used in my project to get an image from a web service.
If a new call to StartDownload is made before the previous has finished, the old one can safely be discarded as it won't be needed any more. This is because a new request is made for each resize event, as one of the parameters to the service is the image size (font size in the image is adapted to the image size).
As a result, when a request is canceled, Qt prints an error message: QCoreApplication::postEvent: Unexpected null receiver. This does not interfere with any functionality, but tells me I haven't handled canceling the request properly. Several of these messages are printed as I resize the window and CancelDownload is called.
Aside from that, any general feedback on my design or C++ idiomacy is welcome!
Note: I'm using C++11 and Qt 5.6.
Header
#ifndef DOWNLOADER_H
#define DOWNLOADER_H

#include <QtNetwork/qnetworkaccessmanager.h>
#include <qbytearray.h>
#include <QObject>

class Downloader: public QObject
{
    Q_OBJECT

public:
    Downloader();
    ~Downloader();
    void StartDownload(QUrl url);

private:
    void CancelDownload();

    QNetworkAccessManager mNetworkAccessManager;
    QNetworkReply *mCurrentReply;

signals:
    void DownloadFinished(QByteArray bytes);

private slots:
    void HandleFinishedDownload();
};

#endif // DOWNLOADER_H

Source
#include <QtNetwork/qnetworkrequest.h>
#include <QtNetwork/qnetworkreply.h>
#include "Downloader.h"

Downloader::Downloader()
    : mNetworkAccessManager(), mCurrentReply(Q_NULLPTR)
{
}

Downloader::~Downloader()
{
    CancelDownload();
    mNetworkAccessManager.deleteLater();
}

void Downloader::CancelDownload()
{
    if (mCurrentReply != Q_NULLPTR) {
        mCurrentReply->abort();
        mCurrentReply->deleteLater();
        mCurrentReply = Q_NULLPTR;
    }
}

void Downloader::StartDownload(QUrl url)
{
    CancelDownload();

    QNetworkRequest request(url);
    mCurrentReply = mNetworkAccessManager.get(request);
    connect(mCurrentReply, &QNetworkReply::finished, this, &Downloader::HandleFinishedDownload);
}

void Downloader::HandleFinishedDownload()
{
    if (!mCurrentReply) {
        qCritical("Null reply in HandleFinishedDownload");
        return;
    }

    if (mCurrentReply->error() == QNetworkReply::NoError) {
        emit DownloadFinished(mCurrentReply->readAll());
    }
    else if (mCurrentReply->error() != QNetworkReply::OperationCanceledError) {
        qWarning() << "Failed download:" << mCurrentReply->errorString();
    }

    mCurrentReply->deleteLater();
    mCurrentReply = Q_NULLPTR;
}

Update
Although I solved the issue described above in a self-answer, I am still looking for a general review!

The error message is caused by me misunderstanding the purpose of deleteLater. As implied by the documentation, the member function is only required when deleting the object inside a slot, like HandleFinishedDownload.

  Note: Do not delete the object in the slot connected to the error() or finished() signal. Use deleteLater().

In all other cases, the standard delete keyword should be used. As a result, here is an updated implementation of CancelDownload.

void Downloader::CancelDownload()
{
    if (mCurrentReply != Q_NULLPTR) {
        mCurrentReply->abort();
        delete mCurrentReply; // this is the only altered line
        mCurrentReply = Q_NULLPTR;
    }
}


Number palindrome checker My daughter, nine, recently had some homework looking at number palindromes. Part of the assignment was to take a number, reverse it, add the two together and repeat until she got a palindrome and to do this for several different numbers.

While helping her with this it struck me it'd be a nice simple idea for a computer program. She & I have messed about in Scratch a bit, but this sounded like a nice problem to do in a more commercial language. Thought I could write it and then show her how it worked.

So I did. It turned out to be not quite as child-friendly as I'd imagined.

using System;
using System.Collections.Generic;
using System.Text;
using System.Linq;

namespace CheckNumberPalindrome
{
    class Program
    {
        static void Main(string[] args)
        {
            string rollingSum;
            int iterations = 1;

            if(IsNumeric(args[0]) == false)
            {
                Console.WriteLine("You did not enter a numeric value");
            }

            rollingSum = RecurseUntilPalindrome(args[0], ref iterations);
            Console.WriteLine(string.Format("The final palindrome is {0}, which took {1} steps.", rollingSum, iterations));
        }

        private static bool IsNumeric(string s)
        {
            long result;
            return Int64.TryParse(s, out result);
        }

        private static string RecurseUntilPalindrome(string s, ref int iterations)
        {
            long rollingSum = AddToPalindrome(s);

            if(IsPalindrome(rollingSum.ToString()))
            {
                return rollingSum.ToString();
            }

            iterations++;
            return RecurseUntilPalindrome(rollingSum.ToString(), ref iterations);
        }

        private static long AddToPalindrome(string s)
        {
            string reversedString = ReverseString(s);
            long result = Convert.ToInt64(s) + Convert.ToInt64(reversedString);
            Console.WriteLine(string.Format("Adding {0} to {1} = {2}", s, reversedString, result));
            return result;
        }

        private static string ReverseString(string s)
        {
            char[] charArray = s.ToCharArray();
            Array.Reverse(charArray);
            return new string(charArray);
        }

        private static bool IsPalindrome(string s)
        {
            return s.SequenceEqual(ReverseString(s));
        }
    }
}

There are two questions here:

How could I make this better or cleaner or more efficient or any of the other things that CR does so well?
What could I do (beside hiding some of the functions in pre-compiled code elsewhere) to make this more child-friendly, or better for teaching/explaining some basic code concepts?

This may help with your second point about code concepts:

How about addressing the special case for when the user enters a palindrome?  It looks to me like you are assuming the entered number will not be a palindrome and you automatically add to it (calling RecurseUntilPalindrome).  How about adding an initial check to account for that case?  I think it would go along well with your numerical value input check (I consider invalid user input as a sort of special case as well).

To address your first point, I think your code looks pretty clean as is! Really easy to follow.


Simple angular app - Shopping website In looking for a bit of feedback about an Angular app I have created.

I'm new to angular I want to clean up the loose ends before I carry on and add new functionality.

Below is the code snippet of index.html which includes custom directives that includes a block of HTML and various angular elements

It also includes a $http service to fetch Json from an external file

{{store.title}}

            <product-title></product-title>

Item is not available for purchase

        <button ng-show="product.canPurchase">Add to cart</button>
        <product-panels></product-panels>

This is my app.js which contains the controllers

var app = angular.module('store', ['store-products']);

app.controller("ReviewController", function () {
// New review array slot is created and assigned blank
this.review = {};

// Form triggers function to push new review slot into array
this.addReview = function (product) {
    product.reviews.push(this.review);
    // Set the review form to blank again
    this.review = {};
}
});

app.controller('StoreController', ['$http', function ($http) {
this.title = "Angular App";
    console.log('test');
var store = this;
//store.products = [];

$http.get('/products.json').then(function (response) {
    console.log(response);
    store.products = response.data;
});
}]);

and finally here is my directives.js. with the logic being stored in external html files.

var app = angular.module('store-products', []);

app.directive('productPanels', function () {

return {
    restrict: 'E',
    templateUrl: 'product-panels.html',
    controller: function () {

        this.tab = 1;

        this.selectTab = function (setTab) {
            this.tab = setTab;
        }

        this.isSelected = function (checkTab) {
            return this.tab === checkTab;
        }

    },
    controllerAs: 'panel',
};
});

app.directive('productTitle', function () {

return {
    restrict: 'EA',
    templateUrl: 'product-title.html'
};
});

I noticed that you have a spot for controllers, but then in your directive you declare a function for the Controller Attribute instead of using a controller that you have already at your disposal from the controller's section.

I would think that you would want to be able to reuse that function that controls tab selection, you should create a controller for that so that you can reuse it on several pages, rather than write from scratch every time you create a page with Panel tabs.


Lazy Load for multiple entities at a time We have a system with a non standard database solution. All trips to the DB are rather expensive. We cannot use entity framework.

Currently our lazy loading is on an entity by entity basis. So if I have a Customer and access their Orders object it only loads the Orders for that customer.

Something like so

//DAL
public List<Entities.Customer> GetCustomersByIds(IEnumerable<int> ids)
{
    var customers = db.GetCustomersByIds(ids);
    foreach(var customer in customers)
    {
        customer.Orders = new ResetLazy<List<Entities.Order>>(() =>
            db.GetOrdersByCustomerId(c.Id));
    }
    return customers;
}

ResetLazy taken from here https://stackoverflow.com/a/6255398/102526

Ideally if we had a collection of Customers and accessed an Orders collection on one of the customers it would load all the Orders for all the Customers. (If not in a collection it would just load it's own orders)

//DAL
public List<Entities.Customer> GetCustomersByIds(IEnumerable<int> ids)
{
    var customers = db.GetCustomersByIds(ids);
    foreach(var customer in customers)
    {
        customer.Orders = new ResetLazy<List<Entities.Order>>(() =>
            GetOrdersByCustomersLazy(customers, customer.Id));
    }
    return customers;
}

protected List<Entities.Order> GetOrdersByCustomersLazy(
    List<Entities.Customer> customers,
    int customerId)
{
    var orders = db.GetOrdersByCustomerIds(
        customers.Select(customer => customer.Id).AsEnumerable());
    foreach(var customer in customers)
    {
        customer.Orders = new ResetLazy<List<Entities.Order>>(() => {
            customer.Orders = new ResetLazy<List<Entities.Order>>(() =>
                GetOrdersByCustomersLazy(customers, customer.Id));
            return orders.Where(order => order.CustomerId == customer.Id).ToList();
        });
    }
    return customers.FirstOrDefault(customer => customer.Id == customerId).Orders.Value;
}

This seems to work well, but it is too complex and not generic.

You code seems to not doing what it is supposed to do.

Assume the following implementation (using the ResetLazy)

Customer and Order

namespace Entities
{
    public class Customer
    {
        public int Id { get; set; }
        public ResetLazy<List<Entities.Order>> Orders {get;set;}
    }
    public class Order
    {
        public int Id { get; set; }
        public int CustomerId { get; set; }
    }
}

Simulated database

public class TheDatabase
{
    public TheDatabase()
    {
        customers.Add(new Entities.Customer { Id = 1 });
        orders.Add(new Entities.Order { Id = 1, CustomerId = 1 });
        orders.Add(new Entities.Order { Id = 2, CustomerId = 1 });
        orders.Add(new Entities.Order { Id = 3, CustomerId = 1 });

        customers.Add(new Entities.Customer { Id = 2 });

        customers.Add(new Entities.Customer { Id = 3 });
        orders.Add(new Entities.Order { Id = 4, CustomerId = 3 });
        orders.Add(new Entities.Order { Id = 5, CustomerId = 3 });
    }
    private List<Entities.Customer> customers = new List<Entities.Customer>();
    private List<Entities.Order> orders = new List<Entities.Order>();
    public List<Entities.Customer> GetCustomersByIds(IEnumerable<int> ids)
    {
        return customers.Where(c => ids.Contains(c.Id)).ToList();
    }
    public IEnumerable<Entities.Order> GetOrdersByCustomerIds(IEnumerable<int> ids)
    {
        return orders.Where(c => IsContained(c.CustomerId, ids));
    }
    private bool IsContained(int id, IEnumerable<int> ids)
    {
        foreach (int currentId in ids)
        {
            if (id == currentId) { return true; }
        }
        return false;
    }
}

And the class containing your methods

class Program
{

    static void Main(string[] args)
    {
        Program p = new Program();
        IEnumerable<int> ids = new List<int>() { 1, 2, 3 };
        List<Entities.Customer> customers = p.GetCustomersByIds(ids);

        int customerId = 1;
        List<Entities.Order> orders = new List<Entities.Order>(customers.Where(c => c.Id == customerId).First().Orders.Value);
        System.Diagnostics.Debug.Assert(orders[0].CustomerId == customerId);
    }

    TheDatabase db;
    private Program()
    {
        db = new TheDatabase();
    }

    public List<Entities.Customer> GetCustomersByIds(IEnumerable<int> ids)
    {
        var customers = db.GetCustomersByIds(ids);
        foreach (var customer in customers)
        {
            customer.Orders = new ResetLazy<List<Entities.Order>>(() =>
                GetOrdersByCustomersLazy(customers, customer.Id));
        }
        return customers;
    }

    protected List<Entities.Order> GetOrdersByCustomersLazy(
        List<Entities.Customer> customers,
        int customerId)
    {
        var orders = db.GetOrdersByCustomerIds(
            customers.Select(customer => customer.Id).AsEnumerable());
        foreach (var customer in customers)
        {
            customer.Orders = new ResetLazy<List<Entities.Order>>(() =>
            {
                customer.Orders = new ResetLazy<List<Entities.Order>>(() =>
                    GetOrdersByCustomersLazy(customers, customer.Id));
                return orders.Where(order => order.CustomerId == customer.Id).ToList();
            });
        }
        return customers.FirstOrDefault(customer => customer.Id == customerId).Orders.Value;
    }
}

The Debug.Assert() method signals the wrong id.

Fixing

By adding a setter to ResetLazy.Value like

set
{

    if (mode != LazyThreadSafetyMode.None)
    {
        lock (syncLock)
        {
            this.box = new Box(value);
        }
    }
    else
    {
        this.box = new Box(value);
    }
}

and changing the GetOrdersByCustomersLazy() method to

protected List<Entities.Order> GetOrdersByCustomersLazy(
    List<Entities.Customer> customers,
    int customerId)
{
    var orders = db.GetOrdersByCustomerIds(
        customers.Select(customer => customer.Id).AsEnumerable());
    foreach (var customer in customers)
    {
        customer.Orders.Value = orders.Where(o => o.CustomerId == customer.Id).ToList();
    }
    return customers.FirstOrDefault(customer => customer.Id == customerId).Orders.Value;
}

The following runs as expected

Program p = new Program();
IEnumerable<int> ids = new List<int>() { 1, 2, 3 };
List<Entities.Customer> customers = p.GetCustomersByIds(ids);

int customerId = 1;
List<Entities.Order> orders = new List<Entities.Order>(customers.Where(c => c.Id == customerId).First().Orders.Value);
System.Diagnostics.Debug.Assert(orders.Count==3 && orders[0].CustomerId == customerId);

customerId = 2;
orders = new List<Entities.Order>(customers.Where(c => c.Id == customerId).First().Orders.Value);
System.Diagnostics.Debug.Assert(orders.Count==0);

customerId = 3;
orders = new List<Entities.Order>(customers.Where(c => c.Id == customerId).First().Orders.Value);
System.Diagnostics.Debug.Assert(orders.Count == 2 && orders[0].CustomerId == customerId);

You shouldn't shorten names of variables. The advantage of typing less is reduced by the lack of readability.

A call to the linq Select() method returns an IEnumerable so a call to AsEnumerable() is not needed.


ECS System in Javascript I'm trying to get a better feel for how an entity-component-system system works. I'm most comfortable in JavaScript, so that's what I wrote it in. I'm mostly looking for ways my code violates ECS principles, but am also looking for ways to make my code more idiomatic.

class Component {
    get type() { return this.constructor.name; }

    constructor(data) {
        this.data = data;
    }
}

class Entity {
    get type() { return this.constructor.name; }

    constructor(id) {
        this.id = id;
        this.components = [];
    }

    get(type) { return this.components.find(c => c.type === type); }

    assign(component) {
        const included = this.get(component.type);
        if (included === undefined) this.components.push(component);
        else this.components[this.components.indexOf(included)] = component;
    }
}

class System {
    get type() { return this.constructor.name; }

    constructor(types, callback) {
        this.types = types;
        this.callback = callback;
    }

    run(entities) {
        entities.forEach(e =>
            this.types.every(t => e.get(t) !== undefined) &&
            this.callback(e)
        );
    }
}

Also, here's a basic example of how the system gets extended.

class ErrorComponent extends Component {
    constructor(error) {
        super(error);
    }
}

class ErrorSystem extends System {
    constructor() {
        super(
            ["ErrorComponent"],
            e => console.error(e.get("ErrorComponent").data)
        );
    }
}
const testComponent = new ErrorComponent("test");
const testComponent2 = new ErrorComponent("test2");

const testEntity = new Entity("test");
testEntity.assign(testComponent);
testEntity.assign(testComponent2);

const entities = [testEntity];
const testSystem = new ErrorSystem();
testSystem.run(entities);

One other thing that I feel like I don't understand about ECS is why I should extend Component and System, but not Entity. It feels asymmetrical. For example, could I say

class ErrorEntity extends Entity {
    constructor(id, error) {
        super(id);
        this.assign(new ErrorComponent(error));
    }
}

?
Or does this violate some principle of ECS?

I didn't like the strings in the ErrorSystem call, so I factored them out. Comments and additional answers are still welcome.

class Component {
    constructor(data) {
        this.data = data;
    }
}

class Entity {
    constructor(id) {
        this.id = id;
        this.components = [];
    }

    type(component) {
        return component.prototype === undefined?
            component.constructor.name:
            component.prototype.constructor.name;
    }

    get(component) { return this.components.find(c => this.type(c) === this.type(component)); }

    set(component) {
        const included = this.get(component);
        if (included === undefined) this.components.push(component);
        else this.components[this.components.indexOf(included)] = component;
    }
}

class System {
    constructor(components, callback) {
        this.components = components;
        this.callback = callback;
    }

    run(entities) {
        entities.forEach(e =>
            this.components.every(c => e.get(c) !== undefined) &&
            this.callback(e)
        );
    }
}

class ErrorComponent extends Component {
    constructor(error) {
        super(error);
    }
}

class ErrorSystem extends System {
    constructor() {
        super(
            [ErrorComponent],
            e => console.error(e.get(ErrorComponent).data)
        );
    }
}

const testComponent = new ErrorComponent("test");

const testEntity = new Entity("test");
testEntity.set(testComponent);

const entities = [testEntity];
const testSystem = new ErrorSystem();
testSystem.run(entities);


A Dockerfile for Composer (a dependency manager for PHP) I recently built a Docker image for Composer. I'd love to get a review of the image, the Bash based wrapper script, its recommended use, and the repository structure.

Here's the Dockerfile for the latest tag:

FROM alpine:edge

MAINTAINER Samuel Parkinson <sam@graze.com>

RUN echo "http://dl-4.alpinelinux.org/alpine/edge/testing" >> /etc/apk/repositories && \
    apk add --no-cache \
    ca-certificates \
    git \
    mercurial \
    subversion \
    php7 \
    php7-curl \
    php7-json \
    php7-openssl \
    php7-phar \
    php7-posix

RUN /usr/bin/php7 -r "readfile('https://getcomposer.org/installer');" | \
    /usr/bin/php7 -- --install-dir=/usr/local/bin --filename=composer

COPY ./composer-wrapper /usr/local/bin/composer-wrapper

VOLUME ["/usr/src/app", "/root/.composer"]

WORKDIR /usr/src/app

ENTRYPOINT ["/usr/local/bin/composer-wrapper"]

The wrapper script, which adds the --ignore-platform-reqs flag for supported commands, so that users don't have to do it themselves as the image doesn't contain many of the common php extensions library's require:

#!/bin/sh

# Loop over each argument.
for argument in "$@"; do
    case "$argument" in
        # Append the argument if the command matches one we need to use `--ignore-platform-reqs` with.
        # Found using the following search: https://github.com/composer/composer/search?q=ignore-platform-reqs+path%3Asrc%2FComposer%2FCommand%2F
        # Uses `set` to update the arguments, see https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html.
        create-project|install|remove|require|update) set -- --ignore-platform-reqs "$@";;

        # Otherwise just pass it on.
        *) ;;
    esac
done

# Call composer with the updated arguments.
exec /usr/bin/php7 /usr/local/bin/composer "$@"

The recommended way to use the image, mounting the source and composer user folder, and read only mounting the users ssh key for use when using git based dependencies:

docker run --rm -it \
    -v $(pwd):/usr/src/app \
    -v ~/.composer:/root/.composer \
    -v ~/.ssh:/root/.ssh:ro \
    graze/composer

And finally the repository itself (which includes tests of the image!):

https://github.com/graze/docker-composer

I'm no expert at bash, and pretty new to Docker too so I'd love to know if there's anything I'm missing, or anything I'm doing that's unconventional.

In the wrapper script,
I don't understand why you need to loop over each argument.
Wouldn't it be enough to use a case "$1" in ... without looping at all?

If you really do need to loop,
for example to make it work even if the command you want to match is not in the first position sometimes,
then I think you want to break out of the loop after finding a match. That is:

# Loop over each argument.
for argument in "$@"; do
    case "$argument" in
        # Append the argument if the command matches one we need to use `--ignore-platform-reqs` with.
        # Found using the following search: https://github.com/composer/composer/search?q=ignore-platform-reqs+path%3Asrc%2FComposer%2FCommand%2F
        # Uses `set` to update the arguments, see https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html.
        create-project|install|remove|require|update) set -- --ignore-platform-reqs "$@"
        break
        ;;

        # Otherwise just pass it on.
        *) ;;
    esac
done

Another small thing,
at the top of the Dockerfile,
I find the echo ... >> /etc/apk/repositories && apk add ... a bit curious.
Under what circumstances might that appending with echo fail?
And why not check the exit code of the other commands that follow?
It seems that either all commands should be chained with &&, or none of them.

Lastly, I have very minimal experience with Docker,
but I'm wondering if instead of multiple RUN commands,
wouldn't it be better to put them in a single shell script,
and RUN that script instead.
(That's not a recommendation, just a general wondering...)


Node.js callback closure/scope Is it a bad idea to use the notfound callback like this? If so, why not? (bad scope/closure? I'm a noob node.js dev.)
Is it a bad idea to use the saveCallback callback like this? I have a feeling it's dangerous, but I don't want to declare saveCallback before queryCallback as save happens after query.

create: function(registerDetails, found, notfound) {

    var query = db.User
        .findOne({ username: registerDetails.username });

    var queryCallback = function(err, user) {
        if (user != null) found(user);
        else {
            var newUser = new db.User();
            newUser.username = registerDetails.username;
            newUser.password = registerDetails.password;
            newUser.save(saveCallback);
        }
    };

    var saveCallback = function(err, user) {
        notfound(user);
    };

    query.exec(queryCallback);
},

What's a more elegant way of writing this so that

// 1) don't nest like crazy.
.exec(function(){
    if () {
        newUser.save(function(){
            notfound(); // really? because we know node.js will ever chain 2 callbacks max. </sarcasm>
        }
    }

// 2) don't make people read "down and back up"
var saveCallback = function() {
    // Dejavu... I've been here before...
};
var queryCallback = function() {
    saveCallback(); // Back from last line, ohhhhh this is what saveCallback used for. Go back up.
};
query.exec(queryCallback); // Ohhhhh so that's what it's used for. Now go back up and read what it does.

Interesting question,

I would go with this:

create: function (registerDetails, found, notfound) {

    db.User.findOne({
        username: registerDetails.username
    }).exec(queryCallback);

    function queryCallback(err, user) {
        if (user != null) {
            found(user);
        } else {
            var newUser = new db.User();
            newUser.username = registerDetails.username;
            newUser.password = registerDetails.password;
            newUser.save(saveCallback);
        }
    };

    function saveCallback(err, user) {
        notfound(user);
    };
},

Basically, I

Used the fact that JavaScript does hoisting, you can use functions prior to having them declared. Given this I put the .exec() on top, I also did away with query which I would have called userQuery.
I put proper braces and newlines around if( user!= null ), to be tested, but if you can get away with it, you should consider if(user)
I moved from var xx = function() to function xx() because anonymous functions in stacktraces are not fun


Python3 - Prime Sieve I've implemented two prime sieves in python 3 to find all prime numbers up to an upper limit n - n exlusive. The first one is rather naive, while the second one only considers odd numbers to begin with (some clever index-arithmetic is necessary).

Naive implementation:

def naive_sieve(n):
    if n < 3:
        return []

    sieve = [False, False, True] + [True, False] * (n // 2 - 1)

    for prime in range(3, int(n**0.5) + 1, 2):
        if sieve[prime]:
            for multiple in range(prime*prime, len(sieve), 2*prime):
                sieve[multiple] = False

    return [number for number, is_prime in enumerate(sieve) if is_prime]

I actually implemented the more advanced sieve twice. One time giving every little step a sub-function with name (to get the understanding right) and the second time without all these definitions (to minimize the number of
function calls).

More readable implementation

def easy_odd_sieve(n):
    if n < 3:
        return []

    def number_of_odd_nums_below(n):
        return n // 2

    def greatest_odd_number_below(n):
       return ceil(n) // 2 * 2 - 1

    def index_of_odd_number(n):
        return (n - 1) // 2

    def odd_number_from_index(i):
        return (2*i + 1)

    sieve = [0] + [1] * (number_of_odd_nums_below(n) - 1)

    for j in range(1, index_of_odd_number(greatest_odd_number_below(n ** 0.5)) + 1):
        if sieve[j]:
            for i in range(index_of_odd_number(odd_number_from_index(j) ** 2), len(sieve), odd_number_from_index(j)):
                sieve[i] = False

    return [2] + [odd_number_from_index(index) for index, is_prime in enumerate(sieve) if is_prime]

Final implementation:

def odd_sieve(n):
    if n < 3:
        return []

    sieve = [0] + [1] * (n //2 - 1)

    for j in range(1, ceil(n ** 0.5) // 2):
        if sieve[j]:
            for i in range((2*j)*(j + 1), len(sieve), 2*j + 1):
                sieve[i] = False

    return [2] + [2*index + 1 for index, is_prime in enumerate(sieve) if is_prime]

My questions regarding this code are:

How does my general python programming style look like?
Correctness. Is the code correct or did I overlook something?
(I've checked that all sieves return the same list for n in range(1, 1000))
Naming. Are the variable names clear to the reader? What would you change?

Your style generally looks fine. Do run flake8 and make the edits suggested, mostly adjusting whitespace. Starting out with zero and one, then switching to False, is a bit odd.

In e.g. last line of odd_sieve(), consider turning that into a three-line list comprehension.

    sieve = [False, False, True] + [True, False] * (n // 2 - 1)

That seems to allocate lots more than sqrt(N) entries.

The function name of number_of_odd_nums_below() appears to be off-by-one, as it's missing a comment explaining one may only pass in odd numbers. With such a restriction it seems that greatest_odd_number_below() would simply become n - 2.

My personal preference is for the naive code. The fancier version feels like it's cheating. If we get to use long division, why stop at 2? Why not 3, 5, 7, 11? When (before sqrt(N)) is the right place to stop, and transition to sieving? The advantage of the sieve is that, once having constructed a bounded list of primes, it can rapidly answer lots of prime queries. It would be interesting to see a particular problem domain involving many repeated queries where a natural tradeoff really does emerge.