LINQ query for a user's submitted time-off requests What is a better way to create this select statement? I tried to just select 'u' and 'us', but the object properties were not accessible when I did this.

var userSubmittedRequests = (from u in db.TimeRequests
                                     join us in db.UserManagers
                                     on u.EmployeeUserID equals us.UserID
                                     join ut in db.Users
                                     on us.ManagerID equals ut.UserID
                                     where (us.ManagerID == userResult.UserID)
                                     where(u.User.disabled == false)
                                     select new PendingTimeOffViewModel
                                     {
                                       TimeRequestID = u.TimeRequestID,
                                       sDateTime = u.sDateTime,
                                       eDateTime = u.eDateTime,
                                       EmployeeUserID = u.EmployeeUserID,
                                       ManagerID = u.ManagerID,
                                       hrID = u.hrID,
                                       ApproveDenyReasonID = u.ApproveDenyReasonID,
                                       ManagerApproval = u.ManagerApproval,
                                       ManagerActionDate = u.ManagerActionDate,
                                       ManagerComment = u.ManagerComment,
                                       hrApproval = u.hrApproval,
                                       hrActionDate = u.hrActionDate,
                                       hrComment = u.hrComment,
                                       UserSubmitDate = u.UserSubmitDate,
                                       Comment = u.Comment,
                                       DayTypeID = u.DayTypeID,
                                       User = u.User,
                                       CompanyDesc = ut.Company.CompanyDesc,
                                       User1 = u.User1,
                                       User2 = u.User2,
                                       DayType = u.DayType,
                                       ManagerIDUM = us.ManagerID,
                                       ManagerADName = ut.ADUserName,
                                       ManagerName = ut.FullName
                                     }).ToList();

View model:

 public class PendingTimeOffViewModel
 {
    public int TimeRequestID { get; set; }
    public int  CompanyID {get;set;}
    public System.DateTime sDateTime { get; set; }
    public System.DateTime eDateTime { get; set; }
    public int EmployeeUserID { get; set; }
    public Nullable<int> ManagerID { get; set; }
    public Nullable<int> hrID { get; set; }
    public Nullable<int> ApproveDenyReasonID { get; set; }
    public bool ManagerApproval { get; set; }
    public Nullable<System.DateTime> ManagerActionDate { get; set; }
    public string ManagerComment { get; set; }
    public bool hrApproval { get; set; }
    public Nullable<System.DateTime> hrActionDate { get; set; }
    public string hrComment { get; set; }
    public string Comment { get; set; }
    public System.DateTime UserSubmitDate { get; set; }
    public int DayTypeID { get; set; }
    public Nullable<int> HoursRequested { get; set; }
    public virtual ApproveDenyReason ApproveDenyReason { get; set; }
    public virtual DayType DayType { get; set; }
    public virtual User User { get; set; }
    public virtual User User1 { get; set; }
    public virtual User User2 { get; set; }
    public virtual Company Company { get; set; }
    public int UserManagerID { get; set; }
    public Nullable<int> UserID { get; set; }
    public Nullable<int> ManagerIDUM { get; set; }
    public string ManagerADName { get; set; }
    public string ManagerName { get; set; }
    public string CompanyDesc { get; set; }
}

Not sure what you mean by a "better" way, but assume it's not having to specify every column. You should be able to do this with an anonymous type, though you'd still have to list navigation properties if you don't lazy load:

var query = [beginning of query] select new { u, us, u.User /* other nav properties */ };

Then you could iterate the results and use a mapping tool (Automapper, FastMapper, ExpressMapper...pick your poison) to map to the VM. Depending on the mapping tool, nav properties may have to be mapped separately:

var userSubmittedRequests = new List<PendingTimeOffViewModel>();

foreach (var request in query.ToList())
{
   userSubmittedRequests.Add(/* Map request to a PendingTimeOffViewModel instance */);
}

BUT, I don't like this approach. It's means you're doing a select * against every table involved: TimeRequests, UserManagers, ApproveDenyReason, etc. Is this for a grid? I've seen too many L2E queries like this start taking forever once the amount of data gets large enough because it's pulling back way more fields than necessary. An approach I've taken for grids is to create a dedicated SQL view that does the joins for me and flattens out the data, then have a DbSet for that view. You can let L2E handle any paging via Take and Skip. And honestly, I'd still rather see every column listed in the L2E select, so SQL is only querying the necessary fields.


Program that moves characters in n steps to the next character in alphabet based on ASCII code I made a variation on the program that moves the character to the next character. This version moves the characters n steps forward and takes care of white spaces in between. It works but wonder if there is a better way of coding this.
It only works with lowercase alphabet characters.

import java.util.Scanner;
public class NextCharacters{
    public static char nextCharacterInAlphabet(char character , int step)
    {
           char nextChar;
           int ascii = (int) character;

           if (ascii ==32)  //ascii code for a space is 32
                     /*space stays space that separates the different
                     strings seperated by the spaces */
                   nextChar = (char) ascii;
           /*at ascii 104=h remainder goes to zero so i have to add 104, have to add
           104 till ascii=122=z*/
            else if ((ascii +step) % 26>=0 && (ascii +step) % 26 <=18)
            {
                nextChar = (char) ( ((ascii +step) % 26)+104);
            }
            else
                    nextChar = (char) ( ((ascii +step) % 26)+78);
           /*first character is the 'a' with ascii value 97. Remainder of 97%26 is 19
           so to come up with 97 you have to add 78 to get the value for a.
           The same applies if you increase the number of positions you want the
           chars to move forward, have to this till it reaches ascii =104 where
           the remainder gets to 0 and have to add 104*/
        return nextChar;
    }

    public static void main(String[] args) {

        Scanner inputChars = new Scanner( System.in );

        Scanner inputSteps = new Scanner( System.in );

        StringBuilder sb = new StringBuilder();

        System.out.println("Please enter the characters ");
        String characterString = inputChars.nextLine();

        System.out.println("Please enter the number of positions you want each character to move forward");
        int numberOfMovesForward = inputSteps.nextInt();
        for (char c : characterString.toCharArray() )
        {
            sb.append(nextCharacterInAlphabet(c,numberOfMovesForward ));
        }
        System.out.println(sb.toString()) ;

    }

}

You should try to state your problem more clearly: "the program that moves the character to the next character" seem to assume we know some other program you're referring to. If I understand what you're trying to do, this is the problem:

"Given a String, replace each alphabetical character with the character STEP positions after in the alphabet, wrapping around if needed."

If this is the problem you're trying to solve, here is how I would do it:

public static char nextCharacterInAlphabet(char character , int step) {
  boolean isLower = Character.isLowerCase(character);
  char c = Character.toLowerCase(character);
  if (c < 'a' || c > 'z')
    return character;
  char nextC = (char)((c - 'a' + step) % 26 + 'a');
  return isLower ? nextC : Character.toUpperCase(nextC);
}


Breadth-First Search heuristics to find solution to Water Jug Challenge I started learning AI recently. In AI, searches should be avoided and everything in AI is a search problem.

One of the easiest problem where we could use general AI search techniques is the water jug problem which states:

  You are given two jugs, a 4-gallon one and a 3-gallon one. Neither has any measuring markers on it. There is a tap that can be used to fill the jugs with water. How can you get exactly 2 gallons of water into the 4-gallon jug?

I solved it using the BFS technique:

#include <bits/stdc++.h>

using namespace std;

vector <pair<int,int> > newStatesSet(pair<int,int> state){
  //generating all possible states from a given initial state
  vector<pair<int,int> > newStates;
  if(state.first==0 && state.second==0){
    newStates.push_back(make_pair(3,0));
    newStates.push_back(make_pair(0,4));
    newStates.push_back(make_pair(3,4));
  }
  if(state.first!=0 && state.second==0){
    newStates.push_back(make_pair(state.first,4));
    newStates.push_back(make_pair(0,state.first));
    if(state.first<3){
      newStates.push_back(make_pair(3,0));
      newStates.push_back(make_pair(3,4));
    }
  }
  if(state.first==0 && state.second!=0){
    int transferQuantity;
    if(state.second>3)
      transferQuantity=3;
    else
      transferQuantity=state.second;
    newStates.push_back(make_pair(transferQuantity,state.second-transferQuantity));
    newStates.push_back(make_pair(3,state.second));
  }
  if(state.first!=0 && state.second!=0){
    newStates.push_back(make_pair(0,state.second));
    newStates.push_back(make_pair(state.first,0));
    if(state.second<4){
      int transferQuantity1=state.second+state.first;
      int transferQuantity2;
      if(transferQuantity1>4){
        transferQuantity2=transferQuantity1-4;
        transferQuantity1=4;
      }
      else
        transferQuantity2=0;
      newStates.push_back(make_pair(transferQuantity2,transferQuantity1));
    }
    if(state.first<3){
      int transferQuantity1=state.second+state.first;
      int transferQuantity2;
      if(transferQuantity1>3){
        transferQuantity2=transferQuantity1-3;
        transferQuantity1=3;
      }
      else
        transferQuantity2=0;
      newStates.push_back(make_pair(transferQuantity1,transferQuantity2));
    }
  }
  return newStates;
}

bool checkGoal(pair<int,int> x,int sum){
  if(x.second==sum)
    return true;
  return false;
}

void reverseTraversal(map <pair<int,int>,pair<int,int> > parent,pair<int,int> initialState,pair<int,int> goalState){
  if(initialState != goalState){
    reverseTraversal(parent,parent[initialState],goalState);
  }
  cout<<"("<<initialState.first<<","<<initialState.second<<")"<<endl;
}

int main()
{
  //capacity (3,4)
  //open list
  queue <pair<int,int> > Q;
  pair <int,int> parNode;
  //contains all the states
  map < pair<int,int> ,int > occured;
  //contains parent
  map < pair<int,int>,pair<int,int> > parent;

  Q.push(make_pair(0,0));
  while(Q.empty()!=true){
    parNode=Q.front();
    Q.pop();
    if(checkGoal(parNode,2))
      break;
    occured[parNode]++;
    vector <pair<int,int> > leafs = newStatesSet(parNode);
    for(int i=0;i<leafs.size();i++){
      if(occured[leafs[i]]==0){
        Q.push(leafs[i]);
        parent[leafs[i]]=parNode;
      }
    }
  }
  reverseTraversal(parent,parNode,make_pair(0,0));
}

What do you think is wrong with this code, and how can it be improved? What corner case have I overlooked, if any? How can I make it less complicated?

Improvements based on suggestion can be found here

I think that the function that computes new states could be simplified in this way:

vector <pair<int,int> > newStatesSet(pair<int,int> state){
  //generating all possible states from a given initial state
  vector<pair<int,int> > newStates;

  if (state.first < 3) {
    newStates.push_back(make_pair(3, state.second));
  }
  if (state.second < 4) {
    newStates.push_back(make_pair(state.first, 4));
  }
  if (state.first > 0) {
    newStates.push_back(make_pair(0, state.second));
  }
  if (state.second > 0) {
    newStates.push_back(make_pair(state.first, 0));
  }
  if (state.second > 0 && state.first < 3) {
    newStates.push_back(make_pair(std::min(3, state.first + state.second),
                        std::max(state.second - (3 - state.first), 0)));
  }
  if (state.first > 0 && state.second < 4) {
    newStates.push_back(make_pair(std::max(state.first - (4 - state.second), 0),
                        std::min(4, state.second + state.first)));
  }

  return newStates;
}

Note that with this function a new state is generated from the previous state by considering a single operation. So, for instance, from the state (0, 0) it produces (3, 0) and (0, 4), but not (3, 4) (which will be generated later) as in your function. I think that this reflect more correctly the idea that each state is derived from the previous one with a single step.


C++ string tokenizing without streams, with certain conditions My concern is mainly the correctness of the algorithm for tokenizing purposes. The code compiles as it is called as a member function in my program. The code does pass my small amount of unit tests for parsing inputs (61 unit tests, all pass). But, I'm slightly stressed out about this part of code, and also paranoid. I could use a pair of foreign mk.I eyeballs looking at my code. This is part of a bigger project and ideally the tokenizing should be bugfree

please take a look and give thoughts.
:'(

BACKGROUND & problem parameters
objectives of my tokenizer:

cannot use stringstreams, run out of programmemory
input cppstrings will not have newline or any such limiter at the
end(will be chucked out before inputting into
tokenize_input_refactored() ), IT will be as though the input cppstring was gotten with getline()
separate cppstrings into tokens based on single spacebar (delimiter), or the end of the string in regular way ("M4 255" is tokenized into "M4", "255")
if there's no delimiter, take the entire cppstring as token
backToBack delimiters is immediately invalid input, like " " twoSpacebars in row (or more spacebars in row)
cannot begin or end with spacebar(delimiter), immediately invalid input
too large string ( 31 or more true chars) is illegal input
empty string is illegal input
the "garbage strings" that are pushed into the vector "illegalcommand" are later on used for comparison with known words
for parsing purposes. Just to be sure that command won't be parsed as
valid. (tokens amount as well as tokens themselves are checked against expected values)

Possible solution as C++ member-function:

tokensVec is just an empty vector datamember of Gcodeparser object. It will be cleared empty after each round of tokenizing (or parsing, the same same effect).

bool GcodeParser::tokenize_input_refactored(const std::string & rawInput) {

    /*find delimiter == singleSpacebar*/
    auto res = rawInput.find(delimiter);

    /*find if there was twoOrMoreDelimiters backToBack "  " twospacebars */
    string backToBackDelim;
    backToBackDelim += delimiter;
    backToBackDelim += delimiter;
    auto res2 = rawInput.find(backToBackDelim);

    int searchInd = 0;
    int pos = 0;
    int size = rawInput.length();
    bool returnableResult = false;

    /*if the input was too large => reject as illegal*/
    if (size > maxCharAmount) {
        tokensVec.push_back("INVALID_COMMAND!");
        return false;
    }
    /*check if empty string*/
    if (size == 0) {
        tokensVec.push_back("INVALID_COMMAND!");
        return false;
    }

    /*found 2delim backtoback*/
    if (res2 != string::npos) {
        tokensVec.push_back("INVALID_COMMAND!");
        return false;
    }

    /*input ended in delimiter or started with delimiter*/
    if (rawInput[0] == delimiter || rawInput[size-1] == delimiter) {
        tokensVec.push_back("INVALID_COMMAND!");
        return false;
    }

    /*delimiter wasnt found => take complete word into vec for later parsing*/
    if (res == string::npos) {
        tokensVec.push_back(rawInput);
        return true;
    }
    else {
        while (pos != string::npos) {
            pos = rawInput.find(delimiter, searchInd);
            if (pos != string::npos) {
                tokensVec.push_back(rawInput.substr(searchInd, pos - searchInd));
                searchInd = pos + 1;
            }
        }
        /*take the last valid token*/
        tokensVec.push_back( rawInput.substr(searchInd, size - searchInd) );
        return true;
    }
}

Well, it seems to do what you want. But, we can do better.

Review:

Avoid dynamic allocation where possible.

string backToBackDelim;
backToBackDelim += delimiter;
backToBackDelim += delimiter;
auto res2 = rawInput.find(backToBackDelim)

becomes

const char backToBackDelim[] = {delimiter, delimiter};
auto res2 = rawInput.find(backToBackDelim, 0, 2)

Don't pass std::string by constant reference unless you depend on the terminator. Pass a view by value instead, specifically C++17 std::string_view. It is more flexible and tends to be more efficient.
An empty line directly after the prototype is curious. Either remove it, or move the opening brace { for the function there.
Define your variables in the smallest scope you can, and invest in good names. What is res / res2, and why do you initialize it all the way up there, and only use it the once down here?
You know what, eliminate it completely, it's bloat.
Choosing good names should also be applied to the function itself. Yours seems to be a placeholder.
Consider marking things const to ease comprehension. No Need to search for possible sites of modification then. That is, if having that extra-variable actually pays for itself with clarity.
You have a bunch of conditions under which you error out. The curious Thing is that you manually duplicated the handling block. Consider merging the conditions so you only need it once.
Don't use else if control-flow doesn't resume after the code-block anyway. Less indentation is better.
You know .substr() will adjust the second argument, if provided, to not leave the string? So, don't try so hard in your last call.

Re-Design:

Mull over your spec. And now try to simplify it. Doing so leads to a simpler, more efficient design, which is optimized for legal input:

Too large or empty => illegal.
Tokenize.
Empty token => illegal.

Rewrite:

bool GcodeParser::tokenize_input_refactored(std::string_view s) {
    // Consider adding here: tokensVec.clear();
    if (auto pos = size(s); pos && pos <= maxCharAmount) {
        while ((pos = s.find(delimiter))) {
            tokensVec.push_back(s.substr(0, pos));
            if (pos == std::string_view::npos)
                return true;
            s = s.substr(pos + 1);
        }
        tokensVec.clear();
    }
    tokensVec.push_back("INVALID_COMMAND!");
    return false;
}


Bash to find all placeholder variables in a file and replace will real variables I am looking to make a generic config file using variables such as:
$PLACEHOLDER_USER

Then after moving the file into the correct position I want to replace every instance of *$PLACEHOLDER_VARIABLE* into the value of $VARIABLE from my settings.sh file.

In the example below *"$PLACEHOLDER_USER"* in log.conf would be replace by "tom"

log.conf

deploy = $PLACEHOLDER_USER

settings.sh

USER="tom"

deploy.sh

FILE="/home/logging/log.conf"
ln -s /home/log.conf $FILE
while IFS== read variable value
do
  sed -i "s/\$PLACEHOLDER_$variable/$value/g" $FILE
done < settings.sh

Is there a better way of doing this?

First up, the shell variable USER is already defined in bash, and is the log-in name of the current user.... so, if you log in as 'rolfl' then echo $USER will print rolfl. There are a number of programs and other systems that may be confused if you change the USER environment variable the way you have.

Next, I don't think that the symbolic link you create does what you think it does:

ln -s /home/log.conf $FILE

This will try to create a symbolic link at /home/logging/log.conf that points to the (presumably pre-existing) file /home/log.conf

This means that whenever anyone processes your settings script, they all modify the same file /home/log.conf and, since the first person will replace the tokens with their name, the other users will have nothing to do....

I presume the line:

ln -s /home/log.conf $FILE

should be

cp /home/log.conf $FILE

Apart from this though, the program seems reasonable (I have not run it). My only comment is that this uses relatively advanced features of the shell. In my experience, it is often better to write these things in perl because people expect perl to do more complicated things. I don't know if this is sounding right, but, even though perl may be slower than this solution, it is also something that sys admins are familiar with, and will be happier to maintain than a complex shell script.... Still, if the people maintaining this code are familiar with what you are doing (and the fact that you are asking on CodeReview rather than using your peers is a suggestion that is not the case) then I would say it is fine.


A counting sort function in C I've tried to implement the counting sort algorithm in C.

What efficiencies can you spot?
I squint at the end when running through the original array again to assign it the sorted array. Maybe there is a better way?

/**
* Sorts array of n values.
* Using counting sort O(n)
*/
void sort(int values[], int n)
{
  //set our known max length, initialize our placer array and then our sorted array
  int maxLength = 65536;
  int placer[maxLength];
  int sorted[n];

  //so we can know which indexes in our placer array are in values array
  for ( int i = 0; i < n; i++ ) {
    placer[ values[i] ] += 1;
  }

  //store the sum of previous elements in each element at each index because this will give us the sorted position
  for (int i = 0; i < maxLength; i++ ) {
    placer[ i + 1 ] += placer[i];
  }

  //fill the sorted array with the values elements using the corresponding placer index
  for ( int i = 0; i < n; i++ ) {
    sorted[ placer[ values[i] ] - 1 ] = values[i];
    placer [ values[i] ] -= 1;

  }

  //set the unsorted values array to the sorted array
  for (int i = 0; i < n; i++) {
    values[i] = sorted[i];
  }
}

Off by one error

This loop goes over by 1 and writes past the last array element:

  for (int i = 0; i < maxLength; i++ ) {
    placer[ i + 1 ] += placer[i];
  }

You could either stop the loop at maxLength - 1, or rewrite the loop to start at 1 like this:

  for (int i = 1; i < maxLength; i++ ) {
    placer[i] += placer[i-1];
  }

Uninitialized variable causes crash

When I ran your program, it crashed due to an uninitialized variable, here:

int placer[maxLength];

Later on, you do placer[values[i]] += 1 without ever having cleared the placer array to zeros, so you can get garbage values in the array.  You can fix this by clearing the array like this:

memset(placer, 0, sizeof(placer));

However, I recommend not using variable length arrays.  One reason is that they are only optionally supported in the latest C standard.  The other reason is that sometimes you can overflow your stack if you allocate a large variable length array.  I would use calloc() here to allocate the counting array.

Simplify the function

It looks like you are using a version of counting sort that is used for radix sort, which needs to move elements around in the array.  For a simple counting sort, you don't need to do that.  After the counting pass, you can just fill in the original array with values from the counts, like this:

// Uses counting sort to sort an array which contains values in the
// range [0..65535].  The counting array is allocated using calloc() in
// order to avoid putting a large array on the stack.
void sort(int values[], int n)
{
    const int maxLength = 65536;
    int      *counts    = calloc(maxLength, sizeof(*counts));

    if (counts == NULL) {
        return;
    }

    for (int i = 0; i < n; i++) {
        counts[values[i]]++;
    }

    int outIndex = 0;
    for (int i = 0; i < maxLength; i++) {
        for (int j = 0; j < counts[i]; j++) {
            values[outIndex++] = i;
        }
    }

    free(counts);
}


Modularized program to find circumference, diameter, and area from a circle's radius I have to create a modularized program that can find out the diameter, circumference, and area of a circle's radius. I'm sure many of you can notice I kind of winged this from a example given from my teacher. Instead of people pointing out the fact that I coded this wrong, could you please give me reasons for what I should do so I can better understand this concept?

import math

def main():
    Radius = 0
    Diameter = 0
    Circumference = 0
    Area = 0

    Radius = GetRadius(Radius)
    Diameter = SetDiameter(Radius, Diameter)
    Circumference = SetCircumference(Radius, Circumference)
    Area = SetArea(Radius, Area)
    ShowResults(Radius, Diameter, Circumference, Area)

def GetRadius(myradius):
    myradius = float(str(input("Enter your radius: ")))
    return myradius

def SetDiameter(myradius, mydiameter):
    mydiameter = myradius * 2
    return mydiameter

def SetCircumference(myradius, mycircumference):
    PIE = 3.14159
    mycircumference = 2 * PIE * myradius
    return mycircumference

def SetArea(myradius, myarea):
    PIE = 3.14159
    myarea = PIE * myradius * myradius
    return myarea

def ShowResults(Radius, Diameter, Circumference, Area):
    print("The Diameter is",mydiameter)
    print("The Circumference is",mycircumference)
    print("The Area is",myarea)

main()

Many little comments here to make things simpler,shorter,more pythonic :

PEP 0008 gives some coding guidelines for python. Among other things, it gives a naming convention. Also, you don't need the "my" prefix everywhere.
You can use math.pi instead of repeating our own definition of pi everywhere. (Isn't it the reason why your import math in the first place)
Most of your function just need to return a value and updating the parameter probably shouldn't be their responsability.
Most of the variables are not really required.
there's no point to define a main function in your case.

After taking these comments into accounts, your code looks like this :

import math

def get_radius_from_user():
    return float(input("Enter your radius: "))

def get_diameter(radius):
    return radius * 2

def get_circumference(radius):
    return 2 * math.pi * radius

def get_area(radius):
    return math.pi * radius * radius

def show_results(radius):
    print("The Diameter is",get_diameter(radius))
    print("The Circumference is",get_circumference(radius))
    print("The Area is",get_area(radius))

show_results(get_radius_from_user())


Reinterpreting a string, treating < as a backspace character I've come up with a solution to this coding challenge using recursion. In summary, the input is a string where the < symbol represents a backspace (up to 1 million characters). The output should be a string where all characters that come before a backspace or series of backspaces are removed, as well as the backspaces themselves. E.g.:

"a<bc<"        --> "b"
"foss<<rritun" --> "forritun"

Here is my solution to the problem, in Java:

import java.util.*;

public class Main {

    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);
        String broken_str = scanner.nextLine();
        System.out.println(fix(broken_str));
    }

    public static String fix(String broken_str) {
        int brack_idx = broken_str.indexOf('<'); // index of the first occurrence of '<'
        if (brack_idx < 0) {
            // if '<' isn't found then the string is fixed
            return broken_str;
        }

        int num_brax = 0; // keeps track of the number of brackets in the current sequence
        while (brack_idx < broken_str.length() && broken_str.charAt(brack_idx) == '<') {
            ++num_brax; ++brack_idx;
        }

        return broken_str.substring(0, brack_idx - 2 * num_brax) // this substring ranges up to the first character that should be removed
                + fix(broken_str.substring(brack_idx)); // this substring is from the index after the last occurrence of '<' until the end of the string
    }

}

This algorithm passes for the first three test cases, but the fourth one results in a runtime error. I've tried to create test cases of my own, and when the string is really long, I get a stack overflow. I was wondering if the algorithm can be optimized.

By the way, I know that this can be done very easily iteratively, but that's kind of boring. If this can only be made efficient iteratively then let me know.

Recursion is a great tool to have available when dealing with some problems, but each recursive call carries a cost. The cost depends on the language being used, and perhaps the OS architecture. As a consequence, the usefulness of recursion is limited by the circumstances in which the recursion is applied.

In Java each function call typically costs a number of kilobytes of memory as that's the size of the stack frame (and each call requires a new stack frame). Also, in Java, the stack size (memory) is pre-allocated as part of the VM settings (see -Xss commandline option) Other languages (for example, Go) have a much cheaper mechanism for stack management, and thus the cost of recursion is reduced. Java typically cannot go further than a few thousand calls deep in the stack, Go can go millions of calls deep. In your use case, the depth of the stack is proportional to the length of the input string, and strings longer than a few thousand characters will cause an out-of-memory problem.

Bottom line, is that recursion is not the solution to use for your problem. Even in other more stack-friendly languages I would still avoid recursion.

Solve it iteratively, using a StringBuilder, a Reader, and an if-statement...

public static String fix(Reader reader) {
    int ch;
    StringBuilder result = new StringBuilder();
    while ((ch = reader.read()) >= 0) {
        if (ch == '<') {
            result.setLength(result.getLength() - 1);
        } else {
            result.append((char)ch);
        }
    }
    return result.toString();
}


Looping through Radgrid in JS is slow, can this be faster? I do a single select in one radgrid, and based upon that selection I want to select multiple rows in a different radgrid:

<telerik:RadGrid ID="rgPaymentLines"> <ClientEvents OnRowSelected="RowPaymentSelected"/> ... </telerik:RadGrid>

<telerik:RadCodeBlock ID="RadCodeBlock1" runat="server">
    <script type="text/javascript">

        function RowPaymentSelected(sender, eventArgs) {
            var grid = $find("<%=rgPaymentLines.ClientID %>");
            var MasterTablePayment = grid.get_masterTableView();
            var paymentRow = MasterTablePayment.get_selectedItems()[0];
            var paymentCell = MasterTablePayment.getCellByColumnUniqueName(paymentRow, "DossierID")
            selectDossierRow(paymentCell.innerHTML);
        }
        function selectDossierRow(dossierID) {
            var gridDossier = $find("<%=rgDossiersAdmin.ClientID %>");
            var MasterTableDossier = gridDossier.get_masterTableView();
            var rows = MasterTableDossier.get_dataItems();

            MasterTableDossier.clearSelectedItems();
            for (var i = 0; i < rows.length; i++) {
                if (MasterTableDossier.getCellByColumnUniqueName(rows[i], "columnDossierID").innerHTML == dossierID) {
                    MasterTableDossier.selectItem(i);
                }
            }
        }

welcome to Code Review and thank you for your question. You need to profile the code to see for yourself where it is slow. Firebug and Chrome can do this. Only then will you be able to optimize your code.

Side note: Be careful about variable names, some of them seem to be in French (dossier).


Simple web application for benchmark There is an article where the author tries to compare performance of different web stacks. The result looks pretty controversial.
I try to prove that in real use case the difference between scores of different stacks will be not so significant(under "real" I mean something which relies on database) .

For this purpose I take two stacks: one from the top side(node+express) and the looser(python+flask). And now I'm trying to implement a little bit more realistic use case for both platforms.

This is the code for the flask application:

#app.py
from flask import render_template
from flask import Flask
import sqlite3

conn = sqlite3.connect('data.db')
app = Flask(__name__)
c = conn.cursor()
if c.execute("SELECT name FROM sqlite_master WHERE type='table' AND name='data'").fetchone() is None:
    c.execute("CREATE TABLE data (value TEXT)")
    conn.commit()
conn.close()

@app.route('/<int:number>')
def index(number=1):
    conn = sqlite3.connect('data.db')
    c = conn.cursor()
    c.executemany("INSERT INTO data VALUES (?)",   [(fib(number),)] * 10);
    list = c.execute("select * from data limit 100").fetchall()
    conn.commit()
    conn.close()
    return render_template('template.html', list=list)

def fib(n):
    if n == 0: return 0
    elif n == 1: return 1
    else:
        return fib(n - 1) + fib(n - 2)

if __name__ == '__main__':
    app.run(debug=False)

#template.html

{% for l in list %}

{{ l[0] }}

{% endfor %}

And the code for node application:

// app.js
var express = require("express");
var jade = require('jade');
var db = new (require('sqlite3').verbose()).Database('data.db');
var app = express();

var fib = function (n) {
  if (n === 0) {
    return 0;
  } else if (n == 1) {
    return 1;
  } else {
    return fib(n - 1) + fib(n - 2)
  }
};

var getApp = function () {
    app.get("/:number", function (req, res) {
      var number = req.param("number");
      query = db.prepare("INSERT INTO data VALUES (?)")
      for(var i=0; i<10; i++) {
        query.run((fib(number)))
      };
      db.all("select * from data limit 100", function(err, rows){
        var rendered = jade.renderFile('template.jade', {list: rows})
        res.send(rendered);
      });
    });
    app.listen(3000);
}

db.get(
  "SELECT name FROM sqlite_master WHERE type='table' AND name='data'",
  function (err, row) {
    if(err !== null) {
      throw new Error();
    }
    if(row === undefined) {
      db.run('create table data (value text)', getApp)
    } else {
      getApp()
    }
  }
);

# template.jade
ul
    each l in list
        li=l.value

Questions

Some advice about node code. How can I make it more idiomatic and beautiful?
There is some strange behavior in the node app. Benchmark results proposed below should describe it. What I'm doing wrong?

I would be glad to achieve any piece of advice about improving these code samples.

Running 30s test @ http://localhost:3000/10
2 threads and 64 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    26.19s     4.89s   28.93s    77.14%
    Req/Sec     0.87      1.46     4.00     86.67%
  64 requests in 30.01s, 81.25KB read
  Socket errors: connect 0, read 0, write 0, timeout 896

Since the Fibonacci algorithm is so nice and clean there is one little nitpicky thing to add. You don't need to directly return 0 and 1 in the base cases, you can just return n. So the function can be rewritten (in Javascript, my experience with Python is extremely subpar) as:

var fib = function(n) {
    if(n === 0 || n === 1) {
        return n;
    }

    return fib(n - 1) + fib(n - 2);
}

The algorithm is beautiful but it also does a lot of extraneous work when run in a loop, that can be removed with the technique of memoization (again, to keep these tests fair, I think (?) this is possible in Python as well but someone else could be of more use in helping with the code). In this specific case, the technique is incredibly easy to employ with a simple Javascript object as a cache

var fib = (function() {
    var cache = {};

    function fibonacci(n) {
        if(n === 0 || n === 1) {
             return n;
        }

        return fibonacci(n - 1) + fibonacci(n - 2);
    }

    return function(n) {
       if(n in cache) {
           return cache[n];
       }

       return fibonacci(n);
    }
}());


Printing all possible sums of two n-sided dice rolls For my CS class, we were tasked with writing a program that calculates all the possible results from rolling two n-sided dice.  For example, the input of 4 would print [2, 3, 3, 4, 4, 4, 5, 5, 5, 5, 6, 6, 6, 7, 7, 8].  Or, if the input was 3, it would print [2, 3, 3, 4, 4, 4, 5, 5, 6].

I was given enough test cases that I noticed the pattern that the number range was [2,n] and the test cases had a pattern where the pattern was always could be represented by printing the number in [2,2n+1] a number in the set [1,n] U (n,1] a corresponding number of times.  For example, for 4, the set would be [1,2,3,4,3,2,1] and the output would be [(one 2), (two 3s), (three 4), (four 5s), (three 6s), (two 7s), and (one 8).  This explanation may be horrible, but see the code and you will understand.

The code:

def allRolls(sides):
    nums = list(range(1,sides+1)) + reverseIt(list(range(1,sides)))
    index = sides-1
    #the return array
    retVal = []
    #The index to look for in the list of nuts
    cIndex = 0
    for i in range(2,sides*2+1):
        for b in range(nums[cIndex]):
            retVal.append(i)
        cIndex+=1
    retVal.sort()
    return retVal

#Reverses the list
def reverseIt(liste):
    newb =[]
    index = len(liste)-1
    while index>=0:
        newb.append(liste[index])
        index= index-1
    return newb

The issue:  everyone in my class got it in many fewer lines of code.  What did I do wrong?

Code test cases:

Input: 2
Output:[2, 3, 3, 4]

Input: 3
Output:[2, 3, 3, 4, 4, 4, 5, 5, 6]

Input: 4
Output: [2, 3, 3, 4, 4, 4, 5, 5, 5, 5, 6, 6, 6, 7, 7, 8]

As @BenC mostly put a code dump, I'll explain ways that your code could have improved.

reverseIt

Programming this is just wrong in Python.
This is as there are two ways to do this.

>>> list(reversed([1, 2, 3]))
[3, 2, 1]
>>> [1, 2, 3][::-1]
[3, 2, 1]

As an alternate way to this you could just calculate the range backwards off the bat.

>>> list(range(10, 0, -1))
[10, 9, 8, 7, 6, 5, 4, 3, 2, 1]

In short you could halve your code just by removing this.

If you wanted to keep it, then you would want to use the reversed range.
And do pretty much what you are now. To get:

def reverseIt(indexable):
    lst = []
    for index in range(len(indexable) - 1, -1, -1):
        lst.append(indexable[index])
    return lst

You could then use a list comprehension, if amount of lines dictates how good your code is.

def reverseIt(indexable):
    return [indexable[index] for index in range(len(indexable) - 1, -1, -1))]

allRolls

nums is pointless if you use a different algorithm. (Which is also more intuitive.)
index is never used.
retVal can be changed to a comprehension.
Manually counting cIndex is a frowned upon in Python.

In short, use a different algorithm.
The pseudo-code for this could be:

fn allRolls(sides):
    list = List()
    for dice1-face in dice-faces:
        for dice2-face in dice-faces:
            add dice1-face + dice2-face to list
    return sorted list

Or in Python:

fn allRoles(sides):
    ret = []
    for a in range(1, sides + 1):
        for b in range(1, sides + 1):
            ret.append(a + b)
    ret.sort()
    return ret

If you take it the next step then you would get @BenC's answer.

Misc

In short, you just used the wrong algorithm.

And in Python you should:

Use all_rolls instead of allRoles to follow function and variable naming conventions.
Have a space on both sides of operators, index >= 0 or newb = [] to increase readability.


Rest Controller in Spring Boot for Rock Paper Scissors REST API I have created a Spring Boot Microservice for playing Rock Paper Scissrors. I have tried to follow best practices, but still would appreciate some critique on my code. Thanks.

@RestController
public class RockPaperScissorsController {

  @Autowired
  private PlayerService playerService;

  @Autowired
  private HttpServletRequest context;

  @Autowired
  private GameSessionService gameSessionService;

  @Autowired
  private GameplayService gameplayService;

  public RockPaperScissorsController(PlayerService playerService,
      GameSessionService gameSessionService) {
    this.playerService = playerService;
    this.gameSessionService = gameSessionService;
  }

  @GetMapping(value = "/ping", produces = "application/json")
  public ResponseEntity<String> ping() {
    return ResponseEntity.ok("{\"response\":\"pong\"}");
  }

  @GetMapping(value = "/player/{playerName}", produces = "application/json")
  public ResponseEntity player(@PathVariable("playerName") String playerName) {
    try {
      Player player = playerService.getPlayer(playerName);
      return ResponseEntity.ok(player);
    } catch (RPSException e) {
      return ResponseEntity.badRequest().body(e.getMessage());
    }
  }

  @PostMapping(value = "/player/{playerName}", produces = "application/json")
  public ResponseEntity playerPOST(@PathVariable("playerName") String playerName) {
    try {
      playerService.createPlayer(playerName);
      return ResponseEntity.ok().body("");
    } catch (RPSException e) {
      return ResponseEntity.badRequest().body(e.getMessage());
    }
  }

  @DeleteMapping(value = "/player/{playerName}", produces = "application/json")
  public ResponseEntity playerDELETE(@PathVariable("playerName") String playerName) {
    try {
      playerService.deletePlayer(playerName);
      return ResponseEntity.ok("");
    } catch (RPSException e) {
      return ResponseEntity.badRequest().body(e.getMessage());
    }
  }

  @PostMapping(value = "/createInvite/{playerName}", produces = "application/json")
  public ResponseEntity createInvite(@PathVariable("playerName") String inviter) {
    try {
      Player player = playerService.getPlayer(inviter);
      GameSession session = gameSessionService.createSessionFrom(new Invite(player));
      return ResponseEntity.ok(session);
    } catch (RPSException e) {
      return ResponseEntity.badRequest().body(e.getMessage());
    }
  }

  @PostMapping(value = "/acceptInvite/{inviteCode}/{playerName}", produces = "application/json")
  public ResponseEntity acceptInvite(@PathVariable("inviteCode") String inviteCode,
      @PathVariable("playerName") String playerName) throws InvalidOperationException {
    try {
      Player player = playerService.getPlayer(playerName);
      return ResponseEntity.ok(gameSessionService.acceptInvite(player, inviteCode));
    } catch (RPSException e) {
      return ResponseEntity.badRequest().body(e.getMessage());
    }
  }

  @GetMapping(value = "/session/{inviteCode}", produces = "application/json")
  public ResponseEntity session(@PathVariable("inviteCode") String inviteCode) {
    return ResponseEntity.ok(gameSessionService.sessions().get(inviteCode));
  }

  @PostMapping(value = "/readyplayer/{playername}", produces = "application/json")
  public ResponseEntity ready(@PathVariable("playername") String playerName) {
    try {
      Player player = playerService.changePlayerState(playerName, Player.State.READY);
      return ResponseEntity.ok(player);
    } catch (RPSException e) {
      return ResponseEntity.badRequest().body(e.getMessage());
    }
  }

  @PostMapping(value = "/play", produces = "application/json")
  public ResponseEntity play(@RequestBody PlayRequest playRequest) {
    try {
      gameplayService.play(playRequest);
      return ResponseEntity.ok().body("");
    } catch (RPSException e) {
      e.printStackTrace();
    }
    return ResponseEntity.status(HttpStatus.NOT_IMPLEMENTED).body("");
  }

}

You do not need ping within your controller. Check out Spring boot actuator
Each method having duplicated error handling. You can move it to @ControllerAdvice
Method names like playerPOST, playerDELETE aren't helpful. Consider this as a normal class and have normal method names like createPlayer, deletePlayer
You need not define produces attribute for each mapping, RestController defaults to JSON
Returning ok("") doesn't make sense. Should be either returning the player object, or use ResponseEntity.created() (201)
Why are you autowiring HttpServletRequest context?
Should use AllArgsConstructor - constructor based autowiring


Synthetic division calculator I've written a python program do perform synthetic division on a polynomial. There are a few constraints (listed in the module docstring of the program) on the polynomial, mainly because I haven't had the time to work out all the edge cases. I would like feedback on everything possible, as I plan to use this to solve other polynomials. All feedback is welcome, appreciated, and considered!

Constraints

  Coefficients must only be one digit, if present

  Constant must be one digit, and must be present (even 0)

  Constant must be positive

synthetic_division.py

"""

SYNTHETIC DIVISION CALCULATOR

This program accepts a polynomial and returns all the
x intercepts.

Program has some constraints:
    - Coefficients must only be one digit, if present
    - Constant must be one digit, and must be present (even 0)
    - Constant must be positive

EX:
    Input: x^3 -4x^2 +x +6
    Output: X = [3, -1, 2]

"""

from functools import reduce

def get_coefficients(equation):
    """

    Returns the coefficients of the passed polynomial. If no coefficent, then 1 is returned.
    Only works for single digit coefficents, if present.

    :param equation: The equation to be analyzed

    """
    parts = equation.split()
    coeffs = []
    for part in parts:
        if part[0] == "x":
            coeffs.append(1)
        if part[0] == "-":
            coeffs.append(int(part[1]) * -1)
        if part[0] == "+" and "x" in part:
            if len(part) == 2:
                coeffs.append(1)
            else:
                coeffs.append(int(part[1]))
    return coeffs

def get_factors(equation):
    """
    Returns a list of factors of the constant

    :param n: Number to get factors from

    """

    constant = int(equation[len(equation) - 1])

    ###########################################
    """
    ===========================================
    Equation from StackOverflow user @agf
    https://stackoverflow.com/a/6800214/8968906
    ===========================================
    """
    factors = list(
        set(
            reduce(
                list.__add__, (
                    [i, constant//i] for i in range(1, int(constant**0.5) + 1) if constant % i == 0
                )
            )
        )
    )
    ###########################################

    #Add negatives
    length = len(factors)
    for i in range(length):
        factors.append(-factors[i])
    return factors

def synthetic_division(coefficients, factors):
    """
    Performs synthetic division with the passed coefficients and factors
    Returns a list of intercepts

    :param coefficients: Coefficients to use in the calculation
    :param factors: Factors to test

    """

    coeffs = coefficients
    facs = factors
    ints = []

    for fac in facs:
        current_sum = 0
        for coeff in coeffs:
            current_sum += coeff
            current_sum *= fac
        if current_sum == 0:
            ints.append(fac)

    return ints

def main(equation):
    """

    Gathers the coefficients, factors and intercepts from the equation

    :param equation: The polynomial to be solved

    """
    coefficients = get_coefficients(equation)
    constant = int(equation[len(equation) - 1])
    coefficients.append(constant)

    factors = get_factors(equation)
    intercepts = synthetic_division(coefficients, factors)

    return intercepts

if __name__ == '__main__':
    EQUATION = "x^3 -4x^2 +1x +6"
    print(f"X = {main(EQUATION)}")

Currently some of your code is very specialized to the exact use case you have. For example, the get_factors function takes the unparsed equation and needs to extract the constant itself. It would be a lot better if it just took a number and the parsing happens elsewhere. This way if you improve the parsing (to include multi-digit numbers, etc), you don't need to change the get_factors function.

At the same time, that function is borderline unreadable. It is also not the most efficient. Consider this alternative, which is shorter in lines of code, IMO way easier to read, and about two times faster:

def get_factors_of_constant(equation):
    c = int(equation[-1])
    factors = set()
    for i in range(1, int(c**0.5) + 1):
        if c % i == 0:
            factors.update([i, c // i, -i, -c // i])
    return list(factors)

For a better parsing, consider the output of the following regex:

import re

equation = 'x^3 -4x^2 +1x +6'
print(re.findall(r'\s?([\+\-]?\s?\d*)?(x)?\^?(\d*)?', equation))
# [('', 'x', '3'), ('-4', 'x', '2'), ('+1', 'x', ''), ('+6', '', ''), ('', '', '')]

It produces tuples of coefficients, whether or not there is an x and exponents. If the coefficient is empty, a 1 is assumed, if the exponent is empty, it is either a 1 or a 0, depending on if there is an x, and if all three are empty, you can skip it.

def parse(equation):
    coefficients, exponents = [], []
    matches = re.findall(r'\s?([\+\-]?\s?\d*)?(x)?\^?(\d*)?', equation)
    for coefficient, x, exponent in matches:
        if coefficient == x == exponent == "":
            continue
        coefficients.append(coefficient.replace(" ", "") or "1")
        exponents.append(exponent or "1" if x else "0")
    coefficients = list(map(int, coefficients))
    exponents = list(map(int, exponents))
    return coefficients, exponents

print(parse(equation))
# ([1, -4, 1, 6], [3, 2, 1, 0])

This can also parse multi-digits and deals both with missing parts (since you have the info on the exponent) and additional whitespace between number and sign:

parse("x^3 - 10x + 2")
# ([1, -10, 2], [3, 1, 0])
parse("x")
# ([1], [1])

It is also not perfect, for example it fails in this case:

parse("-x^2")
# ValueError: invalid literal for int() with base 10: '-'

You would need to add some special cases to the function for it to cover all edge cases, but it should be a good starting point.


Procedural style Rock Paper Scissors game using Python 3 I am a newbie to Python and am trying to do the Rock, Paper and Scissors game to revise my knowledge that I have earned so far.

Overview of the program:

This is a two-player game
Ask for player plays (including name, their choice: rock or paper or scissors using input)
Print out a message of winner
Ask if the user want to start a new game

This code is written in procedural style.

Can anyone show me some weaknesses in my code and how I can improve on them?

def beats(a, b):
    tup = ('rock', 'paper', 'scissors')
    if (a == tup[1] and b == tup[2]) or (a == tup[2] and b == tup[1]):
        return tup[2]
    elif (a == tup[1] and b == tup[0]) or (a == tup[0] and b == tup[1]):
        return tup[1]
    elif (a == tup[0] and b == tup[2]) or (a == tup[2] and b == tup[0]):
        return tup[0]

def players_input():
    name = input('What is your name?: ')
    while True:
        decision = input('Paper, Rock, or Scissors?: ')
        if decision == 'rock' or decision == 'paper' or decision == 'scissors':
            break
        else:
            print("Invalid choice! Try again!")
            continue
    return '{} chooses {}'.format(name, decision)

def play():
    player1 = players_input()
    player2 = players_input()
    print('{}\n{}'.format(player1, player2))
    name1, verb, decision1 = player1.split(" ")
    name2, verb, decision2 = player2.split(" ")
    returnvalue = beats(decision1.lower(), decision2.lower())
    if decision1.lower() == returnvalue and decision2.lower() != returnvalue:
        print('{} wins'.format(name1))
    elif decision2.lower() == returnvalue and decision1.lower() != returnvalue:
        print('{} wins'.format(name2))
    else:
        print('It\'s a tie')

def play_again():
    while True:
        choice = input('Do you want to continue?(Y/N): ')
        if choice.upper() == "Y":
            play()
        elif choice.upper() == "N":
            print("Game over")
            break
        else:
            print("Invalid Input! Try again!")

play()
play_again()

I recommend simplifying:

if decision == 'rock' or decision == 'paper' or decision == 'scissors':

to:

if decision.lower() in ('rock', 'paper', 'scissors'):

This will check whether the decision exists in a given tuple.

This helps simplifying the idea "If a variable contains one of these values".

Another example is "If variable x contains either 1, 3, or 5." Instead of coding it like this:

if x == 5 or x == 3 or x == 1

It can be simplified into:

if x in (1, 3, 5)

See this Gist to see my suggestion as a whole.


NRPE script for monitoring memory usage This script based on previous NRPE script for monitoring load average. Same purpouse - get NRPE friendly output with minimal usage of non built in commands. In this case wasn't need to count with floating point. So I could omit bc.

Yes, I read Why is using a shell loop to process text considered bad practice. And there is a lot of true. Bud I am still thinking about these NRPE scripts as about bash scripting trainnig to get more practice.

Desired output:

MEMORY_TOTAL=2041316kB MEMORY_AVAILABLE=1049260kB | MEMORY_IN_PERCENTAGE=51;80;90

#!/usr/bin/env bash

set -o errexit -o pipefail

warning=80
critical=90

while read -a meminfo_row; do
  # Remove the colon character.
  row_value_lenght=$(( ${#meminfo_row[0]} -1 ))
  row_value=${meminfo_row[0]:0:$row_value_lenght}

  case $row_value in
    MemTotal)
      mem_total=${meminfo_row[1]}
      mem_apercent=$(( meminfo_row[1] / 100 ))
    ;;
    MemAvailable)
      mem_available=${meminfo_row[1]}
    ;;
  esac
done < /proc/meminfo

mem_percentage=$(( mem_available / mem_apercent ))

if [[ -z $mem_percentage ]]; then
  returned_text="MEMORY UNKNOWN - check script"
  returned_code=3
else
  returned_text="MEMORY_TOTAL=${mem_total}kB MEMORY_AVAILABLE=${mem_available}kB | MEMORY_IN_PERCENTAGE=$mem_percentage;$warning;$critical"
  if [[ $mem_percentage -gt $critical ]]; then
    returned_code=2
  elif [[ $mem_percentage -gt $warning ]]; then
    returned_code=1
  else
    returned_code=0
  fi
fi

echo "$returned_text"
exit $returned_code

I don't understand why it should be a warning to have over 80% of the system RAM available.  If anything needs a warning, it should be having under 20% of RAM available, right?

Reading each line into an array is overcomplicated.  The read builtin command is quite good at splitting each line of input into multiple variables.  (row_value_lenght is misspelled, by the way.)

The proc(5) man page says about /proc/meminfo:

  Each line of the file consists of a parameter name, followed by a colon, the value of the parameter, and an option unit of measurement (e.g., "kB").

There's no guarantee that the unit will be kB (as you have assumed in your printout), or that both lines of interest will have the same units.

Splitting up the calculation of mem_percentage is weird.

The echo statement should be line-wrapped for readability.

The cases for exit codes 0, 1 and 2 can be collapsed into one, using arithmetic.

#!/usr/bin/env bash

set -o errexit -o pipefail

warning=80
critical=90

while read -r label number unit ; do
    case "$label" in
      MemTotal:)
        mem_total=$number
        mem_total_unit=$unit
        ;;
      MemAvailable:)
        mem_avail=$number
        mem_avail_unit=$unit
        ;;
    esac
done < /proc/meminfo

if [ -z "$mem_avail" -o "$mem_avail_unit" != "$mem_total_unit" ]; then
    echo "MEMORY UNKNWOWN - check script"
    exit 3
fi

mem_avail_percent=$(( 100 * $mem_avail / $mem_total ))
echo "MEMORY_TOTAL=$mem_total$mem_total_unit" \
     "MEMORY_AVAILABLE=$mem_avail$mem_avail_unit" \
     "|" \
     "MEMORY_IN_PERCENTAGE=$mem_avail_percent;$warning;$critical"
exit $(( ($mem_avail_percent > $warning) + ($mem_avail_percent > $critical) ))


Event-listener implementation While writing a C++ GUI application more or less from scratch I needed some form of an event-listener system, preferably using lambdas. An event should be able to have multiple listeners and the user should not have to worry about the lifetime of the objects. I came up with this bit using shared and weak pointers to determine object lifetime:

#include <functional>
#include <list>

template <typename ... Args> struct event:public std::shared_ptr<std::list<std::function<void(Args...)>>>{

  using handler = std::function<void(Args...)>;
  using listener_list = std::list<handler>;

  struct listener{
    std::weak_ptr<listener_list> the_event;
    typename listener_list::iterator it;

    listener(){ }

    listener(event & s,handler f){
      observe(s,f);
    }

    listener(listener &&other){
      the_event = other.the_event;
      it = other.it;
      other.the_event.reset();
    }

    listener(const listener &other) = delete;

    listener & operator=(const listener &other) = delete;

    listener & operator=(listener &&other){
      reset();
      the_event = other.the_event;
      it = other.it;
      other.the_event.reset();
      return *this;
    }

    void observe(event & s,handler f){
      reset();
      the_event = s;
      it = s->insert(s->end(),f);
    }

    void reset(){
      if(!the_event.expired()) the_event.lock()->erase(it);
      the_event.reset();
    }

    ~listener(){ reset(); }
  };

  event():std::shared_ptr<listener_list>(std::make_shared<listener_list>()){ }
  event(const event &) = delete;
  event & operator=(const event &) = delete;

  void notify(Args... args){
    for(auto &f:**this) f(args...);
  }

  listener connect(handler h){
    return listener(*this,h);
  }

};

Example usage:

#include <iostream>

using click_event = event<float,float>;

struct gui_element{
  click_event click;
  void mouse_down(float x,float y){ click.notify(x, y); }
};

int main(int argc, char **argv) {
  gui_element A,B;
  click_event::listener listener_1,listener_2;

  listener_1.observe(A.click,[](float x,float y){ std::cout << "l1 : A was clicked at " << x << ", " << y << std::endl; });
  listener_2.observe(B.click,[](float x,float y){ std::cout << "l2 : B was clicked at " << x << ", " << y << std::endl; });

  {
  auto temporary_listener = A.click.connect([](float x,float y){ std::cout << "tmp: A was clicked at " << x << ", " << y << std::endl; });
  // A has two listeners, B has one listeners
  A.mouse_down(1, 0);
  B.mouse_down(0, 1);
  }

  listener_2 = std::move(listener_1);

  // A has one listener, B has no listeners
  A.mouse_down(2, 0);
  B.mouse_down(0, 2);
}

Output:

l1 : A was clicked at 1, 0
tmp: A was clicked at 1, 0
l2 : B was clicked at 0, 1
l1 : A was clicked at 2, 0

While the usage is exactly how I wanted, I am not sure the implementation is elegant or optimal. Any way how to improve this?

As I said in the comments, your system is pretty close to a system of signals and slots. Congratulations if you never heard of the pattern before, that's an excellent way to implement an the observer design pattern! I still have few notes:

I don't know which compiler you use, but mine won't compile your code unless I include <memory> for std::shared_ptr. I suppose that it may be transitively included from <functional> or <list> in your implementation. Never rely on transitive includes from the standard library and always include the exact header that contains what you need.
Qt, which is the emblematic library when it comes to signals and slots, tends to use past participle for its signas names. For example, instead of click, it would be clicked:

button.clicked.connect(/* whatever */);

It allows to read the line easily as "when button is clicked, do whatever". Also, it makes the object look more like a trigger and less like an action.
From a design point of view, connect being a method taking only one function, I would expect it to add the function to the list, not to return a listener.
You should encapsulate std::shared_ptr<std::list<std::function<void(Args...)>>> instead of inheriting from it. Frankly, you don't want people to expose every method of std::shared_ptr. You don't want people to think of pointers when they want events, right?

The truth is my answer isn't really personal. Since your class obviously looks like a signal class, everything I am saying tends to mean "make your design closer to those of existing signal classes like Boost.Signals2" :/


Writing multiple if tags that compare two variables I am a beginner in C# coding, and I was trying to compare two int variables:

void CompareNumber() {
    int oneNumber;
    int secondNumber;

    if (oneNumber > secondNumber)
        {
        DoSomething();
        }

    else if (oneNumber < secondnumber)
        {
        DoSomethingElse();
        }

    else if (oneNumber == secondnumber)
         {
         DoSomethingDifferent();
         }
}

While this does work, it looks kinda messy, especially because I compare variables in this manner many times. Is there a more concise way of doing this, making it look neater? (Except for omitting the curly brackets)

Here is my actual code for a simple game that thinks of a number, and you need to guess what it said:

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace GuessName
    {
    class Program
        {
        static void Main(string[] args)
            {
            int previousNumber = 0;
            AskNumber(previousNumber);
            }

        static void AskNumber(int previousNumber)
            {
            Console.Clear();

            int numberTyped;
            int randomNumber = new Random().Next(1, 11);

            while (previousNumber == randomNumber)
                randomNumber = new Random().Next(1, 11);

            Console.WriteLine("I am thinking of a number between 1 and 10. What do you think it is?");

            if (int.TryParse(Console.ReadLine(), out numberTyped) == true)
                CheckNumber(numberTyped, randomNumber);
            else
                InvalidNumber();
            }

        static void CheckNumber(int numberTyped, int randomNumber)
            {
            if (numberTyped < 0 || numberTyped > 10)
                {
                InvalidNumber();
                }
            else if (numberTyped > randomNumber)
                {
                Console.Clear();
                Console.WriteLine("Sorry, but your number is bigger than what I thought");
                Console.WriteLine("Try again, just type what you think:");
                }
            else if (numberTyped < randomNumber)
                {
                Console.Clear();
                Console.WriteLine("Sorry, but your number is smaller than what I thought");
                Console.WriteLine("Try again, just type what you think:");
                }
            else
                {
                Console.Clear();
                WonGame(randomNumber);
                }
            if (int.TryParse(Console.ReadLine(), out numberTyped) == true)
                CheckNumber(numberTyped, randomNumber);
            else
                InvalidNumber();
            }

        static void WonGame(int randomNumber)
            {
            Console.WriteLine("Great job, You did it!");
            Console.WriteLine("Would you like to try again? (y) or would you like to quit? (n)");

            string wantToPlay = Console.ReadLine();
            if (wantToPlay == "y")
                AskNumber(randomNumber);
            else
                Environment.Exit(0);
            }

        static void InvalidNumber()
            {
            Console.Clear();

            Console.WriteLine("Your number is invalid. Would you like to try again? (y)");
            Console.WriteLine("Or would you like to quit? (n)");

            string wantToPlay = Console.ReadLine();
            if (wantToPlay == "y")
                AskNumber(0);
            else
                Environment.Exit(0);
            }
        }
    }

Indentation?
At first I thought it was a mere copy/paste error, but the pattern is throughout the code you submitted, so I have to mention it: it's not the braces you indent, it's what the braces encompass!

    static void Main(string[] args)
        {
        int previousNumber = 0;
        AskNumber(previousNumber);
        }

Should be:
    static void Main(string[] args)
    {
        int previousNumber = 0;
        AskNumber(previousNumber);
    }

Curly braces define a scope. In C#, convention is to place the scope-opening brace on the next line, so this:

void CompareNumber() {

Should be:
void CompareNumber()
{

Speaking of braces:

Is there a more concise way of doing this, making it look neater? (Except for omitting the curly brackets)

Omitting the curly braces does not make code look neater. Proof being this very confusing snippet:

        if (wantToPlay == "y")
            AskNumber(0);
        else
            Environment.Exit(0);
        }

I had to scan the entire method twice to figure out that the last } was in fact closing the scope of the method.
By opposition:
        if (wantToPlay == "y")
        {
            AskNumber(0);
        }
        else
        {
            Environment.Exit(0);
        }
    }

Is brutally in-your-face crystal-clear. Notice the position of the last brace, which closes the method scope. The readability problem stems from the extraneous indentation of scope-delimiting braces - this doesn't suffer the exact same issue:
        if (wantToPlay == "y")
            AskNumber(0);
        else
            Environment.Exit(0);
    }

You should always delimit scopes with braces. if/else statements denote a scope - that scope wants its braces.
Same with loop constructs:

        while (previousNumber == randomNumber)
            randomNumber = new Random().Next(1, 11);

Should be:
        while (previousNumber == randomNumber)
        {
            randomNumber = new Random().Next(1, 11);
        }

The Good
You use descriptive naming. This is often understated - descriptive names are your best friend. They reduce bug-proneness of your code all by themselves, and make code easier and more enjoyable to read. Since programming is 80% reading and 20% writing, enjoyable reading means enjoyable programming.
Good job!

The Bad
The syntax for an if condition goes if ([bool-expression]), where [bool-expression] is any expression that evaluates to true or false. Can you spot the redundancy here?

if (int.TryParse(Console.ReadLine(), out numberTyped) == true)

That's right. TryParse returns a bool that's a perfectly valid [bool-expression] - in fact, anytime you're comparing a Boolean value (or expression) to a Boolean constant for the sake of getting a Boolean expression, you are needlessly repeating yourself... redundantly.
Just do this instead:
if (int.TryParse(Console.ReadLine(), out numberTyped))

Method names should start with a verb. Always. void InvalidNumber() is a bad name, because it doesn't say anything about what it actually does - it doesn't return a number, and doesn't intake one either. Without looking at how it's implemented, it's pretty confusing.

Console applications exit when the Main method executes to its closing brace. I have never used Environment.Exit(0) in any application I've written - in fact, this call is like a "red button", a forceful, ugly way to kill your application (and throw the body in a container in the back alley) rather than letting it gracefully exit through the front door.
You're going to have to restructure things up in order to make that happen though.

CheckNumber shouldn't have the 0 and 10 hard-coded like this; there should be constants defined for the lower and upper bounds, and the AskNumber method should be using them as well - that way if you ever need to change the 10 to a 100, there's only 1 place you'll need to change it.
AskNumber shouldn't be creating a new Random() every time. There should be one single instance of Random that the program uses whenever it needs a random number.
There's a lot more to say about this code, I'll let other reviewers chip in ;)


Ranges of enum members that dwim Inspired by this blogpost I decided to try and make it possible to have ranges of enum members that dwim, aka. that contain the actual members and not a stringified form.

This is the result. I'm looking for comments and wether I overlook a pitfall. Also, maybe EnumMember subset can be expressed in a more performant way?

use Test;

subset EnumMember of Any where .HOW.^name eq "Perl6::Metamodel::EnumHOW";

multi sub infix:<..>( EnumMember $a, EnumMember $b )
{
    die "Can only do ranges for members of the same enum" unless $a.WHAT === $b.WHAT;
    $a.WHAT.^enum_value_list.grep({ $_ === $a fff $_ === $b })
}

enum Foo ( XA => 17, XB => 19, XC => 2, XD => 17, XE => 1, XF => 99 );
enum Goo < YA YB >;

is-deeply( ( XC..XE ).Array, [XC, XD, XE] );
dies-ok({ XA..YB });

Probably, you could do something similar by ... and succ.

enum Foo ( XA => 17, XB => 19, XC => 2, XD => 17, XE => 1, XF => 99 );
enum Goo < YA YB >;
XC, *.succ ... XE andthen .say

Thus your example could be rewritten

use Test;

subset EnumMember of Enumeration;

multi sub infix:<..>( EnumMember:D ::T $a, T:D $b ) {
    $a, *.succ ... $b
}

multi sub infix:<..>( EnumMember $a,  $b ) {
    die 'Can only do ranges for members of the same enum'
}

enum Foo ( XA => 17, XB => 19, XC => 2, XD => 17, XE => 1, XF => 99 );
enum Goo < YA YB >;

is-deeply( ( XC..XE ).Array, [XC, XD, XE] );
dies-ok({ XA..YB });

But you get Seq, not Range. So I prefer redefine the sequence operator ... and make a new role instead of a subtype.

use Test;

role EnumMember {};

multi sub infix:<...>( EnumMember:D ::T $a, T:D $b ) {
    $a, *.succ ... $b
}

multi sub infix:<...>( EnumMember $a,  $b ) {
    fail 'Can only do sequence for members of the same enum'
}

multi sub infix:<…>( EnumMember $a,  $b ) {
  $a ... $b
}

enum Foo does EnumMember ( XA => 17, XB => 19, XC => 2, XD => 17, XE => 1, XF => 99 );
enum Goo does EnumMember < YA YB >;

is (XC … XE), (XC, XD, XE);
dies-ok { XA … YB };


Separate combined words in a sentence and convert them into Pig Latin My program is complete and working, but I would like second opinions on it.

Here are the instructions for the assignment:

  The program will accept a string as input in which all of the words are run together, but the first character of each word is uppercase.
  Convert the string to a string in which the words are separated by
  spaces and only the first word starts with an uppercase letter. For
  example, the string "StopAndSmellTheRoses" would be converted to "Stop
  and smell the roses".
  Then the program will convert each word in the result string of task 1 into "Pig Latin". In one version of Pig Latin, you convert a
  word by removing the first letter, placing that letter at the end of
  the word, and then appending "ay" to the word.

  For example, for the result string "Stop and smell the roses" in task
  1, the Pig Latin string should be "topSay ndaay mellsay hetay osesray".

  Requirements:

  Your program should have 3 methods: a main method, a method for task    1, and a method for task2.
  The method for task 1 should return the result string of task1.
  The method for task 2 should display the Pig Latin string.

import java.util.Scanner;
public class Assignment9
{
    public static void main(String[] args)
    {
    // Variables
    String sentence, revisedSentence, latin = " ";

    // Create a Scanner object for keyboard input
    Scanner keyboard = new Scanner(System.in);

    // Get the input string
    System.out.print("Enter sentence: ");
    sentence = keyboard.nextLine();

    //Close keyboard
    keyboard.close();

    // Task 1
    revisedSentence = WordSeparator(sentence);

    System.out.print("Revised Sentence: " +revisedSentence);

    // Task 2
    PigLatin(revisedSentence, latin);

    System.exit(0);
}

private static String WordSeparator(String sentence)
{
    // Variables
    StringBuilder str = new StringBuilder(sentence);
    int i = 1;

    // While loop repeats until the end of the sentence
    while (i < str.length())
    {
        // Checks for upper case characters
        if(Character.isUpperCase(str.charAt(i)))
        {
            //Inserting space
            str.insert(i, ' ');
            char ch = Character.toLowerCase(str.charAt(i+1));
            str.setCharAt((i+1), ch);
        }
        i++;
    }
    return str.toString();
}

private static void PigLatin(String revisedSentence, String latin)
{
    // Split sentence by spaces
    String[] tokens = revisedSentence.split(" ");

    // Store all the words into an array
    String[] arr = new String[tokens.length];

    // Convert English sentence into Pig Latin
    for (int i = 0; i < tokens.length; i++)
    {
        // Get string from array
        String str = tokens[i];

        // Get first letter from string
        String str1 = str.substring(0, 1);

        // Get substring from string
        String str2 = str.substring(1, str.length());

        // Concatenate the two strings in a required format
        str2 = str2.concat(str1);

        // Concatenate the result and "AY"
        str2 = str2.concat("ay");

        // Store all of the strings into an array
        arr[i] = str2;

        // Make a sentence with all the words
        latin = latin.concat(str2 + " ");
    }

    // Display pig latin verison
    System.out.println("\nPig Latin Version:" +latin );
    }

}

This is in addition to what @thecoder16 already said.

WordSeparator

Methods should be named using verbs instead of nouns.
They usually take some action on the input,
for example this method separates the words that were stuck together.
So separateWords would be better.

Also, this part is not great:

    // Checks for upper case characters
    if(Character.isUpperCase(str.charAt(i)))
    {
        //Inserting space
        str.insert(i, ' ');
        char ch = Character.toLowerCase(str.charAt(i+1));
        str.setCharAt((i+1), ch);
    }

Objections:

The str.charAt(i) and str.charAt(i+1) refer to the same character. This is repetitive, and can be prone to errors, as you have to remember to use i+1 the second time because of the inserted space
The comments only state the obvious. Unnecessary comments are like noise, and a waste of time to read. Let the code speak for itself.

It would be better this way:

    char ch = str.charAt(i);
    if (Character.isUpperCase(ch)) {
        str.insert(i, ' ');
        str.setCharAt(i + 1, Character.toLowerCase(ch));
    }

PigLatin

The biggest issue here is the latin = latin.concat(...) call for two reasons:

String concatenation is inefficient. You should use a StringBuilder like you did in your other method.
Reusing input parameters is not a good practice, as it can be confusing and error prone. It would be better to name local variables differently.

Issues similar to WordSeparator:

The method is poorly named. Perhaps toPigLatin would be better.
The comments are unnecessary noise

Other smaller issues:

Instead of printing output, it would be better to return the result
The String[] arr is unused, so it would be better to remove it
The counting for loop can be converted to an enhanced loop, for example: for (String str : tokens) { ... }
The input parameter revisedSentence carries information that's unnecessary for the functioning of this method. Just sentence would be enough.


Java-like listeners in C++ I'm coming from languages like Java and Python and I'm completely new to C++. Therefore, my knowledge is somewhat limited.
My goal is to implement a listener in C++ myself and I came up with a solution using function pointers and lambdas.
The design is different from other questions that I have seen on this site so I hope it's not a duplicate.
My question: Is my implementation optimal or is it good from a C++ point of view? If there are any optimizations or any better ways please let me know. (Below code is just an example of my design)
class IntegerListener {
public :

    void (*number_changed)(int old_number, int new_number);

    void (*number_increased)(int increased_value);

    //deleting default constructor to make users to define number_changed and number_increased
    IntegerListener() = delete;

    IntegerListener(void (*&&number_changed)(int, int), void (*&&number_increased)(int)) {
        this->number_changed = number_changed;
        this->number_increased = number_increased;
    }

};

class Integer {

private:
    int number;
    std::vector<IntegerListener *> integer_listeners;
public:

    explicit Integer(int num) : number(num) {}

    void set_number(int new_number) {
        for (IntegerListener *&integer_listener:integer_listeners) {
            (*integer_listener).number_changed(number, new_number);
        }
        if (new_number > number) {
            for (IntegerListener *&integer_listener:integer_listeners) {
                (*integer_listener).number_increased(new_number - number);
            }
        }
        number = new_number;
    }

    void add_number_changed_listener(IntegerListener &&integer_listener) {
        integer_listeners.push_back(&integer_listener);
    }
};

and in order to add some listener my idea is something like below:
void test() {
    Integer integer(1);

    // from a java background it represents the java way the best, in my opinion.
    integer.add_number_changed_listener(IntegerListener(
            [](int oldNum, int newNum) {
                std::cout << "changed(from 1listener): (" << oldNum << ") -> (" << newNum << ")" << std::endl;
            },
            [](int increasedValue) {
                std::cout << "increased by(from 1listener): (" << increasedValue << ")" << std::endl;
            }));

    integer.add_number_changed_listener(IntegerListener(
            [](int oldNum, int newNum) {
                std::cout << "changed(from 2listener): (" << oldNum << ") -> (" << newNum << ")" << std::endl;
            },
            [](int increasedValue) {
                std::cout << "increased by(from 2listener): (" << increasedValue << ")" << std::endl;
            }));
    std::cout << "first call: \n";
    integer.set_number(2);
    std::cout << "second call: \n";
    integer.set_number(1);
}

There’s actually quite a bit you can do to simplify the listener class, while at the same time making it easier and more flexible to use!
Let’s start with deleting the default constructor: you actually don’t need to do this. Classes get an automatically-generated default constructor only if there are no other constructors defined. But in your class, there is another constructor defined! So the default constructor is already deleted.
See for yourself! Try this:
// No constructors are defined, so the class gets a default constructor.
class foo {};

// A constructor is defined, so the default constructor is suppressed.
class bar
{
public:
    explicit bar(int) {}
};

// This will compile:
auto f = foo{};

// This will not:
//auto b1 = bar{};

// But of course, this will compile:
auto b2 = bar{42};

So right away, you can drop the line deleting the default constructor.
Now, this is not wrong:
void (*number_changed)(int old_number, int new_number);

void (*number_increased)(int increased_value);

That is indeed the correct way to have two data members that are function pointers.
However…
There is actually a better way:
std::function<void (int, int)> number_changed;
std::function<void (int)> number_increased;

The reason this is better is because std::function is a LOT more powerful than a function pointer. A function pointer only works for regular, free functions, class static functions, and lambdas with no captures.
But std::function works with literally anything that even remotely looks like a function. It works for all the same things a function pointer works for. But it also works for member functions, lambdas with captures (closures), general function objects, bound-argument functions, … and so on and so forth. Like, literally everything works.
For example:
class my_int_listener
{
public:
    void number_changed(int, int) { /* whatever you want */ }
    void number_increased(int) { /* whatever you want */ }

    // This class could have data members, a constructor... whatever
    // you want, and you can use all that stuff in your callbacks.
};

// Constructed however you want, wherever you want - global var, on the
// heap, whatever - just so long as it outlives integer.
auto mil = my_int_listener{};

auto integer = Integer{1};

using namespace std::placeholders;
integer.add_number_changed_listener(
    {
        std::bind(&my_int_listener::number_changed, &mil, _1),
        std::bind(&my_int_listener::number_increased, &mil, _1)
    }
);

The other benefit of std::function is the syntax is soooo much nicer. Function pointers are ghastly. Even when I’m forced to use them, I always use typedefs to hide the ugliness:
class IntegerListener {
public :
    using number_changed_function = void (*)(int, int);
    using number_increased_function = void (*)(int);

    number_changed_function number_changed = nullptr;
    number_increased_function number_increased = nullptr;

    IntegerListener(number_changed_function&& number_changed, number_increased_function&& number_increased) {
        this->number_changed = number_changed;
        this->number_increased = number_increased;
    }
};

That just looks a lot more like natural C++. And of course, with std::function:
class IntegerListener {
public :
    std::function<void (int, int)> number_changed = nullptr;
    std::function<void (int)> number_increased = nullptr;

    IntegerListener(std::function<void (int, int)>&& number_changed, std::function<void (int, int)>&& number_increased) {
        this->number_changed = number_changed;
        this->number_increased = number_increased;
    }
};

Although you could use typedefs there too, which is probably a good idea for avoiding needless repetition.
The one downside of using std::function over raw function pointers is that std::function objects are larger—sometimes quite a bit larger, in order to do the type erasure necessary to do all its tricks. But unless you want to accept the rather severe restrictions that raw function pointers come with, there’s no other option. (Well, there are other options, but they would require a redesign: you could, for example, you could use a polymorphic base class for your listeners, and use dynamic dispatch.)
Now let’s focus on that constructor:
IntegerListener(void (*&&number_changed)(int, int), void (*&&number_increased)(int)) {
    this->number_changed = number_changed;
    this->number_increased = number_increased;
}

I’m not sure why you’d want to use rvalue-references for your constructor parameters here. Did you try something and it would’t compile without them? I can’t see what purpose rvalue-referenes here serve, even theoretically.
The other thing is: when you’re initializing data members, you almost always want to use data member initializers, rather than initializing them in the constructor’s body.
This is probably more what you want:
IntegerListener(void (*number_changed)(int, int), void (*number_increased)(int)) :
    number_changed{number_changed},
    number_increased{number_increased}
{}

Or even more correctly:
IntegerListener(void (*number_changed)(int, int), void (*number_increased)(int)) :
    number_changed{std::move(number_changed)},
    number_increased{std::move(number_increased)}
{}

The moves are because there’s no need to copy the arguments from the function parameters to the data members (assuming you don’t want to use the function parameters again). Moving is more logically correct, and can be much faster (though it doesn’t matter here).
Finally, I can’t imagine any reason why this constructor couldn’t be noexcept. If all it’s doing is just taking the parameters and moving them into the data members, then so long as the moves are noexcept (which, moves should always be noexcept wherever possible, and they are both for raw function pointers and std::function), there’s no chance of failure.
So putting it altogether, if I were writing your listener class, I might do:
class IntegerListener
{
public:
    using number_changed_type = std::function<void (int, int)>;
    using number_increased_type = std::function<void (int)>;

    number_changed_type number_changed = nullptr;
    number_increased_type number_increased = nullptr;

    IntegerListener(
        number_changed_type number_changed,
        number_increased_type number_increased) noexcept :
            number_changed{std::move(number_changed)},
            number_increased{std::move(number_increased)}
    {}
};

Now on to the dispatcher….
std::vector<IntegerListener *> integer_listeners;

I don’t see any reason why you’d want to store pointers to the listeners, rather than just storing the listeners directly. You’re not doing dynamic dispatch (IntegerListener doesn’t have a virtual destructor), and there is no functional difference between storing the objects directly in the vector or storing them remotely. Using pointers for this just means poor performance due to the extra indirection and cache busting, and the need to manage memory, with no benefits.
In fact, as I pointed out in my other comment, what you’re doing is actually just storing the address of temporary objects, which immediately die, leaving you with dangling pointers. If you actually wanted to store pointers, you would have to either manually allocate those pointers (or use global or static objects, I guess)—which means you also have to manually manage that memory with custom copy constructors and a destructor—or (and, really, this is the only option in modern C++… do NOT try manually allocating and managing memory), use smart pointers.
But, as I said, there’s no benefit to using pointers at all, so there’s nothing to be gained from using smart pointers. Just store the objects in the vector directly. It’ll be faster, easier to code, and you won’t end up with critical bugs involving dangling pointers like the one you have (or leaks or double-frees or any of the other crap that comes with manual memory management… seriously do not do manual memory management in C++).
void set_number(int new_number) {
    for (IntegerListener *&integer_listener:integer_listeners) {
        (*integer_listener).number_changed(number, new_number);
    }
    if (new_number > number) {
        for (IntegerListener *&integer_listener:integer_listeners) {
            (*integer_listener).number_increased(new_number - number);
        }
    }
    number = new_number;
}

If you’re going to store pointers in your vectors, there’s really no reason to iterate through them by reference. In fact, you’re just paying for a double indirection which you don’t even use.
But! If you’re going to store the actual listeners in your vectors, then, yes, you should take them by reference… but you should really take them by const reference, because you’re not going to be modifying them.
I generally recommend to use the universal form of range-for. This:
for (auto&& item : range)

always works, for everything, and never does the wrong thing. It should be the default thing you write whenever you write a range-for.
(And no, it is NOT the same as doing for (IntegerListener&& item : range). That usually won’t work… and it won’t work for your case. It’s auto&& or it’s a craps shoot. Only auto&& always works.)
However… if you want to be more explicit about what your code is actually doing, you could do:
for (auto& item : range) // to signal you will be modifying the items
// or
for (auto const& item : range) // if you'll just be reading them

These don’t always work, but they almost always work, and if you really mean what they say—that you’re either modifying the items or you’re definitely not and you’re just inspecting them—then they pretty much always work.
So I’d probably write:
for (auto const& integer_listener : integer_listeners)
    integer_listener.number_changed(number, new_number);

unless I was feeling lazy, in which case I’d use the auto&& form.
void add_number_changed_listener(IntegerListener &&integer_listener) {
    integer_listeners.push_back(&integer_listener);
}

This is really the only broken function in the code. Everything else is correct, though maybe not the most efficient, and maybe not following C++ best practices or styles.
But this function has two problems. First, it takes the argument by rvalue reference. I can’t imagine why you’d want to do that. It doesn’t really make any sense. What you’re saying with that is that you only want to take temporary-constructed integer listeners. In other words, you’re saying:
// this is fine:
integer.add_number_changed_listener(IntegerListener{f1, f2});

// but this is absolutely wrong, and should not compile:
auto il = IntegerListener{f1, f2};
integer.add_number_changed_listener(il);

But why? What’s wrong with making an integer listener and passing it to two integers? Maybe I have two integers that I want to keep track of changes to. Why am I banned from using the same listener for both?
The second problem with this function is that you only take the address of the listener. Even if you weren’t requiring temporary-constructed listeners, that’s an accident waiting to happen, because even if the listener isn’t temporary—which means it’s very likely to die almost immediately—it still might not live long enough to still be active when the dispatcher wants to signal an event.
You have two options here:

either you mandate that users keep listener objects around for as long as needed, and make it their problem to keep track of that (which would be a terrible, bug-prone design); or
you simply keep the listener object with the dispatcher, so it has to stick around as long as the dispatch object exists.

In other words, don’t muck around with pointers and the address-of operator ('&'). Just do the natural thing:
void add_number_changed_listener(IntegerListener integer_listener) {
    integer_listeners.push_back(integer_listener);
}

And even better, and more technically correct: you don’t really want a copy of the listener stored in the vector… you want the actual listener object. So you want to move what you were given into the array:
void add_number_changed_listener(IntegerListener integer_listener) {
    integer_listeners.push_back(std::move(integer_listener));
}

Now as a user of the class, I have control. I get to decide if I want to give you a temporary-constructed listener, or if I want to copy a listener to multiple integers. And no matter what I choose, everything just works.
That’s it for the line-by-line review! Overall, the only “problem” with it that would make me reject it from a code base I was responsible for is that the vector integer_listeners holds pointers rather than actual objects, and those pointers are almost certainly going to be dangling because add_number_changed_listener() only takes temporaries. If you only made two changes:

change integer_listeners to be a std::vector<IntegerListener> (not std::vector<IntegerListener*>); and
change add_number_changed_listener() to store (by copying or—better—moving) the passed listener, not just its address

then I would consider the code acceptable. (Though, of course, there are a number of other improvements you could make, like using std::function rather than raw function pointers`.)
To answer your concerns:

The design is pretty optimal, yes. There are a few minor touch-ups here and there that make it even more efficient, but the basic design is sound. And, actually, it’s a pretty good design, in my opinion.
Is the design good C++? Yes, I think so. Even without the changes I suggested (other than fixing those critical bugs), I’d say it fits well with good C++ philosophy. There are other ways of doing it, of course, and every design has its pros and cons. But this design is, I think, quite good. I’d use it.


Categorising a number in JavaScript I've written a function that takes a number and returns a value depending on where that number lies on a pre-defined scale. That's probably a little confusing so I've put the code below and added a JSFiddle (although you might be better off in the dev console).

var getSize = function(number){
        var sizes = {
            100 : 'url_t',
            240 : 'url_s',
            320 : 'url_n',
            500 : 'url_m',
            640 : 'url_z',
            800 : 'url_c',
            1024 : 'url_l',
        },
        result = sizes[100];

    for (var size in sizes){
        if(number < size){
            result = sizes[size];
            break;
        }
        result = sizes[1024];
    }
    return result;
};
getSize(10) // returns url_t
getSize(130) // returns url_s
getSize(2000) // returns url_l
getSize(600) // returns url_z

It works and it's fairly performant, it's just a bit icky. I've tried to come up with a better solution but it just feels ickier. Is there anything I'm missing that would make this a bit nicer? I was playing about with putting the sizes in an array so they're a little more JSON-ish, but then I got lost.

You might not need to loop through an object, and a simple switch statement would suffice.

function getSize(number) {
    switch (true) {
        case number < 100: return "url_t";
        case number < 240: return "url_s";
        case number < 320: return "url_n";
        case number < 500: return "url_m";
        case number < 640: return "url_z";
        case number < 800: return "url_c";
        default:           return "url_l";
    }
}

As a side note, I've seem people use switch (true) before when they want to use some logic in their case's instead of just hard coded values. Kind of a nice alternative to an if-else construct, and would probably perform better than looping over an object using a for-in loop.

You could still keep the sizes in an object:

var sizes = {
    100 : 'url_t',
    240 : 'url_s',
    320 : 'url_n',
    500 : 'url_m',
    640 : 'url_z',
    800 : 'url_c',
    1024 : 'url_l'
};

function getSize(number) {
    switch (true) {
        case number < 100: return sizes[100];
        case number < 240: return sizes[240];
        case number < 320: return sizes[320];
        case number < 500: return sizes[500];
        case number < 640: return sizes[640];
        case number < 800: return sizes[800];
        default:           return sizes[1024];
    }
}

Edit: A problem arises if you need to change the sizes for your application. That's why I'm leaning towards the first function, which just a switch statement and hard coded values.


Safely getting the next element of a list I'm looking for general review and improvements on this code that safely gets the next element of a list (and wraps to the first element if you exceed the list). It just seems a little excessive for how simple of a task it is.
It should also be noted that this function will only work for lists that do not contain duplicate elements. Unless I modify it to take index numbers instead, I can't see that restriction changing. If length list < 2, it will return Nothing.

nextElem :: Eq e => e -> [e] -> Maybe e
nextElem e xs = nextElem' e (shiftList e xs)
    where
        shiftList e xs = let (ya, yb) = break (== e) xs in yb ++ ya
        nextElem' _ []      = Nothing
        nextElem' _ (x : [])= Nothing
        nextElem' e (x : xs)
            | x == e    = Just (head xs)
            | otherwise = nextElem' e xs

It looks like you are trying to do the same thing in two different ways.

To detect the case where e is not in xs, you should check whether yb is empty.  (If that's what you were trying to do with nextElem' _ (x : [])= Nothing then you should note that it doesn't do that.)  I'm not sure what the purpose of the otherwise case is.  If break didn't find any element equal to e, recursion won't find it, either.  Therefore, a simpler solution would be

nextElem :: Eq a => a -> [a] -> Maybe a
nextElem e xs
  | null match = Nothing
  | null rest  = Just (head xs)
  | otherwise  = Just (head rest)
  where
    (skip, match) = break (== e) xs
    rest = tail match

What is the expected result for nextElem something [something] — a one-element  list that happens to contain the element you are looking for?  I believe you would produce Nothing.  I would have expected something instead.  If you want to preserve the original behavior, where nextElem 'a' "a" produces 'a', then you could insert

nextElem e [_] = Nothing

as a special case to this solution.