Remove a key from a hash table efficiently I am trying to remove a key from a hash table, which is built using linear probing. Removing a key works fine but I know I need to rehash a portion of the table after the removal but my method down below is not quite efficient. It basically rehashes the whole table that is unnecessary. Is there anyway that I could improve the performance while rehashing.

bool remove(int x, int H[], int m)
{
    int hashVal = hash(x, m);
    for (int i = 0; H[(hashVal + i) % m] != EMPTY; i++)
    {
        if (H[(hashVal + i) % m] == x)
        {
            H[(hashVal + i) % m] = EMPTY;

           //Rehash the table
           //I think the problem is here. This for loop condition can be better
            for (int k = 1; H[(hashVal + i + k) % m] != EMPTY ; k++)
            {
                int keyShiftUp = H[(hashVal + i + k) % m];

                H[(hashVal + i + k) % m] = EMPTY;

                insertLP(keyShiftUp, H, m);

                cout << "keyShiftUp: " << keyShiftUp << ends;
            }
            return true;
        }
    }
return false;
}

C++, not C

First things first, I note that remove is a free function. But that's not how we would do something like this in C++. We would prefer to have a type HashTable in which we'd write:

class HashTable {
public:
   ...
   bool remove(int key);
   ...

private:
   int hash(int );

   std::vector<int> table; // vector is better than int*
                           // table is a better name than H
};

Your naming also could use some work - what is x and what is m? I'm guessing m is the capacity and x is the key that we're erasing, but where are the values? We just have int H[], so is this more like a hash set than a hash table? It's unclear.

Looping and separation of concerns

The hash function should just do the hashing (and thus shouldn't take in the capacity). The loop itself should loop over slots, not indices - this will localize your modulo operation so it doesn't creep in everywhere:

int next(int i) {
    return (i+1) % table.size();
}

bool remove(int key) {
    int i = hash(key) % table.size();
    if (table[i] == EMPTY) {          // break early
        return false;
    }

    for (; table[i] != EMPTY; i = next(i)) {
        if (table[i] == key) {
            table[i] = EMPTY;

            for (int k = next(i); table[k] != EMPTY; k = next(k)) {
               ...
            }

            return true;
        }
    }

    return false;
}

Once you reorient looping over additive indices to looping over actual indices, the inner body becomes more straightforward too:

int old_key = table[k];
table[k] = EMPTY;
insert(old_key);

Find the slot

Your i loop is all about finding the slot where the key is at. The body of that loop isn't really a loop, it's a find-type algorithm. So let's rewrite it like one:

bool remove(int key) {
    int i = hash(key) % table.size();
    for (; table[i] != key; i = next(i)) {
        if (table[i] == EMPTY) {          // break early
            return false;
        }
    }

    // now, table[i] == key
    table[i] = EMPTY;
    for (int k = next(i); table[k] != EMPTY; k = next(k)) {
        int old_key = table[k];
        table[k] = EMPTY;
        insert(old_key);
    }

    return true;
}

EMPTY

In addition to not really being a hash table, you're seemingly limiting the possible valid keys to exclude something like 0 or -1 or whatever the value of EMPTY is. May be worth separating the key from the empty concept.


HTML/Attrs Tester Environment I have a few objectives (which are working fine, but I want tips in case I can improve it).

Let me try to make this as clear as possible, as this is a much larger project:

There are three move categories:

Hammer (10 damage)
Scythe (5 damage)
Meditate (0 damage)

I will be having hundreds of additions, but these values above are static indefinitely. I'm trying to update a total damage count, but the kicker is that the player can change this move type in the middle of adding other things.

Essentially, I need it to display an accurate estimated damage count based on other values that will end up occurring (this is entirely GUI based, I have the back-end data working).

HTML:

<input type="radio" value="hammer" class="hammer" data-damage="10" name="attackTypeP1" />
Hammer (10)
<input type="radio" value="scythe" class="scythe" data-damage="5" name="attackTypeP1" />
Scythe (5)
<input type="radio" value="meditate" class="meditate" data-damage="0" name="attackTypeP1" />
Meditate (0)

<input type="button" id="addDmg" value="Add Damage" />
<input type="button" id="remDmg" value="Remove Damage" />

Total

0

JS:

$(function() {
    var dom = $('#total');
    var total = 0;
    var moveVal = 0;
    $('#addDmg').on('click', function() {
        total += 1;
        dom.text(total + moveVal);
    });
    $('#remDmg').on('click', function() {
        total -= 1;
        dom.text(total + moveVal);
    });
    $(':radio').on('click', function() {
        moveVal = $(this).data('damage');
        dom.text(total + moveVal);
    });
});

3 questions:

I'm using data-attr via HTML to find the values. I understand it would be quicker to run this in an array or something, but if it has to traverse the DOM anyway to find out which one was selected, is there any real performance upgrade opposed to using data in HTML?
If these are the only radio buttons on the page, is the way I've built my :radio click function reasonable? Welcoming any tips you could provide if somebody else has advice to make this faster.
Is using a variable for moveVal pretty much the only way to tackle this without adding all the other additions and whatnot EVERY time?

Maybe this will make more sense if I show a full demo?

Play: Js Fiddle: http://jsfiddle.net/epZzP/

Ahh, the origins of a simulator. I have always wanted to create one of those. Anyways...

I suggest you model this all in the JS. If you follow MVC, then all data (or most of it) live in the Model, the data storage. The View would be the HTML, and all event handlers like clicks, change, hovers, live in the controller. Controller updates the model data, like add/remove damage and the view updates values when the model data changes.

Using MVC, or a derivative, allows separation of concerns. For instance, you can easily swap HTML and avoid breaking the logic that controls actions and calculations.


Optimizing bilinear interpolation I have spent countless hours trying to speed up my bilinear interpolation up. I even tried implementing an SSE version (a double version and a float version), but that was even slower than this version.

Does anyone have any ideas?

I wrote this under VS2010 and it is intended to be called from MATLAB as a MEX function (thus the minus one subtraction in interp2_mx because indexing in MATLAB is from 1:end as opposed to 0:end-1).

My function interp2 requires a T (intended float/double type) pointer to Z (mz by nz matrix) and interpolates the points in XI and YI into ZI (which are all the same size mi by ni).

template <typename T>
__forceinline void interp2_mx(const T& x, const T& y,
                          const T* z,
                          const int32_t& n,
                          const int32_t& mm2,
                          const int32_t& nm2,
                          T& val,
                          const T& extrapval = T(0))
{
    int64_t xp = (int64_t)(x) - 1;      // adjust for MATLAB indexing
    int64_t yp = (int64_t)(y) - 1;
    if (xp < 0 || xp > nm2 || yp < 0 || yp > mm2)
    {
        val = extrapval;
    }
    else
    {
    const T* line = z + yp * n + xp;
    T xf = x - (int64_t)(x);   // get decimal portion
    T yf = y - (int64_t)(y);
    T x1mf = (T)1 - xf;
    T y1mf = (T)1 - yf;

    T v00 = x1mf * y1mf * (*(line));
    T v01 = xf * y1mf * (*(line + 1));
    T v10 = x1mf * yf * (*(line + n));
    T v11 = xf * yf * (*(line + n + 1));
    val = v00 + v01 + v10 + v11;
    }
}

template <typename T>
void interp2(const T* z,
         const int32_t& mz, const int32_t& nz,
         const T* xi, const T* yi,
         const int32_t& mi, const int32_t& ni,
         T* zi,
         const T& extrapval = T(0))
{
const int32_t nzm2 = nz - 2;
const int32_t mzm2 = mz - 2;
#pragma omp parallel for
for (int m = 0; m < mi; ++m)
{
    T* line_zi = zi + m * ni;
    const T* x = xi + m * ni;
    const T* y = yi + m * ni;
    for (int n = 0; n < ni; ++n, ++x, ++y, ++line_zi)
    {
        interp2_mx((*x), (*y), z, nz, mzm2, nzm2, (*line_zi));
    }
 }
}

Profile It!
As suggested in the comments, you should profile the code to see where the slowdown is.
Algebra Is Your Friend
One small suggestion I can offer is some algebraic improvements.
A linear interpolation between a and b can be formulated like this:
result = a * interp + b * (1 - interp);

That's 2 adds and 2 multiplies. You can use some algebra to reduce that to:
result = interp * (a - b) + b;

which is 2 adds and 1 multiply. Doing that also removes the need to calculate x1mf and y1mf. So your inner loop would become:
T xf = x - (int64_t)(x);   // get decimal portion
T yf = y - (int64_t)(y);

T a = *line;
T b = *(line + 1);
T top = xf * (a - b) + b;
T c = *(line + n);
T d = *(line + n + 1);
T bottom = xf * (c - d) + d;
val = yf * (top - bottom) + bottom;

That takes you from 8 multiplies to 3, but from 9 adds to 10. So that's likely from 17 instructions to 13.
There are a few more optimizations you can do:
You're subtracting 1 from x and y every time through the loop. You could calculate the values with the 1 already subtracted out in interp2() rather than in interp2_mx():
for (int m = 0; m < mi; ++m)
{
    T* line_zi = zi + m * ni;
    const T* x = xi + m * ni - 1; // <- Do this here instead of on every loop iteration!
    const T* y = yi + m * ni - 1; // <- Same here

You might also have luck by keeping track of a pointer to the next input pixel just incrementing it instead of calculating line every time through the loop, too.
Naming
It would be nice if you could pick more easily understandable names for your functions and variables. Why is this called interp2()? What is interp1()? You should name it bilinearInterp() or something obvious like that. And what does interp2_mx() mean?
Using x and y to represent horizontal and vertical offsets is fine. Those are well-established traditions. But you're naming the start of your image z which is really weird. z is usually reserved for depth, at least when used with x and y. That makes your code very confusing to read.
Likewise, mz and nz are confusing, too. And even worse in iterp2_mx(), mm2 and nm2 are not only confusing but easy to mix up! mz and nz should be something like imageWidth and imageHeight (or matrixWidth and matrixHeight, or inputWidth and inputHeight), and the other variables and arguments should be appropriately named as well.
Why do xi and yi have an i in their names? That makes them look like integers, but they're declared as type T, which you say will likely be float or double.
Errors
Looking at your error handling, I'm not convinced it's sufficient. You check if xp is less than 0 or greater than nm2, and the same for yp. But what happens when xp exactly equals nm2? line ends up pointing to the last pixel in a line. But then you calculate line + 1. That gets you the first element of the next row. And for yp, it means reading a row that doesn't exist when yp equals mm2.


Filtering a list based on a value Based on dropdown value and search text I need to filter a list and return.

Method 1

if (selectedValue == 1 && searchTerm.Length == 0)
return users;

if (selectedValue == 1 && searchTerm.Length > 0)
return users.Where(user=>user.Name == searchTerm).ToList();

if (selectedValue == 2 && searchTerm.Length > 0)
return users.Where(user=>(user.Name == searchTerm && user.Street == "StreetA")).ToList();

if (selectedValue == 2 && searchTerm.Length == 0)
return users.Where(user=>user.Street == "StreetA").ToList();

if (selectedValue == 3 && searchTerm.Length > 0)
return users.Where(user=>(user.Name == searchTerm && user.Street == "StreetB")).ToList();

if (selectedValue == 3 && searchTerm.Length == 0)
return users.Where(user=>user.Street == "StreetB").ToList();

Method 2

if(searchTerm.Length > 0)
{
    if (selectedValue == 1)
    return users.Where(user=>user.Name == searchTerm).ToList();

    if (selectedValue == 2)
    return users.Where(user=>(user.Name == searchTerm && user.Street == "StreetA")).ToList();

    if (selectedValue == 3)
    return users.Where(user=>(user.Name == searchTerm && user.Street == "StreetB")).ToList();
}
else
{
    if (selectedValue == 1)
    return users;

    if (selectedValue == 2)
    return users.Where(user=>user.Street == "StreetA").ToList();

    if (selectedValue == 3)
    return users.Where(user=>user.Street == "StreetB").ToList();
}

I am not convenienced with above logics so please help improve this.

users.Where(user => cond && cond2) is equivalent to users.Where(user => cond).Where(user => cond2)

So you can do the following:

IEnumerable<User> resultStream = users;

if(searchTerm.Length > 0)
    resultStream = resultStream.Where(user.Name == searchTerm);

switch(selectedValue){
    case 1:
        return resultStream.ToList();
    case 2:
        return resultStream.Where(user=>user.Street == "StreetA").ToList();
    case 3:
        return resultStream.Where(user=>user.Street == "StreetB").ToList();
}


Random Function Bias Tester I wanted to create a short little piece of code so that I could see the biases in the random function.

import random
import statistics
import time

num = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
average = []

while True:
    randomNum = random.randint(0, 9)
    num[randomNum] += 1
    average.append(randomNum)
    print(f"\nThe average for all the numbers is {round(statistics.mean(average), round(sum(num) / 20))}")  # The second part of the round function is to keep as few digits on screen while still showing changes with every number. 20 is the arbitrary number I found worked best.
    print(f"The most common number is {num.index(max(num))}\n")
    time.sleep(0.5)

Since I am new-ish to python I wanted to know if, even though its only 15 lines, that I am following good coding practices. That way when my code gets more complicated, I don't have to worry about that as much

Hard coding your data can be lengthy and it can restrict flexibility when carrying out further analysis. You can shorten your code with list comprehension.

import random

n = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]

def main(val):
  ls = [i for i in random.choices(n, k=val)]
  ns = sum(ls)/len(ls)
  print('Length of Data : {}\nSum of Data    : {}\nAverage of Data: {}\nRandom Gen Data: {}'.format(len(ls), sum(ls), ns, ls))

main(100) #Enter amount of data you want to pass through the function

This returns:

Length of Data : 100
Sum of Data    : 399
Average of Data: 3.99
Random Gen Data: [7, 4, 0, 8, 8, 0, 6, 7, 6, 4, 7, 0, 0, 0, 9, 4, 3, 0, 0, 7, 0, 4, 6, 5, 3, 0, 6, 0, 7, 0, 0, 4, 3, 7, 7, 9, 5, 3, 5, 2, 0, 4, 1, 1, 5, 8, 9, 0, 3, 2, 7, 5, 3, 3, 3, 3, 7, 5, 7, 5, 7, 1, 6, 2, 7, 7, 5, 1,6, 6, 4, 7, 8, 2, 0, 1, 5, 3, 5, 4, 9, 7, 7, 2, 0, 3, 4, 6, 3, 4, 3, 5, 6, 3, 0, 1, 3, 8, 0, 1]

It isn't necessary to use a while loop in this instance because you will still need to break your loop which is the same as giving a start and end value to a random range reference.


Sorting an array of strings in C using insertion sort I am a Java programmer who has C this semester. Needless to say, my writing style is heavily influenced by Java, which is probably not a great thing. I'd be grateful for a few nudges in the right direction. Here's the code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define LEN 100
#define WID 80

void sort(char s[][WID], int n);
void display(char s[][WID], int n);

int main()
{
    int n, i;
    char s[LEN][WID];
    printf("Enter number of strings : ");
    scanf("%d", &n);
    printf("Enter strings one by one :\n");
    for (i = 0; i < n; i++) {
        printf("%d : ", i + 1);
        scanf(" %s", s[i]);
    }

    printf("The initial elements are:\n");
    display(s, n);
    sort(s, n);
    printf("The elements after sorting are:\n");
    display(s, n);

    return 0;
}

void sort(char s[][WID], int n)
{
    int i, item;
    char elem[WID];

    for (item = 1; item < n; item++) {
        strcpy(elem, s[item]);
        for (i = item; strcmpi(s[i - 1], elem) > 0 && i > 0; i--) {
            //strcpy(s[i], s[i - 1]);
        }
        memcpy(&s[i + 1], &s[i], (item - i) * WID);
        strcpy(s[i], elem);
    }
}

void display(char s[][WID], int n)
{
    int i;
    printf("\n\n");
    for (i = 0; i < n; i++) {
        printf("%s ", s[i]);
    }
    printf("\n\n");
}

If you see something blasphemous, don't hesitate to call it out.

Never read strings with scanf like this:

scanf(" %s", s[i]);

This allows the user of the program to enter arbitrarily long text which can overwrite all of the programs memory.
There are several options of how to do this safely, one would be to use fgets which allows to specify the maximum length the input buffer can store.
Make sure the input is terminated with '\0' if you do this.
Same goes for the number of strings to read: You never check the counter against LEN.
In sort elem is used as temporary storage, so a better name would be temp.
Last but not least: You perform an awful lot of copying. This is totally unnecessary - you should really read up about pointers in C (they are somewhat similar to references in Java). Allocate the input strings dynamically and then just swap pointers instead of copying all those strings around.
You won't be able to avoid learning about memory management and pointers in C so might as well dig into it.


Python wrapper class around HTTP API I wrote this little class to wrap around a work-in-progress, poorly documented API and I'd like to know what else can be improved. I feel like there's a lot of repetition in the functions and I'd love to know if I could turn all those functions (which do the same thing, just setting a different command in the request object) into just one.

import json
import requests

BASE_URL = "https://chaos.aa.net.uk/"

class Chaos(object):
    """This class allows access to the Andrews & Arnold API.

    Note that it is based on trial and error, there is very little
    official documentation about this API yet, so use at your own risk.
    """

    def __init__(self, username, password):
        """Initialize the class.

        Use the same credentials as on control.aa.net.uk.

        Args:
            username: username like xx00@x
            password: self-explanatory
        """
        self.session = requests.session()
        self.session.headers["User-Agent"] = "Python Chaos Client"
        self.session.auth = (username, password)

    def _request(self, **kwargs):
        """Make an API request, lets Requests check the HTTP status code
        then checks if the "error" string is present in the response
        and raises an exception if that's the case.

        Args:
            **kwargs: will be passed as-is to python-requests
        Returns:
            a dict representation of the APi'S JSON reply
        Raises:
            Exception: the remote server returned an error
        """

        resp = self.session.post(BASE_URL, **kwargs)

        if resp.status_code != requests.codes.ok:
            resp.raise_for_status()

        resp = resp.json()

        if "error" in resp:
            raise APIError(resp["error"])

        return resp

    def info(self, **kwargs):
        return self._request(json={**kwargs, **{"command": "info"}})

    def change(self, **kwargs):
        required = ["broadband", "sim", "voip"]

        if not any(arg in required for arg in kwargs):
            raise InvalidParameters("Missing object of types: " + ", ".join(required))

        return self._request(json={**kwargs, **{"command": "change"}})

    def check(self, **kwargs):
        required = ["order"]

        if not any(arg in required for arg in kwargs):
            raise InvalidParameters("Missing object of types: " + ", ".join(required))

        return self._request(json={**kwargs, **{"command": "check"}})

    def preorder(self, **kwargs):
        required = ["order"]

        if not any(arg in required for arg in kwargs):
            raise InvalidParameters("Missing object of types: " + ", ".join(required))

        return self._request(json={**kwargs, **{"command": "preorder"}})

    def order(self, **kwargs):
        required = ["order"]

        if not any(arg in required for arg in kwargs):
            raise InvalidParameters("Missing object of types: " + ", ".join(required))

        return self._request(json={**kwargs, **{"command": "order"}})

    def usage(self, **kwargs):
        required =  ["broadband", "sim", "voip"]

        if not any(arg in required for arg in kwargs):
            raise InvalidParameters("Missing object of types: " + ", ".join(allowed))

        return self._request(json={**kwargs, **{"command": "usage"}})

    def availability(self, **kwargs):
        required = ["broadband"]

        if not any(arg in required for arg in kwargs):
            raise InvalidParameters("Missing object of types: " + ", ".join(required))

        return self._request(json={**kwargs, **{"command": "availability"}})

class ChaosException(Exception):
    """Base class for all our exceptions.
    """
    pass

class InvalidParameters(ChaosException):
    """Indicates a problem with the request's arguments.
    """
    pass

class APIError(ChaosException):
    """Indicates an error returned by the remote server.
    """
    pass

You're right, you can cut down on the repetition of code. Although bundling all of the functions into one like you mention would work, you lose a bit of the readability that keeping them as class methods gives. Instead, you could just have an internal _command method that collects all of the repeated code:

def _command(self, command_name, required, **kwargs):
    if not any(arg in required for arg in kwargs):
        raise InvalidParameters("Missing object of types: " + ", ".join(required))
    return self._request(json={kwargs, {"command": command_name }})

Note that I've corrected a syntax error in the last line: your parameter used to read json={**kwargs, **{"command": command_name }}.

This makes your Chaos class a lot shorter:

class Chaos(object):
    """This class allows access to the Andrews & Arnold API.

    Note that it is based on trial and error, there is very little
    official documentation about this API yet, so use at your own risk.
    """

    def __init__(self, username, password):
        """Initialize the class.

        Use the same credentials as on control.aa.net.uk.

        Args:
            username: username like xx00@x
            password: self-explanatory
        """
        self.session = requests.session()
        self.session.headers["User-Agent"] = "Python Chaos Client"
        self.session.auth = (username, password)

    def _request(self, **kwargs):
        """Make an API request, lets Requests check the HTTP status code
        then checks if the "error" string is present in the response
        and raises an exception if that's the case.

        Args:
            **kwargs: will be passed as-is to python-requests
        Returns:
            a dict representation of the APi'S JSON reply
        Raises:
            Exception: the remote server returned an error
        """

        resp = self.session.post(BASE_URL, **kwargs)

        if resp.status_code != requests.codes.ok:
            resp.raise_for_status()

        resp = resp.json()

        if "error" in resp:
            raise APIError(resp["error"])

        return resp

    def _command(self, command_name, required, **kwargs):
        """Make an API request, checking that the arguments are valid

        Args:
            command_name: the command name to be passed to the API
            required: a list of names of required arguments
            **kwargs: will be passed as-is to python-requests
        Returns:
            a dict representation of the APi'S JSON reply
        Raises:
            InvalidParameters: the wrong set of arguments was given
        """

        if not any(arg in required for arg in kwargs):
            raise InvalidParameters("Missing object of types: " + ", ".join(required))

        return self._request(json={kwargs, {"command": command_name }})

    def info(self, **kwargs):
        return self._command('info', [], **kwargs)

    def change(self, **kwargs):
        return self._command('change', ["broadband", "sim", "voip"], **kwargs)

    def check(self, **kwargs):
        return self._command('check', ["order"], **kwargs)

    def preorder(self, **kwargs):
        return self._command('preorder', ["order"], **kwargs)

    def order(self, **kwargs):
        return self._command('order', ["order"], **kwargs)

    def usage(self, **kwargs):
        return self._command('usage', ["broadband", "sim", "voip"], **kwargs)

    def availability(self, **kwargs):
        return self._command('usage', ["broadband"], **kwargs)

Since _request only ever gets called by _command you could compress this further by making them one function, but it's fine as is.

As a side note, this code only checks that the minimum set of arguments is given. Depending on your API, it might be a problem if you give extra arguments that aren't required or supported.


Quiz application with tkinter I am making a quiz app using tkinter in Python. I will be having a database that will have questions and options to display in application. Is it good to structure the program as shown, or should it be re-structured in some other way?

import tkinter as tk

def iQuiz(source,row=0,column=0,rowspan=None,columnspan=None):
    storeObj=tk.Frame(source,bg="powder blue")
    storeObj.grid(row=row,column=column,rowspan=rowspan,columnspan=columnspan)
    return storeObj

def button(source,text,command=None,row=0,column=0,rowspan=None,columnspan=None):
    storeObj=tk.Button(source,text=text,command=command)
    storeObj.grid(row=row,column=column,rowspan=rowspan,columnspan=columnspan)
    return storeObj

def radioButton(source,text,indicator='off',command=None,row=0,column=0,rowspan=None,columnspan=None):
     storeObj=tk.Radiobutton(source,text=text,indicator=indicator,command=command)
    storeObj.grid(row=row,column=column,rowspan=rowspan,columnspan=columnspan)
    return storeObj

def label(source,text,row=0,column=0,rowspan=None,columnspan=None):
    storeObj=tk.Label(source,text=text)
    storeObj.grid(row=row,column=column,rowspan=rowspan,columnspan=columnspan)
    return storeObj

class Application(tk.Frame):

    def __init__(self,title):
        tk.Frame.__init__(self)
        self.master.title(title)
        self.option_add('*Font','aerial 20 bold')
        self.pack(expand=tk.YES, fill=tk.BOTH)

        #widget for quiz app
        frame=iQuiz(self,column=1)
        frame.config(width=1000,height=500)
        frame.grid_propagate(0)
        que_label=label(frame,"question is here",row=1)
        opt1=radioButton(frame,"Option A is here",row=2)
        opt2=radioButton(frame,"Option B is here",row=3)
        opt3=radioButton(frame,"Option C is here",row=4)
        opt4=radioButton(frame,"Option D is here",row=5)
        ans_label=label(frame,"\nanswer will be shown on click of show me answer",row=6)
        answer=button(frame,"Show me answer",row=7)

        frame_pre=iQuiz(self,column=0)
        pre=button(frame_pre,'Previous')
        pre.config(bg='Powder blue',fg='white')

        frame_next=iQuiz(self,column=2)
        nxt=button(frame_next,'Next')
        nxt.config(bg='powder blue',fg='white')

if __name__=='__main__':
    Application('Quiz App').mainloop()

There are many things to discuss about your code and I have two options to do do that:

Redesign your program by applying a suitable design pattern.
Obey to my legendary laziness and point out only the minimum things you should care about.

For some considerations, I chose the second option. So here are some random notes you should ponder about:

Do not confuse me

Before diving in any details, I see a bunch of functions and a class. That was the first thing that attracts my attention. This is confusing  because it does not let the maintainer of your code to understand the ideology of your program and whether you belong to the OOP or procedural sects.

Opting only for one of those programming types will help to understand the flow logic of your program. This will also reflect you are not versatile but self-confident and a good decision maker.

As developers, we need to take a set of decision before coding anything. This is a big chance for us to train ourselves to take the right decisions. And the decision regarding which programming type to adopt for your program is not that difficult. Your program needs to comply to an OOP approach because:

You are designing a GUI, so do not forget that the leaves of history inform us that object-oriented concept is the legitimate child of the simulation and graphical design.
Python is FCF programming language. So be pythonic by adopting OOP since even functions are objects in this language.

We do not need to be more gossip because this is already convincing on why
you should write those functions in a proper class instead.

You can be monadic

Let me pick this function, for instance:

button(source,text,command=None,row=0,column=0,rowspan=None,columnspan=None)

Parameters are heretics that must be used parsimoniously because they are a different level of abstraction. The ideal is, when possible, to use monadic functions; and in your situation this is possible because row, column, columnspan and rowspan are already options of the grid() geometry manager, so it is useless1 to inject them as parameters to the enclosing functions. For the command and bg parameters,you also can inject them directly as an option to the widgets you need to use it for (tkinter.Button() in this function)

Container's type as a name: bad!

You used function names such as button() and label() while you already have widget types which names are Button() and Label(). Do not use the container's type as a name for your functions. This is a source of confusion as stated by Robert C. Martin in his book Clean code.

Function names must divulge the function's purpose:

What about this function name you used: iQuiz()? You should rename this function to be informative in that if Donald Trump reads the name of that function he can instantly understand its purpose.

Choose the right names for variables

Your program is small, so maybe I can be enough smart and patient to guess, but also remember, why these variables are useful for: opt1, opt2, opt3, opt4. As your program grows, these variables will make your source code unclear and thus difficult to maintain. Also, if I am a mathematician, I may tend to understand that opt stands for optimum or anything else you do not think.

Code a lighter initializer

You are doing too much things in the initializer __init__(). Initialize only the necessary things and delegate everything else to other functions.

init() takes a useless argument

The string argument you provide to init() is a bad decision. A good practice is to use as less parameters as possible. Plus, you can simply set the title of your GUI in a separate configuration function like this one:

def configure_gui(self):
    self.window_height = 500
    self.window_width = 1000
    self.background_color = 'powder blue'
    self.master.title('Quiz')
    self.master.option_add('*Font','aerial 12')
    self.master.config(width=self.window_width, height=self.window_height)

and use this configuration function within __init__():

def __init(self, master):
   self.master = master
   self.configure_gui()

Improve readability

Put spaces between the the coma and the option in all similar situations to this one:

storeObj.grid(row=row,column=column,rowspan=rowspan,columnspan=columnspan)

Put space also for your variables. I mean you have many situation like this one: pre=button(frame_pre,'Previous') which should be written: pre = button(frame_pre,'Previous')

Remove unpractical graphical choices

You set the font weight to bold and font seize to 20: that is too much. This will lead to an ugly text, and you punish yourself if one of the answers or questions contains a half dozen or more of words.

1. But also may be harmful for the scalability of your program.


Beginner solution to basic quiz exercise, a la Gophercises I just started golang development about two weeks ago and recently finished the recommended introduction book.

I'm now working my way through Gophercises - a sort of collection of exercises to improve beginner's understanding of Golang through small projects.

This is my solution to the first project: writing a quiz (cli-)application.

The requirements are simple:

Read a csv file, each line consisting of a question and an answer:

5+5,10
1+1,2
8+3,11
1+2,3
8+6,14
3+1,4
1+4,5
5+1,6
2+3,5
3+3,6
2+4,6
5+2,7

Print the question to the user
Validate if the supplied answer is correct.
Print the correct answers.

Here is my solution to the problem:

package main

import (
    "bufio"
    "encoding/csv"
    "flag"
    "fmt"
    "io"
    "log"
    "os"
)

type q struct {
    question, answer string
}

func (q q) ask() bool {
    fmt.Println(q.question, " equals: ")
    scanner := bufio.NewScanner(os.Stdin)
    scanner.Scan()
    if scanner.Err() != nil {
        log.Fatal(scanner.Err())
    }
    if scanner.Text() == q.answer {
        return true
    }
    return false
}

func quizLoop(path string, verbose bool) {
    // Loop should:
    // 1. Read records line by line
    // 2. Ask the question (i/o)
    // 3. Keep score.
    file, err := os.Open(path)
    correct, lines := 0, 0

    if err != nil {
        log.Fatal(err)
    }
    defer file.Close()

    reader := csv.NewReader(file)
    for {
        record, err := reader.Read()
        if err != nil {
            if err == io.EOF {
                break
            }
            log.Fatal(err)
        }
        q := q{question: record[0], answer: record[1]}
        if q.ask() {
            if verbose {
                fmt.Println("Correct")
            }
            correct++
        } else if verbose {
            fmt.Println("Incorrect")
        }
        lines++
    }
    fmt.Printf("You had %d/%d correct answers!\n", correct, lines)
}

func main() {
    // Setup flags.
    p := flag.String("path", "problems.csv", "Specify the path to the quiz questions.")
    v := flag.Bool("verbose", false, "A boolean value to check if you want the program to be verbose or not.")
    flag.Parse()

    // Invoke loop.
    quizLoop(*p, *v)
}

As mentioned in my introduction I am rather new to the language, and couldn't see any caveats where it could have been beneficial to use things like interfaces or go routines in this particular project.

These are the things I'm the most interested in having reviewed:

Best Practices
Refactoring
How to use more advanced functionality to solve it (ie. go routines or interfaces)
Adding unit tests to it. What can be tested?
Overall design

Two things I noticed:

    file, err := os.Open(path)
    correct, lines := 0, 0

    if err != nil {
        log.Fatal(err)
    }
    defer file.Close()

Handle errors immediately, don't do other things first:

file, err := os.Open(path)
if err != nil {
    log.Fatal(err)
}
defer file.Close()

correct, lines := 0, 0

Don't leave an opportunity to add more code later before the error handling by separating os.Open from if err != nil. Sooner or later you'll use file where it may be nil.
Creating a new scanner for each answer is slightly wasteful. Since you asked about potential tests, you can clearly test one or two question/answer cycles. To do that, you'll want to pass two io.Readers to quizLoop instead of a filename

func quizLoop(questions io.Reader, answers io.Reader, verbose bool)

You can pass the file and os.Stdin from main, and in tests you can pass bytes.Buffers, for instance, to supply test data.

I don't see an opportunity to leverage Go routines here. This program is inherently sequential. I'm sure you'll get an excercise for that later.


Fastest Parameter Passing in delphi 7? I have a function that accepts a big amount of data as a parameter (sometimes 1 megabyte) I was thinking which would be the fastest way to pass that data, dunno if I got this right, but here is what I think (functionality doesn't matter, all are the same in my app, since data is never used again after this function).

// slow since the data must be copied to another location
// in memory and passed to myfunction
function myfunction(data: string): boolean;

// faster since a pointer to the original data is passed to
// the function and data is never copied
function myfunction(var data: string): boolean;

// no idea ??!!!
function myfunction(const data: string): boolean;

Executive summary

Passing by const is the fastest for Delphi string types.

Details

function myfunction(data: string): boolean;

  Slow since the data must be copied to another location in memory and passed to myfunction

That's not what happens. In fact the caller does nothing more than pass the string variable, stored as a pointer, to the function. What happens in myfunction is that the reference count  to the string is incremented on entry and decremented on exit. This requires an implicit try/finally to be written into the code to ensure that the decrement happens in case of exceptions. The reference counting code and the try/finally consume time.

Note that the string contents are not copied at all because Delphi strings are optimised to use copy-on-write. You only pay the price of copying them if you modify the string.

function myfunction(var data: string): boolean;

  Faster since a pointer to the original data is passed to the function and data is never copied

This is pretty much equivalent to the previous by value code above. The parameter (i.e. the pointer to the string) is passed by reference rather than value, but otherwise there are no differences.

function myfunction(const data: string): boolean;

And now we come to the winner. Since the function is not allowed to modify the contents of the string, the compiler can omit the reference counting code and the try/finally. So this is the fastest of all.


Leetcode Next Permutation in Python My solution to Leetcode Next Permutation in Python.

  Implement next permutation, which rearranges numbers into the lexicographically next greater permutation of numbers.

  If such arrangement is not possible, it must rearrange it as the lowest possible order (ie, sorted in ascending order).

  The replacement must be in-place and use only constant extra memory.

  Here are some examples. Inputs are in the left-hand column and its corresponding outputs are in the right-hand column.

  1,2,3 → 1,3,2
  3,2,1 → 1,2,3
  1,1,5 → 1,5,1

Q:if the following code is Big \$O(N)\$.

def nextPermutation(self, nums):
    """
    :type nums: List[int]
    :rtype: void Do not return anything, modify nums in-place instead.
    """
    def swap(i, j):
        while i < j:
            nums[i], nums[j] = nums[j], nums[i]
            i += 1
            j -= 1

    n = len(nums)
    index = n - 2
    while index > -1 and nums[index] >= nums[index + 1]:
        index -= 1
    if index == -1:
        swap(0, n - 1)
        return
    i = n - 1
    while i > index and nums[i] <= nums[index]:
        i -= 1
    nums[i], nums[index] = nums[index], nums[i]
    swap(index + 1, n - 1)

Maybe it is a personal taste, but I don't like introducing a nested function because some code is repeated in a particular case. Maybe a refactoring
could avoid this.

first you notice that index is naturally offset by one... refactor it:

index = n - 1
while index > 0 and nums[index-1] >= nums[index]:
    index -= 1
if index == 0:
    swap(0, n - 1)
    return
i = n - 1
while i > index-1 and nums[i] <= nums[index-1]:
    i -= 1
nums[i], nums[index-1] = nums[index-1], nums[i]
swap(index, n - 1)

now you can remove the duplicated call to swap:

if index > 0:
    i = n - 1
    while i > index-1 and nums[i] <= nums[index-1]:
        i -= 1
    nums[i], nums[index-1] = nums[index-1], nums[i]
swap(index, n - 1)

and you can remove the nested function:

n -= 1
while index < n:
    nums[index], nums[n] = nums[n], nums[index]
    index += 1
    n -= 1

This is the resulting code. It is clear that this code is O(n) since there are no nested loops.

def nextPermutation(nums):
    """
    :type nums: List[int]
    :rtype: void Do not return anything, modify nums in-place instead.
    """
    n = len(nums)
    index = n - 1
    while index > 0 and nums[index-1] >= nums[index]:
        index -= 1
    if index > 0:
        i = n - 1
        while i > index-1 and nums[i] <= nums[index-1]:
            i -= 1
        nums[i], nums[index-1] = nums[index-1], nums[i]
    n -= 1
    while index < n:
        nums[index], nums[n] = nums[n], nums[index]
        index += 1
        n -= 1


Binding Lists of Commits to a DataGridView in Winforms with an MVP architecture I'm using Winforms and I find this pattern showing up in my code a lot. It works, but I'm unsure if it's best. The goal is to pass an IList of items through a view interface and then bind that list to a DataGridView in the concrete GUI class. I've been accomplishing this by keeping a private BindingList<T> in the control, and converting the IList when getting/setting the properties. I can't help but feel that there's a better way to do this databinding.

The GUI looks like this.

The View interface.

public interface IUnSyncedCommitsView
{
    event EventHandler<EventArgs> Fetch;
    event EventHandler<EventArgs> Pull;
    event EventHandler<EventArgs> Push;
    event EventHandler<EventArgs> Sync;

    IList<ICommit> IncomingCommits { get; set; }
    IList<ICommit> OutgoingCommits { get; set; }
}

The UserControl code behind.

[ExcludeFromCodeCoverage]
public partial class UnSyncedCommitsControl : UserControl, IUnSyncedCommitsView
{
    public UnSyncedCommitsControl()
    {
        InitializeComponent();

        SetText();
    }

    private void SetText()
    {
        CurrentBranchLabel.Text = RubberduckUI.SourceControl_CurrentBranchLabel;
        FetchIncomingCommitsButton.Text = RubberduckUI.SourceControl_FetchCommitsLabel;
        PullButton.Text = RubberduckUI.SourceControl_PullCommitsLabel;
        PushButton.Text = RubberduckUI.SourceControl_PushCommitsLabel;
        SyncButton.Text = RubberduckUI.SourceControl_SyncCommitsLabel;

        IncomingCommitsBox.Text = RubberduckUI.SourceControl_IncomingCommits;
        OutgoingCommitsBox.Text = RubberduckUI.SourceControl_OutgoingCommits;
    }

    private BindingList<ICommit> _incomingCommits;
    public IList<ICommit> IncomingCommits
    {
        get { return _incomingCommits; }
        set
        {
           _incomingCommits = new BindingList<ICommit>(value);
            this.IncomingCommitsGrid.DataSource = _incomingCommits;
        }
    }

    private BindingList<ICommit> _outgoingCommits;
    public IList<ICommit> OutgoingCommits
    {
        get { return _outgoingCommits; }
        set
        {
            _outgoingCommits = new BindingList<ICommit>(value);
            this.OutgoingCommitsGrid.DataSource = _outgoingCommits;
        }
    }

    public event EventHandler<EventArgs> Fetch;
    private void FetchIncomingCommitsButton_Click(object sender, EventArgs e)
    {
        var handler = Fetch;
        if (handler != null)
        {
            handler(this, e);
        }
    }

    public event EventHandler<EventArgs> Pull;
    private void PullButton_Click(object sender, EventArgs e)
    {
        var handler = Pull;
        if (handler != null)
        {
            handler(this, e);
        }
    }

    public event EventHandler<EventArgs> Push;
    private void PushButton_Click(object sender, EventArgs e)
    {
        var handler = Push;
        if (handler != null)
        {
            handler(this, e);
        }
    }

    public event EventHandler<EventArgs> Sync;
    private void SyncButton_Click(object sender, EventArgs e)
    {
        var handler = Sync;
        if (handler != null)
        {
            handler(this, e);
        }
    }
}

This is the pattern in particular that I'm really worried about.

    private BindingList<ICommit> _incomingCommits;
    public IList<ICommit> IncomingCommits
    {
        get { return _incomingCommits; }
        set
        {
           _incomingCommits = new BindingList<ICommit>(value);
            this.IncomingCommitsGrid.DataSource = _incomingCommits;
        }
    }

A problem I see here is that the current approach will generate deeply nested BindingList objects if you repeatedly call the setter with a list you previously got from the getter. If you see what I mean. Probably you don't really do such thing in practice, but it's still ugly.

I'm going to assume that you generally want the getter return the same list that was originally passed to the setter. That is, if you passed some list x to the setter, you want that list x back from the getter, not some wrapped(x) list.

To make this work, you would need to keep the original list, or have a way to access it again if the setter puts it in a wrapper.
It seems this can be done using a BindingSource:

Create a BindingSource with a BindingList and null parameter, and keep it in a field
Bind your grid to this BindingSource
Make the list getter return the underlying list with bindingSource.List
Make the list setter replace the underlying list, either by replacing the list in the BindingList, or by replacing the BindingList in the BindingSource

This last item is a work in progress.
I spent some time searching through the docs for it,
but it seems harder than I expected.
(Btw I found this discussion illuminating.)

If all else fails, you could clear the underlying list and add all elements from the incoming list.
Depending on how you use the getters and setters,
you might want to do some defensive copies as appropriate.

Unrelated to your main concern, the code duplication when calling the fetch/push/... handlers is not great, with the boilerplate null checks. It would be good to create a helper method CallHandlerIfNotNull or similar.


Contest Solution: Hailstones The Problem

  In the January 1984 issue of Scientific American, an interesting sequence of numbers known as a hailstone series was described. The series is formed by generating the next number based on the number just prior to it. If the previous number was even, the next number in the series is half of it. If the previous number was odd, the next number is three times it plus one. Although the series goes up and down, it eventually settles into a steady state of 4, 2, 1, 4, 2, 1, … For example, starting at 21, the hailstone series is: 21, 64, 32, 16, 8, 4, 2, 1, 4, 2, 1, … For 21, it required five steps before the steady state was reached.

  Write a program that computes how many steps will be necessary before the steady state is reached in the hailstone series beginning at a given number.

  Sample Input

  Your program must take its input from the ASCII text file hail.in. The file contains a sequence of one or more integer values, one per line. Sample contents of the file could appear as:

  21
10

  Sample Output

  Your program must direct its output to the screen and must tell the number of steps required to reach the hailstone steady state for each integer recorded in the input file. Your program must format its output exactly as that shown below which is the output corresponding to the sample input above.

  5 steps were necessary for 21.
4 steps were necessary for 10.

hail.py

with open('hail.in') as f:
    l = f.readline()
    while l:
        n, a, b, i = int(l[:-1]), [], { 4, 2, 1 }, 0
        while set(a) & b != b:
            if i == 0:
                a.append(n)
            else:
                a.append(a[i-1]/2 if a[i-1] % 2 == 0 else a[i-1]*3+1)
            i += 1
        print(i - len(b), 'steps were necessary for', str(n) + '.')
        l = f.readline()

Any advice on performance enhancement and solution simplification or that is topical is appreciated!

Your outer loop…

with open('hail.in') as f:
    l = f.readline()
    while l:
        …
        l = f.readline()

… should be more idiomatically written as:

with open('hail.in') as f:
    for l in f:
        …

Also, the inner counting loop, with i += 1, would be more idiomatically written using for i in itertools.count(): ….

This code looks like you are code-golfing:

while l:
    n, a, b, i = int(l[:-1]), [], { 4, 2, 1 }, 0

While n and i are acceptable, the other one-letter names are too cryptic.  Renaming l → line, a → seq, and b → steady would make the code much easier to understand.

Why do you write l[:-1]?  Is the -1 to strip the trailing newline?  You don't need to do that: int() automatically disregards trailing whitespace.

The code for counting the steps for each n should be extracted into a separate function, since it is a deterministic function of n.

You don't need to build the list a to determine how long the sequence is.

Use the // operator for integer division.  Floating-point division is wasteful.

Instead of calling print() with multiple arguments, use one of the string-formatting mechanisms in Python (e.g. str.format() or f-strings).

Suggested solution

from itertools import count

def steps(n):
    """
    Count the number of steps for the hailstone sequence starting at n to
    reach a steady state of 4, 2, 1, 4, 2, 1, ...
    """
    for i in count():
        if n in { 4, 2, 1 }:
            return i
        n = n // 2 if n % 2 == 0 else 3 * n + 1

with open('hail.in') as f:
    for line in f:
        n = int(line)
        print('{0} steps were necessary for {1}.'.format(steps(n), n))


Concatenate three 16-bit integers in one 64-bit integer with C++ I am looking for an elegant alternative of the following code where wordXX contains always 16 bits:

uint64_t wordHi = 0xaa; // I would like u_int16_t over u_int64_t here
uint64_t wordMi = 0xbb; // because it represents the real data better
uint64_t wordLo = 0xcc;

uint64_t largeword = (wordHi << 32) + (wordMi << 16) + (wordLo << 0);

To me this (very simplified) code looks a bit dirty, because reader gets confused about the real size of the wordXX data.

I could store wordHi to largeword and then shift largeword and add wordMi and so on, but this would make the code worse and probably needs more CPU cycles.

Edit: changed u_int64_t to uint64_t in the example as suggested in the comments.

Use a struct and a union to do what you want.  Note that this implementation is verbose and can be combined into a single declaration.  Also depending on your system architecture, you may need to pack the struct or declare the members in a different order.

typedef struct
{
  uint16_t Low;
  uint16_t Mid;
  uint16_t Hi;
  uint16_t Spare;
} Word64_t;

typedef union
{
  uint64_t Int64Val;
  Word64_t WordVal;
} Splitter_t;

Splitter_t splitter;
splitter.WordVal.Low = 0xaa;
splitter.WordVal.Mid = 0xbb;
splitter.WordVal.Hi = 0xcc;

printf("%x", splitter.Int64Val);  //should print 0xaabbcc


BlockingQueue Implemetation using ReentrantLock I was writing my own implementation of BlockingQueue for practice. I am trying to avoid using the synchronized keyword for the methods. I would instead like to use ReentrantLock.

What is the best way to write this implementation? I am not a Java ninja and would greatly appreciate if someone could pinpoint the errors in my code here and suggest better ways of implementing it.

public class MyBlockingQueue<T> {

private Queue<T> queue;
private AtomicInteger limit = new AtomicInteger(10);
private Lock put_lock = new ReentrantLock();
private Lock take_lock = new ReentrantLock();
private Condition put_condition = put_lock.newCondition();
private Condition take_condition = take_lock.newCondition();

public MyBlockingQueue(AtomicInteger limit){
    queue = new LinkedList<T>();
    this.limit = limit;
}

public boolean put(T item) throws InterruptedException{
    put_lock.lockInterruptibly();
    try {
        while(queue.size() == limit.get()) {
            put_condition.await();
        }
        put_condition.signal();
        queue.add(item);
    } finally{
        put_lock.unlock();
    }

    return true;
}

public T take() throws InterruptedException{
    take_lock.lockInterruptibly();
    try {
        while (queue.size() == 0) {
            take_condition.await();
        }
        take_condition.signal();
        return queue.poll();
    } finally {
        take_lock.unlock();
    }
}

These seem mixed up:

    while(queue.size() == limit.get()) {
        put_condition.await();
    }
    put_condition.signal();

and

    while (queue.size() == 0) {
        take_condition.await();
    }
    take_condition.signal();

when putting if queue is full you wait until someone takes
and when taking if queue is empty you wait until someone puts.

You should have test cases with 11 puts followed by 1 take.
and 1 take followed by 1 put. To see they do not deadlock.

What do you know, the example code for Condition is actually Bounded buffer. And you can see it here:
http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/locks/Condition.html

But I like their condition names better;)

Limit is a constant as it is, hence does not need atomic integer or any other synchronization. Just declare it final. best way of making something threadsafe is to make it immutable.


Best practice to initialize instance variables for a POJO when values are null I am using Java 8. I haven't found another question that fits mine exactly, and I've come across some conflicting information on best practices with what I'm trying to do.

I'm creating a POJO which is essentially a business object - to be returned by my API - that will have its fields initialized from another object.

I'm using the @Data lombok annotation to generate the getters/setters/toString.

I'm wondering which of the following options would align with best practices:

Note: It is guaranteed that authUser will be not null when calling the User constructor.

1. Null-checks with if-statements and default field values

@Data
public class User {
    private String userId = "";
    private String fullName = "";
    private String email = "";
    private Set<String> roles = Collections.emptySet();

    public User(AuthUser authUser, Set<String> roles) {
        if (authUser.getUserId() != null) {
            this.userId = authUser.getUserId();
        }

        if (authUser.getFullName() != null) {
            this.fullName = authUser.getFullName();
        }

        if (authUser.getEmail() != null) {
            this.email = authUser.getEmail();
        }

        if (roles != null) {
            this.roles = roles;
        }
    }
}

I feel like this is a safe/generic way to do this, and I could store the values retrieved from the getters to local variables so the get calls aren't redundant.

2. Null-checks using ternaries with redundant get calls

@Data
public class User {
    private String userId;
    private String fullName;
    private String email;
    private Set<String> roles;

    public User(AuthUser authUser, Set<String> roles) {
        this.userId = authUser.getUserId() == null ? "" : authUser.getUserId();
        this.fullName = authUser.getFullName() == null ? "" : authUser.getFullName();
        this.email = authUser.getEmail() == null ? "" : authUser.getEmail();
        this.roles = roles == null ? Collections.emptySet() : roles;
    }
}

The purpose of this is to reduce code-cluttering, but I believe Clean Code argues against ternaries, so I'd probably not prefer this option for the sake of readability.

3. Null-checks with Optionals without redundant get calls

@Data
public class User {
    private String userId;
    private String fullName;
    private String email;
    private Set<String> roles;

    public User(AuthUser authUser, Set<String> roles) {
        this.userId = Optional.ofNullable(authUser.getUserId()).orElse("");
        this.fullName = Optional.ofNullable(authUser.getFullName()).orElse("");
        this.email = Optional.ofNullable(authUser.getEmail()).orElse("");
        this.roles = Optional.ofNullable(roles).orElse(Collections.emptySet());
    }
}

The purpose of this is to reduce code-cluttering and redundant get calls without creating local variables in order to remove that redundancy. I think this is preferable to ternaries but am not sure.

4. Null-checks with Apache StringUtils.defaultString and CollectionUtils.emptyIfNull

@Data
public class User {
    private String userId;
    private String fullName;
    private String email;
    private Set<String> roles;

    public User(AuthUser authUser, Set<String> roles) {
        this.userId = StringUtils.defaultString(authUser.getUserId());
        this.fullName = StringUtils.defaultString(authUser.getFullName());
        this.email = StringUtils.defaultString(authUser.getEmail());
        this.roles = CollectionUtils.emptyIfNull(roles);
    }
}

This a little shorter than using Optionals, but I feel like using native Java is preferable to third-party libraries (ApacheUtils is already included as a dependency).

Multiple facets to this answer:

First, direct responses to the given approaches

1.

Initializing fields with a value, only to reinitialize them in a constructor is not useful - skimming the class, which will be done in time, would lead to confusion. When initializing values with defaults, it would be better to modify them using setters.
The empty String may, however, not be a good default. A Constructor is supposed to instantiate an Object in a way that it can be used - however, a user without a user id appears relatively useless.

2.

This approach is better, but the concepts are not cleanly understood.
First off, Clean Code does not argue against ternaries in general. Ternary operations can be very useful, if utilized correctly. The way you use them here is not a good way to use a ternary. This however is

private static <T> T valueOrDefault(T value, T default) {
  return value != null
    ? value
    : default;
}

This is a ternary which is instantly understandable, as it is entirely alone, and formatted in a way that it makes understanding easier.
This is also a suggestion to use here, as declaring this method within the User class allows for the following constructor:

public User(AuthUser authUser, Set<String> roles) {
  this.userId = valueOrDefault(authUser.getUserId(), "");
  // omitted
}

While this is very readable in both constructor and utility method, it is also technically overengineered here, as the use of generics is not necessary in this case, it merely removes the need to declare two extremely similar functions.

3.

Do not use Optional like this. Optional has a very specific and limited use, which is as a return type on specific methods and function calls.
Also, while this is one way to remove a redundant get-call, this also adds a rather large amount of unnecessary overhead, plus it is much less readable than the second approach.
This reduced readability and increased overhead in comprehension and execution remains even if the formatting in improved for readability.

4.

This is the most acceptable approach in general, however this is only true if the necessary dependencies are already present on the classpath. Adding commons-lang for null-checks only is, while technically adhering to the KISS principle, too much cost for effect.

Second: Considerations

Is this an Object or a data structure? Right now, it is technically speaking both, which is inherently problematic. See here for some explanations, the mentioned book would help you more though.

Considering the construction approach: the constructor seems to copy fields from object a, which is also a kind of user, and append a Set of Strings to the object. So it is neither a copy constructor nor a plain constructor. This is in so far confusing that, when described from a more distanced view,
the User is instantiated by passing a User and Roles. The Passed User is not present in the User Object, but some fields of it are.
If you find this sentence confusing: people who read your code will most likely think along these lines. At least, I do, as I always try to simplify as much as possible.

As this kind of construction CAN be confusing, it may be useful to create a Mapper or Converter accepting AuthUser and Set<String> and returning User. This would separate the data structure from the logic, and it would allow more validation logic to happen without bloating the user object.

This would also allow the user to be a @Value-class, meaning no setters and all fields final, which makes this object almost immutable and easy to share - almost as the Set still can be modified.

Third: the null-problem

Is null really the same as the empty String in case of id, name and email? And, are "no roles" the same as null?
null in java is, despite the dreaded NullPointerException, a very useful value, and replacing it with "I don't want to do null check"-defaults is rarely a good idea.

It would actually be better practice to not modify the values at all and take them at face value, as that allows you to determine if there was an error during authentication or data binding (as data binding errors often lead to empty strings, while authentication errors fail to instantiate objects correctly).

Try to incorporate null-values into your application as valid values - for example as validation values, or as markers for errors.

Also, for good measure, never allow or return null where Collections, Maps, arrays or other similar objects are expected, as effectively no one expects these to be null.


Project Euler problem 12 The statement is as follows :

  The sequence of triangle numbers is generated by adding the natural numbers.
     So the 7th triangle number would be 1 + 2 + 3 + 4 + 5 + 6 + 7 = 28.

  The first ten terms would be:

1, 3, 6, 10, 15, 21, 28, 36, 45, 55, ...

  Let us list the factors of the first seven triangle numbers:

1: 1
3: 1,3
6: 1,2,3,6
10: 1,2,5,10
15: 1,3,5,15
21: 1,3,7,21
28: 1,2,4,7,14,28

  We can see that 28 is the first triangle number to have over five divisors.

  What is the value of the first triangle number to have over five hundred divisors?

My implementation :

import java.util.ArrayList;
import java.util.HashMap;
public class Euler_12{

    // This function finds the prime factorization
    // of a given number
    public static int getDivisors(int number){
        ArrayList<Integer> list = new ArrayList<Integer>();
        while(number%2==0){
            list.add(2);
            number/=2;
        }

        for(int i = 3; i<=Math.sqrt(number); i++){
            while(number%i==0){
                list.add(i);
                number/=i;
            }
        }

        if(number>2){
            list.add(number);
        }

        return findDivisors(list);
    }

    // Uses HashMap to convert multiples in form of
    // 2 2 2 2 4 4 to 2^4 x 4^2
    public static int findDivisors(ArrayList<Integer> list){
        int result = 1;
        HashMap<Integer, Integer> map = new HashMap<Integer, Integer>();
        for(int i : list){
            if(map.containsKey(i))
                map.put(i, map.get(i)+1);
            else
                map.put(i, 1);

        }

        // Better take a look at [1]
        // on why I did this
        for(int i : map.keySet()){
            result = result * (map.get(i) + 1);
        }

        // the result is number of divisors
        return result;
    }

    public static int number = 0;
    public static int sequence = 1;
    // Returns the next Triangular number
    public static int nextTraingularNumber(){
        int temp = number + sequence;
        number = temp;
        sequence++;
        return temp;
    }
    public static void main(String[] args) {
        while(true){
            int i = nextTraingularNumber();
            if(getDivisors(i)>500){
                System.out.println("Found : " + i);
                break;
            }
        }
    }
}

It finds the answer in 0.812 seconds. Looking for improvements, suggestions are welcomed

The \$n\$th triangular number is equal to \$n(n+1)/2\$, which can be factored as \$(n/2)(n+1)\$ or \$n((n+1)/2)\$ depending on whether \$n\$ is even or odd.  In either case the two factors are relatively prime, so the number of divisors can be computed as the product of the number of divisors of each factor, which is much faster to run through your getDivisors function.

There are also much more efficient ways to compute the number of divisors: sieving allows you to compute this over a contiguous range of \$n\$ values simultaneously much faster than doing each one individually.  Even for individual \$n\$, factoring-based methods will be tend to be faster than systematically trying all divisors up to \$\sqrt{n}\$.


Convert all dates in an array to date format I'm returning an array containing dates (and other things) from an API, then looping through the array on the front end to convert them into dates

The backend uses Node.Js and queries a mySql database with the mySql module:

pool.getConnection(function(err, conn) {
  conn.query('SELECT * FROM myData’, function(err, result) {
    if (result) {
      res.json(result);
    }
    conn.release();
  });
});

An Angular.js front end accesses the Express API.

var _getCases = function() {
  return $http.get('/api/case');
};

But the dates are all strings.  Here’s an example of what's received in Angular:

[
  {
    id: 1,
    fruit: “apple”,
    createdOn: "2016-09-02T23:00:00.000Z"
  },
  {
    id: 2,
    fruit: “banana”,
    createdOn: "2016-09-11T23:00:00.000Z”
  },
  {
    id: 3,
    fruit: “cherry”,
    createdOn: "2016-09-13T23:00:00.000Z"
  },
]

I'm converting all the dates to strings in a loop but this seems inefficient.  Is there a better way?

var a = $http.get('/api/case');

for(var i in a){
  a[i].createdOn = new Date(a[i].createdOn);
};

return a;

Firstly, regardless of whether you actually want to transform the fields into dates,you will need to format them for display. This formatting should be applied in the template. In such cases, the idiomatic approach is to define a filter.

angular.filter('formatAsDate', ...);

and then inject the filter into the views controller to make it available.

MyController.$inject = ['formatAsDate'];
function MyController(formatAsDate) { }

However, date formatting is such a common scenario that AngularJS provides a built in, globally available (does not need to be injected) date filter out of the box.

Its first argument is a value representing a date. This may either be a Date, an ISO formatted date string such as you have, or a number in milliseconds.
Its second argument is a format string which specifies how the date should be displayed.

for example

{{'2016-09-02T23:00:00.000Z' | date: 'EEEE MMMM dd, yyyy'}}

will render

Friday September 02, 2016


Implementing a stack with Java I am trying to implement a stack with Java:

public class Stack<E> {

    Node top = null;

    public void push(E data) {
        Node node = new Node(data);
        if (null != top) {
            node.next = top;
        }
        top = node;
    }

    public E pop() {
        if (null == top) {
            throw new IllegalStateException("Stack is empty");
        } else {
            E data = top.data;
            top = top.next;
            return data;
        }
    }

    public E peek() {
        if (null == top) {
            throw new IllegalStateException("Stack is empty");
        } else {
            E data = top.data;
            return data;
        }
    }

    public void print() {
        if (null == top) {
            throw new IllegalStateException("Stack is empty");
        } else {
            Node node = top;
            while(null != node) {
                System.out.println(node.data);
                node = node.next;
            }
        }
    }

    private class Node {

        Node next;
        E data;

        Node(E data) {
            this.data = data;
        }

    }
}

I am looking for feedback for the implementation. Also, I wanted to know if the pop() implementation can cause a memory leak?

I wanted to know if the pop() implementation can cause a memory leak?

It will not cause a memory leak, because java is a garbage collected language. When top is no longer accessible the garbale collecter will free top if it sees fit.

feedback:

In push() the null check is unnessecary, because node.next is already null by default.

public void push(E data) {
    Node node = new Node(data);
    node.next = top;
    top = node;
}

Rather then have a print() method, create a toString() method. toString() is a method of Object that is used across java to get a string representation of an object. This will make it easier to print your object to different streams.
In Node, create a getData() (and optionally setData()) method to control access to the data class, or make data final.
make top private to control use of your class to the supported methods.


How to increase download speed using the following method? I have written an Android application which downloads pdf files through web service. I am using kSoap2 android library to parse the response of web service which basically contains file name & file data.

I have written following code. Please review & tell how to increase the speed I think there is some small defect in code which reduces the speed. I am using version 2.5.1 of kSoap2.

public void downloadPdfFiles(File fileDocsDir, int noOfFiles)
            throws NullPointerException, SoapFault, XmlPullParserException,
            FileNotFoundException, IOException, Exception {

        System.gc();

        // Download files from web service and save them in the folder.
        soapObject = new SoapObject(NAMESPACE, METHOD_NAME);
        soapObject.addProperty("startingFile", noOfFiles);
        soapObject.addProperty("deviceId", deviceId);
        soapObject.addProperty("loginId", loginId);
        soapObject.addProperty("byoinId", "1");

        envelope = new SoapSerializationEnvelope(SoapEnvelope.VER11);

        envelope.setOutputSoapObject(soapObject);

        // Calling the web service
        System.gc();
        androidHttpTransport = new HttpTransportSE(URL);
        androidHttpTransport.call(SOAP_ACTION, envelope);

        // Getting Response through xml mainly generated as soap:reponse.
        responseBean = (SoapObject) envelope.getResponse();

        // Array of PDFInfoBean.
        list = (SoapObject) responseBean.getProperty(0);

        FileOutputStream outputStream = null;
        BufferedOutputStream bufferedOutputStream = null;
        // Get Individual PDF Details from Array.
        SoapObject pdfDetails = null;

        mIncrement = noOfFiles;
        // Log.i("Increment Values", String.valueOf(mIncrement));

        File pdfFile = null;
        for (int i = 0; i < list.getPropertyCount(); i++) {
            pdfDetails = (SoapObject) list.getProperty(i);

            // Get PDF File Name.
            pdfDocName = pdfDetails.getProperty(1).toString();
            Log.i(TAG, "File Name: " + pdfDocName);

            // Check for last file.
            if (pdfDocName.equalsIgnoreCase("EOF")) {
                mFlag = false;
                break;
            }

            // Creating PDF File.
            pdfFile = new File(fileDocsDir, pdfDocName);

            // Writing PDF file received through web service.
            if (pdfFile.exists()) {
                Log.i(TAG, pdfFile.getName() + " File Already Exists");
            } else {
                outputStream = new FileOutputStream(pdfFile);
                bufferedOutputStream = new BufferedOutputStream(outputStream);

                bufferedOutputStream.write(Base64Coder.decode(pdfDetails
                        .getProperty(0).toString()));

                mIncrement = mIncrement + 1;

                bufferedOutputStream.close();
                outputStream.close();

                bufferedOutputStream = null;
                outputStream = null;
            }
            pdfDetails = null;
            pdfDocName = null;
            pdfFile = null;
            System.gc();
        }
        soapObject = null;
        envelope = null;
        responseBean = null;
        list = null;
        androidHttpTransport = null;

        System.gc();
    }

Forcing System.gc() isn't a good practice.
outputStream = new FileOutputStream(pdfFile);
        bufferedOutputStream = new BufferedOutputStream(outputStream);

should be wrapped into 1 variable to do a single close() afterwards:

outputStream = new BufferedOutputStream(new FileOutputStream(pdfFile));

All operations with outputStream should be put in try-finally block:

outputStream = new ...
try {
    outputStream.write(...);
    ...
} finally {
    outputStream.close();
}

Agree with the first comment: your file may simply not fit in memory. In SOAP API there should be some mean of getting the InputStream instead of an in-memory string.

Sorry for the formatting, couldn't fight the editor.

Good luck!


Better pythonic way to pass condition to multiple functions I have the following scenario where most of my functions require to return different things based on a condition.

def get_root_path(is_cond_met=False):
    if is_cond_met:
        return "something"
    else
        return "something else"

def get_filename(is_cond_met=False):
    if is_cond_met:
        return "file name A"
    else
        return "file name B"

is_cond_met is going to be common for all the functions I am calling. I have just put two here however I have more than 15.

The above code works, however, doesn't seem optimal, or pythonic. Is there a better solution for this?

Your example is a little too example-y for me to know if this is actually right for whatever you're actually doing, but if you have two parallel implementations of a set of behavior within the same set of functions, you might think about moving them into classes:

from abc import ABC, abstractmethod

class CondFile(ABC):

    @abstractmethod
    def get_root_path(self) -> str:
        pass

    @abstractmethod
    def get_filename(self) -> str:
        pass

class CondTrueFile(CondFile):

    def get_root_path(self) -> str:
        return "something"

    def get_filename(self) -> str:
        return "file name A"

class CondFalseFile(CondFile):

    def get_root_path(self) -> str:
        return "something else"

    def get_filename(self) -> str:
        return "file name B"

if is_cond_met:
    cond_file = CondTrueFile()
else:
    cond_file = CondFalseFile()
print(cond_file.get_root_path())
print(cond_file.get_filename())

It really depends what the condition represents, but if it results in completely different behavior across fifteen functions, it's a good clue that it represents a different "kind of" thing that your program is dealing with, i.e. a type, which is in turn a good clue that you want a different class to represent that type in your code.


Hackerrank "Almost Equal" solution I've spent the best part of a day on this question. It is marked as Expert level. There are about fifteen submission test cases and my solution manages to satisfy the first four. However, from there on in it hits timeouts due to performance issues.

Hackerrank question

There doesn't seem to be much information or solutions for this question around, so I would appreciate if anyone had any suggestion on how I can improve the performance. Maybe I need to go back to the drawing board with my approach.

N = number of elements in the array
K = The height difference allowed
H = Array of heights
queries = 2D array of ranges within which you must find the number of matching pairs (i.e. how many fighters in this range meet the height requirements to fight each other)

My approach is to copy the subset (I know this is slow and would like to get around it somehow)and then run a quicksort over this new subset.

With this we can now quickly figure out what heights are within range of each other (i.e. if we get height = 1 and we know k = 2, then the maximum height he can fight against is going to be 3).

    static int[] solve(int n, int k, int[] h, int[][] queries) {
    // N = no. heights
    // K = Difference
    // L & R = First and Last fighters
    int[] results = new int[queries.length];
    int count = 1;
    int pairs = 0;
    int l, r =0;

    for(int j=0; j<queries.length; j++) {
        l = queries[j][0];
        r = queries[j][1];
        int[] range = Arrays.copyOfRange(h, l, r+1);

        int[] sortedSubset = sort(range);

        for(int i=0; i<sortedSubset.length-1; i++) {
            count = i+1;
            while((count != sortedSubset.length) && sortedSubset[count] <= (sortedSubset[i]+k)) {

                // While the current number is still in range of the number we are checking i.e. (number + k)
                pairs++;
                count++;
           }
        }
        results[j] = pairs;
        pairs = 0;
    }

    return results;
}

  A Sumo wrestling championship is scheduled to be held this winter
  in the HackerCity where N wrestlers
  from different parts of the world are going to participate.
  The rules state that two wrestlers can fight against each other
  if and only if the difference in their height
  is less than or equal to K, (i.e.)
  wrestler A and wrestler B can fight
  if and only if |height(A)-height(B)|≤K.

  Given an array H[], where
  H[i] represents the height of the ith
  fighter, for a given l, r where 0 <= l <= r < N, can you count the
  number of pairs of fighters between l and r (both inclusive) who
  qualify to play a game?

  Input Format
  The first line contains an integer N and K separated by
  a single space representing the number of Sumo wrestlers who are going
  to participate and the height difference K.
  The second line contains N integers separated by a single space,
  representing their heights
  H[0] H[1] ... H[N - 1].
  The third line contains Q, the number of queries.
  This is followed by Q lines each having two integers l and r
  separated by a space.

  Output Format
  For each query Q, output the corresponding value of
  the number of pairs of fighters for whom the absolute difference of
  height is not greater that K.

  Constraints
  1 <= N <= 100000
  0 <= K <= 109
  0 <= H[i] <= 109
  1 <= Q <= 100000
  0 <= l <= r < N

  Sample Input

5 2
1 3 4 3 0
3
0 1
1 3
0 4

  Sample Output

1
3
6

  Explanation
  Query #0: Between 0 and 1 we have i,j as (0,1) and
  |H[0]-H[1]|=2 therefore output is 1.
  Query #1: The pairs (H[1],H[2]) (H[1],H[3])
  and (H[2],H[3]) are the pairs such that |H[i]-H[j]| <=2.
  Hence output is 3.
  Query #2: Apart from those in Query #1, we have
  (H[0],H[1]), (H[0], H[3]), (H[0], H[4]), hence 6.

  Timelimits

  Timelimits are given here

Constraints
    1 <= N <= 100000
    0 <= K <= 109
    0 <= H[i] <= 109
    1 <= Q <= 100000
    0 <= l <= r < N

My rule of thumb for this kind of challenge is that 109 operations is a limit. So, ignoring the hidden constant of Landau notation, we're looking at something which is \$O(N\sqrt N + Q\sqrt Q + Q\sqrt{N} + K)\$. In particular, each query shouldn't take more than \$\sqrt N\$ time. That would seem to push us towards a preprocessing step which is \$O(N\sqrt N + K)\$ and which allows us to process individual queries fast. Alternatively, we might have a preprocessing step which takes into account the queries, but that's likely to be more complicated.

Since your approach takes (in the worst case) \$O(QN\lg N)\$ time, it's definitely nowhere near complying with my rule of thumb.

If we consider first the idea of a preprocessing step which takes into account the queries, it's quite easy to construct a really nasty case where we have boundaries between every consecutive pair of wrestlers, and each one contributes to multiple queries. E.g. take the following 100000 queries:

(0, 99999), (1, 99998), (2, 99997), ..., (49999,50000),
(0, 49999), (1, 50000), (2, 50001), ..., (49999, 99998)

I don't find that approach promising.

Ideas which might be more promising are bucketing the wrestlers in buckets of width \$K\$, so that each wrestler can only interact with those in the same bucket and some of those in the adjacent buckets - although the "some of" is a problem; and inclusion-exclusion, although I can't quite get that to work. It's "fast enough" to precompute tables \$C[a]\$ which is the number of valid fights between \$i \le a < j\$; \$L[a]\$ which is the number of valid fights between \$i < j \le a\$; and \$R[a]\$ which is the number of valid fights between \$a \le i < j\$. But the closest I can get to the number of valid fights between \$l \le i < j \le r\$ is the difference of that with the number of valid fights between \$i \le l\$ and \$j \ge r\$.


NodeJS and Express with Promises I'm relatively new to NodeJS and Express, and I've recently run into the pyramid of Doom while doing stuff that requires multiple steps.

I've found that promises are very promising indeed. But I definitely need opinions on various aspects of my approach to routes as a whole.

Here's the full code:

I'm not very sure about this constructor whose only responsibility is basically to hold the promised steps and pipe the same input regardless of the results.

function SignupDoctorPostRequest(data) {
    this.validatePlanId = function() {
        return SubscriptionService.hasValidPlan(data.plan);
    };

    this.createUser = function () {
        return UserService.save(data);
    };

    this.subscribeUser = function () {
        return SubscriptionService.create(data);
    };
}

My intent is revealed when we look at the route, where we see the data is only the body of the request.

router.post('/', function (rq, response) {
    var validation = SignupDoctorPostSchema.validate(rq.body); // This is a Joi Schema validation.

    if(validation.error) {
        response.
          status(422).
          json({
            status: 'fail',
            data: joiful.parseValidationError(validation.error) // this is only method I use to parse joi validations into a more consistent object.
          });
        return;
    }

    var request = new SignupDoctorPostRequest(validation.value);

    request.validatePlanId()
    .then(request.createUser)
    .then(request.subscribeUser)
    .then(
        function Success (authenticatedUser) {
          response.status(201).json({});
        }
    )
    .catch(
        function Fail (fail) {
          var statusCode = fail.type === 'Validation' ? 422 : 500;
          var responseStatus = fail.type === 'Validation' ? 'fail' : 'error';
          var responseData = fail.type === 'Validation' ? fail.details : fail.message;

          response.
            status(statusCode).
            json({
              status: responseStatus,
              data: responseData
            });
        }
    );
});

Lastly, I need feedback on handling my responses because I'm responding eagerly if validation fail and then again if the the process fails, I think this is sort of smelly.

Interesting,

I actually really like your constructor, it is neat and shows in one place what the class does. I am not sure I would call it SignupDoctorPostRequest, since it is not a request at all but processes a request. I would consider DoctorSignupModel or since it seems to leverage several models DoctorSignupMetaModel.

As for your eager validation, I have to agree that it does not look very elegant. If you could work SignupDoctorPostSchema.validate( into this.validatePlanId (possibly call it then this.validate), you could rewrite your write to this:

router.post('/', function (rq, response) {

    var metaModel = new DoctorSignupMetaModel(rq.body);

    request.validate() //<- This will do all validation now
    .then(request.createUser)
    .then(request.subscribeUser)
    .then(
        function Success (authenticatedUser) {
          response.status(201).json({});
        }
    )
    .catch(
        function Fail (fail) {
          var statusCode = fail.type === 'Validation' ? 422 : 500;
          var responseStatus = fail.type === 'Validation' ? 'fail' : 'error';
          var responseData = fail.type === 'Validation' ? fail.details : fail.message;

          response.
            status(statusCode).
            json({
              status: responseStatus,
              data: responseData
            });
        }
    );
});


C# to C++: Basic Console Replication I'm working on converting my basic cryptocurrency miner from C# to C++ for performance reasons (I've been told that C++ is faster in many aspects). However, that aside, the focus of this post is to get some feedback for a very basic port of ReadLine, Write and WriteLine as I understand them, from C# to C++. This way, I don't get too far along before learning lessons the hard way.
Console.h
From my understanding, this file is mostly meant for definitions, such as an interface in C# in that implementation should not occur here, though it's not disallowed (unlike C# where it is disallowed):
#pragma once
#include <string>
using namespace std;
class Console
{
public:
    static string ReadLine();
    static void Write(string input);
    static void WriteLine(string input);
};

Console.cpp
The concept of separating definition from implementation is a bit new to me (outside of interfaces and members marked as abstract in C#). The part that threw me off here is that the static keyword is dropped when writing out the function:
#include "Console.h"
#include <iostream>
#include <string>
using namespace std;
string Console::ReadLine() {
    string result;
    cin >> result;
    return result;
}
void Console::Write(string input) { cout << input; }
void Console::WriteLine(string input) { cout << input + "\n"; }

CryptoSandbox.cpp
This is my entry point, I didn't really change much. I simply replaced the original cout << "Hello World" with Console::WriteLine(...), which required inclusion of Console.h to compile, so lesson learned fast there:
#include "Console.h"
#include <string>
using namespace std;
int main() {
    Console::Write("Starting the crypto sandbox. Please tell us your name: ");
    string userName = Console::ReadLine();
    Console::WriteLine("Welcome to the crypto sandbox, " + userName + "!");
}

The goal of this post is to learn what I can simplify, and anything that can be done better. A few questions hitting me hard are:

How does organization not become quickly overwhelming based on how VS Community creates the initial project structure?
Why is static not accepted in the cpp file when the declaration states static void in the h file?

Outside of those two questions (entirely optional, by the way), my primary focus is certainly feedback on my port.

Don't use:
using namespace std;

In the header file it is really bad and can break code. But even in the source file is considered bad practice.
see: Why is “using namespace std;” considered bad practice?

This does not read a line:
string Console::ReadLine() {
    string result;
    cin >> result;   // This reads a single space separated word:
    return result;
}

Use std::getline()
std::string Console::ReadLine() {
    std::string result;
    std::getline(std::cin, result);
    return result;
}

How does organization not become quickly overwhelming based on how VS Community creates the initial project structure?

I don't understand what this means.

Why is static not accepted in the cpp file when the declaration states static void in the h file?

Why waste the developers time adding in words the compiler already knows.

When transferring from one language to another a direct translation is usually not advisable. You have to use the other language in the way it is designed to be used, otherwise you are not going to get the improvements you desire.


Reddit RepostFinder Bot I've been making a bot for Reddit that finds reposts (based on image MD5 comparing) and posts a small comment on the thread (that part will be later, right now it downloads images and compares MD5). If the image is original content, it will add it to its list of hashes. The source code is here. Any suggestions as to what I can improve?

public static bool DownloadImage(string url, bool isDirect) // returns if repost
{ // example url: http://imgur.com/UndN77d
    if (url.Contains('?'))
    {
        url = url.Split('?')[0]; // if it has a query at the end like ?1
    }
    string filename = null;
    if (url.Contains("imgur"))
    { // is imgur, so use ImgurAPI class for it
        string imgurID = url.Split('/')[url.Split('/').Length - 1];
        Console.WriteLine("Processing imgur id {0}", imgurID);
        if (!isDirect)
        { // the sole goal for this block is to get Vars.picToCompare
            string details = Imgur.GetImageDetails(imgurID);
            if (details == null)
            {
                return false;
            }
            Console.WriteLine("Got response: {0}", details); // json
            JObject imgur = JObject.Parse(details);
            if (imgur["success"].ToString() == "false")
            { // auth failed
                Console.WriteLine("Imgur processing failed.. Printing JSON contents: ");
                Console.WriteLine(details);
                Console.ReadKey();
                return false;
            }
            else
            {
                Console.WriteLine("Imgur details recieved successfully!");
                filename = imgur["data"]["link"].ToString();
                url = filename;
                Console.WriteLine("Processed direct URL: " + filename);
                filename = filename.Split('/')[filename.Split('/').Length - 1];
                Console.WriteLine("Processed image filename: " + filename);
                Vars.picToCompare = Path.Combine(Vars.picsFolder, filename);
            }
        }
        else
        {
            Vars.picToCompare = Path.Combine(Vars.picsFolder, url.Split('/')[url.Split('/').Length - 1]); // should be filename
        }
    }
    else
    { // for example mysite.com/blah.jpg
        Vars.picToCompare = Path.Combine(Vars.picsFolder, url.Split('/')[url.Split('/').Length - 1]);
        Console.WriteLine("Processing image url {0}", url);
        Console.WriteLine("Processed image {0}", Vars.picToCompare);
    }
    bool isRepost = false;
    using (WebClient client = new WebClient())
    {
        if (File.Exists(Vars.picToCompare))
            File.Delete(Vars.picToCompare);
        client.DownloadFile(url, Vars.picToCompare);
        Console.WriteLine("Successfully downloaded image {0}", Vars.picToCompare);
    }
    isRepost = Processing.CompareHash(url);
    if (isRepost)
        Console.WriteLine("This is a repost!");
    else
        Console.WriteLine("This is OC. Adding to hashlist..");
    //foreach (string hash in Vars.totalHashes)
    //{
    //    Console.WriteLine("Written hash to file: " + hash);
    //}
    return isRepost;
}

public static bool DownloadImage(string url, bool isDirect) // returns if repost

C# supports something called XML Documentation Comments which allow you to document your code directly while writing it in a processor (as in IDE) friendly way.

public static bool DownloadImage(string url, bool isDirect) // returns if repost

Liar! This does not download the image but does checking on the URL, downloads the image, creates a MD5 sum for that image and then returns if it is already in the list of hashes.

You have one function that does everything, that's bad. Ideally you would split the responsibilities between multiple methods in one class, in some kind of such a structure (pseudo code):

class RepostChecker
    byte[] ComputeMD5(byte[] data)
    byte[] GetImage(string url)
    bool IsKnown(byte[] hash)

All of these methods can be static. So you can chain them like this:

IsKnown(ComputeMD5(GetImage(url)))

Or you can write a convenience method:

bool IsRepost(string url)

Which does the chaining for you. Then you define a class which does hold your hashes:

class Hashes
    static string HashToString(byte[] hash)

    void Add(string hash)
    bool Contains(string hash)
    void Load(whateverDatastoreYouUse)
    void Save(whateverDatastoreYouUse)

Internally this should use a HashSet<String>, not a normal List<String> like your implementation.

These changes would allow a more structured approach to the problem.

if (url.Contains('?'))
{
    url = url.Split('?')[0]; // if it has a query at the end like ?1
}
string filename = null;
if (url.Contains("imgur"))

You should use the Uri class if you work with URLs.

Uri uri = new Uri(url);           // http://imgur.com/blabla?whatever
uri.GetLeftPart(UriPartial.Path); // http://imgur.com/blabla
uri.Host;                         // imgur.com

Ideally you would not have one if {} else {} construct to handle this situation, but an interface which implementations and a static factory:

interface ImageHosterInteractor // Notice that the name is not optimal
    byte[] GetImage(string url)

From this interface you can now derive implementations for imgur and/or any other site. The static factory would look like this:

public static class ImageHosterInteractorFactory
{
    public static ImageHosterInteractor Create(Uri uri)
    {
        if (uri.PathAndQuery.EndsWith(".png")) // Or any other for that matter
        {
            // Implementation which simply loads the data from
            // the given Uri.
            return new ImageHosterInteractors.Plain();
        }

        // The uri does not end with an explicit ending,
        // so there's a good chance that we need to follow
        // redirections or perform some other magic.
        // So we use one of the explicit implementations.

        switch(uri.Host)
        {
            case "imgur.com":
                return new ImageHosterInteractors.Imgur();
                break;

            case "whatever.com":
                return new ImageHosterIntactors.Whatever();
                break;

            default:
                throw new NotImplementedException("There is no implementation assigned to handle: " + uri.Host);

        }
    }
}

That allows your code to simply process the URL, pass it into the factory and get something back that knows how to treat that site and you don't need to worry about it further down.

Uri uri = new Uri(fromwhatever);
ImagerHosterInteractor interactor = ImageHosterInteractorFactory.Create(uri);
byte[] image = interactor.GetImage(uri);

Also adding new sites to handle is as easy as implementing ImageHosterInteractor and adding it to the factory method.

At the moment it looks like you're saving the images you download, is that necessary or is it enough to simply save the hashes?


Enumerating AWS instances in Rails, with error handling I have created a method, which is raising the same exception in case of any error:

def fetch_aws_instances
  region_name = 'us-west-2'
  ec2 = Aws::EC2::Client.new(region: region_name)
  describe_regions_result = ec2.describe_region
  ...
  ...
rescue Seahorse::Client::NetworkingError, Aws::Errors::MissingCredentialsError, Aws::EC2::Errors::UnauthorizedOperation => error
  raise error
end

Now, I am calling this method like this:

begin
  fetch_aws_instances
rescue Seahorse::Client::NetworkingError
  Rails.logger.error 'Network Error: Please check whether the application instance is allowed outbound access to the Internet.'
rescue Aws::Errors::MissingCredentialsError, Aws::EC2::Errors::UnauthorizedOperation
  Rails.logger.error 'Authorization Error: Please ensure IAM role is attached to the application instance.'
end

Am I calling the method in correct way, do I need to rescue all the errors or is there some better way to do it?

The rescue in your fetch_aws_instances method is pointless. Rescuing errors just to re-raise them identically, is the same as not rescuing them at all.

You should only rescue when you want to customize what happens if the error is raisedd. Unless you want to move the Rails.logger calls to the fetch_aws_instances method, you should only have the rescues when you call it (as you do in your second code block)


Can I simplify this recursive grid search? I've written a recursive method for fetching grid neighbours in a symmetrical 2-dimensional grid, the problem is that I've found myself duplicating the code more or less into an overload to prevent my list from re-initializing everytime the method calls on itself.

I'm guessing there are ways to get around this that doesn't involve code duplication. Any help is appreciated.

public List<Cell<int>> GetNeighbours(Cell<int> cell, Direction direction, int neighboursToGet)
        {
            var neighbours = new List<Cell<int>>();

            if (neighboursToGet > 0)
            {
                int x = 0;
                int y = 0;
                Cell<int> neighbour;

                switch (direction)
                {
                    case Direction.Left:
                        x = -1;
                        break;
                    case Direction.Right:
                        x = 1;
                        break;
                    case Direction.Up:
                        y = -1;
                        break;
                    case Direction.Down:
                        y = 1;
                        break;
                }

                neighbour = cells[cell.Position.Row + y, cell.Position.Column + x];
                neighbours.Add(neighbour);
                GetNeighbours(neighbour, direction, neighboursToGet - 1, neighbours);
            }
            return neighbours;
        }

        public List<Cell<int>> GetNeighbours(Cell<int> cell, Direction direction, int neighboursToGet, List<Cell<int>> neighbours)
        {
            if (neighboursToGet > 0)
            {
                int x = 0;
                int y = 0;
                Cell<int> neighbour;

                switch (direction)
                {
                    case Direction.Left:
                        x = -1;
                        break;
                    case Direction.Right:
                        x = 1;
                        break;
                    case Direction.Up:
                        y = -1;
                        break;
                    case Direction.Down:
                        y = 1;
                        break;
                }

                neighbour = cells[cell.Position.Row + y, cell.Position.Column + x];
                neighbours.Add(neighbour);
                GetNeighbours(neighbour, direction, neighboursToGet - 1, neighbours);
            }
            return neighbours;
        }

It seems to me that most of the body of the first GetNeighbours is extraneous. Wouldn't this be enough ?

public List<Cell<int>> GetNeighbours(Cell<int> cell, Direction direction, int neighboursToGet)
{
    var neighbours = new List<Cell<int>>();

    GetNeighbours(cell, direction, neighboursToGet, neighbours);

    return neighbours;
}

Also, it is redundant to accept neighbours as a read/write parameter and to return it. However, if you are going to do it, you could as well write:

public List<Cell<int>> GetNeighbours(Cell<int> cell, Direction direction, int neighboursToGet)
{
    return GetNeighbours(cell, direction, neighboursToGet, new List<Cell<int>>());
}