Grouping consecutive numbers into ranges in Python 3.2; third version (accepts page numbers as an unsorted set) def range_find(pageset):

    pagerangelist=[]

    for page in list(pageset):

        if page in pageset and page-1 in pageset:pagerangelist[-1].append(page)
        elif page in pageset and not (page-1 in pageset):pagerangelist.append([page])

    pagerangestringlist=[]

    for pagerange in pagerangelist:

        if len(pagerange)==1:pagerangestringlist.append(str(pagerange[0]))
        else: pagerangestringlist.append(str(pagerange[0])+'-'+str(pagerange[-1]))

    return ','.join(pagerangestringlist)

def range_find(pageset):

    pagerangelist=[]

    for page in list(pageset):

There is no point in listifying the set just to iterate over it. I think you meant for page in sorted(pageset) to get access to the page in sorted order.

        if page in pageset and page-1 in pageset:pagerangelist[-1].append(page)
        elif page in pageset and not (page-1 in pageset):pagerangelist.append([page])

You still make it hard to read by putting those on the same line.

    pagerangestringlist=[]

If you've got multiple words I suggest under scores. I also suggest dropping types like list/set from your names. I'd call this page_range_strings.

    for pagerange in pagerangelist:

        if len(pagerange)==1:pagerangestringlist.append(str(pagerange[0]))
        else: pagerangestringlist.append(str(pagerange[0])+'-'+str(pagerange[-1]))

I'm going to recommend using string formating. It'll be easier to read then concatenating strings.

    return ','.join(pagerangestringlist)


Generate power sets given a set of elements in java For an array of length n, the possible number of subsets is Math.pow(2, n).  for each value between 0 and Math.pow(2, n), see the index at which the bit is set, and include the corresponding number in the given input to the current subset.

public List<List<Integer>> subsets(int[] nums){
        int numOfSubSets = 1 << nums.length;
        List<List<Integer>> subsets = new ArrayList<>();
        int index = -1;
        subsets.add(new ArrayList<>());
        List<Integer> tempSubset;
        int current = 0;
        for(int i = 1 ; i < numOfSubSets ; i++ ){
            index = -1;
            tempSubset = new ArrayList<>();
            current = i;
            while(current != 0){
                index++;
                if((current & 1) != 0){
                    tempSubset.add(nums[index]);
                }
                current = current >> 1;
            }
            subsets.add(tempSubset);
        }
        return subsets;
    }

Your code contains some unnecessary assignments, where the value assigned to a variable is never used before it is being overwritten. These are the first assignment of index in the line int index = -1; and the first assignment of current in the line int current = 0;. With the assignments gone, only the declarations int index and int current are left, and since it generally makes code easier to understand to reduce the scope of variables to the smallest possible extent and to keep them out of places where they are not needed, I suggest you move the declarations of these variables inside the for loop, since they are not needed outside it and don't need to keep their value from one loop iteration to the next, as they are assigned a new value at the beginning of each loop iteration. The same goes for the variable tempSubset.
You are treating the empty subset as a special case by adding it manually before the for loop and then initializing i to 1 instead of 0. However, this is not necessary. The code would work just as well if you removed the line subsets.add(new ArrayList<>()); and initialized i to 0. This would not only save one line of code, but with one algorithm that works for all cases, and not only for all cases except one, the code will be easier to understand.
A for loop might be more fitting for what your are doing with a while loop. Instead of this:

index = -1;
tempSubset = new ArrayList<>();
current = i;
while(current != 0){
    index++;
    if((current & 1) != 0){
        tempSubset.add(nums[index]);
    }
    current = current >> 1;
}
subsets.add(tempSubset);

You could do this:

tempSubset = new ArrayList<>();
for (int index = 0; (i & 0xFFFFFFFF << index) != 0; index++) {
    if ((i & 1 << index) != 0) {
        tempSubset.add(nums[index]);
    }
}

After these changes, your code will look like this:

public List<List<Integer>> subsets(int[] nums){
    int numOfSubSets = 1 << nums.length;
    List<List<Integer>> subsets = new ArrayList<>();

    for(int i = 0; i < numOfSubSets; i++){
        List<Integer> tempSubset = new ArrayList<>();
        for (int index = 0; (i & 0xFFFFFFFF << index) != 0; index++) {
            if ((i & 1 << index) != 0) {
                tempSubset.add(nums[index]);
            }
        }
        subsets.add(tempSubset);
    }
    return subsets;
}


Program to insert, search for and delete an element from an array I'd like this code to be improved.

package com.array.demo;

import java.util.Arrays;

public class ArraysDemo {

public static void main(String[] args) {
    // TODO Auto-generated method stub

    // array initialization
    int[] arr = { 5, 10, 12, 22, 4 };

    int size = arr.length;

    int i,pos; // loop counter

    System.out.println(arr); // // prints something like '[I@3343c8b3'

    System.out.println("Elements in the array" + Arrays.toString(arr));

    System.out.println(" The size of the array is  " + size);

    // array-search
    int searchKey = 11;

    for ( i = 0; i < size; i++) {
        if (arr[i] == searchKey)
            break;
    }

    if(i==size)
        System.out.println(" Element not found");
    else
        System.out.println("Element found in the position " +i);

    int elemDel=5;

    for ( pos = 0; pos < size; pos++) {
        if (arr[pos] == elemDel)
            break;
    }

    if(pos==size)
        System.out.println("Element not found");
    else
    {
        while(pos<size-1){
            arr[pos] = arr[pos+1];
            pos++;
        }
        size--;
    }

// display the element after deletion
for(i=0;i<size;i++){
    System.out.print(arr[i] + " ");
}
    //System.out.println("Elements in the array" + Arrays.toString(arr));

}

}

The easy stuff first...

You should format your code nicer. It makes it easier to read for everyone who uses the same standard formatting style. Use your IDE's reformatting function (Control + Shift + f in Eclipse, Alt + Control + l in IntelliJ) to reformat before posting on forums.

Remove pointless comments. The auto-generated TODO has no place in working code. And this one for example is really stating the obvious:

// array initialization
int[] arr = { 5, 10, 12, 22, 4 };

In general, try to not write comments. If your code is not self-explanatory without comments, try to rewrite it. A common technique is to extract a few lines of code into a method, and the name of the method will explain what those lines do instead of a comment.

You probably you know you're reinventing the wheel, but just for the record, there is an ArrayList<T> class that can do all the things you're doing in your code.

Object-oriented design

The code in the main method is doing all kinds of things, all of them centered around an array, and various manipulations on it, such as:

find the index of an element
remove an element, if exists
print elements

It would make sense to encapsulate all these by a class to keep track of the array and its size. Maybe something like this:

public class ArraysDemo {

    private final int[] values;
    private int size;

    public ArraysDemo(int... values) {
        this.values = values.clone();
        this.size = values.length;
    }

    int indexOf(int value) {
        for (int i = 0; i < size; i++) {
            if (values[i] == value) {
                return i;
            }
        }
        return -1;
    }

    boolean remove(int value) {
        for (int i = 0; i < size; i++) {
            if (values[i] == value) {
                --size;
                for (; i < size; ++i) {
                    values[i] = values[i + 1];
                }
                return true;
            }
        }
        return false;
    }

    void print() {
        System.out.print("Elements in the array: ");
        for (int i = 0; i < size; ++i) {
            System.out.print(values[i] + " ");
        }
        System.out.println();
    }
}

I tried to mimic the methods in ArrayList<T>. This is a good idea to do when you're reinventing the wheel. For example indexOf to find the index of an element, or return -1 if not found. A remove method that returns true if something was removed.

Using this class, you can rewrite the main method like this:

ArraysDemo arr = new ArraysDemo(5, 10, 12, 22, 4);
arr.print();

int index = arr.indexOf(11);
if (index < 0) {
    System.out.println("Element not found");
} else {
    System.out.println("Element found in the position " + index);
}

if (!arr.remove(5)) {
    System.out.println("Element not found");
}
arr.print();

As a final step, the class can be generalized to work with any type, not only integers. When you do that, remember to change all the conditions with == to use the .equals method instead, for example:

public class ArraysDemo<T> {

    private final T[] values;
    private int size;

    // ...

    int indexOf(int value) {
        for (int i = 0; i < size; i++) {
            if (values[i].equals(value)) {
                return i;
            }
        }
        return -1;
    }
}


Algorithm for computing LCM (least common multiple) I'm trying to implement this algorithm for computing LCM (from the cut-the-knot website). This is my Java code:

public class LcmAlgorithm {

public static void main(String[] args) {
    long[] input = { 28, 50, 14 };
    System.out.println(lcm(input));
}

private static long lcm(long[] inputs) {
    long[] compute = inputs.clone();
    while (!allEqual(compute)) {
        int minIx = minValueIndex(compute);
        compute[minIx] += inputs[minIx];
    }
    return compute[0];
}

private static boolean allEqual(long[] arr) {
    for (int i = 0; i < arr.length - 1; i++) {
        if (arr[i] != arr[i + 1])
            return false;
    }
    return true;
}

private static int minValueIndex(long[] arr) {
    long min = java.lang.Long.MAX_VALUE;
    int ix = 0;

    for (int i = 0; i < arr.length; i++) {
        if (arr[i] < min) {
            min = arr[i];
            ix = i;
        }
    }
    return ix;
}
}

It works, but I'm pretty new to Java and I'm not sure my code is optimal (or maybe it's embarrassing...?). I'd like to hear what do you think of my code and how I can improve it. (BTW, I have a background in .NET programming.)

Thanks.

I've included my review comments in the revised code below as program comments,
unmistakable because there weren't any comments in the OP.
Hmm. That's not a good sign. Though, I can't say that I've helped that much. Backward-looking reviewer comments make a poor substitute for useful forward-looking developer comments.

public class LcmAlgorithm {

    /* Making "main" the only public member clearly limits this as a
       stand-alone "toy", and that's OK -- it is what it is.

       Yet, you might want to get in the habit of deciding up front which
       functions are purposely permanently private and which you'd
       consider exposing as public in a "real world" implementation.
       This makes your intent clearer to a reviewer (or your future self).

       On the other hand, arguably, you should only expose something as public
       when/if you have to
       (because you have an actual external caller -- at least a unit test).
       With that in mind, a good short-term compromise might be to comment
       your intent on the private declaration with e.g. '// candidate for public'.
    */

    public static void main(String[] args) {
        long[] input = { 28, 50, 14 };
        // Use the Object, Luke.
        LcmAlgorithm lcmAlgorithm = new LcmAlgorithm(input);
        System.out.println(lcmAlgorithm.lcm());
    }

    // Was named "inputs" but its role is more important than its origin.
    // Note: some people prefer plurals for collection names.
    // Whatever you choose, be consistent
    // -- not "input" here and "inputs" there.
    private final long[] increment;

    // Was "compute" -- don't verb your data names
    // ("sum" sits on the verb/noun fence).
    private long[] sum;

    private LcmAlgorithm(long[] input) { // candidate for public
        assert input.length > 0;
        // Initializing 'increment' here
        // (vs. with an argument to lcm) allows declaration as final
        increment = input.clone();
        // Initializing both 'increment' and initial 'sum' here
        // saves re-computes if lcm is called more than once.
        sum = input.clone();
    }

    // The name LcmAlgorithm.lcm seems a little redundant
    // -- some alternatives are "compute", stressing the action
    // (perhaps too much?) or "result".
    // "result" may be especially apropos since if you call the
    // function a second time on the same object, you get the
    // result again with very little computation.
    private long lcm() { // candidate for public
        while (!allSumsEqual()) {
            int minIx = minSumIndex();
            sum[minIx] += increment[minIx];
        }
        return sum[0];
    }

    // The next two methods have been slightly simplified, including
    // being made instance methods to get implicit
    // access to the array member, and better leveraging
    // the asserted first element.
    private boolean allSumsEqual() {
        final long firstSum = sum[0];
        for (int i = 1; i < sum.length; i++) {
            if (firstSum != sum[i])
                return false;
        }
        return true;
    }

    private int minSumIndex() {
        long min = sum[0];
        int minIx = 0;

        for (int i = 1; i < sum.length; i++) {
            if (sum[i] < min) {
                min = sum[i];
                minIx = i;
            }
        }
        return minIx;
    }
}


Using promise all with push array The code's purpose is to call to some function async given some condition was true. I've using the following code which is working as expected. Is there a way to write it better, maybe by not using the promise push array?

var _run = (req, res, oModule, oConfig, urlPath,action) => {

    var aPromises = [];
    //requested action to call

    for (var provider in oConfig.providers[0]) {
        if ((oConfig.providers[0][provider].val === action))
        {
            var fnName = oConfig.providers[0][provider].function;
            //Run on provided actions
            if (typeof oModule[oConfig.providers[0][provider].function] === "function") {
                try {
                    logger.info(`Function ${fnName} is called `);
                    aPromises.push(oModule[fnName](req, res));
                }
                catch (err) {
                    logger.error(`Error returned  ${err}`);
                }
            }
        }
    }
    return Promise.all(aPromises);
};

update:

This is the value of config object and in the code I need to call to the function according to the path I got...

{
  "providers": [
    {
      "run": {
        "path": "command1",
        "function": "fn1",
      },
      "save": {
        "path": "test2",
        "function": "fn2",
      }
    }
  ]
}

Instead of using the for loop with if flow control, you could use the arrays functions to convert all more functional.

As your code looks broken I wrote my refactoring in the following test:

'use strict';

// This is just to run the test
const request = {}
const response = {}

// This is a trivial oModule object
const M = {
    fn1: function() {
        return new Promise(() => console.log('fn1'))
    },
    fn2: function() {
        return new Promise(() => console.log('fn2'))
    }
}

// This is the oConfig object as you post in your OP
const C = {
    providers: [
    {
      "run": {
        "path": "command1",
        "function": "fn1",
      },
      "save": {
        "path": "test2",
        "function": "fn2",
      }
    }
  ]
}

// The proposed refactoring
const _run = (req, res, oModule, oConfig, urlPath, action) => {
    let aPromises = Object.keys(oConfig.providers[0])
        .filter(providerName => (providerName == action))
        .map(providerName => {
            let provider = oConfig.providers[0][providerName]
            return oModule[provider.function]
        })
        .filter(func => (typeof func === 'function'))
        .map(func => func(req, res))

    return Promise.all(aPromises)
}

// This run the test to
_run(request, response, M, C, '', 'run')

I used 2 different filter/map run to check the 2 checks on the 2 different objects oConfig and oModule.

I tested this with nodejs v4.4.4 on windows and it works without errors.

It print out:

fn1

I removed the var and use const or let.

It should be the same, not sure if it is more readable.

You never use urlPath so you can remove from arguments.

I didn't put any ; as they are not mandatory in javascript, but if you prefere to use feel free to put at the end of each expression.

Also, I hope that oModule[provider.function](req, res) returns a Promise or your code is blocking, but maybe is so.


Hangman implementation for teaching a Python course I am teaching a Python crash course and thought a Hangman rendition would be fruitful.

I have an end product that I'll work towards with the hour that I have for the course and I was wondering what could be refactored or implemented to help the exchange of information from myself, intermediate level, to absolute beginners.

from random import choice
import os

def clear():
    os.system('clear')

def play_again():
    play_again = input("\nPlay again? Y/n ").lower()

    if play_again != 'n':
        clear()
        game()

    else:
        print("\n\n\n\nCatch ya later!\n\n\n\n")

def get_secret_word():
    words = ['angry', 'beautiful', 'brave', 'careful', 'charismatic', 'clever',
             'creative', 'cute', 'dangerous', 'exciting', 'famous', 'friendly',
             'happy', 'interesting', 'lucky', 'profound', 'popular', 'rich',
             'thin', 'young']

    secret_word = choice(words)
    return secret_word

def get_guess(bad_guesses, good_guesses, secret_word):
    while True:
        guess = input("Guess a letter: ").lower()

        if guess == '':
            print("What was that?")
            board(bad_guesses, good_guesses, secret_word)

        elif len(guess) != 1:
            input("You can only guess a single letter!")
            board(bad_guesses, good_guesses, secret_word)

        elif guess in bad_guesses or guess in good_guesses:
            input("You've already guessed that letter!")
            board(bad_guesses, good_guesses, secret_word)

        elif not guess.isalpha():
            input("You can only guess LETTERS!")
            board(bad_guesses, good_guesses, secret_word)

        else:
            return guess

def board(bad_guesses, good_guesses, secret_word):
    clear()
    print('** Strikes: {}/8 **'.format(len(bad_guesses)))
    print('')

    for guess in bad_guesses:
        print(guess, end=" ")
    print('\n\n')

    for guess in secret_word:
        if guess in good_guesses:
            print(guess, end=" ")
        else:
            print('_ ', end=" ")

    print('')

def game():
    secret_word = get_secret_word()
    good_guesses = []
    bad_guesses = []
    game_done = False

    while not game_done:
        board(bad_guesses, good_guesses, secret_word)
        guess = get_guess(bad_guesses, good_guesses, secret_word)
        if guess in secret_word:
            good_guesses.append(guess)
            found = True
            for guess in secret_word:
                if guess not in good_guesses:
                    found = False
            if found:
                print("\nYou win!\n")
                print("The secret word was {}.".format(secret_word.upper()))
                game_done = True

        elif guess not in secret_word:
            bad_guesses.append(guess)
            if len(bad_guesses) == 8:
                clear()
                print("\n **  ENGHH!  ** ")
                print("\nStrike ! You lost!")
                print("\nThe secret word was {}".format(secret_word.upper()))
                game_done = True

        if game_done:
            play_again()

        else:
            board(bad_guesses, good_guesses, secret_word)

game()

I could come up with a laundry list of improvements.  However, I would like to focus on just one issue, which makes your code an unforgivably bad example for your students: you are misusing functions as goto labels.

Note that game() can call play_again(), which in turn may call game().  This mutual recursion is inappropriate.  If you play many games, then hit CtrlC, you will see that the deepening stack is keeping track of all the previous calls, uselessly preparing for resuming execution after a return.

If you want to do something repeatedly in Python, use a loop.  A reasonable outline would look like this:

def play_again():
    """
    Ask the player whether to play again.
    """
    return 'n' != input("\nPlay again? Y/n ").lower()

def game():
    """
    Play a single game of Hangman.  Return True if the player wins,
    or False if the player loses.
    """
    secret_word = get_secret_word()
    good_guesses = []
    bad_guesses = []

    while True:
        board(bad_guesses, good_guesses, secret_word)
        guess = get_guess(bad_guesses, good_guesses, secret_word)
        if guess in secret_word:
            good_guesses.append(guess)
            if all(letter in good_guesses for letter in secret_word):
                print("\nYou win!\n")
                print("The secret word was {}.".format(secret_word.upper()))
                return True
        else:
            bad_guesses.append(guess)
            if len(bad_guesses) == 8:
                clear()
                print("\n **  ENGHH!  ** ")
                print("\nStrike ! You lost!")
                print("\nThe secret word was {}".format(secret_word.upper()))
                return False

def main():
    """
    Play games of Hangman until the user decides to quit.
    """
    while True:
        game()
        if not play_again():
            print("\n\n\n\nCatch ya later!\n\n\n\n")
            break
        clear()

Additional remarks:

Flag variables, like game_done and found, are almost always a bad idea.  Here, game_done can be eliminated by using proper looping technique, and found can be determined using the all() function.
if guess not in secret_word is redundant.  You can just say else.

Oh, and one more thing, since you should be setting a good example for your students: write a docstring for every function.


Count possible paths through a maze You have to find a path through which the rat move from the starting
  position (0,0) to the final position where cheese is (n,n). List the
  total no of possible paths which the rat can take to reach the cheese.

Sample Input:

7

0 0 1 0 0 1 0

1 0 1 1 0 0 0

0 0 0 0 1 0 1

1 0 1 0 0 0 0

1 0 1 1 0 1 0

1 0 0 0 0 1 0

1 1 1 1 0 0 0

Sample Output:

4

My code:

import java.util.*;

class maze

{

    static int n;
    static int[][] a;
    static int path;

    public static void main(String[] ar)
    {
        Scanner sc = new Scanner(System.in);
        n = sc.nextInt();
        a = new int[n][n];
        for(int i = 0; i < n; i++)
        {
            for(int j = 0; j < n; j++)
            {
                a[i][j] = sc.nextInt();
            }
        }
        search(0,0);
        System.out.println(path);
    }

    public static void search(int i, int j)
    {
        if(!exist(i,j) || a[i][j] == 1)
            return;
        if(i == n-1 && j == n-1)
        {
            path++;
            return;
        }
        a[i][j] = 1;
        search(i+1,j);
        search(i-1,j);
        search(i,j+1);
        search(i,j-1);
        a[i][j] = 0;
    }

    public static boolean exist(int i, int j)
    {
        return i>=0 && j >=0 && i < n && j < n;
    }
}

Can anybody recommend a better solution than this or if there are any, please point out the mistakes in this. I made it using simple logic and I don't know much about graph theory.
Is this DFS or BFS?

There are not many things to criticize here ... the basic algorithm in general looks good.

The three items I would caution you on are:

you are keeping track of the path-count in the paths static variable. This is a pattern that, although works, is not very 'pretty', there's a better way... I'll explain.
you modify the source array. This can be OK, but, in general, when you want to modify the source data you should instead work off a copy of the data.
all your other variables (the maze itself and the size of the maze) are static.

With a slight shift in the way you think of your search method, instead of updating the number of paths, you should instead think 'how many paths from here?'

Also, lets fix the static variable issues too (we will need two methods for this):

public static final int search(int[][] data) {
    int[] mymap = new int[data.length][];
    for (int i = 0; i < data.length; i++) {
        mymap[i] = Arrays.copyOf(data[i], data[i].length);
    }
    return search(mymap, 0, 0);
}

Now, the search(...) recursive method should return an int, and it takes the maze as input, not from a static variable.

To avoud the use of the 'n' variable, we use the basic information from the maze. The following is a copy/paste of your code with slight modifications:

it returns int
return-statements inside return a value now
the input includes the 'maze' (instead of being a static variable)
there are no references to n, just to the a array
it has an internal paths variable
it changes how it calls exist(...)

public static int search(int[][] a, int i, int j)
{
    if(!exist(i,j) || a[i][j] == 1)
        return 0; // no path here.
    if(i == a.length - 1 && j == a[i].length - 1)
    {
        return 1; // 1 path here.
    }
    a[i][j] = 1; // mark that we have seen this spot here
    int paths = 0; // introduce a counter...
    paths += search(a, i+1,j); // add the additional paths as we find them
    paths += search(a, i-1,j);
    paths += search(a, i,j+1);
    paths += search(a, i,j-1);
    a[i][j] = 0;
    return paths; // return the number of paths available from this point.
}

The exist() function will also need to change:

public static boolean exist(int[][] a, int i, int j)
{
    return i>=0 && j >=0 && i < a.length && j < a[i].length;
}

Note that this no longer references the n static variable either.

Finally, with these changes (and, really, in the scheme of things they are 'small'), your main method simply becomes:

    Scanner sc = new Scanner(System.in);
    int n = sc.nextInt();
    int[][] a = new int[n][n];
    for(int i = 0; i < n; i++)
    {
        for(int j = 0; j < n; j++)
        {
            a[i][j] = sc.nextInt();
        }
    }
    System.out.println(search(a));

For this, the a does not need to be static either.

If you code the process like I suggest then the methods become much more generic, and you have other advantages like:

the methods are all thread-safe now (you can find many paths in parallel using the same methods).
the data and the logic are both self-contained.
there are no static variables.

Finally, the variable names you have chosen are really short..... consider renaming the a variable at least.


Listing the differences between corresponding elements of two arrays or two objects Mind the code below:

var deltaArray = function(a,b){
    var deltaPtrs = [];
    var deltaVals = [];
    for (var i=0, l=a.length; i<l; ++i)
        if (a[i] !== b[i])
            deltaPtrs[deltaPtrs.length] = i,
            deltaVals[deltaVals.length] = b[i];
    return [deltaPtrs,deltaVals];
};

var deltaHash = function(a,b){
    var deltaKeys = [];
    var deltaVals = [];
    for (var key in b)
        if (a[key] !== b[key])
            deltaKeys[deltaKeys.length] = key,
            deltaVals[deltaVals.length] = b[key];
    return [deltaKeys,deltaVals];
};

That code is obviously duplicated. Considering that the length of the involved arrays/objects is huge (>1kk elements), and that adding an if inside the loop would be detrimental to performance, is there any way to avoid this duplication?

Edit: here is a JSPerf for your enjoyment.

The simplest answer is to remove deltaArray and use deltaHash for everything. As long as your arrays don't have any extra properties, the results will be identical, except the keys are stored as strings instead of numbers.

I'm not sure it's useful to store the keys in one array and the values in another. Storing the keys in an array could be useful if ordering the keys is important, but that doesn't apply here (since we got the keys from a for...in loop, we encountered them in an undefined order). So, you could let the keys be keys and let the values be values, and if you need an array of keys, use Object.keys.

var deltaHash = function(a, b) {
    var delta = {};
    for (var key in b)
        if (a[key] !== b[key])
            delta[key] = b[key];
    return delta;
};

If you still want a separate function for arrays, it would look like this:

var deltaArray = function(a, b) {
    var delta = {};
    for (var i = 0, l = a.length; i < l; ++i)
        if (a[i] !== b[i])
            delta[i] = b[i];
    return delta;
};

You'd still probably want to eliminate redundancy in this case. We could extract the duplicated code into a separate function and end up with something like this:

var diff = function(a, b, delta, key){
    if (a[key] !== b[key])
        delta[key] = b[key];
};

var deltaHash = function(a, b) {
    var delta = {};
    for (var key in b)
        diff(a, b, delta, key);
    return delta;
};

var deltaArray = function(a, b) {
    var delta = {};
    for (var i = 0, l = a.length; i < l; ++i)
        diff(a, b, delta, i);
    return delta;
};

Somehow that's not very satisfying, though; it still feels redundant. What we have at this point really feels like it wants to be more of a classical design:

function Delta() { }

Delta.prototype.diff = function(a, b, key) {
    if (a[key] !== b[key])
        this[key] = b[key];
};

Delta.prototype.initFromArrays = function(a, b) {
    for (var i = 0, l = a.length; i < l; ++i)
        this.diff(a, b, i);
};

Delta.prototype.initFromObjects = function(a, b) {
    for (var key in b)
        this.diff(a, b, key);
};

But again, most of this is not necessary if you decide to build deltas for arrays the same way you build them for other objects, using something like your original deltaHash for both. Aside from some possible naming and stylistic issues, and the awkwardness of having the keys in a separate array, it should do the trick (although you might want to add a hasOwnProperty check, and use push).

Hope this gives you some ideas.


A custom error to be thrown when numeric values are out of bounds Why did I make this? I'm building an interactive data structure visualization, and when certain errors are thrown from certain parts of the code, they are displayed on screen for the user. I have not included that displaying logic.

The if checks seem somewhat smelly to me, but they might be simple enough to the point that they're fine. I'm interested in best practices, ANY criticism is welcome.

export class OutOfBoundsError extends AppError {
// AppError code is not included, but it is just an extremely simple
//abstract class I needed so I could instanceof to check whether a caught
// error was thrown by my own code
// If you want, pretend I'm extending JS's RangeError instead

constructor(valueName: 'length' | 'index' | 'position', value: number,
              bounds?: {
                lowerBoundInclusive?: number;
                upperBoundInclusive?: number
              }) {
    super();
    const lower = bounds?.lowerBoundInclusive;
    const upper = bounds?.upperBoundInclusive;
    if (lower === undefined) {
      if (upper === undefined) {
        this.message = `${valueName} of ${value} is out of bounds`;
      } else {
        this.message = `${valueName} must be less than or equal to ${upper} but\
         ${value} was given`;
      }
    } else {
      if (upper === undefined) {
        this.message = `${valueName} must be greater than or equal to ${lower}\
         but ${value} was given`;
      } else {
        this.message = `${valueName} must be between ${lower} and ${upper} but\
         ${value} was given`;
      }
    }
  }
}
```

This seems good.
I noticed a couple of things, I will try to explain down below.

Lack of type definition
For the sake of readability, it would be great to have some type definitions to make the code look easier to read.

type ValueName = 'length' | 'index' | 'position';

interface Bounds {
    lowerBoundInclusive?: number;
    upperBoundInclusive?: number;
}

export class OutOfBoundsError extends AppError {
    // ...

    constructor(name: ValueName, value: number, bounds?: Bounds) {
        // ...
    }
}

Since Bounds is an structured type, it deserves to be an interface.
The point of defining types is not just the code itself, helps in case of documentation, code hints (code completion, parameters information, ...) and reusability.
In case of reusability, types should be separated from implementations.

message internal property misplace
Inside your constructor, you are setting the value of message. Unlike Javascript, Typescript needs placeholders for internal properties.
Maybe you had, indeed. Your code does not compile, otherwise.
Anyways, internal properties must be declared between the class definition and its constructor.

export class OutOfBoundsError extends AppError {
    private message: string;

    constructor(name: ValueName, value: number, bounds?: Bounds) {
        // ...
    }
}

Truthy and falsy assertions
Avoid checking with explicit undefined, use if (value) instead.
If value is a number type and in fact you are expecting a 0 to be a valid value, Javascript will coerce it into a false. In that case, check your value like so

if (value != null) {
   // ...
}

By checking with null with a single equal symbol, you are checking for undefined and null, as well.

Two or more depth levels
Functions should not have more than one depth level. Depth levels are done by using if, for, while and switch.
Depth levels don't apply to inner functions, object or array instantiations, ...

interface ErrorMessageArgs {
    value: number;
    upper?: number;
    lower?: number;
}

export class OutOfBoundsError extends AppError {
    private message: string = ``;

    constructor(private name: ValueName, value: number, bounds?: Bounds) {
        super();

        const lower = bounds?.lowerBoundInclusive;
        const upper = bounds?.upperBoundInclusive;

        if (lower != null) {
          this.handleTruthyLower({value, upper});
        } else {
          this.handleFalsyLower({value, upper, lower});
        }
    }

    private handleFalsyLower = (args: ErrorMessageArgs) => {
        const { value, upper } = args;
        // ...
    }

    private handleTruthyLower = (args: ErrorMessageArgs) => {
        const { value, upper, lower } = args;
        // ...
    }
}

I decide to define yet another interface to wrap the error message arguments. By doing this, I can define both functions as monadic functions (just because of Robert).

  The ideal number of arguments for a function is zero (niladic). Next comes one (monadic), followed closely by two (dyadic). Three arguments (triadic) should be avoided where possible. More than three (polyadic) requires very special justification — and then shouldn’t be used anyway.
  Robert C. Martin

message property protection
I assume that a third party which manipulates this class is not allowed to modify the error message. It doesn't make any sense, in my opinion.
That is why it may define a read-only access.

export class OutOfBoundsError extends AppError {
    private _message: string;

    constructor(private name: ValueName, value: number, bounds?: Bounds) {
        super();
        // ...
    }

    private handleFalsyLower = (args: ErrorMessageArgs) => {
        const { value, upper } = args;
        // ... message is filled
    }

    private handleTruthyLower = (args: ErrorMessageArgs) => {
        const { value, upper, lower } = args;
        // ... message is filled
    }

    get message() {
        return this._message;
    }
}

Maybe, for more consistency, _message should be protected if OutOfBoundsError is not a sealed class.
And yes, I know that in some case you defined this message property inside the AppError class. I just mention, just in case.

Ternary operation
This is just a preference. When there is only one line to write inside the if and else statements, I just prefer a ternary operation. It seems cleaner to me.

export class OutOfBoundsError extends AppError {
    protected _message = ``;

    constructor(private name: ValueName, value: number, bounds?: Bounds) {
        super();

        const lower = bounds?.lowerBoundInclusive;
        const upper = bounds?.upperBoundInclusive;

        lower != null
            ? this.handleTruthyLower({value, upper})
            : this.handleFalsyLower({value, upper, lower});
    }

    private handleFalsyLower = (args: ErrorMessageArgs) => {
        const { value, upper } = args;
        this._message = upper != null
                        ? `${this.name} must be less than or equal to ${upper} but\
                            ${value} was given`
                        : `${this.name} of ${value} is out of bounds`;
    }

    private handleTruthyLower = (args: ErrorMessageArgs) => {
        const { value, upper, lower } = args;
        this._message = upper != null
                        ? `${this.name} must be between ${lower} and ${upper} but\
                            ${value} was given`
                        : `${this.name} must be greater than or equal to ${lower}\
                            but ${value} was given`;
    }

    get message() {
        return this._message;
    }
}

Even for void function calls.

Hope it helps and any suggestions are welcome.


A long trigonometric expression I have the following expression in my C++ code:

theta1 = theta0 - (acos((pow(constlength_link1 + length, 2) + (pow(length_base_end, 2) - pow(length_link2, 2)))
     / (2 * length_base_end * (constlength_link1 + length)))) * 180 / M_PI;

I understand that such a code will be very difficult for anyone else to debug. Should I break it down using additional variables?

Are there any other better approaches?

Edit: Sorry about the lack of details:

I am trying to calculate sum of two angles theta1 and theta2. These angles are being calculated by subtracting the inverse of cosine of an expression from a constant angle (theta0 or 180). The expression mentioned in the last line involves a division. The dividend is something like:

  (square of sum of two numbers) +/- (square of a number  - square of
  another number)

The divisor is something like:

  (2 * a number * (sum of two numbers))

The function is as shown below:

float length_angle(float length = 0.0)
{
    float theta1, theta2;

    theta1 = theta0 - (acos((pow(constlength_link1 + length, 2) + (pow(length_base_end, 2) - pow(length_link2, 2)))
     / (2 * length_base_end * (constlength_link1 + length)))) * 180 / M_PI;

    theta2 = 180 - (acos((pow(constlength_link1 + length, 2) - (pow(length_base_end, 2) - pow(length_link2, 2)))
     / (2 * length_link2 * (constlength_link1 + length)))) * 180 / M_PI;

    return theta1 + theta2;
}

I would like to write this expression better so that someone else can read or debug the code if required.

I am trying to calculate sum of two angles theta1 and theta2. These angles are being calculated by subtracting the inverse of cosine of an expression from a constant angle (theta0 or 180). The expression mentioned in the last line involves a division. The dividend is something like:
(square of sum of two numbers) +/- (square of a number - square of another number)
The divisor is something like:
(2 * a number * (sum of two numbers))

Don't tell us, tell the maintainer of your codebase!
float length_angle(float length = 0)
{
    auto square = [](auto x) { return x*x; };

    auto dividend1 = square(constlength_link1 + length) + square(length_base_end) - square(length_link2);
    auto divisor1 = 2 * length_base_end * (constlength_link1 + length);

    float theta1 = theta0 - acos(dividend1 / divisor1) * 180 / M_PI;

    auto dividend2 = square(constlength_link1 + length) - square(length_base_end) + square(length_link2);
    auto divisor2 = 2 * length_link2 * (constlength_link1 + length);

    float theta2 = 180 - acos(dividend2 / divisor2) * 180 / M_PI;

    return theta1 + theta2;
}

Three optimizations I'd make to this code for better API design: First, don't use default function arguments. If I meant to write length_angle(1) but fat-fingered length_angle() instead, give me an error! If I meant to write length_angle(0), trust me to write it. You're hurting more than you're helping when you try to save your caller that one extra character of typing.
Second, use radians. Radians are the right answer for trig in the same way that UTF-8 is the right answer for character encoding. Trust the past couple millennia of mathematicians when they tell you that angles should be measured in radians. This will save you some repetition here, and some unnecessary rounding errors as well.
Third, optionally, use double; it'll give you more accuracy and be no slower — maybe even faster — than float if you're targeting a normal desktop computer. Obviously if you're targeting a 32-bit microprocessor with hardware float and software-emulated double, don't use double. :)
In your real code, the above will be super readable because you'll be using real names for the variables instead of long placeholders like constlength_link1. You can also say sqr or sq instead of square. So:
double length_angle(double length)
{
    auto sq = [](auto x) { return x*x; };

    auto a1 = sq(L1 + length) + sq(L3) - sq(L2);
    auto b1 = 2 * L3 * (L1 + length);

    auto theta1 = theta0 - acos(a1 / b1);

    auto a2 = sq(L1 + length) + sq(L2) - sq(L3);
    auto b2 = 2 * L2 * (L1 + length);

    auto theta2 = M_PI - acos(a2 / b2);

    return theta1 + theta2;
}

Lastly, perhaps notice that theta1 + theta2 is theta0 + M_PI - (acos(a1/b1) + acos(a2/b2)); and perhaps deal with the possibility of division-by-zero when b1 or b2 is zero. Or even factor out the repeated code, now that it's been made blatantly obvious by our renamings:
double length_angle(double length)
{
    auto sq = [](auto x) { return x*x; };

    auto f = [&](auto start, auto a, auto b) {
        auto y = sq(L1 + length) + sq(b) - sq(a);
        auto x = 2 * b * (L1 + length);
        return start - acos(y / x);
    };

    return f(theta0, L2, L3) + f(M_PI, L3, L2);
}


Updating 3 columns conditionally on a database table I need to run this script to update the value of 3 columns based on certain conditions. Right now, this is what I have:

UPDATE dbo.Roles
SET DisplayName = Name
WHERE DisplayName LIKE '';

UPDATE dbo.Roles
SET Name = CONCAT(Tenant, '_', DisplayName)
WHERE Name NOT LIKE ('%' + Convert(varchar(200), Tenant) + '%');

UPDATE dbo.Roles
SET NormalizedName = CONCAT(Tenant, '_', UPPER(DisplayName))
WHERE NormalizedName NOT LIKE ('%' + Convert(varchar(200), Tenant) + '%');

I'm doing this with 3 separate update statements. I can't help but think there's probably a better way, but maybe I'm overthinking. Any suggestions?

I haven't tested this but you should be able to use CASE statements to conditionally update column values, with fallback values of the existing column values:

UPDATE dbo.Roles SET
    DisplayName = CASE WHEN DisplayName LIKE `` THEN Name ELSE DisplayName END,
    Name = CASE WHEN Name NOT LIKE ('%' + Convert(varchar(200), Tenant) + '%') THEN CONCAT(Tenant, '_', DisplayName) ELSE Name END,
    NormalizedName = CASE WHEN DisplayName LIKE '' THEN CONCAT(Tenant, '_', UPPER(DisplayName)) ELSE DisplayName END
WHERE DisplayName LIKE ''
    OR Name NOT LIKE ('%' + Convert(varchar(200), Tenant) + '%')
    OR NormalizedName NOT LIKE ('%' + Convert(varchar(200), Tenant) + '%');

Note that this may be slightly more challenging to read and there may be more updates per column because it would update any row that needs any column updated, but it would be two fewer UPDATE statements.

Your mileage may vary- especially depending on how many rows exist in the table. It would be wise to compare not only the execution times but also the execution plans.


ReactiveX observable that uses another observable I am trying to write an Angular service that will wrap the regular Http service and automatically make authentication calls if Bearer token is not available or invalid.

Here's an illustration of a regular GET call made via the service:

this.httpWithAutomagicAuth
    .get("http://localhost:5001/books")
    .map(res => res.json())
    .subscribe(
        data => {
            self.data = data;
        }
    );

And here's my very sloppy implementation for such service. Obviously, I'm not using the right ReactiveX idioms in the code.

As you can easily notice I am trying to build an observable (Observable.create) that is using another observable returned by this.login(). I am sure there is a better way of chaining/nesting observable for this scenario.

Please, suggest improvements that will make code:

terse
easy to read and understand

@Injectable()
export class HttpWebApiAuthService {

constructor(private http: Http) { }

// ...

public get(url: string, options?: RequestOptionsArgs): Observable<Response> {
    // TODO this code is for test purpose only (basically, it required to enforce the jwtToken retrieval branch execution)
    // this.clearJwtToken();

    if (!this.getJwtToken()) {
        return Observable.create(
            (result: Observer<Response>) => {
                this.login()
                    .map(res => res.json())
                    .subscribe(
                        data => {
                            this.saveJwtToken(data.id_token);

                            this.executeGet(url, options)
                                .subscribe(authenticationResult => {
                                    result.next(authenticationResult);
                                });
                        },
                        error => { console.error("Authentication error", error); },
                        () => { console.info("Authentication complete"); }
                    );
                },
                error => { console.error("OBSERVABLE error: ", error); },
                () => { console.info("OBSERVABLE complete");
            }
        );
    } else {
        return this.executeGet(url, options);
    }
}

private login() : Observable<Response> {
    const authBody =
        {
            "client_id": "...",
            "username": "...",
            "password": "...",
            // ...
        };
    const headers = new Headers();
    headers.append("Content-Type", "application/json");

    return this.http.post("https://AUTH_URL", JSON.stringify(authBody), { headers: headers });
}

// ...

}

I had to use flatMap to achieve what I wanted:

public get(url: string): Observable<Response> {
  return this
    .login()
    .flatMap(token_id => this._http.get(url));  // token_id can be used here
}

public login(): Observable<string> {
  const authBody =
      {
          "client_id": "...",
          "username": "...",
          "password": "...",
          // ...
      };
  const headers = new Headers();
  headers.append("Content-Type", "application/json");

  return this._http
    .post('AUTH_URL', authBody)
    .map(response => response.json().token_id);
}


Simple login user authentication I've created a simple User Login page. Here User is authenticated [username, password] and if it is a valid user, values like username, name and role of the user is stored in the Session.

And according to the role of the user, the user is redirected to different pages. This is simple code, but please help me to make my login page more secure and add more functionality.

using System;
using System.Data;

namespace PROJECT.view.User
{
    public partial class LoginUser : System.Web.UI.Page
    {
        protected void Page_Load(object sender, EventArgs e)
        {
            try
            {
                Session.Clear();
                Session.Abandon();
            }
            catch (Exception)
            {

                throw;
            }
        }

        protected void btnSignIn_Click(object sender, EventArgs e)
        {
            try
            {
                DataTable dt = new DataTable();
                dt = new C_User().Get_LoginUser(inputUserName.Value, inputPwd.Value);
                if(dt != null && dt.Rows.Count > 0)
                {
                    DataRow dr = dt.Rows[0];
                    Session["username"] = dr["USERNAME"].ToString();
                    Session["name"] = dr["NAME"].ToString();
                    Session["role"] = dr["ROLE"].ToString();

                    #region affiliated user pages
                    Int16 userRole = 0;
                    userRole = Convert.ToInt16(Session["role"]);
                    switch (userRole)
                    {
                        case 1:
                            Response.Redirect("Registration.aspx", false);
                            break;
                        case 2:
                            Response.Redirect("Medicine_1.aspx", false);
                            break;
                        case 3:
                            Response.Redirect("PortalRegister.aspx", false);
                            break;
                        case 4:
                            Response.Redirect("Billing.aspx", false);
                            break;
                    }

                    #endregion
                }
                else
                {

                    Response.Redirect("LoginUser.aspx", false);
                    return;
                }
            }
            catch (Exception)
            {

                throw;
            }
        }
    }
}

Let's review from top to bottom.

Page_Load()

No need to have a try..catch here if the code only throw inside the catch. Omitting the try..catch will lead to the same result and removes one level of indentation.

btnSignIn_Click()

Like in Page_Load() there is no need to have that try..catch either.
Classes should be named using PascalCase casing (see: .NET Naming Guidelines), hint: C_User. The same applies to naming of methods. hint: Get_LoginUser(()
By reversing the condition of if(dt != null && dt.Rows.Count > 0) you can return early hence there won't be the need of the else part which saves you another level of indentation like so

if(dt == null || dt.Rows.Count == 0)
{
    Response.Redirect("LoginUser.aspx", false);
    return;
}

DataRow dr = dt.Rows[0];
Session["username"] = dr["USERNAME"].ToString();
....

As regions may be acceptable inside a class they are phroned upon inside a method. Having a region inside a method is usually a clear sign that this method is doing to much.
Why wasting vertical space by first declaring an int and setting it to 0 and on the next line of code another value is assigned to it. Just do

Int16 userRole = Convert.ToInt16(Session["role"]);

The switch could be improved by replacing the magic numbers with meaningful constants.


PHP MYSQL loop to check if LicenseID Values are contained in mysql DB I have some troubles to find the right loop to check if some values are contained in mysql DB. I'm making a software and I want to add license ID. Each user has x keys to use.

Now when the user start the client, it invokes a PHP page that check if the Key sent in the POST method is stored in DB or not.

If that key isn't store than I need to check the number of his keys. If it's > than X I'll ban him otherwise i add the new keys in the DB.

I'm new with PHP and MYSQL. I wrote this code and I would know if I can improve it.

<?php

$user = POST METHOD
$licenseID = POST METHOD

$resultLic= mysql_query("SELECT  id , idUser , idLicense FROM license WHERE idUser = '$user'") or die(mysql_error());
$resultNumber = mysql_num_rows($resultLic);
$keyFound = '0'; // If keyfound is 1 the key is stored in DB

while ($rows = mysql_fetch_array($resultLic,MYSQL_BOTH)) {
   //this loop check if the $licenseID is stored in DB or not
    for($i=0; $i< $resultNumber ; i++) {
        if($rows['idLicense'] === $licenseID) {
            //Just for the debug
            echo("License Found");
            $keyFound = '1';
            break;
    }
    //If key isn't in DB and there are less than 3 keys the new key will be store in DB

    if($keyfound == '0' && $resultNumber < 3) {
        mysql_query( Update users set ...Store $licenseID in Table)
    }

    // Else mean that the user want user another generated key (from the client) in the DB and i will be ban (It's wrote in TOS terms that they cant use the software on more than 3 different station)
    else {
        mysql_query( update users set ban ='1'.....etc );
    }
}

?>

I know that this code seems really bad so i would know how i can improve it. Someone Could give me any advice?

I choose to have 2 tables: users where all information about the users is, with fields id, username, password and another table license with fields id, idUsername, idLicense (the last one store license that the software generate)

First of all: both code and table design can (have?) to be improved:

Tables must be normalized and can be improved by removing data and duplicates

Users table can (?) have username only as natural PK (pure id eliminated)
Optional: licence table will be better understandable if FK will have same name as for Users table (username)

For-cycle for your task is not-so-good style of coding:

You can just test result (size of array) of SELECT idLicense WHERE idLicense = $licenseID (SQLGuru: Fixme!!!)
Total amount of licences per user can be extracted from table with single SELECT COUNT(idLicense) WHERE idUser = '$username' (SQLGuru: Fixme again!!!)


Opening a workbook and copying pages to master workbook I have this code which opens 2 workbooks, copies a sheet and paste then to the master workbook. It is currently taking 3 minutes.  Can this be done quicker (i.e. without opening each workbook to copy)?

It takes roughly 3 minutes to do.

Sub Load()
    Dim masterWB As Workbook
    Dim dailyWB As Workbook
    Dim lastweekWB As Workbook
    Dim R As Range
    Dim B As Range
    Dim C As Range
    Dim Lrow As Long

    Application.DisplayAlerts = False

    'Set Current Workbook as Master
    Set masterWB = Application.ThisWorkbook
    'Set some Workbook as the one you are copying from
    Set dailyWB = Workbooks.Open(Sheets("Control Manager").Range("O2"))

    'Copy the Range from dailyWB and Paste it into the MasterWB
    dailyWB.Sheets("Summary1").Range("A1:BJ200").Copy masterWB.Sheets("Summary").Range("A1").Rows("1:1")
    'formatting and paste as values
    masterWB.Activate
    Worksheets("Summary").Select
    'trim values
    Columns("A:BJ").Select
    Selection.Columns.AutoFit
    Selection.Copy
    Selection.PasteSpecial xlPasteValues

    'repeat for next Sheet
    dailyWB.Sheets("risk1").Range("A1:BB200").Copy masterWB.Sheets("risk").Range("A1").Rows("1:1")
    'formatting and paste as values'
    masterWB.Activate
    Worksheets("risk").Select
    Columns("A:BB").Select
    With Application.WorksheetFunction
        For Each B In Intersect(Columns("A:BB"), ActiveSheet.UsedRange)
            B.Value = .Trim(B.Value)
        Next B
    End With
    Selection.Columns.AutoFit
    Selection.Copy
    Selection.PasteSpecial xlPasteValues

    'repeat for CS sheet
    dailyWB.Sheets("CS today").Range("A1:L3").Copy masterWB.Sheets("CS").Range("A1").Rows("1:1")
    'formatting and paste as values
    masterWB.Activate
    Worksheets("CS").Select
    Columns("A:L").Select
    'trim cells to exclude spaces.
    With Application.WorksheetFunction
        For Each R In Intersect(Columns("A:L"), ActiveSheet.UsedRange)
            R.Value = .Trim(R.Value)
        Next R
    End With
    Selection.Columns.AutoFit
    Selection.Copy
    Selection.PasteSpecial xlPasteValues

    ''''''''''''Get Last Week Data''''''''''''''''''''''

    Set lastweekWB = Workbooks.Open(Sheets("Control Manager").Range("O3"))

    'repeat for next risk Sheet
    lastweekWB.Sheets("risk2").Range("A1:BB200").Copy masterWB.Sheets("risk_lastweek").Range("A1").Rows("1:1")
    'formatting and paste as values

    masterWB.Activate
    Worksheets("risk_lastweek").Select
    Columns("A:BB").Select
    With Application.WorksheetFunction
        For Each B In Intersect(Columns("A:BB"), ActiveSheet.UsedRange)
            B.Value = .Trim(B.Value)
        Next B
    End With
    Selection.Columns.AutoFit
    Selection.Copy
    Selection.PasteSpecial xlPasteValues

    Application.DisplayAlerts = True

    'Close the Workbook without saving
    dailyWB.Close False
    lastweekWB.Close False
    'Clear the Variables
    Set dailyWB = Nothing
    Set masterWB = Nothing
    Set lastweekWB = Nothing
End Sub

Separating tasks into multiple subroutines will make the code easier to test and modify.

This  video: Excel VBA Introduction Part 5 - Selecting Cells (Range, Cells, Activecell, End, Offset) will explain why you rarely need to Select or Activate an Object.

I would use .Range("A1") instead of .Range("A1").Rows("1:1") because Range.Copy targets the first cell in the destination.

Refactored Code

Sub Load()
    LoadDailyWorkbook
    LoadLastWeeksWorkbook
End Sub

Sub LoadDailyWorkbook()
    Const A1BJ200 As String = "A1:BJ200"
    Const A1L3 As String = "A1:L3"
    Dim masterWB As Workbook
    Dim dailyWB As Workbook
    'Set Current Workbook as Master
    Set masterWB = Application.ThisWorkbook
    'Set some Workbook as the one you are copying from
    Set dailyWB = getWorkbook(Sheets("Control Manager").Range("O2"))

    If Not dailyWB Is Nothing Then
        With dailyWB
            'Copy the Range from dailyWB and Paste it into the MasterWB
            .Worksheets("Summary1").Range(A1BJ200).Copy masterWB.Worksheets("Summary").Range("A1")
            TrimRange masterWB.Worksheets("Summary").Range(A1BJ200)
            'repeat for next Sheet
            .Worksheets("risk1").Range(A1BJ200).Copy masterWB.Worksheets("risk").Range("A1")
            TrimRange masterWB.Worksheets("risk").Range(A1BJ200)

            'repeat for CS sheet
            .Worksheets("CS today").Range(A1L3).Copy masterWB.Worksheets("CS").Range("A1").Rows("1:1")
            TrimRange masterWB.Worksheets("CS").Range(A1L3)
            .Close SaveChanges:=False
        End With

    End If
End Sub

Sub LoadLastWeeksWorkbook()
    Const A1BJ200 As String = "A1:BJ200"
    Dim masterWB As Workbook
    Dim lastweekWB As Workbook

    'Set Current Workbook as Master
    Set masterWB = Application.ThisWorkbook

    ''''''''''''Get Last Week Data''''''''''''''''''''''
    Set lastweekWB = getWorkbook(Workbooks.Open(Sheets("Control Manager").Range("O3")))
    If Not lastweekWB Is Nothing Then
        With lastweekWB
            'repeat for next risk Sheet
            .Worksheets("risk2").Range(A1BJ200).Copy masterWB.Worksheets("risk_lastweek").Range("A1")
            TrimRange masterWB.Worksheets("risk_lastweek").Range(A1BJ200)

            TrimRange masterWB.Columns("A:BB")
            .Close SaveChanges:=False
        End With
    End If
End Sub

Function getWorkbook(FullName As String) As Workbook
    If Len(Dir(FullName)) = 0 Then
        MsgBox FullName & " not found found", vbCritical, "File Not Found"
    Else
        Set getWorkbook = Workbooks.Open(FullName)
    End If
End Function

Sub TrimRange(Target As Range)
    Dim results As Variant
    Set Target = Intersect(Target.Parent.UsedRange, Target)
    If Target Is Nothing Then
        Exit Sub
    ElseIf Target.Count = 1 Then
        Target.Value = Trim(Target.Value)
        Exit Sub
    Else
        results = Target.Value

        Dim r As Long, c As Long
        For r = 1 To UBound(results)
            For c = 1 To UBound(results, 2)
                results(r, c) = Trim(results(r, c))
            Next
        Next
        Target.Value = results
    End If
    Target.Columns.EntireColumn.AutoFit
End Sub


Generating a C++ header containing OpenGL function definitions I'm currently working with Win32 and OpenGL a little bit and I needed a way to get the pointers of the OpenGL functions that aren't included in the Windows gl/GL.h header file. Rather than spending hours going through the OpenGL specification and manually typing out each OpenGL function definition in a header file, I created the following Python script which will automatically generate that header file for me.

INITIAL_HEADER_STRING_ONE = """
#ifndef OPENGL_FUNCTIONS_H_
#define OPENGL_FUNCTIONS_H_

#include <Windows.h>
#include <gl/GL.h>
#include <gl/GLU.h>
#include <gl/glext.h>
#include <gl/wglext.h>

"""

INITIAL_HEADER_STRING_TWO = """

void* GetGLProcPointer(const char* name)
{
    void* procPointer = (void*)wglGetProcAddress(name);
    if(
        procPointer == 0 ||
        procPointer == (void*)0x1 ||
        procPointer == (void*)0x2 ||
        procPointer == (void*)0x3 ||
        procPointer == (void*)-1
    )
    {
        HMODULE module = LoadLibraryA("opengl32.dll");
        procPointer = (void*)GetProcAddress(module, name);
    }
    return procPointer;
}

void GenerateGLProcPointers()
{
"""

def generate_header_string(function_names_file_name):
    header_string = INITIAL_HEADER_STRING_ONE

    with open(function_names_file_name, "r") as function_names:
        for line in function_names.readlines():
            stripped_line = line.rstrip("\n");
            stripped_line_upper = stripped_line.upper();
            header_string += "PFN{0}PROC {1};\n".format(stripped_line_upper, stripped_line)
        header_string += INITIAL_HEADER_STRING_TWO

    with open(function_names_file_name, "r") as function_names:
        for line in function_names.readlines():
            stripped_line = line.rstrip("\n");
            stripped_line_upper = stripped_line.upper();
            header_string += "\t{1} = (PFN{0}PROC)GetGLProcPointer(\"{1}\");\n".format(stripped_line_upper, stripped_line)
        header_string += "}\n"
        header_string += "\n#endif"

    return header_string

def generate_header_file(generated_header_string):
    with open("opengl_functions.h", "w+") as opengl_functions_header:
        opengl_functions_header.write(generated_header_string)

def main():
    generated_header_string = generate_header_string("OpenGL_Functions.txt");
    generate_header_file(generated_header_string)

if __name__ == "__main__":
    main()

The text file linked here contains a list of all OpenGL 4.5 function names.

In python it is not necessary to end a line with a ;.

Your function generate_header_file is probably better named write_to_header_file.

I would read the file only once and do everything in one pass in generate_header_string:

from itertools import chain

def generate_header_string(function_names_file_name):
    header_string1 = [INITIAL_HEADER_STRING_ONE]
    header_string2 = []

    def_string1 = "PFN{}PROC {};"
    def_string2 = "\t{1} = (PFN{0}PROC)GetGLProcPointer(\"{1}\");"

    with open(function_names_file_name, "r") as function_names:
        for line in function_names.readlines():
            line = line.rstrip("\n")
            line_upper = line.upper()
            header_string1.append(def_string1.format(line_upper, line))
            header_string2.append(def_string2.format(line_upper, line))
        header_string1.append(INITIAL_HEADER_STRING_TWO)
        header_string2.append("}")
        header_string2.append("\n#endif")

    return "\n".join(chain(header_string1, header_string2))

Here I changed the accumulation variable to lists which I join in the end. This gives some memory boost, because string addition is very costly, especially if the strings become bigger (and your INITIAL_HEADER_STRING_X is already quite big). The lists also get rid of having to manually write \n in the strings, because we can just use it as joining variable at the end.

You will want to give the two variables better names (like header_declaration and header_initialization or whatever fits their use case; I don't know any OpenGL syntax, so I'm not exactly sure).

I also declared the strings you format once before the loop to avoid having to redeclare them every iteration. They could also do with better names, probably something similar to the header lists.


Refactoring fat ASP.NET MVC Controller I have just began porting an old project to ASP.NET MVC . In the end, I'll have a lot of controller methods like the one below, called by AJAX requests done by JQGrid objects in the pages:

AJAX request:

  /UserRole/RolesByUserGridData?userId=2&_search=false&nd=1407171194811&rows=10&page=1&sidx=RoleName&sord=asc

Controller code:

public ActionResult GetRolesByUserGridData(MvcJqGrid.GridSettings gridSettings, int userId)
    {
        IQueryable<Role> allRoles = _roleRepository.GetAsQueryable(where:null, includeProperties:"");
        IQueryable<UserRole> userRolesByUser = _repository.GetAsQueryable(where: c => c.UserId == userId, includeProperties: "Role");

        var gridItems = from role in allRoles
                        join userRole in userRolesByUser on role.Id equals userRole.RoleId into loj
                        from item in loj.DefaultIfEmpty()   // LEFT OUTER JOIN equivalent
                        select new UserRoleByUserViewModel
                        {
                            IsRoleAssociatedWithUser = (item.UserId == userId),
                            RoleName = role.Name
                        };

        SortOrder sortOrder = gridSettings.SortOrder != "desc" ? SortOrder.Asc : SortOrder.Desc;

        var pagedOrderedItems = PagingHelper<UserRoleByUserViewModel>.GetAsOrderedPagedList(
            gridItems, sortOrder, gridSettings.SortColumn,
            gridSettings.PageIndex, gridSettings.PageSize);

        var jsonData = new
        {
            total = pagedOrderedItems.TotalItemCount / gridSettings.PageSize + 1,
            page = gridSettings.PageIndex,
            records = pagedOrderedItems.TotalItemCount,
            rows = (
                from c in pagedOrderedItems
                select new
                {
                    id = "",
                    cell = new[]
                    {
                        "Edit",
                        "Details",
                        c.IsRoleAssociatedWithUser.ToString(),
                        c.RoleName
                    }
                }).ToArray()
        };

        return Json(jsonData, JsonRequestBehavior.AllowGet);
    }

My goal is to return IEnumerable from repositories and have fairly dumb controllers.

How would you suggest refactoring this?

Ideally, the code in my controller method should be:

public ActionResult GetRolesByUserGridData(MvcJqGrid.GridSettings gs, int userId)
{
   var json = _userRoleService
      .GetRolesByUser()
      .ToOrderedList(“Id”, “Asc”)
      .ToPagedList(gs.PageIndex, gs.PageSize)
      .ToJsonData();
   return Json(jsonData, JsonRequestBehavior.AllowGet);
}

where ToOrderedList(), ToPagedList() and ToJsonData() would be extension methods to IQueryable<>, placed outside the Web project.

But it's still not clear to me where should the ViewModels live in this scenario:

A. Have them in the Service layer, which is in a different project - but then they lose all the logic related to Data Annotation, or I need to reference MVC in the Service layer, which seems weird.

B. Have them in the Web project - but why shouldn't the service be aware of the ViewModel and only retrieve the data that the ViewModel needs? Is it really necessary to create a new DTO object only to pass data from the service to the controller?

There's a few issues I can see with this code.

The repositories are are sitting in the presentation layer. A big no no. It's software 101, don't place data access in your presentation layer, and not good to put business logic in the presentation layer.

Those _roleRepository calls need to go in to a RoleRetrievalService or something similar. That centralise the business logic.

Your repositories should to sit behind a service layer which handles all of the calls to the DAL. Your service layer should contain business logic and data transformation which turns those DAL entities in to models which can be consumed by the presentation layer.

The query which does the left join should go in the service layer with the parameters required to execute the query being passed in as parameters. You could also pass in the sort options as well.

That last statement which builds the results (jsonData) is exactly what should be produced by your service layer.

It all hinges one how much time you have and how much technical debt you want to incur :)


Naive string compression Description:

Implement a method to perform basic compression using counts of repeated characters. If the compressed string is not smaller then return the original string.

Code:

class Main {
  public static String compress(String in) {
    if (in == null) {
      throw new IllegalArgumentException("Input string cannot be null");
    }
    if (in.length() <= 1) {
      return in;
    }
    StringBuffer out = new StringBuffer();
    int count = 1;
    for (int i = 1; i < in.length(); i++) {
      char current  = in.charAt(i);
      char previous = in.charAt(i - 1);

      if (current == previous) {
        count++;
      }
      else {
        out.append(previous);
        out.append(count);
        count = 1;
      }
    }
    out.append(in.charAt(in.length() - 1));
    out.append(count);
    return out.toString().length() < in.length() ? out.toString() : in;
  }

  public static void main(String[] args) {
    try {
      System.out.println("Should not happen: " + compress(null));
    } catch (IllegalArgumentException e) {
      System.out.println("Got expected exception for null");
    }
    System.out.println(compress("").equals(""));
    System.out.println(compress("a").equals("a"));
    System.out.println(compress("ab").equals("ab"));
    System.out.println(compress("aa").equals("aa"));
    System.out.println(compress("aabcccccaaa").equals("a2b1c5a3"));
    System.out.println(compress("aabb").equals("aabb"));
  }
}

Question:

The solution was just based on intuition. The last append was by trial and error which I didn't like (I may be more nervous during the real interview). I would like to know if there is any way to avoid such mistakes, possibly using loop invariants?

The main problem IMO is, that you cram it all into a single method which is not helpful to organize your thoughts.

When I tried this for comparison, I came up with the following in pseudo code:

// outer logic
compress(s)
    rle = doRunLengthEncoding(s)
    if rle.length < s.length return rle
    else return s

// base algorithm
doRunLengthEncoding(s)
    res = empty String buffer
    char[] characters = s.toCharArray
    int startIndex = 0
    while startIndex < characters.length
        int endIndex = findEndOfCurrentSequence(startIndex, characters)
        res.append(currentChar).append(endIndex - startIndex)
        startIndex = endIndex
    return res

// return the first index i, where c[i] != c[start], return
// c.length if there is no such index
findEndOfCurrentSequence(start, c)
    ... // simple enough

This way, you divide the problem into manageble pieces, give you thoughts a place to stop every once in a while, and should get rid of this nervousness.

BTW: from an interviewer's point of view (at least I you want into my team in my country ;-)) a well thought pseudocode algorithm is worth more than working code. If you apply for a programmer's job, I just assume that you have the basic skills to use an IDE and kick against some piece of code until it compiles and produces the correct result. What I am looking for is organized thoughts and problem solving skills.

One last thing: there is one and only one correct exception to throw for a null parameter that must not be null: NullPointerException. Ideally by simply using Objects.requireNonNull.


How to fill an ArrayList of ArrayLists with a Left Join? I have a class Employee that contains an ArrayList of Projects.

I'm storing the Employees in one table, and the Projects in another.

I'm trying to find the best way to create an ArrayList of Employee objects based on a result set.  Simply creating an ArrayList of Employees based on a result set is pretty straightforward, but I'm finding that filling an ArrayList of Employees that each contains an ArrayList of Projects isn't so simple.

Right now, my getEmployees() function is using two nested SQL queries to accomplish this, something like this:

public ArrayList<Employee> getEmployees()
{
    PreparedStatement ps1 = null;
    ResultSet rs1 = null;
    PreparedStatement ps2 = null;
    ResultSet rs2 = null;

    ArrayList<Employee> employees = new ArrayList<Employee>();

    String query1 = "SELECT * FROM employees "
                  + "ORDER BY employee_id ASC";

    try
    {
        ps1 = conn.prepareStatement(query1);
        rs1 = ps1.executeQuery();

        while (rs1.next())
        {
            Employee employee = new Employee();

            int employeeID = rs1.getInt("employee_id");
            employee.setEmployeeID(employeeID);
            employee.setName(rs1.getString("employee_name"));

            // Get projects for this employee
            ArrayList<Project> projects = new ArrayList<Project>();

            String query2 = "SELECT * FROM projects "
                          + "WHERE employee_id = ?";

            ps2 = conn.prepareStatement(query2);
            ps2.setInt(1, employeeID);
            rs2 = ps2.executeQuery();

            while (rs2.next())
            {
                Project project = new Project();

                project.setProjectID(rs2.getInt("project_id"));
                project.setName(rs2.getInt("project_name"));

                projects.add(project);
            }

            employee.setProjects(projects);

            employees.add(employee);
        }

        return employees;
    }
    catch (SQLException e)
    {
        e.printStackTrace();
    }
    finally
    {
        try
        {
          // close result sets and prepared statements
        }
        catch (SQLException e)
        {
            e.printStackTrace();
        }
    }

    return null;
}

The above code works, but it seems messy to me.

Is there a way to do this without having to nest two separate SQL queries?  I've tried using a LEFT JOIN to get a single result set back, but I was unable to figure out a way to use that single result set to fill the ArrayList of Employees, each containing an ArrayList of Projects.

EDIT: Assume the tables have the following structures:

employees table:

employee_id  int(11)  NOT NULL  PRIMARY KEY  AUTO_INCREMENT
employee_name  varchar(60)  NOT NULL

projects table:

project_id  int(11)  NOT NULL  PRIMARY KEY  AUTO_INCREMENT
employee_id  int(11)  NOT NULL
project_name  varchar(60)  NULL

A standard way to do this is through break-processing, where you track one value, and when it changes, you do something special.... but you may find it easier to do a more unstructured system:

Map<Integer, List<Project>> employeeProjects = new HashMap<>();
Map<Integer, Employee> employees = new TreeMap<>(); // Treeset ... sorted by employeeID

// join the tables.
String select = " select e.employee_id, e.employee_name, p.project_id, p.project_name "
              + " from employees e
                      left outer join projects p on e.employee_id = p.employee_id"
              + " order by e.employee_id, p.project_name";

// do the select.....

while (rs.next()) {
    Integer employeeID = rs.get("employee_id");
    Employee emp = employees.get(employeeID);
    if (emp == null) {
        emp = new Employee();
        emp.setName(rs.get("employee_name");
        emp.setEmployeeID(employeeID);
        employees.put(employeeID, emp);
        // create a new list for this employee
        employeeProject.put(employeeID, new ArrayList<Project>());
    }

    String projectName = rs.getString("project_name");

    if (!rs.wasNull()) {
        List<Project> projects = employeeProject.get(employeeID);
        Project proj = new Project();
        proj.setID(rs.getInt("project_id"));
        proj.setName(projectName);
        projects.add(proj);
    }

}
rs.close();

Then, once you have the data structured the way you want, you can:

List<Employee> result = new ArrayList<>();
for (Employee emp : employees.values()) {
    emp.setProjects(employeeProjects.get(emp.getEmployeeID());
    result.add(emp);
}
return result;


Copying a file hash to clipboard without PowerShell … isn't easy.

@echo off&(for /f "tokens=*" %%a in ('certutil -hashfile "%1" MD5') do echo %%a&for /f "delims=" %%b in ('echo %%a^|findstr /r /i /x [0-9A-F]*') do if "%%b" neq "" echo %%b|clip&pause&exit)&pause

PowerShell is slow to load so I used batch instead. I use this command to extend the context menu of windows explorer, hence a single line.

There is one minor issue; certutil doesn't support empty files.

Bonus registry entries: https://paste.ee/p/sgSJY

All zero-sized files will have the same checksum using the same given hash algorithm. Hence, use the following batch-script:

@ECHO OFF
SETLOCAL EnableExtensions
for /f %%G in ("%~1") do set "_len=%%~zG"
if %_len% EQU 0 (
  echo MD5 hash of %1
  echo D41D8CD98F00B204E9800998ECF8427E
  echo CertUtil bypassed for zero file size
  echo D41D8CD98F00B204E9800998ECF8427E|clip
) else (
  for /f "tokens=*" %%a in ('certutil -hashfile "%~1" MD5') do (
    echo %%a
    for /f "delims=" %%b in ('echo %%a^|findstr /r /i /x ^[0-9A-F]*[0-9A-F]$') do (
      if "%%b" neq "" echo %%b|clip
    )
  )
)
pause

Note that %%~zG expands %%G to size of file (read call /?).

if "%%b" neq "" echo %%b|clip seems to be superfluous then. Simple echo %%b|clip should suffice.

Sample output:

==> D:\bat\CodeReview\179105.bat out.txt
MD5 hash of out.txt
D41D8CD98F00B204E9800998ECF8427E
CertUtil bypassed for zero file size
Press any key to continue . . .

==> D:\bat\CodeReview\179105.bat D:\bat\CodeReview\179105.bat
MD5 hash of D:\bat\CodeReview\179105.bat:
c2a4cabffac79a26ee5ed7c97cefb44b
CertUtil: -hashfile command completed successfully.
Press any key to continue . . .

==>


Average value for each row I have these classes:

public class DelayReason
{
    public virtual ICollection<WorkOrder> WorkOrders { get; set; }

}
public class WorkOrder
{
    public virtual ICollection<DelayReason> DelayReasons { get; set; }
 //other fields
}

Each DelayReason has 0..* WorkOrders and it's the same for WorkOrders.

This is a data sample:

To do this, I've written this code. Can I optimize that?

  public void DelayReasonAverage()
    {
        List<ReportVM> result = new List<ReportVM>();

        var reasons = _delayReasons.GroupBy(row => row, (k, t) => new
        {
            delayId = k.Id,
            delayTitle = k.Title,
            workOrders = k.WorkOrders,
            count = t.Count()
        }).OrderByDescending(row => row.count).ToList();

        reasons.ForEach(row =>
        {
            var workOrders = row.workOrders.ToList();

            ReportVM reportObj = new ReportVM() { DelayTitle= row.delayTitle };

            for (int i = 0; i < workOrders.Count; i++)
            {
                if (workOrders.Count - 1 > i)
                    reportObj.Count += workOrders[i].CreatedOn.DiffDays(workOrders[i + 1].CreatedOn);
            }
            reportObj.Count = reportObj.Count / row.count;
            result.Add(reportObj);
        });
    }

DiffDays extension Method:

 public static int DiffDays(this DateTime current,DateTime to)
 {
    return to.Subtract(current).Days;
 }

The final results look like this:

While the logic of this:

for (int i = 0; i < workOrders.Count; i++)
{
    if (workOrders.Count - 1 > i)
        reportObj.Count += workOrders[i].CreatedOn.DiffDays(workOrders[i + 1].CreatedOn);
}
reportObj.Count = reportObj.Count / row.count;

is valid.  I think the for loop might not be necessary.  You should be able to take the difference from the first to last:

reportObj.Count = workOrders[0].CreatedOn.DiffDays(workOrders[workOrders.Count - 1].CreatedOn) / row.count;

In looking at the rest of that method it looks like the LINQ queries can be simplified quite a bit.  Something like this should work:

public List<ReportVM> DelayReasonAverage()
{
    return (from DelayReason dr in _delayReasons
            let row = new
            {
                delayId = dr.Id,
                delayTitle = dr.Title,
                workOrders = dr.WorkOrders.ToList(),
                count = _delayReasons.Count()
            }
            orderby row.count descending
            let reportObj = new ReportVM()
            {
                DelayTitle = row.delayTitle,
                Count = (int)Math.Round(row.workOrders[0].CreatedOn.DiffDays
                (row.workOrders[row.workOrders.Count - 1].CreatedOn) / (double)row.count)
            }
            select reportObj).ToList();
}

I noticed your post has this method as void, but it's creating a list, so I changed the signature to reflect that.  I also changed the way the average is calculated, by using Math.Round to get a more accurate reflection of the actual average.


Get the length of data within a buffer (by getting the position of the last byte with data) Background. I start out with a completely empty byte[4096]. As data streams into my application, the byte[4096] gets filled with data starting from first position.

This results in a byte array that is partially filled, but mostly empty, like so...

                0  1  2  3  4  5  6  7  8                       4096
byte[4096] --> [+][+][+][+][+][+][+][+][+][ ][ ][ ][ ][ ][ ] ... [ ]

[+] = Data
[ ] = Empty

I am trying to quickly obtain the position of the last byte with data.

My Question for Code Review

I'm looking for feedback for performance and possibly any correctness in unanticipated cases of this code block:

/// <summary>
/// Get the position of the last byte with data
/// </summary>
public static int GetPositionOfLastByteWithData(this byte[] array)
{
    int i = array.Length - 1;
    // find the end of the buffer by iterating backwards
    while (array[i] == 0) --i;

    return i;
}

Note: This method will throw an exception in the event that the byte[] passed in is totally blank/empty. In the actual use, I do validate that there is data in the byte array before it gets to this method. This method is aimed squarely for high performance.

If your array is completely "empty" your current method will throw an exeption because it will try to get array[-1].

Just use a for loop starting at 0 and if you find the first 0 return index-1. For the case that the whole buffer is filled return array.Length - 1.

Update:

As it seems it is possible that there are 0 values in between valid data like [+][+][+][+][ ][ ][ ][+][+][+][ ]...[ ]. Hence going from start to end of the array doesn't seem a reliable action.

Using a for loop instead of using the while loop enables your code to do the empty check as well like so

public const int NotFound = -1;
public static int GetPositionOfLastByteWithData(this byte[] array)
{
    for (int i = array.Length - 1; i > -1; i--)
    {
        if (array[i] > 0) { return i; }
    }
    return NotFound;
}

As you are talking about streams in your question, wouldn't it be better to store the index of the last written byte ? This would be the easiest and the most safe variant because, assuming you are reading exactly 10 bytes from the stream and later on you need exactly these 10 bytes to restore the read data. If the last byte of that stream would be a 0 like
[+][+][+][+][+][+][+][+][+][ ][ ][ ][ ][ ]...[ ]

both methods, yours and mine, would return 8 but the really read byte count would be 9.


Shell script to archive files to AWS S3 This is a simple script that moves files to S3 if they are > 14 days old. It ignores files that are < 14 days old. If the file is successfully synced to AWS S3 then we can remove it from the server. If not, we leave the file where it is. The echo commands in the script are for debugging. I am aware that this could be a one liner, so interested to see what people suggest.

#!/bin/bash

fortnight=$(python -c "from datetime import datetime, timedelta; print (datetime.now()-timedelta(days=14)).strftime('%Y%m%d')")

for f in /var/mail/catchall-????????.gz
do
    if [[ "$f" < "/var/mail/catchall-$fortnight.gz" ]] ; then
        mv "$f" ./archive
        echo "$f"
        echo "$(basename $f)"
        cd ./archive
            if s3cmd sync "$(basename $f)" s3://vpcgwmail ; then
                echo "S3 sync successful, removing file"
                rm "$(basename $f)"
            else
                echo "s3 sync not succesful"
            fi
        fi
done

It's a fine script, but you have a bug: executing cd ./archive in a loop.
Perhaps you did not notice it because you tested with only a single matching file. If there are multiple matching files, then cd ./archive will not work, unless you have multiple nested archive/archive/archive/... directories, which would be silly.

One solution is to wrap the cd command and the rest of the loop body in a (...) subshell:

if [[ "$f" < "/var/mail/catchall-$fortnight.gz" ]] ; then
    mv "$f" ./archive
    echo "$f"
    echo "$(basename $f)"
    (
        cd ./archive
        if s3cmd sync "$(basename $f)" s3://vpcgwmail ; then
            echo "S3 sync successful, removing file"
            rm "$(basename $f)"
        else
            echo "s3 sync not succesful"
        fi
    )
fi

Another solution is to not cd ./archive at all:

if [[ "$f" < "/var/mail/catchall-$fortnight.gz" ]] ; then
    mv "$f" ./archive
    echo "$f"
    echo "$(basename $f)"
    if s3cmd sync ./archive/"$(basename $f)" s3://vpcgwmail ; then
        echo "S3 sync successful, removing file"
        rm ./archive/"$(basename $f)"
    else
        echo "s3 sync not succesful"
    fi
fi

Whichever alternative is fine.

The indentation was also a bit off, from about the middle of the for-loop body.


Updating an inventory with R using apply functions I have created an inventory that works via QR codes. Briefly: the QR code codes for an email with the book/student checking it out. The email is downloaded into R using the gmailR package (code not shown). The info from the email is taken and added to a table which is compared to a master table (the inventory) and the changes are then made accordingly to update the master table.

The updating works by looking to see if the book is already IN or OUT and then simply flipping it to the opposite. And if it is being checked in the student and the date are erased (switched back to NA).

My original approach to updating the table was to use a function from the apply family but the problem I ran in to was that I do not want to change ALL rows in the master table but only the ones that need to be updated. I could not figure out a way to do this without using a for loop. Is there a way to write this code more efficiently, perhaps using apply, or via any other vectorized functions? Also, any other suggestions about my general design strategy/approach for this inventory would be appreciated.

(By the way, I know I should probably use the date class for the date column but don't worry about that for now).

## Sample data.
# new = List of books to update, the date, and student name.
# master = The inventory
new <- structure(list(book = structure(1:3, .Label = c("Almost moon", "Ava my story", "Catching fire"), class = "factor"), date = structure(c(1L, 1L, 1L), .Label = "8/23/15", class = "factor"), student = structure(1:3, .Label = c("John", "Mary", "Sue"), class = "factor")), .Names = c("book", "date", "student"), row.names = c(NA, -3L), class = "data.frame")
master <- structure(list(book = c("A trick I learned from dead men", "Almost moon", "Austin monthly july 2013", "Ava my story", "Becoming jane austen", "Bossypants", "Catching fire", "Cold mountain", "Comfort food", "Confessions of a jane austen addict"), author = c("Aldridge", "Sebold", "Various", "Gardner", "Spence", "Fey", "Collins", "Frazier", "Jacobs", "Rigler"), status = c("IN", "IN", "IN", "IN", "IN", "IN", "IN", "IN", "IN", "IN"), student = c(NA, NA, NA, NA, NA, NA, NA, NA, NA, NA), date = c(NA, NA, NA, NA, NA, NA, NA, NA, NA, NA)), .Names = c("book", "author", "status", "student", "date"), row.names = c(NA, 10L), class = "data.frame")

## Update inventory
if(sum(new[,1] %in% master$book) == length(new[,1])) {
    matches <- which(master$book %in% new[,1])
    for(i in matches) {
        if(master[i, 3] == "IN") {
            master[i, 3] <- "OUT"
            master[i, 4] <- as.character(new[new$book == master[i, 1], "student"])
            master[i, 5] <- as.character(new[new$book == master[i, 1], "date"])
        } else if(master[i, 3] == "OUT") {
            master[i, 3] <- "IN"
            master[i, 4] <- NA
            master[i, 5] <- NA
        }
    }
} else {
    stop("At least one book not found in database")
}

You can indeed use vectorization. Create two vectors of indices (or booleans like I did) to identify the rows of the master that need to be updated. To find the corresponding row from the new, you can use the match function like I did.

## Update inventory
if (all(new$book %in% master$book)) {
   idx.map       <- match(master$book, new$book)
   is.match      <- master$book %in% new$book
   checking.out  <- is.match & master$status == "IN"
   checking.in   <- is.match & master$status == "OUT"
   master$status[checking.out]  <- "OUT"
   master$status[checking.in ]  <- "IN"
   master$student[checking.out] <- as.character(new$student[idx.map[checking.out]])
   master$student[checking.in ] <- NA
   master$date[checking.out]    <- as.character(new$date[idx.map[checking.out]])
   master$date[checking.in ]    <- NA
} else {
   stop("At least one book not found in database")
}

You will notice another improvement: everywhere I am referring to columns using their name, for example, master$student instead of their index like master[..., 3]. It makes the code more readable and easier to maintain (Imagine what would happen if suddenly a column was inserted at the beginning of master: you would have to modify all your indices...)


Finding the names of students with the second-lowest score I'm teaching myself programming.

I would like to know how I could make this Python3 could more flexibile, for example if I wanted to find students with the 3rd lowest grade, or the lowest, or any other value. Based on this piece of code (that it took me awhile to fathom out) I'm not convinced Python is the best tool for this task.
It uses the data structure it does because the task brief I was working on requested that I use nested lists.

#! /usr/bin/env python3

# Number of student records to expect
n = int(input())

# Take inp of n student records, store in format [[name, score], [name, score], ...]
students = [[input(), float(input())] for studentrecord in range(n)]

# Sort records by score, ascending. Using float else defaults to lexicographical order.
students.sort(key=lambda studentrecord: float(studentrecord[1]))

# Get the lowest mark achieved
lowest_mark = students[0][1]

# Get ind of second lowest scoring student
ind = 0
while students[ind][1] == lowest_mark:
    ind += 1

# Get the second lowest mark achieved
second_lowest_mark = students[ind][1]

second_lowest_scoring_students = []

# Accumulate list of second lowest scoring students.
while students[ind][1] == second_lowest_mark:
    second_lowest_scoring_students.append(students[ind][0])
    ind += 1
    if ind == n: # If iteration reaches the end of the list of records, end.
        break

# Sort name/s alphabetically.
second_lowest_scoring_students.sort()

# Send results to stdout.
for student_name in second_lowest_scoring_students:
    print(student_name)

Your code seems reasonable, if you consider the use of nested lists to be a requirement.  It is clearly written enough that you could just omit most or even all of the comments.  You could simplify the determination of the second-lowest score using itertools.groupby().

It is customary to use _ as the name of a "throw-away" variable whose value will never be used, such as studentrecord in

students = [[input(), float(input())] for studentrecord in range(n)]

I wouldn't blame Python for being the wrong language for this challenge.  Rather, the advice to use nested lists is poor.  This problem would be much more naturally solved if you classified the students by their score.  That would be easily accomplished using a dictionary, specifically a defaultdict(list).

from collections import defaultdict

students_by_score = defaultdict(list)
for _ in range(int(input())):
    name = input()
    score = float(input())
    students_by_score[score].append(name)

second_lowest_score = sorted(students_by_score)[1]
for student_name in sorted(students_by_score[second_lowest_score]):
    print(student_name)

If I had to use a nested list in the solution, I'd still convert it to a dictionary first.  =)

Furthermore, I would consider a list of tuples to be more appropriate than a list of lists for this situation.  A list is good for homogeneous data of variable length.  A tuple, on the other hand, has a connotation of being a fixed number of elements, possibly of different data types.  (Tuples in Python also happen to be immutable.)  Although the nested list works just fine, and the Python interpreter doesn't care, I would choose tuples here because they would convey the right connotation.


StringBuilder: To Build or Not to Build StringBuilder class is a common class that many have become accustom to, including me.

An individual posted a question regarding if there was a comparable method in typescript. Several solutions were mention or linked, but none contained a class. Here is a small class that embodies some of the common StringBuilder methods.

In addition, when joining numeric strings, special care must be taken with the normal typescript/javascript operators. The StringBuilder class will treat all as strings and not attempt a numeric conversion and add values.

It can be expanded upon greatly to fit your needs.

export class StringBuilder
 {
    strArray: Array<string> = new Array<string>();
    constructor()
    {

    }
    Get(nIndex:number): string
    {
      let str:string = null;
      if( (this.strArray.length > nIndex) && (nIndex >= 0) )
      {
         str = this.strArray[nIndex];
      }
      return(str);
    }
    IsEmpty(): boolean
    {
       if(this.strArray.length == 0) return true;
       return(false);
    }
    Append(str: string): void
    {
        if(str != null)
           this.strArray.push(str);
    }
    ToString(): string
    {
        let str:string = this.strArray.join("");
        return(str);
    }

    ToArrayString(delimeter: string): string
    {
        let str:string = this.strArray.join(delimeter);
        return(str);
    }

    Clear()
    {
        this.strArray.length = 0;
    }
 }

In JavaScript/TypeScript it is custom to place the opening braces at the end of the line instead of on a new line by itself.

Insert a space between control statements and the opening round brackets. (Also don't put round brackets around the expression of a return statement.)

Always use braces even when you only have one line after the statement.

Example:

if (str != null) {
   this.strArray.push(str);
}

Always have a space between the colon and type.

Method names should start with a lower case letter, especially since you should be overriding toString.

There should be a blank line between methods.

The empty constructor is pointless and should be left out.

The Get method is not really a good idea. The fact that your string builder uses an array internally shouldn't influence the interface of the class.

If you do have a Get method like this, then you should consider emulating the signature of a regular array and return undefined instead of null, when the index is out of bounds. That actually would save you the checking of the bounds yourself:

get(nIndex: number): string {
    return this.strArray[nIndex];
}

In IsEmpty the expression this.strArray.length == 0 (should be a === BTW) returns a boolean, so you can return that directly:

isEmpty(): boolean {
    return this.strArray.length === 0;
}

Also this may not be correct way to determine an empty StringBuilder. If the array contained only empty strings, shouldn't the StringBuilder still be consider empty? But this could be covered by the next point.

Append should not only ignore null as a parameter, but also undefined and empty strings, so just do:

append(str: string): void {
    if (!str) {
        this.strArray.push(str);
    }
}

ToArrayString doesn't seem to be a good method name. I'd suggest something like joinToString. Also there is no need to use a variable in there.

joinToString(delimeter: string): string {
    return this.strArray.join(delimeter);
}

And avoid a little bit of duplicate code by having toString call ToArrayString/joinToString.

toString(): string {
    return this.joinToString("");
}