Implement a function to check if a tree is balanced Yet another interview question:

  Implement a function to check if a tree is balanced. For the purposes of this question, a balanced tree is defined to be a tree such that no two leaf nodes differ in distance from the root by more than one.

First off, did I understand the question correctly? In other words, we are looking for a subtree in a tree (or rotation of it) that looks like:

     root
    /    \
 leaf_1  leaf_2
  /
leaf1_1

My initial idea was to count the depth of the tree and check if maxDepth - minDepth < 2 (which requires 2 traversal) . But I think I found a more efficient solution. The trade-off is that we should keep the ref to the root. I assume n-ary tree.

Is this correct?

import java.util.LinkedList;

public class Trees {

    Node threeRoot;
    private boolean isBalanced = true;

    Trees(Node threeRoot){
        this.threeRoot = threeRoot;
    }

    static class  Node{
        public Node root;
        public LinkedList<Node> children = new LinkedList<>();
    }

     boolean isBalanced(Node node){
       if (threeRoot == null) {
            isBalanced = true;
        }
        if (threeRoot.children == null || threeRoot.children.isEmpty()) {
            isBalanced = true;
        }

        isBalancedRed(node);
        return isBalanced;
    }

     void isBalancedRed(Node node) {
        if (node.children.isEmpty()) {
            if (node.root != null && node.root.children.size() == 1) {
                isBalanced = false;
            }
        }
        for(Node nodeEl : node.children)
        {
            isBalancedRed(nodeEl);
        }
    }

    public static void main(String[] args){
        Trees tree = new Trees(new Node());
        Node root = tree.threeRoot;
        Node leaf1 = new Node();
        Node leaf2 = new Node();
        Node leaf11 = new Node();

        root.children.add(leaf1);
        leaf1.root = root;
        root.children.add(leaf2);
        leaf2.root = root;

        leaf2.children.add(leaf11);
        leaf11.root = leaf2;

        System.out.println(tree.isBalanced(root));
    }
}

First off, I would use something other than a LinkedList to hold the children of an n-ary tree. A LinkedList requires you to enumerate the list in order to get the reference to any child other than the leftmost, which is going to slow you down.

Second, your code is basically returning false in any situation where the Node's parent has one child that itself doesn't have any children. In your example tree, that's the exact case (you have two children of the root, one of which has another child) but the tree is balanced according to the rules, so I would expect this algorithm to return a lot of false positives (and negatives).

Finding the depth of an n-ary tree is a linear-bound operation; you traverse each branch of the tree recursively, finding the depth at each leaf node. You can find both the maximum and minimum depth in one traversal; at each level, ask each branch for its maximum and minimum depth, passing the depth of the current node. The base case is that of a leaf node (no children); the min and max is the depth of that leaf. You can store this result in a Tuple or in a more specialized MinMax struct, as you please. With those results returned to the calling level, scan them to find the lowest Min and the greatest Max, and return that to the caller.

Here's a basic implementation:

public class MinMax<T>
{
    private T minVal;
    private T maxVal;

    public MinMax(T min, T max)
    {
       minVal = min;
       maxVal = max;
    }

    public T min() {return minVal;}
    public T max() {return maxVal;}
}

private MinMax<Integer> GetMinMaxDepth(Node node, int currDepth)
{
    //base case; no children. Max and min depth at this leaf is currDepth.
    if(node.children == null || node.children.size() == 0)
       return new MinMax<Integer>(currDepth, currDepth);

    //determine the maximum and minimum depth of all child branches,
    //and track the absolute max and min across all of them.
    int min = Integer.MAX_VALUE, max = -1;
    for(Node nextNode : node.children)
    {
        MinMax<Integer> childDepth = GetMinMaxDepth(nextNode, currDepth+1);
        if(childDepth.min() < min) min = childDepth.min();
        if(childDepth.max() > max) max = childDepth.max();
    }

    return new MinMax<Integer>(min, max);
}

boolean isBalanced()
{
    //easy cases; a tree with 0 or 1 elements.
    if(treeRoot == null || treeRoot.children == null || treeRoot.children.size() == 0)
       return true;
    //traverse the tree and find the minimum and maximum depth.
    MinMax<Integer> minMaxDepth = GetMinMaxDepth(treeRoot, 0);

    return minMaxDepth.max() - minMaxDepth.min() > 1;
}

This could probably be optimized in our case to return early if we discover that any node's child depths differ by more than 1. We could hack it by throwing an Exception, but the proper strategy is almost as easy to implement:

private MinMax<Integer> GetMinMaxDepth(Node node, int currDepth)
{
    //base case; no children. max and min depth at this leaf is currDepth.
    if(node.children == null || node.children.size() == 0)
       return new MinMax<Integer>(currDepth, currDepth);

    //determine the maximum and minimum depth of all child branches,
    //and track the absolute max and absolute minimum.
    int min = Integer.MIN_VALUE, max = -1;
    for(Node nextNode : node.children)
    {
        MinMax<Integer> childDepth = GetMinMaxDepth(nextNode, currDepth+1);
        //Exit now if child is unbalanced
        if(childDepth.max() - childDepth.min() > 1) return childDepth;
        if(childDepth.min() < min) min = childDepth.min();
        if(childDepth.max() > max) max = childDepth.max();
        //Exit now if current node is unbalanced
        if(max-min > 1) break;
    }

    return new MinMax<Integer>(min, max);
}

The upside is that we quit as soon as we know the answer to the question (is the tree unbalanced?), which will increase the average performance (but not the worst-case performance on a tree that is balanced or is unbalanced at its furthest extremities). The downside is that we no longer know how unbalanced the tree is; the MaxMin returned to the top level will always have a Max and Min that differ by the first detected difference greater than the threshold (probably 2), not the absolute difference in depth of leaf nodes in the tree.

The one case that is difficult to determine in an n-ary tree is that of a tree that never forks. This algorithm will find a depth difference between any two or more branches, but when there is only one branch, the maximum and minimum depths of the tree are the same and there's nothing to use for comparison. Technically, it would have only one "leaf" node (usually defined as a node with no children) and so by the given definition all (one) of the leaves are the same distance from the root, however if you looked at the map of an N-ary tree that was more like a linked list (or even a V or Y shape) and went deeper than two levels, you wouldn't call it balanced.

Perhaps a change in definition may be required; a node is a "leaf" node if it does not have N child nodes (where N is the order of the N-ary tree). So, in a quaternary tree (4 children per node), if any node has fewer than 4 children, then the depth of the current node is considered as a "minimum" depth and will almost certainly be the minimum depth at that level. That's another easy change:

private MinMax<Integer> GetMinMaxDepth(Node node, int currDepth)
{
    //base case; no children. max and min depth at this leaf is currDepth.
    if(node.children == null || node.children.size() == 0)
       return new MinMax<Integer>(currDepth, currDepth);

    //determine the maximum and minimum depth of all child branches,
    //and track the absolute max and absolute minimum.
    //Exit as soon as we know that this or any child node is unbalanced.
    int min = Integer.MIN_VALUE, max = -1;

    //If this node isn't "full", its depth is the minimum depth for this branch
    if(node.children.size < max_children) min = currDepth;

    for(Node nextNode : node.children)
    {
        MinMax<Integer> childDepth = GetMinMaxDepth(nextNode, currDepth+1);
        //check if child is already unbalanced
        if(childDepth.max() - childDepth.min() > 1) return childDepth;
        if(childDepth.min() < min) min = childDepth.min();
        if(childDepth.max() > max) max = childDepth.max();
        //check if current node is unbalanced
        if(max-min > 1) break;
    }

    return new MinMax<Integer>(min, max);
}


Extending UITableViewCell to preserve UIView background color when selecting the cell When a UITableViewCell has subviews with background color set, when setSelected(_: animated:) gets called, all the views are made transparent.

This issue is also discussed here

I'm trying to solve this issue by extending UITableViewCell

extension UITableViewCell {
    private func backgroundColors(views: [UIView]) -> [UIColor?] {
        var colors = [UIColor?]()
        for view in views {
            colors.append(view.backgroundColor)
        }
        return colors
    }

    private func resetBackgroundColors(views: [UIView], colors: [UIColor?]) {
        for (index, view) in views.enumerate() {
            view.backgroundColor = colors[index]
        }
    }

    func preserveBackgroundColors(views: [UIView], @noescape f: () -> Void) {
        let colors = backgroundColors(views)
        f()
        resetBackgroundColors(views, colors: colors)
    }
}

class MyCell: UITableViewCell {
    override func setSelected(selected: Bool, animated: Bool) {
        preserveBackgroundColors([/* some views */]) {
            super.setSelected(selected, animated: animated)
            /* rest of method implementation */
        }
    }
}

I'm trying to avoid having this functionality in a class hierarchy.

Is this a good implementation?

This is a very strange approach for quite a few reasons.

First, the fact that this is an extension of UITableViewCell makes absolutely no sense to me.  The method is completely procedural.  It takes in an array of views (any views... from any where).  From these views, it creates an array of colors to preserve the current background color.  Then executes a function, f (which by the way, SwiftLint would never allow a single-letter variable name like this) (and you should definitely be using SwiftLint).  And function f may or may not even change the colors of any of these views.  And then we use parallel arrays, a very procedural approach, to reset these colors.

And would we even notice the color change at all?

Or is the intent here to hide the color change?

As for a more concrete review of some of the code you have, this function:

private func backgroundColors(views: [UIView]) -> [UIColor?] {
    var colors = [UIColor?]()
    for view in views {
        colors.append(view.backgroundColor)
    }
    return colors
}

Could be replaced with a single line of code.

let backgroundColors = views.map { $0.backgroundColor }

But... to make our code less procedural... why don't we build a tuple array:

let viewColorPairs = views.map { ($0, $0.backgroundColor) }

And then when it comes time to reset the background colors:

for (view, color) in viewColorPairs {
    view.backgroundColor = color
}

So now we've eliminated your first two functions, and your preserveBackgroundColors function is simplified into:

func preserveBackgroundColors(views: [UIView], @noescape closure: () -> Void) {
    let viewColorPairs = views.map { ($0, $0.backgroundColor) }
    closure()
    for (view, color) in viewColorPairs {
        view.backgroundColor = color
    }
}


General feedback on Conway's Game of Life implementation I've just written an implementation of Conway's Game of Life as an exercise and I'd love to get some feedback on how I can improve this program and hopefully my style in general.

I've tested the code in Python 2.7.3. It requires the PyPNG module to render the state of the simulation.

"""Simulate Conway's Game of Life.

The world is finite. Cells at its border always stay dead.
The frames of the simulation are saved as .png files.
"""

from numpy.random import random_integers
from png import from_array

DEAD = 1
ALIVE = 0

def saveAsImage(array, number):
    """Save the array to a .png file.

    The files are saved as 1-bit greyscale (black/white).
    """

    image = from_array(array, 'L', {'bitdepth': 1})
    image.save('image%03i.png' % number)

def simulate(world, simulationLength):
    """Simulate a certain number from steps starting from a given state.

    The frames are saved as .png images in the current working directory.

    Parameters:
        world: The initial state of the world as a two-dimensional numpy
               array in which world[y][x] describes the state of the
               cell at position x, y
        simulationLength: The number of simulated steps,
                          simulationLength + 1 images are created.
    """

    # Retrieve the dimensions of the world
    worldHeight = len(world)
    worldWidth = len(world[0])

    # Make sure there are no living cells on the border.
    world[0] = DEAD  # top
    world[worldHeight - 1] = DEAD  # bottom
    for y in xrange(1, worldHeight - 2):
        world[y][0] = DEAD  # left
        world[y][worldWidth - 1] = DEAD  # right

    saveAsImage(world, 0)

    for frame in xrange(1, simulationLength + 1):
        # Simulate simulationLength frames

        nextWorld = world.copy()

        for y in xrange(1, worldHeight - 1):
            for x in xrange(1, worldWidth - 1):
                # Iterate over all cells within the border

                neighbors = [world[y - 1][x - 1],
                             world[y - 1][x],
                             world[y - 1][x + 1],
                             world[y][x - 1],
                             world[y][x + 1],
                             world[y + 1][x - 1],
                             world[y + 1][x],
                             world[y + 1][x + 1]]
                livingNeighbors = sum(neighbor == ALIVE
                                      for neighbor in neighbors)

                if world[y][x] == ALIVE:
                    # Living cells with less than 2 or more than three
                    # living neighbors die
                    if livingNeighbors not in (2, 3):
                        nextWorld[y][x] = DEAD
                else:
                    # Dead cells with three living neighbors are reborn
                    if livingNeighbors == 3:
                        nextWorld[y][x] = ALIVE

        world = nextWorld

        saveAsImage(world, frame)

if __name__ == '__main__':
    worldWidth = 100
    worldHeight = 100

    # Populate the world with random living/dead (50% each) cells
    world = random_integers(0, 1, (worldHeight, worldWidth))

    simulate(world, 50)

Separate different concerns in your code.  The function simulate does two things that should be separated, namely changing the state of the world and calling the function that saves it as an image.  As it is, it's impossible to re-use only the state-changing logic.  Instead, move the game logic to a function simulate_step (or whatever you want to call it).

Stick to the PEP8 naming conventions: functions and variables should be written lowercase_with_underscores. Otherwise, you follow the guide very nicely.


You have no respect for logic Inspired by Hosch250, TopinFrassi, Legato and the recent milestone of having our 100th fizzbuzz question on Code Review I decided to add an ArnoldC FizzBuzz to the collection.

I haven't found any best practices about the language, but I assume the code needs to be VERBOSE. For those unfamiliar with the language, a small excerpt of the keywords:

True              NO PROBLEMO
ModuloOperator    I LET HIM GO
DeclareInt        HEY CHRISTMAS TREE
AssignVariable    GET TO THE CHOPPER

The challenge itself doesn't need introduction. Write all numbers from 1 up till and including 100, but:

Write Fizz instead of the number when divisible by 3
Write Buzz instead of the number when divisible by 5
Write FizzBuzz instead of the number when divisible by 15

LISTEN TO ME VERY CAREFULLY modulo
  I NEED YOUR CLOTHES YOUR BOOTS AND YOUR MOTORCYCLE dividend
  I NEED YOUR CLOTHES YOUR BOOTS AND YOUR MOTORCYCLE divisor
  GIVE THESE PEOPLE AIR
    HEY CHRISTMAS TREE quotient
      YOU SET US UP 0
    HEY CHRISTMAS TREE remainder
      YOU SET US UP 0
    HEY CHRISTMAS TREE product
      YOU SET US UP 0
    GET TO THE CHOPPER quotient
      HERE IS MY INVITATION dividend
      HE HAD TO SPLIT divisor
    ENOUGH TALK
    GET TO THE CHOPPER product
      HERE IS MY INVITATION divisor
      YOU'RE FIRED quotient
    ENOUGH TALK
    GET TO THE CHOPPER remainder
      HERE IS MY INVITATION dividend
    GET DOWN product
  ENOUGH TALK
  I'LL BE BACK remainder
HASTA LA VISTA, BABY

LISTEN TO ME VERY CAREFULLY divisible
  I NEED YOUR CLOTHES YOUR BOOTS AND YOUR MOTORCYCLE dividend
  I NEED YOUR CLOTHES YOUR BOOTS AND YOUR MOTORCYCLE divisor
  GIVE THESE PEOPLE AIR
    HEY CHRISTMAS TREE result
    YOU SET US UP 0
    GET YOUR ASS TO MARS result
    DO IT NOW modulo dividend divisor

    HEY CHRISTMAS TREE isZero
    YOU SET US UP 0
    GET TO THE CHOPPER isZero
    HERE IS MY INVITATION result
    YOU ARE NOT YOU YOU ARE ME 0
  ENOUGH TALK

  I'LL BE BACK isZero
HASTA LA VISTA, BABY

IT'S SHOWTIME
    HEY CHRISTMAS TREE maxValue
    YOU SET US UP @NO PROBLEMO
    HEY CHRISTMAS TREE n
    YOU SET US UP 0
    HEY CHRISTMAS TREE divisible
    YOU SET US UP @NO PROBLEMO
    HEY CHRISTMAS TREE multiple
    YOU SET US UP @NO PROBLEMO

    STICK AROUND maxValue
        GET TO THE CHOPPER n
        HERE IS MY INVITATION n
            GET UP 1
        ENOUGH TALK
        GET TO THE CHOPPER maxValue
            HERE IS MY INVITATION 100
            LET OFF SOME STEAM BENNET n
        ENOUGH TALK

        GET YOUR ASS TO MARS multiple
        DO IT NOW divisible n 15
            BECAUSE I'M GOING TO SAY PLEASE multiple
            TALK TO THE HAND "FizzBuzz"
            BULLSHIT
            GET YOUR ASS TO MARS multiple
            DO IT NOW divisible n 5
                BECAUSE I'M GOING TO SAY PLEASE multiple
                TALK TO THE HAND "Buzz"
                BULLSHIT
                GET YOUR ASS TO MARS multiple
                DO IT NOW divisible n 3
                    BECAUSE I'M GOING TO SAY PLEASE multiple
                    TALK TO THE HAND "Fizz"
                BULLSHIT
                    TALK TO THE HAND n
                YOU HAVE NO RESPECT FOR LOGIC
            YOU HAVE NO RESPECT FOR LOGIC
        YOU HAVE NO RESPECT FOR LOGIC
    CHILL
YOU HAVE BEEN TERMINATED

There's incorrect/inconsistent indentation in:

HEY CHRISTMAS TREE result
YOU SET US UP 0

and

HEY CHRISTMAS TREE isZero
YOU SET US UP 0

The ArnoldC wiki says you should declare 0s and 1s as the "macros" @I LIED and @NO PROBLEMO

HEY CHRISTMAS TREE isZero
    YOU SET US UP @I LIED

In your method divisible, the end of method statement is not on the same indentation as the start declaration.

The indentation in these two blocks is two vs four:

IT'S SHOWTIME
    HEY CHRISTMAS TREE maxValue

and:

LISTEN TO ME VERY CAREFULLY divisible
  I NEED YOUR CLOTHES YOUR BOOTS AND YOUR MOTORCYCLE dividend

You ought to stick to one consistently.

IT'S SHOWTIME
    HEY CHRISTMAS TREE maxValue
    YOU SET US UP @NO PROBLEMO

There we go! Using the macros!

Oh, wait...

HEY CHRISTMAS TREE n
YOU SET US UP 0

Never mind...

And again...

    HERE IS MY INVITATION n
        GET UP 1


Exam score query for many types of exams I have the following MySQL SELECT statement. It is working fine except that the code is too long. I have been looking throughout the Internet to figure out how I can make it shorter.

 SELECT regd, Subject, Section, date,
 SUM(CASE WHEN (Name_of_exam = 'First Unit Exam'
 OR Name_of_exam = 'Second Unit Exam'
 OR Name_of_exam = 'Third Unit Exam')
 THEN (Mark_score / Full_mark) *25 END) AS t_scored,
 SUM(CASE
 WHEN (Name_of_exam = 'First Unit Exam'
 OR Name_of_exam = 'Second Unit Exam'
 OR Name_of_exam = 'Third Unit Exam')
 THEN (Full_mark) END) AS t_fm,
 SUM(CASE
 WHEN (Name_of_exam = 'First Term Weekly Test'
 OR Name_of_exam = 'Second Term Weekly Test'
 OR Name_of_exam = 'Third Term Weekly Test'
 OR Name_of_exam = 'Final Term Weekly Test')
 THEN (Mark_score / Full_mark) *25
 END ) AS w_scored,
 SUM(CASE
 WHEN (Name_of_exam = 'First Term Weekly Test'
 OR Name_of_exam = 'Second Term Weekly Test'
 OR Name_of_exam = 'Third Term Weekly Test'
 OR Name_of_exam = 'Final Term Weekly Test')
 THEN (Full_mark) END ) AS w_fm,
 SUM(CASE
 WHEN Name_of_exam = 'Final Unit Exam'
 THEN (Mark_score / Full_mark) *25
 END ) AS f_scored,
 SUM(CASE
 WHEN Name_of_exam = 'Final Unit Exam'
 THEN (Mark_score) END ) AS score_m,
 SUM(CASE
 WHEN Name_of_exam = 'CCE'
 THEN (Mark_score / Full_mark) *25
 END ) AS cce_scored,
 SUM(CASE
 WHEN Name_of_exam = 'CCE'
 THEN (Full_mark) END ) AS cce_fm
 FROM exam_mark
 WHERE regd='23' AND Section='A'
 AND date BETWEEN '2013-11-01' AND '2013-11-15'
 GROUP BY Subject

Any suggestion is welcome.

This kind of expression

 (Name_of_exam = 'First Term Weekly Test'
 OR Name_of_exam = 'Second Term Weekly Test'
 OR Name_of_exam = 'Third Term Weekly Test'
 OR Name_of_exam = 'Final Term Weekly Test')

can usually be shortened to an IN clause.

(Name_of_exam in ('First Term Weekly Test', 'Second Term Weekly Test', etc.))

But there's enough conditional logic in here that I'd have to ask myself, "Am I trying to write a report in SQL?" Reports are better implemented with a report writer.

This doesn't have to do with your question, but a GROUP BY statement that includes only one of 'n' unaggregated columns is almost always a syntax error in standard SQL. MySQL treats this like a feature, but it's a feature best avoided.

It's almost always a syntax error, because standard SQL allows you to group on one of 'n' unaggregated colums when the unaggregated columns are  functionally dependent on the grouped column.


Shorten my URL shortener I am working on a website, and due to some reasons, they want my code to be as short as possible. I have, from my side, tried everything to shorten this code.

So if possible, can you all also help me in making this code shorter?  I know that there are vulnerabilities, and I will solve them later.

For now, just help me make it shorter. There are two files that need to be shortened.

first.php

<?php
$LocationId = (isset($_GET['l'])) ? $_GET['l'] : null;
if( $LocationId )
{ //Make connection to DB
    $DBConn = mysqli_connect("localhost","root","","url") or die("Connection Error");
//Selects the URL which mathces the ID or say shortcode
    $Resut  = mysqli_query($DBConn, sprintf("SELECT url FROM url WHERE id LIKE '%s' LIMIT 0,1", mysqli_real_escape_string($DBConn, $LocationId))) or die( mysqli_error() );
    if( $Resut )
    {   $Row = mysqli_fetch_array($Resut);
        if( isset($Row['url']) && !empty($Row['url']) ) //checks whether URL exist or not
        {        header('HTTP/1.1 301 Moved Permanently');
            header(sprintf('Location: %s', $Row['url'])); //Redirects the user to the long URL
            exit;   }
        else
        {   echo "Invalid URL"; }}} //If wrong ID given by user then returns this error
?>

second.php

<?php
$con=mysqli_connect("localhost","root","","url") or die("Connection Error");

$url = $_POST['url'];
$sql=mysqli_query($con,"INSERT INTO url (url)VALUES('$url')");

$result = mysqli_query($con,"SELECT * FROM url WHERE url='$url'");
while($id = mysqli_fetch_array($result))
  {
  echo " URL Shortened Successfully! ".'<br>'."Your shortened URL is ";
  echo 'http://localhost/redirect/' . $id['id'];
  }

mysqli_close($con);
?>

And one more thing: there are 6-7 lines of whose meaning I can't understand, so please explain this to me this as well. Those 6-7 lines are as follows:

 $Resut  = mysqli_query($DBConn, sprintf("SELECT url FROM url WHERE id LIKE '%s' LIMIT 0,1", mysqli_real_escape_string($DBConn, $LocationId))) or die( mysqli_error() );
    if( $Resut )
    {   $Row = mysqli_fetch_array($Resut);
        if( isset($Row['url']) && !empty($Row['url']) ) //checks whether URL exist or not
        {        header('HTTP/1.1 301 Moved Permanently');
            header(sprintf('Location: %s', $Row['url'])); //Redirects the user to the long URL
            exit;   }

Brevity of code seems an odd point of focus, so I plan on ignoring that. I do, however, have quite a few suggestions, and, convieniently for you, those will probably shorten the code.

Don't assume user provided variables are actually set. For example, don't access $_POST['url'] directly. Either cover it in a isset guard like you've done other places, or use filter_input. Also, don't trust that user provided variables will be strings. They can be arrays. Might want to add a is_string check.

Your code is wide open to SQL injection. Never put an unescaped string into a query. Always either use prepared statements or properly escape strings. Prepared statements tend to be a bit harder to accidentally forget to do, so they are what I usually go with.

Use either $a=val or $a = val consistently. Inconsistent coding style is hard on the brain. I strongly suggest the $a = val style.

Your indention and line breaks are rather sporadic and odd. first.php is particularly bad. Don't include things on the same line as a brace like:

if ()
{ something here

Also, make sure you develop a habit for indention at each code block level and stick with it.

For such a simple script, or die is fine. In general though, it can be a bit of a problem since it doesn't allow for pretty error handling very easily. It's often better to throw an exception so that higher up code can catch it and gracefully render an error instead of just a plain text string. Like I said though, it doens't really apply here since nothing is being rendered anyway.

Queries should never fail in production code. This means that if( $Resut ) shouldn't be necessary. If the query is failing, something is majorly wrong as it means either an entity is missing (like a DB or table) or a syntax error happened. An entity missing is problematic since it means the DB is out of sync with the code, and a syntax error is worrying since it means that code either wasn't tested and legitimately has a syntax error or that SQL injection is occuring.

Basically all of the comments are meaningless. All they do is describe exactly what the code is doing. As long as someone can read PHP, they can already see what the code is doing. Typically comments should describe why something is being done not how. (In algorithmical code, it's fairly common to comment what is going on, but that's because it's much harder to follow than straight forward code like this.)

There's no reason to put the final closing tag in a PHP file. It can end up causing complications with trailing whitespace or non-printed characters, and it isn't syntactically required. For this reason, it's become customary lately to omit the closing tag.

This comes down to personal preference and OCD really, but I tend to avoid closing resources that last until the end of script execution. The mysql connection is about to be cleaned up anyway when PHP tears itself down, so the mysqli_close is really just superfluous. Some people like the explicitness of it though and it gives them peace of mind knowing that they closed something they opened. That's a very valid approach. There's just not technically a need for it, and I like to avoid clutter.

Note that this does not apply to resources that are not tied to the life time of the script. In general, resources should be held as briefly as reasonably possible.

empty($x) is equivalent to isset($x) && $x. This means that isset($x) && !empty($x) is the same as isset($x) && isset($x) && $x. In other words, just use !empty($x) if you want the behavior specified. The isset is redundant.

For code this simple and short, it's not really necessary, but if this grows, I would extract common setup in a bootstrapping mechanism of some sort. This would allow you to avoid having to duplicate your mysqli conneciton across every file. That means you won't have to change the DB credentials in n files if you need. Instead, you could just change them in one.

If the code grows more complex, I would extract functionality into functions. Two obvious functions that could exist now are getUrlFromId and createUrlShortening.

Functions allow your program flow to focus on program flow and not implementation.

Functions also allow for a central point of change. For example, if this grows into a complex website that has tons of pages and whatnot, you're likely going to need to be able to retrieve URLs from many pieces of code. Now imagine that you've found a bug in your URL retrieving code or you want to change a table name or something. You can either Control+F and fix it in a ton of places and hope you haven't missed anything, or you can have one centralized place to change it.

  And one more thing: there are 6-7 lines of whose meaning I can't understand, so please explain this to me this as well.

That block of code queries the database for a record in the table url that has an ID equivalent to the location ID the end user provided (via the l GET parameter -- like blah.php?l=33).

WHERE id LIKE `%s` LIMIT 0, 1

This is wrong for three reasons.

Simple equality should be used like id = blah since there's no pattern matching needed
The limit is unnecessary since id is (or should be) a primary key and primary keys are unique
id is (or should be) an integer, not a string and thus should be treated as such. Don't escape it with mysqli_real_escape_string, and don't interpolate it in as a string. Instead, make sure it's an integer format and put it in as an integer. The easiest way to do this is simply casting the variable like " ... WHERE id = " . (int) $locationID"

If you want to be able to actually detect an invalid integer, you can use something like ctype_digit or filter_input with the proper validation flags:

$blah = (isset($_GET['blah']) && is_string($_GET['blah'])) ? $_GET['blah'] : null;
if ($blah === null) {
    echo "You must provide blah!";
} else if (!ctype_digit($blah)) {
    echo "Blah is invalid!";
} else {
    $query = "SELECT blah FROM bleh WHERE id = " . (int) $blah;
    //(The int cast is unnecessary since you already know it's an int. It can protect
    //against a change in the code flow later allowing an invalid value through though)
}

Not only do prepared statements simplify this situation a bit, they can also allow it to be a bit briefer since you don't have to worry as much about validation or careful interpolation/concatenation.

In second.php: inserting a value only to immediately retrieve it is a bit odd. Just use mysqli_insert_id($conn) instead of fetching back the id.

Also, while($id = mysqli_fetch_array($result)) doesn't make sense. Only one row should be returned since the user just created one shortening. Oddly enough, this loop can actually return more than 1 item though since I'm assuming the url column isn't unique. If this is the functionality you desire, ignore the point about not fetching back the row. That is required to bring back all of the duplicates.


Creating a date adder We are thinking about hiring more developers so I was asked to compose a test.  I have been looking online for ideas.  I found this question: How to add n days to a Date in java without importing Date/Calendar from java API?.  I have attempted this myself as follows:

public class Date : IEquatable<Date>
    {
        private readonly static int[] _daysInMonth = { 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };
        private readonly int _month;
        private readonly int _day;
        private readonly int _year;

        public Date(int day, int month, int year)
        {
            this._day = day;
            this._month = month;
            this._year = year;
        }

        //static because thinking about using in constructor for validation.
        public static int MaxDays(int month, int year)
        {
            int maxDays = _daysInMonth[month - 1];
            if (month == 2)
            {
                if ((year % 400 == 0) || ((year % 4 == 0) && (year % 100 != 0)))
                    maxDays = maxDays + 1;
            }
            return maxDays;
        }

        public Date AddDays(int daysToAdd)
        {
            int day = this._day + daysToAdd;
            int month = this._month;
            int year = this._year;
            while (day > MaxDays(month,year))
            {
                day = day - MaxDays(month,year);
                month++;
                if (month > 12)
                {
                    year++;
                    month = 1;
                }
            }
            return new Date(day, month, year);
        }

        public override bool Equals(object obj)
        {
            return Equals(obj as Date);
        }

        public override int GetHashCode()
        {
            unchecked
            {
                int hash = 17;
                hash = hash * 23 + _day.GetHashCode();
                hash = hash * 23 + _month.GetHashCode();
                hash = hash * 23 + _year.GetHashCode();
                return hash;
            }
        }

        public static bool operator ==(Date date1, Date date2)
        {
            if (ReferenceEquals(date1, null) && ReferenceEquals(date2, null))
                return true;
            if (ReferenceEquals(date1, null) || ReferenceEquals(date2, null))
                return false;
            return date1.Equals(date2);
        }

        public static bool operator !=(Date date1, Date date2)
        {
            return !(date1 == date2);
        }

        public bool Equals(Date other)
        {
            if (ReferenceEquals(other, null))
                return false;
            return _day == other._day && _month == other._month && _year == other._year;
        }
    }

I would be grateful for comments.  I reviewed the terms of posting here: https://codereview.stackexchange.com/help/on-topic.  This is real code (no stubs) that works and therefore I believe it is on topic.

I see the point of the commenters: why implement this, if it has already been implemented? However, I still see benefit for such a question in an interview process. Given the right analysis, you can see how the developer thinks and what question he asks, which is IMHO more important than the actual code.

Questions could be:

What coding guidelines shall I follow?
Besides that single line description, do you have acceptance criteria?
Which unit test framework shall I use?
...

public class Date : IEquatable<Date>

Such low level classes use to be sealed. Could apply here as well.

public static int MaxDays(int month, int year)

Why make it public? The purpose of the class is to add days. There's nothing in the task that qualifies this method to be part of the API, IMHO. Make it private instead.

The class hardly useful, because one cannot access the properties of the changed date. There are no accessors for _month, _day and _year.

Comments: do you expect XML-doc comments on public (API) methods? You don't have them.

// [...] thinking about using in constructor for validation.

What would you do if it's invalid? Throw an exception? Well, Microsoft does it ...

Magic number:

if (month == 2)

Introduce a constant February to give it semantics.

I write code in some languages and operator precedence is always an issue. The line

if ((year % 400 == 0) || ((year % 4 == 0) && (year % 100 != 0)))

is hard to understand. A method IsLeapYear() might help you out of that:

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

At the same time it gives sematics to the magic numbers.

Why do we have AddDays() but not SubtractDays()? Can I pass a negative number for subtracting, or is subtracting not allowed?

Your code does not seem to support subtraction, but the data type is still int and not uint.

return new Date(day, month, year);

Do you expect immutability? Why? For performance reasons / multi threading? Is this premature optimization? Immutability comes at the cost of garbage collection.

Magic number:

if (month > 12)

What if the interviewed person uses _daysInMonth.Length? Is that better or worse than your implementation?

Hash codes etc.: do you provide an IDE which can generate equality members (like Jetbrains R#), or do you expect the developer to come up with that prime stuff by himself?

I have never written such code myself and actually I don't want to work in a company that expects me to write such code (except I'm applying to some security company which has to deal with hashes and encryption).


Is this Scala/Akka idiomatic and correct for Akka 2.1? I'm wondering if there is anything i'm overlooking in the code below. I've taken the Pi calculation example, simplified it and converted it to Akka 2.1.

More specifically, i use the ask pattern to replace the Listener actor.

So is this

A proper and valid conversion?
A reasonable approach?
And is there a better approach to waiting for a result from an optionally distributed parallel reduction of a collection? In other words, is this an Akka equivalent of collection.par.map(work).par.reduce(result) or is there a better way?

object Work extends App {
  // This is just some collection of data to process (possibly remotely).
  val collection = 1 to 10000
  val start = System.currentTimeMillis
  val system = ActorSystem(f"Work${start}%08x${hashCode}%04x")
  // The collection is converted into a stream since it could
  // potentially be huge.
  val master = system.actorOf(Props(new Master(collection.toStream)), name = "master")

  // This replaces what the Listener was doing, i think.
  implicit val timeout = Timeout(12.seconds)
  val result = Await.result(master ? Run, timeout.duration)
  println(s"\n${result}")
  system.shutdown()

  sealed trait WorkMessage
  case object Run extends WorkMessage
  case class Work(start: Int) extends WorkMessage
  case class Result(value: Double) extends WorkMessage

  sealed trait ControlMessage
  case class Complete(elapsed:Long) extends ControlMessage

  class Worker extends Actor {
    def doWork(element: Int): Double = {
      print(s" ${element}")
      return element.toDouble
    }
    def receive = {
      case Work(element) ⇒
        sender ! Result(doWork(element))
    }
  }

  class Master(work: Seq[Int]) extends Actor {
    val nrOfCPUs = Runtime.getRuntime().availableProcessors();
    val nrOfWorkers = nrOfCPUs * 4
    val workSize = work.size
    var nrOfResults: Int = _
    val workerRouter = context.actorOf(
      Props[Worker].withRouter(RoundRobinRouter(nrOfWorkers)), name = "workerRouter")

    var done: ActorRef = _
    def receive = {
      case Run ⇒
        // Save the sender for responding to the ask once all the work is processed.
        done = sender
        work.foreach(element ⇒ workerRouter ! Work(element))
      case Result(value) ⇒
        nrOfResults += 1
        if (nrOfResults >= workSize) {
          done ! Complete(System.currentTimeMillis - start)
          context.stop(self)
        }
    }

  }

}

Here's a gist with the full code and a build.sbt to try:

curl -ks https://gist.github.com/nicerobot/4751388/raw/run.sh | sh

I'm not an akka expert but I did used akka in a project so these are just some of my observations:

result is blocking.

val result = Await.result(master ? Run, timeout.duration)

On case the master takes longer due to load or whatever, the pending result will
exceed the timeout. Try async ask directly.

val result ask(actor, msg).mapTo[String]

In order to remain async you may carry on with an onComplete as
Future callback:

result onComplete {
    case Success(result) => doSomethingOnSuccess(result)
    case Failure(failure) => doSomethingOnFailure(failure)
}

In this case no operation ever blocks the running thread. Read the Akka docs for more details

  And is there a better approach to waiting for a result from an
  optionally distributed parallel reduction of a collection? In other
  words, is this an Akka equivalent of
  collection.par.map(work).par.reduce(result) or is there a better way?

Operating on distributed data-sets is much better done on an in-memory data processing engine. Using the distributed word count as example, in Spark you do:

   file.flatMap(line => line.split(" "))
    .map(word => (word, 1))
    .reduceByKey(_ + _)

Actors are made for message processing such as Twitter or WhatsApp but for  computing tasks that must be fast, you use either MapReduce (Hadoop) for offline crunching of Spark/Storm for real-time processing.  Hope that answers your question.


Unit test for a password-change request I'm studying unit testing in swift and I would like to know if I'm doing the right way.

Here I have the model

struct ChangePasswordRequest: Encodable {
    var email: String
    var newPassword: String
    var token: String
}

And here the unit test

import XCTest
@testable import MyProject

class ChangePasswordRequestTests: XCTestCase {

    var changePasswordRequest: ChangePasswordRequest!

    override func tearDown() {
        // Put teardown code here. This method is called after the invocation of each test method in the class.
        changePasswordRequest = nil
        super.tearDown()
    }

    func testChangePasswordRequest() {

        // given
        let email = "abc@def.com.br"
        let newPassword = "abc123"
        let token =
        """
        rF1rHbYVCpcfyo6K2e7Q68QvKYdeOMt0vDXYehggGU3kx8XiuGmQylXabxbGAyUmSK9RnbL
        """

        // when
        changePasswordRequest = ChangePasswordRequest(email: email, newPassword: newPassword, token: token)

        let requestJSON = """
        {"email":"\(email)","token":"\(token)","newPassword":"\(newPassword)"}
        """

        changePasswordRequest = ChangePasswordRequest(email: email, newPassword: newPassword, token: token)
        guard let encodedRequest = try? JSONEncoder().encode(changePasswordRequest) else {
            XCTFail()
            return
        }

        // then
        XCTAssertEqual(String(data: encodedRequest, encoding: .utf8)!, requestJSON, "Any JSON key differs from model.")

    }

}

I never worked with unit test. It's possible to improve this test?

Some hints on Unit tests.

Use dependency injection to make testing easier
Create mocks to eliminate all external dependencies in tests
Test units of code (functions, methods, classes)

Example of testable class:

class UserManager: UserManagerType {
 let database: DatabaseType
 init(database: DatabaseType) {
  self.database = database
 }

 func getUser(withId id: String) -> User? {
    return database.getItem(withPredicate: "id == \(id)")
 }

 func getAdmins() -> [User] {
    return database.getItems(withPredicate: "type == admin")
 }
}

protocol DatabaseType {
   func getItems(withPredicate predicate: String) -> [User]
   func getItem(withPredicate predicate: String) -> User?
}

Tests:

// create mock for database.
// Try to avoid any external dependencies in tests
class DatabaseMock: DatabaseType {
 var itemsToReturn: [User] = []
 var userToReturn: User?

 func getItems(withPredicate predicate: String) -> [User] {
   return itemsToReturn
 }

 func getItem(withPredicate predicate: String) -> User? {
   return userToReturn
 }
}

// actual test case
class UserManagerTests: XCTestCase {
  var database: DatabaseMock = DatabaseMock()
  var manager: UserManager!
  // setup before each test
  func setUp() {
    database = DatabaseMock()
    manager = UserManager(database: database)
  }

  func testReturnsCorrectUserForId() {
    let expectedUser = User(id: "123")
    database.userToReturn = expectedUser
    let actualUser = manager.getUser(withId: "123")
    XCTAssertNotNil(actualUser)
    XCTAsseerEqual(actualUser.id, expectedUser.id)
  }

  // other tests ...
}

This is not the best example (because UserManager is just a wrapper around database) but I hope you have got some ideas of how write unit tests


Data type detection for a CSV file parser I am building a CSV file parser, and in order to get the appropriate Object to represent different data types found on the parsed files, I wrote the following function:

public static Object stringToDataType(String valueAsString) throws ParseException{
    // detections ordered by probability of occurrence in Buffer_Bank.
    String decimalPattern = detectDecimal(valueAsString);
    if(decimalPattern != null){
        return stringToBigDecimal(valueAsString, decimalPattern);
    }else{
        String integerPattern = detectInteger(valueAsString);
        if(integerPattern != null){
            return stringToBigInteger(valueAsString);
        }else{
            String datePattern = detectDate(valueAsString);
            if(datePattern != null){
                return stringToDate(valueAsString, datePattern);
            }else{
                // value is a String... nothing else to do!
                return valueAsString;
            }
        }
    }
}

In order to call the appropriate 'casting' methods (stringToDate, stringToBigInteger, stringToBigDecimal), I need to use the specific pattern string that gets returned when detecting if a particular value is of that type.
Hence, the pattern value gets used for routing to the appropriate function, and also as an argument in that function call.

To make this method logic 'flatter', I could avoid passing the pattern as an argument, and calling the pattern detection method (detectDecimal, detectInteger, detectDate) again from within the 'casting' method, but that would lead to parsing the same value two times, which seems wasteful at best.

So, is this the best way this method can be written without getting to call the detection methods twice?

EDIT

Here are the detectDate and stringToDate methods, as requested (the methods for BigInteger and BigDecimal are very similar). Please let me know if I need to post anything else:

private static String detectDate(String dateString) {
    for (String regexp: DATE_FORMAT_REGEXPS.keySet()) {
        if(dateString.toLowerCase().matches(regexp)) {
            return DATE_FORMAT_REGEXPS.get(regexp);
        }
    }
    return null; // value is not a date
}

public static Date stringToDate(String valueAsString, String format) throws ParseException{
    SimpleDateFormat dateFormat = new SimpleDateFormat(format);
    Date date = dateFormat.parse(valueAsString);
    return date;
}

Yes, you can make that code a lot more readable by noticing a simple fact. When you have the code

String decimalPattern = detectDecimal(valueAsString);
if(decimalPattern != null){
    return stringToBigDecimal(valueAsString, decimalPattern);
}else{
    // ...
}

The else part is actually unnecessary. If the test is true then we return with a value. This means that you exit early from the method, without going further down.

So you can rewrite it like this:

String decimalPattern = detectDecimal(valueAsString);
if(decimalPattern != null){
    return stringToBigDecimal(valueAsString, decimalPattern);
}
// ...

As you found out, this has the advantage that the subsequent code is shifted by one indentation to the left, making it easier to read. So only with this change, the code now looks like:

public static Object stringToDataType(String valueAsString) throws ParseException {
    // detections ordered by probability of occurrence in Buffer_Bank.
    String decimalPattern = detectDecimal(valueAsString);
    if (decimalPattern != null) {
        return stringToBigDecimal(valueAsString, decimalPattern);
    }
    String integerPattern = detectInteger(valueAsString);
    if (integerPattern != null) {
        return stringToBigInteger(valueAsString);
    }
    String datePattern = detectDate(valueAsString);
    if (datePattern != null) {
        return stringToDate(valueAsString, datePattern);
    }
    return valueAsString;
}

You posted an example of your detect* and stringTo* methods. In fact, you don't need two methods. You only need a single one that is specific to the current type you want to test. What you want is:

Try to interpret the value as a decimal and return it;
If not, try to interpret the value as an integer and return it;
If not, try to interpret the value as a date and return it.

Said like this, it is possibe to realize that you only need 3 methods: tryDate, tryInteger and tryDecimal. Each of those will do the necessary to try to interpret the String and return null if they can't. Basically, the code you currently have inside stringTo* should be merged into the part where you found the correct pattern in detect*. As an example:

private static Object tryDate(String dateString) {
    for (Map.Entry<String, String> entry : DATE_FORMAT_REGEXPS.entrySet()) {
        if (dateString.toLowerCase().matches(entry.getKey())) {
            SimpleDateFormat dateFormat = new SimpleDateFormat(entry.getValue());
            try {
                return dateFormat.parse(dateString);
            } catch (ParseException e) {
                // what to do in this case? Possibilites: throw an exception or return null
            }
        }
    }
    return null; // value is not a date
}

Note that I changed your code so that it loops directly around the entries instead of looping just with the keys. You should also decide what to do in case of a ParseException. You could rethrow a custom runtime exception, wrapping the original, or returning null to signal that the attempt failed.

The big issue is that there is some duplicated logic in this: each time we are detecting a pattern and, if it's not null, applying it to the value. If you were to add more detection algorithm, this method can become very quickly copy/pasted code and clumsy. We need to refactor that and make it generic.

Using Java 8, you can define that as a list of Function.

private static final List<Function<String, Object>> FUNCTIONS =
    Arrays.asList(s -> tryDecimal(s), s -> tryInteger(s), s -> tryDate(s));

The goal of those functions is to convert a String into a Object. Here, we define 3 which are the 3 methods mentioned above. Then we can have

public static Object stringToDataType(String valueAsString) {
    return FUNCTIONS.stream()
                    .map(f -> f.apply(valueAsString))
                    .filter(Objects::nonNull)
                    .findFirst()
                    .orElse(valueAsString);
}

This creates a Stream pipeline over the functions. Each of them is applied to the given String, non-null elements are filtered and only the first one is kept. Effectively, this will select the first non-null element, so it will select the first mapping that matched. If all are null then we return a default value with orElse which is the given value as String.

If somehow you want to keep your current approach with two different methods (one for detecting the pattern and one for parsins), then what you want comes down to creating a custom class that will hold:

A function that returns the pattern detected given the value as String;
A function that returns the object given the value as String and a pattern.

Simply, it could be:

public final class Conversion {

    private final UnaryOperator<String> patternDetector;
    private final BiFunction<String, String, Object> toObjectFunction;

    public Conversion(UnaryOperator<String> patternDetector, BiFunction<String, String, Object> toObjectFunction) {
        this.patternDetector = patternDetector;
        this.toObjectFunction = toObjectFunction;
    }

    public UnaryOperator<String> getPatternDetector() {
        return patternDetector;
    }

    public BiFunction<String, String, Object> getToObjectFunction() {
        return toObjectFunction;
    }
}

Then we can make a static utility that applies a given Conversion to a given value:

private static Object stringToDataType(String valueAsString, Conversion conversion) {
    String pattern = conversion.getPatternDetector().apply(valueAsString);
    if (pattern != null) {
        return conversion.getToObjectFunction().apply(valueAsString, pattern);
    }
    return null;
}

and finally, we can refactor the main method:

private static final List<Conversion> CONVERSIONS = Arrays.asList(
    new Conversion(v -> detectDecimal(v), (v, p) -> stringToBigDecimal(v, p)),
    new Conversion(v -> detectInteger(v), (v, p) -> stringToBigInteger(v)),
    new Conversion(v -> detectDate(v), (v, p) -> stringToDate(v, p))
);

public static Object stringToDataType(String valueAsString) throws ParseException {
    return CONVERSIONS.stream()
                      .map(c -> stringToDataType(valueAsString, c))
                      .filter(Objects::nonNull)
                      .findFirst()
                      .orElse(valueAsString);
}

Now, if we want to add more conversion in the future, we just need to update this list that, arguably, should be made a constant instead of being initialized each time. The Stream pipeline will return the first non-null element and will short-circuit once it has been found. If all are null, there we return the default value which is the given value as string.


Logging leads when interactions happen for a contact I have been trying to get as close to SRP in my code as possible. I have a situation where I need to log leads when interactions happen for a contact. A lead may be created or updated. There are three different states that I would like to set leads to.

Active - new interested interactions causes active lead to be created
Converted - a existing lead is updated to converted if the product was purchased - this is managed with conversion flag
Stale - a existing lead is updated to stale if the contact is not interested

class LeadGeneration

  def initialize contact_id, options={}
    @lead = Lead.find_or_initialize_by(contact_id: contact_id)

    options.reverse_merge!(conversion: false, interested:true)
    @conversion = options[:conversion]
    @interested = options[:interested]
  end

  def call
    if conversion?
      convert_lead
    else
      process_lead
    end
  end

  private

  def lead
    @lead
  end

  def conversion?
    @conversion
  end

  def interested?
    @interested
  end

  def convert_lead
    lead.converted! unless lead.new_record?  # new leads cannot be saved as converted
  end

  def process_lead
    if interested?
      activate_lead
    else
      stale_lead
    end
  end

  def activate_lead
    lead.active!
  end

  def stale_lead
    lead.stale! unless lead.new_record? # new leads cannot be saved as stale
  end
end

Right now I feel like I should be splitting up converting a lead and managing stale/active states.  I'm not really sure what to do and tests are a little confusing right now.

I'm not sure how you intend to use this class, but the call method is very fishy. This method seems to do one of three things: convert_lead, activate_lead or stale_lead. The choice of which of them should actually be executed is made in the options argument during object construction.

This is not very easy to understand from reading the code, and I can't really see the reasoning behind it. Is it an implementation of a Command pattern? that is not very ruby-like.

Since all this class actually does is quite basic delegation (@lead.converted!, @lead.stale! or @lead.active!) I would consider foregoing this class altogether, and simply doing the action. It is unnecessarily long and convoluted, and hides more than explains what it does.

From your comment below I understand that LeadGeneration should be a Service Object. It is true that service objects may have a single method called call, as is suggested in this blog post:

  Service objects are responsible for retrieving and/or manipulating
  data — essentially any work that needs to be done that you might have
  put in a controller or model object before. They typically only have
  one method, which I like to define as “#call” (they're usually named
  something like PictureCreationService, so naming the method #create
  would seem redundant).

But note, though, the reason a single method call is allowable - it is apparent from the class name exactly what the method should do.
If your service object was called LeadConversionService, then a call method would be good enough.

Being a service object, it is even more important than usual in ruby for the class to be small and transparent.

If you want to go to the extreme SRP suggested in the quote above, you'd have three classes, each with #call and #allowed? methods:

class LeadConversionService
  def initialize(lead)
    @lead = lead
  end

  def call
    @lead.converted!
  end

  def allowed?
    !@lead.new_record?
  end
end

OR, you could be a little less extreme, and decide the LeadGeneration (or maybe LeadLifeCycleService?) is a good enough scope, and let it have three method:

class LeadLifeCycleService
  def initialize(lead)
    @lead = lead
  end

  def convert!
    raise LifeCycleException if @lead.new_record?
    @lead.convert!
  end

  def activate!
    @lead.active!
  end

  def stale!
    raise LifeCycleException if @lead.new_record?
    @lead.stale!
  end
end

Writing this solution makes it obvious again, that this service doesn't do much, since it only delegates (so it doesn't even save code in your model...). All it does is validates pre-conditions to your actions, which may be better handled by a state-machine, as @Flambino suggested.


Tips to refactor code : very repetitive code I have this very simple problem : my code is too repetitive. It works, but it could be much cleaner. I don't know exactly where to start.

I'd like to learn to work better, and I'd really like someone to help me with it.

Here's the 2 bits of my code that are very repetitive (everything I've done myself) :

$(document).ready(function () {

  // Setup your Lazy Line element.
  // see README file for more settings
  $('#drawing1').lazylinepainter({
      'svgData': dr1,
      'strokeWidth': 1.2,
      'strokeColor': '#65615a',
      'onComplete': function () {

      }
    })

  $('#drawing2').lazylinepainter({
      'svgData': dr2,
      'strokeWidth': 1.2,
      'strokeColor': '#65615a',
      'onComplete': function () {

      }
    })

  $('#drawing3').lazylinepainter({
      'svgData': dr3,
      'strokeWidth': 1.2,
      'strokeColor': '#65615a',
      'onComplete': function () {

      }
    })

  $('#drawing4').lazylinepainter({
      'svgData': dr4,
      'strokeWidth': 1.2,
      'strokeColor': '#65615a',
      'onComplete': function () {

      }
    })

  $('#drawing5').lazylinepainter({
      'svgData': dr5,
      'strokeWidth': 1.2,
      'strokeColor': '#65615a',
      'onComplete': function () {

      }
    })

  // Paint your Lazy Line, that easy!

})

})(jQuery);

Second part of the code:

var eventsFiredDr1 = 0;

var drawing1 = function () {
  if(eventsFiredDr1 == 0) {

    $('#drawing1').lazylinepainter('paint');

    eventsFiredDr1++; // <-- now equals 1, won't fire again until reload
  }
}

var eventsFiredDr2 = 0;

var drawing2 = function () {
  if(eventsFiredDr2 == 0) {

    $('#drawing2').lazylinepainter('paint');

    eventsFiredDr2++; // <-- now equals 1, won't fire again until reload
  }
}

var eventsFiredDr3 = 0;

var drawing3 = function () {
  if(eventsFiredDr3 == 0) {

    $('#drawing3').lazylinepainter('paint');

    eventsFiredDr3++; // <-- now equals 1, won't fire again until reload
  }
}

var eventsFiredDr4 = 0;

var drawing4 = function () {
  if(eventsFiredDr4 == 0) {

    $('#drawing4').lazylinepainter('paint');

    eventsFiredDr4++; // <-- now equals 1, won't fire again until reload

  }
}

var drawing5 = function () {
  if(eventsFiredDr5 == 0) {

    $('#drawing5').lazylinepainter('paint');

    eventsFiredDr5++; // <-- now equals 1, won't fire again until reload

  }
}

$(window).resize(function () {
  if($(window).width() < 820) {

    $("svg").hide()
    $(".fallback").show()
    $("#drawing2").css("height", "auto")
  }
});

if($(window).width() > 820) {

  $(".fallback").hide()

  $(window).scrollStopped(function () {
    if($("#drawing1").is(":within-viewport")) {
      drawing1()
    }

    if($("#drawing2").is(":within-viewport")) {
      drawing2()
    }

    if($("#drawing3").is(":within-viewport")) {
      drawing3()
    }

    if($("#drawing4").is(":within-viewport")) {
      drawing4()
    }

    if($("#drawing5").is(":within-viewport")) {
      drawing5()
    }

  });

( I know I should put DOM elements in variables, but I planned on doing it when I would refactor the code. )

My first guess would be creating an array but.. No idea how to use it in my code. (I'm a big beginner)

For the code repetition you can use

$(function () { // shorter form of document-ready
    $.each([dr1, dr2, dr3], function (index, data) {
        $('#drawing' + (index + 1)).lazylinepainter({
            svgData: data,
            strokeWidth: 1.2,
            strokeColor: '#65615a',
            onComplete: function () {}
        });
    });
});

For the second part, use boolean values as Josay suggested but place the fired flag inside a function that returns a closure. Each call to createPaintGuard establishes its own fired flag without having to package them all into an array or object.

function createPaintGuard(index) {
    var fired = false;
    return function () {
        if (!fired) {
            $('#drawing' + index).lazylinepainter('paint');
            fired = true;
        }
    };
}

var drawing1 = createPaintGuard(1);
var drawing2 = createPaintGuard(2);
var drawing3 = createPaintGuard(3);

Note that I would probably store the three drawingX variables into an array instead of individually-named variables, but I have no idea how they're being used in the wider context. The code here should be a drop-in replacement for yours.


Shopping Cart Handler Class I've written an eCommerce shopping cart handler class in PHP. Please review the code and inform me of any errors and/or badly written code and/or badly handled logics. Please also inform me of any missing functionality or security loopholes.

class Cart extends Sessions{

protected $cartProducts = array();
protected $dbLink;
protected $cartName;

public function __construct(){

    parent::__construct(); // to run the constructor of parent
    $this->cartName = 'cart';
    try{
        $this->dbLink = new db;
    }
    catch( Exception $e ){
        echo $e->getMessage() , '\n';
    }
}

public function __destruct(){
    $this->dbLink = FALSE;
}

public function cart_exist(){
    return $this->session_exist($this->cartName);
}

public function create_cart(){
    $this->create_session($this->cartName, true);
    $this->cartProducts = &$_SESSION[$this->cartName];
}

public function add_product( $data ){
    if( !$this->cart_exist() ){
        $this->create_cart();
    }
    $this->insert_value($this->cartName, $data );
}

public function display_cart(){
    $this->display_session($this->cartName);
}

public function del_from_cart( $index ){
    unset($_SESSION[$this->cartName][$index]);
}

public function update_cart_item( $index , array $data ){
    if( array_key_exists( $index , $_SESSION[$this->cartName]) ){
        $arr = &$_SESSION[$this->cartName][$index];
        foreach( $data as $key => $value ){
            $arr[$key] = $data[$key];
        }
    }
}

public function remove_cart(){
    $this->remove_session($this->cartName);
}

}

There are a few things:

Comments like this one:

    parent::__construct(); // to run the constructor of parent

provide no real value; they just repeat what the code does.  Prefer reserving comments
for tricky bits of code that aren't self-explanatory.
You never use $dbLink; get rid of it. If subclasses need a database, let them
create one.
__destruct is largely unnecessary here.  You gain nothing from detaching references at
this point; PHP will already release them once your destructor returns.  And if you
followed the previous point, the only thing you're releasing no longer exists anyway.
All your variables are protected, which implies that you are planning for subclasses to
exist.  I'm not sure specializing a cart even makes much sense, though.

Unless you already have subclasses, and those subclasses need basically
unrestricted access to those properties, you should make them private.
(And even if you do, you might want to reexamine whether inheritance is the proper
relationship.)
You have a public constructor and a "create_cart" function?  That's going to confuse
people.  (You already created a Cart when you said new Cart.)  It looks like what you
need here is a name that more clearly describes what the function does.
I'm not a big fan of instance method names that include the containing class's name.
It feels like a symptom (or maybe a cause) of drifting away from OO concepts and back
to proceduralism.

This is just personal preference; take it as you will.  But IMO, if you have a Cart,
then from now until it dies, it will always be a Cart.  You don't have to keep
reminding it of that.  :)
I see that $cartProducts is a reference to an array in $_SESSION.  At that
point, both names refer to the exact same value.  Any change you make to
$this->cartProducts also happens to $_SESSION[$this->cartName], and vice versa.

Which makes me wonder: Why do so many methods mess with
$_SESSION[$this->cartName] rather than with $this->cartProducts?
Variable names like $data aren't very descriptive.  You know a product is being
added; most carts would include a quantity as well.  But i don't know for sure what
$data looks like, for two big reasons.  One is the name.  The other...?
You let the caller stuff whatever garbage it wants to in your cart.

A cart keeps track of items (or products), and typically quantities as well,
and usually has a way to get a total price.  This cart is basically a list of blobs,
and can't currently do any of that stuff easily, because it doesn't exert any type of
ownership.

The list of items is your domain, not the caller's.  Unless this is intended to be a
generic list of "whatever", you decide the structure of what goes into it -- and in
return, you get the ability to do stuff over and above what a stock list can do.
Otherwise, the caller is actually worse off using your cart than mucking around with
the $_SESSION['cart'] array directly.  (At least when they're using $_SESSION,
they know they're farting around with global state.  Your class hides that fact behind
what looks like a per-instance interface.)

You don't necessarily need to say "everything you insert must be an instance of this
particular class".  You could conceivably let the caller decide that much.  But you can
-- and probably should -- require that they exhibit certain behaviors (like having a
get_price() method).  These requirements should be documented.  And if you're
tracking quantities, then do so explicitly -- consider making $quantity a discrete
parameter to the add_product method.

Once you have decided how an item must behave, then you can do stuff that a typical
cart does out of the box.  If you decide items must have a price, for example, then
now you can add a way to get the total price of all items.   That in itself is part
of the bare minimum functionality i expect of any shopping cart class.  If it doesn't
have that, it's just another list.  :P

But the really big thing i'm seeing here is the relationship between carts and sessions.  A Cart might have a session, or use a session.  But it is really a different thing.  Inheriting from Sessions seems to me incorrect, unless the parent class is badly named.

It feels to me, though, like you have the relationship the wrong way around.  A Cart is obviously part of a session, as evidenced by your bunch of functions to manage subsets of session data.

And PHP lets you put objects in a session.

The cart itself could be session data, at which point $session->gimme('some cart name')
can return a whole Cart object, and all this session-management stuff becomes obsolete.

In order to make it work:

The class needs to be loaded, or autoloadable, before session_start().
The class must be serializable...which requires that anything you need to
persist must also be serializable, and/or that anything that's not serializable
not be included in the list of properties to store.
That's usually not a big deal; the only things PHP can't serialize are resources and
quasi-resources like your $dbLink.
(One of a number of reasons $dbLink probably isn't worth keeping.  But even if you
do, you can add a __wakeup() function to reinitialize it.)

The main thing to keep in mind is that if you only want the item info serialized, or you have unserializable fields (like $dbLink), you should provide a __sleep method that tells PHP which properties to serialize.  You'll probably also want a __wakeup() method to reinitialize those items that you didn't serialize.

With all that taken care of...what's left?

class Cart {
    private $items = [];

    public function add_product($product, $quantity) {
        $this->items[] = ['product' => $item, 'quantity' => $quantity];
    }

    public function delete_product_at($index) {
        unset($this->items[$index]);
    }

    public function update_product_at($index, $product, $quantity) {
        if (isset($this->items[$index])) {
            $this->items[$index] = ['product' => $product, 'quantity' => $quantity];
        }
    }

    public function get_total() {
        return array_reduce($this->items, function($sum, $item) {
            return $sum + $item['product']->get_price() * $item['quantity'];
        }, 0);
    }

    # don't really need __sleep or __wakeup, since $dbLink is gone.  But it might be
    # helpful to illustrate their use.
    private $some_value;

    public function __sleep() {
        # in `__sleep`, you return a list of properties you want saved
        return ['items'];
    }

    public function __wakeup() {
        # since $some_value wasn't serialized, it doesn't have a value.
        # before you use it again, set it to something sensible.

        $this->some_value = 42;
    }
}

The main missing piece left at this point is your display_cart function.  Not sure what it does...but frankly, if its name is accurate, it shouldn't be here.  :P

This is a model class, or a business object if you don't care about MVC and such.  It's responsible for managing the data, and for representing it in a way that the UI can do something with it. But most schools of thought on software architecture would say this class shouldn't be involved in the actual presentation/rendering.  That's the job of the code that manages the UI.

If you want to be able to list the cart's contents, two simple ways would be to have a function that returns $this->items, or implement the IteratorAggregate interface and have a getIterator() function return an iterator over $this->items.


High Performance Calculation of Sums and Returns in Go I am writing a finance library with the intention to have the best of Golang's performance.

The function Re calculates the returns of a float list, and function Sum calculates the sum of float list.

//Calculate returns
func Re(data []float64) []float64 {

    re_ := make([]float64, len(data))
    for i := 1; i < len(data); i++ {
        re_[i] = ( (data[i]/data[i-1])-1)
    }

    return re_
}

// sum of float64 list
func Sum(floatList []float64) float64 {

    var totalx float64
    for _, valuex := range floatList {
        totalx += valuex
    }

    return totalx
}

is there any further possible optimisation on these two functions or another more performant way to write them?

In Go, when you want to measure function performance, use the Go testing package to run a benchmark.

For example,

returns.go:

package finance

// Calculate returns
func Re(data []float64) []float64 {
    var re []float64
    if len(data) > 0 {
        re = make([]float64, len(data))
    }
    for i := 1; i < len(data); i++ {
        if data[i-1] != 0 {
            re[i] = (data[i] / data[i-1]) - 1
        }
    }
    return re
}

// Calculate sum
func Sum(data []float64) float64 {
    var sum float64
    for _, x := range data {
        sum += x
    }
    return sum
}

returns_test.go:

package finance

import "testing"

func BenchmarkRe(b *testing.B) {
    data := make([]float64, 5*12)
    for i := range data {
        data[i] = float64(i)
    }
    b.ResetTimer()
    for N := 0; N < b.N; N++ {
        Re(data)
    }
}

func BenchmarkSum(b *testing.B) {
    data := make([]float64, 5*12)
    for i := range data {
        data[i] = float64(i)
    }
    b.ResetTimer()
    for N := 0; N < b.N; N++ {
        Sum(data)
    }
}

Output:

$ go test returns.go returns_test.go -bench=. -benchmem
BenchmarkRe-4     7545566   156 ns/op     480 B/op    1 allocs/op
BenchmarkSum-4   28272992    41.5 ns/op     0 B/op    0 allocs/op
$

I looked at some other alternatives, but that's the fastest.

Note: I fixed your divide-by-zero bug.


Space and time efficient Algorithm for generating prime numbers I have developed an algorithm for generating prime numbers (which turns out to be an optimized version of the Sieve of Eratosthenes) by reducing the space by a factor of 64.

/*
 * Explanation of algorithm

 * 1. Avoid storing all the even numbers as they cnt b prime numbers - so divide the size by 2

 * 2. Represent each bit of int as a odd number 1,3,5,7 and so on. Each int has 32 bits and will store prime numbers between 0 to 63.

 * 3. Again divide the counter by 32, so now the final size of the array would be 32*2=64 (because we are also avoiding even numbers)

 * 4. 1st Loop (i)-  Loop from i=3 upto squareroot of the given range and increment by 2(because we don't care about the even numbers)

 * 5. 2nd Loop (j)- Loop from square of the current number(i) till its less than the counter and increment j by 2*i (Again avoid the even number multiples)

 * 6. Mark all the multiples of the j to 1/true.(In our code we set the corresponding bit to 1)
 for e:g for setting 9 to be non-prime we simply divide it by 2 and set that bit to 1.

 * 7. While printing we check if the bit is set to 0. if yes, then print the number by multiplying (32*[currentIndexInArray]+bit-position)*2 +1.

 * Note: it will correctly print all numbers from 3 to given range(should be multiples of 64)

 *  Even if its not multiple of 64, it still works but wont consider the last 32 numbers in the range.

 *  Also, it prints 1 in the output and not 2, which probably can be handle by extra conditions.
 *  its more focused on generating primes above number 3.
 */

public class PrimeNumberOptimized {
    static int range = 256;

    public static void main(String[] args) {

        long startTime = System.currentTimeMillis();

        int[] NonPrimeNumbers = new int[(range>>6)+1];

        int k=0;
        for(int i=3;i*i <= range;i+=2){
            //Increment k, everytime i goes beyond int size
            if((i>>1)%32==0)
                k++;

            //Check if it is a prime number, if yes, mark all the bits which are multiples of this number.
            if((NonPrimeNumbers[k]&(1<<(i>>1)))==0)
            {
                for(int j=i*i;j <= range;j+=2*i){// increment j by 2*i, for :eg if i=5, then mark directly 15 skipping 10 as every alternate multiple will be an even number
                    //set the bit to 1. if i=3 and j=9, then we go to the 1st index in array and set the 9/2th bit in array value.
                    NonPrimeNumbers[j>>6]|=1<<(j>>1);
                }
            }
        }

        int primeCnt = 0;
        range >>= 6; // Divide the range by 64 as we need to iterate through the array which is reduced by factor of 64.

        for(int arrayIndex=0;arrayIndex < range;arrayIndex++){
            for(int bitPosition=0;bitPosition<32;bitPosition++) // Go through each bit of the array value
            {
                if((NonPrimeNumbers[arrayIndex]&(1<<bitPosition))==0) // check if the current bit is set to 0, if yes, then it represent prime number.
                {
                    System.out.println(((32*arrayIndex+bitPosition)*2)+1); // multiply by 32 to get the correct index in array, add the bitPosition to it and multiply by 2(again we are avoiding even numbers)
                    primeCnt++;
                }
            }
        }

        System.out.println("-Total no of Primes generated->"+(primeCnt));
        System.out.println("Total time ="+(System.currentTimeMillis()-startTime));
    }

}

Please have a look into this and let me know if it can be optimized any further. Compared with most of the algorithms found online and so far it's quite fast compare to other algorithms.

Coding style

First, let me comment on your coding style / readability:

NonPrimeNumbers does not follow Java Naming Conventions. Local variables should start with lower case.

Your variable names are simply bad.

i is the candidate prime number, so its name should reflect that, e.g. candidatePrime.
k should become arrayIndex (as you did when printing the results).
j should become multiple.
When finding the primes, you should also use the variable name bitPosition instead of (i>>1)%32.

Don't reuse variables for two different meanings, as you did with range. Use two different ones, e.g. primesRange and arrayRange.

At mostly all places where you added comments, introduce method calls instead of one big ugly main() method.

(NonPrimeNumbers[k]&(1<<(i>>1)))==0 then should read isPrime(candidatePrime) or isPrime(arrayIndex,bitPosition).
NonPrimeNumbers[j>>6]|=1<<(j>>1) should become setNonPrime(multiple).
and so on.

Of course, NonPrimeNumbers then has to become an instance field instead of a local var.

Performance

You seem to be looking for performance. I won't do a detailed profiling of your program, that task is up to you. But you can check some modifications that might speed up your program (esp. for bigger prime numbers):

Instead of the single for(int i=3;i*i <= range;i+=2) loop you could try two nested loops, with an intermediate check if a prime number is possible:

for (int arrayIndex=0; arrayIndex<arrayRange; arrayIndex++) {
    if (nonPrimeNumbers[arrayIndex] != 0xffffffff) {
        for (int bitPosition = 0, pattern = 1;
             bitPosition < 32;
             bitPosition++, pattern <<= 1) {
            ...
        }
    }
}

Maybe you can mostly do without the candidatePrime variable, just with arrayIndex and bitPosition, only using it when you found a prime.

You have to try what gives you the best performance...


Number of prime numbers between 1 and n I came up with the following solution to find the number of prime numbers from 1 to n.

I'm wondering if there is a more optimal way.

var max = 10;
var compositeNumbers = {};

mainLoop:
for (var i=2; i<= max;i++)
{
  smallLoop:
   for (var j=2; j<=Math.ceil(Math.sqrt(i));j++)
   {
      if (i % j == 0)
      {
          compositeNumbers[i]=1;
          continue mainLoop;
      }
   }

}

console.log(max - Object.keys(compositeNumbers).length);

I've created the following performance tests to test the changes: http://jsbin.com/EqiKini/3

I'm going to start out this review by pointing out your algorithm is slightly off! Take a look at compositeNumbers after your code above and you'll see it is: {2: 1, 4: 1, 6: 1, 8: 1, 9: 1, 10: 1}. Clearly 2 is not composite but it will normally work because you exclude 1. The way I would address this is by doing a check if max is less than 3.

function findNumPrimes(max) {
  if(max <= 1) return 0;
  var compositeNumbers = {1: 1}; //1 is composite
  //if (max > 3) {//in retrospect no need for this pre condition as it will be handled by the loop
  mainLoop: for (var i = 4; i <= max; i++) {
    smallLoop: for (var j = 2; j <= Math.ceil(Math.sqrt(i)); j++) {
      if (i % j === 0) {
        compositeNumbers[i] = 1;
        continue mainLoop;
      }
    }
  }
  return max - Object.keys(compositeNumbers).length;
}

As you'll notice I slightly adapted your code so you would use it like console.log(findNumPrimes(10000)); //1229

The first notable (and obvious) speed improvement I'll point out is you should probably cache Math.ceil(Math.sqrt(i)) in a variable (2 times + speed improvement for max > 100).

An even better implementation would probably make use of the ideas from the Sieve of Eratosthenes. I added a case to the performance tests using the sieve find prime number algoritm posted here.


Program for helping people practice their maths Please mention anything that could help me.

from easygui import *
import random

choice = ["+", "-", "x"]
question_number = 1
correct = 0
msgbox("This program is to help you get better at doing maths, you will get given a random set of random questions including +, - and x , at the end the program will tell you how many right you got out of 10.")
while question_number <=10:
        question = random.choice(choice)
        num_1 = random.randint(1,10)
        num_2 = random.randint (1,10)
        user_answer = integerbox(msg="What is " + str(num_1) + question + str(num_2) +" ?",lowerbound=-1000, upperbound = 1000, title="Maths Question Number " + str(question_number))
        question_number = question_number + 1
        if question==("+"):
                    realanswer = num_1+num_2
                   if user_answer==realanswer:
                        correct = correct + 1
        elif question==("x"):
                    realanswer = num_1*num_2
                    correct = correct + 1
        elif question==("-"):
                    realanswer = num_1-num_2
                    correct = correct + 1
    msgbox ("You got " + str(correct) + " correct " + "out of 10. ", title="Maths Question")

Doing something n times

You have a loop asking 10 questions, which you implement like:

question_number = 1
...
while question_number <= 10:
    ...
    question_number = question_number + 1
    ...

This works, but we can do better. The problem with this loop is that it's error-prone. It's three lines of code to express a simple concept, and they're far apart. What if you added a continue before the increment? You'd ask more questions than you meant to.

A better way is just:

for question_number in range(10):
    ...

That loop will iterate 10 times, all in one line of code.

Repetition, repetition, repetition

You have

correct = correct + 1

in three places. Whenever we repeat ourselves, we risk repeating ourselves incorrectly. In this case, we did! For addition questions, we check if the user actually gave us the right answer. For the other two questions, we forgot to check! There's no if statement!

Much better to first determine what the right answer is:

if operation == '+':
    real_answer = num1 + num2
elif operation == '-':
    real_answer = num1 - num2
elif operation == 'x':
    real_answer = num1 * num2

And then just check it in one spot:

if user_answer == real_answer:
    correct += 1

[Advanced] But even this is super repetitive with setting up real_answer, so we can do better still. The value of operation basically defines a function that we need to apply to the two numbers. Let's make that more explicit by actually defining functions:

def add(a, b):
    return a + b

def sub(a, b):
    return a - b

def mul(a, b):
    return a * b

choice = {'+': add, '-': sub, '*': mul}

Now we can do:

real_answer = choice[operation](num1, num2)
if user_answer = real_answer:
    correct += 1

This can be shortened further with the standard library. Python gives us operator which defines all of these functions:

choice = {'+': operator.add,
    '-': operator.sub,
    '*': operator.mul
    }

The advantage of avoiding the repetition is how easy it becomes to add new operators. Let's say we want to test division? We would just have to add one line of code:

choice = {'+': operator.add,
    '-': operator.sub,
    '*': operator.mul,
    '/': operator.div  # <== just this
    }

Spacing

The ultimate Python style guide is PEP-8. Please use 4-space tabs throughout, not 8. That is too much spacing. No spacing between a function and the parens:

num_1 = random.randint(1,10)   # this one is good
num_2 = random.randint (1,10)  # this is an extra space

Also see the guidelines regarding variable names (prefer real_answer to realanswer, etc.)


Using RestTemplate with HttpComponentsClientHttpRequestFactory in a multithreaded environment I am working on a project in which I need to make an HTTP URL call to my server, running RESTful service, which returns the response as a JSON String. I am using RestTemplate here along with HttpComponentsClientHttpRequestFactory to execute a URL.

I have set up an HTTP request timeout (READ and CONNECTION time out) on my RestTemplate by using HttpComponentsClientHttpRequestFactory.

Here is my main code which is using the future and callables:

public class TimeoutThreadExample {

    private ExecutorService executor = Executors.newFixedThreadPool(10);
    // does it have to be static final?
    private static final RestTemplate restTemplate = createRestTemplate();

    // does this look right with the way I am creating `RestTemplate`?
    private static RestTemplate createRestTemplate(){
        // is it ok to create a new instance of HttpComponentsClientHttpRequestFactory everytime?
       HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory();
       requestFactory.setReadTimeout(READ_TIME_OUT);
       requestFactory.setConnectTimeout(CONNECTION_TIME_OUT);
       return new RestTemplate(requestFactory);
     }

    public String getData() {
        Future<String> future = executor.submit(new Task(restTemplate));
        String response = null;

        try {
            response = future.get(500, TimeUnit.MILLISECONDS);
        } catch (TimeoutException e) {
            e.printStackTrace();
        } catch (InterruptedException e) {
            e.printStackTrace();
        } catch (ExecutionException e) {
            e.printStackTrace();
        }

        return response;
    }
}

Here is my Task class which implements the Callable interface and uses the RestTemplate:

class Task implements Callable<String> {

    private RestTemplate restTemplate;

    public Task(RestTemplate restTemplate) {
        this.restTemplate = restTemplate;
    }

    public String call() throws Exception {

        String url = "some_url";
        String response = restTemplate.getForObject(url, String.class);

        return response;
    }
}

Now I have everything working in my above code. Is the way I am creating RestTemplate along with HttpComponentsClientHttpRequestFactory thread-safe and efficient? Since RestTemplate is very heavy to be created so not sure whether I have it right or not. I'd like to improve anything in creation of RestTemplate with HttpComponentsClientHttpRequestFactory.

Will you have many instances of TimeoutThreadExample in your real application? If not, then the restTemplate field will be better non-static, initialized in the constructor:

private final RestTemplate restTemplate;

public TimeoutThreadExample() {
    HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory();
    requestFactory.setReadTimeout(READ_TIME_OUT);
    requestFactory.setConnectTimeout(CONNECTION_TIME_OUT);
    restTemplate = new RestTemplate(requestFactory);
}

On the other hand, if you will have many instances of TimeoutThreadExample, then it will be better to make restTemplate static, and initialize it in a static initializer block instead of using a method:

private static final RestTemplate restTemplate;

static {
    HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory();
    requestFactory.setReadTimeout(READ_TIME_OUT);
    requestFactory.setConnectTimeout(CONNECTION_TIME_OUT);
    restTemplate = new RestTemplate(requestFactory);
}

As for this question:

  // is it ok to create a new instance of HttpComponentsClientHttpRequestFactory everytime?

In both alternatives I offered, the instance is really created only once, so that's ok.

  The way I am creating RestTemplate along with HttpComponentsClientHttpRequestFactory is thread safe and efficient?

Initializing static variables is thread safe. And since you're creating RestTemplate and HttpComponentsClientHttpRequestFactory only once, it's efficient too.


linux rearange field and sort by column I have (tab-separated) input files:

id1    id2    ....

id1    id3    ....

id3    id4    ....

id2    id1    ....

id3    id4    ....

.....

I need to

rearrange col1 and col2 by numerical sort.  For now I do this in a python script.
sort by col1 then col2. For now I am doing this by taking the output of the python script and using GNU sort.

My question is the following: is there a way to merge step 1 and step 2 (using GNU sort or any other GNU/Linux command-line tools)?

Is there an efficient alternative GNU/Linux command for step 1?

result:

id1    id2    ....

id1    id2    ....

id1    id3    ....

id3    id4    ....

id3    id4    ....

.....

My code actually works, I am looking to improve its speed.

Here's the Python program:

import argparse
import subprocess
import os

parser = argparse.ArgumentParser(description='')
parser.add_argument('-blast', help='input', required=True)
parser.add_argument('-out', help='output', required=True)
args = parser.parse_args()

def get_tmp():
# return a name for temporary file.

    dir = os.listdir(".")
    cpt = 0

    name = "tmp_{}".format(cpt)

    while name in dir:
        cpt += 1
        name = "tmp_{}".format(cpt)

   return name

# get a temporary name
tmp_name = get_tmp()

# open inputfile in reading and output in writing
with open(args.blast) as input_blast, open(tmp_name, 'w') as tmp_file:

    for line in input_blast:

        spt = line.strip().split()
        tmp_file.write('\t'.join(sorted(spt[0:2]) + spt[2:]) + '\n')

# sort by field one and two
child = subprocess.Popen("sort -k1 -k2 {} > temps_sort && mv temps_sort {}".format(os.path.abspath(tmp_name), args.out),shell=True)
child.wait()

There is no reason to use an external file and command. Python can sort strings just fine, unless the file is too big

something like this should work

def rearrange_ids(file):
    pattern = re.compile(r'id(\d+)')
    for line in file:
        spt = line.strip().split()
        if spt:
            ids = sorted(int(pattern.findall(id_string)[0]) for id_string in spt[0:2])
            yield ids, '\t'.join(['id%i' % i for i in ids] + spt[2:]) + '\n'

with open(input_file, 'r') as file, open(output_filename, 'w') as output_file:
    output_lines = sorted(rearrange_ids(file), key=lambda x: x[0])
    output_file.writelines(line for ids, line in output_lines)

id1   id2 ....
id1   id2 ....
id1   id3 ....
id3   id4 ....
id3   id4 ....

edit

I changed my original algorithm because id did not keep the original order when comparing, and sorted 10 before 2


Cube root in Python 3 using Bisection Search: Numbers in -1 to 0 Here is my code, which doesn't seem to work for numbers in between -1 and 0 (works perfectly fine otherwise)

I think the code enters an infinite loop, somehow, hence I get no result after entering the number. Please have a look and let me know what changes should be made. I am a beginner at programming and would appreciate any help!

    cube = float(input('Enter:'))
if abs(cube) > 1:
    num_guesses = 0
    epsilon = 0.0001
    low = 0
    high = cube
    guess = (high + low) / 2.0

    while abs(abs(guess) ** 3 - abs(cube)) >= epsilon:
        if cube > 0:
            if guess ** 3 < cube:
                low = guess
            else:
                high = guess
            guess = (low + high) / 2.0
            num_guesses += 1
        else:
            if guess ** 3 > cube:
                low = guess
            else:
                high = guess
            guess = (low + high) / 2.0
            num_guesses += 1

if abs(cube) < 1:
    num_guesses = 0
    epsilon = 0.0001
    low = cube
    high = 1
    guess = (high + low) / 2.0

    while abs(abs(guess) ** 3 - abs(cube)) >= epsilon:
        if cube > 0:
            if guess ** 3 < cube:
                low = guess
            else:
                high = guess
            guess = (low + high) / 2.0
            num_guesses += 1
        else:
            low = -1
            high = cube
            if guess ** 3 > cube:
                high = guess
            else:
                low = guess
            guess = (low + high) / 2.0
            num_guesses += 1

print(num_guesses)
print('The cube root of',cube,'is closest to',guess)

Thanks a lot!

There are some problems with your code, the algorithm being (too complicated, it is also) wrong.

In addition to that, you haven't made any functions, which you should strongly consider doing.

The better way of writing this script is to write two (or more) functions, one function being the actual cuberoot function which accepts one (or two) argument, namely the number you want to find the cube root for.  It should return one number, the cube root.

Writing functionality in function, or perhaps encapsulating functionality in functions allows you to more easily test the function.  Having all functionality in functions, also allows you to import cuberoot from a different file.  This is essential when building more complex applications and libraries later.

I have attached one example of how you can modularize your code so that it is both easy to test and easy to import, and not least of all, it's easy to read and thus understand.

As you can see in the while loop, it is quite close to your example, but a bit simplified.

I have also added an assert statetment in the main function to easily verify that the result is correct.  Adding automatic tests are now quite easy as well.

The bottommost __main__ part is there to ensure that the main function is called only if we call the file as a script:

$ python cuberoot.py -0.5
-0.7936859130859375

Here is the refactored code:

def cuberoot(cube, epsilon=0.0001):
    """Compute cube root of n using binary search.

    We yield the result when |result**3-n|<epsilon.
    """
    high = 2 + abs(cube)
    low = -high
    while True:
        guess = (low + high) / 2.0
        err = abs(cube - guess ** 3)
        if err < epsilon:
            return guess
        if guess ** 3 > cube:
            high = guess
        else:
            low = guess

def main():
    """Read one number from command line and print its cube root."""
    from sys import argv

    try:
        cube = float(argv[1])
    except ValueError:
        exit("Usage: cuberoot n [n must be a number]")
    except IndexError:
        exit("Usage: cuberoot n")

    cr = cuberoot(cube)
    assert abs(cr ** 3 - cube) < 0.0001  # verify the result
    print(cr)

if __name__ == "__main__":
    main()


Route Improvement in Laravel 8 I have below route. I am using Laravel 8.
Route::post('/update-crew',
    array(
        'uses' =>  'App\Http\Controllers\Directory\Crew\API\CrewAPIController@Update',
        'as'   =>  'apiUpdateCrew',
    )
);

Is there any way to use namespace instead of writing the path in string. My route contains both uses and as.
can I improve the code by keeping the uses and as and instead of string path, can I use namespace something like below?
use App\Http\Controllers\Directory\Crew\API\CrewAPIController;

Route::post('/update-crew',
    array(
        'uses' =>  ['CrewAPIController', 'Update'],
        'as'   =>  'apiUpdateCrew',
    )
);

Is this possible?

I recently heard about Laravel 8 changing the way routing was declared, as this video explains that developers upgrading from Laravel 7 or earlier will need to either switch to using the PHP callable syntax or fully namespace the controller in the string syntax (as your first code snippet does) (see the section Routing under Upgrading To 8.0 From 7.x).
Yes adding that use statement at the top then you should be able to reference the class without the namespace before it.
You can use the ::class keyword for name resolution instead of using a string literal:
'uses' =>  [CrewAPIController::class, 'Update'],

Many populate IDEs will index the class names and allow you to then easily jump to the class definition -e.g. ctrl/command + click on name).

I also wasn't familiar with the 'as' syntax, then I found this comment on this accepted SO answer to What does “as” keyword mean in Laravel routing?

As keyword is in older version, if you change the documentation to 5.2 you can see the as keyword. In newer version it's ->name

So you could use the name() method to make the Named route instead:
Route::post('/update-crew', [CrewAPIController::class, 'Update'])
    ->name('apiUpdateCrew');