User management in Node.js I have moved all my web-server code dealing with user management into a single Node.js module that I pasted below. I have spent time polishing it, and churned it through JSHint. I am generally happy with the code, except for the error handling. I find it clunky at best.

How can I improve error handling in the following code? Are there design or API guidelines I have not followed?

// ???
// Deal with the case where an admin no longer has admin rights, but still has a session open

var server = require('./server.js').server;
var User = require('./database.js').User;

// Define the user API
var API = {
    list: [list, 'private'],
    login: [login, 'public'],
    logout: [logout, 'private'],
    add: [add, 'admin'],
    remove: [remove, 'admin'],
    edit: [edit, 'admin']
};

// Attach path handlers
for(var label in API) {
    var denied = 'Permission denied';

    var wrapper = (function (label) {
        return function (req, res) {
            var callback = API[label][0];
            var permission = API[label][1];

            if(!req.session) {
                if(permission !== 'public') {
                    res.send(denied);
                    return;
                }
            } else if((permission === 'admin') && (req.session.rights !== 'Administrator')) {
                res.send(denied);
                return;
            }

            callback(req, res);
        };
    }(label));

    server.post('/user/' + label, wrapper);
}

function list(req, res) {
    User.find({}, null, {sort: {username: 1}}, function (err, users) {
        res.send(users);
    });
}

function login(req, res) {
    var errorMessage = 'Invalid user/password combination';

    find(req.body.username, function(err, user) {
        if(printError(err)) {
            return;
        }

        // Check we have a user
        if(!user) {
            res.send(errorMessage);
            return;
        }

        // Check for a username/password match
        user.comparePassword(req.body.password, function(err, isMatch) {
            if(printError(err)) {
                return;
            }

            if(isMatch) {
                // Populate the session cookie
                req.session.username = user.username;
                req.session.rights = user.rights;

                res.send('OK');
            } else {
                res.send(errorMessage);
            }
        });
    });
}

function logout(req, res) {
    // Clear session
    req.session.username = undefined;
    req.session.rights = undefined;
    res.send('OK');
}

function add(req, res) {
    find(req.body.username, function (err, user) {
        if(user) {
            res.send('The user already exists');
        } else {
            new User(req.body).save(function (err) {
                if(printError(err)) {
                    return;
                }

                res.send('OK');
            });
        }
    });
}

function remove(req, res) {
    find(req.body.username, function (err, user) {
        user.remove(function (err) {
            if(printError(err)) {
                return;
            }

            res.send('OK');
        });
    });
}

function edit(req, res) {
    find(req.body.username, function (err, user) {
        if(printError(err)) {
            return;
        }

        // Populate username and rights
        user.username = req.body.user.username;
        user.rights = req.body.user.rights;

        // Only update nonempty passwords
        if(req.body.user.password !== '') {
            user.password = req.body.user.password;
        }

        user.save(function(err) {
            if(printError(err)) {
                return;
            }

            res.send('OK');
        });
    });
}

function find(username, callback) {
    User.findOne({
        username: username
    }, callback);
}

function printError(err) {
    if(err) {
        console.error('ERROR: ', err.message);
        // console.trace();
    }

    return err;
}

From a once over

Not everybody in CR agrees, but I find tabular data best presented in an aligned format:

var API = {
  list:   [list  , 'private'],
  login:  [login , 'public' ],
  logout: [logout, 'private'],
  add:    [add   , 'admin'  ],
  remove: [remove, 'admin'  ],
  edit:   [edit  , 'admin'  ]
};

For your error handing, I would mention that using express is very nice for error handling. If you insist on doing it yourself you could do something like

function handle( f ){
  return function( err ){
    if(err){
      console.error('ERROR: ', err.message);
      return;
    }
    f.apply( this , arguments );
  }
}

Then you can

function remove(req, res) {
  find(req.body.username, handle( function (err, user) {
    user.remove( handle( function (err) {
      res.send('OK');
    }));
  }));
}

For your error message constants, I would declare them all together on top
I am ambivalent about using an array for your API config instead of an object, if you keep using that, I would suggest you use named constants declared on top:

    var callback   = API[label][CALLBACK];   //Where CALLBACK is earlier declared as 0
    var permission = API[label][PERMISSION]; //Where PERMISSION is earlier declared as 1


Is there a more optimized way for this MySql Query with below query i'm getting the results that i want but it's quite slow taking nearly 0.2 seconds (on an i5 machine). Is there a more optimized way to get the same results.

Basically this query fetches the last 20 rows in the table and shows only one row per yzr.name and orders by yzr.favorite_count

SELECT      gzt.name AS gazete,
            yz.*,
            yzr.name,
            yzr.gazete_id,
            yzr.yazar_image_link,
            yzr.favorite_count,
            DATE_FORMAT(tarih,'%d.%m.%Y') As formatted_tarih

FROM        yazar AS yzr

INNER JOIN  yazi   yz  ON yzr.id=yz.yazar_id
INNER JOIN  gazete gzt ON yzr.gazete_id = gzt.id

GROUP By    yzr.name
ORDER BY    yzr.favorite_count DESC

LIMIT 20 OFFSET 0

As you have no range specific functions applied in the SELECT clause I don't understand why you need the GROUP BY clause.

The expression is way to simple to be optimized (its hard to see how you can go wrong).

For any optimization advice you will need to provide the table definitions and an explanation of the Normal Form the data has been decomposed to. Optimization of SQL queries usually lies in the table structure and not decomposing the data to full actualized third Normal Form.


AngularJS shopping cart using local storage Two weeks ago I started writing some Javascript and AngularJS code for a customer. As a proud newby I must also be aware of the dangers on the big bad Internet. In particular vulnerabilities such as XSS attacks.

I've already performed a vulnerability scan and this showed I didn't restrict session cookies to httponly. However nothing about the Javascript or AngularJS scripting.

My familiarity with JavaScript and AngularJS is somewhat limited.

var kvv = angular.module('kvv', ['ngStorage']);

kvv.controller('CartController', ["$scope", "$localStorage", "$sessionStorage", "$timeout", function($scope, $localStorage, $sessionStorage, $timeout) {

if ($localStorage.items === undefined) {
    $localStorage.items = [];
};

$scope.localStorage = $localStorage

$scope.remove = function(index) {
  $localStorage.items.splice(index, 1);
};

$scope.checked = false;

$scope.addToCart = function(index, title, desc, price, timeout) {

  $scope.checked = true;
  var found = false;
  angular.forEach($localStorage.items, function(items) {
      if (items.id  === index) {
        $timeout(function() {
          (items.quantity++);
          $scope.checked = false;
        }, 750);
        found = true;
    }
  });
  if (!found) {
    $timeout(function() {
        $localStorage.items.push(angular.extend({
        id: index,
        title: title,
        quantity: 1,
        price: price}, index))
        $scope.checked = false;
        },750);
    }
};

$scope.itemWithoutBtw = function(index) {
    var itemWithoutBtw = 0;
    angular.forEach($localStorage.items, function(items) {
        itemWithoutBtw += items.price / 106 * 100;
    })
    return itemWithoutBtw;
};

$scope.total = function(index) {
        var total = 0;
        angular.forEach($localStorage.items, function(items) {
            total += items.quantity * items.price;
        })
        return total;
};

$scope.totalBtw = function(index) {
        var totalBtw = 0;
        var total = $scope.total();
        angular.forEach($localStorage.items, function(items) {
            totalBtw = total / 106 * 6;
        })
        return totalBtw;
};

$scope.totalWithBtw = function(index) {
        var totalWithBtw = 0;
        angular.forEach($localStorage.items, function(items) {
            totalWithBtw += (items.quantity * items.price) + (items.quantity * items.price  / 100 * 6);
        })
        return totalWithBtw;
};
$scope.totalWithoutBtw = function(index) {
        var total = $scope.total();
        var totalBtw = $scope.totalBtw();
        var totalWithoutBtw = 0;
        angular.forEach($localStorage.items, function(items) {
            totalWithoutBtw = total - totalBtw;
        })
        return totalWithoutBtw;
};

$scope.orderTotal = function(index) {
        var orderTotal = 0;
        angular.forEach($localStorage.items, function(items) {
            orderTotal += items.quantity;
        })
        return orderTotal;
};

$scope.orderDelete = function(index) {
    delete $localStorage.items;
    $localStorage.items = [];
    $('html, body').animate({scrollTop: 0}, 2500);
};

}]);

This is interesting,

you have quite a bit business logic in your code there..
If I were try to break this, I would definitely change the price of the items I buy in the local storage to 1 cent and see if the server side would accept that price. (It should not, and recalculate any pricing from item master data). Next I would place a breakpoint in some of your btw related routines and make it return 1 cent and see if the server side could be broken that way.

Other than that, I see no security flaws in there. Let me know if you want me to do a review of other aspects of your code.


Another way to run reports on a SpiceWorks SQLite database using a linked server in SSMS? I created a linked server in SSMS so that I could run reports on a SpiceWorks SQLite database.  there are a few quirks in the whole system.

These simple queries are becoming some of the most expensive queries on my reporting SQL Server, I kind of know why, is there a way that I can do this differently?

The Code

I have two queries.

TimeToCloseByAssignee

SELECT
    id AS ticketNum
    , Summary
    , Assignee
    , closed_at
    , created_at
    , ROUND((DATEDIFF(Hour, created_at, closed_at))/24. , 2) AS DaysOpen
FROM OPENQUERY(SPICEWORKS,'
    SELECT tickets.id
        , users.email as Assignee
        , substr(tickets . summary, 1,100) AS Summary
        , tickets.closed_at
        , tickets.created_at
    FROM tickets
        INNER JOIN users ON tickets.assigned_to = users.id
WHERE status=''closed''
        AND
        master_ticket_id IS NULL;')

GetCreatorByTicketID

SELECT * FROM OPENQUERY(SPICEWORKS, '
    SELECT users.email
        , tickets.id
    FROM tickets
        INNER JOIN users ON tickets.created_by = users.id')
WHERE id = @TicketID

I am using SSRS to handle the nice presentation of the data.

the problem with this, is that I can't give the underlying SQLite query boundaries from SSRS Parameters, it just doesn't like it. There are some questions on StackOverflow with answers and ways to do it, but this was the only way that I could make it work the way I wanted it to.   perhaps I wasn't interpreting the answers correctly.  either way I don't think it would have been clean and straightforward anyway.

Any thoughts?

Ouch.... you are not filtering the data you are selecting from the OPENQUERY source....

But, let's get some things straight first.... you say:

  either way I don't think it would have been clean and straightforward anyway

Using OPENQUERY automatically excludes any pretext of 'clean and straightforward'. They are incompatible.

So, as soon as you use OPENQUERY you can assume that you have already got ugly code (with a fat ass).

The trick is to use makeup - to cover up the blemishes.

In this case, you can filter the records, but you need to be creative.... you have:

SELECT * FROM OPENQUERY(SPICEWORKS, '
    SELECT users.email
        , tickets.id
    FROM tickets
        INNER JOIN users ON tickets.created_by = users.id')
WHERE id = @TicketID

I would suggest the following:

declare @ticketsql as nvarchar(1024)
;

set @ticketsql = '
    SELECT users.email
        , tickets.id
    FROM tickets
        INNER JOIN users ON tickets.created_by = users.id
    WHERE tickets.id = ' + @TicketID
;

select * FROM OPENQUERY(SPICEWORKS, @ticketsql)


Multiple uses of $window.scroll I'm using $window.scroll twice in a jQuery script for different uses. Should I try and combine them?

jQuery(document).ready(function($) {

// Add class .fixed when .site-header hits top of viewport

var distance = $('.site-header').offset().top,
    $window = $(window);

$window.scroll(function() {
    if ( $window.scrollTop() >= distance ) {
        $(".site-header").addClass("fixed");
    }
    if ( $window.scrollTop() < distance ) {
        $(".site-header").removeClass("fixed");
    }
});

// Add class .scrolled on scroll to control show/hide of back-top button

$(window).scroll(function() {
    var scroll = $(window).scrollTop();

    if (scroll >= 200) {
        $("body").addClass("scrolled");
    }
    if (scroll <= 200) {
        $("body").removeClass("scrolled");
    }
});

});

You have an extra if statement in these that should be structured as an if/else statement, or even an if/else if statement

var distance = $('.site-header').offset().top,
    $window = $(window);

$window.scroll(function() {
    if ( $window.scrollTop() >= distance ) {
      $(".site-header").addClass("fixed");
    }
    if ( $window.scrollTop() < distance ) {
      $(".site-header").removeClass("fixed");
    }
});

You should write it like this instead:

$window.scroll(function() {
    if ( $window.scrollTop() >= distance ) {
        $(".site-header").addClass("fixed");
    } else if ( $window.scrollTop() < distance ) {
        $(".site-header").removeClass("fixed");
    }
});

If one is true you don't need to check the other condition.

Same thing with the other set of code as well

$(window).scroll(function() {
    var scroll = $(window).scrollTop();

    if (scroll >= 200) {
        $("body").addClass("scrolled");
    }
    if (scroll <= 200) {
        $("body").removeClass("scrolled");
    }
});

should be something like this

$(window).scroll(function() {
    var scroll = $(window).scrollTop();

    if (scroll >= 200) {
        $("body").addClass("scrolled");
    } else if (scroll < 200) {
        $("body").removeClass("scrolled");
    }
});

You were actually performing both if statements if scroll = 200, which is probably not what you wanted so I changed the equality expression to reflect what I thought you wanted.

Both functions could have an if/else instead of an if/else if because if you don't meet the criteria for the first statement you meet the criteria for everything else with the else if.

I also removed an unnecessary variable in the second function as well

$window.scroll(function() {
    if ( $window.scrollTop() >= distance ) {
        $(".site-header").addClass("fixed");
    } else {
        $(".site-header").removeClass("fixed");
    }
});

$(window).scroll(function() {
    if ($(window).scrollTop() >= 200) {
        $("body").addClass("scrolled");
    } else {
        $("body").removeClass("scrolled");
    }
});

Really what you are doing here is overloading that function, but you don't give it any options which function to run, so what would most likely happen is that both sets of functions will run.

I am not sure what the difference is between $(window).Scroll(function() ... and $window.Scroll(function()... is, maybe that is how you distinguish between which version of the function to run.

If you are going to run both code at the same time you could just put them together inside one function call.


Splitting a command line in C Is this a good C program for splitting a command line without doing any expansion on it? Don't worry too much about main() and the output -- those are for testing.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <assert.h>
void pullwhitespace(char **input);
int main (int argc, char **argv)
{
        if (argc != 2) {
                fputs("Must have exactly one command line argument\n", stderr);
                return 64;
        }
        char *input = argv[1];
        char *outstring = malloc(strlen(argv[1]) + 1);
        char *outptr = outstring;
        char a;
        pullwhitespace(&input);
NORMAL:
        /* We are in an unquoted part of the input */
        switch ((a = *input++))
        {
        case ' ' :
        case '\t':
                *outptr++ = '\0';
                pullwhitespace(&input);
                goto NORMAL;
        case '\n':
        case '\0':
                goto DONE;
        case '\\':
                if (((a = *input++)) == '\n') {
                        break;
                } else if (a) {
                        *outptr++ = a;
                        goto NORMAL;
                } else {
                        goto FAIL;
                }
        case '\'':
                goto QUOTE;
        case '"':
                goto DQUOTE;
        default:
                *outptr++ = a;
                goto NORMAL;
        }
        assert(0);
QUOTE:
        /* We are in a single quoted string */
        while (((a = *input++)) != '\'') {
                if (a) {
                        *outptr++ = a;
                } else {
                        goto FAIL;
                }
        }
        goto NORMAL;
DQUOTE:
        /* We are in a double quoted string */
        switch ((a = *input++))
        {
        case '\0':
                goto FAIL;
        case '\\':
        {
                char b = *input++;
                switch (b)
                {
                case '\0':
                        goto FAIL;
                default:
                        *outptr++ = a;
                case '"':
                case '\\':
                        *outptr++ = b;
                case '\n':
                        break;
                }
        }
        goto DQUOTE;
        default:
                *outptr++ = a;
                goto DQUOTE;
        case '"':
                goto NORMAL;
        }
        assert(0);
FAIL:
        fprintf(stderr, "%s: Invalid string input\n", argv[0]);
        return 1;
        assert(0);
DONE:
        *outptr++ = '\0';
        *outptr++ = '\0';
        errno = 0;
        fwrite(outstring, 1, (size_t)(outptr - outstring), stdout);
        int error = errno;
        if(ferror(stdout)) {
                fprintf(stderr, "%s: write error: %s\n", argv[0], strerror(error));
                return 1;
        }
        fflush(stdout);
        if ( errno != 0) {
                fprintf(stderr, "%s: write error: %s\n", argv[0], strerror(error));
                return 1;
        }
        return 0;
}
void pullwhitespace (char **inputptr)
{
        char *input = *inputptr;
        char a;
        while (( a = *input)) {
                if (a != ' ' && a != '\t') {
                        return;
                }
                input++;
        }
}

I know that it is FULL of goto statements (more than every other program I have ever made put together) -- is that okay?

The reason for the goto statements is that this is meant as a lightweight state machine. Intended applications include parsing command line arguments or (possibly - this is far fetched) for the parser (not supporting code) to be the basis of a kernel module to parse shebang lines, allowing multiple arguments to be passed to a shebang interpreter.

I doubt that this code actually works. Especially pullwhitespace doesn't do what you probably intended (hint: it doesn't modify the argument, it makes a copy of the pointer and advances it). Have you tested it before submitting it for review here?

Anyway: You might want to have a look here for alternative ways to implement a state machine. You approach is similar to Remo.D's answer in the linked question but I think his macros make it more readable.

A few things I noticed:

When you are in NORMAL and read a white space you do this *outptr++ = '\0'; Given that strings in C are NULL terminated character sequences this seems dodgy as it will terminate the output right there unless you tell someone to read further by supplying a length. If you intend to split on white space then you should store the results in a char ** like argv.
Again in NORMAL:

case '\\':
        if (((a = *input++)) == '\n') {
                break;
        } else if (a) {
                *outptr++ = a;
                goto NORMAL;
        } else {
                goto FAIL;
        }

So if you find a \ followed by a newline you break the switch which is followed by an assert(0). Normally assert should be used to state invariants as "this function expects this condition to be true and it is probably a bug in the program if it is not". Specifically assert(0) should be used as "should never execute this code path". It seems weird that external user input can trigger an assert just because the program doesn't like he input. Especially since you have a FAIL state anyway.

Also the else condition evaluation depends on the side effect of the if condition evaluation - not very nice. You should read the next character first in a separate line and then evaluate. This makes it much clearer. So this case could probably have been written as:

case '\\':
        a = *input++;
        if (a && a != '\n') {
               *outptr++ = a;
               goto NORMAL;
        }
        goto FAIL;

If you are in DQUOTE and encounter a \ then you read another character from the input. However that character simply gets swallowed because in the default case you do this: *outptr++ = a; which will write the \ to the output and skip the current character (held in b).


Moving average of a data series How can I rework the code up that ema(x, 5) returns the data now in emaout? I'm trying to enclose everything in one def.

Right now its in a for loop and a def.

x = [32.47, 32.70, 32.77, 33.11, 33.25, 33.23, 33.23, 33.0, 33.04, 33.21]

def ema(series, period, prevma):
    smoothing = 2.0 / (period + 1.0)
    return prevma + smoothing * (series[bar] - prevma)

prevema = x[0]
emaout =[]

for bar, close in enumerate(x):
    curema = ema(x, 5, prevema)
    prevema = curema
    emaout.append(curema)

print
print emaout

x could be a NumPy array.

ema uses series in one place, where it references series[bar]. But bar is a global variable which is frowned upon. Let's pass series[bar] instead of series to ema.
That would be series[bar] in the for loop, but series[bar] is the same thing as close, so let's pass that instead.
ema's period is only used to calculate smoothing. Let's calculate smoothing outside and pass it in.
But now ema's a single line, so let's inline it
Let's put all of that logic inside a function
We use enumerate in the for loop, but we don't use bar

Result:

x = [32.47, 32.70, 32.77, 33.11, 33.25, 33.23, 33.23, 33.0, 33.04, 33.21]

def function(x, period):
    prevema = x[0]
    emaout =[]

    smoothing = 2.0 / (period + 1.0)
    for close in x:
        curema = prevma + smoothing * (close - prevma)
        prevema = curema
        emaout.append(curema)

    return prevema
print
print function(x, 5)

We can better by using numpy.

def function(x, period):
    smoothing = 2.0 / (period + 1.0)
    current = numpy.array(x) # take the current value as a numpy array

    previous = numpy.roll(x,1) # create a new array with all the values shifted forward
    previous[0] = x[0] # start with this exact value
    # roll will have moved the end into the beginning not what we want

    return previous + smoothing * (current - previous)


Pascal Triangle c# code space complexity can you give me your impression of my implementation of Pascal triangle problem?
I'm especially interested in space complexity evaluation
thank you all!

public List<List<int>> generate(int A)
{
    List<List<int>> result = new List<List<int>>();

    int currenctRowLength = 1;
    List<int> previousRow = new List<int>();
    for (int i = 0; i < A; i++)
    {
        List<int> row = new List<int>();
        row.Add(1);
        currenctRowLength = i + 1;
        int j = 1;
        while (j < currenctRowLength - 1)
        {
            row.Add(previousRow[j] + previousRow[j - 1]);
            j++;
        }

        if (i > 0)
        {
            row.Add(1);
        }

        previousRow=row;
        result.Add(row);
    }
    return result;
}

As I think greybeard was trying to say, unless you are know that memory consumption/allocation costs are a serious concern, then this code is fine from a memory point of view. The space-complexity is the same as the output (quadratic in A), so you can't do better than that.

There a couple of small things to be said:

as GreyBeard says, A is completely meaningless, and should really have a lowerCamelCase name as a parameter (e.g. dimension, size?); generate ought to be GeneratePascalTriangle or something meaningful (public -> UpperCamelCase) (msdn reference)
currenctRowLength should be currentRowLength, and should only be defined inside the for loop (i.e. where it is assigned); I like that this is its own variable.
you are using a while loop over j, when a for could be more readable (simply because people are used to reading for-loops), and doesn't leak j into the outer scope.
List<List<int>> is a pretty horrid type to be returning: much better to have IReadOnlyList<IReadOnlyList<T>> if the return value is meant to be immutable (which is assignable from List<List<T>> or T[][]).
I'd be inclined to initialise previousRow to null, rather than an empty list: it isn't read until i == 2; assigning it to a 'meaningful' value is defensive and only can work to obscure bugs in the rest of the code.

Mindless Performance Musing

If memory usage/allocations/GC hammering is a real concern, then you can improve matters by not using resizing lists. You havn't given us any idea of what the code is actually meant to do, so it's a bit hard to suggest an alternative, but you could either use arrays (which obvious are non-dynamic) or you can preserve the signature by creating lists with a given initial capacity with the .ctor(int) overload. Note that this will give a performance benefit, but change the time-complexity (which is also quadratic in A).

Code

Putting all of that together (and using arrays instead of lists, because mindless performance is fun, and I don't think it harms the readability here much):

/// <summary>
/// Generates a pascal triangle with the given number of rows
/// </summary>
public static IReadOnlyList<IReadOnlyList<int>> GeneratePascalTriangle(int numberOfRows)
{
    int[][] result = new int[numberOfRows][];

    int[] previousRow = null;
    for (int i = 0; i < numberOfRows; i++)
    {
        int currentRowLength = i + 1;

        int[] row = new int[currentRowLength];
        result[i] = row;

        // start and end
        row[0] = 1;
        row[currentRowLength - 1] = 1;

        // middle
        for (int j = 1; j < currentRowLength - 1; j++)
        {
            row[j] = previousRow[j] + previousRow[j - 1];
        }

        previousRow = row;
    }

    return result;
}


Receive array of data and access employee info fields This method receives an array of data and does some stuff in a database.  I need to get some reviews on it if possible.

public function doSomeStuff($arr = array())
{
    $id = $arr['Employee']['id'];
    $name = $arr['Employee']['name'];
    $status = $arr['Employee']['status'] == 'Disabled' ? 0 : 1;
    $user_id = $arr['Employee']['user_id'];
    $query = "update `mytable` set `status` = $status, `name`=$name   WHERE `user_id` = ?";
    self::_runthis($query, array($user_id));
}

I am looking to see if this is fool-proof for the data it will receive and will process it.

Instead of accessing the employee key every time, just pass in the array rooted at the Employee key
What happens if all of the array indexes are not set?  You should either check for that or use an object on which you know they all exist

Important mostly for data integrity (what if you create an Employee array somewhere and forget a status or something?)
But also accessing non-set array keys issues a notice

I suspect there's something wrong with your class design as a whole.  In particular, the same class should probably not have a method to update an employee and a method to run a query (you don't happen to have something extending some vague DB class do you...?)
Why is name not a place holder like the user_id?
Use real names when posting code here -- doSomeStuff is very vague, and that hampers our ability to review
A text status should probably not be being passed into this since it seems like you're dealing with a low level record here
Don't quote entity names unless you need to.  It breaks compatibility across SQL-dialects for no reason
What if $name has spaces?  What if it has a ' in it?  Look into SQL injection and prepared statements.  (They're not just about security.  There also about correctness.  In it's current form, your code has a very major bug.)
A default value for the parameter shouldn't be provided.  Would you want someone to call $obj->doSomeStuff();


Count bits that differ between integers X and Y I'm trying to solve the following problem :

  You are given two integers, X and Y.  Calculate the number of bits that will be changed in order to convert integer X to integer Y.

Here is my solution

function findNumberOfBits(x, y){
    var bitCount = 0;
    var z = x ^ y;  //XOR x and y
        while (z !== 0){
          bitCount += z & 1;
          z = z >> 1;  //shift Z by 1 bit to the right
          //console.log("bitCount ", bitCount);
          //console.log(z);
         }
    return bitCount;
}

var result = findNumberOfBits(12, 16);

Is this correct? My tests seem to say yes, but just to make sure.
Is this the most efficient way to write it in JavaScript?

Comments

Your current comments aren't all that helpful, as they are just repeating excactly what the code already told me.

To improve them, think about why you are doing what you are doing, and what the result represents. For example:

  var z = x ^ y;  //XOR x and y

Right, so ^ is bitwise xor. But why are you doing this? What is z after this operation?

var z = x ^ y;  // set bits in z that are different in x and y to 1.

This is a bit more helpful (but you can improve on the phrasing if you want).

Misc

Your indentation is off a bit.
remove debug statements

If you do this, your code might look like this:

function findNumberOfBits(x, y) {
    var bitCount = 0;
    var z = x ^ y;  // set bits in z that are different in x and y to 1.
    while (z !== 0) { // not all bits are 0
        bitCount += z & 1; //  add least significant bit
        z = z >> 1;  // iterate over z; set next bit as lsb
    }
    return bitCount;
}

The comments might be a bit much (you can remove some if you want), but they are not just repeating the code.

  Is this correct? My tests seem to say yes, but just to make sure.

Looks good to me.

  Is this the most efficient way to write it in JavaScript?

If your code should work platform-independent, probably. Otherwise, no. You need the xor, but to count the number of set bits in the result, there might be better approaches.

What you are calculating is called the hamming distance, and here you can see some efficient implementations for the hamming weight (the count of set bits).

See also this question about counting the number of set bits in an integer.


Moving and organizing files in download folder This is a Python script that I use on my Android tablet.  It moves the files in the Download folder to a folder on the external SD card.  The files are sorted based on their extensions and are moved to folders (for each different type of extension).  If it encounters a new file type, it will create a new folder.  If the file name already exists, the new file will be renamed by appending the first five hash digits to the end of the file name.  The original file is deleted.

How can I improve my program?  What changes should I make?

import os
import hashlib

download_dir = "/sdcard/Download/"
save_dir = "/mnt/external_sd/Download/"

FileList = os.listdir(download_dir)
for File in FileList:
    #print File
    extension = ''.join(os.path.splitext(File)[1])
    name = ''.join(os.path.splitext(File)[0])
    ext = extension.strip('.')

    if os.path.isdir(download_dir + File):
        pass
    else:
        if os.path.exists(save_dir + ext + "/" + File):
            Data = open(download_dir + File, "r").read()
            m = hashlib.sha1()
            m.update(Data)
            h = (m.hexdigest())[0:5]
            file(save_dir + ext + "/" +name+"-"+h+"."+ext, "w").write(Data)
            print File, " ","-->"," ",name+"-"+h+"."+ext
            os.remove(download_dir + File)

        elif os.path.exists(save_dir + ext):
            Data = open(download_dir + File, "r").read()
            file(save_dir + ext + "/" + File, "w").write(Data)
            print File #, " ","-->"," ",name+"."+ext
            os.remove(download_dir + File)

        if os.path.exists(save_dir + ext) != True:
            os.makedirs(save_dir + ext)
            Data = open(download_dir + File, "r").read()
            file(save_dir + ext + "/" + File, "w").write(Data)
            print File , " ","-->"," ","/"+ext+"/"+name+"."+ext
            os.remove(download_dir + File)

File, FileList, and Data are neither class names nor constants; variable names should be lower_case in Python.  I'd call it filename instead of file to avoid clashing with Python's built-in file() constructor.

Avoid calling splitext() twice with name, extension = os.path.splitext(filename).

For portability, clarity, and to avoid accidental omission of the path component separator, it would be better to use os.path.join(dirname, filename) instead of dirname + filename, even if you only plan to run it on Android.

You open files for reading and writing but don't close them.  In particular, it's important to make sure the destination file handle is successfully closed before removing the source, or you might lose data.  Also, it's preferable to call open() instead of file().

The structure of your program can be simplified.

if a:
    ... # body a
else:
    if b:
        ... # body b
    elif c:
        ... # body c
    if c != True:
        ... # body d

can be written as

if a:
    ... # body a
elif b:
    ... # body b
elif c:
    ... # body c
else:
    ... # body d

assuming that bodies b and c do not influence the c != True test.  Also, I believe you intend for cases b, c, and d to be mutually exclusive, but I suspect it may be possible to execute both bodies b and d in your original program.

Bodies b, c, and d are very similar to each other.  You should try to reduce the redundancy in your code.  One approach would be to copy the contents to a tempfile in the destination directory (computing the hash as you copy, in case you need it), then successively try to os.rename() the temporary file to various destination filenames.


Secure logout PHP code with cookies I have the following PHP logout script and I would like to know the loopholes present in it. How can I improve it?

<?php

require_once 'config.php';

if(session_id() != "") {
    session_destroy();
}

if(isset($_COOKIE[session_name()])) {
    // Delete the session cookie
    setcookie(session_name(), "", time() - 1);
}

if(isset($_COOKIE['authUser'])) {
    $cookie_contents = $_COOKIE['authUser'];
    list($selector, $plain_token) = explode(':', $cookie_contents);

    //Empty the values
    $query = $dbh->prepare("UPDATE auth_tokens SET token=:token, expires=:expires WHERE selector =:selector");
    $query->execute(array(
        ":token" => "",
        ":expires" => "",
        ":selector" => $selector
    ));

    $count = $query->rowCount();
    if($count == 0) {
        die('Something went wrong..Could not log you out.' );
    }

    // Delete the authentication cookie
    setcookie('authUser', "", time() - 1);
}

header('Location: login.php');

I would suggest that, as you are looking to implement your code around session management, you would think about it more globally.  Having several lines of procedural code to manage session logout, rather than a logout() method on a class that provides methods to manage sessions, limits re-use of this code.  Why should you need to route a user to a specific URL to log them out as opposed to just having ability to do something like Session::destroy() from anywhere in your application, with that method basically doing what is in your code now?

Perhaps take a look at existing libraries to see if they meet you needs (for example something like https://github.com/auraphp/Aura.Session).

For better depth of knowledge around securely handling sessions, I would recommend you read and understand the PHP sessions "basics" documentation at http://php.net/manual/en/features.session.security.management.php

In particular, I would point you to this section:

  Session Data Deletion

  Obsolete session data must be inaccessible and deleted. Current
  session module does not handle this well.

  Obsolete session data is better to be removed as soon as possible.
  However, active sessions MUST NOT be removed immediately. To satisfy
  these requirements, you MUST implement time-stamp based session data
  management by yourself.

  Set and manage expiration time-stamp in $_SESSION. Prohibit access to
  obsolete session data. When obsolete session data access is detected,
  it is advised to remove all authenticated status from the user's
  sessions and force them to re-authenticated. Obsolete session data
  access could be an attack. To do this, you must keep track active
  sessions per user.

  Note: Access to obsolete session could happen by unstable network
  and/or concurrent access to web site also. Server tried to set new
  session ID via cookie, but Set-Cookie packet may not be reached to
  client due to lost connection. One connection may issue new session ID
  by session_regenerate_id(), but another conncurrent connection may not
  get the new session ID yet. Therefore, you must prohibit access to
  obsolete session a while later. i.e. Time-stamp based session
  management is mandatory. In short, do not destroy session data by
  session_regenerate_id() nor session_destroy(), but use time-stamp to
  control access to session data. Let session_gc() to remove obsolete
  data from session data storage.
  In short, do not destroy session data by session_regenerate_id() nor
  session_destroy(), but use time-stamp to control access to session
  data. Let session_gc() to remove obsolete data from session data
  storage.

Note particularly here the bolded text.

What you should take away from this is that you need to begin to think of sessions and authentication (login) separately.  Really the sole purpose of the session is to persist data on the server between client calls.  PHP Sessions are not authentication or reliable data access control mechanisms.

You do not need to "destroy" the session at all upon a logout event in your application.  You can simply regenerate_session_id() upon this change in authentication state (just as you would need to do when user logs in). But before that action you would need to programmatically set the data to expire on the current session id.  This is the only reliable way to "lock out" this session data.  This of course means that you would also always need to validate that the session id passed to the page does not contain expired data directly after calling session_start() near the beginning request processing and treating this as an invalid session if expired data is encountered.

Also, think of it from an application functionality perspective.  Just because a user is logged out, doesn't mean your application might not need to still store some data in session.  For example, maybe you might need to store something in session before performing a redirect to let the destination page know some piece of information (like the fact that the user just logged out).  That target page might need to change the display on page the user is redirected to (like perhaps a successful logout message in your case).

if(session_id() != "") {
    session_destroy();
}

Why the conditional here? It doesn't seem necessary. Would you ever expect to be in a condition when session_id would NOT be set anyway? (Again I question use of session_destroy() at all based on earlier comment)

if(isset($_COOKIE[session_name()])) {
    // Delete the session cookie
    setcookie(session_name(), "", time() - 1);
}

Again, the conditional doesn't seem necessary.  Why would you ever get into the state where you have an active session and don't have this cookie set?

If you are running your sessions in strict mode (you should be).  Then this code is really not necessary, as the cookie would in essence be ignored going forward.  If you are trying to be specific about overall cookie size then this code still makes sense.

$cookie_contents = $_COOKIE['authUser'];
list($selector, $plain_token) = explode(':', $cookie_contents);

//Empty the values
$query = $dbh->prepare("UPDATE auth_tokens SET token=:token, expires=:expires WHERE selector =:selector");
$query->execute(array(
    ":token" => "",
    ":expires" => "",
    ":selector" => $selector
));

$count = $query->rowCount();
if($count == 0) {
    die('Something went wrong..Could not log you out.' );
}

I would consider taking this functionality and building a class (or function) around it (perhaps userAuthPersistence or similar).  You could get this to something like:

if(isset($_COOKIE['authUser'])) {
    try {
        userAuthPersistence::persist($_COOKIE['authUser']);
    } catch (Exception $e) {
        // do something here to handle error
    }

    // Delete the authentication cookie
    setcookie('authUser', "", time() - 1);
}

list($selector, $plain_token) = explode(':', $cookie_contents);

I would consider moving away from using your own serialization methodology (in this case simply concatenating with :) in favor of using PHP's serialization methods, JSON, or some other well-structured, well-supported approach to serializing structured data into a string.  I think over time you will find them more powerful, more flexible, and less fragile.

$query = $dbh->prepare("UPDATE auth_tokens SET token=:token, expires=:expires WHERE selector =:selector");

This line of code is to long and harder to read.  Consider getting in the habit of making sure the SQL within your PHP is visually differentiated from surrounding code to make it easier to read.

$sql = <<<'EOT'

UPDATE auth_tokens
SET
    token = :token,
    expires = :expires
WHERE selector = :selector

EOT
$statement = $dbh->prepare($sql);

I like using heredoc or nowdoc syntax (especially on longer queries), but even breaking up the SQL string across lines in a standard string assignment works.

Note also the change in variable name from $query to $statement which is my guess as to what you are actually representing in this variable.

die('Something went wrong..Could not log you out.' );

It is generally a bad idea to just echo errors to standard out.  Consider at least logging an error here.  You also might want to have portion of code closer to display layer handle the actual end user messaging.


Catching multiple types of exceptions when writing JSON I have a class which throws a lot of exceptions:

    try {
        mapper.writeValue(outStream, myVal);
    } catch (JsonGenerationException e) {
        e.printStackTrace();
    } catch (JsonMappingException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }

I think this looks messy. Is it ok to just catch a generic Exception and not catch each individual exception if within the catch clause of each exception I'm just printing the stack trace?

It is okay to just catch Exception and print the stack trace.
However rather than catching the top level Exception class, you may want to look at the Java 7 exception handling: http://docs.oracle.com/javase/7/docs/technotes/guides/language/catch-multiple.html

In your case:

try {
    mapper.writeValue(outStream, myVal);
} catch (JsonGenerationException | JsonMappingException | IOException e) {
    e.printStackTrace();
}


Print the upper half triangle of a matrix Here is what I've written for a program to print upper half triangle of a matrix. Now I need a way to do it without using goto:

    #include <iostream>
using namespace std;

int main()
{
    int a[10][10], r, c, i, j;
    cout << "Enter the number of rows and columns";
    cin >> r >> c ;
    cout << "Enter the matrix :";
    for(i=0;i<r;++i)
    {
                    for(j=0;j<c;++j)
                    {
                                    cin >> a[i][j];
                    }
    }
    cout << "The matrix is : \n";
    for(i=0;i<r;++i)
    {
                    for(j=0;j<c;++j)
                    {
                                    if(j<i)
                                    goto loop;
                                    cout << a[i][j] << "\t";
                                    loop:
                                         if(j<i)
                                         cout<< " \t";
                                    if(j==(c-1))
                                    cout<<"\n";
                    }
    }
    cin >> r;
}

Any help is appreciated.

When looking at your code I just want to say: Please reconsider your indentation!. This code should be rather easy to read, but you've complicated it quite a bit by not using braces around the blocks of the if statements, and leaving them at the same indentation, and why the excessive amount of indentation with the for loops?

So here are some style suggestions:

Keep indentation consistent at 4 spaces – Don't change indentation between different blocks, but keep it the same all the time
Always indent after if, for, while – Don't skip it like you've done in your inner loop
Add space after , and ;, and between operators – Having the blocks like for(i=0;i<r;++i) makes your code hard to read. Use spaces like in for (i=0; i<r; ++i) or if (j == (c-1)))
Use descriptive variable names – Whilst i and j is common as loop variables, please use more descriptive names like row, column, or matrix for the other variables to describe what they represent
Use braces around the blocks – Your code is prime example of the need to use braces around the blocks of if, for and while. By always adding braces around your code blocks, you are sure that the correct code is executed (and your 'faulty' indentation) won't give you any surprises!

And here are some code suggestions:

Use the if statement properly as suggested by ratchet freak – Either use it directly, or depending on which triangle you want to output a loop where you avoid the if completely. i.e. for (j=0; j<i && j < c; ++j), possibly with an indentation before that if needed and not zero based. If you don't understand this, do go with the suggestion by ratchet freak
Bug/Feature: If you enter r or c larger than 10, it fails miserably – You should add error handling if the input is outside of your intended range, or you should make your code more dynamic allowing for larger ranges.


A roman to integer converter For an interview question I made a roman to integer converter:

def romanToInt(self, s):
        """
        :type s: str
        :rtype: int
        """
        mapping = {"I": 1, "V":5, "X":10, "L":50, "C":100, "D":500, "M":1000}
        numeral_list = list(s)

        num_list = [[mapping[i], -1] for i in numeral_list]
        count = 0
        i = 0
        while(i < len(num_list) - 1):
            if num_list[i] < num_list[i + 1]:
                count += num_list[i + 1][0] - num_list[i][0]
                num_list[i+1][1] = 1
                num_list[i][1] = 1
                i += 2
            else:
                count += num_list[i][0]
                num_list[i][1] = 1
                i += 1
        if num_list[-1][1] == -1:
            count += num_list[-1][0]
        return count

As you can see I sometimes miss the last digit because I didn't want to get an index error. To avoid that I added an extra attribute that would check if the last element was checked or not (in cases where s[len(s)-2] < s[len(s)-1], s[len(s)-1] is checked, but if s[len(s)-2] > s[len(s)-1] then s[len(s)-1] is not checked.

Having an extra check and using extra space just for one element is highly erroneous. Where am I going wrong in my logic?

You should stick to variable names according to the python style guide, PEP8. This means using lower_case for variables and functions.

You could also use more descriptive names, for example roman instead of s.

The complicated checking you do with having 2-tuples and deciding on that can be circumvented by iterating over zip(numbers, numbers[1:]) to have both the number and the following number available at the same time. Still, we need to manually add the last digit at the end. Similar to the answer to this question I would propose to use something like:

def roman_to_int(roman):
    """Convert from Roman numerals to an integer."""
    values = {'M': 1000, 'D': 500, 'C': 100, 'L': 50,
              'X': 10, 'V': 5, 'I': 1}
    numbers = [values[char] for char in roman]
    total = 0
    for num1, num2 in zip(numbers, numbers[1:]):
        if num1 >= num2:
            total += num1
        else:
            total -= num1
    return total + num2


DRY Up Rails HTTP Basic Auth Can anyone think of a way to DRY up this code or is there a way to make it more efficient or quicker. Its what authenticates a user with my API and i want to make it as fast as possible.

def authenticate!
  authenticate_or_request_with_http_basic do |access_key, secret_key|
    @current_app = App.find_by_access_key(access_key)
    @current_app.secret_key == secret_key ? true : false
  end
end

There is not much to say, just two notes:

Common mistake of redundant boolean check: some_boolean ? true : false -> some_boolean.
You can either use the bang find method or check the returned value, but not just use without a check.

So simply:

def authenticate!
  authenticate_or_request_with_http_basic do |access_key, secret_key|
    @current_app = App.find_by_access_key(access_key)
    @current_app && @current_app.secret_key == secret_key
  end
end


Functionality for automatic retry after exception I have made the following system to automatically retry network calls if some exception is thrown. (Earlier posted on stackOverFlow, solution inspired by what @Boris answered)

public static <T> Optional<T> autoRetry(@NonNull final DoWork<T> task, @NonNull final Optional<Predicate<T>> resultRejector) {

        for (int retry = 0; retry <= StaticData.NETWORK_RETRY; ++retry) {

            try {

                Thread.sleep(retry * StaticData.NETWORK_CALL_WAIT);
                final T resultAfterWork = task.doWork();
                /**
                 * If the result was not
                 * desirable we RETRY.
                  */
                if(resultRejector.isPresent() && resultRejector.get().apply(resultAfterWork))
                    continue;
                /**
                 * Else we return
                 */
                return Optional.fromNullable(resultAfterWork);
            } catch (InterruptedException | UnknownHostException e) {
                e.printStackTrace(); //To be replaced by proper logging
                return Optional.absent();
            } catch (IOException e) {
                e.printStackTrace(); //To be replaced by proper logging
            }
        }
        return Optional.absent();
    }

I take care to not retry after InterruptedException &
UnknownHostException.
I retry 5 times. After each failure I perform an exponential back
off, starting from 300ms going upto 1500ms.
The predicate is used to verify the validity of the expected result.

The DoWork class :

public abstract class DoWork<T> {
    protected abstract T doWork() throws IOException;
}

I did not use Supplier because I need to throw an IOException.

Here is an example of how I use it. (this example updates the GCM Id)

//Context is android specific stuff, basically if app dies, reference will return null.

public static boolean updateGCM(final long id, final WeakReference<Context> reference) {

final String regId = autoRetry(new DoWork<String>() {
    @Override
    protected String doWork() throws IOException {

        final Context context = reference.get();
        if(context == null)
            return "QUIT";
        return GoogleCloudMessaging.getInstance(context)
                .register("XXXXXXXXXX");
    }
}, Optional.<Predicate<String>>of(new Predicate<String>() {
    @Override
    public boolean apply(@Nullable String input) {
        return TextUtils.isEmpty(input); //I need to retry if the returned GCM ID was null/empty
    }
})).orNull();
//Context becomes null when the application is exited
//If retires failed or application exited, we return
if(TextUtils.isEmpty(regId) || regId.equals("QUIT"))
    return false;

Log.i("Ayush", "Uploading newGcmId to server");
final Boolean result = autoRetry(new DoWork<Boolean>() {
    @Override
    protected Boolean doWork() throws IOException {

        //if everything is fine, send id to server
        return true;
    }
}, Optional.<Predicate<Boolean>>absent()).orNull(); //No need of predicate hence send absent

return !(result == null || !result);
}

So on a scale of 1 to 10 how professional is this approach ? :)
Also I do not want to retry if the network is genuinely failing, like
UnknownHostException, or something bad happens which wont get
resolved with retrying. Any possibility of improving exception handling ?

public static <T> Optional<T> autoRetry(@NonNull final DoWork<T> task, @NonNull final Optional<Predicate<T>> predicate) {

No predicate, please. Call it something like acceptResult or resultRejector... you know what I mean.

Thread.sleep(retry * StaticData.NETWORK_CALL_WAIT);

This doesn't do what you write, as the first time retry=0.

  I take care to not retry after InterruptedException & UnknownHostException.

The latter may be retryable as it may be a temporary network or DNS problem. But it's your choice.

final String regId = autoRetry(new DoWork<String>() {...

I'm too lazy to copy it in my Eclipse and without it, it's unreadable as I have no idea where the expressions ends. What about a private final DoWork<String> regIdDoWork = ... or alike?

Optional.<Predicate<Boolean>>absent()).orNull(); //No need of predicate hence send absent

How nicer would the world be if there was no Optional? :D Just null.

Note that even if you like Optional, you should probably use it for as return types only. This is the place where it may prevent bugs and where it doesn't add so much verbosity.


Optimize "nearest" Element query prototype I have devised a recursive function to grab the "nearest" element. This is a bit different than Element.closest, because instead of traversing just the immediate parent(s); it traverses up but checks left-right-up-down.
Is there a more efficient check for siblings that I am missing?

if (Element.prototype.nearest === undefined) {
  const nearest = (el, selector) => {
    if (el == null) return undefined;
    const prev = el.previousElementSibling;
    if (prev?.matches(selector)) return prev;
    const next = el.nextElementSibling;
    if (next?.matches(selector)) return prev;
    const parent = el.parentElement;
    if (parent?.matches(selector)) return parent;
    const relative = parent.querySelector(selector);
    if (relative) return relative;
    return nearest(el.parentElement, selector);
  };
  Element.prototype.nearest = function(selector) {
    return nearest(this, selector);
  };
}

const handleClick = (e) => {
  const btn = e.target;
  console.log(btn.nearest('.child'));
  console.log(btn.nearest('.grand-parent'));
  console.log(btn.nearest('#child-1'));
}

document.querySelectorAll('.btn').forEach(btn =>
  btn.addEventListener('click', handleClick));
.parent {
  display: flex;
}

      <button class="btn">
        Click
      </button>

      <button class="btn">
        Click
      </button>

Question

"Is there a more efficient check for siblings that I am missing?"

As done in the example code you gave. No.
You can rewrite the code to remove some of the unneeded complexity. See rewrites at bottom of answer.
Bug
The line...
    const relative = parent.querySelector(selector);

...will throw if parent is null
Unsafe prototype extension
It is a bad idea to add to an existing prototype without precautions. Element.prototype.nearest may not be used at the moment, but at any time it could be added to the DOM, breaking any code that you use it in.
There are three ways to avoid conflicts/future proof prototype extensions.
.1 100% safe. Don't!!! extending the prototype is an open trap ready to bite at any moment.
.2 Almost safe. Use a non standard naming convention eg Element.prototype.select_nearest The standard will not use this name, but this does not mean it is 100% safe as anyone else may use the name.
.3 Safe but awkward. Use a Symbol to define the prototype name. This will guarantee the uniqueness of the name. Unfortunately it does mean you will need to use bracket[notation] to access the property and share that property with code that needs access. For example
// define
const nearest = Symbol("nearest");
Element.prototype[nearest] = function() { /* ... code ... */ }   // prototype is global

// call
btn[nearest](".child");

To give access to the symbol in global scope use Symbol.for

{   // scope block
    const nearest = Symbol.for("nearest");  // find or create
    Element.prototype[nearest] = function() { log("Nearest") }  // prototype is global
}

// Access in unconnected scope
{
    Symbol.for("nearest")
    btn[nearest](".child");

    // or
    btn[Symbol.for("nearest")](".child");
}

Style
A note on style.
Its a bad habit not to delimit code blocks with {} eg if (prev?.matches(selector)) return prev; is safer as if (prev?.matches(selector)) { return prev; }
Unbound function?
Why add to the element prototype a function that calls an unbound arrow function. This just makes the code unduly complex.
You could either call the function nearest directly eg nearest(btn, '.child'); or keep it on the prototype and avoiding the secondary call and the need to pass this See rewrites.
Rewrite
Several rewrites as examples, though apart from the bugs and the points outlined above I don't see any real need for a rewrite.
As prototype
const nearest = Symbol.for("nearest");
Element.prototype[nearest] = function(selector) {
    const prev = this.previousElementSibling;
    if (prev?.matches(selector)) { return prev }
    const next = this.nextElementSibling;
    if (next?.matches(selector)) { return next }
    const parent = this.parentElement;
    if (parent) {
        if (parent.matches(selector)) { return parent }
        return  parent.querySelector(selector) ?? parent[nearest](selector);
    }
}

As function

function nearest(self, sel) {
    const prev = self.previousElementSibling;
    if (prev?.matches(sel)) { return prev }
    const next = self.nextElementSibling;
    if (next?.matches(sel)) { return next }
    const parent = self.parentElement;
    if (parent) {
        if (parent.matches(sel)) { return parent }
        return  parent.querySelector(sel) ?? parent.nearest(parent, sel);
    }
}

or
const matches = (el, sel) => el && el.matches(sel) ? el : undefined;
function nearest(self, sel) {
    const parent = self.parentElement;
    const sibling = matches(self.previousElementSibling, sel) ??
        matches(self.nextElementSibling, sel) ??
        matches(parent, sel) ??
        parent?.querySelector(selector) ??
        parent?.nearest(parent, selector);
}


Entity Relationship Diagram for Grade Calculator I was wondering if anyone would be able to review my Grade Calculator for a course ERD? I'm looking for ways to improve the current design (if any improvements need to be made that is) and if anyone could review the current MySQL query which is there. In order for this to be reviewed I was told I needed an SQL query to accompany it.

SELECT
    `yearGroup`.`id` AS `year_group_id`,
    `yearGroup`.`year` AS `year_group`,
    CONCAT(
    TRUNCATE
        (
            COALESCE(
                SUM(
                    `assignment`.`percentage_achieved` *(
                        `assignment`.`percentage_weighting` / 100
                    )
                ),
                0
            ) /(
            SELECT
                COUNT(*)
            FROM
                `unit`
            WHERE
                `unit`.`year_group_id` = `yearGroup`.`id`
        ),
        2
        ),
        '%'
) AS `grade`
FROM
    `assignment`
RIGHT JOIN `unit` ON `unit`.`id` = `assignment`.`unit_id`
RIGHT JOIN `yearGroup` ON `yearGroup`.`id` = `unit`.`year_group_id`
WHERE
    `yearGroup`.`course_id` = 1
GROUP BY
    `yearGroup`.`id`

I've got a few thoughts:

The user table is related to the course table in a funny way. At first when I was looking at the diagram I assumed users were students, but after staring for a while, that seemed less and less likely. I'm now under the impression that the user is the owner of the course, and I think that having the user_id in the course table is a little funny. I would use a separate mapping table like so:

The lookup tables seem fairly useless as you are using the actual value in the base tables instead of a lookup id. Changing it to look like this will add usefulness:

Your naming is a little strange. I would change the course table to be named courseInfo, and change yearGroup to be named course. This might be a little nitpicky as this is a generally well done diagram. :P

Notes in response to comments:

If you want to share a course between users, you will need to change your schema to be similar to mine. If, as suggested in the comments, only one user will ever use a course, then you're probably okay with the schema your using. I just don't generally think of a user as an attribute of a course. It's a little ambiguous: is that user a teacher, student, or what have you.
One thing to think about for lookup tables is that if you're not using an id, then the value is duplicated. If you ever update the lookup table, you have to change all values in the other table as well.
I do definitely think yearGroup is a misnomer. Where I come from, courses are offered by semester or some other frequency besides yearly.


Angular service for fetching and passing data with observables I have an Angular service consisting of three methods:

This method makes a call to my server to get a URL

private itemUrl = "http://fooserver/api/get/item/details";
getItemDetailsUrl(productId): Observable<any> {
    return this.http
        .get(this.itemUrl, {
            params: { productId: productId }
         })
        .flatMap(res => this.getItemDetails(res.json().itemUrl))
        .map(item => item);
}

This takes that URL from #1 and grabs the XML data returned from the request and passes it to #3

getItemDetails(url): Observable<any> {
    return this.http
        .get(url)
        .flatMap(item => this.parseItemFromUrl(item));
}

The XML passed is then converted to a JSON object using a third party library and then returned:

parseItemFromUrl(xml): any {
    let item;
    xml2js.parseString(xml.text(), function (err, result) {
        item = result.ItemLookupResponse.Items[0].Item[0];
    });
    return Observable.of(item);
}

This code does return the expected JSON object back. But since this is my first time really using Observables, I'd like to know of any improvements or changes I could make and if I used the appropriate array methods in the appropriate places.

One of the improvements would be to use an HttpClient instead of Http for making the API calls.

If you use HttpClient, you won't have to call the res.json() everytime you subscribe to the response.

To use HttpClient, you'll first have to import HttpClientModule from @angular/common/http. Then you'll have to add it to the imports array of your @NgModule decorator.

Then you can inject HttpClient as a dependency and you will have access to the usual Http methods on the instance of HttpClient as well.

With that, you first part of the code will be reduced to:

private itemUrl = "http://fooserver/api/get/item/details";
getItemDetailsUrl(productId): Observable < any > {
  return this.http
    .get(this.itemUrl, {
      params: {
        productId: productId
      }
    })
    .flatMap(res => this.getItemDetails(res.itemUrl));
}

Also, I figured, there was a redundant map operation after the flatMap which wasn't really required. Rest all, I guess is in place in your code and I'm not really if it can be improved further. But I'll be looking out for answers to this question just in case.


CSRF-prevention mechanism for a form I am using codeigniter framework in PHP. In signup form I have:

Controller:

$info['csrf_token'] = array('signup'=>$this->csrf_token('signup'));
echo $this->load->view('/signup',$info,TRUE);

// generate token:
public function csrf_token($form_name){
    if (function_exists('mcrypt_create_iv')) {
        $t = bin2hex(mcrypt_create_iv(32, MCRYPT_DEV_URANDOM));
    } else {
        $t = bin2hex(openssl_random_pseudo_bytes(32));
    }

    $sess_array = array();
    $sess_array[$form_name] = $t;
    $this->session->set_userdata('csrf', $sess_array);

    return $t;
}

View:

<form>
<input type="hidden" name="csrf_token" value="<?=hash_hmac('sha256', 'signup', $csrf_token['signup'])?>" />
</form>

Validation check on form submit:

$csrf_sess = $this->session->userdata('csrf');
if(hash_hmac('sha256', 'signup', $csrf_sess['signup'])==$_POST['csrf_token']){
    return true; // Valid request.
}else{
    return false;
}

Will it prevent website from CSRF (Cross-Site Request Forgery)?

From what it looks like, you are correctly checking the submission form for a valid CSRF token before processing, therefore a separate, malicious website [EDIT: without access to the hidden CSRF token on the submission form] would be unable to execute CSRF.

From here: Cross-Site Request Forgery (CSRF) is a type of attack that occurs when a malicious web site, email, blog, instant message, or program causes a user's web browser to perform an unwanted action on a trusted site for which the user is currently authenticated


Get the suffix for the date string Sometimes I have the date as part of the file name.  And then when Emacs autocompletes the file name, it puts the part of the date.  In these situations I have to write the rest of the date manually.  Here is a solution, so that I can press Super+d and the suffix of today's date is written in the buffer.  For example, if point is after "2019", when I press Super+d it becomes "2019-03-09".

(defun make-suffix (word1 word2)
  (cl-labels ((is-prefix (prefix list)
                         (cond ((null prefix) t)
                               ((null list) nil)
                               ((equal (car prefix) (car list))
                                (is-prefix (cdr prefix) (cdr list)))
                               (t nil)))
              (delete-x-elements (x list)
                                 (cond ((= x 0) list)
                                       ((null list) list)
                                       (t (delete-x-elements (- x 1)
                                                             (cdr list)))))
              (make-suffix-rec (l1 l2)
                               (cond ((null l1) l2)
                                     ((is-prefix l1 l2)
                                      (delete-x-elements (length l1) l2))
                                     (t (make-suffix-rec (cdr l1) l2)))))
    (concat (make-suffix-rec (string-to-list word1) (string-to-list word2)))))
(global-set-key (kbd "s-d")
                (lambda ()
                  (interactive)
                  (let ((cw (current-word))
                        (ds (format-time-string "%Y-%m-%d")))
                          (insert (make-suffix cw ds)))))

So what do you think?  Is it a good solution?  Is it the right way to use elisp and Emacs?

I'm concerned that make-suffix is sufficiently general that we'll have name collisions with other packages - or even new Emacs versions.  Perhaps prefix with some tag (I tend to begin names with my initials to disambiguate my own functions, for example; that would give you something like an/make-suffix).

Instead of binding a lambda to the keystroke, prefer to give it a name.  That makes it easier to bind to other keys (perhaps interactively) and for Control+h k to give the best output.

Talking of which, let's have some docstrings, please!

Finally, since the function will work only on writable buffers, we should have (interactive "*") to avoid wasting effort when used in a read-only context.


Filter out comments in Delphi source code Today I am working on refactoring and documenting some ancient Delphi code.  In that language, which is derived from Pascal, there are three forms of comments:
(* This is the original Pascal style comment, which is never used in my files *)
{ this is TurboPascal-style comment which I want to remove }
// This is a Delphi-style comment which  I want to remove

As noted in this small sample, I want to strip out the last two styles of comments.  The first kind of comment is never used in the code files I'm using, so there is no special processing done if they are encountered.  I decided to do this using a std::streambuf.  The code and some sample input is below.  I'm interested in any comments for improvement.
commentfilterbuf.h
#ifndef COMMENTFILTER_H
#define COMMENTFILTER_H
#include <streambuf>

class commentfilterbuf : public std::streambuf {
    std::streambuf* sbuf;
    char buffer = EOF;
public:
    commentfilterbuf(std::streambuf* sbuf);
    int underflow();
};
#endif // COMMENTFILTER_H

commentfilterbuf.cpp
#include "commentfilterbuf.h"

commentfilterbuf::commentfilterbuf(std::streambuf* sbuf)
    : sbuf(sbuf)
{
}

int commentfilterbuf::underflow() {
    traits_type::int_type i;
    // skip comments which are delimited with { and }
    for (i = sbuf->sbumpc(); i == '{'; i = sbuf->sbumpc()) {
        while ((i = sbuf->sbumpc()) != '}');
    }
    if (traits_type::not_eof(i)) {
        // check for single line comments like this one
        if (i == sbuf->sgetc() && i == '/') {
            while ((i = sbuf->sbumpc()) != '\n');
        }
        buffer = traits_type::to_char_type(i);
        setg(&buffer, &buffer, &buffer+1);
    }
    return i;
}

testfilter.cpp
#include "commentfilterbuf.h"
#include <iostream>
#include <fstream>
#include <string>

int main(int argc, char* argv[])
{
    if (argc != 2) {
        std::cerr << "Usage: testfilter inputfile\n";
        return 0;
    }
    std::ifstream fin{argv[1]};
    if (!fin) {
        std::cerr << "Error: could not open " << argv[1] << "\n";
        return 1;
    }
    commentfilterbuf sbuf(fin.rdbuf());
    std::istream in(&sbuf);
    std::string line;
    for (std::string line; std::getline(in, line); ) {
        std::cout << line << '\n';
    }
}

sample.in
program HelloWorld;

{$APPTYPE CONSOLE}
{ This is a comment. }
{
    this is a multiline comment.
}
{  This is a Turbo Pascal comment }
// This is a Delphi comment. All is ignored till the end of the line.
// let's try to confuse the parser
{ comment 1 // Comment 2 }begin
// comment 1 { comment 2 }foo

  // Here's a comment
  WriteLn('Hello World');  { and another }
end.

I only have a few minor comments.
The first is about your empty while loops, such as:
    for (i = sbuf->sbumpc(); i == '{'; i = sbuf->sbumpc()) {
        while ((i = sbuf->sbumpc()) != '}');
    }

I generally prefer to at least put the semicolon on a line by itself, to make it fairly obvious that it's intentional that the semicolon wasn't just accidentally entered at the end of a line where it shouldn't have been:
for (i = sbuf->sbumpc(); i == '{'; i = sbuf->sbumpc()) {
    while ((i = sbuf->sbumpc()) != '}')
        ;
}

In this case, there's no other statement there before the closing brace that the loop could be controlling so it's probably less serious than usual, but I still prefer consistency (even though: "a foolish consistency is the hobgoblin of small minds").
Second, if you're going to do all this, I'd probably add a little commentfilterstream class on this order:
#include <fstream>
#include <istream>

class commentfilterstream : public std::istream {
    std::filebuf buffer;
    commentfilterbuf buf;

public:
    // or use an `std::filesystem::path` instead of a string.
    commentfilterstream(std::string const& filename)
        : buf(&buffer)
        , std::istream(&buf)
    {
        buffer.open(filename.c_str(), std::ios_base::in);
    }

    ~commentfilterstream() { buffer.close(); }
};

This lets you simplify the client code a bit to become something like this:
int main(int argc, char* argv[])
{
    if (argc != 2) {
        std::cerr << "Usage: testfilter inputfile\n";
        return 0;
    }

    // Opening stream is now one step, just like usual:
    commentfilterstream in{argv[1]};

    if (!fin) {
        std::cerr << "Error: could not open " << argv[1] << "\n";
        return 1;
    }

    std::string line;
    for (std::string line; std::getline(in, line); ) {
        std::cout << line << '\n';
    }
}

Finally, I'd probably put the buffer and filter into a namespace (e.g., Delphi) to give a little better indication of their intended purpose (Delphi::commentfilterstream).


Finding the Mth to last Element Challenge:

  Write a program which determines the Mth to last element in a list.

Specifications:

  The first argument is a path to a file.
  The file contains series of space delimited characters
  followed by an integer.

  The integer represents an index in the list (1-based), one per line.
  Print out the Mth element from the end of the list, one per line.

  If the index is larger than the number of elements in the list,
  ignore that input.

Solution:

import java.io.File;
import java.io.FileNotFoundException;
import java.util.Scanner;

public class MToLast {
    public static void main(String[] args) throws FileNotFoundException {
        Scanner input = new Scanner(new File(args[0]));

        while (input.hasNextLine()) {
            retrieveAndPrintMToLast(input.nextLine());
        }
    }

    private static void retrieveAndPrintMToLast(String line) {
        int targetIndex = Integer.parseInt(line.replaceAll("\\D", ""));
        String sanitized = line.replaceAll("[^a-zA-Z]", "");
        int limit = sanitized.length();

        if (targetIndex <= limit) {
            System.out.println(sanitized.charAt(limit - targetIndex));
        }
    }
}

This passes my tests, I tried to account for some edge cases, is there anything I missed or something I can improve?

For anyone interested, this challenge, and many others like it are available here.

Your solution looks nice and compact, but it's not necessarily how I would go about it. I come from a background where you have to anticipate faulty input, and so I raise my eyebrows at the first two lines of your method where you blindly start replacing the input.

What happens if you read a line of characters like A B 3 D 5 1? Do you still get A as a result? Numeric characters are still characters. Also, \n is char. The class is much more than what is captured by your [^a-zA-Z] pattern.

The problem specification is straightforward, but you're trying to attack it with regexes instead of something more appropriate. As soon as you transform your input, you are working with data that is manipulated based on your assumptions. This will lead to strange bugs later on that are hard to track down.

I would also suggest changed the method from returning void to char, then let whatever calling method take care of the output.

Finally, make sure you're testing all the bases. What happens when there's an empty string? A string with just one character, or just one integer. What if the last integer is less than 0?


Consecutive animations I'm new to Core Animations, I want to fade out two different layers one after another.I write them in a CATransaction block. I wonder if there is better approach to make such animations.

[CATransaction begin];

    CABasicAnimation *fadeOutLoadingBar = [CABasicAnimation animationWithKeyPath:@"opacity"];
    fadeOutLoadingBar.fromValue = [NSNumber numberWithFloat:0.3];
    fadeOutLoadingBar.toValue = [NSNumber numberWithFloat:0.];
    fadeOutLoadingBar.fillMode = kCAFillModeForwards;
    fadeOutLoadingBar.beginTime = [_loadingLayer convertTime:CACurrentMediaTime() fromLayer:nil];
    fadeOutLoadingBar.duration = .8;
    fadeOutLoadingBar.removedOnCompletion = NO;
    [_loadingLayer addAnimation:fadeOutLoadingBar forKey:nil];

    [self setupCheckMarkLayer]; //initialize _checkMarkLayer
    CABasicAnimation *fadeOutCheckMark = [CABasicAnimation animationWithKeyPath:@"opacity"];
    fadeOutCheckMark.fromValue = [NSNumber numberWithFloat:0.3];
    fadeOutCheckMark.toValue = [NSNumber numberWithFloat:0.];
    fadeOutCheckMark.fillMode = kCAFillModeForwards;
    fadeOutCheckMark.beginTime = [_checkMarkLayer convertTime:CACurrentMediaTime() fromLayer:nil] + 0.5;
    fadeOutCheckMark.duration = 1.0;
    fadeOutCheckMark.removedOnCompletion = NO;
    _checkMarkLayer.opacity = 0.;
    [_checkMarkLayer addAnimation:fadeOutCheckMark forKey:nil];

    [CATransaction setCompletionBlock:^{
        self.userInteractionEnabled = YES;
        [self.delegate finishedLoading];
        [self setupLoadingLayer];
    }];

    [CATransaction commit];

We have some magic numbers.  Also, why don't we use modern syntax for our NSNumber objects?

NSNumber *opacityFromValue = @0.3f;
NSNumber *opacityToValue = @0.0f;

CGFloat fadeLoadBarDuration = 0.8;
CGFloat fadeCheckmarkDuration = 1.0;

CGFloat timeCheckmarkOffset = 0.5;

And now use these variables in place of the magic numbers.

As an added bonus, we'll get slightly better performance by not having to instantiate the opacity values twice each.  NSNumber is an immutable object, so it doesn't matter that we pass the same one both times.


Servlet filter which detects if the authenticated user is a service account My concern with this code is that I am forcing execution repeatedly through the same pathway for the sake of a readability that I'm not convinced by.

It's a servlet filter which detects if the authenticated user is a service account (non-human) and then does some checks on them within the if clause.

import org.springframework.security.core.Authentication;
import javax.servlet.Filter;
import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;

public class ServiceAccountFilter implements Filter {

    @Override
    public void doFilter(ServletRequest request,
                         ServletResponse response,
                         FilterChain chain)
            throws IOException, ServletException {
        if (request instanceof HttpServletRequest
            && ((HttpServletRequest) request).getUserPrincipal() != null
            && ((HttpServletRequest) request).getUserPrincipal()
                    instanceof Authentication
            && ((Authentication)
                ((HttpServletRequest) request).getUserPrincipal())
                .getPrincipal() != null
            && ((Authentication)
                ((HttpServletRequest) request).getUserPrincipal())
                .getPrincipal() instanceof MyUser
            && ((MyUser) ((Authentication)
                ((HttpServletRequest) request).getUserPrincipal())
                .getPrincipal()).getUsername() != null
            && ((MyUser) ((Authentication)
                ((HttpServletRequest) request).getUserPrincipal())
                .getPrincipal()).getUsername().length() > 6) {
            MyUser myUser = (MyUser) ((Authentication)
                    ((HttpServletRequest) request).getUserPrincipal())
                    .getPrincipal();
            // app-specific tests
        }
        chain.doFilter(request, response);
    }

Would it be better to instantiate all of these objects and create a deeply nested set of simplified if clauses?

My opinion is that it might be marginally more performant but it wouldn't be any more readable - or am I already blind to how unreadable it is?

One thing that is sure is that it will be more readable to move those tests in one method :

public void doFilter(ServletRequest request,
                     ServletResponse response,
                     FilterChain chain)
        throws IOException, ServletException {
    if ( isAuthenticated(request) ) {
        MyUser myUser = getAuthenticatedUser(request);
        // app-specific tests
    }
}

Then it depends but most of the time your code is more readable when extracting intermediate steps in variables or methods.

private boolean isAuthenticated(ServletRequest request) {
    return (request instanceof HttpServletRequest) &&
           hasValidPrincipal((HttpServletRequest) request);
}

private boolean hasValidPrincipal(HttpServletRequest request) {
    MyUser user = getAuthenticatedUser(request);
    return user!=null && user.getUsername().length()>6;
}

private MyUser getAuthenticatedUser(HttpServletRequest request) {
    Object principal = request.getUserPrincipal();
    Authentication authentication = null;
    if ( !(principal instanceof Authentication) ) {
        return null;
    }
    authentication = (Authentication) request.getUserPrincipal();
    Object user = authentication.getPrincipal();

    return (user instanceof MyUser)
        ?(MyUser) user
        :null;
    }
}


Finding product palindromes I am new to perl and want some help in tuning this script if possible cause it takes time if I increased my array. also if I use warnings I get a lot of warnings which I'm unable to handle. I'm using perl (v5.24.0) built for darwin-thread-multi-2level.

Here is an example of what I want to achieve:
The number 886 is like 8*86 = 688, if we reverse it 886 which is the original number.
Also the same for 920781, 9*20781 = 187029, reverse 920781,
etc...

my $start_run = time();
use strict;
foreach (1..1000000000) {
   if ($_==reverse(substr($_, 0, 1) * substr($_, 1)))
      { print $_."\n" ;}
    if ($_==reverse(substr($_, 0, 2) * substr($_, 2)))
    { print $_."\n" ;}
    if ($_==reverse(substr($_, 0, 3) * substr($_, 3)))
    { print $_."\n" ;}
    if ($_==reverse(substr($_, 0, 4) * substr($_, 4)))
    { print $_."\n" ;}
    if ($_==reverse(substr($_, 0, 5) * substr($_, 5)))
    { print $_."\n" ;}
    if ($_==reverse(substr($_, 0, 6) * substr($_, 6)))
    { print $_."\n" ;}
    if ($_==reverse(substr($_, 0, 7) * substr($_, 7)))
    { print $_."\n" ;}
}
my $end_run = time();
my $run_time = $end_run - $start_run;
print "Time taken: ".$run_time." sec\n";

I want some help in tuning this script if possible cause it takes time
  if I increased my array

Yes, the script can be tuned. I will discuss optimizing you script at the end of this post.

The warnings pragma

  also if I use warnings I get a lot of warnings which I'm unable to
  handle

Yes correct, you will get many warnings in case you add the statement use warnings. Using the warnings pragma is highly recommended, see for example:

Why use strict and warnings?
Why not use strict and warnings in Perl?

In short, the warnings pragma help you find bugs in your code. If you add the use warnings to the top of your program, the first warning you get is

Argument "" isn't numeric in multiplication (*)

This happens because in the first iteration of the for loop, $_ will be equal to 1, and at line 4 of your program where you mulitply the substrings:

substr($_, 0, 1) * substr($_, 1))

the first substring will be "1" and the second will be the empty string "". When you try multiply with the empty string (and warnings are disabled) it will be converted silently to a zero, see perldata. To demonstrate:

$ perl -E 'say 1 + ""'
1

but if you turn on warnings (use the -w flag):

$ perl -wE 'say 1 + ""'
Argument "" isn't numeric in addition (+) at -e line 1.
1

you will get the warning Argument "" isn't numeric.

The next warning you get will be

substr outside of string

this happens because substr returns an undefined value
if the substring is beyond either end of the string. In this case, substr will also produce a warning (if you have enabled warnings with use warnings). The reason why you get the warning is that $_ is equal to "1" and at line 6 of your script you try call substr $_, 2. To demonstrate:

$ perl -wE '$_="1"; my $s  = substr $_, 2'
substr outside of string at -e line 1.

The next warning you will get is:

Use of uninitialized value in multiplication (*)

This happens also at line 6, where you have

substr($_, 0, 2) * substr($_, 2)

Here the first substring will be "1" ( since $_ is equal to "1" in the first iteration of the for loop), whereas the second substring will be undefined, as discussed above. Now if you try multiply 1 * undef you will get 0, and a warning (if you have enabled warnings). To demonstrate:

$ perl -wE 'say 1 * undef'
Use of uninitialized value in multiplication (*) at -e line 1.
0

The problem with the code

From the above discussion, it should be clear that the problem with the code that causes the warnings are that it does not use substr correctly when the numbers ( i.e $_ in the for loop ) are less than 8 digits long. If the numbers are more than 8 digits long another problem arises. In this case, the problem is of logical nature in that the program will fail to check all possible palindromes. For example, for a 9 digit number there is no check

if ($_ == reverse(substr($_, 0, 8) * substr($_, 8))) { ... }

Fortunately, these problems can be solved easily using nested for loops:

for (1 .. $max_integer) {
    for my $length ( 1 .. ((length $_) - 1) ) {
        say if $_ == reverse (substr ($_, 0, $length) * substr ($_, $length) );
    }
}

Here, I have defined a variable $max_integer which should be set to the highest integer to check for. For your case, it should be set to 1 billion:

my $max_integer = 1_000_000_000;

The inner for loop takes care of all the if statements in your original code.

Programming style

In Perl there is more than one way to do it (TMTOWTDI). The language was designed with this idea in mind, in that it "doesn't try to tell the programmer how to program". Still, in my experience (I started programming Perl 3 years ago) there are certain programming style guidelines that have been adopted by the community. I have seen these programming styles in practice reading source code of CPAN modules and looking at answers at the Perl tag at stackoverflow.com. Some of these guidelines are described in perlstyle, in Chapter 21 of Programming Perl and in the book Perl Best Practices.

Put all use statements at the top of your program. Since all use statements occur at compile time, it often does not matter for the Perl interpreter where in the source file you place them1. To demonstrate:

 $ perl -E 'say getcwd(); use Cwd'

The above code works fine, even if use Cwd comes after its exported function getcwd is called in the code. However, the above code will
likely confuse humans. You can read more about compile time versus run time in Chapter 16 of the book Programming Perl.

Perl pragmas are a subclass of Perl modules that can have a lexical scope. Since these pragmas only stay in effect within the scope in which they are used, their position in the code is important also to the Perl interpreter. However, these pragmas are typically used at the package scope ( to enable them in the whole source file ), so pragmas are also commonly placed at the top of the source file, to enhance readability and maintainability for a humans.

So for your use case, you should move the use strict to the top of the file, and add a use warnings below it (it also help for a human reader if you keep them in sorted order):

use strict;
use warnings;

Variable naming conventions. I think snake_case is the most common style for a lexical variable, though I have seen many CPAN authors that have used camelCase naming convention. I also prefer snake_case since I believe it easier to read.

Funny enough, I changed my habits several times during my career. I remember I used to make most of my variables camelCase in the beginning, and I could not understand why someone would prefer a variable name like $max_iterations instead of $maxIterations. Two things were disturbing me: first, I thought shorter names were better, and second, I didn't like the idea of using three keystrokes when I could use two keystrokes. However, the shift came after reading stuff and meeting other developers and seeing other people's code, I gradually realialized that code is read much more often than it is written and the benefits of improved readability far outweigh the disadvantage of typing an extra character. Also, I think it is wise to stick to the snake_case convention used by the majority of the Perl community, because consistency is a good thing.

For your case, you already follow the snake_case naming convention for lexical variable (for example in the first line you have my $start_run = time()) so in my opinion: just keep it up.
Spaces around operators. I prefer

if ($_ == gen_number()) { ... }

or even

if ( $_ == gen_number() ) { ... }

to

if ($_==gen_number()) { ... }

The reason is that I find the former easier to read. Adding spaces around most operators is also advocated in perlstyle.

For your case, you could consider adding more spaces around operators in your code.
Indent style. Placement of braces. In Perl "uncuddled elses" and "K&R" style for indenting control constructs is used and recommended almost exclusively (as far as I know). See wikipedia article on indent style and perlstyle. Example of K&R style with uncuddled elses:

if ( $a == 1 ) {
    ....
}
else {
    ....
}

For you case, you have indentation that does not conform with this style. For example:

if ($_==reverse(substr($_, 0, 1) * substr($_, 1)))
      { print $_."\n" ;}

this should be better written to conform with the style guidelines as:

if ( $_ == reverse (substr($_, 0, 1) * substr($_, 1)) ) {
    print $_ . "\n";
}

Optimization

I cannot see how the Perl script itself can be made run substantially faster, however I think the algorithm can be programmed to run much faster in C than in Perl. If the algorithm is only part of a larger program in Perl, it is not necessary to convert the whole program to C. By using the CPAN module Inline::C you can incorporate small pieces of C code into a larger Perl program.

I tested this on Ubuntu 16.04, Perl version 5.22, and found that the version using Inline::C can be approximately 25 times faster than the pure Perl version. Example:

use feature qw(say);
use strict;
use warnings;
use Benchmark qw( cmpthese );
use Inline C => './ppalin_c.c';

my $max = 10_000;

my %sub_info = (
    c    => sub { ppalin_c( $max ) },
    perl => sub { ppalin( $max ) },
);

cmpthese( -3, \%sub_info );

sub ppalin {
    my ( $max ) = @_;

    for (1 .. $max) {
        for my $length (1 .. length($_) - 1) {
            say if $_ == reverse (substr ($_, 0, $length) * substr ($_, $length) );
        }
    }
}

where ppalin_c.c is:

void ppalin_c( int max ) {
    char buf[256];

    for ( int i = 1; i <= max; i++ )
    {
        int a = i;
        char *ptr = buf;
        do {
            int b = a % 10;
            a /= 10;
            *ptr++ = b + '0';
        } while ( a );
        int len = ptr - buf;
        for ( int j = 1; j < len; j++ )
        {
            int fact1 = getfactor( buf, j );
            int fact2 = getfactor( buf+j, len - j);
            int reverse = fact1 * fact2;
            int match = 1;
            char *ptr2 = buf + len;
            for (int k = 0; k<len; k++)
            {
                int b = reverse % 10;
                reverse /= 10;
                if ( *--ptr2 != (b + '0') ) {
                    match = 0;
                    break;
                }
            }
            if ( match ) printf( "%d\n", i );
        }
    }
}

int getfactor( char *ptr, int len )
{
    int pow = 1;
    int b = 0;
    while (len--) {
        int c = (*ptr++ - '0');
        b += c * pow;
        pow *= 10;
    }
    return b;
}

The output:

       Rate  perl     c
perl 97.5/s    --  -96%
c    2544/s 2509%    --

which shows that the C version is 2509% faster than the Perl version for this particular run.

Footnotes:

1. The position of use statements in the source file could be important for the interpreter in some special cases. For example, if compile time code in one module depended on compile time code in another module.


Structuring functions receiving and returning promises? I keep running into the same pattern with code using promises in javascript.

When writing a function which takes a promise and returns a promise, obviously I want to reject the promise I'm returning when the promise I received is rejected (and usually I want to pass the error down the chain verbatim).  This results in a fair amount of boilerplate:

var fs = require ('fs');
var when = require ('when');
var nodefn = require ("when/node/function");

var get_some_data = function (filename) {
    return nodefn.call (fs.readFile, filename);
};

var parse_some_data = function (filename) {
    var deferred = when.defer ();

    var input = get_some_data (filename);
    input.then (function (data) {
        // imagine some complicated but synchronous process here which
        // may also call deferred.reject() if something goes wrong.
        deferred.resolve (JSON.parse (data));
    }, function (error) {
        deferred.reject (error);
    });

    return deferred.promise;
};

var do_something = function (filename) {
    var deferred = when.defer ();

    var input = parse_some_data (filename);
    input.then (function (data) {
        // imagine some complicated but synchronous process here which
        // may also call deferred.reject() if something goes wrong.
        data['foo'] = 'bar';
        deferred.resolve (data);
    }, function (error) {
        deferred.reject (error);
    });

    return deferred.promise;
};

Is there a better way to structure this code?  Do promise libraries have features which I'm not using, which would reduce the boilerplate?  (The above code uses when from cujo.js, but if other promise libraries have better solutions I'm interested in hearing about them).

You're making more work for yourself than necessary.

Check this out:

var fs = require('fs');
var nodefn = require("when/node/function");

var get_some_data = function(filename) {
  return nodefn.call(fs.readFile, filename);
};

var parse_some_data = function(filename, throwError) {
  return get_some_data(filename).then(function(data) {
      // Mimic a synchronous error
      if (throwError) {
        throw new Error('Error parsing data')
      }

      return JSON.parse(data);
    });
};

var stringify_some_data = function(filename, throwParseError, throwLogError) {
  return parse_some_data(filename, throwParseError).then(function(parsedData) {
    // Mimic a synchronous error
    if (throwLogError) {
      throw new Error('Error stringifying data.');
    }

    return JSON.stringify(parsedData);
  });
};

var onSuccess = function(dataString) {
  console.log('Success! ' + dataString);
};
var onError = function(err) {
  console.log('Error! ' + err.message);
};

// "Success! { "data": "obj" }"
stringify_some_data('./data.json').then(onSuccess, onError);

// "Error! ENOENT, open './not_a_file.json'"
stringify_some_data('./not_a_file.json').then(onSuccess, onError);

// "Error! Error parsing data"
stringify_some_data('./data.json', true).then(onSuccess, onError);

// "Error! Error stringifying data."
stringify_some_data('./data.json', false, true).then(onSuccess, onError);

A couple of things to note:

An error thrown anywhere in the promise chain (synchronous or not) will bubble down to your onError handler
Return values of Promise objects are passed as arguments to the next onSuccess handler

I'm still working on grokking how the Promise/A+ spec works. If you've used jQuery.Deferred, you should know that it does not follow the spec (which made using When.js a big learning curve for me). Here's a pretty decent article explaining the Promise/A+ spec. Brian Cavalier (a When.js author) has some blog posts on asynchronous programming which are worth reading, as well.


Different functions in Java I wonder what is better code practice or just what looks better in Java:

Version 1:

protected Boolean doSomething(int amount) {
    if (amount < 1) return false;

    return insertToDb(fillImages(fetchInfo(REQUEST_URL, amount)));
}

Version 2:

protected Boolean doSomething(int amount) {
    if (amount < 1) return false;

    Data[] data = fetchInfo(REQUEST_URL, amount);

    fillImages(data);
    // or even
    // data = fillImages(data);

    return insertToDb(data);
}

This could be a problem if we had more functions chained like that. I would like to make my code nicer, for anyone else who might have to edit it, and just to know that I am not writing anything that looks stupid.

I definitely prefer the second version. The steps taken just "leap out" of the code and the method is easier to read. It will also be a lot easier to debug (or to pinpoint the problem in a stack trace).

Always go for readability, it makes for easier understanding when going back to your old code and it's a lot easier for someone else to understand what is going on.

I also prefer the "data = fillImages(data)" if possible. It makes clear that the data is modified.