Calculating energy from mass and light speed I'm learning Java JDK 1.8 since one day and I'm using Java's methods to the first time. I have never used Java before because I'm a Python/Django developer.

I created a method which let to get energy in function of two parameters: mass and light speed. I would like to know if my method is well-written or if I made some mistakes about method, call, etc.

import java.util.Scanner;

public class methodes {

    public static double Energie(double m, double c)
      {
       return (double) (m*Math.pow(c, 2));
      }

    public static void main(String[] args) {

        Scanner sc = new Scanner(System.in);

        int C = 300000 ;
        double m ;

        System.out.println("Valeur de la masse en kg : ");
        m = sc.nextDouble();

        double E = Energie(m,C);
        System.out.println("Pour " + m + " kg donné, l'énergie équivaut à " + E + " Joules");

    }

}

The implementation looks fine. Only the conventions on capital letters and spacing might be a bit different in the java community.

A class name always starts with a capital letter

public class Methodes

General utility classes are usually called Util but Methodes is fine depending on your own project preference. Especially when you're not native English it might change depending on who you work with, and how well you know the language.

Method names start with a lower case. And the opening { is placed at the same line as the method name

public static double energie(double m, double c) {
    return (double) (m*Math.pow(c, 2));
}

If you really want to place { at the line after the method name that's fine as long as you're consistent. So either do it always, or do it never. (big discussion in general about which is better, the community overal prefers on the same line).

Variables start with a lower case letter and are camel case, constants are written full uppercase and separated with _. Preferably use meaningful names, although m, C and e in this context are already pretty decent.

public class ExampleClass {
    //this is a constant. Since java 7 (?) you can also use _ in numbers to read easier.
    public static final int SPEED_OF_LIGHT = 300_000;

    public static int calculateEnergy(double mass) {
        return (double) (mass*Math.pow(SPEED_OF_LIGHT , 2));
    }

    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);

        System.out.println("Valeur de la masse en kg : ");
        double mass = sc.nextDouble();

        double energy = calculateEnergy(mass);
        System.out.println("Pour " + mass + " kg donné, l'énergie équivaut à "
                          + energy + " Joules");
    }
}


Comparing database values with values from a GridView I have a block of C# code here that stores data to an ArrayList from the database and compares each with the values from the GridView.

Is there a way to reduce the line of code and improve its readability?

 while (DataReader.Read())
{
    arrHostName.Add(DataReader.GetString(0));
    arrUsers.Add(DataReader.GetString(2));
    arrPSName.Add(DataReader.GetString(3));
}
foreach (Infragistics.Win.UltraWinGrid.UltraGridRow row in Grid2.Rows)
{
    string rowHostName = row.Cells["HostName"].Value.ToString();
    string rowUsers = row.Cells["Users"].Value.ToString();
    string rowPSName= row.Cells["PS_NAME"].Value.ToString();

    foreach (string a in arrHostName)
    {
        if (rowHostName == a.ToString())
        {
            row.Appearance.BackColor = Color.Yellow;
            row.Appearance.FontData.Bold = Infragistics.Win.DefaultableBoolean.True;
        }
    }
    foreach (string a in arrUsers)
    {
        if (rowUsers == a.ToString())
        {
            row.Appearance.BackColor = Color.Yellow;
            row.Appearance.FontData.Bold = Infragistics.Win.DefaultableBoolean.True;
        }
    }
    foreach (string a in arrPSName)
    {
        if (rowPSName == a.ToString())
        {
            row.Appearance.BackColor = Color.Yellow;
            row.Appearance.FontData.Bold = Infragistics.Win.DefaultableBoolean.True;
        }
    }
}

It looks like what you want is ArrayList.Contains.

string rowHostName = row.Cells["HostName"].Value.ToString();
string rowUsers = row.Cells["Users"].Value.ToString();
string rowPSName= row.Cells["PS_NAME"].Value.ToString();

if (arrHostName.Contains(rowHostName)
    || arrUsers.Contains(rowUsers)
    || arrPSName.Contains(rowPSName))
{
    row.Appearance.BackColor = Color.Yellow;
    row.Appearance.FontData.Bold = Infragistics.Win.DefaultableBoolean.True;
}

And if you only use the collections above to check if they contain some values, you should prefer HashSet over ArrayList.

Of course, you should then change the names of the local variables accordingly, from arrHostName to hostNames and so on. It is always better not to clutter variable names with type information in C#, whose type system is good enough.


Java/Kotlin JSON parsing improvement I consume an API which gives me this type of JSON:

{
  "data": {
    "name": "Start",
    "pid": "1",
    "position": {
      "data": {
        "x": "31",
        "y": "330"
      },
      "metadata": "empty"
    }
  },
  "metadata": "empty"
}

I have created the classes with objects with the same structure as the above JSON. I use the retrofit lib in Android which inside uses GSON for parsing the JSON.

My model classes would be like this:

MResponse.class

public class MResponse {

    @SerializedName("data")
    public User user;

    String metadata;
}

User.class

public class User {

    public String name;

    public String pid;

    @SerializedName("position")
    public PositionData positionData;
}

PositionData.class

public class PositionData {

    @SerializedName("data")
    public Position position;

    public String metadata;
}

Position.class

public class Position {

    public String x;

    public String y;
}

Now this works fine for me. But as you can see for every model I have to create a parent which will have the same structure just changes the child. This fact doubles the classes that I use for my models. I would like to ask if there is a better way to avoid all these classes.

I don't want to use inner classes. I was thinking that the guys that have done the JSON like this must have had a reason why they did it like this and also a way to make the parsing more easier.

Usually I was used to parse this kind of JSON structure:

{
  "data": {
    "name": "Start",
    "pid": "1",
    "position": {
      "x": "31",
      "y": "330"
    }
  }
}

And here it's easier if I would follow the solution above.

EDIT

Also any solution in Kotlin is welcomed

You can create a generic container/entity for "data" and "metadata" and reuse it. e.g.:

data class MEntity<T>(
        var data: T? = null,
        var metadata: String? = null
)

data class User(
        var name: String? = null,
        var pid: String? = null,
        var position: MEntity<Position>? = null
)

data class Position(
        var x: String? = null,
        var y: String? = null
)

Due to type erasure you must use a TypeToken to deserialize the JSON into a generic type:

gson.fromJson<MEntity<User>>(json, object : TypeToken<MEntity<User>>() {}.type)

This isn't very convenient but thankfully Kotlin allows us to use reified type parameters to define an extension function to simplify this:

inline fun <reified T> Gson.fromJsonToGeneric(json: String): T {
    return fromJson(json, object : TypeToken<T>() {}.type)
}

Now the usage becomes much simpler:

val userMEntity = gson.fromJsonToGeneric<MEntity<User>>(json)
println(userMEntity)

Output:

MEntity(data=User(name=Start, pid=1, position=MEntity(data=Position(x=31, y=330), metadata=empty)), metadata=empty)


Find the word in a string that has the most repeated characters The problem: I want to find the word in a string that has the most repeats of a single letter, each letter is independent. Does not need to be consecutive.

Currently I am jumping each character and checking, which doesn't seem very efficient. Do you have any ideas that might improve the number of processes that are required to find which word has the most repeats?

function LetterCountI(str) {

  var repeatCountList = [];
  var wordList = str.split(/\W/); //regular expression \W for non word characters split at.
  var wordCount = wordList.length; // count the number of words
  for (var i=0; i< wordCount ; i++)
  { var mostRepeat = 1;                 // set the default number of repeats to 1
    var curWord = wordList[i];             // set the current word to the ith word from the list

   for(var ii=0; ii<curWord.length ; ii++)
    { var repeatCount = 1;                   // set default repeat count to 1
      var curChar = curWord[ii];               //set the current character to the iith
     for (var iii=0; iii<curWord.length; iii++)  //judge if it is the same as the iiith
     {var against = curWord[iii];
      if (iii!=ii)                               // if it is Not the same referenced postion
      {if(curChar==against)                      // see if the string values match
      {repeatCount=repeatCount+1}}}               // increase counter if match against
       if (repeatCount>=mostRepeat)              // record repeat for the highest only
       {mostRepeat=repeatCount}

    }
   repeatCountList = repeatCountList.concat(mostRepeat)    // take the highest from each word
     }

  mostRepeat = 0;                     // set the repeats value to -
  for (j=0;j<wordCount; j++)            // go through the repeats count list
  { if(repeatCountList[j]>mostRepeat)       // if it has more repeats than the highest So FAR
       { mostRepeat = repeatCountList[j];        // record if higher than last
        var x = j;}}                      // record the index of the most repeat that is the new high
  var ans = [];
  if (mostRepeat == 1)              // check if there are no repeats at all.
  {ans=-1}                // question want to return -1 if there are no repeats
  else
  {ans=wordList[x]}          // display the word from the list with the most repeat characters

  // code goes here
  return ans;

}

Any help is appreciated.

I liked that you split the string into words using a regular expression.  That helps a lot.

Your code formatting (indentation and braces) is haphazard.  It shouldn't be that hard to follow the standard conventions for code formatting, and it will make things easier for yourself if you do.

I think your function tries to do too much.  It would help to break down the problem.  I've extracted part of the problem into a self-contained task:

  Given a word, how many times does the most frequent character appear?

For that, you can write a function, and test it (e.g. mostFrequentCount('hello') should return 2).

/**
 * Given an array (or a string), returns the number of times the most frequent
 * element (or character) appears.
 */
function mostFrequentCount(elements) {
    var bins = {};
    for (var i = 0; i < elements.length; i++) {
        bins[elements[i]] = (bins[elements[i]] || 0) + 1;
    }
    var max = 0;
    for (var c in bins) {
        max = Math.max(max, bins[c]);
    }
    return max;
}

That should simplify the main code.  Rather than commenting each line (in effect writing everything once for the computer and once for other programmers), I've tried to make the code read like English by using very human-friendly variable names.

function wordsWithMaxRepeatedCharacters(string) {
    var maxRepeatedCharacters = 0, wordsWithMaxRepeatedCharacters = [];

    var words = string.split(/\W/);
    for (var w = 0; w < words.length; w++) {
        var word = words[w];
        var numRepeatedCharacters = mostFrequentCount(word);

        if (maxRepeatedCharacters < numRepeatedCharacters) {
            maxRepeatedCharacters = numRepeatedCharacters;
            wordsWithMaxRepeatedCharacters = [word];
        } else if (maxRepeatedCharacters == numRepeatedCharacters) {
            wordsWithMaxRepeatedCharacters.push(word);
        }
    }
    return wordsWithMaxRepeatedCharacters;
}


Convert geodetic coordinates to geocentric (cartesian) I'd like to retain as much float precision as possible without having to resort to specialized libraries so I need this code to work with only standard modules.

I'm doing this in QGIS where specialized libraries aren't available by default. numpy would be OK to use.

Sources for the computations can be found on Wikipedia.

Here is my code:

import math

## Ellipsoid Parameters as tuples (semi major axis, inverse flattening)
grs80 = (6378137, 298.257222100882711)
wgs84 = (6378137, 298.257223563)

def geodetic_to_geocentric(ellps, lat, lon, h):
    """
    Compute the Geocentric (Cartesian) Coordinates X, Y, Z
    given the Geodetic Coordinates lat, lon + Ellipsoid Height h
    """
    a, rf = ellps
    lat_rad = math.radians(lat)
    lon_rad = math.radians(lon)
    N = a / math.sqrt(1 - (1 - (1 - 1 / rf) ** 2) * (math.sin(lat_rad)) ** 2)
    X = (N + h) * math.cos(lat_rad) * math.cos(lon_rad)
    Y = (N + h) * math.cos(lat_rad) * math.sin(lon_rad)
    Z = ((1 - 1 / rf) ** 2 * N + h) * math.sin(lat_rad)

    return X, Y, Z

Input:

lat = 43.21009
lon = -78.120123
h = 124
print(geodetic_to_geocentric(wgs84, lat, lon, h))

Output:

(958506.0102730404, -4556367.372558064, 4344627.16166323)

This online converter gives the following output:

X: 958506.01027304
Y: -4556367.37255806
Z: 4344627.16160147

So, quite close for X and Y but slightly different for Z. Is this a rounding error ? If so, on which side ? Is there a better strategy to do the computations that may retain better precision ?

I can't explain the discrepancy with the online conversion tool without seeing the source code for the latter.

1. Review

The comment for the ellipsoid parameters should give the units for the semi-major axis. Also, I think "reciprocal flattening" would be a better description of the second parameter, given that you use the name rf later on in the code.
The docstring for geodetic_to_geocentric should give the units (degrees) for the latitude and longitude parameters.
"Height above ellipsoid" would be a clearer description for the h parameter.
The parameters to geodetic_to_geocentric could be spelled out in full, which would help understand their meaning. Parameter names are documentation too.
Comments would help understand the meaning of values like N.
The expressions (N + h) * cos(lat), sin(lat), and (1 - 1 / rf) ** 2 are evaluated twice. These could be stored in local variables to avoid duplicated work.

2. Revised code

from math import cos, radians, sin, sqrt

# Ellipsoid parameters: semi major axis in metres, reciprocal flattening.
GRS80 = 6378137, 298.257222100882711
WGS84 = 6378137, 298.257223563

def geodetic_to_geocentric(ellipsoid, latitude, longitude, height):
    """Return geocentric (Cartesian) Coordinates x, y, z corresponding to
    the geodetic coordinates given by latitude and longitude (in
    degrees) and height above ellipsoid. The ellipsoid must be
    specified by a pair (semi-major axis, reciprocal flattening).

    """
    φ = radians(latitude)
    λ = radians(longitude)
    sin_φ = sin(φ)
    a, rf = ellipsoid           # semi-major axis, reciprocal flattening
    e2 = 1 - (1 - 1 / rf) ** 2  # eccentricity squared
    n = a / sqrt(1 - e2 * sin_φ ** 2) # prime vertical radius
    r = (n + height) * cos(φ)   # perpendicular distance from z axis
    x = r * cos(λ)
    y = r * sin(λ)
    z = (n * (1 - e2) + height) * sin_φ
    return x, y, z


Treap with implicit keys I implemented an implicit treap and tested it a little. It works fine but I'd like to know if there are ways to improve/simplify things and/or if some parts are incorrect. Later, I'll augment code to allow for more operations (interval reversal, shifts, etc..).

Treap node.

struct Node
{   int v, p, c; /* value, priority, subtree size */
    Node *l, *r; /* left and right subtrees */

    Node(int val, int priority = rand())
    {   v = val;
        p = priority;
        l = r = 0;
    }
};

Size of subtree rooted at node x.

int count(Node *x)
{   return x ? x->c : 0;
}

Merge.

void join(Node **x, Node *left, Node *right)
{   if(left == 0 || right == 0)
        *x = left ? left : right;
    else
    {   if(left->p > right->p)
        {   join(&left->r, left->r, right);
            *x = left;
        }
        else
        {   join(&right->l, left, right->l);
            *x = right;
        }
        (*x)->c = count((*x)->l) + count((*x)->r) + 1;
    }
}

Split.

void split(Node *x, int k, Node ** left, Node **right)
{   Node *tmp;
    if(x == 0) *left = *right = 0;
    else
    {   int c = count(x) - count(x->r);
        if(k < c)
        {   split(x->l, k, left, &tmp);
            *right = x, (*right)->l = tmp;
        }
        else
        {   split(x->r, k - c, &tmp, right);
            *left = x, (*left)->r = tmp;
        }
        x->c = count(x->l) + count(x->r) + 1;
    }
}

Insert element.

void insert(Node **t, int k, int v)
{   Node *n = new Node(v);
    Node *left, *right;
    split(*t, k, &left, &right);
    join(t, left, n);
    join(t, *t, right);
}

Remove element.

void remove(Node **t, int k)
{   Node *left, *mid, *tmp, *right;
    split(*t, k, &left, &mid);
    split(mid, k + 1, &tmp, &right);
    join(t, left, right);
}

You should be giving your variables proper names. This would really improve the readability of your code. After all, code is read more times than it is written. As an outsider not all too well versed in advanced data structures, it's really hard to tell what k or v is supposed to be. Your code will effectively be self documenting if you change c and all other one character identifiers to something more reasonable like subtree_size or at least subsize. You won't need comments like /* left and right subtrees */.


Indian numbering system formatting The following code converts a number into Indian numbering system format.

I know locale can also help in getting similar format but I wanted something that can work on any system because not all locales are present in all OSs.

The code was written for Python 3.5 hence no variable annotations.

from decimal import Decimal
from itertools import islice, zip_longest
from typing import Any, Iterable, Iterator, Sequence, Tuple, TypeVar, Union  # noqa: ignore=F401

_T = TypeVar('_T')

def grouper(iterable: Iterable[_T], n: int, fillvalue: Any = None) -> Iterator[Tuple[_T, ...]]:
    "Collect data into fixed-length chunks or blocks"
    # grouper('ABCDEFG', 3, 'x') --> ABC DEF Gxx
    args = [iter(iterable)] * n
    return zip_longest(fillvalue=fillvalue, *args)

def indian_numbering_format(
    amount: Union[Decimal, int, str], places: int = 2,
    curr: str = '', pos: str = '', neg: str = '-'
) -> str:
    """
    Formats a number and returns it in Indian numbering format
    https://en.wikipedia.org/wiki/Indian_numbering_system

    places:  optional number of places after the decimal point
    curr:    optional currency symbol before the sign
    pos:     optional sign for positive numbers: '+' or blank
    neg:     optional sign for negative numbers: '-' or blank

    Note that if the fractional part only contains 0 it will be
    dropped.
    """
    q = Decimal(10) ** -places  # type: Decimal
    amount = Decimal(amount).quantize(q)
    sign, digits, exponent = amount.as_tuple()
    integer_part, fractional_part = (), ()  # type: Sequence[int], Sequence[int]
    exponent = exponent or len(digits)
    integer_part = digits[:exponent]
    # If fractional part only contains 0s then ignore it.
    if set(digits[exponent:]) != {0}:
        fractional_part = digits[exponent:]
    integer_part_digits = reversed(''.join(map(str, integer_part)))
    thousand = ''.join(islice(integer_part_digits, 3))[::-1]
    hundreds = ','.join(map(''.join, grouper(integer_part_digits, 2, fillvalue='')))[::-1]
    sep = ',' if hundreds else ''
    fractional_part_digits = '.' + ''.join([str(d) for d in fractional_part]) if fractional_part else ''
    return '{sign}{curr}{hundreds}{sep}{thousand}{fractional_part}'.format(
        sign=neg if sign else pos,
        curr=curr,
        thousand=thousand,
        sep=sep,
        hundreds=hundreds,
        fractional_part=fractional_part_digits
    )

I was wondering if the code can be refactored in any way to make it much more simpler.

I don't think I'd get so fancy with the grouper; the data is small, a constant table will do:

comma_after_digit = set(3, 5, 7, 9, 11, 13, 15, 17)

result = '.' + ''.join(map(str, fractional_part))
for i, d in enumerate(integer_part_digits):
    if i in comma_after_digit:
        result = ',' + result
    result = d + result
result = {0:pos, 1:neg}[sign] + result
return result

Also:

Try and be a bit more consistent: you use reversed() to reverse a string on one line, and then the next you use [::-1].  You also use map(str, digits) in one place and [str(x) for x in digits] another! They do the same thing, but it's nice to the reader to pick one idiom and stick with it.
thousand = ''.join(islice(integer_part_digits, 3))[::-1] can be simplified into thousand = integer_part_digits[-4::-1]
Using .format() is overkill for concatenating strings.  + works just fine.  If you want to use it, you should 1) factor out constant strings (like the decimal point) and 2) use the args as your temporary variables, like:

return '{sign}{curr}{hundreds}{sep}{thousand}.{fractional_part}'.format(
    sign=neg if sign else pos,
    curr=curr,
    thousand=integer_part_digits[-4::-1],
    sep=',' if exponent > 3 else '',
    hundreds=','.join(map(''.join, grouper(integer_part_digits, 2, fillvalue='')))[::-1],
    fractional_part=''.join(map(str,fractional_part)) if fractional_part else ''
)

According to the wikipedia page you cite, there's no comma after the 1 in 10**19, so your code is wrong :) ... which leads inevitably to:
Needs tests!


Virtual Slots Implementation What I can do to make this code more efficient?

It is supposed to be like one of those slot machines where you gamble on it and stuff (like at a casino).

#include <iostream>
#include <conio.h>
#include <string>
#include <ctime>
#include <iomanip>

using namespace std;

int main() {

    srand(time(0));

    int slot1;
    int slot2;
    int slot3;
    double total_money = 0;
    double total_wins = 0;
    double total_tries = 0;
    double win_percentage;
    char play_again;

    cout << "Welcomes to Virtual Slots." << endl;  // Welcome the user
    cout << "Match 2, win that amount." << endl;  // Explain prizes
    cout << "Match 3, win 3x that amount.\n" << endl;

    do {

        slot1 = rand() % 10 + 1;     // Get random numbers for slots
        slot2 = rand() % 10 + 1;
        slot3 = rand() % 10 + 1;

        cout << "****SLOTS!!****" << endl;
        cout << "***************" << endl;
        cout << "* [" << slot1 << "] [" << slot2 << "] [" << slot3 << "] *" << endl;  // Show slots
        cout << "***************\n" << endl;

        if (slot1 == slot2 == slot3) {  // If all three slots match, user getx 3x money

            cout << "THREE TIMES MATCH!!" << endl;
            cout << "You win $" << (slot1 * 3) << ".\n" << endl;

            total_money += slot1;  // Count money
            total_wins++;  // Count wins
        }

        else if (slot1 == slot2 || slot1 == slot3 || slot2 == slot3) {  // if user gets 2 slots matched, user gets that amount of money

            cout << "TWO TIMES MATCH!!" << endl;
            cout << "You win $" << slot1 << ".\n" << endl;

            total_money += slot1;  // Count money
            total_wins++;  // Count wins
        }

        else {  // No match, no money

            cout << "NO MATCH.\n" << endl;
        }

        total_tries++;  // Count tries

        win_percentage = (total_wins / total_tries);  // Percentage of wins

        cout << "Current Total: $" << setprecision(4) << total_money << endl;                // Show user his statistics
        cout << "Win Percentage: " << setprecision(3) << win_percentage << "%" << endl;

        cout << "Play Again? (Y/N) " << endl;  // Ask the user if s/he wants to play again
        cin >> play_again;

        system("PAUSE");
        system("CLS");

    } while (play_again != 'N');  // Keep going until user says No

    cout << "See you next time!" << endl;

    _getch();
    return 0;
}

The usual improvements we often suggest here on Code Review:
Using namespace:
Try not to using namespace std. Read a discussion about this here. Instead, just explicitly declaring the elements you are using would be better:
using std::cout;
using std::cin;
... etc...

Or don't do any of that and prefix library calls with std::. Not that much more typing anyway.
system("PAUSE") & friends:
The system() function asks the OS to run whatever program you specify as its argument. It just so happens that on Windows there are two programs in the default path called "PAUSE" and "CLS". This is not portable. On Unix those programs do not exist. But the biggest evil of system() is that it is a major security hole. For example, if a hacker manages to replace "CLS" with an exploit program, your program is going to be the one who fires it. So no serious code outside a learning demo should use those. Be aware of this issue.
Windows extensions:
_getch() and <conio.h> are Windows extensions. You can replace _getch() with the standard std::cin.get().
C++11 <random> library:
The new standard has introduced a more powerful pseudo-random number generation library: <random>. This library should be used instead of rand(), srand() for new code.
Funky if:
This
if (slot1 == slot2 == slot3)

As was noted in the comments, is nonsense. You have to use the && (AND) operator:
if ((slot1 == slot2) && (slot2 == slot3)) {  // If all three slots match, user getx 3x money


Binary tree implemented using references I implemented a binary tree using references because I couldn't see why people don't use references instead of pointers to implement data structures such as binary trees:

#include <iostream>

struct Node {
  Node(Node& l, Node& r, const char* v) : left_child(l), right_child(r), value(v) {}
  const char* value;
  Node& left_child;
  Node& right_child;
};

Node null_node(null_node, null_node, "END_OF_TREE");

bool is_leaf(Node& node){
  return &node.left_child == &null_node && &node.right_child == &null_node;
}

void post_order(Node& node){
  if (is_leaf(node)){
    std::cout << node.value;
  }
  else{
    if (&node.left_child != &null_node){
       post_order(node.left_child);
    }
    if (&node.right_child != &null_node){
       post_order(node.right_child);
    }
    std::cout << node.value;
  }
}

int main(){
  Node n1(null_node, null_node, "1");
  Node n2(null_node, null_node, "2");
  Node n3(n1, n2, "3");
  Node n4(null_node, null_node, "4");
  Node n5(n3, n4, "5");
  post_order(n5);
}

I still can't see why references are inferior to pointers. What's wrong with using references for binary tree (other than that you can't insert or delete elements)?

Also I'm completely new to C++ so any feedback on the code itself is appreciated as well. Thank you.

What's wrong with using references for binary tree (other than that you can't insert or delete elements)?

Well, it does work, as you've shown. But in order to use references instead of pointers, you had to make two awkward changes to the "natural" pointer-based code:

(1) You had to invent a null_node object, which is a global variable, and suffers all the problems of global variables (for example, it won't play well with dynamic libraries, a.k.a. shared objects, a.k.a. DLLs, because you might end up with a program containing two null_node variables — one from the main program and one from the DLL).

(2) You had to prefix all your accesses with an extra & character.

Compare your way (slightly reformatted for style and const-correctness):

void post_order(const Node& node) {
    if (&node.left_child != &null_node) {
        post_order(node.left_child);
    }
    if (&node.right_child != &null_node) {
        post_order(node.right_child);
    }
    std::cout << node.value;
}

to the "natural" approach:

void post_order(const Node *node) {
    if (node->left_child) {
        post_order(node->left_child);
    }
    if (node->right_child) {
        post_order(node->right_child);
    }
    std::cout << node->value;
}

Which one is easier to read (and incidentally to write)?

Notice that C++'s syntax for pointers dates back all the way to C, i.e., the mid-1970s. It's had a long time to polish and refine and streamline that syntax. You're trying to invent something "better", without the benefit of controlling the language definition. You shouldn't be surprised that the built-in, ~40-year-old syntax is really suitable for the exact job it was designed for. :)

By the way, if you want to deal with empty trees in the "natural" way, consider the following refactoring:

void post_order(const Node *node) {
    if (node != nullptr) {
        post_order(node->left_child);
        post_order(node->right_child);
        std::cout << node->value;
    }
}

Straight-line code is best code!


Compile time positive and negative powers of two I'm getting started on template meta-programming and I wrote a template to compute compile-time positive and negative float powers of 2. I would like some feedback on readability and overall code complexity as I feel like I might be defining too many structures for such a simple computation. The code has to be C++98 compatible.

// Sign of an integer
template<bool b> struct SIGN {static const int value = ( b * 2 ) - 1; };

// Positive powers of two (fast exponentiation)
template<unsigned int N> struct POW2_POS {
    static const float value =
      POW2_POS<N%2>::value
    * POW2_POS<N/2>::value
    * POW2_POS<N/2>::value;
};
template<> struct POW2_POS <1> { static const float value = 2.0f; };
template<> struct POW2_POS <0> { static const float value = 1.0f; };

// Negative powers of two
template<unsigned int N> struct POW2_NEG {static const float value = 1.0f / POW2_POS<N>::value; };

// Calls POW2_POS or POW2_NEG depending on boolean parameter
template<bool b, unsigned int N> struct POW2_SIGNED;
template<unsigned int N> struct POW2_SIGNED<true, N> { static const float value = POW2_POS<N>::value;};
template<unsigned int N> struct POW2_SIGNED<false, N> { static const float value = POW2_NEG<N>::value;};

// Integer power of two
template<int N> struct POW2 {
    static const float value = POW2_SIGNED<(N>=0), SIGN<(N>=0)>::value * N>::value;
};

Example program

#include <iostream>
int main()
{
    std::cout<<POW2<0>::value <<"\n";
    std::cout<<POW2<1>::value <<"\n";
    std::cout<<POW2<4>::value <<"\n";
    std::cout<<POW2<7>::value <<"\n";
    std::cout<<POW2<-4>::value <<"\n";
}

Output

1
2
16
128
0.0625

Your code is not C++98 compliant

Standards prior to C++11 only allows const integer static member initialisation (see here).

So unfortunately your code is not C++98 compliant and in fact does not compile. So technically your code is off-topic on this site. As it is not working as intended. Your code probably only compiles because you're using a compiler that has an extension to allow non integer const static members.

To get around this typically you would define a static method that returns the value. In fact, this is why std::numeric_limits<T>::max() is a function instead of a static member value. However these cannot be evaluated in a constant expression context. You need C++11 constexpr for that afaik.

General advice

Some general advice. Template parameters are compile time constants by definition. Any sensible compiler should do any necessary casts during compile time so you will not get a run-time penalty for using higher precision.

For this reason I advice that you use the largest types possible in your template to get the most precision at no additional cost. I.e. change float to double and int to long long.

You should consider wrapping the templates that are not intended for use by the user in a suitable namespace. Typically I use a nested namespace called detail.

Also identifiers in ALL_CAPS_WITH_UNDERSCORES are typically reserved for macros. So I advice against using this naming convention for your identifiers as it will confuse many readers.

Better implementation

If we restrict ourselves to the bits of the code that are C++98 compliant, then we can have a much easier implementation for the integer exponentiation.

template<unsigned long long exponent>
struct ipow_2{
    static const unsigned long long value = (1 << exponent);
};

If you want a C++98 compliant negative exponent you can use:

template<long long exponent>
struct pow_2{
    static double value(){
        if (exponent == 0) {
            return 1;
        }
        else if (exponent > 0) {
            double half = pow_2<exponent/2>::value();
            return (1<<(exponent&1))*half*half;
        }
        return 1 / pow_2<-exponent>::value();
    };
};

But be aware that the same caveat for non-integer const initialisation still applies so it's not hugely useful in TMP for example.

If we accept use C++11 you can simplify and generalize to:

// Tail call optimization will transform this into the equivalent
// loop
constexpr int64_t ipow_p(int64_t base, int exp, int64_t ans = 1) {
    return exp < 1 ?
        ans : ipow_p(base*base, exp/2, (exp % 2) ? ans*base : ans);
}

constexpr double ipow(int base, int exp){
    return exp > 0 ? ipow_p(base, exp) : 1.0 / ipow_p(base, -exp);
}

This is usable in a constant (TMP) context thanks to the constexpr qualifier and At -O2 the call will be expanded to a constant.


Scoring solution for Farkle (dice game)? I've written the following function on Python (3.3) to take a list of 6 integers and return the maximum score for a game of Farkle. While the results are correct I don't feel it's a very Pythonic implementation. I'd like to hear from the community on how I could make it better and more Pythonic. Any and all tips are most welcome!

"""
    written and tested in Python 3.3.6
"""

from random import randint

def score(my_roll, sides=6):
    """
        Calculate a Farkle score using the traditional scoring method, which is:
            4 1s:     2,000 points
            3 1s:     1,000 points
            Single 1: 100 points
            Single 5: 50 points
            Triple of any non-1 number:    100 x number showing
            Quadruple of any non-1 number: Double the triple score

        Notes:
            - Doubles score nothing (unless a 1 or a 5) as above
            - All scoring dice must be rolled in a single turn (i.e. they
            are not additive over turns)
            - Rolling all 6 will be two sets and scored accordingly
                [4] + [2]
                [3] + [3]
                [2] + [2] + [2]

        Examples:
            1,1,1,5,5,5 ==> 1500 (1000 for 1s + 250 for 3 5s)
            1,1,1,1,6,6 ==> 2000 (2000 for 4 1s)
            5,3,6,5,3,3 ==> 400 (300 for 3 3s + 100 for 2 5s)
            1,2,2,3,3,5 ==> 150 (100 for a 1 and 50 for a 5)
    """
    #create a table to hold the count of each die roll
    dice_array = [0] * sides
    score = 0

    #add up the number appearances of each die roll and store it in the table
    for dice in my_roll:
        dice_array[dice-1] += 1

    """
        based on the above scoring, determine the MAXIMUM score; in actual Farkle the
        player would choose which die to 'bank' and which to re-roll
    """
    for (i, count) in enumerate(dice_array):
        dice = i + 1  #this makes it easier to keep track of the die we're on

        if dice == 1:
            if count == 6: score += 2200
            if count == 5: score += 2100
            if count == 4: score += 2000
            if count == 3: score += 1000
            if count in [1, 2]: score += (count * 100)
        else:
            if count >= 4: score += (dice * 200)
            if count >= 3: score += (dice * 100)
            if (dice == 5 and count != 3): score += (count * 50)

    return score

#test cases
while True:
    roll = input("Enter 6 values (1-6) separated by a space: ")
    roll = [random.randint(1,6) for i in range(0, 6)] if roll == "" else [int(i) for i in roll.split(' ')]
    print(str(score(roll)) + " : " + str(roll))

"Pythonic" programming

Code is "pythonic" when it expresses its intention clearly, is easy to read or even looks like pseudo-code, and uses as little low-level garbage as possible. Let's take a look at a couple examples of low-level work in your code:

dice_array = [0] * sides
score = 0
for dice in my_roll:
    dice_array[dice-1] += 1
# as an aside, this isn't an array, it's a list.

This is wonky because it buids a fixed-size list, then plays with values-as-list-indexes to get a count. That might be fast, but it's certainly ugly. Let's not do that.

import collections

counts = collections.Counter(my_roll)

This is much better. It builds a dict-like object, a collections.Counter that has the counts for any dice present in the roll. counts[non_rolled_die] will throw a KeyError while counts.get(non_rolled_die, 0) will act like your current code does.

Your counting loop isn't a whole lot better. Let's look:

for i, count in enumerate(dice_array):
    dice = i + 1

We'll stop here for just long enough to mention that enumerate takes a keyword argument start that lets you tell it where to start counting from. This should be:

for die, count in enumerate(dice_array, start=1):

But since I switched to using a collections.Counter, we can just do:

for die, count in counts.items():
    if die == 1:
        if count == 6: score += 2200
        if count == 5: score += 2100
        if count == 4: score += 2000
        if count == 3: score += 1000
        if count in [1, 2]: score += (count * 100)
    else:
        if count >= 4: score += (dice * 200)
        if count >= 3: score += (dice * 100)
        if (die == 5 and count != 3): score += (count * 50)

First of all, avoid these repeated if statements. Try instead:

    if die == 1:
        if count == 3: score += 1000
        elif count == 4: score += 2000
        elif count > 4:
            score += 2000 + (count - 4) * 100

However there's a better approach. We already know that the triple value for each number is 100 * number, except for 1 which is 100 * 10 * number. Let's use that!

for die, count in counts.items():
    die_score = 0
    if count == 4:
        die_score += die * 2000 if die == 1 else die * 200
        count -= 4
    elif count == 3:
        die_score += die * 1000 if die == 1 else die * 100
        count -= 3

This should handle triplets and quadruplets of any value, and removes from count the number of dice consumed by the grouping. Now let's look at single dice values.

    if die in [1, 5]:
        single_value = 100 if die==1 else 50
        die_score += single_value * count

In fact we can factor some of that out to drop the if block.

    # somewhere earlier in the function
    single_values = {1: 100, 5:50}

    # then inside the loop we're looking at here....
    die_score += single_values.get(die, 0) * count

Finally at the end of our loop (just before exiting it), we add the single die_score to the roll-wide score.

    score += die_score

Testing

I'm a big fan of the unittest module for writing tests. In your case doctest may work better (since you've written examples already that only have to be tweaked slightly to use doctest), but if you start writing more intense functions, it's important to be able to grow your testing suite appropriately. Let's write some tests!

# ./test_farkle.py

import unittest
import farkle

class FarkleTests(unittest.TestCase):
    cases = [([1,1,1,5,5,5], 1500),
             ([1,1,1,1,6,6], 2000),
             ([5,3,6,5,3,3], 400),
             ([1,2,2,3,3,5], 150)]

    def testRolls(self):
        for got, want in self.cases:
            self.assertEqual(farkle.score(got), want)
            # fails test if `farkle.score(got) != want`

A change in data structure

It should occur to you that for every (die, count) pair, score increases by a given amount. This means we can hardcode that data in something like a dict.

# {die: {count: value, ... }, ... }
valuedict = {1: {1: 100,
                 2: 200,
                 3: 1000,
                 4: 2000,
                 5: 2100,
                 6: 2200},
             2: {3: 200,
                 4: 400},
             3: {3: 300,
                 4: 600},
             4: {3: 400,
                 4: 800},
             5: {1: 50,
                 2: 100,
                 3: 500,
                 4: 1000,
                 5: 1050,
                 6: 1100},
             6: {3: 600,
                 4: 1200}}

Now your function becomes pretty simple!

def score(my_roll, sides=6):
    valuedict = ...  # the whole deal above
    counts = collections.Counter(my_roll)
    score = 0
    for die, count in counts.items():
        score += valuedict[die][count]
    return score

Or even more simply:

def score(my_roll, sides=6):
    valuedict = ...
    counts = collections.Counter(my_roll)
    return sum(valuedict[die][count] for die,count in counts.items())


Dijkstra in C++ to find shortest path for every vertex of a directed graph I am trying to implement the Dijkstra algorithm in C++ that finds the shortest path for a graph from a text file. The text file contains a matrix that represents a directed graph.

What I am trying to do in this code is use every vertex as the source. I finished the code but I am still very skeptical if my code is right or not.

int minDistance(int dist[], bool sptSet[], int V)
{
// Initialize min value
int min = INT_MAX, min_index;

for (int v = 0; v < V; v++)
    if (sptSet[v] == false && dist[v] <= min)
        min = dist[v], min_index = v;

return min_index;}
double  dijkstra(int vertex, int **graph, int src)
{
auto start_time = chrono::high_resolution_clock::now();
int* dist = new int[vertex];     // The output array.  dist[i] will hold the shortest
// distance from src to i

bool* sptSet = new bool[vertex]; // sptSet[i] will true if vertex i is included in shortest
// path tree or shortest distance from src to i is finalized

src = k;
// Initialize all distances as INFINITE and stpSet[] as false
for (int i = 0; i < vertex; i++)
    {
    dist[i] = INT_MAX, sptSet[i] = false;
    }

// Distance of source vertex from itself is always 0
dist[src] = 0;

// Find shortest path for all vertices
    for (int count = 0; count < vertex - 1; count++)
    {
    // Pick the minimum distance vertex from the set of vertices not
    // yet processed. u is always equal to src in first iteration.
    int u = minDistance(dist, sptSet, vertex);

    // Mark the picked vertex as processed
    sptSet[u] = true;

    // Update dist value of the adjacent vertices of the picked vertex.
    for (int v = 0; v < vertex; v++)

        // Update dist[v] only if is not in sptSet, there is an edge from
        // u to v, and total weight of path from src to  v through u is
        // smaller than current value of dist[v]
        if (!sptSet[v] && graph[u][v] && dist[u] != INT_MAX && dist[u] + graph[u][v] < dist[v])
        {
        dist[v] = dist[u] + graph[u][v];
        }
    }
k++;
delete[] dist;
delete[] sptSet;
}

auto end_time = chrono::high_resolution_clock::now();
double elapsed = chrono::duration_cast<chrono::milliseconds>(end_time - start_time).count();

return elapsed;
}

I'm sure it'll be fairly easy for you to confirm if your code is working (you can find test cases all over the place), so I'm not looking too closely at correctness, so much as overall logic.

So, on the whole, it seems to make logical sense, but it's not an efficient implementation of Dijkstra. As you know, Dijkstra's algorithm involves having an ever increasing collection of nodes and picking the smallest path out of your collection (it's that simple).

The main flaw is in your minDistance() method. What you're doing there is effectively iterating over all the nodes you've selected and finding the shortest part out. That's an O(n) operation. This step in Dijkstra is crucial and should be done in O(log N) time. This can be done using a set or a heap. Since you always want the minimum path out, all you need to do is keep track of the paths out and pick the min at each stage.

As I mentioned, you can do this with a set/heap. I think this example should help clear this up.


How do I make the search in the typeahead widget more efficient I have a typeahead widget to get the state or city's population according to what the user types

Instead of issuing a GET request every time the user is done typing, the way this typeahead widget works is fetch data beforehand and store it in memory and every time user types, we are iterating through the data to retrieve the stuff that we need.

The live demo is here: https://codesandbox.io/s/saveddatatypeahead-lcyfg?file=/src/index.js

So the data is fetched like this

const data = [];

fetch(endpoint)
  .then(res => res.json())
  .then(result => {
    data.push(...result);
  });

and we listen for the input event on the input field, the timeId is for debouncing the searching, we don't want to search too frequently

searchInput.addEventListener("input", e => {
  if (e.target.value.length > 2) {
    if (timeId) {
      clearTimeout(timeId);
    }
    timeId = setTimeout(() => {
      displayMatches(e.target.value);
    }, 500);
  }
});

and the search is performed here

function findMatches(regex, data) {
  // the time complexity is
  // O(M x K)
  // M is the length of a city or state
  // K is the keyword to match
  // e.g. M - 'New York' K - 'yo'
  return data.filter(
    place => place.city.match(regex) || place.state.match(regex)
  );
}

Here the regex is constructed based on what the user typed const regex = new RegExp(text, "ig");

So for now it seems to me that the time complexity for the searching here is  O(N x M x K)
N is the number of entries in the data array, M is the average length of every city or state's name, and K is the keyword the user typed.

So my question is, is there a way to make the search more efficient? For now I have done two things to improve the efficiency.

1.if what the user types is less than 3 characters, we don't perform the search

2.debounce the search and only fire it when the user is done typing

Right the amount of data is not huge but I can see that it doesn't scale well with this naive implementation of search. Maybe building a trie can help? I am not sure.

Also any other suggestions and critiques about this widget are highly welcomed

The way the regular expression engine works is, it iterates along every character in the input string, checking if a match to the pattern starts there. For example, searching for "jose" in "san jose" will result in the match being attempted at index 0, which immediately fails, and the bumpalong increments to index 1, which also immediately fails. This continues until index 4, at which point the j is matched successfully, so the pattern continues, matching o, s, e. When the pattern is just composed of ordinary characters, the length of the pattern doesn't have much of an effect, because on the vast majority of indicies, it'll immediately fail. Rarely, it'll have to iterate a few characters in the pattern before an index fails, eg searching for san jo against San Francisco at index 0, but this is uncommon and still only goes through part of the pattern. So, pattern length doesn't affect computational power required much; it isn't something to worry about.

When turning user input into a regular expression, make sure to escape the input text first, otherwise the constructed pattern's syntax could be invalid or not match what it should. (You could also make a whitelist of characters, and remove any character not on the whitelist from the input)

But for the expensive operation you want to optimize, since it looks like you're just trying to find whether a substring exists in a larger string, a regular expression is overkill - use String.prototype.includes instead. Turn the input and data strings to lower case first, to preserve case-insensitive search - for the data, do this once, on pageload, not inside findMatches. Store the lower-cased results in another property of course, lowerCity and lowerState. There may occasionally have unusual characters like San José, so call String.prototype.normalize on both the input and data, to get a string without accents and diacritics. (You need to use the regex later to construct the HTML, of course, but you don't need to use it when performing the expensive search to find the matching places)

To further cut down on computations required, a possible approach is create an object indexed by characters a to z. Have each property value be an array containing the places that contain that character. Then, when searching for a place, find the least common character in the search string, then look up and search through that array on the object. For example, when searching for "Jose", the least common character is J, so you'd look up the j property. Taking the cities listed on this page as an example, this results in having to search through only 40 cities instead of 2000. (This approach requires a bit more memory, though)

Here's a quick example of the algorithm, taking a few of the places from your gist, and displaying matches as you type:

const places=[{city:"New York",growth_from_2000_to_2013:"4.8%",latitude:40.7127837,longitude:-74.0059413,population:"8405837",rank:"1",state:"New York"},{city:"Los Angeles",growth_from_2000_to_2013:"4.8%",latitude:34.0522342,longitude:-118.2436849,population:"3884307",rank:"2",state:"California"},{city:"Chicago",growth_from_2000_to_2013:"-6.1%",latitude:41.8781136,longitude:-87.6297982,population:"2718782",rank:"3",state:"Illinois"},{city:"Houston",growth_from_2000_to_2013:"11.0%",latitude:29.7604267,longitude:-95.3698028,population:"2195914",rank:"4",state:"Texas"},{city:"Philadelphia",growth_from_2000_to_2013:"2.6%",latitude:39.9525839,longitude:-75.1652215,population:"1553165",rank:"5",state:"Pennsylvania"},{city:"Phoenix",growth_from_2000_to_2013:"14.0%",latitude:33.4483771,longitude:-112.0740373,population:"1513367",rank:"6",state:"Arizona"},{city:"San Antonio",growth_from_2000_to_2013:"21.0%",latitude:29.4241219,longitude:-98.49362819999999,population:"1409019",rank:"7",state:"Texas"}];

const placesByChar = {};
for (const place of places) {
  for (const char of new Set(place.city.toLowerCase() + place.state.toLowerCase())) {
    if (!placesByChar[char]) placesByChar[char] = [];
    placesByChar[char].push(place);
    place.cityLower = place.city.toLowerCase();
    place.stateLower = place.state.toLowerCase();
  }
}
const charFrequencies = [['z',128],['j',188],['q',205],['x',315],['k',1257],['v',2019],['b',2715],['p',3316],['g',3693],['w',3819],['y',3853],['f',4200],['m',4761],['c',4943],['u',5246],['l',7253],['d',7874],['h',10795],['r',10977],['s',11450],['n',12666],['i',13318],['o',14003],['a',14810],['t',16587],['e',21912]]
  .map(([char]) => char);
const input = document.querySelector('input');
const results = document.querySelector('div');
input.addEventListener('input', () => {
  // validate, debounce, and:
  const lowerValue = input.value.toLowerCase();
  const char = charFrequencies.find(([char]) => lowerValue.includes(char));
  if (!char) return;
  results.textContent = '';
  placesByChar[char]
    .filter(place => place.cityLower.includes(lowerValue) || place.stateLower.includes(lowerValue))
    .forEach((place) => {
      results.textContent += place.city + ',';
    });
});
<input>

Your listener has a bug: if it runs when a displayMatches is currently being debounced, but the input length is 1 or 0, the outer if will not be entered, but the timeout won't be cleared. If you enter text, then clear it, the still-running displayMatches(e.target.value); will display all places. You should clear the timeout regardless - and since you have a reference to the input element already, you can use that instead of e.target.value:

let timeId;
searchInput.addEventListener("input", e => {
  clearTimeout(timeId);
  const { value } = searchInput;
  if (value.length < 3) {
    return;
  }
  timeId = setTimeout(() => {
    displayMatches(value);
  }, 500);
});

Your fetch has no catch. If there are errors (gist deleted, site down, network offline), it would be more user-friendly to display them to the user, rather than having the app silently fail.

You should usually be quite wary of concatenating data into a string that gets inserted as HTML, eg:

${text}

It might be safe with this dataset, but for others, it would allow for arbitrary code execution (and, for example, send the user's login information in their cookies to a malicious actor).

const text = `<img src onerror="alert('evil')">`;
const html = `<span class='type-ahead__result-city--hl'>${text}</span>`;
document.body.innerHTML = html;

Either escape special characters first, or set the text by assigning to the textContent of the element after it's been created.


Spaceship Operation in for Lexicographic Comparison I have written a program in Java 8 using BlueJ. There is just one function of type byte that accepts two String values and return 1 if the first word comes before the second in alphabetical order, and return -1 if the first word comes after the second in alphabetical order,  and 0 if both are equivalent strings. I would welcome any suggestions on how to make my code faster and more readable.

public class Test
{
    public static byte spaceshipAlphabetical (String str1, String str2)
    {
        int str1Length = str1.length();
        int str2Length = str2.length();
        int strLength;
        if
        (str1Length < str2Length)
        {
            str2 = str2.substring(0, str1Length);
            strLength = str1.length();
        }
        else
        {
            str1 = str1.substring(0, str2Length);
            strLength = str2.length();
        }
        str1 = str1.toUpperCase();
        str2 = str2.toUpperCase();
        char charStr1;
        char charStr2;
        int intChar1 = 0;
        int intChar2 = 0;
        for
        (int i = 0; i < strLength; i++)
        {
            charStr1 = str1.charAt(i);
            charStr2 = str2.charAt(i);
            intChar1 = (int) charStr1;
            intChar2 = (int) charStr2;
            if
            ((intChar1 < intChar2) || (intChar2 < intChar1) || ((intChar2 == intChar1) && (i == (strLength - 1))))
            {
                break;
            }
        }
        if
        ((intChar1 < intChar2) || ((intChar2 == intChar1) && (str1Length < str2Length)))
        {
            return 1;
        }
        else if
        ((intChar2 < intChar1) || ((intChar2 == intChar1) && (str2Length < str1Length)))
        {
            return -1;
        }
        else
        {
            return 0;
        }
    }

[1] By Java convention curly brackets belong in the same line as the preceding statement/expression.

Instead of this:

if
()
{}
else
{}

for
()
{}

do this:

if () {
    ...
} else {
    ...
}

for () {
    ...
}

I suggest you read the Google Java style guide for more tips: https://google.github.io/styleguide/javaguide.html

[2] You don't have to introduce an intermediate variable for holding chars, you can directly assign the result of charAt() method to an int variable: intChar1 = (int) str1.charAt(i);

[3] You don't have to do all those substring operations to make the strings the same length. Rewrite the main for loop to do as many iterations as there are characters in the shorter string. Written more succinctly: min(length(A), length(B) iterations

[4] To lexicographically compare two strings, you have to find the first different character in both strings and handle the remaining edge cases after the main for loop. Some pseudocode:

for i = 0; i < (min(length(A), length(B)); i++
    if A[i] != B[i]
        if A[i] < B[i] return -1
        else return 1

// If you didn't return from the method at this point, there are only three options left
String A is shorter than string B - return -1
String B is shorter than string A - return 1
String A and string B are equal - return 0


Compare result files from two different runs the smart way? The program counts attributes of various (geo-)objects and prints them to different files.

One of the options compares two different datasets with each other.

parser.add_option("-c", "--compare datasets", dest="compare",
                  help="If you want to campare two datasets (from different years for example) set the c-flag to 1. Run both datasets\
                    one after the other. After the second run you will find a file named COMPARE_* in the output folder. The default\
                  behaviour is no comaprison.")

I am concerned about the behavior of my current implementation. The basic idea is to pickle a list of file names when the -c flag is set to 1. When the program is run for the second time with the -c flag it will load all file names needed for the comparison from the pickle file.

if options.compare == '1':
    # check if there is a pickle file
    p = glob.glob('%s/*.pikl' % data_output)
    if len(p) > 1:
        raise Exception('There are two pickle files. Comparison not possible.')
    elif len(p) == 1:
        pfiles_list = pickle.load(open(p[0],'rb'))
        if len(pfiles_list) == len(files_to_compare):
            for i,(f1,f2) in enumerate(zip(pfiles_list,files_to_compare)):
                print 'comparing files...'
                if i == 0:
                    csv_cmp_lazy(f1,f2, compare_file)
                else:
                    csv_cmp_lazy(f1,f2, compare_file, append_file = True)
            info_str = 'Deleting pickle file %s' % os.path.basename(p[0])
            print info_str
            logging.info(info_str)
            os.remove(p[0])
        else:
            raise ValueError('len of files current and last run are not equal: Number of file current run/last run: %s/%s' % (len(files_to_compare),len(pfiles_list))
    else:
        pickle.dump(files_to_compare, open(pikl_file,'wb'))

This implementation relies on a few assumptions:

that the pickle file won't be moved away from data_output
that os.remove(p[0]) will be successful

Not very secure, right? Besides, imagine the user finds an error in his data and wants to do the second run again: the comparison will fail because the pickle file won't be there anymore. This can be very annoying as the program may take longer than one hour to terminate (depending on input options and dataset).

Any suggestions on how to do this in a smarter way?

Simplify all the things! You and your users will benefit from simplicity.

Option parsing

If the --compare option is either True or False, make it a boolean flag with False as the default value. Your users won't have to remember if the --compare argument is 1, true or yes, they will simply write --compare.

You should also consider moving from optparse to argparse: optparse is deprecated since Python 2.7 and Python 3.2 and could be removed in any future release.

About your assumptions

If the pickle file is moved away from data_output, then the users will simply see that the file is missing. You could catch the resulting exception to ask them to first run the program without comparison.
If the removing fails, then something is really wrong anyway.

I don't think resolving those specific issues is the way to go, and it should be easier for your users to provide this comparison feature differently.

Comparison logic

Your current solution is mostly a hack. It looks like you only asked yourself: what is the simplest way to implement this? The question should be different: what is the simplest way to use this?

Say I want to compare 2010 and 2011. I first run your program with 2010, then with 2011 plus comparison. What if I actually made a typo and typed 2010 instead of 2009? What if one of the options for 2011 was wrong? I have to start all over again. Very frustrating since the processing is slow.

I don't know your program well enough to be sure, but the simplest way could be to provide a --dump option, so that users would dump the first dataset (slow), dump the second dataset (slow), then compare the two (fast!).

It would also be even clearer to provide two binaries, one for processing the datasets, and another one for comparison: this would reduce confusion.

Users are now in control and understand what happens and how the comparison works. Fixing any issue does not require to run processing for the two datasets. User won't delete the dumped files, and if they did, that would be their fault since you exposed those files to them.


Min / Max function of 1D array in C / C++ Due to software constraints, I cannot use the standard libraries, <math.h>, <algorithm>, templates, inline, or Boost. I am also using standard C (ISO C99) such that array is not a reserved keyword like it is in Visual Studio.

Is this the "best" possible implementation of a min function? This needs to be robust and fast. Is there a more efficient C++ implementation?

double min(double* array, int size){
    // returns the minimum value of array
    static double val = array[0];
    for (int i = 1; i < size; ++i){
        val = val <= array[i] ? val : array[i];
    }
    return val;
}

It's not quite the best.  There's some things IMHO that is hindering its performance and usefulness.

There's no point in declaring val as a static variable.  In fact, you've killed any chance of it being usable in multi-threaded programs.

The body of the loop is performing an assignment in every single iteration when it doesn't need to.  If you want it to be its best, you should only be doing so when it is required.

Your overall structure is fine, assuming we can expect well-formed inputs where array is nonempty and size is positive, I'd just change it so it's more like this:

double min(const double *arr, size_t length) {
    // returns the minimum value of array
    size_t i;
    double minimum = arr[0];
    for (i = 1; i < length; ++i) {
        if (minimum > arr[i]) {
            minimum = arr[i];
        }
    }
    return minimum;
}


Clear cache files older than 30 minutes This is what I came up with to delete files not modified within the last 30 minutes from a subdirectory named cache.

'''
Clear cache files older than 30 minutes.
'''

import os
import time

outer = os.getcwd()
cache = os.path.join(outer, 'cache')
count = 0
for f in os.listdir(cache):
        f = os.path.join(cache, f)
        use_by = time.time() - 30 * 60
        if os.path.getatime(f) < use_by:
                os.remove(f)
                count += 1
print("Removed {} files.".format(count))

Would love input as to how to make it more concise, as well as other improvements or details I might have missed.

Had tried to put os.getcwd() within os.path.join but got:

  File "/Users/mikekilmer/Envs/GLITCH/bin/../lib/python2.7/posixpath.py",         line 77, in join
    elif path == '' or path.endswith('/'):
AttributeError: 'builtin_function_or_method' object has no attribute 'endswith'

Instead of this:

for f in os.listdir(cache):
        # ...
        use_by = time.time() - 30 * 60
        if os.path.getatime(f) < use_by:
                # ...

Consider this:

use_by = time.time() - 30 * 60
for f in os.listdir(cache):
        # ...
        if os.path.getatime(f) < use_by:
                # ...

In this version, use_by is set only once, before the loop begins,
which is slightly more efficient.
Unless, you really want the timing to be re-evaluated in every iteration.

It can be error-prone and lead to nasty bugs to re-assign the loop variable (as well as method arguments), like here:

for f in os.listdir(cache):
        f = os.path.join(cache, f)

And in any case, I would recommend using more meaningful variable names, for example:

for filename in os.listdir(basedir):
    path = os.path.join(basedir, filename)

The code you posted sounds like a good idea.
It could be turned into a reusable, general function with parameters:

basedir: base directory to clean
limit: time limit to use, in minutes

Something like this:

def wipe_unused(basedir, limit):
    """
    Remove files in *basedir* not accessed within *limit* minutes

    :param basedir: directory to clean
    :param limit: minutes
    """
    atime_limit = time.time() - limit
    count = 0
    for filename in os.listdir(basedir):
        path = os.path.join(basedir, filename)
        if os.path.getatime(path) < atime_limit:
            os.remove(path)
            count += 1
    print("Removed {} files.".format(count))

if __name__ == '__main__':
    wipe_unused(os.path.join(os.getcwd(), 'cache'), 30 * 60)


Load multiple text files with same name but in different folders I have the following code, which basically load some data files that I have in different folders, take an average of every repeat at each Temperature and then plot the results. The code works fine, and it was OK when I had only a couple of set of data. But now I have 9 different set of temperature each with 5 repeat each and the code is becoming too long in my opinion. Is there a way to consolidate it and make it more readable and efficient?

import numpy as np
import matplotlib.pyplot as plt

steps    = np.loadtxt('/home/aperego/data/HexaPaper/nvt/303K/1st/Average_MSD.txt',usecols=[0])

def load(temp,pos):
    return np.loadtxt('/home/aperego/data/HexaPaper/nvt/'+temp+'K/'+pos+'/Average_MSD.txt',usecols=[1])

# T = 303 K
msd303_1 = load('303','1st')
msd303_2 = load('303','2nd')
msd303_3 = load('303','3rd')
msd303_4 = load('303','4th')
msd303_5 = load('303','5th')

msd303 = np.vstack((msd303_1,msd303_2,msd303_3,msd303_4,msd303_5)).T
msd303_mean = np.mean(msd303,axis=1)
msd303_std = np.std(msd303,axis=1)

# T = 313 K
msd313_1 = load('313','1st')
msd313_2 = load('313','2nd')
msd313_3 = load('313','3rd')
msd313_4 = load('313','4th')
msd313_5 = load('313','5th')

msd313 = np.vstack((msd313_1,msd313_2,msd313_3,msd313_4,msd313_5)).T
msd313_mean = np.mean(msd313,axis=1)
msd313_std = np.std(msd313,axis=1)

# T = 323 K
msd323_1 = load('323','1st')
msd323_2 = load('323','2nd')
msd323_3 = load('323','3rd')
msd323_4 = load('323','4th')
msd323_5 = load('323','5th')

msd323 = np.vstack((msd323_1,msd323_2,msd323_3,msd323_4,msd323_5)).T
msd323_mean = np.mean(msd323,axis=1)
msd323_std = np.std(msd323,axis=1)
plt.yscale("log")
plt.xscale("log")
plt.plot(steps,msd303_mean,label ='303K')
plt.plot(steps,msd313_mean,label ='313K')
plt.plot(steps,msd323_mean,label ='323K')
plt.legend(loc="best")
plt.show()

You are right to question your current approach.  With 9 sets of data with 5 repeats of each, you would need 45 variables with your current approach.  Using a list or dictionary for one dimension would reduce you to only 5 or 9 variables depending on which dimension became the list/dictionary.  Use a list/dictionary of lists/dictionaries could reduce the required variables to 1.

Using '/long/path/'+var1+'suffix/'+var2+'/filename.ext' is hard to read.  First, the PEP8 standard requires spaces around the + operators (as well as other places, such as after commas).  Using a .format() statement can make the string manipulation a bit clearer, and allow configuration to be move out of functions to a global scope, making changes easier.

Whether you want lists or dictionaries for the data, whether you want to store the individual msd[temperature][repeat] for later use or whether you just need to read the data in to compute the mean & standard deviation and can then discard the data, and so on is unclear.  Here (untested) is a rough reworking of your code to allow you to extend the temperatures and repeats as required; adapt as necessary.  The msd variable is a dictionary of lists, so the variable msd323_5 has become msd[323][4].

import numpy as np
import matplotlib.pyplot as plt

# Configuration

FILENAME = '/home/aperego/data/HexaPaper/nvt/{}K/{}/Average_MSD.txt'
REPEATS = ('1st', '2nd', '3rd', '4th', '5th')
TEMPERATURES = (303, 313, 323)

# Read data

def load(temp, pos):
    return np.loadtxt(FILENAME.format(temp, pos), usecols=[1])

steps = np.loadtxt(FILENAME.format(TEMPERATURES[0], REPEATS[0]), usecols=[0])

msd = {}
msd_mean = {}
msd_std = {}

for temperature in TEMPERATURES:

    repeats = []

    for repeat in REPEATS:

        sample = load(temperature, repeat)
        repeats.append(sample)

    data = np.vstack(repeats).T

    msd[temperature] = data
    msd_mean[temperature] = np.mean(data, axis=1)
    msd_std[temperature] = np.std(data, axis=1)

# Plot the data

plt.yscale("log")
plt.xscale("log")

for temperature in TEMPERATURES:
    plt.plot(steps, msd_mean[temperature], label="{}K".format(temperature))

plt.legend(loc="best")
plt.show()


GLSL function used to differentiate between pixels above/below a line I've recently acquired a program which is capable of using a GLSL fragment shader as a plugin. After a short amount of experimentation, I've realized that - for my use-case - I desperately need a function that does the equivalent of the following:

Takes two 2D points and calculates a line between them
Returns 1.0 if the current pixel is above (or below, callers choice) the line, and 0.0 otherwise. If the points are horizontally coincident, "above" == "to the left"

For this reason I've created the slice function. For my use-case this function has the potential to be called many, many times within a single shader, and should be As Performant As Possible™, but I am no GLSL guru...

My current, working code is below. The function arguments are as follows:

aspect: the current pixel's coordinate
p1, p2: the points defining the line
top: whether above or below the line returns 1.0

//Note: !(x ^^ y) == XNOR(x, y)
float slice(vec4 aspect, vec2 p1, vec2 p2, bool top) {
    // degenerate case - shade left or right
    if (p2.x == p1.x) return float(!(top ^^ aspect.x < p1.x));

    // we must center the line over the leftmost point
    vec2 base = p2.x > p1.x ? p1 : p2;

    //calculate the line's slope
    float slope = (p2.y-p1.y) / (p2.x-p1.x);

    //return 1.0 or 0.0 according the rules defined above
    return float(!(top ^^ (aspect.y - base.y)>slope*(aspect.x - base.x)));
}

Can this code be made to run faster using only GLSL?

I would try using the dot product for your test. If you take the vector from p1 to p2 and form the dot product with the vector from p1 to aspect, it will return the cosine of the angle between them. The cosine of an angle is positive when the angle is between -90° and 90° and it's negative when the angle is in the other 180° of the circle. What this means is that when aspect falls on one side of the line it will be positive and on the other it will be negative. So your function could be rewritten as something like this:

float slice(vec4 aspect, vec2 p1, vec2 p2, bool top) {
    aspect /= aspect.w;    // <- may be unnecessary if w is guaranteed to be 1.0
    vec2 v1 = aspect.xy - p1;
    vec2 v2 = p2 - p1;
    float cosTheta = dot(v1, v2);
    float result step(0.0, cosTheta);
    return top ? result : 1.0 - result;
}

I haven't tested this, but it removes all conditionals and logical operators. If you don't need the divide by w then there are no divisions, either. I think this could be faster, but you'll need to profile to be sure.

Note that this assumes that "on the line" can be taken as being on one side or the other. If you need to differentiate that case, I think you'll need either a conditional or some clever non-conditional code to extract that case.


Python script searching for string in textfile and inserting another textfile before match I'm very new to Python, and far away from writing my own scripts. For my work with lilypond I needed a script that parses a text file, searches for a string and inserts another textfile before the match. I have been searching quite a lot for this kinda script and I did not find any. I ended up combining the snippets I found on here and other sites and came up with this script, which is working:

#!/usr/bin/env python

# usage:
# $ python thisfile.py text.txt searchstring insert.txt

import sys

f2 = open(sys.argv[3])
data = f2.read()
f2.close()

with open(sys.argv[1], "r+") as f1:
    a = [x.rstrip() for x in f1]
    index = 0
    for item in a:
        if item.startswith(sys.argv[2]):
            a.insert(index, data)
            break
        index += 1
    f1.seek(0)
    f1.truncate()
    for line in a:
        f1.write(line + "\n")

I also got a very detailed answer on Stack Overflow, telling what is actually going on in the code, before I was far away from understanding any detail.

What I got out of it so far is the following problem:

  If anything would go wrong with reading in the data from f1 or f2, f1.truncate() would delete the original content of f2, then not being able to (re)write the appropriate content the content would get lost. A much more secure way would be using some kind of temporary file, or at least moving the original content of f1 there before calling truncate().

I would be glad for any comments on this "problem", and any others if there are.

The problem is a general one in data processing, one that programmers have to think about all the time! When changing a file \$F\$ from \$A\$ to \$B\$ it's tempting to implement it like this:

Read \$A\$ from \$F\$.
Compute \$B\$ from \$A\$.
Delete \$F\$.
Write \$B\$ to \$F\$.

But we need to consider the possibility that something will go wrong. Maybe the user will type control-C on the keyboard and interrupt the program? Maybe there will be a power cut? Maybe the disk will not have enough room for \$B\$? If any of these things happened after step 3 and before step 4, then you would be left in a situation where \$F\$ contains neither \$A\$ nor \$B\$. So you have lost your data and can't get it back.

This is why we try to design systems so that operations are atomic — either they succeed completely or they fail completely. In this case we would use the following procedure:

Read \$A\$ from \$F\$.
Compute \$B\$ from \$A\$.
Write \$B\$ to a temporary location \$G\$.
Replace \$F\$ with \$G\$.

This works because operating systems (usually!) give us an atomic implementation of step 4. In Python we can use os.rename, where you can see that the documentation says:

  If successful, the renaming will be an atomic operation

In this design, if something goes wrong before step 4, the file \$F\$ still contains \$A\$, and so we haven't lost our data, and so we have a chance to fix the problem and try again.

So in this case, I'd write something like this (but this is not tested, so don't use it blindly!):

import os
import shutil
import sys
import tempfile

# usage:
# $ python thisfile.py text.txt searchstring insert.txt
text_file, searchstring, insert_file = sys.argv[1:]

with tempfile.NamedTemporaryFile('w', delete=False) as temp:
    with open(text_file) as f1:
        inserted = False # Have we inserted insert_file yet?
        for f1_line in f1:
            if not inserted and f1_line.startswith(searchstring):
                with open(insert_file) as f2:
                    for f2_line in f2:
                        temp.write(f2_line)
                inserted = True
            temp.write(f1_line)

os.rename(temp.name, text_file)

Here I've used the library function tempfile.NamedTemporaryFile to choose somewhere to put the temporary file.

 Update

So here's another reason why it's a good idea to make operations atomic — you might have made a programming error! The code I wrote above works correctly on my operating system (macOS) but as it says in the os.rename documentation:

  The operation may fail on some Unix flavors if src and dst are on different filesystems

So I'm guessing that you're on some kind of Linux system. On these systems you've got to ensure that the temporary file \$G\$ is on the same filesystem as \$F\$, and the only reliable way to do that is to put it in the same directory as \$F\$:

import os
import shutil
import sys
import tempfile

# usage:
# $ python thisfile.py text.txt searchstring insert.txt
text_file, searchstring, insert_file = sys.argv[1:]

# Directory and name of text_file.
dirname, basename = os.path.split(text_file)

# Create temporary file in same directory as text_file.
with tempfile.NamedTemporaryFile('w', dir=dirname, prefix=basename,
                                 delete=False) as temp:
    with open(text_file) as f1:
        inserted = False # Have we inserted insert_file yet?
        for f1_line in f1:
            if not inserted and f1_line.startswith(searchstring):
                with open(insert_file) as f2:
                    for f2_line in f2:
                        temp.write(f2_line)
                inserted = True
            temp.write(f1_line)

os.rename(temp.name, text_file)

Writing reliable code that works on different platforms is not easy!


Inventory system/management Just wondering how my code looks, consider it a "professors input" since I'm not taking classes this semester. Also I'd like to note that this will soon be transitioning to a database approach where I use a MySQL database to get the InventoryItems.
Inventory.h
#pragma once

#include <vector>
#include <iostream>

#include "InventoryItems.h"

class Inventory
{
public:
    Inventory();
    ~Inventory();

    void createNewInventoryItem(std::string itemName = "", unsigned int maxQuantity = 0, unsigned int orderThreshold = 0,
                                                                         double price = 0.0, int quantityInInventory = 0);
    void deleteInventoryItem(int posInVector);
    InventoryItems* getItem(int posInVector);
private:
    std::vector<InventoryItems*> m_inventory; //NOTE: does not use a smart pointer, instead the pointer is deleted from the heap in the deleteInventoryItem function
                                              //      if deleteInventoryItem is not called at the end of the inventory objects life cycle the inventory objects deconstructor will
                                              //      loop through all elements in m_inventory and call deleteInventoryItem for each item
};

Inventory.cpp
#include "Inventory.h"

Inventory::Inventory()
{
    std::cout << "Inventory created" << std::endl;
}

Inventory::~Inventory()
{
    for (int i = 0; i < m_inventory.size(); i++) {
        deleteInventoryItem(i);
    }
    std::cout << "Inventory destroyed" << std::endl;
}

void Inventory::createNewInventoryItem(std::string itemName, unsigned int maxQuantity, unsigned int orderThreshold,
                                                                             double price, int quantityInInventory)
{
    InventoryItems* newItem = new InventoryItems;
    newItem->createInventoryItem(itemName, maxQuantity, orderThreshold, price, quantityInInventory);
    m_inventory.emplace_back(newItem);
}

void Inventory::deleteInventoryItem(int posInVector)
{
    delete m_inventory.at(posInVector);
    m_inventory.erase(m_inventory.begin() + posInVector);
}

InventoryItems* Inventory::getItem(int posInVector)
{
    return m_inventory.at(posInVector);
}

The inventory class will really come into it's own, this is just a proof of concept before I incorporate my database (I.E. inventoryItems is acting as a place holder for the DB and queries, hence me not including it here).

Overview
It's a mistake to be using RAW pointers you should definitely be using some form of smart pointer here.
It does two things for you:

It will greatly simplify your code.
It will remove the bugs !!!

If you have owned RAW pointers in your class then you MUST implement the rule of three. You do (have owned RAW pointers) but your don't (implement the rule of three). As a result your code is broken.
{
    Inventory   a;
    a.createNewInventoryItem("Wine");

    Inventory   b(a); // You did not implement the copy constructor
                      // nor did you disable it, so the default version
                      // is generated. So this copy is allowed.
                      //
                      // Both a and b have a copy of the same RAW pointer
                      // internally.
}
// b is destoryed (which destroys the pointer)
// a is destroyed (which also destroys the pointer) but that is UB as
// it has already been destroyed.

There are several other things that go wrong but you get the drift.
If you had used std::unique_ptr<> this would have prevented the Inventory object from being copied. If you had used std::shared_ptr<> copying would have been allowed (not sure if the semantics would be correct).

I would point out that you only need to store pointers and have dynamic allocation if you have polymorphic types. Otherwise you can simply store the object directly in the container:
//  This will handle most situations.
std::vector<InventoryItems>    m_items;
                 ///     ^^^ note: No pointer

You do need pointers for polymorphic types:
class Animal { /* Stuff */}
class Cat: public Animal { /* Stuff */}
class Dog: public Animal { /* Stuff */}

std::vector<Animal*>            m_items;
// Here we need pointers because we may store a Cat or a Dog
// So the object stored is not an Animal and we don't know how
// large it is at compiler time.

Interface Design:
class Inventory
{
public:
    void createNewInventoryItem(/*Stuff*/);
    void deleteInventoryItem(int posInVector);
    InventoryItems* getItem(int posInVector);
};

Sure you have a create/delete here and a get.
But when you create an item you don't know where in the container it is. So you can not directly get it back. You could use something about the input parameters that you could reuse (like the item name) or I would expect the create to return some identifier that can be used by the delete and get.
Alternatively your interface simply displays all the items and then you manipulate it from there. To do this you need to loop over all the items. But how many items do you have to loop over? In this case I would expect the class to provide some way to know the range of items available. It could be a size() method or alternatively you could provide iterators of the class so you know how to loop over the inventory.

Representation of the Inventory:
I can create two inventory items that have the same name:
    Inventory   a;
    a.createNewInventoryItem("Wine");
    a.createNewInventoryItem("Wine");

There are now two wine items in the inventory. This does not make sense to me. When you add a new item you should check to see if it already exists. If it does then you need to combine the data (or throw an error). You can do this with std::vector but there are other containers that will help you (maybe std::map).
Code Review
    void createNewInventoryItem(std::string itemName = "", unsigned int maxQuantity = 0, unsigned int orderThreshold = 0, double price = 0.0, int quantityInInventory = 0);

You can create an inventory item without specifying anything! If you pass no parameters it still works and you get an item with no name and no value.

When you create an inventory item it does not return anything. So how do you know what item to delete?
    void deleteInventoryItem(int posInVector);

You can randomly delete items (but there is no understanding of what item will be deleted (unless you get every item first and examine it).
Personally I would index items by their itemName. I would then allow both getItem() and deleteInventoryItem() to work by passing the name.

Don't return a pointer from a get(). I would return a reference to the object. Just because you store it as a pointer does not mean you need to expose that fact to the users of your class.
    InventoryItems* getItem(int posInVector);

Don't use std::endl here; prefer to use "\n" instead.
    std::cout << "Inventory created" << std::endl;

The difference between these two is that std::endl in addition to outputting the newline character it forces the stream to flush. There is no need to flush the stream as this happens automatically when it needs to happen. The problem with forcing a manual flush is that it can be inefficient and cause the application to slow down if there is to many flushes.
If you look at questions on SO asking why writing to standard output in C++ is so slow the actual solution is simply to stop using std::endl and allow the stream to flush itself (and the code magically speeds up).

Sure you can loop like this:
    for (int i = 0; i < m_inventory.size(); i++) {
        deleteInventoryItem(i);
    }

But I would note that this is broken because you call deleteInventoryItem() which not only deletes the item but removes it.
Imagine you have a vector with 4 items.

You call delete on item 0. This deletes the item and removes it. Thus moving everything in the vector down one item (new length is 3)
You increment i (i is now 1)
You call delete on item 1. This deletes the item and removes it. Thus moving everything in the vector down one item (new length is 2)
you increment i (i is now 2)
You exit the loop.

You only deleted two items.
But a vector is a range. So you can use the range based loop:
    for (auto& item: m_inventory) {
        delete item;
    }

    m_inventory.clear();

Why do you have a two phase creation of a InventoryItems?
    InventoryItems* newItem = new InventoryItems;
    newItem->createInventoryItem(itemName, maxQuantity, orderThreshold, price, quantityInInventory);

You should pass all those parameters to the constructor.

One thing I would add is that you don't know the number of items in the inventory. So you just have to loop over them until you get an exception thrown. It may be worth adding a size() function so you can get the size so you know the limit of the number of items.

You don't even need to use pointers here. There is no polymorphic objects involved. You should simply have used normal objects in the vector and it would have worked as expected.

Don't use unsigned integer types. In most situations they will cause you a lot of headaches. If that extra bit is something you need then you are already at the boundary of your system and should upgrade the integer type to the next larger type to get a bunch more bits.
Save the unsigned integer types for flags.

Don't use double for monetary units. Not all values can be represented exactly and you are going to run into rounding errors when you start adding things up.
Rather use an integer type. If you are using dollars in a double then simply use an integer but store the number of cents.

Potential Solution
How I would do it:
#include <map>
#include <iostream>

#include "InventoryItems.h"

class Inventory
{
    using Container = std::map<std::string, InventoryItems>;
public:
        Inventory();

        void createNewInventoryItem(std::string itemName,
                                    int maxQuantity = 0,
                                    int orderThreshold = 0,
                                    int price = 0,
                                    int quantityInInventory = 0);

        void deleteInventoryItem(std::string const& itemName);
        InventoryItems& getItem(std::string const& itemName);

        using iterator = typename Container::iterator;

        iterator begin() {return m_inventory.begin();}
        iterator end()   {return m_inventory.end();}

    private:
        Container    m_inventory;
};

Inventory::Inventory()
{
    std::cout << "Inventory created" << std::endl;
}

void Inventory::createNewInventoryItem(std::string itemName,
                                       int maxQuantity,
                                       int orderThreshold,
                                       int price,
                                       int quantityInInventory)
{
    auto find = m_inventory.find(itemName);
    if (find == itemName.end()) {
        m_inventory.emplace_back(std::piecewise_construct,
                                 std::forward_as_tuple(itemName),
                                 std::forward_as_tuple(maxQuantity, orderThreshold, price, quantityInInventory));
    }
    else {
        // I don't know how your inventory works.
        // This is just s guess/suggestion.
        find->second.setMax(std::max(maxQuantity, find->second.getMax());
        find->second.setOrderThreshold(std::max(orderThreshold, find->second.getOrderThreshold());
        find->second.setPrice(price);
        find->second.increaseInventory(quantityInInventory);
    }
}

void Inventory::deleteInventoryItem(std::string const& itemName)
{
    m_inventory.erase(itemName);
}

InventoryItems& Inventory::getItem(std::string const& itemName)
{
    auto find = m_inventory.find(itemName);
    if (find == m_inventory.end()) {
        throw std::outofbound("Or something like that");
    }
    return find.second;
}


Modern OpenGL wrapper abstractions (shaders) I am experienced with other languages, but not so much with the latest C++ standards. I was wondering if anyone had any tips on how to improve my code or any critique on my implementation. This is not designed to be used as an external or public library and I hope my use of the initializer lists are suitable (unwrapped pointers as they have no owner and aren't copied or stored).

I'm using spdlog and the Logger typedef is a typedef std::shared_ptr<spdlog::logger> Logger; in another (common) header.

program.cpp

#include <render/program.h>

Program::Program(Logger logger) :
    logger(std::move(logger)) {}

Program::operator GLuint() const {
    return program_id;
}

void Program::create(std::initializer_list<Shader *> shaders) {
    program_id = glCreateProgram();

    for (const Shader *shader : shaders) {
        glAttachShader(program_id, (GLuint) *shader);
    }

    glLinkProgram(program_id);

    GLint link_status;
    glGetProgramiv(program_id, GL_LINK_STATUS, &link_status);

    if (!link_status) {
        logger->error("Failed to link program");

        GLint log_length;
        glGetProgramiv(program_id, GL_INFO_LOG_LENGTH, &log_length);

        if (log_length > 0) {
            GLchar info_log[log_length - 1];
            glGetProgramInfoLog(program_id, sizeof(info_log), nullptr, info_log);

            logger->error((const char *) info_log);
        }
    }

    for (const Shader *shader : shaders) {
        glDetachShader(program_id, (GLuint) *shader);
    }
}

void Program::destroy() const {
    glDeleteProgram(program_id);
}

void Program::use() const {
    glUseProgram(program_id);
}

program.h

#pragma once

#include <engine.h>
#include <render/shader.h>
#include <initializer_list>

class Program {
public:
    explicit Program(Logger logger);

    explicit operator GLuint() const;

    void create(std::initializer_list<Shader *> shaders);

    void destroy() const;

    void use() const;

private:
    const Logger logger;
    GLuint program_id;
};

shader.cpp

#include <render/shader.h>
#include <fstream>

Shader::Shader(Logger logger, std::string path) :
    logger(std::move(logger)), path(std::move(path)) {}

Shader::operator GLuint() const {
    return shader_id;
}

void Shader::create(GLenum type) {
    std::ifstream input_stream(path);

    if (!input_stream) {
        logger->error("Unable to open shader file");

        return;
    }

    std::string source;
    char buffer;

    source.reserve(120);

    while (input_stream.get(buffer)) {
        source += buffer;
    }

    shader_id = glCreateShader(type);

    const GLchar *sources[] = {source.c_str()};
    glShaderSource(shader_id, 1, sources, nullptr);
    glCompileShader(shader_id);

    GLint compile_status;
    glGetShaderiv(shader_id, GL_COMPILE_STATUS, &compile_status);

    if (!compile_status) {
        logger->error("Failed to compile shader: {}", path);

        GLint log_length;
        glGetShaderiv(shader_id, GL_INFO_LOG_LENGTH, &log_length);

        if (log_length > 0) {
            GLchar info_log[log_length - 1];
            glGetShaderInfoLog(shader_id, sizeof(info_log), nullptr, info_log);

            logger->error((const char *) info_log);
        }
    }
}

void Shader::destroy() const {
    glDeleteShader(shader_id);
}

shader.h

#pragma once

#include <engine.h>
#include <string>

class Shader {
public:
    Shader(Logger logger, std::string path);

    explicit operator GLuint() const;

    void create(GLenum type);

    void destroy() const;

private:
    const Logger logger;
    const std::string path;
    GLuint shader_id;
};

Use glDetachShader() after linking the program.

Shaders are ref-counted on the driver side and prevented from cleanup until they have been detached from all used programs. You are hogging GPU resources for no good reason.
GLchar info_log[log_length - 1]; : eeep!

On top of being dangerous (you could possibly end up allocating megabytes of memory on the stack), it's not even legal C++. Just use a std::vector<char>.

Also, that -1 gives me the heeby-jeebies. It's just one byte, copy the null terminator.
Your way of loading an entire stream in a string is pretty awful and slow. You should figure out the length, make a single allocation, and read all the data at once like so:

input_stream.seekg(0, input_stream.end);
auto length = input_stream.tellg();
input_stream.seekg(0, input_stream.beg);
std::vector<char> source(length);
input_stream.read(source.data(), length);

Holding on to loggers as member variables is pretty bad in general. I have never seen a production system that requires that kind of granularity, and the multithreaded handling of these shared_ptr<>s could come and bite you in the future. Loggers are definitely one of those things that belong in singletons/global variables in my opinion.

Or even better: throw an exception with the error as its content. Let higher-level code handle the logging.
The create() functions should be invoked in the constructors (or just be the constructors). That's the whole point of RAII: bookending resource management in constructors and destructors. An additional benefit is that you won't need to keep the path and loggers around, just the GLuint.
On that same note, destroy() should just be the destructor, for the same reasons as above.
Speaking of destroy: in your current version, what would happen if destroy() is called and create() was not? Any sequence of invocations of the public interface of the class should be expected and handled (if only with an assert at worst).
void create(std::initializer_list<Shader *> shaders);

This is pretty constraining interface-wise. It's okay to have an initializer_list-based overload, but your primary interface should be a std::vector<Shader*>, or even better: a pair of iterators (or range if your code base has them).
I don't like the casts to GLuint as accessor for the programs. If you are going to have a wrapper, the wrapper should be handling all interactions that require the native handle if possible, and selectively give access using friend where absolutely necessary.
Finally, a heads up: this is pretty bare bones. You will find that once you start adding data-driven handling of Uniform Buffers Objects (you are talking about modern GL, after all) and vertex attribute bindings, a major refactor of all this code will almost certainly be needed.

Edit elaborating here instead of the comments:

  I've been told not to include too much work in constructors, and considering it's calling GL functions I figured that was considered a lot of work.

The primary role of a constructor is to establish the class invariant (the states the object is allowed to be in be in between public function calls), which you should always strive to keep as constrained as possible, because every public function should handle any and all valid invariant states. In your current code, the invariant for Shader is: The GLuint handle is either undefined, pointing to a valid shader, or pointing to a failed shader.

A better alternative would be: The GLuint handle points to a valid shader. Note that you cannot establish that invariant without using exceptions.

Limiting work done in the constructor is good, but not if it comes at the cost of a having to widen the invariant.

Similarly, keep in mind that unless there is a bug in the code, you can always count on the destructor being called, even in weird exception-handling scenarios. That's why releasing resources is always best done there. Doing the cleanup in there makes it so a user of the class can never forget to call destroy().

  I've also been told that initializer_lists are lightweight, thus its use. Thoughts on any of that?

They are lightweight, but fixed in size at compilation time. What if you wanted to load a list of shader to put in a program from a file? Again, they are fine as a secondary overload, but should not be the primary interface unless you can guarantee that the length will never be data-driven.