Checking directional moves for a game I am making a 2D Java game at school, and at the movement I use a switch. The code works, however, my teacher won't sign my code until I have removed code duplication.

Short info: My Keyactionlistener sends a String of direction that is used in the switch to move. Inside that very same switch I also check the object in the next field to see if it can be picked up or can move through. However, the code is really long this way and I need to shorten it down somehow.

public void checkAndMove(String direction) {
        switch (direction) {

            case "up":
                if (!field.checkIfBlocked(getfieldX(), getfieldY() - 1)) {
                    if (field.checkIfItem(getfieldX(), getfieldY() - 1).equals("friend")) {
                        showEndMessage = true;
                    }
                    if (field.checkIfItem(getfieldX(), getfieldY() - 1).equals("bazooka")) {
                        plusAmmo();
                        levelmaker.removeBazooka();
                    }
                    if (field.checkIfItem(getfieldX(), getfieldY() - 1).equals("helper")) {
                        showShortestRoute();
                    }
                    move(0, -1);
                    if(showEndMessage == true){
                        endMessage();
                    }
                    levelmaker.scorePlusPlus();
                    changeImage("imgUp");
                    break;
                } else {
                    break;
                }
            case "down":
                if (!field.checkIfBlocked(getfieldX(), getfieldY() + 1)) {
                    if (veld.checkIfItem(getfieldX(), getfieldY() + 1).equals("friend")) {
                        endMessage();
                    }
                    if (field.checkIfItem(getfieldX(), getfieldY() + 1).equals("bazooka")) {
                        plusAmmo();
                        LevelMaker.removeBazooka();
                    }
                    if (field.checkIfItem(getfieldX(), getfieldY() + 1).equals("helper")) {
                        showShortestRoute();
                    }
                    move(0, 1);
                    levelmaker.scorePlusPlus();
                    changeImage("imgDown");
                    break;
                } else {
                    break;
                }
            case "left":
                if (!field.checkIfBlocked(getfieldX() - 1, getfieldY())) {
                    if (field.checkIfItem(getfieldX() - 1, getfieldY()).equals("friend")) {
                        endMessage();
                    }
                    if (field.checkIfItem(getfieldX() - 1, getfieldY()).equals("bazooka")) {
                        plusAmmo();
                        LevelMaker.removeBazooka();
                    }
                    if (field.checkIfItem(getfieldX() - 1, getfieldY()).equals("helper")) {
                        showShortestRoute();
                    }
                    move(-1, 0);
                    levelmaker.scorePlusPlus();
                    changeImage("imgLeft");
                    break;
                } else {
                    break;
                }
            case "right":
                if (!field.checkIfBlocked(getfieldX() + 1, getfieldY())) {
                    if (field.checkIfItem(getfieldX() + 1, getfieldY()).equals("friend")) {
                        endMessage();
                    }
                    if (veld.checkIfItem(getfieldX() + 1, getfieldY()).equals("bazooka")) {
                        plusAmmo();
                        LevelMaker.removeBazooka();
                    }
                    if (field.checkIfItem(getfieldX() + 1, getfieldY()).equals("helper")) {
                        showShortestRoute();
                    }
                    move(1, 0);
                    levelmaker.scorePlusPlus();
                    changeImage("imgRight");
                    break;
                } else {
                    break;
                }
        }

Put these

if (!field.checkIfBlocked(getfieldX() + 1, getfieldY())) {
    if (field.checkIfItem(getfieldX() + 1, getfieldY()).equals("friend")) {
           endMessage();
    }
    if (field.checkIfItem(getfieldX() + 1, getfieldY()).equals("bazooka")) {
        plusAmmo();
        LevelMaker.removeBazooka();
    }
    if (field.checkIfItem(getfieldX() + 1, getfieldY()).equals("helper")) {
        showShortestRoute();
    }

into a function.

Example:

public boolean performActions(int x, int y) {
    if(field.checkIfBlocked(x, y)
        return false;

    String status = field.checkIfItem(x, y);
    if(status.equals("friend")) {
        endMessage();
    } if(status.equals("bazooka")) {
        plusAmmo();
        LevelMaker.removeBazooka();
    } if(status.equals("helper")) {
        showShortestRoute();
    }
    return true;
}

and checkAndMove() becomes:

public void checkAndMove(String direction) {
    switch (direction) {

        case "up":
            if (performActions(getfieldX(), getfieldY() - 1)) {

                // rest of the stuff
            }
            break;
        case "down":
            if (performActions(getfieldX(), getfieldY() + 1)) {

                // rest of the stuff
            }
            break;
        case "left":
            if (performActions(getfieldX() - 1, getfieldY())) {

                // rest of the stuff
            }
            break;
        case "right":
            if (performActions(getfieldX() + 1, getfieldY())) {

                // rest of the stuff
            }
            break;
    }
}


Login script that checks two users and their corresponding passwords Please let me know what you think. Is the code well written?  Am I using best practices (like when I chose === over ==)?  Is my script too verbose?

Note: I'm only asking because the tutorial I'm using is a little old.

/* A login script to that checks two users and their
corresponding passwords and then greets one of them
*/
var input = '',
  guess = '',
  montyPswrd = 'Cheese',
  montyUser = 'Monty',
  chipUser = 'Chip',
  chipPswrd = 'Gadget';

var isMonty = function (name) {
    if (name === montyUser) {
        document.write('
Welcome to the site ' + montyUser + '
');
    } else {
        alert('Your not monty');
    }
};

var isChip = function (name) {
    if (name === chipUser) {
        document.write('
Welcome to the site ' + chipUser + '
');
    } else {
        alert('Your not chip');
    }
};

input = prompt('Enter Username:', 'username');

switch (input) {
    case montyUser:
        guess = prompt('Enter Password','password');
        break;
    case chipUser:
        guess = prompt('Enter Password','password');
    case null:
        break;
    default:
        alert('wrong useranme!');
        break;
}

paswrdCheck:
if (guess === '') {
    // then the user hit ESC or cancel
    // no point in asking for password
    break paswrdCheck;
} else {
    switch (guess) {
    case montyPswrd:
        isMonty(input); // makes sure monty is matched with right password
        break;
    case chipPswrd:
        isChip(input); // makes sure chip is matched with right password
        break;
    case null:
        break;
    default:
        alert('wrong password!');
        break;
    }
}

This is tagged programming-challenge, but just to be sure it's said: This obviously is not secure in any sense; the passwords are in the source.
Anyway, I'm assuming security isn't the point.

First of all: Spellcheck. "useranme" and "Your not chip/monty".

Secondly, my question is "why 2 users?" What if it's 3? Or 6,123,345? Right now you'd have to duplicate everything in your code to accommodate more users. Obviously that'll be extremely cumbersome.

A much better approach is to write code to check any given user/password against a list of users (rather than named variables).

So, first we'll need a list of users (or, in this case, a JS object):

var users = {
  'Monty': 'Cheese',
  'Chip':  'Gadget'
};

Then, some login logic (this can be a function, or several functions, or many other things - this is just a simple implementation)

var username = prompt('Enter Username:', 'username');
if( !username ) { // if username is blank, null or otherwise false'y
  return; // stop here if no name is given
}

// alert and stop if the username doesn't exists
if( !users.hasOwnProperty(username) ) {
  alert('Unknown username');
  return;
}

// now we can get and check the password
var password = prompt('Enter Password:','password');
if( users[username] === password ) {
  document.write('
Welcome to the site ' + username + '
');
} else {
  alert("You're not " + username + "!");
}

And done.


Displaying random quotes from an array Is there anything that I could have done better in this code?

<!DOCTYPE html>

<meta charset="utf-8">
<title>Quotes</title>

I think your code looks pretty good already. It follows conventions and your indentation and variable names are precise. There's not much to review.

But if you're targeting modern browsers (IE9+) I would suggest a different approach using map and would separate the random logic into a function. I'd also declare the element up top so I know at first glance that the code is working with the DOM. It may seem like more code at first but it's good for code reuse, plus I think it reads better; the intent is more clear:

(function() {
  "use strict";

  var el = document.getElementById("quotes");

  var quotes = [
    ["Stay Hungry. Stay Foolish.", "Steve Jobs"],
    ["Good Artists Copy, Great Artists Steal.", "Pablo Picasso"],
    ["Argue with idiots, and you become an idiot.", "Paul Graham"],
    ["Be yourself; everyone else is already taken.", "Oscar Wilde"],
    ["Simplicity is the ultimate sophistication.", "Leonardo Da Vinci"]
  ];

  function rand(xs) {
    return xs.slice(0).sort(function(){
      return .5 - Math.random();
    });
  }

  function quote(q) {
    return "
&ldquo;"+ q[0] +"&rdquo; &mdash; "+ q[1] +"
";
  }

  el.innerHTML = rand(quotes).map(quote).join('');

}());


Getting the neighbors of a Point in a 2D grid I am trying to find an algorithm to find all the defined neighbors of a 2D Point (efficiently would be nice, but does not make much of a difference).

Here is my existing code:

private static ArrayList<Point> getNeighborsOfPoint(int i, int j) {
        ArrayList<Point> neighbors = new ArrayList<Point>();
        if (CONSIDER_CORNERS) {
            if (i < rows && j < cols && i >= 1 && j >= 1 && array[i][j] != 0) {
                neighbors.add(new Point(i-1, j-1));
            }
            if (i < rows && j < cols-1 && i >= 1 && j >= 0 && array[i][j] != 0) {
                neighbors.add(new Point(i-1, j+1));
            }
            if (i < rows-1 && j < cols && i >= 0 && j >= 1 && array[i][j] != 0) {
                neighbors.add(new Point(i+1, j-1));
            }
            if (i < rows-1 && j < cols-1 && i >= 0 && j >= 0 && array[i][j] != 0) {
                neighbors.add(new Point(i+1, j+1));
            }
        }

        if (i < rows && j < cols && i >= 1 && j >= 0 && array[i][j] != 0) {
            neighbors.add(new Point(i-1, j));
        }

        if (i < rows && j < cols && i >= 0 && j >= 1 && array[i][j] != 0) {
            neighbors.add(new Point(i, j-1));
        }
        if (i < rows && j < cols-1 && i >= 0 && j >= 0 && array[i][j] != 0) {
            neighbors.add(new Point(i, j+1));
        }

        if (i < rows-1 && j < cols && i >= 0 && j >= 0 && array[i][j] != 0) {
            neighbors.add(new Point(i+1, j));
        }

        return neighbors;
    }

CONSIDER_CORNERS is a boolean that is settable by the user, and Point is just a simple class that has an x and y int value. rows and cols are the number of elements in each of the two dimensions.

What I'm looking for mainly is better readability.

Improvements

Use x and y names to avoid confusion about what is i and what is j
I personally find it preferrable to put the x parameter first
rows and cols should not be static variables, so neither should this method
Use a double for-loop to loop over the neighbors
Declare by interface and not implementation (use List<Point> when possible)
Use some maths to check if you're at a corner or not

Code
private List<Point> getNeighborsOfPoint(int x, int y) {
    List<Point> neighbors = new ArrayList<Point>();
    for (int xx = -1; xx <= 1; xx++) {
        for (int yy = -1; yy <= 1; yy++) {
            if (xx == 0 && yy == 0) {
                continue; // You are not neighbor to yourself
            }
            if (!CONSIDER_CORNERS && Math.abs(xx) + Math.abs(yy) > 1) {
                continue;
            }
            if (isOnMap(x + xx, y + yy)) {
                neighbors.add(new Point(x + xx, y + yy));
            }
        }
    }
    return neighbors;
}

public boolean isOnMap(int x, int y) {
    return x >= 0 && y >= 0 && x < cols && y < rows;
}

In this code, I have not added your array[i][j] != 0 check, but that can easily be added into isOnMap, although you could then rename it to isValidNeighbor or something.
Going the extra mile
Having this method, it is very easy to add support for some very crazy neighbor styles. This might not fit your game, but using crazy neighbor styles can transform a concept into an entirely different game.
For example, take Minesweeper Flags (where the objective is to find the mines and not avoid them).

We all know what an 8 means, right? 8 mines around the number, plain and simple.

Now, what if we transform the way we look for neighbors? To what I call "Chess style" (based on how a Knight moves in Chess).

This "simple" change leads to a much greater challenge when playing, leading to all kinds of fun.
The change required for this is really simple, extract a NEIGHBOR_SET array:
private static final Point[] NEIGHBOR_SET = new Point[]{ new Point(-2, -1), new Point(-2, 1),
   new Point(-1, -2), new Point(-1, 2), new Point(1, -2), new Point(1, 2), new Point(2, -1), new Point(2, 1) };

private List<Point> getNeighborsOfPoint(int x, int y) {
    List<Point> neighbors = new ArrayList<Point>();
    for (Point neighbor : NEIGHBOR_SET) {
        if (isOnMap(x + neighbor.x, y + neighbor.y)) {
            neighbors.add(new Point(x + neighbor.x, y + neighbor.y));
        }
    }
    return neighbors;
}

Using this method, the special case logic for handling corners and the self neighbor is no longer necessary.
Of course, using the ordinary neighbor set is also possible with this method:
private static final Point[] NEIGHBOR_SET = new Point[]{ new Point(-1, -1), new Point(-1, 0),
   new Point(-1, 1), new Point(0, -1), new Point(0, 1), new Point(1, -1), new Point(1, 0), new Point(1, 1) };


CGI for htpasswd registration I decided to go with a very simple CGI user registration program to pair with Apache's mod_auth_form wrapper around a private site. It was only just after I finished writing that I realized I'd written security-sensitive code in a language I'm not my best at.

Is this code secure from parameter injection attacks? I.e., if the attacker fiddles with the POST values, can they run arbitrary commands on the server as the web server user?

#!/bin/bash

echo "Content-type: text/html"
echo ''

cat <<EOT
<!doctype html>

                <title>Registered</title>
                <link rel="icon" href="/favicon.png">

EOT

POST=$(cat)
if [ "$REQUEST_METHOD" != "POST" ] || [ ! "$POST" ]; then
        echo "
Error

Please go back and try again
";
        exit 1;
fi

function input {
        echo "$POST" | sed 's/^.*'"$1"'=\([^&]\+\)&.*$/\1/' | python -c "import sys, urllib.parse as p; print(p.unquote(sys.stdin.read()));"
}

USERNAME=$(input username)
PASSWORD=$(input password)
PASSWORD2=$(input password2)

if [ "$PASSWORD" != "$PASSWORD2" ]; then
        echo "
Error

Your passwords do not match.
";
        exit 1;
fi

if [ ${#PASSWORD} -lt 10 ]; then
        echo "
Error

Please pick a longer password.
";
        exit 1;
fi

echo "$PASSWORD" | htpasswd -i ../registrations "$USERNAME"

cat <<EOT

You have registered

Now, please email me so I can enable your account.

EOT

exit 0

To clarify - I'm not concerned about the line of python - it urldecodes/unescapes the field from the POST data and is extremely battle-tested. I put that in to avoid bugs/security concerns over other solutions with fewer dependencies (many with scary-looking bash constructs).

The sed is also pretty straightforward - it pulls out the named parameter from the urlencoded POST data. & is escaped as %26 and so will never show up in the input. I wrote/modified the sed there and am confident enough that it'll behave as I expect (sed is more familiar to me than bash).

Essentially, please feel free to ignore the implementation of the input function and assume it properly extracts POST variables into bash variables. My concerns center entirely around whether that user supplied data (which can be essentially any sequence of bytes) in bash variables will break or escape from any of the places I use it.

It's a couple of decades (!) since I did any CGI, but I'll cast a quick eye over this.  I hope other reviewers will pitch in and fill the gaps (or correct my errors).

It's an interesting choice to use the same TITLE for success and failure.  It might be better to write a small function:

header()
{
cat <<EOT
Content-type: text/html; charset=us-ascii

<!doctype html>

                <title>$*</title>
                <link rel="icon" href="/favicon.png">

EOT
}

Note also the form of function declaration - that's more portable than the function keyword, and to be preferred.  We now use it like

if [ "$REQUEST_METHOD" != "POST" ] || [ ! "$POST" ]
then
    header 'Error'
    echo '
Error

Wrong HTTP method; please go back and try again
';
    exit 1;
fi

Also, for that exit, I'd exit 0 there, because the script has succeeded in its job (to report the status to the user).  There's no error as far as the caller (the Web server) is concerned - everything is working correctly.

A further refinement: if we add another cat to the end of header(), it can write the message, too:

#renamed to reflect new purpose
write_html()
{
cat <<EOT
Content-type: text/html; charset=us-ascii

<!doctype html>

                <title>$*</title>
                <link rel="icon" href="/favicon.png">

$*

EOT
cat
}

if [ "$REQUEST_METHOD" != "POST" ] || [ ! "$POST" ]
then
    write_html 'Error' <<"EOT"

Wrong HTTP method; please go back and try again

EOT
fi

There seems to be a couple of errors in the sed command in input():

s/^.*$1=\([^&]\+\)&.*$/\1/

The most obvious is the requirement for & after the target parameter - this means we'll be unable to extract the last parameter in the line.  The more subtle bug is that we can be confused if one parameter is a suffix of another, because we didn't anchor the beginning of $1 at all.  Finally, if $1 isn't present in the string, we'll use the entire request as value; we can fix that by printing only if there's a substitution:

# untested
sed -n 's/^.*[?&]'"$1"'=\([^&]\+\).*$/\1/p'

Here, I rely on the greediness of + to take us to the next & or end of line.

Do have another look through the Python library to see if there's something there to help us unpack POST data - since we're using Python anyway for decoding characters, there's no penalty to adding code there.

Since we're using Bash, we could write sed <<<"$POST" instead of echo in that function.

BTW, good job getting the quoting right there!

From here onwards, nothing I haven't already mentioned.


Finding Pattern Score in Python I saw this problem in C++ on here and decided to try it in Python, which was much simpler. I've used the same problem blurb as in the link above, so they are consistent. I'm sure my code can be improved.
You are given 3 strings: text, pre_text and post_text. Let L be a substring of text. For each substring L of text, we define pattern_score as follows:
pre_text_pattern_score = highest n, such that first n characters of L are equal to the last n characters of pre_text and occur in the same exact order.
post_text_pattern_score = highest n such that last n characters of L are equal to the first n characters of post_text and occur in the same exact order.
pattern_score = pre_text_pattern_score + post_text_pattern_score. For example, if L = "nothing", pre_text = "bruno", and post_text = "ingenious", then
pre_text_pattern_score of L is 2 because the substring "no" is matched, and
post_text_pattern_score is 3 because the substring "ing" is matched.
pattern_score is 5 = 2 + 3 Your program should find a non-empty substring of text that maximizes pattern_score.
If there is a tie, return the substring with the maximal pre_text_pattern_score.
If multiple answers still have a tied score, return the answer that comes first lexicographically. Complete the definition of function string calculateScore(string text, string prefix,string suffix)
Constraints:

text, pre_text, and post_text contain only lowercase letters ('a' -
'z')
1 <= |text| <= 50
1 <= |pre-text| <= 50
1 <= |post-text| <= 50 (where |S| denotes the number of characters in string S)

Sample case #1 text: "nothing" prefix: "bruno" suffix: "ingenious" Returns: "nothing"
this is "no" from "bruno" and "ing" from "ingenious", so "nothing" is returned.
Sample case #2 text: "ab" prefix: "b" suffix: "a" Returns: "a"
def calculateScore(text, prefixString, suffixString):
    result = {}
    lenText = len(text)

    while lenText > 0:
        for i in range(len(text) + 1 - lenText):
            substring = text[i:i + lenText]

            # calc the pre_text_pattern_score
            pre_text_pattern_score = min(len(prefixString), len(substring))

            while pre_text_pattern_score > 0 and substring[:pre_text_pattern_score] != prefixString[-pre_text_pattern_score:]:
                pre_text_pattern_score -= 1

            # calc the post_text_pattern_score
            post_text_pattern_score = min(len(suffixString), len(substring))

            while post_text_pattern_score > 0 and substring[-post_text_pattern_score:] != suffixString[:post_text_pattern_score]:
                post_text_pattern_score-= 1

            # calculate the pattern_score
            pattern_score = pre_text_pattern_score + post_text_pattern_score

            if not pattern_score in result:
                # resets the dictionary key
                result[pattern_score] = []

            result[pattern_score].append(substring)

        lenText -= 1 # reduce lenText by 1

    # store the highest key, so we can sort the right item to return
    maximum_pattern_score = max(result.keys())

    # make sure to sort the lexicographically lowest string of the highest key
    result[maximum_pattern_score].sort()

    # return the lexicographically highest key
    return result[maximum_pattern_score][0]

Code Style
Remember to follow PEP 8 where you can. In particular, you should use snake_case rather than camelCase.
Making your Intentions Clear
Be sure to use comments to explain why your steps are needed rather than just what your code does:
lenText -= 1 # reduce lenText by 1

It is reasonably clear what your code is doing, but you could comment to explain why you're doing this. I'd be tempted to rewrite your substring finding loop as:
# Iterate over all possible substrings of text
for start_index in range(0, len(text)):
    for end_index in range(start_index + 1, len(text) + 1):
        L = text[start_index:end_index]
        # Your logic here

Storing Results
If you preferred, you could save some memory by only storing the patterns that have the highest score we've seen so far.
# Stores the highest seen pattern score so far.
highest_score = 0
# Stores the substrings of text that give highest_score.
highest_substrings = []

You then just need to check if your current score is strictly greater than the highest score seen so far and empty highest_substrings, then if you see a score greater than or equal to highest_score you add that substring to highest_substrings.


Improving fading functions Regarding these two functions, which do the same thing...

Large and confusing:

var fade = function(flag){

    if (volumeIntervalId)
        clearInterval(volumeIntervalId);

    switch (flag){
        case 'in':
            volumeIntervalId = setInterval(function(){
                var vol = songInstance.getVolume();
                if (vol < 1){
                    songInstance.setVolume( vol + volumeIncrement );
                }else{
                    clearInterval(volumeIntervalId);
                }
            }, volumeIntervalDelay);
            break;
        case 'out':
            volumeIntervalId = setInterval(function(){
                var vol = songInstance.getVolume();
                if (vol > 0){
                    songInstance.setVolume( vol - volumeIncrement );
                }else{
                    clearInterval(volumeIntervalId);
                    songInstance.stop();
                }
            }, volumeIntervalDelay);
            break;
        default:
            clearInterval(volumeIntervalId);
            break;
    }

},

Slighty smaller but maybe more confusing:

var fade = function(flag){

    if (volumeIntervalId)
        clearInterval(volumeIntervalId);

    volumeIntervalId = setInterval(function(){
        var vol = songInstance.getVolume();
        if (flag === 'in'){
            if (vol < 1) {
                songInstance.setVolume( vol + volumeIncrement );
                return;
            }
        } else if (flag === 'out'){
            if (vol > 0) {
                songInstance.setVolume( vol - volumeIncrement );
                return;
            }else{
                songInstance.stop();
            }
        }
        clearInterval(volumeIntervalId);
    }, volumeIntervalDelay);

},

How could I make that simpler/cleaner/easier to read? Maybe split into more functions?

It's interesting how the 2nd function's setInterval anonymous function keeps the value of 'flag' due to the closure. Is there any risk? Should I avoid that?

Maybe (the obvious) use two functions (fadeIn and fadeOut)? Why do people (like me) tend to complicate things like that?

Update

I guess this is better:

fadeIn: function(){
    if (volumeIntervalId) clearInterval(volumeIntervalId);

    volumeIntervalId = setInterval(function(){
        var vol = songInstance.getVolume();
        if (vol < 1){
            songInstance.setVolume( vol + volumeIncrement );
        }else{
            clearInterval(volumeIntervalId);
        }
    }, volumeIntervalDelay);
},

fadeOutAndStop: function(){
    if (volumeIntervalId) clearInterval(volumeIntervalId);

    volumeIntervalId = setInterval(function(){
        var vol = songInstance.getVolume();
        if (vol > 0){
            songInstance.setVolume( vol - volumeIncrement );
        }else{
            songInstance.stop();
            clearInterval(volumeIntervalId);
        }
    }, volumeIntervalDelay);
},

The updated code looks much better but you could try to extract some more common structure.

You can define fading in a general way that allows you to plug in some fading strategies. You already have fade in and fade out but you'll be able to reuse the same code for other 'fading'.

fade: function(fadingStrategy){
    if (volumeIntervalId) clearInterval(volumeIntervalId);
    volumeIntervalId = setInterval(fadingStrategy, volumeIntervalDelay);
}

You can then define fadeIn and fadeOut by simply passing the appropriate strategies to fade.

I think there is still room for improvement. Your code relies on some global state through volumeIntervalId and volumeIntervalDelay. Getting rid of the second one is trivial. You can just pass it as a parameter to fade, making it even more flexible as it will now support also different combinations of fading strategies and fading intervals.

Clearing the timed action could be a bit more tricky. One possible solution could be to make your function return another function that closes over volumeIntervalId. In that way you will only need to keep track of the function you got from the invocation of fade and invoke it when you want to stop its action.

fade: function(fadingStrategy, volumeIntervalDelay){
    var volumeIntervalId = setInterval(fadingStrategy, volumeIntervalDelay);
    return function(){clearInterval(volumeIntervalId);};
}

Disclaimer
I'm not a Javascript expert so I cannot guarantee that the code I wrote works flawlessly but I hope you at least got some ideas on how you could improve your code.


Identifying array elements, rearranging them, and replacing them I am completing the CodingBat exercises for Java, and, having learnt from the answers to the previous one, I completed the one that follows:

  Return a version of the given array where all the 10s have been removed. The remaining elements should shift left towards the start of the array as needed, and the empty spaces a the end of the array should be 0. So {1, 10, 10, 2} yields {1, 2, 0, 0}. You may modify and return the given array or make a new array.

Here is my code:

public int[] withoutTen(int[] nums) {

    int slow = 0;

    for (int fast = 0; fast < nums.length; fast++) {
        if (nums[fast] != 10) {
            nums[slow] = nums[fast];
            slow++;
        }
        if (fast == nums.length-1) {
            for (int i = fast; i >= slow; i--) {
                nums[i] = 0;
            }
        }
    }
    return nums;

}

Please bear in mind I am doing these without importing anything extra like java.util.Arrays etc., as, primarily, this is not accepted by the assessor and, secondarily, I want to get to grips with arrays without importing anything extra yet.

I was taking influence from the comments I received on the previous question, by using a fast and slow variable for looping through the array, and limiting the code to one main for loop, for the sake of making my code generally better and putting efficiency into practice.

How could this be optimised? Is it ok to begin another loop only for the circumstance of fast reaching the last element in the array? It feels like I am repeating myself by saying "if we get to the last element..." because we absolutely will do anyway.

I played around with setting fast outside the loops, so that the second for loop can begin after the first (as opposed to nesting it). At that point, fast would be set to the last value anyway from having completed the previous loop, but more and more values would be set outside the loops. Which way makes more sense?

It's extraordinarily confusing to have if (fast == nums.length - 1) as a conditional within a loop that continues while fast < nums.count returns true.  This is the last part of the loop, and it only executes on the last iteration of the loop... so why is it even in the loop at all?

A couple of comments in our code can help us out.  We have two distinct parts:

Shuffling the non-tens to the front.
Zero-ing out the tens.

So let's use comments to mark these in distinct blocks.  And, let's move the second part to its own loop.

And while we're at it, let's make the method more generic to replace any number with another given number.

public int[] replaceNum(int[] nums, int eliminationNum, int replaceNum) {
    int nextMoveIndex = 0;

    // shuffle non-elims to the front
    for (int i = 0; i < nums.length; ++i) {
        if (nums[i] != eliminationNum) {
            nums[nextMoveIndex++] = nums[i];
        }
    }

    // zero out the elimination number
    while (nextMoveIndex < nums.length) {
        nums[nextMoveIndex++] = replaceNum;
    }

    return nums;
}


Extending native JavaScript Promise with cancellation support Updated, below is a more current and simple version of this. Also, if you track the progress of TC39's "ECMAScript Cancellation" proposal, this thread might be worth checking out.

I'm playing with Promise Extensions for JavaScript (prex) library and I like it a lot. Amongst other things, this library appears to be a prototype behind the current ECMA TC39 proposal for cancellation and it uses the familiar cancellation token approach, popularized by .NET Task API.
I want to extend the standard Promise class with cancellation support, similar to how it is implemented in Bluebird (i.e., with an optional oncancel callback) but using prex.CancellationToken.
Here is a draft that can be run with NodeJS:
const prex = require('prex');

class CancellablePromise extends Promise {
  constructor(executor, token) {
    if (!token) {
      // if no token supplied, just delegate to the parent class
      super(executor);
      return;
    }

    const observeCancellation = async () => {
      // prex.Deferred is similar to TaskCompletionSource in .NET
      const deferred = new prex.Deferred();

      executor(
        deferred.resolve,
        deferred.reject,
        cancelListener =>
          deferred.cancelListener = cancelListener);

      const registration = token.register(() => {
        try {
          // capture the CancelError
          token.throwIfCancellationRequested();
        }
        catch (cancelError) {
          try {
            // the token cancellation callback is synchronous,
            // and so is the executor-provided cancelListener callback
            deferred.cancelListener && deferred.cancelListener(cancelError);
            // reject here if cancelListener has not resolved/rejected it
            deferred.reject(cancelError);
          }
          catch (error) {
            // in case cancelListener throws
            deferred.reject(error);
          }
        }
      });

      try {
        return await deferred.promise;
      }
      finally {
        registration.unregister();
      }
    };

    super((resolve, reject) =>
      observeCancellation().then(resolve, reject));
  }
}

// delayWithCancellation
function delayWithCancellation(timeoutMs, token) {
  console.log(`delayWithCancellation: ${timeoutMs}`);

  return new CancellablePromise((resolve, reject, setCancelListener) => {
    token.throwIfCancellationRequested();
    const id = setTimeout(resolve, timeoutMs);
    setCancelListener(e => clearTimeout(id));
  }, token);
}

// main
async function main() {
  const tokenSource = new prex.CancellationTokenSource();
  setTimeout(() => tokenSource.cancel(), 2000); // cancel after 1500ms

  const token = tokenSource.token;

  await delayWithCancellation(1000, token);
  console.log("successfully delayed."); // we should reach here

  await delayWithCancellation(1500, token);
  console.log("successfully delayed."); // we should not reach here
}

main().catch(error => console.log(error));

You can override the executor and can pass an onCancel function to it like following:
class ResponsePromise extends Promise {
  constructor(executor) {
    const onCancel =  (cb) => {
      //using nextTick because we cant use "this" before super()
      setTimeout(()=> {
        this.cancelCb = cb;
      })
    }

    const oExecutor = (resolve, reject) => {
      executor(resolve, reject, onCancel);
    }

    super(oExecutor);
  }
  cancel() {
    if (this.cancelCb) {
      this.cancelCb();
    } else {
      console.warn('onCancel not provided');
    }
  }

I have explained this in http://w3.cafe/promise-with-progress-javascript/


Saving and updating records I have made two SQL statements as I was unable to figure out how to solve my problem. One is called Product_GetID and the other Product_SaveRecord.  I wanted the save record to read and see if there was an ID entered in a field in a VB front end and if there was not, it would assign one for it, but I was unable to figure this out.  It works, but I know there should be a better way to do this.

  ALTER PROCEDURE [dbo].[Product_GetID]

AS
BEGIN
DECLARE @NewProductID as int
/* Get New Record */
SET @NewProductID = (SELECT ISNULL(MAX(ProductID) + 1, 1) FROM Product)
SELECT ISNULL(MAX(ProductID) + 1, 1) AS NewIDNo FROM Product
INSERT INTO Product (ProductID, CategoryID, Description, Price)
VALUES        (@NewProductID, 0, '', 0)
END

ALTER PROCEDURE [dbo].[Product_SaveRecord]
-- Add the parameters for the stored procedure here
@ProductID as int, @CategoryID as tinyint, @Description as varchar(80),       @FullDescription as         varchar(240), @Price as decimal(8, 2), @MarkupPer as   decimal(8, 2), @LabourHours as decimal(8, 2), @LabourRate as decimal(8, 2), @Stock as integer

AS
BEGIN

/* Update Record */
UPDATE Product SET CategoryID = @CategoryID,
Description = @Description,
FullDescription = @FullDescription,
Price = @Price,
MarkupPer = @MarkupPer,
 LabourHours = @LabourHours,
LabourRate = @LabourRate,
 Stock = @Stock
 WHERE ProductID = @ProductID
END

Nitpicks

Why is this indented?

  ALTER PROCEDURE [dbo].[Product_GetID]

This indentation is inconsistent:

UPDATE Product SET CategoryID = @CategoryID,
Description = @Description,
FullDescription = @FullDescription,
Price = @Price,
MarkupPer = @MarkupPer,
 LabourHours = @LabourHours,
LabourRate = @LabourRate,
 Stock = @Stock
 WHERE ProductID = @ProductID
END

The comments are just noise, it's pretty obvious from the way the code reads to figure out what it does. Especially comments like this:

/* Update Record */
UPDATE Product SET CategoryID = @CategoryID,

Vertical white space would make your code easier to read. For example:

ALTER PROCEDURE [dbo].[Product_SaveRecord]
@ProductID as int, @CategoryID as tinyint, @Description as varchar(80),       @FullDescription as         varchar(240), @Price as decimal(8, 2), @MarkupPer as   decimal(8, 2), @LabourHours as decimal(8, 2), @LabourRate as decimal(8, 2), @Stock as integer

Compare to:

ALTER PROCEDURE [dbo].[Product_SaveRecord]
    @ProductID as int,
    @CategoryID as tinyint,
    @Description as varchar(80),
    @FullDescription as varchar(240),
    @Price as decimal(8, 2),
    @MarkupPer as   decimal(8, 2),
    @LabourHours as decimal(8, 2),
    @LabourRate as decimal(8, 2),
    @Stock as integer;

Also, consistency would be good in how you capitalize keywords. Either do all caps, or all lower caps; it's less confusing that way.

End your SQL statements

Not using the delimiter ; throughout your query makes it more difficult to understand the logic of your code. Use ; and even GO if need be. MS SQL Server is forgiving on syntax errors (try PostgreSQL for fun), that doesn't mean you shouldn't follow good practices of ending statements properly.

Your question

You're trying to insert a row and then use the ID of the row you inserted. There is an easier way to do this using @@IDENTITY in Transact-SQL.

Here would be a simpler way to do this your way, with above advice applied:

ALTER PROCEDURE [dbo].[Product_GetID]
    @ProductID AS INT,
    @CategoryID AS TINYINT,
    @Description AS VARCHAR(80),
    @FullDescription AS VARCHAR(240),
    @Price AS DECIMAL(8, 2),
    @MarkupPer AS DECIMAL(8, 2),
    @LabourHours AS DECIMAL(8, 2),
    @LabourRate AS DECIMAL(8, 2),
    @Stock AS INTEGER
AS
BEGIN
INSERT INTO Product (CategoryID, Description, Price)
VALUES (0, '', 0);
UPDATE Product
SET CategoryID = @CategoryID,
Description = @Description,
FullDescription = @FullDescription,
Price = @Price,
MarkupPer = @MarkupPer,
LabourHours = @LabourHours,
LabourRate = @LabourRate,
Stock = @Stock
WHERE ProductID = @@IDENTITY;
END

BUT...

Why do you not simply insert your parameters within one single INSERT transaction?

ALTER PROCEDURE [dbo].[Product_GetID]
    @ProductID AS INT,
    @CategoryID AS TINYINT,
    @Description AS VARCHAR(80),
    @FullDescription AS VARCHAR(240),
    @Price AS DECIMAL(8, 2),
    @MarkupPer AS DECIMAL(8, 2),
    @LabourHours AS DECIMAL(8, 2),
    @LabourRate AS DECIMAL(8, 2),
    @Stock AS INTEGER
AS
BEGIN
INSERT INTO Product(
    CategoryID,
    Description,
    FullDescription,
    Price,
    MarkupPer,
    LabourHours,
    LabourRate,
    Stock
) VALUES (
    @CategoryID,
    @Description,
    @FullDescription,
    @Price,
    @MarkupPer,
    @LabourHours,
    @LabourRate,
    @Stock
);
END


Best C# idiom to convert the items on an object array to a string? I have something akin to

object[] values = getValues();
string renderedValues = string.Join("-",
                          Array.ConvertAll<object,string>(values,
                          new Converter<object,string>(o2s)
                        ));

where o2s is

public static string o2s(object o) { return o.ToString(); }

Comments welcome!

There exists a method for that conversion already:
string renderedValues = string.Join(
  "-",
  Array.ConvertAll<object, string>(values, Convert.ToString)
);

Update:
In framework 4 an overload that takes an object array was added, so it will do the conversion for you:
string renderedValues = string.Join("-", values);


Proper way to log progress and resume in Python I often have a need to log my progress when doing a repetitive task.  Logging allows me to continue where I left off if execution is halted (for example, perhaps my VPS server was rebooted by my host).

My current working solution is a bit ad hock.  Any input on improving this would be appreciated:

class RepeatingTask(object):
    def __init__(self, first, logfile):
        self.current = first
        self.logfile = logfile

    def page(self):
        self.process_request()
        self.increment()
        self.log_progress()

    def process_request(self):
        # do processing here...
        pass

    def increment(self):
        # set self.current = next item for processing
        # when items are exhausted, set self.current=None
        pass

    def log_progress(self):
        if self.current is None:
            os.remove(self.logfile)
        else:
            with open(self.logfile, 'w') as logfile:
                logfile.write(json.dumps([["current","key-xyz"],
                                          [self.current,self.val_abc]]))

    def resume(self):
        try:
            with open(self.logfile, 'r') as logfile:
                content = logfile.read()
        except IOError:
            return False
        content = json.loads(content)
        content = dict(zip(content[0],content[1]))
        self.current = content['current']
        self.val_abc = content['key-xyz']
        return True

if __name__ == "__main__":
    task_object = RepeatingTask("first_item_to_process","/path/to/logfile")
    if task_object.resume():
        print "Resuming progress, current:", task_object.current
    else:
        print "Starting freash, current:", task_object.current
    while task_object.current is not None:
        task_object.page()
        # do some work
        print "Next:", task_object.current

You may want to consider using the standard Python logging module to log in the information, and just keep the parser.

import logging

logger = logging.getLogger('repeatingApp')
hdlr = logging.FileHandler('/path/to/logfile')
hdlr.setFormatter(logging.Formatter('%(message)s'))
logger.addHandler(hdlr)
logger.setLevel(logging.WARNING)

class RepeatingTask(object):
    def page(self):
        self.process_request()
        self.increment()
        logger.info(json.dumps([["current","key-xyz"],
                                [self.current,self.val_abc]]))


Arranging words in sentences alphabetically For an online contest whose problem is here, I wrote some code.

  In this problem the input will consist of a number of lines of English text consisting of the letters of the English alphabet, the punctuation marks ' (apostrophe), . (full stop), , (comma), ; (semicolon), : (colon) and white space characters (blank, newline).
  Your task is print the words in the text in lexicographic order (that is, dictionary order). Each word should appear exactly once in your list. You can ignore the case (for instance, "The" and "the" are to be treated as the same word.) There should be no uppercase letters in the output.

  For example, consider the following candidate for the input text:

This is a sample piece of text to illustrate this
problem.

  The corresponding output would read as:

a
illustrate
is
of
piece
problem
sample
text
this
to

  Input format

  The first line of input contains a single integer \$N\$, indicating the number of lines in the input. This is followed by \$N\$ lines of input text.

  Output format

  The first line of output contains a single integer \$M\$ indicating the number of distinct words in the given text. The next \$M\$ lines list out these words in lexicographic order.

  Test data

  You may assume that \$N ≤ 10000\$ and that there are at most 80 characters in each line. You may also assume that there are at the most 1000 distinct words in the given text.

  Example

  We now illustrate the input and output formats using the above example.

  Sample input

2
This is a sample piece of text to illustrate this
problem.

  Sample output

10
a
illustrate
is
of
piece
problem
sample
text
this
to

It is working perfectly as far as I know. When I run my code, it passes all tests.

import java.util.ArrayList;
import java.util.Collections;
import java.util.Scanner;

/**
 * Created by aditya on 14-10-2014.
 */
public class Main {

    public static void main(String args[]) {

        Scanner scan = new Scanner(System.in);
        int n = Integer.parseInt(scan.nextLine());
        ArrayList<String> lines = new ArrayList<String>();
        ArrayList<String> words = new ArrayList<String>();
        ArrayList<String> words_2 = new ArrayList<String>();
        boolean once_entered = true;
        for (int i = 0; i < n; i++) {
            lines.add(i, scan.nextLine() + " ");
        }
        for (int i = 0; i < n; i++) {
            String word = "";
            for (int j = 0; j < lines.get(i).length(); j++) {
                char char_0 = lines.get(i).toLowerCase().charAt(j);
                if ((int) (char_0) >= (int) ('a') && (int) (char_0) <= (int) ('z')) {
                    word += char_0;
                    once_entered = false;
                } else if (!once_entered) {
                    words.add(word);
                    word = "";
                    once_entered = true;
                }
            }
        }
        for (int i = 0; i < words.size(); i++) {
            boolean contains =false;
            for(int j=0;j<words_2.size();j++){
                if(words_2.get(j).contentEquals(words.get(i)))
                    contains=true;
            }
            if(!contains)
                words_2.add(words.get(i));
        }
        Collections.sort(words_2);
        System.out.println(words_2.size());
        for (int i = 0; i < words_2.size(); i++) {
            System.out.println(words_2.get(i));
        }
    }
}

Your code here is far more complicated than it needs to be. The trick you need to follow is four-fold:

Use your library functionality
Use Regular Expressions
Use buffered input/output for performance
bend the rules on the input side.

You go to great lengths to store the data in two arrays (with bad names words and words_2), and you use the one to check for words in the other. The right structure in Java is a Set, which automatically, and efficiently does the check for you. Further, there's another specialized set, called the TreeSet which stores the data in sorted order already.

Regular Expressions are a good way to identify patterns in the data. They take some getting used to, but, because of the way they can be pre-compiled, and reused, they are fast. Note that the Scanner class looks for regular expressions.

When using input, and output, you get better performance if you use fewer operations. You should wrap the System.in in a BufferedInputStream, and perform all the output to a StringBuilder, and then dump that to System.out in one operation.

Now, about bending the rules on the input side.....

Many programming challenges are set up to work in many languages, including things like C, C++, etc. Some of those languages need to have an idea of how much space to allocate before processing the data. That's why the description says it will contain the number of lines, as well as the maximum line length. If you assume that the input will be valid, then there is no reason to process the data line by line, you can do it word by word, and just forget about the line count in the first line....

How would I do it?

    try (Scanner scan = new Scanner(new BufferedInputStream(System.in));) {
        // find word boundaries...
        scan.useDelimiter(Pattern.compile("[\\s;:,.'\n]+", Pattern.MULTILINE));
        scan.nextLine(); // ignore the count
        Set<String> words = new TreeSet<>();
        while (scan.hasNext()) {
            String word = scan.next().toLowerCase();
            words.add(word);
        }
        StringBuilder sb = new StringBuilder();
        sb.append(words.size()).append("\n");
        for (String word : words) {
            sb.append(word).append("\n");
        }
        System.out.print(sb.toString());
        System.out.flush();
    }

On further reflection, I would split the logic in to two parts, a word-finder, and a word printer:

Set<String> words = parseWords(System.in);
System.out.println(formatWords(words));


Python Pandas Dataframe code takes too long to finish I have a Pandas DataFrame with users subscription dates in the following format:

UserId, StartingDate, EndingDate

And I try to calculate the Churn Rate metric for every day.

What Churn Rate is:

  The churn rate, also known as the rate of attrition, is the percentage of subscribers to a service who discontinue their subscriptions to that service within a given time period.

So, for every day, I go back 1 month, I get a list of unique users that had an active subscription and check how many of those don't have it any more.

I wrote the code, but it takes ages to finish, so I am looking for any kind of performance issues that might have

import pandas as pd
from datetime import datetime
from datetime import timedelta

df = pd.read_csv("subscritpions.csv")

#make sure both columns are in datetime type
df['StartingDate'] =  pd.to_datetime(df['StartingDate'])
df['EndingDate'] =  pd.to_datetime(df['EndingDate'])

#get the first date of the dataframe to start the loop with it and set the stop date as today
start = pd.to_datetime(df.StartingDate.min())
minDate = start
stop = datetime.now()

def getUsersFromADate(df,date):
    return df.loc[(df['StartingDate'] <= date) & (df['EndingDate'] >= date)].UserId.unique()

churn = []
while start <= stop:
    # first 30 days doesn't have a churn rate. So just append a 0 value
    if start < minDate + pd.DateOffset(months=1):
        churn.append(0)
    else:
        usersBefore = getUsersFromADate(df, start - pd.DateOffset(months=1))
        usersNow = getUsersFromADate(df, start)
        lost = 0
        for u in usersBefore:
            if u not in usersNow:
                lost += 1
        churn.append(lost/len(usersBefore))
    start = start + timedelta(days=1)  # increase day one by one

Example of my data:

   UserId StartingDate  EndingDate
0      1   2013-05-09  2015-04-24
1      1   2015-04-29  2017-04-02
2      1   2017-04-05  2017-12-06
3      2   2014-02-13  2018-02-07
4      3   2013-04-25  2018-04-19

You could do the entire thing in pandas and numpy as their Cython implementation will be much faster than iterating over objects in Python.

First simulate some data,

import pandas as pd, numpy as np
from datetime import datetime

num_samples = 50000
user_ids = np.arange(num_samples)
dates = pd.date_range("2012-01-01","2015-01-01")
start_dates = dates[np.random.randint(0,len(dates),50000)]

data = pd.DataFrame(data={"user_id": user_ids, "start_date":start_dates})
data["end_date"] = data.start_date.apply(lambda x: x + pd.DateOffset(days=np.random.randint(0,300)))

which results in data:

start_date  user_id   end_date
2013-12-15        0   2014-09-24
2013-12-13        1   2014-01-17
2014-08-29        2   2015-03-25
2014-04-13        3   2015-01-04
2014-01-21        4   2014-06-22

We need an output dataframe so that we don't have to painfully iterate over each date one by one:

output = pd.DataFrame({"date":pd.date_range(data.start_date.min(), datetime.now())})

and now define your functions to calculate the churn rate and apply it over the entire data frame:

def get_users_from_date(df, date):
    return df[(date >= df.start_date) & (date <= df.end_date)].user_id

def calc_churn(df, date):
    users_before = get_users_from_date(df,date-pd.DateOffset(months=1))
    users_now = get_users_from_date(df,date)

    if len(users_before):
            return len(np.setdiff1d(users_before,users_now)) / len(users_before)
    else:
            return 0

output["churn"] = output.date.apply(lambda x: calc_churn(data,x))

This finishes for me in just:

  5.42 s ± 65.5 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

Since we are using np.setdiff1d to find the difference, we don't need to unique each subset of user ids beforehand. Finding the unique subsets in get_user_from_date then passing them in, even with assume_unique=True, gets a time of:

  5.98 s ± 75 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

while using Python set difference instead of numpy arrays gets a time of:

  7.5 s ± 114 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

and finally, combining Python sets with list comprehensions, similar to your current implementation never finished running.

The main bottleneck is in comparing the sets of user ids, so doing that in either numpy or pandas instead of iterating over Python objects will improve performance.

On a different note, your code looks good and is really easy to read but you should use snake case instead of camel case to fit Python convention.


Haskell Collatz Conjecture In an attempt to begin properly programming in Haskell, I wrote two functions for calculating numbers and sequences with Collatz's conjecture. I also commented the program quite a lot, because it helps me with my remembering of certain things.

-- The Collatz sequence takes in a number, and either
-- divides by 2 if it is even, or
-- multiplies it by 3 and adds 1 if it is odd.
-- The famous Collatz conjecture asks whether the sequence,
-- from any positive initial value, will reach 1.

-- a guard expression is used for the 'collatz' variable,
-- used for finding the next number in a sequence. This is
-- neater than if / else if.
collatz n
    | n `mod` 2 == 0 = n `div` 2 -- if n is even, return n / 2
    | otherwise = 3 * n + 1 -- else return 3n + 1

-- another guard expression, this time for the variable that
-- generates whole sequences. it is recursive, and takes
-- advantage of the 'collatz' variable too.
cltzList n
    | n <= 0 = error "The number must be positive."
    | n == 1 = [n] -- if n is 1 return n:[]
    | otherwise = n:cltzList (collatz n) -- else, recursive-call

How have I done? Are there any big mistakes, or small things I can improve?

There's nothing wrong with your code in terms of functionality, but there are a number of things we can do to make it more stylish and idiomatic.

The #1 most important thing you have missed is to always provide a type signature for top-level definitions. Just reading the type signature of a function can often give you an intuition as to what that function will do (think of IO actions, or higher order functions) or confirm that what you've written actually does something like what you're expecting after you've called the compiler to check your work.

collatz :: Int -> Int
cltzList :: Int -> [Int]

There are a few small tweaks to your definitions that we should make to come in line with good Haskell style, these won't impact functionality but help your function definitions to read more like English definitions instead of instructions for a computer.

When checking to see if a number is even, use the function even instead of checking the value in modulo 2. The definition of even may be doing the same operation under the hood but using even expresses your intent instead of the method. Changes like these will improve the readability of your code in larger projects remarkably. (Additionally, abstracting out even allows the implementer of that function to change the definition to be more efficient without requiring any changes to your code, e.g., even may check the least significant bit instead of performing any mathematical operation at all.)

collatz n | even n = n `div` 2

When calling error, you should always include the module and function name so that users of your code know what function the error actually originated from. I.e, error "Collatz.cltzList: The number must be positive" That way if someone calls your function with a negative number they see this

*** Exception: Collatz.cltzList: The number must be positive

instead of this

*** Exception: The number must be positive.

On this line | n == 1 = [n] I would change [n] to [1]. That's the only value n will ever be, so it's better to be explicit than to be clever.

You have far too many comments that have little to no value. For instance, you can assume that unless you're writing an "Intro to Haskell" book your readers will know what pattern guards are and so you don't need to explain what they are or why you used them.

And finally in my version below I've cleaned up the alignment with liberal use of whitespace. This is mostly an aesthetic effect but I find beautiful code easier to read than otherwise, and this is not an uncommon style in Haskell code.

-- Implements the Collatz sequence.
--
--        n / 2     if n is even
-- c(n) =
--        3 * n + 1 if n is odd

module Collatz where

collatz :: Int -> Int
collatz n | even n    = n `div` 2
          | otherwise = 3 * n + 1

-- Produces the Collatz sequence for a given non-zero natural number
collatzList :: Int -> [Int]
collatzList n | n <= 0    = error "Collatz.collatzList: n must be positive"
              | n == 1    = 1 : []
              | otherwise = n : collatzList (collatz n)


Grabbing hashed password from a database I have just discovered the beauty of prepared statements in PHP and how they protect against SQL injection. In my first time using them, I am attempting to grab a hashed password from a database and seeing if it matches up against the password the user inputted into the login form.

Is this the most efficient way to:

Grab the data from the database?
Match the passwords using password_verify()?

<?php
//will correspond with 'loginLogin.html page
require("opendboLogin.php");

$user = $_POST['username']; //these are grabbed from the login form (textbox)
$password = $_POST['password'];

$stmt2 = $conn->stmt_init();
$stmt2 = $conn->prepare("SELECT apassword FROM login WHERE auser = ?");
$stmt2->bind_param("s", $user);
$stmt2->execute();
$stmt2->store_result();
$stmt2->bind_result($hashedPassword);
$stmt2->fetch();
//echo $hashedPassword; this returns hashed password string from DB
$numberofrows = $stmt2->num_rows; //this is an integer!!
$stmt2 -> close();

if($numberofrows > 0) //if username exists in database
{
    if (password_verify($password, $hashedPassword)) //if user-inputted password (from form) equals hashed password from DB...
    {
        print("Password is valid, login successful!");
        session_start();
        $_SESSION['sess_user'] = $user;
        header("location:member.php"); //redirect user to member page
    }
    else
    {
        echo 'Invalid password for the username: ' . $user; //if password didn't match DB, we tell them
    }
}
else
{
    echo 'The username, ' . $user . ', does not exist! Please try again.'; //if num_rows is 0, we know username doesnt exist
}
?>

Regarding your questions, yes, that looks fine to me.

XSS

When echoing user input - or really any variable data - you need to protect against XSS attacks. Otherwise, attackers can inject JavaScript code, which will then be executed in the context of the browser of the victim, leading to bypass of CSRF protection, cookie stealing, phishing, or injection of JavaScript keyloggers.

Specifically, these statements are vulnerable:

echo 'Invalid password for the username: ' . $user;
echo 'The username, ' . $user . ', does not exist! Please try again.';

You can defend against XSS by using htmlspecialchars($string, ENT_QUOTES, 'UTF-8'); when echoing data.

Nesting

Personally, I don't like these kinds of nested ifs. It's hard to see which if clause the different else statements close. With just two, it's still manageable, but when the code is extended, it will become very difficult to read.

A different structure might look like this:

if($numberofrows <= 0) {
    // return false or echo and die
}

if (!password_verify($password, $hashedPassword)) {
    // return false or echo and die
}

// return true, or start session, redirect

Now, it's a lot clearer which error message results from which check.

Misc

it's good practice to regenerate the session id when the state of the session changes to prevent session fixation (not an issue with default php.ini, but with some settings it is an issue). Use session_regenerate_id(true); for this.
using relative URLs in the Location header violates current standards, so if you can, use absolute URLs.
your comments don't really add any clarity to the code, so I would just remove them.
in PHP, it is generally standard to put curly brackets of if/else statements on the same line.
do you really need the call to store_result? It doesn't seem necessary to me.
your spacing is not internally consistent (eg $stmt2 -> close(), if().
I would either use print or echo, but not both for the same task.
as a security precaution, you should always die after a redirect as clients do not have to follow it, and thus code below the redirect can be executed as well. In this case, it doesn't matter because the redirect is for a successful login, but it's good practice.


Given a sorted (increasing order) array, write an algorithm to create a binary tree with minimal height I'm trying to find the complexity of this code and some suggestions for improving the code quality. and handling the code gracefully, esp in the areas of exception handling, checking edge cases, presentation, etc.

// Node Class

class Node {
    int id;
    Node left;
    Node right;
    Node(int id) {
        this.id = id;
        left = null;
        right = null;
    }
}

// ArrayIntoBinaryTree Class

import java.util.Scanner;
public class ArrayIntoBinaryTree {

    /*
     Algorithm:
        1. Insert into the tree the middle element of the array.
        2. Insert (into the left subtree) the left subarray elements
        3. Insert (into the right subtree) the right subarray elements
        4. Recurse
     */

    ArrayIntoBinaryTree arrayIntoBinaryTree = new ArrayIntoBinaryTree();

    public static void main(String[] args) {
        int[] arr = initializeArray();
        if (arr == null) {
          throw new NullPointerException("Input array is empty");
        }
        Node node = addToTree(arr, 0, arr.length);
    }

    // find Subarray - Use Recursion.
    static Node addToTree(int[] arr, int first, int last) {
        // Exit condition
        if(first<last) {
            return null;
        }
        int midElement = arr[(first+last)/2];
        Node newNode = new Node(arr[midElement]);
        newNode.left = addToTree(arr, first, midElement-1);
        newNode.right = addToTree(arr, midElement+1, last);
        return newNode;
    }

    static int[] initializeArray() {
        Scanner sc = new Scanner(System.in);
        System.out.println("Enter the size of array: ");
        int size = sc.nextInt();

        if (size < 1) {
          return null;
        }

        System.out.println("Now enter " + size + " number of elements.");
        int[] arr = new int[size];
        for(int i=0; i<size; i++) {
            arr[i] = sc.nextInt();
        }
        return arr;
    }
}

Bug 1

if(first<last) {
    return null;
}

This is wrong. It will return a null for the very root of the tree. Change to

if (last - first < 1) {
    return null;
}

Bug 2

int midElement = arr[(first+last)/2];
Node newNode = new Node(arr[midElement]);
newNode.left = addToTree(arr, first, midElement-1);
newNode.right = addToTree(arr, midElement+1, last);

This is wrong too. You should have instead:

int middleIndex = (first + last) / 2;
Node newNode = new Node(arr[middleIndex]);
newNode.left = addToTree(arr, first, middleIndex);
newNode.right = addToTree(arr, middleIndex + 1, last);

Advice 1

if(first<last) {
    ...
}

According to Java coding conventions, you must have a single space after if and before an opening (. Also, each binary operator must have a single space before and after it. Putting things together, you must write

if (first < last) {
    ...
}

Advice 2

I suggest you move the tree node to its own file and declare it non-public. Also, I suggest you roll a public tree type that can validate that data is traversed in-order in the same order as the elements in the input array.

Alternative implementation

IntBinaryTreeNode.java

final class IntBinaryTreeNode {

    private int datum;
    private IntBinaryTreeNode leftChild;
    private IntBinaryTreeNode rightChild;

    IntBinaryTreeNode(int datum) {
        this.datum = datum;
    }

    int getDatum() {
        return datum;
    }

    IntBinaryTreeNode getLeftChild() {
        return leftChild;
    }

    IntBinaryTreeNode getRightChild() {
        return rightChild;
    }

    void setLeftChild(IntBinaryTreeNode leftChild) {
        this.leftChild = leftChild;
    }

    void setRightChild(IntBinaryTreeNode rightChild) {
        this.rightChild = rightChild;
    }
}

IntBinaryTree.java

import java.util.function.Consumer;

/**
 * Implements a simple binary tree in which each node contains an integer. This
 * implementation does not enforce any order so it is not a binary search tree.
 */
public class IntBinaryTree {

    private final IntBinaryTreeNode root;

    IntBinaryTree(IntBinaryTreeNode root) {
        this.root = root;
    }

    public void inOrderTraversal(Consumer<Integer> consumer) {
        inOrderTraversal(root, consumer);
    }

    private void inOrderTraversal(IntBinaryTreeNode node,
                                  Consumer<Integer> consumer) {
        if (node == null) {
            return;
        }

        inOrderTraversal(node.getLeftChild(), consumer);
        consumer.accept(node.getDatum());
        inOrderTraversal(node.getRightChild(), consumer);
    }
}

IntArrayToBinaryTreeConverter.java

    public final class IntArrayToBinaryConverter {

    public IntBinaryTree convert(int[] array) {
        IntBinaryTreeNode root = convert(array, 0, array.length);
        return new IntBinaryTree(root);
    }

    /**
     * Converts a range {@code array[fromIndex], ..., array[toIndex - 1]} into
     * a binary search tree.
     *
     * @param array     the array of integers to convert into nodes.
     * @param fromIndex the starting, inclusive index of the range to convert.
     * @param toIndex   the ending, exclusive index of the range to convert.
     * @return
     */
    private IntBinaryTreeNode convert(int[] array,
                                            int fromIndex,
                                            int toIndex) {
        int rangeLength = toIndex - fromIndex;

        if (rangeLength < 1) {
            return null;
        }

        int middleIndex = fromIndex + ((toIndex - fromIndex) >>> 1);
        IntBinaryTreeNode node =
                new IntBinaryTreeNode(array[middleIndex]);
        node.setLeftChild(convert(array, fromIndex, middleIndex));
        node.setRightChild(convert(array, middleIndex + 1, toIndex));
        return node;
    }
}

Main.java

import java.util.Scanner;

public class Main {

    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);
        System.out.print("Enter the array length: ");
        int arrayLength = scanner.nextInt();
        System.out.print("Enter all the array components: ");
        int[] array = new int[arrayLength];

        for (int i = 0; i < arrayLength; ++i) {
            array[i] = scanner.nextInt();
        }

        IntBinaryTree tree =
                new IntArrayToBinaryConverter().convert(array);

        tree.inOrderTraversal(System.out::println);
    }
}


Implementing a string class in C++ I recently attended an interview for a C++ position in a very reputed bank.

The interviewer started off by asking me to implement a string class but was only interested in constructor, copy-constructor, assignment operator and destructor.

Basically, he wanted to know how I would implement Rule of Three and nothing else.  He was not even interested in move operations.

So, I implemented the assignment operator in terms of copy constructor:

class String
{
  /*....*/
String& operator=(String other)
{
   std::swap(m_data, other.m_data);
   return *this;
}

private:
   char* m_data;
};

Since this is pretty common way to implement assignment operator, I thought he would like it. The problem started with that.

He questioned why I implement it that way. When I explained that this is exception safe way of implementing it, he asked what's wrong in implementing it:

String& operator=(const String& other)
{
   if (this != &other)
   {
     delete[] m_data;
     m_data = new char[strlen(other.m_data) + 1] {'\0'};
     strcpy(m_data, other.m_data);
     return *this;
   }
}

His argument was:

  We are about to overwrite the data anyways so what's wrong in deleting first even if it's not exception safe?

In the moment it did not occur to me that code will crash in the event of an exception but I did try to argue that internal data should retain original values in the event of an exception.

Had he argued on the basis that copy-and-swap will always do reallocation even if there is enough size, etc, I would have felt happy. But his argument was that he wants performance at any cost.

Can somebody help me with a comparative review of these two operator= implementations so that I am in a better position to argue my case / understand the interviewer's position if I encounter similar situations in the future?

Your version

String& operator=(String other)
{
   std::swap(m_data, other.m_data);
   return *this;
}

is

Exception safe
Correct (assuming that copy and move constructors are implemented correctly)
Fairly efficient in C++11 if the move constructor is implemented
Potentially slightly less efficient than implementing separate copy & move constructors

The interviewer's version

String& operator=(const String& other)
{
   if (this != &other)
   {
     delete[] m_data;
     m_data = new char[strlen(other.m_data) + 1] {'\0'};
     strcpy(m_data, other.m_data);
     return *this;
   }
}

is terrible, for the following reasons:

If allocation fails, m_data becomes a dangling pointer and destruction (or another assign) will lead to a double delete.
If this == &other it doesn't return a value and therefore leads to undefined behavior (again).
Will fail if other.m_data == 0 (this might be ok if null strings are disallowed, but that would make even weak exception safety complicated)
Less seriously, it might lead to the content of m_data being initialized twice (depending on the optimizer and it's knowledge of strcpy)
Cannot be extended to support C++11 by implementing only an additional move constructor.

Furthermore, the style could be improved as well:

By using an early return instead of a long indented block
By not using C-style strings (and having a length member instead, although your version is presumably implemented in the same way as only m_data is swapped)
strlen and strcpy are not prefixed with std:: which they should be in C++ (having strlen and strcpy in the global namespace is allowed but not required by the standard.

A fixed version (still using a C-style representation) might look as follows:

String& operator=(const String& other)
{
   if(this == &other)
      return *this;

   delete[] m_data;
   m_data = 0; // weak exception safety, which might be sufficient, but only if null strings are allowed, complicating the following code

   if(!other.m_data) // strlen will fail if other.m_data == 0
      return *this;

   std::size_t length = std::strlen(other.m_data);
   m_data = new char[length + 1];
   std::memcpy(m_data, other.m_data, length + 1);

   return *this;
}


I believe there is a much simpler way of doing this.

If I correctly understand the problem, the idea is to produce a single column of frequencies.

with open('input.txt', 'r') as infile, open('out.csv', 'w') as outfile:
    print >> outfile, "Frequency"
    for line in infile:
        for freq in line.split()[2:]:
            print >> outfile, freq

This iterates over each line in the input file.  It uses split to eliminate whitespace and to split each line into fields.  It then discards the first two, which are assumed to be "Frequencies" and "--" and writes the remaining fields to the output file.


Collect all fields having given Modifier(s) I want to collect all fields which have given Modifier(s) of a class. I have written following pieces of codes:

/**
 * Method getFields
 * @author TapasB
 * @since 03-Nov-2013 - 2:03:17 pm
 * @version DAM 1.0
 * @param instanceClass
 * @return
 */
@SuppressWarnings({ "unchecked", "rawtypes" })
public static List<Field> getFields(Class<?> instanceClass) {
    List<Field> fields = new ArrayList<Field>();
    Class<?> searchType = instanceClass;

    while (!Object.class.equals(searchType) && searchType != null) {
        fields.addAll(Arrays.asList(searchType.getDeclaredFields()));
        searchType = searchType.getSuperclass();
    }

    return Collections.unmodifiableList(new ArrayList(selectDistinct(fields, "name")));
}

/**
 * Method getFields
 * @author TapasB
 * @since 03-Nov-2013 - 2:31:59 pm
 * @version DAM 1.0
 * @param instanceClass
 * @param includedModifiers
 * @param excludedModifiers
 * @return
 */
public static List<Field> getFields(Class<?> instanceClass, int[] includedModifiers, int[] excludedModifiers) {
    List<Field> fields = getFields(instanceClass);
    List<Field> filteredFields = new ArrayList<Field>();

    if(ArrayUtils.isEmpty(includedModifiers)) {
        filteredFields.addAll(fields);
    } else {
        for(Field field : fields) {
            boolean fieldCanBeAdded = false;

            for(int includedModifier : includedModifiers) {
                if((includedModifier & field.getModifiers()) != 0) {
                    fieldCanBeAdded = true;
                }
            }

            if(fieldCanBeAdded) {
                filteredFields.add(field);
            }
        }
    }

    if(ArrayUtils.isNotEmpty(excludedModifiers)) {
        Iterator<Field> fieldIterator = filteredFields.iterator();

        while (fieldIterator.hasNext()) {
            Field field = fieldIterator.next();
            boolean fieldNeedsToBeRemoved = false;

            for(int excludedModifier : excludedModifiers) {
                if((excludedModifier & field.getModifiers()) != 0) {
                    fieldNeedsToBeRemoved = true;
                }
            }

            if(fieldNeedsToBeRemoved) {
                fieldIterator.remove();
            }
        }
    }

    return Collections.unmodifiableList(filteredFields);
}

The method getFields(Class<?> instanceClass) searchs for fields in an iterative manner to all the super classes of the given class and stops at Object.

Here the selectDistinct is the method of LambdaJ API. It removes the fields with same name which is present in the parent class also in child class.

I was wondering if it is a good way to achieve what I want.

To run it:

public static void main(String[] args) throws IllegalArgumentException, IllegalAccessException, InvocationTargetException {
    int[] includedModifiers = new int[]{Modifier.PRIVATE};
    int[] excludedModifiers = new int[]{Modifier.FINAL};
    List<Field> fields = getFields(ArrayList.class, includedModifiers, excludedModifiers);

    for(Field field : fields) {
        System.out.println(field.getName());
    }
}

The main will print only the fields which have modifier only private and exclude the private final.

There is no issue so far I found with this code. I just want to know if there is any better solution exists or not.

I've played with your code and have a few broad observations....

The code is neat enough and generally readable.
JavaDocs are not complete - return values are not documented
@since JavaDoc tag traditionally records a version number, not a date.

I don't think there is a generally 'better' way to do what you are doing, but I think some of the specific details are worth commenting on..... and my last paragraph is an important one, so please make sure you consider it....

Some more specific comments:

Just check that you really need both getFields methods to be public. Exposing just the second version may be a better option and make the getFields(Class<?> instanceClass) version private. This is a matter of style and usage. In general I feel people make things public when they shouldn't. Whether that is right this time or not is your decision though
I feel you overuse the Collections.unmodifiableList(...). Generally this is only useful to protect data that is encapsulated in your class. In this case, the data is not something you need to protect, and worse, you need to 'undo' the unmodifiable state of the list in your exclude/include function. In this case, there is no value in making the lists read-only and if a 'user' decides they want to change the list they can do it without damaging your class, and all you do is make it harder for them (and yourself).
including the LambdaJ function dependency is perhaps unnecessary. The logic you want to do can be accomplished in a few lines with an iterator and a set. The burden of maintaining a dependency on an external library is potentially significant. If you have other needs for LambdaJ then sure, use it, but otherwise you may be satisfied with:

Iterator<Field> it = fields.iterator();
Set<String> names = new HashSet<String>();
while (it.hasNext()) {
    Field f = it.next();
    if (!names.add(f.getName())) {
        it.remove();
    }
}

Finally, I have to question why you want to remove 'shadowed' fields at all. They are different fields and you are losing information if you remove them. Are you sure you want them gone?


How to deal with some of the dependencies? I am very beginning in PHP and Design Patterns. I have been studying the beautiful book "Head first: Design pattern" and I have been working in some of the codes originally presented in Java in order to learn more about PHP-OOP. The code below presents a kind of Observer Pattern. It is working well, however there are still some design choices that bother me? Can one help me improve this piece of code.

Questions:

Am I using a good PHP practice?
The entire code seems to be based on the idea that there are three pieces of information that are relevant, namely $temperature, $humidity and $ pressure. However, it seems that this is not a good design, since I may eventually in the future need another piece of information such as for instance pluviometric index... Is there a good way to avoid this kind of dependency?
Is it a good choice the class currentConditionsDisplay to implement both Observer and DisplayElement? It seems to be another undesirable dependency.
Are there other kinds of undesirable dependencies?
Is a better design pattern to define currentConditionsDisplay as static?

Any help is very welcome.

<?php

interface Observer{
    public function update($temp,$humidity,$pressure);
}

interface Subject{
    public function registerObserver(Observer $o);
    public function removeObserver(Observer $o);
    public function notifyObserver();
}

interface DisplayElement{
    public function display();
}

class WeatherData implements Subject{
    private $observers;
    private $temperature;
    private $humidity;
    private $pressure;

    public function __construct(){
        $this->observers=array();
    }
    public function registerObserver(Observer $o){
        array_push($this->observers,$o);
    }
    public function removeObserver(Observer $o) {
        unset($this->observers["$o"]);
    }
    public function notifyObserver(){
        foreach($this->observers as &$o){
            $o->update($this->temperature,$this->humidity,$this->pressure);
        }
    }
    public function measurementsChanged(){
        $this->notifyObserver();
    }
    public function setMeasurements($temperature,$humidity,$pressure){
        $this->temperature=$temperature;
        $this->humidity=$humidity;
        $this->pressure=$pressure;
        $this->measurementsChanged();
    }
}

class CurrentConditionsDisplay implements Observer, DisplayElement{
    private $temperature;
    private $humidity;
    private $weatherData;

    public function __construct(Subject $weatherData){
        $this->weatherData=$weatherData;
        $weatherData->registerObserver($this); // $this reffers to the currentConditionsDisplay
    }

    public function display(){
        echo "Current conditions: "."$this->temperature"." F degrees and "."$this->humidity"." humidity";
    }

    public function update($temperature,$humidity,$pressure){
        $this->temperature=$temperature;
        $this->humidity=$humidity;
        $this->display();
    }
}

$weatherData=new WeatherData();

$currentConditionsDisplay=new currentConditionsDisplay($weatherData);

$weatherData->setMeasurements(80,60,30.45);

?>

Your are using the observer-pattern in a right way. The pattern itself is probably one of the most important ones. In PHP it is not used that much though as applications have a rather quick life-cycle. Yet good to know the pattern of course. Some minor remarks about your code though:

All methods in an interface are public, no need to repeat yourself (in interfaces).
The name Observer and Subject indicate an use-case independent use. Yet your Observer are coupled to your Weather component. Better names would be WeatherDataObserver and WeatherDataSubject.
I'd consider it standard to have at least a whitespace between the function's header and the opening bracket: registerObserver(Observer $o) {
Not sure if your WeatherData is actually a service. For me the name WeatherData indicates a set of values, not necessarily the current values. I'd probably rename it to WeatherDataService.
notifyObserver should not be public. I don't want anyone else to trigger (possibly false positives) notifications. Better remove it from the interface too.
measurementsChanged serves no purpose yet. Replace it with notifyObserver.

Your concerns are right. If I remember the book correct, they present an alternative approach on this pattern: instead of pushing the changed values to the observers, just inform them something has changed and let them pull the new values from the subject. This changes the signature of your observer to function update(Subject $weatherData) and in the update method of your CurrentConditionsDisplay you get the new values by:

public function update(Subject $subject){
    $this->temperature = $subject->getTemperature();
    // and so on
}

Another approach would be some event'ed one. Send an event-object about what has changed. This pushed the changes to your observers and still avoids dependencies. This approach is a bit more work though.
This is a pattern from java user-interfaces (or any views that can update themselves). In PHP you usually don't use this pattern to update user interfaces as PHP can't update the UI :). You'd access the WeatherData directly when rendering the view. Nothing really bad about this here though.
Fine for me from what I can see.
No. Generally there are very very very few proper uses for static methods. Static methods are completely independent from others and object internal state. Successive calls to static methods should always have the same result (and should be side-effect free), no matter what happened in the application meanwhile. Usually this applies for simple helper functions only (e.g. abs or other math. helpers). Even this could be discussed though. CurrentConditionsDisplay has some internal state though, therefore static is a no-go.


Blackjack casino game Blackjack (twenty-one) is a casino game played with cards. The goal of
  the game to draw cards that total as close to 21 points as possible
  without going over. All face cards count as 10 points, aces count as 1
  or 11, and all other cards count their numeric value.

  The game is played against a dealer. The player tries to get closer to
  21 (without going over) than the dealer. If the dealer busts (goes
  over 21) the player automatically wins (provided the player had not
  already busted). The dealer must always take cards according to a
  fixed set of rules. The dealer takes cards until he or she achieves a
  total of at least 17. If the dealer's hand contains an ace, it will be
  counted as 11 when that results in a total between 17 and 21
  inclusive; otherwise, the ace is counted as 1.

  Write a program that simulates multiple games of blackjack and
  estimates the probability that the dealer will bust. Hints: treat the
  deck of cards as infinite. You do not need to keep track of the cards
  in the hand, just the total so far (treating an ace as 1) and a bool
  variable hasace that tells whether or not the hand contains an ace. A
  hand containing an ace should have 10 points added to the total
  exactly when doing so would produce a stopping total(something between
  17 and 21 inclusive).

It runs, but I'm just looking to improve it if possible.

def printIntro():
    print("This program simulates a bunch of blackjack games.")

def getInputs():
    while True:
        n = eval(input("How many games should be simulated? "))
        if n <= 0:
            print("You must enter a number greater than zero!")
        else:
            break

    return n

def simOneGame():
    score = 0
    blacklist = [2, 3, 4, 5, 6, 7, 8, 9, 10, 10, 10, 10, 11]
    while not gameOver(score):
        blackscore = blacklist[randrange(len(blacklist))]
        score = score + blackscore

        if score >= 17:
             break
        if blackscore == 11 and (score >= 6 and score <= 10):
            score = score + 11
        else:
            score = score + 1

    return score

def simNGames(n):
    holds = 0
    busts = 0
    for i in range(n):
        score = simOneGame()
        if score >= 22:
            busts = busts + 1
        else:
            holds = holds + 1

    return holds, busts

def gameOver(score):
    return score >= 22 or score >= 17

def printSummary(holds, busts):
    n = holds + busts
    print("Number of holds and busts: Holds: {0:0} Busts: {1:0}".format(holds, busts))
    print("Percentage of holds and busts: Holds: {0:0.1%}".format(holds / n), end = " ")
    print("Busts: {0:0.1%}".format(busts / n))

def main():
    printIntro()
    n = getInputs()
    score = simOneGame()
    holds, busts = simNGames(n)
    printSummary(holds, busts)

if __name__ == "__main__":
    main()

def printIntro():
    print("This program simulates a bunch of blackjack games.")

Python style guide recommend lowercase_with_underscores for function names

def getInputs():
    while True:
        n = eval(input("How many games should be simulated? "))

not a really good idea to use eval, because that allows the user to input any python expression which may not make sense. Instead use int, which will only allow numbers

        if n <= 0:
            print("You must enter a number greater than zero!")
        else:
            break

    return n

def simOneGame():
    score = 0
    blacklist = [2, 3, 4, 5, 6, 7, 8, 9, 10, 10, 10, 10, 11]
    while not gameOver(score):
        blackscore = blacklist[randrange(len(blacklist))]

Python has a function, random.choice() which returns a random element from a list.

        score = score + blackscore

        if score >= 17:
             break

Why? The loop will terminate anyways. There doesn't seem to be a reason to break here

        if blackscore == 11 and (score >= 6 and score <= 10):
            score = score + 11
        else:
            score = score + 1

I assume that this has to do with handling aces, but I don't understand the logic. You've already added blackscore to score, so I wouldn't think this would do the right thing. Also, couldn't you need to change the value of an ace to a 1, from an eleven due to later cards?

    return score

You only really care about busts/holds so maybe you should return True or False?

def simNGames(n):
    holds = 0
    busts = 0
    for i in range(n):
        score = simOneGame()
        if score >= 22:
            busts = busts + 1
        else:
            holds = holds + 1

    return holds, busts

def gameOver(score):
    return score >= 22 or score >= 17

That's the same as score >= 17, no reason to include the first part of the condition

def printSummary(holds, busts):
    n = holds + busts
    print("Number of holds and busts: Holds: {0:0} Busts: {1:0}".format(holds, busts))
    print("Percentage of holds and busts: Holds: {0:0.1%}".format(holds / n), end = " ")
    print("Busts: {0:0.1%}".format(busts / n))

def main():
    printIntro()
    n = getInputs()
    score = simOneGame()
    holds, busts = simNGames(n)
    printSummary(holds, busts)

if __name__ == "__main__":
    main()


Subscribing and manipulating data from API in Angular I'm trying to display the fetched data on a table. I structured the data based on the columns table in the UI and stored them in an array, then *ngFor on the table rows on the frontend. Good or bad practice? Or are there other ways to do this in a much simpler way?

    getLicensesofDealer(id) {
        this._license.get_license_by_id(id).subscribe(
            (data: License[]) => {
                let counter = 1;

                // Structure data and store in an array
                data.forEach(l => {
                    const license = {
                        unique_key: l.licenseId,
                        count: counter,
                        key: l.licenseKey,
                        date: l.dateCreated
                    }
                    this.license_data.push(license);
                    counter++;
                })
                // Structure data and store in an array

            }
        )
    }

just my personal opinion, so pick what you like and ignore the rest. :-)

In general i think it is a good practice to use your "own" data structure in the frontend.
if the backend changes their format, then you only have to change your mapping functions, and the rest could stay the same. If you use the backend format directly, then your code depends on that format. Means, if the backend changes a name, you would have to change that name all over your application...

Some points in general:

subscribe/unsubscribe
the first thing i would like to mention is, that if you subscribe in a component, then this subscription will be still active, after your component is long dead. Or to make it more explicit, when you "subscribe", then call the method "subscribe" on the observable. And you provide a (fat arrow) function to it. This function is then stored in the observable. Now, each time the observable emits a value, the function is executed. Because this function now "lives" in the observable, it does not get cleaned up when your component is destroyed. This can create memory leaks and/or unwanted behavior.

Therefor if your oberservable does not complete by itself (e.g. an http call, will only do one emit and then "complete") you should always clean up your subscriptions. This could be done by saving them all into a "Subscription" object.

private subscriptions: Subscription = new Subscription();
// ...
this.subscriptions.add(
    this.myObservable.subscribe( (...)=>{...})
)
this.subscriptions.add(
    this.myOtherObservable.subscribe( (...)=>{...})
)

ngOnDestroy(){
  this.subscriptions.unsubscribe();
}

forEach vs. map
forEach is used if you want to execute something for each element of an array. Map is used, if you want to change the format of each element. In your case you want to change the format, so i would prefer "map"

let counter = 1;
this.licence_data = data.map( el => {
   return {
     count: counter++,
     unique_key: el.licenceId,
     key: el.licenceKey,
     date: el.dateCreated
   };
}

Readability by extracting into methods
I personaly like it, when i don´t have to read 5 lines (often much more) of code to understand what happens here. Its much easier if a method is called and the name of the method gives me a good clue what happens in it. As a result, sometimes i don´t even have to look into the method, because the name alone tells me that the problem i am hunting is not there. OR the name gives me some context which allows me to understand the content of the method more easily.
And last but not least, splitting code into multiple methods means to separate the code into chunks which communicate with each other over interfaces. And that makes changes much easier, because, as long as i do not change the interface (the method signature), as long can i do changes without having to fear to break something outside. That makes refactorings much easier.

this._licence.get_licence_by_id(id).subscribe(
  (data: Licence[] => this.licence_data = this.mapToUIFormat(data)
);

this.mapToUIFormat(licences: Licence[]): MyUiFormat { // i would give that format an interface
    let counter = 1;
    return data.map(  licence => {
       return {
         count: counter++,
         unique_key: licence.licenceId,
         key: licence.licenceKey,
         date: licence.dateCreated
       };
    }

}

The advantage of this method is, that its a "pure" method. It only expects some input, and it returns an output that ONLY depends on the input. It also has no side effects, means it does not change anything else. That makes it easier to understand and very easy to test.

If you want you could also extract the mapping of a single licence into the ui format in a extra method. It depends on the complexity of the code, but in some circumstances it really helps to structure the code.


Reducing boilerplate of binary layout handling in Haskell Here is the code: https://github.com/EarlGray/haskell-snippets/blob/master/ext2hs/Ext2.hs

My problem is that it is quite painful to make large Haskell "records" and deserialize a binary layout with Data.Binary.Get manually like this:

instance Binary Superblock where
  get = binGetSuperblock

binGetSuperblock = do
  uint13  <- replicateM 13   getWord32le
  ushort6 <- replicateM 6    getWord16le
  uint4   <- replicateM 4    getWord32le
  ushort2 <- replicateM 2    getWord16le
  return Superblock {
    sInodesCount = uint13 !! 0,           sBlocksCount = uint13 !! 1,
    sReservedBlocksCount = uint13 !! 2,   sFreeBlocksCount = uint13 !! 3,
    sFreeInodesCount = uint13 !! 4,       sFirstDataBlock = uint13 !! 5,
    sLogBlockSize = uint13 !! 6,          sLogClusterSize = uint13 !! 7,
    sBlocksPerGroup = uint13 !! 8,        sClustersPerGroup = uint13 !! 9,
    sInodesPerGroup = uint13 !! 10,       sMountTime = uint13 !! 11,
    sWriteTime = uint13 !! 12,

    sMountsCount = ushort6 !! 0,    sMaxMountsCount = int (ushort6 !! 1),
    sMagic = ushort6 !! 2,          sState = ushort6 !! 3,
    sErrors = ushort6 !! 4,
    sLastCheckTime = uint4 !! 0,    sCheckInterval = uint4 !! 1,
    sCreatorOS = uint4 !! 2,        sRevLevel = (uint4 !! 3, ushort6 !! 5),

    sDefaultResUID = ushort2 !! 0,  sDefaultResGID = ushort2 !! 1,

    sDynRev = Nothing,
    sPrealloc = Nothing,
    sJournaling = Nothing
  }

(this is ext2fs superblock structure).

First of all, I would like to make binary layout as much declarative as possible, abstracting away those ugly sequences of getWord32le in Get monad with temporary fields. I want something like my python code, where binary fields are unpacked to a map according to a struct format string

The second thing I need is a more concise and more clear way for initializing a large Haskell record (creating a lot of temporary named values in the monad is pain).

One way I think of is to read binary fields into a heterogeneous map forall a. BinaryField a => Map String a, accessing each value by its name and pre-extracting most used ones to Haskell.
Perhaps Template Haskell and code generation from a declarative format may help, but I have no experience with TH yet.

I would be glad to hear any suggestions or hints about a more idiomatic way (are there any relevant tools?) or about the code in general (though it's just my learning code and it's very incomplete at the moment).

To add one more suggestion - building records can be simplified quite a bit by using the RecordWildCards extension, which automatically populates fields from visible names (see GHC docs). Personally, I consider it borderline essential for dealing with large records. I would probably write this like follows:

{-# LANGUAGE RecordWildCards #-}
-- ...
binGetSuperblock = do
  [sInodesCount,         sBlocksCount,
   sReservedBlocksCount, sFreeBlocksCount,
   sFreeInodesCount,     sFirstDataBlock,
   sLogBlockSize,        sLogClusterSize,
   sBlocksPerGroup,      sClustersPerGroup,
   sInodesPerGroup,      sMountTime,
   sWriteTime]                            <- replicateM 13 getWord32le
  [sMountsCount,         sMaxMountsCount,
   sMagic,               sState,
   sErrors,              sRevLevel2]      <- replicateM 6  getWord16le
  [sLastCheckTime,       sCheckInterval,
   sCreatorOS,           sRevLevel1]      <- replicateM 4  getWord32le
  [sDefaultResUID,       sDefaultResGID]  <- replicateM 2  getWord16le
  return Superblock {
    sMaxMountsCount = int sMaxMountsCount,
    sRevLevel = (sRevLevel1, sRevLevel2),
    sDynRev = Nothing,
    sPrealloc = Nothing,
    sJournaling = Nothing,
    ..
    }


Java practise exam question Doing practise questions for a Java exam which have no answers (useful) I have been asked to do the following:

Write a class called Person with the following features:

a private int called age;
a private String called name;
a constructor with a String argument, which initialises name to the received value and age to 0;
a public method toString(boolean full). If full is true, then the
method returns a string consisting of the person’s name followed by their
age in brackets; if full is false, the method returns a string consisting of
just the name. For example, a returned string could be Sue (21) or Sue;
a public method incrementAge(), which increments age and returns the
new age;
a public method canVote() which returns true if age is 18 or more and
false otherwise.

My code is as follows can someone point out any errors?

public class Person
{
    private int age;
    private String name;

    public Person(String st)
    {
      name = st;
      age = 0;
    }

   public String toString(boolean full)
   {
     if(full == true)
     {
        return name + "(" + age + ")";
     }
     else
     {
        return name;
     }
   }

   public int incrementAge()
   {
      age++;
      return age;
   }

   public boolean canVote()
   {
     if(age >= 18)
     {
        return true;
     }
     else
     {
        return false;
     }
   }
}

instead of

if(full == true)

use

if (full)

and instead of

if(age >= 18)
{
   return true;
}
else
{
   return false;
}

use:

return age >= 18