Extension to BindingList that allows for cancelable notifications of changing and deleting This is an extension of the System.ComponentModel.BindingList<T> class that allows for notifications of item changing and deleting. These notifications provided the outgoing item and the incoming item, as well as a Cancel property to cancel the change or deletion.

In additon, it also allows for a bulk addition of items, which is just a loop of AddItem (which will trigger a change notification for each item added).

On top of bulk addition, there is a SetItems which will clear the collection and add all the items provided, but will only fire one Reset notification.

using System;
using System.Collections.Generic;
using System.ComponentModel;

namespace BindingListExtension
{
    public class BindingListEx<T> : BindingList<T>
    {
        public event EventHandler<ListItemRemovingEventArgs<T>> ListItemRemoving;
        public event EventHandler<ListItemChangingEventArgs<T>> ListItemChanging;

        protected override void RemoveItem(int index)
        {
            var args = new ListItemRemovingEventArgs<T>(this[index], index);
            ListItemRemoving?.Invoke(this, args);
            if (args.Cancel) return;
            base.RemoveItem(index);
        }

        protected override void SetItem(int index, T item)
        {
            var args = new ListItemChangingEventArgs<T>(this[index], item, index);
            ListItemChanging?.Invoke(this, args);
            if (args.Cancel) return;
            base.SetItem(index, item);
        }

        public void SetItems(IEnumerable<T> items)
        {
            RaiseListChangedEvents = false;
            Clear();
            AddItems(items);
            RaiseListChangedEvents = true;
            OnListChanged(new ListChangedEventArgs(ListChangedType.Reset, -1));
        }

        public void AddItems(IEnumerable<T> items)
        {
            if (items != null)
            {
                foreach (T item in items)
                {
                    Add(item);
                }
            }
        }
    }

    public class ListItemRemovingEventArgs<T> : CancelEventArgs
    {
        public ListItemRemovingEventArgs(T oldItem, int index)
        {
            OldItem = oldItem;
        }

        public T OldItem { get; }

        public int Index { get; }
    }

    public class ListItemChangingEventArgs<T> : ListItemRemovingEventArgs<T>
    {
        public ListItemChangingEventArgs(T oldItem, T newItem, int index) : base(oldItem, index)
        {
            NewItem = newItem;
        }

        public T NewItem { get; }
    }
}

ListChangedType.Reset

AddItems is nothing more than a glorified wrapper method for calling Add multiple times. However, guidelines suggest that ListChangedType.Reset could and probably should also be called here. Since Reset should be called when..

  Much of the list has changed. Any listening controls should refresh
  all their data from the list.

To give this method some purpose other than looping elements and call Add, consider using the same deferral strategy as you did with SetItems.


Search for the biggest amplitude in an array I have a assignment to find which season has the biggest temperature amplitude. Input par is the int array with temperatures:

int T[]= {-1,-10,10,5,30,15,20,-10,30,10,29,20}

which means:

winter: -1,-10,10 spring: 5,30,15 summer: 20,-10,30 autumn: 10,29,20

temp is always an int,
int[] T - T.length%4 is equal to 0 (same number of temperatures for every season, could be 2, 3 etc.)

{

    public String solution(int[] T){
    int length = T.length;
    int count = length/4;
    int i=0,j=0,indx=0;
    String name="";
    int maxAmpl;

    int[] winter = new int[count];
    int[] spring = new int[count];
    int[] summer = new int[count];
    int[] autumn = new int[count];
    int[] diff = new int[4];

    for(j=0,i=0;j<count; j++,i++){
        winter[i] = T[j];
    }

    for(j=count,i=0;j<count*2; j++,i++){
        spring[i] = T[j];
    }

    for(j=count*2,i=0;j<count*3; j++,i++){
        summer[i] = T[j];
    }

    for(j=count*3,i=0;j<count*4; j++,i++){
        autumn[i] = T[j];
    }

    Arrays.sort(winter);
    Arrays.sort(spring);
    Arrays.sort(summer);
    Arrays.sort(autumn);

    diff[0] = winter[count-1]-winter[0];
    diff[1] = spring[count-1]-spring[0];
    diff[2] = summer[count-1]-summer[0];
    diff[3] = autumn[count-1]-autumn[0];

    maxAmpl=diff[0];

    for(int k=1; k<4; k++){
        if (diff[k]>maxAmpl){
            maxAmpl = diff[k];
            indx = k;
        }
    }

    switch(indx){
        case 0:
            name = "WINTER";
            break;
        case 1:
            name = "SPRING";
            break;
        case 2:
            name = "SUMMER";
            break;
        case 3:
            name = "AUTUMN";
            break;
    }
    return name;
}

Some key points:

No need to sort the array to find min/max value
No need to copy the array, can search in the original array using an offset count * N

Have a look at this:

public String solution(int[] T) {
    final int count = T.length / 4;
    int indx = 0;
    int last = Integer.MIN_VALUE;
    for (int i = 0; i < 4; ++i) {
        int diff = IntStream.of(T).skip(count * i).limit(count).max().getAsInt()
                 - IntStream.of(T).skip(count * i).limit(count).min().getAsInt();
        if (diff > last) {
            indx = i;
            last = diff;
        }
    }
    final String[] seasons = { "WINTER", "SPRING", "SUMMER", "AUTUMN" };
    return seasons[indx];
}


Finding Duplicate (Key, Value) in Map I wrote a function for checking if a Map[(String, String)] contains an element with a matching key and value:

  def findDupes(map: Map[String, String], key: String, value: String):
                           Option[(String, String)] = {
    val dupes = map.collect { case (x, y) if(x == key && y == value) => key }
    dupes match {
      case Nil     => None
      case x :: _  => Some(key, value)
    }
  }

Testing

scala> map
res10: scala.collection.immutable.Map[String,String] = Map(1 -> HELLO, 2 -> WORLD)

scala> findDupes(map, "1", "HELLO")
res8: Option[(String, String)] = Some((1,HELLO))

scala> findDupes(map, "1", "FOO")
res9: Option[(String, String)] = None

Use map get key contains value, to test if a given key-value-pair is already part of a Map.

Your solution is extremely inefficient, because you iterate through the entire Map (with collect) just to find one value. get returns an Option which can be checked for the containing value.


Removing numbers from the middle of an array I have an array like this (make sure you scroll to the right):

[0,0,0,0,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,0,0,0,0,0,0,0,0,0,0,0,0,0]

Where it will always be the same length. And there will always be some number of 0's surrounding some number of 1's. I am trying to find a good/efficient/smart way to turn the middle of the array of into 0's when there is a padding of 4 1's on each side. The result would be something like:

[0,0,0,0,1,1,1,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,1,1,1,0,0,0,0,0,0,0,0,0,0,0,0,0]

Where you now have a padding of 4 1's on either side.

This is how I implemented it:

var a = [0,0,0,0,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,0,0,0,0,0,0,0,0,0,0,0,0,0];

var hitOne = false;
var oneCount = 0;
var oneStopPoint = a.lastIndexOf(1) - 3;
for(var n in a){
    if(n == oneStopPoint){
        break;
    }
    if(a[n] == 1 && !hitOne) {
        hitOne = true
        oneCount++;
    } else if(a[n] == 1 && hitOne) {
        oneCount++;
    }
    if(oneCount > 4){
        a[n] = 0;
    }
}
console.log(a);

http://jsfiddle.net/4gk9L9uv/1/

Firstly: Don't use for...in on arrays.

Secondly, I suppose you can find the 1-boundaries, and add/subtract 4:

var left = array.indexOf(1) + 4;
var right = array.lastIndexOf(1) - 4;
for(; left <= right ; left++) {
  array[left] = 0;
}

Not particularly clever, but it does the job.

(Note: lastIndexOf is widely supported in modern runtimes, but older ones may not have it)

But also take @itsbruce's advice from the comments, and consider another structure for these data.


Class and Static practice, IDS and a max instance count I was practicing working with classes and the static property included with them and came up with a simple ID counter and max number list. I also added a quick debug error throw if it goes over. Here's what I came up with:

#include <iostream>

// A class that has static variables to save information for the ID's of its instances.
class HoldIdentity{
private:
    static int const maxInstances = 20;
    static int created; // = 0; error
public:
    static int getMax(){
        return maxInstances;
    }
    // A static function that will return an array on the heap of every /possible/ remaining Identity Holder.
    static HoldIdentity* const createAll(){
      int length = maxInstances - created;
      HoldIdentity*JAM = new HoldIdentity[maxInstances - created];
      for (int i = 0; created < maxInstances; JAM[i++]=HoldIdentity());
      return JAM;
    };
private:
    int id;
public:
    HoldIdentity(){
        id = created++;
        if (created > maxInstances){
            created--;
            std::string error = "To many instances!";
            throw (error);
        }
    }
    // Destructor displays the id.
    ~HoldIdentity(){
        displayID();
    }
    void displayID() const{
        std::cout << "my id:" << id << std::endl;
    }
};

//int HoldIdentity::created = 0;
int main(){
    using namespace std;
    HoldIdentity numba1{};
    numba1.displayID();
    HoldIdentity *allInstances = HoldIdentity::createAll();

    // because creating more than the max causes an error.
    // I suppose I should have put all prior code in here as well.
    try{
        HoldIdentity numbaPast{};
        HoldIdentity numbaPast2{};
        HoldIdentity numbaPast3{};
    }
    catch (...){
        // I think I could have used a string to retrieve the message?
        cout << " too many instances! ";

    }

    // free array from the heap.
    delete[] allInstances;
    system("pause");
    return 0;
}

edit: Is there any little bit of code here that could be improved? I've only just started working on C++(again) this week so I'm sure there's many alternatives. But specifically if my own code for determining and handling IDS are bad. Besides that, is my error handling a good practice?

Why don't you initialize created in the definition?

What does JAM stand for?

Why do you calculate length but never use it?

I'm not 100% sure on this one but I think you don't need the for loop calling the constructor. new should construct every object in the array separately.


Localization manager based on ReactiveCocoa I have a localization manager class which can change app language on the fly. It's using ReactiveCocoa and my main concern is that I'm not using some of RAC parts correctly.

First (in - (void)configureSignals):

+ (instancetype)sharedInstance
{
    static i2KRLMLocalizationManagerObject *sharedInstance = nil;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        sharedInstance = [[[self class] alloc] init];
        [sharedInstance initialConfiguration];
    });
    return sharedInstance;
}

- (void)initialConfiguration
{
    [self configureSignals];
    [self configureAppLocalizations];
}

- (void)configureSignals
{
    RACReplaySubject *subject = [RACReplaySubject replaySubjectWithCapacity:1];
    self.localeConnection = [subject multicast:subject];
    [self.localeConnection autoconnect];

    RACReplaySubject *languagesSubject = [RACReplaySubject replaySubjectWithCapacity:1];
    self.languageNamesConnection = [languagesSubject multicast:languagesSubject];
    [self.languageNamesConnection autoconnect];

    RACReplaySubject *languageIndexSubject = [RACReplaySubject replaySubjectWithCapacity:1];
    self.languageIndexConnection = [languageIndexSubject multicast:languageIndexSubject];
    [self.languageIndexConnection autoconnect];
}

It feels for me that multicast should be used differently(and is used wrong currently, although it works), although I'm not sure how to use it correctly.

Those signals are used to propagate NSLocale, array of supported languages and current language index in that array. I use multicast so that if there is more then 1 subscriber, signal will be shared and current values will be just replayed. I'm not sure that signal multicasting itself is idiomatic/correct way. Also I'm not completely sure if connection is needed at all or I can just go with ReplaySubject.

Second (like here):

- (void)changeToLocale:(NSLocale *)locale
{
    [self reloadBundleWithLocaleID:[locale localeIdentifier]];
    [(RACSubject *)[self localeSignal] sendNext:locale];
}

Cast has to be made now because in interface it's declared as - (RACSignal *)localeSignal; and is actually

- (RACSignal *)localeSignal
{
    return self.localeConnection.signal;
}

which is defined as RACSignal in ReactiveCocoa as well, but since it's definitely RACSubject(RACReplaySubject is subclass of RACSubject and since sendNext is available on RACSubject and not on RACSignal I need to cast to avoid compiler complaints).

I use that getter instead of self.languageNamesConnection.signal because if I ever change this to something I wouldn't have to go all over and remove traces of that. But the cast looks just wrong.

I don't like this cast at all, although not sure how to improve this(since I'm sending values through multicasted signal; and probably get rid of this sending altogether if possible).

Also general suggestions on improving would be very helpful(I think there should be more FRP way to do this).

Full repo with example is here, class is here.

Excerpts from answers to my question on RAC repo:

For the first question -

  Subjects are naturally multicasted, so you could replace the multicast connection property with @property RACReplaySubject *localeSubject and subscribe directly to that in place of localeSignal.

So there's no need to use connection here.

For the second part - it's enough to have RACSignal in interface and RAC(Replay)Subject in the implementation.


Averages and standard deviations for a Knn application I'm working on a small command line application in c# where data is read from a CSV file along with a query and KNN is calculated for the query based on the data.

My problem isn't the algorithm at all, I understand it. Im having difficulty working with my code though. I think it's down to bad planning.

Here's my object for holding a "row" of data. the data is based on weather data and if something was played that day or not.

class dataRow
{
    public double sun {get; set;}
    public double temp {get; set;}
    public double humidity {get; set;}
    public double wind {get; set;}
    public bool play {get; set;}

    public override string ToString()
    {
        return string.Format("\n\tSun: {0}\n\tTemp: {1}\n\tHumidity: {2}\n\tWind: {3}\n\tPlay: {4}", sun, temp, humidity, wind, play);
    }

    public double[] toArray()
    {
        return new double[] { sun, temp, humidity, wind };
    }
}

I'm trying to Znormalize the data and the query before I work out euclidean distance between points but I'm stuck in a rut of annoying nested loops and I have a hunch there's a better way to code what I'm doing

class KNN
{
    public int numK;
    List<dataRow> dataSet;
    dataRow QueryRow;
    dataRow stdDev;
    dataRow avg;

    public KNN(String[] input)
    {
        numK = Int32.Parse(input[0]);
        dataSet = dataImport.import(input[1]);
        //inputRow = dataImport.import(input[2])[0];
        avg = getAverage(toDoubleList(dataSet));
    }

    public List<double[]> toDoubleList(List<dataRow> input)
    {
        var doubleList = new List<double[]>();

        for(int i =0; i<input.Count; i++)
        {
            doubleList[i] = input[i].toArray();
        }

        return doubleList;
    }

    public dataRow getAverage(List<double[]> input)
    {
        var avgTotals = new double[4];
        var avgRow = new dataRow();

        for (int y = 0; y < 3; y++)
        {
            double avg = 0;

            for (int x = 0; x < input.Count; x++)
            {
                avg += input[x][y];
            }

            avgTotals[y] = avg / input.Count;
        }

        avgRow.sun = avgTotals[0];
        avgRow.temp = avgTotals[1];
        avgRow.sun = avgTotals[2];
        avgRow.wind = avgTotals[3];

        return avgRow;
    }

    public dataRow getStdDev(List<double[]> input)
    {
        var stdDevTotals = new double[4];
        var stdDevRow = new dataRow();

        for (int y = 0; y < 3; y++)
        {
            double stdDev = 0;

            for (int x = 0; x < input.Count; x++)
            {
                stdDev += Math.Pow(input[x][y] - this.avg.toArray()[y], 2);
            }

            stdDev = Math.Sqrt(stdDev/input.Count);
            stdDevTotals[y] = stdDev / input.Count;
        }

        stdDevRow.sun = stdDevTotals[0];
        stdDevRow.temp = stdDevTotals[1];
        stdDevRow.sun = stdDevTotals[2];
        stdDevRow.wind = stdDevTotals[3];

        return stdDevRow;
    }
}

Any advice or ideas? (All harsh criticism welcome!)

As others mentioned in comments, you should follow the .Net naming guidelines. It will make your code consistent with all other .Net code.
Try to avoid abbreviated names like KNN, because they can be confusing. And if that's supposed to mean k-nearest neighbors, then I don't see how does your class compute that.

This applies also to names like numK.
Computing some statistics on your data should be separate from parsing inputs. So the constructor of KNN should take directly numK and dataSet and it should be the job of its caller to parse the inputs.
If the common approach is to create a DataRow once and then never change it again, consider making it immutable: make the property setters private and create a constructor that can be used to set them.

You could also add a constructor from an array of doubles, as an counterpart of ToArray().
Both your GetAverage() and GetStdDev() follow the same pattern. I would try to abstract away this pattern into a separate method, to avoid repetition:

public static DataRow AggregateRows(
    this IList<DataRow> rows, Func<IEnumerable<double>, double> func)
{
    return new DataRow
    {
        Sun = func(rows.Select(row => row.Sun)),
        Temp = func(rows.Select(row => row.Temp)),
        Humidity = func(rows.Select(row => row.Humidity)),
        Wind = func(rows.Select(row => row.Wind))
    };
}

With this, both methods can be significantly simplified using the LINQ method Average():

public DataRow Average()
{
    return dataSet.AggregateRows(values => values.Average());
}

public DataRow StdDev()
{
    return dataSet.AggregateRows(
        values =>
        {
            var average = values.Average();
            var variance = values.Average(value => Math.Pow(value - average, 2));
            return Math.Sqrt(variance);
        });
}

Note that the structure of the expression for computing variance is very similar to the way it's normally written: E[(X - E[X])2].

Also, if you do it this way, you won't need the ToArray() method anymore.
If you have many types whose ToString() follows the same pattern, you could implement it just once using reflection and then use that implementation everywhere.


Evaluating f(N,R) mod(m) The goal is to find \$f(n,r)\mod(m)\$ for the given values of \$n\$, \$r\$, \$m\$, where \$m\$ is a prime number.

$$f(n,r) = \dfrac{F(n)}{F(n-r) \cdot F(r)}$$

where

$$F(n) = 1^1 \cdot 2^2 \cdot 3^3 \cdot \ldots  \cdot n^n$$

Here is the Python code snippet I've written:

    n_r = n - r
    num = 1
    den = 1

    if n_r > r:
        for j in xrange(n_r + 1, n + 1):
            num = num*(j**j)
        for k in xrange(2, r + 1):
            den = den*(k**k)

        answer = num/den
        ans = answer%m
        print ans

    else:
        for j in xrange(r + 1, n + 1):
            num = num*(j**j)
        for k in xrange(2, n_r + 1):
            den = den*(k**k)

        answer = num/den
        ans = answer%m
        print ans

This code runs for small values of \$n\$ (when \$n <= 100\$). But for large values of \$n\$ ~ \$10^5\$, the code seems to be inefficient, exceeding the time limit of 2 sec. How can I generalize and optimize this computation?

Your function is slow for large values of n because the intermediate
values of num and den quickly grow to huge integers, making the multiplication slow.

You can improve that
by reducing each intermediate result "modulo m", so that all numbers will
always be in the range 0 ... m-1.

The final division num/den must then be computed as a "modular division":

def mod_div(a, b, m):
    """modular division

    Returns a solution c of a = b * c mod m, or None if no such number c exists.
    """
    g, x, y = egcd(b, m)
    if a % g == 0:
        return (a * (x // g)) % m
    else:
        return None

using the "Extended Euclidean Algorithm, for example with the code from
http://en.wikibooks.org/wiki/Algorithm_Implementation/Mathematics/Extended_Euclidean_algorithm#Recursive_algorithm

def egcd(a, b):
    if a == 0:
        return (b, 0, 1)
    else:
        g, y, x = egcd(b % a, a)
        return (g, x - (b // a) * y, y)

The if/else can be avoided, either as @Janos suggested, or by replacing
r by n-r if the latter is smaller. Your function would then look like this:

def f(n, r, m):

    if n - r < r:
        r = n - r

    num = 1
    den = 1

    for j in xrange(n - r + 1, n + 1):
        num = (num * (j ** j % m)) % m
    for k in xrange(2, r + 1):
        den = (den * (k ** k % m)) % m

    return mod_div(num, den, m)

At least for large values of n this should be faster. I made a test
with f(10000, 100, 747164718467):

Your code: 25 seconds
Above code: 0.2 seconds

A possible further improvement would be to compute the powers j ** j % m also
with a "modular power" method, such as power_mod() from http://userpages.umbc.edu/~rcampbel/Computers/Python/lib/numbthy.py.
The reduced the time for calculating f(10000, 100, 747164718467)
to 0.05 seconds on my computer.


Initialize and sum an array in Java using threads This bit of code initializes an array of any given size where each index corresponds to a thread and a value. That is, index 0 has 0, index 1 has 1, and so on. Then, the values, 0 to n, in the indexes are summed.

This code as part of an assignment though I'm wondering if there are any ways it could be improved? Namely, is there an alternative to using 2 for loops and calling thread.join() on them?

I'd be open to any advice and would gladly take it!

public class Processor {

    static volatile int numberOfValues = 17;
    static double processedArray[];
    static Thread threadsArray[];
    static volatile int sum;
    static Object lock1 = new Object();
    static Object lock2 = new Object();

Adding values to the array, each index in the array gets a thread to initialize that value:

    private static void initializeArray() {
        threadsArray = new Thread[numberOfValues];
        processedArray = new double[numberOfValues];

        for (int i = 0; i < threadsArray.length; i++) {
            threadsArray[i] = new Thread(new Runnable() {
                public void run() {
                    synchronize(lock1) {
                        processedArray[numberOfValues - 1] = numberOfValues;
                        numberOfValues--;
                    }

                }
            });
            threadsArray[i].start();
        }

Here is the first for loop joining all threads after the initialization of the array:

        for (int i = 0; i < threadsArray.length; i++) {
            try {
                threadsArray[i].join();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }

Summing the values in the array:

        for (int i = 0; i < threadsArray.length; i++) {
            threadsArray[i] = new Thread(new Runnable() {
                public void run() {
                    synchronize(lock2) {
                        sum += processedArray[numberOfValues];
                        numberOfValues++;
                    }

                }
            });
            threadsArray[i].start();
        }

Here is the second for loop joining all the threads after summing:

        for (int i = 0; i < threadsArray.length; i++) {
            try {
                threadsArray[i].join();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }

    }

Main:

    public static void main(String args[]) {
        initializeArray();
        for (int i = 0; i < threadsArray.length; i++) {
            System.out.println(processedArray[i]);
        }

        System.out.println("Sum: " + sum);
    }
}

In this answer I want to take a look at what your code is doing, and why it doesn't really make sense to use threads like this.

One of the main point of using threads is to do long-running things at the same time (more details here). Some examples:

keeping a GUI responsive while doing other work in the background.
Spreading long calculations over multiple cores.
Doing long-running tasks like waiting for incoming connections or collecting statistics.

If you do them on the same thread, they have to wait for each other. Keep each one on a separate thread, and they'll happily run side-by-side.

In your code, this doesn't really happen.

     for (int i = 0; i < threadsArray.length; i++) {
        threadsArray[i] = new Thread(new Runnable() {
            public void run() {
                synchronize(lock1) {
                    processedArray[numberOfValues - 1] = numberOfValues;
                    numberOfValues--;
                }

            }
        });
        threadsArray[i].start();
    }

Here you're creating your first set of threads, to set the values of the array. However, all of the code inside the thread is synchronized over one object.

static Object lock1 = new Object();

Only one thread can hold a lock at a time. So while one thread is doing its thing, the others are just waiting. This means that all the threads you've created are waiting for each other. They're not running at the same time at all!

But think about why threads are useful: they allows you to do long running tasks at the same time. Except these threads aren't doing anything long. They're just setting a value in an array. That's super fast. It probably takes longer to make the thread than to set the value.

So the same code could have been written like this, without threads:

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

And it would have been more readable, shorter and faster.

Addendum

As I've said before, this problem should not be solved using threads. However as an exersize let's look at why you need a lock, and how to get rid of it.

public void run() {
    synchronize(lock1) {
        processedArray[numberOfValues - 1] = numberOfValues;
        numberOfValues--;
    }
}

You're sharing the variable numberOfValues between threads. This means that without lock:

One thread could excecute the first line.
Another thread could execute the same line.

Because numberOfValues hasn't been decremented, both proccessArray[] values are now set to the same value.

Instead of using a normal integer as a counter you could use an AtomicInteger.

static AtomicInteger numberOfValues = new AtomicInteger(17);

Your code will look like this:

public void run() {
    int index = numberOfValues.decrementAndGet();
    processedArray[index] = index;
}

You won't need a lock now, since all access to the AtomicInteger is atomic. There's no way to set a proccessArray value without also decrementing the counter.

But we can do better. Instead of using a counter, why don't we just use the index of the loop you're using to create the threads? The code will look like this:

for (int i = 0; i < threadsArray.length; i++) {
    final int index = i;
    threadsArray[i] = new Thread(new Runnable() {
        @Override
        public void run() {
            processedArray[index] = index;
        }
    });
}

Now you don't need a shared counter, and you wont't need a lock.


Inserting into a database using JDBC I am trying to insert into a database using JDBC and each thread will be inserting into the database. I need to insert around 30-35 columns. I wrote a stored procedure that will UPSERT into those columns.

The problem I am facing is that in the run() method, I have around 30 columns written over there for insertion. Is there any way I can simplify my run method so that it doesn't looks so messy which is looking right now for me? And I have a few more columns as well. So if I keep on adding new columns there, it will be looking so messy at one point in my run method.

Is there any way to clean this, keeping in mind the thread safety issues?

class Task implements Runnable {

    private Connection dbConnection = null;
    private CallableStatement callableStatement = null;

    public Task() {

    }

    @Override
    public void run() {

    dbConnection = getDbConnection();
    callableStatement = dbConnection.prepareCall(Constants.UPSERT_SQL);

    callableStatement.setString(1, String.valueOf(userId));
    callableStatement.setString(2, Constants.getaAccount(userId));
    callableStatement.setString(3, Constants.getaAdvertising(userId));
    callableStatement.setString(4, Constants.getaAvgSellingPriceMainCats(userId));
    callableStatement.setString(5, Constants.getaCatAndKeywordRules(userId));
    callableStatement.setString(6, Constants.getaClvBehavior(userId));
    callableStatement.setString(7, Constants.getaClvChurn(userId));
    callableStatement.setString(8, Constants.getaClvInfo(userId));
    callableStatement.setString(9, Constants.getaClvSegmentation(userId));
    callableStatement.setString(10, Constants.getaCsaCategoriesPurchased(userId));
    callableStatement.setString(11, Constants.getaCustomerService(userId));
    callableStatement.setString(12, Constants.getaDemographic(userId));
    callableStatement.setString(13, Constants.getaFinancial(userId));
    callableStatement.setString(14, Constants.getaGeolocation(userId));
    callableStatement.setString(15, Constants.getaInterests(userId));
    callableStatement.setString(16, Constants.getaLastContributorsPurchased(userId));
    callableStatement.setString(17, Constants.getaLastItemsLost(userId));
    callableStatement.setString(18, Constants.getaLastItemsPurchased(userId));
    callableStatement.setString(19, Constants.getaLastProductsPurchased(userId));
    callableStatement.setString(20, Constants.getaLastSellersPurchasedFrom(userId));
    callableStatement.setString(21, Constants.getaMainCategories(userId));
    callableStatement.setString(22, Constants.getaMessaging(userId));
    callableStatement.setString(23, Constants.getaPositiveSellers(userId));
    callableStatement.setString(24, Constants.getaPromo(userId));
    callableStatement.setString(25, Constants.getaScores(userId));
    callableStatement.setString(26, Constants.getaSegmentation(userId));
    callableStatement.setString(27, Constants.getaSellers(userId));
    callableStatement.setString(28, Constants.getaSrpBuyerUpiCount(userId));
    }
  }

    private Connection getDBConnection() {

    Connection dbConnection = null;

          Class.forName(Constants.DRIVER_NAME);
      dbConnection = DriverManager.getConnection(url,user,password);

      return dbConnection;
    }

Extract a method.

class Task implements Runnable {

    ...
    private completeStatement (CallableStatement stmt, Strung userId)
    {
        callableStatement.setString(1, String.valueOf(userId));
        ...
    }

    @Override
    public void run() {
        dbConnection = getDbConnection();
        callableStatement = dbConnection.prepareCall(Constants.UPSERT_SQL);
        completeStatement(callableStatement,userId)

    }
}

private Connection getDBConnection() {
    ...
}

But find a more suitable name for what your statement is doing.


Using random to generate strings I have code that reads from files of surnames, female and male names and randomly generates a name in the following format: lastname, male/female name.

The files are in the following format:

  JAMES    3.318    4,840,833 1

I have the following working code:

def picks(filename):
    with open(filename) as f:
        lines = [line.strip('\n') for line in f]
        pick = randint(0, len(lines))
    return lines[pick].split()[0]

def random_name(number_of_names):
    nselect = ''
    for _ in range(1, number_of_names+1):
        g = randint(0,1)
        if g==1:
            nselect = picks('femalenames.txt')
        else:
            nselect = picks('malenames.txt')
        print(g)
        print( '{0:s}, {1:s}\n'.format(picks('surnames.txt'), nselect))

random_name(5)

The code generates the correct output but I feel the code could be trimmed a bit.

Your intuition was accurate! Here is a shortened version with (almost-)identical functionality:

from random import choice

def picks(filename):
    with open(filename) as f:
        lines = [line.strip() for line in f]
    return choice(lines).split()[0]

def random_names(number_of_names):
    for _ in range(number_of_names):
        nselect = picks(choice(('femalenames.txt', 'malenames.txt')))
        print('{0:s}, {1:s}\n'.format(picks('surnames.txt'), nselect))

random_name(5)

Note the following:

str.strip removes whitespace (including newlines) by default, so you don't need to make the argument explicit;
random.choice is a neater way to select a random item from a list than generating and using an index;
random_names is a better name for the function, as you can specify a varying number of names;
You don't need to initialise nselect;
Why carefully adjust an index you aren't actually using by one? 0 is the default start for range, so we can just use number_of_names directly; and
Selecting directly from the two possible filenames makes the functionality more obvious (although you can no longer print(g), which was a bad name anyway).


Finding specific small byte arrays in large binary files I'm trying to quickly and efficiently find every recurring position of small byte arrays (4 bytes) in large binary files (several GBs). My current method is as follows:

Stream stream = File.OpenRead(filepath);

List<long> searchResults = new List<long>(); //The results as offsets within the file
int searchPosition = 0; //Track of how much of the array has been matched
int[] searchPattern = { 0x00, 0x01, 0x02, 0x03 }; // The array to search

while(true) //Loop until we reach the end of the file
{
    var latestbyte = stream.ReadByte();
    if(latestbyte == -1) break; //We have reached the end of the file

    if(latestbyte == searchPattern[searchPosition]
    {
        searchPosition++;
        if(searchPosition == searchPattern.Length)
        {
            searchResults.Add(stream.Position);
        }
    }
    else
    {
        searchPosition = 0;
    }
}

It's slow, and seems quite inefficient (3-4 seconds for a small 174MB file, 35 seconds for a 3GB one).

How can I improve the performance?

I looked into Boyer-Moore, but is it really worth it, considering the pattern i'm looking for is only 4 bytes?

You should be aware, that if you have a search pattern like:

byte[] pattern = { 0x00, 0x01, 0x02, 0x03 };

and a file sequence as

..., 0x00, 0x00, 0x01, 0x02, 0x03,...

Then it won't be found, because the first 0x00 will increment searchPosition to 1, so the second 0x00 will test false in if(latestbyte == searchPattern[searchPosition].

The solution is to reset the searchPosition whenever the latestbyte equals searchPattern[0]:

while(true) //Loop until we reach the end of the file
{
    var latestbyte = stream.ReadByte();
    if(latestbyte == -1) break; //We have reached the end of the file

    if (latestbyte == searchPattern[0])
      searchPosition = 0;

    if(latestbyte == searchPattern[searchPosition]
    {
        searchPosition++;
        if(searchPosition == searchPattern.Length)
        {
            searchResults.Add(stream.Position);
            searchPosition = 0;
        }
    }
}

OBS: This is only working for search patterns with  bytes, that are all different (see Firdas comment).

The position of the found pattern, you add to the result is actually the position of the last byte in the pattern. Maybe the start position is more interesting:

searchResults.Add(stream.Position - searchPattern.Length);

As Sylvain suggests you can improve performance a bit by reading larger chunks of bytes and evaluate them in turn successively:

List<long> GetPatternPositions(string path, byte[] pattern)
{
  using (FileStream stream = new FileStream(path, FileMode.Open))
  {
    List<long> searchResults = new List<long>(); //The results as offsets within the file
    int patternPosition = 0; //Track of how much of the array has been matched
    long filePosition = 0;
    long bufferSize = Math.Min(stream.Length, 100_000);

    byte[] buffer = new byte[bufferSize];
    int readCount = 0;

    while ((readCount = stream.Read(buffer, 0, buffer.Length)) > 0)
    {
      for (int i = 0; i < readCount; i++)
      {
        byte currentByte = buffer[i];

        if (currentByte == pattern[0])
          patternPosition = 0;

        if (currentByte == pattern[patternPosition])
        {
          patternPosition++;
          if (patternPosition == pattern.Length)
          {
            searchResults.Add(filePosition + 1 - pattern.Length);
            patternPosition = 0;
          }
        }
        filePosition++;
      }
    }

    return searchResults;
  }
}

The improvement is not that impressive, but maybe you can experiment with the bufferSize to optimize it?


Get list of files in directory with exclude option This method returns the list of files (absolute path) in a folder (or tree). It allows filtering by extensions or filenames.

The method receives the following parameters:

string path: folder path to scan for files.
string[] exclude: can contain filenames such as "read.me" or extensions such as "*.jpg".
SearchOption searchOption: TopDirectoryOnly to scan only the specified folder or AllDirectories to scan tree folder under the specified path.

The method return files that doesn't appear in exclude array and a) hasn't extension, or b) extension doesn't appear in exclude array.

    public static IEnumerable<string> GetFiles(string path, string[] exclude, SearchOption searchOption = SearchOption.AllDirectories)
    {
        IEnumerable<string> files = Directory.EnumerateFiles(path, "*.*", searchOption);
        var resultFiles = new List<string>();

        if (exclude.Length > 0)
        {
            foreach (var filename in files)
            {
                string extension = Path.GetExtension(filename);

                if (Array.IndexOf(exclude, Path.GetFileName(filename)) >= 0)
                {
                    continue;
                }

                if (string.IsNullOrEmpty(extension) || Array.IndexOf(exclude, "*" + extension) < 0)
                {
                    resultFiles.Add(filename);
                }
            }
        }

        return resultFiles;
    }

A few things I noticed:

If the exclude array is empty you return an empty list instead of every file.
reversing your logic would allow you to check everything in one statement.
Since you're returning an IEnumerable<string> already, using the yield operator would make sense here.
Accepting an IEnumerable<string> instead of an string[] for the exclude parameter, gives the user more options.  Also this allows you to leverage the LINQ extensions better.

Your code could look something like this:

public static IEnumerable<string> GetFiles(string path, IEnumerable<string> exclude, SearchOption searchOption = SearchOption.AllDirectories)
{
    IEnumerable<FileInfo> files = new DirectoryInfo(path).EnumerateFiles("*.*", searchOption);
    foreach (var filename in files)
    {

        if (!exclude.Any(x => x == filename.Name || (x.StartsWith("*") && x.Contains(filename.Extension))))
        {
            yield return filename.Name;
        }

    }
}


A simple cipher that uses the sums of previous characters I'm fairly happy with the code itself, but I'd like you to tear it apart.

def sumcipher(inpt, o=0, encrypt=True):
    if not isinstance(inpt, str):
        raise TypeError("Input must be a string")
    if encrypt:
        return ''.join([chr(((sum([ord(i) - 32 for i in inpt[j::-1]]) + (o % 96)) % 96) + 32) for j in range(len(inpt))])
    else:
        L = ""
        for i in range(len(inpt)):
            L += chr((((ord(inpt[i]) - 32) - sum([ord(j) - 32 for j in L])) % 96) + 32)
        return L

I'm mostly looking for ways to make it more elegant/simple.

PS: I'm very inexperienced with asking questions.

As with CRC, this doesn't offer cryptographic strength.

The o in the public API needs an informative name, perhaps "offset".

In a duck-typed language, feel free to omit checks such as:

if not isinstance(inpt, str):
    raise TypeError("Input nust be a string")

since the first call to ord() will have much the same effect. Do go to the trouble of writing a docstring which describes the arguments.

Rather than an if, you might break this into two functions.

Try to show parallel structure. You chose to build up a list of strings in the 1st case, yet appended to a string in the 2nd case.

One of your modulo operations, (o % 96), seems to be redundant.

Consider naming your magic numbers, 32 and 96. Or do away with them entirely: produce binary output which you then base64 encode.

The nested loops offer quadratic performance. You have an opportunity to store sums in a temp variable if you want linear performance.


Creating an image in Excel using the colors from an input image I'm having some performance issues in this code and need some suggestions on how to improve it. I'm not interested in "general code style" improvements unless they have performance implications.

The purpose is to create an image in Excel using the colors from an input image. What is my performance issue is that it takes time setting the styles (about 45% of time each when profiling) and it is increasing linearly by each "pixel" created.

Currently it becomes annoyingly slow with pictures larger than about 50 x 50.

style.setFillForegroundColor(new XSSFColor(color));
style.setFillPattern(FillPatternType.SOLID_FOREGROUND);

I was thinking of doing it in parallel, but I don't think POI is thread safe when trying. I also tried to do with HSSF (but got reduced number of colors and styles allowed and maximum 255 pixels wide, however it was fast).

My next thinking was to reduce number of colors used in the image and cache styles for each color so I can reuse the style for several pixels. I have not tried it yet however.

Do you have any other suggestions?

package test.test;

import java.awt.Color;
import java.awt.image.BufferedImage;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.util.function.IntConsumer;
import java.util.stream.IntStream;

import javax.imageio.ImageIO;

import org.apache.poi.ss.usermodel.Cell;
import org.apache.poi.ss.usermodel.FillPatternType;
import org.apache.poi.xssf.usermodel.XSSFCell;
import org.apache.poi.xssf.usermodel.XSSFCellStyle;
import org.apache.poi.xssf.usermodel.XSSFColor;
import org.apache.poi.xssf.usermodel.XSSFRow;
import org.apache.poi.xssf.usermodel.XSSFSheet;
import org.apache.poi.xssf.usermodel.XSSFWorkbook;

public class ExcelDrawXSSF {

    public static void main(String[] args) {
        try (XSSFWorkbook workbook = new XSSFWorkbook()) {
            XSSFSheet sheet = workbook.createSheet("Pic");
            // Can take any picture jpg/png if you want to try
            BufferedImage image = ImageIO.read(new File("DisplayImage.jpg"));

            int height = image.getHeight();
            int width = image.getWidth();

            for (int y = 0; y < height; y++) {
                XSSFRow row = sheet.createRow(y);
                for (int x = 0; x < width; x++) {
                    int rgb = image.getRGB(x, y);
                    Color color = new Color(rgb, true);
                    XSSFCell cell = row.createCell(x);
                    setColor(workbook, cell, color);
                    System.out.println(y + "," + x);
                }
            }

            setSmallerColumnWidth(sheet, width);

            try (FileOutputStream outputStream = new FileOutputStream("Pic.xlsx")) {
                workbook.write(outputStream);
            }
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }

    }

    private static void setSmallerColumnWidth(XSSFSheet sheet, int width) {
        IntConsumer setToDefaultWidth = col -> sheet.setColumnWidth(col, 4 * 256);
        IntStream.range(0, width).forEach(setToDefaultWidth);
    }

    private static void setColor(XSSFWorkbook workbook, Cell cell, Color color) {
        long start = System.nanoTime();
        XSSFCellStyle style = workbook.createCellStyle();

        // Here is the performance issue.
        style.setFillForegroundColor(new XSSFColor(color));
        style.setFillPattern(FillPatternType.SOLID_FOREGROUND);

        cell.setCellStyle(style);
        System.out.println((System.nanoTime() - start) / (1000 * 1000));
    }

}

AFAIR, styles are "heavy cost" objects in the workbook. Thus, create each XSSFCellStyle only once and reuse the XSSFCellStyle object for the same color.


Stored procedure to implement a search filter I've implemented a stored procedure for a search process SQL Server 2008. I'm not sure if everything I created is correct. Also I'm wondering if this code has any leaks or vulnerability on SQL Injection. Here is my stored procedure:

USE [TestDB]
GO
/****** Object:  StoredProcedure [dbo].[Search_Dictionary]    Script Date: 08/09/2018 19:17:48 ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
ALTER PROCEDURE [dbo].[Search_Dictionary]
  @FilterBy int = NULL,
  @Name varchar(50) = NULL,
  @Code char(2) = NULL
AS
BEGIN
    SET NOCOUNT ON;

    SELECT RecID, Status, Code, Name
    FROM Dictionary
    WHERE
        (@FilterBy = 1 AND Name LIKE '%'+@Name+'%')
        OR
        (@FilterBy = 2 AND Code = @Code)
        OR
        (@FilterBy = 3 AND @Name IS NULL AND @Code IS NULL);
END

Here is example on how I call this procedure:

EXEC Search_Dictionary @FilterBy = 1, @Name = "Grant", @Code = NULL;

I just want to prevent, if for example Filter By is 2 that should search query by Code column returns any result if user pass word Grant. In that case should return 0 records.

Also if anyone have any suggestions on how to improve the code please let me know.

A user could cause a dos attack by passing in complex like filters, you might want to sanitize the @name parameter. https://www.owasp.org/index.php/Testing_for_SQL_Wildcard_Attacks_(OWASP-DS-001)


Persist data by serializing/deserializing objects that are sent to it I've created the following class to persist data by serializing/deserializing objects that are sent to it. I would like to know if there is a better way of writing this class, or if my class is fine the way it is.

using System.IO;
using System.Runtime.Serialization.Formatters.Binary;
using System.Runtime.Serialization.Formatters.Soap;

namespace Education_PersistingData
{
public class PersistData<T>
{
    private readonly T _obj;
    private readonly string _filePath;
    private readonly string _fileName;

    public PersistData(T obj, string filePath, string fileName)
    {
        this._obj = obj;
        this._filePath = filePath;
        this._fileName = fileName;
    }

    /// <summary>
    /// Serializes objects to a SOAP .xml format
    /// </summary>
    public void SerializeToSoapFormat()
    {
        try
        {
            SoapFormatter soapFormatter = new SoapFormatter();
            Stream dataStream = File.Create(_filePath + _fileName);
            soapFormatter.Serialize(dataStream, _obj);
            dataStream.Close();
        }
        catch (IOException ex)
        {
            throw new IOException(ex.Message);
        }
    }

    /// <summary>
    /// Serializes objects to a Binary .txt format
    /// </summary>
    public void SerializeToBinaryFormat()
    {
        try
        {
            BinaryFormatter binaryFormatter = new BinaryFormatter();
            Stream dataStream = File.Create(_filePath + _fileName);
            binaryFormatter.Serialize(dataStream, _obj);
            dataStream.Close();
        }
        catch (IOException ex)
        {
            throw new IOException(ex.Message);
        }
    }

    /// <summary>
    /// Deserializes a SOAP .xml file format
    /// </summary>
    /// <returns>Deserialized object</returns>
    public T DeserializeSoapFormat()
    {
        try
        {
            SoapFormatter soapFormatter = new SoapFormatter();
            Stream dataStream = File.OpenRead(_filePath + _fileName);
            T result = (T)soapFormatter.Deserialize(dataStream);
            dataStream.Close();
            return result;
        }
        catch (IOException ex)
        {
            throw new IOException(ex.Message);
        }
    }

    /// <summary>
    /// Deserializes a Binary .txt file format
    /// </summary>
    /// <returns>Deserialized object</returns>
    public T DeserializeBinaryFormat()
    {
        try
        {
            BinaryFormatter binaryFormatter = new BinaryFormatter();
            Stream dataStream = File.OpenRead(_filePath + _fileName);
            T result = (T)binaryFormatter.Deserialize(dataStream);
            dataStream.Close();
            return result;
        }
        catch (IOException ex)
        {
            throw new IOException(ex.Message);
        }
    }
}
}

There is no point in catching the IOException and then throwing a new exception. You only loose information here. Either handle it or remove the try-catch altogether.

Also, use using to always dispose IDisposable objects.


Destructor for a linked list The full code is located here: https://gist.github.com/4521540

It's a dummy List in C++. My concern is with freeing up memory.
It doesn't crash when I run my code. It looks like my if/else covers everything.

It starts by deleting the second item if there is. That's what the while loop does.
If there is only one present (or left with one), delete startNode.

startNode is a pointer points to the first item in the list.
endNode always points to the last item in the list.

Am I deleting the pointer or the underlying object?

I do mostly Python. Last time I wrote a serious C++ homework was about 2 years ago in my algorithm class so I really can't remember everything off my head.

~List()
{
    // there is at least one item
    if(startNode != 0)
    {
        // release memory starting from the second item
        ListNode *current, *soon;
        current = this->startNode->next;
        while(current != 0)  // if there are at least two items
        {
            /* When there is no more items after current,
             * delete current and leave.
             * Otherwise, free up current and move on to
             * the next item.
             */
            if(current->next != 0)
            {
                soon = current->next;
                delete current;
                current = soon;
            }
            else
            {
                delete current;
                break;
            }

        }
    }

    delete this->startNode;
    delete this->endNode;
}

Also, do I need to delete the myList in my main program? I think when I exit the program, the destructor is automatically called.

First, to answer your questions about new/delete: Everything you new, you must delete at some point, or you leak memory. When you new, you are given a pointer to the object that has been allocated. Similarly, when you use delete, you must use a pointer to that same object, and the memory that was allocated will be freed. After doing this, the pointer will be pointing at freed memory and you should not delete this again. It doesn't matter where you use new, in main() or otherwise, you need to delete it at some point.

Now to your code: Your code will double-delete either the first node when the list size is 1, or the last node, when the list size is greater than 1. Double deletes cause undefined behaviour including crashes.

Let's look at list of size 1 first. Your while loop won't be entered, and you basically skip down to the end where you do

delete this->startNode;
delete this->endNode;

Since startNode and endNode should both be pointing to the only node in the list, it will get deleted twice!

If you have a list of more items, the loop will delete every item except the first. Then the last two statements occur. In this case, this->endNode will be the original end of the list (since you never alter it here) and so it will be deleted twice.

Here is a simpler version that just deletes every node, starting from the first.

~List
{
    ListNode* current = startNode;
    ListNode* next;

    while (current != NULL) {
        next = current->next;
        delete current;
        current = next;
    }
}


shared_instance implementation using C++ and RAII I created a little singleton-like class that gets deleted when there is no reference to it anymore (so its lifetime shall be somewhat predictable). I aimed to make it thread-safe.

#include <memory>
#include <mutex>

template< typename t_type >
struct shared_instance
{
    static std::shared_ptr< t_type > get_shared_instance()
    {
        static std::weak_ptr< t_type > weak;
        static std::mutex mutex;

        auto result = weak.lock();
        if( nullptr == result )
        {
            std::unique_lock< std::mutex > lock{ mutex };

            result = weak.lock();
            if( nullptr != result )
            {
                return result;
            }

            result = std::make_shared< t_type >();
            weak = result;
        }
        return result;
    }
};

template< typename t >
struct context
{
    context()
    : instance{ shared_instance< t >::get_shared_instance() }
    { }

private: std::shared_ptr< t > instance;
};

I would be glad if you could comment on the implementation itself and how I could tackle non-default-constructible objects (I know of perfect forwarding and variadic templates but I have not found an elegant way to evaluate them lazily).

Non-Default Constructors:

Adding support for non-default constructible objects in a "lazy" fashion, could be done by adding a factory function of std::function<std::shared_ptr< t_type >()>.

template< typename t_type >
struct shared_instance
{
    using t_factory = std::function<std::shared_ptr< t_type >()>;

    static t_factory factory; // MUST be set before calling get_shared_instance

    static std::shared_ptr< t_type > get_shared_instance()
    {
        ...

            assert(factory); // or throw, or return empty shared_ptr

            result = factory(); // instead of make_shared

        ...
    }

};

// and define the static member...
template< typename t_type >
typename shared_instance< t_type >::t_factory shared_instance< t_type >::factory;

...

int main()
{
    context<int> c; // not ok... haven't set factory yet!

    // do this before any call to get_shared_instance
    shared_instance<int>::factory = [] () { return std::make_shared<int>(5); }; // store things in the lambda by value, not reference...

    context<int> c; // ok
}

I'm not sure this counts as elegant though, as it gives you a two-step initialization process. This could be fixed by recognising the shared_instance itself as a kind of factory, making its contents non-static, and passing it into the contexts:

template< typename t >
struct context
{
    context(instance_provider< t >& provider): // now it can't be created unless this exists...
        instance{ provider.get_shared_instance() }
    { }

private:

    std::shared_ptr< t > instance;
};

int main()
{
    auto provider = instance_provider<int>([] () { return std::make_shared<int>(5); });

    context<int> c(provider);
}

Further Considerations:

Does the C library you're using say anything about context lifetime itself? (e.g. Winsock reference counts calls to WSAStartup() and WSACleanup() internally, so as long as the calls are paired properly the C library handles cleaning up)

The main feature that this shared_instance class seems to provide is destroying The Instance promptly after the last context is destroyed. RAII itself could be achieved by just storing a unique pointer over the maximum possible lifetime of any contexts, which would be simpler. Is this prompt destruction actually important?

If not, then it's just a singleton, which we either live with, or do the work to pass a pointer or reference to the thing you need where it needs to be.


HashMap keys to sorted List The idea is to obtain the keys as a list ordered by the natural ordering of the keys but first of them all the ones that have priority, stated by the method hasPriority(). The custom comparator code block feels out of place and I wonder if it is possible to improve the code:

private List<String> orderedKeys() {
    return this.variables.entrySet()
            .stream()
            .sorted((e1, e2) -> {
                if (e1.getValue().hasPriority() && e2.getValue().hasPriority()) return e1.getKey().compareTo(e2.getKey());
                if (e1.getValue().hasPriority()) return -1;
                if (e2.getValue().hasPriority()) return 1;
                return e1.getKey().compareTo(e2.getKey());
            })
            .map(e -> e.getKey())
            .collect(Collectors.toList());
}

Edit: this.variables is a reference to a Map of String Keys and custom objects as values that have the boolean method hasPriority().

You can use the new utilities in the comparator class. Unfortunately you'll have to help the compiler a bit with the generic type induction.

Assuming, that the value class is "Whatever", this can be done like this:

    map.entrySet().stream()
        .sorted(Comparator.comparing((Map.Entry<String, Whatever> e) -> e.getValue().hasPriority())
                          .thenComparing(Map.Entry::getKey))
        .map(Map.Entry::getKey)
        .collect(Collectors.toList());


License and Registration, Sir The following code will generate a number plate and then generate a random speed and see if it's speeding

import random, string, time

class Car:
    def __init__(self):
        self.plate = self.genPlate()
        self.limit = 60
        self.checkSpeeding()

    def checkSpeeding(self):
        self.speeding = random.randint(0, 1)

        if self.speeding:
            self.speed = random.randint(self.limit+3, self.limit+20)
        else:
            self.speed = random.randint(self.limit-20, self.limit+2)

    def genPlate(self):
        plateFormat = ['L', 'L', 'N', 'N', 'L', 'L', 'L']
        genPlate = []

        for i in plateFormat:
            if i == 'L':
                genPlate.append(random.choice(string.ascii_letters[26:]))

            elif i == 'N':
                genPlate.append(str(random.randint(0, 9)))

        genPlate.insert(4, " ")

        return  "".join(genPlate)

allCars = []
x = 0

while True:
    allCars.append(Car())

    print(allCars[x].plate + " was going " + str(allCars[x].speed) + "mph in a " + str(allCars[x].limit) + "mph zone")
    if allCars[x].speeding:
        print("speeding fine issued")

    print("\n")

    time.sleep(5)
    x += 1

Here is a link to the original source

Consider the real world scenario a little bit. Does a car know what the speed limit is? Does it issue itself a fine if it's speeding? Or do the police do this?  Assuming we're talking about the current state of things and not some near future world where cars do these things, police do.

A more proper OOP approach would be to create a Policeman class with a IsSpeeding method that takes a Car in as an argument. The Policeman would then be responsible for determining whether or not the car was speeding and issuing tickets when necessary.


Reading Maps of strings and doubles from CSV I want to have 2 classes responsible for reading dictionaries from CSV.
The 1st class takes 2 columns from CSV and makes Map<String, String>.
The 2nd class takes 2 columns from CSV and makes Map<String, Double>.

I want to solve it according to DRY rule so I've made a generic method for making Map<T1, T2> from csv which will be used by 1st class and 2nd class. It goes like this:

package Main;

import java.io.File;
import java.io.FileNotFoundException;
import java.util.HashMap;
import java.util.Map;
import java.util.Scanner;
import java.util.function.Function;

public interface MapFromCsv<T1, T2> {

    public default Map<T1, T2> getMap(String FILE_TO_LOAD, Function<String, T1> foo1, Function<String, T2> foo2){
        Map<T1, T2> mapResult = new HashMap<>();
        File f = new File(FILE_TO_LOAD);
        try (Scanner sc = new Scanner(f)) {
            while (sc.hasNextLine()) {
                String tmp = sc.nextLine();
                String[] tmp2 = tmp.split(";");
                mapResult.put((T1) foo1.apply(tmp2[0]), (T2) foo2.apply(tmp2[1]));
            }
        } catch (FileNotFoundException fntf) {
            System.err.println("File " + f.getPath() + "was not found.");
        } catch (Exception e) {
            System.err.println("Something went wrong with reading " + f.getPath());
        }
        return mapResult;
    }
}

and implementation of the first class for making Map<String, String> goes like this:

package Main;

import java.util.Map;

public enum StringMapFromCsv implements MapFromCsv<String, String> {

    INSTANCE;
    final String FILE_TO_LOAD = "resources/stringMap.csv";
    private Map<String, String> stringMap;

    private StringMapFromCsv() {
        initStringMap();
    }

    private void initStringMap() {
        this.stringMap = getMap(FILE_TO_LOAD);
    }

    public void reloadStringMap() {
        initStringMap();
    }

    public Map<String, String> getStringMap() {
        return stringMap;
    }

}

and implementation of the second class for making Map<String, Double> goes like this:

package Main;

import java.util.Map;

public enum DoubleMapFromCsv implements MapFromCsv<String, Double>{

    INSTANCE;
    final String FILE_TO_LOAD = "resources/doubleMap.csv";
    private Map<String, Double> doubleMap;

    private DoubleMapFromCsv() {
        initDoubleMap();
    }

    private void initDoubleMap() {
        this.doubleMap = getMap(FILE_TO_LOAD);
    }

    public void reloadDoubleMap() {
        initDoubleMap();
    }

    public Map<String, Double> getDoubleMap() {
        return doubleMap;
    }

}

Anu suggestions?

This really looks like you over-designed the solution. And especially tricky since it won't work right now. (You're calling an unknown method getMap(FILE_TO_LOAD), it's missing the 2 mapping functions as parameters).

Why not just create a factory class that creates maps from the CSV files?

public class MapFactory {
    private static final String STRING_FILE = "resources/stringMap.csv";
    private static final String DOUBLE_FILE = "resources/doubleMap.csv";

    //private constructor to prevent pointless instance creation of this class
    private MapFactory() {
    }

    private static Map<String, String> createStringMap() {
        return createMapFromFile(STRING_FILE, Function.identity());
    }

    private static Map<String, Double> createDoubleMap() {
        return createMapFromFile(DOUBLE_FILE, Double::parseDouble);
    }

    public static <T> Map<String, T> createMapFromFile(String inputFile, Function<String, T> parseFunction) {
        Map<String, T> mapResult = new HashMap<>();
        File f = new File(inputFile);
        try (Scanner sc = new Scanner(f)) {
            while (sc.hasNextLine()) {
                String line = sc.nextLine();
                String[] lineParts = line.split(";");
                mapResult.put(lineParts[0], parseFunction.apply(lineParts[1]));
            }
        } catch (FileNotFoundException fntf) {
            System.err.println("File " + f.getPath() + "was not found.");
        } catch (Exception e) {
            System.err.println("Something went wrong with reading " + f.getPath());
        }
        return mapResult;
    }
}

You should really change the names of "stringMap" and "doubleMap" to something more meaningful. I can't do that for you here since you did not provide the actual use of your code.

Notice how I just extracted the common part into it's own method here?

I did throw away the map field so you'll need to re-read the file on each new creation. This greatly depends on how you plan on using these maps. With this solution the different classes that use the maps are free to modify it without breaking any other classes that use the map.

Alternatively you can also save the map in a field and use (thread safe) lazy loading to only initialise the map once, the first time it's used like this:

public class MapFactory {
    private static final String STRING_FILE = "resources/stringMap.csv";
    private static final String DOUBLE_FILE = "resources/doubleMap.csv";

    //private constructor to prevent pointless instance creation of this class
    private MapFactory() {
    }

    private static class StringMapHolder {
        private static final Map<String, String> STRING_MAP =createMapFromFile(STRING_FILE, Function.identity());
    }

    public static Map<String, String> createStringMap() {
        return StringMapHolder.STRING_MAP;
    }
    private static class DoubleMapHolder {
        private static final Map<String, Double> DOUBLE_MAP = createMapFromFile(DOUBLE_FILE, Double::parseDouble);

    }

    public static Map<String, Double> createDoubleMap() {
        return DoubleMapHolder.DOUBLE_MAP;
    }

    private static <T> Map<String, T> createMapFromFile(String inputFile, Function<String, T> parseFunction) {
        Map<String, T> mapResult = new HashMap<>();
        File f = new File(inputFile);
        try (Scanner sc = new Scanner(f)) {
            while (sc.hasNextLine()) {
                String line = sc.nextLine();
                String[] lineParts = line.split(";");
                mapResult.put(lineParts[0], parseFunction.apply(lineParts[1]));
            }
        } catch (FileNotFoundException fntf) {
            System.err.println("File " + f.getPath() + "was not found.");
        } catch (Exception e) {
            System.err.println("Something went wrong with reading " + f.getPath());
        }
        return mapResult;
    }
}


Locating child of a nearby ancestor with jQuery I have the following markup, and I'm wondering how best to locate the nearest previous dpicker input element and give it focus with jQuery when a.dpicon is clicked.

  <input autocomplete="off" class="dpicker" type="text" name="mdpdate" id="mdpdate">

I've used this, and it works, but it seems pretty cumbersome:

$(".dpicon").click(function(){
  $(this).parent().parent().prev().children(".dpicker").focus();
});

Also, there will be times that the 'dpicon' element will be before the input in markup, not after it. Is there a way to catch both instances?

I would make the link between the 2 implicit.

Then the code is

$(".dpicon").click(function(){
  $( this.href ).focus()
}

It is a creative (ab)use of href, but it still follows the spirit in my mind.

If you have the luxury of going HTML5, you can go with a data attribute.

Then the code is

$(".dpicon").click(function(){
  $( this.getAttribute( "data-dpicker" ) ).focus()
}


Inserting using multiple contexts into LocalDB I've written this ActionResult to model an Order that goes through. I couldn't think of another way to write this without using context Db<Sets>. It runs kinda slow, taking around 4 seconds to complete. Is there way to optimize the code or is this kinda has to happen since I'm doing so many inserts?

public ActionResult New(CustomerOrderModel model)
{
    ViewBag.LimitNotExceeded = checkLimit();
    if (ModelState.IsValid)
    {
        int prepaid;
        int userIdLoggedIn = WebSecurity.CurrentUserId;
        try
        {
            //
            //Add Customer
            using (var ctx = new CustomerContext())
            {
                ctx.CustomerInfos.Add(model.CustomerModel);
                ctx.SaveChanges();
            }

            //
            //Add Order
            model.OrderInfo.UserProfileUserID = userIdLoggedIn;
            model.OrderInfo.Customer_id = model.CustomerModel.id;
            model.OrderInfo.DateCreated = DateTime.Now.ToLocalTime();
            using (var ctx = new OrdersContext())
            {
                ctx.OrderInfos.Add(model.OrderInfo);
                ctx.SaveChanges();
            }

            //
            //Add Code
            key = "s121sd";
            KeyCode TheKey = new KeyCode();
            TheKey.code = key;
            TheKey.Customer_id = model.CustomerModel.id;
            TheKey.nif = model.CustomerModel.nif;
            TheKey.UserProfileUserId = userIdLoggedIn;
            TheKey.DateCreated = DateTime.Now.ToLocalTime();
            using (var ctx = new CodesContext())
            {
                ctx.CodesInfos.Add(TheKey);
                ctx.SaveChanges();
            }

            //
            //Assign and Create debts
            using (var ctx = new UsersContext())
            using (var dbx = new DebtsContext())
            {
                prepaid = ctx.UserProfiles.Find(userIdLoggedIn).Prepaid;
                if (prepaid > 0)
                {
                    --prepaid;
                    string query = "UPDATE dbo.UserProfile SET Prepaid='" + prepaid + "' WHERE UserId='" + userIdLoggedIn + "';";
                    ctx.Database.ExecuteSqlCommand(query);
                }
                else
                {
                    Debts NewDebt = new Debts();
                    NewDebt.paid = false;
                    NewDebt.DateCreated = DateTime.Now.ToLocalTime();
                    NewDebt.UserProfileUserId = userIdLoggedIn;
                    NewDebt.Order_id = model.OrderInfo.id;
                    dbx.DebtInfos.Add(NewDebt);
                    int debts = dbx.DebtInfos.Count(q => q.UserProfileUserId == userIdLoggedIn);
                    if (debts <= ctx.UserProfiles.Find(userIdLoggedIn).MaxDebt)
                    {
                        //
                        //Warn them when they reach their last
                    }
                    dbx.SaveChanges();
                    string query = "UPDATE dbo.UserProfile SET Prepaid='" + prepaid + "' WHERE UserId='" + userIdLoggedIn + "';";
                    ctx.Database.ExecuteSqlCommand(query);
                }
            }
            //DateTime a = DateTime.FromBinary(model.OrderInfo.Timestamp);
            return RedirectToAction("Index", "Home");
        }

        catch (DataException ex)
        {
            ViewBag.ChangeResult = ex;
            ModelState.AddModelError("", "Unable to save changes. Try again, and if the problem persists see your system administrator.");
        }
    }
    return View(model);
}

I'm not sure i understand exactly why you have multiple contexts. If you keep to one context you can do one SaveChanges(). That should save you some time in storing data.

Do all your SaveChanges() take equal amount of time?

You could add a timer inside the contexts to print out a console message with time to confirm how long a using statement takes to come to an end.

If you are relying on how long it takes to show in the view, i suggest using a tool like Glimpse or MiniProfiler to give you feedback on time spent rendering/servertime.


Getting names/values of multiple properties Lets say I have one method, which extracts property name and value:

public TModel Get (Expression<Func<object>> param)
{
        using (OracleConnection connection = new OracleConnection(GetConnectionString()))
        {
            connection.Open();

            var propertyName= ((MemberExpression)param.Body).Member.Name;

            var value = param.Compile()();

            // GetTableName() returns table name of TModel
            var query = $"SELECT * FROM {GetTableName()} WHERE {propertyName}='{value}'";

            var output = connection.Query<TModel>(query);
            connection.Dispose();
            return output.FirstOrDefault();

        }
}

and using it as:

var model = Get(() => foo.FirstProperty);

However, if I want to get name and value from the unknown number of properties, I do this:

public TModel Get(params Expression<Func<object>>[] param)
        using (OracleConnection connection = new OracleConnection(GetConnectionString()))
        {
            connection.Open();

            var query = new StringBuilder();

            query.Append($"SELECT * FROM {GetTableName()} WHERE ");
            for (int i = 0; i < param.Length; i++)
            {
                var propertyName = ((MemberExpression)param[i].Body).Member.Name;

                var value = param[i].Compile()();

                query.Append($"{propertyName} = '{value}'");
                if (i + 1 < param.Length) query.Append(" AND ");
            }

            var output = connection.Query<TModel>(query.ToString());
            connection.Dispose();
            return output.FirstOrDefault();
        }
    }

But, implementation looks a bit 'ugly' and verbose:

var model = Get(() => foo.FirstProperty, () => foo.SecondProperty); // and so on

I could send manualy name of the property + value, however, I would like to keep the implementation as simple as possible and less verbose.

Is there any way to simplify this?

If you want to iterate through all of the properties then reflection will help you to do that. You can find more at MS docs
Something like this:

public TModel Get<TParam>(TParam param)
{
        var allParams = typeof(TParam).GetProperties().Select(p => $"{p.Name} ='{p.GetValue(param)}'");
        var condition = string.Join(" AND ", allParams);

        using (OracleConnection connection = new OracleConnection(GetConnectionString()))
        {
            connection.Open();

            // GetTableName() returns table name of TModel
            var query = $"SELECT * FROM {GetTableName()} WHERE {condition}";

            var output = connection.Query<TModel>(query);
            return output.FirstOrDefault();
        }
}

But you have to think how to escape SQL injections. Because the value of query variable in current implementation could have potential vulnerabilities.


Converting a string to a list of key-value pairs I'm trying to convert a string (here after called input) into a vector of key-value pairs, all in C++03 (cannot use C++11, cannot use Boost libs).

The string in question can contain whitespaces. A comma (,) delimits key-value pairs and the equals sign (=) separates the key from the value.
A sample string (also in the code could be: "ab=xy z,time a=1s". Notice the lack of a final ,.

Any improvement is most welcome.

#include <iostream>
#include <sstream>
#include <string>
#include <vector>
#include <map>

struct Flag_name_value {
    std::string name;
    std::string value;
};

std::istringstream& operator>>(std::istringstream& iss, Flag_name_value& fnv)
{
    char ch;
    std::ostringstream oss;

    while (iss >> std::noskipws >> ch) {
        std::cout << ch << "\n";
        switch(ch) {
            case('='):
                fnv.name = oss.str();
                oss.str("");
                break;
            case(','):
                fnv.value = oss.str();
                oss.str("");
                break;
            default:
                oss << ch;
                break;
        }

        if (!fnv.name.empty() && !fnv.value.empty())
        {
            return iss;
        }
    }

    // We end here if the string didn't end with ','
    fnv.value = oss.str();
    return iss;
}

int main () {

    std::string input("ab=xy z,time a=1s");

    std::istringstream iss(input);

    std::vector<Flag_name_value> flags;
    while (true) {
        Flag_name_value f;
        iss >> f;
        flags.push_back(f);
        if (!iss) break;
    }

    return 0;

}

First, you can use std::getline to extract key-value pairs from the original string. This can be done by passing , as a delimiter to std::getline:

while (std::getline(iss, token, ',')) { ... }

Notice that you don't have to perform any checks on the stream this way. Once std::getline will reach the end of the stream it will set the eofbit on it, so the loop condition will evaluate to false on next iteration.

Next, use can use std::string::find to find an index of the occurrence of the = character in strings filled by std::getline:

size_t pos = token.find('=');

After that use can use this index to extract two substrings - your key and your value:

flag.name    = token.substr(0, pos);
flag.value   = token.substr(pos + 1);

Notice that you can omit the second argument of std::string::substr, since it has the default value of std::string::npos. This value is very huge (in fact, it equals to the maximum value that can be represented by a size type of std::string), but std::string::substr never reads past the end of line, so everything will be good.

Tie all this together and you'll get a small and readable solution:

std::string input("ab=xy z,time a=1s");
std::istringstream iss(input);

std::vector<Flag_name_value> flags;

std::string token;
Flag_name_value flag;
while (std::getline(iss, token, ',')) {
    size_t pos   = token.find('=');
    flag.name    = token.substr(0, pos);
    flag.value   = token.substr(pos + 1);
    flags.push_back(flag);
}


Analyze terraform template files to return infrastructure cost I am trying to write a program I am calling "terraform cashier." Terraform is a tool released by hashicorp to allow you to codify your infrastructure. I thought it would be useful to know how much your infrastructure would cost you before you deployed it, and have implemented a prototype of that for AWS. I also had to write a pricing data ingestion tool and API to power this particular tool, but those seem to be working fine, so I'd like to keep the review to the cli tool itself.

The repo can be found here.

I do a lot of type casting in the processTerraformFile function. How can I avoid that? Am I processing the terraform files correctly? A particular ugly segment of the function is below. The main goal of the function is to import the data in the terraform templates so that it can be processed inside of cashier. I'm just identifying the resources and counting them.

arrayOfResources, success := decodedOutput["resource"].([]map[string]interface{})

if success == true {
    for _, resource := range arrayOfResources {
        for key := range resource {
            switch key {
            case "aws_instance":
                resourceKeys, resourceKeysSuccess := resource[key].([]map[string]interface{})
                if resourceKeysSuccess == true {
                    for resourceKey := range resourceKeys[0] {
                        instanceType, instanceTypeSuccess := resourceKeys[0][resourceKey].([]map[string]interface{})[0]["instance_type"].(string)
                        if instanceTypeSuccess == true {
                            masterResourceMap = countResource(masterResourceMap, key, instanceType)
                        }
                    }
                }
            default:
                fmt.Println("resource type not recognized: ", key)
            }
        }
    }
}

I wrote some basic tests as well, but decided to not test the main() function. Is this standard? It started to feel strange when I was making API calls in my test.

Usually, the code is a bit worse, indentation-wise:

if res, ok := decodedOutput["resource"].([]map[string]interface{}); ok {
    for _, resource := range res {
        for key := range resource {
            switch key {
            case "aws_instance":
                if rk, ok := resource[key].([]map[string]interface{}); ok {
                    for resourceKey := range rk[0] {
                        if itype, ok := resourceKeys[0][resourceKey].([]map[string]interface{}); ok {
                            if d, ok := itype[0]["instance_type"].(string); ok {
                                masterResourceMap = countResource(masterResourceMap, key, d)
                            }
                        }
                    }
                }

            default:
                fmt.Println("resource type not recognized: ", key)
            }
        }
    }
}

Now, from a Terraform point-of-view, you already have a semi-assurance that the type you're trying to cast has been cleaned up, this due to how you declare the values of the terraform file. You usually define it with the schema package, so while it's running, you know Terraform already checked the types for you.

The overall I've seen is that providers, for example, do not tests against a true type casting from interfaces, again, due to Terraform pre-checking the information to match the schema definition, so if it is a string but someone passed an int, then Terraform will exit and fail way earlier than reaching your type casting.


Converting numbers to words in Haskell Here's a program I wrote to convert numbers into English words using Haskell.

import Data.Char
import Data.List

type WordNum = String

ones :: (Integral a, Show a) => a -> WordNum
ones 1 = "one"
ones 2 = "two"
ones 3 = "three"
ones 4 = "four"
ones 5 = "five"
ones 6 = "six"
ones 7 = "seven"
ones 8 = "eight"
ones 9 = "nine"
ones n = error (show n ++ " is not a one-digit value")

teens :: (Integral a, Show a) => a -> WordNum
teens 10 = "ten"
teens 11 = "eleven"
teens 12 = "twelve"
teens 13 = "thirteen"
teens 14 = "fourteen"
teens 15 = "fifteen"
teens 16 = "sixteen"
teens 17 = "seventeen"
teens 18 = "eighteen"
teens 19 = "nineteen"
teens n  = error (show n ++ " is not a teen")

tens :: (Integral a, Show a) => a -> WordNum
tens 1 = "ten"
tens 2 = "twenty"
tens 3 = "thirty"
tens 4 = "forty"
tens 5 = "fifty"
tens 6 = "sixty"
tens 7 = "seventy"
tens 8 = "eighty"
tens 9 = "ninety"
tens n  = error (show n ++ " is not a tens place value")

groups :: [WordNum]
groups = ["", " thousand", " million", " billion", " trillion"]

groupToWord :: (Integral a, Show a) => a -> String
groupToWord n
    | n == 0    = ""
    | n < 10    = ones n
    | n < 20    = teens n
    | n < 100   = tens (n `div` 10) ++ ' ' : (groupToWord $ n `mod` 10)
    | n < 1000  = ones (n `div` 100) ++ " hundred " ++ (groupToWord $ n `mod` 100)
    | otherwise = error (show n ++ " is not a 3-digit group")

-- Splits a number into groups in reverse order
splitNum :: (Integral a, Show a) => a -> [a]
splitNum n
    | n <= 999  = [n]
    | otherwise = (n `mod` 1000) : splitNum (n `div` 1000)

numToWord :: (Integral a, Show a) => a -> String
numToWord n
    | n == 0     = "zero"
    | n >= 10^15 = error "Doesn't support numbers bigger than trillions"
    | otherwise  = concat $ intersperse ", " [w ++ g | (w,g) <- reverse (zip wordGroups groups)]
    where
        wordGroups = toWordGroups $ splitNum n

        toWordGroups :: (Integral a, Show a) => [a] -> [WordNum]
        toWordGroups (g:gs) = groupToWord g : toWordGroups gs
        toWordGroups _ = []

It seems like there's a lot of redundancy, and I'm also not very happy with how I had to join the lists. Also, is there a way to do more consing and less appending? I was hoping to cons the groups back together after they were made words so I wouldn't have to reverse the zipped list afterward. Also, as always, I'd really appreciate general comments on improvements to style and best practice.

Pattern matching or list element selection?

It's possible to reduce the size of ones, tens, teens and so on if we use !! on a list instead of pattern matching. That's a completely other style though:

ones :: (Integral a, Ord a) => a -> WordNum
ones n
  | n > 0 && n < 10 = onsies !! fromIntegral n
  | otherwise       = error $ "ones: not a one-digit value"
  where
    onsies = words "one two three four five six seven eight nine"

Note that I removed the Show a constraint, because it's not necessary if our value n is in our range. It's only necessary in the error case. Instead, we should add where the error happened. Newer variants of GHC/base include a call-stack, therefore we will know the malfunctioning line, but it's always nice to know at least the function name. It also follows the base-style error messages, e.g.

head (x:_) = x
head []    = error "Prelude.head: empty list"

Either way, back to the code. Several functions are partial. Usually, you want your exported functions to be total, e.g. they never return _|_ (an infinite loop, an error, undefined,...). So instead of the ones above, we could write

ones :: (Integral a, Ord a) => a -> Maybe WordNum
ones n
  | n > 0 && n < 10 = Just $ onsies !! fromIntegral n
  | otherwise       = Nothing
  where
    onsies = words "one two three four five six seven eight nine"

However you wouldn't export all your helper functions either way, so lets keep the error variant. By the way, I've used words "one..." since my keyboard is slightly broken, but it's the same as ["one","two","three",...].

divMod, map and other standard library functions

Sometimes re-inventing the wheel is fun, but usually we want to try to get the most out of our standard library functions.

divMod

There are several occasions where we use both d = a `div` b and m = a `mod` b. If we use both, we can use (d,m) = a `divMod` b. That prevents typos like

    | n < 100   = tens (n `div` 10) ++ ' ' : (groupToWord $ n `mod` 11)
                                                    -- whoops        ^

So let us use it in splitNum:

splitNum :: Integral a => a -> [a]
splitNum n
    | d == 0    = [n]
    | otherwise = m : splitNum d
  where
    (d,m) = n `divMod` 1000

Note that quotRem will return the same values as divMod on positive numbers and is slightly faster.

zipWith and map

You use zip in your list comprehension just to ++ the zipped elements together. We can achieve this in a single step with zipWith:

[w ++ g | (w,g) <- reverse (zip wordGroups groups)]
= [ w ++ g | (w,g) <- reverse (zipWith (,)  wordGroups groups)]
= [ x      | x     <- reverse (zipWith (++) wordGroups groups)]
= reverse $ zipWith (++) wordGroups groups

toWordGroups is map groupToWord. If we use both functions in numToWord, we end up with

numToWord :: (Integral a, Ord a) => a -> String
numToWord n
    | n == 0     = "zero"
    | n >= 10^15 = error "numToWord: Doesn't support numbers bigger than trillions"
    | otherwise  = concat $ intersperse ", " $ reverse $ zipWith (++) wordGroups groups
    where
        wordGroups = map groupToWord $ splitNum n


Generating readable text in a human language from machine-readable data I have been generating English-language text from some machine-readable data, but now I want internationalization of my script to be relatively easy. The challenge is that some data might be missing and thus should be omitted from the output, possibly precluding any approach involving just "plugging in" numbers.

Currently, I have functions like these, the output concatenated together in the main program:

function UserinfoJsFormatQty(qty, singular, plural) {
    return String(qty).replace(/\d{1,3}(?=(\d{3})+(?!\d))/g, "$&,") + "\u00a0" + (qty == 1 ? singular : plural);
}

function UserinfoJsFormatDateRel(old) {
    // The code below requires the computer's clock to be set correctly.
    var age = new Date().getTime() - old.getTime();
    var ageNumber, ageRemainder, ageWords;
    if(age < 60000) {
        // less than one minute old
        ageNumber = Math.floor(age / 1000);
        ageWords = UserinfoJsFormatQty(ageNumber, "second", "seconds");
    } else if(age < 3600000) {
        // less than one hour old
        ageNumber = Math.floor(age / 60000);
        ageWords = UserinfoJsFormatQty(ageNumber, "minute", "minutes");
    } else if(age < 86400000) {
        // less than one day old
        ageNumber = Math.floor(age / 3600000);
        ageWords = UserinfoJsFormatQty(ageNumber, "hour", "hours");
        ageRemainder = Math.floor((age - ageNumber * 3600000) / 60000);
    } else if(age < 604800000) {
        // less than one week old
        ageNumber = Math.floor(age / 86400000);
        ageWords = UserinfoJsFormatQty(ageNumber, "day", "days");
    } // ...
    return ageWords;
}

It is supposed to generate output like:

  A reviewer and rollbacker, 2 years 9 months old, with 8,624 edits. Last edited 7 hours ago.

The age, edit count, date of last edit, or all of them could be missing. How could I improve my current design without resorting to a tabular display format? (All the information needs to nicely fit into a single status line.)

I see a couple of potential issues with regards to internationalization:

pluralization is more complex than (qty == 1 ? singular : plural). What about the value 0? Also in some languages, different plural forms are in use depending on the number.
numbers should be formatted according to language, using appropriate separators
concatenation (that you mention is done in main program) must be avoided because the order of words and phrases will often vary in translations in different languages

I would advise to :

localize translations using named parameters to be replaced: this solves the ordering issue, and avoids concatenation.
format values, for number formatting and pluralization, using a separate function localized for each language
replace parameters with formatted values in parameterized translations using a template engine: this should keep some potentially buggy regular expressions out of your code :)

You may be interested in having a look at:

JavaScript Internationalisation, a post by Matthew Somerville on 24 Ways
jQuery-global, "a jQuery plugin for the globalization of string, date, and number formatting and parsing"

and last but not least, the i18n API part of the Scalable JavaScript Application framework, which I designed for Legal-Box :)

eric-brechemier/lb_js_scalableApp, the project home page on GitHub
lb.core.Sandbox.js, check the i18n part of the API, methods starting with "i18n."

I may be able to provide more practical suggestions if you show more of your code, especially at the "top", part of the main program.


What do you think of this improvement of Linq's GroupBy method? Linq already has a set of GroupBy methods for enumerable, which returns IEnumerable<IGrouping<TKey, TSource>>. I dislike this return type because it means I have to do a where query everytime I want to find a group. I try to make something that returns a dictionary instead, as below:

 public static class GroupByExtension
    {
        public static Dictionary<TKey, List<TSource>> GroupToDictionary<TSource, TKey>(
            this IEnumerable<TSource> source,
            Func<TSource, TKey> keySelector)
        {
            return source.GroupBy(keySelector).ToDictionary
                (grouping => grouping.Key, grouping => grouping.ToList());
        }
    }

I can test it like this

List<int> list = new List<int>() { 1, 2, 33, 4, 20, 43, 21, 93, 26, 31, 113 };
Dictionary<int, List<int>> numbersGroup = list.GroupToDictionary(i => i / 10);
//group number by result of division by ten.

So, I can call numbersGroup[1], numbersGroup[3], etc. as opposed to the more clumsy and less efficient Where's clauses if I only have IEnumerable<IGrouping<TKey, TSource>>

IEnumerable<IGrouping<int, int>> numbersGroup2 = list.GroupBy(i => i / 10);
IGrouping<int, int> group =
            numbersGroup2.Where(grouping => grouping.Key == 3).First();
//and this only returns an IGrouping, not a proper list,
//and where can be expensive for large list because it uses O(n) IIRC
//the first() could have also been avoided because we know that the keys are unique

What do you think of this code? What drawback do you see? And what is the reason Microsoft use  IEnumerable<IGrouping<TKey, TSource>>

I think the reason it's done this way is to make GroupBy() consistent with the rest of LINQ: everything is as lazy as possible, so that following queries can be efficient.

And there already is a method that does almost the same thing as your GroupToDictionary(), it's called ToLookup().


Sphere-Plane collision test I'm worried about my own attempt at a solution for a Sphere-Plane collision testing. I originally implemented a solution I found on a mix of Game Development websites around the place, which was similar enough to the following:

bool intersecting(BoundingSphere const& s, Vector3 const& s_origin, BoundingPlane const& p, float const p_distance) {
    Vector3 const p_origin = p.normal * p_distance;
    Vector3 const cp = s_origin + p.normal * dot((p_origin - s_origin), p.normal);

    return (distance2(s_origin, cp) < s.radius * s.radius);
}

But I have recently spent a bit more time doing linear algebra since and basic geometry, and decided upon revising the above solution (to see if I understood the problem space better).

In thinking about it as a simple check: distance of a point from a line being less than the radius. I came up with the following:

bool intersecting2(BoundingSphere const& s, Vector3 const& s_origin, BoundingPlane const& p, float const p_distance) {
    return abs<float>(dot(p.normal, s_origin) - p_distance) < s.radius;
}

It has so far passed all my tests practically, but I still worry about my mathematical ability and I don't want an edge case to bite me in the ass that I might have missed. One thing I have definitely noticed is the solution is highly sensitive to the normalised form of the plane normal. If it is simply a rough estimation, ie not truly normalised, it will fail completely (exponential error).

The fact I never saw a solution that short worries me haha.

edit: My testbed: http://pastie.org/3512889

proposed tags: collision-detection && math

I'm not sure what kind of error your getting but here is a very simple collision test:

#include <cmath>
#include <iostream>

class float3 {
        public:
                float x,y,z;
};

class float4 {
        public:
                float a,b,c,d;
                float norm() const{
                        return sqrt(a*a+b*b+c*c);
                }
};

typedef float4 plane;

class sphere {
        public:
                float3 position;
                float radius;
};

float project( const float3 & spherePos, const plane & p){
        return std::abs( spherePos.x*p.a + spherePos.y*p.b + spherePos.z*p.c + p.d );
}

bool const collision( const sphere & s, const plane & p){
        return project(s.position,p)/p.norm() < s.radius;
}

int main( int argc, char * argv[]){

        plane p = {0.0,-1.0,0.0, 2.0 };
        sphere s = { {0.0,1.5,0.0}, 1.0 };

        if ( collision(s,p) )
                std::cout << "Collision detected" << std::endl;

}

The advantage here is that you don't need a normalized plane. The line that bothers me is this:

return abs<float>(dot(p.normal, s_origin) - p_distance) < s.radius;

Just write your solution such that it can handle un-normalized planes. If you're working with normalized planes (which may be what is contained in "p.normal"?) I believe you need :

return abs<float>(dot(p.normal, s_origin) + p_distance) < s.radius;

See the wolfram page on this topic - it is straight forward. It is not clear how you have implemented BoundingPlane - but if you are getting errors, that would be the first place to look.


Implementing BST in javascript Is this an efficient and clean implementation?

function BST(items) {
    this.tree = {};
    if (items) items.forEach(this.add, this);
}

BST.prototype.add = function(item) {
    if (!this.tree.value) {
        this.tree.value = item;
        return item;
    }
    var currNode = this.tree;
    var inserted = false;
    while (!inserted) {
        if (item > currNode.value) {
            if (currNode.rightNode) currNode = currNode.rightNode;
            else {
                currNode.rightNode = {
                    value: item
                };
                inserted = true;
            }
        } else {
            if (currNode.leftNode) currNode = currNode.leftNode;
            else {
                currNode.leftNode = {
                    value: item
                };
                inserted = true;
            }
        }
    }
    return item;
};

BST.prototype.printTree = function() {
    console.log(this.tree);
};

Interface

Why return the value added? That's unusual and unexpected.

Bug

It's unusual to allow adding duplicate values.
It would be better to ignore.

Avoid flag variables when possible

The flag variable inserted is unnecessary.

while (true) {
    if (item > currNode.value) {
        if (currNode.rightNode) currNode = currNode.rightNode;
        else {
            currNode.rightNode = {
                value: item
            };
            return;
        }
    } else {
        if (currNode.leftNode) currNode = currNode.leftNode;
        else {
            currNode.leftNode = {
                value: item
            };
            return;
        }
    }
}

I replaced the insert = true statements with return.
If you really want to return item, then you can either use that instead of return, or use break.