Getting statistics in rest api endpoint We have following endpoint on our api that should grab some events from db, aggregate them and output as a json. But I am having hard time reading this function and hope you could help me out with some ideas how to refactor this to something more reasonable. Or maybe its fine and I just need to read harder?

[HttpGet]
public AmazonDTO GetCRSTimeStatuses(string countryId, string regionId, string storeId, string dateFrom, string dateTo)
{
    List<CRSTimeStatus> res = new List<CRSTimeStatus>();

    try
    {
        DateTime _dateFrom;
        DateTime.TryParse(dateFrom, out _dateFrom);

        _dateFrom = _dateFrom.Date;

        DateTime _dateTo;
        DateTime.TryParse(dateTo, out _dateTo);

        int _countryId;
        int.TryParse(countryId, out _countryId);

        int _regionId;
        int.TryParse(regionId, out _regionId);

        int _storeId;
        int.TryParse(storeId, out _storeId);

        PageDetailType _pageDetailType = ModelHelper.GetActualPageType(_countryId, _regionId, _storeId, tillId: 0);

        var eventValue = _cache.Get<List<Event>>("eventsChache");

        var events = new List<Event>();
        var countries = new List<Country>();

        // Read base relation data from DB.
        countries = _countryRepository.GetAll().ToList();
        _regionRepository.GetAll().ToList();
        _storeRepository.GetAll().ToList();
        _askoTillRepository.GetAll().ToList();

        if (_hostingEnvironment.IsDevelopment())
        {
            EventCache evc = new EventCache(_eventRepository, _cache);
            evc.SetEventsCache(_dateFrom, _dateTo);
        }

        events = _cache.Get<List<Event>>("eventsChache");

        Dictionary<int, List<Event>> eventsForPos = new Dictionary<int, List<Event>>();

        events = events == null ? new List<Event>() : events;

        var forEvets = events.FindAll(x => x.DateFrom >= _dateFrom && (x.DateTo <= _dateTo || !x.DateTo.HasValue));

        foreach (Event e in forEvets)
        {
            if (eventsForPos.ContainsKey(e.PosTillId))
            {
                eventsForPos[e.PosTillId].Add(e);
            }
            else
            {
                eventsForPos[e.PosTillId] = new List<Event>() { e };
            }
        }

        foreach (var item in countries)
        {
            var tills = item.Regions.SelectMany(t => t.Stores.Where(e => e.PosTills != null).SelectMany(r => r.PosTills));

            foreach (var till in tills)
            {
                till.Events = eventsForPos.ContainsKey(till.Id) ? eventsForPos[till.Id] : null;
            }

        }

        switch ((PageDetailType)_pageDetailType)
        {
            case PageDetailType.CountriesDetail:

                break;
            case PageDetailType.CountryDetail:
                res = ModelHelper.GetGraphStatusesCountry(countries, _countryId);
                break;
            case PageDetailType.RegionDetail:
                res = ModelHelper.GetGraphStatusesRegion(countries, _regionId);
                break;
            case PageDetailType.StoreDetail:
                res = ModelHelper.GetGraphStatusesStore(countries, _storeId);
                break;
            case PageDetailType.TillDetail:

                break;
            default:
                break;
        }
    }
    catch (Exception ex)
    {
        _logger.LogError(ex.ToString());
        return AmazonDTO.Error(((int)HttpException.InternetServerError).ToString(), ex.ToString());
    }

    return AmazonDTO.Data(res);
}

My initial suggestion is to remove data parsing at the start and move it elsewhere. The code also looks like it is trying to do three things at once based on incoming parameters, maybe split it into three endpoints or use some form of inheritance would help here.

Any ideas are welcomed!

Not using return values of TryParse

The method receives several parameters as string,
and parses them to other types such as DateTime or int.
It's surprising to see calls to TryParse,
and using the value in the out parameters without checking the returned value.

The code basically assumes that the string values are of the correct format.
But if that assumption is always true,
that fact should be at least documented,
but it would be even better to pass the real types instead of string values.
If the assumption is not guaranteed to be true,
then the program may crash.

Repetitive code

The parameter parsing code (string values to DateTime or int) are repetitive, and take at least 2 lines per value.
It would be better to add some helper functions to simplify this,
so that you could write like this:

DateTime _dateFrom = parseDateTime(dateFrom).Date;
DateTime _dateTo = parseDateTime(dateTo);

Pointless initializers

Here, it's pointless to initialize countries with an empty list when it's going to be reassigned right after:

var countries = new List<Country>();

countries = _countryRepository.GetAll().ToList();

The same goes for events.

Single responsibility principle

This method is doing too much,
that's why it's so long.
It would be better to split it up to smaller logical units.

Statements with side effects?

What are these for?

_regionRepository.GetAll().ToList();
_storeRepository.GetAll().ToList();
_askoTillRepository.GetAll().ToList();

It looks like loading objects, converting them to List, and doing nothing with the values.
What's going on here?
At the minimum this should be documented (with comments).

Group related statements together

It's hard to read when related statements are not grouped together, for example:

var events = new List<Event>();

// more than 10 lines of code ...

events = _cache.Get<List<Event>>("eventsChache");

// some code ...

events = events == null ? new List<Event>() : events;

It's hard to follow how the events variable is used when there are unrelated statements in between.

Local variable or field?

The variable names use an inconsistent mix of styles,
which makes the code harder to read.
For example, some local variables have a _ prefix,
others do not.
Some variables defined outside the method also have _ prefix.
Those should probably be properties instead.

Use var more

You used var at some places, but not at others.
For example Dictionary<int, List<Event>> eventsForPos = new Dictionary<int, List<Event>>(); is really tedious and a good candidate for using var.

Bug?

This condition looks like a bug:

x.DateTo <= _dateTo || !x.DateTo.HasValue

If x.DateTo doesn't have a value, won't x.DateTo <= _dateTo crash?
It should be safer if you flip that condition:

!x.DateTo.HasValue || x.DateTo <= _dateTo

Simpler foreach

This can be written simpler:

foreach (Event e in forEvets)
{
    if (eventsForPos.ContainsKey(e.PosTillId))
    {
        eventsForPos[e.PosTillId].Add(e);
    }
    else
    {
        eventsForPos[e.PosTillId] = new List<Event>() { e };
    }
}

Like this:

foreach (Event e in forEvets)
{
    if (!eventsForPos.ContainsKey(e.PosTillId))
    {
        eventsForPos[e.PosTillId] = new List<Event>();
    }
    eventsForPos[e.PosTillId].Add(e);
}

And btw there's a typo in forEvets.

Group case statements with same body together

Instead of this:

switch ((PageDetailType)_pageDetailType)
{
    case PageDetailType.CountriesDetail:
        break;
    case PageDetailType.CountryDetail:
        res = ModelHelper.GetGraphStatusesCountry(countries, _countryId);
        break;
    case PageDetailType.RegionDetail:
        res = ModelHelper.GetGraphStatusesRegion(countries, _regionId);
        break;
    case PageDetailType.StoreDetail:
        res = ModelHelper.GetGraphStatusesStore(countries, _storeId);
        break;
    case PageDetailType.TillDetail:
        break;
    default:
        break;
}

It would be better like this:

switch ((PageDetailType)_pageDetailType)
{
    case PageDetailType.CountryDetail:
        res = ModelHelper.GetGraphStatusesCountry(countries, _countryId);
        break;
    case PageDetailType.RegionDetail:
        res = ModelHelper.GetGraphStatusesRegion(countries, _regionId);
        break;
    case PageDetailType.StoreDetail:
        res = ModelHelper.GetGraphStatusesStore(countries, _storeId);
        break;
    case PageDetailType.CountriesDetail:
    case PageDetailType.TillDetail:
    default:
        break;
}


Getting substrings from a string c# I have a c# application which displays the names of assessments and the zone which they are performed upon, separated by a comma and a space. For instance:

assessment1, Firewall
mom's assessment, LAN

When the user selects on of these assessments, I need to separate the assessment name from the zone name. I also need to replace any single quotes in the string by two single quotes so I can query my database without issues (this i cannot change, it has to be done because no SQLiteParameters are used in the function that queries my database).

This is how I do this currently (note that _view.SelectedAssessment is the entire string of both the assessment name and zone):

string zone = _view.SelectedAssessment.Substring(_view.SelectedAssessment.IndexOf(", ") + 2);
string assessmentName = _view.SelectedAssessment.Substring(0, _view.SelectedAssessment.Length - zone.Length - 2);

Is there a better way to do this? Any improvements?

Make it regex with named groups:

var matcher = new Regex(@"(?<assesment>.+?),\s*(?<zone>.+)");
var match = matcher.Match("assessment1, Firewall");
var assesment = match.Groups["assesment"].Value;
var zone = match.Groups["zone"].Value;

where

(?<assesment>.+?) - catches assesment ungreedy ? so that it stops at ,
,\s* - the separator is , with an optional white space (zero or more times)
(?<zone>.+) - catches zone greedy

Sample at regex101.com

I wasn't aware of that but @Vogel612 kindly mentioned that backtracking as I use it in my solution above might be sometimes a problem: Why Using the Greedy .* in Regular Expressions Is Almost Never What You Actually Want and The Explosive Quantifier Trap.

Using the (?<assessment>[^,]+) expression would be much faster and safer here.

  That change drops the match you propose from 30 steps until matching to 9, so it saves roughly 60% of execution time.

Another valuable remark by @forsvarir suggests using the statick Regex.IsMatch method over creating a new instance each time. This should be much faster here.

  The difference in performance is due to the caching of regular expressions used in static method calls.

There is a lot more to that with pleny of examples on Optimizing Regular Expression Performance, Part I: Working with the Regex Class and Regex Objects [Ron Petrusha]

Why do I prefer the regex solution?

I can test it with regex101
I can use the same pattern for validation and data extraction. I know whether the data is in the correct format without checking each element of the array values[i] separately.
I can easier restrict the characters or casing or splitting rules
I can use named groups which I find easier to understand because I can see what I work with rather then just having a string[] especially that each filed has a meaning - otherwise I'd use the Regex.Split method if was just an array of some values
I can trim the data at the same time (requires some additional \s*)
I can use the same pattern for Regex.Replace if I wanted to change it


Investment/betting simulation I am creating an investment/betting simulation; since is the first I create , I am sure it can be done better, both from a coding and a simulation point of view.

I would like to ask you what you think and if you can share your experience.

There are 5 main variables that the user has to input:

z --> it is my average winning percentage. As discuss later, a random number between 0 and 100 will be generate. If the generated number (x) is greater than (z), then the trade is loss; if x < z, is a win.
invest --> how much money invested per trade, es. 10$. In case of a win, the return is always the 70%. i.e. bet 10$, win 7$ (plus the bet)
days --> how many days I trade. there is fixed amount of trade per day, which is between 9 and 33
simulazioni --> the number of simulations
varianza --> is the shifting probability. If varianza is 5% and my percentage (z) is 50%, my actual variance could be either 45% or 55%. This is calculated randomly for each trade.

As you can see from the code at the bottom, the simulation is based on "for loops". The main one is the simulations, second one are the days and the last one calculates if the trading is positive or negative.

There are also dictionaries that I use to collect data from daily profit and total profit (i.e. "datatot"; these dictionaries are used to plot data later on (this bit is not included in the code).

import random
import matplotlib.pyplot as plt
import matplotlib.mlab as mlab
import numpy as np

data=[]
data2=[]
datatot = []
portafoliotot = 0
portafolio = 0
pos= 0
neg= 0

#input variables
z= input("Percentuale: ")
invest = input("Investimento:")
days = input ("Giorni:")
simulazioni = input("Simulazioni:")
varianza = input("Varianza:")

for simulazione in range(0,int(simulazioni)):

    for giorno in range(0,int(days)):

        portafoliotot = 0 #simulation portfolio
        portafolio = 0 #daily portfolio
        pos = 0 #n of positive trades
        neg = 0 #n of negative trades

        for trade in range(0,int(random.randint(9,33))):

            x=random.randint(0,100) #random number to see if the trade is pos or neg

            calcvarianza = 2 * np.random.rand() - 1 # calculation of (varianza). if positive, is going to be added to (z), otherwise subtracted. this is done for each trade.

            if calcvarianza > 0:
                varianza = - int(varianza)

            if x<(int(z)+int(varianza)): #checking if the trade is pos or neg

                portafolio += (int(invest)/100)*70
                pos +=1
            else:
                portafolio += -int(invest)
                neg +=1

        portafoliotot += portafolio
        data.append(portafoliotot)
        data2.append(portafoliotot)

    datatot.append(sum(data2))
    data2 = []

Idiomatic Python suggestions:

Use argparse rather than input to set the configuration. It is easy to use, and very powerful.

General suggestions:

data and data2 are literally considered the world's worst variable names. Naming is well known to be a hard problem, but it's also a great skill to learn. Naming a thing well gives much needed information to anyone reading the script, including yourself.
Comments like "checking if the trade is pos or neg" are code smells which point to an opportunity to extract a class or function.
Your program contains many magic values, like 33. Use constants or set them as defaults for arguments to make it clear what these numbers mean.
Your program neither prints nor returns anything. In other words it can be replaced by the single line pass.
This may sound chauvinist, but use English names for things in your program. The world's developers have avoided localising programming languages for a long time, and in a programming job you are very likely going to be expected to write everything except user interfaces in English. As long as practically every function, class, package and module in the world is named in English, you are going to improve maintainability by using a single language in your programs. Disclaimer: I'm not a native English speaker.


One-way flight trip implementation in Python I came across the following problem (found on the SO link below):
https://stackoverflow.com/questions/2991950/one-way-flight-trip-problem

  Given a flight trip that includes a number of transfers: we have a
  ticket for each part of the trip; we do not stop twice in the same
  airport; each ticket contains source and destination airport; all the
  tickets we have are randomly sorted, and we forgot the original
  departure airport and the last destination. We want to design an
  algorithm to reconstruct the trip with minimum big-O complexity.

I tried to solve the problem in python in O(n) time complexity. Here is my solution:

L = [('A', 'B'), ('B', 'C'), ('C', 'D'), ('D', 'E'), ('E', 'F'), ('F', 'G'), ('G', 'H'), ('H', 'I')]
# Note: the input L should normally be shuffled.

def getDepartureArrival(L):
    sources, destinations = zip(*L)
    sources = set(sources)
    destinations = set(destinations)

    for s in sources:
        if s not in destinations:
            depart = s
            break
    for d in destinations:
        if d not in sources:
            arriv = d
            break
    return depart, arriv
#

def getTrip(H):
    trip = [depart]
    k = depart
    while k != arriv:
        k = H[k]
        trip.append(k)
    return trip
#

depart, arriv = getDepartureArrival(L)
H = {s:d for (s,d) in L}
print getTrip(H)

I would like to know if it is possible to have a clearer and more compact Python code than the one I wrote with more than 4 consecutive loops.

You should not use global variables when not necessary. You can just directly pass depart and arriv to getTrip.

You should strive to have readable variable names. Just use arrival and departure.

Python has an official style-guide, PEP8, which recommends using lower_case_with_underscore for variables and functions.

You can use map to cast both sources and destinations to sets directly. You can also use set subtraction to get the difference value only in one of them.

The dict constructor can directly take an iterable of pairs to build a dictionary.

You can make get_trip a generator, which simplifies the logic slightly.

I would add a if __name__ == "__main__": guard to allow importing stuff from this module without running the whole code.

import random

def get_departure_arrival(flights):
    """
    Find departure and arrival airport from chain of flights.

    Effectively the opposite of the itertools recipe pairwise:

    >>> get_departure_arrival([(s0, s1), (s1, s2), (s2, s3)])
    (s0, s3)

    Pairs do not have to be in order:

    >>> get_departure_arrival([(s0, s1), (s2, s3), (s1, s2)])
    (s0, s3)
    """
    sources, destinations = map(set, zip(*flights))

    depart = (sources - destinations).pop()
    arriv = (destinations - sources).pop()

    return depart, arriv

def get_trip(flights, depart, arriv):
    """
    Reconstruct a flight trip from pairs of departure and
    arrival airports and the first and last airport of the trip.
    """
    flight_dict = dict(flights)
    while depart != arriv:
        yield depart
        depart = flight_dict[depart]

if __name__ == "__main__":
    flights = [('A', 'B'), ('B', 'C'), ('C', 'D'), ('D', 'E'),
               ('E', 'F'), ('F', 'G'), ('G', 'H'), ('H', 'I')]
    random.shuffle(flights)

    depart, arriv = get_departure_arrival(flights)
    print list(get_trip(flights, depart, arriv))


Code to calculate minimum number of pushes required to put the whole dominoes set down The question is this one on UVa, and it basically gives us n, the number of dominoes and (a,b) relations such that pushing a will cause b to fall down too. First t, the test cases is inputted and then number of vertices and edges, and the (a,b) relations.

My approach

My approach was I first run a dfs on the graph (till all elements are reached) and maintain an array that shows when each vertex was reached [somewhat inspired from Kosaraju's algo for SCCs]. If a->b->c is my dfs tree, reached[c] will be equal to 3, reached[b] be 2, and reached[a] be 1. This step is O(vertices+edges).

Then I run DFSs from vertices starting from those with minimum reached value. Every time the DFS runs, I increment a counter. This is also O(vertices+edges).

I tried for smaller test cases and got the right answer, but submitting on UVa gives me TLE. I don't know if my logic is wrong or if my implementation is messed up somewhere. The code is here:

#include<bits/stdc++.h>
using namespace std;

int n,counter,reached[100002],visited[100002],visiting[100002];
void dfs(int u,vector< vector<int> > graph){
    visiting[u]++;
    for(auto const& i:graph[u]){
        if(!visiting[i])
            dfs(i,graph);
    }
    visited[counter]=u;
    counter--;
}
void dfs2(int u,vector< vector<int> > graph){
    reached[u]++;
    for(auto const& i:graph[u]){
        if(!reached[i])
            dfs2(i,graph);
    }
}
int main()
{
    int t;
    cin>>t;
    while(t--){
        memset(visiting,0,sizeof visiting);
        memset(visited,0,sizeof visited);
        cin>>n;
        vector< vector<int> > graph(n+1);
        int m;
        counter=n;
        cin>>m;
        while(m--)
        {
            int a,b;
            cin>>a>>b;
            graph[a].push_back(b);
        }
        int mcounter=0;
        for(int i=1;i<n+1;i++)if(!visiting[i])dfs(i,graph);
        memset(reached,0,sizeof reached);
        for(int i=1;i<n+1;i++){
            if(!reached[visited[i]]){
                dfs2(visited[i],graph);
                mcounter++;
            }
        }
        cout<<mcounter<<'\n';
    }
    return 0;
}

Alternate algorithm
I would suggest the following:

After reading the list of domino dependencies, you should be able to compute a set of "root" dominos.  A "root" domino is simply a domino that is not pushed by any other domino.
Each root domino must be pushed, since there is no other domino to push it.  So for each root domino, push it, and do a dfs to mark all dominos knocked down by it.
After step #2, there may be dominos left standing.  These dominos must either be part of a cycle, or hanging off of a cycle.
For each remaining domino, do a dfs on it, looking for a cycle.
If the domino did not cycle, then do not increment the push count, but mark all the dominos in the dfs as knocked over.  This was a domino hanging off a cycle, and you know that this domino's parent(s) will eventually get knocked over by some other push.
If the domino did cycle, then keep a list of each domino that was part of its cycle (note that there could be more than one cycle through the same domino).  After doing the dfs and marking all dominos as knocked down, go through the list of dominos that were part of its cycle.  If any of them have a parent that is still standing, then do not increment the push count.  The reason is that the still standing parent will get pushed by some other domino, and it will knock down this whole cycle.  In other words, if a cycle of dominos has an external parent, then it can be ignored because the push from another cycle that triggers the external parent will knock down this cycle.

I haven't actually written a program to test this algorithm, so there could be a flaw in my logic dealing with cycles.  But I think it should work, and it should be fast because you never process a node more than once.


function swap 2 nodes in doubly linked list Helo everyone, I need to review on function swapNode in doubly linked list. It works but I want to make it clearer and better. Are there any errors like memory leak in my code?

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

struct node;
struct list;

typedef struct node node;
typedef struct list list;

struct node
{
    int point;
    char name[30];
    node *next;
    node *prev;
};

struct list
{
    node *head;
    node *tail;
    int count;
};

node *createNewNode(int point, char name[30], node *prev, node *next);
list *createList();
void insertHead(list *listNode, int point, char name[30]);
bool compareName(char a[30], char b[30]);
void swapNode(list *listNode, char nameA[30], char nameB[30]);

int main()
{
    list *listNode = createList();

    insertHead(listNode, 10, "abc def");
    insertHead(listNode, 9, "qwe rty");
    insertHead(listNode, 8, "ui op");
    insertHead(listNode, 30, "fgh jkl");
    insertHead(listNode, 1234, "akaka");

    swapNode(listNode, "fgh jkl", "akaka");

    node *temp = listNode->head;
    while (temp != NULL)
    {
        printf("%-20s%d\n", temp->name, temp->point);
        temp = temp->next;
    }

    return 0;
}

node *createNewNode(int point, char name[30], node *prev, node *next)
{
    node *newNode = (node *)malloc(sizeof(node));
    newNode->point = point;
    strcpy(newNode->name, name);
    newNode->next = next;
    newNode->prev = prev;
    return newNode;
}

list *createList()
{
    list *listNode = (list *)malloc(sizeof(list));
    listNode->count = 0;
    listNode->head = NULL;
    listNode->tail = NULL;
    return listNode;
}

void insertHead(list *listNode, int point, char name[30])
{
    node *newNode = allocateNewNode(point, name, NULL, listNode->head);
    if (listNode->head)
        listNode->head->prev = newNode;
    listNode->head = newNode;
    if (listNode->tail == NULL)
        listNode->tail = newNode;
    ++listNode->count;
}

bool compareName(char a[30], char b[30])
{
    for (int i = 0; i < 31; i++)
    {
        if (a[i] != b[i])
            return false;
        if (a[i] == '\0')
            break;
    }

    return true;
}

void swapNode(list *listNode, char nameA[30], char nameB[30])
{
    node *A = NULL, *B = NULL;
    node *temp = listNode->head;

    for (int i = 0; i < listNode->count; i++)
    {
        if (compareName(temp->name, nameA))
            A = temp;
        else if (compareName(temp->name, nameB))
            B = temp;
        temp = temp->next;
        if (A && B)
            break;
    }
    if (!A || !B)
        return false;
    else if (A == B)
        return false;

    node p=*A;
    *A=*B;
    *B=p;

    B->next = A->next;
    B->prev = A->prev;

    A->next = p.next;
    A->prev = p.prev;
}

Don't Return Values from void Functions

    if (!A || !B)
        return false;
    else if (A == B)
        return false;

The function void swapNode(List *listNode, char nameA[30], char nameB[30]) is declared void, which means it doesn't return a value, yet it attempts to return false in two places. Some compilers actually report this as an error. Rather than return false; it should just be return;.

The two if statements above could be rewritten as one if statement

    if ((!A || !B) || (A == B))
    {
        return;
    }

Missing Error Checking

The C programming memory allocation functions malloc(size_t size), calloc(size_t count, size_t size) and realloc( void *ptr, size_t new_size) may fail. If they do fail then they return NULL. Any time one of these functions are called, the result should be tested to see if it is NULL. Referencing fields through a NULL pointer yields unknown behavior and is generally a bug.

Node *safeMalloc(size_t size)
{
    Node* newNode = malloc(size);
    if (newNode == NULL)
    {
        fprintf(stderr, "Memory allocation failed in safeMalloc\n");
        exit(EXIT_FAILURE);
    }

    return  newNode;
}

Node *createNewNode(int point, char name[30], Node *prev, Node *next)
{
    Node *newNode = safeMalloc(sizeof(*newNode));
    newNode->point = point;
    strcpy(newNode->name, name);
    newNode->next = next;
    newNode->prev = prev;

    return newNode;
}

Declarations of Node Structs

It might have been easier to write the struct declarations as

typedef struct node
{
    int point;
    char name[30];
    struct node *next;
    struct node *prev;
} Node;

typedef struct list
{
    Node *head;
    Node *tail;
    int count;
} List;

Complexity

The function void swapNode(List *listNode, char nameA[30], char nameB[30]) can be simplified by breaking it into 2 functions, one that does the comparisons and then calls a swaping function as necessary:

void doSwap(Node *A, Node*B)
{
    Node p=*A;
    *A=*B;
    *B=p;

    B->next = A->next;
    B->prev = A->prev;

    A->next = p.next;
    A->prev = p.prev;
}

void swapNode(List *listNode, char nameA[30], char nameB[30])
{
    Node *A = NULL, *B = NULL;
    Node *temp = listNode->head;

    for (int i = 0; i < listNode->count; i++)
    {
        if (compareName(temp->name, nameA))
        {
            A = temp;
        }
        else if (compareName(temp->name, nameB))
        {
            B = temp;
        }
        temp = temp->next;
        if (A && B)
        {
            break;
        }
    }

    if ((A && B) && (A != B))
    {
        doSwap(A,B);
    }
}

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

  that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

A Good Habit for Programming in C and C++

For readability and maintainability a good good habit (best practice) to get into is to always put the actions in if statements and loops into braces ({ and }) as shown in the previous example. One of the major causes of bugs is to add a single line to the contents of an iff statement and to forget to add the necessary the necessary braces. This type of problem is very hard to track down when it doesn't result in a compiler error.

Leaks

  Are there any errors like memory leak in my code?

If the code was part of a larger project there would be memory leaks, the function free(void *ToBeFreed) is never called. It might be better if some linked list operations such as deleteNode() were added to the code.


How could I make my controller a little less hideous? I have a controller. It functions as follows:

A user imports a file and decides whether the files contents contain new entries to be placed into a database, or existing entries entries to be updated.

The user clicks on the import button and is sent to the controller below. If there are any warnings or errors, the validation results screen is shown - to help the users correct their data.

If the file doesn't contain any errors or warnings, the validation screen will be skipped and their file contents will be inserted/updated into the database.

This is clearly far too much logic for a controller as I have read that the controller is simply meant to decide which view to send the data to. I am having a rather difficult time determining how to extract out the data.

I am looking for suggestions on what my next step should be. Thank you to anyone who takes time to help me.

public ActionResult ValidationResultsScreen(HttpPostedFileBase file, DatabaseAccessMode databaseAccess)
{
    try
    {
        var fileLocation = Path.Combine(Path.GetTempPath(), string.Join(Guid.NewGuid().ToString(), file.FileName));

        Session["TempFileLocation"] = fileLocation;
        Session["DatabaseAccessMode"] = databaseAccess;

        file.SaveAs(fileLocation);

        var importer= new Importer(_connectionString, _logManager, databaseAccess,Server.MapPath("..\\bin"));

        var parsedValues= importer.ParseFile(fileLocation);

        var failedValues = ParsedValues.Where(x => x.ParsedState != ParsedState.Success);

        if (failedValues.Any())//if there were any warnings or errors display the validation results screen
        {
            return View(processedValues);
        }
        //else import the file to the database

        var transactionResults = importer.UploadToDatabase(parsedValues);

        var failedDBTransactions= RetrieveFailedResults(transactionResults);

        return failedDBTransactions.Any()
            ? View("DatabaseImportErrorScreen", failedDBTransactions)
            : View("DatabaseImportResults", transactionResults);
    }
    catch (Exception exception)
    {
        ViewBag.ExceptionMessage = exception.Message;
        return View("DefaultErrorScreen");
    }
}

Based on your case your controller seems to be mixing multiple things at the same time. The total context is not clear but it seems this is a controller method in a controller that does more than this, correct?

In general I would suggest to see it different. An import is an object, so create a seperate controller for it: ImportsController. That can be a very standard controller:

Add: User add a new file, controller just sends it to Import model -> save. The model saves it in a temp file or whatever it needs to do. You can implement validation here already but you can also say: "A upload and saved file is ok. That is a correct creation of an import."

Pseudo code:

if(dataIncoming) {
    this->Import->save(data)
    //handle file save errors, no data received etc. All coming back from model which should take care of the validation.
    //redirect to edit
}

Edit: Next step is to review the import. Here you can ask the model to validate.

Pseudo code:

ON SAVE (if this is requested without a save/submit action just show data and validation results)
this->Import->parseFile(data) //validate

//show parse file issues to user if any
return View(processedValues);

if(no errors in parsefile) {
  //try database save
  Import->saveInDatabase
  //show validation errors if needed

  //if all ok you can redirect (or have user interaction) to delete
}

View: Always shows you the data of the import

Delete: Can be used to remove a wrong import upload (user uploaded file twice) or when import is done. You can validate this.

Here you can implement also a soft delete for example.

Pseudo code:

Import->delete

So as you see I would move all that file, tmpfile and other parts to the model by seeing an import as a standalone object. That makes it possible to have all encapsulated and also well testable. A controller method doing all steps is much harder to test. For example you would need to catch all different views rendered etc. I prefer to split it up.


Filling up a hand with random cards that are not yet drawn - Monte Carlo I would like to know if there is a more efficient way to speed up below code. This function is meant to fill in a set of poker hand with the remaining
cards using Mersenne Twister for a Monte Carlo simulation.

void HandEvaluator::RandomFill(std::vector<std::shared_ptr<Card>>& _Set, std::vector<std::shared_ptr<Card>>& _Dead, unsigned int _Target){
    //Add the cards that are currently in Set as dead cards
    for (auto const& CardInSet : _Set)
    {
        if (CardInSet == nullptr)
            break;

        _Dead.push_back(CardInSet);
    }

    bool IsDead;
    unsigned int RequiredAmt = _Target - _Set.size();

    unsigned int CardIndex = 0;
    std::uniform_int_distribution<unsigned int> CardsDistribution(0, 51);

    for (unsigned int Index = 0; Index < RequiredAmt; Index++)
    {
        while (true)
        {
            IsDead = false;
            CardIndex = CardsDistribution(MTGenerator);

            for (auto const& Dead : _Dead)
            {
                if (ReferenceDeck[CardIndex]->GetRank() == Dead->GetRank() && ReferenceDeck[CardIndex]->GetSuit() == Dead->GetSuit())
                {
                    IsDead = true;
                    break;
                }
            }

            if (!IsDead)
            {
                _Set.push_back(ReferenceDeck[CardIndex]);
                break;
            }
        }
    }
}

The Visual Studio Profiler had identified that this line

    CardIndex = CardsDistribution(MTGenerator);

is the main culprit behind the high compute time. Is Mersenne Twister itself not meant for a Monte Carlo Simulation and another PRNG should be used instead ? Or there are some inefficient lines that I had missed out ?

In the end, I opt to replace Mersenne Twister with another PRNG, xoroshiro128+, it managed to cut the compute time by around 15%. I also did some minor optimization on the for-loop but the improvement is minuscule. Anyway, here is the same function but with new PRNG:

Seeding in HandEvaluator's constructor:

HandEvaluator::HandEvaluator()
{
    Initialize();
    MTGenerator.seed(std::chrono::high_resolution_clock::now().time_since_epoch().count());

    s[0] = std::chrono::high_resolution_clock::now().time_since_epoch().count();
    std::cout << "Seed 1: " << s[0] << "\n";

    s[1] = std::chrono::high_resolution_clock::now().time_since_epoch().count();
    std::cout << "Seed 2: " << s[1] << "\n";
}

HandEvaluator's RandomFill function:

void HandEvaluator::RandomFill(std::vector<std::shared_ptr<Card>>& _Set, std::vector<std::shared_ptr<Card>>& _Dead, unsigned int _Target)
{
    //Add the cards that are currently in Set as dead cards
    for (auto const& CardInSet : _Set)
    {
        if (CardInSet == nullptr)
            break;

        _Dead.push_back(CardInSet);
    }

    bool IsDead;
    unsigned int RequiredAmt = _Target - _Set.size();

    for (unsigned int Index = 0; Index < RequiredAmt; Index++)
    {
        while (true)
        {
            _Set.push_back(ReferenceDeck[next() % 52]);

            IsDead = false;

            for (auto const& Dead : _Dead)
            {
                if (Dead->IsEqualTo(_Set[_Set.size() - 1]))
                {
                    IsDead = true;
                    break;
                }
            }

            if (IsDead)
                _Set.pop_back();
            else
                break;
        }
    }
}


Simple static integer stack This question is about improving my C++ coding skills. I was asked to implement a simple static integer stack in C++ as an assignment. I've come up with the following code:

class myStaticIntStack
{

    int stackSize;
    int storedElements;
    int *elements;

public:

    myStaticIntStack();
    myStaticIntStack( int aNumber );

    ~myStaticIntStack();

    int peek();

    int pop();

    void push( int element );

};

myStaticIntStack::myStaticIntStack()
{
    this->stackSize = 1;
    this->elements = new int(0);
    this->storedElements = 0;
}

myStaticIntStack::myStaticIntStack( int stackSize )
{
    this->stackSize = stackSize;
    this->elements = new int[ stackSize ];
    this->storedElements = 0;
}

myStaticIntStack::~myStaticIntStack()
{
    if( this->elements != NULL )
    {
        if( stackSize > 1 )
            delete[] this->elements;
        else
            delete this->elements;
    }

}

void myStaticIntStack::push( int newElement )
{
    if( this->storedElements == this->stackSize )
        cout << "Stack is full, you must POP an element before PUSHing a new one!" << endl;
    else
    {
        this->elements[ (this->stackSize - 1) - this->storedElements ] = newElement;
        this->storedElements++;
    }
}

int myStaticIntStack::pop()
{
    if( this->storedElements == 0 )
    {
        cout << "Stack is empty, you must PUSH an element before POPping one!" << endl;
        return -1;
    }
    else
    {
        storedElements--;
        return this->elements[ (this->stackSize - 1) - this->storedElements ];
    }
}

int myStaticIntStack::peek()
{
    if( this->storedElements == 0 )
    {
        cout << "Stack is empty, you must PUSH an element before PEEKing one!" << endl;
        return -1;
    }
    else
    {
        return this->elements[ this->stackSize - this->storedElements ];
    }
}

int main()
{
    myStaticIntStack aStack(3);

    cout << "Popped Element: " << aStack.pop() << endl;

    aStack.push(1);
    cout << "Stack Top is: " << aStack.peek() << endl;
    aStack.push(2);
    cout << "Stack Top is: " << aStack.peek() << endl;
    aStack.push(3);
    cout << "Stack Top is: " << aStack.peek() << endl;
    aStack.push(4);
    cout << "Stack Top is: " << aStack.peek() << endl;

    cout << "Popped Element: " << aStack.pop() << endl;
    cout << "Stack Top is: " << aStack.peek() << endl;
    cout << "Popped Element: " << aStack.pop() << endl;
    cout << "Stack Top is: " << aStack.peek() << endl;
    cout << "Popped Element: " << aStack.pop() << endl;
    cout << "Stack Top is: " << aStack.peek() << endl;

    return 0;
}

The code is compiling and running correctly, this is standard output:

Stack is empty, you must PUSH an element before POPping one!
Popped Element: -1
Stack Top is: 1
Stack Top is: 2
Stack Top is: 3
Stack is full, you must POP an element before PUSHing a new one!
Stack Top is: 3
Popped Element: 3
Stack Top is: 2
Popped Element: 2
Stack Top is: 1
Popped Element: 1
Stack is empty, you must PUSH an element before PEEKing one!
Stack Top is: -1

However I was given a B due to the following reasons:

There was a better way of implementing it.
I didn't manage well the case in which I'm trying to POP an element from an empty stack, since it's confusing that pop() returns a value even if Stack is empty.

Can you please help me understand how I could improve my code?

First of all, your code has a bug: You did not follow the rule of three.
Code such as myStack1 = myStack2; will cause the pointer to be deleted twice - which is undefined behavior.

int storedElements;

This seems kind of misleading. This variable doesn't hold the stored elements. It holds the number of stored elements. Perhaps storedElementsCount or something like that would be better?

myStaticIntStack( int aNumber );

Why did you call the variable aNumber here? aNumber is a terrible name. It tells me essentially nothing. Later you used stackSize which is a FAR better name. Why didn't you use it here too?

And, by the way, consider using size_t to store sizes and capacities instead of ints. This would apply to stackSize and storedElements.

myStaticIntStack::myStaticIntStack()
{
    this->stackSize = 1;
    this->elements = new int(0);
    this->storedElements = 0;
}

Seriously? The default behavior for the stack is to create a stack with maximum size 1? That is kind of... worthless. It's OK to allow this in the myStaticIntStack(int) constructor, but as a default it just seems odd.

Consider not even allowing a default constructor.

myStaticIntStack::myStaticIntStack( int stackSize )
{
    this->stackSize = stackSize;
    this->elements = new int[ stackSize ];
    this->storedElements = 0;
}

Can the stackSize be zero? Can it be negative? Consider adding an assertion.
By the way, if you make stackSize be a size_t, the negative case becomes impossible, since size_t is unsigned. But you'll still need to handle the "stackSize == 0" case.

A size of 0 might be acceptable. If that is the case, add a comment stating that and why it is so.

Consider declaring this constructor explicit.

myStaticIntStack::~myStaticIntStack()
{
    if( this->elements != NULL )
    {
        if( stackSize > 1 )
            delete[] this->elements;
        else
            delete this->elements;
    }

}

Why do you need a NULL check? Are you expecting the destructor to be called with elements == NULL? If this merely defensive programming and this case is never supposed to happen, then leave it as an assertion.

Also, that whole delete vs delete[] is weird. What if I use the constructor myStaticIntStack(1)? Then you'll be deleteing something created with new[].

I'd change the code (from the constructors) so that new[] - not new - is always used and, therefore, delete[] is always the right thing to do in the destructor.

void myStaticIntStack::push( int newElement )
{
    if( this->storedElements == this->stackSize )
        cout << "Stack is full, you must POP an element before PUSHing a new one!" << endl;
    else
    {
        this->elements[ (this->stackSize - 1) - this->storedElements ] = newElement;
        this->storedElements++;
    }
}

This is improper error handling that violates the single responsibility principle.
Your function should either throw an exception, merely assert the condition or return an error code. Printing to the command line should be done elsewhere - probably outside this class.

this->elements[ (this->stackSize - 1) - this->storedElements ] = newElement;

I'm pretty sure you could simplify this. For a stackSize=16, the first position is at index 15. Why 15? Why not 0? That'd simplify the code to just elements[storedElements] = newElement;. Just remember to also fix the pop/peek code.

Just because the stack is Last-in-first-out doesn't mean you have to fill the last indexes in the internal array first. That's an implementation detail.

  it's confusing that pop() returns a value even if Stack is empty.

Then don't  return a value if the Stack is empty. Throw an exception or abort with an assertion. Problem solved. Next.
As I stated before, your couts are in the wrong place. If you fix that the "Pop return" problem might just naturally go away.

If you decide to keep it like that (return -1), then consider making -1 a named constant since magic numbers are bad.

return ERROR_STACK_IS_EMPTY;

int myStaticIntStack::peek()

Same thing about cout.
Also, peek() doesn't change anything, does it? Then consider making it const.

int myStaticIntStack::peek() const

Consider splitting your myStaticIntStack in two files: myStaticIntStack.hpp and myStaticIntStack.cpp.
Your main function would be in a third file.

Some extra thoughts:

I suspect the "better way" might be to just use a vector internally. This would painlessly solve your "rule of three" problem. Bonus points since it'd make it easier to "resize" the stack after being created if you ever wanted to add that feature.
Personally, I'd uppercase the first letter of the class name, but that's just personal preference. Nothing wrong with your particular style.
Consider adding a bool empty() function, that checks if the stack is empty.
Consider adding a bool full() function, that checks if the stack is full.
Those two functions above could make your error detection code easier to understand. If your assignment forbids adding extra functions, consider adding them as private functions.
Consider adding a int size() function, that returns storedElements.
Consider adding a int capacity() function, that returns stackSize
Normally, a class like this would be implemented as a template since it's useful to have stacks for more than just integers. Not sure if you've learned about templates yet.

If you follow QuentinUK's suggestion of removing the this->, beware. There is one case that might cause you trouble.
In the constructor, this->stackSize = stackSize; is correct, but stackSize = stackSize; would not be.
To work around this issue, you could use something like this:

/*optionally add "explicit" here*/ myStaticIntStack::myStaticIntStack( int stackSize ) :
    stackSize(stackSize)
{
    elements = new int[ stackSize ];
    storedElements = 0;
}

This is all I can think of right now. Hope this helps.


Reading transactions from a file and writing them to a database table I am learning Node.js and I would like to see if I'm using it correctly:

var mysql = require('mysql');
var fs = require('fs');

var connectionProps = JSON.parse(fs.readFileSync('connection.json', 'utf8'));

var connection = mysql.createConnection({
  host: connectionProps.host,
  user: connectionProps.user,
  password: connectionProps.password,
  database: connectionProps.database
});

connection.connect();

function checkErr(err) {
  if (err) {
    connection.end();
    throw err;
  }
}

connection.query('SELECT * FROM TRANSACTION_TYPE', function(err, rows, fields) {
  checkErr(err);

  var typeNameToId = {};

  for (var i = 0; i < rows.length; i++) {
    typeNameToId[rows[i].NAME] = rows[i].ID;
  }

  fs.readFile(process.argv[2] + '.csv', 'utf8', function(err, content) {
    checkErr(err);

    var data = [];
    var lines = content.split(/\r\n?|\n/);

    for (var i = 0; i < lines.length; i++) {
      if (lines[i].trim() === '') {
        continue;
      }

      data.push(lines[i].split(','));
      var typeName = data[data.length - 1][2];
      data[data.length - 1][2] = typeNameToId[typeName];
      var dateTokens = process.argv[2].split('-');
      data[data.length - 1].push(new Date(dateTokens[0], dateTokens[1] - 1, dateTokens[2]));
    }

    connection.query(
      'INSERT INTO TRANSACTION (DESCRIPTION, AMOUNT, TRANSACTION_TYPE_ID, TRANSACTION_DATE) VALUES ?',
      [data],
      function(err) {
        connection.end();

        if (err) {
          throw err;
        }
      }
    );
  });
});

Connection

You get your connection with the standard mysql.createConnection method. OK, but you also use bare JSON sub-fields to initialise it. What if they're not there? You're going to get a ReferenceError whent the call tries to access it. I'd advise checking that and logging the mistake.

function createDatabaseConnection(properties) {
    if(properties.host && properties.user && properties.pass && properties.database) {
        return mysql.createConnection({
            host: properties.host,
            user: properties.user,
            password: properties.password,
            database: properties.database
        });
    }
    else {
        console.log("Could not retrieve all required details to connect to the database.");
    }
}

var connection = createDatabaseConnection(connectionProps);

Naming

There are a few - not many - places where your naming could be better.

checkErr - you could name this handleErrors, which would reflect slightly better what it does.
Table names - are your tables really called TRANSACTION_TYPES, all caps? If they are, OK, there's no standard, but if you're only doing it to match the SQL caps style, you shouldn't be.
typeNameToId- that reads like a method name. Perhaps call thistypeIds`?

Magic arguments

process.argv[2] - how do you know that what you want is always argument number 3? This might be a good point to look at implementing switches on the command line. You seem to be getting a file name here, perhaps you should use -f filename as the switch.

You've also got a number of array accesses which are done with magic numbers, though that's more common and not a massive issue.

Query errors

In your second query:

connection.query(
  'INSERT INTO TRANSACTION (DESCRIPTION, AMOUNT, TRANSACTION_TYPE_ID, TRANSACTION_DATE) VALUES ?',
  [data],
  function(err) {
    connection.end();

    if (err) {
      throw err;
    }
  }
);

You're redefining how you handle errors. If you simply pass a reference to checkErr (or whatever you've called it, by then) as the third argument instead of the function, you get consistent error handling. I notice your method definition here is different to that in checkErr - is there a reason you're closing the connection before checking if the error actually exists this time?


Using conditional if in react I have the following code working fine:
rowData is an object and I want to display download button only if the completePathName is not null and hence I did the following which is working fine. I am wondering if I can improve the following code in some manner or the conditional if that I have used is fine?
buttonTemplate = (rowData: any, column: any)  => {
        //if(typeof rowData !=='undefined' && typeof rowData.completePathName !=='undefined'){
        if(rowData.completePathName === null){
            return (

              console.log("Inside first return  - testing rowData:"),
              console.log(rowData),
              console.log(rowData.completePathName),

                            <Button type='button' icon="pi pi-pencil" style={{marginRight: '5px'}} onClick={(e) => this.handleClick(rowData, e)} tooltip='Edit'/>
                            <Button icon="pi pi-trash" style={{marginRight: '5px'}} tooltip='Delete' />

            );

        }
        else {
            return (

                console.log("Inside second return  - testing rowData:"),
                console.log(rowData),
                console.log(rowData.completePathName),

                            <Button type='button' icon="pi pi-pencil" style={{marginRight: '5px'}} onClick={(e) => this.handleClick(rowData, e)} tooltip='Edit'/>
                            <Button icon="pi pi-trash" style={{marginRight: '5px'}} tooltip='Delete' />
                            <Button icon="pi pi-download" tooltip='Download' />

            );

        }

    }

The only thing that changes in the JSX is the download button, so it'd be more appropriate to use conditional rendering when the JSX comes to that line to decide whether you need to render the last button or not:
const ButtonTemplate = (rowData: any) => (

            <Button type='button' icon="pi pi-pencil" style={{marginRight: '5px'}} onClick={(e) => this.handleClick(rowData, e)} tooltip='Edit'/>
            <Button icon="pi pi-trash" style={{marginRight: '5px'}} tooltip='Delete' />
            {
              rowData.completePathName !== null &&
              <Button icon="pi pi-download" tooltip='Download' />
            }

);

Other notes:

Since you don't use the column parameter, you should remove it entirely

Using any defeats the whole point of using TypeScript, since it effectively disables type-checking for that expression - if you can give proper types to things, it'll make your life a whole lot easier. Consider figuring out how you can type rowData. It may look something like:
rowData: {
  completePathName: string | null
}

(or something similar - just for an example)

If you want to be able to call this as a component, give it an upper-case name: ButtonTemplate, not buttonTemplate. That'll let you do
<ButtonTemplate {...{ rowData, column }} />

to render it.

You have a lot of inline styles, which makes the JSX harder to read. Consider separating out the style into CSS instead. With Sass, you could give the container a class of buttons-container, and then do:
.buttons-container {
    text-align: center;
    width: 6em;
    button:not([tooltip="Download"]) {
        margin-right: 5px;
   }
}

and remove all the inline styles from the JSX.


Simple text localization system My first question relates to the first Python module that I published on Python. This is a very small and simple module, the package contains only one class. This is a port of a Perl module Locale::Wolowitz this is a module for managing translations, but uses the serialization format JSON or YAML. Below the code of my Python class, I will like to have an opinion on the code:

import glob
import re
import os.path
from collections import defaultdict

class Pylocwolowitz(object):
    '''Pylocwolitz is a very simple text localization system, meant to be used
by web applications (but can pretty much be used anywhere). Yes, another
localization system.'''

def __init__(self, path, format_deserializer='json'):

    if format_deserializer not in ('json', 'yaml'):
        raise ValueError('FormatNotSupported')

    self.path = path
    self.format_deserializer = format_deserializer
    self.locales = defaultdict(dict)
    self._find_file()

def _find_file(self):
    '''Find all json files'''
    listing = glob.glob(
        os.path.join(self.path, '*.' + self.format_deserializer))
    for infile in listing:
        self._make_loc(infile)

def _make_loc(self, infile):
    '''Store content of the file in a memory'''
    lang = os.path.basename(infile).split('.')[0]
    if self.format_deserializer == 'json':
        import json
        with open(infile) as fil:
            data = json.load(fil)
    else:
        import yaml
        with open(infile) as fil:
            data = yaml.load(fil)
    for key, value in data.items():
        if isinstance(value, dict):
            self.locales[key].update(value)
        else:
            self.locales[key].update({lang: value})

def loc(self, key, lang, values=None):
    '''Return the string key, translated to the requested language (if such
    a translation exists, otherwise no traslation occurs). Any other
    parameters passed to the method are injected to the placeholders in the
    string (if present).'''

    if self.locales[key].get(lang) is None:
        return key

    ret = self.locales[key][lang] if key in self.locales else key
    if values is None:
        return ret
    else:
        return ret % values

I get an error with Python 3.2 on travis.ci but the tests pass on Python 2.x and PyPy, if you have an idea let me know. I'm not sure, but I think as I read it was better to use format or template, rather than % with Python 3 is that I'm wrong.

import glob
import re
import os.path
from collections import defaultdict

class Pylocwolowitz(object):
    '''Pylocwolitz is a very simple text localization system, meant to be used
by web applications (but can pretty much be used anywhere). Yes, another
localization system.'''

def __init__(self, path, format_deserializer='json'):

Calling it format_deserializer suggests to me that it should be a function. Perhaps it should just be format?

    if format_deserializer not in ('json', 'yaml'):
        raise ValueError('FormatNotSupported')

    self.path = path
    self.format_deserializer = format_deserializer
    self.locales = defaultdict(dict)
    self._find_file()

def _find_file(self):
    '''Find all json files'''

You are finding files, find a file. Consider changing the name.

    listing = glob.glob(
        os.path.join(self.path, '*.' + self.format_deserializer))
    for infile in listing:

I'd do

 path = os.path.join(self.path, '*.' + self.format_deserializer)
 for infile in glob.glob(path):

Which I think comes out a bit cleaner

        self._make_loc(infile)

def _make_loc(self, infile):
    '''Store content of the file in a memory'''

Avoid abbreviations, I have no idea what loc is.

    lang = os.path.basename(infile).split('.')[0]

There is a splitext function which you might find useful here. You also don't ue this until way latter. Perhaps you should determine it later.

    if self.format_deserializer == 'json':
        import json
        with open(infile) as fil:
            data = json.load(fil)
    else:
        import yaml
        with open(infile) as fil:
            data = yaml.load(fil)

Instead, you could do

 deseralizer = __import__(self.format_deserializer)
 with open(infile) as fil:
    data = deserializer.load(fil)

That would avoid doing the same thing twice.

    for key, value in data.items():
        if isinstance(value, dict):
            self.locales[key].update(value)
        else:
            self.locales[key].update({lang: value})

def loc(self, key, lang, values=None):
    '''Return the string key, translated to the requested language (if such
    a translation exists, otherwise no traslation occurs). Any other
    parameters passed to the method are injected to the placeholders in the
    string (if present).'''

    if self.locales[key].get(lang) is None:
        return key

Do you actually want to skip formatting this case?

    ret = self.locales[key][lang] if key in self.locales else key

Why are you checking if key is in self.locales? It will be because the previous statement will have put it in regardless. And if it's not there, the first statement would have already returned. Also, its more pythonic to try and fetch it and catch the exception like:

   try:
       ret = self.locales[key][lang]
   except KeyError:
       ret =  key

That way you only lookup the dictionary once. But we can actually do even better by using get with a default

   ret = self.locales[key].get(lang, key)

That, I think, is a very slick approach.

    if values is None:
        return ret
    else:
        return ret % values

Using .format would probably be preferred


Store TotalCount in ViewState Is there a more concise way of doing this?

const string TOTAL_COUNT_KEY = "totalCount";
private int TotalCount
{
    get
    {
        int safeInt = 0;
        if (ViewState[TOTAL_COUNT_KEY] != null)
            safeInt = (int)ViewState[TOTAL_COUNT_KEY];
        return safeInt;
    }
    set { ViewState[TOTAL_COUNT_KEY] = value; }
}

You can use the ? operator :

const string TOTAL_COUNT_KEY = "totalCount";
private int TotalCount
{
    get
    {
        return ViewState[TOTAL_COUNT_KEY] != null ? (int)ViewState[TOTAL_COUNT_KEY] : 0;
    }
    set { ViewState[TOTAL_COUNT_KEY] = value; }
}

You can use the null propagation operator ?.

const string TOTAL_COUNT_KEY = "totalCount";
private int TotalCount
{
    get
    {
        return ((int?)ViewState[TOTAL_COUNT_KEY])?.Value ?? 0;
    }
    set { ViewState[TOTAL_COUNT_KEY] = value; }
}

It works this way:

We cast the ViewState[TOTAL_COUNT_KEY] to int?,
This way, it is either null or the value (let's say 42).
Then, the ?. operator will return null if the viewstate property is null, otherwise it'll return the value, 42.
If it is null, the ?? operator will return 0.

I think the ? operator is the clearest, the second one is a little more ninja and might not be appropriate if some member of your team (if you have one) don't fully understand the operators.


Reading data with different but similar semantics I am writing code that reads data from a DB, where boolean values are stored as 0/1 nullable integers.

I decided to write a simple function to encapsulate the logic which converts the integer values to boolean. Thing is, I sometimes need to regard the db null value as boolean false, and sometimes just as null.

So I've come up with the following two extension methods:

// Read the specified column value from row as nullable boolean,
// treating integer 0 as false, and any other non-null value as true.
public static bool? ReadAsBoolNullable(this DataRow row, string columnName)
{
    if (row.IsNull(columnName))
    {
        return null;
    }
    else
    {
        return row[columnName] != 0;
    }
}

// Read the specified column value from row as boolean,
// treating null and integer 0 as false, and any other value as true.
public static bool ReadAsBool(this DataRow row, string columnName)
{
    bool? data = row.ReadAsBoolNullable(columnName);
    return data.HasValue ? data.Value : false;
}

My main concern about this code is that I potentially have hundreds of millions of records, each having up to 10 boolean (not nullable) columns. So the second function will be executed a lot and will have to call the first one a lot. Could this damage performance significantly, given that the function itself doesn't do much?

As with all performance related questions, if and only if you can gather profiling evidence that shows that your current code is meaningfully detrimental to performance, don't call the first method in the second one, but rather repeat the code. This goes slightly against DRY (just barely, considering the very short code), but you will have to weigh the value of maintainability vs that of whatever performance gain is achieved.

In practice, I suspect that the effect on performance will be negligible if not non-existent. It's not as if you're not calling a slew of other methods and properties anyways.

The key point is profile profile profile.


Map in which values are concretions of a generic class Let's say I have a pool of adapter classes. All of them are concretions of a generic interface, such as:

interface Adapter<Target,Source> {
    Target adapt(Source object);
}

Now, my idea was to implement such a pool as a map in which keys are target and source classes:

final DoubleKeyMap<Class<?>, Class<?>, Adapter<Class<?>,Class<?>>> adapters;

And clients can ask the pool for the appropriate adapter in this manner:

(Adapter<Target, Source>) adapters.get(targetClass, sourceClass);

However, I'm not sure this is the 'best' way of accomplishing it.

Your clients do not need about how you are providing the Adapter<>. They just have to provider the Target and Source class instances.

Here is one possible implementation,

public class DoubleKeyMap {

    private Map<Wrapper, Adapter<Target,Source>> adapterMap;

    public DoubleKeyMap(Map<Wrapper, Adapter<Target,Source>> adapterMap) {
        this.adapterMap = adapterMap;
    }

    public Adapter<Target,Source> get(Class<Target> targetClass, Class<Source> sourceClass) {

        Wrapper wrapper = new Wrapper(targetClass, sourceClass);

        return adapterMap.get(wrapper);
    }

    private class Wrapper {

        private Class<Target> target;

        private Class<Source> source;

        private Wrapper(Class<Target> target, Class<Source> source) {
            this.target = target;
            this.source = source;
        }
    }
}

Hide the wrapper from outside world by making it private.

The client just provide Target and Source Class instances

class Client {
    public static void main(String[] args) throws ClassNotFoundException {
        DoubleKeyMap doubleKeyMap = new DoubleKeyMap(Collections.emptyMap());

        Adapter<Target, Source> targetSourceAdapter = doubleKeyMap.get(Target.class, Source.class);
    }
}


LINQ to Entities query of tests for my classes, with many subqueries This MVC4 controller method, which uses EF6, is taking a long time to run (over 2 seconds). I've added verbose comments for the purpose of this post only.

public async Task<ActionResult> Index()
{
  // Get a list of all the classes I teach.
  var myClasses = await CurrentUser.GetClassesAsync(db);

  // Get a list of all tests...
  var tests = await db.Tests
    // Which are either authored by me or involve one of my classes...
    .Where(o => o.AuthorId == CurrentUser.Id || (o.Author.SchoolId == CurrentUser.SchoolId && o.Classes.Any(c => myClasses.Contains(c.ClassName))))
    // And for each one I want...
    .Select(o => new TestOverviewItem
    {
      // The ID, title and test status
      Id = o.Id,
      Title = o.Title,
      Status = o.Status,
      // A list of which of my classes are registered for the test
      Classes = o.Classes.Where(c => myClasses.Contains(c.ClassName)).Select(c => c.ClassName).ToList(),
      // An indication of whether any results exist for students in my classes
      AnyUploads = o.Results.Any(r => r.User.Classes.Any(c => myClasses.Contains(c.ClassName))),
      // How many unmarked results there are for students in my classes
      RemainingToMark = o.Results.Count(r => r.MarkedBy == null && r.QuestionNumber > 0 && r.User.Classes.Any(c => myClasses.Contains(c.ClassName))),
      // An indication of whether I created the test
      IsOwner = o.AuthorId == CurrentUser.Id
    })
    .OrderByDescending(o => o.Id).ToListAsync();
  return View("Index", new OverviewModel(CurrentUser, tests, myClasses));
}

There are about 50 tests in total which match the criteria, and each one has ~4 Classes and ~2000 Results. I understand that all the subqueries must be slowing it down, but I'm not sure how to optimise.

I bet the database does not have the indexes to process this combination of conditions, especially with a contains on a name column. You will get table scans. Break off the || (o.Author.SchoolId == CurrentUser.SchoolId... part to return a new list of AuthorIds and you can add the CurrentUser.Id so the db.Tests where condition is just using the primary key.

The idea here is to preprocess your contains to be lists of ints to make the indexes happy.


Truncating article text I need to show article's content based on this logic:

if article's content has to be truncated (e.g for preview)

if there's [read more] substring in the content

replace first [read more] with link to the article
truncate content to the first occurence of [read more]

else

truncate content to the first 500 characters

remove all [read more] from content.

I've written a presenter for this, how can it be refactored?
(_h gives access to the helpers)
This is just one of the responsibilities that this presenter has, there are already a few methods (public & private), so... should I extract it into a helper?

class ArticlePresenter
  # ... omitted
  DEFAULT_LENGTH  = 500
  READ_MORE_REGEX = /\[read more\]/i

  def content(truncate)
    content = article.content
    if truncate
     index = content.index(READ_MORE_REGEX)
     if index
       url = _h.article_url(article)
       content = _h.truncate(content, length: index, omission: " [Show more](#{url})")
     else
       content = _h.truncate(content, length: DEFAULT_LENGTH)
     end
   end

   content.gsub!(READ_MORE_REGEX, '')

   html = MarkdownService.instance.render content
   content_tag :div, html
 end

About the naming, and grouping your code.

First improvement, change name of the variable from truncate to should_truncate, than you get

if should_truncate

instead of

if truncate

It is suggested to use methods to encapsulate code that is inside a block, for example code that is execute inside if statement, loops, or any other block of code. In your case

  def content(should_truncate)
    content = article.content
     if should_truncate
       content = truncate_article_content
     end
     content.gsub!(READ_MORE_REGEX, '')

     html = MarkdownService.instance.render content
     content_tag :div, html
  end

private

  def truncate_article_content
    index_at = article.content.index(READ_MORE_REGEX)
    if index_at
      truncate_length index_at
    else
      first_characters
    end
  end

  def truncate_length(length)
    url = _h.article_url(article)
    _h.truncate(article.content, length: length, omission: " [Show more](#{url})")
  end

  def first_characters(length= DEFAULT_LENGTH)
    _h.truncate(article.content, length: length)
  end


Generator expression in combination with StopIteration: kind of a hack? The first piece of code should cover the standard expected data. But as you can easily see, the data not always conveys to this standard. The code works great but I wonder if this is thought be a miss-use of the try/except clause. Better to go with if/else?

try:
    short_name = short_name.upper()
    m = (k for k,v in c.countries_dict.iteritems() if v[0] == short_name).next()
    m = [m]
except StopIteration:
    if re_type == 'MAP_INFO' or re_type == 'SHAPE' or re_type == 'ARC_INFO':
        reg_dict = REGIONS
        short_name = short_name.lower()
    elif re_type == 'NAV_SHAPE':
        reg_dict = NAV_REGIONS
        short_name = short_name.upper()
    m = []
    for k,v in reg_dict.iteritems():
        if k == short_name:
            for tup in v:
                m.append(tup[1])

I would discourage this particular usage. Not exactly because it's a risk or a hack—in other circumstances it might even be the right choice for good performance—but because I find it quite obfuscatory. (This happens to echo the usual guidance to avoid using exceptions for flow control, although it's harder to apply that guidance around Python's iterators.)

Instead I would suggest trying to make it clear what you are doing. I believe in this case you are trying to find the "first" key that maps to a list of values containing short_name in index 0. (I say "first" because it's in dictionary order, which isn't a particularly meaningful; it's probably more useful to say an arbitrary key.) If such a key does not exist, you wish to fall through to your except case. A reader is going to find that motive hard to discern. Here is an alternative way to write the first couple lines using itertools:

from itertools import islice

short_name = short_name.upper()
# find a key of an item containing short_name, if available
m = list(islice((k for k,v in c.countries_dict.iteritems() if v[0] == short_name), 1))
if not m:
    # due to bad data, not all short_name values are mapped correctly; try another way
    # code from original except StopIteration

For bonus readability you might split that up into a named generator function that finds all matches, and then filter to the first match with islice or another helper. Clearly performance isn't your goal here, as you're scanning a dict by value. (If you have a lot of these to look up, you may want to create the reverse dict instead of scanning multiple times.)

I'm far from certain that this code is what you intended. Since in the fallback case you build up a list of potentially multiple values, it seems surprising that in the primary case you explicitly exclude alternate keys. If you wanted to include all keys, you can skip islice and use a simple list comprehension: (Note that by slicing the list comprehension with [:1], it would yield the same value for m as the previous approach, and is more readable, although it potentially does more work.)

short_name = short_name.upper()
m = [k for k,v in c.countries_dict.iteritems() if v[0] == short_name]
if not m:
    # due to bad data, not all short_name values are mapped correctly; try another way
    # code from original except StopIteration


Monty Hall simulation with any number of doors After answering Monty hall python simulation I wondered what the outcome were to be if there were more than three doors, say four or seven. And so I decided to modify the problem slightly to adjust for this.

There are \$x\$ amount of doors. Behind one door is a car, the others are goats. You pick a door, and the host reveals a goat in a different door. You're then given the choice to change from the selected door to any door except the open door, or the already selected door.

And so I decided to find out how many times a person is likely to win if they always pick a random door when asked if they want to switch door. And so programmed the following:

import random

def monty_hall(amount, doors=3):
    if doors < 3:
        raise ValueError(f'doors must be greater than three, not {doors}')

    wins = 0
    for _ in range(amount):
        player_choice = random.randrange(doors)
        car_placement = random.randrange(doors)
        other_doors = set(range(doors)) - {player_choice, car_placement}

        shown_door = random.choice(list(other_doors))
        swap_doors = set(range(doors)) - {player_choice, shown_door}
        final_choice = random.choice(list(swap_doors))

        wins += final_choice == car_placement
    return wins

print(monty_hall(1000000, 3))
print(monty_hall(1000000, 4))
print(monty_hall(1000000, 5))

I then decided to optimize the above code, and came up with the following ways to do this:

Change player_choice to doors - 1. The first player choice doesn't have to be random.
Change other_doors and swap_doors to account for (1). Rather than removing an item that will always be in the set, remove it from the range and the second set. And so other_doors becomes: set(range(doors - 1)) - {car_placement}.
Remove player_choice as it's no longer used.
Change shown_door to always be the first door. However if the car is the first door, show the second. shown_door = car_placement == 0.
Remove other_doors as it's no longer used.
Change swap_doors to be a list, rather then a set with an item being removed. This requires the use of an if else to manually split the list if the car is in the first door. And so can become:

if car_placment == 0:
    swap_doors = [0] + list(range(2, doors - 1))
else:
    swap_doors = list(range(1, doors - 1))
final_choice = random.choice(swap_doors)

Rather than making my code WET, as I did in (6), we can instead move the car to the second door if it's in the first, and use the else whatever the case.
Finally we can merge all the code into a sum and a generator comprehension.

This resulted in the following code:

import random

def monty_hall(amount, doors=3):
    if doors < 3:
        raise ValueError(f'doors must be greater than three, not {doors}')

    rand = random.randrange
    return sum(
        max(rand(doors), 1) == rand(1, doors - 1)
        for _ in range(amount)
    )

print(monty_hall(1000000, 3))
print(monty_hall(1000000, 4))
print(monty_hall(1000000, 5))

Are there any improvements that I can make on either the optimized or non-optimized solutions? Also are there any issues with the optimized solution, as my method for optimization weren't exactly scientific. As far as I know it looks good.

Code is only runnable in Python 3.6+

I would have used an helper function in the first version so that your monty_hall function is:

def monty_hall(amount, doors=3):
    if doors < 3:
        raise ValueError(f'doors must be greater than three, not {doors}')

    return sum(player_wins_a_car(doors) for _ in range(amount))

This also applies to the second version but removes some of the speed optimizations you made there; so not sure if it’s worth keeping.

However, in the first version, I would use list-comprehensions to reduce a bit the memory needed. It would also exhibit the actions more like a real player would do. Since creating the set from range will already iterate over each elements, using the list-comp won't be much more costy:

import random

def player_wins_a_car(doors):
    player_choice = random.randrange(doors)
    car_placement = random.randrange(doors)
    shown_door = random.choice([door for door in range(doors)
                                if door not in (player_choice, car_placement)])
    final_choice = random.choice([door for door in range(doors)
                                  if door not in (player_choice, shown_door)])

    return final_choice == car_placement

For the second version, if you were to keep two functions that could lead to more explanatory stuff like:

def player_wins_a_car(doors):
    # Choose a door that hide a car. Car is never behind door 0,
    # if it were behind door 0, consider door 0 and 1 to be
    # swapped so that host always open door 0.
    car_placement = random.randrange(doors) or 1
    # Consider an initial choice of door `doors - 1` and host
    # showing door 0 unconditionaly. Always choosing a random
    # door out of these two.
    final_choice = random.randrange(1, doors-1)

    return car_placement == final_choice

I’m using or here as the only value triggering max would be 0, not sure which one performs better.


Kindergarten Garden challenge: ugly zip-group-flatten? I wrote up a solution for the "Kindergarten Garden" problem on exercism.io.

  My class will be handed a string that describes two rows of plants. Each student (either in a pre-set list or given as an argument) (sorted alphabetically) owns a 2x2 of plants. The method Garden.plants accepts a student name and returns a list of which plants they have, in the order:

12
34

My code just feels ugly and unreadable, though. I tried to refactor some of it out into a method, but it still didn't feel great to me, so I separated my ugly one-liner into a bunch of assignments to temporary variables, but it still feels like it could be better.

from functools import partial

def grouper(iterable, n):
    return zip(*[iter(iterable)] * n)
def flatten(seq):
    for item in seq:
        if hasattr(item, '__iter__') and not isinstance(item, str):
            yield from flatten(item)
        else:
            yield item

class Garden(object):
    plant_names = {"G":"Grass",
                   "C":"Clover",
                   "R":"Radishes",
                   "V":"Violets"}
    def __init__(self, rows, students=None):
        self.rows = rows.splitlines()
        if students is None:
            students = ["Alice", "Bob", "Charlie", "David", "Eve", "Fred",
                        "Ginny", "Harriet", "Ileana", "Joseph",
                        "Kincaid", "Larry"]
        self.students = sorted(students)
        self.plant_assignments = {student: list(plant_group) for student,plant_group
                in zip(self.students, self.plant_groups(self.rows))}
    def get_plant_name(self,plant_letter):
        try:
            return self.plant_names[plant_letter]
        except KeyError:
            raise ValueError("Plant letter {} is not defined".format(plant_letter))
    def plants(self,name):
        return self.plant_assignments[name]
    def plant_groups(self,rows):
        """Group each row in twos, then zip, flatten,
        and map in the expected way so:

        "VVGGCC\nVVGGCC"

        becomes:

        [('Violet','Violet','Violet','Violet'),
         ('Grass','Grass','Grass','Grass'),
         ('Clover','Clover','Clover','Clover')]"""
        grouper_by_two = partial(grouper, n=2)
        grouped_by_two = map(grouper_by_two, rows)
        cubewise = zip(*grouped_by_two)
        flat_cube = flatten(cubewise)
        flat_cube_with_names = map(self.get_plant_name, flat_cube)
        yield from grouper(flat_cube_with_names, 4)

As an example in and out:

# sample input:
>>> g = Garden("GCRR\nVVRR", ["Alice","Bob"])
# sample output:
>>> g.plants("Alice")
["Grass","Clover","Violets","Violets"]
>>> g.plants("Bob")
["Radishes","Radishes","Radishes","Radishes"]

It's not a bad solution, but it doesn't feel quite fluently expressed in Python.  In particular, I feel like the conversions between lists and generators are unnatural.  It would also help if you toned down the functional thinking in favour of list comprehensions.

I'll start by mentioning that blank lines between method definitions would make your code more readable, as suggested in PEP 8.  Also, constants should be named using ALL_CAPS.

Let's start with the constructor:

def __init__(self, rows, students=None):
    self.rows = rows.splitlines()
    if students is None:
        students = ["Alice", "Bob", "Charlie", "David", "Eve", "Fred",
                    "Ginny", "Harriet", "Ileana", "Joseph",
                    "Kincaid", "Larry"]
    self.students = sorted(students)
    self.plant_assignments = {student: list(plant_group) for student,plant_group
            in zip(self.students, self.plant_groups(self.rows))}

You aren't taking full advantage of the default parameter for students.
self.rows and self.students are never used outside of the constructor.  As they aren't useful as part of the state of the Garden object, they could just be local variables.
The dict(iterable) constructor would be a more succinct way to define self.plant_assignments than the set comprehension.

The "public" method, plants(), would be clearer if you renamed name to student.

The functions get_plant_name() and plant_groups() are for "internal" use.  Interspersing them among methods in the public interface makes your code less readable.

They are also "pure" functions that don't depend on any object state.  I suggest declaring them as static methods.

The heart of the solution is plant_groups():

def plant_groups(self,rows):
    """(Docstring omitted)"""
    grouper_by_two = partial(grouper, n=2)
    grouped_by_two = map(grouper_by_two, rows)
    cubewise = zip(*grouped_by_two)
    flat_cube = flatten(cubewise)
    flat_cube_with_names = map(self.get_plant_name, flat_cube)
    yield from grouper(flat_cube_with_names, 4)

In Python, map() is usually slightly better written as a list comprehension instead; partial() makes the case for a list comprehension even more compelling.

Instead of cubewise, I suggest the name plots, which forms a clear and simple mental image that is highly relevant to the problem being solved.

flatten() works recursively.  I suggest using a list comprehension (see implementation below) so that such recursion would no longer be necessary.  In fact, by flattening all the way to a linear list, you need to reorganize the individual pots into groups of 4.

Using yield from is an unnecessary complication.  It just requires the caller to convert the results back into a list using list(plant_group).

Putting everything together…

from itertools import chain

def grouper(iterable, n):
    return zip(*[iter(iterable)] * n)

def flatten(seq):
    yield from chain(*seq)

class Garden(object):
    PLANT_NAMES = {
         "G": "Grass",
         "C": "Clover",
         "R": "Radishes",
         "V": "Violets",
    }

    STUDENTS = [
        "Alice", "Bob", "Charlie", "David", "Eve", "Fred",
        "Ginny", "Harriet", "Ileana", "Joseph",
        "Kincaid", "Larry"
    ]

    @staticmethod
    def get_plant_name(plant_letter):
        try:
            return Garden.PLANT_NAMES[plant_letter]
        except KeyError:
            raise ValueError("Plant letter {} is not defined".format(plant_letter))

    @staticmethod
    def plant_groups(rows):
        grouped_by_two = [grouper(row, 2) for row in rows]
        plots = zip(*grouped_by_two)
        return [
            [Garden.get_plant_name(plant) for plant in flatten(plot)]
            for plot in plots
        ]

    def __init__(self, rows, students=STUDENTS):
        rows = rows.splitlines()
        students = sorted(students)
        self.plant_assignments = dict(zip(students, self.plant_groups(rows)))

    def plants(self, student):
        return self.plant_assignments[student]


Is there a better way to return values with merge sort? I have been attempting to learn Scala lately and so have produced a couple of different sorting algorithms to build up my basics.

I think I have been lucky with the methods returning exactly what I have wanted, but for MergeSort I have implemented recursive methods that could return one of two different values and could logically be placed into two different values of the same type (as I believe Scala does)

In order to get my program to sort properly I have put a temp variable in order to grab the returning value and then be able to use it.

Is there a better way to return the values that will not need me to create these temporary values or should I have been doing something else?

Example:

def merge(left: ArrayBuffer[Int], right: ArrayBuffer[Int]): ArrayBuffer[Int]

returns an ArrayBuffer, how does the scala language figure out which value to put the returned value into or does it need a variable on the returning end to capture the value that is being returned?

merge(left, right)

This does nothing while,

var tempResult = merge(left, right)

Solves the problem.

Am I doing something wrong in the first instance (due to my experience with single parameter methods returning to the proper variable), or is this the standard way to return the values, by returning to a variable?

MergeSortClass.scala

import scala.collection.mutable.ArrayBuffer
class MergeSortClass {

def mergeSort(array: ArrayBuffer[Int]): ArrayBuffer[Int] = {

if (array.size <= 1) {

  array

} else {

  var left = ArrayBuffer[Int]()
  var right = ArrayBuffer[Int]()
  var middle = array.size / 2

  for (i <- 0 until middle) {

    left += array(i)

  }

  for (i <- middle until array.size) {

    right += array(i)

  }

//-->>>TheseValues Below

  var leftTemp = mergeSort(left);
  var rightTemp = mergeSort(right);

  var result = merge(leftTemp, rightTemp)

  result

}

}

def merge(left: ArrayBuffer[Int], right: ArrayBuffer[Int]): ArrayBuffer[Int] = {

var result = ArrayBuffer[Int]()

while (left.size > 0 || right.size > 0) {

  if (left.size > 0 && right.size > 0) {

    if (left(0) <= right(0)) {

      result += left(0)
      left.remove(0)

    } else {

      result += right(0)
      right.remove(0)

    }
  } else if (left.size > 0) {

    result += left(0)
    left.remove(0)

  } else if (right.size > 0) {

    result += right(0)
    right.remove(0)

  }

}

result

 }

}

MergeSortObject.scala

import scala.collection.mutable.ArrayBuffer
object MergeSortObject {

  def main(args: Array[String]): Unit = {

val array = ArrayBuffer(10, 5, 15, 25, 7, 11, 17, 35, 34, 16)

for (i <- 0 to array.length - 1) {

  print(array(i))

  if (i == array.length - 1) {

  } else {

    print(", ")

  }

}

println()

var ms = new MergeSortClass()

//-->>>TheseValues Below

var temp = ms.mergeSort(array)
//ms.test(array) (not shown here)

for (i <- 0 to temp.length - 1) {

  print(temp(i))

  if (i == temp.length - 1) {

  } else {

    print(", ")

  }

}

}

}

The expression merge(x,y) will always return a value. val r = merge(x,y) stores the value so that you can use it again.

You could write your code

var leftTemp = mergeSort(left);
var rightTemp = mergeSort(right);

var result = merge(leftTemp, rightTemp)

result

as

merge(mergeSort(left),mergeSort(right))

It is a matter of preference if you wish to use leftTemp and rightTemp like this, but the use of result, and then returning it on the next line is not idiomatic Scala.

You must assign values to an identifier when you want to use them more than once (as you do with temp in your final example). But as I said above you may want to do this for other reasons such as readability and keeping down line lengths.


Optimizing nested sql query I have a code, which dynamically creates an SQL query based on the selected tables and columns. One of the sample query, which is generated is as following:
SELECT
    agr.Name,
    agr.Link,
    comp1.Name as 'Party 1',
    cnt1.Name as 'Party 1 Country',
    comp2.Name as 'Party 2',
    cnt2.Name as 'Party 2 Country'
FROM
    [dbo].[Agreements] agr
    LEFT JOIN [dbo].[Companies] comp1 on agr.Party1CompanyId = comp1.BusinessId
    AND agr.TenantId = comp1.TenantId
    LEFT JOIN [dbo].[Countries] cnt1 on comp1.CountryId = cnt1.BusinessId
    AND comp1.TenantId = cnt1.TenantId
    LEFT JOIN [dbo].[Companies] comp2 on agr.Party2CompanyId = comp2.BusinessId
    AND agr.TenantId = comp2.TenantId
    LEFT JOIN [dbo].[Countries] cnt2 on comp2.CountryId = cnt2.BusinessId
    AND comp2.TenantId = cnt2.TenantId
WHERE
    agr.ValidFrom <= DATETIMEFROMPARTS(2019, 1, 1, 00, 00, 00, 1)
    AND agr.ValidTo >= DATETIMEFROMPARTS (2019, 12, 31, 23, 59, 59, 0)
    AND agr.TenantId = 1
    AND comp1.ValidFrom <= DATETIMEFROMPARTS(2019, 1, 1, 00, 00, 00, 1)
    AND comp1.ValidTo >= DATETIMEFROMPARTS (2019, 12, 31, 23, 59, 59, 0)
    AND comp2.ValidFrom <= DATETIMEFROMPARTS(2019, 1, 1, 00, 00, 00, 1)
    AND comp2.ValidTo >= DATETIMEFROMPARTS (2019, 12, 31, 23, 59, 59, 0)
    AND cnt1.ValidFrom <= DATETIMEFROMPARTS(2019, 1, 1, 00, 00, 00, 1)
    AND cnt1.ValidTo >= DATETIMEFROMPARTS (2019, 12, 31, 23, 59, 59, 0)
    AND cnt2.ValidFrom <= DATETIMEFROMPARTS(2019, 1, 1, 00, 00, 00, 1)
    AND cnt2.ValidTo >= DATETIMEFROMPARTS (2019, 12, 31, 23, 59, 59, 0);

I wanted to ask the community, if there is a room for the optimisation of this query so that they do are not too long and still understandable and as per SQLs best practices. The concerns I have is more on the joins to the same table multiple time or same "WHERE" conditions for almost all tables.
The possible optimisation suggestions to the query above are sufficient for me to rewrite some bits, which eventually should be corrected. Thanks.

The following are a few suggestions on how I'd write the SQL.

define your date parameters as variables
create a CTE of table expressions to simplify parts of your query

Source Control
If you don't already have a database project, create one in Visual Studio. Then check it in to source control. Microsoft Azure DevOps Services is free & private for teams of 5 or less (this is per project, so 5 developers per project). Then you'll be able to track changes you make to your stored procedures, views, tables, etc.
Formatting
I use the following tools for SSMS and Visual Studio, ApexSQL Refactor and Poor Man's T-Sql Formatter. I use it when I have to edit other developer's code. It's a great way to standardize your SQL. I find it does most of the formatting for me, but I'll still make a few changes after.

Also check out the book Clean Code. It will change the way you think about naming conventions.

Revised SQL
Without table definitions and sample records I was unable to test this, but it should give you a good start.
DECLARE @ValidFrom  AS DATETIME = '01-Jan-2019';
DECLARE @ValidTo    AS DATETIME = '31-Dec-2019 23:59:59';

WITH
agreements_list
AS
(
   SELECT
        [agr].[Name]
      , [agr].[Link]
   FROM
      [dbo].[Agreements]
   WHERE
      1=1
      AND [TenantId] = 1
      AND [ValidFrom] >= @ValidFrom
      AND [ValidTo] <= @ValidTo
)
,
companies_list
AS
(
   SELECT
        [PartyName] = comp.[Name]
      , [CountryName] = cnt.[Name]
   FROM
      [dbo].[Companies] AS comp
      LEFT JOIN [dbo].[Countries] AS cnt ON comp.[CountryId] = cnt.[BusinessId] AND comp.[TenantId] = cnt.[TenantId]
   WHERE
      1=1
      AND comp.[ValidFrom] >= @ValidFrom
      AND comp.[ValidTo] <= @ValidTo
)
SELECT
     [agr].[Name]
   , [agr].[Link]
   , [Party 1] = [comp1].[PartyName]
   , [Party 1 Country] = [comp1].[CountryName]
   , [Party 2] = [comp2].[PartyName]
   , [Party 2 Country] = [comp2].[CountryName]
FROM
   agreements_list AS [agr]
   LEFT JOIN companies_list AS [comp1] ON [agr].[Party1CompanyId] = [comp1].[BusinessId] AND [agr].[TenantId] = [comp1].[TenantId]
   LEFT JOIN companies_list AS [comp2] ON [agr].[Party2CompanyId] = [comp2].[BusinessId] AND [agr].[TenantId] = [comp2].[TenantId]


Rewrite method to decide the ShowLevel based on roles I have different roles in my system.

HOD
Staff
Non staff

I am having a form at the front end and fields shall be visible to only users with specific roles.

The ShowInGui levels are like this:

Show to All
Show to staff/HOD
Show to HOD

Any other do not show

I am planning to write a method which will take the user role and decides the access.

Could this method be better written?

public boolean canBeShownInGUI(Role role, int showInGui) {
    if (showInGui == 1) {
        return true;
    } else if (showInGui == 2 && (role == Role.HOD || role == Role.STAFF)) {
        return true;
    } else if (showInGui == 3 && role == Role.HOD) {
        return true;
    } else
      return false;
   }

I presume Role is an enum (if it is not, it should be). An enum is a great candidate for a switch statement, and it also makes the conditions very clear:

switch (role) {
    case HOD:
        return showInGui >= 1;
    case STAFF:
        return showInGui == 1 || showInGui == 2;
}
return showInGui == 1;

Note that, if you want you can embed that logic in to the enum itself, and have a simple call to an enum method:

return role.isVisible(showInGui);

Additionally, if you reverse the values of the showInGui to be:

0 -> nobody
1 -> HOD only
2 -> HOD & STAFF
3 -> everyone

then your logic is simplified further to:

switch (role) {
    case HOD:
        return showInGui >= 1;
    case STAFF:
        return showInGui >= 2;
}
return showInGui >= 3;


First attempt at generating prime numbers I had to look up some other solutions online because I could not figure it out on my own. I really wish I could have come up with it completely on my own, but that didn't happen. Nevertheless, please review my prime number program and let me know if there are any improvements I should make or advice your would like to give me about it.

     var primes = [];                    // will become a list of prime numbers

primes_loop:
for (var n = 2; n < 10; n++) {

    if (n === 2) {
        primes.push(n);              // first prime number is stored
        continue primes_loop;        // continue iteration of the loop
    }

    divisors_loop:
    for (var i = 2; i < n; i++) {

        if (n % i === 0) {
            break divisors_loop;     // n is not prime if condition is true
        }
        else {
            primes.push(n);          // update prime list with the prime number
        }

    }

}

for (var index = 0; index < primes.length; index++) {
    console.log(primes[index]);
}

First of all, there's a slight problem with your code. I get this output:

2, 3, 5, 5, 5, 7, 7, 7, 7, 7, 9

So it repeats quite a bit, and -- wait, 9 is not a prime number. Oops.

For, say, numbers up to 25, you'd get 21 instances of 23 in your array. And we'd get a 24 too, although that's not prime. We'll deal with that in a second.

The way your code runs, you don't need the labels (primes_loop and divisors_loop); when you continue or break you do so for the "closest" loop you're in. So simply writing continue with no label will skip to the next iteration of the outer (prime) loop, and break will break the inner (divisor) loop.

However, a label might be handy for a revised version that actually does find primes and only primes (of course, there are other, smarter approaches to all of this, but I'm just sticking to the code that was posted):

var primes = [],
    limit = 10,
    n, divisor;

outerLoop: for( n = 2 ; n <= limit ; n++ ) {
  for( divisor = 2 ; divisor < n ; divisor++ ) {
    if( n % divisor === 0 ) {
      continue outerLoop; // not a prime, try the next n
    }
  }
  primes.push(n); // if we made it all the way here, then n is prime
}

After that, you'll be left with a primes array that looks like this:

[ 2, 3, 5, 7 ]

No repetition, and no false positives.

Of course, it'd be nicer to wrap as a function:

function findPrimes(limit) {
  var primes = [], n, divisor;

  outerLoop: for( n = 2 ; n <= limit ; n++ ) {
    for( divisor = 2 ; divisor < n ; divisor++ ) {
      if( n % divisor === 0 ) {
        continue outerLoop;
      }
    }
    primes.push(n);
  }

  return primes;
}


Min Heap Java Implementation I have implemented a min heap and looking for a simpler solution.

Is there anyone for suggesting a simpler solution for this?

Thanks.

import com.sun.org.apache.xalan.internal.xsltc.dom.MultiValuedNodeHeapIterator;

import java.util.Arrays;

public class MinHeap {

    private int capacity = 10;
    private int size = 0;

    int[] items = new int[capacity];

    private int getLeftChildIndex(int parentIndex) { return 2 * parentIndex + 1 ;}
    private int getRightChildIndex(int parentIndex) { return 2 * parentIndex + 2 ;}
    private int getParentIndex(int childIndex) { return (childIndex -1) / 2 ;}

    private boolean hasLeftChild(int index) { return getLeftChildIndex(index) < size; }
    private boolean hasRightChild(int index) { return getRightChildIndex(index) < size; }
    private boolean hasParent(int index) { return getParentIndex(index) >= 0; }

    private int  leftChild(int index) { return items[getLeftChildIndex(index)]; }
    private int  rightChild(int index) { return items[getRightChildIndex(index)]; }
    private int  parent(int index) { return items[getParentIndex(index)]; }

    private void swap(int indexOne, int indexTwo){
        int temp = items[indexOne];
        items[indexOne] = items[indexTwo];
        items[indexTwo] = temp;
    }

    private void ensureExtraCapacity() {
        if (size == capacity) {
            items = Arrays.copyOf(items, capacity * 2);
            capacity *= 2;
        }
    }

    private int peek() {
        if (size == 0) throw new IllegalStateException();
        return items[0];
    }

    private int poll() {
        if (size == 0) throw new IllegalStateException();
        int item = items[0];
        items[0] = items[size - 1];
        size--;
        heapifyDown();
        return item;
    }

    public void add(int item) {
        ensureExtraCapacity();
        items[size] = item;
        size++;
        heapifyUp();
    }

    public void heapifyUp() {
        int index = size - 1;
        while ( hasParent(index) && parent(index) > items[index] ) {
            swap (getParentIndex(index),index);
            index = getParentIndex(index);
        }
    }

    public void heapifyDown() {
        int index = 0;
        while ( hasLeftChild(index) ) {
            int smallerChildIndex = getLeftChildIndex(index);
            if (hasRightChild(index) && rightChild(index) < leftChild(index)) {
                smallerChildIndex = getRightChildIndex(index);
            }

            if (items[index] < items[smallerChildIndex]) {
                break;
            } else {
                swap(index, smallerChildIndex);
            }
            index = smallerChildIndex;
        }
    }

}

I have one suggestion for your code.

Create a method to check if the current size is invalid, and reuse it (MinHeap#peek and MinHeap#poll methods).

private void assertCurrentSizeIsValid() {
   if (size == 0) {
      throw new IllegalStateException();
   }
}