Python/Pandas code creating 12 month product balance forecast I have an input DataFrame with one row per product with balances, rates, etc. I want to be able to take that starting balance, and other inputs from the row (characteristics of the product), and forecast out 12 months of balances.  What is the best approach to turn this one row per product information (there are a lot of individual products) and turn it into a summary forecast 12 months forward looking?  The below is an example of one type of product that would have to be modelled.

The below code works, but I would like to know whether there is a better way overall to create forecasts with scenarios using python, pandas or numpy.

Code that gets me from one to the other:

Creates generator function that creates Ordered Dict, when called in pandas DataFrame outputs the example output.

EDIT: Added imports and sample dataframe

# Add relevant packages
import pandas as pd
pd.options.display.float_format = '{:.2f}'.format
import numpy as np
import datetime as dt
from collections import OrderedDict
from dateutil.relativedelta import relativedelta
%matplotlib inline

# SAMPLE DATA

df = pd.DataFrame({
 'balance': {1: 1500, 2: 700},
 'freq': {1: 999, 2: 999},
 'maturity': {1: '2018-01-31', 2: '2018-01-31'},
 'period': {1: 'months', 2: 'months'},
 })

def dmd_flow(balance, start_date, num_periods, run_off_rate, new_funds_rate, int_rate, period='monthly'):

    """
    Implements the cash flow modeling for demand products

    Arguments:
    balance -- initial balance of product at t0
    start_date -- calendar date of t0
    num_periods -- number of period to model
    run_off_rate -- annualized rate at which deposit runs off- straight line
    new_funds_rate -- annualized rate at which funds increase (net)
    int_rate -- rate paying on deposit which would be used if modeling interest payable
    period -- the period used for modeling i.e. monthly, annual

    Returns:
    liq_sched -- a schedule of liquidity based on the inputs ordered by time (ordered dictionary)
    """

    p=1
    b_bal = balance
    e_bal = balance
    current_date = start_date

    while p <= num_periods:
        if period == 'monthly':
            e_bal = round(b_bal - (balance*1/12*run_off_rate), 4)
            e_bal += round((balance*1/12*new_funds_rate), 4)
        if period == 'annual':
            e_bal = round(b_bal - (balance*run_off_rate), 4)
            e_bal += round((balance*new_funds_rate), 4)

        yield(OrderedDict([('Month', current_date),
                           ('Period', p),
                           ('Beg Bal', b_bal),
                           ('End Bal', e_bal),
                          ]))
        p += 1
        current_date += relativedelta(months=+1, day=31)
        b_bal = e_bal

def create_timeseries_df(input=None, func=None, args=(dt.date(2018,1,31), 0, 0, 0, 0, 'monthly')):
    '''takes a series as an input and applies the fuctions to create the timeseries in the right shape'''
    new_df = input['balance'].apply(
     func, args=args)
    return pd.DataFrame([o_dict for gen in new_df for o_dict in gen])

out_df = create_timeseries_df(df, func=dmd_flow, args=(dt.date(2018,1,31), 12, 0, .10, .03, 'monthly'))
out_df.groupby(['Month', 'Period'])[['Beg Bal', 'End Bal']].sum()

Example input DF (can use sample DF included above, and run in order to get the resulting output:

Example Output Created using code:

The most confusing part in your code is that new_df in create_timeseries_df is actually a pd.Series full of generators! It took me a while to understand what is actually going on there. What was the point of using them in the first place? I don't think you win anything from it.

I also don't understand what was the point in using OrderedDict. Usual dict should be enough.

p=1
while p <= num_periods:
    ...
    p += 1

This is not a Pythonic way. Instead you should do it like this:

for p in range(num_periods):
   ...  # Don't forget that p will start from 0, not 1

if period == 'monthly':
    e_bal = round(b_bal - (balance*1/12*run_off_rate), 4)
    e_bal += round((balance*1/12*new_funds_rate), 4)
if period == 'annual':
    e_bal = round(b_bal - (balance*run_off_rate), 4)
    e_bal += round((balance*new_funds_rate), 4)

No need to repeat twice. No need to repeat twice. For example, if you decide to change precision from 4 to 5 then you have to change it in 4 places!
See "Why is DRY important?" for explanations why such code is bad.
I suggest to rewrite it to something like this (not sure about the dict with coefficients though):

term_coefficients = {'monthly': 1 / 12,
                     'annual': 1.}
precision = 4
...
coefficient = term_coefficients[period]
e_bal = round(b_bal + balance * coefficient * (new_funds_rate - run_off_rate),
              ndigits=precision)

Actually you don't need dateutil.relativedelta to get desired timeseries. Pandas already has this kind of functionality. Check this out:

pd.DataFrame({'Month': pd.date_range(start=start_date,
                                     periods=periods_count,
                                     freq='M')})

Your column Period is actually just an index column + 1:

some_df['Period'] = some_df.index + 1

I don't know if you really need this column at all because you already have column 'Month' that you can use for grouping data.

So, I decided to refactor your code. I split the logic in several functions in a different way than you did. I added type hints and changed format of a docstring to reST (see list of different formats). Unfortunately, I have no idea how to name the functions and some variables. It's up to you to name them properly.
If you have any questions, I will add explanations to my answer.

import datetime as dt
from typing import List

import pandas as pd

pd.options.display.float_format = '{:.2f}'.format

TERM_COEFFICIENTS = dict(monthly=1 / 12,
                         annual=1.)

def get_final_result(*,
                     balances: List[float],
                     start_date: dt.date,
                     periods_count: int,
                     new_funds_rate: float,
                     run_off_rate: float,
                     precision: int = 4.,
                     term: str = 'monthly'):
    """
    Implements the cash flow modeling for demand products
    :param balances: initial balances of product at t0
    :param start_date: calendar date of t0
    :param periods_count: number of period to model
    :param run_off_rate: annualized rate
    at which deposit runs off - straight line
    :param new_funds_rate: annualized rate
    at which funds increase (net)
    :param term: the period used for modeling
    i.e. monthly, annual
    :param precision: number of digits in final balance
    :return: a schedule of liquidity
    based on the inputs ordered by time
    """
    output_df = pd.DataFrame()

    for balance in balances:
        df = get_df(balance=balance,
                    start_date=start_date,
                    periods_count=periods_count,
                    new_funds_rate=new_funds_rate,
                    run_off_rate=run_off_rate,
                    precision=precision,
                    term=term)
        output_df = output_df.append(df)

    return output_df.groupby(
            ['month', 'period'])[['start_balance', 'end_balance']].sum()

def get_df(*,
           balance: float,
           start_date: dt.date,
           periods_count: int,
           run_off_rate: float,
           new_funds_rate: float,
           precision: int,
           term: str) -> pd.DataFrame:
    df = pd.DataFrame(dict(month=pd.date_range(start=start_date,
                                               periods=periods_count,
                                               freq='M')))
    df['period'] = df.index + 1

    terms_balances = get_balances(balance,
                                  periods_count=periods_count,
                                  term_coefficient=TERM_COEFFICIENTS[term],
                                  run_off_rate=run_off_rate,
                                  new_funds_rate=new_funds_rate,
                                  precision=precision)

    df['start_balance'] = terms_balances[:-1]
    df['end_balance'] = terms_balances[1:]

    return df

def get_balances(balance: float,
                 *,
                 periods_count: int,
                 term_coefficient: float,
                 run_off_rate: float,
                 new_funds_rate: float,
                 precision: int) -> List[float]:
    balances = [balance]
    for _ in range(periods_count):
        balances.append(round(get_end_balance(
                balance,
                start_balance=balances[-1],
                term_coefficient=term_coefficient,
                run_off_rate=run_off_rate,
                new_funds_rate=new_funds_rate),
                ndigits=precision))
    return balances

def get_end_balance(balance: float,
                    *,
                    start_balance: float,
                    term_coefficient: float,
                    run_off_rate: float,
                    new_funds_rate: float) -> float:
    return (balance * term_coefficient * (new_funds_rate - run_off_rate)
            + start_balance)

if __name__ == '__main__':
    data = pd.DataFrame({
        'balance': {
            1: 1500,
            2: 700},
        'freq': {
            1: 999,
            2: 999},
        'maturity': {
            1: '2018-01-31',
            2: '2018-01-31'},
        'period': {
            1: 'months',
            2: 'months'},
        })

    print(get_final_result(balances=data['balance'].tolist(),
                           start_date=dt.date(2018, 1, 31),
                           periods_count=12,
                           run_off_rate=0.,
                           new_funds_rate=0.1,
                           precision=4))


Conway's Game of Life Java Implementation Keep state the same if it doesn't fall in the following conditions:

Change to 2 if there are exactly 2 ones in the neighbors
Change to 3 if there are more than 2 ones in the neighbors

It is also wrapping around so neighbors can be cyclic.

I wrote a basic structure and modularized it to individual functions. I wish to understand better coding techniques and how I can improve.

package code.Misc;
public class changeStateGame {
public static void main(String args[]){

    int current[][] = {
            {0,0,0,0,0,0,0},
            {0,0,0,0,1,0,0},
            {0,0,0,1,1,0,0},
            {0,0,1,1,0,0,0},
            {0,0,0,0,0,0,0}
    };
    int aux [][] = new int [current.length][current[0].length];

    for(int i=0;i<current.length;i++){
        for(int j=0;j<current[0].length;j++){
            aux[i][j]=0;
        }
    }

    System.out.println("Current State");
    printStateGame(current);
    System.out.println("Changed State");
    printStateGame(changeState(current,aux,0,aux[0].length));

}

public static void printStateGame(int a[][]){
    for(int i=0;i<a.length;i++){
        for(int j=0;j<a[0].length;j++){
            System.out.print(a[i][j]+" ");
        }
        System.out.println();
    }
}

public static int [][] changeState(int c[][],int a[][], int row, int col){

    for(int i=0;i<a.length;i++){
        for(int j=0;j<a[0].length;j++){
            a[i][j]=checkNeighbors(c,i,j);
        }
    }
    return a;
}

public static int checkNeighbors(int a[][],int row,int col){

        int neighborCount = 0;
        for(int i=row-1;i<row+1;i++){
            for(int j=col-1;j<col+1;j++){

                        if (i < 0)
                            i = a.length - 1;
                        if (j < 0)
                            j = a[0].length - 1;
                        if (i == a.length)
                            i = 0;
                        if (j == a[0].length)
                            j = 0;
                        if(i==row){
                            if(j==col){
                                continue;
                            }
                        }
                        if(j==col){
                            if(i==row){
                                continue;
                            }
                        }
                        neighborCount += checkElement(a[i][j]);

            }
        }
    return modifyElement(a[row][col],neighborCount);
}
public static int checkElement(int a){
    if(a==1)
        return 1;
    else
        return 0;
}

public static int modifyElement(int a, int count){
    if(count<2){
        return a;
    }
    else if (count == 2){
        return 2;
    }
    else {
        return 3;
    }
}
}

Bug 1

Your neighbor counting loops are only iterating twice instead of three times, so you aren't counting neighbors to the right or below.

Bug 2

If you were to correct Bug 1, your program would infinite loop in checkNeighbors because you modify your loop variables inside the loop when they wrap around.  So for example, if col is the rightmost column, you will set j to 0 inside the loop which will cause the j loop to never terminate.

Bug 3

You sometimes set your array elements to 2 or 3.  But in Life, each array element should be 0 or 1 only.  Your modifyElement function is incorrect.

Dead code

These two blocks of code are identical, so the second is useless:

                    if(i==row){
                        if(j==col){
                            continue;
                        }
                    }
                    if(j==col){
                        if(i==row){
                            continue;
                        }
                    }

Unused arguments

The changeState function has two unused arguments: row and col.


Calculating highs, lows, and volumes of stocks I'm trying to review and figure the issues in the above code. Any inputs to point out problems are welcome.

using namespace std;
typedef basic_string<char> string;

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

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

        if (nLow < nCurLow)
            nCurLow = nLow;
    }

    int     nCurLow;
    int     nCurHigh;
};

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

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

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

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

    int     i = 0;
    int     sum = 0;

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

        cerr << Stocks[i] << endl;
        cerr << Intervals[i] << endl;
        cerr << Volumes[i] << endl;
        cerr << Highs[i] << endl;
        cerr << Lows[i] << endl;
    }

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

    map<std::string, int>       TotalVolumes;

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

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

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

    map<std::string, CHighLow>  HighLows;

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

    cerr << "Writing files" << endl;

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

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

        ++itr;
    }

    return 1;
}

First, the obvious ones:

Change nCurLow and nCurHigh to floats since you are reading float from file.
Change

nCurLow(0)

to

nCurLow(std::numeric_limits<float>::infinity())

and

nCurHigh(0)

to

nCurHigh(-std::numeric_limits<float>::infinity())

because initializing min/max with 0 can lead to bugs if values are all >0 or <0.
Drop typedef basic_string<char> string; and use std::string instead.
Check argc if you have arguments before you use argv (prevents crash when invalid arguments are passed).
Don't use cerr only when you want to report error, use std::cout instead.
Don't increment i like this: ... &Lows[i++]);. In fact drop those arrays all together and use std::vector, and std::string instead (because that while so broken in so many ways ...).
The iteration looks bogus, for (int s = 0; s <= i; ++s) because of <= so change it to ranged-based fors.
Avoid stdcmp; use operator== from std::string.

These points only refer to potential errors and bugs.

Now for the less obvious ones:

I see a comment //write to a file. std::cout does not write to file but to standard output. use file streams if writing to a file is the intended behavior
This code:

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

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

What does it actually do? It looks totally inefficient. The obvious thing is that TotalVolumes[stockname] can be evaluated only once. The second is that j can be equal to s.
The rest of the code seems to make excessive use of memory. It looks like it can be simplified a lot, with fewer iterations, less memory usage, less confusion.
It would be nice if there would be some comments to describe what the code actually does.
return 1 in main. If this will be used in an automated tool return 1 might mean that an error has occurred. Change it to return 0 and return 1 only when an actual error has happened (invalid argument, file not found, corrupted file data, etc).
More error checking.

There could be other things, but having some test data would be nice. There could be soft bugs in the code.


HumanDate for easier date calculations I need to calculate some dates like yesterday, this week/month, last three/ten days etc.

To make it a little bit easier and don't think about the Date weirdness all the time I created this helper:

var HumanDate = function() {

  var date = new Date();

  this.day = function(numOfDays) {
    if (numOfDays === undefined) {
        return date.getDate();
    }
    else {
        date.setDate(date.getDate() + numOfDays);
        return this;
    }
  };

  this.month = function(month) {
    if (month === undefined) {
      return date.getMonth() + 1;
    }
    else {
      date.setMonth(date.getMonth() + month);
      return this;
    }
  };

  this.year = function() {
    return date.getFullYear();
  };

  this.dayOfWeek = function() {
        var dayOfWeek = date.getDay();
    return dayOfWeek === 0 ? dayOfWeek + 1 : dayOfWeek;
    };

  this.beginningOfTheWeek = function() {
    return this.day(-this.dayOfWeek() + 1);
  }

  this.endOfTheWeek = function() {
    return this.day(7 - this.dayOfWeek());
  }

  this.beginningOfTheMonth = function() {
    return this.day(-this.day() + 1);
  }

  this.endOfTheMonth = function() {
    return this.day(7 - this.dayOfWeek());
  }

  this.toJSDate = function(format) {
    return date;
  }

};

Examples:

console.log('day: ' + new HumanDate().day());
console.log('month: ' + new HumanDate().month());
console.log('year: ' + new HumanDate().year());
console.log('day of week: ' + new HumanDate().dayOfWeek());

console.log('yesterday: ' + new HumanDate().day(-1).day());
console.log('next month: ' + new HumanDate().month(1).toJSDate());
console.log('last month: ' + new HumanDate().month(-1).toJSDate());
console.log('beginning of the week: ' + new HumanDate().beginningOfTheWeek().toJSDate());
console.log('end of the week: ' + new HumanDate().endOfTheWeek().toJSDate());
console.log('beginning of the month: ' + new HumanDate().beginningOfTheMonth().toJSDate());
console.log('end of the week: ' + new HumanDate().endOfTheWeek().toJSDate());

Output:

day: 7
month: 9
year: 2016
day of week: 3
yesterday: 6
next month: Fri Oct 07 2016 20:04:50 GMT+0200 (W. Europe Summer Time)
last month: Sun Aug 07 2016 20:04:50 GMT+0200 (W. Europe Summer Time)
beginning of the week: Mon Sep 05 2016 20:04:50 GMT+0200 (W. Europe Summer Time)
end of the week: Sun Sep 11 2016 20:04:50 GMT+0200 (W. Europe Summer Time)
beginning of the month: Thu Sep 01 2016 20:04:50 GMT+0200 (W. Europe Summer Time)
end of the week: Sun Sep 11 2016 20:04:50 GMT+0200 (W. Europe Summer Time)

I need it for Google Sheets scripting & calculations so I'd rather not use an external library - (at least for now)

Some thoughts:

I find your method naming to be really confusing. To me, if I called methods named .day(), .month(), year(), etc. I would be expecting methods named in this manner to behave more like a getter than a setter.

For example, this usage seems very confusing:

new HumanDate().day(-1).day()

Consider naming like modifyDay(), modifyMonth(), etc.

You have unnecessary else conditions.

For example:

if (numOfDays === undefined) {
    return date.getDate();
}
else {
    date.setDate(date.getDate() + numOfDays);
    return this;
}

Could more clearly be written as:

if (numOfDays === undefined) {
    return date.getDate();
}

date.setDate(date.getDate() + numOfDays);
return this;

Here there is no need for else since if returns.

The dayOfWeek() method looks odd.  Why convert Sunday to Monday in all cases?
toJSDate() seems like an odd name for a function. The word "to" tends to connote cases where some casting or data transformation might take place.  Here you are just returning the underlying date Object. Maybe just getDateObj() or something more descriptive of what is happening. Of course, the caller can always just access the .date property of the object to get the same result.
I think some of the logic you are applying to try force you week to start on a certain day, might best be something you want to specify in configuration on instantiation, or via more appropriate specificity around methods that deal with work week constructs. Just totally re-mapping the days returned by underlying Date object seems to sort of hide this away from the caller.
I think that perhaps you might need to look at the time component here as well.  To me, if I am trying to get the beginning of a week or month, I would be expecting to get 00:00:00 as the time component. On the other hand, if maybe the methods were named something like modifyToBeginningOfMonth() perhaps the current behavior would be more expected.
Consider passing the Date object dependency in the constructor.  Right now you limit this class to only working with Date object for current timestamp.  If you took a Date object as parameter, you could set this Date object on your HumanDate object as is currently done, but allow the caller to fully utilize the capabilities of Date to set the timestamp to whatever they want to start with.  This also makes the dependency of this class really clear.
You might give consideration to defining this class more as an extension of the underlying Date class.  That way you can eliminate needing to wrap underlying date properties and methods.


Simple way to prevent duplicate logging of errors I've been tasked with updating our current error logging system to prevent the logging of similar errors (from a single client program) that were probably caused by the same event. The code here is what I have added in order to accomplish this. Are there any obvious issues I am missing or is there a simpler way to accomplish this? I need to deal with multiple threads.

Here is the simple error object we use:

public class ErrorObject
{
    //to use when sending an object to a background worker
    public ErrorObject(Exception exception, string additionalinfo,DateTime? dateTime = null,bool showTouser = false)
    {
        Exception = exception;
        AdditionalInfo = additionalinfo;
        Time = dateTime ?? DateTime.Now;
    }

    public Exception Exception { get; set; }
    public string AdditionalInfo { get; set; }
    public DateTime Time { get; set; }
    public bool ShownToUser { get; set; }
}

Other code calls this function to log errors.

public static Int32 LogError(Exception ex, bool runInBackground = true, string additionalInfo = "",
    bool ShownToUser = false)
{

    int retVal = -1;
    if (runInBackground)
    {
        ErrorObject newError = new ErrorObject(ex,additionalInfo,DateTime.Now,ShownToUser);

        ThreadPool.QueueUserWorkItem(LogErrorInBackground, newError);
    }
    else
    {
        try
        {   //Do a Sql Insert in current thread (slow for remote labs...)
            retVal = InsertError(ex, additionalInfo, ShownToUser);
        }
        catch{}
    }
    return retVal;
}

//Store 4 previous errors to compare against
private static Queue<ErrorObject> PreviousErrors = new Queue<ErrorObject>(4);
private static object PreviousErrorLock = new object();

private static void LogErrorInBackground(object state)
{
    ErrorObject error = (ErrorObject) state;

    if (! QueueNewError(error))
    {
        InsertError(error.Exception, error.AdditionalInfo, error.ShownToUser);
    }
}

/// <summary>Adds an error to the PreviousErrors Queue and removes the error at the front of the list.</summary>
/// <returns>Returns true if the error is a duplicate of any other previous errors in the queue.</returns>
private static bool QueueNewError(ErrorObject newError)
{
    //Assume the new error is not a duplicate
    bool isDuplicate = false;

    lock (PreviousErrorLock)
    {

        foreach (ErrorObject previousError in PreviousErrors)
        {
            if (previousError.AdditionalInfo == newError.AdditionalInfo &&
            Math.Abs((previousError.Time - newError.Time).TotalSeconds) < 3 &&
            previousError.Exception.GetType() == newError.Exception.GetType() &&
            previousError.Exception.Message == newError.Exception.Message &&
            previousError.Exception.TargetSite == newError.Exception.TargetSite)
            {
                isDuplicate = true;
                break;//Exit the loop
            }
        }

        if( PreviousErrors.Count > 0)
            PreviousErrors.Dequeue();
        PreviousErrors.Enqueue(new ErrorObject(newError.Exception, newError.AdditionalInfo, newError.Time));
    }
    return isDuplicate;
}

ErrorObject newError = new ErrorObject(…);

This is one of the cases where most people will probably agree that you can use var here. Doing that doesn't lose any information (you can still see at a glance that newError is an ErrorObject) and it makes the code shorter and more DRY.

return retVal;

In general, you shouldn't use integer retVals. If you need to report errors, use exceptions. If you want to report success or failure, use a bool. If you want to report multiple possible results, use an enum. If you want to report some kind of count (though I don't see how would that make sense here), at least name your variable something like count, not retVal.

//Store 4 previous errors to compare against
private static Queue<ErrorObject> PreviousErrors = new Queue<ErrorObject>(4);

This sounds like a job for a simple reusable custom collection. Something like:

class BoundedQueue<T> : IEnumerable<T>
{
    private readonly int count;
    private readonly Queue<T> queue;

    public BoundedQueue(int count)
    {
        this.count = count;
        this.queue = new Queue<T>(count);
    }

    public void Enqueue(T item)
    {
        if (queue.Count == count)
            queue.Dequeue();

        queue.Enqueue(item);
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }

    public IEnumerator<T> GetEnumerator()
    {
        return queue.GetEnumerator();
    }
}

lock (PreviousErrorLock)

Since you want to lock access to the PreviousErrors object, you should use that in the lock, you don't need another object for this. (An object used just for locking makes sense when you logically want to do something like lock (this), but that's not the case here.)

foreach (ErrorObject previousError in PreviousErrors)
{
    if (previousError.AdditionalInfo == newError.AdditionalInfo &&
    Math.Abs((previousError.Time - newError.Time).TotalSeconds) < 3 &&
    previousError.Exception.GetType() == newError.Exception.GetType() &&
    previousError.Exception.Message == newError.Exception.Message &&
    previousError.Exception.TargetSite == newError.Exception.TargetSite)
    {
        isDuplicate = true;
        break;//Exit the loop
    }
}

You should extract this equality testing code into a separate method, possibly as ErrorObject.Equals(). With that, this code would simplify to:

bool isDuplicate = PreviousErrors.Contains(newError);

break;//Exit the loop

You don't need this comment. Everyone knows what break does inside a loop.

Also, I find the queue logic suspicious: when you detect a duplicate, you add it to the queue anyway. So if the sequence of exceptions is something like 1, 2, 2, 2, 2, 1, then you're going to report 1 twice, even though it's among the 2 most recent error kinds.


JavaScript parameters - best practices for settings object/callback I have the following function which runs as expected and is defined in the window object:

function myFunction(url, timeout, type, sc, ec) {
    return $.ajax({
        url: url,
        timeout: timeout,
        type: type,
        success: function (data) {
            if (typeof sc === 'function') {
                sc(data);
            }
        },
        error: function (jqXhr) {
            if (typeof ec === 'function') {
                ec(jqXhr);
            }
        }
    });
}

Which works as expected.

However, I am now expanding this function to be a whole lot more complex. As a result to this, I was wondering whether I could get some style guides (I'm a bit OCD when it comes to things like this in JS).

Additional features I am adding are as follows:

showLoader - the ability to show a modal loader
loaderMessage - the message displayed if showloader === true
storeAs - how to store the downloaded data

The new function now looks as follows:

(function (window, document, $, undefined) {
    function myFunction(settings, sc, ec) {
        var options = $.extend({
            showLoader: false,
            loaderMessage: 'Loading...',
            storeAs: null,
            timeout: 60000,
            url: 'http://www.google.co.uk',
            type: 'GET',
            success: function (data) {
                if (options.storeAs) {
                    localStorage.setItem(options.storeAs, JSON.stringify(data));
                }

                if (typeof sc === 'function') {
                    sc(data);
                }
            },
            error: function (jqXhr) {
                if (typeof ec === 'function') {
                    ec(jqXhr);
                }
            }
        }, settings);

        if (options.showLoader) {
            showLoader(options.loaderMessage); // external function to show loader
        }

        return $.ajax(settings);
    }

    window.app = {
        comms: {
            ajax: function (options, sc, ec) {
                return myFunction(options, sc, ec);
            }
        }
    };
}(this, this.document, jQuery));

Anonymous function

I like to now define all my code within anonymous functions since it frees up the global name space.

However, I have two questions:

Is it recommended/do I need to make use of the JavaScript new keyword? Is there any advantage to doing so?
If so, then how would I go about using it? I have got to grips with the majority of the language, just not the new constructor/keyword?

Parameters

My second change will be to address the parameters - five parameters in JavaScript is quite readable. However, any more isn't.

I am thinking of using a settings object as seen above:

Couple of questions surrounding this however:

Is it wise to use just one options object, as I am already doing here? $.ajax receives this settings object, but my settings object contains not just jQuery.ajax settings, but function settings - obviously jQuery will ignore these since it will use just the necessary options, but is there any reason to avoid this?
I could combine the success callback, and error callback into the options object but this looks quite messy. Is it better to provide the callbacks as separate parameters or move those into the options callback too?

What is the difference between:

var options = $.extend({}, {
    showLoader: false,
    loaderMessage: 'Loading...',
    storeAs: null
}, settings);

And:

var options = $.extend({
    showLoader: false,
    loaderMessage: 'Loading...',
    storeAs: null
}, settings);

Is there anything else I should be doing which I am missing, which is generally recommended in JavaScript?

I would prefer a setting object for each function you want to call instead of merging them together into one, because then it is clear who needs this parameters. Otherwise this information gets lost. And who knows what jquery will do with this options in future releases? Maybe something useful, maybe not.

I think it is okay to add functions to objects but you can it make a little nicer when you define these not directly in the options object, instead only referencing.

(function (window, document, $, undefined) {

    function successHandler
    ...

    function myFunction(settings, sc, ec) {
    ...

      {
         success: successHandler
      }

Also I would prefer real names and no two character abbrevations. That makes the code more readable for you and the rest of the world.


Rock, paper, scissors in Python 3 I am new to Python (and to programming). I tried to implement this game in Python and took some help from the code reviews of similar questions posted on this website.

# rock paper scissors game

import random
import sys

choices = ["rock", "paper", "scissors"]

def check_victory(computer_choice, user_choice):
    if user_choice == computer_choice:
        return "tie"
    elif computer_choice == "rock" and user_choice == "paper":
        return "won"
    elif computer_choice == "paper" and user_choice == "scissors":
        return "won"
    elif computer_choice == "scissors" and user_choice == "rock":
        return "won"
    else:
        return "lost"

def play():
    # get input from user
    user_choice = None
    while user_choice not in choices:
        user_choice = input("Enter your choice: ")
        # check if user wants to quit
        if user_choice == "quit":
            sys.exit()

    computer_choice = random.choice(choices)

    # output result
    print(f"Computer choose: {computer_choice}")
    print("You " + check_victory(computer_choice, user_choice), end="\n\n")

if __name__ == "__main__":
    while True:
        play()
```

overall this code looks nice and tidy, and it passes pylint with minimal errors which is a good start!

Some observations:

If you're returning in an if block you don't need to use elif and/or else since on match the code will never continue.  I'd probably use or instead of 3 elif statements to avoid having the `return "won" repeated. And maybe carry the result in a variable - this allows you to set a default and avoid one test. So something like:

def check_victory(computer_choice, user_choice):
    result = "lost"
    if user_choice == computer_choice:
        result = "tie"
    if computer_choice == "rock" and user_choice == "paper" or \
       computer_choice == "paper" and user_choice == "scissors" or \
       computer_choice == "scissors" and user_choice == "rock":
        result = "won"
    return result

Another thing might be to replace some of the descriptive comments with docstrings - less important in a small program, but a useful habit to get into for more complex projects later on.

Finally since you're using f-strings for the first print, you could use them for both?

print(f"Computer choose: {computer_choice}")
print(f"You {check_victory(computer_choice, user_choice)}\n\n")

Only other things are 'features' like documenting the choices available, providing 'shortcuts' like r/p/s etc - but those are 'optional extras' and nothing to do with the code quality!


Project Euler #211: Divisor Square Sum I started doing hackerrank problems only recently and here's my attempt to solve this problem.

Basically, you're given two integers N and K, so you need to find the sum of all 1 <= n <= N such that the sum is at most K away from a perfect square.

For example, for N = 65 and K = 0, we calculate the sum of squares of factors for all numbers from 1 to 65, i.e. 1, 5, 10, ... and see which of these sums is a perfect square (or more precisely 0 away from a perfect square). For 65, that happens to be 1 and 42, so the answer would be 43.

#include <cmath>
#include <vector>
#include <unordered_map>
#include <iostream>

using namespace std;

// Lookup table
unordered_map<uint_fast16_t, uint_fast16_t> fac_sq_diff;

auto sum_fac_sq(uint_fast16_t n) {
    uint_fast16_t sum = 0;
    // Calculate factors in pairs,
    // eg. 20 => (1, 20), (2, 10), (4, 5)
    auto sqrtn = sqrt(n);
    for (auto i = 1; i <= sqrtn; ++i) {
        if (n % i == 0) {
            sum += (i * i);
            auto j = n / i;
            if (i != j) {
                sum += (j * j);
            }
        }
    }
    return sum;
}

uint_fast32_t solve(uint_fast16_t n, uint_fast16_t k) {
    uint_fast32_t result = 0;
    int_fast32_t diff = 0;
    for (uint_fast16_t j = 1; j <= n; ++j) {
        auto idiff = fac_sq_diff.find(j);
        if (idiff == fac_sq_diff.end()) {
            auto sum = sum_fac_sq(j);
            // Closest perfect square:
            // https://stackoverflow.com/questions/6054909
            auto sqrtsum = static_cast<uint_fast16_t>(sqrt(static_cast<float>(sum)) + .5f);
            diff = sum - (sqrtsum * sqrtsum);
            diff = abs(diff);
            idiff = fac_sq_diff.emplace(j, diff).first;
        }

        if (idiff->second <= k) {
            result += j;
        }
    }
    return result;
}

int main() {
    // Expected input:
    // 11
    // 65 0
    // 269 1
    // 312 2
    // 745 3
    // 1457 4
    uint_fast16_t q = 1;
    cin >> q;

    // Find max N and reserve lookup table mem
    vector<pair<uint_fast16_t, uint_fast16_t>> NK;
    uint_fast16_t max_N = 0;
    for (auto i = 0; i < q; ++i) {
        uint_fast16_t N = 0, K = 0;
        cin >> N >> K;
        NK.emplace_back(N, K);
        max_N = max(N, max_N);
    }
    fac_sq_diff.reserve(max_N);

    for (auto &nk: NK) {
        cout << solve(nk.first, nk.second) << endl;
    }
    return 0;
}

No matter what I try, I am not able to get past the first test (in terms of speed). :(

I also tried the geometric summation formula to calculate the sum of factors squared, but my implementation turned out to be slower than this one.

As always, before tackling the Project Euler problems, do your math homework. Few hints:

Do not brute force. \$\sigma_2(n)\$ is multiplicative. That allows it to be computed in a much more efficient way.
Do not throw away your work. \$\sigma_2(n)\$ does not depend on \$k\$. Tabulate it once toward the maximal possible \$N\$, or extend it as \$N\$ grows.

Besides that, the constraints say \$1 < N < 6\cdot 10^6, 0 < K < 10^6\$. uint_16 is surely insufficient.


JavaScript function for printing a countdown As part of my project, a game, I have progress bars with countdown timers. These are updated every 250ms, to keep a relatively smooth animation if the countdown time is short.

Here's my function:

var time_format = function(time) {
    var elms, names, ret, i, n;
    elms = [
        time / 29030400,
        time / 2419200 % 12,
        time / 604800 % 4,
        time / 86400 % 7,
        time / 3600 % 24,
        time / 60 % 60,
        time % 60
    ];
    names = "yr mo w d h m s".split(" ");
    ret = [];
    for( i=0; i<elms.length; i++) {
        n = Math.floor(elms[i]);
        if( n) ret.push(n+names[i]);
    }
    ret = ret.join(" ");
    if( ret == "") ret = "0s";
    return ret;
};

I'm just wondering if any optimisations can be made to this code. There may be up to ten progress bars on a single page (after that data is paginated). My main concern is that Math.floor call, is there a faster way to floor a number?

Try this:

var time_format = (function() {

    var names = "yr mo w d h m s".split(" ");

    return function(time) {
        var elms, ret, i, n;
        elms = [
            time / 29030400,
            time / 2419200 % 12,
            time / 604800 % 4,
            time / 86400 % 7,
            time / 3600 % 24,
            time / 60 % 60,
            time % 60
        ];
        ret = [];
        for( i=0; i<elms.length; i++) {
            n = elms[i];
            n = n | n;
            if( n) ret.push(n+names[i]);
        }
        ret = ret.join(" ");
        if( ret == "") ret = "0s";
        return ret;
    }
}());

It moves the array initialization out of the function and into a closure so it's only evaluated once.

And, it uses a faster way to do the floor operation.  See this article for different ways to do floor and their timing.


A simple Pygame that has a square move and jump How does this Pygame code look? Is the run loop usually in a class? I'm a beginner and was wondering what the best format and structure is for using OOP in Pygame.

import pygame
pygame.init()

win = pygame.display.set_mode((500, 500))
pygame.display.set_caption("Game")

class Game:

    def __init__(self):
        self.x = 250
        self.y = 250
        self.width = 50
        self.height = 50
        self.vel = 5
        self.isJump = False
        self.jumpCount = 10

    def run(self):
        run = True
        while run:
            pygame.time.delay(30)
            for event in pygame.event.get():
                if event.type == pygame.QUIT:
                    run = False

            keys = pygame.key.get_pressed()

            if keys[pygame.K_LEFT] and self.x > 0:
                self.x -= self.vel

            if keys[pygame.K_RIGHT] and self.x < 500 - self.width:
                self.x += self.vel

            if not self.isJump:
                if keys[pygame.K_UP] and self.y > 0:
                    self.y -= self.vel

                if keys[pygame.K_DOWN] and self.y < 500 - self.height:
                    self.y += self.vel

                if keys[pygame.K_SPACE]:
                    self.isJump = True
            else:
                if self.jumpCount >= -10:
                    self.y -= (self.jumpCount * abs(self.jumpCount)) * 0.5
                    self.jumpCount -= 1
                else:
                    self.jumpCount = 10
                    self.isJump = False

            win.fill((0, 0, 0))
            self.draw()

        pygame.quit()

    def draw(self):
        pygame.draw.rect(win, (0, 0, 255), (self.x, self.y, self.width, self.height))
        pygame.display.update()

g = Game()
g.run()

Few areas to improve:

self.vel. When I first saw this field/variable I thought: "What is that?".I guess many other people may have the same first feeling.I would rename it to an explicit and clear name: self.velocity
self.isJump and self.jumpCount violate Python naming conventions (instance variable names should be all lower case, words in an instance variable name should be separated by an underscore). Therefore, rename them to:

...
self.is_jump
self.jump_count`

run = True flag. That flag for just controlling the while loop flow is redundant.We're just starting the loop with while True: and breaking it with break.So it becomes:

def run(self):
    while True:
        pygame.time.delay(30)
        keys = pygame.key.get_pressed()

        # handling pressed keys
        if keys ...

        win.fill((0, 0, 0))
        self.draw()

        for event in pygame.event.get():
            if event.type == pygame.QUIT:
                break
    pygame.quit()


Advent of Code 2018 day 1 part 2: find the first aggregated number that occures twice I am using the coding puzzels of https://adventofcode.com for improving my F# skills.

Problem:

Day 1 Part 2:

Starting with frequency 0, a list of numbers should be added successive generating new frequencies. Searched is the first frequency which occurs twice. If the end of the list has been reached without success; the list should be processed again.

  Example Input: [+1, -2, +3, +1]

  Current frequency  0, change of +1; resulting frequency  1.
  Current frequency  1, change of -2; resulting frequency -1.
  C* urrent frequency -1, change of +3; resulting frequency  2.
  Current frequency  2, change of +1; resulting frequency  3.
  (At this point, we continue from the start of the list.)
  Current frequency  3, change of +1; resulting frequency  4.
  Current frequency  4, change of -2; resulting frequency  2, which has already been seen.

  In this example, the first frequency reached twice is 2.

Solution

let input = [1;-2;3;1]

type State =
    | FinalState of int
    | IntermediateState of int * int Set

type State with
    static member Zero = IntermediateState(0, Set.empty.Add(0))

let foldIntermediateState (freqHistory: int Set, freq : int) : State =
    let isFinal = freqHistory |> Set.contains freq
    if isFinal then
        FinalState (freq)
    else
        IntermediateState (freq, freqHistory.Add(freq))

let foldState state value =
    match state with
        | FinalState _ -> state
        | IntermediateState (lastFreq, freqHistory) -> foldIntermediateState (freqHistory, lastFreq + value)

let rec processListRec state lst =
    let result = lst |> Seq.fold foldState state
    match result with
        | FinalState result -> printfn "The result is: %i" result
        | IntermediateState _ -> lst |> processListRec result

input |> Seq.map int |> processListRec State.Zero

Questions

In fact, I am proud to have found a solution without mutable states that looks (at least for me) functional! :D. So any feeback that helps improving the solution or even alternative function approches for solving that problem are welcome :).

It also looks functional to me, and I like your use of union type State.

You can define the Zero static member of State as:

type State =
    | FinalState of int
    | IntermediateState of int * int Set
    static member Zero = IntermediateState(0, Set.empty.Add(0))

You don't have to create a separate with-definition.

In general I avoid writing the result to the console in the algorithm function it self. Instead it is more useful to return the result and let the caller handle that as needed:

let rec processListRec state lst =
    let result = lst |> Seq.fold foldState state
    match result with
    | FinalState result -> result
    | IntermediateState _ -> lst |> processListRec result

Used as:

printfn "%A" (processListRec State.Zero input)

You need some kind of stop condition or an input check if all changes are positive or negative.

For instance will [1;1;1] run forever, because no frequency will ever occur twice.

A raw recursive solution could be:

let findReoccurantFrequency input =
    if (input |> List.groupBy (fun x -> x > 0)) |> List.length < 2 then
        raise (Exception "Invalid input (only positive or negative changes)")
    else
        let rec runner lst freq freqs =
            if freqs |> List.contains freq then
                freq
            else
                let head, tail = if lst = [] then input.Head, input.Tail else lst.Head, lst.Tail
                runner tail (freq + head) (freq::freqs)
        runner input 0 []

I'm not saying that it's a better solution, just another approach for the exercise and inspiration. Especially I'm not proud of the input check.

Edit

As pointed out by JanDotNet in his comment, my input validation is poor, so it's maybe better to leave it out. Below is a revised version of my implementation using a Set instead of a List for the frequencies:

let findReoccurantFrequency input =
    let rec runner lst freq freqs =
        match freqs |> Set.contains freq with
        | true -> freq
        | false ->
            let head, tail = if lst = [] then input |> List.head, input |> List.tail else lst.Head, lst.Tail
            runner tail (freq + head) (freqs.Add(freq))
    runner input 0 Set.empty


Booking validation in a big if-elseif block I have pasted a part of my event handler code block. I understand this code really looks like a mess specially the object that is created every time the message is changing. Is cleanup possible in this code? Any inputs will help.

btn.addEventListener('click', procs.debounce(bookDiningBtnHandler, 400));
var bookDiningBtnHandler = function() {
    btnContext = this, message = {};
    btnContext.touchEnabled = false;
    btnContext.children[0].bubbleParent = 'false';
    hideKeyBoard();
    // Avoid overlap of keyboard with the prompt..
    removeDatePicker();
    // Avoid overlap of picker with the prompt..

    var isValidEmail = procs.checkValidEmail(email.value);
    var isIntPositive = procs.isPositiveInteger(guest.value);

    if (name.value != '' && isValidEmail != false && isIntPositive != false && guest.value > 0 && date.text != 'Date' && time.text != 'Time') {
        if (guest.value > guestLimit) {
            message = {
                "ref" : 0,
                "title" : "Attention",
                "text" : "Booking for more than 6, Please book through XYZ!"
            };
            customAlert(message);
            return;
        }
        booking = {
            "action" : "diningBook",
            "emailReservations" : Alloy.Globals.data.dining.ER,
            "name" : name.value,
            "email" : email.value,
            "guest" : guest.value,
            "date" : date.text,
            "time" : time.text
        };

        if(checkLastBooked(booking, Alloy.Globals.BDD)){
            message = {
                "ref" : 0,
                "title" : "Attention",
                "text" : "Bookings were made earlier!"
            };
            customAlert(message);
            return;
        }
        Alloy.Globals.BDD = booking;
        procs.sendEmail(booking, function(e) {
            treatTheAnswer(e);
            customAlert(message);
            //Ti.API.info('email  passed!');
        });
        function treatTheAnswer(resultSentEmail) {
            if (resultSentEmail) {
                message = {
                    "ref" : 0,
                    "title" : "Booking sent",
                    "text" : "A member of our team will contact you to confirm your BK"
                };
            } else {
                message = {
                    "ref" : 1,
                    "title" : "Error ",
                    "text" : "Something went wrong. Please try again later!"
                };
            }
        }

    } else {
        message = {
            "ref" : 0,
            "title" : "Missed info",
            "text" : "Missing information, Please fill in all fields!"
        };
        if (!isValidEmail && isIntPositive) {
            message = {
                "ref" : 1,
                "title" : "Error",
                "text" : " Invalid Email Address. Please re-enter valid Address!"
            };
        }
        customAlert(message);
        //Ti.API.info(' validation  failed!');

    }
};

Conditionals

isValidEmail != false && isIntPositive != false

I'd make that isValidEmail == true && isIntPositive == true. You could even short these to isValidEmail && isIntPositive. This makes your code more readable AND uses less characters (less that has to be read). It's a win-win.

var isIntPositive = procs.isPositiveInteger(guest.value);
... isIntPositive != false && guest.value > 0 ...

I don't know what isPositiveInteger does, but I have the feeling this check (guest.value > 0) is redundant. If you don't allow 0 then explicitly decline that value with guest.value != 0.

Duplication

You have a lot of message = { ... }. I'd create a separate function that takes the arguments. Then I'd create separate functions for the constants to be wrapped in, so you get things like this:

function createInvalidEmailMessage(){
    return createMessageObject(1, "Error", "Invalid Email Address. Please re-enter valid Address!");
}
function createMessageObject(ref, title, text){
    return {
        "ref": ref,
        "title": title,
        "text": text
    };
}


Very complicated Java HelloWorld app This program attempts to shows the basic concepts of inheritance and polymorphism.  In what ways could the code be modified to better demonstrate those concepts? The interface, abstract class and the class are all in separate files.

Greeting Interface

interface Greeting {
    void sayHello(String greeting, String name);
}

Abstract Hello Class

abstract class Hello {
    String name;
    String greeting;
    public abstract void setName(String name);
    public abstract void setGreeting(String greeting);
    public abstract String getName();
    public abstract String getGreeting();
}

Class that says "Hello"

public class Test extends Hello implements Greeting {
    private String name;
    private String greeting;

    public void setName(String name) {
    this.name = name;
    }
    public void setGreeting(String greeting) {
    this.greeting = greeting;
    }
    public String getName() {
    return name;
    }
    public String getGreeting() {
    return greeting;
    }
    public void sayHello(String g, String n) {
    System.out.print(g + " " + n);
    }
    public static void main(String[] args) {
    Test t = new Test();
    t.setGreeting("Hello");
    t.setName("World!");
    String h = t.getGreeting();
    String w = t.getName();
    t.sayHello(h,w);

    }
}

Your example implementation is not good enough. It's not very complicated at all, as you suggest with the title; rather, it is really simple.

The Hello abstract class is not needed. There is no reason why a class (like a WelcomeEmailContentBuilder or something of the sort) should want to extend Hello - something like a WelcomeEmailContentBuilder might already be extending a ContentBuilder or a EmailContentBuilder. You are abusing the is-a relationship.

Consider this rewrite of your code:

interface Greeting {
    void sayHello(String greeting, String name);
}

public class Test implements Greeting {

    public void sayHello(String greeting, String name) {
        System.out.print(greeting + " " + name);
    }

    public static void main(String[] args) {
        Greeting g = new Test();
        g.sayHello("Hello", "World!");
    }

}

This code is just as functional as your original code. But it's both way shorter and doesn't muck about with an abstract class that you don't need. It also makes use of the Greeting interface for a variable - showing off that you can call functions on objects if you only have an interface.

As for making your code actually show off the use of inheritance and polymorphism, that's up to you to figure out.


Sorting and searching problem related to a group of people standing in a rectangle Please be brutal, and let me know how I've done on this problem, provided I coded it at an interview for a top tech firm.

Time it took me: 44 minutes

Worst case time complexity: O(n2)?  Am I right?

Space Complexity: O(n)?

Problem:

  A group of people stand before you arranged in rows and columns.
  Looking from above, they form an R by C rectangle of people. You will
  be given a String[] people containing the height of each person.
  Elements of people correspond to rows in the rectangle. Each element
  contains a space-delimited list of integers representing the heights
  of the people in that row.

  Your job is to return 2 specific heights in
  a int[]. The first is computed by finding the shortest person in each
  row, and then finding the tallest person among them (the
  "tallest-of-the-shortest"). The second is computed by finding the
  tallest person in each column, and then finding the shortest person
  among them (the "shortest-of-the-tallest").

Definition:

  Class: TallPeople

  Method: getPeople

  Parameters: String[]

  Returns: int[]

  Method signature: int[] getPeople(String[] people) (be sure your method is public)

  Constraints:

  people will contain between 2 and 50 elements inclusive.
  Each element of people will contain between 3 and 50 characters inclusive.
  Each element of people will be a single space-delimited list of positive integers such that:

  Each positive integer is between 1 and 1000 inclusive with no extra
  leading zeros.
  Each element contains the same number of integers.
  Each element contains at least 2 positive integers.
  Each element does not contain leading or trailing whitespace.

Examples:

{"9 2 3",
 "4 8 7"}

  Returns: { 4,  7 }

  The heights 2 and 4 are the shortest from the rows,
  so 4 is the taller of the two. The heights 9, 8, and 7 are the tallest
  from the columns, so 7 is the shortest of the 3. 1)

{"1 2",
 "4 5",
 "3 6"}

  Returns: { 4,  4 }

{"1 1",
 "1 1"}

  Returns: { 1,  1 }

Answer:

public static int[] getPeople(String[] people){
    int maxOfMinHeight = Integer.MIN_VALUE;
    int minOfMaxHeight = Integer.MAX_VALUE;
    int count=0;

    String[][]findMaxOfMin = new String[people.length][people[0].split(" ").length];
    int[][] findMinOfMax = new int[people[0].split(" ").length][people.length];

    for(String s : people){
        String[] sort  = s.split(" ");
        Arrays.sort(sort);
        findMaxOfMin[count++]=sort;
    }
    for(int i=0; i<findMaxOfMin.length; i++){
        maxOfMinHeight = Math.max(maxOfMinHeight, Integer.valueOf(findMaxOfMin[i][0]));
    }
    count=0;
    int cols = people[0].split(" ").length;
    for(int i=0; i<cols; i++){
        int[] temp = new int[people.length];
        for(int j=0; j<people.length; j++){
            temp[j] = Integer.valueOf( ((String[])people[j].split(" "))[i]);
        }
        Arrays.sort(temp);
        findMinOfMax[count++]=temp;
    }
    for(int i=0; i<findMinOfMax.length; i++){
        minOfMaxHeight = Math.min(minOfMaxHeight, findMinOfMax[i][people.length-1]);
    }
    return new int[] {minOfMaxHeight, maxOfMinHeight};
}

Split it up!  The current method does more than one thing, I'd create at least three helper methods:

int[][] parseInput(String[] input);
int getMaxOfMinHeightInColumns(int[][] peoples);
int getMinOfMaxHeightInRows(int[][] peoples);

It would be easier to follow and understand.
I looks like a bug that the following testcase fails:

@Test
public void test() {
    String[] input = {"9 2 3",
     "4 8 7"};
    int[] result = getPeople(input);
    assertArrayEquals(new int[] {4, 7}, result);
}

Reusing the count variable is a bad sing:

count=0;

Try to use two different variables for different purposes and more descriptive names. What does it count? Put that into the name of the variable.
A few explanatory variable would be more readable here and remove some duplication:

String[][]findMaxOfMin = new String[people.length][people[0].split(" ").length];
int[][] findMinOfMax = new int[people[0].split(" ").length][people.length];

Consider creating one for people.length and another for people[0].split(" ").length (columnNumber, rowNumber, for example).
(String[]) casting seems unnecessary here:

temp[j] = Integer.valueOf( ((String[])people[j].split(" "))[i]);


First user login function: Security and efficency opinions I am writing an application for work. This application will sit on a closed network but I still have to be concerned about security. There will be a maximum of three types of user for this application: User (read only), admin (data edits only) and super admin (access all areas). But at the moment I am trying to get a feel for security on a basic level.

Just so I am clear about the user options: I meant that only 3 people will be permitted to use the application. So, there will only ever be ONE basic "User" and not a number of basic users.

Would one of you kind people please look at the following code to review its efficiency and security with the possibility of offering some tips on improvement.

function loginUser($username, $password)
{

    include('assets/con.php');

    /* Set the salt. */
    $salt = 'JwethYGkuuygTYFIYTFrfkuyGUTf75kUY4uyk6';

    /* Apply SHA512 password and salt encryption. */
    $password = hash('SHA512', $password);
    $salt = hash('SHA512', $salt);

    $query = 'SELECT username FROM users WHERE username = ? AND password = ? AND pass_salt = ?';
    if( $statement = $con->prepare($query) )
    {
        $statement -> bind_param( 'sss', $username, $password, $salt );
        $statement -> execute();
        $statement -> store_result();
        $num_row = $statement -> num_rows;
        $statement -> bind_result($username);
        $statement -> fetch();
        $statement -> close();
    }

    else die( "Failed to prepare query." );

    if( $num_row === 1 )
    {
        $_SESSION['userName'] = $username;
        return true;
    }
    return false;
}

Hashing

Let's start with the correct solution for hashing passwords: use bcrypt; PHP offers it with password_hash.

It's secure, and it's really easy to use (it's just a simple function call, and it manages salts etc for you).

What is wrong with your approach:

simple sha512 is way too fast; it's still better than plaintext, but it's really not the correct approach.
you don't have a per-user salt.
you don't even have a per-site salt. pass_salt doesn't serve any purpose, the check will always match as you are using it wrong and are thus still open to rainbow attacks etc (it should be something like $password = hash('SHA512', $password . $salt);, but again, site-salts are not enough).

Misc

your comment talks about encryption, but you are never encrypting, only hashing.
putting spaces around -> is unusual and leads to harder to read code.
don't die in functions, it makes it impossible for the calling code to recover. Just throw an exception.
pass the connection to the function instead of including it to make your function easier to reuse and test.

Apart from these minor issues and the very weak hashing, your code looks good to me.


Header only c++ singleton pattern implementation I want to implement the singleton pattern in a header only c++ library. (Please refrain from sharing your opinion on the pattern itself!) This would be trivial if I could rely on the compiler implementing thread safe c++11 function-local static-duration object initialization. i.e.

static inline MyClass& singleTon()
{
    static MyClass instance;
    return instance;
}

Unfortunately MSVC prior to 2015 does not comply with this standard. See "Magic Statics" so I have tried to implement it through a class:

#include <mutex>
/**
 * Header only implementation of the singleton pattern.
 * Use like this:
 * MyClass& singleton = SingleTon<MyClass>().get();
 */
template<class Type>
struct SingleTon
{
    static std::unique_ptr<Type> instance;
    static std::once_flag flag;
    Type& get()
    {
        auto& capture = instance;// lambdas can't capture static members
        std::call_once(flag, [&capture] { capture.reset(new Type{}); });
        return *instance.get();
    }
};

template<class Type>
std::unique_ptr<Type> SingleTon<Type>::instance;

template<class Type>
std::once_flag SingleTon<Type>::flag;

So my function now looks like:

static inline MyClass& singleton()
{
    return SingleTon<MyClass>().get();
}

I hope that this function is thread safe given that the ctor of Typeis nothrow.

I have a problem with this because

template<class Type>
struct SingleTon
{
    static std::unique_ptr<Type> instance;

This is still a static storage duration object. This means its initialization can happen after a call to Type& get(). So you have to solve this problem first.

So the instance variable needs to be wrapped in a function to make sure initialization of instance is guaranteed. If you are going to do that you may as well not dynamically create the object.

How about this:

template<class Type>
struct SingleTon
{
    private:
        static Type& instance()
        {
            // Use static function scope variable to
            // correctly define lifespan of object.
            static Type instance;
            return instance;
        }
    public:
        static Type& get()
        {
            // Note the constructor of std::once_flag
            // is a constexpr and is thus done at compile time
            // thus it is immune to multithread construction
            // issues as nothing is done at runtime.
            static std::once_flag flag;

            // Make sure all threads apart from one wait
            // until the first call to instance has completed.
            // This guarantees that the object is fully constructed
            // by a single thread.
            std::call_once(flag, [] { instance(); });

            // Now all threads can go get the instance.
            // as it has been constructed.
            return instance();
        }
};

The Elephant in the room (Linker)

Now that we solved this issue. There is a larger elephant in the room to solve. (which is also true for the original code).

Since this is a template class with its whole definition in the header file there will be several versions of this function spread around the code (one in each compilation unit). It becomes the job of the linker to remove all but one copy. If you have a simple application that just uses object files this does not seem to be an issue.

BUT I have come across linkers that were unable to do this when the object were compiled into separate dynamically loaded libraries. Thus in effect your singleton becomes a singleton per runtime library that uses it. This is why you don't find Singelton libraries defined like this with a template class as the harness.


Array of complex numbers A class Array which contains two data members: a pointer on array of complex numbers and the length of the array. Implement a function for reading the array. The class should have a constructor, destructor and copy constructor. In the main program, enter the array of complex numbers.

main.cpp

#include <iostream>
#include "complex.h"

using namespace std;

class Array{
private:
complex *n;
int length;
public:
Array(void);
Array(int,complex *);
~Array();
Array(const Array &);
void print_array(void);
};

Array::Array(void){
}

Array::Array(int _length, complex *_n){
length=_length;
n=new complex[length];
for(int i=0;i<length;i++){
n[i]=_n[i];
}
}

Array::~Array(){
delete [] n;
n=0;
}

 Array::Array(const Array & _n){
 length=_n.length;
 n=new complex[length];
 for(int i=0;i<length;i++){
 n[i]=_n.n[i];
 }
 }

 void Array::print_array(void){
 for(int i=0;i<length;i++){
 cout<<""<<(n[i]).return_real()<<"+("<<(n[i]).return_imaginary()<<")j"    <<endl;
 }
 }

 int main()
 {
 complex *numbers;
 int length;
 float a,b;
 cout<<"How many complex numbers you will enter? "<<endl;
 cin>>length;
 numbers=new complex[length];
 cout<<"Enter real and imaginary parts of "<<length<<" complex numbers"   <<endl;
 for(int i=0;i<length;i++){
 cin>>a>>b;
 numbers[i]=complex(a,b);
 }
 Array n1(length, numbers);
 n1.print_array();
 Array n2(n1);
 }

complex.cpp

#include "complex.h"

complex::complex(void){
}

complex::complex(float a,float b){
real=a;
imaginary=b;
}

complex::~complex(){
}

float complex::return_real(void){
return real;
}

float complex::return_imaginary(void){
return imaginary;
}

complex.h

#ifndef COMPLEX_H_INCLUDED
#define COMPLEX_H_INCLUDED

class complex{
private:
float real;
float imaginary;
public:
complex(void);
complex(float,float);
~complex();
float return_real(void);
float return_imaginary(void);
};

#endif // COMPLEX_H_INCLUDED

I tested it and it works. I want to know if there is something I can do to improve it.

The code isn't well indented, but maybe that's only your post on codereview (if it isn't, please consider indenting your code, that helps!)

There's a flaw in your code: you thought about reimplementing Array's copy constructor (which is good), but you didn't think about the copy assignment operator (which is implicitly defined).
Currently, this code is legal, and is very likely to cause undefined behaviour (that may or may not "work", you don't know, usually you'll get a segfault when it doesn't):

#include "array.hpp" // let's say you moved the array definition there

void uselessAssignment(const Array& a) {
  Array b;
  b = a; // call to the implicitly defined assignment operator!
  // call to b destructor (you just deleted the memory allocated by a)
}

int main(int, char*[]) {
  complex complexNumbers[42];
  Array a(42, complexNumbers);
  uselessAssignment(a);
  a.printArray();
  return 0;
}

To correct that, either redefine the implicit assignment operator or mark it as delete.

Otherwise that seems like a fine minimal implementation, it could be improved though:
- you forgot a delete in the main, that's not extremely important because the OS will free the memory anyway when the program terminates, but that's not really clean
- i've learnt today that you should avoid prefixing your names with an underscore (stack overflow link)
- in complex.cpp you can use the initializer list to initialize the real and imaginary parts (your code works fine, i just personally find it cleaner):

complex::complex(float a, float b) :
real(a),
imaginary(b) {}

- you may want your complex number to use something else than a float to store the real and imaginary parts:

template<class Real = float, class Imaginary = Real>
struct complex {
  //...
  Real real;
  Imaginary imaginary;
};

- you can use std::vector to handle your array rather than allocating/deallocating yourself, it's a bit safer (you wouldn't have the problem with the copy assignment), but i often directly use dynamic allocation myself so i'm not one to talk
- rather than this:

float a, b;
for(int i=0;i<length;i++){
  cin>>a>>b;
  numbers[i]=complex(a,b);
}

you can overload operator>> for complex rather than using the assignment operator:

std::ostream& operator>>(std::ostream& output, complex& number) {
  float a, b;
  output >> a >> b;
  number = complex(a, b);
  return output;
}
// ...
int main(int, char*[]) {
  // ...
  for(int i = 0; i < length ; ++i )
    cin >> numbers[i];
  // ...
}

(you can also declare the operator>> as a friend in complex so that it can directly access its members; when the assignment isn't trivial, it may also slightly improve performance, but not here)
- of course you could define arithmetic operations on complex numbers

...and probably other changes, but that's a rather long list already (and that also depends on what you expect from your classes).

Edit:
Ah something important if you manually handle memory in your class, you may want to define the move constructor and the move assignment operator, that may improve performance in some cases:

Let's say you have some function which doubles each complex number in an array and returns a new array (it doesn't modify the original one), let's say its protoype is Array doubleEachComplex(Array&);
If you use it like this:

Array array;
// ...
array = doubleEachComplex(array);

The compiler will:
- construct array
- do things with array (initialize it for instance)
- call doubleEachComplex
- create a fresh Array object
- call the copy assignment operator of Array (if no move assignment operator is defined)
- delete the fresh Array object (used by the copy)

But the copy assignment operator can't modify the "source" object, so it will usually perform a deep copy. A better solution would be to swap the contents of each array since the second one is a temporary (that's what a move constructor could do).
Actually, in that case an even better solution would have been to modify it in place, but that's not always possible/a good idea.


contains() algorithm for std::vector I wrote the following function for the purpose of reusability:

template<typename T>
bool contains(vector<T> v, T x)
{
      if (v.empty())
           return false;
      if (find(v.begin(), v.end(), x) != v.end())
           return true;
      else
           return false;
}

Is this the correct way to do, oor is there any better (or more efficient) way?

template<class C, class T>
auto contains(const C& v, const T& x)
-> decltype(end(v), true)
{
    return end(v) != std::find(begin(v), end(v), x);
}

Changes:

Removed the superfluous check for emptyness.
Parameterized on the container too, so all containers possible.

Removed from consideration if v is not a container of some sort.

Return the condition directly, no need for sticking it into a condition.

Of course, if you do many containment-tests on big containers, using an optimized container with optimized algorithms instead of vector might be a good idea, though measure it.

A bit more complicated, but using the container-provided find() for best performance where applicable:

#include <algorithm>

template<class C, class T>
inline auto contains_impl(const C& c, const T& x, int)
-> decltype(c.find(x), true)
{ return end(c) != c.find(x); }

template<class C, class T>
inline bool contains_impl(const C& v, const T& x, long)
{ return end(v) != std::find(begin(v), end(v), x); }

template<class C, class T>
auto contains(const C& c, const T& x)
-> decltype(end(c), true)
{ return contains_impl(c, x, 0); }


Is there anything wrong with this unusual NSMutableArray setter? This behaves to an outside observer mostly like @property (copy), except it has the very nice property of automatically performing any side effects that may exist in remove<Key>AtIndexes: and insert<Key>:atIndexes:. If I'm going to have such side effects, this seems really handy (DRY!)--is there anything wrong with this strange-looking accessor method?

- (void)setEdges:(NSArray *)newEdges
{
    if (newEdges != edges)
    {
        [self removeEdgesAtIndexes:[NSIndexSet indexSetWithIndexesInRange:NSMakeRange(0, [edges count])]];
        [edges release];
        edges = nil;
        if (newEdges)
        {
            edges = [NSMutableArray new];
            [self insertEdges:newEdges atIndexes:[NSIndexSet indexSetWithIndexesInRange:NSMakeRange(0, [newEdges count])]];
        }
    }
}

You should add edges = nil; either after [edges release]; or as the else clause. Otherwise passing a nil argument will make edges a dangling pointer to a released object.

Other than that, I don't see any issues with this implementation.


Passing a value from my Gruntfile to an external Grunt task In my Gruntfile, I use the grunt-contrib-watch task to watch a directory for file changes:

watch: {
    releases: {
        files: ['source/md/releases/*.md'],
        tasks: ['process:auto']
    }
}

Also, in the Gruntfile, I have a "watch" event handler. Inside this handler, I temporarily store the file path of the file that was modified, so that my task can read this information and only process that file:

grunt.event.on('watch', function (action, filepath, target) {
    if (target === 'releases') {
        grunt.file.write('filepath.txt', filepath);
    }
});

Then, inside my Grunt task (which is located in "tasks/process.js"), I read that file:

if (this.target === 'auto') {
    path = grunt.file.read('filepath.txt');

    // process file at given path

    grunt.file.delete('filepath.txt'); // delete this temporary file
}

Having to share the file path information via a file certainly doesn't seem optimal, but I haven't found a better solution yet. Note that writing the file path information directly into the configuration of my task from within the "watch" handler:

grunt.config('process.auto.filepath', filepath);

only works with the spawn:false setting in the "watch" configuration. I cannot set this option because then there is a 10 second delay between "watch" events (i.e. file modifications are ignored in that period).

Since file is an object, and it has a constructor, that can be use to store the file path:

grunt.event.on('watch', function (action, filepath, target) {
    if (target === 'releases') {
        grunt.file.constructor.filepath = filepath;
    }
});

References

Why Composition is Harder with Classes – JavaScript Scene – Medium
grunt/file.js at master · gruntjs/grunt


String Concatenation and Comparison I have the below code. I have list of status which contain may contain 4 values. R,UR,DO,NDO

based on this list. i need to convert to Y,N values

If list contains R the Readstatus="Y"

If list contains UR the Readstatus="N"

If both the ReadStatus="Y,N"

If list contains DO the DoneStatus="Y"

If list contains NDO the DoneStatus="N"
If both the DoneStatus="Y,N"

the below code works but its too ugly. is there any thing i can change

                Dim Readstatus As String = ""
                Dim DoneStatus As String = ""
                If Not String.IsNullOrEmpty(Status) Then
                    Dim listStatus = Status.Split(",").ToList()
                    If listStatus.Contains("R") Then
                        Readstatus = "Y"
                    End If
                    If listStatus.Contains("UR") Then
                        If String.IsNullOrEmpty(Readstatus) Then
                            Readstatus = "N"
                        Else
                            Readstatus = Readstatus & ",N"
                        End If

                    End If
                    If listStatus.Contains("DO") Then
                        DoneStatus = "Y"
                    End If
                    If listStatus.Contains("NDO") Then
                        If String.IsNullOrEmpty(DoneStatus) Then
                            DoneStatus = "N"
                        Else
                            DoneStatus = DoneStatus & ",N"
                        End If
                    End

It's interesting to notice that when you parse your input, you break a comma-separated string into a list of values. The end result of your code can be achieved much more cleanly if you take a similar (but inverted) approach, i.e. generate a list of values and then have the comma-separated string generated from that list.

It's counterintuitive for the logical branching in your code to be significantly more complex than the logical branching in your description of the behavior. These should map one to one. You can just translate your pseudocode logic exactly as is.

The reason your code ended up being more complex is because you needed to add the whole "add a comma or don't" logic to it - which can be avoided by relying on String.Join, which performs that logic for you, as long as you supply it with a collection of values.

  If list contains R the Readstatus="Y"
  If list contains UR the Readstatus="N"

Dim readStatusList As New List(Of String)

If listStatus.Contains("R") Then
    readStatusList.Add("Y")
End If
If listStatus.Contains("UR") Then
    readStatusList.Add("N")
End If

Dim readStatus As String = String.Join(",", readStatusList)

The exact same applies to the "done" status:

  If list contains DO the Donestatus="Y"
  If list contains NDO the Donestatus="N"

Dim doneStatusList As New List(Of String)

If listStatus.Contains("DO") Then
    doneStatusList.Add("Y")
End If
If listStatus.Contains("NDO") Then
    doneStatusList.Add("N")
End If

Dim doneStatus As String = String.Join(",", doneStatusList)


Summing Sequence with Combinatorics I am looking for help for two things.

proof of correctness (the tests have passed, but I do not now how to prove it correct)
Improvements on the Algorithm Efficiency.

The algorithm goes through 2 arrays arrays through permutations. The ordering is set up based on the array index, NOT the number inside the index. Noteably, if both parallel arrrays are set up diffrent, the algorithm should run fine.The algorithm then adds the function to the sum, which estimates computes the series.
Code is below.

def f(x:float, y:float, a:float)->float:
    """
    param x:float, x coordinate
    param y:flaot, y coordinate
    param a:float, paramter of the curve
    """
    return x + (y * a)

def main():
    """
    algorithm:
    Suppouse arrays are orderd by thier index and NOT the element inside
    Go through an ordering which meets that (one ordering is enough)
    add on the function at that point to the sequence
    """
    x = [1.0, 2.0, 3.0]
    y = [2.0, 3.0, 4.0]
    a = 2.0
    seq = [0.0] * len(x)
    for row in range(0, len(x) + 1):
        for col in range(0, len(x) + 1):
            for at in range(row, col):
                seq[at] = seq[at] + f(x[at], y[at], a)
    print(seq)

if __name__ == "__main__":
    main()

Disclaimer: Not a Code Reviewer
Just some comments:

Looks not bad at all;

First thing first, read through PEP8 if you like – I should also do that myself ( ˆ_ˆ )

Name things just a bit more descriptive, I understand that math people code like that and use single variable naming a lot;

Comment concise (one perfect scenario – which does not exist – would be the understandability of the code without any comment);

Use unittest if you like;

Turn on your IDE's spell checking;

Not sure about what we are doing overall, but my guess is that we might be able to start the second loop from the row + 1:

def get_linear_calc(x: float, y: float, coeff: float) -> float:
    """
    param x:float, x coordinate
    param y:flaot, y coordinate
    param coeff:float, parameter of the curve
    """
    return x + (y * coeff)

def get_ordered_sequence(x, y, coeff):
    """
    algorithm:
    Suppose arrays are ordered by their index and NOT the element inside
    Go through an ordering which meets that (one ordering is enough)
    add on the function curr that point to the sequence
    """
    seq = [0.0] * len(x)
    for row in range(len(x) + 1):
        for col in range(row + 1, len(x) + 1):
            for curr in range(row, col):
                seq[curr] += get_linear_calc(x[curr], y[curr], coeff)
    return seq

if __name__ == "__main__":
    x = (1.0, 2.0, 3.0, 2.0)
    y = (2.0, 3.0, 4.0, 1.0)
    coeff = 2.0
    print(get_ordered_sequence(x, y, coeff))

PS: I don't name things well, ignore my naming.


Running async tasks and cancelling after a timeout if necessary I have a few tools that run automatically. In those tools I usually run two to six jobs that run asynchronically. Sometimes when there is more data then expected or to prevent them from running indefinitely in case of a bug I cancel them.

The following example demonstrates my pattern with 90% accuracy. This is, the only parts that are left out is the actual business logic, IoC and the names of the processors are generic. Everthing else is pretty much the same.

I was wondering how good or bad this pattern is? It's been working fine for many days but maybe there is something that I missed and will give me headaches someday?

I start the application from the Main where I initialize the processors and the Timer (from System.Threading). Then I wait untill they all are done or cancel the one that might be either hanging or working with too much data at once.

void Main()
{
    var processors = new Processor[]
    {
        new Processor1(),
        new Processor2(),
        new Processor3(),
    };

    var token = new CancellationTokenSource();
    var timer = new Timer(
        callback: state => token.Cancel(),
        state: null,
        dueTime: TimeSpan.FromSeconds(5),
        period: TimeSpan.FromMilliseconds(-1));

    try
    {
        Task.WaitAll(processors.Select(p => p.Start(token.Token)).ToArray());
    }
    catch (AggregateException ex)
    {
        Console.WriteLine(ex);
    }
    finally
    {
        // When I use a try/catch already, I usually don't use 'using' to avoid deep nesting
        timer.Dispose();
        token.Dispose();
    }
}

Each processor is derived from the Processor class that provides logging for unexpected errors and awaits the core method to return.

abstract class Processor
{
    public Task Start(CancellationToken cancellationToken)
    {
        try
        {
            return Task.Run(async () => await StartCore(cancellationToken));
        }
        catch (Exception ex)
        {
            // logging
            return Task.FromException(ex);
        }
    }

    protected abstract Task StartCore(CancellationToken cancellationToken);
}

The processors themselves just do some work.

class Processor1 : Processor
{
    protected override Task StartCore(CancellationToken cancellationToken)
    {
        Console.WriteLine(GetType().Name + " started.");
        for (int i = 0; i < 6; i++)
        {
            cancellationToken.ThrowIfCancellationRequested();
            Thread.Sleep(TimeSpan.FromSeconds(1));
        }
        Console.WriteLine(GetType().Name + " finished.");
        return Task.FromResult<object>(null);
    }
}

class Processor2 : Processor
{
    protected override Task StartCore(CancellationToken cancellationToken)
    {
        Console.WriteLine(GetType().Name + " started.");
        for (int i = 0; i < 2; i++)
        {
            cancellationToken.ThrowIfCancellationRequested();
            Thread.Sleep(TimeSpan.FromSeconds(1));
        }
        Console.WriteLine(GetType().Name + " finished.");
        return Task.FromResult<object>(null);
    }
}

class Processor3 : Processor
{
    protected override Task StartCore(CancellationToken cancellationToken)
    {
        Console.WriteLine(GetType().Name + " started.");
        for (int i = 0; i < 4; i++)
        {
            cancellationToken.ThrowIfCancellationRequested();
            Thread.Sleep(TimeSpan.FromSeconds(1));
        }
        Console.WriteLine(GetType().Name + " finished.");
        return Task.FromResult<object>(null);
    }
}

Can't you replace

protected abstract Task StartCore(CancellationToken cancellationToken);

with

protected abstract void StartCore(CancellationToken cancellationToken);

and call it as

Task.Run(() => {
                  try
                  {
                      StartCore(cancellationToken);
                  }
                  catch
                  {
                     //logging
                     throw;
                  }
                });

? Why do you need async/await?

You don't really need a timer, you can use WaitAll overload that takes TimeSpan:

var tasks = processors.Select(p => p.Start(token.Token)).ToArray();
if (!Task.WaitAll(tasks, TimeSpan.FromSeconds(5)))
{
    token.Cancel();
    // wait for cancellation, if necessary
    // Task.WaitAll(tasks);
}

You don't show your business logic and you don't say what kind of processing you are doing, so it's hard to tell whether inheritance is the best approach. It doesn't look like it based on your examples. Maybe it is better to have a single Processor class that would contain all the boiler-plate code you require (similar to BackgroundWorker) and inject StartCore as delegate/interface instead.


Split up an iterable into batches I was writing a program that had a list of e-mail messages. It needed to group them into batches, with the condition that each batch must have at max 20 messages, and be maximally 50k in size (no individual message is bigger than 50k). Obviously itertools.groupby() is a candidate for this, but I really didn't want to keep around a key variable, so I decided to write my own batcher:

def batch(iterable, condition_func=(lambda x:True), max_in_batch=None):
    def Generator():
        nonlocal n, iterable, condition_func, max_in_batch
        num_in_batch = 1
        yield n
        while True:
            n = next(iterable)
            if num_in_batch == max_in_batch or condition_func(n):
                return
            num_in_batch += 1
            yield n

    n = next(iterable)
    while True:
        yield Generator()

Sample run:

>>> gen = batch(itertools.count(1),(lambda x:x%10==9),7)
>>> list(next(gen))
[1, 2, 3, 4, 5, 6, 7]
>>> list(next(gen))
[8]
>>> list(next(gen))
[9, 10, 11, 12, 13, 14, 15]
>>> list(next(gen))
[16, 17, 18]
>>> list(next(gen))
[19, 20, 21, 22, 23, 24, 25]
>>> list(next(gen))
[26, 27, 28]

What do you think? Is this Pythonic?

Bugs

>>> gen = batch(range(50), lambda x:x%10==9, 7)
>>> for i in gen:
    print(list(i))

Traceback (most recent call last):
  File "<pyshell#149>", line 1, in <module>
    for i in gen:
  File "<pyshell#142>", line 13, in batch
    n = next(iterable)
TypeError: 'range' object is not an iterator

Damn, I expected to be able to use iterables as the iterable parameter, not iterators.

Let's fix it:

def batch(iterable, condition_func=(lambda x:True), max_in_batch=None):
    iterator = iter(iterable)
    def Generator():
        nonlocal n, iterator, condition_func, max_in_batch
        num_in_batch = 1
        yield n
        while True:
            n = next(iterator)
            if num_in_batch == max_in_batch or condition_func(n):
                return
            num_in_batch += 1
            yield n

    n = next(iterator)
    while True:
        yield Generator()

and try again:

>>> gen = batch(range(50), lambda x:x%10==9, 7)
>>> for i in gen:
    print(list(i))

[0, 1, 2, 3, 4, 5, 6]
[7, 8]
[9, 10, 11, 12, 13, 14, 15]
[16, 17, 18]
[19, 20, 21, 22, 23, 24, 25]
[26, 27, 28]
[29, 30, 31, 32, 33, 34, 35]
[36, 37, 38]
[39, 40, 41, 42, 43, 44, 45]
[46, 47, 48]
[49]
[49]
[49]
[49]
[49]
[49]
[49]
[49]
[49]
Traceback (most recent call last):
  File "<pyshell#157>", line 2, in <module>
    print(list(i))
  File "C:\Python33\lib\idlelib\PyShell.py", line 1336, in write
    return self.shell.write(s, self.tags)
KeyboardInterrupt

Dang… Let's try to limit that:

def batch(iterable, condition_func=(lambda x:True), max_in_batch=None):
    iterator = iter(iterable)
    def Generator():
        nonlocal n, on_going, iterator, condition_func, max_in_batch
        num_in_batch = 1
        yield n
        on_going = False
        while True:
            n = next(iterator)
            on_going = True
            if num_in_batch == max_in_batch or condition_func(n):
                return
            num_in_batch += 1
            yield n

    n = next(iterator)
    on_going = True
    while on_going:
        yield Generator()

Way better!

Use a for loop to iterate over an iterator

Iterating over elements and maintaining a counter of how much elements were iterated over is usually done using a for loop combined with enumerate:

def Generator():
    nonlocal n, on_going, iterator, condition_func, max_in_batch
    yield n
    on_going = False
    # Start enumerate at 1 because we already yielded n
    for num_in_batch, item in enumerate(iterator, 1):
        on_going = True
        n = item
        if num_in_batch == max_in_batch or condition_func(n):
            break
        yield item

Using this construct also help handling the on_going flag (I don't like the way we have to change it twice to continue iterating): when the break is not reached, we know we have exhausted the iterator.

def Generator():
    nonlocal n, on_going, iterator, condition_func, max_in_batch
    yield n
    # Start enumerate at 1 because we already yielded n
    for num_in_batch, item in enumerate(iterator, 1):
        n = item
        if num_in_batch == max_in_batch or condition_func(n):
            break
        yield item
    else:
        on_going = False

Use closures instead of nonlocals

Specifying iterator, condition_func, and max_in_batch in the nonlocal clause is not necessary as they are not modified within Generator. Instead you should use standard scope resolution to do the job.

In fact, your use of nonlocal to get non-trivial side effects on n had me wondered if an inner function is really the way to go. Let's try OOP instead:

class Batch:
    def __init__(self, iterable, condition=(lambda x:True), limit=None):
        self.iterator = iter(iterable)
        self.condition = condition
        self.limit = limit
        try:
            self.current = next(self.iterator)
        except StopIteration:
            self.on_going = False
        else:
            self.on_going = True

    def group(self):
        yield self.current
        # start enumerate at 1 because we already yielded the last saved item
        for num, item in enumerate(self.iterator, 1):
            self.current = item
            if num == self.limit or self.condition(item):
                break
            yield item
        else:
            self.on_going = False

    def __iter__(self):
        while self.on_going:
            yield self.group()

A bit more verbose but it seems to be a bit more explicit too. You use it as you would for your batch function:

>>> gen = Batch(range(50), lambda x:x%10==9, 7)
>>> for i in gen:
    print(list(i))

[0, 1, 2, 3, 4, 5, 6]
[7, 8]
[9, 10, 11, 12, 13, 14, 15]
[16, 17, 18]
[19, 20, 21, 22, 23, 24, 25]
[26, 27, 28]
[29, 30, 31, 32, 33, 34, 35]
[36, 37, 38]
[39, 40, 41, 42, 43, 44, 45]
[46, 47, 48]
[49]


Quicksort implementation in Go Learning Go and trying to get a grip of the various concepts it uses. Following is my implementation of Quicksort (choosing last element as pivot). It takes the length of the array and the subsequent array to be sorted as user input.

package main

import "fmt"

func main() {
    var n int

    fmt.Print("Enter Size: ")
    fmt.Scan(&n)

    var slice = make([]int, n)

    for i := 0; i < n; i++ {
        fmt.Print("arr[",i,"]: ")
        fmt.Scan(&slice[i])
    }

    fmt.Print("Array: ", slice, "\n")
    quickSort(slice, 0, n - 1)
    fmt.Print("Sorted Array: ", slice)
}

func quickSort(a []int, low int, high int) {
    if(low < high) {
        p := partition(a, low, high)
        quickSort(a, low, p - 1)
        quickSort(a, p + 1, high)
    }
}

func partition(a []int, low int, high int) int {
    pivot := a[high]
    i := low
    for j := low; j < high; j++ {
        if(a[j] <= pivot) {
            swap(&a[i], &a[j])
            i += 1
        }
    }
    swap(&a[i], &a[high])
    return i
}

func swap(a, b *int) {
    temp := *a
    *a = *b
    *b = temp
}

Comments on coding style, following/avoiding language convention, use of pointers, use of slices, and overall correctness of the program will be appreciated.

Most importantly, use slices to your advantage. You do not need high and low arguments, as a slice can represent a part of a slice. For example, instead of quickSort(a, low, p - 1) you could write quickSort(a[low:p]).

Swapping can be written much more elegantly with multiple assignment: a, b = b, a

Use fmt.Printfln instead of fmt.Print("Array: ", slice, "\n"). Once you have learned printf, it is almost always clearer than concatenating strings. In this simple case fmt.Println would be appropriate as well, though.

Other than that the code is clean. It seems you have used gofmt and using int instead of uint for indices is the correct choice. (Uint should only be used where absolutely needed, because it is inconvenient and error-prone to calculate with.)