Hash read/write/delete functions Here is my JavaScript function to manipulate the hash:

var hash = {
    get: function(key){
        var lh = location.hash;

        if(lh.charAt(0) === "#"){
            lh = lh.slice(1);
        }

        var pairs = lh.split("&");
        var obj = [];
        for(var i = 0; i < pairs.length; i++){
            var pair = pairs[i];
            if(pair.indexOf("=") > 0){
                pair = pair.split("=");
                obj[pair[0]] = pair[1];
            }else{
                obj[pair] = 1;
            }
        }
        return typeof key !== 'undefined' ? obj[key] : obj;
    },
    set: function(key, value, cb){
        var newHash = this.removeKey(key);

        if(newHash.length > 1){
            newHash += "&";
        }
        newHash += key + (typeof value != 'undefined' ? "="+value : "");
        var url = location.href.split("#")[0] + (newHash !== "#" ? newHash : "");

        history.pushState(undefined, document.title, url);
        if(cb){
            cb();
        }
    },
    del: function(key){
        var newHash = this.removeKey(key);
        var url = location.href.split("#")[0] + (newHash !== "#" ? newHash : "");

        history.pushState(undefined, document.title, url);
    },
    removeKey: function(key){
        var string = location.hash;

        if(string.charAt(0) !== "#"){
            string = "#" + string;
        }

        var regex = new RegExp("[#&]"+key+"(=[^&]+|)", 'gi');
        string = string.replace(regex, "");

        if(string.charAt(0) === "&"){
            string = "#" + string.slice(1);
        }

        return string !== '' ? string : '#';
    }
};

I am trying to make it as light as possible.

Streamlined code for get:

get: function(key){
    var lh = location.hash.replace(/^#/, "");
    var pairs = lh.split("&");
    var obj = [], pair;

    for(var i = 0; i < pairs.length; i++){
        pair = pairs[i].split("=");
        if (pair.length === 1) {
            pair.push(1);
        }
        obj[pair[0]] = pair[1];
    }
    return typeof key !== 'undefined' ? obj[key] : obj;
},

I also don't see why there's a callback passed to .set()?  There's no async operation and the callback is executed at the end of the function with no args so the caller could just put that same code right after the .set() call.  There doesn't seem to be any reason for a callback argument.


Converting phone numbers to words with Python For fun, I wrote a script to convert the last four digits (or fewer) of a phone number into a valid word. I wanted to see if my personal phone number could be converted into something like "999-9999-BEAR." I used the unix words file as my dictionary.

I'm wondering how I could make this simpler, since there seems to be a lot of repetition within the function. I'd also like to expand it to consider more digits of the phone number.

I am mostly self taught with Python, so please let me know if I can do anything to make my code prettier!

import pandas as pd

words_df = pd.read_csv(r'C:\Python\Scripts\Data_Playground\unix_words.csv',encoding='latin1')

words_list = words_df['WORDS'].tolist()

phone_dict = {
    '0':None,
    '1':None,
    '2':['a','b','c'],
    '3':['d','e','f'],
    '4':['g','h','i'],
    '5':['j','k','l'],
    '6':['m','n','o'],
    '7':['p','q','r','s'],
    '8':['t','u','v'],
    '9':['w','x','y','z']
}

my_number = '0000009376' # zero example
#my_number = '0000003357' # eels example

def generate_words(phone_number):

    winners = []

    num = str(phone_number[-4:])
    first_letter = ''
    second_letter = ''
    third_letter = ''
    fourth_letter = ''

    if '0' in num or '1' in num:
        print('Sorry, your phone number contains a "0" and/or a "1" and cannot be converted.')

    else:
        #print('Starting number: {0}'.format(num))

        for i in range(len(phone_dict[num[:1]])):
            first_letter = phone_dict[num[:1]][i]

            for j in range(len(phone_dict[num[1:2]])):
                second_letter = phone_dict[num[1:2]][j]

                potential_word = first_letter + second_letter
                if potential_word in words_list:
                    winners.append(potential_word)

                for k in range(len(phone_dict[num[2:3]])):
                    third_letter = phone_dict[num[2:3]][k]

                    potential_word = first_letter + second_letter + third_letter
                    if potential_word in words_list:
                        winners.append(potential_word)

                    for l in range(len(phone_dict[num[3:4]])):
                        fourth_letter = phone_dict[num[3:4]][l]

                        potential_word = first_letter + second_letter + third_letter + fourth_letter
                        #print(potential_word)
                        if potential_word in words_list:
                            winners.append(potential_word)

        if len(winners) == 0:
            print('\n No Winners. :(')

        else:
            print('\nWinners! \n')
            for winner in winners:
                full_winner = my_number[:(len(my_number)-len(winner))] + winner.upper()
                formatted_winner = full_winner[:3] + '-' + full_winner[3:6] + '-' + full_winner[6:]
                print(formatted_winner)

generate_words(my_number)

Currently you are checking all possible combinations of letters and see if it is in your dictionary. It is easier to convert all dictionary words (of the right length) into phone numbers and see if each matches your number. This is especially true if you want to check multiple phone numbers (you only need to do this conversion once). In fact, you can completely pre-compute this by using a dictionary mapping from phone numbers to all (four letter) words in the dictionary.

import pandas as pd
from collections import defaultdict

INVERSE_PHONE_DICT = {"a": "2", "b": "2", "c": "2", "d": "3", ...}

def parse_dictionary(file_name):
    words_df = pd.read_csv(file_name, encoding='latin1')
    words = words_df[words_df.WORDS.str.len() == 4].WORDS.str.lower()

    word_numbers = defaultdict(set)
    for word in words:
        numbers = "".join(INVERSE_PHONE_DICT[c] for c in word)
        word_numbers[numbers].add(word)
    return word_numbers

if __name__ == "__main__":
    file_name = r'C:\Python\Scripts\Data_Playground\unix_words.csv'
    word_numbers = parse_dictionary(file_name)

    phone_number = '0000009376'
    for x in word_numbers.get(phone_number[-4:], []):
        print(f"{phone_number[:3]} - {phone_number[3:6]} - {x.upper()}")

Note that this does not deal with words of length other than four.

I also used collections.defaultdict for easy building of the mapping, used the fact that pandas.Series are directly iterable, used an f-string to simplify the string parsing at the end, completely removed the generate_words function, ensured that the words are of the right length and lower case, added a if __name__ == "__main__": guard to allow importing from this script and follower Python's official style-guide, PEP8, regarding spaces after , in argument lists.


Getting the last date where a given week day occurred I'm trying to learn a little bit about functional programming and as my tool I chose F# since I'm a .NET developer and the environment is more natural to me.

In one of my pet projects I'm dealing with dates, so I created a function to get the last date where a given week day occurred. For example, last Tuesday was yesterday (DateTime.Now.AddDays(-1.0)).

#light

module DateTimeExtensions
    open System

    let rec whenWasLastBasedOnDate (weekDay:DayOfWeek, currentDate:DateTime) =
        match currentDate with
        | _ when currentDate.DayOfWeek = weekDay -> currentDate
        | _ -> whenWasLastBasedOnDate(weekDay, currentDate.AddDays(-1.0))

    let whenWasLast (weekDay:DayOfWeek) =
        let currentDate = DateTime.Now
        match currentDate with
        | _ when currentDate.DayOfWeek = weekDay -> currentDate
        | _ -> whenWasLastBasedOnDate(weekDay, currentDate.AddDays(-1.0))

    let now = DateTime.Now
    let lastSunday = whenWasLast DayOfWeek.Sunday
    let lastMonday = whenWasLast DayOfWeek.Monday
    let lastTuesday = whenWasLast DayOfWeek.Tuesday
    let lastWednesday = whenWasLast DayOfWeek.Wednesday
    let lastThursday = whenWasLast DayOfWeek.Thursday
    let lastFriday = whenWasLast DayOfWeek.Friday
    let lastSaturday = whenWasLast DayOfWeek.Saturday

The most obvious point to make is that whenWasLast repeats the entire code of whenWasLastBasedOnDate. You can (and should) simply write the whole method by just calling whenWasLastBasedOnDate:

let whenWasLast (weekDay:DayOfWeek) =
    whenWasLastBasedOnDate(weekDay, DateTime.Now)

Another point is that you should write AddDays(-1.0) as AddDays -1.0. In languages with ML-like syntax it is generally discouraged to add redundant parentheses around function arguments because it encourages the misconception that the parentheses are part of the method call syntax.

On a somewhat more subjective note, I think that using if would read nicer than a match without any patterns.

On a more general design note it is generally considered good style in functional programming to use higher order functions to express the code more abstractly. For example instead of saying "Check if the given day was a weekDay. If so, return it, if not repeat with the day before" you could say "Look at the last seven days and return the one who is a weekDay". This is not only more idiomatic functional programming, but also makes it impossible to get into infinite recursion by getting the exit condition wrong. In code it would look like this:

[-6.0..0.0] |> Seq.map currentDate.AddDays |>
    Seq.find (fun d -> d.DayOfWeek = weekDay)


Improving performance of a subroutine that checks for a vacancy in a lattice I use the following subroutine to check whether a small subsection of a 3D Int array is all equal to zero. If any value in the particular subsection is non-zero, I exit and return .false.. Within CheckForVacancy, subroutine bc gets called which takes care of out of bounds array indices.

This subroutine gets called millions of times during a particular run and it is responsible for about half the running time of the entire program. In the distant past, I tried to optimize it by changing the order in which I access the Lattice array and by using the any built-in with array slices but I never saw much improvement. Furthermore, the "array slices" solution can get complicated when dealing with the boundaries of the array. I also tried fiddling with optimization flags but I was largely experimenting blindly at that point.

The code is as follows:

LOGICAL FUNCTION CheckForVacancy (x, y, z)
  ! Checks for vacancy for a site [x,y,z]

  use atrpmodule

  IMPLICIT NONE

  INTEGER, INTENT(IN) :: x, y, z
  INTEGER :: Sx, Sy, Sz, i
  INTEGER, DIMENSION(1:26) :: SpaceX = (/1,1,1,0,-1,-1,-1,0,1,1,1,0,-1,-1,-1,0,0,1,1,1,0,-1,-1,-1,0,0/)
  INTEGER, DIMENSION(1:26) :: SpaceY = (/0,0,0,0,0,0,0,0,1,1,1,1,1,1,1,1,1,-1,-1,-1,-1,-1,-1,-1,-1,-1/)
  INTEGER, DIMENSION(1:26) :: SpaceZ = (/1,0,-1,-1,-1,0,1,1,1,0,-1,-1,-1,0,1,1,0,1,0,-1,-1,-1,0,1,1,0/)
  !---------------------------------------------------------------------------

  checkforvacancy=.true.
  do i=1,26
     Sx = x + SpaceX(i)
     Sy = y + SpaceY(i)
     Sz = z + SpaceZ(i)
     call bc(Sx,Sy,Sz)
     if (lattice(Sx,Sy,Sz)/=0)then
        CheckForVacancy=.false.
        exit
     endif
  enddo

END FUNCTION CheckForVacancy

SUBROUTINE bc (x, y, z)
  ! Takes case of boundary conditions
  USE atrpmodule
  IMPLICIT NONE

  INTEGER :: x, y, z
  !---------------------------------------------------------------------------
  IF (x < 1) then
     x = x + LattXDimm
  elseIF(x > LattXDimm) then
     x = x - LattXDimm
  endif

  IF (y < 1) then
     y = y + LattYDimm
  elseIF (y > LattYDimm) then
     y = y - LattYDimm
  endif

  IF (z < 1) then
     z = z + LattZDimm
  elseIF (z > LattZDimm) then
     z = z - LattZDimm
  endif

END SUBROUTINE bc

For reference, Lattice is defined like this:

INTEGER, DIMENSION(:,:,:), ALLOCATABLE :: Lattice

It is allocated depending on program input and then set equal to zero. Also for reference, I am using the GNU compiler.

I suspect that the answer to this questions may be that I cannot do better, but I wanted to see if any Fortran programmers can spot something that I am missing or may want to try.

Three thoughts about this.

First is unless the lattice is small you won't actually run into a boundary condition most times. Check if a boundary condition is possible once and have two CheckForVacancy functions one with and one without boundary checks.

Second thought is remove the boundary check altogether by adding duplicate elements.  In one dimension if you had the following

Index:1,2,3,4,5
Value:3,4,3,2,1

Make the array

Index:0,1,2,3,4,5,6
Value:1,3,4,3,2,1,3

So you trade extra storage to remove the bounds checks.

Finally, you still have a lot of no_ops in the for loop. All those add zero statements do nothing.  You could unroll the loop to eliminate them at the expense of more code. You removed the 0,0,0 option already I think.

Your comment indicates that the lattice gets modified during execution, that makes removing the bounds check more complex.

Depending on the size of the lattice and the number of times you call CheckForVacancy for an individual cell you could either memoize the cells adjacent to a cell or pre-compute them.  This would work if you have enough memory and if you call the function with the same values multiple times.


Simplified encryption API wrapper for RSA and AES I'm working on an encryption library to simplify how my team uses encryption for api credentials and receiving encrypted messages from other teams/projects.

This code is wrapping java.security and BouncyCastle implementations. These classes are just simplifying the calls to those libraries. But I'm struggling with designing the appropriate abstraction for my top-level classes.

I started with these top-level interfaces to define the domain:

public interface Key {
    byte[] getBytes();
}

public interface Message {
    byte[] getBytes();
}

public interface Secret {
    byte[] getBytes();
}

public interface Decrypter {
    Message decrypt(Secret secret, Key key);
}

public interface Encrypter {
    Secret encrypt(Message message, Key key);
}

This worked well to wrap RSA encryption:

public class KeyPair {
    private final Key publicKey;
    private final Key privateKey;

    public Key getPublicKey() {
        return publicKey;
    }

    public Key getPrivateKey() {
        return privateKey;
    }
}

public class RsaEncrypter implements Encrypter {

    @Override
    public Secret encrypt(Message message, Key publicKey) {
        // Perform Encryption
    }
}

public class RsaDecrypter implements Decrypter {
    @Override
    public Message decrypt(Secret secret, Key privateKey) {
        // Perform the decryption
    }
}

But now that I'm applying it to our AES Encryption use cases I've encountered a problem. The Secret contains an InitializationVector because we're using AES in CBC mode.

So I've got this:

public class AesSecret implements Secret {
    private byte[] cipherText;
    private byte[] initilizationVector;

    @Override
    public byte[] getBytes() {
        return cipherText;
    }

    public byte[] getInitilizationVector() {
        return initilizationVector;
    }
}

public class AesDecrypter implements Decrypter {
    @Override
    public Message decrypt(Secret secret, Key key) {
        try {
            return decrypt((AesSecret) secret, key);
        } catch (ClassCastException e) {
            throw new IllegalArgumentException("AesDecrypter only supports subclasses of AesSecret", e);
        }
    }

    public Message decrypt(AesSecret secret, Key key) {
        // Do the AES Decryption
    }
}

The ClassCastException makes me think that I'm violating the Liskov Substitution Principle, and introducing the parallel hierarchy code-smell. I've read the Visitor pattern is a common solution to this code smell, but I haven't figured out how it would apply to my situation.

Any suggestions? Or am I over-thinking this?

I don't think you are over-thinking it, casting is always a code-smell. The fundamental problem is that you separate logic from data, the key (data) and the actual usage of that key (data). This is a problem in object-orientation.

Here is an alternative possible design, which does not do that:

public interface SecretKey {
    PublicKey generatePublicKey();

    CipherText encrypt(byte[] plaintext); // Symmetric encrypt
}

public interface PublicKey {
    CipherText encrypt(byte[] plaintext); // Asymmetric encrypt
}

public interface CipherText {
    byte[] decrypt();
}

It all depends of course on your requirements, this may not exactly match what you are looking for, but the key idea here is that the data and function are not separated.


User-logging authentication I'm little frustrated because I don't know how well I can refactor this script. I wrote the authentication function but it's too complicated, maybe too large in one function, and seems like spaghetti code. How will I be able to make this a little better?

I'm using Restify framework for NodeJS. There are a function that checks if a user is logged. There is a session hash (pnp_access_token) that I need check with another webservice, but I don't need check all times, only the first, so I'm using a cache. I'm using Redis to set a cache and only check the first time.

I don't want refactor business rules.  What can be done about this?

"use strict";
var errorResponse = require('../errorResponse'),
    paynopain = require('../../core/paynopain'),
    User = require('../../core/User'),
    cache = require('../../core/cache/cache'),
    validateValues = require('../validateValues'),
    API_PATH = 'v1/';

function authenticate(req, res, next) {

    if (req.url.indexOf(API_PATH) == 0){

        var pnpUserId;
        var validated = validateValues(req, ['pnp_access_token']);
        if(validated.error){
            return next(validated.error);
        }
        var pnpAccessToken = req.params.pnp_access_token;

        cache.read('pnpToken:' + pnpAccessToken)
            .then(function(userId){
                if(userId){
                    storeUserIdInRequest(req, next, userId);
                }else{
                    var user = new User(req.logger);
                    return paynopain.getIdFromAccessToken(pnpAccessToken)
                        .then(function(pnpUserIdResult){
                            pnpUserId = pnpUserIdResult;
                            return user.findByPnpID(pnpUserIdResult);
                        })
                        .then(function(userData){

                            if(userData._id){
                                return storeUserIdInRequest(req, next, userData._id.toString());
                            }else{
                                var newUser = {
                                    pnpId: pnpUserId,
                                    radius: 1000
                                };
                                return user.create(newUser)
                                    .then(function(userData){
                                        return storeUserIdInRequest(req, next, userData._id);
                                    });
                            }

                        });
                }
            })
            .fail(function(e){

                if(e.message === 'invalid_grant'){
                    next(errorResponse.unauthorized());
                }else{
                    req.logger.log('error', 'Error authenticating', {
                        error: {
                            message: e.message,
                            stack: e.stack
                        }
                    });
                    next(errorResponse.internalError());
                }

            });

    }else{
        next();
    }

}

function storeUserIdInRequest(req, next, userId){

    var oneHourInSeconds =  60 * 60;
    cache.write('pnpToken:' + req.params.pnp_access_token, userId, {
        expire: oneHourInSeconds
    })
        .then(function(){
            req.userId = userId;
            req.logger.setExtra({
                userId: userId
            });
            next();
        });

}

function init(server){
    server.use(authenticate);
    require('./configuration/routes')(API_PATH, server);
    require('./products/routes')(API_PATH, server);
    require('./tracking/routes')(API_PATH, server);
    require('./lists/routes')(API_PATH, server);
}
module.exports = init;

Interesting question,

I think the first thing to do is read this.

You could replace this:

if (req.url.indexOf(API_PATH) == 0){
  //Lots of code
}else{
  next();
}

To

if (req.url.indexOf(API_PATH) != 0){
  next();
  return;
}
//Lots of code,with one less level of indentation

This early exit makes the rest of the code look less busy.

Furthermore, in cases like this

req.logger.log('error', 'Error authenticating', {
  error: {
    message: e.message,
    stack: e.stack
  }
});

You should notice that the object you build has the exact same property names as in the e object, you might as well do the the far cleaner

req.logger.log('error', 'Error authenticating', { error: e });

On a side note, I would also log invalid log-ins, this way you can detect when someone might be trying to hack in to your system.

You can remove some of the arrowness here as well:

                    .then(function(pnpUserIdResult){
                        pnpUserId = pnpUserIdResult;
                        return user.findByPnpID(pnpUserIdResult);
                    })
                    .then(function(userData){

                        if(userData._id){
                            return storeUserIdInRequest(req, next, userData._id.toString());
                        }else{
                            var newUser = {
                                pnpId: pnpUserId,
                                radius: 1000
                            };
                            return user.create(newUser)
                                .then(function(userData){
                                    return storeUserIdInRequest(req, next, userData._id);
                                });
                        }

I am not sure why you need 2 then's, so I will just merge them.

                    .then(function(pnpUserIdResult){
                        pnpUserId = pnpUserIdResult;
                        var userData = user.findByPnpID(pnpUserIdResult);
                        if(userData._id){
                            return storeUserIdInRequest(req, next, userData._id.toString());
                        }
                        return user.create({
                            pnpId: pnpUserId,
                            radius: 1000
                        }).then(function(userData){
                            return storeUserIdInRequest(req, next, userData._id);
                        });

From a naming perspective;

I get really annoyed by the pnp acronym all over the place, it should be established on top that this for pnp and then you can just go with user and userId or user.id
paynopain <- this is a meaningless name, why would it have getIdFromAccessToken?
var user = new User(req.logger);, whatever user is here, it does not represent a user since you only use it for findByPnpID..
radius: 1000, what does radius mean for a user ? What does 1000 mean ?

Finally, for storeUserIdInRequest (which lies about what it does), you dont have to wait for the cache to be set, you can simply (and this should be faster) do

function storeUserIdInRequest(req, next, userId){
  cache.write('pnpToken:' + req.params.pnp_access_token, userId, {
      expire: 60 * 60 //1 hour in seconds
  });
  req.userId = userId;
  req.logger.setExtra({
    userId: userId
  });
  next();
}


Refactor mass assigning from a hash Here is the piece of my controller action that takes a session[:auth] hash and then assigns the values to the ActiveRecord object represented by @user whilst the key names happen to be the same.

def new
  @user = User.new

  if session[:auth].present?
    @user.first_name = session[:auth].extra.raw_info.first_name
    @user.last_name = session[:auth].extra.raw_info.last_name
    @user.username = session[:auth].extra.raw_info.username
    @user.email = session[:auth].extra.raw_info.email
  end
end

I wonder if there's a way to eliminate the duplications in assigning the values from the hash to the object.

To add something to Flambino's answer, I'd suggest moving any non-trivial code to the model (code is then easier to test, re-usable and controllers are kept simple):

class User
  def self.new_from_auth(auth)
    attributes = [:first_name, :last_name, :username, :email]
    user_attributes = if auth
      info = auth.extra.raw_info
      Hash[attributes.map { |attr| [attr, info.send(attr)] }]
    else
      {}
    end
    User.new(user_attributes)
  end
end

class UsersController
  def new
    @user = User.new_from_auth(session[:auth])
  end
end


Getting correct English grammar Does someone know of a better way to get proper grammar in sentences than this method of mine? I don't want to continue with this for my entire huge program if there is a much more elegant way.

#include <iostream>
#include <string>

enum Gender {Male, Female};
enum Capitalized {Capital, NonCapital};

struct Person {
    std::string name;
    Gender gender;
    Person (const std::string& newName, Gender sex) : name (newName), gender (sex) {}
} *you;

std::string youHeShe (const Person* person, Capitalized capital = NonCapital) {
    return (capital == NonCapital) ?
        ( (person == you) ? "you" : (person->gender == Male) ? "he" : "she" ) :
        ( (person == you) ? "You" : (person->gender == Male) ? "He" : "She" );
}

std::string isAre (const Person* person) {
    return (person == you) ? "are" : "is";
}

std::string verb (const Person* person, const std::string& verb) {
    return (person == you) ? verb : verb + "s";
}

int main() {
    std::string yourName;
    std::cout << "What is your name? ";
    std::getline (std::cin, yourName);
    you = new Person (yourName, Male);
    Person *mary = new Person ("Mary", Female), *bob = new Person ("Bob", Male);
    Person* people[] = {you, mary, bob};
    for (const Person* x : people)
        std::cout << youHeShe(x, Capital) << " (" << x->name << ") " << isAre(x) <<
        " awesome because " << youHeShe(x) << " " << verb(x, "rock") << "!" << std::endl;
}

Output:

  What is your name? John Doe

  You (John Doe) are awesome because you rock!

  She (Mary) is awesome because she rocks!

  He (Bob) is awesome because he rocks!

In my program, there are sentences involving either the 3rd person or the 2nd person, so the grammar has to be correct based on which it is.

Of course, my method will involve many more such grammar functions, but I just need to know if there is a better way before I write those other functions (and special cases).

It seems like you would need to write 3 different sentences for every out you want.
Format strings can help here (maybe not in the edge cases - however you have given no example of those)

"%s rock%s!".
The first string is then either "You", "He", or "She".
The second one is for the "He, She, It the s must fit" rule.

What about the am/are/is/... distinction? You could configure a place holder

#define ARE_IS "{AREIS}"
const std::string DUMB_MESSAGE = "%s" ARE_IS " dumb!";

and then replace ARE_IS with the correct form.

How fitting this solution is, how flexible and how much work you will have with that - we can't guess that with the information given.


Arduino controller to control the backlighting of a gear selector Background

The gear selector on a car allows the driver to select which state the transmission should be in.

In the example picture, one of the five letters (P, R, N, D & S) lights up to indicate which state the transmission is presently in.

The position of the rotary dial is sufficient to determine which state the gear selector should be in (and subsequently which letter should light up). With this analog input (coming from a pair of Hall sensors), an Arduino board should be able to figure out which LED should be lit.

Code at present

This is my first attempt at writing C code, so pardon any Perl dialect.

"U" represents an undefined state

#define NOFIELD 505L       // Calibration constant
#define TOMILLIGAUSS 1953L // Unit conversion factor

#define P_led 13
#define R_led 12
#define N_led 11
#define D_led 10
#define S_led 9
#define U_led 8

#define numLED 5

struct interval
{
    int lower;
    int upper;
};

struct state
{
    char name;
    int led;
    interval gauss1;
    interval gauss2;
} ;

state shift_condition[] = {
  { 'P', P_led, {  74,  95 }, { -86, -40 } },
  { 'R', R_led, {  61,  92 }, { -20,  10 } },
  { 'N', N_led, { -30,   6 }, {  36,  90 } },
  { 'D', D_led, { -80, -53 }, {   2,  50 } },
  { 'S', S_led, { -25,  15 }, { -80, -25 } },
};

state prev_state;
long prev_gauss1;
long prev_gauss2;
bool gear_defined;

bool inRange( long value, struct interval range )
{
  return range.lower <= value && value <= range.upper;
}

void selectGear(int raw1, int raw2)
{

    long gauss1 = ( raw1 - NOFIELD ) * TOMILLIGAUSS / 1000;
    long gauss2 = ( raw2 - NOFIELD ) * TOMILLIGAUSS / 1000;

    // if no signal, no need to switch gear, bail out
    if ( prev_gauss1 == gauss1 && prev_gauss2 == gauss2 ) {
      return;
    }

    // update values for next evaluation

    prev_gauss1 = gauss1;
    prev_gauss2 = gauss2;

    // if new readings in existing state range,
    // no need to switch gear, bail out
    if ( gear_defined && inRange(gauss1, prev_state.gauss1) && inRange(gauss2,prev_state.gauss2) ) {
      return;
    }

    for( int i=0; i<numLED; i++) { // determine new state

        state curr_state = shift_condition[i];

        if ( inRange(gauss1, curr_state.gauss1) && inRange(gauss2, curr_state.gauss2) ) {

            Serial.print("Switching from ");
            Serial.print(prev_state.name);
            Serial.print(" to ");
            Serial.println(curr_state.name);

            digitalWrite(prev_state.led, LOW);
            digitalWrite(curr_state.led, HIGH);

            prev_state = curr_state;
            gear_defined = true;
            return;
        }
    }

    // no states matched, default to "undefined" state

    gear_defined = false;
    digitalWrite(prev_state.led, LOW);
    digitalWrite(U_led, HIGH);
}

void setup()
{
    Serial.begin(9600);
    pinMode(P_led, OUTPUT);
    pinMode(R_led, OUTPUT);
    pinMode(N_led, OUTPUT);
    pinMode(D_led, OUTPUT);
    pinMode(S_led, OUTPUT);
    pinMode(U_led, OUTPUT);
}

void loop()
{
    selectGear(analogRead(1),analogRead(2));
}

Questions/Concerns

As this is my first attempt at writing C code, what constructs/conventions could I leverage to write more robust code? The "undefined" state in particular bothers me, as does the fact that there doesn't seem to be a built-in way to count the length of an array.
Are there any gotchas pertaining to Arduino and typedefs? I tried to set a typedef on a struct and the code didn't compile.
I used char name to act as a pseudo-hash key. Is there a smarter way to go about defining dictionaries or hashes with (or without?) structs in C?
In the context of Arduino programming, how good is this implementation of a finite state machine? Are there any issues that may not be obvious in the course of this limited testing?
How would the use of pointers benefit memory usage here?

The main purpose of posting this question is to learn better C (& Arduino) by doing.

I see some things that may help you improve your code.

The Arduino language isn't C

The language used for the Arduino isn't quite C and isn't quite C++, so if your goal is "to learn C," you might want to be careful about the differences.  Specifically, the Arduino's use of setup and loop is unique to it.  Also, all of the "built-in" things such as digitalWrite and Serial are non-standard.  You can still learn useful things by learning to program the Arduino, but it's important to remain aware of the differences.

Know the differences between C++ and C

As mentioned above, the Arduino language isn't quite C or C++, but in most ways, it's closer to C++ than C.  One such difference that affects this code is the use of the struct interval.  In C++ the name of a struct automatically defines a type.  In C, it does not.  So the state in plain C would need to be written like this:

struct state
{
    char name;
    int led;
    struct interval gauss1;
    struct interval gauss2;
};

Alternatively, one could use a typedef in the declaration of the interval structure like this:

typedef struct interval_s
{
    int lower;
    int upper;
} interval;

Eliminate global variables where practical

Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error.  Eliminating global variables where practical is always a good idea, whether programming for desktop machines or for embedded systems.  For global variables such as prev_gauss1 and prev_gauss2, consider wrapping them in objects to make it easier to see that they're related.  Here again, this is a difference in the Arduino language where it is common to have global variables and plain C where it is rare.

Understand the fundamental types

The analogRead() call returns an int (which is 2 bytes on the Arduino), but the converted value of gauss1 and gauss2 is a long which is 4 bytes long on the Arduino.  However, all of the numbers in the table are small enough to fit into an int which suggests that you probably don't really need a long there.

Minimize runtime conversions

It's not critical here, because normal drivers don't expect microsecond shifting, but in general, it's good to minimize the work that an embedded system must do at runtime.  This tends to make the program more efficient and more responsive, as well as usually resulting in smaller code.  In this case, the raw readings are taken from the sensors, converted using a simple linear equation and then compared to fixed values in the table.  Instead, the table could store the corresponding raw values and the comparison could be made without conversion.

Make use of defined constants

The numLED constant is defined, correctly, but their connection to the array of states is not explicit.  I'd suggest changing the declaration from this:

state shift_condition[] = {

To this:

state shift_condition[numLED] = {

Reconsider naming

The names gauss1 and gauss2 are not terrible, but it might be worth considering what the values really mean.  Because they're actually sensor data, it might be better to name them for what they represent rather than the units.

Set states explicitly

The only time the U_led is written, it's set HIGH if no states are matched.  If it ever gets to that point, there's no recovery even if it gets in range of one of the gears.  Further, it's often good in embedded systems to set states explicitly.  So when you set one LED, it's often good to explicitly unset the LEDs that are not supposed to be lit.  The reason for this is that glitches can happen in the real world.  For example, an energetic gamma particle can flip a bit in RAM and it's useful to have a recovery for such events.


Parse a simple key=value config file in C Write a program that should read a file formatted strictly like
  this below:

$ cat sample

bingo=2
bingo2=939
bingo3=text

  The left and right side of = are consecutively parameter name and
  its respective value. The values can be integer or strings. Each parameter is
  separated by a newline. The parameter name and its value will not be more than
  127 bytes.

This program is fed an input search token, and if found, parse_param_file() will fill the input arguments with string or integer value.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

#define TEXT "sample"

/*
 * Function: parse_param_file
 * Arguments:
 * token    - param name that needs to be looked up
 * buf      - buffer for string value. Pass NULL if unwanted
 * buf_int  - buffer for integer value. Pass NULL if unwanted
 * filename - string formatted path to the file
 */
int parse_param_file(const char *token, char **buf, int *buf_int,
        const char *filename) {
    if (!token || (!buf && !buf_int)) {
        printf("%s: invalid args\n", __FUNCTION__);
        return 1;
    }
    FILE *f = fopen(filename, "r");
    if (f == NULL) {
        printf("%s: %s not found\n", __FUNCTION__, filename);
        return 1;
    }
    char *left = malloc(128);
    char *right = malloc(128);
    char *dlim_ptr, *end_ptr;
    char fbuf[256];
    while (fgets(fbuf, 256, f)) {
        dlim_ptr = strstr(fbuf, "=");
        end_ptr = strstr(dlim_ptr, "\n");
        strncpy(left, fbuf, dlim_ptr - fbuf);
        strncpy(right, dlim_ptr + 1, end_ptr - dlim_ptr - 1);
        if (strcmp(left, token) == 0) {
            /* Param found */
            if (buf != NULL)
                *buf = strdup(right);
            if (buf_int != NULL)
                *buf_int = atoi(right);
            fclose(f);
            free(left);
            free(right);
            return 0;
        }
    }
    fclose(f);
    printf("%s: Param not found in %s\n", __FUNCTION__, filename);
    free(left);
    free(right);
    return 1;
}

/* How to use parse_param_file() */
int main(void) {
    char *buf = malloc(128);
    int val = 0;

    /* Test 1 */
    if (parse_param_file("bingo2", NULL, NULL, TEXT))
        fprintf(stderr, "parse_param_file() failed\n");
    printf("buf : %s\n", buf);
    printf("int : %d\n", val);
    /* Test 2 */
    if (parse_param_file("bingo3242", NULL, &val, TEXT))
        fprintf(stderr, "parse_param_file() failed\n");
    printf("buf : %s\n", buf);
    printf("int : %d\n", val);
    /* You must clear *buf and val if you want to reuse them as args again */
    /* Test 3 */
    if (parse_param_file("bingo3", &buf, &val, TEXT))
        fprintf(stderr, "parse_param_file() failed\n");
    printf("buf : %s\n", buf);
    printf("int : %d\n", val);

    free(buf);
    return 0;
}

Interface
I'm not overly fond of the interface you've defined to the function. I think trying to combine reading integers and reading strings into a single function makes it more difficult to use. For most C code, I think the old guiding principle of UNIX ("do one thing, and do it well") provides excellent guidance. As such, I'd probably have two separate functions, one for reading a string, the other for reading a number.
I haven't tested it to be sure, but at first glance it also looks like if you ask to read a number, it does a poor job of signalling failure. For example, if your file has: foo=0 and you try to read foo as a number, it'll set the number to 0, and return 0. But if the file contains foo=Joe instead, it looks to me like it'll do exactly the same.
I'd prefer that if it couldn't convert the rest of the line after the = to a number that it return 1 to indicate that it has failed.
Memory Allocation
Since you only use our left and right inside of your function, and free them again before returning, there's probably no need to allocate them on the heap. You can just allocate them as local arrays, and use them. The obvious exception to this would be if you were targeting a tiny micro-controller that might well have less than 256 bytes available on the stack. In that case, however, you usually won't have a file system either, so the entire function becomes irrelevant.
Standard Library
I think judicious use of fscanf can simplify the code quite a bit.
For reading a string, it seems like the obvious interface would be to just return the value part of the pair, and if it's not found, return a NULL pointer:
char *read_string(FILE *file, char const *desired_name) {
    char name[128];
    char val[128];

    while (fscanf(file, "%127[^=]=%127[^\n]%*c", name, val) == 2) {
        if (0 == strcmp(name, desired_name)) {
            return strdup(val);
        }
    }
    return NULL;
}

When reading a number, we probably need something closer to your original interface, with a return value to indicate success/failure, and receive a pointer to an int where we write the value (if it's found):
int read_int(FILE *file, char const *desired_name, int *ret) {
    char *temp = read_string(file, desired_name);

    char *stop;
    *ret = strtol(temp, &stop, 10);
    int ret_val = stop == NULL || *stop != '\0';
    free(temp);
    return ret_val;
}

Note that as it stands right now, the return value reflects converting the entire remainder of the line (everything after the =) to the desired number. If part of it can be converted, that part will be written to the destination, but the return will still indicate failure. For example, if you had foo=1234abcd, attempting to read foo as a number would give you 1234, but return 1 because the abcd part couldn't be converted.
It would also be possible to simply ignore any trailing garbage, so foo=1234abcd would read the 1234 part, and return 0 to signal success:
int read_int(FILE *file, char const *desired_name, int *ret) {
    char *temp = read_string(file, desired_name);

    char *stop;
    *ret = strtol(temp, &stop, 10);
    int ret_val = stop == temp;
    free(temp);
    return ret_val;
}

In this case, we just need to check that at least one character was converted, in which case stop will have be advanced at least one character after the input.


Random IP Address Generator randip.py

I know there are probably better, easier ways to do this.
It was just a bit of a learning exercise for the sake of familiarising myself with Python.

It takes a single argument (positional parameter):
Either a 4 (IPv4) or a 6 (IPv6).

Usage:

./randip.py 4
61.104.170.242

./randip.py 6
4bfc:391d:3ec8:68ef:0ec8:529b:166d:2ece

Code:

#!/usr/bin/env python3

from sys import argv

from random import randint, choice
from string import hexdigits

def random_ip(v):

    if v == 4:
        octets = []
        for x in range(4):
            octets.append(str(randint(0,255)))
        return '.'.join(octets)

    elif v == 6:
        octets = []
        for x in range(8):
            octet = []
            for x in range(4):
                octet.append(str(choice(hexdigits.lower())))
            octets.append(''.join(octet))
        return ':'.join(octets)

    else:
       return

def main():
    print(random_ip(int(argv[1])))

if __name__ == '__main__':
    main()

Python is often described as a "batteries included" kind of language, and this is no exception.

There's a module just for IP address manipulation and another module to generate random numbers. Put together, they do exactly what you want, in a way that's slightly more readable (IMO).

For this example, I'll assume that the variable v contains either 4 or 6.

from random import getrandbits
from ipaddress import IPv4Address, IPv6Address

if v == 4:
    bits = getrandbits(32) # generates an integer with 32 random bits
    addr = IPv4Address(bits) # instances an IPv4Address object from those bits
    addr_str = str(addr) # get the IPv4Address object's string representation
elif v == 6:
    bits = getrandbits(128) # generates an integer with 128 random bits
    addr = IPv6Address(bits) # instances an IPv6Address object from those bits
    # .compressed contains the short version of the IPv6 address
    # str(addr) always returns the short address
    # .exploded is the opposite of this, always returning the full address with all-zero groups and so on
    addr_str = addr.compressed

print(addr_str)

Here, addr_str will hold a fully random IPv4 or IPv6 address.

You can even generate random addresses from a subnet like this:

from random import getrandbits
from ipaddress import IPv4Network, IPv4Address

# network containing all addresses from 10.0.0.0 to 10.0.0.255
subnet = IPv4Network("10.0.0.0/24")

# subnet.max_prefixlen contains 32 for IPv4 subnets and 128 for IPv6 subnets
# subnet.prefixlen is 24 in this case, so we'll generate only 8 random bits
bits = getrandbits(subnet.max_prefixlen - subnet.prefixlen)

# here, we combine the subnet and the random bits
# to get an IP address from the previously specified subnet
addr = IPv4Address(subnet.network_address + bits)
addr_str = str(addr)

print(addr_str)

Here, addr_str will always contain IP addresses like 10.0.0.184, 10.0.0.42 and so on. It works the same way with IPv6 addresses, except in that case you'd have to import IPv6Network and IPv6Address instead.


Project Euler 23 implementation This code to solve Problem 23 in Project Euler gives the correct answer. But, running the last expression takes around 40 seconds. I welcome advice on how to improve performance here.

(defn sum-div [num]
  (->> (range 2 (+ 1 (int (Math/ceil (Math/sqrt num)))))
       (filter #(= (mod num %) 0))
       (map (fn [x]
              (let [d (/ num x)]
                (if (= x d)
                  x
                  [x d]))))
       flatten
       distinct
       (reduce +)
       (+ 1)))

(def abundants
  (->> (range 12 28123)
       (map #(vector % (sum-div %)))
       (filter #(> (second %) (first %)))
       (map first)
       set))

(defn is-sum-of-two-abundants?
  [num]
  (let [sub-ab (->> abundants
                    (filter #(< % num))
                    set)]
    (some (fn [ab]
            (sub-ab (- num ab)))
          sub-ab)))

(->> (range 28124)
     (filter (complement is-sum-of-two-abundants?))
     (reduce +))

I played around with this for a good half hour without any luck. I was consistently getting ~57 seconds on my crappy M3 laptop.

As a last ditch effort though, I changed

(->> abundants
     (filter #(< % num))
     set)

to

(->> abundants
     (filterv #(< % num)) ; Note the v
     set)

And got it down to 25 seconds! Since you seem to be using a faster computer than me, you'll likely get an even lower time.

Why does this cause such a dramatic change? ¯\_(ツ)_/¯. filterv is strict while filter is lazy, but I have no idea why that makes such a difference here. I would have expected it to be slower since the strict filtering would require a full iteration on its own, then the adding to the set would require another full iteration. Apparently something else is going on. I'll update this answer if I think of what may be happening.

I cleaned up the code a bit too, although none of the other changes made any performance impact. These are mostly just personal stylistic and practice recommendations. See the comments:

(def lower-limit 12) ; Defining these so they aren't magic,
(def upper-limit 28123) ; especially since upper-limit is used in two places

(defn sum-div [num]
  (->> (range 2 (inc (int (Math/ceil (Math/sqrt num)))))
       (filter #(= (mod num %) 0))
       (map (fn [x]
              (let [d (/ num x)]
                (if (= x d)
                  x
                  [x d]))))
       (flatten) ; I personally prefer wrapping for consistency
       (distinct)
       (apply +) ; apply is generally regarded as more idiomatic for summing
       (inc))) ; Use inc instead of (+ 1)

(def abundants
  (->> (range lower-limit upper-limit)
       (map #(vector % (sum-div %)))
       (filter #(> (second %) (first %)))
       (map first)
       (set)))

(defn is-sum-of-two-abundants?
  [num]
  (let [sub-ab (->> abundants
                    (filterv #(< % num))
                    (set))]
    (some (fn [ab]
            (sub-ab (- num ab)))
          sub-ab)))

(time
  (->> (range (inc upper-limit)) ; Use upper limit
       (remove is-sum-of-two-abundants?) ; remove is literally equal to (filter (complement ...))
       (apply +)))

"Elapsed time: 26714.867469 msecs"
=> 4179871


Hackerrank challenge - Dictionaries and Maps This is a solution for the Day 8 hackerrank.com challenge.  The basic idea of the challenge is to create a mapping of names to phone numbers based on the the given input, and later look up the phone numbers for certain names. If the entry exists, print it and its number. If it doesn't, print out "Not found"

Here's my solution:

n = gets.strip.to_i
phonebook = Hash.new

n.times do
    name = gets.strip
    number = gets.strip
    phonebook[name] = number
end

n.times do
    check_name = gets.strip
    if phonebook.has_key?(check_name)
        print "#{check_name}=#{phonebook[check_name]}"
        puts ""
    else
        puts "Not found"
    end
end

I was wondering about more efficient and elegant Ruby solution.  I'm not sure how big sample input was for some of those test cases, but Testcase #1,2,3 take up to 0.4s which seems a little bit too high for me.

You got it (mostly) right, I'd say. I wouldn't worry about runtime on hackerrank. They're not going to throw massive resources at every piece of code someone uploads. You're basically sharing their server with everyone else, so they're throttling things. Plus, there's just basic network lag.

Still, there's a bug:

N is the number of phonebook entries to read, but after that you should read to the end of input. But your code just reads N times in both cases.

Other things:

The Ruby convention is 2 spaces of indentation. Not 4 spaces, not tabs.
Don't use print and then puts with an empty line; just use puts.
Don't use strip unless you want exactly what it does. Most often, you'll see gets.chomp for input handling. #chomp only strips the trailing linebreak, but leaves other whitespace intact.

And you can shorten the if..else to a ternary:

N = gets.chomp.to_i # make this a constant, because why not
phonebook = {}      # shorter way to write Hash.new

N.times do
  name = gets.chomp
  number = gets.chomp
  phonebook[name] = number
end

until STDIN.eof? do # read to end-of-file (i.e. end of input)
  name = gets.chomp
  number = phonebook[name]
  puts number ? "#{name}=#{number}" : "Not found"
end

However, you could also do something else:

N = gets.chomp.to_i
phonebook = Hash.new { "Not found" } # set a default value for undefined keys

N.times do
  name = gets.chomp
  number = gets.chomp
  phonebook[name] = "#{name}=#{number}" # set the entire output string
end

# Will print the name=number string or "Not found"
puts phonebook[gets.chomp] until STDIN.eof?


Optimizing string replacement program that uses recursion I have the following string replacement program for a string of size 256:

I am replacing the string

  "\'"

with the string

  &apos;

Please suggest any modifications if needed or any mistake I have done.

#include <stdio.h>
#include <string.h>

int replace_str(char *SrcStr, char *SubStr, char *RepStr, char *FinalStr )
{

    static char buffer[256]="";
    char *TempStr= NULL;
    char *Tmp2 = malloc(250);
    memset(Tmp2,0,256);

    int ret = 0;

    TempStr = strstr(SrcStr, SubStr);

    if ( TempStr )
    {
        memcpy(Tmp2,TempStr,strlen(TempStr));

        strncpy(buffer, SrcStr, TempStr-SrcStr);

        buffer[TempStr-SrcStr] = '\0';
        sprintf(buffer+(TempStr-SrcStr), "%s%s", RepStr, Tmp2+strlen(SubStr));

        puts(buffer);
        free(Tmp2);
        ret = replace_str(buffer, SubStr, RepStr, FinalStr);
    }
    else
    {

        memcpy(FinalStr, buffer , strlen(buffer));

    }
    return ret;
}

int main(void)
{
    char *str1 = malloc(256);
    memset(str1, 0 ,256);
    memcpy(str1, "pra\'dipta\'kumar\'rout",strlen("pra\'dipta\'kumar\'rout"));

    char str2[10] = "&apos;";
    char str3[3] = "\'" ;
    char buff[256] ={0};

    replace_str(str1, str3, str2,buff);

    puts(buff);

    return 0;
}

Many strange numbers. Why is Tmp2 only 250 long?
Why is buffer static?
Why is Tmp2 not freed in the else branch? Most likely the malloc() should also only happen in the then branch.
The replace_str function should take a length argument for FinalStr, now this might cause a buffer overflow.
Your naming is very strange: Both TmpStr and Tmp2 are temporary strings, but they are named quite different. Better give them proper names.
if (TmpStr) uses implicit boolean conversion of pointers. That's bad practice.
Your variable naming convention is inconsequent, some are UpperCamelCase, some are lower case.
Why is there a call to puts? The function should work on a string, not print on stdout.
Now that I think about it: Using fixed size malloc is generally stupid.
Either make that dependent on the actual needs or just use char[256].
Also you might want to use calloc instead of malloc.
Potential buffer overflow in memcpy(Tmp2,TempStr,strlen(TempStr));
Potential buffer overflow in sprintf(buffer+(TempStr-SrcStr), "%s%s", RepStr, Tmp2+strlen(SubStr));
Also why are you using strncpy and sprintf for the same purpose (string concatenation)? You might want to look at strncat.
Unnecessary memory eating: The temporary strings are not needed for the recursion, better free before.
Similar for buffer.


Find the nearest point of a given set of points Suppose there are a set of given points (represented by x and y two dimensional coordinates), and for any given point A, I want to find the nearest distance point among the given set of point.

My current solution is straightforward: just find min among all distances. The issue of my implementation is, if we want to calculate nearest point among the given set of points for another point B, I need to calculate distance again.

My question is, suppose the given set of points are fixed, is there any way to optimize (e.g. pre-process), so that search nearest point is much faster?

import sys
import random
def distance(p1, p2):
    return (p1[0]-p2[0])**2 + (p1[1]-p2[1])**2
def search_point(points, target_point):
    result = sys.maxint
    nearest_point = -1
    for p in points:
        d = distance(p, target_point)
        if d < result:
            result = d
            nearest_point = p
    return nearest_point

if __name__ == "__main__":
    points = []
    for i in range(10):
        points.append((random.randint(0,20),random.randint(0,20)))
    target_point = (random.randint(0,20), random.randint(0,20))
    print 'result', search_point(points, target_point)
    print 'target_point', target_point
    print 'raw points', points
    print 'distances', [distance(p, target_point) for p in points]

It makes no sense to save the distances to disk (disk i/o is slower than the computation), but it might make sense to "memoize" your function:

distance2_cache = {}
def distance2(p1,p2):
    "Compute the distance squared, using cache."
    try:
        return distance2_cache[(p1,p2)]
    except KeyError:
        distance2_cache[(p1,p2)] = d2 = (p1[0]-p2[0])**2 + (p1[1]-p2[1])**2
        return d2

Note that this might actually be slower than your original function because dictionary lookup may be more expensive than 2 multiplications (which dwarf two subtractions and an addition).

Edit: envelopes

If your pool of points is huge, and you have a stream of point that have to be matched against the pool (i.e., find the pool element which is the closest to input point), you can use lattices/envelopes: suppose your pool points have coordinates from 0 to 1. You can split them into 100 boxes by splitting each coordinate into 10. E.g., the box number 35 will have 0.2<=x<0.3 and 0.4<=y<0.5. Then for each new point you only need to check "very few" boxes (instead of all 100).

Specifically, you need to find the closest point in the box where the point landed, and then compare the distance to that closest point to the distance to the boundary of the box.

If the closest point is closer than the boundary, you are done.

Otherwise you need to check those neighboring boxes that are closer than the closest point (up to 11! if the target point and its closest point are almost in the opposite diagonal nodes). However, this should not happen if the number of points in the box is "large".

A good rule of thumb is that each box should contain as many points as there are boxes, e.g., if you have 10,000 points, there should be 100 boxes.

PS. The further development of this approach results in K-d tree.


Changing element visibility based on multiple checkbox states I am writing a script in JavaScript & HTML and have some optimization problems. In the for, loop I check a variable named visible in every if clause. I think it's meaningless because once I know that the variable value is false there is no need to check the remaining if clauses but skip to the last line. I thought about goto but don't want to use it, and don't know how to fix it.

<script>
function filterServices(){
    var download = document.getElementById('download').checked;
    var free = document.getElementById('free').checked;
    var pay = document.getElementById('pay').checked;
    var audio = document.getElementById('audio').checked;
    var video = document.getElementById('video').checked;
    var offline = document.getElementById('offline').checked;

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

        var serwis = serwisy[i];
        var el = document.getElementById(serwis[0]);
        var visible = true;

        if(download && visible){    visible = serwis[2] == 1 ? true : false;}
        if(audio && visible){   visible = (serwis[3] & (1 << 1)) != 0 ? true : false;}
        if(video && visible){   visible = (serwis[3] & (1 << 2)) != 0 ? true : false;}
        if(offline && visible){ visible = (serwis[3] & (1 << 3)) != 0 ? true : false;}
        if(free && visible){    visible = (serwis[4] & (1 << 1)) != 0 ? true : false;}
        if(pay && visible){ visible = (serwis[4] & (1 << 2)) != 0 ? true : false;}

        el.style.visibility = visible ? "visible" : "hidden";
        }
    }

Here's my attempt to fix it. However, it was not approved because I am allowed to use only logical operators and no reference to the HTML code.

<script>
function filterServices(){
    var download = document.getElementById('download').checked;
    var free = document.getElementById('free').checked;
    var pay = document.getElementById('pay').checked;
    var audio = document.getElementById('audio').checked;
        var video = document.getElementById('video').checked;
    var offline = document.getElementById('offline').checked;

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

        var serwis = serwisy[i];
        var el = document.getElementById(serwis[0]);

        if(download){
            if(serwis[2] != 1){ el.style.visibility = "hidden"; continue;}}
        if(audio){
            if((serwis[3] & (1 << 1)) != 0){    el.style.visibility = "hidden"; continue;}}
        if(video){
            if((serwis[3] & (1 << 2)) != 0){    el.style.visibility = "hidden"; continue;}}
        if(offline){
            if((serwis[3] & (1 << 3)) != 0){    el.style.visibility = "hidden"; continue;}}
        if(free){
            if((serwis[4] & (1 << 1)) != 0){    el.style.visibility = "hidden"; continue;}}
        if(pay){
            if((serwis[4] & (1 << 2)) != 0){    el.style.visibility = "hidden"; continue;}}

        el.style.visibility = "visible" : "hidden";
        }
    }

Does anyone have any idea how to improve this code?

First iteration

You neglected to localize i.

I'd like to rewrite the statements to make it more obvious that the visible flag is something that can be falsified by each if condition.

function filterServices() {
    var download = document.getElementById('download').checked;
    var free = document.getElementById('free').checked;
    var pay = document.getElementById('pay').checked;
    var audio = document.getElementById('audio').checked;
    var video = document.getElementById('video').checked;
    var offline = document.getElementById('offline').checked;

    for (var i = 0; i < serwisy.length; i++) {
        var serwis = serwisy[i];
        var visible = true;

        if (download) { visible = visible && (serwis[2] == 1); }
        if (audio)    { visible = visible && (serwis[3] & (1 << 1)); }
        if (video)    { visible = visible && (serwis[3] & (1 << 2)); }
        if (offline)  { visible = visible && (serwis[3] & (1 << 3)); }
        if (free)     { visible = visible && (serwis[4] & (1 << 1)); }
        if (pay)      { visible = visible && (serwis[4] & (1 << 2)); }

        document.getElementById(serwis[0]).style.visibility = visible ? "visible" : "hidden";
    }
}

Second iteration

Let's make it even more obvious by combining everything into one giant conjunction.

function filterServices() {
    var download = document.getElementById('download').checked;
    var free = document.getElementById('free').checked;
    var pay = document.getElementById('pay').checked;
    var audio = document.getElementById('audio').checked;
    var video = document.getElementById('video').checked;
    var offline = document.getElementById('offline').checked;

    for (var i = 0; i < serwisy.length; i++) {
        var serwis = serwisy[i];
        var visible = (!download || (serwis[2] == 1)))
                   && (!audio    || (serwis[3] & (1 << 1)))
                   && (!video    || (serwis[3] & (1 << 2)))
                   && (!offline  || (serwis[3] & (1 << 3)))
                   && (!free     || (serwis[4] & (1 << 1)))
                   && (!pay      || (serwis[4] & (1 << 2)));

        document.getElementById(serwis[0]).style.visibility = visible ? "visible" : "hidden";
    }
}

Third iteration

But that looks like a mess, especially since the bit operations take a bit of thinking to understand.  Let's add some readability back in by introducing functions to explain the bit operations.

function filterServices() {
    var wantDownload = document.getElementById('download').checked;
    var wantFree     = document.getElementById('free').checked;
    var wantPay      = document.getElementById('pay').checked;
    var wantAudio    = document.getElementById('audio').checked;
    var wantVideo    = document.getElementById('video').checked;
    var wantOffline  = document.getElementById('offline').checked;

    function hasDownload(serwis) { return serwis[2] == 1; }
    function hasAudio(serwis)    { return serwis[3] & (1 << 1); }
    function hasVideo(serwis)    { return serwis[3] & (1 << 2); }
    function hasOffline(serwis)  { return serwis[3] & (1 << 3); }
    function hasFree(serwis)     { return serwis[4] & (1 << 1); }
    function hasPay(serwis)      { return serwis[4] & (1 << 2); }

    for (var i = 0; i < serwisy.length; i++) {
        var serwis = serwisy[i];
        var visible = (!wantDownload || hasDownload(serwis))
                   && (!wantAudio    || hasAudio(serwis))
                   && (!wantVideo    || hasVideo(serwis))
                   && (!wantOffline  || hasOffline(serwis))
                   && (!wantFree     || hasFree(serwis))
                   && (!wantPay      || hasPay(serwis));

        document.getElementById(serwis[0]).style.visibility = visible ? "visible" : "hidden";
    }
}

I have to say, though, that the serwisy data structure doesn't seem well designed.  Consider changing it from a two-dimensional array to an array of objects with reasonable names for the keys, so that you can write things like serwisy[i].elementId instead of serwisy[i][0].


Contact Form Sanitized Inputs I have the following as my Ajax / PHP contact form on a site I am building.  I have sanitized the inputs but just wanted to make sure I havent missed anything...

    // Only process POST reqeusts.
    if ($_SERVER["REQUEST_METHOD"] == "POST") {
        // Get the form fields and remove whitespace.
        $name = strip_tags(trim($_POST["name"]));
        $name = str_replace(array("\r","\n"),array(" "," "),$name);
        $phone = trim($_POST["phone"]);
        $phone = filter_var($phone, FILTER_SANITIZE_NUMBER_INT);
        $email = filter_var($_POST['email'], FILTER_SANITIZE_EMAIL);
        $subject = strip_tags(trim($_POST["subject"]));
        $subject = str_replace(array("\r","\n"),array(" "," "),$subject);
        $message = strip_tags(trim($_POST["message"]));
        $message = str_replace(array("\r","\n"),array(" "," "),$message);

        // Check that data was sent to the mailer.
        if ( empty($name) OR empty($phone)) {
            // Set a 400 (bad request) response code and exit.
            http_response_code(400);
            echo "Oops! There was a problem with your submission. Please complete the form and try again.";
            exit;
        }

        // Set the recipient email address.
        $recipient = "email@domain.com";

        // Set the email subject.
        $emailsubject = "New contact from $name";

        // Build the email content.
        $email_content = "Name: $name\n";
        $email_content .= "Email: $email\n\n";
        $email_content .= "Phone:\n$phone\n";
        $email_content .= "Subject:\n$subject\n";
        $email_content .= "Message:\n$message\n";

        // Build the email headers.
        $email_headers = "From: $name <$email>";

        // Send the email.
        if (mail($recipient, $emailsubject, $email_content, $email_headers)) {
            // Set a 200 (okay) response code.
            http_response_code(200);
            echo 'Thank You! Your message has been sent.';
        } else {
            // Set a 500 (internal server error) response code.
            http_response_code(500);
            echo "Oops! Something went wrong and we couldn't send your message.";
        }

    } else {
        // Not a POST request, set a 403 (forbidden) response code.
        http_response_code(403);
        echo "There was a problem with your submission, please try again.";
    }

The good news is that your script doesn't look exploitable.  In particular, stripping out line break characters protects against header-splitting attacks.  Good job there.

The bad news is that it can mangle the text unnecessarily.  For example, if a user submits the form with the subject line

  Your <blink> tag is annoying

… it will come across as

  Subject: Your  tag is annoying

And for what gain?  strip_tags() is meant as a feeble defense against inappropriate HTML tags, but here you are sending plain text mail — a problem that has absolutely nothing to do with HTML.

It also probably does not make sense to strip all line termination characters from the message.

A more general concern I have is your use of the term sanitize, as it leads to confusion.  I recommend striking that word from your programming vocabulary (even if the PHP documentation uses it), to be replaced by three specific terms:

Canonicalization (or "Normalization"): transforming input from multiple representations of the same data into one preferred form.

For example, if your form accepts a credit card number, then you should strip out all whitespace, because they are not a meaningful part of the data.  Another example would be to lower-case an e-mail address.
Validation: rejecting input that violates your rules.  Validation failure should cause the user to have to resubmit after fixing the errors.

For example, if your form accepts a credit card number, then you should reject any submission that does not contain the right number of digits, the correct leading digits for the accepted card types, and a correct Luhn checksum.

For an e-mail form, your validation rules might require a name to be non-empty, a subject line that is one line of a reasonable length, and a plausible-looking e-mail address.
Escaping: transforming strings so that one kind of string can be safely embedded inside another kind of string without being misinterpreted.

Any time you compose a string that will be interpreted by another computer system, be it an e-mail message, HTML page, or SQL query, assume that you are vulnerable to some kind of injection attack. Every single one of these "human-friendly" languages (as opposed to binary formats such as JPEG images) will have delimiters of special significance. Header-splitting, HTML/JavaScript injection, and SQL injection attacks all have the same root cause: careless string concatenation or interpolation.

Therefore, before concatenating or interpolating strings, stop and think: "What is the appropriate escaping mechanism that I should be using?"  In the case of e-mail headers, RFC 2047 suggests that the relevant escaping mechanism is mb_encode_mimeheader().  Sometimes, the answer is that no escaping is required — the e-mail message body in this example is one of those rare occasions where that's true.

Canonicalization provides user-friendliness.  Validation enforces your business logic.  Escaping, not canonicalization or validation, upholds security.  The term "sanitize" conflates the three mechanisms, leading you do write improperly engineered code.

Suggested solution

function canonicalize(&$params) {
    $params['email'] = filter_var($params['email'], FILTER_SANITIZE_EMAIL);
    $params['phone'] = filter_var($params['phone'], FILTER_SANITIZE_NUMBER_INT);
}

function validate(&$params) {
    $errors = array();
    if (empty($params['name'])) {
        $errors['name'] = 'A name is required';
    }
    if (empty($params['email'])) {
        $errors['email'] = 'Invalid e-mail address';
    }
    if (empty($params['phone'])) {
        $errors['phone'] = 'A phone number is required';
    }
    return $errors;
}

function recipient(&$params) {
    return "email@example.com";
}

function subject(&$params) {
    return mb_encode_mimeheader("New contact from " . $params['name'], 'UTF-8', 'Q');
}

function body(&$params) {
    // Don't bother escaping e-mail body; it's for human consumption.
    return sprintf(
        "Name: %s\n" .
        "Email: %s\n" .
        "Phone: %s\n" .
        "Subject: %s\n" .
        "Message:\n%s\n",
        $params['name'], $params['email'], $params['phone'],
        $params['subject'], $params['message']);
}

function headers(&$params) {
    return sprintf(
        "From: %s <%s>",
        mb_encode_mimeheader($params['name'], 'UTF-8', 'Q'),
        $params['email']
    );
}

// Only process POST reqeusts.
if ($_SERVER["REQUEST_METHOD"] != "POST") {
    // Not a POST request, set a 405 (Method Not Allowed) response code.
    http_response_code(405);
    echo "There was a problem with your submission, please try again.";
} else {
    canonicalize($_POST);

    if (($errors = validate($_POST))) {
        display_form($errors);
    } elseif (!mail(recipient($_POST), subject($_POST), body($_POST), headers($_POST))) {
        // Set a 500 (Internal Server Error) response code.
        http_response_code(500);
        echo "Oops! Something went wrong and we couldn't send your message.";
    } else {
        // Set a 200 (Success) response code.
        http_response_code(200);
        echo 'Thank You! Your message has been sent.';
    }
}

In the rewrite above, I also proposed:

Breaking up the code into functions.
Rearranging the flow such that all of the error handlers come first.  (There are many ways to fail along the one true path to success.)
Changed the HTTP status code for non-POST requests to 405.
A stub (to be implemented) so that the user can get more informative feedback on validation failure.


Declaring JavaScript variable and then overwriting it multiple times I'm wondering if I am doing this in the most efficient way.

So I'm declaring my function el which is equal to a macro (Google Tag Manager), then I reassign or overwrite that previously-defined var which grabs the innerText/textContent of the element.  It then converts the string to lowerCase and then capitalizes the first letter of the string, before finally returning the 'cleaned-up' element.

   function() {
      var el = {{element}}
      el = (el.innerText || el.textContent);
      el = el.toLowerCase();
      el = el.charAt(0).toUpperCase() + el.slice(1);

      return el;

    }

You can using chaining, move the toLowerCase() to simplify a little and return the last statement directly:

function() {
      var el = {{element}};
      el = (el.innerText || el.textContent);
      return el.charAt(0).toUpperCase() + el.slice(1).toLowerCase();
}

I'm personally not a fan of using a single variable for many purposes (I think it interferes with readability and maintainability) so I would probably do this:

function() {
      var el = {{element}};
      var text = (el.innerText || el.textContent);
      return text.charAt(0).toUpperCase() + text.slice(1).toLowerCase();
}


Preparing a YouTube downloader based on Windows form The program this function was written for is a Windows forms program.

This function downloads videos from the internet one after the other. It runs in a separate thread (so the user can drag the form around or whatever), but otherwise the form is disabled, so no more instances can be created without doing so explicitly in the code.

It does not, in fact, download the video. Instead, it sets up the function that will download the video, including all restrictions and such that relate to the user's options before clicking "download".

However, I feel like the function is bloated and clunky, among other concerns. Can someone help me increase its efficiency?

public void SetupDownloadingProcess(int retryCount, Collection<Tuple<string, int, VideoType>> urlList)
    {

        int previouslySelectedIndex = MainForm.SelectedQueueIndex;

        ObservableCollection<Tuple<string, int, VideoType>> finishedUrlList = new ObservableCollection<Tuple<string, int, VideoType>>();

        if (MainForm.UrlListNumberItems > 0)
        {

            MainForm.StartDownloadingSession(true);

            int position = 0;

            for (int count = 0, GlobalVariablesurlListCount = urlList.Count; count < GlobalVariablesurlListCount; count++)
            {

                if(MainForm.CurrentlyDownloading)
                {

                    try
                    {

                        Tuple<string, int, VideoType> url = urlList[count];

                        MainForm.SelectedQueueIndex = position;

                        position++;

                        Tuple<string, int, VideoType> tempTuple = DownloadVideos(url, position);

                        if (tempTuple != null)
                        {

                            finishedUrlList.Add(tempTuple);

                        }

                        Storage.WriteUrlsToFile(finishedUrlList, true);

                    }
                    catch(Exception ex)
                    {

                        var exceptionMessage = ex.Message;

                        if(retryCount <= 3)
                        {

                            MainForm.StatusBar = string.Format(CultureInfo.InstalledUICulture, "URL {0}: {1}. Retrying.... ({2})", count + 1, Trunicate(exceptionMessage, 50), retryCount < 3 ? (retryCount + 1).ToString(CultureInfo.CurrentCulture) : "Final Try");

                            System.Threading.Thread.Sleep(850);

                            SetupDownloadingProcess((retryCount + 1), urlList);

                        }
                        else
                        {

                            finishedUrlList.Clear();

                            MainForm.StatusBar = Trunicate(exceptionMessage, 100);

                        }

                    }

                }

            }

        }

        MainForm.CurrentlyDownloading = false;

        MainForm.StartDownloadingSession(false);

        MainForm.StartDownButtonEnabled = true;

        GlobalVariables.urlList = GetUnfinishedDownloads(finishedUrlList);

        Storage.WriteUrlsToFile(GlobalVariables.urlList, false);

        MainForm.RefreshQueue(previouslySelectedIndex, true);

    }

I prefer not to post all separate references as that would take forever and eat up a lot of room and be boring, but for the truly curious, my entire project can be found here.

I'm still relatively new to C# and coding in general, so please be patient with me and as thorough as possible.

Perhaps it's a copy-paste error, but please reduce the blank lines in your code. Simply removing most of them reduces this example with one third.

Is there a reason why you use WinForms? Quite frankly I feel the technology is outdated; WPF offers you far better ways to do the same thing. For instance, you can use MVVM.

Why use Tuple<string, int, VideoType> when it would be far clearer to create a custom class? The next person to look at this code will first need to figure out what the string and the int represent, whereas if this had been a custom class they'd know the first was the URL and the second... well, I don't know what, but you get my drift.

Trunicate? Do you mean Truncate?

GlobalVariablesurlListCount is a horrible name. A property should never start with "GlobalVariables". Later on there's a GlobalVariables.urlList, so now I'm wondering if GlobalVariablesurlListCount is a typo.

Even if that's the case, having a class called GlobalVariables to pass data around indicates a bad design, and its properties don't seem to follow the capitalization guidelines: urlList should be PascalCase.

Also, urlList shouldn't be called "List", even if it is a List<T>. Just call it Urls.

I realize naming is often hard, but please avoid the temptation to name things "temp", e.g. tempTuple.

Also, why is the method named DownloadVideos when I assume it only downloads a single video?

And why would DownloadVideos return a tuple? Is this the same tuple it receives as a parameter?

(Note that I did not look at your full code at the link you provided.)

The string.Format that assigns a status to MainForm.StatusBar is 200+ characters long and contains a conditional operator. IMHO it is far to messy and far too hard to understand. Is there even a good reason to display "Final Try"? Why not simply have "(1/3)", which would also inform the user how many retries are being attempted?

Your method is called SetupDownloadingProcess but it does far more than that, doesn't it? It interacts with the UI, it logs urls that were downloaded, it even retries the download attempt. There's a lot here that looks like functionality that should be handled differently; it almost looks like you've stumbled upon mimicking a queue or a background worker.


Different methods of API that use similar Services I have three methods in API running similar code snippets. For this reuse the code inserted a switch-case according to the API method.  It's good practice that? I mean, this isn't a high coupling and low cohesion? There is another better way?

API

@ApiOperation(value = "Gerar permalinks de todos os arquivos com validação", response = Permalink.class)
    @RequestMapping(value = "{validacaoId}/permalink/all", method = RequestMethod.POST)
    public Permalink gerarPermalinkComValidacao(@PathVariable("validacaoId") String validacaoId) {
        return permalinkService.createPermalink(validacaoId, "Permalink com validação");
    }

    @ApiOperation(value = "Gerar permalinks de todos os arquivos NOKS com validação", response = Permalink.class)
    @RequestMapping(value = "{validacaoId}/permalink/noks", method = RequestMethod.POST)
    public Permalink gerarPermalinkNoks(@PathVariable("validacaoId") String validacaoId) {
        return permalinkService.createPermalink(validacaoId, "Permalink NOKS com validação");
    }

    @ApiOperation(value = "Gerar permalinks de todos os arquivos sem validação", response = Permalink.class)
    @RequestMapping(value = "{validacaoId}/permalink", method = RequestMethod.POST)
    public Permalink gerarPermalinkSemValidacao(@PathVariable("validacaoId") String validacaoId) {
        return permalinkService.createPermalink(validacaoId, "Permalink sem validação");
    }

Service

public Permalink createPermalink(String validacaoId, String tipo) {
        Validacao validacao = validacaoDAO.findById(validacaoId);
        Permalink permalink = new Permalink().setId(Util.getMd5Time(validacaoId));
        if (validacao == null) {
            throw new NotFoundException("Erro ao gerar permalink: ID da validação inválido.");
        }
        permalinkDAO.createPermalinkDirectory(validacao.getId());
        BasicDBList pecas;
        switch (tipo) {
            case "Permalink com validação":
                pecas = getAllFilesWithValidation(validacao.getId(), permalink.getId());
                break;
            case "Permalink NOKS com validação":
                pecas = noksWithValidation(validacao.getId());
                break;
            case "Permalink sem validação":
                pecas = getAllFiles(validacao.getId(), permalink.getId());
                break;
            default:
                throw new IllegalArgumentException("Tipo de permalink inválido.");
        }
        if (!pecas.isEmpty()) {
            permalink.setEmail(validacao.getEmail())
                    .setLogo(permalinkDAO.getLogo(validacao.getInstancia()).getString("logo", ""))
                    .setDataCriacao(DateTime.nowISODate())
                    .setTipo(tipo)
                    .setValidacaoId(validacao.getId())
                    .setInstancia(validacao.getInstancia())
                    .setPecas(pecas);
            permalinkDAO.create(permalink);
            return permalink;
        }
        throw new InternalServerErrorException("Erro ao obter peças para permalink.");
    }

Use English for your variable names, request routes, and Exception messages. Imagine a remote developer joins your company (very common these days), they wouldn't be able to understand the stack traces.
validate your parameters before creating unnecessary objects

Validacao validacao = validacaoDAO.findById(validacaoId);
if (validacao == null) {
    throw new NotFoundException("Validacao cannot be null");
}
Permalink permalink = new Permalink().setId(Util.getMd5Time(validacaoId));

The switch statement however is fine.


Architecture-dependent to DataView I have this code here which varies depending on the system architecture (typed arrays depends on endianness). I'm trying to get a 32bit ARGB data array. I was trying to learn DataView, which has a nice way of dealing with "endian-ness".

Can you please help to revise this to use DataView, while working basically as close to the logic as the code below currently follows?

Documentation on DataView: MDN :: DataView

// Getting pixels as a byte (uint8) array
var imageData = ctx.getImageData(0, 0, can.width, can.height);
var pixels8BitsRGBA = imageData.data;

// Reverting bytes from RGBA to ARGB
var pixels8BitsARGB = new Uint8Array(pixels8BitsRGBA.length + 8); // +8 bytes for the two leading 32 bytes integers
for(var i = 0 ; i < pixels8BitsRGBA.length ; i += 4) {
    pixels8BitsARGB[i+8 ] = pixels8BitsRGBA[i+3];
    pixels8BitsARGB[i+9 ] = pixels8BitsRGBA[i  ];
    pixels8BitsARGB[i+10] = pixels8BitsRGBA[i+1];
    pixels8BitsARGB[i+11] = pixels8BitsRGBA[i+2];
}

// Converting array buffer to a uint32 one, and adding leading width and height
var pixelsAs32Bits = new Uint32Array(pixels8BitsARGB.buffer);
pixelsAs32Bits[0] = can.width;
pixelsAs32Bits[1] = can.height;

console.log(pixelsAs32Bits);

Using DataView is pretty much the same thing. Probably slower though. But it allows you to see that you only have to worry about endianness when dealing with values using more than one byte, like you do when storing width and height using 4 unsigned bytes each.

// Getting pixels as a byte (uint8) array
var imageData = ctx.getImageData(0, 0, can.width, can.height);
var pixels8BitsRGBA = imageData.data;

var i;
var pixelsAs32Bits = new Uint32Array(pixels8BitsRGBA.length + 8); // +8 bytes for the two leading UInt32
var pixelsARGB = new DataView(pixelsAs32Bits.buffer);

// Reverting bytes from RGBA to ARGB
pixelsARGB.setUint32(0, can.width, true); // little endian
pixelsARGB.setUint32(4, can.height, true);  // little endian
for(i = 0; i < pixels8BitsRGBA.length ; i += 4) {
    pixelsARGB.setUint8(i + 8, pixels8BitsRGBA[i + 3]);
    pixelsARGB.setUint8(i + 9, pixels8BitsRGBA[i]);
    pixelsARGB.setUint8(i + 10, pixels8BitsRGBA[i + 1]);
    pixelsARGB.setUint8(i + 11, pixels8BitsRGBA[i + 2]);
}

console.log(pixelsAs32Bits);


Calculating the ranks of classmates' exam grades The questions from hackerearth:

  Geeko is in worry now because an exam is coming up and he has to know
  what rank he can get on exams. So he goes back into the the school records
  and finds the amazing pattern.

  He finds that if a student is having a current rank \$n\$, then his rank on
  the final exam will be the count positive numbers between in the range
  \$[1,n]\$ which are relatively prime to \$n\$.

  As being a geek, he became curious now and wants to calculate the rank of
  all his classmates on the final exam.  But he finds this task a bit hard,
  so he asks you programmers to solve this task for him.

  Input: The first line of each test file contains a integer \$t\$ denoting
  the number of test case.  Each test case contains a numbers \$n\$
  representing the current rank of each student

  Output: for each test case output single integer the rank of student
  in final exam in new line.

  Constraints:

  \$1 <= t <= 2000\$

  \$1 <= n < 10^6\$

My solution:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.ArrayList;
import java.util.Scanner;

class relative {

    public static void main(String[] args) throws IOException {
        BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
        int t = Integer.parseInt(br.readLine());
        ArrayList<Integer> input = new ArrayList<Integer>();

        ArrayList<Integer> result = new ArrayList<Integer>();
        // int input[] = new int[t];
        // int result[] = new int[t];
        for (int i = 0; i < t; i++) {
            int count = 1;
            int in;
            input.add(in = Integer.parseInt(br.readLine()));
            for (int j = 2; j < in; j++) {
                if (in % j == 0)
                    continue;
                else if (checkRelativePrime(j, in))
                    count++;
            }
            result.add(count);
        }
        for (int i : result)
            System.out.println(i);
    }

    static boolean checkRelativePrime(int a, int b) {
        int i = 2;
        while (a > 1) {
            if (a % i == 0) {
                {
                    if (b % i == 0)
                        return false;
                }
                a = a / i;
            } else
                i++;
        }

        return true;
    }
}

in your checkRelativePrime method you have

static boolean checkRelativePrime(int a, int b) {
    int i = 2;
    while (a > 1) {
        if (a % i == 0) {
            {
                if (b % i == 0)
                    return false;
            }
            a = a / i;
        } else
            i++;
    }

    return true;
}

you have an extra set of brackets
you have a statement in purgatory.

a = a / i;

it sits in-between the ending bracket of your if statement and the else statement that is supposed to be attached to the if statement
if you use brackets for one part of an if statement, you should use them for the entire if statement. I recommend using brackets even for one-liners no matter what, it's cleaner and leaves less room for mistakes.

I think you wanted to keep that if statement separate from the declaration to the a variable, but you don't need to do that(especially if you are using brackets properly)

like this

while (a > 1) {
    if (a % i == 0) {
        if (b % i == 0) {
            return false;
        }
        a = a / i;
    } else {
        i++;
    }
}

if you enter that if statement and return false then a = a /i; will never happen and your boolean method will return false


Find the kth-to-last element of a singly linked list I wrote this code for finding the \$k\$th-to-last element in a singly linked list.

class Node:
  def __init__(self, v = None, next = None):
    self.v = v
    self.next = next

a = Node("Australian Sheperd")
b = Node("Beagle")
c = Node("Cairne Terrier")
d = Node("Dobermann")
e = Node("English Mastiff")

a.next = b
b.next = c
c.next = d
d.next = e

count = 1

def kthToLastNode(k, root):
  global count
  if root.next:
    count = kthToLastNode(k, root.next)
  if count == k:
    print(root.v)
  return count + 1

For example:

>>> kthToLastNode(3,a)
Cairne Terrier
6

When called on a list with \$n\$ nodes, the code goes all the way through the list, recursing \$n\$ times, using \$\Theta(n)\$ stack space, and returning \$n\$ times. Is there a way to improve the space performance of the function?

1. Review

There is no docstring. What does kthToLastNode do? What does it return? A docstring would explain that you have to pass k=1 to get the last item, k=2 to get the second-to-last item, etc.
It would be more useful, I think, if kthToLastNode returned the \$k\$th-to-last node instead of printing the node's value and returning the length of the linked list.
If you pass a value for k that's out of range (for example, longer than the length of the linked list) then there should be some error handling. For example, you might raise an exception.
Using a global count means that kthToLastNode can only be called once: if you call it again count has the wrong value and the function prints nothing and returns the wrong result.

2. Alternative approaches

Instead of recursing, iterate! Here are two implementation ideas.

Iterate over the list twice, first to find its length, and then to find the \$k\$th-to-last node. It would be worth making a separate function to find the length: this will make the code clearer, and we might have other places where we need to find the length, so if we make a function we'll be able to reuse it.

def linked_list_length(node):
    "Return the length of the linked list starting at node."
    length = 0
    while node:
        length += 1
        node = node.next
    return length

def kth_to_last_node(node, k):
    """Return the kth-to-last node of the linked list starting at node.
    Pass k=1 for the last node, k=2 for the next-to-last node, and so
    on. Raise IndexError if k is out of range.

    """
    length = linked_list_length(node)
    if k < 1 or k > length:
        raise IndexError("index out of range")
    for _ in range(length - k):
        node = node.next
    return node

This uses \$O(1)\$ space but has to traverse the list twice.
Append the nodes one by one to a queue of length \$k\$, so that when we reach the end of the list, the first element of the queue will be the \$k\$th-to-last node. We can implement the queue using collections.deque.

from collections import deque

def kth_to_last_node(node, k):
    """Return the kth-to-last node of the linked list starting at node.
    Pass k=1 for the last node, k=2 for the next-to-last node, and so
    on. Raise IndexError if k is out of range.

    """
    queue = deque(maxlen=k)
    while node:
        queue.append(node)
        node = node.next
    if k < 1 or len(queue) != k:
        raise IndexError("index out of range")
    return queue[0]

This traverses the list once but uses \$O(k)\$ space.

3. General discussion

Recursion and iteration are very similar: anything you can do with one, you can do with the other. In some cases the code is simpler and clearer if you use recursion, in other cases if you use iteration, and sometimes it doesn't make much difference. So it's hard to give general advice: there's no real substitute for trying both and seeing which you like better! For example, here's a case where recursion was nicer.

In the case of singly linked lists, it's worth pointing out that we don't have much need for this data structure in Python, because the built-in list data type is nearly always better than a linked list. You can get the \$k\$th-to-last element of the list a by writing a[-k], and this takes \$O(1)\$ time and \$O(1)\$ space.

The cases in which we do want to use linked lists in Python are those in which we need to efficiently insert new elements into the middle of a list. See this answer for a rare example. But in these cases we probably don't need to find the \$k\$th-to-last element.


Reporting sums of shopping carts I have an object (ShoppingCart) that has a list of CartItems, which contains the related Product and the quantity bought.

public class ShoppingCart extends BaseEntity {

    @OneToMany(mappedBy = "shoppingCart", cascade = CascadeType.ALL, orphanRemoval = true)
    private List<CartItem> cartItems = new ArrayList<>();

    @DateTimeFormat(pattern = "dd/MM/yyyy hh:MM:ss")
    private LocalDateTime dateTime;

    @Enumerated
    private PaymentMethod paymentMethod = PaymentMethod.CASH;

public class CartItem extends BaseEntity {

    @ManyToOne
    private Product product;

    @ManyToOne(fetch = FetchType.LAZY)
    @JoinColumn
    private ShoppingCart shoppingCart;

public class Product extends BaseEntity {

    @NotEmpty
    private String name;

    private double price;

    private int quantity;

I have a view that sums all shopping carts, and I don't know if I should store that sum inside ShoppingCart entity, or iterate to find the sum.
Here's the code, right now I'm iterating.

@GetMapping("/")
    public String getProduct(@ModelAttribute("reportDto") ReportDto reportDto, Model model) {

        double total = 0, cash = 0, credit = 0, debit = 0;
        int quantity;
        List<ShoppingCart> results;
        if (reportDto.getBeginDate() == null || reportDto.getEndDate() == null) {
            results = shoppingCartService.findAll();
        } else {
            results = shoppingCartService.findByDateTimeBetween(reportDto.getBeginDate(), reportDto.getEndDate());
        }
        total = results.stream()
                .mapToDouble(shoppingCart -> shoppingCart.getCartItems().stream()
                        .mapToDouble(cartItem -> cartItem.getProduct().getPrice() * cartItem.getQuantity()).sum())
                .sum();
        quantity = results.stream().mapToInt(
                shoppingCart -> shoppingCart.getCartItems().stream().mapToInt(cartItem -> cartItem.getQuantity()).sum())
                .sum();

        cash = results.stream().filter(shoppingCart -> shoppingCart.getPaymentMethod().isCash())
                .mapToDouble(shoppingCart -> shoppingCart.getCartItems().stream()
                        .mapToDouble(cartItem -> cartItem.getProduct().getPrice() * cartItem.getQuantity()).sum())
                .sum();

        credit = results.stream().filter(shoppingCart -> shoppingCart.getPaymentMethod().isCredit())
                .mapToDouble(shoppingCart -> shoppingCart.getCartItems().stream()
                        .mapToDouble(cartItem -> cartItem.getProduct().getPrice() * cartItem.getQuantity()).sum())
                .sum();

        debit = results.stream().filter(shoppingCart -> shoppingCart.getPaymentMethod().isDebit())
                .mapToDouble(shoppingCart -> shoppingCart.getCartItems().stream()
                        .mapToDouble(cartItem -> cartItem.getProduct().getPrice() * cartItem.getQuantity()).sum())
                .sum();
        model.addAttribute("quantity", quantity);
        model.addAttribute("cash", cash);
        model.addAttribute("credit", credit);
        model.addAttribute("debit", debit);
        model.addAttribute("total", total);
        return "reports/reports";
    }

Also, any feedback regarding the code will be appreciated!

First of all i think making a view in your database which sums your needs will be better because generally sql engine sum operator's performance is way better than lopping values in programming languages.

Second if we go through your code:
You have done mapping for all objects one by one. Java stream mapping is similar to for loops in compile time. So here what you make is similar to creating for loop foreach object which is bad for performance.
To be performance wise you can create one for loop and generate your sum values in it.

Note: BigDecimal is the preferred way to store double money values in Java