Simple Cookie Service - Returns value of cookie based on cookie name passed in Introduction

So I have written a simple service that, when loaded, reads all of the cookies in the document, reduces them to a simple object where the cookie name is the key, and the cookie value is the value, and then allows you to get a cookie by name by checking cookies[cookieName].

It works great, but I guess my biggest questions are:

Questions

Where can I improve the code of course
Is it worth optimizing the code to use a for loop instead of reduce, or is it better to leave the code more succinct by using reduce?
Will most developers know what reduce does? Should I update the code to use for simply to make it easier for other team members to understand what the code does?

But of course, feel free to pick it apart however, I'm always up for learning better ways of coding.

Other Stuff

I'm sure this code could be enhanced by allowing the setting of cookies as well, but for now my needs are simply to read cookies as my application should never be adding or modifying cookies. Therefore set functionality was intentionally left out to discourage those implementing the class from doing so.

The Code

export class CookieService {

    // Constructor
    constructor(){

        // Initialize the cookie service
        this.init()
    };

    // Holds the resulting cookies object after init is run
    cookies: any;

    // Initializes the cookie service by getting all cookies on the document and creating an object that holds each cookie by name in a key/value pair
    init: Function = () => {

        // Get the document cookie
        this.cookies = document.cookie

            // Split it on the ";" character to get each individual cookie in an array
            .split(";")

            // Reduce the array into an object
            .reduce(

                // Pass in the cookies object and the current value to the reduce callback function
                ( cookies, currentValue ) => {

                    // Split the cookie into key and value
                    let cookie = currentValue.split("=");

                    // Sanity check to make sure we got two values from the split and each value has a length.  Filters out malformed cookies.
                    if ( cookie.length === 2 && typeof cookie[0] === "string" && cookie[0].length && typeof cookie[1] === "string" && cookie[1].length ){

                        // Get the key. Trim removes any spaces from the resulting key to ensure proper matches.
                        let key = String(cookie[0]).trim();

                        // Get the value.  Trim removes any spaces from the resulting value to ensure proper matches.
                        let value = String(cookie[1]).trim();

                        // Set the new cookie on the cookies object
                        cookies[key] = value;

                        // Return the new cookies object
                        return cookies;
                    }
                }, {}
            )
    };

    // Gets a cookie from the cookies object
    getCookie: Function = cookieName => {

        // If we have a cookies array, and the requested cookie exists
        if ( typeof this.cookies !== "undefined" && this.cookies[cookieName]){

            // Return the requested cookie
            return this.cookies[cookieName];

        } else {

            // Otherwise return false to let the requester know the cookie was not found
            return false;
        }
    }
}

Re using reduce(): There are many Javascript programmers who like to find clever ways to use this method, and your pattern is probably popular among them. I'm not one of them. I would use forEach() instead, and assign to this.cookies[key] within it. But if you like reduce(), that should be fine; it's a common enough pattern that you shouldn't have to worry about other programmers not understanding it.

However, there's a bug in the way you're using reduce(). The callback function has to return a result every time, but your function only returns in the if block. You should move return cookies; outside the if.

You have some unnecessary code when expanding currentValue.
String.prototype.split always returns an array of strings, so it's not necessary to test if typeof cookie[i] == "string". And once you have tested this, it's not necessary to use String(cookie[i]). You don't have to go overboard on defensive coding.

When returning the cookie, you can simplify the code. It's not necessary to test the value and then return false if it's not truthy; you can simply return the value, and if it doesn't exist you'll return undefined, which is just as falsey as false. And rather than test specifically for typeof this.cookie == "undefuned", you can just test if the property is truthy (if you want to be really defensive, you could go the other way and test whether it's actually an array).

Since you're using other EcmaScript 6 features, you could use destructuring of the split() assignment, rather than referring to cookie[0] and cookie[1] repeatedly. Then you can simplify the validation checks -- all the invalid values are also falsey, so if (key && value) will suffice.

export class CookieService {

    // Constructor
    constructor(){

        // Initialize the cookie service
        this.init()
    };

    // Holds the resulting cookies object after init is run
    cookies: any;

    // Initializes the cookie service by getting all cookies on the document and creating an object that holds each cookie by name in a key/value pair
    init: Function = () => {

        // Get the document cookie
        this.cookies = document.cookie

            // Split it on the ";" character to get each individual cookie in an array
            .split(";")

            // Reduce the array into an object
            .reduce(

                // Pass in the cookies object and the current value to the reduce callback function
                ( cookies, currentValue ) => {

                    // Split the cookie into key and value
                    let [key, value] = currentValue.split("=");

                    // Sanity check to make sure we got two values from the split and each value has a length.  Filters out malformed cookies.
                    key = key && key.trim();
                    value = value && value.trim();
                    if ( key && value ){

                        // Set the new cookie on the cookies object
                        cookies[key] = value;

                    }

                    // Return the updated cookies object
                    return cookies;

                }, {}
            )
    };

    // Gets a cookie from the cookies object
    getCookie: Function = cookieName => {

        // If we have a cookies array, and the requested cookie exists, return it
        return this.cookies && this.cookies[cookieName];

    }
}


Determine whether a node is reachable I am working on this HackerRank problem but running into time constraint issues.

Basically, given two arrays, we need to determine if the ith element of the second array is reachable by the ith element of the first array, where edges are determined by whether or not the gcd of the two node values is greater than some threshold. Below is my attempt but it times out for large inputs.

def computeGCD(x, y):
   while(y):
       x, y = y, x % y
   return x

def find_all_paths(graph, start, end, path=[]):
    path = path + [start]
    if start == end:
        return [path]
    if start not in graph.keys():
        return []
    paths = []
    for node in graph[start]:
        if node not in path:
            newpaths = find_all_paths(graph, node, end, path)
            for newpath in newpaths:
                paths.append(newpath)
    return paths

def connectedCities(n, g, originCities, destinationCities):
    res = []
    graph = {i+1:[] for i in range(n)}
    for i in originCities:
        for j in range(n):
            if i != j+1 and computeGCD(i, j+1) > g:
                graph[i].append(j+1)

    for i in range(len(originCities)):
        paths = find_all_paths(graph, originCities[i], destinationCities[i])
        if len(paths) > 0:
            res.append(1)
        else:
            res.append(0)

    return res

Can you help me determine if there is something I can be doing more efficiently here, or if solving this via a graph is even the most appropriate way?

def computeGCD(x, y):

Just call it gcd. It's part of a computer program: you don't need to say that it computes.

    graph = {i+1:[] for i in range(n)}
    for i in originCities:
        for j in range(n):
            if i != j+1 and computeGCD(i, j+1) > g:
                graph[i].append(j+1)

This is buggy: it doesn't build the whole graph. Consider test case connectedCities(16, 1, [14], [15]): there is a path 14 -- 6 -- 15 with GCDs respectively 2 and 3.

As a matter of style, I would find the code more readable if it iterated over range(1, n+1) and didn't have to continually increment the variable.

    for i in range(len(originCities)):
        paths = find_all_paths(graph, originCities[i], destinationCities[i])
        if len(paths) > 0:
            res.append(1)
        else:
            res.append(0)

Spot the big performance problem: to determine whether any foo exists, it suffices to find one foo. You don't need to find every foo in the universe and then count them.

But just fixing that still leaves a smaller performance problem: if there are a lot of queries (i.e. if originCities and destinationCities are long) then it's quicker to do an expensive preprocessing to get an object which answers queries fast than it is to evaluate each query separately. As a big hint, in graph-theoretic terms the queries are "Are these two vertices in the same connected component?".

Note that if you really want to squeeze the asymptotic complexity (and it's not too bad for practical performance either), \$O(n \lg \lg n + q)\$ is achievable1 where \$q\$ is the number of queries.

1 Technically there's also a factor of \$\alpha(n)\$ where \$\alpha\$ is the inverse Ackermann function, but for practical purposes it's constant.


Heap's algorithm implementation - permutations I wrote an implementation of Heap's permutation algorithm in JavaScript (ES2015):

const swap = (array, pos1, pos2) => {
  const temp = array[pos1];

  array[pos1] = array[pos2];
  array[pos2] = temp;
}

const heapsPermute = (() => {
  const result = [];

  return (array, n = array.length) => {
    const newArray = typeof array === 'string'
      ? array.split('')
      : array;

    if (n === 1)
      result.push(newArray.join(''));

    for (let i = 1; i <= n; i++) {
      heapsPermute(newArray, n - 1);

      const j = n % 2
        ? 0
        : i - 1;

      swap(newArray, j, n - 1);
    }

    return result;
  };
})();

console.log(heapsPermute('abcdef').length);

It should take an array|string argument and return array of strings with all permutations.

What do you think about it. Is there room for improvement anywhere?

From a short review:

Keep the typeof check out of the recursive part, you only need to do this during the first call of the function
The fat arrow syntax is good for inline functions, in this case I would highly encourage you to use the function keyword
If nis 1, get out of the function after the push, there is no need to execute the remainder
I like verbThing for naming function, so perhaps use swapListElements and permutateList permuteList
"heap" is not a JavaScript thing, I would not use it in your naming unless you actually created a "Heap" object
newArray is not a great name, especially since most of the time it is not actually a new array, but the array that was passed to the function
I had to re-read the code that uses j, the variable is not intuitively named, and the code could use a comment there
Furthermore, the ternary statements over 3 lines are mind breaking, I would keep them on 1 line

This is my version of Heap's algorithm, with the above taken into account

function swap(list, pos1, pos2){
  const temp = list[pos1];
  list[pos1] = list[pos2];
  list[pos2] = temp;
}

//Implements Heap's permutation algorithm
//https://en.wikipedia.org/wiki/Heap%27s_algorithm
function permute(list) {

  var out = [];
  list = typeof list === 'string' ? list.split('') : list;
  permuteList(list, list.length);

  function permuteList(list, n) {
    var i;

    if (n == 1) {
      out.push(list.join(''));
    } else {
      for (i = 0; i < n - 1; i++) {
        permuteList(list, n - 1);
        if (n % 2) {
          swap(list,0, n - 1);
        } else {
          swap(list,i, n - 1);
        }
      }
      permuteList(list, n - 1);
    }
  }
  return out;
}

console.log(permute('abc'));
console.log(permute('abcdef').length);


Leap year determination using guard clause I have a function to check the leap year as follows

bool IsLeapYear(int year)
{
    if (year % 4 == 0)
    {
        if (year % 100 == 0)
        {
            if (year % 400 == 0)
                return true;
            else
                return false;
        }
        else
            return true;
    }
    else
        return false;
}

I was wondering whether it would be correct to change this function by using guard clauses like below

bool IsLeapYear(int year)
{
    if (year % 4 != 0)
    {
        return false;
    }

    if (year % 4 == 0 && year % 100 == 0 && year % 400 != 0)
    {
        return false;
    }

    return true;
}

Is it going to introduce any side effects by using guard clause like this or is there a better way to write this function?

Compilers

Welcome to Code Review. First of all, "better" is a very personal opinion term, unless it has been defined. From a compiler's point of view, both functions do the same. As compilers are data flow analysis beasts, a decent one will simplify the function's flow to the following variant (see https://godbolt.org/z/yZCPOv):

ret = 0
if year & 3 /= 0
   goto end
ret = 1
if year % 100 /= 0
   goto end
ret = (year % 400 == 0)

end:
   return ret
return 0

As you can see, with -Os (or any other optimization level), your functions will result in the same assembler. This already answers your first (implicit) question:

  I was wondering whether it would be correct to change this function by using guard clauses like below

"Better"

Since both generate the same assembler, both are the same. We can of course check that by hand, as there are only four equivalence classes we have to check, but that's left as an exercise.

Regardless, the better way is the one that's easier to understand and follows fits into the rest of your code. If all your code is written in the first style, use that one. If you usually bail out early, use the latter. Neither have advantages over the other as soon as we enable optimizations. Without optimizations, your latter variant uses less jumps (but only a single one), and that's still compiler dependent.

In the end, use the variant you like most, but make sure you can still understand and explain it to yourself or someone else at a later time.

Just for completion, I personally prefer variants that have less jumps but are still readable, so I'd probably write something along

//! @returns whether @c year is a leap year
bool IsLeapYear(int year)
{
    //! A year is a leap year if its divisible by 4, but not by 100,
    //! @b unless it's also divisible by 400.
    return  (year %   4 == 0)
        && ((year % 100 != 0) || (year % 400 == 0));
}


JSON call for stock info using YQL I got this code working but it feels clunky. I am wondering if there is a way I can improve it.

(I'm intermingling JavaScript and jQuery). I'm using jquery-1.11.1.min.js hosted on code.jquery.com.

Here is the jsFiddle

JS:

HTML:

        S&amp;P 500

        Nasdaq 100

        SPDR Gold

        iShares Silver

        Nikkei 225

        FTSE

CSS:

Update Here is the final product of the great suggestions below and some additional changes. I also added a 'stretchy' ability that targets the parent container size and when that view able space gets to small to work with it hides the stock information bar.  I did this to reduce clutter for mobile users.

http://jsfiddle.net/franktudor/LZm2f/

and it works like a charm and I hope someone else finds this useful for their website.

Don't repeat element IDs. What you want is a data-* attribute, e.g. data-property. Or you could use class names, since those can repeat and you'll want to style things anyway. Of course, you could also build your elements as needed.
Use CSS classes properly. If you've set the containing DIV's class, you just need to write your CSS to automatically style its descendants properly, instead of setting a class like redtext. While you're at it, don't use an element name in your class names. If you want target a specific tag a selector like div.green is what you need, in which case the class name would just be green. However, green is problematic too; you're not saying it should be green, you're saying that that index/stock is up or rising. So name your classes that; i.e. name them something semantically appropriate.
It'd be simpler to first collect all the symbol names, and then query Yahoo once for all of them, instead of firing off queries one at a time.
Use $.getJSON's ability to accept a data object, rather than manually creating a query string

For instance, you could use this HTML (repeat as needed, unless you want to simply build it in JS):

  S&amp;P 500

and this CSS:

.symbol { /* basic styling */ }

.symbol.up { /* styling for a symbol that's "up" */ }
.symbol.up .change { /* styling for the change span */ }

.symbol.down { /* styling for a symbol that's "down" */ }
.symbol.down .change { /* styling for the change span */ }

As for the JavaScript

var elements, symbols, query;

// get the elements
elements = $(".symbol");

// get the symbols and join them to a comma-separated string
symbols = elements.map(function () {
    var symbol = $(this).data("symbol")
    return "'"+symbol+"'";
}).toArray().join(",");

// get the quotes
$.getJSON("http://query.yahooapis.com/v1/public/yql", {
    format: "json",
    diagnostics: "true",
    env: "http://datatables.org/alltables.env",
    q: "select * from yahoo.finance.quotes where symbol in (" + symbols + ")"
}, function (data, xhr, status) {
    // TODO: sanity checking of the data
    data.query.results.quote.forEach(function (quote) {
        var element = elements.filter("[data-symbol='"+quote.Symbol+"']:first");
        if(/^\+/.test(quote.PercentChange)) {
            element.addClass("up");
        } else {
            element.addClass("down");
        }
        element.children("[data-property]").each(function () {
            var child = $(this),
                prop = child.data("property");
            child.text(quote[prop]);
        });
    });
});

Here's a jsfiddle

Of course, a much simpler approach would to be build elements as needed. Keep the CSS but remove all the HTML (except the indices div, just so we have a place to put stuff) in favor of letting your JS do the work of setting it all up.

var symbols = ['^GSPC', '^NDX', 'GLD', 'SLV', '^N225', '^FTSE'],
    properties = [
        { classname: 'name', property: 'Name' },
        { classname: 'result', property: 'LastTradePriceOnly' },
        { classname: 'change', property: 'Change' }
    ];

function buildElement(quote) {
    var container = $("

").addClass("symbol");
    properties.forEach(function (prop) {
        var child = $("").addClass(prop.classname);
        child.text(quote[prop.property]);
        container.append(child);
    });
    if(/^\+/.test(quote.PercentChange)) {
        container.addClass("up");
    } else {
        container.addClass("down");
    }
    return container;
}

$.getJSON("http://query.yahooapis.com/v1/public/yql", {
    format: "json",
    diagnostics: "true",
    env: "http://datatables.org/alltables.env",
    q: "select * from yahoo.finance.quotes where symbol in ('" + symbols.join("','") + "')"
}, function (data, xhr, status) {
    // do some sanity checking of the data here
    var elements = data.query.results.quote.map(buildElement);
    $("#indices").append(elements);
});

Here's a jsfiddle of that.


Diceware Passphrase Generator I've written a simple Diceware passphrase generator based on this Stack Overflow question and the top answer.

Briefly, in the Diceware method, a die is rolled 5 times for each word in the passphrase and the results matched to the corresponding word in the word list which is of the form:

16226   cask
16231   casket
16232   cast
16233   caste
16234   cat
16235   catch

I'm still very much a Python and programming beginner and appreciate any feedback, but my main concerns are:

Is the method I use to generate keys biased?
Is my handling of command line arguments correct or could it be done better?
Is the way I've broken my code down into individual functions reasonable or should certain functions be combined or broken apart?
Are there any other glaring weaknesses?

import os
import sys

WORDS = "diceware_wordlist.txt"

def exists_arg():
    """Checks if argv input was given."""
    if len(sys.argv) == 1:
        return False
    else:
        return True

def test_arg(arg):
    """Checks if input is of the required form and returns
    various failure cases.
    """
    try:
        arg = int(arg)
    except ValueError:
        return 'Please enter an integer.'
    if arg < 5:
        return 'Length must be >= 5.'
    elif arg > 10:
        return 'Length must be <= 10'
    else:
        return 0, arg

def load_dict():
    """Returns a dictionary of the number to word mappings
    in the Diceware words file.
    """
    word_dict = {}
    with open(WORDS, 'r', 0) as in_file:
        for line in in_file:
            tmp = line.split()
            key = tmp[0]
            value = tmp[1]
            word_dict[key] = value
    return word_dict

def generate_key():
    """Takes 6 bytes one at a time from the OS random device,
    converts them to integers, adds them together, and takes
    the modulo 6 value as a piece of a 5 digit key.

    Returns a key for a value in the Diceware dictionary.
    """
    key = ''
    for i in range(5):
        digit = 0
        for j in range(6):
            digit += ord(os.urandom(1))
        key += str(digit%6 + 1)
    return key

def generate_passphrase(length):
    """Returns a randomly chosen passphrase based on desired length."""
    word_dict = load_dict()
    passphrase = ''
    for i in range(1, length + 1):
        key = generate_key()
        passphrase += word_dict[key] + ' '
    return passphrase

def main():
    flag = True
    while True:
        if flag and exists_arg():
            arg = sys.argv[1]
            flag = False
        else:
            arg = raw_input('Length of desired passphrase: ')
        check = test_arg(arg)
        if check[0] == 0:
            print generate_passphrase(check[1])
            return
        else:
            print check

if __name__ == '__main__':
    main()

Is my handling of command line arguments correct or could it be done better?

It would be better to not reinvent the wheel and use argparse instead.

  Is the way I've broken my code down into individual functions reasonable or should certain functions be combined or broken apart?

It's quite fine the way you did it.
Every function seems to do just one thing, it's good like that.

  Are there any other glaring weaknesses?

Instead of this:

if len(sys.argv) == 1:
    return False
else:
    return True

Write like this:

return len(sys.argv) != 1

Instead of this:

with open(WORDS, 'r', 0) as in_file:

I'd recommend simply:

with open(WORDS) as in_file:

Instead of this:

for i in range(1, length + 1):
    key = generate_key()
    passphrase += word_dict[key] + ' '

This is simpler:

for _ in range(length):
    key = generate_key()
    passphrase += word_dict[key] + ' '

I replaced the loop variable i with _,
because it's a common convention when the loop variable is not used.

Lastly,
I recommend to switch to start using Python 3 instead of 2.7.


Quickly applying gravity force between bodies I have a function for applying gravity forces between every possible pair of bodies on my game. It is the most used function, and can run more than 100k times per frame so every minor improvement on performance will make a HUGE difference here.

I replaced some divisions by multiplications and the general FPS increased from 5fps to 20fps with ~1000 bodies. This is how performance is affected by this function.

Precision can be decreased if it increases performance considerably. Just make sure you make it clear that a change will decrease precision in your review.

applyGravityBetween(bodyA, bodyB, collisionCallback) {
    var distX = bodyB.x - bodyA.x,
        distY = bodyB.y - bodyA.y,
        distSqr = distX * distX + distY * distY,
        forceA, forceB, dist;

    if (distSqr > (bodyA.radius + bodyB.radius) * (bodyA.radius + bodyB.radius)) {

        // ALERT: Radical actions were taken here to make faster code. Division was avoided at max.

        dist = 1 / Math.sqrt(distSqr); // Dividing one by the distance allows us to multiply instead of dividing later when setting actual velocities, which is more performant.
        forceA = bodyB.mass * dist * dist;
        forceB = bodyA.mass * dist * dist; // Instead of dividing by `distSqr` we can multiply by `dist` twice.

        bodyA.vx += forceA * distX * dist;
        bodyA.vy += forceA * distY * dist;

        bodyB.vx -= forceB * distX * dist;
        bodyB.vy -= forceB * distY * dist;
    } else if (typeof collisionCallback === "function" && bodyA.collidable && bodyB.collidable) collisionCallback(bodyA, bodyB);
}

EDIT:

Here's a benchmark for each separate piece from this function, so you can focus on what you're going to improve:

var declarations took 3.199ms on average to run. Collision check took 3.342ms. Math.sqrt() took 3.122ms.

This question is also related to this one, so if you're interested you can go there too and... Review.

Forward note: To benchmark I simply commented out //self.addCollision(bodyA, bodyB); in step(). Referencing your github code.

To optimize just this method without looking at anything else we can do the
following. This gives a physics workload at ~22ms compared with the original ~27ms on my system.

applyGravityBetween(bodyA, bodyB, collisionCallback) {

    var dx = bodyB.x - bodyA.x,
    dy = bodyB.y - bodyA.y,
    r  = (bodyA.radius + bodyB.radius);

    if ( ( ( dx > r || -dx > r ) || ( dy > r || -dy > r ) ) || ( dx * dx + dy * dy > r * r ) ) {

            r = Math.sqrt(dx * dx + dy * dy);

            if( bodyB.mass == bodyA.mass ){

                r = bodyA.mass / (r * r * r);

                bodyA.vx += r * dx;
                bodyB.vx -= r * dx;

                bodyB.vy -= r * dy;
                bodyA.vy += r * dy;

            }else{

                r = 1 / (r * r * r);

                bodyA.vx += bodyB.mass * r * dx;
                bodyB.vx -= bodyA.mass * r * dx;

                bodyB.vy -= bodyA.mass * r * dy;
                bodyA.vy += bodyB.mass * r * dy;
            }

    } else if( typeof collisionCallback === "function") collisionCallback(bodyA, bodyB);

}

The above code improves by doing the following.

simple bounding box collision check before using a more intensive distance check
remove setting of variables as much as possible, i found in practice this is worth 2 or 3 multiplications in terms of cost. sometimes it's better to repeat the calculations.
Use a comparison of the masses to speed up similar calculations - this is slightly faster where everything starts off with the same mass and could be useful for high N and similar mass simulations, otherwise I would take it out.

Areas for improvement;

This is not coded up but it is common in n-body simulations to group and approximate close,similar & far away masses by grouping them together as one. This can really help as N gets larger because it grows really fast.

2. Reduce Callbacks.

You only asked to optimise the gravity function so this is just an extra.. If you put the following directly into your step() method you can instantly double the speed of the simulation. I found a physics time of 8.8 ms on average - Incorporating all improvements that's almost a third of the original time.

    var i1, i2, bodyA, bodyB, dx, dy, r;

    for (i1 = 0; i1 < this.dynamicBodies.length; ++i1) {
        bodyA = this.dynamicBodies[i1];

        for (i2 = i1 + 1; i2 < this.dynamicBodies.length; ++i2) {
            bodyB = this.dynamicBodies[i2];

            dx = bodyB.x - bodyA.x;
            dy = bodyB.y - bodyA.y;
            r  = (bodyA.radius + bodyB.radius);

            if ( ( ( dx > r || -dx > r ) || ( dy > r || -dy > r ) ) || ( dx * dx + dy * dy > r * r ) ) {

                    r = Math.sqrt(dx * dx + dy * dy);

                    if( bodyB.mass == bodyA.mass ){

                        bodyA.vx += (r = bodyA.mass / (r * r * r)) * dx;
                        bodyB.vx -= r * dx;

                        bodyB.vy -= r * dy;
                        bodyA.vy += r * dy;

                    }else{

                        bodyA.vx += bodyB.mass * (r = 1 / (r * r * r)) * dx;
                        bodyB.vx -= bodyA.mass * r * dx;

                        bodyB.vy -= bodyA.mass * r * dy;
                        bodyA.vy += bodyB.mass * r * dy;
                    }

            } else {
                //this.addCollision(bodyA, bodyB);
            }

        }
    }


Encryption via multiplication of hex-converted, multiplied ASCII values The idea behind the encryption is that it stores the key within the final encrypted password. When the user inputs a password for the first time, they also have to pick an element from a list of elements, and that determines where in the final, encrypted password the key is stored.

This is crucial for decryption.

The algorithm proceeds as follows:

User inputs a password.

The password is split into the individual characters, then stored in a list.

User inputs a key. (The key corresponds to an element in a list in the program, either 1, 2 or 3)
Each character in the list is translated into its ASCII number, and the original list is overwritten.
Each ASCII number is then multiplied by a random number.

The random number is stored as the variable x.

Each multiplied number is then converted into hex.
Random hex values are added to either side of the already encrypted password.
The element of the list that corresponds to the key (random_key) is replaced by the hex value of x.
The final encrypted password is stored in a file.

Note: Decryption is possible, but, I'm still working on it.

import random
import getpass

def joinit(iterable, delimiter):
    it = iter(iterable)
    yield next(it)
    for x in it:
        yield delimiter
        yield x

password = getpass.getpass('Please input a password.\n')
lst = []
key = input('Please pick a list number. Write it down!!')
random_key = [1,2,3,4,1,2,3,4,1,2,3,4,1,2,3,4]
for i in range(0, len(password)-1):
    lst.append(ord(password[i]))
print lst
x = random.randint(1,1000)
for i in range(0, len(password)-1):
    lst[i] *= x
for i,z in enumerate(lst):
        lst[i] = hex(z)
print lst
i = 2
lst2 = []

for i in range(0,4):
    hexhex = random.randint(1000,2000)
    hex(hexhex)
    lst.insert(0,hex(hexhex))
    print hexhex
for i in range(0,4):
    hexhex = random.randint(1000,2000)
    hex(hexhex)
    lst.append(hex(hexhex))
    print hexhex
lst[key] = hex(x)
result = []
for e in lst:
    result.append(e)
    result.append('|')
result.pop()

penultimate = ''.join(str(v) for v in result)
print result
print '\n'*3
print penultimate
file = '/home/vhx/Documents/code/tkinter/encryption/password.txt'
target = open(file, 'w')
target.truncate()
target.write(penultimate)

I designed this encryption method for a login system that encrypts the inputted password before comparing it to the one stored in the file. This way the password is never decrypted, and thus cannot (hopefully) be stolen.

I have a few questions:

Can the code be made any more efficient? Is there anything in there that can be simplified or removed altogether?
Is the idea itself sound (security wise)?
Has something like this been tried before?

I don't do security.
The information is provided as if this were not a security problem.

First things first. Python's random number generation is pseudo-random.
The source code is really small, and consisted of iirc 4 %.

If we look at the docs for what this may mean on your security.

  Warning: The pseudo-random generators of this module should not be used for security purposes.
  Use os.urandom() or SystemRandom if you require a cryptographically secure pseudo-random number generator.

That instantly destroys all the security of this security program.

Style

PEP8

There should be two blank lines either side of top level functions or classes.
You need white space after commas. [1, 2].
random_key = [1,2,3,4,1,2,3,4,1,2,3,4,1,2,3,4]
You should have one space either side of operators. 2 * 2.
The exception to this is so show precedence. 2*2 + 2.
'\n'*3

General style

You should use better variable names. x is not descriptive. lst and lst2 don't 'say' what they do.

When using range 0 is the default value for start. range(0, len(password)-1) == range(len(password)-1).

You should not have all your code in the top level of the program. As a minimum you should wrap it in a if __name__=='__main__':. And then you should have it so that it calls main().

Code

There are two features of the language that you should really like.

1. List comprehensions.

lst = []
for i in range(0, len(password)-1):
    lst.append(ord(password[i]))

Using a list comprehension, you can simplify this.

lst = [
    ord(password[i])
    for i in range(len(password)-1):
]

You can then just slice the string.

lst = [ord(char) for char in password[:-1]]

2. str.join:

result = []
for e in lst:
    result.append(e)
    result.append('|')
result.pop()

This can simply be reduced to

result = '|'.join(str(char) for char in lst)

Conclusion

If we simplify everything, and remove the prints, we get the following:

def get_hexes():
    return [hex(random.randint(1000,2000)) for _ in range(4)]

password = getpass.getpass('Please input a password.\n')
key = input('Please pick a list number. Write it down!!')
x = random.randint(1,1000)

lst = get_hexes() + [hex(ord(char) * x) for char in password[:-1]] + get_hexes()
lst[key] = hex(x)
result = '|'.join(str(char) for char in lst)

path = '/home/vhx/Documents/code/tkinter/encryption/password.txt'
with open(file, 'w') as target:
    target.truncate()
    target.write(result)

From this, we can tell easily, that it's not that secure.
You know that the | and the first and last 4 sections are just red herrings.
This is as they are always just there and we can ignore them.
And there is a lot more noise than needed, 0x.

And, the password will always have a character missing, and can have another missing.

I think the best example of a good password and list number is aa and 5. This is as then my password will be any two letters. All I need to remember is 5.

Also, you can tell when the key is in the first or last 4 sections, this is as the minimum for them are 1000, where the maximum for the key is 1000.

If you want better feedback on your algorithm, go to the cryptography site.


Changing images when hovering over links I wrote a script to change a static image by hovering over navigation links. It works great, however, the script is very long and I'm guessing there is a much more efficient way to write this code. Can anyone give me tips on a better way to do this?

HTML

Link 1

Link 2

Link 3

JQuery

$(document).ready(function() {
$('#navigation li a#1').hover(
    function(){
        $(this).addClass('hover');
        $('.box img').attr('src', 'image1.jpg');
    },
    function(){
        $(this).removeClass('hover');
        $('.box img').attr('src', 'default.jpg');
    }
);
$('#navigation li a#2').hover(
    function(){
        $(this).addClass('hover');
        $('.box img').attr('src', 'image2.jpg');
    },
    function(){
        $(this).removeClass('hover');
        $('.box img').attr('src', 'default.jpg');
    }
);
$('#navigation li a#3').hover(
    function(){
        $(this).addClass('hover');
        $('.box img').attr('src', 'image3.jpg');
    },
    function(){
        $(this).removeClass('hover');
        $('.box img').attr('src', 'default.jpg');
    }
);
});

Refactor the function from the data. Then you can just bind the same function to all of the links.

html

Link 1

js

$('#navigation li a').hover(
    function(){
        $(this).addClass('hover');
        $('.box img').attr('src', $(this).data('img');
    },
    function(){
        $(this).removeClass('hover');
        $('.box img').attr('src', 'default.jpg');
    }
);


Finding cells in a 3D grid with free neighbors I have a program, where the get_free function below has the most major computational cost by far, and I was wondering if there was a faster way to do it. Perhaps an all-numpy solution would be faster.

The task is to find the indices of the third dimension in a three-dimensional boolean array where the cell (as specified by the two first dimensions) and its neighbors (as given by the neighbors2 function) are 0. That's what get_free does.

Don't worry too much about understanding the neighbors2 function, it doesn't do much heavy lifting. The indices can be returned in two formats, as specified by the docstring, which can be helpful for some numpy-functions.

Any feedback, regarding code quality in general or for optimizing performance is appreciated.

import numpy as np
import timeit

rows, cols = 7, 7
depth = 70
state = np.random.choice([0, 1], size=(rows, cols, depth)).astype(bool)

def neighbors2(row, col, separate=False):
    """
    If 'separate' is True, return ([r1, r2, ...], [c1, c2, ...])
    if not, return [(r1, c1), (r2, c2), ...]

    Returns a list of indices of neighbors (in an hexogonal grid)
    within a radius of 2, not including self.
    """
    if separate:
        rs = []
        cs = []
    else:
        idxs = []

    r_low = max(0, row-2)
    r_hi = min(rows-1, row+2)
    c_low = max(0, col-2)
    c_hi = min(cols-1, col+2)
    if col % 2 == 0:
        cross1 = row-2
        cross2 = row+2
    else:
        cross1 = row+2
        cross2 = row-2
    for r in range(r_low, r_hi+1):
        for c in range(c_low, c_hi+1):
            if not ((r, c) == (row, col) or
                    (r, c) == (cross1, col-2) or
                    (r, c) == (cross1, col-1) or
                    (r, c) == (cross1, col+1) or
                    (r, c) == (cross1, col+2) or
                    (r, c) == (cross2, col-2) or
                    (r, c) == (cross2, col+2)):
                if separate:
                    rs.append(r)
                    cs.append(c)
                else:
                    idxs.append((r, c))
    if separate:
        return (rs, cs)
    else:
        return idxs

def get_free(cell):
    """
    Return the indices of a a cell that are 0 and
    where all its neighbors are 0 for the same depth
    """
    candidates = np.where(state[cell] == 0)[0]
    neighs = neighbors2(*cell, False)
    free = []
    # Exclude elements that have non-zero value in neighboring cells
    for candidate in candidates:
        non_zero = False
        for neigh in neighs:
            if state[neigh][candidate]:
                non_zero = True
                break
        if not non_zero:
            free.append(candidate)

print(timeit.timeit("get_free((4, 4))", number=100000,
      setup="from __main__ import get_free"))

It seems like your neighbors2 is a pure function. I think you could speed things up by memoizing the results:

def neighbors2(row, col, separate=False):
    global _neighbors2_memoized

    args = (row, col, separate)

    if not _neighbors2_memoized:
        _neighbors2_memoized = { } # Start with empty dict
    elif row_col_sep in _neighbors2_memoized:
        return _neighbors2_memoized[args]

    # rest of your code

    if separate:
        result = (rs, cs)
    else:
        result = idxs

    _neighbors2_memoized[args] = result
    return result

Of course, a little searching will yield hundreds of memoization decorators and the like for python.

Update

Also, I think what you are doing is looking for "layers" or "levels" at which the given cell has all its neighbors empty.

If so, consider recoding your check to use intersect1d(). The neighbors are going to be constant - it's the same neighbors at each level.

So make a similar candidates array for each neighbor, and treat it like a set: "this is the set of levels where neighbor (ij) is zero".

Then, compute the intersection of that set with the candidates set (which is just the set of levels where cell is zero).

This is basically a large and: cell is zero AND neighbor(ij) is zero AND ...

You can execute len(neighbors) numpy operations instead of looping in python.

def get_free_setwise(cell):
    """
    Return the indices of a a cell that are 0 and
    where all its neighbors are 0 for the same depth
    """
    neighbors = neighbors2(*cell, False)

    candidates = np.where(state[cell] == 0)[0]

    for n in neighbors:
        candidates = np.intersect1d(candidates, np.where(state[n] == 0)[0])
        if len(candidates) == 0:
            break

    return candidates

print(timeit.timeit("get_free((4, 4))", number=10000,
      setup="from __main__ import get_free"))
print(timeit.timeit("get_free_setwise((4, 4))", number=10000,
      setup="from __main__ import get_free_setwise"))

Output of this was (note: number=10,000):

0.8349439720041119
6.3053770850092405

About seven times slower. However, when I raised depth to 700, the times were almost equal, and when I raised depth to 7000, the times were

3.6607714939891594
2.716483567011892

So the numpy-centric version does better as depth increases (FWIW).

Update 2

Using bitwise ops I was able to get better performance:

# Neighborhood pre-computation for get_free5. This is a one-time thing - the
# neighborhoods will never change as long as the rows,cols stay the same.
# NOTE however that these arrays are TRANSPOSED, because that's what free5
# does.

neighborhood = []
for r in range(rows):
    neighborhood.append([])
    for c in range(cols):
        hood = np.zeros((rows, cols), dtype=np.bool_)
        hood[r,c] = 1
        for nb in neighbors2(r, c):
            hood[nb] = 1
        hood = hood.T # Transpose!
        # Now hood is all 1's for neighbors. Do this same computation
        # in the same fashion that the state-updater does.
        bytestr = np.packbits(hood).tobytes() + b'\0'
        uint = np.frombuffer(bytestr, dtype=np.uint64)
        neighborhood[r].append(uint[0])

neighborhood = np.array(neighborhood, dtype=np.uint64)
# Neighborhood[r,c] is a bitmask with all neighbors + (r,c) set to 1
# NOTE: +(r,c) - the neighborhood explicitly includes the center point,
# which is not true for the neighbors function. I am using this to test for
# the center ALSO being zero at the same time - just one pass through the
# state array.

def get_free5(cell, *, verbose=False):
    """
    Return the indices of a a cell that are 0 and
    where all its neighbors are 0 for the same depth
    """
    result = list(np.nonzero(state_asbits & neighborhood[cell] == 0)[0])

    # This if statement is used in my self-test code. Delete all this code
    # for production use.
    if verbose:
        orig = get_free(cell)
        missing = [n for n in orig if n not in result]
        extra = [n for n in result if n not in orig]
        print("Missing from result, found in orig:")
        for n in missing:
            print("orig: (transposed)", n)
            print(etats[n])
            print("bits: {:64b}".format(state_asbits[n]))
            print("mask: {:64b}".format(neighborhood[cell]))

        print("Found in result, not found in orig:")
        for n in extra:
            print("orig: (transposed)", n)
            print(etats[n])
            print("bits: {:64b}".format(state_asbits[n]))
            print("mask: {:64b}".format(neighborhood[cell]))

    return result

# Setup for get_free5(). This code has to be run whenever the state[]
# array changes or is computed. It's an alternate encoding of the state array
# so if you don't need state[] for anything except this check, you can get
# rid of it. Otherwise, you'll have to call this before you start calling
# get_free5()

# Transpose, to get all the values at the same depth in a contiguous area.
etats = state.T
bitplane = []
for d in range(state.shape[-1]):
    #bytestr = np.packbits(etats[d]).tobytes()
    #if len(bytestr) % 8 != 0:
    #    bytestr += b'\00' * (8 - len(bytestr) % 8)

    # Hard-coding for 7x7 - append one extra byte
    bytestr = np.packbits(etats[d]).tobytes() + b'\0'
    uint = np.frombuffer(bytestr, dtype=np.uint64)
    bitplane.append(uint[0])

# state_asbits[n] is an array of 64-bit numbers, holding bits for each of the
# 49 cells at depth=n. If the value in state[r,c,n] is 1, then the bit at
# state_asbits[n].get_bit(r,c) == 1. These values can be trivially checked
# using bitwise AND operations, with the mask values in the neighbors array.
state_asbits = np.array(bitplane)

This code runs faster than the original version, but that doesn't include the computations I have to do for the state_asbits array. I think that's pretty cheap, but I don't know how often you are updating it.

$ python test.py
original code 8.314752619000501
free5: bitwise 1.1757051760068862


RPN calculator in Chicken Scheme My first attempt at Scheme (Chicken Scheme) - a simple RPN calculator. Please comment on exception handling as well as the general coding style. Here is the code:

(use stack)
(use srfi-69)
(use simple-exceptions)

(define prompt "> ")

(define stack (make-stack))
(define words (make-hash-table))

(define (init-words)
  (hash-table-set! words "bye" '(quit))
  (hash-table-set! words ".s" '(stack-print))
  (hash-table-set! words "+" '(add))
  (hash-table-set! words "-" '(subtract))
  (hash-table-set! words "*" '(multiply))
  (hash-table-set! words "/" '(divide))
  (hash-table-set! words "." '(pop-print))
  (hash-table-set! words "dup" '(dup))
  (hash-table-set! words "drop" '(drop))
  (hash-table-set! words "swap" '(swap)))

(define (stack-print)
  (display (stack->list stack))
  (newline))

(define (pop-print)
  (display (stack-pop! stack))
  (newline))

(define (add)
  (stack-push! stack
           (+ (stack-pop! stack)
          (stack-pop! stack))))

(define (subtract)
  (swap)
  (stack-push! stack
           (- (stack-pop! stack)
                  (stack-pop! stack))))

(define (multiply)
  (stack-push! stack
           (* (stack-pop! stack)
          (stack-pop! stack))))

(define (divide)
  (swap)
  (stack-push! stack
           (/ (stack-pop! stack)
          (stack-pop! stack))))

(define (dup)
  (let ((first (stack-pop! stack)))
    (stack-push! stack first)
    (stack-push! stack first)))

(define (drop)
  (stack-pop! stack))

(define (swap)
  (let ((first (stack-pop! stack))
    (second (stack-pop! stack)))
    (stack-push! stack first)
    (stack-push! stack second)))

(define (f-read)
  (display prompt)
  (flush-output)
  (read-line))

(define (f-parse line)
  (string-split line))

(define (process word)
  (cond
   ((hash-table-exists? words word)
    (eval (hash-table-ref words word)))
   ((string->number word)
    (stack-push! stack (string->number word)))))

(define (f-eval words)
  (cond
   ((null? words) "")
   (else (process (car words))
     (f-eval (cdr words)))))

(define (f-print line)
  (display line))

;; user-interrupt ?

(define (repl)
  (guard
   (exn (((exception-of? 'user-interrupt) exn)
     (quit))
    ((exception? exn)
     (display (message exn))
     (newline)
     (repl))
    (else
     (display (arguments exn))
     (newline)
     (repl)))
   (init-words)
   (f-print (f-eval (f-parse (f-read))))
   (repl)))

You've actually run into a big gotcha here. Scheme doesn't actually specify the order to evaluate operands. This can be a problem for code with side effects, especially if you aren't careful to encapsulate the order you expect explicitly. Any time you see an exclamation mark at the end of the function, pay attention to ensure a correct and defined semantic order.

(define (pop-print)
  (display (stack-pop! stack))
  (newline))

Something like this is fine, as (define ...) creates an implicit begin in the syntax. (define (x a) (zig a) (zag b)) gets transformed into something closer to (let ((x (lambda (a) (begin (zig a) (zig b)). cond has a similar effect.

However ...

(define (add)
  (stack-push! stack
           (+ (stack-pop! stack)
          (stack-pop! stack))))

is also fine, because order doesn't matter, but we aren't sure which pop will get its number first. Divide and subtract may randomly behave incorrectly, especially on different Scheme implementations.

Your swap is so close though, but to be right it would have needed to use a begin and a let* instead of the normal let. The Star version behaves as if you explicitly nested each sequential binding, whereas let is allowed to make bindings in any order. begin likewise forces text-order sequential evaluation.

(define (swap)
  (let* ((first (stack-pop! stack))
         (second (stack-pop! stack)))
    (begin (stack-push! stack first)
           (stack-push! stack second))))

Here's a method of forcing order via nested lambda notation, as every time you invoke a function, you're creating a new binding environment.

(define (two-args f) ;takes a function as an argument
 (lambda () ;explicitly return a function of zero arguments
   ((lambda (x)
            ((lambda (y)
                     (stack-push! stack (f y x)))
                                  ; the deeper term has priority
             (stack-pop! stack))
    (stack-pop! stack))

And then define the functions in terms of above

(define add      (two-args +))
(define subtract (two-args -))
(define multiply (two-args *))
(define divide   (two-args /))

And a little trickier

(define swap
  (two-args (lambda (y x)(begin (stack-push! stack x)
                                y))

Why are you initializing the words hash table every time though the repl? You could initialize it at creation like so...

(define words
  (let ((temp (make-hash-table))
    (begin
      (hash-table-set! temp "bye" '(quit))
      ...
      temp)))

As for the exception handler, I can't comment as I don't know that much about them in Scheme.


The user inputs are the gross income and number of dependents. The program calculates the income tax based on the inputs and the tax law I'm learning the ropes of Java. So far everything is going good. Can you please review my code and tell me what I can do to make it cleaner - or what I need to improve on?. I'll appreciate it so much.
This program:
The user inputs are the gross income and number of dependents. The program calculates the income tax based on the inputs and the tax law and then displays the income tax.
Here's a screen shot of the code in the IDE and a flowchart.
(IDE)

(Flowchart)

import java.util.*;
public class lab4 {
    public static void main(String args[]) {
        Scanner input = new Scanner(System. in);

        System.out.println("Enter gross income: ");
        double grossIncome;
        grossIncome = input.nextDouble();

        System.out.println("Enter the number of dependents: ");
        double dependents;
        dependents = input.nextDouble();

        final double TAX_RATE = 0.2;
        final double STANDARD_DEDUCTION = 10000.00;
        final double DEPENDENT_DEDUCTION = 2000.0;
        final double grossIncomeafterdeduction = grossIncome - (STANDARD_DEDUCTION + DEPENDENT_DEDUCTION * dependents);

        double incomeTax;
        incomeTax = grossIncomeafterdeduction * TAX_RATE;

        double grossincometax;
        grossincometax = grossIncome - incomeTax;

        System.out.println("The income tax is $ " + incomeTax);

        System.out.println("The gross income after tax $ " + grossIncometax);
    }
}

Only import what you need from java util not the whole package.
Move constants to member level and make static, or pass in these parameters to your program.
Use a money package for calculations involving money.


User register script PHP So, I needed to make me a register script for my website so pepole can register, and just wanted to know if It's okey.
So, is there any vulns in this script i made?It works perfectly, just that i wanted to have someone to look over it here.
Or if there could be some inprovments that could make it better.

 <?php
        $Username = $_POST['Username'];
        $Password = $_POST['Password'];
        $Email    = $_POST['Email'];
        $IP       = $_SERVER['REMOTE_ADDR'];
        if ($IP != "84.***.***.**") { die(); } // Beta, Restrict users from using it until it's done
        /* Random Activate code generator */
        function GenerateCode() {
            $R1 = rand(1,9);
            $R2 = rand(1,9);
            $R3 = rand(1,9);
            $R4 = rand(1,9);
            $R5 = rand(1,9);
            $R6 = rand(1,9);
            $C1 = "$R1$R2$R3$R4$R5$R6"; // Simple complie method
            return $C1;
        }
        function Encrypt($Pass) {
            $B64 = base64_encode($Pass);
            $MD5 = md5($B64);
            $Hash = base64_encode($MD5);
            return $Hash;
        }
        /* Mysql data */
        $MysqlUsername = "root";
        $MysqlPassword = "**********";
        $MysqlHostname = "localhost";
        $MysqlDatabase = "teamgamersnet";

        $Sql = new mysqli($MysqlHostname, $MysqlUsername, $MysqlPassword, $MysqlDatabase);
        if ($Sql->connect_error){ echo $Sql->connect_error; }
        $sUser = $Sql->real_escape_string($Username);
        $sPass = $Sql->real_escape_string($Password);
        $sMail = $Sql->real_escape_string($Email);
        $xPass = $Sql->real_escape_string(Encrypt($Password));
        if (!$Username | !$Password | $Email) { die("Please fill in all the fields"); }
        $CheckUser = $Sql->query("SELECT Username FROM `users` WHERE `Username` = '".$sUser."'");
        $CheckMail = $Sql->query("SELECT `E-Mail` FROM `users` WHERE `E-Mail` = '".$sMail."'");
        if ($CheckUser->num_rows == 1) { die("Username allready exist"); }
        if ($CheckMail->num_rows == 1) { die("E-Mail allready exist"); }
        $xCode = GenerateCode();
        $Insert = "
        INSERT INTO `teamgamersnet`.`users` (`ID`, `Username`, `Password`, `E-Mail`, `IP`, `Activated`, `ActivateId`)
        VALUES (NULL, '".$sUser."', '".$xPass."', '".$sMail."', '".$IP."', 'false', '".$xCode."');
        ";
        $Create = $Sql->query($Insert);
        if (!$Create) { echo "S**t, a error happened D:";}
        $to = $Email;
        $subject = 'User account activation | Team Gamers Net';
        $message = '

Thank you for registering at Team Gamers Net

Just one more step and that is to activate your account

You registred with username '.$Username.'

And your password you selected (not shown)

To activate and contiune, please click HERE

        ';
        $headers = "From: DoNotReply@TeamGamers.Net \r\n";
        $headers .= "Reply-To: Admin@heisteknikk.com \r\n";
        $headers .= "MIME-Version: 1.0\r\n";
        $headers .= "Content-Type: text/html; charset=ISO-8859-1\r\n";
        mail($to, $subject, $message, $headers);
        header("Location: /?register=true");
    ?>

Ok, there are a couple of things wrong with your code and your method of organizing the code. I've pointed them out to you in the code below within comments. The comments preceded with # are my comments.

<?php

/* Mysql data */
$MysqlUsername = "root";
$MysqlPassword = "**********";
$MysqlHostname = "localhost";
$MysqlDatabase = "teamgamersnet";

$Username = $_POST['Username'];
$Password = $_POST['Password'];
$Email    = $_POST['Email'];

# Check for empty variables
if (empty($Username) || empty($Password) || empty($Email)) { die("Please fill in all the fields"); }

# We're only using $IP once, so why set it above? Let's do a direct comparison here
// Beta, Restrict users from using it until it's done
#OLD::
#if ($IP != "84.***.***.***") { die(); }

#NEW::
if ($_SERVER['REMOTE_ADDR'] != "84.***.***.**") { die(); }

/* Random Activate code generator */
function GenerateCode() {
    # You're setting 7 variables with like variables, and returning them
    #OLD::
    #$R1 = rand(1,9);
    #$R2 = rand(1,9);
    #$R3 = rand(1,9);
    #$R4 = rand(1,9);
    #$R5 = rand(1,9);
    #$R6 = rand(1,9);
    #$C1 = "$R1$R2$R3$R4$R5$R6";
    #return $C1;

    #NEW::
    return rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9);
}

/* Encrypt the password */
function Encrypt($Pass) {
    # Again, setting multiple variables in the name of readability over performance
    #OLD::
    #$B64 = base64_encode($Pass);
    #$MD5 = md5($B64);
    #$Hash = base64_encode($MD5);
    #return $Hash;

    #NEW::
    return base64_encode( md5( base64_encode($Pass) ) );
}

# Let's move the Mysql Connection Data to the top, so its easier to modify in the future
#OLD::
/* Mysql data */
#$MysqlUsername = "root";
#$MysqlPassword = "**********";
#$MysqlHostname = "localhost";
#$MysqlDatabase = "teamgamersnet";

#NEW::
# Moved to top

$Sql = new mysqli($MysqlHostname, $MysqlUsername, $MysqlPassword, $MysqlDatabase);
if ($Sql->connect_error){ echo $Sql->connect_error; }

/* Let's add a comment here: Escape the user data */
$sUser = $Sql->real_escape_string($Username);
$sPass = $Sql->real_escape_string($Password);
$sMail = $Sql->real_escape_string($Email);
$xPass = $Sql->real_escape_string(Encrypt($Password));

# Let's move this to the top, right after these are set from _POST, to avoid all the
# prior computation before running into this. Also, use empty() or isset() instead of ! checking
# See: http://www.phpbench.com/
#OLD::
#if (!$Username | !$Password | $Email) { die("Please fill in all the fields"); }

#NEW::
# Moved to top, also - syntax errors fixed: it's || not |

/* Let's add a comment here: Check if the user exists */
$CheckUser = $Sql->query("SELECT Username FROM `users` WHERE `Username` = '".$sUser."'");
$CheckMail = $Sql->query("SELECT `E-Mail` FROM `users` WHERE `E-Mail` = '".$sMail."'");
if ($CheckUser->num_rows == 1) { die("Username allready exist"); }
if ($CheckMail->num_rows == 1) { die("E-Mail allready exist"); }

/* Let's add a comment here: Generate an activation code */
$xCode = GenerateCode();

/* Let's add a comment here: Write out the Insert statement  */
$Insert = "INSERT INTO `teamgamersnet`.`users` (`ID`, `Username`, `Password`, `E-Mail`, `IP`, `Activated`, `ActivateId`)
VALUES (NULL, '".$sUser."', '".$xPass."', '".$sMail."', '".$IP."', 'false', '".$xCode."');";

/* Let's add a comment here: Insert the SQL statement */
$Create = $Sql->query($Insert);
# Why are we echoing? Shouldn't we be dying?
#OLD::
#if (!$Create) { echo "S**t, a error happened D:";}

#NEW::
if ( empty($Create) || !isset($Create) ) { die("Sorry, an error occurred during Create."); }

/* Let's add a comment here: Build the message, and send it to the user. */
$to = $Email;
$subject = 'User account activation | Team Gamers Net';
$message = '

Thank you for registering at Team Gamers Net

Just one more step and that is to activate your account

You registred with username '.$Username.'

And your password you selected (not shown)

To activate and contiune, please click HERE

';
$headers = "From: DoNotReply@TeamGamers.Net \r\n";
$headers .= "Reply-To: Admin@heisteknikk.com \r\n";
$headers .= "MIME-Version: 1.0\r\n";
$headers .= "Content-Type: text/html; charset=ISO-8859-1\r\n";
mail($to, $subject, $message, $headers);

header("Location: /?register=true");
?>

Here's the same code, with my comments removed:

<?php

/* Mysql connection data */
$MysqlUsername = "root";
$MysqlPassword = "**********";
$MysqlHostname = "localhost";
$MysqlDatabase = "teamgamersnet";

$Username = $_POST['Username'];
$Password = $_POST['Password'];
$Email    = $_POST['Email'];

/* Check for empty variables */
if (empty($Username) || empty($Password) || empty($Email)) { die("Please fill in all the fields"); }

/* Beta, Restrict users from using it until it's done */
if ($_SERVER['REMOTE_ADDR'] != "84.***.***.**") { die(); }

/* Random Activate code generator */
function GenerateCode() {
    return rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9);
}

/* Encrypt the password */
function Encrypt($Pass) { return base64_encode( md5( base64_encode($Pass) ) ); }

$Sql = new mysqli($MysqlHostname, $MysqlUsername, $MysqlPassword, $MysqlDatabase);
if ($Sql->connect_error){ echo $Sql->connect_error; }

/* Escape the user data */
$sUser = $Sql->real_escape_string($Username);
$sPass = $Sql->real_escape_string($Password);
$sMail = $Sql->real_escape_string($Email);
$xPass = $Sql->real_escape_string(Encrypt($Password));

/* Check if the user exists */
$CheckUser = $Sql->query("SELECT Username FROM `users` WHERE `Username` = '".$sUser."'");
$CheckMail = $Sql->query("SELECT `E-Mail` FROM `users` WHERE `E-Mail` = '".$sMail."'");
if ($CheckUser->num_rows == 1) { die("Username allready exist"); }
if ($CheckMail->num_rows == 1) { die("E-Mail allready exist"); }

/* Generate an activation code */
$xCode = GenerateCode();

/* Write out the Insert statement  */
$Insert = "INSERT INTO `teamgamersnet`.`users` (`ID`, `Username`, `Password`, `E-Mail`, `IP`, `Activated`, `ActivateId`)
VALUES (NULL, '".$sUser."', '".$xPass."', '".$sMail."', '".$IP."', 'false', '".$xCode."');";

/* Insert the SQL statement */
$Create = $Sql->query($Insert);

/* Check if SQL insert went well, otherwise die */
if ( empty($Create) || !isset($Create) ) { die("Sorry, an error occurred during Create."); }

/* Build the message, and send it to the user. */
$to = $Email;
$subject = 'User account activation | Team Gamers Net';
$message = '

Thank you for registering at Team Gamers Net

Just one more step and that is to activate your account

You registred with username '.$Username.'

And your password you selected (not shown)

To activate and contiune, please click HERE

';
$headers = "From: DoNotReply@TeamGamers.Net \r\n";
$headers .= "Reply-To: Admin@heisteknikk.com \r\n";
$headers .= "MIME-Version: 1.0\r\n";
$headers .= "Content-Type: text/html; charset=ISO-8859-1\r\n";
mail($to, $subject, $message, $headers);

header("Location: /?register=true");
?>


Simple image gallery I have a simple gallery that hides and shows images. It works fine, however, I am not satisfied with my approach, and my code seems redundant. Can you check my code and give a better idea on how I can improve it?

HTML

                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/big_img_1.JPG" id="big_img_1" class="big_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/big_img_2.JPG" id="big_img_2" class="big_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/big_img_3.JPG" id="big_img_3" class="big_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/big_img_4.JPG" id="big_img_4" class="big_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/big_img_5.JPG" id="big_img_5" class="big_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/big_img_6.JPG" id="big_img_6" class="big_img">

                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_1.jpg" id="thumbs_img_1" calss="thumbs_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_2.jpg" id="thumbs_img_2" calss="thumbs_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_3.jpg" id="thumbs_img_3" calss="thumbs_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_4.jpg" id="thumbs_img_4" calss="thumbs_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_5.jpg" id="thumbs_img_5" calss="thumbs_img">
                <img src="<?= IMAGEPATH ?>front_page/phinfo/house_rentals/westgate/thumbnails/thumbs_img_6.jpg" id="thumbs_img_6" calss="thumbs_img">

CSS

.big_img_wrapper, .big_img_wrapper img{
        width: 370px;
        height: 246px;
        /*display: none;*/
    }
    .thumbs_img_wrapper{
        padding:0;
    }
    .thumbs_img_wrapper img{
        width: 111px;
        height: 70px;
        margin: 14px 0 0 14px;
    }
    #thumbs_img_1, #thumbs_img_4{
        margin: 14px 0 0 0;
    }

jQuery

$(document).ready(function(){

    $('img.big_img').hide(); // Hides all big images
    $('img#big_img_1').fadeIn('slow'); // Serve as default image

    $('img#thumbs_img_1').click(function(){
        $('img.big_img').hide(); // Hides all big images
        $('img#big_img_1').fadeIn('slow'); //Slowly fades in selected image
    });

    $('img#thumbs_img_2').click(function(){
        $('img.big_img').hide(); // Hides all big images
        $('img#big_img_2').fadeIn('slow'); //Slowly fades in selected image
    });

    $('img#thumbs_img_3').click(function(){
        $('img.big_img').hide(); // Hides all big images
        $('img#big_img_3').fadeIn('slow'); //Slowly fades in selected image
    });
    $('img#thumbs_img_4').click(function(){
        $('img.big_img').hide(); // Hides all big images
        $('img#big_img_4').fadeIn('slow'); //Slowly fades in selected image
    });
    $('img#thumbs_img_5').click(function(){
        $('img.big_img').hide(); // Hides all big images
        $('img#big_img_5').fadeIn('slow'); //Slowly fades in selected image
    });
    $('img#thumbs_img_6').click(function(){
        $('img.big_img').hide(); // Hides all big images
        $('img#big_img_6').fadeIn('slow'); //Slowly fades in selected image
    });
});

replace all .click() functions with:

$('img.thumbs_img').click(function(){
        $('img.big_img').hide(); // Hides all big images
        var id = $(this).attr('id');
        id = id.replace("thumbs_img_", "big_img_");
        $('img#'+id).fadeIn('slow'); //Slowly fades in selected image
    });


Trace-like extensions for unit testing Below is a set of extensions that I use in unit tests.

The basic idea is that when creating or modifying some sort of user output type of string, I want to eyeball the output to see that it makes sense, even though it might be 'correct' in a unit test sense. But when doing almost any other tests and certainly when regression testing, I don't want the added noise or time it takes to do so.

For example:

[TestCase("US1")]
[TestCase("123")]
[TestCase("E$R")]
public void Get_From_IsoAlphaCode_IfThreeLetterCodeIsNotAllAlpha_Error(string val)
{
    var ex = Assert.Throws<IsoAlphabeticCodeFormatException>(() => Currency.Get(val));
    ex.Look();  // *** eyeball the whole exception using extension ***
    Assert.That(ex.Message, Is.StringStarting(IsoAlphabeticCodeFormatException.Not3AlphaCharsMessage));
    Assert.That(ex.ParamName, Is.StringMatching("isoAlphaCode"));
    Assert.That(ex.ActualValue, Is.EqualTo(val));
}

    // OUTPUT
IsoAlphabeticCodeFormatException: Badly formed ISO 4217 Aplhabetic Code.
The passed code was not three (3) alphabetic characters.
Parameter name: isoAlphaCode
Actual value was bubba.
   at Support.Guard.AgainstBadIsoAlphaCodeFormat(String paramName, String value) in C:\Support\Guard.cs:line 26
   at Currency.Get(String isoAlphaCode) in C:\Currency.cs:line 731
   at Tests.Currencies.CurrencyTests.<>c__DisplayClass6.<Get_From_IsoAlphaCode_IfThreeLetterCodeIsWrongSize_Error>b__5() in C:.Tests\Currencies\CurrencyTests.CacheFetching.cs:line 95
   at NUnit.Framework.Assert.Throws(IResolveConstraint expression, TestDelegate code, String message, Object[] args)

21 passed, 0 failed, 0 skipped, took 4.71 seconds (NUnit 2.5.10).

So, the design goals here are:

be short and easy to use
be configurable

And I have two concerns about the class as is:

Am I rewriting some existing functionality (ie, does Trace, Debug, NUnit, log4net, etc already do this?
What is a better way to configure the verbosity level?

Here's my code:

///
/// Helpers to 'eyeball' artifacts of a unit test
///
public static class UnitTestLoggingExtensions
{
    private enum OutputOption { OFF, VERBOSE, TERSE, }

    private static OutputOption option = OutputOption.TERSE;

    /// Writes the passed string and any passed format arguments.
    public static void Look(this string msg, params object[] args)
    {
        if (msg == null) {
            Debug.Write("");
            return;
        }
        switch (option)
        {
            case OutputOption.OFF:
                break;
            case OutputOption.VERBOSE:
                Debug.WriteLine(_createVerboseMessage(msg, args), "INFO");
                break;
            case OutputOption.TERSE:
                Debug.WriteLine(string.Format(msg, args), "INFO");
                break;
            default:
                throw new ArgumentOutOfRangeException();
        }
    }

    /// Writes the passed exception's message.
    public static void Look(this Exception ex) { Look(ex.Message); }

    /// Writes the passed object, invoking ToString().
    public static void Look(this object o) { Look(o.ToString()); }

    /// Writes the passed items, invoking ToString().
    public static void Look<T>(this IEnumerable<T> items)
    {
        foreach (var o in items)
        {
            o.Look();
        }
    }

    /// Writes the passed items along with a header.
    public static void Look<T>(this IEnumerable<T> items, string header)
    {
        header.Look();
        items.Look();
    }

    private static string _createVerboseMessage(string format, params object[] args)
    {
        return string.Format("[{0}]{1}->   {2}",
            DateTime.Now.ToString("o"),
            Environment.NewLine,
            string.Format(format, args));
    }
}

I use log4net in my test code along with the ObjectPrinter library and the log4net ConsoleAppender.

When I'm actively working on a test where I want to see additional output I just crank up my logging level in my log4net config file so I can see the additional debug information in my ReSharper test runner console window.

private static readonly ILog Log = LogManager.GetLogger(typeof (MyTestFixture));

[TestCase("US1")]
[TestCase("123")]
[TestCase("E$R")]
public void Get_From_IsoAlphaCode_IfThreeLetterCodeIsNotAllAlpha_Error(string val)
{
    var ex = Assert.Throws<IsoAlphabeticCodeFormatException>(() => Currency.Get(val));
    Log.Debug(ex); // <-- only prints when log4net is configured to log DEBUG msgs
    Assert.That(ex.Message, Is.StringStarting(IsoAlphabeticCodeFormatException.Not3AlphaCharsMessage));
    Assert.That(ex.ParamName, Is.StringMatching("isoAlphaCode"));
    Assert.That(ex.ActualValue, Is.EqualTo(val));
}

ObjectPrinter isn't required, but it outputs more information into the log by reflecting over the passed in objects public properties. The Log.Debug statement outputs something like this in the console window.

2012-01-01 19:38:26,362 [7] DEBUG NUnitTestHarness1.MyTestFixture [IsoAlphabeticCodeFormatException]: hashcode { 11076195 }
{
    Message : Badly formed ISO 4217 Aplhabetic Code.
    StackTrace :    at Currency.Get(String val) in d:\source\spikes\NUnitTestHarness1\NUnitTestHarness1\Class1.cs:line 31
           at NUnitTestHarness1.MyTestFixture.c__DisplayClass1.b__0() in d:\source\spikes\NUnitTestHarness1\NUnitTestHarness1\Class1.cs:line 18
           at NUnit.Framework.Assert.Throws(IResolveConstraint expression, TestDelegate code, String message, Object[] args)
    Source : NUnitTestHarness1
    TargetSite : Int32 Get(System.String)
    HelpLink : {NULL}
    ParamName : {NULL}
    ActualValue : {NULL}
    Data : {}
    InnerException : {NULL}
}


Determining if a number is divisible I've already reduced it and have tried making the main code is_divisible=, followed directly by the (2...).each... code, but that didn't work.

Any suggestions on reducing the size of this code? I have tests for other methods that use the code (so I know if changes are working) but not unit tests for this method yet.

  def self.divisible?(n)
    is_divisible= false
    (2...n).each do |divisor|
      division=n/divisor
      if n== divisor*division
        is_divisible= true
      end
    end
    is_divisible
  end

def self.divisible?(n)
    is_divisible= false
    (2...n).each do |divisor|
      division=n/divisor
      if n== divisor*division
        is_divisible= true
      end
    end
    is_divisible
  end

since you're returning a flag just after computing it, you could just return true instead of setting the flag, and return false if you get to the end. This saves you one line. But, Ruby arrays have the any? and all? predicates. Let's rewrite your code to use any?. This reduces the if condition to only its head. I hope you don't mind if I normalise the whitespace along the way:

def self.divisible?(n)
  (2...n).any? do |divisor|
     division = n / divisor
     n == divisor * division
  end
end

we could inline division (n == (n / division) * division), but let's use the modulo operator instead. Note that unlike your algorithm, modulo even works for floats and even rationals.

def self.divisible? (n)
  (2...n).any? do |divisor|
     n % divisor == 0
  end
end

If you don't mind, I'll reduce the line count further by performing a style change here.

def self.divisible? (n)
  (2...n).any? {|divisor| n % divisor == 0}
end

I don't think you can get shorter than that.

but if your intention is to generate a list of primes, you should only test divisibility by said primes.

def primes_until n
  list = []
  (2..n).each do |x|
    list << x unless list.any? {|d| x % d == 0}
  end
  list
end

Written as a reduce:

def primes_until n
  (2..n).reduce([]) do |list, x|
     list << x unless list.any? {|d| x % d == 0}
     list
  end
end


Powershell cmdlet authenticating to Aerohive API After dipping my toes in PowerShell for over a year i think it is time for creating a real cmdlet. My first mission was creating a regular PS script to take in a .csv and create a wifi account using the Aerohive REST api.

Although it works, i find the script bloated and ugly, and want to swap large code block for proper cmdlets that can also be used by my colleagues.

This is the first step, authenticating against the api. At first i'm only trying to get the basic functions to work. In a later phase i'll add error handling and stuff.

For now i'm also using restsharp. If i ever complete this, i want to swap it for the builtin .net classes so there are no external dependencies.

The next cmdlets will be for creating and deleting wifi accounts. But at first i wanna know if i'm using the correct syntax, proper use of variables, and if everything is secure enough.

using System;
using System.Management.Automation;
using RestSharp;

namespace Aerohive
{
    [Cmdlet(VerbsCommon.New, "Token")]
    [OutputType(typeof(Token))]
    public class NewTokenCmdlet : PSCmdlet
    {
        [Parameter(
            Mandatory = true,
            ValueFromPipelineByPropertyName = true,
            HelpMessage = "The api key, as provided by the IDManager web interface.")]
        public string apiKey { get; set; }

        [Parameter(
            Mandatory = true,
            ValueFromPipelineByPropertyName = true,
            HelpMessage = "The username you use to login on the web interace of the IDManager")]
        public string username { get; set; }

        [Parameter(
            Mandatory = true,
            ValueFromPipelineByPropertyName = true,
            HelpMessage = "The password you use to login on the web interface of the IDManager")]
        public string password { get; set; }

        protected override void BeginProcessing()
        {
            base.BeginProcessing();
        }

        protected override void ProcessRecord()
        {
            WriteVerbose("Authenticating ...");

            var client = new RestClient();
            client.BaseUrl = new Uri("very.long.url/to/authenticate/against");

            var request = new RestRequest("authenticate", Method.POST);
            request.AddHeader("content-type", "application/json");
            request.AddJsonBody(new
            {
                token = apiKey,
                userName = username,
                password = password
            });

            WriteVerbose("response");
            IRestResponse<Token> response = client.Execute<Token>(request);
            WriteObject(response.Data);
        }

        protected override void EndProcessing()
        {
            base.EndProcessing();
        }
    }
    public class Token
    {
        public string token { get; set; }
        public int errorCode { get; set; }
        public string errorMessage { get; set; }
    }
}

You are writing a powershell commandlet... are you sure you want to hardcode the url?

client.BaseUrl = new Uri("very.long.url/to/authenticate/against");

I think it would be better to provide a parameter for this one too. At least make it optional with a default value if you don't want to type it everythime but hardcoding it - well, I woudn't.

It doesn't take long and you'll find another use for it but then you'll say if I only made a parameter for this :-)


My Haskell word count program Here's my first significant Haskell program - it presents a count of each word seen on stdin. Any comments are welcome.

import Data.Map

emptyMap = empty :: Map String Integer

countWord m s = alter (\x -> case x of
                               Nothing -> Just 1
                               Just n -> Just (n+1)) s m

mapPairs m = concat [ k ++ ": " ++ (show v) ++ "\n" | (k,v) <- toList m ]

mapTotal = sum . elems

mapOutput m = (mapPairs m) ++ "Total words: " ++ (show (mapTotal m)) ++ "\n"

wc = interact $ mapOutput . (foldl countWord emptyMap) . words

Some of the questions I have are:

Am I using Map efficiently here? (I realize there are other solutions which do not use a Map - the main point of this exercise was to learn how to use Data.Map.)
How can I make sure that emptyMap, countWord and mapTotal all use the same integral type (Int vs. Integer)? For instance, the type of mapTotal is Map a Integer -> Integer, and so I'll get a type error if I change the definition of emptyMap to empty :: Map String Int. Ideally I'd like to make the choice between Int and Integer in one place for all three definitions.

Your usage of Data.Map is sound. I see nothing that could be improved as long as you stay with Data.Map.

If you give all of the functions explicit type declarations, so that the Monomorphism Restriction doesn't apply, you can use generic integral types:

mapTotal :: Integral i => Map a i -> i

Some quick style-related things you could change:

First:

(\x -> case x of
  Nothing -> Just 1
  Just n -> Just (n+1))

... is the same as

Just . maybe 1 (+1)

Second:

You don't need parens around many of your function calls; if you have something like a +.- (foo bar baz) ¤:* b, it means the same thing as a +.- foo bar baz ¤:* b because every operator has lower precedence than function application.

Third:

You use very long indentations. A tip is to insert newlines after = and before control structures like case to make the code more compact. I find this style to be quite good.


Encoding arbitrary data for stream transmission Basically, what this function should do is encode an arbitrary set of data in memory to a series of bytes that starts with 0x02, ends with 0x03, and encodes each instance of 0x02, 0x03, or 0x25 by preceding it with 0x25. It does this to make it safe for stream transmission, for example, a TCP socket. It returns a non-null-terminated array of char allocated with malloc (and therefore must be freed by the caller) and the length of the new array.

size_t encodedata(char** outdata, const void* data, size_t size)
{
    const char* dchr = data;
    size_t n = 2;
    for (size_t i = 0; i < size; i++)
        n += dchr[i] == 0x02 ? 2 : dchr[i] == 0x03 ? 2 : dchr[i] == 0x25 ? 2 : 1;

    char* encdata = malloc(n);

    encdata[0] = 0x02;
    encdata[n-1] = 0x03;

    for (size_t i = 0, j = 1; i < size && j < (n - 1); i++) {
        if (dchr[i] == 0x02 || dchr[i] == 0x03 || dchr[i] == 0x25)
            encdata[j++] = 0x25;
        encdata[j++] = dchr[i];
    }

    *outdata = encdata;
    return n;
}

The main function, which tests encodedata, runs without problems:

int main(int argc, char* argv[])
{
    char* out;
    char* in = "1" "\x02" "%" "\x03" "3";

    size_t n = encodedata(&out, in, 6);

    printf("size: %d\n", n);

    for (size_t i = 0; i < n; ++i)
        printf("%x ", out[i]);

    printf("\n");

    assert(n == 11);
    assert(out[0] == 0x02);
    assert(out[1] == '1');
    assert(out[2] == 0x25);
    assert(out[3] == 0x02);
    assert(out[4] == 0x25);
    assert(out[5] == 0x25);
    assert(out[6] == 0x25);
    assert(out[7] == 0x03);
    assert(out[8] == '3');
    assert(out[9] == 0x00);
    assert(out[10] == 0x03);

    free(out);
}

Assuming the encoding format itself is not subject to review, it's a pretty good implementation. Things that could be improved (small stuff):

Chaining ?: is not necessary here, compare:

n += dchr[i] == 0x02 ? 2 : dchr[i] == 0x03 ? 2 : dchr[i] == 0x25 ? 2 : 1;

to:

n += (dchr[i] == 0x02 || dchr[i] == 0x03 || dchr[i] == 0x25) ? 2 : 1;

Prefer unsigned char* for work on raw binary sequences, char* means a string of text by convention
0x02, 0x03, and 0x25 all have meanings, so better give them meaningful names, e.g.

const unsigned char ENC_START = 0x02, ENC_END = 0x03, ENC_ESCAPE = 0x25;

In the test:

char* in = "1" "\x02" "%" "\x03" "3";
size_t n = encodedata(&out, in, 6);

it's not clear where 6 came from, so I'd opt for:

char in[] = "1" "\x02" "%" "\x03" "3";
size_t n = encodedata(&out, in, sizeof(in));

Moreover, unless the function signature you proposed is a requirement, I would opt for a buffered design that encodes data in chunks on the fly. It's a little more complicated because you would need to remember the state between calls -- how much input data was processed so far (think of fread). For stream transmission, you probably call encodedata, send the result, and throw it away immediately after. So elliding malloc and relieving the caller from encoding the whole stream at once are both welcome here. One of many possible designs may look like this:

struct encodestate
{
    void* in;
    size_t inlen;
    unsigned char out[256];  // use apprioprate chunk length for your stream
    size_t outlen;
};

/* Writes encoded data into state.out, updates state.outlen
   and for internal use updates state.in and state.inlen.

   Returns whether there's more input data to process (should be called again).
*/
bool encode(struct encodestate* state);

Example of use:

unsigned char in[] = "1" "\x02" "%" "\x03" "3";

for (struct encodestate state = {in, sizeof in}; encode(&state); )
    send(somewhere, state.out, state.outlen);


Comparing md5.Sum to text from file In learning golang, I wrote a small CLI utility that will take paths as arguments and list out their md5 hash as hex strings. Included are two flags that alter functionality, --check | -c which takes an additional file, hashes both, and returns whether or not they match (and exits with the proper return code), and --text | -t that takes the path to a text file that is presumed to have exactly the contents of the hexified MD5 sum and checks it (as above).

The repository is here and code is below:

package main

import (
    "bytes"
    "crypto/md5"
    "flag"
    "fmt"
    "io/ioutil"
    "log"
    "os"
    "strings"
)

var checkSum, textFile string

// build Flags
func init() {
    const (
        checkSumDefault = ""
        checkSumUsage   = "File to check against"
        textFileDefault = ""
        textFileUsage   = "File that has the md5 hash as its only content"
    )

    flag.StringVar(&checkSum, "check", checkSumDefault, checkSumUsage)
    flag.StringVar(&checkSum, "c", checkSumDefault, checkSumUsage)
    flag.StringVar(&textFile, "text", textFileDefault, textFileUsage)
    flag.StringVar(&textFile, "t", textFileDefault, textFileUsage)
}

func makeHash(fname string) [16]byte {
    data, err := ioutil.ReadFile(fname)
    if err != nil {
        panic(fmt.Sprintf("Failed to read file %s", fname))
    }
    return md5.Sum(data)
}

func main() {
    flag.Parse()
    var result int

    if checkSum != "" {
        // check mode -- against file
        checkSumHash := makeHash(checkSum)
        argHash := makeHash(flag.Arg(0))
        if checkSumHash == argHash {
            fmt.Print("They match!")
            result = 0
        } else {
            fmt.Print("No match")
            result = 1
        }
    } else if textFile != "" {
        // check mode -- against text

        checkSum, err := ioutil.ReadFile(textFile)
        if err != nil {
            log.Println(err)
            log.Fatalf("Can't read text file %s", textFile)
        }
        checkSum = bytes.TrimSpace(checkSum)
        argHash := makeHash(flag.Arg(0))
        if strings.EqualFold(fmt.Sprintf("%x", argHash), string(checkSum)) {
            fmt.Print("They match!")
            result = 0
        } else {
            fmt.Print("No match")
            result = 1
        }
    } else {
        // print mode
        for _, fname := range flag.Args() {
            hash := makeHash(fname)
            fmt.Printf("%x\n", hash)
        }
        result = 0
    }
    os.Exit(result)
}

I feel like I should refactor out the different functions and have each one return its success code. That's something I'll do when I have a little time free to poke at it.

The biggest question, however, is the best way to compare the [16]byte argHash to the []byte checkSum if --text is specified. Currently I'm using bytes.TrimSpace, casting to string,  and comparing that with Sprintf("%x", argHash). Is there a better way? Seems silly to convert both to string rather than comparing bytes, but I don't know a better way to do it.

To clearly state our problem/task: We want to compare 2 checksums, one supplied as a text being the hexadecimal representation, and the other being an array of the "raw" bytes.

2 forms exist: a hexadecimal representation and raw bytes. To compare them, we need the same representation. So 2 possible paths:

1. Convert the second to hex representation

Let's see your proposed solution. If we want to handle leading/trailing spaces, and if the text input may contain lowercased and uppercased hex digits, your solution is as simple as it can be.

A variation may be to convert the text to lowercased, and then we can simply compare strings without strings.EqualFold(). The lowercase conversion can be done by calling strings.ToLower() or since we already have the input as []byte, by bytes.ToLower():

checkSum = bytes.ToLower(bytes.TrimSpace(checkSum))
argHash := makeHash(flag.Arg(0))
if fmt.Sprintf("%x", argHash) == string(checkSum) {
    // They match
} else {
    // They don't match
}

2. Convert the first to raw bytes

Compare slices ([]byte)

We may choose to convert the text checksum back to a []byte which holds the raw bytes of the checksum (NOT the bytes of the UTF-8 encoded hex representation).

We can parse the hex representation simply with hex.DecodeString(). Or even better: since we have the text checksum as a []byte, we can use hex.Decode() which takes input as a []byte.

As an extra gain, we don't even have to care about lower or upper case: hex.Decode() handles that for us.

And we can convert [16]byte to []byte by simply slicing it. Once we have 2 []byte, we can use bytes.Equal() to compare them (in Go slices are not comparable unlike arrays).

checkSum = bytes.TrimSpace(checkSum)
dst := make([]byte, 16)
if _, err := hex.Decode(dst, checkSum); err != nil {
    // Invalid input, not hex string or not 16 bytes!
} else {
    argHash := makeHash(flag.Arg(0))
    if bytes.Equal(argHash[:], dst) {
        // They match
    } else {
        // They don't match
    }
}

Compare arrays ([16]byte)

As a variation of the previous solution, we will use arrays to do the comparison as arrays are comparable.

Since makeHash() already returns an array [16]byte, we only need to get the raw bytes of the text checksum into an array. The simplest and fastest is to create an array [16]byte, and pass such a slice to hex.Decode() that shares its backing array with our new array. We can obtain such a slice by simply slicing the array:

checkSum = bytes.TrimSpace(checkSum)
dst := [16]byte{}
if _, err := hex.Decode(dst[:], checkSum); err != nil {
    // Invalid input, not hex string or not 16 bytes!
} else {
    if makeHash(flag.Arg(0)) == dst) {
        // They match
    } else {
        // They don't match
    }
}

Compare manually (byte-by-byte)

We can also do the comparison manually, it's relatively easy and straightforward.

But first to do it manually, let's create a simple helper function which tells if a hex digit (the text representation) equals to the raw data:

func match(hex, raw byte) bool {
    if raw < 10 {
        return hex-'0' == raw
    }
    return hex-'a'+10 == raw || hex-'A'+10 == raw
}

And with this the solution:

checkSum = bytes.TrimSpace(checkSum)
argHash := makeHash(flag.Arg(0))
if len(checkSum) != 2*len(argHash) {
    // Quick check: length differ, they don't match!
} else {
    equal := true
    for i, v := range argHash {
        if !match(checkSum[i*2], v >> 4) || !match(checkSum[i*2+1], v & 0x0f) {
            equal = false
            break
        }
    }
    // Now the variable "equal" tells if they are equal
}


Network MAC address conversion to IPv6 link-local address One of the ways I learn a new scripting language is to implement a subroutine to convert a network MAC addr into the IPv6 link-local address, as described in RFC 4862. So here is a quick few lines.

My question to you is:

can this code be shrunk any more?
is there a more pythonic way of doing this?
is there anything non-optimal?
is the code future proof to 3.4 python?

Especially with casting a string to an int, and finally back to hex, seems overly weird.

Also, I did recently discover some IP related factory functions, but so far I've not discovered any builtin way of doing this. I'm using python 2.7, but my Linux distro is soon to switch to 3.x.

#!/usr/bin/python
"""
convert mac addr to ipv6 link local (rfc 4862)
"""

# the mac should become: fe80::2177:02ff:fed2:ff9b
mac='23:77:02:d2:ff:9b'

def mac_to_ipv6_linklocal(mac):

  # remove the most common macaddr delimiters, dots, dashes, etc.
  # cast the hex string to an base-16 int for math safety.
  # use xor of 02 on the 2nd most significant hex char.
  # Remove/slice the '0x' off the begining from using hex().

  m = hex(int(mac.translate(None,' .:-'),16)^0x020000000000)[2:]
  return 'fe80::%s:%sff:fe%s:%s' %(m[:4],m[4:6],m[6:8],m[8:12])

print mac_to_ipv6_linklocal(mac)

Note that the purpose of the hex() function is to produce a hexadecimal integer literal (a string that can be used in Python source code), which is why it returns a hex value with the 0x prefix. Use the format() function instead, as it gives you much more control over the output.

For example, format(value, '012x') formats the integer value to a 12-digit hexadecimal (using lowercase digits), padding out the result to the minimum width with leading zeros as needed:

>>> mac_value = 0x237702d2ff9b
>>> format(mac_value, '012x')
'237702d2ff9b'
>>> format(42, '012x')
'00000000002a'

In this case, I'd actually split out the various bytes of the MAC address binary value that make up the IPv6 parts, and use str.format() to interpolate those into the IPv6 string.

def mac_to_ipv6_linklocal(mac):
    # Remove the most common delimiters; dots, dashes, etc.
    mac_value = int(mac.translate(None, ' .:-'), 16)

    # Split out the bytes that slot into the IPv6 address
    # XOR the most significant byte with 0x02, inverting the
    # Universal / Local bit
    high2 = mac_value >> 32 & 0xffff ^ 0x0200
    high1 = mac_value >> 24 & 0xff
    low1 = mac_value >> 16 & 0xff
    low2 = mac_value & 0xffff

    return 'fe80::{:04x}:{:02x}ff:fe{:02x}:{:04x}'.format(
        high2, high1, low1, low2)

I followed the PEP 8 whitespace recommendations to format the code here; whitespace around operators and after commas, for example.

In Python, we usually don't strive for shorter code; we aim for readable code. Sometimes you can express an algorithm in a very concise and readable way, but avoid code-golfing for the sake of compactness.

Python 3.3 added a ipaddress module; this module does not however include a utility to generate an IPv6 address from a given MAC address.


Haskell monad- and error-handling-style As an answer to excersise 2.3 in Chris Okasaki's book, Purely Functional Data Structures,

  Inserting an existing element into a binary search tree copies the
  entire search path even though the copied nodes are indistinguishable
  from the originals. Rewrite insert using exceptions to avoid this
  copying. Establish one handler per insertion rather than one handler
  per  iteration.

I've written the following haskell implementations of insertion in a binary heap:

import Data.Maybe

data Tree e = Leaf | T (Tree e) e (Tree e) deriving Show

empty = Leaf

insert x Leaf = T Leaf x Leaf
insert x tree@(T l e r)
  | x == e = tree
  | x < e = T (insert x l) e r
  | otherwise = T l e (insert x r)

ins x t =
  t >>= \t -> case t of
                Leaf   -> Just $ T Leaf x Leaf
                tree@(T l e r)
                  | x == e -> Nothing
                  | x < e -> (ins x (Just l)) >>= \nt -> Just $ T nt e r
                  | ot herwise -> (ins x (Just r)) >>= \nt -> Just $ T l e nt

fastInsert x t =
  fromMaybe t $ ins x $ Just t

The fastInsert function (using ins as a 'helper') and the insert function will return the same tree given the same input, but fastInsert wont copy sub-trees if no insertion is needed (if the tree already contains the node to be inserted).

The question is: how would this look if written more idiomatically, with regard to monadic operations/syntax in ins?

Another question: I'm using monads as a substitute for exceptions as suggested in the book. The point is to avoid rebuilding (reference copying) the tree when returning in the recursive calls. How should I have gone about this?

There are couple of points that I'd suggest:

Be sure to always include types for top-level functions. Without them, the code becomes very quickly unreadable. For example, would you know what ins does and what is its type, if you didn't write it yourself? In many cases you can immediately judge what a function does just looking at its name and its type.
If you do

  | x < e  = ...
  | x == y = ...
  | otherwise = ...

you're performing the comparison twice. In most cases this shouldn't be a problem, but if writing

case compare x e of
  LT -> ...
  EQ -> ...
  GT -> ...

will compare only once, and also allows the compiler to do more optimizations.
This isn't a problem, just a note: Be aware that fastInsert trades speed for lazyiness. If you examine the root of the tree returned by a fastInsert call, the tree must be examined to check if it contains the inserted element or not. Under most circumstances you wouldn't mind that.
There is no need for ins to take Maybe (Tree e) as the argument, only to apply >>= on it. Most monadic functions take pure arguments and return a monadic one.
It's often convenient to use functions from Control.Applicative to lift pure functions/values to monadic computations. In particular pure, <$> and <*>. See also Functor, Applicative and Monad.

I wasn't sure what error handling you have in mind, as these operations should never fail.

The updated program:

import Control.Applicative
import Data.Maybe

data Tree e = Leaf | T (Tree e) e (Tree e) deriving Show

empty = Leaf

insert :: (Ord e) => e -> Tree e -> Tree e
insert x Leaf = T Leaf x Leaf
insert x tree@(T l e r) = case compare x e of
  EQ -> tree
  LT -> T (insert x l) e r
  GT -> T l e (insert x r)

ins :: (Ord e) => e -> Tree e -> Maybe (Tree e)
ins x Leaf = return $ T Leaf x Leaf
ins x tree@(T l e r) = case compare x e of
  EQ -> Nothing
  LT -> (\l' -> T l' e r) <$> ins x l
-- you could also write a point-free version, if you like,
-- which better preserves the order of arguments
--  LT -> T <$> ins x l <*> pure e <*> pure r
  GT -> T l e <$> ins x r

fastInsert :: (Ord e) => e -> Tree e -> Tree e
fastInsert x t =
  fromMaybe t $ ins x t


Conditionally eliminating one of two colliding bodies I like to explicit return function even for void functions to reduce the use of if-elses, like this code snippet:

private void processContactBetweenPlayers(Contact contact) {
    final Body bodyA = contact.getFixtureA().getBody();
    final Body bodyB = contact.getFixtureB().getBody();
    if (!isPlayer(bodyA) || !isPlayer(bodyB)) {
        return;
    }
    final float vx_a = Math.abs(bodyA.getLinearVelocity().x);
    final float vx_b = Math.abs(bodyB.getLinearVelocity().x);
    final float vy_a = Math.abs(bodyA.getLinearVelocity().y);
    final float vy_b = Math.abs(bodyB.getLinearVelocity().y);
    final float a_power = vx_a + vy_a;
    final float b_power = vx_b + vy_b;
    if ((a_power > b_power)) {
        if (save(bodyB)) return;
        markForDestroy(bodyB);
    } else {
        if (save(bodyA)) return;
        markForDestroy(bodyA);
    }
}

The function returns if both bodies are not players. But doing maybe this practice reduces the code readability. What do you think?

IMHO this is perfectly fine.

if (!isPlayer(bodyA) || !isPlayer(bodyB)) {
    return;
}

This is an early return, saving you one indentation level.

    if (save(bodyB)) return;
    markForDestroy(bodyB);

This is not as good. Here, I'd go for

    if (!save(bodyB)) markForDestroy(bodyB);

Note that braces should be used around single statements as well, i.e.,

    if (!save(bodyB)) {
        markForDestroy(bodyB);
    }

  But doing maybe this practice reduces the code readability.

No, used wisely it helps readability. When reading top down, you know that both bodies are players. In an if-block you know this too, but the indentation may get excessive. Moreover, using the early return makes clear that only players are dealt with.

As stated in the comments, processContact would be a much better name. A method called processContactBetweenPlayers could be extracted (not needed now as it's rather short). Such a method should use something like

checkArgument(isPlayer(bodyA) && isPlayer(bodyB));

in case it's public, otherwise I'd recommend

assert isPlayer(bodyA) && isPlayer(bodyB);

While asserts are often frowned upon, this is exactly what they're good for.