Fetching items from database with "Outstanding" status I am fetching around more than 300000 items through a query:

@items = current_company.item_masters.user_hierarchies_filter(current_user).accessible_by(current_ability).paginate(:conditions => condition.compile, :include=> includable_tables,
        :order => (sort_column + " " + sort_direction),
        :page=> new_params[:page],:per_page => new_params[:per_page_records].blank? ? 10 : new_params[:per_page_records])

And the scope used in that query is :

named_scope :user_hierarchies_filter, lambda { |user|
    {select: "DISTINCT item_masters.id", conditions: ["item_masters.company_id = ?", user.company_id]}
  }

It produces this log message:

ItemMaster Load (43445.6ms)   SELECT DISTINCT item_masters.id FROM `item_masters` WHERE (item_masters.status IN ('Outstanding')) AND (((item_masters.company_id = 1) AND (`item_masters`.company_id = 1))) ORDER BY item_number asc LIMIT 331230, 10

It takes around 44 seconds to load items. Can anyone please suggest any option to reduce the loading time of this query?

As far as I can see this is a very old Rails code. Please follow these steps:

Refactor the code a little bit:

scope user_hierarchies_filter: lambda { |user|
  select('distinct item_masters.id').
  where("item_masters.company_id = ?", user.company_id)
}

and

    @items = current_company.item_masters.
    user_hierarchies_filter(current_user).accessible_by(current_ability).
    includes(includable_tables).
    order(sort_column + " " + sort_direction).
    paginate(page: new_params[:page],
             per_page: new_params[:per_page_records].blank? ? 10 : new_params[:per_page_records])

from the generated query:

SELECT DISTINCT item_masters.id
FROM `item_masters`
WHERE (item_masters.status IN ('Outstanding'))
AND (((item_masters.company_id = 1)
AND (`item_masters`.company_id = 1)))
ORDER BY item_number asc LIMIT 331230, 10

I propose a couple of things:

Remove the duplicated condition on the company_id.
Explain the query and try to see why is so slow. I guess you are missing an index on the status, company_id and item_number columns: do things get better if you run the following migration?

add_index :item_masters, :company_id
add_index :item_masters, :status
add_index :item_masters, :item_number


Random D20 number generator I'm teaching myself code using Zed Shaw's Learn Python The Hard Way, and I got bored during one of the memorization lessons so I thought I would make a random D20 number generator for when I play RPGS.

How can I make this code better? Is there anything stupid I'm doing?

import random

name = raw_input('Please Type in your name > ')

print "\nHello %s & welcome to the Random D20 Number Generator by Ray Weiss.\n" % (name)

first_number = random.randint(1, 20)

print first_number

prompt = ("""
Do you need another number? Please type yes or no.
""")

answer = raw_input(prompt)

while answer == "yes":
    print random.randint(1, 20)
    answer = raw_input(prompt)

if answer == "no":
print "\nThank you %s for using the D20 RNG by Ray Weiss! Goodbye!\n" % (name)

Eventually I would love to add functionality to have it ask you what kind and how many dice you want to role, but for now a review of what I've done so far would really help.

Here's my take:

from random import randint
name = raw_input('Please Type in your name > ')

print """
Hello {} & welcome to the Random Number Generator by Ray Weiss.
""".format(name)
upper = int(raw_input('Enter the upper limit > '))
n = int(raw_input("How many D{} you'd like to roll? ".format(upper)))

for _ in xrange(n):
    print randint(1, upper)
print """
Thank you {} for using the D{} RNG by Ray Weiss! Goodbye!
""".format(name, upper)

Changes as compared to your version:

directly import randint because it's the only function you use in random;
use the new string formatting method (str.format);
take the upper bound from user instead of hard-coding 20;
take the number of rolls from user instead of repeatedly asking if that's enough;
use a loop to make the repetition actually work. The self-repeating code asking the user if we should continue is now gone.


Compute shortest distance between point and a rectangle I want to compute the shortest distance between a position (x,y) and a rectangular box defined by (x_min, y_min) and (x_max, y_max). See the picture below with some examples. I know that in two dimensions there are 9 cases I have to take into account.

Since this part of my code is used very often, I would like to know what is the fastest possible way to do that? Here is what I have done so far:

#include <stdio.h>
#include <stdlib.h>
#include <math.h>

#define sqr(x) ((x)*(x))

double distanceBoxParticle2D(double x, double y, double x_min, double y_min, double x_max, double y_max);

int main(){
    double x = -6;
    double y = -6;
    double x_min = -5;
    double x_max = 5;
    double y_min = -5;
    double y_max = 5;

    double distance = distanceBoxParticle2D(x, y, x_min, y_min, x_max, y_max);
    printf("distance = %f\n", distance);

}

double distanceBoxParticle2D(double x, double y, double x_min, double y_min, double x_max, double y_max){
    double r = 0;

    if(x < x_min){
        if(y < y_min){
            return r = sqrt( sqr(x_min-x) + sqr(y_min-y) );
        }else if(y > y_max){
            return r = sqrt( sqr(x_min-x) + sqr(y_max-y) );
        }else if((y >= y_min) && (y <= y_max)){
            return r = x_min - x;
        }
    }else{
        if((x > x_min) && (x < x_max)){
            if(y < y_min){
                return r = y_min - y;
            }else if(y > y_max){
                return r = y - y_max;
            }
        }else{
            if(x > x_max){
                if(y < y_min){
                    return r = sqrt( sqr(x_max-x) + sqr(y_min-y) );
                }else if(y > y_max){
                    return r = sqrt( sqr(x_max-x) + sqr(y_max-y) );
                }else if((y >= y_min) && (y <= y_max)){
                    return r = x - x_max;
                }
            }
        }
    }

    return r = 0; // if (x,y) is inside of the box
}

The example is only for two dimensions. Later I want to do the same in 3D. There I have to consider 27 cases, which is a lot. Is there any smart and fast solution for this case or do I have to hard-code every case?

Bug

If either x == x_min or x == x_max, your function will return 0 even if the point is outside of the rectangle.  This is because none of your if statements included the case where x == x_min or x == x_max, so the flow falls through to the end of the function where you return 0.

Extraneous variable

Your function has a variable r that is set but never used.  You should remove that variable so that lines like this:

        return r = x_min - x;

can turn into this:

        return x_min - x;

Extraneous third comparison

Consider this chain of three if statements:

    if(y < y_min){
        return r = sqrt( sqr(x_min-x) + sqr(y_min-y) );
    }else if(y > y_max){
        return r = sqrt( sqr(x_min-x) + sqr(y_max-y) );
    }else if((y >= y_min) && (y <= y_max)){
        return r = x_min - x;
    }

The point can either be above the rectangle, below the rectangle, or within the rectangle.  If the point is neither above nor below the rectangle, then it must be within the rectangle.  Therefore, the third comparison can be removed, like this:

    if(y < y_min){
        return r = sqrt( sqr(x_min-x) + sqr(y_min-y) );
    }else if(y > y_max){
        return r = sqrt( sqr(x_min-x) + sqr(y_max-y) );
    }else{
        return r = x_min - x;
    }

Rewrite

In this rewrite, I defined a HYPOT macro to simplify the expression that you use 4 times.  I also modified the indentation so that each case would be at the same indentation level.  Lastly, I reordered the if statements so the order would be "below, inside, above" instead of "below, above, inside", although the order really doesn't matter.  This rewrite was a tiny bit faster (2%) than the original code, possibly due to removing the extraneous third comparisons mentioned above.

#define HYPOT(x, y) sqrt(sqr(x) + sqr(y))

double distanceBoxParticle2D(double x, double y, double x_min, double y_min,
        double x_max, double y_max)
{
    if (x < x_min) {
        if (y < y_min)
            return HYPOT(x_min-x, y_min-y);
        else if (y <= y_max)
            return x_min - x;
        else
            return HYPOT(x_min-x, y_max-y);
    } else if (x <= x_max) {
        if (y < y_min)
            return y_min - y;
        else if (y <= y_max)
            return 0;
        else
            return y - y_max;
    } else {
        if (y < y_min)
            return HYPOT(x_max-x, y_min-y);
        else if (y <= y_max)
            return x - x_max;
        else
            return HYPOT(x_max-x, y_max-y);
    }
}

Here is a more concise variant you might prefer:

double distanceBoxParticle2D(double x, double y, double x_min, double y_min,
        double x_max, double y_max)
{
    if (x < x_min) {
        if (y <  y_min) return HYPOT(x_min-x, y_min-y);
        if (y <= y_max) return x_min - x;
                        return HYPOT(x_min-x, y_max-y);
    } else if (x <= x_max) {
        if (y <  y_min) return y_min - y;
        if (y <= y_max) return 0;
                        return y - y_max;
    } else {
        if (y <  y_min) return HYPOT(x_max-x, y_min-y);
        if (y <= y_max) return x - x_max;
                        return HYPOT(x_max-x, y_max-y);
    }
}


Copying one dimensional array bottom to top I want to convert a one dimensional array to a two dimensional array with the following conditions and have followed the code from this question - https://stackoverflow.com/questions/1817631/iterating-one-dimension-array-as-two-dimension-array. The 2-dimensional array has to be copied from the one dimensional array and written bottom to top. The second condition is that while the 1-dim array is being copied bottom to top the first row and the last column of the original 1-dimensional array must not appear in the two dimensional array if one were to visualize the 1-dim as a 2-dim array.

int maxrowIndex = rows-1;
for (int row = 0; row < rows; row++)
    {
        for (int column = 0; column < columns ; column++)
            {
               twodim[row][column] = onedim[maxrowIndex*columns+column];

            }
        maxrowIndex--;
    }

Here is the second code

int maxrowIndex = rows-1;
for (int row = 0; row < rows-1; row++)
    {
        for (int column = 0; column < columns-1 ; column++)
            {
               twodim[row][column] = onedim[maxrowIndex*columns+column];

            }
        maxrowIndex--;
    }

Did I get it right ? Is there any way I can improve the code ?

OK, so from what I can understand about your problem is that you want to "chunk" up a one-dimensional array into a two-dimensional array with rows rows and columns columns.

I'm going to assume row-major form since that's a little bit more typical (and seems to be what your code does too).

First of all, I don't understand the condition where it has to be written from bottom to up. I don't see any reason for this, but we'll assume we do have to do that for whatever reason.

Your code does not quite work, because you've confused the row variable and the maxrowIndex variable. In the part where you are attempting to calculate the one-dimensional index based on a given row and column you use this maxrowIndex variable which hints at it actually storing the current row index. But you do not use this when indexing your two-dimensional array.

In terms of style, I have to say that I'm not a fan of opening the brace { on the next line, and especially not indenting twice each time you nest.

Here's how I would approach the problem:

Edit: clarification - the data was original written north to south ([0][0] would have been the north west corner) and we need to flip vertically so that the new [0][0] should be the south west corner.

// We ignore the first row of the original array (which is the last row of the new 2d array
for (int row = 0; row < rows - 1; row++) {
    // From first column to second last column (skips last column)
    for (int column = 0; column < columns; columns++) {
        int originalRow = rows - 1 - row;
        twodim[row][column] = onedim[originalRow*columns + column];
    }
}


Efficiently creating a list of formatted hex values My background is in C, and I'm finally learning this new-fangled "Python" that all the cool kids are talking about.

I want to create a list of formatted hexadecimal values like so:

['0x00','0x01','0x02','0x03' (...) '0xfe','0xff']

Please note that the leading zero's shouldn't be stripped. For example, I want 0x03, not 0x3.

My first successful attempt was this:

hexlist=list()
tempbytes = bytes(range(256))
for value in tempbytes:
    hexlist.append("0x" + tempbytes[value:value+1].hex())
del tempbytes

But, wow this is ugly.  Then I tried to make it more Pythonic ("Pythony"?) like so:

hexlist = ["0x"+bytes(range(256))[x:x+1].hex() for x in bytes(range(256))]

My thoughts:

OMG! That's harder to read, not easier!
Not only that, but I had to invoke range twice, which I assume is inefficient.
I bet there's a much better way that I haven't been exposed to yet...

My questions:

Is the second much less efficient?
Is there a better way to do this?
What's the best thing to do here in keeping with python style?

The shortest and most readable way to do it would probably be

hexlist = [hex(x) for x in range(256)]

Time

Your first : 0.1  ms per loop for \$10^5\$ loops

Your second: 1    ms per loop for \$10^5\$ loops

The above  : 0.02 ms per loop for \$10^5\$ loops

I'm unable to com up with a worse example, but there is always a worse way to do it, exactly as there is always preferable way to do it.

Edit
To do this with filled zeros I would suggest to treat the first 10 numbers as a special case.

hexlist = [hex(x) if x > 15 else "{:#04x}".format(x) for x in range(256)]

new time: 0.04 ms. Even if it isn't as pretty.

Edit:

And when considering that format can format binary numbers, why not hex?

hexlist = ["{:#04x}".format(x) for x in range(256)]

I guess that this is hard coded for this range, but anyway.

Time: 0.1ms for per loop for \$10^5\$ loops.

And more edits

Using old style:.

hexlist = ["0x%02x" % n for n in range(256)]

Time: 0.08ms per loop for \$10^5\$ loops.

And specifically generating what we want.

hexlist = ["{:#04x}".format(x) for x in range(16)] + \
              [hex(x) for x in range(16, 256)]

Time: 0.03ms per loop for \$10^5\$ loops.


Prefix trees for auto suggest I wrote a quick prefix tree implementation. I know it may be far from perfect, but I need to make sure that I got the basics right.

My main concerns are:

Have I settled for the correct data structures?
Is this a scalable solution? What sort of problems can I run into if this is a huge tree?

public class PrefixTree {
//isLeaf would point out the end of a word in the tree
boolean isLeaf;
HashMap<Character, PrefixTree> map;

public PrefixTree() {
    map=new HashMap<Character, PrefixTree>();
    isLeaf=false;
}

//insert a new word into the tree
public void put(String s) {
    if(s.length()==0) {
        //if the word ends here, we need to point it out using the isLeaf
        //boolean field, and the insertion job is done, so return
        isLeaf=true;
        return;
    }
    //if the first character of the string doesn't exits
    //we create a new node for the and insert it into the map
    if(!map.containsKey(s.charAt(0))) {
        map.put(s.charAt(0), new PrefixTree());
    }
    //now this newly create node or if it already existed
    //would contain the rest of the substring
    map.get(s.charAt(0)).put(s.substring(1));
}

//tree travelsal to examine the contents of the tree
public void travelTree(String perm) {
    if(isLeaf) {
        System.out.println(perm);
    }
    if(map.size()==0) return;
    for(Character c:map.keySet()) {
        map.get(c).travelTree(perm+c);
    }
}

//this overloaded function is used as a helper function with the getSuggestions
//functions
//doesn't need to be called on from the root
public void travelTree(String perm, List<String> sl) {
    if(isLeaf) {
        sl.add(perm);
    }
    if(map.size()==0) return;
    for(Character c:map.keySet()) {
        map.get(c).travelTree(perm+c, sl);
    }
}

public List<String> getSuggestions(String q) {
    //I am passing along this List object into the nested recursive calls
    //from here onwards, is this a better approach than having every
    //recursive function call create an arraylist object and append the
    //return items and finally return the final list
    List<String> suggestions=new ArrayList<String>();
    getSuggestions(q, "", suggestions);
    return suggestions;
}

public void getSuggestions(String q, String perm, List<String> sl) {
    if(q.length()==0) {
        travelTree(perm, sl);
        //we don't return as we need to go into further depth for more
        //suggestions
    }
    if(map.size()==0) return;
    if(q.length()>0 && map.containsKey(q.charAt(0))) {
        //make sure we call getSuggestions on specific node instead of
        //current instance's getSuggestions
        map.get(q.charAt(0)).getSuggestions(q.substring(1), perm+q.charAt(0), sl);
    }
}
}

Overall it looks like you have created a clean and simple piece of code there. I still have some suggestions for you:

Nitpicks:

Your indentation could use  some rethinking. By convention everything in curly braces is indented by one level (4 spaces / 1 tab). Your class body isn't indented "properly"
Your Prefix tree could use some eager initialization. Your constructor is completely superfluous if you just declare your fields (which should be private, btw.) like the following:

public class PrefixTree {
    // isLeaf [...]
    private boolean isLeaf = false;
    private Map<Character, PrefixTree> map = new HashMap<>();

I have additionally changed the type of map to a Map (which is an interface) instead of HashMap. The advantage of that is: you can change the map implementation at any time you like.

Comments:

Your comments are overall clear and simple. They explain the why and the what though, which is not desirable for comments IMO. Comments in code should be explaining why some decisions were made. The code should be clean enough that the why is obvious.

Also there is a huge load of spelling mistakes in the comments. These mean additional mental strain, because the reader has to decode the actual word from the sentence context.

Additionally I see that many of your methods are commented wrt what they do. You should definitely use javadoc for that. It allows you to document methods (and their purpose) in a way readable to both the programmer and easily usable by IDEs:

/**
 * Insert a new word into the tree
 *
 * @param s The word to be inserted
 */
public void put(String s) {

Scoping:

As mentioned in a sidenot earlier on in the nitpicks: Your fields should be private. I'd even go so far to say that map should also be final.

Additionally you are exposing a helper function (travelTree) to the public. IMO that method should definitely be private.

Your concerns:

Have you settled for the correct data structures?
To me this one looks like a yes. Good job on chosing a map to represent the character choices at each tree node.

But there is one big problem I have with the Type you're assuming to be used with the tree, namely String. While a String is rather "universal" I'd rather see you using CharSequence which is a superclass of String. It also exposes .length() and .charAt() methods. Only your call to .substring()  would have to be changed to .subsequence(1, len - 1)
Is this a scalable solution? What kind of problems can I run into if this is a huge tree?
Well it's rather simple. This solution is quite scalable, mostly since you're really effective with the memory you're using and since traversing the tree is mostly following pointers in a HashMap. What you have to expect though for large trees is extremely large memory consumption (unsurprisingly) and increased traversal times.

You currently have no way of shortcutting a traversal through a solitary branch with a "long" word like "Xylophone". This means you might be "wasting" some time in traversal.

Summa Summarum:

The code is clean and relatively simple. There is minor violations of java conventions all over the place, comments have too many spelling errors, but your choice of data structure is a good one.

Overall: Well done, but you can do better ;)


Bash script - compile and run c++ code for coding competitions This is a simple bash script that I use to compile and run single C++ files for coding competitions.
Features:

Detects if there is a corresponding .in file next to it, and if so uses that as stdin

e.g. if the file problem1.cpp and problem1.in are in the same directory, the script will redirect stdin from problem1.in

Compile each file to a temp directory so it doesn't clutter the working directory
Configurable g++ warning flags as needed

Note: \033[32m and \033[0m are terminal color codes that make the text green
#!/bin/bash
# Compiles and runs .cpp code
# if there exists an .in file next to the .cpp file
# it will use that as input

if [ -z $1 ]; then
  echo -e "Please choose an input file"
  exit 1
fi

FILE="$1"
FILE_IN="${FILE%.*}.in"

clear
echo -e "\033[32mCompiling...\033[0m"
TMPFILE=$(mktemp /tmp/run-cpp.XXXXXXXXXX)
WARNING_FLAGS="-Wuninitialized -Wmaybe-uninitialized"
g++ $FILE -std=c++17 $WARNING_FLAGS -O3 -o $TMPFILE

if [ $? -eq 0 ]; then
  echo -e "\033[32mRunning...\033[0m"
  if [ -f $FILE_IN ]; then
    $TMPFILE < $FILE_IN
  else
    $TMPFILE
  fi
  ERROR=$?
fi
rm $TMPFILE 2> /dev/null
exit $ERROR

Usage:
./runcpp.sh /path/to/my-cpp-file.cpp

#!/bin/bash

I don't see why you're using Bash for this - there should be no problem using standard POSIX shell.

if [ -z $1 ]; then

I would recommend "$1" there, even though extra arguments will cause [ to return false for other reasons.  Passing as a single argument won't emit any error messages.

  echo -e "Please choose an input file"

echo -e isn't portable, and isn't necessary here anyway.

FILE="$1"
FILE_IN="${FILE%.*}.in"

Avoid all-caps names for variables in your own program - these are generally reserved for environment variables which change programs' behaviour.

clear

I think that's a bit rude - some of us like to be able to compare results against the previous run.  If I want to clear before I run, I can type that easily.

echo -e "\033[32mCompiling...\033[0m"

Don't embed terminal-specific escape codes like that!  Even though most recent terminals support the ANSI escapes, there are others - and if you redirect to file, you don't want it littered with control characters.  Use tput to generate the correct escapes for your $TERM (and slightly more readable code).

TMPFILE=$(mktemp /tmp/run-cpp.XXXXXXXXXX)

That's not a very descriptive variable name.  It's more informative to say what it's for (the executable to create).  And why hard-code /tmp as the directory?  Prefer $TMPDIR if set (perhaps systems with per-user temp directories, or with a choice of fast or large temporary storage).

WARNING_FLAGS="-Wuninitialized -Wmaybe-uninitialized"

That's quite a lax set of warnings.  If you care about the quality of your source, add a few more.  I suggest -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Weffc++.  If you only care about performance, then perhaps -Wall -Wextra -Wno-parentheses -Warray-bounds might be sufficient.
Since we use this variable only once, perhaps we should inline its use, thereby not triggering a Shellcheck warning where we (correctly) expand it without quotes.

g++ $FILE -std=c++17 $WARNING_FLAGS -O3 -o $TMPFILE

"$FILE" here, too.  I'd write "$TMPFILE" even though we constructed it to be a safe name.

if [ $? -eq 0 ]; then

Anti-pattern - just use the preceding command directly after if.  Or, enable the -e flag of the shell, to just exit if the compilation fails.

  echo -e "\033[32mRunning...\033[0m"

tput again.

  if [ -f $FILE_IN ]; then

Quotes again.

    $TMPFILE < $FILE_IN
  else
    $TMPFILE
  fi

As we don't use stdin, we could just redirect stdin using exec, and not need two different commands.

  ERROR=$?
fi
rm $TMPFILE 2> /dev/null

Why didn't we remove the temporary file if we exited early?

exit $ERROR

We wouldn't need to store the error if we made removing the temp-file an exit trap.

Modified code
#!/bin/sh
# Compiles and runs C++ source code. A corresponding file
# ending with .in will be used as input, if present

set -eu

if [ $# -ne 1 ] || [ -z "$1" ]
then
  echo "Usage: $0 SOURCE"
  exit 1
fi

executable=$(mktemp -t run-cpp.XXXXXXXXXX)
trap 'rm $executable' EXIT

green=$(tput setaf 2)
normal=$(tput sgr0)

echo "${green}Compiling...${normal}"
g++ -o "$executable" -std=c++17 -O3                     \
    -Wall -Wextra -Wwrite-strings -Wno-parentheses      \
    -Wpedantic -Warray-bounds -Weffc++                  \
    "$1"

echo "${green}Running...${normal}"
input=${1%.*}.in
if [ -f "$input" ]
then exec <"$input"
fi

"$executable"

You might also be interested in my approach to a similar situation, which uses Make rather than shell.


User auth class and security Just curious if my authentication is secure enough, really. Also wondering if I should ditch the secret variable and just use password everywhere instead.

Note: the method logic must remain seperated because the same User object will have CRUD methods added at a later date.

The table structure:

CREATE TABLE IF NOT EXISTS `users` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `username` varchar(255) NOT NULL,
  `password` varchar(255) NOT NULL,
  `secret` varchar(255) DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB  DEFAULT CHARSET=latin1 AUTO_INCREMENT=2 ;

The class:

class User {
    public $db;
    public $id;
    public $username;
    public $password;
    public $secret;
    public $signed_in = false;

    function __construct ($db) {
        $this->db = $db;
    }

    function redirect ($url) {
        header('Location: '.$url);
        die();
    }

    function exists () {
        $sql = "select * from `users` where `username`=:username";
        $stm = $this->db->prepare($sql);
        $stm->bindParam(':username', $_POST['username']);
        $stm->execute();
        $res = $stm->fetch();

        if (!empty($res['password'])) {
            if (password_verify($_POST['password'], $res['password'])) {
                $this->id = $res['id'];
                $this->username = $res['username'];
                return true;
            }
        }

        return false;
    }

    function signIn () {
        $this->secret = password_hash($_POST['password'], PASSWORD_DEFAULT);

        if ($_POST['remember']) {
            setcookie('username', $this->username, time() + 31556926, '/');
            setcookie('secret', $this->secret, time() + 31556926, '/');
        }
        $_SESSION['username'] = $this->username;
        $_SESSION['secret'] = $this->secret;

        $sql = "update `users` set `secret`=:secret where `id`=:id";
        $stm = $this->db->prepare($sql);
        $stm->bindParam(':secret', $this->secret);
        $stm->bindParam(':id', $this->id);
        $stm->execute();
    }

    function signedIn () {
        if (!empty($_COOKIE['username'])) {
            $this->username = $_COOKIE['username'];
            $this->secret = $_COOKIE['secret'];
        }
        else if (!empty($_SESSION['username'])) {
            $this->username = $_SESSION['username'];
            $this->secret = $_SESSION['secret'];
        }

        $sql = "select * from `users` where `username`=:username and `secret`=:secret";
        $stm = $this->db->prepare($sql);
        $stm->bindParam(':username', $this->username);
        $stm->bindParam(':secret', $this->secret);
        $stm->execute();
        $res = $stm->fetch();

        if ($res) {
            array_walk_recursive($res, 'sanitize');
            foreach ($res as $k => $v) {
                if (property_exists($this, $k)) {
                    $this->$k = $v;
                }
            }
            $this->signed_in = true;
        }
    }
}

How the class is used at the top of all pages in the app:

$user = new User($db); // create user object, $db is a PDO object
$user->signedIn(); // determine if they're signed in, populate $user->signed_in boolean

Signing the user in via a form:

if (isset($_POST['username'])) {
    if ($user->exists()) {
        $user->signIn();
        $user->redirect('dashboard.php');
    }
}

Any feedback is greatly appreciated. Like I said my primary concern here is security.

Don't store passwords on the client side. If your database leak, no one can stop the attacker from sending directly the hash instead of the password. In this case the attacker don't need to know the password at all.
You really want a method redirect in your User class? Think about SRP.
Your User class is half Model and half DAO. And you don't want that. A model full of query SQL isn't a good model. A data access object handles this separation between SQL and Model.


type/class __repr__ formatter for Python objects I am building a list of Python objects that I use fairly regularly that have a length property, (len, 'len') and I was trying to determine the best way to simplify the type/class formatter that the object uses.  For example:
 type([]), returns "< type 'list'>" and type(collections.Counter()) returns "< class 'collections.Counter'> "

I had assumed that the formatter that was used could be accessed simply and the extraneous information removed to facility a particular documentation format that I prefer.  In this situation, I had hoped to be able to remove '< type ... >' or '< class ... >' from the resultant output.  The kludge that I have come up with entails parsing the string into its components and returning the portion I want.  To ensure generality, for objects that I may not currently use, I had hoped for a better solution.

Insights regarding options to this issue, or more general comments would be appreciated.  The line in question is flagged with a comment.  Apologies, if I am not supposed to included doc information with posted scripts.

"""
Script:   empty_tests.py
Modified: 2015-05-25
Purpose:
  checks on objects that I use, that have a __len__ property
Notes:
- collections.OrderedDictionary and other classes in collections behave
  in a similar fashion
- for NumPy arrays, use size property rather than __len__
"""
import numpy as np
import collections
c0 = collections.Counter()
c1 = collections.Counter([0])
objs = [ [],[1],(),(1),{},{1:"one"},"","1",None,True,1,False,0,c0,c1 ]
is_empty = [ True  if not i else False for i in objs ]
t = [ type(i).__name__ for i in objs ]            # correct based on comment
#t = [str(type(i)).split("\'")[1] for i in objs ] # line in question
print("\n{:<15} {:<6} {:<10}".format("Object","Empty","Type"))
for i in range(len(objs)):
    print("{:<15} {:<6} {:<10s}".format(objs[i],str(is_empty[i]),t[i]))

Output with the above...the commented out line worked

Object          Empty  Type
[]              True   list
[1]             False  list
()              True   tuple
1               False  int
{}              True   dict
{1: 'one'}      False  dict
                True   str
.... etc

I have tried ....

>>> type([])
<type 'type'>
>>> repr(type([]))
<type 'list'>"
>>> str(type([]))
<type 'list'>"
>>> print str(type([])), repr(type([]))
<type 'list'> <type 'list'>
>>>

EDIT
the "name" property wasn't listed where I thought it would be, so if the object has a "format" property, check there.

>>> help(type(object).__format__)
Help on method_descriptor:
__format__(...)
    default object formatter
>>> dir(type(object).__format__)
    ['__call__', '__class__', '__delattr__', '__doc__', '__format__',
     '__get__', '__getattribute__', '__hash__', '__init__', '__name__',
     '__new__', '__objclass__', '__reduce__', '__reduce_ex__', '__repr__',
     '__setattr__', '__sizeof__', '__str__', '__subclasshook__']
    >>>

This was answered in comments, but for completeness:

The name of a class is given by its __name__ attribute.
It's not a good idea to rely on dir to show you all the special methods and attributes. If you read the documentation you'll see that it says:

  The resulting list is not necessarily complete.

The place to look for documentation on special methods is the Data model section of the Python Language Reference.
Python 3.3 added __qualname__, which gives the qualified name, which is more informative than __name__ for nested class definitions.
__name__ doesn't include the module name, so collections.Counter.__name__ is just "Counter". If you want to prepend the module name, find it using the __module__ attribute.
True if not i else False is a long-winded way to write not i.
If possible, iterate over objects, not over indexes. Instead of:

is_empty = [ True  if not i else False for i in objs ]
t = [ type(i).__name__ for i in objs ]
for i in range(len(objs)):
    print("{:<15} {:<6} {:<10s}".format(objs[i],str(is_empty[i]),t[i]))

write:

for obj in objs:
    print("{!r:15} {!r:6} {:10}".format(obj, not obj, type(obj).__name__))


PHP Extending PDO Class I am trying to convert myself from using mysql extension to using PDO, with that being said I was curious if I am going about this the right way.

I have a custom logging system that I built I am wondering if the way I implemented this would work or not. I am planning on duplicating the main functions of PDO and doing the same as what I did prepare?

What I wanted to know is if the way I turned on exceptions and internally using try/catch blocks to capture and log errors then rethrowing the exception after I logged the error is the right way to do this or is there another more efficient way of doing this.

class rDB extends PDO {

private $dsn    = "mysql:host=10.10.10.43";
private $user   = "user";
private $pass   = "pass";

/**
 * @var Logger Writes messages to custom csv log based on debug level of 1-5
 * 1 Being Critical Error, 5 Being a simple Note
 */
private $logger;

public function __construct($logger) {

    $this->logger = $logger;
    $this->logger->log(5, 'Connecting to MySQL DB');

    try {
        /* Call to Parent Function */
        parent::__construct($this->dsn, $this->user, $this->pass);

        $this->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

        $this->logger->log(5, 'Connected to MySQL DB');
    }
    catch(PDOException $e) {
        $this->logger->log(2, 'Connection to MySQL DB Failed-> '.$e->getPrevious());
        throw $e;
    }
}

public function prepare($statement, array $driver_options = 'array()') {
    try {
        $this->logger->log(5, 'Preparing statement');
        /* Call to Parent Function */
        $value = parent::prepare($statement, $driver_options);
    }
    catch(PDOException $e) {
        $this->logger->log(2, 'Error preparing statement-> '.$e->getMessage());
        throw $e;
    }
    $this->logger->log(5, 'Statement is prepared.');
    return $value;
}
}

This mostly looks good to me.
I don't think that logging exceptions should be the concern of your rDB class. Also, you could utilize the PSR logger interface

class rDB extends PDO implements \Psr\Log\LoggerAwareInterface
{
    //do not override the PDO constructor

    public function setLogger(\Psr\Log\LoggerInterface $logger)
    {
        $this->logger = $logger;
    }

    //override the PDO methods as you have done but, call
    //$this->log() instead of $this->logger->log()
    //i.e.
    public function prepare($statement, $driver_options = array())
    {
        $this->log(\Psr\Log\LogLevel::INFO, $statement, array(
          'driver_options' => $driver_options
        ));
        return parent::prepare($statement, $driver_options);
    }

    private function log($level, $message, $context = array())
    {
        //note: only log if a logger is set
        if ($this->logger) {
            $this->logger($level, $message, $context);
        }
    }
}

A factory method added to your rDb class could be useful to construct an instance for you:

public static function createInstance($params)
{
    $dsn = array_key_exists('dsn', $params) ? $params['dsn']: null;
    $user = array_key_exists('user', $params) ? $params['user'] : null;
    $pass = array_key_exists('pass', $params) ? $params['pass']: null;
    $logger = array_key_exists('logger', $params) ? $params['logger']: null;

    $rdb = new rDB($dsn, $user, $pass);
    $rdb->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    if ($logger) {
        $rdb->setLogger($logger);
    }

    return $rdb;
}

EDIT
I suggest using a factory method to construct your instance instead of overriding the constructor primarily because 1) Changing the argument list in any overridden method effectively changes the interface. This makes it more difficult to replace one class with another. 2) I feel that constructors should do no more than serve as a way to inject dependencies that are always needed and initialize default values.

In your application, somewhere during initialization/bootstrap:

//Depending on your environment (production, development, etc),
//you can configure different kinds of loggers. For example,
//a development logger would probably log EVERYTHING but a
//production logger might only log error, critical, alert, emergency
$logger = new YourLoggerClassWhichImplementsLoggerInterface();

try {
    $dbConfig = //read in a configuration from somewhere
    $rdb = rDB::createInstance(array(
        'dsn' => $dbConfig->dsn,
        'user' => $dbConfig->user,
        'pass' => $dbConfig->pass,
        'logger' => $logger,
    ));

    //Now pass $rdb to whatever needs it (i.e. a controller).
} catch (Exception $e) {
    $logger->error($e->getMessage(), array('exception' => $e));
    //Display some kind of error page to the user
}


"The Story of a Tree" solved using depth-first search Found this problem in hackerrank.

  One day Bob drew a tree, with \$n\$ nodes and \$n-1\$ edges on a piece of paper. He soon discovered that parent of a node depends on the root of the tree. The following images shows an example of that:

  Learning the fact, Bob invented an exciting new game and decided to play it with Alice. The rules of the game is described below:

  Bob picks a random node to be the tree's root and keeps the identity of the chosen node a secret from Alice. Each node has an equal probability of being picked as the root.
  Alice then makes a list of \$g\$ guesses, where each guess is in the form \$u, v\$ and means Alice guesses that \${\rm parent}(v) = u\$ is true. It's guaranteed that an undirected edge connecting \$u\$ and \$v\$ exists in the tree.
  For each correct guess, Alice earns one point. Alice wins the game if she earns at least \$k\$ points (i.e., at least \$k\$ of her guesses were true).

  Alice and Bob play \$q\$ games. Given the tree, Alice's guesses, and the value of \$k\$ for each game, find the probability that Alice will win the game and print it on a new line as a reduced fraction in the format p/q.

Solution: There is a tree with some edges marked with arrows. For every vertex in a tree you have to count how many arrows point towards it. For one fixed vertex this may be done via one depth-first search (DFS). Every arrow that was traversed during DFS in direction opposite to its own adds 1. If you know the answer for vertex \$v\$, you can compute the answer for vertex \$u\$ adjacent to \$v\$ in \$O(1)\$.

It's almost the same as for \$v\$, but if there are arrows \$u→v\$ or \$v→u\$, their contributions are reversed. Now you can make the vertex \$u\$ crawl over the whole graph by moving to adjacent vertices in the second DFS.

def gcd(a, b):
    if not b:
        return a
    return gcd(b, a%b)

def dfs1(m, guess, root, seen):
    '''keep 1 node as root and calculate how many arrows are pointing towards it'''
    count = 0
    stack = []
    stack.append(root)
    seen.add(root)
    while len(stack):
        root = stack.pop(len(stack)-1)
        for i in m[root]:
            if i not in seen:
                seen.add(i)
                count += (1 if root in guess and i in guess[root] else 0)
                stack.append(i)
    return count

def dfs2(m, guess, root, seen, cost, k):
    '''now make every node as root and calculate how many nodes
       are pointed towards it; If u is the root node for which
       dfs1 calculated n (number of arrows pointed towards the root)
       then for v (adjacent node of u), it would be n-1 as v is the
       made the parent now in this step (only if there is a guess, if
       there is no guess then it would be not changed)'''
    stack = []
    stack.append((root, cost))
    seen.add(root)
    t_win = 0
    while len(stack):
        (root, cost) = stack.pop(len(stack)-1)
        t_win += cost >= k
        for i in m[root]:
            if i not in seen:
                seen.add(i)
                stack.append((i, cost - (1 if root in guess and i in guess[root] else 0) + (1 if i in guess and root in guess[i] else 0)))
    return t_win

q = int(raw_input().strip())
for a0 in xrange(q):
    n = int(raw_input().strip())
    m = {}
    guess = {}
    seen = set()
    for a1 in xrange(n-1):
        u,v = raw_input().strip().split(' ')
        u,v = [int(u),int(v)]
        if u not in m:
            m[u] = []
        m[u].append(v)
        if v not in m:
            m[v] = []
        m[v].append(u)
    g,k = raw_input().strip().split(' ')
    g,k = [int(g),int(k)]
    for a1 in xrange(g):
        u,v = raw_input().strip().split(' ')
        u,v = [int(u),int(v)]
        if u not in guess:
            guess[u] = []
        guess[u].append(v)
    cost = dfs1(m, guess, 1, seen)
    seen = set()
    win = dfs2(m, guess, 1, seen, cost, k)
    g = gcd(win, n)
    print("{0}/{1}".format(win/g, n/g))

gcd is missing a docstring.
gcd is tail-recursive, but Python doesn't have tail-recursion elimination, and so:

>>> gcd(3**7000, 2**10000)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
[...]
RecursionError: maximum recursion depth exceeded

It is more robust to turn the tail-recursion into a loop:

def gcd(a, b):
    """Return the greatest common divisor of a and b."""
    while b:
        a, b = b, a % b
    return a

and then:

>>> gcd(3**7000, 2**10000)
1

Python has a built-in greater common divisor function, fractions.gcd, so you could use that and avoid writing your own.
The only use for gcd is to print a fraction in lowest terms. The built-in fractions.Fraction class can do that, so you could write:

from fractions import Fraction
print("{0.numerator}/{0.denominator}".format(Fraction(win, n)))

The names dfs1 and dfs2 are not very informative. Sure, they do some kind of depth-first search, but which one is which?
The docstrings for dfs1 and dfs2 don't explain the arguments m, guess, seen, cost, and k. What am I supposed to pass for these arguments?
The seen argument to dfs1 and dfs2 always gets an empty set which is not used again after the function returns. So it would better for these functions to use a local variable and initialize it with an empty set. Then there would be no risk of passing the wrong value for this argument.
Instead of creating an empty list and appending an item to it:

stack = []
stack.append(root)

it's simpler to create a list with one item:

stack = [root]

Instead of testing the length of a list:

while len(stack):

it's simpler to test the list itself:

while stack:

Instead of computing the index of the last element of a list:

root = stack.pop(len(stack)-1)

you can use a negative index:

root = stack.pop(-1)

but by default the pop method pops the last element, so you can write:

root = stack.pop()

Instead of writing this complex condition:

count += (1 if root in guess and i in guess[root] else 0)

you can take advantage of the fact that True equals 1 and False equals 0, and write:

count += root in guess and i in guess[root]

The only use you make of guess is to test if it contains a directed edge. But this is slightly awkward because the test takes two steps. It would be better if you built a data structure that was better adapted to the use you intend to make of it. In this case, you could build a set of directed edges. So instead of:

guess = {}
for a1 in xrange(g):
    u,v = raw_input().strip().split(' ')
    u,v = [int(u),int(v)]
    if u not in guess:
        guess[u] = []
    guess[u].append(v)

write:

guess = set()
for _ in range(g):
    u, v = map(int, raw_input().split())
    guess.add((u, v))

and then in dfs1 you can write:

count += (root, i) in guess

(The construction of guess can be turned into a set comprehension, like this:

guess = {tuple(map(int, raw_input().split())) for _ in range(g)}

but I think the explicit loop is a bit clearer.)
Similarly, in dfs2 you have the complicated expression:

stack.append((i, cost - (1 if root in guess and i in guess[root] else 0) + (1 if i in guess and root in guess[i] else 0)))

Given the set of directed edges, this simplifies to:

stack.append((i, cost - ((root, i) in guess) + ((i, root) in guess)))

The top-level code is hard to test, because you can't call it from a Python program or the interactive interpreter. Better to put it in its own function, conventionally called main.
The top-level code is full of one-letter variables (m, q, g, k, u, v). It is hard to guess what these mean. Although it makes sense to stick with the names from the HackerRank problem description, a comment or two would help remind the reader of what they mean.
When you don't use a loop variable like a1, it's conventional to use the name _.
Some of the repetition in the input code can be avoided by using collections.defaultdict:

# Map from node to list of neighbours.
tree = defaultdict(list)
for _ in range(n - 1):
    u, v = map(int, raw_input().split())
    tree[u].append(v)
    tree[v].append(u)


Writing a thread-safe queue in C++ I created a SafeQueue class, which stores pointers. It has two methods:

push: Adds a new pointer to the queue
next: If the queue is empty, returns nullptr. Otherwise it returns the front element, and pop the queue

Most of the time, I have one producer and one consumer. But there may be more.

The producer just simply calls .push(ptr)

The consumer(s) call .next(), until a nullptr is returned. Or they continue the loop forever.

The reason I did this, is to eliminate .isEmpty function, so between the .empty() and .front() no other thread can pop the queue.

It works, but I don't think, that it is the preferable or an optimal solution for the problem.

The code is:

template<class T>
class SafeQueue {

    std::queue<T*> q;
    std::mutex m;

public:

    SafeQueue() {}

    void push(T* elem) {

        m.lock();
        if(elem != nullptr) {
            q.push(elem);
        }
        m.unlock();

    }

    T* next() {

        T* elem = nullptr;

        m.lock();
        if(!q.empty()) {
            elem = q.front();
            q.pop();
        }
        m.unlock();

        return elem;

    }

};

I see some things that may help you improve your code.

List all required #includes

The code needs the following #includes to actually compile.  Since they therefore form part of the interface, they should be included in the file and in a code review:

#include <mutex>
#include <queue>

Be clear about ownership

If the intention is to have a thread-safe queue, then passing pointers in and out is definitely not the way to go.  The problem is with object ownership.  Even if your thread-safe queue works perfectly, all of its inherent goodness is all too easily bypassed because you're using pointers.  For example:

SafeQueue<std::string> sq;

{
    std::string msg1{"this message exists"};
    sq.push(&msg1);
}   // msg1 is now destroyed, but queue still has pointer
std::cout << *sq.next() << " no longer\n";  // kaboom!

The problem is that the queue doesn't actually own the object (or at least have a std::shared_ptr) so there's not much point in perfecting the queue until that's addressed.

Choose better names

I have never thought of push and next as inverse operations, and I'll bet you never have either.  The push member function name is OK, since it actually does that, but next is just a strange name.  I'd say use pop or pop_front might be better names.

Minimize locking duration

In the push code, we have this:

void push(T* elem) {
    m.lock();
    if(elem != nullptr) {
        q.push(elem);
    }
    m.unlock();
}

But why acquire a lock if you don't need it?  It just slows things down.  You could instead write that like this:

void push(T* elem) {
    if (elem == nullptr) {
        return;
    }
    m.lock();
    q.push(elem);
    m.unlock();
}

Or better, see the next suggestion:

Use RAII to reduce errors

If you happened to forget to remove the lock on the code above, Bad Things would likely happen.  Fortunately, in C++, there's a handy idiom that is very often used in C++ and it's called Resource Allocation is Initialization.  In this context, we use a std::lock_guard like this:

void push(T elem) {
    std::lock_guard<std::mutex> lock(m);
    q.push(elem);
}

The lock_guard automatically gets locked on creation and unlocked on deletion, so when it goes out of scope, the lock is released even if you forget.

Return an indicator of success

Since we've already established that passing pointers in or out is a problem, I'd suggest changing the interface for the next() function.  Have it take a reference (so the caller must supply one) and then return a bool to indicate success.  That might look like this:

bool next(T& elem) {
    std::lock_guard<std::mutex> lock(m);
    if (q.empty()) {
        return false;
    }
    elem = q.front();
    q.pop();
    return true;
}


Break an array into n equal or almost-equal sized sublists I want to break a larger list into $num-lists smaller lists. I thought that there might be an option for rotor that would already do it, but I haven't found one.

#!/bin/env perl6

my @a = 1 .. 13;

my @lists = fair-lists(@a, 3);

say @$_ for @lists;

sub fair-lists ( @array, $num-lists )
{
    my $num-elements = @array.elems;
    my $num-large    = $num-elements % $num-lists; # remainder
    my $num-small    = $num-lists - $num-large;

    my $small-size = floor($num-elements/$num-lists);
    my $large-size = $small-size + 1; # same as ceiling($num-element/$num-lists);

    my $first-large = ($num-small * $small-size);
    my $last-small  = $first-large - 1;

    my @first-sublist  = @array[ 0 .. $last-small];
    my @second-sublist = @array[ $first-large .. $num-elements - 1 ];

    my @small-lists = @first-sublist.rotor($small-size);
    my @large-lists = @second-sublist.rotor($large-size);
    my @all-lists   = (@small-lists, @large-lists).flat;
    return @all-lists;
}

Is there a more concise and more clear way to do this in Perl 6?

Using roundrobin on a rotored array works

sub fair-lists ( @array, Int $num-lists ) {

  # "invert" the following "matrix"
  roundrobin

    # make it at least $num-lists long
    |@array[{ 0 ..^ ( $_ max $num-lists ) }]\

    # split it into chunks that are as big as the number of $num-lists
    .rotor: $num-lists, :partial

}

say fair-lists( 1..1, 3 ).perl;
# ((1,), (Any,), (Any,)).Seq

say fair-lists( 1..4, 3 ).perl;
# ((1, 4), (2,), (3,)).Seq

test it


VIM colors downloader in Python Recently, I wanted to change my vim colors to something new. So I went to the vim colors website and then I decided that I wanted to download ALL the colors.

So I wrote this simple Python script that will download all the files from: Vim Colors

I'm still learning Python and it would be amazing to get some feedback / suggestions if possible.

import os
import json
import requests
from bs4 import BeautifulSoup

class Spider:
    def __init__(self, total_pages=40):
        self.base_url = "http://vimcolors.com/?page="
        self.total_pages = total_pages
        self.download_dir = 'colors/'
        # If we don't have the download directory
        if not os.path.isdir(self.download_dir):
            print(self.download_dir, 'does not exist, trying to create it...')
            # create it...
            os.mkdir(self.download_dir)

    def download(self, name, url):
        # If we have already downloaded this file, just skip
        if os.path.isfile(self.download_dir + name):
            print('File:', name, 'already exists; skipping.')
            return

        try:
            # Get the response
            response = requests.get(url)
            # If response is 404 (Not Found), just exit
            if response.status_code == 404:
                raise Exception('File not found')
            # Create the file
            with open(self.download_dir + name, 'wb') as file_path:
                # Write content to the file
                file_path.write(response.content)
                # Confirm the download
                print('Downloaded', name)
        except:
            # This is a very generic error, perhaps I'll change it sometime :)
            print('Could not download the file', name)
            pass

    def crawl(self):
        def repo_formatter(scheme):
            return scheme['github_repo']['address'].replace('github.com', 'raw.githubusercontent.com') \
                   + '/master/colors/' + scheme['name'] + '.vim'

        # Loop over all the pages
        for page in range(self.total_pages):
            page_source = requests.get(self.base_url + str(page + 1))
            plain_text = page_source.text
            soup = BeautifulSoup(plain_text, 'lxml')

            # Get the data
            json_data = json.loads(soup.find('div', {'id': 'data'}).attrs['data-colorschemes'])

            # Download the files
            for data in json_data['colorschemes']:
                self.download(data['name'] + '.vim', repo_formatter(data))

colors_spider = Spider()
colors_spider.crawl()

Very nice code in general! Here are a few points, though:

Think about using os.makedirs instead of os.mkdir. The former allows creating nested directories (so os.makedirs("~/long/new/path") will create all other folders in betweetn as well, if needed).

You should never have a bare except. This prevents you to e.g. CTRL-C to abort the program, since it will also be caught. Use at least except Exception, which all normal exceptions inherit from (but not some special exceptions like pressing CTRL-C).

In general it is usually not the best idea to have a lot of string addition. You could use format instead in repo_formatter:

def repo_formatter(scheme):
    base_url = scheme['github_repo']['address'].replace('github.com', 'raw.githubusercontent.com')
    return '{}/master/colors/{}.vim'.format(base_url, scheme['name'])

You have some very obvious comments in your code, especially in download. Comments should explain why you do something and not what. The latter should be obvious from your code! And in that function it is really easy to follow what happens, because you have chosen appropriate variable names :)


Stored procedure to describe the reason that a particular employee is unsuitable for a particular task I used the following Stack Overflow questions as references for writing this code:

Select columns from result set of stored procedure
What is the equivalent of String.Join on TSQL?

The idea behind my stored procedure is to give an explanation of why a particular worker is unsuitable to care for a particular patient under specific circumstances. For example, you obviously can't assign an RN to do physical therapy visits.

I have a Disciplines table, which has a structure that's kind of like this:

--------------------------------------------
| Discipline | Discipline ID | Other Field |
--------------------------------------------
| RN         | 1             | 2           |
--------------------------------------------
| PT         | 2             | 5           |
--------------------------------------------
| CNA        | 3             | 7           |
--------------------------------------------
| MD         | 4             | 23           |
--------------------------------------------

Obviously this is hypothetical data and I've modified the column names a bit (this is mostly for illustration).

What I want is to retrieve the list of Disciplines as a single string and concatenate it with an existing string. Workers might have several disciplines. (This is determined in a table that's not shown). An example of one possible output would be The worker does not have the appropriate disciplines. They have the following discipline(s): RN, PT (if that employee is both a RN and a PT - not too likely, obviously, but let's assume that they are).

I haven't included the entire stored procedure, but this is pretty representative and is the main part that I'm looking for a critique of:

 -- @IsAppropriate is a bit type
 SELECT @IsAppropriate = dbo.fnCheckIfWorkerHasDisciplineForSvc(@BusinessUnitID, @WorkerID, @ServiceTypeID, GetDate(), GetDate())

IF @IsAppropriate = 0
BEGIN
    SET @output = 'The worker does not have the appropriate disciplines. They have the following discipline(s):'

    CREATE TABLE #Result
    (
            Discipline nvarchar(max),
            IsPrimary bit,
            EffectiveDate DateTime,
            EndDate DateTime
    )

            -- Get which discipline a particular worker practices (PT, MD, RN, etc.)
    INSERT #Result EXEC spGetWorkerDiscipline @WorkerID

    DECLARE @disciplines nvarchar(max);

            -- Get them as a nice list
    SELECT @disciplines = coalesce(@disciplines + ', ', '') + Discipline from #Result

    SET @output = @output + @disciplines

    DROP TABLE #Result

            -- Skip the rest of the stored procedure
            -- See comments below - is this a terrible thing to do?
    GOTO eom
END

-- Check some other reasons that the worker might be inappropriate...

-- Return the resulting string to the user
eom:
select @output

I verified that the query does, in fact, work. However, I have a few concerns:

Is using GOTO like that in SQL really terrible? If so, is there a better way to accomplish the same thing? I'm almost embarrassed to post code that contains it, since I'm aware that, in general, GOTO is considered harmful.
Will I take a performance hit for creating and dropping a temporary table like that? Could some of the other suggestions in the linked posts be a better choice here?
Am I inappropriately mixing my architectural layers by creating a string for use on the front end in the database like this? (I'm using ASP.NET Web Forms).

A handful of critiques:

As mentioned in the comments, this is undefined behavior: SELECT @val = @val + ColumnName FROM AnyTable. As such, you should avoid it. If you need to, use STUFF or STRING_AGG instead
When creating a temp table, (n)varchar columns should specify their collation (e.g. COLLATE DATABASE_DEFAULT ) to avoid errors in case server collation and database collation differ.
Using RETURN or a chain of IF ELSE IF etc is going to raise fewer eyebrows than GOTO
Don't just blindly insert into a table; always specify a column list.
Its cheaper to use table variables than temp tables; SQL Server doesn't need to do as much work for statistics and fewer latches of creation are required.
You generally don't need to drop the temp table (assuming this is called from a stored procedure) as that'll get done automatically by SQL Server
You could probably just return the list of disciplines + an error code, and let the consuming application handle presenting it in a friendly way. This also makes I18N someday in the future much easier.


Accept an object and generate a email I have a question regarding optimizing a php email class. It's basically a wrapper class than needs to be able to accept different objects and always output an email. I'm trying to make it generic, in it's current form it's working but I still feel like it may be to tightly coupled. I was hoping someone here could make some suggestions on how to improve it. I would really appreciate it.

    require_once 'PHPMailerAutoload.php';

    class Email{

    public $fromEmailAddress;
    public $displayName;
    public $body;
    public $toEmailaddress;

    public function sendEmail(Email $email){

        $mail = new PHPMailer;
        $mail->isSMTP(); // Set mailer to use SMTP
        $mail->Host = 'exploud-xxxxx.ny.xxxxxx.com'; // Specify main and backup server
        $mail->setFrom($this->fromEmailAddress, $this->displayName);
        //$mail->addAddress($email);
        $mail->addAddress($this->toEmailaddress); // Add a recipient
        $mail->addCC($this->fromEmailAddress);

        $mail->WordWrap = 50; // Set word wrap to 50 characters
        $mail->isHTML(true); // Set email format to HTML
        $mail->Subject = "Load Tender " . " - " . $this->displayName;
        $mail->Body = $this->body;

        if (!$mail->send()) {
            echo 'Message could not be sent.';
            echo 'Mailer Error: ' . $mail->ErrorInfo;
            exit;
        }
        return true;

    }

}

Different php file, auto load the Email class:
Take the Bill of Lading object that was created and email it

$bol = new Email();

$bol->body="Bill of Lading body";
$bol->displayName='Jon jonhnson';
$bol->fromEmailAddress = 'jraxxxxor@xxxxxx.com';
$bol->toEmailaddress = 'jraxxxxor@axxxxxandxxxs.com';

$bol->sendEmail($bol);
/**
 *
 *
 */

Take the tender object and email it
    $tender = new Email();

$tender->body="Tender body";
$tender->displayName='Jon jonhnson';
$tender->fromEmailAddress = 'jraynor@aol.com';
$tender->toEmailaddress = 'jraxxor@xxxxxxx.com';

$tender->sendEmail($tender);

The code is written in a nice, clean style.
It's easy to read like that, which helps the reviewer a lot, keep it up!

This sort of thing doesn't make much sense:

class Email {
    // ...
    public function sendEmail(Email $email) {

It's strange to pass an object of its own type as a method parameter.
Normally you would do such thing in a copy-constructor or some kind of clone or factory method, where you want to build a copy from the parameter.
But that's not the case here.
And in any case, the sendEmail implementation as it is doesn't use the $email parameter for anything: you can safely remove it.

Another big problem with this method is that it makes assumptions that are not enforced programmatically: before the method is called, body, displayName and other fields must be set. If you forget to set some of them, the program may still work and might not raise obvious errors, but you can experience nasty bugs.

Here's a safer alternative:

public function sendEmail($fromEmailAddress, $displayName, $body, $toEmailaddress) {
    // ...
}

There are no hidden assumptions here: the only way to call this method is by adding all parameters. You cannot forget any of them, the code won't compile.

Note that with this implementation the class becomes a utility class with no member data.
At which point it can be just a standalone function,
it doesn't really need to belong to a class.

Another issue is error handling:

if (!$mail->send()) {
    echo 'Message could not be sent.';
    echo 'Mailer Error: ' . $mail->ErrorInfo;
    exit;
}

So if email sending fails, you exit, effectively crashing the program.
Most probably you want to return false instead,
and the caller should handle gracefully.

Even if this script runs as a batch program without a user friendly web interface,
the exit points of programs are best controlled in one place,
the caller of this method, or even higher in the call stack,
not buried inside utility methods.


Raise error when slug already exists I looked into possible ways to raise an error to generate an error when a slug already exist.

This is the model:

# models.py
from django.db import models
class Question(models.Model):
    title = models.CharField(max_length=100)
    slug = models.SlugField(max_length=100, db_index=True, unique=True)

for which I use a generic view:

# views.py
from .models import Question
class QuestionCreate(generic.CreateView):
    model = Question
    fields = ['title', 'text']
    def form_valid(self, form):
        instance = form.save(commit=False)
        slug = slugify(form.cleaned_data['title'])
        if Question.objects.filter(slug=slug).exists():
             form.add_error("title","This Question already exist")
             return self.form_invalid(form)

The slug is generated from the question title.

Then it was brought to my attention that I don't need to save the form first:

class QuestionCreate(generic.CreateView):
    model = Question
    fields = ['title', 'text']
    def form_valid(self, form):
        slug = slugify(form.cleaned_data['title'])
        if Question.objects.filter(slug=slug).exists():
             form.add_error("title","This Question already exist")
             return self.form_invalid(form)

Is this the best practice?

Other possibilities I considered:

# models.py
from django.db import models, IntegrityError
from django.forms import ValidationError
class Question(models.Model):
    title = models.CharField(max_length=100)
    slug = models.SlugField(max_length=100, db_index=True, unique=True)
    def save(self):
        slug = slugify(self.title)
        try:
            self.slug = slug
            super().save()
        except IntegrityError:
            raise ValidationError("Question already exist")

but it seems not possible to display this error message to the user.

The final option I considered:

# models.py
from .forms import QuestionForm
class QuestionCreate(generic.CreateView):
    model_form = QuestionForm

# forms;py
from django.forms import ModelForm
from .models import Question

class QuestionForm(ModelForm):
    class Meta:
         model = Question
         fields = ['title', 'text']

I still have to figure out how to raise an error message in this last situation, it's somewhere in the docs.

Which is the best practice? Or is this all acceptable code?

The good

So, you're trying to allow users to create new questions, but those questions must have a slug which is unique. You're enforcing this on the database level with unique=True, which is a good start.

You are using a form to handle the creation of new questions, which is definitely the right way to do this. It'll also be really useful when you need to build in the ability to edit a question, since you can just reuse the same form.

The bad

But you're enforcing the uniqueness of a question outside of the form. The form ultimately has the business logic that determines whether or not the input is valid, which is why Django has built-in support for validating forms. If you find issue with isolating your form validation logic inside of a form, I would recommend breaking it out as general utilities which can be easily tested.

Right now you're enforcing these constraints either on the model level (through save, which is definitely not the right place) or on the view level (through form_valid, which is not right but also not horrible).

Django designed the generic views to be easy to implement, assuming you follow the standard Django conventions. If you don't follow the conventions, you end up with cases like your form_valid where you have to do additional validation and then decide that it's actually invalid, forcing them to form_invalid. You shouldn't have made it to form_valid, since the state is not actually valid.

The fix

You should move your uniqueness logic into the form. Since you are only validating a single field, the title field, you only need to handle the logic for that field. This means that you can isolate your logic inside a custom cleaning method.

class QuestionForm(ModelForm):

    class Meta:
         model = Question
         fields = ['title', 'text']

    def clean_title(self, title):
        from django.utils.text import slugify
        from django.core.exceptions import ValidationError

        slug = slugify(title)

        if Question.objects.filter(slug=slug).exists():
            raise ValidationError('A question with this title already exists.')

        return title

class QuestionCreate(generic.CreateView):
    form_class = QuestionForm

This means that you no longer need the form_valid override and you don't need to worry about having to replicate that validation logic in other places which use this form.


UIView position updates based on device motion via RxSwift I've created a simple function to work as a level, where the dot on screen is centered when the device is completely flat. It changes position based on device orientation. I use RxSwift to update the position of the dot. I'm relatively new to RxSwift, so I'm worried that I'm not using it correctly or to its full value.
I also don't understand capturing self all that well, so please feel free to analyze that as well.
Any input is appreciated!
class ViewController: UIViewController {

    @IBOutlet var levelDot: UIView!
    var positiveMax: (x: CGFloat, y: CGFloat)!

    override func viewDidLoad() {
        levelDot.center = self.view.center
        super.viewDidLoad()
        positiveMax = (view.frame.maxX, view.frame.maxY)
        initGryo()
    }

    fileprivate func initGryo() {
        let pitch = PublishSubject<Double>()
        let roll = PublishSubject<Double>()
        let motion = CMMotionManager()
        guard motion.isDeviceMotionAvailable else { return }
        motion.deviceMotionUpdateInterval = 1/60
        motion.startDeviceMotionUpdates()
        let gyroTimer = Timer(fire: Date(), interval: 1/60, repeats: true, block: { timer in
            if let attitude = motion.deviceMotion?.attitude {
                let yModifier: CGFloat = 1.3 // modifier is to make small changes seem bigger on the screen

                _ = pitch.map( { (self.positiveMax!.y + CGFloat($0)*self.positiveMax!.y * yModifier) })
                    .map( {$0 / 2} )
                    .subscribe(onNext: { centerY in

//prevent dot from moving off screen
                        guard centerY < self.positiveMax.y else {
                            self.levelDot.center.y = self.positiveMax.y
                            return
                        }
                        guard centerY > 0 else {
                            self.levelDot.center.y = 0
                            return
                        }
//dot position -- does this capture self? would making levelDot a weak var prevent this?
                        self.levelDot.center.y = centerY

                    })
                pitch.onNext(attitude.pitch)

                let xModifier: CGFloat = 1.15 // modifier is to make small changes seem bigger on the screen
                _ = roll.map( { (self.positiveMax!.x + CGFloat($0)*self.positiveMax!.x*xModifier) })
                    .map({ $0 / 2 })
                    .subscribe(onNext: { centerX in
//prevent dot from moving off screen
                        guard centerX < self.positiveMax.x else {
                            self.levelDot.center.x = self.positiveMax.x
                            return
                        }
                        guard centerX > 0 else {
                            self.levelDot.center.x = 0
                            return
                        }
//dot position
                        self.levelDot.center.x = centerX
                    })
                roll.onNext(attitude.roll)
            }
        })
        RunLoop.current.add(gyroTimer, forMode: .default)

    }

}

You are correct that you are not using it correctly. For example Subjects are only needed to convert non-rx code into Rx code or to handle cycles. The fact that you have two of them is problematic.
When working on an Observable sequence, start with the effect you want to achieve the figure out what causes the effect to change. In this case, the effect is the updating of the center of the levelDot view.
The center of a view doesn't have a reactive wrapper on it in RxCocoa, so the first step is to make that:
extension Reactive where Base: UIView {
    var center: Binder<CGPoint> {
        return Binder(base) { view, point in
            view.center = point
        }
    }
}

With the above, you will now be able to .bind(to: levelDot.rx.center). In order to do that you need an Observable<CGPoint> that is emitted every time the CMMotionManager updates. CoreMotion is also not in the RxCocoa library by default, but the tools are available in RxCocoa to wrap it.
extension Reactive where Base: CMMotionManager {
    func startDeviceMotionUpdates(to queue: OperationQueue) -> Observable<CMDeviceMotion> {
        return Observable.create { [base] observer in
            base.startDeviceMotionUpdates(to: queue, withHandler: handler(observer: observer))
            return Disposables.create { [base] in
                base.stopDeviceMotionUpdates()
            }
        }
    }
}

func handler<T>(observer: AnyObserver<T>) -> (T?, Error?) -> Void {
    return { data, error in
        if let data = data {
            observer.onNext(data)
        }
        else {
            observer.onError(error ?? RxError.unknown)
        }
    }
}

(I moved the handler into its own function because it can be used for many of the other CoreMotion handler callbacks.)
With the above, you can now call myCoreMotionManager.rx.startDeviceMotionUpdates(on:) with whatever queue you want to use. It will create an Observable. When that observable is subscribed to, it will call the supplied closure which calls the manager's startDeviceMotionUpdates(to:withHandler:) method. The handler will send onNext events to the server whenever the device motion updates. When the observable is unsubscribed to, the disposable will turn off the updates.
Now, it's just a matter of mapping from one to the other which can be done with a pure function. Since you have outside data that is needed in that function, you need a function factory:
func point(limiatedBy positiveMax: CGPoint) -> (CMDeviceMotion) -> CGPoint {
    return { deviceMotion in
        let attitude = deviceMotion.attitude
        let x = max(0, min(positiveMax.x, positiveMax.x + CGFloat(attitude.roll) * positiveMax.x * 1.15 / 2))
        let y = max(0, min(positiveMax.y, positiveMax.y + CGFloat(attitude.pitch) * positiveMax.y * 1.3 / 2))
        return CGPoint(x: x, y: y)
    }
}

Lastly, you forgot the DisposeBag to unwind everything when the view controller goes out of scope...
With all of the above available, your view controller is quite simple. It almost writes itself and you might want to try writing it before you examine the code below:
class ViewController: UIViewController {

    @IBOutlet var levelDot: UIView!
    var positiveMax: CGPoint = CGPoint.zero
    let disposeBag = DisposeBag()

    override func viewDidLoad() {
        super.viewDidLoad()
        levelDot.center = CGPoint(x: view.bounds.midX, y: view.bounds.midY)
        positiveMax = CGPoint(x: view.bounds.maxX, y: view.bounds.maxY)
        initGryo()
    }

    fileprivate func initGryo() {
        let motion = CMMotionManager()
        guard motion.isDeviceMotionAvailable else { return }
        motion.deviceMotionUpdateInterval = 1/60

        motion.rx.startDeviceMotionUpdates(to: OperationQueue.main)
            .map(point(limiatedBy: positiveMax))
            .bind(to: levelDot.rx.center)
            .disposed(by: disposeBag)
    }
}

The other critique you asked for involves capturing self... You should never capture self in a .map(_:) or filter for that matter. Just don't do it.


Implementation of HeapSort The following code is an implementation of heapsort on an array

public static void heapSort(int[] inputArray){
    /* Creates an array A which will contain the heap */
    /* A has size n+1 to allow 1-based indexing */
    int n = inputArray.length;
    int[] A = new int[n+1];
    int temp = 0;

    /* Copies the array inputArray into A, with inputArray[i] being stored in A[i+1] */

    for(int i=0; i<n; i++){
        A[i+1] = inputArray[i];
    }
    constructHeap(A, n, 1);
    removeMax(A, n);
    copyBack(A, inputArray);
}

    /* Transforms A into a max-heap using a 'bottom-up' algorithm. */
public static void constructHeap(int[] A, int n, int i){
    if(2*i>n) return;
    constructHeap(A, n, 2*i);
    constructHeap(A, n, 2*i+1);
    bubbleDown(A, n, i);
}
/*recursively swaps parent/child relationships until the max-heap property is satisfied. */
public static void bubbleDown(int[] A, int n, int i){
    if(2*i>n) return;
    int leftChild = 2*i;
    int rightChild = 2*i+1;
    int max = leftChild;
    if(rightChild<=n && A[max]<A[rightChild]){
        max = rightChild;
    }
    if(A[i]<A[max]){
        int temp = A[i];
        A[i] = A[max];
        A[max] = temp;
        bubbleDown(A, n, max);
    }
}

    /* Performs a sequence of n 'remove-maximum' operations, storing the removed element at
       each step in successively smaller indices of A */

public static void removeMax(int[] A, int i){
    for(int i=n; i>0; i--){
        int temp = A[1];
        A[1] = A[i];
        bubbleDown(A, i, 1);
        A[i] = temp;
}

    /* Copies the sorted values in A back into inputArray, with inputArray[i] getting
       the value from A[i+1] */

public static void copyBack(int[] A, int[] inputArray){
    for(int i=0; i<inputArray.length; i++){
        inputArray[i] = A[i+1];
    }
}

Your constructHeap method works in O(n), and you call in O(n) times from removeMax method, so your code works in O(n^2), so it is not a correct implementation of Heapsort.

Comments:

public static void heapSort(int[] inputArray) {

Why do you need another array? Heapsort is in-place.

  /* Creates an array A which will contain the heap */

Why do you need 1-based indexing? You don't seem to use it anywhere, and 0-based is more convenient.

  /* A has size n+1 to allow 1-based indexing */
  int n = inputArray.length;

  int[] A = new int[n + 1];

You don't use this variable.

  int temp = 0;

  /* Copies the array inputArray into A, with inputArray[i] being stored in A[i+1] */

You should replace this loop with System.arraycopy(...) call

  for (int i = 0; i < n; i++) {
    A[i + 1] = inputArray[i];
  }
  constructHeap(A, n, 1);
  removeMax(A, n);
  copyBack(A, inputArray);
}

Consider transforming such comments into valid javadoc.

/* Transforms A into a max-heap using a 'bottom-up' algorithm. */
public static void constructHeap(int[] A, int n, int i) {
  if (2 * i > n) {
    return;
  }
  constructHeap(A, n, 2 * i);
  constructHeap(A, n, 2 * i + 1);
  bubbleDown(A, n, i);
}

Comment?

public static void bubbleDown(int[] A, int n, int i) {
  if (2 * i > n) {
    return;
  }
  int leftChild = 2 * i;
  int rightChild = 2 * i + 1;
  int max = leftChild;
  if (rightChild <= n && A[max] < A[rightChild]) {
    max = rightChild;
  }
  if (A[i] < A[max]) {
    int temp = A[i];
    A[i] = A[max];
    A[max] = temp;
    bubbleDown(A, n, max);
  }
}

  /* Performs a sequence of n 'remove-maximum' operations, storing the removed element at
     each step in successively smaller indices of A */

public static void removeMax(int[] A, int i) {
  if (i == 0) {
    return;
  }
  int temp = A[1];
  A[1] = A[i];
  constructHeap(A, i, 1);
  A[i] = temp;

So you make O(n) recursive calls? This is causing StackOverflowException with large n and harming your running time. Consider transforming this tail recursion into a loop.

  removeMax(A, i - 1);
}

  /* Copies the sorted values in A back into inputArray, with inputArray[i] getting
     the value from A[i+1] */

public static void copyBack(int[] A, int[] inputArray) {

ditto

  for (int i = 0; i < inputArray.length; i++) {
    inputArray[i] = A[i + 1];
  }
}


Stack data structure in Python 3 I have implemented a stack in Python 3, and it works well. If anything needs to be improved, I would appreciate the criticism.

class Stack:

        def __init__(self):
            self.stack = []

        def isEmpty(self):
            return self.stack == []

        def push(self, data):
            self.stack.append(data)

        def pop(self):
            data = self.stack[-1]
            del self.stack[-1]
            return data

        def peek(self):
            return self.stack[-1]

        def sizeStack(self):
            return len(self.stack)

stack = Stack()
stack.push(1)
stack.push(2)
stack.push(3)
print(stack.sizeStack())
print("Popped: ", stack.pop())
print("Popped: ", stack.pop())
print(stack.sizeStack())
print("Peek:", stack.peek())
print(stack.sizeStack())

From a pure Python standpoint, a list is already a stack if you narrow its methods down to only pop and append.
l.append(x) will put x at the end of the list, and l.pop() will remove the last element of the list and return it.

So generally speaking, when you need a stack in Python, you'll just make a list and use append and pop.
The same goes for queues, but you'd use l.pop(0) instead of l.pop().

Considering this, I would say it's just plain unpythonic to implement a stack structure; instead, just use a list, that really is a dequeue and thus can be used as both a stack and a queue.

That said, I understand that append is not the common naming for the stack structure, and that push makes the intent of the method more clear.
Therefore, I would implement a stack as a direct subclass of list, and just "rename" the append method:

class Stack(list):
    def push(self, x):
        super().append(x)

If I wanted to make it more fool-proof, I'd also override pop so that it cannot take a parameter, unlike list.pop

class Stack(list):
    ...
    def pop(self):
        return super().pop()

Now the operation performed by the peek method can usually be done by stack[-1] (where stack is an instance of Stack), but it could be implemented just as follows:

class Stack(list):
    ...
    def peek(self):
        return self[-1]

Addendum

Some comments advising against subclassing list came out, notably mentioning the "composition over inheritance" principle.
That was an enriching discussion, and I'd like to amend my answer accordingly.

I'm no expert in design patterns, but as I understand it, this principle advocates the use of composition instead of inheritance to provide more flexibility.
One benefit of composition here is clearly to provide a cleaner API, with only the common stack methods and not insert, remove and so on that come from list and have no sense for stacks.

On the other hand, the inheritance I suggested happens to violate Liskov's substitution principle.
Although there's no problem with adding push and peek methods with respect to this principle, my implementation changes the signature of the pop method, which is not allowed by this principle.

From this perspective, I think that not only composition is valid here and adds in the benefit of a clean API, but also that inheritance is plain incorrect here.
Therefore, if I were forced to implement a stack class in Python, I'd go with composition and do as follows:

class Stack:
    def __init__(self):
        self._data = []

    def push(self, x):
        self._data.append(x)

    def pop(self):
        return self._data.pop()

    def peek(self)
        return self._data[-1]

    def __len__(self):
        return len(self._data)

But again, as I said, I would never do that in real life because a list can already serve as a stack, and stack = [] would perfectly convey how stack would be intended to be used.

References:

On the Liskov substitution principle:

https://en.wikipedia.org/wiki/Liskov_substitution_principle
https://stackoverflow.com/q/56860/7051394

On composition over inheritance:

https://en.wikipedia.org/wiki/Composition_over_inheritance
https://stackoverflow.com/a/53354/7051394 (also happens to mention Liskov principle)


Ruby on rails instance loops I am trying to use a custom sql query to display different attributes for a product i.e. Size and Price. The query I have when running in console displays as it should

SELECT products.id, products.name, variant_properties.description, LEFT(variant_properties.description,1) as short_desc, variants.price FROM products
  INNER JOIN product_properties ON product_properties.product_id = products.id
  INNER JOIN variant_properties on product_properties.property_id = variant_properties.property_id AND variant_properties."primary" = true
  INNER JOIN properties ON properties.id = product_properties.property_id AND properties.id = variant_properties.property_id AND properties.display_name = 'Size'
  INNER JOIN variants on variants.product_id = products.id AND variants.id = variant_properties.variant_id

In my HAML template I have done the following

- @products.each_with_index do |product, i|
  .product-list.grid-block
    .small-8.grid-content.text-center
      %h4= product.name.titlecase
      - @sizes.each do |size|
        = link_to size.short_desc, product, class: 'hollow button tiny'
        %small= size.price

and in the controller

products = Product.active
    # products = Product.active.includes(:variants)

    product_types = nil
    if params[:product_type_id].present? && product_type = ProductType.find_by_id(params[:product_type_id])
      product_types = product_type.self_and_descendants.map(&:id)
    end
    if product_types
      @products = products.where(product_type_id: product_types)
    else
      @products = products
    end

      @sizes = Product.find_by_sql("SELECT products.id, LEFT(variant_properties.description,1) as short_desc, variants.price FROM products
  INNER JOIN product_properties ON product_properties.product_id = products.id
  INNER JOIN  variant_properties on product_properties.property_id = variant_properties.property_id
  INNER JOIN properties ON properties.id = product_properties.property_id AND properties.id = variant_properties.property_id AND properties.display_name = 'Size'
  INNER JOIN  variants on variants.product_id = products.id AND variants.id = variant_properties.variant_id")

I'm not seeing anything wrong with your SQL or your view. The controller can be cleaned up a lot. In fact, you could reduce your controller method to two lines.

The Product.find_by_sql call should be encapsulated in the Product class as its own static method
The SQL should be stored in a constant
The logic for finding products by product type, or defaulting to active products should also be a static method on the Product class

Basically, you should be able to do this in your controller:

@products = Product.find_by_product_types params[:product_type_id]
@sizes = Product.sizes

The Product class could be amended like below:

class Product < ActiveRecord::Base
  SQL_FIND_SIZES = "SELECT products.id,
                           LEFT(variant_properties.description,1) AS short_desc,
                           variants.price
                    FROM products
                    INNER JOIN product_properties ON product_properties.product_id = products.id
                    INNER JOIN variant_properties ON product_properties.property_id = variant_properties.property_id
                    INNER JOIN properties ON     properties.id           = product_properties.property_id
                                             AND properties.id           = variant_properties.property_id
                                             AND properties.display_name = 'Size'
                    INNER JOIN variants ON variants.product_id = products.id AND variants.id = variant_properties.variant_id"
  def self.sizes
    self.find_by_sql SQL_FIND_SIZES
  end

  def self.find_by_product_types(product_type_id)
    return [] if product_type_id.nil?

    product_type = ProductType.find_by_id product_type_id
    product_types = product_type.self_and_descendants.map(&:id) if product_type

    return if product_types.nil?
             self.active
           else
             self.where product_type_id: product_types
           end
  end
end


Using EF 6.1 for updating objects and their children I have an application that allows a user to create a poll that includes questions and answer choices as part of the poll. The hierarchy is a poll has one or more questions, a question has one or more answers, and each answer has a vote.

The following code is my implementation that allows the updating of an existing poll. The update allows for properties of the poll to be updated, including the addition or removal of children.

The implementation works, but it feels like there is a better way to achieve the functionality. As you will see from my code, I'm creating a context object each time I want to add or update any of the objects.

 public string Update(Poll poll)
    {
        try
        {
            if (poll == null)
                return null;

            using (var context = new PollContext())
            {
                context.Entry(poll).State = poll.PollId == Guid.Empty ? EntityState.Added : EntityState.Modified;
                context.Polls.AddOrUpdate(poll);
                context.SaveChanges();
            }

            //update the questions
            using (var context = new PollContext())
            {
                foreach (var question in poll.Questions)
                {
                    context.Entry(question).State =
                        question.QuestionId == Guid.Empty ? EntityState.Added : EntityState.Modified;
                    context.Questions.AddOrUpdate(question);
                }

                context.SaveChanges();
            }

            //update the answers
            using (var context = new PollContext())
            {
                foreach (var answers in poll.Questions.Select(question => question.Answers).ToList())
                {
                    var questionId = answers.ElementAt(0).QuestionId;
                    var originalAnswerList = GetOriginalAnswerList(questionId);

                    using (var context2 = new PollContext())
                    {
                        foreach (var originalAnswer in originalAnswerList)
                        {
                            if (answers.All(a => a.AnswerId != originalAnswer.AnswerId))
                            {
                                Delete(originalAnswer);
                                Delete(originalAnswer.Votes);
                            }
                        }
                    }

                    foreach (var answer in answers)
                    {
                        context.Entry(answer).State =
                            answer.AnswerId == Guid.Empty ? EntityState.Added : EntityState.Modified;
                        context.Answers.AddOrUpdate(answer);

                        context.Votes.AddOrUpdate(answer.Votes);
                    }
                }

                context.SaveChanges();
            }

            return "Poll updated";
        }
        catch (NullReferenceException ex)
        {
            throw new NullReferenceException("A null exception occurred in updating poll.\r\n" + ex.InnerException);
        }
        catch (Exception ex)
        {
            throw new Exception("An exception occurred in the updating poll.\r\n" + ex.InnerException);
        }

    }

You don't have to use a using statement. You can use an IoC container such as Autofac and let it dispose the PollContext for you.
I think you probably should update the children collection in memory first and call context.SaveChange() only once at the end, so the whole update operations are wrapped with in a unit of a work.
I don't think try/catch is needed here as you only re-throw the exception, which could be handled on a higher level.
There's lots of repeating of:

context.Entry(poll).State = poll.PollId == Guid.Empty ? EntityState.Added : EntityState.Modified;
context.Polls.AddOrUpdate(poll);

You should probably create a generic function for it.


Network helper class with retry logic on failure I wrote an helper class which allow me to request the body content from a site that is created by AJAX, for doing so I'm using Puppeteer Sharp. This is the class:

using System;
using System.IO;
using System.Net;
using System.Text;
using System.Threading.Tasks;
using PuppeteerSharp;

namespace App.Helpers
{
    public class NetworkHelper
    {
    /// <summary>
    /// Get the html of a page waiting for an AJAX call until the selector is visible.
    /// </summary>
    /// <param name="url">Link needed for execute the request.</param>
    /// <param name="selector">Selector to wait.</param>
    /// <param name="attempts">Max attempts until request fail.</param>
    /// <returns>Contains the html of the requested page.</returns>
    public static async Task<string> LoadAndWaitForSelector(Uri url, string selector, int attempts = 5)
    {
        try
        {
            //Create new browser page
            using (Page page = await Handler.Browser.NewPageAsync())
            {
                //Navigate on the requested content and wait for selector
                await page.GoToAsync(url.ToString(), timeout: 0);
                await page.WaitForSelectorAsync(selector); //may throw error for timeout
                return await page.GetContentAsync();
            }
        }
        catch (WaitTaskTimeoutException)
        {
            //there are other attempts, rerty
            if (attempts != 0)
            {
                attempts--;

                //add some delay
                await Task.Delay(10000);

                return await LoadAndWaitForSelector(url, selector, attempts);
            }
            throw;
        }
    }
}

Question: Could this class be improved more?

Also, the Browser is static, so each instance use the same browser for avoid memory leak:

public static Browser Browser { get; set; }

the method is called in the following way:

var html = await NetworkHelper.LoadAndWaitForSelector(
            new Uri("some url"), "#archive-tables");

thanks.

Just wanted to add that you may want to consider using Polly instead of rolling your own retry mechanism - this is a library built specifically for this kind of retry mechanism (as well as many more complicated scenarios). Using Polly would look something like the following.

Note that I have renamed the attempts parameter to retryAttempts to make it clear that this does not define the total number of attempts but rather the total number of retry attempts after the initial one, to keep the behaviour the same as your sample code.

public class NetworkHelper
{
    public static Task<string> LoadAndWaitForSelectorAsync(
        Uri url, string selector, int retryAttempts = 5)
    {
        // Create a policy that will...
        var policy = Policy
            // Retry for any `WaitTaskTimeoutException` raised during execution.
            .Handle<WaitTaskTimeoutException>()
            .WaitAndRetryAsync(
                // Retry the specified number of times after the initial attempt.
                retryCount: retryAttempts,
                // Wait for the given duration between each retry attempt. Note that
                // this can depend on the retry number if required.
                sleepDurationProvider: retryNumber => TimeSpan.FromSeconds(10));

        // Execute the operation to get the selector within the retry policy. If the
        // final retry attempt does not succeed, the resulting `WaitTaskTimeoutException`
        // will not be handled by the policy and will be thrown.
        return policy.ExecuteAsync(() => TryGetSelectorAsync(url, selector));
    }

    private static async Task<string> TryGetSelectorAsync(Uri url, string selector)
    {
        using (Page page = await Handler.Browser.NewPageAsync())
        {
            await page.GoToAsync(url.ToString(), timeout: 0);
            await page.WaitForSelectorAsync(selector);
            return await page.GetContentAsync();
        }
    }
}


Class for database connections in PHP I have made this class for database connections to my applications. It eases out the task of making connections to the database with knowing the credentials in advance and provides a way to execute the queries.

Class dbConnection {

    public $CONN;
    private $dbservername, $dbusername, $dbpassword, $dbname;

    public function __construct() {

        $this->dbservername = "localhost";
        $this->dbusername = "dbroot";
        $this->dbpassword = "pass";
        $this->dbname = "test";
        $this->open(); //open the connection on instantization
    }

    public function open() {

    // Create connection
        $this->CONN = new mysqli($this->dbservername, $this->dbusername, $this->dbpassword, $this->dbname);

        // Check connection
        if ($this->CONN->connect_error) {
            die("Connection failed: " . $CONN->connect_error);

        }

    }

    public function prepare($query) {
    //prepare the data
        return $this->CONN->prepare($query);

    }

    public function close() {
    //close the connection
        $this->CONN->close();

    }

}

I am using prepared statements in the queries, of course.

I would like a review to determine if the function is feasible in all situations with performance and security on top considerations. I want to know if I should make any adjustments to it. Also, a comment on the coding style would be of great use.

Structure

Currently, there is no reason to have prepare or close, they are just delegating to the public field. A user of your class may just as well use that, and may be confused about the difference.

There is also no need for open() to be public, as there is no need to ever reopen the connection.

If you think you do need a wrapper around mysqli, it should: 1) actually add new functionality or make the existing functionality considerably easier to use 2) be an extensive wrapper, so that there is no need to make CONN public so that it can be accessed directly.

Also, don't die in a class, it makes it difficult to (re-)use. What if you want to fall back on something else? Or show a proper error message? Throw an exception instead.

Also, it would be a good idea to pass the credentials to the class, so it is more flexible (you may eg want to load them from a config file or at least a file only containing the credentials, so you can exclude them from versioning, etc).

Style

Your indentation is off, and you have quite a bit of unnecessary vertical whitespace.

Variable names should all follow a common naming scheme, for example camelCase. I see no apparent reason why $CONN is all uppercase.

Class names should start with an upper-case character.

Comments

Your comments just repeat what the code already told me (close() closes, prepare() prepares, and so on). Comments like these will lead me to believe that all your comments will not be useful to me, which leads to me missing comments that actually are important.

Result

If you follow all my advise, your code may look like this:

class DBConnection {

    public $connection;

    public function __construct($dbServername, $dbUsername, $dbPassword, $dbNam) {
        $this->connection = new mysqli($dbServername, $dbUsername, $dbPassword, $dbName);
        if ($this->connection->connect_error) {
            throw Exception("Connection failed: " . $connection->connect_error);
        }
    }
}

Wrapping mysqli like this may still give you some flexibility later on, but personally, I'm not sure that it is worth it. I would either use mysqli directly, or wrap it in such a way that no element of mysqli is actually required outside of the class.