Return path to a value in a nested object Description:

Its not a programming challenge but I though to write a small utility which will return the path to a primitive value in a possibly nested object. The idea is not original.

Code:

console.clear();

function each(obj, cb, path) {
  path = path || [];
  for (let k in obj) {
    let val = obj[k];
    if (isLeaf(val)) {
      if (cb(val, path.concat(k)) === false) return false;
    }
    else {
      if (each(val, cb, path.concat(k)) === false) return false;
    }
  }
  return true;
}

let deep = {foo: {bar: true}, baz: 2, blurg: {fop: {hif: []}}};

// If the value is one of the primitive type?
function isLeaf(val) {
  return val !== Object(val) || Array.isArray(val);
}

each(deep, (val, path) => {
  console.log(val);
  console.log(path);
  //return false;
});

/*
          { }
    (foo) / \  (baz)
{bar: true} {2}
  (bar) /
  {true}
*/

Note:

Initially I had no return statement and it worked fine but I thought to give client code to give opportunity to stop the iteration once it finds the desired path/value so after fiddling a bit I had to add couple to return statement and it worked. So, I gained an useful insight on how to
unroll the recursive call from any level.

Questions:

As always I had trouble designing the recursive solution and it was done after some hit and try. I need to know if there is some mental model to keep in mind while doing these kinds of problems?

Having said that I would like to know if the solution is readable, clear and concise?

What modifications I need to make if need to make it part of open source library?

Recursion & JavaScript don't mix.

I am a big fan of recursion, but with JavaScript there are some issues that make recursion a dangerous animal to play with.

Call stack limit

One of JavaScript's limitations, lack of proper tail calls, (ES6 has it slated but there is a world of noncompliance out there) is both a saving grace and a curse.

Consider the object

var cyclic = {me : null};

Your function deals with it and exits. But there are many objects in the JavaScript environment that are self referencing. It is perfectly normal for me to do,

cyclic.me = cyclic;

and your code will thankfully bail with the lovely

RangeError: Maximum call stack size exceeded at Object (native)

This is because there is a limit to the depth of the call stack. It is also why recursion is not always safe. It may be ridiculous in this example but a recursive function can not be said to be safe. You can not know when you will exceed the call stack, especially if you are calling the recursive function innocently from within another recursive function. You can not enforce calling conditions so recursion is really something to be avoided in JavaScript.

As for the cyclic object. You will have to implement a check to ensure you are not chasing your own tail.

So the rest will ignore the fact that you should not be writing recursive code in JavaScript unless you extremely careful.

Some problems

Not so much about recursion, you have the understanding of how they work.

Bad naming. Functions each and isLeaf to findPrimitives and  isPrimitive
It is not immediately clear what the callback function is for, and its return type is a ambiguous. See below for more.
Memory usage. Don't use Array.concat inside recursive code, if you can avoid it. Array.concat creates a new reference to the array, so each step into the recursion you create a new reference that is closed over, effectively creating may similar copies of the same data.
isleaf now isPrimitive is failing. There is a more robust way to test if the current "leaf" is a primitive type. See rewrite at bottom.

The callback function is ambiguous. One of JavaScript's flaws is no way to force a return type for a function. Many people consider undefined == false, and not undefined !== false and would expect that the unspecified return value (functions return undefined by default) to mean false. I would remove the ambiguity and require that the callback function answer the question "Has the object you are looking for been found?" and return true to exit from the recursion"

So by changing the callback return requirement to true to exit early the findPrimitive return value makes more sense.

if(findPrimitives(deep,report) === false){  // did not find what I am looking for

Neither right or wrong.

There are some differing schools of thought as to the use of the let token and the importance of block scope. When I see it being used inconsistently and interchangeably with either const or (in this case) var, I consider that it is not being correctly used and must consider if in fact the author has sound logic for creating block scoped variables or just using a trendy new feature because it is there.
There were many ways to make the function fail, though it is only an exercise, testing is still very important. Bad habits can form and incorrect assumption made when you don't test your experiments to their destruction.
I am not a fan of anonymous functions if they can be avoided as it make debugging so much more difficult. I have seen traces that are just anon calling anon all the way from the top,the only way to trace the actual path is to step out one by one.
For all of us JavaScript programmers it is a time of transition. Some will use ES6 to the full, and others avoid it because of the legacy browsers (Dam you IE for freezing just as ES6 was adopted!). But with ES6 it's in for a penny, in for a pound. You have clearly opted in for ES6 so you should use it to the full. Currently your code is a mixture of ES5 and ES6, being incompatible to ES5 only environments and not getting the full benefit of ES6.

Style

Your style is not horrible, but it's not excellent either.

There are two issues. Your use of let and naming as noted above. And  the following

If you can say that you have never ever had to look for a bug caused by

if(bar){
    if(blah) foo = true;
    boo = false;
}

that was meant to be

if(bar){
    if (blah) {
        foo = true;
        boo = false;
    }
}

then ignore this point. If you have you will know how hard they are to spot, especially when you have masses of complex code. There is one way to avoid this frustration. All conditional statements are blocks and blocks require the embrace of {curlies}. You can still keep the single line space saving style if ( blah ) { foo=true; }

The rewrite.

This was hard to do as I would implement this type of function completely differently. So it is a compromise between your implied requirements (from your code) and my A.D.D. driving me to start it from scratch.

I changed the arguments. Separating out the current object / property name. I keep the same path array reference rather than create a new one each time. I use Object.keys to get an array of property names rather than for(k in obj) I vet for callback function. I vet for obj being a primitive or not having properties. I changed isPrimitive to a more suitable solution.

"use strict"; // make code run quicker and helps catch many potencial bugs
              // that otherwise may go unnoticed until it's to late.
function isPrimitive (val) {
    // typeof null returns object but for this null is a primitive
    return val === null || ["object","function"].indexOf(typeof val) === -1;
}
// added propName as argument. It is the name of the object being examined
// ES6 default argument assignment. Rather than in the function having
// path = path || [] put it in
// the function argument declaration
function findPrimitives (obj, callback, propName, path = []) {
    var keys, name;                 // don't use let if the variables are just one block
                              // in from the function scope on entry and exit.
    if(typeof callback !== "function"){ // no point if there is no callback
        return undefined;     // Return neither true or false as the question
                              // has not been asked.
    }
    if (typeof obj === "object") {
        keys = Object.keys(obj); // As this is ES6 code Object.keys can be used
                                 // to get all the own properties
    } else {
        if( isPrimitive(obj) ) {
            return callback(obj, propName, path);
        }
        return false;
    }
    path.push(propName);
    for(name of keys){
        if (isPrimitive(obj[name]) ) {
            if (callback(obj[name], name, path) === true) {  // explictly true to exit
                return true; // This is early exit not popping values
                             // of the path to keep it intact
            }
        } else {
            if (findPrimitives(obj[name], callback, name,  path)) { // implied true
                return true; // This is early exit not popping values of
                             // the path to keep it intact
            }
        }
    }
    path.pop();  // done with this branch pop it from the path
    return false;
}

and debugging code, is just a quick run test and is not an example of how to test code. It is to help debug and refine the code.

var deep = {
    foo: {
        bar: true
    },
    baz: 2,
    blurg: {
        fop: {
            hif: [],
            hof: /hi/g, // regExp
            now : new Date(),
            what : "String",
            undef : undefined,
            nulls : null,
            nan : NaN,
        }
    },
    verb : function(){return "hi"},
    cur : 10,

};
var cyclic = {}
cyclic.me = cyclic;

function report(value,name,path){  // avoiding anon function for callback
    log(path.join(".")+"."+name + "=" + value + " type ("+(typeof value)+")")
}
findPrimitives(deep,report);
findPrimitives(deep,report,"deep");
findPrimitives(deep,report,"deep",["window"]);

// the following will be inconsistant with expectations
findPrimitives(deep);
findPrimitives("hi");
findPrimitives("hi",report);

// the following will crash and burn....
findPrimitives(cyclic,report);


BMI calculator in python using if statements I am a beginner in the python language. I wrote a BMI calculator and I am wondering if there are any improvements I could make to the code. This is the code below:

print('Enter your weight in kilo:')
weightInKilo = float(input())

print('Enter your height in feet:')

heightInFeet = float(input())

heightInMeter = heightInFeet * 12 * 0.025

bodyMassIndex = weightInKilo / (heightInMeter ** 2)

if bodyMassIndex < 15:
    print('Your BMI = ' + str(bodyMassIndex)  + ' You are very severely underweight.')

elif bodyMassIndex >= 15 and bodyMassIndex <= 16 :
    print('Your BMI = ' + str(bodyMassIndex)     + ' You are severely underweight.')

elif bodyMassIndex > 16 and bodyMassIndex <= 18.5:
    print('Your BMI = ' + str(bodyMassIndex) + ' You are underweight.')

elif bodyMassIndex > 18.5 and bodyMassIndex <= 25:
    print('Your BMI = ' + str(bodyMassIndex) + ' You are Normal(healthy weight).')

elif bodyMassIndex > 25 and bodyMassIndex <= 30:
    print('Your BMI = ' + str(bodyMassIndex) + ' You are overweight.')

elif bodyMassIndex > 30 and bodyMassIndex <= 35:
    print('Your BMI = ' + str(bodyMassIndex) + ' You are moderately obese.')

elif bodyMassIndex > 35 and bodyMassIndex <= 40:
    print('Your BMI = ' + str(bodyMassIndex) + ' You are severely obese.')

elif bodyMassIndex > 40:
    print('Your BMI = ' + str(bodyMassIndex) + ' You are very severely obese.')

input('Please press Enter to exit')

This kind of programming exercice, despite its apparent simplicity, is a good opportunity to learn various things.

Style

Python has a Code Style guide called PEP 8. If you begin with Python, I highly recommend reading it every now and then and trying to apply it.

In your case, a few things could be improved regarding style:

blank lines between the different branches of if are more an inconvenience from my point of view
variables should follow the snake_case naming convention (instead of camelCase)

Builtin input

As you've noticed for the end of the function, the input builtin takes an optional prompt parameter. This could be used at the beginning of the function as well.

Duplicated code

A lot of code looks like the same line with minimal variations. Having duplicated code makes the code more tedious to read and harder to maintain (if you need to change something, you'll need to change it in many places). One of the principles of software programming is Don't Repeat Yourself (also written DRY).

In your case, we could store the body category in a variable and only print the ouput from a single place:

Chained comparison

This is very specific to Python but instead of body_mass_index > 30 and body_mass_index <= 35, we can write: 30 < body_mass_index <= 35 using chained comparisons.

Magic numbers

Magic Numbers are usually frowned upon. In our case, one needs a bit of thinking to understand where the 12 and 0.025 come from. A better way to handle this is to store them in a variable with a meaningful name.

I am not quite sure about the best naming convention for names related to conversion constant. I've followed the names suggested by Solomon Ucko in the comments.

At this stage, we have the following code:

INCHES_PER_FOOT = 12
METERS_PER_INCH = 0.0254

weight_in_kilo = float(input('Enter your weight in kilo:'))
height_in_feet = float(input('Enter your height in feet:'))

height_in_meter = height_in_feet * INCHES_PER_FOOT * METERS_PER_INCH
body_mass_index = weight_in_kilo / (height_in_meter ** 2)

if body_mass_index < 15:
    category = 'very severely underweight'
elif 15 <= body_mass_index <= 16 :
    category = 'severely underweight'
elif 16 < body_mass_index <= 18.5:
    category = 'underweight'
elif 18.5 < body_mass_index <= 25:
    category = 'Normal(healthy weight)'
elif 25 < body_mass_index <= 30:
    category = 'overweight'
elif 30 < body_mass_index <= 35:
    category = 'moderately obese'
elif 35 < body_mass_index <= 40:
    category = 'severely obese'
elif body_mass_index > 40:
    category = 'very severely obese'

print('Your BMI = ' + str(body_mass_index) + ' You are ' + category + '.')
input('Please press Enter to exit')

Which can still be improved.

Mutually exclusive conditions

Because of the way we check body_mass_index, if it is under 15, we get into the first case so there is no need to check elif 15 <= body_mass_index in the else part. This is guaranteed to be always true. Similarly, half the checks have no effect.

Code organisation

To make the code easier to understand (and easier to reuse, to test, etc), it is a good habit to split in into smaller reusable chunks. In our case, defining functions could be a nice touch.

Disclaimer: Next paragraph can be a bit overwhelming for a beginner, do not worry if you do not fully get it. It highlights how to solve problems you may not be interested in yet but it is a good chance to do things properly.

Also, if we want to be able to actually reuse your functions, we want to be able to import the file. Currently, if we do so, we get stuck into the parts asking for user inputs. Thus, the usual strategy is the following: split your code into 2 parts:

code defining functions/constants/classes/etc but without any side-effect or user interactions
code actually doing things (input/output, etc) behind an if __name__ == "__main__": guard. As pointed out by Solomon Ucko, it's generally a good idea to have this performed via a main() function and have the last two (non-blank) lines be if __name__ == '__main__': main(). This allows the interactivity to be triggered as needed.

At this stage, we have:

INCHES_PER_FOOT = 12
METERS_PER_INCH = 0.0254

def convert_feet_to_meter(height_in_feet):
    return height_in_feet * INCHES_PER_FOOT * METERS_PER_INCH

def get_body_mass_index(height_in_meter, weight_in_kilo):
    return weight_in_kilo / (height_in_meter ** 2)

def get_category(body_mass_index):
    if body_mass_index < 15:
        return 'very severely underweight'
    elif body_mass_index <= 16 :
        return 'severely underweight'
    elif body_mass_index <= 18.5:
        return 'underweight'
    elif body_mass_index <= 25:
        return 'Normal(healthy weight)'
    elif body_mass_index <= 30:
        return 'overweight'
    elif body_mass_index <= 35:
        return 'moderately obese'
    elif body_mass_index <= 40:
        return 'severely obese'
    else:
        return 'very severely obese'

def main():
    weight_in_kilo = float(input('Enter your weight in kilo:'))
    height_in_feet = float(input('Enter your height in feet:'))
    height_in_meter = convert_feet_to_meter(height_in_feet)
    body_mass_index = get_body_mass_index(height_in_meter, weight_in_kilo)
    category = get_category(body_mass_index)
    print('Your BMI = ' + str(body_mass_index) + ' You are ' + category + '.')
    input('Please press Enter to exit')

if __name__ == "__main__":
    main()

Going further, a few more details could be improved.

String formatting

Python offers many tools to format strings so that you do not need to use string concatenations. You can refer to PyFormat.info for documentations and examples.

You could write:

print('Your BMI = %f You are %s.' % (body_mass_index, category))

Or the newer technique:

print('Your BMI = {} You are {}.'.format(body_mass_index, category))

Also, in Python 3.6, yet another soution was added: F-strings.

Data over code

This may be a bit overkill here but sometimes a lot of code can be replaced by a small amount of code working on a properly filled data structure. In our case, the get_category function does the same thing for all categories: check the if we are under a given limit and if so, return the category name.

Disclaimer: Next part works under the assumption that body_mass_index < 15 should actually use <= like the other cases.

# List of pairs (higher-limit, name) sorted
CATEGORIES = [
    (15, 'very severely underweight'),
    (16, 'severely underweight'),
    (18.5, 'underweight'),
    (25, 'Normal(healthy weight)'),
    (30, 'overweight'),
    (35, 'moderately obese'),
    (40, 'severely obese'),
]

def get_category(body_mass_index):
    for limit, name in CATEGORIES:
        if body_mass_index <= limit:
            return name
    return 'very severely obese'


Project Euler 39: Integer right triangles I just finished Project Euler 39:

  If p is the perimeter of a right angle triangle with integral length sides, {a, b, c} … For which value of p ≤ 1000, is the number of solutions maximised?

I'm pretty satisfied with my solution. I derive the formula for the hypotenuse as follows:

$$\begin{align}
a + b + c &= p \tag{Def. perimeter}\\
b + c &= p - a \tag{2}\\
\\
a^2 + b^2 &= c^2 \tag{Pythagorean}\\
a^2 &= c^2 - b^2 \\
a^2 &= (c - b)(c + b) \\
a^2 &= (c - b)(p - a) \tag{from (2)} \\
\frac{a^2}{p - a} &= c - b \tag{7} \\
\\
(c - b) + (c + b) &= \frac{a^2}{p - a} + (p - a) \tag{from (7) and (2)}\\
2c &= \frac{a^2}{p - a} + \frac{(p - a)^2}{p - a} \\
2c &= \frac{a^2 + (p - a)^2}{p - a} \\
c &= \frac{a^2 + (p - a)^2}{2\ (p - a)}
\end{align}$$

Any optimization would be appreciated.

from timeit import default_timer as timer

start = timer()
def find_pythagorean_triples(perimeter):
    count = 0
    for a in range(3, perimeter / 2 - 1): # triangle inequality
        c = (a**2 + (perimeter - a)**2) / (2*(perimeter - a)) # derived formula
        b = (c**2 - a**2)**0.5
        if b.is_integer() and a + b + c == perimeter:
            count += 1
    return count

def find_most_solutions_upto(limit):
    data = {"most": 0, "num": 0}
    for p in range(limit/2, limit + 1): # scale upwards
        if find_pythagorean_triples(p) > data['most']:
            data['most'] = find_pythagorean_triples(p)
            data['num'] = p
    return data['num']

ans = find_most_solutions_upto(1000)
elapsed_time = (timer() - start) * 1000 # s --> ms

print "Found %d in %r ms." % (ans, elapsed_time)

You are using a dictionary data = {"most": 0, "num": 0} where you could just as well use local variables. The locals would be more readable and faster.
The built-in max function is convenient when looking for a maximum. You could write find_most_solutions_upto like this:

def find_most_solutions_upto(limit):
    return max(xrange(limit/2, limit + 1), key=find_pythagorean_triples)


Function that takes an array of integers and returns that array rotated to the right N components Example:
  if \$N=2\$, given the input array \${[1, 2, 3, 4, 5, 6]}\$ the function should return \${[5, 6, 1, 2, 3, 4]}\$

Here is my code:

def copyDigit(list,index,item):

        """
                Copy the item to the indexed location in the given list
        """
        list[index] = item
        return list

def rotateList(noListToRorate,timesRotate):

        """
                Rotate List to right
        """
        print "Function received {0},{1}".format(noListToRorate,timesRotate)
        for rotate in range(N):
            lastItemInList = noListToRorate[-1]
            for i in range(len(noListToRorate)-2,-1,-1):
                itemToShift = noListToRorate[i]
                noListToRorate = copyDigit(noListToRorate,i+1,itemToShift)
            noListToRorate[0] = lastItemInList
        print "Rotate once: {0}".format(noListToRorate)
        return noListToRorate

if __name__ == '__main__':

        """
           This program will rorate right the given list N no of times
        """

        arrayList = [1,2,3,4,5,6]
        N = 2
        print "Rotate an array: " ,(arrayList), "No of times: ", N
        finalList = rotateList(arrayList,N)
        print "Rotated List: ", finalList

The obvious solution

Not sure if you know the obvious solution and just want to re-invent the wheel or if you are just not aware of this but you could just do :

>>> l = [1, 2, 3, 4, 5, 6]
>>> n = 2
>>> l[-n:] + l[:-n]
[5, 6, 1, 2, 3, 4]

The actual review : details

You have split your code into multiple functions, documented them, and written the usual if __name-- == 'main': test : this is already a pretty good beginning.

Also, pep8 only complies about some whitespace not following the Python Style Guide (this is worth fixing it anyway).

However, here a few things that do not look good in your code and could easily be fixed :

your code can be python 3 compliant by adding parentheses in your calls to print.
you have the same typo rorate in multiple places
list is a bad name for a list as you can easily mix-up with the type
your function and variable names do not follow the convention.

After fixing all this, you get something like :

def copy_digit(lst, index, item):
    """
            Copy the item to the indexed location in the given list
    """
    lst[index] = item
    return lst

def rotate_list(lst, nb_rotate):
    """
            Rotate List to right
    """
    print("Function received {0},{1}".format(lst, nb_rotate))
    for rotate in range(N):
        last_item = lst[-1]
        for i in range(len(lst) - 2, -1, -1):
            item_to_shift = lst[i]
            lst = copy_digit(lst, i + 1, item_to_shift)
        lst[0] = last_item
    print("Rotate once: {0}".format(lst))
    return lst

if __name__ == '__main__':
    """
       This program will rotate right the given list N no of times
    """

    array = [1, 2, 3, 4, 5, 6]
    N = 2
    print("Rotate an array: ", array, "No of times: ", N)
    final_list = rotate_list(array, N)
    print("Rotated List: ", final_list)

A subtle bug

What if I was to call :

final_list = rotate_list(array, 2)

? Surprisingly enough, I'd get an error. I'll let you have a look at this because it is not very complicated.

Clearer interface for your functions

Your function copy_digit updates the list and returns it. There is no real need for this as the caller would already have the initial list. Then you can just have :

def copy_digit(lst, index, item):
    """
            Copy the item to the indexed location in the given list
    """
    lst[index] = item

and

...
copy_digit(lst, i + 1, item_to_shift)
...

Of course, the need for a function seems a bit doubtful here. Let's inline this :

...
lst[i + 1] = item_to_shift
...

Then, it seems like the item_to_shift variable is not really required anymore :

for i in range(len(lst) - 2, -1, -1):
    lst[i + 1] = lst[i]

And more generally

In Programming Pearls (sorry I couldn't find the relevant full text), Jon Bentley describes different solutions to the problem. Some are quite original and actually very easy to implement on top of being efficient. I'll let you google "programming pearls rotate" if you want to read more about this. It is definitely worth a read (and the whole book is too).


PHP Authentication Security Can someone look over this?  This is the entire authentication file.  It's referenced at the beginning of all my catalog editing files to make sure only the specified user is logged in.  I want to make sure it's secure.

<?php
//      pageauth.php
//      Asks for a username and a password and checks it.

if($_SESSION['logged_in'] != 1){
    if(!isset($_SERVER['PHP_AUTH_USER'])){
        header('WWW-Authenticate: Basic realm="Catalog Administration"');
        header('HTTP/1.0 401 Unauthorized');
        echo '
Hey! You can\'t be here!

Try logging in first!
';
        exit;
    }elseif(md5($_SERVER['PHP_AUTH_USER']) != "04b2f0a4ad7772ca864aa569917b2d2d"){
        echo '
Wrong Username!

Only the admin username and password are accepted.
';
    exit;
    }elseif(md5($_SERVER['PHP_AUTH_PW']) != "ed972411dfcca5313ab151694af01da8"){
        echo '
Wrong Password!

For obvious reasons, we need a correct password!
';
        exit;
    }else{
        session_start();
        $_SESSION['logged_in'] = 1;
    }
}
?>

I just need to authenticate for one user.  I don't need to go into databases or anything, so I thought PHP_AUTH would be a good solution.

md5 usage may be okay on small projects, but it's generally considered weak and shouldn't be used. As of the MD5 wikipedia page:

  The security of the MD5 hash function is severely compromised.

and http://us1.php.net/md5

  It is not recommended to use this function to secure passwords, due to the fast nature of this hashing algorithm.

So perhaps you should look into PHP's other hashing methods, say hash() itself. You can read more here: http://us1.php.net/manual/en/function.hash.php

Important Edit:

I read a little bit more and I am incorrect! The hash method is okay. It's not great like bcrypt! I highly suggest you read both of these and understand them, I'm sure it will come in handy later on! https://security.stackexchange.com/questions/17421/how-to-store-salt and https://security.stackexchange.com/questions/211/how-to-securely-hash-passwords. And directly relating to PHP, http://us1.php.net/manual/en/function.password-hash.php


Calculation of lexicographic permutations Just for fun, I studied the generation of lexicographic permutations and came across the following algorithm which I transcribed into Common Lisp, as good as I can, being a hobbyist:

(defun lexicographic-permutation% (order n)
  "Returns the Nth permutation of integers from 0 upto order."
  (let* ((range (loop :for i :upto order :collect i))
         (len (1+ order))
         (permutation (make-array len :initial-contents range)))
    (loop :while (< count n)
          :for count :from 1
          :for i = order
          :for j = len
          :do
          (loop :while (>= (aref permutation (1- i))
                           (aref permutation i))
                    :do (decf i))
          (loop :while (<= (aref permutation (1- j))
                           (aref permutation (1- i)))
                :do (decf j))
          (rotatef (aref permutation (1- i))
                   (aref permutation (1- j)))
          (incf i)
          (setf j len)
          (loop :while (< i j)
                :do (rotatef (aref permutation (1- i))
                             (aref permutation (1- j)))
                    (incf i)
                    (decf j))
          :finally (return permutation))))

While it is working and fast I am unsure about the explicit side-effects using setf and incf and the structure of the loops. I know that Common Lisp is not a strict functional language and allows several paradigmas which is considered to be an advantage. But I was asking myself if the algorithm could be written in a clearer idiom in CL.

I was thinking about packing the rotatef-pattern in an extra function which could be inlined. But this is not really what I am worried about, s. above. Furthermore the usage of loop itself is fine for me, because I like its DSL.

LOOP syntax violation

There is a LOOP syntax violation: your LOOP starts with a WHILE clause and FOR clauses follow. This is not allowed according to the ANSI CL LOOP syntax. WHILE is a main clause, which follow after the variable clauses.

WHILE is not needed

The while...for... is just a for.

LOOP iterations in the sub-LOOPs are creating side-effects

I would propose to rewrite the sub-LOOPS to return values. That would make it easier to refactor them into functions.

My version of your code

I tried to keep the main loop small and remove the side-effects.
If the function calls are too much overhead, we could let Lisp inline them with a INLINE declaration.

Each of the sub-functions have a purpose and can be described and tested separately.

(defun lexicographic-permutation% (order n &aux (len (1+ order)))
  "Returns the Nth permutation of integers from 0 upto order."
  (labels ((iota (n)
             (loop :for i :upto n :collect i))
           (compute-i (permutation order)
             (loop :for i :downfrom order
                   :when (< (aref permutation (1- i))
                            (aref permutation i))
                   :do (return i)))
           (compute-j (permutation len i)
             (loop :for j :downfrom len
                   :when (> (aref permutation (1- j))
                            (aref permutation (1- i)))
                   :do (return j)))
           (swap-ij (permutation i j)
             (rotatef (aref permutation i) (aref permutation j)))
           (swap (permutation i j)
             (loop :for i1 :from i :and j1 :downfrom j
                   :while (< i1 j1)
                   :do (swap-ij permutation (1- i1) (1- j1)))))
    (let ((permutation (make-array len :initial-contents (iota order))))
      (loop :for count :from 1 :upto n
            :for i = (compute-i permutation order)
            :for j = (compute-j permutation len i)
            :do
            (swap-ij permutation (1- i) (1- j))
            (swap    permutation (1+ i) len))
      permutation)))


Generic error message factory The purpose of this code is to centralize all error / status messages to present to the user. For example, registering an account and the user email address is already registered. A status code is set then sent off to get a friendly error message to present to the user. All services will have its own set of status code and status messages which is handled by a status handler for filtering. While this is only a basic class I want to make sure that I am going in the right direction before extending.

Status codes

namespace Services.RegistrationService
    {
        public enum StatusCodes
        {
            UserAlreadyExists,
            // more to come

        }
    }

List of statuses for registration

using Framework;
using System.Collections.Generic;

namespace Services.RegistrationService
{
    public class RegistrationStatusMessages : MessageStatusHandler
    {
        public void AddErrors(List<StatusCodes> codes)
        {
            foreach(var errorCodes in codes)

            switch (errorCodes)
            {
                case StatusCodes.UserAlreadyExists: { base.add("User already exists", "EmailAddress", true); break; }
            }
        }
    }
}

Message status handler

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

namespace Framework
{
    public class MessageStatusHandler
    {
        protected List<MessageStatusHandler> _errorList;

        private string _friendlyErrorMessage;
        private bool _isError;
        private string _propertyName;

        public MessageStatusHandler()
        {
            _errorList = new List<MessageStatusHandler>();

        }

        protected void add(string friendlyErrorMessage, string propertyName, bool isError)
        {
            _errorList.Add(new MessageStatusHandler { _friendlyErrorMessage = friendlyErrorMessage, _propertyName = propertyName, _isError = isError });
        }

        public bool HasErrors()
        {
            return _errorList.Any(x => x._isError == true);
        }
    }
}

Again, it is a simple class setup that centralizes status codes, error messages for each service. Is there any way to improve on this, or am I going in the right direction?

Interesting idea, much better than having status codes / messages / magic strings wandering everywhere. This is also a good starting point for making messages loclaziable - if they are centralized, the can also be taken from resources in centralized manner.

Regarding possible improvements:

I would consider changing AddErrors(List<StatusCodes> codes) to AddErrors(params StatusCodes[] codes) - it will be easier to add just one error (what I suppose is most probable scenrio) or many with sam emethod without need to construct the List object.
It might be interesting to add some events to the Manager class - this way some important Listeneres might be configured, i.e. when status Unauthorized appears some listener just cancels all current responses and redirects user to error / login page. Or DatabaseNotResponding might have listener attached that counts occurences per minute and when there is more than one or two (what can happen in heavilly trafficked network) reports this immediatelly to the Admin. These two examples might not be a best design anyway - but I just point the purpose.
Probably you are thought about this already, but I would definitelly divide StatusCodes into specialized classes for ease of use like SystemStatusCodes, UserStatusCodes, OrderStatusCodes etc...
I think there is some error in your code in class MessageStatusHandler: protected List<MessageStatusHandler> _errorList;. I don't see a point in creating collection of handlers inside on of the exact handler type, perhaps you thought about gathering more results in one object - but I think this part needs more designing that just that simple exmaple.


Declaring arrays knowing their maximum size I know the maximum array size is 6. So what is the best practice to declare an array?

In the following code I used it like this:

<!DOCTYPE html>

My understanding is that in this case, a memory location will be allocated at initialization time itself, and so while running, it will just assign the data to the array, which will provide some better performance. But if it will not take the 6 data, some memory space will be wasted.

And another way of implementation is:

Here memory will be allocated dynamically when assigning the variable, so memory will not be wasted.

I know declaring an array with new() constructor is not a good practice (as mentioned here)), but if we know the maximum size, I want to know about the best practice. So what is the best practice here for declaring an array in JavaScript?

Interesting question,

first off, you are worrying about the wrong things. Even if the first approach provides some better performance, (which I think unlikely), it would be premature optimization.

The right way would be simply:

var marks = [40, 100, 1, 5, 25, 10];
document.getElementById("demo").innerHTML = marks[3];

Now, from a memory perspective, I am pretty sure that the only thing happening with new Array(6); is the setting of length to 6. It does not mean that memory is allocated. If you think about that, assigning memory would be silly because JS does not know what you will put in that array, it could be an integer or it could be a really long string, or a function or an object, you get the point..

My advise is to code in an idiomatic way, simply expressing what needs to be done.


Transforming full names to surname and initial I have a large data frame that I want to merge with another dataset. In order to do so I need to get the names of individuals into a certain format. The function below converts the name in 'column' to the desired format (mostly) and stores it in newColumn.

My question is, is there a better (faster and/or more pythonic) way to do this?

The main aim is to transform full names into surname and initials, such as:

Novak Djokovic = Djokovic N.
Jo-Wilfred Tsonga = Tsonga J.W.
Victor Estrella Burgos = Estrella Burgos V.
Juan Martin Del Potro = Del Potro J.M.

def convertNames(df,column, newColumn):
    df[newColumn] = 'none'

    for player in df[column]:

        names = player.split(' ')

        if len(names) == 2:

            if (len(names[0].split('-')) > 1):
                newName = names[1]+' '+names[0].split('-')[0][0]+'.'+names[0].split('-')[1][0]+'.'
            else:
                newName = names[1]+' '+names[0][0]+'.'

        elif len(names) == 3:
            newName = names[1]+' '+names[2]+' '+names[0][0]+'.'

        else:
            newName = names[2]+' '+names[3]+' '+names[0][0]+'.'+names[1][0]+'.'

        df[newColumn][df[column] == player] = newName

    return df

You are doing way too much split()-ing.  You split on ‘-‘, and if you find the length of the split is greater than 1, you split on the ‘-‘ twice more, to get the first and the second part of the hyphenated name.  Split once, and save the result in a list, and access the list elements!

You are doing too much in convertNames().  It would be better to create a convertName() method, which just processes the player name into the desired form.  Then you could call that method from convertNames().

def convertName(player):
    names = player.split(' ')

    if len(names) == 2:
        names[0:1] = names[0].split('-', 1)

    surname = min(len(names)-1, 2)

    return ' '.join(names[surname:]) + ' ' + ''.join(name[0]+'.' for name in names[:surname])

# Test data
for player in ('Novak Djorkovic', 'Jo-Wilferd Tsonga', 'Victor Estrella Burgos', 'Juan Martin Del Potro'):
    print(player, ':', convertName(player))


Is my Python tkinter application properly structured? I started a simple OOP GUI project with tkinter in Python since I've been studying the basics for a while, but I'm not sure if my code is properly structured and following the programming conventions. I haven't finished it yet but the sooner I learn the good practices the better. My goal is to make a mini game. For now it only has a main window and a character selection window with some widgets in it. (For didactic purposes, I want it to be done with an OOP approach).

import tkinter as tk
from itertools import product

# Creating main page
class MainApplication(tk.Frame):
    def __init__(self, master, *args, **kwargs):
        tk.Frame.__init__(self, master, *args, **kwargs)
        self.master = master

        # produce the set of coordinates of the buttons
        main_positions = product(range(2), range(5))

        # Creating main page widgets
        frame = tk.Frame(root, bg="#34495e")
        frame.grid()

        lb = tk.Label(frame, width=111, height=4, bg="#2c3e50", text="Champions", fg="white", font=50,
                      justify=tk.CENTER)
        lb.grid(row=0, column=0, columnspan=5, pady=(0, 50), sticky="snew")

        # Creating multiple buttons
        for i, item in enumerate(main_positions):
            button_main = tk.Button(frame, width=16, height=8, bg="#2c3e50", relief=tk.RIDGE, fg="white",
                                    justify=tk.CENTER, text=f"Champion {item[1] + 1}",
                                    command=ChampionWindow.champions_window)

            button_main.grid(row=item[0] + 1, column=item[1], pady=(0, 50))

        button = tk.Button(frame, width=30, height=3, bg="#2c3e50", relief=tk.RIDGE, text="Done", fg="white", font=20,
                           command=root.destroy)

        button.grid(row=3, columnspan=5, pady=(0, 150))

# Creating champion select window
class ChampionWindow(tk.Frame):
    def __init__(self, master, *args, **kwargs):
        tk.Frame.__init__(self, master, *args, **kwargs)
        self.master = master

    @staticmethod
    def champions_window():
        ch_window = tk.Tk()
        ch_window.title("Champion Select")
        ch_window.resizable(False, False)
        ch_window.configure(background="#192a56")

        champion_position = product(range(30), range(5))

        def scroll(ch_event):
            ch_canvas.configure(scrollregion=ch_canvas.bbox("all"))

        def select_champion():
            pass

        ch_canvas = tk.Canvas(ch_window, bg="blue", width=470, height=500)
        ch_frame = tk.Frame(ch_canvas, bg="#273c75")

        vscrollbar = tk.Scrollbar(ch_window, orient="vertical", command=ch_canvas.yview)

        ch_canvas.configure(yscrollcommand=vscrollbar.set)
        ch_canvas.grid(sticky="snew")

        vscrollbar.grid(row=0, column=3, sticky="sn")

        ch_canvas.create_window((0, 0), window=ch_frame, anchor="nw")
        ch_frame.bind("<Configure>", scroll)

        # Creating multiple buttons
        for x, itm in enumerate(champion_position):
            btn = tk.Button(ch_frame, width=12, height=6, bg="#2c3e50", relief=tk.RIDGE, fg="white", justify=tk.CENTER,
                            command=select_champion)

            btn["text"] = f"Pick champ {x+1}"
            btn.grid(row=itm[0], column=itm[1])

if __name__ == "__main__":
    root = tk.Tk()
    MainApplication(root)
    root.configure(background="#273c75")
    root.geometry("1000x570+450+200")
    root.resizable(False, False)
    root.title("Champion")
    root.mainloop()

Images:

   - The buttons open the character selection window

- In the future the buttons will pick the selected character that will be represented by an image with PIL library.

Don't create more than one instance of Tk

You should only ever create exactly one instance of Tk. If you need to make more windows, useToplevel`.

You aren't using inheritance properly

This code is wrong:

class MainApplication(tk.Frame):
    def __init__(self, master, *args, **kwargs):
        tk.Frame.__init__(self, master, *args, **kwargs)
        ...
        # Creating main page widgets
        frame = tk.Frame(root, bg="#34495e")
        ...
        lb = tk.Label(frame, ...)

First, you inherit from tk.Frame. This is good, in that when done properly you can treat MainApplication as if it were a standalone widget. However, the first thing you do after creating this frame is to create another frame as a child of root. That makes no sense since the object is itself already a frame. It completely negates any advantage of inheriting from tk.Frame.

The proper way to do this is to not create the "main page widgets" frame. Just remove that, and then put all widgets created by MainApplication inside self. The basic structure should look like this:

class MainApplication(tk.Frame):
    def __init__(self, master, *args, **kwargs):
        tk.Frame.__init__(self, master, *args, **kwargs):

        lb = tk.Label(self, ...)
        ...
            button_main = tk.Button(self, ...)
        ...
        button = tk.Button(self, ...)
        ...

if __name__ == "__main__":
    root = tk.Tk()
    app = MainApplication(root)
    # app is a frame, so add it to root. Since it's the
    # only widget in root, pack is the simplest method
    # to add it to the root window
    app.pack(fill="both", expand=True)

With that, everything in MainApplication is encapsulated inside the MainApplication frame, Nothing is leaking out into other widgets.

Don't inherit from Frame if it's not a frame

Your ChampionWindow inherits from Frame, but I can't see any reason to do so based on the way you coded it. You never use an instance of ChampionsWindow anywhere so there's no point in inheriting from anything.

Since the only purpose of ChampionWindow appears to be to hold a static method which creates a new window, I recommend inheriting from Toplevel and moving all code from the static method into a normal method called from __init__.

Don't remove control from the user

This is more of a personal preference, but I don't see any reason to remove the user's ability to resize the window with root.resizable(False, False). Usually, when I see that, it's a sign you didn't take the time to make your UI responsive. That seems to be the case here - the UI doesn't respond properly to resizing. Instead of just removing that ability, I recommend doing a few things to make the UI respond properly to resizing.


custom hash from group_by hash I am trying to create a custom hash from a table. User has many transactions. A transaction has a member_id, company_id, contributions. I am trying to create a custom hash which contains transactions(contributions) happened through a member_id to a company.

    {
    member_id: {
        company: {
            id: "",
            name: ""
        },
        transactions: [
            {
                particulars: "",
                contribution: {
                    contribution1: "",
                    contribution2: ""
                }
            }
        ]
       }
   }

I can simply use

transactions.group(:member_id, :company_id).count

to get the count, but unable to list the transactions grouped to them(Are there any ways?). So i am grouping transaction using group_by(&:member_id) and looping through the hash to generate a hash to my requirement.

Can any one review the bellow code and suggest better way to do this?

user = User.find_by(id: 123)
return {} unless user

transactions = user.transactions
return {} unless transactions.present?

data = {}
transactions.group_by(&:member_id).each do |key, value|
  transactions = []
  company = {}
  value.each do |record|
    company['id'] = record.company_id
    company['name'] = record.company_name
    particulars = record.particulars
    contribution = {}
    contribution['contribution1'] = record.contribution1
    contribution['contribution2'] = record.contribution2
    details = { "particulars": particulars, "contribution": contribution }
    transactions << details
  end
  data[key] = { "company": company, "transactions": transactions }
end

It sounds like you're trying to group transactions by member_id and company_id, though in the code you posted you are only grouping by member_id and then overwriting the company with each new transactions' company.

If you pass a block to group_by, though, you can group by more than a simple method on the object, for instance you can group by an array of attributes that match, such as:

transactions.group_by { |transaction| [transaction.member_id, transaction.company_id] }

and then you still have to loop through and format the transactions how you want, but you can clean that up as well, by using map and each_with_object:

transactions.
  group_by { |transaction| [transaction.member_id, transaction.company_id] }.
  each_with_object({}) do |((member_id, _), transactions), data|
    data[member_id] = {
      # no need to keep rebuilding the company hash, all transactions belong to
      # the same company
      company: { id: transactions.first.company_id,
                 name: transactions.first.company_name },
      transactions: transactions.map do |transaction|
        {
          particulars: transaction.particulars,
          contribution: { contribution1: transaction.contribution1,
                          contribution2: transaction.contribution2 }
        }
      end
    }
  end

This can be cleaned up even more, by moving the generation of these hashes to presenters but that's just moving the code around at this point, regardless you still need to go through all the transactions and present them as the hash, so the loop is unavoidable.


Macro for allocation in C As we all know, the syntax of allocating memory is a bit clunky in C. The recommended way is:

 int *p;
 int n=10;
 p = malloc(n*sizeof *p);

You can use sizeof (int) instead of sizeof *p but it is bad practice.

I made a solution to this with a macro:

#define ALLOC(p,n) do{ *p=malloc(n*sizeof **p); } while(0)

This get called this way:

 int *p;
 int n=10;
 ALLOC(&p, n);

The reason I want to pass the address of p rather than p, is to offer a bit of protection. When you call a function with foo(x), then foo can never change the value of x. If you're not passing the address, the argument will not change. (Sure there are ways around that too.)

Any comments?

p = malloc(n*sizeof *p);

This is dangerous if n gets large, because the multiplication could overflow. After the overflow, too little memory has been allocated but code will continue without detecting the error.

This is especially dangerous if n comes from untrusted source, such as some file format or remote user. Then it gives attacker easy way to overwrite parts of memory with exploit code.

The easy safe solution is to use calloc() which will detect the overflow (at least on most common libc implementations). If you really need to use malloc() for some reason, you can check for overflow separately.


Transform a vector with the FFI in Haskell In Haskell, I want to apply a transformation to each element of a vector using the FFI. I am rather ignorant in C, I am more or less novice in Haskell and this is the first time I use storable vectors in Haskell.

This is what I did, taking the transformation \x -> x+1 as an example. The C file:

double* bar (int n, double *arr){
    double* y;
    y = (double*) calloc(n, sizeof(double));
    for(int i=0; i<n; i++)
      y[i] = arr[i]+1;
    return y;
}

The Haskell code:

import Foreign.C.Types
import Foreign (newForeignPtr)
import qualified Data.Vector.Storable as SV

foreign import ccall "bar" c_bar :: CInt -> Ptr CDouble -> Ptr CDouble

haskellBar :: SV.Vector CDouble -> IO (SV.Vector CDouble)
haskellBar sv = do
    let n = SV.length sv
    ptr <- SV.unsafeWith sv (return . (c_bar (fromIntegral n)))
    fptr <- newForeignPtr_ ptr
    return $ SV.unsafeFromForeignPtr0 fptr n

This works, but as I'm almost ignorant in C and new to storable vectors, I really don't know whether it's a good way and I would appreciate some advice.

I'm also new to the FFI by the way, and I don't know if I should write foreign import ccall safe/unsafe (I have not understood the role of safe/unsafe), or whether safe/unsafe is useless here.

calloc can fail

We start with the C code first, since it's somewhat shorter. Aside from the name bar, we see one problem: calloc might return 0. Therefore, we should check that:

double* bar(int n, double *arr){
    double* y;
    y = (double*) calloc(n, sizeof(double));

    if(!y) {
      return 0;
    }

    for(int i=0; i<n; i++){
      y[i] = arr[i]+1;
    }
    return y;
}

Who frees the memory?

However, there is still one problem. A user needs to know how add_one conjured that memory in order to use the correct free variant. That's something that can be handled with documentation. Still, there is no need for us to call calloc, we can just give the responsibility to the user:

double* add_one(double * dest, const double * src, size_t count){
    for(size_t i = 0; i < count; ++i) {
      dest[i] = src[i] + 1;
    }
    return dest;
}

Note that this uses const to make sure that we don't overwrite  src by accident. We also use size_t to be ready for any kind of size the user might throw at us. The user is now responsible to allocate the memory, so they will also know how to free it.

We return the pointer because we followed memcpy's interface, but we could also just make it a void function instead.

Haskell part

Let's go to Haskell. We can see that there is something off: you import newForeignPtr, but you use newForeignPtr_. That's probably since newForeignPtr needs the aforementioned free function. That's in Foreign.Marshal.Alloc:

fptr <- newForeignPtr finalizerFree ptr

But before we delve into that, let us take a step back and look at the foreign import:

foreign import ccall "bar" c_bar :: CInt -> Ptr CDouble -> Ptr CDouble

Unfortunately, your c_bar does not have that type. Remember, whenever you see A -> B in a function, you should always get the same B if you've used the same A:

all (== foo a) [foo a,foo a,foo a,foo a,foo a] -- all the same

But that does not hold for your function. It has a side-effect: every call will return another Ptr CDouble. We have to put it in IO:

foreign import ccall "bar" c_bar :: CInt -> Ptr CDouble -> IO (Ptr CDouble)

Note that you can use the unsafe keyword here. It basically means that bar will never call any Haskell function and will stay in its C world. With this, we would end up with

haskellBar :: SV.Vector CDouble -> IO (SV.Vector CDouble)
haskellBar sv = do
    let n = SV.length sv
    ptr <- SV.unsafeWith sv (c_bar (fromIntegral n))
    fptr <- newForeignPtr finalizerFree ptr
    return $ SV.unsafeFromForeignPtr0 fptr n

Which does not differ from your variant too much.

The add_one variant is similar:

foreign import ccall unsafe "add_one" c_addOne :: Ptr CDouble -> Ptr CDouble -> CSize -> IO (Ptr CDouble)

It's in IO, it uses unsafe to be a little bit faster (but must not call Haskell from C), and it uses the correct types. It's wrapper is almost the same, but this time, we allocate the memory in Haskell:

addOne :: SV.Vector CDouble -> IO (SV.Vector CDouble)
addOne sv = do
   fptr <- mallocForeignPtrArray n
   SV.unsafeWith sv $ \v -> withForeignPtr fptr $ \f -> c_addOne f v (fromIntegral n)
   return $ SV.unsafeFromForeignPtr0 fptr n
 where
   n = SV.length sv

The mallocForeignPtrArray makes sure that the memory gets freed correctly.


A simple javascript plugin architecture I've implemented the following code which allows me to bolt modules of code on to my core library via the prototype chain. It's quite prescriptive so any user creating a module has to create an init method and also has to define a queue method:

(working fiddle: http://jsfiddle.net/wG9Pv/)

//Constructor
test = function() {
    this.tester = 'The instance was successfully created';
};

//A core method
test.prototype.amethod = function() {

};

//Extend method which facilitates the module system
test.extend = function(name,plugin) {
    test.prototype[name] = plugin.init;
    //later do something with the queue method that's been fed in via the 'plugin' object
};

//Add a module
test.extend('method1', {

    init: function(val) {
        console.log(this.tester);
    },

    queue: function() {

    },

    render: function() {

    }
});

//add another module
test.extend('method2',{

    init: function() {
        console.log('Init of the plugin has been fired');
    },

    draw : function() {}
});

var test = new test();
test.method1('boom!');
test.method2();

Now this all works well but is this a good way of achieving it?

Looks fine to me,

I would consider making extend a property of prototype though.

For nitpickings, I think

function Test() {
    this.tester = 'The instance was successfully created';
};

is preferred to

test = function() {
    this.tester = 'The instance was successfully created';
};

This ways developers will know to use new when using test.
Please read Learning JavaScript Design Patterns for the cons of using the new keyword.

You probably want to store the entire plugin object provided to extend, not just keep init.

You probably want to throw an exception if the plugin does not have an init  property.

I am not sure if you realize that method1 and method2 are at the end of the script also "core methods"?


Finds customer who live within 100km and output them to a text file I had an interview task where I had to output customers who live 100km within a particular latitude and longitude and then output them to a file with their username and ID. The customer file looks like the following:
{"latitude": "52.986375", "user_id": 12, "name": "Christina McArdle", "longitude": "-6.043701"}
{"latitude": "51.92893", "user_id": 1, "name": "Alice Cahill", "longitude": "-10.27699"}
{"latitude": "51.8856167", "user_id": 2, "name": "Ian McArdle", "longitude": "-10.4240951"}
{"latitude": "52.3191841", "user_id": 3, "name": "Jack Enright", "longitude": "-8.5072391"}

Here is the code to implement the calculation for the great-circle-distance
/**
 * solution accuired from
 * https://www.movable-type.co.uk/scripts/latlong.html
 */

const greatCircleDistanceCalc = (lat1, lon1, lat2, lon2) => {
  const R = 6371; // Radius of the earth in km
  const dLat = deg2rad(lat2 - lat1); // deg2rad below
  const dLon = deg2rad(lon2 - lon1);
  const a =
    Math.sin(dLat / 2) * Math.sin(dLat / 2) +
    Math.cos(deg2rad(lat1)) *
      Math.cos(deg2rad(lat2)) *
      Math.sin(dLon / 2) *
      Math.sin(dLon / 2);
  const c = 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1 - a));
  const d = R * c; // Distance in km
  return d;
};

const deg2rad = (deg) => {
  return deg * (Math.PI / 180);
};

module.exports = {
    greatCircleDistanceCalc,
};

Here is the main app.js where the calculation is used and a customer file is read and a new file is outputted.
const fs = require("fs");
const { greatCircleDistanceCalc } = require("./greatCircleDistanceCalc.js");

const customerFile = fs.readFileSync("./customers.txt", "utf-8"); //Read in file
const customerArr = customerFile.split("\n").map((s) => JSON.parse(s)); //Convert file into array of objects

const dublinOffice = {
  latitude: "53.339428",
  longitude: "-6.257664",
};

const invitedArr = [];

//Sort data in ascending order
const sortedCustomers = customerArr.sort((a, b) => {
  return a.user_id - b.user_id;
});

const closestCustomers = (sortedCustomers, arr) => {
  for ({ latitude, longitude, user_id, name } of sortedCustomers) {
    if (
      greatCircleDistanceCalc(
        dublinOffice.latitude,
        dublinOffice.longitude,
        latitude,
        longitude
      ) <= 100
    ) {
      invitedArr.push(`${name}:${user_id}`);
    }
  }
  writeInvitedCustomer(arr);
};

const writeInvitedCustomer = (arr) => {
  const writeStream = fs.createWriteStream("Output.txt");
  const pathName = writeStream.path;

  arr.forEach((value) => writeStream.write(`${value}\n`));
  writeStream.on("finish", () => {
    console.log(`wrote all the array data to file ${pathName}`);
  });

  writeStream.on("error", (err) => {
    console.error(`There is an error writing the file ${pathName} => ${err}`);
  });

  writeStream.end();
};

closestCustomers(sortedCustomers, invitedArr);

Overall I am happy with the solution but I think the only thing is I am not sure how to write a test for this.
As always any feedback and suggestions are much welcome.

Generally I find the code very unstructured making reading it a bit confusing:
First you execute some code outside of a function to read the data from a file, then you define a constant (dublinOffice), then you define a variable (invitedArr), but don't use it in the next block of code, then you sort the data, then you define two functions, and finally execute one of the functions.
Further more the function closestCustomers both gets invitedArr passed an argument (and uses it as such) and uses it directly as a global variable.
Personally I'd have the main code calling functions first (after defining constants) and then define all functions at the end of the source code using function so that they are hoisted. Other people may define the functions first and then use them, but that is a matter of taste, as long it's not all mixed up.
More, smaller functions would also be helpful. Reading the data should be in its own function and closestCustomers does too much (filtering, converting the objects to strings, and calling writeInvitedCustomer). This would also make testing easier.
Using the array methods filter() and map() would help the code be more readable. Personally, I'd start with something like:
const inputFileName = "customers.txt";
const outputFileName = "output.txt";

const dublinOffice = {
  latitude: "53.339428",
  longitude: "-6.257664",
};
const maxDistance = 100;

const invitedCustomers = readCustomers(inputFileName)
  .filter(withinKms(dublinOffice, maxDistance ))
  .sort(userIdComparator)
  .map(formatCustomerForOutput); // `map` may be considered to be moved inside `writeInvitedCustomers`

writeInvitedCustomers(invitedCustomers, outputFileName);

function readCustomers(filename) {
  // ...
}

function withinKms(location, kms) {
   return (customer) => greatCircleDistanceCalc(customer.latitude, customer.longitude, location.latitude, location.longitude) <= kms;
}

function userIdComparator({user_id: userId1}, {user_id: userId2}) {
   return userId1 - userId2;
}

function formatCustomerForOutput({name, user_id}) {
   return `${name}:${user_id}`;
}

// etc.

One other point: I'm not familiar with node's WriteStream, but it looks wrong for me to assign the event handlers after using it by calling its write method. Can't write cause an error?
NB: This is probably not your choice, if this was an interview task, but I find the file format for customers.txt (multiple JOSN objects in one file) strange. A straight forward JSON array would make more sense.


The Boyer-Moore Majority Vote Algorithm I have started learning Java recently and was looking into some easy algorithms.
I found the Boyer Moore voting algorithm here.

I am trying to get better at writing good code for my solutions. Please give me your suggestions.

import java.util.Scanner;

public class MajorityVoteAlgorithm {

    public static void main(String[] args) {

        Scanner keyboard = new Scanner(System.in);
        int size = keyboard.nextInt(); // define size of array
        int[] array = new int[size];

        // initialize array
        for (int i = 0; i < array.length; i++) {
            array[i] = keyboard.nextInt();
        }

        majorityelement(array);

    }

    private static void majorityelement(int[] array) {
        int count = 0;
        int candidate = 0;
        for (int i = 0; i < array.length; i++) {
            if (count == 0) {
                candidate = array[i];
                // set the count as 1
                count = 1;
                continue;
            } else if (candidate == array[i])
                count++;
            else {
                count--;
            }
        }

        if (count == 0) {
            System.out.println("No majority element found");

        } else {
            // set the count to zero to count the occurrence of the candidate
            count = 0;
            for (int i = 0; i < array.length; i++) {
                if (candidate == array[i])
                    count++;
            }

            if (count > array.length / 2)
                System.out.println("Majority element is " + candidate);
            else
                System.out.println("No majority element found");
        }

    }
}

int[] is too strong a requirement. The code does not do any random access. Consider changing an argument from array to stream (this way you may process data not fitting in memory).
The method does two (technically) unrelated jobs: finding the maybe dominant element, and verification that the element is indeed dominant. I recommend splitting it into two methods, for the following reasons:

Each loop does an important job and deserves a name.
If it is guaranteed that the dominant element exists, verification is a waste of time.
If it is guaranteed that the dominant element exists, the requirements can be relaxed even more to an input iterator.

Do not print anything from the algorithm. Let main handle results.


Simple FIFO Job list in Java For the first time I have to play with Threads in java.

Basically, my code is a simple FIFO job queue.
The main thread regularly puts jobs in the queue wich need to be executed.

The code below is a striped down version of what I am doing :

public class JobPerformer implements Runnable {
    private final ArrayList<Job> jobList = new ArrayList<Job>();
    private final AtomicBoolean threadStop = new AtomicBoolean(false);
    private Thread thread;
    public JobExecuter(){
       thread = new Thread(this)
       thread.start();
    }

    public void perform(Job job, bool async){
        if(async){
            jobList.add(job);
            jobList.notifyAll();
        }
        else{
            job.perform();
        }
    }

    public void perform(Job job){
        this.perform(job, false);
    }

    public void shutdown(){
         threadStop.set(true);
         if(thread.getState() == Thread.State.WAITING){
             thread.interrupt();
         }
    }

    public void run(){
        while(! threadStop.get()){
            if(jobList.isEmpty()){
                try{
                    jobList.wait();
                } catch (InterruptedException ex) {}
            }
            jobList.get(0).perform();
            jobList.remove(0);
        }

        // Finish the jobs still in the queue
        while(!jobList.isEmpty()){
             jobList.get(0).perform();
             jobList.remove(0);
        }
    }
}

I have two questions :

I use wait() and notifyAll() to avoid polling. But nowhere do I synchronize. Since it's a FIFO queue, threads are just adding jobs at the end or reading at the start. I don't see any conflict that would make me say "Hey, I have to make sure that during this op. only one thread can be working on this object", is there a possible conflict I am overseeing that would require me to synchronize anyway ?
I am using an "Atomic boolean" because of this question : Multithreading correctly done? What was wrong with using a normal boolean ?

Why don't you use the classes provided in java.util.concurrent that can handle these requirements? Or is this written for practice?

There is still problem if you don't synchronize, because the ArrayList.add() method is not thread-safe thus it can happen that two threads trying to add a new job into the queue nearly the same instant (to the same slot in the array), then it is possible that the one adding later will overwrite the job added by the first thread.

Using normal boolean does not guarantee, that if one thread writes the value, and another accesses immediately after that then it will see the new value. You can use volatile with boolean as well, as in mentioned in the answer you linked. See an explanation for volatile

Or refer to Java Concurrency in Practice, pretty good book.


Time-delayed function queue I recently had an interesting requirement in a test environment where I needed to simulate actions at varied (random) intervals in a node back-end. They needed to be sequentially executed, but queued so I could fire and move on. Using a typical timeout/interval here wouldn't work, since if I set a timeout for fn1 to execute after 500ms and then set a timeout for fn2 to execute after 250ms, fn2 would execute before fn1, which was not desired.

To solve this, I created a queue object that executes queued commands sequentially. It works perfectly for my scenario, but I was wondering if there were any opportunities for improvement. I realize I should probably be checking input parameters to make sure the passed in function is indeed a function, as well as the delay being a number. Maybe there are other opportunities for improvement. Let me know!

var TimerQueue = (function () {
    var timers = [];
    var running = false;
    var currentInterval;
    var currentTimer;

    this.addTimer = function (fn, delay) {
        timers.push({fn: fn, delay: delay});

        function exec() {
            currentTimer.fn();
            clearInterval(currentInterval);
            if (timers.length > 0) {
                currentTimer = timers.shift();
                currentInterval = setInterval(exec, currentTimer.delay);
            } else {
                running = false;
            }
        }

        if (!running) {
            running = true;
            currentTimer = timers.shift();
            currentInterval = setInterval(exec, currentTimer.delay);
        }
    };

    this.clear = function () {
        if (currentInterval) {
            clearInterval(currentInterval);
        }
        timers = [];
        running = false;
    };

    return this;
});

Usage:

var queue = new TimerQueue();
queue.addTimer(function () { console.log('1st') }, 500);
queue.addTimer(function () { console.log('2nd') }, 250);

I would do it as a singleton, so that the core functionality can be protected and private. All that is exposed is the Interface, .add(),start(),clear(),and the semaphore .done

Using prototype is only an advantage if the object that it belongs to is instanced many times. If you only create the object a few times, or once then adding to the prototype is actually more work for the javascript engine.

I also added a start method so that the queue could be created and started when needed. Once started you can add to the queue as long as done === false

// singleton. Assuming you only need one copy of timed queue
const timedQueue = (function(){
    var API;                // internal referance to interface
    const queue = [];       // array to hold functions
    var task = null;        // the next task to run
    var tHandle;            // To stop pending timeout
    function next(){  // runs current scheduled task and  creates timeout to schedule next
        if(task !== null){          // is task scheduled??
            task.func();            // run it
            task = null;            // clear task
        }
        if(queue.length > 0){       // are there any remain tasks??
            task = queue.shift();   // yes set as next task
            tHandle = setTimeout(next,task.time) // schedual when
        }else{
            API.done = true;
        }
    }
    return API = {
        add : function(func,time){
            queue.push({func : func, time: time});
        },
        start : function(){
            if(queue.length > 0 && API.done){
                API.done = false;   // set state flag
                tHandle = setTimeout(next,0);
            }
        },
        clear : function(){
            task = null;            // remove pending task
            queue.length = 0;       // empty queue
            clearTimeout(tHandle);  // clear timeout
            API.done = true;        // set state flag
        },
        done : true,
    }
})();

// usage.
function test1(){
    console.log("Test1 run");
}
function test2(){
    console.log("Test2 run");
}

timedQueue.add(test1,1000)
timedQueue.add(test2,1000)
timedQueue.add(test1,3000)
timedQueue.add(test2,100)
timedQueue.start();


Displaying all application errors in a view I'm learning ASP.NET Core with MVC pattern and I didn't find anything useful related to this argument.

Actually I'm looking for a way to handle all the application errors inside a single View. The first thing that I did was create the View for display the error, this is the design (pretty simple though):

<h2>@(ViewBag.ErrorMessage == null ? "An error happened" : "" + @ViewBag.ErrorMessage + "")</h2>

and this is the Error controller:

public class ErrorController : Controller
{
    [HttpGet]
    public IActionResult Index(string errorMessage)
    {
        ViewBag.ErrorMessage = errorMessage;
        return View();
    }
}

Essentially, I used the ViewBag for valorize a property called ErrorMessage which contains the parameter errorMessage.

Let me show an example of this logic for the email confirmation:

public async Task<IActionResult> ConfirmEmail(string userId, string token)
{
    if (userId == null || token == null)
    {
        return RedirectToAction("Index", "Error");
    }

    var user = await _userManager.FindByIdAsync(userId);

    if (user != null)
    {
       if (user.EmailConfirmed)
       {
            return RedirectToAction("Index", "Error", new { errorMessage = "Email already confirmed" });
       }

       IdentityResult result;

       try
       {
           result = await _userManager.ConfirmEmailAsync(user, token);
       }
       catch (InvalidOperationException ex)
       {
           return RedirectToAction("Index", "Error", new { errorMessage = ex.Message });
       }

       if (result.Succeeded)
       {
           //TODO: Send another email
           return View("ConfirmEmail", user);
       }
    }

    return RedirectToAction("Index", "Error", new { errorMessage = "Utser not found" });
    }

Now, I'm not an expert yet of ASP.NET Core so I don't know if my practice is good enough for a production environment, someone could maybe propose a better way or improve my solution?

While I see no problem design-like or code-like, in the point of view of the user, It can be frustrating.

I guess the method ConfirmEmail is called from a form.

So, imagine how frustrating it is if you fill a form and if there is any error in this form, it redirects you to the Error View ?

Imo, Error display should be handled by Javascript/Html/Css on the same page which thrown the error to avoid losing any data from user's pov. Like, displaying a notification or even an error message below the field.

How you display the error should be specific of the error.


Recognising and comparing poker hands I wrote a little program that recognises poker hand combinations and compares hands if the ranks of both hands are the same. I had tested my program on possible combinations for each rank and the outcomes were correct. I would like to know how it is possible to improve the design of my code and maybe cut a few steps.

I am also planning on expanding the whole program a bit further e.g. recognising the best combination from two initial cards and five cards on the table, so making code programmer-friendly is a priority for me.

vector<string> cards{"6H", "7H", "8H", "9H", "TH"}; //Or any other hand

char rank = 0;
vector<char> vals;

//Extracting values of each card without a suit
for(vector<string>::iterator it = cards.begin(); it != cards.end(); ++it)
{
    if((*it)[0] == 'T')vals.push_back(58);
    else if ((*it)[0] == 'J')vals.push_back(59);
    else if ((*it)[0] == 'Q')vals.push_back(60);
    else if ((*it)[0] == 'K')vals.push_back(61);
    else if ((*it)[0] == 'A')vals.push_back(62);
    else vals.push_back((*it)[0]);
}

// Checking for pairs, triples and quadruples
sort(vals.begin(), vals.end(), greater<char>());
for(vector<char>::iterator it = vals.begin(); it != vals.end(); it)
{
    int counter = 0;
    auto nx = next(it);
    while(*it == *nx && nx != vals.end())
    {
        ++counter;
        ++nx;
    }
    switch(counter)
    {
        case 1:
            if(rank == 0)
            {
                rank = 1;
                rotate(vals.begin(), it, nx); //Pairs are compared first
            }
            else if(rank == 1)
            {
                rank = 2;
                rotate(vals.begin()+2, it, nx); //2nd pairs are compare after 1st pairs
            }
            else if(rank == 3)rank = 6;
            break;
        case 2:
            if(it != vals.begin())rotate(vals.begin(), it, nx); //Tripples are always compared first
            if(rank == 0) rank = 3;
            else if(rank == 1)rank = 6; //Full House
            break;
        case 3:
            rank = 7;
            if(it != vals.begin())rotate(vals.begin(), it, nx);
    }
    it = nx;
}

//Checking for flash and street
if(cards[0][1] == cards[1][1] && cards[1][1] == cards[2][1] && cards[2][1] == cards[3][1] && cards[3][1] == cards[4][1])rank = 5;
if(vals[0] == vals[1]+1 && vals[1] == vals[2]+1 && vals[2] == vals[3]+1 && vals[3] == vals[4]+1)
{
    if(rank == 0)rank = 4;
    else rank = 8;
}
cout << "RANK: " << static_cast<int>(rank) << endl;

Overall, I can't quite tell how this is supposed to be structured.  It looks like it all belongs to a single function which has been omitted.  In either case, different things are happening here, so they should be separated into additional functions as such.

For instance, each section you've already preceded with a comment should have a function.  They may all do card operations, but their purposes still differ.  If someone needed to find a certain area to examine, then that would take more time to find, even with comments.
Since you're already using C++11, you can change all of your vector::iterators to auto, which will help keep your code simpler.

Better yet, you can just use a range-based for loop:

for (auto& it : cards) {}

This isn't very idiomatic:

(*it)[0]

Since this vector contains std::strings, you can use std::string::front() to acquire the first character from an element:

it.front();

Note: the . operator here may only work if you follow my second suggestion.  Otherwise, you may need to use the -> operator.
This is really hard to read and maintain:

if(cards[0][1] == cards[1][1] && cards[1][1] == cards[2][1] && cards[2][1] == cards[3][1] && cards[3][1] == cards[4][1])rank = 5;

I had to scroll all the way to the right to see that it does indeed perform an action.  While it's okay to have if statements on a single line, doing so with a long line is a bad idea.  In this case, put the rank = 5; on a separate line with curly braces, just like with your other code.
This is also hard to read:

if(vals[0] == vals[1]+1 && vals[1] == vals[2]+1 && vals[2] == vals[3]+1 && vals[3] == vals[4]+1)

With this many conditions, you should at least list them vertically in some way so that each one is clearly visible.

(This advice also applies to the previous bulletpoint.)

Moreover, do keep in mind that long vertical sections aren't as bad as long horizontal sections.  If, say, someone were to read this on a terminal with line-wrapping, then your code's readability for them would decrease even more.


Optimising multiple streams to single loop I am trying to find the best way to optimise the converters below to first follow the flow I call convertAndGroupForUpdate, which triggers the conversions and relevant mappings.

Any help to optimise this code would be massively appreciated.

public List<GroupedOrderActionUpdateEntity> convertAndGroupForUpdate(List<SimpleRatifiableAction> actions) {
    List<GroupedOrderActionUpdateEntity> groupedActions = new ArrayList<>();

    Map<String, List<SimpleRatifiableAction>> groupSimple = actions.stream()
        .collect(Collectors.groupingBy(x -> x.getOrderNumber() + x.getActionType()));

    groupSimple.entrySet().stream()
        .map(x -> convertToUpdateGroup(x.getValue()))
        .forEachOrdered(groupedActions::add);

    return groupedActions;
}

public GroupedOrderActionUpdateEntity convertToUpdateGroup(List<SimpleRatifiableAction> actions) {
    List<OrderActionUpdateEntity> actionList = actions.stream().map(x -> convertToUpdateEntity(x)).collect(Collectors.toList());

    return new GroupedOrderActionUpdateEntity(
        actions.get(0).getOrderNumber(),
        OrderActionType.valueOf(actions.get(0).getActionType()),
        actions.get(0).getSource(),
        12345,
        actions.stream().map(SimpleRatifiableAction::getNote)
            .collect(Collectors.joining(", ", "Group Order Note: ", ".")),
        actionList);
}

public OrderActionUpdateEntity convertToUpdateEntity(SimpleRatifiableAction action) {
    return new OrderActionUpdateEntity(action.getId(), OrderActionState.valueOf(action.getState()));
}

Specifying a downstream Collector for Collectors.groupingBy

There is an alternative Collectors.groupingBy(Function, Collector) method that lets you specify further steps that you want to do with the intermediary List<SimpleRatifiableAction> values after the grouping.

Then, with a bit of renaming, some help from method references, plus some convenience methods like having a SimpleRatifiableAction.getKey():

public String getKey() {
    return getOrderNumber() + getActionType();
}

You can have a method that reads:

// dropping method visibility modifier for brevity
List<GroupedOrderActionUpdateEntity> process(List<SimpleRatifiableAction> actions) {
    return new ArrayList<>(actions.stream()
            .collect(Collectors.groupingBy(SimpleRatifiableAction::getKey,
                    Collectors.collectingAndThen(Collectors.toList(),
                                                  this::createUpdateEntity)))
            .values());
}

Looping once, aggregating multiple values

Inside convertToUpdateGroup(List), now renamed as createUpdateEntity(List), you are streaming twice on the List argument. While this shouldn't be an issue for most cases, there is still an option to just loop once should it be one of the remaining places to optimize (hopefully with some runtime analysis/micro-benchmarking to prove it).

This is achieved by creating the StringJoiner instance yourself (instead of relying on Collectors.joining()). To avoid similar List.get(0) calls, you can also get a reference to it once.

Putting it altogether:

// dropping method visibility modifier for brevity
GroupedOrderActionUpdateEntity createUpdateEntity(List<SimpleRatifiableAction> actions) {
    SimpleRatifiableAction first = actions.get(0);
    StringJoiner joiner = new StringJoiner(", ", "Group Order Note: ", ".");
    List<OrderActionUpdateEntity> updateEntities = new ArrayList<>();
    actions.forEach(v -> {
        joiner.add(v.getNote());
        updateEntities.add(v.createUpdateEntity());
    });
    return new GroupedOrderActionUpdateEntity(
            first.getOrderNumber(),
            OrderActionType.valueOf(first.getActionType()),
            first.getSource(),
            12345,
            joiner.toString(),
            updateEntities);
}

SimpleRatifiableAction.createUpdateEntity() is also another convenience method that you can consider:

public OrderActionUpdateEntity createUpdateEntity() {
    return new OrderActionUpdateEntity(getId(), OrderActionState.valueOf(getState()));
}


Each element is the sum of itself with the next element of a List: now do this point-free in Haskell Here it is the best I can do (I am a noob using Haskell):

map (\(y, z) -> y + z) (zip x (tail x))

I'm looking for a point-free solution.

Pointfree, eh? Let's ask lambdabot.

<DanBurton> @pl map (\(y, z) -> y + z) (zip x (tail x))
<lambdabot> zipWith (+) x (tail x)

Assuming that x is simply the input to this "function"

<DanBurton> @pl \x -> map (\(y, z) -> y + z) (zip x (tail x))
<lambdabot> map (uncurry (+)) . ap zip tail

Personally I'd go with the former; I'm not a fan of Lambdabot's gratuitous use of uncurry and ap.


Reading datasets from files to lists I'm working on a simple program and am trying to make it faster.

import time
import numpy as np
import matplotlib.pyplot as plt
import pp

first_file = open("ac_data.dat", 'r') #real dataset includes about 20000 terms
res_file = open("res.dat", 'w')
times_file = open("times.dat", 'a')

#times = [0.000000, 0.000500, 0.001000, 0.001500, 0.002000, 0.002500, 0.003000, 0.003500, 0.004000, 0.004500, 0.005000, 0.005500, 0.006000, 0.006500, 0.007000, 0.007500, 0.008000, 0.008500, 0.009000, 0.009500]
#magn = [-13.876622, -10.014824, -16.356894, -11.639914, -13.103313, -14.335239, -12.250072, -10.727098, -8.701272, -9.632907, -9.673712, -10.541722, -14.075446, -13.097790, -12.495679, -10.322924, -14.979391, -14.895666, -11.874325, -9.287736]

times = []
magn = []

for i in first_file:
  dat = [float(j) for j in i.split()]
  times.append(dat[0])
  magn.append(dat[1])

length = len(magn) #supposed to be equal 20000 for original data

#autocorrelation function
def rxx_func(amp):
    N = len(amp)
    rxx = [0]*N

    for m in xrange(N):
        for n in xrange(N-m):
            rxx[m]+=amp[n]*amp[n+m]
    return rxx

#just prove with in-built
def autocorr(x):
    result = np.correlate(x, x, mode='full')
    return result[result.size/2:]

#Parallel or ordinary?
answer = int(raw_input('Non-arallel = 0, parallel = 1 '))

if answer == 0:
    print 'Non-parallel calc was started'
    start = time.time()

    rxx = rxx_func(magn)

    end = time.time()
    calc_time = end - start

    time_string = 'Non-parallel: N = %i    T = %f\n'%(length,calc_time)

else:
    print 'Parallel calc was started'
    ppservers = ()
    ncp = 4
    job_server = pp.Server(ncp, ppservers=ppservers)
    print "Starting pp with", job_server.get_ncpus(), "workers"

    arg_n = tuple(magn)
    job = job_server.submit(rxx_func, (arg_n,), (), ())

    start = time.time()

    rxx = job()
    job_server.print_stats()

    end = time.time()
    calc_time = end - start

    time_string = 'Parallel with %i CPUs: N = %i    T = %f\n'%(ncp,length,calc_time)

print (" \n Task for %i terms takes %f seconds for calc" %(length, calc_time))
print (" Max value of Autocorrelation func achieves %f" %(max(rxx)))
print (" And it'll be normalized to 50 \n")

#normalization to 50
norm_const = 50/max(rxx)

proves = autocorr(magn)

for k in xrange(length):
    rxx[k] = rxx[k]*norm_const
    proves[k] = proves[k]*norm_const

#plotting
plt.plot(times, magn)
plt.plot(times, rxx)
plt.plot(times, proves,'*')

plt.show()

for j in xrange(length): #saving results
    st = '%f    %f    %f\n'%(times[j], rxx[j], proves[j])
    res_file.write(st)

times_file.write(time_string) #saving calc times for comparsion

first_file.close()
res_file.close()
times_file.close()

The program reads datasets from files to lists. Typical datasets look like commented #magn and #time. Real datasets will consist of 20000 lines or more.

I've trying use the "parallel Python" package, but it runs even more slower than non-parallel code.

For example, some results:

Non-parallel: N = 20000    T = 74.530000 sec
Parallel with 2 CPUs: N = 20000    T = 80.229000 sec
Parallel with 4 CPUs: N = 20000    T = 80.594000 sec

And I can't figure out why. Maybe I don't understand how it must be used.

I don't do much python so I might be wrong but to my understanding, this:

job = job_server.submit(rxx_func, (arg_n,), (), ())

just submits a single job but it doesn't automatically parallelize it. If you want to process the input in parallel you need to submit n jobs each working on 1-nth of the input and then combine the results. I think your parallel execution code should look something like this:

slice_size = len(magn) / ncp;

# submit a job for each chunk
jobs = [job_server.submit(rxx_func, (magn,(i-1)*slice_size, slice_size), (), ()) for i in xrange(ncp)]

# combine the results into one list, requires #import itertools
rxx = list(itertools.chain.from_iterable([job() for job in jobs]))

You will have to change your rxx_func to accept a start index and a count which defines for how many items it is responsible:

def rxx_func(amp, start_index, count):
    N = len(amp)
    rxx = [0]*count

    for m in xrange(start_index, start_index + count - 1):
        for n in xrange(N-m):
            rxx[m]+=amp[n]*amp[n+m]
    return rxx

I'm sure there is plenty which can be optimized in the above.


Spring Boot should we always use interfaces even with one implementation quick question. I'm using Spring Boot and I created e.g. this class
public interface ProductService {

    Page<Product> getPage(Pageable pageable);

}

@Service
public class ProductServiceImpl implements ProductService {

    @Autowired
    private ProductRepository productRepository;

    @Override
    public Page<Product> getPage(String name, Pageable pageable) {
        return productRepository.findAll(pageable);
    }
}

And this interface has only one implementation. And what is better to use interface with one implementation and inject interface or just create a class without interface and inject class?

The answer to this question may vary depending on the opinions regarding the various so-called "good practices".
If we take into account the dependency inversion principle (In SOLID principles), then we should always aim for our classes to depend on abstractions and not on implementations.
However we could also take into account the "KISS" principle (Keep it stupidly simple) and in this way I could tell you that if your application is not very large or complex, then do not use an interface.
Answering your question: "what is better to use interface with one implementation and inject interface or just create a class without interface and inject class?"
I recommend using an interface even if it has only one implementation, so in the future if the implementation changes, the class that depends on said interface will not be affected by said change, unless the interface signature changes as well.


Iterating over an object that contains arrays I have an object that has an array inside with another object inside.  I iterate over the object and array to print out the first name and last name of the people inside the array of objects.  I am wondering if there is a better way to do this.
Without having to use obj[i].first_name + " " + obj[i].last_name

Here is my code

    var users = {
 'Students': [
     {first_name:  'Michael', last_name : 'Jordan'},
     {first_name : 'John', last_name : 'Rosales'},
     {first_name : 'Mark', last_name : 'Guillen'},
     {first_name : 'KB', last_name : 'Tonel'}
  ],
 'Instructors': [
     {first_name : 'Michael', last_name : 'Choi'},
     {first_name : 'Martin', last_name : 'Puryear'}
  ]
 }
    for(item in users)
    {
        console.log(item);
        var obj = users[item]
        for(var i = 0; i < obj.length; i++)
        {
            var obj2 = obj[i];
      var name = "";
            for(j in obj2)
            {
          name += obj2[j] + " ";
            }
      console.log(name);
      name ="";
        }
    }

To directly answer your question, yes, there is a better way: template literals and using array methods.

In the following snippet, it uses Object.keys and array.reduce to concatenate all arrays in the object into one array, array.map to generate an array of name strings from that array, and array.forEach to ultimately loop through the names array and print names.

const users = {
  'Students': [
    {first_name:  'Michael', last_name : 'Jordan'},
    {first_name : 'John', last_name : 'Rosales'},
    {first_name : 'Mark', last_name : 'Guillen'},
    {first_name : 'KB', last_name : 'Tonel'}
  ],
  'Instructors': [
    {first_name : 'Michael', last_name : 'Choi'},
    {first_name : 'Martin', last_name : 'Puryear'}
  ]
}

Object.keys(users)
  .reduce((names, userType) => names.concat(users[userType]), [])
  .map(userObject => `${userObject.first_name} ${userObject.last_name}`)
  .forEach(name => console.log(name));

If you're not into ES6, here's the ES5 equivalent:

const users = {
  'Students': [
    {first_name:  'Michael', last_name : 'Jordan'},
    {first_name : 'John', last_name : 'Rosales'},
    {first_name : 'Mark', last_name : 'Guillen'},
    {first_name : 'KB', last_name : 'Tonel'}
  ],
  'Instructors': [
    {first_name : 'Michael', last_name : 'Choi'},
    {first_name : 'Martin', last_name : 'Puryear'}
  ]
}

Object.keys(users)
  .reduce(function(names, userType) {
    return names.concat(users[userType]);
  }, [])
  .map(function(userObject) {
    return ((userObject.first_name) + " " + (userObject.last_name));
  })
  .forEach(function(name) {
    return console.log(name);
  });

As for your code:

Indentation needs to be more consistent.
Names in JS follow the camelCase convention.
Most of JS code I've seen use the "egyptian braces" (collapsed braces).
Note that variables declared with var is function-scoped. Loops don't scope vars locally.
for-in loops should be guarded with an object.hasOwnProperty check.


Linking input/output states and conditions in an input file Consider the following CSV file:

A; B; ;
B; ; A;
C; ; E F;
D; ; E;
E; C; ;

The fields:

$1: the jname. A unique id of the entry.
$2: a " "(space)-separated list of incond.
$3: a " "(space)-separated list of outcond.

For the "link" A-B to be valid, jname A must define B as outcond, and job B must define A as incond.

In the above example, D-E is not a valid "link" because E doesn't define D as incond.
C-F is not a valid "link" because F doesn't exist.

A cond is not valid if the link it forms is not valid. The script must detect all non valid conds and which jobs are infected.

#!/usr/bin/awk -f

BEGIN {
    FS=" *; *";
    delim = "-";
    conds[""]=0;
}

{

    icnd_size = split($2, incond_list, " ");

    for (i=1; i<=icnd_size; ++i) {
    conds[incond_list[i] delim $1]++;
    }

    ocnd_size = split($3, outcond_list, " ");

    for (i=1; i<=ocnd_size; ++i) {
    conds[$1 delim outcond_list[i]]--;
    }

}

END {

    for (i in conds) {
    sz = split(i, answer, delim);

    if (conds[i] == 1) {
        j = answer[2];
        c = answer[1];
        inorout = "INCOND";
    }

    if (conds[i] == -1) {
        j = answer[1];
        c = answer[2];
        inorout = "OUTCOND";
    }

    if (conds[i] != 0)
        print "Invalid", inorout, c, "on job", j;
    }
}

The script works, although I do not have large data to test against. I see 2 problems with it:

the script will break if some cond has the character delim in the name
the script might break (and/or return false positives) if a line is inserted twice or if two lines have the same jname.

I could use any tip on addressing the two problems, as well as any critique of the code, it's literally my first Awk code.

Your main questions

  The script works, although I do not have large data to test against.

You don't necessarily need a large dataset.
It's better to think of all possible corner cases.
For example, your sample data demonstrates failures of OUTCOND but not of INCOND.
Also, although there is an example of more than one outgoing links,
but there is no example of more than one incoming links.
There are not too many interesting cases,
if you add examples for all them,
then you can be fairly confident in your solution.

  The script will break if some cond has the character delim in the name

If you want to be really safe, you could add a sanity check for that, and raise an error when such name is found, for example by calling exit with a non-zero value.

  The script might break (and/or return false positives) if a line is inserted twice or if two lines have the same jname.

Ditto.

Simplify

Many things can be simplified in this code.

The conds[""]=0; is unnecessary, you can simply delete that line.

Instead of this:

icnd_size = split($2, incond_list, " ");

for (i=1; i<=icnd_size; ++i) {
conds[incond_list[i] delim $1]++;
}

You don't really need the return value of split,
because instead of a counting loop,
you can use a more idiomatic for-each loop:

split($2, inconds, " ");

for (i in inconds) {
    conds[inconds[i] delim $1]++;
}

The same goes for outconds as well.

Mutually exclusive if statements

These if statements cannot be both true at the same time:

if (conds[i] == 1) {
    # ...
}

if (conds[i] == -1) {
    # ...
}

So they should be chained together with an else if.

Formatting

Instead of this:

for (i=1; i<=ocnd_size; ++i) {
conds[$1 delim outcond_list[i]]--;
}

It would be better to write like this:

for (i = 1; i <= ocnd_size; ++i) {
    conds[$1 delim outcond_list[i]]--;
}

Naming

Some of the names are not so great.
For example sz, i, j, c in the END block.
sz is actually unnecessary,
and I would rename the others to pair, job, and cond,
respectively.

Putting it together

Consider this alternative implementation:

#!/usr/bin/awk -f

BEGIN {
    FS = " *; *";
    delim = "-";
}

{
    split($2, inconds, " ");

    for (i in inconds) {
        conds[inconds[i] delim $1]++;
    }

    split($3, outconds, " ");

    for (i in outconds) {
        conds[$1 delim outconds[i]]--;
    }
}

END {
    oformat = "Invalid %s %s on job %s\n";
    for (pair in conds) {
        split(pair, parts, delim);

        if (conds[pair] == 1) {
            job = parts[2];
            cond = parts[1];
            inorout = "INCOND";
        } else if (conds[pair] == -1) {
            job = parts[1];
            cond = parts[2];
            inorout = "OUTCOND";
        }
        if (conds[pair] != 0) print "Invalid", inorout, cond, "on job", job;
    }
}


Finding the percentile corresponding to a threshold I need to find which percentile of a group of numbers is over a threshold value. Is there a way that this can be speed up? My implementation is much too slow for the intended application. In case this changes anything, I am running my program using mpirun -np 100 python program.py. I cannot use numba, as the rest of this program uses try/except statements.

import numpy as np
my_vals = []
threshold_val = 0.065
for i in range(60000):
    my_vals.append(np.random.normal(0.05, 0.02))

upper_bound = 100
lower_bound = 0
perc = 50.0
val = np.percentile(my_vals, perc)
while abs(val - threshold_val) > 0.00001:
    print val
    if val > threshold_val:
        upper_bound = perc
        perc = (perc + lower_bound)/2
    else:
        lower_bound = perc
        perc = (perc+upper_bound)/2
    val = np.percentile(my_vals, perc)

print perc

If I understand your question, you are asking to determine the percentile value of the first value that exceeds the threshold. The thing to do is the following:

Specify the number of random.normal deviates in the call and pre-sort the array. That way you can just look at each element in the array, knowing that the next one is bigger than the last.
Just count the number of values that fail to exceed the threshold, stopping after you find one that exceeds it. No need to continue beyond this point.
Then, do the arithmetic.

my_vals = sorted(np.random.normal(0.05, 0.02, 60000))
count_vals = 0
for i in my_vals:
    count_vals += 1
    if i > threshold_val:
        break
percentile_val = 100 * (count_vals/len(my_vals))
print('{0:0.6}'.format(percentile_val))

As a check, you could then calculate the value of the element at that percentile using np.percentile.

print(np.percentile(my_vals, percentile_val))

Even easier is to do the counting this way:

count_vals = sum(i > threshold_val for i in my_vals)
percentile_val = 100 * (count_vals/len(my_vals))