Should this code be in the code behind or separate class? I'm developing a log-on page where users either select or create an associated organization when they sign up.  There can be either 1 user per organization or many users per organization, this is specified at runtime via web.config.

I have a separate project that handles all user account creation. I don't know if this code should be in the Webforms project or the user account creation project since it is driven by the user's selection in the GUI?

    private IpasLogOnManagement.IpasOrganization EnsureOrganizationExists()
    {
        IpasLogOnManagement.IpasOrganization organization = null;

        if (OrganizationsSeparateFromUsers)
        {
            if (lstOrganizations.SelectedIndex == -1)
            {
                try
                {
                    organization = organizationProvider.SearchForOrganizationByName(txtOrganizationName.Text);
                }
                catch (IndexOutOfRangeException)
                {
                    //Organization does not exist, so create it
                    organization = manager.CreateOrganization(txtOrganizationName.Text);
                }
            }
            else
            {
                string selectedName = lstOrganizations.Items[lstOrganizations.SelectedIndex].Text;
                organization = organizationProvider.SearchForOrganizationByName(selectedName);
            }
        }
        else
        {
            organization = manager.CreateOrganization(txtEmail.Text);
        }
        return organization;
    }

Since you actually have a business layer, that's where I'd put it.  You should let the business layer handle the finding/creating of an organization, so all the WebForms code needs to do is ask for an IpasLogOnManagement.IpasOrganization object based on the name the user selected (lstOrganizations) or entered (txtOrganizationName).


Prevent re-entrant function call in C++ The purpose of this is to prevent a re-entrant function call in a single thread, e.g. prevent the situation where func() calls bar() which calls func().

This is currently not used in any multithreaded code (and does not plan to be).

struct TranLock
{
static int count;
    bool failed;
    TranLock() { failed = count; if (!failed) ++count; }
    ~TranLock() { if (!failed) --count; }
};
int TranLock::count = 0;

Usage:

void func()
{
    TranLock lock;
    if ( lock.failed )
        return;   // or take other action

    // the function's logic here
}

Is there a better idiom for this, e.g. involving std::shared_ptr ? C++11 features OK.

Update: As pointed out by Greg Hewgill, if multiple functions use TranLock they will all have a shared lockout. Currently I only use it in one function, although it would be nice to see two different possible implementations:

Lock which can be shared amongst functions
Individual lock per function

(perhaps this could be controlled with a template parameter or something).

The obvious problem with this is it will detect any function that uses TranLock calling any other function that also uses TranLock (since there is only a single global count). That is, if func() (which uses TranLock) calls bar() (which also uses TranLock) then bar() will erroneously fail.

In order to make a lock like this that can be used with multiple functions, create a class with no static members, then in each function where you want to use a lock, create a static instance of the class. That way, each function will have its own independent counter. However, you can't rely on the class destructor in this case because the destructor of static objects is not called when the object goes out of scope.

You could use a hybrid of these two techniques, for example

struct TranLock
{
    int &count;
    TranLock(int &count): count(count) { failed = count; if (!failed) ++count; }
    ~TranLock() { if (!failed) --count; }
};

void func()
{
    static int TranLock_count;
    TranLock lock(TranLock_count);
    if (lock.failed) {
        ...
    }
    ...
}

Finally, you don't really need to maintain an int count at all. Since you only ever increment count when it is zero, a bool flag would do equally well.


A simple Javascript Breakout game I wrote a simple breakout game in javascript. I was wondering if anyone could give me some advice on improving my code. I wrote somethings a bit oddly so I am more then welcome to criticism (constructive would be preferred however.)

let canvas = document.getElementById("mainCanv");
let ctx = canvas.getContext("2d");
let brickLength = 14;
let score = 0;
let paddle = {
 w:80,
 h:15,
 x:180,
 y:canvas.height-40,
}
let ball = {
 w:20,
 h:20,
 x:200,
 y:200,
 speedY:Math.sin(0.6),
 speedX:Math.cos(0.6),
 speed:5
}

let keys = {
 Left:false,
 Right:false
}

let bricks = initBricks(brickLength);

function update(){
 //Draw everything
 ctx.fillStyle = 'black'
 ctx.fillRect(0,0,canvas.width, canvas.height)
 ctx.fillStyle = "green"
 ctx.fillRect(paddle.x, paddle.y, paddle.w, paddle.h)
 ctx.fillStyle = "red"
 ctx.fillRect(ball.x, ball.y, ball.w, ball.h)
 ctx.fillStyle = "white";
 ctx.fillText("Score: " + score, 20,20)
 for(let i = 0; i < brickLength; i++){
  if(bricks[i].showing){
   ctx.fillStyle = "blue";
   ctx.fillRect(bricks[i].x, bricks[i].y, bricks[i].w, bricks[i].h);
   if(ball.y<bricks[i].y+bricks[i].h && ball.y > bricks[i].y-bricks[i].h&& ball.x > bricks[i].x - bricks[i].w && ball.x < bricks[i].x +bricks[i].w){
    bricks[i].showing = false;
    ball.speedY *= 1;
    ball.speedX *= -1;
    score += 10;
   }
  }
 }

 //Do score calculations
 if(score % 13 == 0 && score >= 130){
  bricks = initBricks(brickLength);
  ball.speed += 2;
 }

 //Realtime movement
 if(keys.Right) {
  paddleMove(-5);
 }
 if(keys.Left) {
  paddleMove(5);
 }

 ballMove(ball, 5, 5)

 //Check out of bounds
 if(ball.y>480 || ball.y > paddle.y){
  ball.y = canvas.height/2
  ball.x = canvas.width/2
  ball.speed = 5;
  score = 0;
  bricks = initBricks(brickLength);
 }
}

//Key checking
window.addEventListener("keydown", function(event){
 switch(event.keyCode){
  case 37:
   keys.Right = true;
   keys.Left = false;
   break;
  case 39:
   keys.Right = false;
   keys.Left = true;
   break;
 }

})
window.addEventListener("keyup", function(event){
 if(event.keyCode == 37 || event.keyCode == 39){
  keys.Right = false;
  keys.Left = false;
 }
})

//Movement functions
function paddleMove(dir){
 paddle.x += dir;
 if(paddle.x<0||paddle.x>canvas.width-paddle.w){
  paddle.x -= dir;
 }
}

function ballMove(ball){

 ball.x+=ball.speedX*ball.speed;
 ball.y+=ball.speedY*ball.speed;

 if(ball.y<0 || (ball.y > paddle.y-paddle.h && ball.x < paddle.x+paddle.w && ball.x > paddle.x-paddle.w)){
  ball.speedY *= -1;
 }

 if(ball.x<0||ball.x>480 ){
  ball.speedX *= -1;
 }
}

//Initialise bricks
function initBricks(length){
 let bricks = [length];
 for(let i = 0; i < length; i++){
  bricks.push({
   x:0+i*40,
   y:60,
   w:20,
   h:20,
   showing:true
  })
 }
 return bricks
}

setInterval(update, 1000/60);
<!DOCTYPE html>
<html>
<head>
 <title>Breakout</title>
</head>
<body>
<canvas id="mainCanv" width="500" height="500" style="background-color:black"></canvas>
<script type="text/javascript" src="game.js"></script>
</body>
</html>

Code review

Not bad, code is nice and clean, and naming is good.

Game play has some logic flaws in the way you handle the keyboard and collisions.

Some code style points.

Use constants for variables that do not change.
Learn to add semicolon to the end of lines. Don't rely on automatic semicolon insertion as that has some traps.
Separate operators with spaces to make the codes easier to read. EG if(ball.x<0||ball.x>480 ){  should be if (ball.x < 0 || ball.x > 480) {
KeyboardEvent.keyCode is a depreciated property and could be removed at any time. Use KeyboardEvent.key or KeyboardEvent.code
Never use setInterval to time rendering as it is not synced to the display refresh. Use requestAnimationFrame instead.
Add "use strict"; as the first line of the JavaScript to tell JavaScript to run in a stricter and more performant mode.
You have let bricks = [length]; this does not create an array of length, it creates an array with the first item length. To set an array's length use the length property bricks.length = length;
window is the default object, there is really no reason for you to reference it. You don't use it for window.document, window.setInterval, or any of the variables you declare in global scope so why use it for window.addEventListener

Code design and logic

See the rewrite for more details regarding the points below.

The update function has taken on too much responsibility. Try to limit what a function does to one thing only.
Keyboard logic is incorrect. You assume only one control key to be down at any one time. Players will sometimes hold one direction and tap the other. Your code does not allow this giving the illusion that the first key down no longer works. It is better to just hold the key state of each key.
Your paddle/ball collision has some logic flaws. A general rule of game collisions is to NEVER allow the game state to enter an impossible state. In this case the ball and paddle overlapping.

As you have the code the ball will on occasion get stuck to the paddle.

When you do a collision remember that it happens over a period of time between the previous and current frame. You need to work out when the ball hit during that frame and where it will be at the start of the next frame (usually some distance from the collision as it is already moving away)
Objects can also hold functions. It is easier to manage the various parts of a game if you encapsulate functionality into the game objects as well. In the rewrite you will see the main object, ball, paddle and bricks all contain the functions they need to draw, update, init/reset and various other functions as needed.
You can inherit properties of existing types using Object.assign, or the spread operator ... allowing you to setup properties and functions that are the same for many different objects. The bricks are an Array with properties added to draw, etc. Each array item is a brick with its own properties and functions.
Rather than have a semaphore to indicate if a brick is showing or not you can remove the brick from the array and that will save you iterating many bricks that are not showing.
Having many magic numbers spread in your code you can create a settings object to hold everything in one place. This makes it much easier to change the game settings, saving the hassle of going over the code to find every instance of a setting. In the rewrite the object game has everything needed to set up the game.
The order of actions for a game is usually move everything , test collisions and adjust position of everything, then draw everything

The rewrite.

The rewrite is as an example only. There are many ways to organised the code for an app, each has it pros and cons. These will vary with the individual, with the best style being one that best suits your way of thinking. Don't let code dogma spoil the fun of writing a game.

The rewrite has added some things.

I needed to test if it was all working, and being lazy, playing the game to find problems was too much effort, so I added an autoplay that plays the game at many time normal speed so that I could make sure all was OK. Check the game.autoPlay  to turn off.

I also added a few more bricks.

I have change all the collision logic to a more robust form that takes in account the object's travel during the previous frame. As there are 4 types of collision Ball / Paddle, Ball / Wall, Ball / Brick, and Paddle/Wall each somewhat different in needs, thus the code base has grown somewhat.

I was tempted to add ball hit deviation depending on where the ball hit the paddle, but left that out in the end.

"use strict";

requestAnimationFrame(mainLoop); // request first frame. This will only happen after all the code below has been evaluated

// I am using direct reference to the canvas by its ID
//const canvas = document.getElementById("mainCanv");
const ctx = canvas.getContext("2d");

var score = 0;

/*===========================================================================================
  all game constants and settings
  ============================================================================================*/
const game = {
    backgroundColor : "black",
    autoPlay : true,
    auto : {
        playSpeed : 10, // Number of autoplay frames per 60th second Note one extra normal frame is also rendered
        follow : 0.7, // how well auto play ball follows 1 is perfect, 0.8 will always hit, < 0.8 ocational miss to 0 wont move
    },
    paddle : {  // details for the paddle
        init : {  // this is added directly to the paddle object
            w : 80,
            h : 15,
            color : "green",
        },
        fromBottom : 40, // these are added programmatic
        speed : 5,
    },
    ball : {
        init : { // this is added directly to the ball object
            w: 20,
            h: 20,
            x: 200,
            y: 200,
            color : "red"
        },
        dir : 0.6, // these are added programmatic
        speed : 4,
    },
    brick : {
        init : { // this is added directly to each brick object
            w : 20,
            h : 20,
            color : "blue",
        },
        spacingX : 40, // these are added programmatic
        spacingY : 40,
        top : 60,
        rowCount : 14,
        count : 14 * 4,
        value : 10, // points its worth
    },
    score : { // where to put the score and its color
        x : 20,
        y : 20,
        color : "white",
    },
}

/*===========================================================================================
  helper Function
  ============================================================================================*/
function doRectsOverlap(rectA, rectB){ // both rectA, and rectB must have properties x,y,w,h
    return ! (
        rectA.y > rectB.y + rectB.h ||
        rectA.y + rectA.h < rectB.y ||
        rectA.x + rectA.w < rectB.x ||
        rectA.x > rectB.x + rectB.w
    );
}
/*===========================================================================================
  Common object properties and functions
  ============================================================================================*/
const rectangle = {
    draw() {
        ctx.fillStyle = this.color;
        ctx.fillRect(this.x, this.y, this.w, this.h);
    }
}

/*===========================================================================================
  The main animation loop
  ============================================================================================*/
function mainLoop() {
 ctx.fillStyle = game.backgroundColor;
 ctx.fillRect(0, 0, canvas.width, canvas.height)
    inPlay();
    requestAnimationFrame(mainLoop); // request next frame
}

/*===========================================================================================
  When in play
  ============================================================================================*/
function inPlay() {
    if (ball.outOfBounds) {
        reset(); // reset game when out of bounds
    }
    // auto play loops over main game draws everything but bricks
    // at a faded alpha level
    if (game.autoPlay) {
        ctx.globalAlpha = 1 / (game.auto.playSpeed /4);
        for (let i = 0; i < game.auto.playSpeed; i++) {
            ball.update();
            paddle.update();
            bricks.update();

            paddle.testBall(ball);
            paddle.draw();
            ball.draw();

        }
        ctx.globalAlpha = 1;
        if (keys.any) {
            game.autoPlay = false;
        }
    }
    // main render for normal play at alpha = 1;
    ball.update();
    paddle.update();
    bricks.update();

    paddle.testBall(ball);

    paddle.draw();
    ball.draw();
    bricks.draw();

    if (bricks.length === 0 && paddle.hitBall) { bricks.init() }
    paddle.hitBall = false;
    drawScore();
}

function reset(){
    ball.reset();
    bricks.init();
    paddle.reset();
    keys.clear();
    score = 0;
}

function drawScore(){ // does the score thing
 ctx.fillStyle = game.score.color;
 ctx.fillText("Score: " + score, game.score.x, game.score.y)
}

/*===========================================================================================
  Keyboard state and event handler
  ============================================================================================*/
const keys = {
    ArrowLeft : false,
    ArrowRight : false,
    any : false,
    event(event) {
        if (keys[event.code] !== undefined) {
            keys[event.code] = event.type === "keydown";
        }
        keys.any = event.type === "keydown" ? true : keys.any;
    },
    clear(){
        for(const key of Object.keys(keys)) {
            if (typeof keys[key] !== "function") { keys[key] = false }
        }
    }
};
addEventListener("keydown", keys.event);
addEventListener("keyup", keys.event);

/*===========================================================================================
  Brick related code
  ============================================================================================*/
// Assign some functions to an array of bricks
const bricks = Object.assign([], { // The array to get some more properties
    init() {
        this.length = 0; // zero length
        for (let i = 0; i < game.brick.count; i++) {
            this.push({
                ...rectangle,
                ...game.brick.init,
                x: (i % game.brick.rowCount) * game.brick.spacingX,
                y: game.brick.top + (i / game.brick.rowCount | 0)  * game.brick.spacingY,
            })
        }
    },
    draw() {
        for (const brick of this) { brick.draw() }
    },
    update() {
        var i;
        for (i = 0; i < this.length; i ++) {
            const brick = this[i];
            if (ball.testBrick(brick)) { // check ball against brick remove brick if hit
                this.splice(i--, 1); // remove brick
                score += game.brick.value;
            }
        }
    }
});

/*===========================================================================================
  Ball related code
  ============================================================================================*/
const ball = {
    ...rectangle,  // assign common
    ...game.ball.init,  // assign ball init values
    dx : 0, // common name for speed is delta so I use deltaX, and deltaY shortened to dx,dy
    dy : 0,
    outOfBounds : true, // this forces reset at start of game
    reset() {
        Object.assign(this,
            game.ball.init, {
                dx : Math.cos(game.ball.dir) * game.ball.speed,
                dy : Math.sin(game.ball.dir) * game.ball.speed,
                outOfBounds : false,
            }
         );
    },
    update() {
        this.x += this.dx;
        this.y += this.dy;
        if (this.x <= 0) {
            this.x = -this.x; // the ball will have moved away from the wall the same distance it has moved into the wall
            // MUST MOVE AWAY +
            this.dx = Math.abs(this.dx); // Set the direction as always positive. If you do -this.dx it may stuff up when you have
                                         // the ball and paddle interacting at the same time
        } else if (this.x > canvas.width - this.w) {
            this.x = (canvas.width - this.w) - (this.x - (canvas.width - this.w));
            // MUST MOVE AWAY -
            this.dx = -Math.abs(this.dx);
        }
        if (this.y < 0) {
            this.y = -this.y;
            this.dy = Math.abs(this.dy);
        }
        if (this.y > canvas.height) { // ball out of bounds
            this.outOfBounds = true;
        }

    },
    checkbrickTopBot(brick){
        if (this.dy < 0) { // moving up
            this.y = (brick.y + brick.h) + ((brick.y + brick.h) - this.y);
            this.dy = -this.dy;
            return true;
        }
        // must be moving down
        this.y = brick.y - ((this.y + this.h) - brick.y) - this.h;
        this.dy = -this.dy;
        return true;

    },
    checkbrickLeftRight(brick){
        if (this.dx < 0) { // moving up
            this.x= (brick.x + brick.w) + ((brick.x + brick.w) - this.x);
            this.dx = -this.dx;
            return true;
        }
        // must be moving down
        this.x = brick.x - ((this.x + this.w) - brick.x) - this.w;
        this.dx = -this.dx;
        return true;
    },

    testBrick (brick) { // returns true if brick hit

        if (doRectsOverlap(this, brick)) {
            // use ball direction to check which side of brick to test
            // First normalize the speed vector
            var speed = Math.sqrt(this.dx * this.dx + this.dy * this.dy);
            var nx = this.dx / speed;
            var ny = this.dy / speed;
            var xDist,yDist;
            // now find the distance to the brick edges in terms of normalised spped
            if (this.dx < 0) {
                xDist = ((this.x - this.dx) - (brick.x + brick.w)) / nx;
            } else {
                xDist = ((this.x - this.dx + this.w) - brick.x) / nx;
            }
            if (this.dy < 0) {
                yDist = ((this.y - this.dy) - (brick.y + brick.h)) / ny;
            } else {
                yDist = ((this.y - this.dy + this.h) - brick.y) / ny;

            }
            // the edge that is hit first is the smallest dist
            if (xDist <= yDist) { // x edge first to hit
                return this.checkbrickLeftRight(brick);
            }
            return this.checkbrickTopBot(brick);
        }
        return false;
    },

};

/*===========================================================================================
  Paddle related code
  ============================================================================================*/
const paddle = {
    ...rectangle,  // assign common
    ...game.paddle.init, // assign startup
    x: (canvas.width / 2 - game.paddle.init.w / 2) | 0,  // | 0 floors the result (same as Math.floor)
    y: canvas.height - game.paddle.fromBottom,
    hitBall : true,
    leftEdge : 0,
    rightedge : canvas.width -  game.paddle.init.w,
    reset() {
        this.x =  (canvas.width / 2 - game.paddle.init.w / 2) | 0;  // | 0 floors the result (same as Math.floor)
        this.y = canvas.height - game.paddle.fromBottom;
    },
    update() {
        if (game.autoPlay) {
            keys.ArrowRight = false;
            keys.ArrowLeft = false;
            if (Math.random() < game.auto.follow) {
                var x = ball.x + ((Math.random()- 0.5) * this.w * 2);
                if (ball.x > this.x + this.w * (3/4)) { keys.ArrowRight = true }
                if (ball.x < this.x + this.w * (1/4)) { keys.ArrowLeft = true }
            }
        }
        if (keys.ArrowRight) { this.x += game.paddle.speed }
        if (keys.ArrowLeft) { this.x -= game.paddle.speed }
        this.x = this.x < this.leftEdge ? this.leftEdge : this.x;
        this.x = this.x >= this.rightEdge ? this.rightEdge : this.x;
    },
    testBall(ball) {
        if (doRectsOverlap(this, ball)) {
            // we now know that the ball must be hitting the paddle.
            var cx = (ball.x + ball.w / 2); // get ball x center
            // check for side hits. If so ball goes away from edge
            if (cx < this.x) { // hits paddle left side
                ball.y = this.y - ((ball.y + ball.h) - this.y) - ball.h;
                ball.x = this.x - ball.w;
                ball.dy = -Math.abs(ball.dy); // Must be negative
                ball.dx = -Math.abs(ball.dx); // Must be negative
                this.hitBall = true;

            } else if( cx > this.x + this.w) { // hits paddle right side
                ball.y = this.y - ((ball.y + ball.h) - this.y) - ball.h;
                ball.x = this.x + this.w;
                ball.dy = -Math.abs(ball.dy); // Must be negative
                ball.dx = Math.abs(ball.dx); // Must be positive
                this.hitBall = true;

            } else { // must be hits bat top or have run over the ball too lat
                if (ball.y > this.y + this.h / 2) { // if ball is more than halfway through the bat too lat to rescue so let it continue on
                    return;
                }
                ball.y = this.y - ((ball.y + ball.h) - this.y) - ball.h;
                ball.dy = -Math.abs(ball.dy); // Must be negative
                this.hitBall = true;
            }
        }

    },
}
<!-- the id is used to reference the element canvas -->
<canvas id="canvas" width="540" height="400"></canvas>


Alternate method of iterating than os.walk Recently, I became tired of how slowly os.walk seems to run (really, it should be called os.crawl), and made something recursive which moves much faster.  I was using it to take directories I wanted and add them to an array, but this time I need to add all files to an array that aren't in an 'exclude array.' Does anyone have any advice?

import os

exclude_folders = ['excludefolder1', 'excludefolder2']
product_path_names = ['folder1', 'folder2', 'folder3']
# This function replaces the os.walk, it is faster and stops scanning subdirectories
# when it find a good folder name.
def findDir(path):
    for directory in os.listdir(path):
     # If it's a folder that we want to scan, go for it.
        if(os.path.isdir(os.path.join(path, directory))):
            # If folder is a product name, add it.
            if(directory.lower() in product_path_names):
                print os.path.join(path, directory)
                product_dirs.append(os.path.join(path, directory))
                with open(event_log_path, "a") as log:
                    log.write(os.path.join(path, directory) + "\n")
                break
            # If folder is not a product name, scan it.
            else:
                #print os.path.join(path, directory)
                if(directory.lower() not in exclude_folders):
                    findDir(os.path.join(path, directory))

Style

According to PEP 8, functions should be named using lower_case().
Python code typically doesn't have parentheses around if conditions.
You call os.path.join() everywhere.  It would be worthwhile to assign the joined path to a variable.

Error handling

If any operation fails (for example, if you do not have permission to list some directory), then the whole search aborts with an exception.

Modularity and reusability

The inclusion and exclusion lists should be parameters rather than globals.

By the Single Responsibility Principle, a function should do just one thing.  The function you have written not only finds the directories that have the desired names, it also prints their path to sys.stdout, appends the results to a list, and logs the occurrence.

By making the function into a generator, you would give the caller the flexibility to do whatever it wants with the results.  The caller can even have the option to terminate early after the first result.  find_dirs() would then become a more generically useful function.

from __future__ import print_function
import os
import sys

def find_dirs(root_dir, names, exclude_folders=[]):
    try:
        for entry in os.listdir(root_dir):
            entry_path = os.path.join(root_dir, entry)
            entry_lowercase = entry.lower()
            if os.path.isdir(entry_path):
                if entry_lowercase in names:
                    yield entry_path
                elif entry_lowercase not in exclude_folders:
                    for result in find_dirs(entry_path, names, exclude_folders):
                        yield result
    except OSError as e:
        print(e, file=sys.stderr)

product_dirs = []
event_log_path = '/tmp/findlog.txt'
with open(event_log_path, 'a') as log:
    for lib in find_dirs('/', ['lib', 'library'], ['user']):
        print(lib)
        product_dirs.append(lib)
        log.write(lib + "\n")
        break      # Stop after finding the first match


Binary Tree Preorder Traversal What is the best way to move _add and _preorder methods in class Node to add_node and preorder methods in class BinaryTree such that they have the below class structure:

class Node:
    __init__
    __repr__

class BinaryTree:
    __init__
    add_node
    preorder

Complete Code::

class Node(object):

    def __init__(self, item, left=None, right=None):
        self.item = item
        self.left = None
        self.right = None

    def __repr__(self):
        return '{}'.format(self.item)

    def _add(self, value):
        new_node = Node(value)

        if not self.item:
            self.item = new_node
        elif not self.left:
            self.left = new_node
        elif not self.right:
            self.right = new_node
        else:
            self.left = self.left._add(value)
        return self

    def _preorder(self):
        print(self.item)
        if self.left:
            self.left._preorder()
        if self.right:
            self.right._preorder()

class BinaryTree(object):

    def __init__(self):
        self.root = None

    def add_node(self, value):
        if not self.root:
            self.root = Node(value)
        else:
            self.root._add(value)

    def preorder(self):
        if self.root:
            return self.root._preorder()

if __name__ == '__main__':

    binary_tree = BinaryTree()
    print( "Adding nodes 1 to 10 in the tree...")
    for i in range(1, 11):
        binary_tree.add_node(i)

    print()
    print( "Printing preorder...")
    binary_tree.preorder()

class Node(object):

    def __init__(self, item, left=None, right=None):
        self.item = item
        self.left = None
        self.right = None

The parameters left and right are not used in the constructor!  Remove them, or actually use them.

    def _add(self, value):
        new_node = Node(value)

        if not self.item:
            self.item = new_node
        ...

Given self.item is unconditional set in the constructor, this first test seems unnecessary.

        elif not self.left:
            self.left = new_node
        elif not self.right:
            self.right = new_node

Your binary tree does not seem to have any ordering imposed on the left/right nodes.  If there is no left node, you create one even if the new value is larger than the current node.  Similarly, if there is a left node, but no right node, you create a right node even if the value is smaller than the current node.

        else:
            self.left = self.left._add(value)
        return self

Finally, if both left and right exist, you unconditionally add the new node to the left sub tree, which will create a very unbalanced tree.  Moreover, the newly created new_node is discarded, to be recreated by the _add() method one level down, so is wasteful.  And the sole purpose of return self seems to be so that there is a return value to assign to self.left, which results in the the original value self.left being assigned to itself, so again useless busy work.

  A binary tree does not have any ordering imposed - nodes are added in the following preference: root -> left -> right. Ordering is imposed i[sic] a binary search tree.  – Saurabh

With your sample code, adding the values 1 to 10 to your BinaryTree, you get the following "tree":

                           1
                          / \
                         2   3
                        / \
                       4   5
                      / \
                     6   7
                    / \
                   8   9
                  /
                10

This is not really a binary tree; it is a stick with leaves.  It could be represented as a list of tuples: [(1,3), (2,5), (4,7), (6,9), (8,), (10,)].

If you wish to create a general binary tree, which does not impose an ordering, you need to include methods for building the desired tree, such as finding the "5" node, and explicitly adding to the right branch.  A generic add_node() doesn't have enough detail to control how the tree is built.


jQuery form builder for parsing email array into single input I have the following in script in my web app which contains a lot of duplication. How can I cut down on this duplication and generally improve my code?

$('a#newAuthority').on("click", function() {
    var $newAuthorityContainer = $(".new-authority-container");

    var $row = $('
', {
        class: 'row'
    }).appendTo($newAuthorityContainer);

    var $large11 = $('
', {
        class: 'large-11 columns'
    }).appendTo($row);

    var $input = $('<input>', {
        type: 'text',
        placeholder: 'email@domain.com',
        class: 'authority-email'
    }).appendTo($large11);

    var $large1 = $('
', {
        class: 'large-1 columns'
    }).appendTo($row);

    var $removeContainer = $('
', {
        class: 'remove-container'
    }).appendTo($large1);

    var $remove = $('', {
        href: '#',
        class: 'remove'
    }).html('').appendTo(
        $removeContainer);
    return false;
});
$('a#newMember').on("click", function() {
    var $newMemberContainer = $(".new-member-container");

    var $row = $('
', {
        class: 'row'
    }).appendTo($newMemberContainer);

    var $large11 = $('
', {
        class: 'large-11 columns'
    }).appendTo($row);

    var $input = $('<input>', {
        type: 'text',
        placeholder: 'email@domain.com',
        class: 'authority-email'
    }).appendTo($large11);

    var $large1 = $('
', {
        class: 'large-1 columns'
    }).appendTo($row);

    var $removeContainer = $('
', {
        class: 'remove-container'
    }).appendTo($large1);

    var $remove = $('', {
        href: '#',
        class: 'remove'
    }).html('').appendTo(
        $removeContainer);
    return false;
});

$('input.button').on("click", function() {

    var $authorityInputs = $('.authority-email');

    var authortyEmails = [].map.call($authorityInputs, function(
        $authorityInputs) {
        return $authorityInputs.value;
    }).join(',');

    if ($('body.teamsnew').length) {
        $('input#team_authority_emails').val(authortyEmails);
    }

    if ($('body.teamsedit').length) {
        $('input#team_authority_emails').val('{' + authortyEmails + '}');
    }

    var $memberInputs = $('.member-email');

    var memberEmails = [].map.call($memberInputs, function(
        $memberInputs) {
        return $memberInputs.value;
    }).join(',');

    if ($('body.teamsnew').length) {
        $('input#team_member_emails').val(memberEmails);
    }

    if ($('body.teamsedit').length) {
        $('input#team_member_emails').val('{' + memberEmails + '}');
    }
});

$(document).on('click', '.remove', function() {
    $(this).closest('.row').remove();
    return false;
});

Extract the code that creates a row to a function, make it return the row created with all the elements inside of it, so that you can use it like this:

var $newAuthorityContainer = $(".new-authority-container");
var $row = createRow().appendTo($newAuthorityContainer);

And then again later:

var $newMemberContainer = $(".new-member-container");
var $row = createRow().appendTo($newMemberContainer);

Alternatively, pass in the container that you want to add the row to, for example:

var $newMemberContainer = $(".new-member-container");
createRow($newMemberContainer);

The function for this latter example:

function createRow($container) {
    var $row = $('<div />', {
        class: 'row'
    }).appendTo($container);

    var $large11 = $('<div />', {
        class: 'large-11 columns'
    }).appendTo($row);

    var $input = $('<input>', {
        type: 'text',
        placeholder: 'email@domain.com',
        class: 'authority-email'
    }).appendTo($large11);

    var $large1 = $('<div />', {
        class: 'large-1 columns'
    }).appendTo($row);

    var $removeContainer = $('<div />', {
        class: 'remove-container'
    }).appendTo($large1);

    var $remove = $('<a>', {
        href: '#',
        class: 'remove'
    }).html('<i class="fa fa-fa fa-remove"></i>').appendTo(
        $removeContainer);
}

I don't see other troubling duplication.

However, I see a bit troubling typo in the variable name authortyEmails. This could lead to errors, so I suggest to fix that.


Circular Buffer Implementation I'm implementing a "live analytics" feature and I need to always keep track of the last N items that were added to a collection so I figured I'd make a circular buffer since I couldn't find one.

I tried to figure out to use Interlocked but couldn't find out how to use Increment or CompareExchange in an obviously correct way - so I took the lock.

Review/advice is appreciated.

public class CircularBuffer<T>
{
    private readonly T[] _data;
    private readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();
    private int _i = 0;
    private bool _isOneCycleFinished = false;
    private readonly int _size;

    public CircularBuffer(int size)
    {

        if (size < 0)
        {
            throw new ArgumentException($"{nameof(size)} cannot be negative");
        }
        _i = 0;
        _data = new T[size];
        _size = size;
    }

    public IEnumerable<T> Latest()
    {
        try
        {
            _lock.EnterReadLock();
            if (!_isOneCycleFinished)
            {
                return _data.Take(_i).ToList();
            }
            return _data.Concat(_data).Skip(_i).Take(_size).ToList();
        }
        finally
        {
            _lock.ExitReadLock();
        }
    }

    public void Add(T t)
    {
        try
        {
            _lock.EnterWriteLock();
            _i = (_i + 1)%_size;
            if (!_isOneCycleFinished && _i == 0)
            {
                _isOneCycleFinished = true;
            }
            _data[_i] = t;
        }
        finally
        {
            _lock.ExitWriteLock();
        }
    }
}

Like @Gareth mentioned in his comment it would be perfect to use the ConcurrentQueue<T>.

You answered @Gareth the queue needs to be bounded in size but where is the problem. We can just add a check to the Add() method and if the queue's size is equal to the bounded size we will just dequeue one item.

If the passed in size parameter of the constructor is 0 you will get a DivideByZeroException at _i = (_i + 1)%_size; and it doesn't really make sense to have a CircularBuffer with size == 0, does it ?

As you will notice in the adjusted code, I have placed the _lock.EnterWriteLock(); outside of the try..finally because what happens if the call to EnterWriteLock() fails ?

public class CircularBuffer<T>
{
    private readonly ConcurrentQueue<T> _data;
    private readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();
    private readonly int _size;

    public CircularBuffer(int size)
    {
        if (size < 1)
        {
            throw new ArgumentException($"{nameof(size)} cannot be negative or zero");
        }
        _data = new ConcurrentQueue<T>();
        _size = size;
    }

    public IEnumerable<T> Latest()
    {
        return _data.ToArray();
    }

    public void Add(T t)
    {
        _lock.EnterWriteLock();
        try
        {
            if (_data.Count == _size)
            {
                T value;
                _data.TryDequeue(out value);
            }

            _data.Enqueue(t);
        }
        finally
        {
            _lock.ExitWriteLock();
        }
    }
}

although using a ConcurrentQueue is IMO the cleaner way I will post a solution using a generic array as well like so

public class CircularBuffer<T>
{
    private readonly ReaderWriterLockSlim slimLock = new ReaderWriterLockSlim();
    private readonly int capacity;
    private readonly T[] buffer;
    private readonly int upperBound;

    private int currentIndex = -1;
    private int currentStart = 0;

    public CircularBuffer(int size)
    {
        if (size < 1)
        {
            throw new ArgumentException($"{nameof(size)} cannot be negative nor zero");
        }

        buffer = new T[size];
        capacity = size;
        upperBound = size - 1;
    }

    public void Add(T value)
    {
        slimLock.EnterWriteLock();
        try
        {

            if (IsFull || currentIndex == upperBound)
            {
                currentStart = FetchNextSlot(currentStart);
                IsFull = true;
            }

            currentIndex = FetchNextSlot(currentIndex);
            buffer[currentIndex] = value;

        }
        finally
        {
            slimLock.ExitWriteLock();
        }

    }

    public bool IsFull { get; private set; }

    private int FetchNextSlot(int value)
    {
        return (value + 1) % capacity;
    }

    public IEnumerable<T> Latest()
    {
        slimLock.EnterReadLock();
        try
        {
            return FetchItems().ToArray();
        }
        finally
        {
            slimLock.ExitReadLock();
        }
    }

    private IEnumerable<T> FetchItems()
    {
        IEnumerable<T> fetchedItems = Enumerable.Empty<T>();
        if (IsFull)
        {
            return FetchItems(currentStart, upperBound);
        }
        return fetchedItems.Concat(FetchItems(0, currentIndex));
    }
    private IEnumerable<T> FetchItems(int start, int end)
    {
        for (int i = start; i <= end; i++)
        {
            yield return buffer[i];
        }
    }
}


LeetCode 76: Minimum Window Substring - Java I'm posting my Java code for the Minimum Window Substring. If you have time and would like to review, please do so, I appreciate that.
Problem

Given a string string and a string target, find the minimum window
in string which will contain all the characters in target in
complexity O(n).
Example:
Input: string = "ADOBECODEBANC", target = "ABC" Output: "BANC"
Note:
If there is no such window in string that covers all characters in
target, return the empty string "". If there is such window, you are
guaranteed that there will always be only one unique minimum window in
string.

Java
import java.util.*;
import javafx.util.*;

class Solution {
    public String minWindow(String string, String target) {
        if (string == null || target == null || string.length() == 0 || target.length() == 0 || string.length() < target.length()) {
            return "";
        }

        int minLeft = 0, minRight = 0, min = string.length();
        boolean flag = false;
        int targetLength = target.length();
        Map<Character, Integer> map = new HashMap<>(targetLength);

        for (char character : target.toCharArray()) {
            map.put(character, -~map.getOrDefault(character, 0));
        }

        int left = 0, right = 0;

        while (right < string.length()) {
            char character = string.charAt(right);
            map.put(character, map.getOrDefault(character, 0) - 1);

            if (map.get(character) > -1) {
                targetLength--;
            }

            while (targetLength == 0 && left <= right) {
                flag = true;
                int curLength = -~right - left;

                if (curLength <= min) {
                    minLeft = left;
                    minRight = right;
                    min = curLength;
                }

                char leftChar = string.charAt(left);
                map.put(leftChar, -~map.getOrDefault(leftChar, 0));

                if (map.get(leftChar) > 0) {
                    targetLength++;
                }

                left++;
            }

            right++;
        }

        return flag == true ? string.substring(minLeft, -~minRight) : "";
    }
}

Reference

Question

Solution

Discussion

I have some suggestions.
Extract some of the logic to methods.
In your code, I see at least three sections of code that could be in methods. In my opinion, those extraction will help with the reading and make the code a bit shorter.

The validation of the parameters.

Before
if (string == null || target == null || string.length() == 0 || target.length() == 0 || string.length() < target.length()) {
   return "";
}

After
public String minWindow(String string, String target) {
   //[...]
   if (haveInvalidParameters(string, target)) {
      return "";
   }
   //[...]
}

private boolean haveInvalidParameters(String string, String target) {
   return string == null || target == null || string.length() == 0 || target.length() == 0 || string.length() < target.length();
}

The map initialization.

Before
//[...]
Map<Character, Integer> map = new HashMap<>(targetLength);
for (char character : target.toCharArray()) {
   map.put(character, -~map.getOrDefault(character, 0));
}
//[...]

After
public String minWindow(String string, String target) {
   //[...]
   Map<Character, Integer> map = initializeMap(target, targetLength);
   //[...]
}

private Map<Character, Integer> initializeMap(String target, int targetLength) {
   Map<Character, Integer> map = new HashMap<>(targetLength);
   for (char character : target.toCharArray()) {
      map.put(character, -~map.getOrDefault(character, 0));
   }
   return map;
}

The substring section.

Before
public String minWindow(String string, String target) {
   //[...]
   return flag == true ? string.substring(minLeft, -~minRight) : "";
}

After
public String minWindow(String string, String target) {
   //[...]
   return applySubstring(string, minLeft, minRight, flag);
}

private String applySubstring(String string, int minLeft, int minRight, boolean flag) {
   return flag == true ? string.substring(minLeft, -~minRight) : "";
}

Try to keep the variable declarations at the top of the blocks.
Generally in Java, we try to put the variable declarations at the top of the blocks.

Code Conventions for the Java (Revised April 20, 1999)

Before
int left = 0, right = 0;

After
int minLeft = 0, minRight = 0, min = string.length();
int left = 0, right = 0;

You can simplify the boolean expression
return flag == true ? string.substring(minLeft, -~minRight) : "";

is equals to
return flag ? string.substring(minLeft, -~minRight) : "";


NGON (many polygons) on SPOJ I am trying to solve NGON problem. I am using bottom up dynamic programming here. Recurrence function is:

$$\begin{array}{rl}
    f(a,b) &= f(a-1,b) + f(a-1,b-1)\,a_i +\frac{f(a-1,b-2)\,a_i(a_i-1)}{2}, a>0,b>0 \\
    f(a,0) &= 1, \\
    f(0,b) &= 0, \\
\end{array}$$

\$a_i\$ being the points on \$a^{th}\$ side.

I have used a \$O(n^2)\$ algorithm. I can't think anything other algorithm asymptotically faster than this but this is still getting TLE on SPOJ may be due to heavy modulo and long long int calculations. Can you please help me optimize it or recommend a better algorithm (if any)?

#include<stdio.h>
#define MAX 1010
#define MODULO 1000000007

int main()
{
    int test_cases,i,a,b;
    int sides,points[MAX];
    unsigned long long int result[MAX][MAX],temp;
    for(scanf("%d",&test_cases);test_cases>0;test_cases--)
    {
        scanf("%d",&sides);
        for(i=0;i<sides;i++)
        {
            scanf("%d",&points[i]);
        }

        result[0][0]=1;
        for(a=1;a<=sides;a++)
        {
            result[a][0]=1;
        }
        for(b=1;b<=sides;b++)
        {
            result[0][b]=0;
        }

        for(a=1;a<=sides;a++)
        {
            for(b=1;b<=sides;b++)
            {
                result[a][b]=(result[a-1][b]+result[a-1][b-1]*points[a-1])%MODULO;
                if(b>1)
                {
                    temp=(result[a-1][b-2]*points[a-1]*(points[a-1]-1))%MODULO;
                    temp=temp/2;
                    result[a][b]=(result[a][b]+temp)%MODULO;
                }
            }
        }
        printf("%lld\n",result[sides][sides-1]);
    }
    return 0;
}

You code is very crushed together; you hardly have any spaces in a single line.

Your code would be a lot more legible if you added spaces.

For example, you could turn this line:

result[a][b]=(result[a-1][b]+result[a-1][b-1]*points[a-1])%MODULO;

Into:

result[a][b] = (result[a-1][b] + result[a-1][b-1] * points[a-1]) % MODULO;

This is highly debatable topic, but I have been taught by people that it is always better to use the pre-increment operator over the post-increment operator.

By that I mean this:

++i

Over this:

i++

This is a little nit-picky.

In the signature of main, since you aren't looking for any command like arguments, you should put void in the arguments space.


SELECT from "vertical" table and display on "horizontal" table I have this table in my db:

table
----------
cityID
value
parameterID

The table looks like this:

cityID     value      parameterID
500        30         1
500        60         2
500        7          3
600        25         1
600        50         2
600        6          3
....       ...        .....

where 1,2, 3 stands for temperature, humidity and cm of rain.

I need to display this data this way:

   city       temp        humidity       rain
   500        30           60            7
   600        25           50            6

What I did was this (after SELECT * FROM table):

 /*retrieve all values and put them in loooong arrays */

while($row = $result->fetch_assoc()) {
$selectcityID_array[] = (isset ($row['cityID']) ? $row['cityID'] : "");
$selectparameterID_array[] = (isset ($row['parameterID']) ? $row['parameterID'] : "");
$selectvalue_array[] = (isset ($row['value']) ? $row['value'] : "");
 }
/* remove doubles from cities and parameters*/
$selectcityID_array_unique = array_values (array_unique($selectcityID_array) );
$selectparameterID_array_unique = array_values (array_unique($selectparameterID_array) );

/*chunk the values do that I have an "horizontal" array for every city*/
$value_array_chunked = array_chunk($selectvalue_array, count($selectparameterID_array_unique));

for ($i=0; $i < count($selectcityID _array_unique); ++$i){ /*for every city*/
   for ($i_content=0; $i_content < count($selectparameterID_array_unique); ++$i_content){ /*and for every parameter*/
        echo $ value_array_chunked[$i][$i_content];
        echo ' ';
     }

     echo '<br>';

}

It works, but is this the only (and best) way to do it?

You can use conditional aggregation to get the expected result set:

SELECT cityID,
       MAX(CASE WHEN parameterID = 1 THEN value END) AS temp,
       MAX(CASE WHEN parameterID = 2 THEN value END) AS humidity,
       MAX(CASE WHEN parameterID = 3 THEN value END) AS rain
FROM mytable
GROUP BY cityID

Demo here


Find the total volume traded for each stock and also find the high and low for each of those stocks I would like to know whether the following code can be changed in any way as to improve its functionality. Could there be any potential bugs that may arise as a result of how the code is written?

The program is designed to find the total volume traded for each stock
and to also find the high and low for each of those stocks.

Are there any lines of code that are poorly written?  Either syntactical or just a better way to express the logic.

#include <cstdio>
#include <cstring>
#include <map>
#include <iostream>
#include <string>

using namespace std;

typedef basic_string<char> string;

class   CHighLow
{
public:
    CHighLow() : nCurLow(0), nCurHigh(0) {};

    void    add(int nHigh, int nLow)
    {
        if (nHigh > nCurHigh)
            nCurHigh = nHigh;

        if (nLow < nCurLow)
            nCurLow = nLow;
    }

    int& getSum()
    {
        int sum = nCurLow + nCurHigh;
        return sum;
    }

    int     nCurLow;
    int     nCurHigh;
};

int main(int argc, char* argv[])
{
    if (!strcmp("version", argv[1]))
    {
        cerr << "Using version 1.0 VWAPer" << endl;
        return 0;
    }

FILE*   file = fopen(argv[2], "r");

    cerr << "Reading file" << argv[2] << endl;

    char    line[256];
    char    Stocks[1000][10];
    int     Intervals[1000];
    int     Volumes[1000];
    float   Highs[1000];
    float   Lows[1000];

    int     i = 0;
    int     sum = 0;

    while (fgets(line, 256, file))
    {
        sscanf(line, "%s %d %d %f %f",
Stocks[i], &Intervals[i],
&Volumes[i], &Highs[i], &Lows[i++]);
    }

    cerr << "Calculating total volumes" << endl;

    Map<std::string, int>       TotalVolumes;

    for (int s = 0; s <= I; ++s)
    {
        std::string stockname = Stocks[s];

        for (int j =0; j <= i; ++j)
        {
            if (!strcmp(Stocks[j], stockname.c_str()))
            {
                TotalVolumes[stockname] += Volumes[j];
            }
        }
    }

    cerr << "Calculating high lows" << endl;

    map<std::string, CHighLow>  HighLows;

    for (int s = 0; s <= i; ++s)
    {
        HighLows[Stocks[s]].add(Highs[s], Lows[s]);
        cout << HighLows[Stocks[s]].getSum();
    }

    cerr << "Writing files" << endl;

    for (int s = 0; s <= i; ++s)
    {
        cout << Stocks[s] << "," << Intervals[s] << "," <<
TotalVolumes[Stocks[s]] / Volumes[s] * 100 << endl;
    }

    map<std::string, CHighLow>::iterator itr = HighLows.begin();
    while (itr != HighLows.end())
    {
        cout << (*itr).first << "," << (*itr).second.nCurLow << "," <<
(*itr).second.nCurHigh << endl;

        ++itr;
    }

    return 1;
}

I see a number of things that could help you improve your code.

Don't abuse using namespace std

Putting using namespace std within your program is generally a bad habit that you'd do well to avoid.

Don't redefine standard names

This definition is a problem:

typedef basic_string<char> string;

It redefines std::string (because you've used the global namespace).  Better would be to simply use std::string without redefining it.

C++ is case sensitive

Your code includes a line:

Map<std::string, int> TotalVolumes;

There is no Map, but there is std::map.  Write the line like this instead:

std::map<std::string, int> TotalVolumes;

Don't rely on unspecified execution order

The code currently contains this:

sscanf(line, "%s %d %d %f %f",
    Stocks[i], &Intervals[i],
    &Volumes[i], &Highs[i], &Lows[i++]);

The problem is that it appears you're assuming that i is incremented after the sscanf completes, but that's not guaranteed by the standard and the code is ambiguous.  Instead, write it like this:

sscanf(line, "%s %d %d %f %f",
    Stocks[i], &Intervals[i],
    &Volumes[i], &Highs[i], &Lows[i]);
++i;

Use const where possible

The CHighLow::getSum function does not (and should not) alter the underlying data structure and should therefore be declared const.

Don't return a reference to a local variable

The CHighLow::getSum() function is written as returning a reference to an int.  In addition to this being somewhat unusual, in this case, it's also incorrect.  The reason is that it's returning a reference to sum which is a local variable within the function.  When the function returns the variable goes out of scope and is destroyed, so the reference is now pointing to unallocated memory.  That's a serious bug!  Instead, just return an int as in:

int getSum() const
{
        return nCurLow + nCurHigh;
}

Eliminate unused variables

Unused variables are a sign of poor code quality, so eliminating them should be a priority.  In this code, sum is set to 0 within main() but then never actually used.  My compiler also tells me that.  Your compiler is probably also smart enough to tell you that, if you ask it to do so.

Check array length before dereferencing

If the user fails to give the program command line arguments, this line will fail:

FILE*   file = fopen(argv[2], "r");

It would fail because there might might not be any argv[2] so you should check the argc value first and exit with an error, perhaps telling the user what the program requires, if there aren't enough arguments.

Make member data private

The the two data members of the CHighLow class are public so any function or code can alter their contents.  This is not generally good design.  Instead, make them private.  Because the only instance their needed is for output, that can be neatly solved with the next suggestion.

Write a custom stream inserter for a class

The code currently contains this:

std::map<std::string, CHighLow>::iterator itr = HighLows.begin();
while (itr != HighLows.end())
{
        std::cout << (*itr).first << "," << (*itr).second.nCurLow << "," <<
                     (*itr).second.nCurHigh << std::endl;

        ++itr;
}

There are a number of things that could be improved here.  First, use a "range-for" and second, use a custom stream inserter.  That results in this code:

for (const auto &stock: HighLows) {
    std::cout << stock.first << "," << stock.second << '\n';
}

Then in the class, create a custom inserter:

friend std::ostream& operator<<(std::ostream &out, const CHighLow &hilo) {
    return out << hilo.nCurLow << "," << hilo.nCurHigh;
}

Use C++ file I/O

Don't use the ancient FILE* I/O but instead use modern C++ std::fstream.  The interface is neater and you can write a nice class extractor instead of having all of the details for extracting data placed within main.

Don't use std::endl if '\n' will do

Using std::endl emits a \n and flushes the stream.  Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl.

Don't return nonzero from main unless there's an error

The convention in programming in C and C++ is that the returned integer from main() is considered by the operating system to be construed as an error code.  For that reason, the non-error condition is signalled by returning a 0.  Your code ends with return 1 which will be interpreted by most operating systems as an indication that an error was encountered.  Because the compiler will automatically generate the equivalent of a return 0; at the end of main(), I'd advocate simply omitting it.


Improving Makefile and general C++ project structure I am a single developer on a numerics project. As it is growing, I would like to improve

the Makefile
the general organisation of the project
structuring of the files
workflow

As it stands now I have a few classes (where a few depend on each other) and a main.cc. In total maybe 20 files. From that the executable is build via make and ./projectName creates data files, which are later processed with gnuplot script files. This whole process can also be compressed in a shell script run.sh.

I am working under Linux using gnu make and g++ compiler.

What bothers me is:

As I add new classes make takes more and more time.
Everything is build from scratch each time make runs, even if  only one class file is changed.
Related to that I have seen others using a build/ directory where object files files are stored. I haven't used .obj files so far. Does that solve the problem of (2.) and if yes how do I create them using make?
A debug and release build would be nice too, but I have seen questions on that on Stack Overflow, so I'll get to that.

What I have is the following:

Dir Tree

ProjectName
|+data/     <-- data files generated by program
|+scripts/  <-- scripts that handle data processing
|~includes/
| |-headerClass1.h
| |-headerClass2.h
| |-...
| |-main.h
| |-parameters.h
|~src/
| |-main.cc
| |-class1.cc
| |-class2.cc
| |...
|-Makefile
|-tags
|-projectname <--executable

Makefile

workdir:=$(shell pwd)

SRCS =  $(workdir)/src/main.cc   \
          $(workdir)/src/class1.cc \
          $(workdir)/src/class2.cc \
                ...

INCLUDE = -I$(workdir)/include/\
                    -I$(workdir)/src/

CTALL = $(workdir)/include/*\
                $(workdir)/src/*

CTAGS = ctags

DEL = /bin/rm -f

RESULT = projectname
CC = g++
LIBS= -lm -lboost_timer -lboost_filesystem -lboost_system
OPT = -O3 -Wall  -ggdb -D__STDC_FORMAT_MACROS -std=c++0x -fipa-matrix-reorg

all: clean $(RESULT) ctags

$(RESULT): $(SRCS)
    $(CC) $(LIBS) $(OPT) $(INCLUDE) $(SRCS) -o $@

# Rules for generating the tags.
# #-------------------------------------
ctags: $(CTALL)
        $(CTAGS) $(CTALL)

clean:
    @echo "cleaning ..."
    $(DEL) $(RESULT)

Common Conventions

The C++ compiler is named CXX
What you call RESULT is usually named TARGET
In addition to SRC you probably need OBJ
Commands are usually replaced by uppercase version varaiable names that name themselves.

ie. RM not DEL

Building issues

It rebuilds everything every time for two reasons.

The all: rule performs a clean: before it starts. So obviously it has to rebuild everything:

all: clean $(RESULT) ctags
   # ^^^^^^ Get rid of this

RESULT is dependent on SRC and rebuilds the executable from scratch each time. What you need to do is depend on OBJ and then you only need to link the objects together to build the executable. Each object will have its own dependency and only be re-built if the source file changes.

Note: Any common commands should follow the conventions set by the implicit rules:

The implicit rule for linking is:

$(CC) $(LDFLAGS) n.o $(LOADLIBES) $(LDLIBS)

You have a slightly more complex case but I would thus make my link command:

$(RESULT): $(OBJ)
    $(CXX)  $(LDFLAGS) -o $@  $(OBJ) $(LOADLIBES) $(LDLIBS)

In your case you have a set of extra libs I would then add them as follows

LDLIBS  += -lm -lboost_timer -lboost_filesystem -lboost_system
  #    ^^^^  Notice the +=
  #          This makes it easy to add future enhancements to the makefile

The default rule for building *.o files from C++ files is:

$(CXX) $(CPPFLAGS) $(CXXFLAGS) -c

Your header files inclusion is part of the pre-processor stage so should be added to the CPPFLAGS macro.

CPPFLAGS += $(INCLUDE)

Now your object files will build as you expect.

But normally you don't want to build your object files into the same directory as your source files. This is because you can have different build versions. eg you can have a debug build and a release build. You should never mix and match object from different builds. In fact no compiler manufacturer makes any guarantees about object files built with different compiler flags. So any differences in compiler flags can potentially make the object files incompatible. As a result build your object files into different sub directories based on the type of build.

Personally I build release into a directory called release and debug into a directory called debug. I know very boring. I also suffix any targets with their type.

Say I am building the lib plop.

libplop.<version>.so        // release version of plop
libplopD.<version>.so       // debug version of plop
libplopS.<version>.so       // Single threaded version (only build this if there is a specific difference for a single threaded version of the library that can be exploited (rare))
libplopSD.<version>.so      // Single threaded debug version

This way I can install both normal and debug version of a library and explicitly link other code against them.

Anyway back to your make file. To achieve this I normally do

OBJ     = $(patsubst %.cc,$(BUILD)/%.o, $(SRC))
BUILD  ?= .
TARGET  = projectname$(BUILD_EXT)
   # BUIlD_EXT is the suffix I place on executable to tell if it is debug/relese and version information
   # BUILD defines the build type. If this is not specified an unadulterated
   # build is done into the current directory (rather than debug/release)
   # You can do this by directory building target `make target`

## You can add build specific flags like this
## Set BUILD_EXT using the same technique.
CXX_BUILD_FLAGS_debug   = -g
CXX_BUILD_FLAGS_release = -O3
CXXFLAGS       += $(CXX_BUILD_FLAGS_$(BUILD))

all:  debug ctags
  #  by default build debug version
  #  if you want release you must explicitly ask for it

debug:
    $(MAKE) BUILD=debug target
release:
    $(MAKE) BUILD=release target

target: $(TARGET)

$(TARGET): $(OBJ)
    $(CXX)  $(LDFLAGS) -o $@  $(OBJ) $(LOADLIBES) $(LDLIBS)

$(BUILD)/%.o: %.cc
    $(CXX) $^ -c -o $@ $(CPPFLAGS) $(CXXFLAGS)

Organization of files.

I normally put header and source files into the same directory. And I normally break these into directories based on libraries or executable.

But I add an install target into to my makefile that uploads the binary and required header files to a build directory for other projects so they can use it.

# Note this is not real makefile (pseudo make)
# It is designed to show intention not real make instructions.
install:
   $(generate_source_control_tag)
   $(CP) $(TARGET)  $(BUILD)/bin    # used for executables
   $(CP) $(TARGET)  $(BUILD)/lib    # used for libs
   $(CP) $(TARGET_INCLUDES) $(BUILD)/include/$(TARGET_BASE)/

This way if I am working on a project I can leave it (potentially in mid modification) and work on another library without the two being affected. The two will only ever interact after they have been installed into the build directory.

This means I also add the following too my makefile

 CPPFLAGS += -I$(BUILD)/include
 LDFLAGS  += -L$(BUILD)/lib

Now if project B wants to use the library from project A. Then the convention becomes.

 #include "A/A_HeaderFile_1.h"

 LDLIBS += -lA


Matching maximal nodes with velocities I have written a code which works very good for small data-files (see below). The data file 'WSPL.dat' which I want to evaluate has a size of 2.6 GB and the data file  'VELOC.dat' has a size of 4.3 GB. The production of the data 'inputv.dat' takes almost 45 minutes and it has a size of 2.6 GB. At the final step, the code should produce a 'output_veloc_max.dat'. After 30 hours of calculations the last output could not be produced. Nothing was written in the final data. I can imagine that the opening of such a big data and writing a new data which grows continusly can lead to this problem.

My questions:

Can I optimize this code in such a way that it works faster?
If the code cannot be further optimized, should I rewrite the code in multiprocessing code?
If rewriting for multi processing helps, how should I do it?

from __future__ import print_function
import time
import re
c=[]

def get_num(x):
    return int(''.join(ele for ele in x if ele.isdigit()))

with open('WSPL.dat', 'r') as f:
    for line in f:
            if "ND" in line:
                print(line)
                c=get_num(line)
                print(c)
                break

linenum = 0
print("Anzahl der Knoten: ",c)

print('Program started.\n')
maxima = [[float('-inf'), ''] for _ in range(c)]

with open('WSPL.dat') as f:
    for line in f:
        if line.startswith('TS'):
            for maximum, line in zip(maxima, f):
                linenum += 1
                value = float(line)
                if value > maximum[0]:
                    maximum[:] = value, linenum,line

with open('VELOC.dat', 'r') as f, open('inputv.dat', 'w') as outfile:
    for line in f:
        try:
            line = line.strip()
            columns = line.split()
            vx = float(columns[0])
            vy = float(columns[1])
            print("{:.2f}\t{:.2f}".format(vx, vy), file=outfile)
        except ValueError:
            pass

linenum=[x[1] for x in maxima]
i=1
d = {}
with open('inputv.dat', 'r') as f5, open('output_veloc_max.dat', 'w') as out:
    out.write('VECTOR\nND    {0:2d}\nST  0\nTS      0.00\n'.format(c))
    for num, line1 in enumerate(f5, 1):
        if num in linenum:
            d[num] = line1
    out.writelines([d[i] for i in linenum])

print('Programm ist beendet.')
name3 = input("Eingabedatei drucken ")

A very small example for 'WSPL.dat' can be like this:

SCALAR
ND    3
ST  0
TS        10.00
    666.0000
    0.0000
    0.0000
SCALAR
ND    3
ST  0
TS      3600.47
  355.1744
  255.0201
  255.2748
SCALAR
ND    3
ST  0
TS      7200.42
  555.5984
  255.4946
  355.7014

A very small example for 'VELOC.dat' can be like this:

VECTOR
ND    3
ST  0
TS        10.00
   0.000   0.000
   0.000   0.000
   0.000   0.000
VECTOR
ND    3
ST  0
TS      3600.47
  -0.280   1.385
  -0.494   1.239
  -0.574   1.647
VECTOR
ND    3
ST  0
TS      7200.42
  -0.609   2.114
  -0.900   1.734
  -0.871   2.293

The lines under the TS in VLOC.dat and WSPL.dat belong to a node. for this small example we got 3 nodes (ND=3). The program finds the maximum value for each node in WSPL.dat and writes the line numbers in inputv.dat. Then opens the inputv.dat and VLOC.dat and finds the values for the given line numbers and write in another file 'output_veloc_max.dat'.

Performance

Your problem report baffles me.  You say that you can successfully produce inputv.dat, a 2.6 GB file, in 45 minutes.  Yet, filtering inputv.dat to produce output_veloc_max.dat fails after 30 hours?  The only possible bottleneck is if num in linenum — and that would only be inefficient if the number of nodes is huge.  To make it work efficiently for a large number of nodes, you could use a set instead of a list:

linenum = set(x[1] for x in maxima)

Multiprocessing would not help you much, since the task is not very parallelizable.  It is possible to analyze WSPL.dat in parallel with the VELOC.dat→inputv.dat conversion, but producing output_veloc_max.dat must wait.  Since these data files are line-oriented, you cannot easily skip to arbitrary points within them, so you must process VELOC.dat sequentially.  The only significant optimization would be to avoid writing out inputv.dat as an intermediate result and reading it back in.

Style

Drop imports that you not using, like import time and import re.  Since you have tagged this question as python-3.x, you also don't need from __future__ import print_function.

The initialization of c as c=[] is garbage.  The real assignment is c=get_num(line).  The get_num function is weird, in that it gathers up all digits that appear on a line, returning 24 even if the text is "2 cool 4 me".

I'm not a fan of your maxima two-dimensional array.  By storing pairs in a list, you end up with code that is obfuscated by subscripts, like if value > maximum[0] and [x[1] for x in maxima].  Ideally, lists should be used for homogeneous data of variable length.  This pair would be better written as a tuple or a namedtuple.  But since you don't care about the values as soon as you have obtained the line numbers, you might as well make two separate variables max_samples and max_indices.

The way you parse VELOC.dat is odd.  You indiscriminately attempt to interpret the first column of every line as a float, then ignore the line if that fails.  I would guess, looking at your excerpt of VELOC.dat, that a better approach would be to look for a line containing "TS", then take the following c lines, where c is the number of nodes — the same strategy that you use when parsing WSPL.dat.

This program is large enough that it should be split into functions, so that each chunk of code has a name that reflects its purpose, and so that each function has its own local variables.

from itertools import count

def n_lines_after_ts(n, f):
    """
    Filter the file, extracting the next n lines after each occurrence of "TS".
    Each result is a triple consisting of:

    * A sequential sample number (counting up from 0)
    * The node number for that sample (cycling 0, 1, ..., n-1)
    * The line from the file
    """
    line_counter = count()
    for line in f:
        if line.startswith('TS'):
            for node in range(n):
                yield next(line_counter), node, next(f)

def num_nodes(wspl_dat):
    """
    Parse the file to extract the first number that appears after "ND".
    """
    for line in wspl_dat:
        if line.startswith('ND'):
            return int(line.partition('ND')[-1])

def wspl_max_indices(num_nodes, wspl_dat):
    """
    Find the indices of the maximum sample for each node.
    """
    max_samples = [float('-inf')] * num_nodes
    max_indices = [None] * num_nodes
    for line_num, node, sample in n_lines_after_ts(num_nodes, wspl_dat):
        sample = float(sample)
        if sample > max_samples[node]:
            max_samples[node] = sample
            max_indices[node] = line_num
    return max_indices

def extract_veloc(num_nodes, veloc_dat, indices):
    """
    Extract the specified indices from the velocity data.
    """
    set_of_indices = set(indices)
    lines = {
        line_num: sample
        for line_num, _, sample in n_lines_after_ts(num_nodes, veloc_dat)
        if line_num in set_of_indices
    }
    return (lines[i] for i in indices)

with open('WSPL.dat') as wspl_dat_file, \
     open('VELOC.dat') as veloc_dat_file, \
     open('output_veloc_max.dat', 'w') as out:
    num_nodes = num_nodes(wspl_dat_file)
    print('Anzahl der Knoten: {0}'.format(num_nodes))
    max_wspl_indices = wspl_max_indices(num_nodes, wspl_dat_file)
    out.write('VECTOR\nND    {0:2d}\nST  0\nTS      0.00\n'.format(num_nodes))
    out.writelines(extract_veloc(num_nodes, veloc_dat_file, max_wspl_indices))
print('Programm ist beendet.')


Get available port from specified range I have a situation where we need to be able to run multiple instances of a Spring app with dynamically assigned ports defined at runtime. I need to be able to get an available port at runtime, create some associated file structure for dedicated logs &c, and plug the port number into the command-line arguments passed to Java.

Below is an adaptation of some code I pulled off the Unix exchange and tweaked. It currently seems to do what I need - runs in single-digit milliseconds, accepts upper an lower bounds or provides reasonable defaults, won't spin off into an infinite loop if someone does something stupid like specifying the same upper and lower bound as a port that's already in use, doesn't actually send a newline into a port that's already in use, etc.

Anyone see a flaw, or a way to make it cleaner?

I know there's a possible race condition, and suggestions for that are welcome. I don't need non-bash portability.

getPort() {
    local -i lo=${1:-50000} hi=${2:-65000}
    local -i range=$(( hi - lo + 1 )) candidate
    for try in {1..999}
    do  candidate=$(( lo + ( $RANDOM$RANDOM % range ) ))
        { printf "" >/dev/tcp/127.0.0.1/$candidate ||
          printf "" >/dev/tcp/-thisIP,redacted-/$candidate
        } >/dev/null 2>&1 || { echo $candidate; return 0; }
    done
    return 1;
}

ADDENDUM

Since I am only interested in ports on the local machine, I should be ok if I test both localhost and every IP for this machine, which in my case will be only one. (IP redacted above.)

Yes?

Also, since someone could pass in 0 and 65000 as args, just using a 16bit $RANDOM alone doesn't cover all cases. I took the cheesy-easy route and used $RNADOM$RANDOM, which has some weirdness since about 2/3 of the time the 5th digit from the right will only be one of [123], and the distribution curve will slope up from the low end, but these aren't problematic in my case. Still, I'd love to see examples of how to smooth it out.

There's an obvious flaw if the candidate port is closed on 127.0.0.1 but open on a different interface that you want your service to bind to.  That's particularly relevant if you want it to listen on all interfaces (0.0.0.0).

If your service binds to the localhost address, then that's obviously not a concern.


Web API controller to get details of a travel plan or a leave request My web API has 3 controllers for get, post and users CRUD. I named it as DetailsControllor, FormControllor and AuthControllor. In the details controllor, I used a switch case to find out which type of details needed, but I found that code is repeated in switch case. Can I use generics to minimize code or is there any other way to carry out these things?

 [HttpPost]
    [Route("details/{id}")]
    public async Task<HttpResponseMessage> GetDetails(string id)
    {

        try
        {
            var content = Request.Content.ReadAsStringAsync().Result;
            HttpResponseMessage response = Request.CreateResponse();
            switch (id)
            {
                case "leave":
                    RequestBase LeaveInput = jsonSerializer.Deserialize<RequestBase>(content);
                    Task<LeaveForm> LeaveOutput = repo.GetLeaveRequest(LeaveInput);
                    response.Content = new ObjectContent(typeof(object), LeaveOutput, jsonFormatter);
                    break;

                case "travel":
                    RequestBase travelInput = jsonSerializer.Deserialize<RequestBase>(content);
                    Task<BaseTravel> travelOutput = repo.GetTravel(travelInput);
                    response.Content = new ObjectContent(typeof(object), travelOutput, jsonFormatter);
                    break;

            }
            return response;
        }
        catch (Exception e)
        {

             HttpResponseMessage responseToClient = Request.CreateResponse((HttpStatusCode)422);
             responseToClient.Content = new StringContent(e.message, Encoding.Unicode);
         throw new HttpResponseException(responseToClient);
        }
    }

I'd re-think your switch:

RequestBase LeaveInput = jsonSerializer.Deserialize<RequestBase>(content); is repeated, the only difference it is assigned to a different variable depending on the case.
response.Content = new ObjectContent(typeof(object), travelOutput, jsonFormatter); is also repeated, with the only difference being the second parameter.

In fact, the three lines of each case are virtually identical, except for repo.GetLeaveRequest() and repo.GetTravel(), which suggests to me that the switch should happen at a lower level.

Perhaps you could do something like this:

private ObjectContent GetObjectContent(string id, string content)
{
    var input = jsonSerializer.Deserialize<RequestBase>(content);

    object output;
    switch (id)
    {
        case "leave":
            output = repo.GetLeaveRequest(LeaveInput);
            break;

        case "travel":
            output = repo.GetTravel(travelInput);
            break;
    }

    return new ObjectContent(typeof(object), output, jsonFormatter);
}

Thus the contents of your try could become:

var content = Request.Content.ReadAsStringAsync().Result;
var response = Request.CreateResponse();
response.Content = GetObjectContent(content);
return response;

Note that I'm not sure this works, consider it more like an idea than workable code. (I just don't feel like starting up VS to test this.)

Just a couple of quick remarks:

Local variables like LeaveInput and LeaveOutput should be camelCase.
You explicitly use RequestBase and HttpResponseMessage and Task<LeaveForm>: why not use var like elsewhere in your code?


Generating all possible permutations of the string This generates all possible permutations of the string.  I am also unable to understand the time complexity of this problem. Can someone please explain the complexity to me?

import java.util.ArrayList;

public class permutations {

    public ArrayList<String> performPermutations(String s){
        ArrayList<String> arrayList = new ArrayList<String>();
        if (s == null) {
            return null;
        }

        else if (s.length() == 0) {
            arrayList.add("");
            return arrayList;
        }

        else {
            for (int i = 0; i < s.length(); i++) {
                ArrayList<String> remaining = performPermutations(s.substring(0, i) + s.substring(i + 1));
                for (int j = 0; j < remaining.size(); j++) {
                    arrayList.add(s.charAt(i) + remaining.get(j));
                }
            }
            return arrayList;
        }
    }

    public static void main(String[] args) {
        permutations p = new permutations();
        ArrayList<String> arr = p.performPermutations("abcde");
            for(int i = 0; i<arr.size();i++) {
                System.out.println(arr.get(i));
            }
        }
    }
}

There are quite a few ways this code can be improved:

All class names should start with a capital, and be in CamelCase. Specifically, the class permutations should be Permutations. The way it doesn't look like a variable, as in here:

permutations p = new permutations();

performPermutations method

Why does it declare a return type of ArrayList<String>? Is there anything special about the implementation of the list? It should just be List<String>.
You have an unnecessary if-else structure, since each if statement returns a value. You can remove all of the elses.
In the first if statement, you return null. This is a bad idea in many cases. An empty list would be more favorable, just in case any calling code wants to operate on it immediately afterwards. (Notice that your main method would fail with a NullPointerException if the passed in list was empty).
You can combine the first and second if statements into one. This would be your "base" case:

if (s == null || s.isEmpty() { return arrayList; }

Never compare a list length to 0, use isEmpty() instead. Also, s is a poor name for a method parameter. Try to be creative and descriptive. Finally, there's no need to add an empty string if the list is empty.
There's a lot of existing posts on string permutations you may find interesting. Seeing a recursive call embedded in two for loops is probably unnecessarily complex.

How this method could look (but can still be improved):

public List<String> performPermutations(final String s){

    final List<String> result = new ArrayList<String>();

    if (s == null) {
        return result;
    }

    for (int i = 0; i < s.length(); i++) {
        final List<String> remaining = performPermutations(s.substring(0, i) + s.substring(i + 1));
        for (int j = 0; j < remaining.size(); j++) {
            result.add(s.charAt(i) + remaining.get(j));
        }
    }

    return result;
}


Executing asynchronous functions in sequence I implemented a function for practice that takes a collection of asynchronous functions and a callback. It executes the functions in sequence then calls
the callback with the results (coming from the callbacks of the async functions). It partially mimics the behavior of series function in the async library

The function

const series = (tasks, cb) => {
    let results = [];
    const iterator = index => {
        if (index === tasks.length) {
            return;
        }
        const task = tasks[index];
        task(val => {
            results.push(val);
            iterator(index + 1);
            if (index === tasks.length - 1) {
                cb(results);
            }
        });
    }
    iterator(0);
}

Usage

const tasks = [
    cb => {
        setTimeout(() => {
            cb('done 1');
        }, 1000);
    },
    cb => {
        setTimeout(() => {
            cb('done 2');
        }, 1000);
    },
    cb => {
        setTimeout(() => {
            cb('done 3');
        }, 1000);
    }
];

series(tasks, results => {
    console.log(results);
});

// outputs ['done 1', 'done 2', 'done 3'] after 3 seconds

What do you think about my approach? How would you implement it? Using plain JavaScript.

Quick review.

It is not bullet proof. So much so that it is a bug dressed in javascript.

You must protect against improper use, inadvertent error, or failure to complete.

The next bit of code shows a variety of ways it will fail.

series([cb => {
    if (Math.random() < 0.01) { throw "Spanner" }
    const handle = setTimeout(() => {
        cb('done');
        console.log("Not really done");
        cb('Done again');
    }, 1000);
    // Indeterminate completing clause
    setTimeout(() => clearTimeout(handle), Math.random() * 2000);
}, "Iron filings"], undefined); // undefined for a free throw if it got to the end

Why not use promises and async functions???

The following is immune to any of the above spannering.

async function series(tasks, tasksComplete) {
    if (Array.isArray(tasks)) {
        const timeoutPeriod = 5000; // 5 seconds and your out. Task can still complete
        const results = [];
        while (tasks.length) {
            const task = tasks.shift();
            if (typeof task === "function") {
                await new Promise(done => {
                        setTimeout(() => done("Too slow!") , timeoutPeriod);
                        task(done);
                    })
                    .then(result => results.push(result))
                    .catch(error => results.push(error))  // assuming good to continue
            }
        }
        if (typeof taskComplete === "function") { tasksComplete(results) }
    }
}

Update

In lieu of the above answer and comment you made the following might suit your needs a little better.

Rather than pass a callback to the series function, that function returns a promise which will resolve when all tasked have completed successfully, if there is a problem along the way the function will terminate returning the error.

Also the results stack is passed to each task. Rather than the last results the entire results stack is passed. It is up to the task which results it needs to use.

Also added are some options. Stop on error if true will stop on the first error, and timeout is greater then 0 will have a timeout error on tasks.

// Returns promise to do tasks in series
// stopOnError is false then continues tasks if there is an error placing
//             the error on the results stack
// timeout if > 0 will throw error if task takes too long.
// Promise resolves with the results stack for both completed and error
function series(tasks, stopOnError = true, timeout = 0) {
    const results = [];
    async function doEachTask(completed, failed) {
        while (tasks.length) {
            const task = tasks.shift();
            if (typeof task === "function") {
                await new Promise((done,error) => {
                        if (timeout) {
                            setTimeout(()=> error(new Error("Too slow!")) , timeout);
                        }
                        task(done, results);
                    })
                    .then(result => results.push(result))
                    .catch(error => {
                        results.push(error);
                        if (stopOnError) { failed(results) }
                    });

            }
        }
        completed(results);
    }
    return new Promise(doEachTask);
}

Used as follows

series([
    (complete, data) => {
         setTimeout(()=> {
             complete("Task1 complete");
         },1000)
    },
    (complete, data) => {
         console.log("Task 2 has data : "+data[0]);
         setTimeout(()=> {
             complete("Task2 complete");
         },1000)
    }])
    .then(results => console.log(results))
    .catch(results => console.log("Error and available results : " + results));


Storing an integer counter in indexer I've been implementing a custom internal server error page in ASP.Net MVC which will check if the current user is either an administrator or accessing the page from localhost, and if so, show them a whole bunch of details about the error to debug it with, otherwise just send them to a basic HTML error page.

So far, it works great, but one problem I had was that if there is an error in a partial view on the page, the system gets stuck in a loop trying to report the error.

To avoid this, I'm storing a temporary counter of how many times the current action has requested the error page in TempData, but I find the amount of lines and style of the code to get, set and check this variable a bit verbose:

using System.Web.Mvc;

namespace Test
{
    public class ErrorController : Controller
    {
        [ActionName("500")]
        public ActionResult InternalServerError(string aspxerrorpath = null)
        {
            int detectRedirectLoop = (TempData.Peek("redirectLoop") as int?) ?? 0;
            TempData["redirectLoop"] = detectRedirectLoop + 1;
            if((int) TempData.Peek("redirectLoop") <= 1)
            {
                // Check if user is admin or running locally and display error if so
            }
            return Redirect("/GeneralError.htm");
        }
    }
}

Is there a better/prettier/shorter way of doing this?

I suggest the following:

public class ErrorController : Controller
{
    private const string RedirectLoopCounterName = "RedirectLoopCounter";
    private const int MaxRedirectLoopCount = 1;

    private int RedirectLoopCounter
    {
        get { return ((int?)TempData.Peek(RedirectLoopCounterName)) ?? 0; }
        set { TempData[RedirectLoopCounterName] = value; }
    }

    private int IncreaseRedirectLoopCounter()
    {
        return ++RedirectLoopCounter;
    }

    [ActionName("500")]
    public ActionResult InternalServerError(string aspxerrorpath = null)
    {
        var isRedirectLoop = IncreaseRedirectLoopCounter() > MaxRedirectLoopCount;
        if(isRedirectLoop)
        {
            return Redirect("/GeneralError.htm");
        }

        // Check if user is admin or running locally and display error if so
    }
}

Create a constant for the counter's name as already mentioned by @BCdotWEB
Create a property for getting and setting its value
Create a method for actually increasing the counters value
Additionaly you could replace the 1 by a constant
Finally you can replace the condition by a helper variable to document it better
I would also invert the if


Ranked beginner quiz program I have been trying to learn JavaScript for the past few days and decided it was time to put what I had learnt into practice.  Here is the program I created. It's a five-question quiz which counts the number of correct questions as you go through the quiz and also ranks you at the end of the program.

//I am going to place a variable here to make initialise the number of correct values at 0
var correct = 0;

// ask questions prompts
var answer1 = prompt("Name a Programming language thats also a gem");
if ( answer1.toUpperCase() === 'RUBY') {
  correct += 1;
}
var answer2 = prompt("Name a Programming language thats also a snake");
if ( answer2.toUpperCase() === 'PYTHON') {
  correct += 1;
}
var answer3 = prompt("What language do we use to style webpages?");
if ( answer3.toUpperCase() === 'CSS' || answer3.toUpperCase() === 'CASCADING STYLE SHEETS') {
  correct += 1;
}
var answer4 = prompt("What language do you use to build the structure of a web page?");
if ( answer4.toUpperCase() === 'HTML' || answer4.toUpperCase() === 'HYPER TEXT MARKUP LANGUAGE') {
  correct += 1;
}
var answer5 = prompt("What core language do you use to add interactivity to a web page");
if ( answer5.toUpperCase() === 'JAVASCRIPT') {
  correct += 1;
}

// OUTPUT RESULTS
document.write("
You got " + correct + " out of 5 questions correct.
");

//output rank

if ( correct === 5 ) {
  document.write("
You earned a gold crown!
");
} else if ( correct >= 3) {
  document.write("
You earned a silver crown.
");
} else if ( correct >= 1) {
  document.write("
You earned a bronze crown.
");
} else {
  document.write("
Sorry, no crown for you. Go home and study.   n
");
}

Have I coded the program with what is considered best practices, and if not, what could I do to improve it?

See https://github.com/coolaj86/knuth-shuffle for a shuffle function.

function shuffle(array) {
  var currentIndex = array.length, temporaryValue, randomIndex ;

  // While there remain elements to shuffle...
  while (0 !== currentIndex) {

    // Pick a remaining element...
    randomIndex = Math.floor(Math.random() * currentIndex);
    currentIndex -= 1;

    // And swap it with the current element.
    temporaryValue = array[currentIndex];
    array[currentIndex] = array[randomIndex];
    array[randomIndex] = temporaryValue;
  }

  return array;
}

And write something like:

var correct = 0, answer = new Array();
var myQuestions = [ // my delimiter is ::, use any delimiter you like
  "Name a Programming language thats also a gem::ruby",
  "Name a Programming language thats also a snake::python",
  "What language do we use to style webpages?::css",
  "What language do you use to build the structure of a web page?::html",
  "What core language do you use to add interactivity to a web page?::javascript"
]
shuffle(myQuestions);

for(i = 0; i < myQuestions.length; i++) {
  answer[i] = prompt(myQuestions[i].substring(0,myQuestions[i].indexOf("::")));
  if(answer[i].toUpperCase() == myQuestions[i].substring(myQuestions[i].indexOf("::")+2).toUpperCase()) { // +2 because string is 2 characters and array starts at 0
    correct++;
  }
}

And so on …

I always say to minimize absolute code (never use it if it's possible). In this way, your code is more flexible leaving room for additions and improvements (you can improve on this code to include multiple answers as seen in your original code).


Is this a good object oriented design for a file validator? I'm working on a module that downloads a file saves it then validates if the file contents are valid.

For simplicity lets assume that the file consists of 3 segments: Header, Body & Footer.

Now here is the design I thought of:

class File{
    private $header;
    private $body;
    private $footer;

    function  __construct($pathToFile){
        $this->header= new Header($this->getFileHeader());
        $this->body= new Header($this->getFileBody());
        $this->footer= new Header($this->getFileFooter());
    }
/* getFileHeader(), getFileBody(), getFileFooter() are private functions of the File class
that basically reads the contents of the corresponding segment in the file. */

    function isValid(){
        if(!$this->header->isValid()) return false;
        if(!$this->body->isValid()) return false;
        if(!$this->footer->isValid()) return false;
    }
}

Header Segment class(I'll only show this one as the Body segment and Footer segment have similar structure)

class Header{
    private $content;
    private $validators=array();

    function  __construct($content){
        $this->content= $content;
        $this->validators= array(new NotEmptyValidator($content), new AllNumbersValidator($content));
        //for simplicity the validators will be only 2 and set in the constructor.
    }

    function isValid(){
        foreach($this->validators as $validator){
             if(!$validator->isValid()) return false;
        }
        return true;
    }

}

Finally the Validator class:

class NotEmptyValidator{
    private $content;

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

    function isValid(){
         return (empty($this->content));
    }

}

Questions:

Overall does this look like a good object oriented design? Can it be improved?
The Header class has an array of validators. I'm not sure if this is the best approach as I borrowed this idea from Zend framework form validators. Is there a better implementation?
Am I over delegating the task of validation?

Update:

I've simplified the design and striped many details to focus mainly on the validation process.

Another point The file I'm trying to validate is an EDI FACT file, so I suppose the file downloaded will have to be parsed first then validated.

Maybe reading the file contents in the File class is a bad idea and I should create a class Parser that takes the downloaded file and then create the File object itself then comes the validation.

That isn't OOP, that's procedural, with a thin wrapping that looks like OOP.  OOP will require that your validation be done on objects that share data and behavior.

Depending upon your file, this may or may not make sense.

OTOH, good procedural code is definitely better than the common PHP "just run it all together inside a single page" that frequently gets used.

EDIT: Expanding my answer after a comment.

First off, I should be clear that I wasn't criticizing your existing code, just saying they don't make it OOP. OOP is about data and behavior not just having things called classes and constructors.

Secondly, EDI probably means transactions of different types with well definined behavior for the various types -- that would be an opportunity to use/benefit from OOP.  I would try to design a base class or interface that can be applied to all lines/transactions and then have derived classes for the header/footer and body lines.

You need to focus on the similaririty of your data and your process for handling/validating it, your OOP implemention will come out of that.  It's a little to early to be asking about OOP.


Bubble Sort implementation in Java I want to know if there is any other better way to write this algorithm. What other data structures could be used to make it more simple and effective?

public class BubbleSort {

        public static void bubbleSort(int array[]) {
            int n = array.length;
            int k;
            for (int m = n; m >= 0; m--) {
                for (int i = 0; i < n - 1; i++) {
                    k = i + 1;
                    if (array[i] > array[k]) {
                        swapNumbers(i, k, array);
                    }
                }
                printNumbers(array);
            }
        }

        private static void swapNumbers(int i, int j, int[] array) {

            int temp;
            temp = array[i];
            array[i] = array[j];
            array[j] = temp;
        }

        private static void printNumbers(int[] input) {

            for (int i = 0; i < input.length; i++) {
                System.out.print(input[i] + ", ");
            }
            System.out.println("\n");
        }

        public static void main(String[] args) {
            int[] input = { 4, 2, 9, 6, 23, 12, 34, 0, 1 };
            bubbleSort(input);

        }
    }

Output:

    2, 4, 6, 9, 12, 23, 0, 1, 34,
    2, 4, 6, 9, 12, 0, 1, 23, 34,
    2, 4, 6, 9, 0, 1, 12, 23, 34,
    2, 4, 6, 0, 1, 9, 12, 23, 34,
    2, 4, 0, 1, 6, 9, 12, 23, 34,
    2, 0, 1, 4, 6, 9, 12, 23, 34,
    0, 1, 2, 4, 6, 9, 12, 23, 34,
    0, 1, 2, 4, 6, 9, 12, 23, 34,
    0, 1, 2, 4, 6, 9, 12, 23, 34,
    0, 1, 2, 4, 6, 9, 12, 23, 34

Efficiency

As someone else mentioned, the inner loop only needs to run up to m-1.
The standard optimization is that if an entire inner loop runs without ever swapping, then the array is now sorted and you can quit early.  Notice how it prints the same thing the last 4 times.

Style

If you've done bullet point 1 above, the only usage of n will be for (int m = n..., so you can just replace that with its definition and get rid of int n = array.length; above.
You don't need to declare 'int k;' at the top, Java is smart enough to make that optimization.  You can just use int k = i + 1;.
In swapNumbers, combine the first two lines into int temp = array[i];

Looking Forward

Right now, it's difficult to reuse bubbleSort because it spews output to the console.  I get that you want to see what it's doing, but once it's working you should take that out.  The sort function should only be responsible for sorting; the user of the function (in this case, the main method) can decide what to do with the results (e.g., print them).

If you want to be able to debug it in the future, then put it behind a debugging flag:

// this goes top-level in the BubbleSort class:
private static final boolean DEBUG = false;
// then, inside bubbleSort:
if (DEBUG)
    printNumbers(array);