@janos already covered this a little bit, but if you want to use the built-in random(x, y) used in Processing.JS, I'd do something like this:

var numberOfDots = random(1, 6);

But, we need to do one more thing. Since the random(x, y) function in Processing.JS can return non-integer results, we need to use the floor function to round the returned value down, and then add one. This means that your calculation simply becomes this:

var numberOfDots = floor(random(1, 6)) + 1;

Now you should be able to generate random numbers from 1-6 in a cleaner, and more readable manner.

I've also noticed that you're repeating a lot of code. @Caridorc already touched on this a little bit, but I'm going to expand on it a bit further. Rather than keeping it in the mouseClicked override, you can simply put it at the top of your code like this:

fill(245, 242, 242);
rect(100, 100, 200, 200);

mouseClicked = function() {
    ...
}

This is a good thing to remember when you're doing graphics programming. If there's no need to re-draw something every frame, or click, then don't. In this case, you don't need to re-draw and fill the rectangle, so it can be moved entirely out of the mouseClicked override. In the case of this program though, I felt that the rectangle wasn't needed, so I just removed it altogether.

Expanding on the subject of code repetition, you're also repeating a lot of ellipse calls. I'd highly suggest implementing a function that automatically draws x amount of circles for you. This is what I came up with:

var drawDiceDots = function(dotPositions, dotDiameter) {
    for(var i = 0; i <= dotPositions.length - 1; ++i) {
        ellipse(dotPositions[i][0], dotPositions[i][1], dotDiameter, dotDiameter);
    }
};

To use this function, you supply a list of x-y positions in the format [[x, y], ...]. For example, in order to render the dots on the 6-dot side of a die, you'd do this:

drawDiceDots([
    [150, 150],
    [150, 250],
    [250, 250],
    [250, 150],
    [250, 200],
    [150, 200]
], 45);

Finally, rather than using a chain of if statements, I'd recommend using a switch/case construct. A typical switch/case construct looks something like this:

switch(someVariable) {
    case someValue:
        ...
        break;

    ...
}

But, we can get better than a chain of if statements. We can either use a JavaScript object or a list. In this case, I'm going to use a list. To do this, you'd first create a list that looks like this:

var possibleDiceFaces = [
    [100, [200, 200]],
    [70, [150, 150], [250, 250]],
    [60, [150, 150], [200, 200], [250, 250]],
    [60, [150, 150], [150, 250], [250, 250],[250, 150]],
    [60, [150, 150], [150, 250], [250, 250], [250, 150], [200, 200]],
    [45, [150, 150], [150, 250], [250, 250], [250, 150], [250, 200], [150, 200]]
];

The first value of each list is the diameter of the circles, and the rest are positions. The nice thing about this is that it's easily expandable, rather than having to add a new case or if statement each time you want to add a "side".

In addition, thanks to the tips from @Jonah, I'd also recommend the following things:

Separate responsibilities, e.g, simply call a dice drawing function in the mouseClick override, rather than having a huge pile of case statements.
Use more domain-oriented names for things, rather than implementation-oriented.

After making all these changes, plus a few more, I ended up with the following, improved (hopefully) code:

/**
 * Draw x amount of dots given a list of positions
 * for a dice.
 * @param {list[][]} dotPositions - A list of dot positions in the form [x, y].
 * @param {number}   dotDiameter  - The diameter of the dots.
 */
var drawDiceDots = function(dotPositions, dotDiameter) {
    for(var i = 1; i <= dotPositions.length - 1; ++i) {
        fill(0, 0, 0);
        ellipse(dotPositions[i][0], dotPositions[i][1], dotDiameter, dotDiameter);
    }
};

/**
 * Draw the fact of a dice. By default, there are 6 different faces,
 * but this number can be changed to add more.
 */
var drawDiceFace = function(rolledNumber) {
    var possibleDiceFaces = [
        [100, [200, 200]],
        [70, [150, 150], [250, 250]],
        [60, [150, 150], [200, 200], [250, 250]],
        [60, [150, 150], [150, 250], [250, 250],[250, 150]],
        [60, [150, 150], [150, 250], [250, 250], [250, 150], [200, 200]],
        [45, [150, 150], [150, 250], [250, 250], [250, 150], [250, 200], [150, 200]]
    ];

    if(rolledNumber in possibleDiceFaces) {
        drawDiceDots(possibleDiceFaces[rolledNumber], possibleDiceFaces[rolledNumber][0]);
    }
    else {
        throw "The rolled number specified is not valid.";
    }
};

mouseClicked = function() {
    background(255, 255, 255);
    var rolledNumber = floor(random(1, 6)) + 1;
    drawDiceFace(rolledNumber);
};

Finally, I'd just like to nitpick a few things:

Generally you should write braces in JavaScript on the same line as the declaration, due to evil things like automatic semicolon insertion.
You should properly indent everything, including comments.
Don't use comments when your code is self-explanatory.

Other than that, nice work! :)


Printing lines in a file, filtering by the first column I am setting up a project which involves a lot of data analysis, each data point represented by a line in a large text file. In order to test for certain types of data, I added a tag to each line (It needs to remain in one file, though). Now I want to conveniently choose which type of data I want to include, currently I have five types (positive, negative, neutral, objective, objective-OR-neutral). Coming from C, I wrote my parse(filename) function the following way:

# parsing.py
# flags
POS = 1
NEG = 2
NEU = 4
OBJ = 8
OON = 16

# 'flag transformation dictionary'
ftd = {u'positive': POS,
       u'negative': NEG,
       u'neutral': NEU,
       u'objective': OBJ,
       u'objective-OR-neutral': OON}

# Read flagged data from file, return only entries with the right flag.
# Each line is: <flag><\t><content>
def parse(f_loc, flags=0):
    for line in open(f_loc):
        flag, content = line.strip().split('\t')
        if ftd[flag] & flags:  # this wouldn't be as pretty with kwargs
            yield content

Using this utility function somewhere else would then look like this:

# parsing_test.py
from parsing import parse, POS, NEG, NEU, OBJ, OON

# print all data flagged as 'positive', 'negative', or 'neutral'
for entry in parse('file.txt', POS | NEG | NEU):
    print(entry)

# print all data, no matter the flag
for entry in parse('file.txt', POS | NEG | NEU | OBJ | OON):
    print(entry)

I am just starting this project and am very conscious about good style (especially intuitiveness, extendability, robustness, and speed), so I would appreciate any criticism.

For one thing, in a bitmasking expression it reads much more naturally if you put the "variable" on the left and the "constant mask" on the right. In the same way that we write if x == 0 instead of if 0 == x, we also prefer to write if x & mask rather than if mask & x.

def parse(f_loc, flags=0):
    for line in open(f_loc):
        flag, content = line.strip().split('\t')
        if flags & ftd.get(flag, 0):
            yield content

However, your bitflag identifiers POS, NEU, OON, etc., are pretty much unreadable, especially given that you already have English names for them right there in the source file! If I were you, I'd replace the entire function with simply

def parse(f_loc, flags):
    for line in open(f_loc):
        flag, content = line.strip().split('\t')
        if flag in flags:
            yield content

and then call it as

# print all data flagged as 'positive', 'negative', or 'neutral'
flags = ['positive', 'negative', 'neutral']
for entry in parse('file.txt', flags):
    print(entry)

# print all data, no matter the flag
flags = ['positive', 'negative', 'neutral', 'objective', 'objective-OR-neutral']
for entry in parse('file.txt', flags):
    print(entry)

At this point the code is simple enough that you don't even really need the parse function anymore!

If you're concerned about efficiency, and you have the power to change your input file format, consider switching to single-character flags and fixed-width fields so that you don't need to strip or split your lines.

def parse(f_loc, flags):
    for line in open(f_loc):
        if line[0] in flags:
            yield line[:1].rstrip()

# print all data flagged as 'positive', 'negative', or 'neutral'
for entry in parse('file.txt', "+-n"):
    print(entry)

# print all data, no matter the flag
for entry in parse('file.txt', "+-noO"):
    print(entry)

One last thing: IMHO

from parsing import parse
... parse('file.txt', flags) ...

is generally inferior to

import parsing
... parsing.parse('file.txt', flags) ...

This way, when I see the call site, my first question isn't "what is parse?" (could be a local variable, a function in this file, etc. etc.); instead, my first question is "what is parsing?" (which I generally know off the top of my head because it's the name of a module, and it's easy to remember all the modules I'm using). Plus, if I ever need to use any other functions out of parsing, they're all at my fingertips:

from parsing import parse, NEU  # change
... parse('file.txt', NEU) ...  # change

is inferior to

import parsing                  # no change needed
... parsing.parse('file.txt', parsing.NEU) ...  # change


Assignment function as alternative to std::tie What do you think about this assignment "idiom"?
template <typename ...A>
constexpr auto assign(A& ...a) noexcept
{
  return [&](auto&& ...v)
//  noexcept(noexcept(((a = std::forward<decltype(v)>(v)), ...)))
    {
      ((a = std::forward<decltype(v)>(v)), ...);
    };
}

It seems a better alternative to me than the std::tie() idiom. Uncommenting the comment could cause a segfault in gcc.
Usage:
int a, b, c;
assign(a, b, c)(1, 2, 3);

This "idiom" seems strictly more confusing than the std::tie(a,b,c) = std::make_tuple(1,2,3) idiom. (I mean, the latter is an idiom because multiple people use it and know it. Your thing doesn't count as an idiom yet because nobody uses or understands it.)
Your thing does save a lot of template instantiations — you don't have to instantiate std::tuple at all, for example. That's potentially nice for people who are doing lots of multiple assignments in header files and care about compile times. But who's doing lots of multiple assignments in header files??
Your thing doesn't fix the most important shortcoming of std::tie, which is that you can't use it to introduce a new variable. It's still fundamentally a std::cin-style API, where the caller is supposed to define all their variables with garbage values and then use this API to mutate them into the desired state.
// To mutate three variables:

int a, b, c;
assign(a, b, c)(1, 2, 3);
std::tie(a, b, c) = std::tuple{1, 2, 3};  // same deal, easier to read, harder to compile

// To define three new variables (ish):

auto [a, b, c] = std::tuple{1, 2, 3};

"C++17 structured binding that also includes an existing variable" gives the skeleton of an API that could be used to define and mutate different sets of variables at the same time:
// To define one new variable and mutate two others:

auto [a] = AndTie(_1, b, c) = std::tuple{1, 2, 3};

But again, I wouldn't describe this as an "idiom", because if you showed it to an ordinary C++ programmer who knew all the C++ idioms, they likely still wouldn't understand it at a glance.

Oh, and a code-style nit: I would prefer to see your fold-expression with spaces around the , operator, to emphasize that it's an operator and not a separator. Like this:
((a = std::forward<decltype(v)>(v)) , ...);

and in fact as long as we're saving on function template instantiations, let's do
((a = static_cast<decltype(v)>(v)) , ...);

The inner set of parentheses is redundant, I think, but I'm keeping them. (Precisely because I had to say "I think." I'm not even gonna go find out. They're a good idea.)


Find minimum element in a rotated array Here's an implementation of finding the minimum element in a rotated array using binary search (one that is sorted in ascending order, and all elements are distinct).

Please let me know of any improvements that can be made to it.

import java.util.List;

public class RotatedArray {
    /**
     * Finds and returns the index at which the smallest value in a rotated array is located.
     *
     * @param rotatedArray    a rotated array that's sorted in ascending order (all elements are distinct)
     * @return the index of the smallest value, or -1 if rotatedArray is empty
     */
    public int findReset(List<Integer> rotatedArray) {
        if (rotatedArray.isEmpty()) {
            return -1;
        }

        // when array is already sorted
        if (rotatedArray.size() == 1 || rotatedArray.get(0) < rotatedArray.get(rotatedArray.size() - 1)) {
            return 0;
        }

        int midpoint = rotatedArray.size() / 2;

        // when left side is monotonically increasing
        if (rotatedArray.get(0) < rotatedArray.get(midpoint)) {
            return findReset(rotatedArray.subList(midpoint + 1, rotatedArray.size())) + midpoint + 1;
        }

        // Prevents stack overflow error from occurring (e.g. test case [1,2,3,4,0])
        if (rotatedArray.size() == 2) {
            return midpoint;
        }

        // when right side is monotonically increasing
        return findReset(rotatedArray.subList(0, midpoint + 1));
    }

}

The method should be static.
The method should throw NoSuchElementException when the list is empty.
I prefer to index by [left, right) instead of using subList
Choose a good pivot location, and you don't need the size() == 2.

In code:

    public static int findReset(List<Integer> rotatedArray) {
        if (rotatedArray.isEmpty()) {
            throw new NoSuchElementException("No reset for an empty list");
        }
        return findReset0(rotatedArray, 0, rotatedArray.size());
    }
    private static int findReset0(List<Integer> rotatedArray, int left, int right) {
        // when array is already sorted
        if(left + 1 == right || rotatedArray.get(left) < rotatedArray.get(right - 1))
            return left;
        int middle = (left + right) / 2;
        if (rotatedArray.get(left) <= rotatedArray.get(middle - 1)) {
            // when left side is monotonically increasing
            return findReset0(rotatedArray, middle, right);
        } else {
            // when right side is monotonically increasing
            return findReset0(rotatedArray, left, middle);
        }
    }

Here is a version with manual tail recursion optimization:

    public static int findReset(List<Integer> rotatedArray) {
        if (rotatedArray.isEmpty()) {
            throw new NoSuchElementException("No reset for an empty list");
        }
        int left = 0, right = rotatedArray.size();
        while(left + 1 < right && rotatedArray.get(left) >= rotatedArray.get(right - 1)) {
            int middle = (left + right) / 2;
            if (rotatedArray.get(left) <= rotatedArray.get(middle - 1)) {
                // when left side is monotonically increasing
                left = middle;
            } else {
                // when right side is monotonically increasing
                right = middle;
            }
        }
        return left;
    }


Optimizing very simple piece of “Game of Life” code by taking advantage of NumPy's functionality Here is the code as it stands right now:

import numpy as np
from matplotlib import pyplot as plt
from matplotlib import animation
from random import randint

arraySize = 50
Z = np.array([[randint(0, 1) for x in range(arraySize)] for y in range(arraySize)])

def computeNeighbours(Z):
    rows, cols = len(Z), len(Z[0])
    N = np.zeros(np.shape(Z))

    for x in range(rows):
        for y in range(cols):
            Q = [q for q in [x-1, x, x+1] if ((q >= 0) and (q < cols))]
            R = [r for r in [y-1, y, y+1] if ((r >= 0) and (r < rows))]
            S = [Z[q][r] for q in Q for r in R if (q, r) != (x, y)]
            N[x][y] = sum(S)

    return N

def iterate(Z):
    Zprime = Z.copy()
    rows, cols = len(Zprime), len(Zprime[0])
    N = computeNeighbours(Zprime)

    for x in range(rows):
        for y in range(cols):
            if Zprime[x][y] == 1:
                if (N[x][y] < 2) or (N[x][y] > 3):
                    Zprime[x][y] = 0
            else:
                if (N[x][y] == 3):
                    Zprime[x][y] = 1

    return Zprime

fig = plt.figure()

Zs = [Z]
ims = []

for i in range(0, 100):
    im = plt.imshow(Zs[len(Zs)-1], interpolation = 'nearest', cmap='binary')
    ims.append([im])
    Zs.append(iterate(Zs[len(Zs)-1]))

ani = animation.ArtistAnimation(fig, ims, interval=250, blit=True)
plt.show()

I am interested in knowing what sequence of optimizations one would perform for this sort of an application, so that I can get a handle on how to use NumPy's power for my current project, which is simply a (perhaps) three dimensional, cellular automaton with many rules.

I'll just be cheeky and post a slightly modified version of my SO answer here.

So first things first, you want to get rid of the loops. They are slow to execute.

The first loop:

for x in range(rows):
        for y in range(cols):
            if Z[x][y] == 1:
                if (N[x][y] < 2) or (N[x][y] > 3):
                    Z[x][y] = 0
            else:
                if (N[x][y] == 3):
                    Z[x][y] = 1

could be replaced by:

set_zero_idxs = (Z==1) & ((N<2) | (N>3))
set_one_idxs = (Z!=1) & (N==3)
Z[set_zero_idxs] = 0
Z[set_one_idxs] = 1

Here you are generating boolean arrays that indicate which values satisfy your criteria and then you are setting those values to either 0 or 1.

Benchmarking this we see a significant improvement in speed (x 180):

# new version without loop
In [49]: %timeit no_loop(z,n)
1000 loops, best of 3: 177 us per loop

# original version with loop
In [50]: %timeit loop(z,n)
10 loops, best of 3: 31.2 ms per loop

The second major loop:

for x in range(rows):
        for y in range(cols):
            Q = [q for q in [x-1, x, x+1] if ((q >= 0) and (q < cols))]
            R = [r for r in [y-1, y, y+1] if ((r >= 0) and (r < rows))]
            S = [Z[q][r] for q in Q for r in R if (q, r) != (x, y)]
            N[x][y] = sum(S)

could be replaced by:

N = np.roll(Z,1,axis=1) + np.roll(Z,-1,axis=1) + np.roll(Z,1,axis=0) + np.roll(Z,-1,axis=0)

[Note: this is actually wrong for the game of life, as the code should check for diagonal neighbours as well. This is simply a refactoring of the original for loop construct.]

Here there is an implicit assumption that the array does not have bounds and that x[-1] is next to x[0]. If this is a problem, you could add a buffer of zeros around your array with:

shape = Z.shape
new_shape = (shape[0]+2,shape[1]+2)
# b_z is a new array which will be our buffer
b_z = np.zeros(new_shape)

# set the middle of the array equal to the original `Z`
b_z[1:-1,1:-1] = Z

# do our rolls on the buffered array so that we don't have boundary isssues
b_n = np.roll(b_z,1,axis=1) + np.roll(b_z,-1,axis=1) + np.roll(b_z,1,axis=0) + np.roll(b_z,-1,axis=0)

# write back the part of the array that is of interest to us
N = b_n[1:-1,1:-1]

and for a benchmark:

# original function with loops
In [4]: %timeit computeNeighbours(z)
10 loops, best of 3: 140 ms per loop

# new function without a buffer
In [5]: %timeit noloop_computeNeighbours(z)
10000 loops, best of 3: 133 us per loop

# new function with a buffer to remove boundary counts
In [6]: %timeit noloop_with_buffer_computeNeighbours(z)
10000 loops, best of 3: 170 us per loop

So just a small improvement of a factor of x 1052. Hooray for Numpy!


Split a string based on a delimiter in C Basically, this code splits a string into substrings based on a token. The printf call is just a placeholder for something useful for each substring and can be substituted for something else, like a spell checker, profanity checker, etc...

With the input ("The quick brown fox jumps over the lazy dog", 44, ' ') the output is:

Processed: The
Processed: quick
Processed: brown
Processed: fox
Processed: jumps
Processed: over
Processed: the
Processed: lazy
Processed: dog

Code:

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

/**
 * Splits a string into substrings delimited by a token.
 * Returns the number of substrings found in the string.
 */
size_t __cdecl splitstring(
    _In_reads_or_z_(len) char *str,
    _In_ size_t len,
    _In_ char token
)
{
    char *temp = malloc(len + 1), *p = str, *q = str;
    memset(temp, 0, len);
    size_t i, cnt = 0;
    if (!temp)
    {
        _wperror(L"malloc");
        exit(EXIT_FAILURE);
    }

    for (i = 0; i < len + 1; i++)
    {
        if (str[i] == token || str[i] == '\0')
        {
            q = &str[i];
            strncpy_s(temp, len, p, q - p);
            printf("Processed: %s\n", temp);
            memset(temp, 0, len);
            cnt++;
        }
        // Useful property of boolean expressions in C: they are always either 1 or 0
        // Adjust p to point either to the next space or the next letter depending on if
        // it's the first word being processed.
        p = q + (p != &str[0]);
        if (!*p) break;
    }

    free(temp);
    temp = NULL;

    return cnt;
}

Check allocated pointers before using them
It's good that we have a if (!temp) check, but unfortunately we used temp as argument to memset() before we did the check, so it's too late!
We could (and should) use calloc() if we need to zero out the memory we allocated.  In this case, it's not necessary (we'll be overwriting it anyway), so we should just omit the memset() call.
Use const where appropriate
The pointers str, p and q can all point to const char; that allows us to safely pass a string literal as argument, for example, and it helps users understand what guarantees they are given.
Document the arguments
The meaning of the argument len isn't obvious from its name or from the function's comment.  It seems that it's the largest substring we expect to handle.
Even better would be to make the code adapt (using realloc(), perhaps) to the string it's given, rather than requiring the user to specify that.  I'll leave that as an interesting exercise for you.
Try not to mix indexing and pointer arithmetic
It's a little confusing to index into str using i, but to also have pointers p and q pointing into the string, too.
The break at the end of the loop
Since p initially points to the beginning of the string, we can move the test at the end of the loop to be part of the loop condition.  This makes the loop structure easier to follow:
for (size_t i = 0;  i <= len && *p;  ++i)

(I changed the test of i slightly, for simplicity and also as an almost-automatic aversion to integer overflow).
We don't need a loop to find the next instance of token
We could use strchr() to find the next token in str.  On GNU platforms, there's also strchrnul() which returns a pointer to the terminating NUL (instead of a null pointer) when the character isn't present.
Consider discarding empty words
Many users will expect the same result from "foo bar" and "foo  bar" - it's a surprise to find that this code considers there to be a zero-length word between the two spaces in the latter string.  Even more surprising is " foo" which splits to "" and " foo".

Reworked code
I started again from scratch, taking into account these observations.  The interface is a bit different, as in real life we want to be able to re-use the function to perform different actions on the tokens found, so we accept a function and a state pointer for the function.
This is standard portable C - you're of course able to add your annotation header to this if you want.
#include <stdlib.h>
#include <string.h>

/**
 * @brief Call a function for each token found in the input string.
 * @param s input string
 * @param sep nul-terminated set of delimiter characters, or NULL for default
 * @param f callback function
 * @param data callback data
 * @return true if successful, false on any failure
 */
int process_tokens(const char *s, const char *sep,
                   void (*f)(const char*,void*), void *data)
{
    if (!sep) {
        /* use default word separators */
        sep = " \t\n.!?()";
    }

    char *buf = NULL;
    size_t buf_len = 0;

    while (*(s+= strspn(s, sep))) {
        /* s is now pointing at next word character */
        size_t word_len = strcspn(s, sep);

        /* ensure we have sufficient storage */
        if (buf_len <= word_len) {
            buf_len = 2 * word_len + 1;
            char *t = realloc(buf, buf_len);
            if (!t) {
                free(buf);
                return 0;
            }
            buf = t;
        }

        /* copy word into storage, and call user function */
        memcpy(buf, s, word_len);
        buf[word_len] = 0;
        f(buf, data);

        /* advance to next separator */
        s += word_len;
    }

    free(buf);
    return 1;
}

/* A simple test program */

#include <stdio.h>

static void print_token(const char *s, void *data)
{
    size_t *count = data;
    ++*count;
    printf("Processed: \"%s\"\n", s);
}

int main()
{
    size_t count = 0;
    const char *input = "  Pack my box  with five-dozen liquor jugs.  ";
    if (!process_tokens(input, NULL, print_token, &count)) {
        fprintf(stderr, "Error splitting input string\n");
        return 1;
    }

    printf("Processed %zu tokens\n", count);
}


These words are so dirty Challenge:

  Write a program that will clean up the words from extra numbers and symbols.

Specifications:

  The first argument is a path to a file.
  Each line includes a test case with a list of words: letters are both lowercase and uppercase, and are mixed with extra symbols.
  Print the words separated by spaces in lowercase letters.
  The length of a test case together with extra symbols can be in a range from 10 to 100 symbols.
  The number of test cases is 40.

Solution:

import java.io.File;
import java.io.FileNotFoundException;
import java.util.Scanner;

public class CleanWords {
    public static void main(String[] args) {
        try (Scanner input = new Scanner(new File(args[0]))) {
            while(input.hasNextLine()) {
                System.out.println(sanitize(input.nextLine()));
            }
        } catch (FileNotFoundException fnfe) {
            System.out.println(args[0] + " doesn't exist");
        }
    }

    private static String sanitize(String input) {
        StringBuilder result = new StringBuilder();
        for (String s : input.split("[^a-zA-Z]+")) {
            result.append(' ').append(s);
        }
        return result.toString().toLowerCase().trim();
    }
}

Sample Input:

(--9Hello----World...--)
Can 0$9 ---you~
13What213are;11you-123+138doing7

Sample Output:

hello world
can you
what are you doing

I'm wondering if:

It would be more efficient to call trim within the loop?
If there's a less costly way than looping twice between calling both toLowerCase and trim

These may seem like givens, but it's been a long time since I did one of these, and I wanted to get back into it with a seemingly simple one. I passed on the first try which I largely have CR to thank for. For those interested, the source is CodeEval.com.

This can be significantly cleaned up and simplified using Java 8. Let's start with a quick review, though before rewriting it all :)

public static void main(String[] args) {
    try (Scanner input = new Scanner(new File(args[0]))) {

You perform no validation whatsoever here. When this is called without an argument, you'll get a ArrayIndexOutOfBoundsException... not nice. Validate your inputs (even when the assignment says you'll get a path)

Also you should consider using JSR-203 (aka. java.nio) API. It's cleaner and more informative wrt. Exceptions and general handling of problematic inputs.
Also it integrates very nicely with Streams :D

But you're using try-with-resources, which is a good start.

        while(input.hasNextLine()) {
            System.out.println(sanitize(input.nextLine()));
        }

This is a small indication your main-method is doing too much. You do:

Open file
read that file
call sanitize
print
error handling

Following the Single Responsibility Principle by the letter would mean every single one of these should be a separate method. I personally find that overkill, but I'd at least shift the responsibilities a little by separating result retrieval (aka. sanitize()) from printing.

Which gets me to sanitization or rather how you do it:

    for (String s : input.split("[^a-zA-Z]+")) {

What you're doing here is interesting and basically a cool trick, but.. you're cheating around what you actually want.

You want to extract words consisting of [a-z] (case insensitive). What you do is split you String on everything else and then reassemble it.

So there's a few things you could've done instead. First possibility would've been to extract the words with a Pattern and then assembling them, which might've been more obvious:

Pattern words = Pattern.compile("([a-z]+)", Pattern.CASE_INSENSITIVE);
Matcher m = words.matcher(s);
StringBuilder sb = new StringBuilder();
while (m.find()) {
   sb.append(m.group()).append(' ');
}

This basically accomplishes the same as your code, but is more explicit as to what you want to match. But this is a lot of code for something dead simple. It boils down to what your split does in combination with the loop.

What you do is replace everything, that's not a latin letter with a space. Why not write that?

s.replaceAll("[^a-zA-Z]+", " ");

this eliminates the for-loop you have and .. basically does the same :D

Now that we got this out of the way, let's look at how it looks with JSR203 and Streams :)

public static void main(String[] args) {
    try (Stream<String> lines = Files.lines(Paths.get(args[0]))) {
        final String output = lines.map(CleanWords::sanitize)
             .collect(Collectors.joining(System.lineSeparator()));
        System.out.println(output);
    } catch (IOException ex) {
        System.out.println("Could not read File");
    }
}

private static String sanitize(String input) {
     return input.replaceAll("[^A-Za-z]+", " ").toLowerCase().trim();
}


Finding number of days until a certain date I'm using Node.js and Moment in order to retrieve the days until the next 8th:

const getDaysTillSalary = (day) => {
    const today = moment();
    const currentDay = moment().format('D');
    let currentMonth = moment().format('M');
    let currentYear = moment().format('Y');
    const isCurrentDayGreater = parseInt(currentDay) > day;

    currentYear = isCurrentDayGreater && parseInt(currentMonth) === 12 ? moment().add(1, 'year').format('Y') : currentYear;
    currentMonth = isCurrentDayGreater ? moment().add(1, 'months').format('M') : currentMonth;

    const salaryDate = moment(`${day}-${currentMonth}-${currentYear}`, 'DD-MM-YYYY');

    return salaryDate.diff(today, 'days');
}

getDaysTillSalary(8);

I'm sure there is a cleaner and maybe efficient way to solve this problem, and will be glad to get some feedback.

const currentDay = moment().format('D');

You use .format() a lot, instead use Moment.js's getters like .date(). This way you also don't have to use parseInt() since the getters return number's.

What about this solution:

const getDaysTillSalary = (day) => {
  let date = moment();
  let days = 0;
  while(date.date() !== 8) {
    date = date.add(1, 'day');
    days++;
  }
  return days;
}

It creates a variable and stores the current Moment.js date in it. Then it repeatedly adds one day using .add() and counts up until date.date() (the day of the month) returns 8. The days variable is then returned.


Binary Insert Method - handling edge cases I've created a function that uses binary search to insert an element into an array (similar to this question).  I haven't benchmarked it, but based on my knowledge it should get \$O(1)\$ in the best case (i.e., appending an item to the end of the list) and \$O(nlogn)\$ in the average/worst case (I have no numbers to back this up - frankly I don't have much experience with benchmarking, so have mercy on me.)

Here's the binary search algorithm:

def binary_search(a, x):
    mid = 0
    min = 0
    max = len(a)

    # Deal with the edge cases
    if x < a[min]:
        return -1
    if x > a[max-1]:
        return max

    # Now that we know that the value is in range,
    # perform the actual search
    while min < max:
        mid = mid_point(min,max)
        if x < a[mid]:
            max = mid - 1
        elif x > a[mid]:
            min = mid + 1
        else:
            return mid

    # Another edge case
    return min if a[min] >= x else min + 1

This will return the index to insert the element into. The function used to perform the insertion is simple:

def binary_insert(array, value):
    index = binary_search(array,value)
    if index < 0: # Just append the value to the end of the list
        array.insert(0,value)
    else:
        array.insert(index,value)

The function works on a pre-sorted list (a requirement of a binary search) and maintains the list in sorted order. For example inserting [0..10) (aka [0,1,2,...,8,9]) into [-1, 0, 1, 4, 5, 6, 7, 8, 9, 10] yields [-1, 0, 0, 1, 1, 2, 3, 4, 4, 5, 5, 6, 6, 7, 7, 8, 8, 9, 9, 10]

You may have noticed that the return statement (the last one) for binary_search is very messy, and not very intuitive.  Would like to be able to incorporate that logic into the while loop somehow, or at least simplify it.  Does anyone have an idea of how I could do this? (Side question: how do I benchmark this?)

EDIT:

This is the mid_point function.

def mid_point(x, y):
    return (x+y)/2

Here are some thoughts:

For benchmarking there are two modules folks usually use:

cProfile

bettertimeit

I put an example in a previous post located here:

Profiling

Also, can you please post the entire script? Just want to make sure I have full context. Namely mid_point.


Count values in one dataframe that are less than all the values in another I currently have 2 dataframes, A and B. These dataframes are generated in runtime, and increase in size according to parameters in the program execution.

I need to evaluate how many times a value in dataframe A is lesser than all the values in dataframe B.

For example:

Dataframe A
+-----+-------+
| id  | value |
+-----+-------+
|   1 | 0.23  |
|   2 | 1.2   |
+-----+-------+
Dataframe B
+-----+-------+
| id  | value |
+-----+-------+
|   1 | 0.22  |
|   2 | 1.25  |
|   3 | 0.3   |
|   4 | 0.5   |
|   5 | 0.9   |
|   6 | 0.0   |
+-----+-------+

I need to check how many values greater than 0.23 (for example) are in dataframe B. in this case 4 of the 6.

My first try with this was using this code. In this case, bio_dataframe is dataframe A, an random_seq_df is dataframe B.

for bio_row in bio_dataframe.itertuples():
    total = 0
    for ran_row in random_seq_df.itertuples():
        if bio_row[2] < ran_row[2]:
            total += 1

As you can see, i use itertuples for fast iteration of the rows of the dataframes.
This approach works "well" for dataframes below 25000 rows, but beyond that it starts to get painfully slow.

So my next approach was this.

  final_res is a column in the dataframe.

for bio_row in bio_dataframe.itertuples():
    a = bio_row[2]
    total = random_dataframe.eval('final_res > @a')

It works excellent until 100000 rows, beyond that the story repeats.

I'm hitting a wall here and I've run out of ideas to test. Is there any way to improve the code? Am I missing something, or some snippet to make it faster?

It may be a good idea to sort the columns you wish to compare first. That way you can iterate over both lists a single time. The idea is to use the index to keep track of how many items are larger than the values in your first list. A relatively untested function is:

import pandas as pd
import time

df1 = pd.DataFrame()
df2 = pd.DataFrame()
df1['a'] = [0.23, 1.2]
df2['a'] = [0.22, 1.25, 0.3, 0.5, 0.9, 0.0] * 1000 # Add some extra work

def sol(da, db):
    # Sort your columns
    x = sorted(da)
    y = sorted(db)

    t = []  # The results
    yi = iter(y)  # Use of an iterator to move over y
    yindex = 0
    y_item = next(yi)

    for val in x:
        # Search through y to find the index of an item bigger than val
        while y_item <= val:
            if yindex == len(y) - 1:
                t.append(0)
                return t
            y_item = next(yi)
            yindex += 1
        # Use the length of y and the index to find the number of item
        # larger than the current val
        t.append(len(y) - yindex)
    return t

t0 = time.time()
print sol(df1['a'], df2['a'])
print time.time() - t0

>>> [4000, 1000]
0.00200009346008

The problem with your initial solution is that you iterate over your second list for every value in your first list giving you an n^2 run time. This can be seen if you increase the length of the first list a bit. Ive modified your function a little to demonstrate:

def sol0(df1, df2):
    b = df2['a']
    for bio_row in df1.itertuples():
        a = float(bio_row[1])
        total = pd.eval('b > a')
        df1.set_value(bio_row[0], 'sim_p_val', total.sum())
    return df1

Testing with the following shows how the n^2 approach can get slow with longer lists, using the sorted list approach is ~750x quicker in this example:

df1['a'] = [0.23, 1.2] * 1000
df2['a'] = [0.22, 1.25, 0.3, 0.5, 0.9, 0.0] * 1000

t0 = time.time()
res = sol(df1['a'], df2['a'])
print time.time() - t0

df1['sim_p_val'] = [0]*len(df1)
t0 = time.time()
df1 = sol0(df1, df2)
print time.time() - t0

# Verify the result is the same
print res == sorted(list(df1['sim_p_val']), reverse=True)

>>>0.0024299621582
1.78539991379
True


Programs for different language learning - Python I've created a program for my own purposes in three different languages, Perl, Python, and Ruby. Each program will be posted in with their own tag but I will add links to them through on each post. The program itself is very simple, all it does is ask the user about three different languages which are saved into an array, it then prompts the user to choose a language that runs through an if statement, and if the user chooses the language they're writing in it then takes a random language from the array and outputs it.

See also Perl

See also Ruby

Python:

import random

def welcome():
    choices = ["Perl", "Python", "Ruby"]
    lang = 3
    print "Welcome, to the test script, this will test what language you would like to learn.. In order to find out these choices, write this same definition in all three different languages\n"
    print "There are", lang, "languages to choose from please pick one\n"
    user_choice = raw_input(choices)
    if user_choice == "Ruby":
        print("You have chosen Ruby!\n")
    elif user_choice == "Perl":
        print("You have chosen Perl!\n")
    else:
        print("You're already writing in Python!! Let me choose for you! ")
        print random.choice(["Perl", "Ruby"])
welcome()

Creating this program in Python was actually fairly easy, it was my second program I've ever written in Python and reminds me a lot of Ruby. However I kept getting an error while running it due to the fact that I didn't import random, after figuring this out I found out that I had to write out each language's name instead of making it random from the array.. Also Python ended up being the shortest maxing out at 22 lines, I will be adding the execution time of the program shortly.

I also have some questions about Python:

In Python is there a equivalent to Ruby's puts?
In the function print random.choice(["Perl", "Ruby"]) is there a way I can pull from the array instead of having to write out language names

You should use len rather than hard-code the length of choices.
You should use str.format to add lang into the string. 'there are {} languages'.format(lang).
You mix print '' and print(''). Pick one or the other. The former is preferred in Python2, the latter in Python3.
Reduce the amount of prints. They are slow, and merging them into one is much faster.
You should use raw_input().lower(), so that the user can enter 'rUBy'. Where it converts it to lower case, 'ruby'.
Use elif not else. If I enter C, I don't want to be told I entered Python.

  In Python is there a equivalent to Ruby's puts?

In Python2 and 3 you can use '\n'.join(list).
In Python3 you can use print(*list, sep='\n').

  In the function print random.choice(["Perl", "Ruby"]) is there a way I can pull from the array instead of having to write out language names

You may mean filter the list, so it does not include 'Python'.

random.choice(filter(lambda lang: lang != "Python", choices))

import random

def welcome():
    choices = ["Perl", "Python", "Ruby"]
    user_choice = raw_input(
        (
            "Welcome, to the test script, this will test what language you "
            "would like to learn.. In order to find out these choices, write "
            "this same definition in all three different languages\n\n"
            "There are {} languages to choose from please pick one\n"
            "{}"
        ).format(len(choices), choices)
    ).lower()

    if user_choice == "ruby":
        print "You have chosen Ruby!\n"
    elif user_choice == "perl":
        print "You have chosen Perl!\n"
    else user_choice == "python":
        print "You're already writing in Python!!Let me choose for you! \n{}".format(
            random.choice(["Perl", "Ruby"]))
    else:
        print "Invalid choice."

welcome()


Using volatile instead of synchronized for a simulation I want to know if using volatile in this scenario will give better performance than using synchronized, specifically for the paused and running instance variable in the SimulationManager class.

public class SimulationManager {

    private List<SimulationPanel> simulations;
    private boolean paused = false;
    private boolean running = false;
    private volatile PausableStopabbleThread observer;

    public SimulationManager() {
        simulations = new ArrayList<>();
    }

    public SimulationManager(List<SimulationPanel> simulations) {
        this.simulations = Collections.unmodifiableList(simulations);
    }

    public void start() {
        if (isRunning()) {
            return;
        }
        for (SimulationPanel sim : simulations) {
            sim.tableInsertion.start();
        }
        setRunning(true);
        observer = new SimulationsObserver();
        observer.start();

    }

    public void play() {
        if (!isRunning() && !isPaused()) {
            return;
        }
        setPaused(false);
        for (SimulationPanel sim : simulations) {
            sim.tableInsertion.play();
        }
        observer.play();

    }

    public void reset() {
        if (!isRunning()) {
            return;
        }
        setRunning(false);
        setPaused(false);
        observer.stopWork();
        observer = null;
        for (SimulationPanel sim : simulations) {
            sim.tableInsertion.stopWork();
        }

    }

    public void pause() {
        if (!isRunning() && isPaused()) {
            return;
        }
        setPaused(true);
        observer.pause();
        for (SimulationPanel sim : simulations) {
            sim.tableInsertion.pause();
        }

    }

    private synchronized void setPaused(boolean b) {
        this.paused = b;

    }

    private synchronized boolean isPaused() {
        return paused;
    }

    public synchronized boolean isRunning() {
        return running;
    }

    private synchronized void setRunning(boolean running) {
        this.running = running;
    }

    /**
     * Specifies when the simulations has finished
     *
     * @author Victor J.
     *
     */
    class SimulationsObserver extends PausableStopabbleThread {

        private final int nSimulations = simulations.size();

        @Override
        public void run() {
            int finished = 0;
            while (!stopRequested()) {
                for (SimulationPanel sim : simulations) {
                    if (!sim.tableInsertion.isAlive()) {
                        finished++;
                    }
                }
                if (finished == nSimulations) {
                    setRunning(false);
                    return;

                } else {
                    finished = 0;
                }
                pausePoint();
            }

        }

    }

}

volatile should perform better and is appropriate in this situation. A volatile read/write is almost as fast as a non volatile read/write on modern architectures. On the other hand, locking with synchronized will have an overhead, especially in a highly contented scenario (many threads fighting for the lock).

Note however that unless you call the pause/run methods thousands or millions of times per second, you will probably not notice the difference.

Additional comments

This line:

this.simulations = Collections.unmodifiableList(simulations);

probably does not do what you think it does: this.simulations is unmodifiable, but the calling code can still modify the original collection, and the changes will be reflected to your local version (or they won't depending on memory consistency). It would probably be better to do a defensive copy:

this.simulations = new ArrayList<> (simulations);

Another note: because your simulations is effectively immutable,

this.simulations = new ArrayList<> ();

could be replaced by:

this.simulations = Collections.emptyList();

And by the way, you could make simulations final.


Optimizing voxel merge algorithm I have a bunch of voxel having an int value inside a 16^3 grid. I want to merge individual voxels with the same int value together in order to create the least possible amount of boxes the fastest way possible.

Here is my algorithm (kotlin) :

fun convert (grid: VoxelGrid): List<Box> {
    val dim = grid.dimension
    val boxes = mutableListOf<Box>()

    for (width in dim - 1 downTo 1) {
        for (height in dim - 1 downTo 1) {
            for (depth in dim - 1 downTo 1) {
                val dims = Vec3(width, height, depth)
                for (x in 0 until (dim - width)) {
                    for (y in 0 until (dim - height)) {
                        for (z in 0 until (dim - depth)) {
                            val min = Vec3(x, y, z)
                            val box = Box(min, min + (dims))

                            if (!contains(boxes, box)) {

                                val voxels = box.voxels()
                                val first = voxels[0]
                                val value = grid[first.x, first.y, first.z]
                                if (value != 0 && voxels.all {
                                            grid[it.x, it.y, it.z] == value
                                        }) {
                                    boxes.add(box)
                                }
                            }
                        }
                    }
                }
            }
        }
    }

    return boxes
}

private fun contains(boxes: MutableList<Box>, other: Box): Boolean {
    for (existing in boxes) {
        if (existing.intersects(other)) {
            return true
        }
    }

    return false
}

Here is a link to the helper classes

This works, but for a 16^3 grid (the last test case) it takes around 8s to complete which is way too long.

In order to optimize, I think that before calling convert(), I could group every voxel by their int value, then compute the AABB for each group and only call the algorithm for each AABB. This could work pretty well but for values that are spread out around the original grid.

Is there any other way to optimize this?

Try to determine the complexity of the algorithms you are implementing. The code you wrote above has a complexity of O(N⁹), where N is equal to grid.dimension. This is because you have 6 nested for loops, and then voxels.all{} which itself also has to loop over all three dimensions of the box voxels.

Grouping voxels by their int value is a good idea. You correctly worry about the case of voxels with the same value being spread out. The solution to that is to divide the box into connected voxels with the same int value. For example, if you had only one dimension, then if you have the values [0, 1, 2, 2, 1, 2], then you would divide this into 5 groups: [0], [1], [2, 2], [1] and [2]. It should be possible to do this in O(N³) time.

Now that you have a set of smaller shapes, the function that decomposes those shapes into boxes should run a lot faster. The question is now whether you want to have that function as fast as possible, or as accurate as possible. The function as you wrote does not necessarily result in the least amount of boxes. Apart from the fact that it doesn't actually try out boxes in a strict order from largest to smallest, even trying the largest boxes first won't necessarily result in the least amount of boxes being produced. Take for example this 2D array:

1 0 0 1
1 1 1 1
1 0 0 1

The voxels with value 1 form a H-shape. If you remove the largest possible box, you end up with:

1 0 0 1
x x x x
1 0 0 1

Now you are left with 4 1x1 boxes, so in total this will result in 5 boxes for the H-shape. If instead you would start by taking out the left and right side, you are left with the middle bar, resulting in just 3 boxes. Finding the solution with the least amount of boxes is actually an NP-complete problem, and an algorithm that would produce that solution would take even longer to run that yours. Given that your algorithm already does not give acceptable performance, you should instead focus on implementing the fastest algorithm that produces acceptable results.

The common approach is to start by finding strips of adjacent voxels in the x-direction. Then try to find adjacent strips with the same size and x-position in the y-direction, and merge them into squares. Finally do the same in the z-direction and merge adjacent squares into boxes. This should be O(N³) as well. If you know that some shapes are more common than others, you could tweak this algorithm to reduce the number of boxes.


Excessive use of let a problem in Clojure? I have this set of functions - it works, but I'm concerned about the use of lets.

(defn- create-counts [coll]
  "Computes how many times did each 'next state' come from a 'previous state'.
   The result type is {previous_state {next_state count}}."
  (let [past    coll
        present (rest coll)
        zipped  (map vector past present)
        sorted  (sort zipped)
        grouped (group-by first sorted)
        seconds (map (fn [[key pairs]] [key (map second pairs)]) (seq grouped))
        freqs   (map (fn [[key secs]]  [key (frequencies secs)]) seconds)]
    (into {} freqs)))

I started learning functional programming in Haskell some time ago, and there, because of laziness, it's commonplace to use this equivalent of Clojure let, because what's not needed, isn't ever computed:

asnwer = z
  where
    notused = map (^1000) [1000..10000]
    x = [1..]
    y = take 10 x
    z = map (*2) y

Is it okay to use Clojure let in this way? Look for example at the create-counts fn definition - I use it quite heavily there.
Would it be better to just put it all inside one expression?
What's the best practice for this?

I like how everything's named, but I don't know if there's not a performance penalty hidden somewhere.

When I try to match the Haskell example with Clojure code, it seems there's eager evaluation:

(let [notused (do (Thread/sleep (rand 10000000)) :done)
      x       (iterate inc 1)
      y       (take 10 x)
      z       (map #(* 2 %) y)]
  z)

;=> sleeps till the judgement day

I could imagine preventing this with futures or some clever let-like macro, but really it seems to me that I'm trying to do something Haskell-y (lazy) in Clojure. What's the idiomatic way?

First of all, you could use abstract structural binding (a.k.a. destructuring) to reduce the number of let bindings:

(defn- create-counts_org [[_ & present :as coll]]
  "Computes how many times did each 'next state' come from a 'previous state'.
   The result type is {previous_state {next_state count}}."
  (let [zipped  (map vector coll present)
        sorted  (sort zipped)
        grouped (group-by first sorted)
        seconds (map (fn [[key pairs]] [key (map second pairs)]) (seq grouped))
        freqs   (map (fn [[key secs]]  [key (frequencies secs)]) seconds)]
    (into {} freqs)))

I think there's nothing wrong in using let bindings that are technically not necessary to improve readability and comprehensibility.

But in your function, I think it is pretty clear what every step does:

sorted -> the first thing I see is the sort function
grouped ->  the first thing I see is the group-by function
seconds / freqs -> I think these are also pretty clear, using second and frequencies

And because each let binding just uses the previous binding once, I would probably use the thread-last (->>) macro:

(defn- create-counts [[_ & rest :as coll]]
  "Computes how many times did each 'next state' come from a 'previous state'.
   The result type is {previous_state {next_state count}}."
  (->>  (map vector coll rest)
        (sort)
        (group-by first)
        (map (fn [[key pairs]] [key (map second pairs)]))
        (map (fn [[key secs]]  [key (frequencies secs)]))
        (into {})))

I think this also answers your better to just put it all inside one expression question. Also, even if there where a performance penalty in using let bindings, it would probably be negligible.


C++ dynamic array implementation As a C++ beginner coming from Java, I have become increasingly confused on the topic of memory management and how to avoid memory leaks. Is the code below risking a memory leak that I'm not currently aware of? Any help or constructive feedback would be greatly appreciated.
#pragma once

template <class T>
class DynamicArray {
private:
    T *m_arr;
    int m_length; //amount of elements currently being stored in the array
    int m_capacity; //actual size of the array
public:
    DynamicArray();
    ~DynamicArray();
    T get(int index); //O(1)
    void add(T obj); //no need to push any objects forward, O(1)
    void insert(int index, T obj); //pushes forward all objects in front of the given index, then sets the obj at the given index, O(n)
    void set(int index, T obj); //sets the given index of m_arr as obj, O(1)
    void remove(int index); //removes the object at the given index and pushes all the array contents back, O(n)
    int size(); //O(1)
    void print();
};

#include <iostream>
#include "Header.h"

template<class T>
DynamicArray<T>::DynamicArray() : m_arr(new T[1]), m_length(0), m_capacity(1) {}

template<class T>
DynamicArray<T>::~DynamicArray() {
    delete[] m_arr;
}

template<class T>
T DynamicArray<T>::get(int index) {
    if (index < m_length && index >= 0)
        return m_arr[index];
    else throw ("Index out of bounds!");
}

template<class T>
void DynamicArray<T>::set(int index, T obj) {
    if (index < m_length && index >= 0) {
        m_arr[index] = obj;
    } else throw ("Index out of bounds!");
}

template<class T>
void DynamicArray<T>::add(T obj) {
    if (m_length == m_capacity) {
        T *new_arr = new T[m_length * 2];

        for (int i = 0; i < m_length; i++) {
            new_arr[i] = m_arr[i];
        }

        delete[] m_arr;
        m_arr = new_arr;
        m_capacity = m_capacity * 2;
    }
    m_arr[m_length] = obj;
    m_length++;
}

template<class T>
void DynamicArray<T>::insert(int index, T obj) {
    if (index < m_length && index >= 0) {
        int size;
        if (m_length == m_capacity) size = m_length * 2;
        else size = m_capacity;
        T *new_arr = new T[size];

        for (int i = 0, j = 0; i < m_length; i++, j++) {
            if (i == index) {
                new_arr[j] = obj;
                j++;
            }
            new_arr[j] = m_arr[i];
        }

        delete[] m_arr;
        m_arr = new_arr;
        m_capacity = m_capacity * 2;
        m_length++;
    } else throw ("Index out of bounds!");
}

template<class T>
void DynamicArray<T>::remove(int index) {
    if (index < m_length && index >= 0) {
        T *new_arr = new T[m_capacity];

        for (int i = 0, j = 0; i < m_length; i++, j++) {
            if (i == index) i++;
            if(i < m_length) new_arr[j] = m_arr[i];
        }

        delete[] m_arr;
        m_arr = new_arr;
        m_capacity = m_capacity * 2;
        m_length--;
    } else throw ("Index out of bounds!");
}

template<class T>
int DynamicArray<T>::size() {
    return m_length;
}

template<class T>
void DynamicArray<T>::print() {
    std::cout << m_arr[0];
    for (int i = 1; i < m_length; i++) {
        std::cout << ", " << m_arr[i];
    }
}

Welcome to C++, and welcome to Code Review.
C++ memory management is, as you probably have realized, tough and
error-prone.  There are many things that can easily go wrong.
Assuming that no exception is thrown, I don't see obvious memory leaks
in your code; however, there are still some issues worth discussing.
You can take a look at my implementation of a non-resizable dynamic
array or a stack-based full-fledged vector for some
inspiration.
Special member functions
You have not defined copy constructors or move constructors, so the
compiler will synthesize corresponding constructors that simply copy
all the members — which is completely wrong, as now the two
dynamic arrays will point to the same memory.  Not only are the
elements shared between the copies, causing modifications to one array
to affect the other, but the two copies will attempt to free the same
memory upon destruction, leading to a double-free error, which is
way more serious than a memory leak.
Initialization semantics
It is generally expected that the constructor of the element type is
called \$n\$ times if \$n\$ elements are pushed into the dynamic
array.  In your code, however, this is not the case, whre the amount
of constructors called is determined by the capacity of the dynamic
array.  Elements are first default initialized, and then copy-assigned
to.
The correct way to solve this problem requires allocating an
uninitialized buffer, and using placement new (or equivalent features)
to construct the elements, which is another can of worms.
Exception safety
Think of what happens when the construction of an element throws an
exception — your code will halt halfway, and there will be a
memory leak.  Resolving this problem would require a manual try
block, or standard library facilities like
std::uninitialized_copy (which essentially do the same under
the hood) if you switched to uninitialized buffers and manual
construction.
Move semantics
All of the elements are copied every time, which is wasteful.  Make
good use of move semantics when appropriate.
Miscellaneous
Used std::size_t instead of int to store sizes and indexes.1
get, size, and print should be const.  Moreover, get should
return a const T&.  In fact, get and set would idiomatically be
replaced by operator[].
Don't throw a const char*.  Use a dedicated exception class like
std::out_of_range instead.
Manual loops like
for (int i = 0; i < m_length; i++) {
    new_arr[i] = m_arr[i];
}

are better replaced with calls to std::copy (or
std::move).
Re-allocating every time insert is called doesn't seem like a good
idea.  A better trade-off might be to append an element and then
std::rotate it to the correct position (assuming rotation
doesn't throw).
Also, print might take an std::ostream& (or perhaps
std::basic_ostream<Char, Traits>&) argument for extra flexibility.

1 As Andreas H. pointed out in the comments, this recommendation is subject to debate, since the use of unsigned arithmetic has its pitfalls.  An alternative is to use std::ptrdiff_t and std::ssize (C++20) instead.  You can write your own version of ssize as shown on the cppreference page if C++20 is not accessible.


Using a new thread to post to observers I have implemented my own Observer pattern. I want to post my events in a new thread. This thread would simply call all the the observers/listeners with the posted event.

public class EventManager
{
    private ArrayList<Observer> observerList = new ArrayList<>();

    public void post(Object event)
    {
       new Thread(new MyRunnable(this, event)).start();
    }

    public void postHelper(Object event)
    {
        for(int i = 0; i < observerList.size(); i++)
        {
            Observer observer = observerList.get(i);
            observer.handle(event);
        }
    }

    private static class MyRunnable implements Runnable
    {
        EventManager eventManager;
        Object event;

        public MyRunnable(EventManager eventManager, Object e)
        {
            this.eventManager = eventManager;
            event = e;
        }

        @Override
        public void run()
        {
            eventManager.postHelper(event);
        }
    }
}

public interface Observer
{
    void handle(Object event);
}

How can I improve this? My main concern is that I am creating a thread everytime, which could potentially leave me out of memory.

You can limit the number of threads you're using via a thread pool. For example:

public class EventManager {
    private static ExecutorService pool = Executors.newFixedThreadPool(8);
    [....]

and then use it like so:

pool.execute(new MyRunnable(this, event));

You should also take care of shutting down the pool when/if you dispose the EventManager.
For further information check the API docs (Android in your case).


Initialization lock for singleton I'd like someone to double-check my use of Atomic primitives, because multi-threading is hard.

What I don't want to do is locking the whole get method, or having too many pre-checks per get() call.

private final static AtomicReference<T> REFERENCE = new AtomicReference<>();

private final static AtomicBoolean IS_CREATED = new AtomicBoolean(false);

public static MyObject get() {
    if (IS_CREATED.compareAndSet(false, true)) {
        REFERENCE.set(new MyObject());
    }
    return REFERENCE.get();
}

As vnp points out, your code is incorrect because a thread may read REFERENCE after IS_CREATED is set but before REFERENCE is set. Here are a couple alternatives:

use double-checked locking:

private static volatile T reference;
public static T get() {
    if (reference == null)
        synchronized(ThisClass.class) {
            if (reference == null)
                reference = new MyObject();
        }
    return reference;
}

use static class initialization:

private static class Lazy {
    private static final T reference = new MyObject();
}
public static T get() {
    return Lazy.reference;
}

use optimistic creation with CAS (this might create multiple instances but only one instance will be returned):

private static final AtomicReference<T> reference = new AtomicReference<>();
public static T get() {
    if (reference.get() == null)
        reference.compareAndSet(null, new MyObject());
    return reference.get();
}


Getting list of fruits into array list var fruitList = [];
var fruitExpired = GetFruitStatus();
var fruitName = "Mango";
var companyName = "Limited";

if(fruitExpired)
{
   var expiredCrateId = GetExpiredCrate().ID;
   list.push({
    expiredCrate : expiredCrateId,
    fruitName : name,
    fruitCompany : companyName
   });
}
else
{
   var CrateId = GetCrate().ID;
   list.push({
    crateId : expiredCrateId,
    fruitName : name,
    fruitCompany : companyName
   });
}

Can I bring list.push outside if else ? this will help remove duplicate code, only difference is that if expired I am adding property  expiredCrate : expiredCrateId, otherwise  crateId : expiredCrateId,

You can create a fruit with the common properties before the if:

var fruitList = [];
var fruitExpired = GetFruitStatus();
var fruitName = "Mango";
var companyName = "Limited";

var fruit = {
    fruitName: name,
    fruitCompany: companyName
}

if (fruitExpired) {
    fruit.expiredCrate = GetExpiredCrate().ID;
} else {
    fruit.crateId = GetCrate().ID;
}

list.push(fruit);

Also, beware where you put { } in your code, it's not C# and a { after if or else should be on the same line.


Does the class have unnecessary and violating Single responsibility principle method? I encountered with the following code in the work. The MyItemCoordinator should receive MyItem objects, process them, add them to MyItemList collection and then pass them to the listeners.

My questions are:

When I saw NewMyItem method I thought that MyItemCoordinator is a factory but when I saw NewMyItem's implementation I found out that it creates, sorts, calling to listeners and it's not static. So MyItemCoordinator is not a factory and NewMyItem method does way more things that it should to do according to its name, right?
If it's not a factory so why NewMyItem method receives so many params, creates an object, adds to the collection? Why not to do:

example:

var coordinator = new MyItemCoordinator();
var myItem = new MyItem(...PARAMS..)
coordinator.AddMyItemToList(myItem)

Inside AddMyItemToList we can sort and call the listeners. This is more clear way to do, isn't it?
So, is this implementation correct? Doesn't it have something wrong?

public class MyItemCoordinator
{
    ...

    public IList<MyItem> MyItemList = new List<MyItem>();

    public void NewMyItem(int id, ...15_MORE_PARAMS)
    {
        MyItem myItem = new MyItem(id,...ONLY_5_PARAMS_WITHIN_15_ARE_USED);
        AddMyItemToList(myItem);
        SortMyItemList();
        if (MyItemDetectedHandler != null)
            MyItemDetectedHandler(this);
    }

    public void AddMyItemToList(MyItem myItem)
    {
        //makes some checking and adds myItem to MyItemList
    }

    ...
}

You're right, but you should take this to its logical conclusion.

Creating item belongs to a factory class.
Tracking a list of items (coordinating?) is another responsibility.
Handling the new item (or list? Not clear from the code) belongs to a third class.

With this design you can reliability test each component in isolation. Merging these responsibilities into a single class makes testing difficult and blocks alternate implementations.


A wrapper class for Sessions in PHP I have been using $_SESSION, $_POST, $_GET, $_SERVER globals directly without ever knowing there was a concept/practice called, "Wrapping globals in classes" until, yesterday. As I did, I was able to immediately understand its effective benefit / usefulness. But I searched for a good wrapper, for $_SESSION, $_GET and $_POST but could not find anything simple/good. So, as a test I made this for the session global.

<?php

/**
 * A SESSION Wrapper class.
 *
 * @category Session
 * @version  1.0.0
 * @Nile
 */

namespace Nile\Lib;

class Session
{
    protected static $sessionLife = 1200;

    public static function start()
    {
        if(!headers_sent() && !session_id()){
            if(session_start()){
                session_regenerate_id();
                return true;
            }
        }
        return false;
    }

    public static function set($Key, $value)
    {
        $_SESSION[$Key] = $value;
    }

    public static function has($Key)
    {
        return (bool)(isset($_SESSION[$Key])) ? $_SESSION[$Key] : false;
    }

    public static function get($Key)
    {
        return (isset($_SESSION[$Key])) ? $_SESSION[$Key] : false;
    }

    public static function del($Key)
    {
        if(isset($_SESSION[$Key])){
            unset($_SESSION[$Key]);
            return false;
        }

    }

    public static function destroy()
    {
        if(isset($_SESSION)){
            session_destroy();
        }
    }

    public static function dump()
    {
        if(isset($_SESSION))
        {
            print_r($_SESSION);
            return ;
        }

        throw new \Exception("Session is not initialized");
    }

}

And this is the simple session initialization.

Session::start(); // does session_start();
Session::set('user', 'isLogedIn'); //does $_SESSION['user'] = 'isLoggedIn';

Considering this is my first wrapper, I would like a review and what I could add next. Something that is not very complicated, just easy to understand and a useful feature for this class.

I have been using php for over 10 years and never found a need for a session wrapper, but maybe you find it easier/better.

There are a few improvements that could be made with your code

If we are checking to see if the session has a key, a simpler test is

public static function has($Key)
{
    // return (bool)(isset($_SESSION[$Key])) ? $_SESSION[$Key] : false;
    return array_key_exists($Key, $_SESSION);
}

The get function here would probably be more useful with a default value option then just returning false.

public static function get($Key, $default=false)
{
    return (self::has($Key)) ? $_SESSION[$Key] : $default;
}

The del function, I am unsure what you are trying to achieve by returning false, but returning nothing if it isn't set?
Personally I wouldn't bother to check if it is set or not, just unset it, and return nothing. I would also call it delete, so it is painfully obvious to the use what it does

    public static function delete($Key)
    {
       if(isset($_SESSION){
            unset($_SESSION[$Key]);
       //         return false;
       }
    }

The dump function, why do you throw an exception if the session doesn't exist, but not anywhere else if the session doesn't exist? I would re-write it like a guard clause rather then having a return halfway through the function.

public static function dump()
{
    if(!isset($_SESSION))
    {
        throw new \Exception("Session is not initialized");
    }

    print_r($_SESSION);
}

Another useful function you might add is get_once. I do something similar for when I store an error message in the session, then redirect to a new page and display the error message. After that the error message is no longer relevant so I remove it from the session.

public static function get_once($Key, $default=false)
{
    $value = self::get($Key, $default);
    self::delete($Key);
    return $value;
}

Other things you could do is to manage different "namespaces" (maybe not the best word to describe it) within a session.

// keep in mind if you do this, you can't use static everywhere like you have

function __construct($namespace) {
    $this->namespace = $namespace;
}

function set($key, $value) {
   $_SESSION[$this->namespace][$this->key] = $value;
}

// Then you can use simple keys that don't overwrite each other
$user_session = new session('user');
$user_session->set('name', 'Donald Duck');

$page_session = new session('page');
$page_session->set('name', 'Home Page');


Fizzbuzz but without if Code first, elaboration later:

// Fizzbuzz: 1..100, %3->fizz, %5->buzz, %15->fizzbuzz
// noif: do not use if/switch statements or ternary expressions
// LIMIT must be a power of 10
// WIDTH must be equal to log10(LIMIT)

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

#define LIMIT 100
#define WIDTH 2

char *
itoa(int n)
{
    static char out[WIDTH + 1];

    for(char *outp = out + WIDTH - 1; outp >= out; --outp, n /= 10)
        *outp = '0' + n % 10;

    return out;
}

void
fizzbuzz(int n)
{
    static char *FBSTR[4] = {"", "Fizz", "Buzz"};

    FBSTR[3] = itoa(n);
    fputs(FBSTR[1 * (n%3 == 0)], stdout);
    fputs(FBSTR[2 * (n%5 == 0)], stdout);
    fputs(FBSTR[3 * (n%3 > 0 && n%5 > 0)], stdout);
    putchar('\n');
}

int
main(void)
{
    for(int i = 1; i <= LIMIT; ++i)
        fizzbuzz(i);

    return 0;
}

I've been reading a little bit about the anti-if campaign, and decided to try my hand at a challenge: Fizzbuzz but without if statements.

Because I'm not trying to be cheap, that also means no switch statements,

switch(n % 3){
    case 0: fputs("fizz", stdout); break;
}

no ternary expressions,

fputs((n % 3 == 0) ? ("fizz") : (""), stdout);

and no contrived while loops.

int lock = 0;

while(lock++ == 0 && n % 3 == 0)
    fputs("fizz", stdout);

char *
itoa(int n)
{
    static char out[WIDTH + 1];

    for(char *outp = out + WIDTH - 1; outp >= out; --outp, n /= 10)
        *outp = '0' + n % 10;

Nice bounds-check (outp >= out). This ensures that you don't overrun your buffer when given a very large integer. However, since out-1 is not a valid pointer value, outp >= out is an incorrect bounds-check for the same reason that

int decr(int x) {
    int y = x - 1;
    if (y > x) return 0;  // oops, overflow happened
    return y;
}

is an incorrect overflow-check.

Your bounds-check code also has another similarity to the above overflow-check code: When overflow happens, you return an in-range value that the caller might reasonably confuse for a valid result of the function. When itoa(x) returns "42", does it mean that x was 42, or does it mean that x was 1042 and overflow happened? It would be much better to say, if overflow happens, we return NULL.

Of course you'll have to express that "if" without if. ;)

itoa returns char*, but you don't expect the caller to modify the pointed-to chars, so you should declare it as returning a pointer to non-modifiable chars: const char*. Const is a contract.

Similarly, static char *FBSTR[4] should be static const char *FBSTR[4].

static const char *FBSTR[4] = {"", "Fizz", "Buzz", NULL};

would be much clearer to the human reader; it shows that you didn't miscount the number of array elements. In fact, you might even write

static const char *FBSTR[] = {"", "Fizz", "Buzz", NULL};

although the downside of that is that the number of indices is actually really important to your algorithm, so I can see a valid argument for keeping the 4 in this case.

fputs(FBSTR[1 * (n%3 == 0)], stdout);
fputs(FBSTR[2 * (n%5 == 0)], stdout);
fputs(FBSTR[3 * (n%3 > 0 && n%5 > 0)], stdout);
putchar('\n');

I did puzzle this out, but printing all those empty strings feels like a strange way to go about it. Why not simply

int idx = 1*(n%3 == 0)
        + 2*(n%5 == 0)
        + 3*(n%3 != 0 && n%5 != 0);
fputs(FBSTR[idx], stdout);
putchar('\n');

In which case you can combine the fputs and putchar into

puts(FBSTR[idx]);

If you think it would be easier to understand, you could even rewrite the index computation using bitwise operations:

const char *FBSTR[] = {itoa(n), "Fizz", "Buzz", "FizzBuzz"};
int idx = (n%3 == 0) + 2*(n%5 == 0);
puts(FBSTR[idx]);

To avoid reinventing the wheel, you could replace itoa(n) with snprintf(somelocalbuffer, sizeof somelocalbuffer, "%d", n). However, for this toy example, itoa does have a more ergonomic interface; and reinventing the wheel isn't necessarily a bad thing.


C++ implementation of the decorator pattern This is a C++ implementation of the decorator pattern. I am also learning about interfaces and (pure) virtual functions.

#include <iostream>
#include <string>

// interface

class AbstractObject {
protected:
    std::string name;
    virtual void displayTo(std::ostream&) const = 0;
public:
    friend class Decorator;
    friend std::ostream& operator<<(std::ostream&, const AbstractObject&);

    AbstractObject(std::string name) : name(name) {}
};

std::ostream& operator<<(std::ostream& os, const AbstractObject& object) {
    object.displayTo(os);
    return os;
}

// implementation

class Object : public AbstractObject {
protected:
    void displayTo(std::ostream& os) const {
        os << name;
    }
public:
    Object(std::string name): AbstractObject(name) {}
};

class Decorator : public AbstractObject {
protected:
    const AbstractObject& ref_decoratedObject;
    void displayTo(std::ostream& os) const {
        os << name << " decorates ";
        ref_decoratedObject.displayTo(os);
    }
public:
    Decorator(std::string name, const AbstractObject& ref_decoratedObject)
    : AbstractObject(name), ref_decoratedObject(ref_decoratedObject) {}
};

int main() {
    AbstractObject& human = Object("Alice");
    AbstractObject& shirt = Decorator("blue shirt", human);
    AbstractObject& jacket = Decorator("red jacket", shirt);

    std::cout << human << std::endl;
    std::cout << jacket << std::endl;

    getchar();

    return 0;
}

Friend Functions.

Its a lot simpler to just place friend functions directly into the class declaration.

Rather than:

class AbstractObject {
    friend std::ostream& operator<<(std::ostream&, const AbstractObject&);
};

std::ostream& operator<<(std::ostream& os, const AbstractObject& object) {
    object.displayTo(os);
    return os;
}

Simply do this:

class AbstractObject {
    friend std::ostream& operator<<(std::ostream& os, const AbstractObject& object) {
        object.displayTo(os);
        return os;
    }
};

Avoid protected

Protected very rarely gives you any benefit. It should be avoided.

class AbstractObject {
protected:
    // All the people that use `name` are already friends.
    // So protected is not useful. Other people using name
    // should be calling `displayTo()` to get the name displayed.
    // But the real question is why does an abstract object have
    // a name? If i does you should know how to display it.
    std::string name;

    // Personally I see no reason not to make this public.
    // But you could make it private and only callable by your
    // friend the output operator.
    virtual void displayTo(std::ostream&) const = 0;
};

Classes with Virtual methods should have virtual destructor

When you create a class with a virtual method. This means you are calling that method through an object of another type. This type is usually dynamically allocated and thus will be dynamically destroyed.

If you delete it via a pointer to the base class the wrong destructor will be called (unless it it virtual).

AbstractObject*  obj = new Decorator("red jacket", shirt);

// Do stuff.

delete obj; // Here obj is an AbstractObject* so we call AbstractObject::~AbstractObject()
            // to destroy the object. This is wrong as obj points at a
            // Decorator object and we want to call Decorator::~Decorator()
            // To achieve this the AbstractObject destructor must be
            // virtual to make sure that chaining to the most
            // derived version of the function happens.

Temporary abuse:

    AbstractObject& human = Object("Alice");
    AbstractObject& shirt = Decorator("blue shirt", human);
    AbstractObject& jacket = Decorator("red jacket", shirt);

Sure that works. But I can not call any Object methods on human as all I can see is the AbstractObject interface. Same with shirt and jacked I can not see their properties or fields.

It would be better to do it like this:

    Object    human("Alice");
    Decorator shirt("blue shirt", human); // this works as Object is an AbstractObject
    Decorator jacket("red jacket", shirt);// This works as Decorator is an AbstractObject

Design

Yes decorator can be a useful pattern.
But in C++ we usually use "Duck Typing" to solve the same problem.

Duck Typing: If it quacks like a duck it must be duck like.
             If it has the same method name  with the same parameters
             then we should be able to use it in the same way.

This is usually used in conjunction with templates.

template<typename Duck>
void print(Duck const& duck, std::ostream& str) {
    duck.displayTo(str);
}

Here we have a print() method. It takes any type (don't have to be related) as long as they support the displayTo(std::ostream& str) method the above function will work.

Refactored original

#include <iostream>
#include <string>

class AbstractObject
{
    public:
        virtual ~AbstractObject() {}
        friend std::ostream& operator<<(std::ostream& os, const AbstractObject& object)
        {
            object.displayTo(os);
            return os;
        }
        virtual void displayTo(std::ostream&) const = 0;
};

class Object : public AbstractObject
{
    private:
        std::string name;
    public:
        Object(std::string name)
            : name(name)
        {}
        void displayTo(std::ostream& os) const
        {
            os << name;
        }
};

class Decorator : public Object
{
    private:
        const AbstractObject& ref_decoratedObject;
    public:
        Decorator(std::string name, AbstractObject const& ref_decoratedObject)
            : Object(name)
            , ref_decoratedObject(ref_decoratedObject)
        {}
        void displayTo(std::ostream& os) const
        {
            Object::displayTo(os);
            os << " decorates " << ref_decoratedObject;
        }
};

int main() {
    Object      human("Alice");
    Decorator   shirt("blue shirt", human);
    Decorator   jacket("red jacket", shirt);

    std::cout << human << std::endl;
    std::cout << jacket << std::endl;


Encapsulating game data Consider such a code (javascript):

var jsonTmp = '{ "groups": [ { "name": "G1", "status": "inactive" }, { "name": "G2", "status": "active"}, { "name": "G3", "status": "inactive" }] }'
var dataTmp = JSON.parse(jsonTmp)

function g(key) {
    this.key = key;
    this.name;
    this.status;

    this.setStatus = function (status) {
        this.status = status;
    }

    this.setName = function (name) {
        this.name = name;
    }

}

function main() {
    this.groups = []

    this.createGroups = function () {
        $.each(dataTmp.groups, function (key, group) {
             myGame.groups[key] = new g(key)
             myGame.groups[key].setStatus(group.status)
             myGame.groups[key].setName(group.name)
        })
    }

    this.getGroupsWithStatus = function (status) {
        var groups = [];
        $.each(myGroups, function (key, group) {
            if ( group.status == status ) { groups.push( key )}
        })
        return groups
    }

}

var myGame = new main();
var myGroups = myGame.groups
myGame.createGroups();
myGame.getGroupsWithStatus("inactive")

I am attempting to make good and effective, also readable OOP logic, and since it is my first fully oop project please make some advices:

I create game (myGame), that takes dynamically (from jsonTmp) data to create multiple objects, called groups. Because amount of groups may be different they do not have a name like

var group1 = new g("abc")
var group2 = new g("cde")

They are stored without a instialized name in array of objects (here myGame.groups or myGroups).

Question 1: is that a good idea of storing objects to access them via key of object in array? I will be also searching for particular value of property of objects - like status here with getGroupsWithStatus. Do you have any better ideas to solve such a logic?

Question 2: can i set a name of instance in different way then

var instanceName = new g("abc")

and do i really need it if my objects are stored in array?

Question 3:
In createGroups method I would rather use some kind of pointer instead of myGame.groups[key]:

  this.createGroups = function () {
        $.each(dataTmp.groups, function (key, group) {
        var currentGroup;
        currentGroup = new g(key)
        currentGroup.setStatus(group.status)
        currentGroup.setName(group.name)
        myGame.groups[key] = currentGroup;
        })
    }

I guess that solution is more readable. Am i right?

Sorry for maybe dumb questions, but i always used structural languages and oop logic is quite hard for me to use, and most of tutorials say about how to make class or inherit props of objects. Maybe you can also provide some good oop in practice tutorials?
Thansk in advance.
Kalreg.

Q1: Sure
Q2: Technically you are not setting a name, and that variable is hence misleading
Q3: You could do that, but I would propose you just send the whole object to the constructor, and let that constructor do the .. constructing ;)

Further notes:

g is a terrible name, Group is a much better one (capital G because this function is a constructor)
Using the jQuery each feels wrong, just go bare metal and use the Array built-ins
Speaking of which getGroupsWithStatus becomes a one-liner with the Array.filter() built-in
The reliance of main() on myGroups is unhealthy, you are treating myGroups as a global, it should be local to main, especially if you are putting querying functions within main
If you are not using a fat arrow syntax function, then you should provide a function name (getGroupsWithStatus = function (status) => getGroupsWithStatus = function queryGroupsWithStatus(status)

This is my counter proposal:

var jsonTmp = '{ "groups": [ { "name": "G1", "status": "inactive" }, { "name": "G2", "status": "active"}, { "name": "G3", "status": "inactive" }] }'

function Group(key, properties) {
    this.key = key;

    properties = properties || {};
    this.name = properties.name;
    this.status = properties.status;
}

function main() {
    this.groups = [];

    this.createGroups = function (json) {
      var data = JSON.parse(json)
      this.groups = data.groups.map( (o,key) => new Group(key,o) );
    }

    this.getGroupsWithStatus = function querygetGroupsWithStatus(status) {
      return this.groups.filter( group => group.status == status  )
    }
}

var myGame = new main();
myGame.createGroups(jsonTmp);
console.log( myGame.getGroupsWithStatus("inactive") );


Calculate proper dosage of supplements to take Well, that's my first "program" after couple days of working with Python and programming itself. That may be stupid since everything there is so basic but I wanted to ask if there are any spaces to improve it. Idea was to give the user proper dosage of 4 supplements regarding if this is training day or not.
Is there any way to achieve it faster, more simple, better? Appreciate all feedback!
Edit
Thanks for all your replies! I will go through all of them and learn from them what i can, still confused in programming world.
dayon = {"BCAA" : "10g",
  "White" : "50g",
  "Glutamine" : "20g",
  "Vitamins" : 10}

dayof= {"BCAA" : "5g",
    "White" : "25g",
    "Glutamine" : "10g",
    "Vitamins" : 5}

username = input("What is your name?\n")
trening = input("Are you going to the gym today?\n")

def dosage():
    if trening == "yes":
       return (username + ", you have to take: " +
        "\nBCAA in dosage of: " + str(dayon["BCAA"]) +
        "\nWhite in dosage of: " +str(dayon["White"]) +
        "\nGlutamine in dosage of: " + str(dayon["Glutamine"]) +
        "\nVitamins in number of: " +str(dayon["Vitamins"]))
    elif trening == "no":
        return (username + ", you have to take: " +
        "\nBCAA in dosage of: " + str(dayof["BCAA"]) +
        "\nWhite in dosage of: " + str(dayof["White"]) +
        "\nGlutamine in dosage of: " + str(dayof["Glutamine"]) +
        "\nVitamins in number of: " + str(dayof["Vitamins"]))
    else:
        print ("Please answer yes or no")

print (dosage())

Here is the list of things I would improve:

for handling the proper answer on the "Are you going to the gym today?", I would start the while loop until "Yes" or "No" is entered
put the execution logic under if __name__ == '__main__':
define a message template with appropriate placeholders - format it with "day on" or "day off" objects and the entered username
define constants in upper case as per PEP8
let the "dosage" function return the dosage object - print the message outside of the function - for the better logic separation
add meaningful comments and docstrings

The code with all the suggested changes applied:

DAY_ON = {
    "BCAA": "10g",
    "White": "50g",
    "Glutamine": "20g",
    "Vitamins": 10
}

DAY_OFF = {
    "BCAA": "5g",
    "White": "25g",
    "Glutamine": "10g",
    "Vitamins": 5
}

MESSAGE_TEMPLATE = """
{username}, you have to take:
    BCAA in dosage of: {obj[BCAA]}
    White in dosage of: {obj[White]}
    Glutamine in dosage of: {obj[Glutamine]}
    Vitamins in number of: {obj[Vitamins]}
"""

def get_dosage(is_training_day):
    """Returns appropriate dosage depending on whether it is a training day or not"""
    return DAY_ON if is_training_day else DAY_OFF

if __name__ == '__main__':
    username = input("What is your name?\n")

    while True:
        choice = input("Are you going to the gym today?\n")

        if choice in ('Yes', 'No'):
            break
        else:
            print("You can only enter 'Yes' or 'No'.")

    dosage = get_dosage(is_training_day=choice == 'Yes')
    print(MESSAGE_TEMPLATE.format(username=username, obj=dosage))

Demo:

$ python3 test.py
What is your name?
Chubaka
Are you going to the gym today?
What?
You can only enter 'Yes' or 'No'.
Are you going to the gym today?
No

Chubaka, you have to take:
    BCAA in dosage of: 5g
    White in dosage of: 25g
    Glutamine in dosage of: 10g
    Vitamins in number of: 5

If you are using Python3.6+, you can also use f-strings for string formatting:

dosage = get_dosage(is_training_day=choice == 'Yes')
print(f"""
{username}, you have to take:
    BCAA in dosage of: {dosage['BCAA']}
    White in dosage of: {dosage['White']}
    Glutamine in dosage of: {dosage['Glutamine']}
    Vitamins in number of: {dosage['Vitamins']}
""")


Setting the default display name for my model I have a method in my Ruby code that sets the default name for my model. Rubocop complains that Assignment Branch Condition Size is too high, 21.24/15. How can I improve this?

def set_default_display_name
   return unless display_name.blank?
   count = user.credentials.where(type: type).count
   if count == 0
     self.display_name = name
   elsif user.credentials.where(display_name: name).count == 0
     self.display_name = name
   else
     self.display_name = "#{name} #{count + 1}"
   end
end

The method is doing three things:

Determining whether or not to set display_name.
Determining the default display name.
Setting display_name to the default value.

Each of those adds to the Abc metric. The biggest contributor to the Abc metric is determining the default display name. To me, that also seems like the most logically separate. We can lower the Abc metric by extracting that responsibility to its own method.

def set_default_display_name
  self.display_name = default_display_name if display_name.blank?
end

def default_display_name
  credentials_of_type = user.credentials.where(type: type)
  return name if credentials_of_type.none? || user.credentials.where(display_name: name).none?
  "#{name} #{credentials_of_type.count + 1}"
end

Extracting default_display_name makes it slightly simpler to test the default display name logic. You could also use that logic elsewhere, e.g., a UI could show the current display name and ask if they want to reset it to the default display name.

At this point, I'd question whether set_default_display_name is necessary or if it could be inlined.

Another option, is to keep your code as is and modify your .rubocop.yml file to increase the threshold of the Abc metric, or disable it entirely. The Abc metric is all about code size, not complexity. Theoretically, bugs increase with code size. You'd have to determine if this low of a threshold makes it easier to mask bugs. (Based on an earlier thread, the answer is probably, "yes", since your original code seems to have a bug. I retained the functionality of your original code in this example.)

--- Addendum ---

@ramonrails makes a good point in the comments. I'm usually fine with return guard clauses at the top of ruby methods. However, I agree that having a return in the middle of this method is not ideal.

This answer was only addressing the ABC question. If this were my code, I'd extract the credentials logic completely from this "view" code.

Something like:

def default_display_name(type_credentials)
  type_credentials.any? ? "#{name} #{type_credentials.count + 1}" : name
end

We're far afield from the question at that point though.


Drawing 800+ circles in a space-themed shoorter game I am making a space-themed shooter game, and I need to render many "lasers" at the same time. I am using the library Slick2D, a wrapper around lwjgl. Unfortunately, when there are a lot of these "lasers" on the screen at one time, the game drops from its usual 60 FPS to around 30 or even 20.

Things I've noticed:

Not calling drawSelf = no lag
Not calling .fill = no lag
As is = lag as stated in title

public void drawSelf(Graphics g){
    int[] pos = tr.toSlick(blast.getPosition());
    g.setColor(Color.green);
    Circle circle = new Circle(pos[0], pos[1], tr.xscale/2);
    g.fill(circle);
}

public int[] toSlick(float x, float y){
    int [] output = new int[2];
    output[0] = (int)(x*xscale+width/2);
    output[1] = (int)(-1*y*yscale+height/2);
    return output;
}

tr is the coordinate transformer object (I'm using jbox2d physics, but that's not impacting anything), and blast is the "Body".

Can I have some suggestions on making this code faster or am I stuck because of Slick's internals?

By looking at documentation, I found another method that is faster at drawing many circles.

I replaced last 2 lines in drawSelf with:

g.fillOval(pos[0],pos[1],tr.xscale,tr.yscale);

and I got a lot less lag. Turns out it did have to do with internals.


Base class for feed-parsing I have written a class which I will use as a base class, allowing other classes to extend this class.

How can I improve this? Am I breaking any conventions or styling?

import requests
import feedparser
from BeautifulSoup import BeautifulSoup

class BaseCrawler(object):
    ''' Base Class will be extended by other classes'''

    def __init__(self, url):
        self.url = url

    def test(self):
        return self.url

    def get_feed(self):
        feed = feedparser.parse(self.url)
        return feed

    def post_request(self):
        res = requests.post(self.url, self.data)
        return res

    def get_request(self):
        res = requests.get(self.url)
        self.raw = res.content
        return self.raw

    def build_entity(self):
        raise NotImplementedError("Subclasses should implement this!")

    def process(self):
        return NotImplementedError("Subclasses should implement this!")

    def build_query(self):
        return NotImplementedError("Subclasses should implement this!")

I'm not sure what you're intending to do with this class so I can only offer some general tips.

Make private variables private:

def __init__(self, url):
    self._url = url

Use properties instead of get_ methods. For example:

@property
def request(self):
    return self._requests.get(self._url)

# to make it writeable:
@request.setter
def request(self, value):
    self._requests.set(value)

If you're using Python 3, have a look at abc.ABCMeta for better solution for abstract base classes with abstract methods.


Blocking requests within Express Is this the correct way to block requests in Express?

app.js

app.use("/", indexRouter);

app.use(function(req, res, next) {
  next(createError(404));
});

app.use(function(err, req, res, next) {
  res.locals.message = err.message;
  res.locals.error = req.app.get("env") === "development" ? err : {};
  console.log(err);
  res.status(err.status || 500);
  res.json({ error: err.message });
});

controller

exports.fetch_seat = (req, res, next) => {
  const errors = validationResult(req);
  if (!errors.isEmpty()) {
    return res.status(422).json({ error: true, message: errors.array() });
  }

  Seat.getSeatByNumber(req.params.seatNumber, (err, seat) => {
    if (err) res.json(err);
    else res.json(seat);
  });
};

model

Seat.findSeat = function(key, value) {
  return new Promise((resolve, reject) => {
    const seat = file.find(r => r[key] === value);
    if (!seat) {
      reject({
        error: true,
        message: "Seat not found",
        status: 404
      });
    }
    resolve(seat);
  });
};

Seat.getSeatByNumber = function(seatNumber, result, next) {
  try {
    this.findSeat("seatNumber", seatNumber)
      .then(seat => {
        console.log(seat);
        if (seat !== undefined && Object.keys(seat).length > 0) {
          return result(null, seat);
        } else {
          return result({
            error: true,
            message: "Seat not found",
            status: 404
          });
        }
      })
      .catch(error => {
        console.log(error);
        console.log("1st catch");
        return result(error, null);
      });
  } catch (error) {
    console.log("outer catch");
    console.log(error);
    return result(error);
  }
};

I have changed the functions within my model to promises. do i need it to be promises from the controller?

If possible, always whitelist acceptable sources instead of trying to blacklist harmful ones. Also, don't use try-catch for control flow, use it for what it's designed for: error handling.

All that being said, you aren't passing the promise out of the model, because you wait for the response with .then, so no you wouldn't have to make the controller methods promises as well, but why wouldn't you? JS is single-threaded, so always default to asynchronous operations to be non-blocking.