Validity checks for a user signup process Background:

I have been working on a service which allows users to signup on different apps and on each login checks if the request is valid or not based on a series of checks.

The below snippet is small part of the whole application but covers my question and is working fine for now:

Code:

'use strict';

const bcrypt = require('bcrypt');
const boom = require('boom');
const joi = require('joi');
const flatten = require('lodash/flatten');
const pick = require('lodash/pick');

const models = require('../../models');
const { AccessToken, App, User } = models;

const debug = require('debug')('microauth:test');

const loginSchema = joi
  .object({
    appname: joi.string().required(),
    email: joi.string().required(),
    password: joi.string().required(),
  })
  .required();

async function run(req, res, next) {
  const { appname, email, password } = joi.attempt(req.body, loginSchema);

  const app = await getApp(appname);
  if (!app) {
    throw boom.badRequest(`Invalid app name: ${appname}.`);
  }
  if (app.isInactive()) {
    throw boom.badRequest('App is not active.');
  }

  const { isAuthorized, user } = await authorize({ email, password });
  if (!user) {
    throw boom.notFound('User not found.');
  }
  debug(`User ${user.get('email')} is authorised? ${isAuthorized}`);
  if (!isAuthorized) {
    throw boom.unauthorized('Invalid email or password.');
  }

  const { result } = await isUserBelongsToApp(user, app.get('name'));
  if (!result) {
    throw boom.badRequest(`User is not authorised to access app.`);
  }

  return successResponse(email, app.get('secret'), res);
}

async function getApp(name) {
  return await App.findOne({ name });
}

async function authorize({ email, password }) {
  const user = await User.findOne(
    { email, status: 'active' },
    { withRelated: ['apps', 'roles.permissions'] }
  );

  let isAuthorized = false;
  if (user) {
    isAuthorized = await bcrypt.compare(password, user.get('password'));
  }
  return { isAuthorized, user };
}

async function isUserBelongsToApp(user, appname) {
  let result = false;
  let app = null;
  app = user.related('apps').findWhere({ name: appname });
  if (app) {
    result = true;
  }
  return { result, app };
}

async function successResponse(email, secret, res) {
  const userFields = [
    'device',
    'email',
    'firstname',
    'language',
    'lastname',
    'phone',
    'uid',
  ];
  const roleFields = ['name', 'description'];
  const permissionFields = ['name', 'object', 'action'];

  let user = await User.findOne(
    {
      email: email,
    },
    {
      withRelated: ['roles.permissions'],
    }
  );
  user = user.toJSON();
  const result = Object.assign({}, { ...user });
  result.roles = [];
  result.permissions = [];

  if (user.roles) {
    result.roles = user.roles.map(role => pick(role, roleFields));
    result.permissions = user.roles.map(role => {
      return role.permissions.map(permission =>
        pick(permission, permissionFields)
      );
    });
  }
  result.permissions = flatten(result.permissions);
  const { token, expiration } = new AccessToken(secret).create(result);
  res.json({ token, expiration });
}

module.exports = run;

Questions:

The code above belongs to the controller of the applications, is that the right place to do all these checks?

Right now the main logic seems pretty obvious but each step depends of the previous step. Is there any better way to write the same logic?

This is only a partial review.

I would declare your constants outside of your functions, as they are constant.
Also, the constants you did declare outside of your functions should be chained.
Finally, you should never call a function more than once. If you are truly using functional-programming then you should do the following:

const required = joi.string().required(),
      loginSchema = joi
  .object({
    appname: required,
    email: required,
    password: required,
  })
  .required();

because a given function must return the same output for the same input.

Rewrite

'use strict';

const bcrypt = require('bcrypt'),
      boom = require('boom'),
      joi = require('joi'),
      flatten = require('lodash/flatten'),
      pick = require('lodash/pick');

const models = require('../../models'),
      { AccessToken, App, User } = models;

const debug = require('debug')('microauth:test');

const userFields = [
    'device',
    'email',
    'firstname',
    'language',
    'lastname',
    'phone',
    'uid',
  ],
      roleFields = ['name', 'description'],
      permissionFields = ['name', 'object', 'action'];

const required = joi.string().required(),
      loginSchema = joi
  .object({
    appname: required,
    email: required,
    password: required,
  })
  .required();

async function run(req, res, next) {
  const { appname, email, password } = await joi.attempt(req.body, loginSchema);

  const app = await getApp(appname);
  (!app) && (throw boom.badRequest(`Invalid app name: ${appname}.`);)

  (app.isInactive()) && (throw boom.badRequest('App is not active.');)

  const { isAuthorized, user } = await authorize({ email, password });
  (!user) && (throw boom.notFound('User not found.');)

  debug(`User ${user.get('email')} is authorised? ${isAuthorized}`);
  (!isAuthorized) && (throw boom.unauthorized('Invalid email or password.');)

  const { result } = await isUserBelongsToApp(user, app.get('name'));
  (!result) && (throw boom.badRequest(`User is not authorised to access app.`);)

  return successResponse(email, app.get('secret'), res);
}

async function getApp(name) {
  return await App.findOne({ name });
}

async function authorize({ email, password }) {
  const user = await User.findOne(
    { email, status: 'active' },
    { withRelated: ['apps', 'roles.permissions'] }
  );

  let isAuthorized = false;
  if (user) {
    isAuthorized = await bcrypt.compare(password, user.get('password'));
  }
  return { isAuthorized, user };
}

async function isUserBelongsToApp(user, appname) {
  let result = false;
  let app = null;
  app = user.related('apps').findWhere({ name: appname });
  if (app) {
    result = true;
  }
  return { result, app };
}

async function successResponse(email, secret, res) {

  let user = await User.findOne(
    {
      email: email,
    },
    {
      withRelated: ['roles.permissions'],
    }
  );
  user = user.toJSON();
  const result = Object.assign({}, { ...user });
  result.roles = [];
  result.permissions = [];

  if (user.roles) {
    result.roles = user.roles.map(role => pick(role, roleFields));
    result.permissions = user.roles.map(role => {
      return role.permissions.map(permission =>
        pick(permission, permissionFields)
      );
    });
  }
  result.permissions = flatten(result.permissions);
  const { token, expiration } = new AccessToken(secret).create(result);
  res.json({ token, expiration });
}

module.exports = run;


SQL with several joins Is this query acceptable in terms of performance (I get the correct data) or can it be optimized?

SELECT
    bankInstitution.name,
    person.firstName,
    person.surname,
    offer.offerId,
    bank.personId,
    bank.bankOtherName,
    bank.sortCode,
    bank.number,
    loan.loanId
    offer.campaignId,
FROM bank
    JOIN loan ON bank.personId = loan.personId AND bank.isCurrent = 1
    JOIN offer ON loan.loanId = offer.loanId
    JOIN person ON person.personId = bank.personId
    JOIN bankInstitution ON bankInstitution.bankInstitutionId = bank.bankInstitutionId
WHERE offer.CampaignId = 1 AND offer.Processed Is NULL

There is an illegal comma before FROM.

I would put bank.isCurrent = 1 in the WHERE-clause instead of as a join condition.

For consistency, I would reverse the equalities in the join conditions:

JOIN loan ON loan.personId = bank.personId
JOIN offer on offer.loanId = loan.loanId

… to match …

JOIN person ON person.personId = bank.personId
JOIN bankInstitution ON bankInstitution.bankInstitutionId = bank.bankInstitutionId

None of the above affects performance.  What you have is a very ordinary joining of tables, and your query expresses it in the usual way.  There's not much more that can be done for performance by tweaking the query.

What you should do, though, is ensure that all of the joins are being performed using indexes, not full-table scans.  You can verify that by prepending ANALYZE to your SELECT, which will give you the query execution plan.  If any of the joins is being performed with a full-table scan, create an index on the relevant column.  If your schema is properly defined, with the PRIMARY KEYs and FOREIGN KEYs declared, then you should already be fine, I think.


Sorting alphabetically ascending and descending using .sort() I have created this basic use case of .sort() for my own reference. I am asking for a review to see if there are bad practices present or areas that could be optimised.

Is my use of modifying the array correct? Do I need to place the newly sorted list into a new array?
Using .map() I found it was parsing it as a string. Is insertAdjacentHTML() the right method to use here?

const listArea = document.getElementById('listArea');
const btnAsc = document.getElementById('asc');
const btnDes = document.getElementById('des');

// The Original Array
let architects = [
  "Lloyd Wright",
  "Hadid",
  "Mies van der Rohe",
  "Le Corbusier",
  "Foster",
  "Gaudi",
  "Piano",
  "Gropius",
  "Niemeyer",
  "Aalto",
  "Saarinen",
]

// Empty Array to Store Sorted Array
let sortedList = [];

// Click Events for the sort buttons
btnAsc.addEventListener('click', () => sortAsc(architects));
btnDes.addEventListener('click', () => sortDes(architects));

// Sort the array in ascending order
function sortAsc(arr) {
  // Clear the sortedList array
  sortedList.length = 0;

  // Convert each item in the architects array to lower case
  for(const element of arr) {
    // Push these lowercase elements into the sortedList array
    sortedList.push(element.toLowerCase());
  }
  sortedList.sort();
  populateList(sortedList);
}

function sortDes(arr) {
  sortedList.length = 0;
  for(const element of arr) {
    sortedList.push(element.toLowerCase());
  }
  sortedList.sort().reverse();
  populateList(sortedList);
}

function populateList(arr) {
  // Empty out the listArea of all content
  while(listArea.firstChild) {
    listArea.removeChild(listArea.lastChild);
  }
  // Map each item in the supplied array to an li, and remove comma at end of each
  let listArchitects = arr.map(item => `<li>${item}</li>`).join('');
  listArea.insertAdjacentHTML('beforeend', listArchitects);
}

window.addEventListener('onload', populateList(architects));
#listArea {
  text-transform: capitalize;
}

    <button id="asc">Sort Ascending</button>
    <button id="des">Sort Descending</button>

Some of the things I'd point out in your code for improvements:

Arrays can be a const since you will not modify the array itself but just push new elements to the array.
In your functions sortAsc, sortDesc, you are iterating through arr just to make it lowercase and iterating through the array again in sortedList.sort(). In such scenarios, think about how you can reduce number of iterations.
sort() does in-place sorting, so no need to create a new array.
In function populateList by running arr.map you are creating another new array which I don't think is necessary.

const listArea = document.getElementById('listArea');
const btnAsc = document.getElementById('asc');
const btnDes = document.getElementById('des');

const architects = [
  "Lloyd Wright",
  "Hadid",
  "Mies van der Rohe",
  "Le Corbusier",
  "Foster",
  "Gaudi",
  "Piano",
  "Gropius",
  "Niemeyer",
  "Aalto",
  "Saarinen",
];

function sortAsc() {
  architects.sort();
  populateList();
}

function sortDes() {
  architects.sort().reverse();
  populateList();
}

function populateList() {
  listArea.innerHTML = '';
  architects.forEach( (a) => {
    const list = document.createElement('li');
    list.innerText = a;
    listArea.appendChild(list);
  });
}

btnAsc.addEventListener('click', sortAsc);
btnDes.addEventListener('click', sortDes);

window.addEventListener('onload', populateList());

    <button id="asc">Sort Ascending</button>
    <button id="des">Sort Descending</button>


Graph implementation with Dijkstra So, here is the code:
#pragma once
#include <vector>
#include <iostream>
#include <queue>
#include <functional>

using node = int;
using cost = int;
using arc = std::pair<node, cost>;

using AdjList = std::vector<std::vector<arc>>; // first is node, second is cost

class Graph {
private:
    AdjList adjList;
    std::vector<node> nodes;

public:
    Graph(int n) : nodes{ n },  adjList{ n } {}
    void addEdge(const node v, const node u, const cost c) {
        adjList[v].push_back({ u, c });
        adjList[u].push_back({ v, c });
    }

    std::vector<int> dijkstra(const node n) const {
        std::vector<cost> dist(adjList.size(), INT_MAX);
        std::vector<bool> visited(adjList.size(), false);
        dist[n] = 0;
        std::priority_queue<node, std::vector<node>, std::greater<int>> to_visit;
        to_visit.push(n);
        while (!to_visit.empty()) {
            node v = to_visit.top();
            to_visit.pop();
            visited[v] = true;
            for (const arc& a : adjList[v]) {
                node u = a.first;
                cost c = a.second;
                if (!visited[u] and (dist[u] == INT_MAX or dist[u] > dist[v] + c)) {
                    dist[u] = dist[v] + c;
                    to_visit.push(v);
                }
            }
        }
        return dist;
    }

    friend std::ostream& operator<<(std::ostream& stream, const Graph& g) {
        for (size_t i = 0; i < g.adjList.size(); i++) {
            stream << "node " << i << "\n";
            for (const arc& p : g.adjList[i]) {
                stream << "connected with " << p.first << " cost: " << p.second << " ";
            }
            stream << "\n";
        }
        return stream;
    }

};

I think I did a decent job, but I want to make sure I'm not missing anything. I'm using just a adjacent list for storing the arcs. I was unsure if I should reserve some memory for the vectors there, because maybe it's going to get a little bit slow with the push_back operation, but I also wanted to be able to get rid of the O(n^2) for storing.

Graph seems quite reasonable:
But your distance implementation is not an implementation of Dijkstra.
Dijkstra finds only the cheapest path between two nodes (technically).
But I suppose you can use the technique to find the distance from a node to all others nodes. Dijkstra works by finding the cheapest distance to a node then expanding from there (it never re-expands a node, because it does not need to). To do this you need a sorted frontier list so you always expand from the node with the lowest cost to get there and keep track of nodes that were visited.

Some notes:
// Node Id can only by positive.
using node = int;

// Dijkstra assumes all costs are positive.
using cost = int;

Lets have a look at your search:
    // Returning the min distances to all nodes.
    // Sure: but you can not tell what the route to get there is
    //       so this does not help you traverse the graph it only
    //       tells you the shortest distances.
    //       Also how do you tell the difference between a
    //       a distsance of INT_MAX and a node that was never reached.
    std::vector<int> dijkstra(const node n) const {

        // I think this is what is causing you to go wrong.
        // The distance is usually part of the to_visit.
        // You sort the list by the distance.
        std::vector<cost> dist(adjList.size(), INT_MAX);

        // Sure. You need this for dijkstra
        //       In a classic implementation I would have used
        //       std::set<Node> as we don't need (in general) to traverse
        //       the whole graph to find a route from A to B.
        //       Since you are finding the distance to all nodes. This seems
        //       like a better choice.
        std::vector<bool> visited(adjList.size(), false);

        dist[n] = 0;

        // This is a mistake.
        // The order here based on the node-id.
        // It should be based on the cost to get to a node.
        // You always keep it sorted by the cost to get to the node
        // But you may have multiple routes to a node in the list with
        // different costs so after extraction from the list you check
        // if already visited (because if it is already visited then
        // this is not the shortest route).
        //
        // I am sure I could construct a graph that would break this.
        std::priority_queue<node, std::vector<node>, std::greater<int>> to_visit;
        to_visit.push(n);
        while (!to_visit.empty()) {
            node v = to_visit.top();
            to_visit.pop();
            visited[v] = true;
            for (const arc& a : adjList[v]) {
                node u = a.first;
                cost c = a.second;

                // In dijkstra the cost to a node is not known until
                // you actually visit it for the first time (as you may
                // see a node from many other nodes.
                if (!visited[u] and (dist[u] == INT_MAX or dist[u] > dist[v] + c)) {
                    dist[u] = dist[v] + c;
                    to_visit.push(v);
                }
            }
        }
        return dist;
    }

This is what it normally looks like:
    struct DN {
        node  src;
        node  dst;
        cost  c;

        friend bool operator<(DN const& lhs, DN const& rhs) {return lhs.c < rhs.c;}
    };

    std::vector<node> dijkstra(node begin, node end) const
    {
        std::vector<node>       route(adjList.size(), INT_MAX)
        std::set<node>          visited;
        std::priority_queue<DN> frontier;

        frontier.push({INT_MAX, begin, 0});

        while (!frontier.empty()) {
            node v = frontier.top();
            frontier.pop();

            // If we have already been here go to next value.
            if (visited.find(v.dst) != visited.end()]) {
                continue;
            }

            // OK. Found it and save the cost.
            visited.insert(v.dst);
            route[v.dst] = v.src;

            // If we reached the end exit.
            if (v.dst == end) {
                break;
            }

            for (const arc& a : adjList[v.dst]) {
                // Add nodes to frontier list.
                // That is sorted by the cost to the node.
                frontier.push({n, std::get<0>(a), v.c + std::get<1>(a));
            }
        }
        // You can start from the end node
        // and retrace the path the beginning node.
        return route;
    }


Logging and error middleware I'm looking for code review on the following basic log/error middleware. The logging middleware uses logger.js which has a bunch of methods like log.error() (which makes a request out to error tracking service like Sentry).

The handleErrors middleware is intended to handle any errors that occur in any of the api routes. So, for example, I have a route /api/users and if an error occurs in that request, the catch will execute and pass along the error to the handleErrors middleware.

const getUsers = (req, res, next) => {
  return apiHttpClient.get(req, '/api/users')
    .then(users => res.status(HttpStatus.OK).json(users))
    .catch(err => next(err));
};

The error middleware should create an error context for the client without exposing the specific error. So for example, if a 400 error occurs, the message sent to the browser would look like:

{
  "message": "Bad Request",
  "status": 400
}

But depending on if the NODE_ENV is production, the logging middleware should log out the specific error:

{
  "name":"Example",
  "hostname":"",
  "pid":18322,
  "level":40,
  "payload":{
    "name":"BadRequest",
    "message":"A specific error message",
    "statusCode":400,
    "errorCode":400
  },
  "msg":"400: A specific error message",
  "time":"2017-10-22T15:26:06.824Z",
  "v":0
}

Usage

express.js

const { logErrors, handleErrors } = require('./errorMiddleware');

// other middleware
// api routes

app.use(logErrors())
app.use(handleErrors());

errorMiddleware.js

const log = require('../../logger');
const HttpStatus = require('http-status-codes');

const isServerError = (statusCode) => {
  return statusCode >= HttpStatus.INTERNAL_SERVER_ERROR;
};

const isProdEnv = (env) => {
  return env === 'production';
};

const getStatusCode = (statusCode) => {
  return statusCode || HttpStatus.INTERNAL_SERVER_ERROR;
};

const createErrorContext = (statusCode) => {
  return {
    message: HttpStatus.getStatusText(statusCode),
    status: statusCode
  };
};

const createErrorMessage = (err) => {
  const statusCode = getStatusCode(err.statusCode);
  return `${statusCode}: ${err.message}`;
};

const deleteErrorStack = (env, err) => {
  if (isProdEnv(env)) delete err.stack;
};

const getLogLevel = (statusCode) => {
  return isServerError(statusCode) ? 'error' : 'warn';
};

const logErrors = () => {
  return (err, req, res, next) => {
    const statusCode = getStatusCode(err.statusCode);
    const logLevel = getLogLevel(statusCode);
    const errMessage = createErrorMessage(err);
    const extraData = req.path;

    log[logLevel](errMessage, err, extraData);

    next(err);
  };
};

const handleErrors = () => {
  return (err, req, res, next) => {
    const statusCode = getStatusCode(err.statusCode);
    const errorContext = createErrorContext(statusCode);

    deleteErrorStack(req.app.get('env'), err);
    res.status(statusCode).json(errorContext);
  };
};

module.exports = {
  logErrors: logErrors,
  handleErrors: handleErrors
};

Some thoughts:

Consider making your middleware package into a proper (private?) npm package and consume it as such in the main application. This would also allow better reuse across other applications that may need this behavior.
const log = require('../../logger'); This relative reference seems problematic. I would guess your logger should also be a proper npm package (or in same package as your middleware if this is a custom logger).  At a very minimum, I would think you would want to use path or similar to get to an absolute file reference based on the location of your middleware file to the working directory.
Your methodology for determining 'production' environment seems fragile. Why is it up to your middleware package to have a hard-coded string in it that determines whether production behavior is to be expressed?
The deleteErrorStack() function seems superfluos and actually obfuscates the logic of what is happening in the handleErrors function.  Why not just have this single line of code inside the handleErrors function so intent is much clearer.

For example:

const handleErrors = () => {
  return (err, req, res, next) => {
    const statusCode = getStatusCode(err.statusCode);
    const errorContext = createErrorContext(statusCode);

    if(isProdEnv(req.app.get('env')) {
        delete err.stack;
    }
    res.status(statusCode).json(errorContext);
  };
};

One could possibly argue that the deletion of the stack trace information from err is logic this should exist within your context creation method, as this is really a display concern, not an error handling concern.


Construct an 2D array of contiguous elements comparing master and incident arrays Requirement:

Based on mainArray which contains the master list of items, and compArray which has items of interest; construct a 2 dimensional third array that contains the array of contiguous elements.

The purpose of this requirement is for plotting poly-line on map wherever an incident occurred in contiguous manner, plotted using the finalArray of locations.

Example:

const mainArray = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15];
const compArray = [1, 2, 3, 6, 7, 11, 12, 13, 15];

// produces finalArray
[[ 1, 2, 3 ],[ 6, 7 ],[ 11, 12, 13 ],[ 15 ]]

My current solution:

console.clear();

const mainArray = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15];
const compArray = [1, 2, 3, 6, 7, 11, 12, 13, 15];
let compArrayIndex = 0;
let finalArray = [];
let currentArray = null;

for (let i = 0; i < mainArray.length; i++) {
  const e = mainArray[i];

  if (e === compArray[compArrayIndex]) {
    if (!currentArray) {
      currentArray = [];
    }
    currentArray.push(e);
    compArrayIndex++;
  } else {
    if (currentArray) {
      finalArray.push([...currentArray]);
      currentArray = null;
    }
  }
}

if (currentArray) {
  finalArray.push([...currentArray]);
  currentArray = null;
}

console.log(mainArray);
console.log(compArray);
console.log('finl arr:');
for (let i = 0; i < finalArray.length; i++) {
  const e = finalArray[i];
  console.log(e);
}

The code could be reduced to a single line by utilizing Array.prototype.reduce()

const mainArray = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15];
const compArray = [1, 2, 3, 6, 7, 11, 12, 13, 15];

const finalArray = mainArray.reduce((a, b) => {
  // `c` is boolean result of checking if `compArray` includes `b`
  // `x` is first, and if matching elements found, last array in `finalResult`
  const [c, x] = [compArray.includes(b), a[a.length - 1]];
  // if `c` push `b` to last array of `a`
  if (c) x.push(b)
  // else if `x.length` push a new array to `a`
  else if (x.length) a.push([]);
  // return `a`
  return a;
}, [[]]); // pass initial array containing single array `a` to `reduce`

console.log(finalArray);


Blocktastic optimisation I have written the following code as the level rendering part of a game engine I am writing. I am very happy how it works but as it is the background / level graphics made up of individual blocks it needs to be quick.

What it does it look take the graphics from a resource loaded elsewhere and scales it onto the block if it is in the viewing window.

Blocksize is the size of the blocks and can be increased of decreased and is used to scale the blocks to give the Zoom in and out effect.

How can I improve this code both in terms of speed and shorten it to improve readability?

public void render(Graphics g){

    //Ensure player co-ords updated
    camerax=playerx-(gamewidth/2);
    cameray=playery-(gameheight/2);

    int blockwidth=blocksize-2;
    //Draw coloured blocks
    int screenx=-(int)camerax-blocksize;
    for (int x=0;x<sizex;x++){
          screenx+=blocksize;
          if (screenx>-blocksize && screenx<gamewidth){
              int screeny=-(int)cameray-blocksize;
              for (int y=0;y<sizey;y++){
              screeny+=blocksize;
              if (screeny>-blocksize && screeny<gameheight){
                if (tiles[x][y][0]>0){
                    g.drawImage(levelgraphics, screenx,screeny, screenx+blockwidth,screeny+blockwidth, graphicsize,0,graphicsize*2,graphicsize, null);
                } else {
                    g.setColor(new Color( tiles[x][y][1]));
                    g.fillRect(screenx,screeny,blockwidth,blockwidth);

                    g.setColor(Color.WHITE);
                    g.drawString(String.valueOf(x+y*sizex), screenx+5,screeny+20);
                }
              }
            }
        }
     }

    //world - camera = screen, algebra that world = screen + camera
    //Add camerax to mouse screen co-ords to convert to world co-ords.
    int cursorx_world=(int)camerax+(int)GameInput.mousex;
    int cursorx_grid=(int)cursorx_world/blocksize;  // World Co-ords / gridsize give grid co-ords
    int cursorx_screen=-(int)camerax+(cursorx_grid*blocksize);

    //Add cameray to mouse screen co-ords to convert to world co-ords.
    int cursory_world=(int)cameray+(int)GameInput.mousey;
    int cursory_grid=(int)cursory_world/blocksize;
    int cursory_screen=-(int)cameray+(cursory_grid*blocksize);

    //Draw on screen cursor
   if (cursorx_grid>=0 && cursory_grid>=0 && cursorx_grid<sizex && cursory_grid<sizey){
            //if (detect(cursorx_grid,cursory_grid)){
            //if (tiles[cursorx_grid][cursory_grid][0]>1){
                g.setColor(Color.yellow);
                g.drawRect(cursorx_screen,cursory_screen,blockwidth,blockwidth);
            //}
    }
    //Draw on screen map
    for (int x=0;x<sizex;x++){
        for (int y=0;y<sizey;y++){
            g.setColor(new Color((int) tiles[x][y][1]));
            g.fillRect(mapx+x*mapsize, mapy+y*mapsize, mapsize, mapsize);
        }
    }

   //Outline
            g.drawRect(mapx,mapy,mapsize*sizex ,mapsize*sizey );

   //Map location box
            g.setColor(Color.RED);
            g.drawRect(mapx+mapsize*((int)camerax/blocksize),mapy+mapsize*((int)cameray/blocksize),(gamewidth/blocksize)*mapsize ,(gameheight/blocksize)*mapsize );
}

The last bit of the code adds a map onto the screen and pops a scaled box on it so players can see what part of the map they are looking at.

White spaces are free, use them

Your code is hard to read as all operators are cramped together.

Avoid branches

A missed branch prediction can force your CPU to discard all speculative execution it has done. Although modern CPU's are pretty good at branch prediction, it's a good practice to avoid branches when possible. And in your case it will reduce iterations in the loop.

int screenx = -(int)camerax - blocksize;
for (int x = 0; x < sizex; x++){
    screenx += blocksize;
    if (screenx > -blocksize && screenx < gamewidth){

should be:

int screenx = -(int)camerax - blocksize;
int xstart = Math.max(0, -(blocksize + screenx) / blocksize);
int xend = Math.min(sizex, (gamewidth - screenx) / blocksize);
screenx += blocksize * xstart; // Edit: We need to keep this value updated too.
for (int x = xstart; x < xend; ++x){
    screenx += blocksize;

and similarly for the inner loop. Depending on the ratio camera size to level size (if I'm interpreting your code correctly) this can give a big speedup.

Hint: The y direction iteration start and end values are independent of the current iteration in x direction so calculate the yend and ystart before entering the first loop.

Caution: The above xstart and xend may be off-by-one as I don't know what all the variables are, adjust to taste.

Edit: Oops just realized this is what Neil mentioned in the first point on his answer. But I'll leave this here as it's an example of what he said.

Edit2: Missed the incrementation of screenx prior to entering the loop.


Show/hide text field depending on select option I'm new with JavaScript and need your advice. The idea is to show/hide text field depending on select option. I have a working code, but I believe it could be more readable and better. What have I missed?

   <script type="text/javascript">
<!--
    function showHide(){
        var selectedValue =document.getElementById("choice_criteria_id").value;
        var src_addr = document.getElementById("id_src_addr");
        var dst_addr = document.getElementById("id_dst_addr");
        var port = document.getElementById("id_port");
        var port_range = document.getElementById("id_port_range");

        if (selectedValue == "source_ip"){
            src_addr.style.display = "inline";
            dst_addr.style.display = "none";
            port.style.display = "none";
            port_range.style.display = "none";

        } else if (selectedValue == "dest_ip"){
            dst_addr.style.display = "inline";
            src_addr.style.display = "none";
            port.style.display = "none";
            port_range.style.display = "none";

        } else if (selectedValue == "port"){
            port.style.display = "inline";
            src_addr.style.display = "none";
            dst_addr.style.display = "none";
            port_range.style.display = "none";

        } else if (selectedValue == "port_range"){
            port_range.style.display = "inline";
            src_addr.style.display = "none";
            dst_addr.style.display = "none";
            port.style.display = "none";
        }
}

-->
</script>

You can use Conditional (ternary) Operator.

Syntax :

condition ? expr1 : expr2

Reformated code :

function showHide(){
    var selectedValue =document.getElementById("choice_criteria_id").value;
    document.getElementById("id_src_addr").style.display = (selectedValue == "source_ip") ? "inline" : "none";
    document.getElementById("id_dst_addr").style.display = (selectedValue == "dest_ip") ? "inline" : "none";
    document.getElementById("id_port").style.display = (selectedValue == "port") ? "inline" : "none";
    document.getElementById("id_port_range").style.display = (selectedValue == "port_range") ? "inline" : "none";
}

Example in CodePen


Fizz Buzz Test Driven Development Task:
Create the Fizz Buzz game using Test Driven Developoment. Print the correct word for range 1-100.

The implementation:

public class Main {

    static final String FIZZ = "Fizz";
    static final String BUZZ = "Buzz";
    static final String FIZZ_BUZZ = "Fizz Buzz";

    public static void main(String[] args) {
        IntStream.range(1, 101).forEach((x) -> System.out.println(getWordForNumber(x)));
    }

    public static String getWordForNumber(int x) {
        if (isDivisibleWithoutRemainder(x, 15)) {
            return FIZZ_BUZZ;
        } else if(isDivisibleWithoutRemainder(x, 3)) {
            return FIZZ;
        } else if(isDivisibleWithoutRemainder(x, 5)) {
            return BUZZ;
        }
        return Integer.toString(x);
    }

    private static  boolean isDivisibleWithoutRemainder(int dividend, int divisor) {
        return (dividend%divisor == 0);
    }

}

The JUnit Test:

final class Number {
    private final int n;

    Number (int number) {
      n = number;
    }

    final boolean isMatchedToWord(final String word) {
      return Main.getWordForNumber(n).equals(word);
    }
}

class MainTest {
    @Test
    public void numbersAreMatchedToWords_Works() {
      assertTrue(new Number(1).isMatchedToWord("1"));
      assertTrue(new Number(2).isMatchedToWord("2"));
      assertTrue(new Number(3).isMatchedToWord("Fizz"));
      assertTrue(new Number(4).isMatchedToWord("4"));
      assertTrue(new Number(5).isMatchedToWord("Buzz"));
      assertTrue(new Number(6).isMatchedToWord("Fizz"));
      assertTrue(new Number(10).isMatchedToWord("Buzz"));
      assertTrue(new Number(15).isMatchedToWord("Fizz Buzz"));
      assertTrue(new Number(30).isMatchedToWord("Fizz Buzz"));
    }
}

Test driven development

In test driven development you let the code and the tests evolve by repeating the following steps:

Add a new test case that fails, the simplest possible
Fix the implementation the simplest possible way so that all test cases pass
Optionally refactor the existing code, while keeping all tests passing
Repeat from step 1

There are variations to these steps, but one thing is certain: in the end you will have many test cases. The posted code has one, which means it wasn't really developed using TDD.

Keep it simple

It's good to keep everything as simple as possible. In the test class, do you really need a Number class to wrap an number, so that you create an instance and then call a method on that instance? A simple method would have been enough, that takes an integer as parameter and returns a string.


Check for pangrams in elisp I'm trying to find pangrams ie sentences that contain all the letters of the alphabet, for instance "The quick brown fox jumps over the lazy dog".

I'm an absolute beginner in elisp, and it ended up being a really difficult exercice for me. My idea what to put all the letters in a set check if all the letters of the alphabet are in this set.

I don't think it was a very good idea because elisp doesn't seem to really have sets, so I ended up using a hashmap, which seems very clunky.

Another things that really bothers me is the two if in is-pangram. I feel that they make the function really difficult to reason about, although it's doing something very basic.

(defun add-to-hashmap (hashmap item)
  "Insert ITEM into HASHMAP and return HASHMAP."
  (puthash item t hashmap)
  hashmap)

(defun iter-hashmap-keys (hashmap)
  "Return a sequence of the keys in HASHMAP"
  (let ((keys '()))
    (maphash
     (lambda (key _) (setq keys (cons key keys)))
     hashmap)
    keys))

(defun get-letters (s)
  "Return the set of letters in S.
The set is a hashmap where keys are ascii characters and values are `t`."
  (setq letters (make-hash-table :test 'equal))
  (seq-reduce
   (lambda (hashmap letter) (add-to-hashmap hashmap (downcase letter)))
   (split-string s "")
   letters))

(defvar ALPHABET
  (let* ((letters (get-letters "abcdefghijklmnopqrstuvwxyz"))
         (_ (remhash "" letters)))
    letters))

(defun is-pangram (s)
  "Return `t` is S is a pangram, `nil` otherwise."
  (setq letters (get-letters s))
  (if (seq-some
       ;; this function returns returns `nil` if the given letter is
       ;; *present* in the set, and `t` otherwise. That way, seq-some
       ;; returns a non-nil value when there's a letter that is *not* in
       ;; the set.
       (lambda (letter) (if (gethash letter letters) nil t))
       (iter-hashmap-keys ALPHABET))
      nil
      t))

(provide 'pangram)
```

(setq letters (get-letters s))

You are setting the same letters variable from different functions; in Emacs Lisp, that sets a global variable which is bad style: (1) that pollutes the global namespace, (2) that could overwrite an existing variable, and (3) the functions are not reentrant and could lead to bad suprises with side-effects. Better introduce variables with let.

Instead of doing (remhash "" letters), you can pass T for OMIT-NULLS in split-string.

Another approach is to transform the input text so as to downcase all letters, remove spaces, remove duplicate letters and check if it corresponds to the expected alphabet. For example:

(equalp (delete-duplicates
         (sort (coerce
                (downcase
                 (remove ?\s "The quick brown fox jumps over the lazy dog"))
                'list)
               '<))
        (coerce "abcdefghijklmnopqrstuvwxyz" 'list))


Count digits in a given number using recursion Here is my code that finds the number of digits in a given integer (either positive or negative). The code works and results in expected output.

'''
Program that returns number of digits in a given integer (either positive or negative)
'''
def ndigits(x):
    # Assume for the input 0 the output is 0
    if(x == 0):
        return 0
    if(abs(x) / 10 == 0):
        return 1
    else:
        return 1 + ndigits(x / 10)

pro:

code is clean and easy to understand

con:

a recursive solution may not be optimized to a loop by your interpreter so there might be a lot of memory (stack) waste because of the recursion overhead. (So you could implement it as a loop instead of a recursion)
You do not need the if(abs(x) / 10 == 0) branch. So to simplify your code you could remove it.

Simplified recursive code:

def ndigits(x):
    # Assume for the input 0 the output is 0
    if(x == 0):
        return 0
    else:
        return 1 + ndigits(abs(x) / 10)

Simplified tail-recursive code:
end-recursive methods are likely to be detected by the interpreter and transformed to a loop. So this code might be faster and may nor waste memory.
For more information see wikipedia Tail call

def ndigits(x):
    return ndigits_(0, abs(x))

def ndigits_(s,x):
    if(x == 0):
        return s
    else:
        return ndigits(s+1, x / 10)


Code in Python that parses fund holdings pulled from EDGAR, given a ticker or CIK Challenge:
Write code in Python that parses fund holdings pulled from EDGAR, given a ticker or CIK.
Example:

For this example, we will use this CIK: 0001166559
Start on this page.
Enter in the CIK (or ticker), and it will take you here.
Find the "13F" report documents from the ones listed. Here is a "13F-HR".
Parse and generate tab-delimited text from the XML.

Goals:
The code should be able to use any mutual fund ticker. Try morningstar.com or lipperweb.com to find valid tickers.
Be sure to check multiple tickers, since the format of the 13F reports can differ.
My solution
from bs4 import BeautifulSoup
import requests
import re

def getHoldings(cik):
    """
    Main function that first finds the most recent 13F form
    and then passes it to scrapeForm to get the holdings
    for a particular institutional investor.
    """
    urlSec = "https://www.sec.gov"
    urlForms =  "{}/cgi-bin/browse-edgar?action=getcompany&CIK={}&type=13F".format(urlSec, cik)
    urlRecentForm = urlSec + BeautifulSoup(requests.get(urlForms).content,
                                  'lxml').find('a', {"id":"documentsbutton"})['href']
    contents = BeautifulSoup(requests.get(urlRecentForm).content, 'lxml')
    urlTable = "{}{}".format(urlSec,contents.find_all('tr', \
                            {"class" : 'blueRow'})[-1].find('a')['href'])
    return scrapeForm(urlTable)

def scrapeForm(url):
    """
    This function scrapes holdings from particular URL
    """
    soup = BeautifulSoup(requests.get(url).content, 'lxml')

    holdings = set([h.text for h in soup.find_all((lambda tag : 'issuer' in tag.name.lower()))])
    if(not holdings):
        print("No Holdings at: {}".format(url))
        return
    return holdings

Could you provide me some feedback on my code? I completed this challenge recently and just received a general rejection email, so I want to know how could I improve my solution.

There are a few improvements I would apply to the code.

Code Style

address PEP8 violations, in particular:

variable and function naming - your functions and variables follow the camel case convention, but PEP8 and Python community advocate for lower_case_with_underscores naming style
watch for the use of whitespaces around operators and in expressions

remove unused imports - re module is unused
the backslash is unnecessary and can be removed
the parentheses around not holdings are redundant and can be removed
you can create a set using a set comprehension directly:

holdings = {h.text for h in soup.find_all((lambda tag: 'issuer' in tag.name.lower()))}

I would also define urlSec url and urlForms url template as proper constants
I think you are also overloading the code with too many things in a single expression. Apply the "Extract Variable" refactoring method to improve readability and simplify the code
use urljoin() to join parts of a URL

Web-scraping and HTML-parsing

since you are issuing multiple requests to the same domain, you may re-use requests.Session() instance, which may have a positive impact on performance:

  if you're making several requests to the same host, the underlying TCP connection will be reused, which can result in a significant performance increase

you may also win performance on HTML parsing by utilizing SoupStrainer class which allows parsing only the specific things in the DOM tree

Improved code:

from urllib.parse import urljoin

from bs4 import BeautifulSoup, SoupStrainer
import requests

class Scraper:
    BASE_URL = "https://www.sec.gov"
    FORMS_URL_TEMPLATE = "/cgi-bin/browse-edgar?action=getcompany&CIK={cik}&type=13F"

    def __init__(self):
        self.session = requests.Session()

    def get_holdings(self, cik):
        """
        Main function that first finds the most recent 13F form
        and then passes it to scrapeForm to get the holdings
        for a particular institutional investor.
        """

        forms_url = urljoin(self.BASE_URL, self.FORMS_URL_TEMPLATE.format(cik=cik))

        # get the recent form address
        parse_only = SoupStrainer('a', {"id": "documentsbutton"})
        soup = BeautifulSoup(self.session.get(forms_url).content, 'lxml', parse_only=parse_only)
        recent_form_url = soup.find('a', {"id": "documentsbutton"})['href']
        recent_form_url = urljoin(self.BASE_URL, recent_form_url)

        # get the form document URL
        parse_only = SoupStrainer('tr', {"class": 'blueRow'})
        soup = BeautifulSoup(self.session.get(recent_form_url).content, 'lxml', parse_only=parse_only)
        form_url = soup.find_all('tr', {"class": 'blueRow'})[-1].find('a')['href']
        form_url = urljoin(self.BASE_URL, form_url)

        return self.scrape_document(form_url)

    def scrape_document(self, url):
        """
        This function scrapes holdings from particular document URL
        """
        soup = BeautifulSoup(self.session.get(url).content, 'lxml')

        holdings = {h.text for h in soup.find_all((lambda tag: 'issuer' in tag.name.lower()))}
        if not holdings:
            print("No Holdings at: {}".format(url))
            return

        return holdings


Calculate your total happiness, which depends on the integers in the given sets I am a beginner in programming. And I am learning python as my first language. I went on to do some challenges on HackerRank and stumbled upon this problem. The problem goes like this:

  There is an array of \$\ n \$ integers. There are also \$\ 2 \$
  disjoint sets , \$\ A \$ and \$\ B \$ each containing \$\ m \$
  integers. You like all the integers in set \$\ A \$ and dislike all
  the integers in set \$\ B \$. Your initial happiness is \$\ 0 \$. For
  each \$\ i \$ integer in the array, if \$\ i \in A\$, you add \$\ 1 \$
  to your happiness. If \$\ i \in B\$, you add \$\ -1\$ to your
  happiness. Otherwise, your happiness does not change. Output your
  final happiness at the end.

  Input Format  The first line contains integers \$\ n \$ and \$\ m \$ separated by a space.  The second line contains \$\ n \$
  integers, the elements of the array.  The third and fourth line
  contains \$\ m \$ integers, of \$\ A \$ and \$\ B \$
  respectively.

  Output Format  Output a single integer, your total happiness.

  Sample input  3 2 1 5 3 3 1 5 7
  Sample output 1

Here's my attempt at the answer. My code is working fine with small inputs but when the values of \$\ n \$ and \$\ m \$ are in millions, it gives a Terminated due to timeout :( error. Please help me out with time-efficient code that will execute within the time limits.

elems_count = input().split()
my_set = input().split()
happy_set = input().split()
sad_set = input().split()

happy = 0
for i in range(int(elems_count[0])):
    if my_set[i] in happy_set:
        happy = happy + 1
    else:
        happy = happy - 1
print(happy)

As pointed out in the comments by both MrVajid and AJNeufeld, the previous solution (now deleted because it is actually irrelevant now; see post history) actually did not work.
This is because in reality, the task requires the input array to not be a set, aka it should be allowed to have duplicate entries.

In fact, I had solved that challenge successfully a while ago, using:

n, m = [int(x) for x in input().split()]

array = [int(x) for x in input().split()]
A = {int(x) for x in input().split()}
B = {int(x) for x in input().split()}

happiness = 0

for value in array:
    change = 1 if value in A else -1 if value in B else 0
    happiness += change

print(happiness)

This is using set comprehension ({ }) and passes just fine on HackerRank.

Iteration over the input array as a list is still \$O(n)\$, see here, but containment checks for sets is constant time, \$O(1)\$. Previously, with \$A\$ and \$B\$ being lists, this was also linear as \$O(m)\$, leading to \$O(n*m)\$ aka quadratic time complexity.

Since this task on HackerRank is specifically about set usage, it will judge pedantically about time complexity, with only the approach using sets not running into timeout errors.

Notice that your solution also has an error apart from the set one.
You check for containment in happy_set and decrement happy for a negative result.
However, you first need to check for containment in sad_set first.
Otherwise, you are much sadder than required.


Bank database program I have been trying to improve my Python coding and was just wondering if there was a more pythonic way to perform what I have done so far, namely around the date in the main() function.
import datetime
import sybase

account_list = []

def get_date(): # Date -1 day unless Monday then -3 days
    d = datetime.date.today()
    if d.weekday() == 0:
        date = d - datetime.timedelta(days=3)
        return date
    else:
        date = d - datetime.timedelta(days=1)
        return date

def get_unique_accounts(date):
    conn = sybase.Connection('')
    c = conn.cursor()
    c.execute("SELECT DISTINCT acct from accounts WHERE date = @date", {"@date": date})
    result = c.fetchall()
    for row in result:
        account_list.append(row[0])
    c.close()
    conn.close()

def check_balance(date):
    conn = sybase.Connection('')
    c = conn.cursor()
    c.execute("SELECT TOP 1 name,balance from balance WHERE date = @date", {"@date": date})
    result = c.fetchone()
    c.close()
    conn.close()
    return result

def main():
    date = get_date()
    get_unique_accounts(date) # This will be used for future functions
    check_balance(date)

if __name__ == "__main__":
    main()

I will focus on the get_date() method. First of all, what sort of date is it? Currently the name makes me think it will get the current date, but it's sort of a banking date? Something along the lines of get_last_banking_day() would be more descriptive?
You can move the return to the end of the function as that is the same in both blocks of the if/else:
def get_last_banking_day(): # Date -1 day unless Monday then -3 days
    d = datetime.date.today()

    if d.weekday() == 0:
        date = d - datetime.timedelta(days=3)
    else:
        date = d - datetime.timedelta(days=1)

    return date

You could clean up the import and only import the 2 things needed from datetime, date and timedelta.:
from datetime import date, timedelta

The function would become:
def get_last_banking_day(): # Date -1 day unless Monday then -3 days
    d = date.today()

    if d.weekday() == 0:
        date = d - timedelta(days=3)
    else:
        date = d - timedelta(days=1)

    return date

Now we can look at variable names and do some cleanup, especially the d variable. Always try to avoid non-descriptive names:
def get_last_banking_day(): # Date -1 day unless Monday then -3 days
    today = date.today()

    if today.weekday() == 0:
        last_banking_day = today - timedelta(days=3)
    else:
        last_banking_day = today - timedelta(days=1)

    return last_banking_day

As the today - timedelta(days=1) is the default we use it as such and set the value to this and only use an if-statement to set the exception:
def get_last_banking_day(): # Date -1 day unless Monday then -3 days
    today = date.today()
    last_banking_day = today - timedelta(days=1)

    if today.weekday() == 0:
        last_banking_day = today - timedelta(days=3)

    return last_banking_day


Printing JUnit test results in file by changing and using the 'out' static variable of 'System' class I'm building a framework for comparing files (in my own way) using JUnit. All test cases have been packaged in a JAR which is ran independently using a .bat file I wrote. I needed to output the test results in a file instead of the console.

I was just using normal System.out.println() in the TestRunner class and also in the various test case classes for printing the output on the console.

I found a solution that I could use in the project (Method number 3 of this article).  I redirected the output stream to my output file. Following is the relevant code from the TestRunner class:

public class TestRunner {

    public static void main(String[] args) {
        System.out.println("Testing Started...");

        // Save the System.out instance
        PrintStream oldPrintStream = System.out;

        FileOutputStream outFile = null;

        try {

            outFile = new FileOutputStream("result.txt");

            PrintStream newPrintStream = new PrintStream(outFile);
            System.setOut(newPrintStream);

            Result result = JUnitCore.runClasses(TestSuite.class);

            // Print the results in desired format

        } catch (FileNotFoundException ex) {
            Logger.getLogger(TestRunner.class.getName()).log(Level.SEVERE, null, ex);
        } finally {
            // Reset the old System.out instance
            System.setOut(oldPrintStream);
            System.out.println("Testing Completed! Check output folder for result.");

            try {
                outFile.close();
            } catch (IOException ex) {
                Logger.getLogger(TestRunner.class.getName()).log(Level.SEVERE, null, ex);
            }
        }
    }
}

The code is working fine but is it the correct way to do this?

Looks good enough, but there's some improvements in the io API you're not using. Consider:

try (OutputStream outFile = Files.newOutputStream(Paths.get("result.txt"),
       StandardOpenOption.WRITE, StandardOpenOption.CREATE);
     PrintStream newSysOut = new PrintStream(outFile)) {
     System.setOut(newSysOut);
     Result result = JUnitCore.runClasses(TestSuite.class);
     printResults(result);
} catch (IOException ex) {
     // your code here
} finally {
     System.setOut(oldPrintStream);
}

This should give you more fine-grained Exceptions when your operations fail (not that I'm using it here).
Also you don't need to close the Resources you use yourself, because of the try-with-resources-block.

Small additional note: Your comments are extraneous for this example. I'd suggest removing them.


Integer square root in x86 assembly (NASM) This program calculates the square root of an unsigned 32-bit number with some bit fiddling to get a very close approximation, then one iteration of the Babylonian method to get an exact value. I don't know if this could be faster, but hope it can.

I have trouble with clearly commenting my code, so I will gladly explain unclear portions of code.

[global sqrti]
sqrti:
        mov     eax, [esp+4]
        xor     ecx, ecx       ; clear register to put our approximation in
        ; make the lower 16 bits of CX equal the even-numbered bits of EAX
        shr     eax, 3
        rcr     cx, 1
%rep 14
        shr     eax, 2
        rcr     cx, 1
%endrep
        ; do one iteration of the Babylonian method
        or      ecx, 1          ; prevent a division by zero
        mov     eax, [esp+4]
        xor     edx, edx        ; clear top half of DIV's implied operand
        div     ecx
        add     eax, ecx
        shr     eax, 1
        ret

The first thing which strikes me as odd is how you calculate your initial estimation. If you only take every 2nd bit into account that means a value like 0b10101010101010 would yield a 0 as your initial estimation (or 1 after the  or ecx, 1 adjustment) - not very good. Also it's not clear to me why you skip the first 3 bits, that makes even your best estimates off by a factor of 2.

But I can see what you try to do: just take the "half" of the bits of the initial value. I think you can fix that by first or-ing the even with the uneven bits together and then apply the shifting and roring:

        mov     eax, [esp+4]
        test    eax, eax,
        jnz     positive
        ret              ; Return immediately when input is 0
positive:
        mov     ecx, eax
        shl     ecx
        or      eax, ecx
        bsr     ecx, eax ; Get highest bit for later adjustment
        xor     edx, edx
loop:
        shr     eax, 2
        rcr     dx
        jnz     loop
        shr     cl       ; Adjust for skipped bits
        inc     cl
        rol     dx, cl
        mov     ecx, edx ; Use ecx to store estimation

Note that shr modifies the zero flag, but rcr doesn't so you can jump out of the loop as soon as eax turns out to be 0.

Furthermore note that 1 is an implied parameter for shifting operations if none is given. So you can safe a byte each time you leave it out (and possibly a cycle).

The other thing which is strange is that you only do one iteration. You should repeat it till your estimated value varies only by 0 or 1 between iterations, otherwise it's hard to see how your sqrti function will be useful.

And I don't think you need the or ecx, 1 statement. Just make sure to return immediately if [esp+4] turns out to be 0. And if its positive your ecx register should never become 0 if you get the initial estimation right.


Prevent recursive calls in WinForm event handlers Changing TextBox.Text within the TextBox.TextChanged event can trigger a
recursive call to the same event handler. My class EventHelper is designed
to separate the action performed in the event handler from the mechanism that
prevents the recursive calls. The calls can be used as in:

public static class EventHelperTest
{
    public static void ExampleUsage()
    {
        var eventHelper = new EventHelper();

        var textBox = new TextBox();
        textBox.TextChanged += (s, e) => eventHelper.ExecuteOnce(() =>
        {
            textBox.Text = textBox.Text + "a";
        });

        textBox.Text = "trigger ";

        Debug.Assert(textBox.Text == "trigger a");
    }
}

where the EventHelper is:

public class EventHelper
{
    bool skip;

    public void ExecuteOnce(Action f)
    {

        if (skip)
            return;

        skip = true;
        f();
        skip = false;
    }
}

I am looking for a review of the EventHelper class.

You can improve the EventHelper by reusing the sender argument so that you have only a single instance of the helper. The helper maintains a HashSet<object> that stores senders that are currently handled. This will allow you to use the return value of the Add method which is false if the sender already has been added an thus acts like your skip variable. Finally you Remove the sender from the set as if you were setting skip to false.

public class EventHelper
{
    private readonly ISet<object> _senders = new HashSet<object>();

    public void ExecuteOnce(object sender, Action action)
    {
        if (!_senders.Add(sender))
        {
            return;
        }

        action();
        _senders.Remove(sender);
    }
}

You would now call it with one more parameter:

eventHelper.ExecuteOnce(s, () =>

As far as the code is concerned you should always use {} and perhaps parameter names longer then a single letter f.


Is ToEnumerable for multiple items a good practice? In the similar vein as ToIEnumerable and FromSingleItem

Is this a good improvement for making lists on the fly?

SyntaxSugar.ToIEnumerable("test1","test2","test3");

vs.

new []{"test1","test2","test3"};

Code:

 public static class SyntaxSugar
{
    public static IEnumerable<T> ToIEnumerable<T>(params  T[] items)
    {
        return items;
    }
}

not technically an extension to prevent T clutter

Cons:

longer codefragment

Pros:

more finger friendly- no [] or {}
Encapsulates the collection away from being treated as an array.

As developers we're pretty used to where the keys for all kinds of brackets, braces and parenthesis are and while I agree you may save (a negligible amount of) twists and turns of the wrist, you're just substituting keystrokes for keystrokes, IMO.

You've already made me (as a colleague) have to remember this exists, as opposed to what I know naturally within the language itself; I've no aversion to such a thing, obviously it is part of our job to build on things with the language, but only in cases where it is a concern - this really isn't a concern. Also, I now have to potentially type even more to get my list.

Or is it a concern?..

With all the other keystrokes required from day-to-day second-to-second that utilise modifier keys I think we can rule out the 'saving' this seems to want to achieve, when you look at it relatively.

On the other hand, for the most part we use a powerful IDE (I pity you if this isn't the case), any modern IDE will give you many shortcuts for productivity (such as Visual Studio's snippet insertion, Intellisense and auto-completion) which means we don't need to create unique constructs for such trivial concerns. Sure, it also means your construct can be utilised just as quickly, but becomes redundant as it isn't necessarily quicker. The argument could then be put forth that using these tools stands to mitigate human error.

And, what's wrong with an array? If you don't want something to be treated as an array, use a collection of different sorts.


Print the next smallest and next largest number that have the same number of 1 bits in their binary representation Given an integer , print the next smallest and next largest number that have the same number of 1 bits in their binary representation.

How can I improve this?

void printNextNumber(int num){
    int inputNumOfBits = calcBits(num);

    int  nextLargest = num + 1, nextSmallest = num - 1;

    //nextLargest
    while (true){
        if (calcBits(nextLargest) == inputNumOfBits){
            break;
        }
        nextLargest++;
    }

    //nextSmallest
    while (true){
        if (calcBits(nextSmallest) == inputNumOfBits){
            break;
        }
        nextSmallest--;
    }
    System.out.println("Next largest: " + nextLargest);
    System.out.println("Next smallest: " + nextSmallest);
}

private int calcBits(int x){
    int count = 0;
    int i = 0;

    while (i < 32){
        if ((x & (1<<i)) != 0){
            count++;
        }
        i++;
    }
    return count;
}

You are solving this the "mathematical" way.

what about the "lego" way?

Your number consists of a string with "1"s and "0"s.

[edit]
thanks to @graybeard who pointed that the method must be changed to fulfill the requirement:

The next bigger number is when you switch the position of the most right "0" that is followed by a "1" with that following "1".

int nextBigger(int number){
  char[] binaryDigits = BigIntger.valueOf(number).toString(2).toCharArray();
  int lastZeroFollowedByIndex = findLastZeroFollowedByOneIn(binaryDigits);
  binaryDigits[lastZeroFollowedByIndex]    = '1';
  binaryDigits[lastZeroFollowedByIndex +1] = '0';
  return new BigInteger(new String(binaryDigits),2).intValue();
}


Knuth–Morris–Pratt string match algorithm The Knuth–Morris–Pratt string search algorithm is described in the paper Fast Pattern Matching in Strings (SIAM J. Computing vol. 6 no. 2, June 1977). The initial step of the algorithm is to compute the next table, defined as follows:

  The pattern-matching process will run efficiently if we have an auxiliary table that tells us exactly how far to slide the pattern, when we detect a mismatch at its jth character pattern[j]. Let next[j] be the character position in the pattern which should be checked next after such a mismatch, so that we are sliding the pattern j − next[j] places relative to the text.

The authors give the example of the pattern abcabcacab. If there is a mismatch at j=7:

abcabcacab
abcabca?

Then the pattern should be moved 3 places to the right and matching should continue with the 4th character of the pattern:

   abcabcacab
abcabca?

so next[7] = 4. In some cases we know we can skip the mismatched character entirely, for example if there is a mismatch at j=3:

abcabcacab
abc?

then the search should continue from the character after the mismatch:

    abcabcacab
abc?

These special cases are indicated by next[j] = −1.

(If you're reading the paper, note that the authors use indexes starting at 1 as in Fortran, but the Python convention is to use indexes starting at 0, so that's what I'm giving here.)

This is the code that computes the next table. Please review.

def findPattern(pattern):

    j = -1
    next = [-1] * len(pattern)
    i = 0 # next[0] is always -1, by KMP definition

    while (i+1 < len(pattern)):
        if (j == -1) or (pattern[j] == pattern[i]):
            i += 1
            j += 1
            if pattern[i] != pattern[j]:
                next[i] = j
            else:
                next[i] = next[j]
        else:
            j = next[j]

    return next

if __name__ == "__main__":

    print findPattern("aaaab")
    print findPattern("abaabc")

Output:

[-1, -1, -1, -1, 3]
[-1, 0, -1, 1, 0, 2]

1. Review

There's no docstring.
There's no need for parentheses around conditions (Python is not C), so instead of:

while (i+1 < len(pattern)):

you can write:

while i+1 < len(pattern):

The loop while i+1 < len(pattern) calls the len function on each iteration, even though pattern has not changed. You could avoid this wasted call by caching len(pattern) in a local variable.
The or operator has lower precedence than comparison operators, so instead of:

if (j == -1) or (pattern[j] == pattern[i]):

you can omit the parentheses:

if j == -1 or pattern[j] == pattern[i]:

When there's a choice about whether to test for equality or inequality, then I think it's usually clearer to test for equality, so I would write if pattern[i] == pattern[j] instead of if pattern[i] != pattern[j].
There's a small inefficiency in your code. If the test j == -1 or pattern[j] == pattern[i] passes then you set j = next[j] and go round the while loop again. But the condition on the while loop is a condition on i, which has not changed, so you waste the test. It is better to go straight to the test on j, like this:

m = len(pattern)
while i + 1 < m
    while j > -1 and pattern[i] != pattern[j]:
        j = next[j]
    i += 1
    j += 1
    if pattern[i] == pattern[j]:
        next[i] = next[j]
    else:
        next[i] = j

After making this change, i always increases by 1 on each iteration of the main loop, so we could use a for loop instead to make this clear.

2. Revised code

def kmp_table(pattern):
    """Compute the "next" table corresponding to pattern, for use in the
    Knuth-Morris-Pratt string search algorithm.

    """
    m = len(pattern)
    next = [-1] * m
    j = -1
    for i in range(1, m):
        while j > -1 and pattern[i-1] != pattern[j]:
            j = next[j]
        j += 1
        if pattern[i] != pattern[j]:
            next[i] = j
        else:
            next[i] = next[j]
    return next


Using statement in context of streams and WebClients Mostly a question on which approach would be better and when utilizing the using keyword, would it still be neccesary to close streams programatically. If an object implements IDisposable should using always be used?

Link to my full answer using this sample: Stackoverflow Answer

Option 1:

WebClient webClient;
webClient = new WebClient();
string json = webClient.DownloadString(@"urlToTheJsonResponse");

MemoryStream stream = new MemoryStream((Encoding.UTF8.GetBytes(json)));
DataContractJsonSerializer ser = new DataContractJsonSerializer(typeof(GISData));
stream.Position = 0;
GISData data = (GISData)ser.ReadObject(stream);
stream.Close();

Option 2:

WebClient webClient;
MemoryStream stream;
string json;
GISData data;

using (webClient = new WebClient())
{
    json = webClient.DownloadString(@"urlToTheJsonResponse");
}

using (stream = new MemoryStream((Encoding.UTF8.GetBytes(json))))
{
    DataContractJsonSerializer ser = new DataContractJsonSerializer(typeof(GISData));
    data = (GISData)ser.ReadObject(stream);
    stream.Close();
}

using (webClient = new WebClient())
{
    webClient.DownloadFile(data.href, "C:/" + data.href.Substring(data.href.LastIndexOf("/") + 1));
}

Under normal circumstances, you should always either call Dispose() explicitly or use a using block. This applies especially for streams and related types (like StreamWriter), where not disposing can have very visible bad consequences (e.g. the end of the text won't be written to the file).

But there are some types for which calling Dispose() doesn't actually do anything useful. And WebClient and MemoryStream are among them. They are IDisposable primarily because they inherit that interface from their base class (Component and Stream, respectively). So, I think your Option 1 is fine.

If you are unsure whether you should dispose objects of some type or whether it's one of the few types where not disposing is safe, err on the side of caution and dispose it.


Delegating to a lazily loaded object in Ruby I have an API that takes a class as a configuration parameter, but I don't want to load the class (I'm using Rails) at the time that I'm setting up the API. I figured I could get around this by passing it an object that acts like the class in every way (by delegating to it) but doesn't actually reference the class, and thus load the class. until it us used.

This is what it expects:

MyAPI.configure.model = UserModel

But I'd like to give it something like:

MyAPI.configure.model = ->{ UserModel }

This is what I have. It works as well as expected but I have a feeling I take better advantage of Ruby's features and/or the standard library.

def lazy_delegate(&block)

  delegate = BasicObject.new

  def delegate.method_missing(message, *args)
    @target ||= @target_block.call
    @target.send message, *args
  end

  def delegate.__target__=(block)
    @target_block = block
  end

  delegate.__target__ = block

  delegate
end

I'm using this like:

MyAPI.configure.model = lazy_delegate{ UserModel }

Is there a better way to accomplish this? Does Ruby's standard library have anything that would make this cleaner?

You could pass in a string instead of a proc and use Object.const_get().

class_const = Object.const_get 'String'
=> String
class_const.new "hello"
=> "hello"

With a call-able constructor.

constructor = class_const.public_method :new
=> #<Method: Class#new>
constructor.call "ohai"
=> "ohai"

You need to guard against NameErrors.

Object.const_get('NotThere')
NameError: uninitialized constant NotThere

A lazy load method to build the dependency (Error checking elided for clarity).

def model
  @model ||= build_model('arg1', 'arg2')
end

def build_model(*args)
  Object.const_get(self.configure.model_name).send(:new, *args)
end


Calculating 24 from 4 numbers (math Poker game) This is a game of math in Poker. Two people each have a pile of cards. They then each place 2 cards on the table at the same time. Every card is treated as a number. The players need to find a way to calculate 24 from the numbers. For example, you are given 3 5 4 8, and you can get 24 by (3+8-5) * 4, or 8*4-5-3. This code tries to solve this problem.

Can anyone help me to improve it?

comp()

def comp( nums,i,j,exps,op):
    nums0=[];
    for x in range(0,len(nums)):
        if not (x==i or x==j):
            nums0.append(nums[x])

    if(op=='+'):
        tmp = nums[i]+nums[j];
    elif(op=='*'):
        tmp = nums[i]*nums[j];
    elif(op=='-'):
        tmp = nums[i]-nums[j];
    elif(op=='/'):
        if(nums[j]<0.0001 and nums[j]>-0.0001 ):
            tmp=100000;
        else:
            tmp = nums[i]*1.0/nums[j];
    elif(op=='^'):
        return comp(nums,j,i,exps,'-');
    elif(op=='%'):
        return comp(nums,j,i,exps,'/');

    exps.append(str((nums[i],op,nums[j])));
    nums0.append(tmp);
    return nums0

cal()

def cal(nums, exps):
    if(len(nums)==1):
        return (nums[0]>23.9999 and nums[0]<24.0001);
    pairs = {};
    for i in range(0,len(nums)):
        for j in range(i+1,len(nums)):
            tmp_key= str((min(nums[i],nums[j]),max(nums[i],nums[j])));
            #print tmp_key
            if(pairs.has_key(tmp_key)):
                continue;
            #print 'put '+tmp_key
            pairs[tmp_key]=''
            for op in ('+','-','*','/','^','%'):
                nums0 = comp(nums,i,j,exps,op)
                if(cal(nums0,exps)):
                    print str(exps);
                exps.pop();

cal([3,9,10,7],[]);

cal([5,5,5,1],[]);

cal([10,9,4,1],[]);

A few pieces of feedback:

Semi-colons in Python aren't necessary, unless you're trying to issue two commands on one line like a=5;b=6.  I couldn't find a pep style guide that specifically bans them, but most Python code doesn't use them, as they are unnecessary.
You set temp_key to a str, but that is unnecessary.  I'm assuming you added that because you probably had it as a list before and got a "unhashable" error when trying to use a list as a dictionary key.  This is a perfect spot to use a tuple, and your code will work if you just remove the str (leaving the ()), because tuples are hashable and immutable.
Instead of relying on floating point accuracy, which isn't a problem given the magnitude of your examples, an alternative would be to use the fractions class and all your calculations would be exact.
Instead of recursively calling the comp function for your inverse negative and inverse divide, it'd probably be much simpler to just write tmp = nums[j]-nums[i].  I'd suggest it for the inverse divides too.


Parsing latitude and longitude, using Try[T] to ignore errors I wrote this question asking about how to implement my error handling while still maintaining functional code and based on 200_success's very interesting note on the spaces, I decided to trim() the String first, but I wanted to avoid using try/catch blocks and use more of Scala's specific features, so I thought of a solution with the Try type:

private def tryParseDouble(value: String) = Try(value.toDouble)

def toCoordinates(coordinates: String): Seq[Coordinate] = {
  val coordinatesList = coordinates.split(",")
  coordinatesList flatMap { coordinate =>
    coordinate.trim().split("\\s+").map(tryParseDouble) match {
      case Array(Success(lat), Success(lon)) => Some(Coordinate(lat, lon))
      case _                                 => None
    }
  }
}

Would this be an acceptable way to implement error handling? Or am I overthinking this and a simple try/catch would be just as good?

That looks pretty good to me.

Try is preferable over try/catch since it gives more options on how to process the Failure case.  In particular, the fact that Try can be flatMap'd means that you an build a for-comprehension to cover the processing and essentially abort on the first failure.

for {
  x <- Try("1".toInt)
  y <- Try("a".toInt)
  z <- Try("3".toInt)  // never invoked due to above error
} yield x + y + z

// returns: Failure(java.lang.NumberFormatException: For input string: "a")

For parsing a String to an Int it doesn't matter as much, but if the body of the Try is expensive then some of that cost can be avoided.

Depending on the context you're working in then it might behoove you to capture and log the Failures as well; for some one-off code silently ignoring the failures might be fine, but in an application it can save a lot of heartache when dirty data inevitably makes its way in.


Haskell Hamming Sequence Logic It was suggested I place this code here. I had posted it on StackOverflow for a kind of review.

I have tried to generate Hamming numbers using primes but the speed of the generation slows the higher the number generated and the prime list grew proportionately. I then started to find if a number could be factored down to 2,3 or 5. This was faster but still far away from linear.

Then I found in Excel that a candidate number when evenly divided by one of 2,3 or 5 can be found in previously generated hamming numbers if it is itself a Hamming number.

I thought this approach would result in faster Hammond number generation.

I have played with generating the last of a large set of Hamming numbers using my standard methods. Once the final set is generated all previous Hammings can selectively be extracted from it. The last set just takes to long to generate even though once it is, the speed becomes linear. I just need a faster way to generate any Hamming list.

I do not know if anyone has ever tried to exploit the fact that each successive candidate number can be tested for Hamming status by dividing it by one of 2,3 or 5 and testing if the quotient is a member of the Hamming list generated to that point.

The function takes two parameters, a seed list of (reversed) Hamming numbers less than 10 and a candidate list of any size. I prefer to us a list of [2,3,5] multiples I call base

base = scanl (\b a -> a+b) 2 $ cycle [1,1,1,1,2,1,1,2,2,1,1,2,2,1,1,2,1,1,1,1,2,2]

hamx ls (x:xs)
   | even x        && elem (div x 2) ls = hamx (x:ls) xs
   | mod  x 3 == 0 && elem (div x 3) ls = hamx (x:ls) xs
   | mod  x 5 == 0 && elem (div x 5) ls = hamx (x:ls) xs
   | null xs = ls
   | otherwise = hamx ls xs

I tried filter and any and others in place of elem. The list generates in reverse. The elem finds Hamming matches faster from the bottom and new Hamming numbers appear also at the bottom. So, the bottom is the top.

I am now working on the 2,3,5 Hamming multiples used in other algorithms. I know how not to generate duplicates. I might be able to integrate these select multiples into the above code. For example, the select list will only use 15 multiples of 5 in 6,103,515,625. All of the 10s are already in the 2 multiples and many 5 multiples are found in 3 multiples. The only 3 multiples needed are from odd Hamming numbers. All 2 multiples are needed.

The main problem is that elem traverses list and that is slow. It is better to use Data.Set or HashMap as this data structures allow sub-linear membership checking.

import qualified Data.Set as Set

hamx :: [Integer] -> [Integer] -> [Integer]
hamx ls = reverse . Set.toList . loop (Set.fromList ls)
  where
    loop res [] = res
    loop res (x:xs)
     | even x        && Set.member (div x 2) res = loop (Set.insert x res) xs
     | mod  x 3 == 0 && Set.member (div x 3) res = loop (Set.insert x res) xs
     | mod  x 5 == 0 && Set.member (div x 5) res = loop (Set.insert x res) xs
     | otherwise = loop res xs

Or a bit shorter (but less readable) version:

import Data.List (foldl')
import qualified Data.Set as Set

hamx :: [Integer] -> [Integer] -> [Integer]
hamx ls = reverse . Set.toList . loop (Set.fromList ls)
  where
    loop = foldl' (\res x -> if predicate res x then Set.insert x res else res)
    predicate res x = any (\y -> mod x y == 0 && Set.member (div x y) res) [2, 3, 5]

Using IntSet instead of Set is several times faster, but Daniel Fischer's solution is still several orders of magnitude faster.

I put a project with criterion benchmark here.


Yet another FizzBuzz Since today is apparently a good day for...unlikely FizzBuzz implementations, I decided to contribute another, this one in C++:

#include <iostream>
#include <functional>
#include <vector>
#include "xrange.h"

int main() {
    std::vector<std::function<void(int)> > funcs {
        [](int i) { std::cout << i << "\n"; },
        [](int i) { std::cout << "fizz\n";  },
        [](int i) { std::cout << "buzz\n";  },
        [](int i) { std::cout << "fizzbuzz\n"; }
    };

    auto test = [](int i){return (i % 3 == 0) + 2 * (i % 5 == 0); };

    for (auto i : xrange(1, 101))
        funcs[test(i)](i);
}

This uses the same xrange I posted in a previous answer.

I'd be particularly interested in eliminating having to use test, but a sequence like: [[ is parsed as introducing an attribute rather than using the return from a lambda as an index (and inserting white space doesn't seem to help). I could use .at() instead, but that imposes runtime overhead I'd also rather avoid.

I'd be inclined to make a simple fixed array instead of a std::vector for funcs:

const std::function<void(int i)> funcs[] {
    [](int i) { std::cout << i << "\n"; },
    [](int ) { std::cout << "fizz\n";  },
    [](int ) { std::cout << "buzz\n";  },
    [](int ) { std::cout << "fizzbuzz\n"; }
};

This is useful because it eliminates the overhead of memory allocations for a std::vector and the calls for constructor and destructor.  As an embedded systems guy, I tend to think about whether code can be in ROM rather than RAM, and whether work can be done at compile-time rather than run-time.

Note, too that I've omitted the variable name for the last three lambdas.  There's no real difference except that it prevents the compiler from generating a warning about "unused parameter i" in those three lines.

Also, I think you're overthinking the lambda.  The test lambda isn't really necessary at all since you can call it like this:

funcs[ (i % 3 == 0) + 2 * (i % 5 == 0) ](i);

So (without xrange.h, although one could use that also) it would look like this:

#include <iostream>
#include <functional>

int main() {
    const std::function<void(int i)> funcs[] {
        [](int i) { std::cout << i << "\n"; },
        [](int ) { std::cout << "fizz\n";  },
        [](int ) { std::cout << "buzz\n";  },
        [](int ) { std::cout << "fizzbuzz\n"; }
    };

    for (int i=1; i <= 100; ++i)
        funcs[ (i % 3 == 0) + 2 * (i % 5 == 0) ](i);
}


LeetCode 1463: Cherry Pickup II I'm posting my code for a LeetCode problem copied here. If you would like to review, please do so. Thank you for your time!
Problem

Given a rows x cols matrix grid representing a field of cherries.
Each cell in grid represents the number of cherries that you can
collect.
You have two robots that can collect cherries for you, Robot #1 is
located at the top-left corner (0,0), and Robot #2 is located at the
top-right corner (0, cols-1) of the grid.
Return the maximum number of cherries collection using both robots by
following the rules below:

From a cell (i,j), robots can move to cell (i+1, j-1) , (i+1, j) or (i+1, j+1).
When any robot is passing through a cell, It picks it up all cherries, and the cell becomes an empty cell (0).
When both robots stay on the same cell, only one of them takes the cherries.
Both robots cannot move outside of the grid at any moment.
Both robots should reach the bottom row in the grid.

Example 1:

Input: grid = [[3,1,1],[2,5,1],[1,5,5],[2,1,1]]
Output: 24
Explanation: Path of robot #1 and #2 are described in color green and
blue respectively.
Cherries taken by Robot #1, \$(3 + 2 + 5 + 2) =
> 12\$.
Cherries taken by Robot #2, \$(1 + 5 + 5 + 1) = 12\$.
Total of cherries: \$12 + 12 = 24\$.
Example 2:

Input: grid = [[1,0,0,0,0,0,1],[2,0,0,0,0,3,0],[2,0,9,0,0,0,0],[0,3,0,5,4,0,0],[1,0,2,3,0,0,6]]
Output: 28
Explanation: Path of robot #1 and #2 are described in
color green and blue respectively.
Cherries taken by Robot #1,
\$(1 + 9 + 5 + 2) = 17\$.
Cherries taken by Robot #2, \$(1 + 3 + 4
> + 3) = 11\$.
Total of cherries: \$17 + 11 = 28\$.
Constraints:

\$rows == grid.length\$
\$cols == grid[i].length\$
\$2 <= rows, cols <= 70\$
\$0 <= grid[i][j] <= 100 \$

Inputs
[[3,1,1],[2,5,1],[1,5,5],[2,1,1]]
[[1,0,0,3],[0,0,0,3],[0,0,3,3],[9,0,3,3]]
[[1,10,0,3,86,40],[0,0,0,3,86,40],[0,0,3,3,86,40],[9,0,3,3,86,40], [1,0,40,3,86,40],[0,22,0,3,86,40],[99,0,3,3,86,40],[9,0,3,3,86,40]]

Outputs
24
22
819

Code
#include <vector>
#include <cstring>

struct Solution {
    int map_cherries[70][70][70] = {};

    inline int cherryPickup(std::vector<std::vector<int>> &grid) {
        std::memset(map_cherries, -1, sizeof(map_cherries));
        const int row_length = grid.size();
        const int col_length = grid[0].size();
        return depth_first_search(grid, row_length, col_length, 0, 0, col_length - 1);
    }

private:
    const inline int depth_first_search(
        std::vector<std::vector<int>> &grid,
        const int row_length,
        const int col_length,
        int row,
        int left_robot,
        int right_robot
    ) {
        if (row == row_length) {
            return 0;
        }

        if (map_cherries[row][left_robot][right_robot] != -1) {
            return map_cherries[row][left_robot][right_robot];
        }

        int max_cherries = 0;

        for (int left = -1; left < 2; left++) {
            for (int right = -1; right < 2; right++) {
                const int curr_left_robot = left_robot + left;
                const int curr_right_robot = right_robot + right;

                if (curr_left_robot > -1 and curr_left_robot < col_length and curr_right_robot > -1 and curr_right_robot < col_length) {
                    max_cherries = std::max(max_cherries, depth_first_search(
                                                grid,
                                                row_length,
                                                col_length,
                                                row + 1,
                                                curr_left_robot,
                                                curr_right_robot
                                            ));
                }
            }
        }

        int cherries = grid[row][left_robot];

        if (left_robot != right_robot) {
            cherries += grid[row][right_robot];
        }

        return map_cherries[row][left_robot][right_robot] = max_cherries + cherries;
    }
};

References

Problem

Discuss

Solution

Code Review
This initializes all members to zero:
    int map_cherries[70][70][70] = {};

Then you manually initialize the map again:
        std::memset(map_cherries, -1, sizeof(map_cherries));

This is not doing what you think it is doing.
        std::memset(map_cherries, -1, sizeof(map_cherries));

This is initializing every byte to -1. Each integer is probably 4 bytes (or more long). So each byte is being set to -1. Now it happens that in 2's compliment this work (if you set all bytes to -1 then the integer is also -1). But there is no guarantee in the standard that C++ will use 2's compliment as the representation of integers (even if it is very common).

This returns a const inline int!
    const inline int depth_first_search(
        ...
    )

I know it is legal but come on. That's hard to read. If you have to add inline put it first so we can make the return type readable.
    inline const int depth_first_search(...)

Now we can see that the return type is const int. That still does not make that much sense. Just return the an int.
The inline keyword has no functional use to the C++ compiler (the compiler writers stopped believing developers had a good idea about this years ago). So this keyword has no effect on the code being generated. The only reason it is used is when the function is defined in the header file but not in the class it is needed to let the linker know there may be multiple definitions (as it may have been included multiple times). But when the method is part of the class this is redundant as it is automatically inline.
The general consensus is don't use inline unless you need to use inline. The only time you need to use inline is for method definitions in header files where they are not defined as part of the class.
    int depth_first_search(...)

If you don't intend to modify a reference pass it by const reference.
    const inline int depth_first_search(
        std::vector<std::vector<int>> &grid,
         ...
    )

This will prevent mistakes where you accidentally modify the data.

Nice short circuit here.
        if (row == row_length) {
            return 0;
        }

But here: Not sure this will ever happen!
Nice optimization. I like it.
        if (map_cherries[row][left_robot][right_robot] != -1) {
            return map_cherries[row][left_robot][right_robot];
        }

It took me a while to spot this as an optimization. I would add a comment on WHY this is working and how this is a shortcut optimization.

This if statement is a bit complex. I would split it into a couple of statements.
                if (curr_left_robot > -1 and curr_left_robot < col_length and curr_right_robot > -1 and curr_right_robot < col_length) {

                 // You can do in 1 statement if you want.
                 // But I like the use of a function here as it tells you
                 // what the test is and thus documents the code is doing.
                 if (!inBounds(curr_left_robot)) {
                     continue;
                 }
                 if (!inBounds(curr_right_robot)) {
                     continue;
                 }

Sure that's legal.
        return map_cherries[row][left_robot][right_robot] = max_cherries + cherries;

Makes the line very long though.
I would split into three lines;
        // Add cherries in this row to the previous row.
        cherries += max_cherries;

        map_cherries[row][left_robot][right_robot] = cherries;
        return cherries;


Algorithm for finding inversions in Merge Sort Please help in code reviews for readability, optimizing it, functionality etc.

import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.math.BigInteger;
import java.util.ArrayList;

public class Merge {

    static BigInteger cntInv = new BigInteger("0");

    static void mergeSort(int[] array){

        if(array.length > 1){

            int[] left = leftArray(array);
            int[] right = rightArray(array);

            mergeSort(left);
            mergeSort(right);

            merge(array,left,right);
        }
    }

    static int[] leftArray(int[] array){

        int size = array.length/2;
        int[] left = new int[size];
        for(int i =0 ; i<size;i++){
            left[i] = array[i];
        }
        return left;
    }

    static int[] rightArray(int[] array){

        int mid = array.length/2;
        int size = array.length - mid ;
        int[] right = new int[size];

        for(int i = 0 ; i<size;i++){
            right[i] = array[i + mid];
        }
        return right;
    }

    static void merge(int[] array, int[] left, int[] right){

    int iLeft = 0;
    int iRight = 0;

    /** Traverse through the array */
    for(int i=0;i<array.length;i++){
        if(iRight >= right.length || (iLeft<left.length && left[iLeft]<= right[iRight]) ){

            array[i] = left[iLeft];
            iLeft++;

        }else{

            if(left.length > 0 && iLeft < left.length) {

                cntInv = cntInv.add(BigInteger.valueOf(left.length - iLeft));

            }

            array[i] = right[iRight];
            iRight++;
        }
    }

}

static void display(int[] array){

    for(int i=0; i<array.length; i++){

    //System.out.println("Index of " + i + " : " + array[i]);

    }

}

private static int[] buildIntArray(ArrayList<Integer> integers) {
    int[] ints = new int[integers.size()];
    int i = 0;
    for (Integer n : integers) {
        ints[i++] = n;
    }
    return ints;
}

public static void main(String args[]) throws NumberFormatException, IOException{

    ArrayList<Integer> arrayList = new ArrayList<Integer>();
    BufferedReader reader = new BufferedReader(new FileReader("./num_list.txt"));
    String line = null;
    while ((line = reader.readLine()) != null) {
        arrayList.add(Integer.parseInt(line));
    }

    int[] array = buildIntArray(arrayList);   // new int[10];

    System.out.println("List of number in the array befor sort");

    display(array);

    mergeSort(array);

    System.out.println("List of number in the array after sort");

    display(array);

    System.out.println("Done" + cntInv);

    }

}

For performance reasons, in Merge Sorts, it is common to have just one temporary array for the data, and then you use positions and lengths to work inside the relevant parts of the data.

The above is hard to describe without a code exammple, so the following will have to do.... a typical merge sort algorithm has a recursive method that looks something like:

public void mergeSort(int[] array, int[] temparray, int offset, int length) { ... }

Then, inside that system you do not need to keep creating internal arrays. The array-portion is merged in to the temparray, then copied back (or visa-versa - copied in to the temp array, then merged back).

Still, if you insist on creating mini-arrays for each merge set, you should at least consider using the native code in Java that does the bulk of the work.... for example, you could 'lose the right/left array methods, and your mergeSort could look like:

static void mergeSort(int[] array){

    if(array.length > 1){

        int mid = array.length / 2;

        int[] left = Arrays.copyOfRange(array, 0, mid);
        int[] right = Arrays.copyOfRange(array, mid, array.length - mid);

        mergeSort(left);
        mergeSort(right);

        merge(array,left,right);
    }
}

As for the merge method, it is OK, but it is done as a 'pull' mechanism.... it pulls the values in to the final array by comparing the values from the source arrays. I find it easier to use a push mechanism.... Again, hard to describe without code:

static void merge(int[] array, int[] left, int[] right){

    int iLeft = 0;
    int iRight = 0;
    int iOut = 0;
    /** Traverse through the source arrays */
    while(iLeft < left.length || iRight < right.length){
        if(iLeft < left.length && (i_right >= right.length || left[iLeft] <= right[iRight]) {
            array[iOut++] = left[iLeft++];
        } else {
            array[iOut++] = right[iRight++];
        }
    }

}


Table printing code using a fluent interface Let's say you want to display something like:

One Two Three Four
1   2   3     4

And since you hate the idea of predefining the widths of the columns you want to print it with something like this:

public static void main(String[] args) {
    new Columns()
       .addLine("One", "Two", "Three", "Four")
       .addLine("1", "2", "3", "4")
       .print()
       ;
}

Either listing below can do this.  Which is easier to read?  How would you improve either of them?  Do either paint us into a corner?  Any multithreaded concerns? How could it be made extensible?  Would the getThis() trick help here or is it a bad idea?

Listing A

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

public class Columns {

    List<List<String>> lines = new ArrayList<>();
    List<Integer> maxLengths = new ArrayList<>();
    int numColumns = -1;

    public Columns addLine(String... line) {

        if (numColumns == -1){
            numColumns = line.length;
            for(int i = 0; i < numColumns; i++) {
                maxLengths.add(0);
            }
        }

        if (numColumns != line.length) {
            throw new IllegalArgumentException();
        }

        for(int i = 0; i < numColumns; i++) {
            maxLengths.set(  i, Math.max( maxLengths.get(i), line[i].length() )  );
        }

        lines.add( Arrays.asList(line) );

        return this;
    }

    public void print(){
        System.out.println( toString() );
    }

    public String toString(){
        String result = "";
        for(List<String> line : lines) {
            for(int i = 0; i < numColumns; i++) {
                result += pad( line.get(i), maxLengths.get(i) + 1 );
            }
            result += System.lineSeparator();
        }
        return result;
    }

    private String pad(String word, int newLength){
        while (word.length() < newLength) {
            word += " ";
        }
        return word;
    }
}

Listing B

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

public class Columns {

    List<List<String>> lines = new ArrayList<>();
    List<Integer> maxLengths = new ArrayList<>();
    int numColumns = -1;
    String[] line;

    public Columns addLine(String... line) {

        this.line = line;
        learnNumOfColumnsOnce();
        ensureConsistantNumOfColumns();
        widenColumnsThatNeedIt();
        rememberLine();
        return this;
    }

    private void learnNumOfColumnsOnce() {
        if (numColumns == -1){
            numColumns = line.length;
            for(int i = 0; i < numColumns; i++) {
                maxLengths.add(0);
            }
        }
    }

    private void ensureConsistantNumOfColumns() {
        if (numColumns != line.length) {
            throw new IllegalArgumentException();
        }
    }

    private void widenColumnsThatNeedIt() {
        for(int i = 0; i < numColumns; i++) {
            maxLengths.set(  i, Math.max( maxLengths.get(i), line[i].length() )  );
        }
    }

    private void rememberLine() {
        lines.add( Arrays.asList(line) );
    }

    public void print(){
        System.out.println( toString() );
    }

    public String toString(){
        String result = "";
        for(List<String> line : lines) {
            for(int i = 0; i < numColumns; i++) {
                result += padWithSpaces( line.get(i), maxLengths.get(i) + 1 );
            }
            result += System.lineSeparator();
        }
        return result;
    }

    private String padWithSpaces(String word, int newLength){
        while (word.length() < newLength) {
            word += " ";
        }
        return word;
    }
}

Which is easier to read? How would you improve either of them? Do either paint us into a corner?

I love A. It is simple, clear, concise and meaningful. B is the same as A but with one major difference: the method addLine was cut into several small pieces.

In my opinion, the issue with B is that it extracts from one meaningful method (adding a line) a lot of small awkward methods, and in the process completely obscuring the real task at play: none of those methods are really reusable and probably none of those methods would make sense in a context other than addLine (think removeLine, withColor, withFont...).

Think back and focus on what you want the code to do. We have addLine and we expect it to add a line to our columns. Clearly, there are several code paths to deal with: notably, the first case is to determine the length of each line, etc. But a method can have 30 lines of code without it doing too much work, because it wouldn't make sense to cut that work in pieces: they are all part of the same unit of work. All of the pieces are needed to make the lot work, and they need to be called and assembled in a specific way that is unique to adding a line. In that sense, having multiple methods may be fragile because they are coupled to each-other.

Of course, if that work would require lots of lines of code, you can move part of in a dedicated method. Additionally, work that requires few lines of code could also be extracted in multiple utility methods.

The point is to find a balance between small, easy to read methods and logical unit of work. Part of it can also rely on opinions — both approaches are correct and none of them are fundamentally bad.

  Any multithreaded concerns?

Both approaches won't work in a multi-threaded environment because Columns isn't thread-safe: it has state, for example with List<List<String>> lines and List<Integer> maxLengths. One possible way to make it thread-safe would be to return a new Columns instance each time addLine is called (for that, we can add a private constructor taking lines, maxLengths and numColumns), and making Columns immutable in the process (do not update the fields of this but use the new values to construct the new instance and return it).

  How could it be made extensible?

It is already extensible enough. The data structures for Columns are kept hidden; I can see a removeLine, withColor (setting a color for each column), withFont (setting a font for each column) added without refactoring the current code.

  Would the getThis() trick help here or is it a bad idea?

Do you really intend to have classes inhering from Columns? Keep it simple, you most likely aren't going to need it.

Other comments on the code, applicable to both examples:

You are throwing an IllegalArgumentException in case a line with a different length is added:

throw new IllegalArgumentException();

That's great, but consider adding a message so that it is clearer for the user what went wrong. IllegalArgumentException with no message is not really helpful.
Both in toString and in pad, you are concatenating String with the += operator. This can lean to serious performance issues. When used inside a loop, you should always use a StringBuilder (or a StringJoiner).