Benchmarking copy semantics vs. move semantics I am still in the process of learning modern C++, and decided to write this dummy program for actually benchmarking copy semantics vs. move semantics:

#include <chrono>
#include <iostream>
#include <cstdint>

using std::cout;
using std::endl;

class List {
public:
    void push_back();
    List() : sz{0}, head{nullptr}, tail{nullptr} { }

    List(List&& lst) : head{lst.head}, tail{lst.tail}, sz{lst.sz} {
        cout << "Move constructor" << endl;
        lst.head = nullptr;
        lst.tail = nullptr;
        lst.sz = 0;
    }

    List(const List& lst)
    {
        cout << "Copy constructor" << endl;
        for (size_t i = 0; i != lst.sz; ++i)
        {
            push_back();
        }
    }

    List& operator=(List&& lst)
    {
        cout << "Move assignment" << endl;
        head = lst.head;
        tail = lst.tail;
        sz = lst.sz;
        lst.head = nullptr;
        lst.tail = nullptr;
        lst.sz = 0;
        return *this;
    }

    List& operator=(const List& lst)
    {
        cout << "Copy assignment" << endl;
        head = nullptr;
        tail = nullptr;
        sz = 0;

        for (size_t i = 0; i != lst.sz; ++i)
        {
            push_back();
        }

        return *this;
    }

private:

    struct ListNode {
        ListNode* next;
    };

    ListNode* head;
    ListNode* tail;
    size_t sz;
};

void List::push_back()
{
    ListNode* new_node = new ListNode;
    new_node->next = nullptr;

    if (head == nullptr)
    {
        head = new_node;
    }
    else
    {
        tail->next = new_node;
    }

    tail = new_node;
    ++sz;
}

class CurrentTime {
    std::chrono::high_resolution_clock m_clock;

public:
    uint64_t milliseconds()
    {
        return std::chrono::duration_cast<std::chrono::milliseconds>
            (m_clock.now().time_since_epoch()).count();
    }
};

List createList(CurrentTime& ct)
{
    List lst;
    constexpr size_t HUNDRED_MILLION = 100 * 1000 * 1000;

    auto start = ct.milliseconds();
    for (size_t i = 0; i != HUNDRED_MILLION; ++i)
    {
        lst.push_back();
    }
    auto end = ct.milliseconds();

    cout << "createList in " << (end - start) << " milliseconds." << endl;

    return lst;
}

int main() {
    CurrentTime ct;

    auto start = ct.milliseconds();
    List list;
    list = createList(ct);
    auto end = ct.milliseconds();

    cout << "Duration: " << (end - start) << " milliseconds." << endl;

    start = ct.milliseconds();
    List list2(list);
    end = ct.milliseconds();

    cout << "Duration: " << (end - start) << " milliseconds." << endl;

    start = ct.milliseconds();
    List list3(std::move(list));
    end = ct.milliseconds();

    cout << "Duration: " << (end - start) << " milliseconds." << endl;
}

Did I manage to capture the idiomatic use of move facilities in this demonstration program? Tell me anything that comes to mind.

OK if we are going to do some timing then lets fix a few things.

When I compile this with optimizations -O3 it crashes. So getting timmings will be hard.

So lets fix the copy constructor.

Member variables are initialized depending on the context.

List a;          // Members are default initialized.
List a = List(); // Members are zero    initialized.

If the members are default initialized then POD members are left in an undefined state. So the following constructor:

    List(const List& lst)
    {
        cout << "Copy constructor" << endl;
        for (size_t i = 0; i != lst.sz; ++i)
        {
            push_back();
        }
    }

leaves all its members uninitialized (and thus in an undefined state) when used like this:

List list2(list);

Thus the call th push_back() is going to start manipulating some undefined pointers.

Fix by either explicitly initializing all members in the constructor or by chaining a constructor that will do it for you. Sinve your default constructor initializes the members correctly I will chain that.

    List(const List& lst)
        : List()
    {
        cout << "Copy constructor" << endl;
        for (size_t i = 0; i != lst.sz; ++i)
        {
            push_back();
        }
    }

IO and timing do not work well.

The IO operations are notoriously complex and using IO can change a lot. You want to avoid all IO operation during any timing phase and print the results out at the end after all timing information has been generated.

Standard Idioms

There are a couple standard idioms you should follow (as most code will be written like this).

Rule of Five. You forgot a destructor. Though it will not affect your timmings you should probably write one just to be tidy.

Copy and Swap. The copy assignment operator is usually written in terms of copy and swap.

Swap on move. Movement is usually implemented as a swap operation. That way you don't have to explicitly tidy up your content at this point if you don't need to. But the moved from object will do the tidy up eventually if it needs to. Currently your move assignment will leak if used.

Noexcept: The move operators should be marked as no except. This allows containers they are used within to perform certain operations that would not otherwise be allowed to maintain the strong exception guarantee.

class List {
public:
    void push_back();
    void swap(List& other) noexcept
    {
        using std::swap;
        swap(sz,   other.sz);
        swap(head, other.head);
        swap(tail, other.tail);
    }
    List()
       : head{nullptr}
       , tail{nullptr}
       , sz{0}
    {}

    // Copy
    List(List const& lst)
        : List()
    {
        for (size_t i = 0; i != lst.sz; ++i)
        {
            push_back();
        }
    }
    List& operator=(List const& lst)
    {
        List tmp(lst);
        swap(tmp);

        return *this;
    }

    // Move
    List(List&& lst) noexcept
       : List()
    {
        swap(lst);
    }

    List& operator=(List&& lst)
    {
        swap(lst);
        return *this;
    }

    // Destroy
    ~List()
    {
        ListNode* next;
        for(auto tmp = head; tmp; tmp = next)
        {
            next = tmp->next;
            delete tmp;
        }
    }
private:

    struct ListNode {
        ListNode* next;
    };

    ListNode* head;
    ListNode* tail;
    size_t sz;
};

void List::push_back()
{
    ListNode* new_node = new ListNode{nullptr};

    if (head == nullptr)
    {
        head = new_node;
    }
    else
    {
        tail->next = new_node;
    }

    tail = new_node;
    ++sz;
}

class CurrentTime {
    std::chrono::high_resolution_clock m_clock;

public:
    uint64_t milliseconds()
    {
        return std::chrono::duration_cast<std::chrono::milliseconds>
            (m_clock.now().time_since_epoch()).count();
    }
};

List createList(CurrentTime& ct)
{
    List lst;
    constexpr size_t HUNDRED_MILLION = 100 * 1000 * 1000;

    for (size_t i = 0; i != HUNDRED_MILLION; ++i)
    {
        lst.push_back();
    }

    return lst;
}

int main() {
    CurrentTime ct;

    auto start = ct.milliseconds();
    List list = createList(ct);
    auto end = ct.milliseconds();

    cout << "Created Duration: " << (end - start) << " milliseconds." << endl;

    start = ct.milliseconds();
    List list2(list);
    end = ct.milliseconds();

    cout << "Copy Duration: " << (end - start) << " milliseconds." << endl;

    start = ct.milliseconds();
    List list3(std::move(list));
    end = ct.milliseconds();

    cout << "Move Duration: " << (end - start) << " milliseconds." << endl;

    start = ct.milliseconds();
    list = list2;
    end = ct.milliseconds();

    cout << "Copy Assignment: " << (end - start) << " milliseconds." << endl;

    start = ct.milliseconds();
    list = std::move(list3);
    end = ct.milliseconds();

    cout << "Move Assignment: " << (end - start) << " milliseconds." << endl;

}

Note: Your attempt at measuring move assignment was wrong. You were measuring the time required to create the list then move it. So I removed that output.


Linux Commands in Python I've decided to write some Linux commands in Python. Below is a list, along with some constraints (if you're unfamiliar with Linux, the top of the program along has a description about each command and what it does):

ls: No constraints, just lists all files/directories in current working directory. No flags.
cd: Can only pass .. or another directory. No flags.
tree: No flags or directories can be passed.
clear: No constraints. No flags.
whatis: Only defined commands can be passed. No flags.
cat: Requires a full path to the file. No flags.

I would like feedback on all the functions embodied by the program below, but whatis in particular. I feel there is a better way than checking each individual function. Any and all recommendations are appreciated.

"""
This is a program to simulate a command line interface

Commands to reimplement:
    - ls ( lists all files and directories in current directory )
    - cd [directory] ( changes directory )
    - tree ( lists all files, directories, sub directories and files from current directory )
    - clear ( clears the console screen )
    - whatis [command] ( gives a description about the command )
    - cat [file] ( outputs the content of the file )
"""

import os

def ls() -> None:
    """
    Lists all files and directories in current directory
    """
    current_directory = os.getcwd()
    for _, directories, files in os.walk(current_directory):
        for file in files:
            print(file)
        for directory in directories:
            print(directory)

def cd(directory: str) -> str:
    """
    Changes directory
    """
    current_directory = os.getcwd()

    if directory == "..":
        current_directory = current_directory.split("/")[:-1]
        current_directory = ''.join(f"{x}/" for x in current_directory)
        return current_directory
    if os.path.isdir(directory):
        return directory
    return "Not a directory"

def tree() -> None:
    """
    Lists all files, directories, sub directories and files from current directory
    """
    current_directory = os.getcwd()
    tab_count = 1
    print(current_directory.split("/")[-1])
    for _, directories, files in os.walk(current_directory):
        for file in files:
            print("|" + ("-" * tab_count) + file)
        for directory in directories:
            print("|" + ("-" * tab_count) + directory)
        tab_count += 1

def clear() -> None:
    """
    Clears the console screen
    """
    print("\n" * 100)

def whatis(command: str) -> None:
    """
    Prints a description about the passed command
    """
    if command == "ls":
        print(ls.__doc__)
    elif command == "cd":
        print(cd.__doc__)
    elif command == "tree":
        print(tree.__doc__)
    elif command == "clear":
        print(clear.__doc__)
    elif command == "whatis":
        print(whatis.__doc__)
    elif command == "cat":
        print(cat.__doc__)
    else:
        print("Not a valid command!")

def cat(file_path: str) -> None:
    """
    Accepts a path to a file, and outputs the contents of that file
    """
    if os.path.exists(file_path):
        with open(file_path, "r") as file:
            print(''.join(line for line in file))
    else:
        print("Not a file!")

Here are a few comments. In general, your code deviates in surprising ways from the UNIX commands (and not just by missing flags or options):

Your ls command does more than you claim it does. os.walk recursively "walks" down from the current directory, so it returns the content of all subfolders as well (without indicating in which subfolder each file or directory actually is).
When working with paths you should use the library pathlib or at least os.path consistently. This allows your tools to also work in Windows, where the symbol separating paths is \ and not /. You could use os.path.join in your cd function. However, all that code there is not needed, because os.path.isdir("..") is also True. Use os.path.abspath to get the absolute name of a path. In my home directory os.path.abspath("..") returns /home.
Don't use special return values to indicate failure. What if I want to have a directory called "Not a directory"? It may not be the most common name for a directory, but it is at least a plausible name. Instead raise an exception and catch it in the surrounding scope.
Note that your cd does not actually change the directory, so any calls in another function to os.getcwd() (like in ls) will still return the old directory.
Instead of hard-coding 100 blank lines, try to find out how many lines the terminal has, as taken from this answer by @brokkr:

import os
rows, columns = os.popen('stty size', 'r').read().split()

This probably works only on linux, though.
Instead of reading the whole file into memory, joining it and then printing it, you can print it one line at a time. This is slightly slower, but has no memory limitation:

def cat(file_path: str) -> None:
    """
    Accepts a path to a file, and outputs the contents of that file
    """
    if os.path.exists(file_path):
        with open(file_path, "r") as file:
            for line in file:
                print(line, end="")
    else:
        print("Not a file!")

As long as all your functions are defined in the same module, you can simplify your whatis command:

def whatis(command: str) -> None:
    """
    Prints a description about the passed command
    """
    try:
        print(globals()[command].__doc__)
    except KeyError:
        print("Not a valid command!")

Although this does open you up slightly to a user getting access to the __doc__ attribute of any object in the global namespace (not sure if that is a security risk, though). To avoid this, you can also built a white-listed dictionary:

DOCS = {f.__name__: f.__doc__ for f in [ls, cd, clear, tree, whatis, cat]}

def whatis(command: str) -> None:
    """
    Prints a description about the passed command
    """
    try:
        print(DOCS[command])
    except KeyError:
        print("Not a valid command!")

I think the docstrings should be in the same style as linux manpages, i.e. like you have in your module docstring.


General purpose input validation GUI I decided to build a GUI that would seemlessly create user interfaces for validation functions, functions that check if a string satisfies given rules.

An example 'hello, world!' usage is:

from gui_validation import gui_validation

def is_valid_salutation(salutation):
    """
    Salutations to be valid must start with one of:
         ['hello', 'hi', 'howdy'] + ',' [COMMA]
    and must end with '!' [EXCLAMATION MARK]

    >>> is_valid_salutation('howdy, moon!')
    True
    >>> is_valid_salutation('random phrase')
    False
    """
    return any(salutation.startswith(i+',') for i in ['hello', 'hi', 'howdy']) \
           and salutation.endswith('!')

if __name__ == "__main__":
    gui_validation(is_valid_salutation)

As you can see the only argument required is the function itself.

gui_validation.py

"""
General purpose user input validation GUI.
"""
import tkinter
from tkinter.constants import X
from tkinter import messagebox

def gui_validation(validator, title=None, text=None, button_text="Validate"):
    """
    Provides a general purpose gui to validate user input.
    The user will be prompted to enter a string and will be
    given feedback by the `validator` function.

    This interface avoids verbosity and assumes the title to be
    the `validator` name and the text to be the `validator` doc if not +
    told explicitly.
    """
    if title is None:
        title = validator.__name__.replace('_',' ').capitalize()
    if text is None:
        text = validator.__doc__

    def validate():
        if validator(user_input.get()):
            messagebox.showinfo("Input valid.",
                                "Congratulations, you entered valid input.")
        else:
            messagebox.showerror("Input NOT valid.",
                                 "Please try again and enter a valid input.")

    root = tkinter.Tk()
    root.wm_title(title)

    title_label = tkinter.Label(root, text=title, font='25px')
    title_label.pack(fill=X, expand=1)

    text_label = tkinter.Label(root, text=text, font='20px')
    text_label.pack(fill=X, expand=1)

    user_input = tkinter.Entry(root)
    user_input.pack()

    button = tkinter.Button(root, text=button_text, command=validate)
    button.pack()

    root.mainloop()

I was wondering:

Would a class make the code clearer or just more verbose?
Am I asking too little, should I force the user to give more details?
Is it OK to use big fonts? The small fonts are much less legible to me.
Any other improvement?

If the goal is "general purpose" I would be inclined to start from a much simpler premise, allowing users (i.e. developers) to build GUIs with the features that they need. Using object-oriented techniques you could subclass the tkinter widgets and extend them as needed. For example:

import tkinter as tk
from tkinter import messagebox

class ValidEntry(tk.Entry):

    def __init__(self, *args, validator=lambda text: True, **kwargs):
        super().__init__(*args, **kwargs)
        self.validator = validator

    def get(self):
        text = super().get()
        if not self.validator(text):
            raise ValueError('Invalid input')
        return text

This is now a reusable component; it can be dropped in wherever a regular tk.Entry is required (indeed the default validator allows it to be a direct replacement if validation isn't actually needed). The next step up is then:

class ValidatorFrame(tk.Frame):

    INSTRUCTION_FONT = '20px'

    def __init__(self, root, *args, button_text='Validate', instructions=None,
                 validator=lambda text: True, **kwargs):
        super().__init__(root, *args, **kwargs)
        if instructions is None:
            instructions = validator.__doc__
        self.instructions = tk.Label(
            root,
            font=INSTRUCTION_FONT,
            justify=tk.LEFT,
            text=instructions
        )
        self.instructions.pack(expand=1, fill=tk.X)
        self.user_input = ValidEntry(root, validator=validator)
        self.user_input.pack(expand=1, fill=tk.X)
        self.validate = tk.Button(root, command=self.validate, text=button_text)
        self.validate.pack()

    def validate(self):
        try:
            text = self.user_input.get()
        except ValueError:
            messagebox.showerror(
                "Input NOT valid.",
                "Please try again and enter a valid input."
            )
        else:
            messagebox.showinfo(
                "Input valid.",
                "Congratulations, you entered valid input."
            )

Again, this is a component that can be easily used elsewhere. One final step outward:

class TestGui(tk.Tk):

    TITLE_FONT = '25px'

    def __init__(self, *args, title=None, validator=None, **kwargs):
        super().__init__(*args, **kwargs)
        self.title_label = tk.Label(self, font=self.TITLE_FONT, text=title)
        self.title_label.pack(expand=1, fill=tk.X)
        self.frame = ValidatorFrame(self, validator=validator)
        self.frame.pack(expand=1, fill=tk.X)
        if title is None:
            title = validator.__name__.replace('_', ' ').capitalize()
        self.wm_title(title)

By building up from basic components, you give yourself and your users a lot more flexibility. You can now launch this (using the validator you've already written) as simply as:

if __name__ == '__main__':
    app = TestGui(validator=is_valid_salutation)
    app.mainloop()

It's slightly more code, but a lot more flexible. One thing I haven't put much thought into is where the text would go once valid...

A few other things to note:

import tkinter as tk saves you repeating a few extra characters all over the place;
You tend to end up with a lot of keyword arguments to widgets, so I keep them in alphabetical order; and
Factoring out the styling information makes it easier to reuse, too.


Indian currency format using C Indian Currency format is 12345 → "12,345" (for odd length) and 123456 → "1,23,456" (for even length). I have included all possibilities such as

Minus sign: "-12,345"
Decimal Point: "-12,345.345" or "12,345.123"
Zero Condition 000000.123 → "0.123"
Minus and Zero Condition '-000000.123' -> "-0.123"

int main()
{
    char        currency[20];
    char        *tmpCurrency=NULL;
    int         length = 0;
    int         j = 0, commas = 0,n=0;
    int  i = 0, cnt = 0;
    char        *s=NULL;
    int        signCount=0;
    puts("Enter the Currency");
    gets_s(currency,20);

    n = strspn(currency+1, "0");                            //to find number of zeros present in the array.

    if (currency[n] == '.' || currency[n]=='\0' )         //for Condtion like 0000000 or 000000.123 so as to maintain one zero so that o/p will be 0 or 0.123
    {

        memmove(currency, currency + n, strlen(currency));
    }
    else
    {
        memmove(currency, currency + n, strlen(currency));
    }

signCount==1?currency[0]='-':NULL;

    (s = strchr(currency, '.')) ? (length = s - currency) : (length = strlen(currency));   // Considering decimal Point and restricing to Put commas after decimal Point

    if (length>3)
    commas = (length / 2) - 1;                                                            // No of Commas to put

    tmpCurrency = (char*)malloc((length + commas+1) * (sizeof(char*)));

    while (currency[i] != '\0')
    {
        if (cnt == commas)   //maintaining count to check how many commas have been put in the character array
        {
            tmpCurrency[j] = currency[i];
            i++;
            j++;
            continue;
        }
          if (length % 2 == 0)                            //When length is even comma comes after 1 digit eg 1,23,456. so what i have done adjusting length ann checking when it is becoming even and putting comma.
        {
            tmpCurrency[j] = currency[i];
            if (currency[i] != '-' && currency[i+1]!='.')
            {
               tmpCurrency[j = j + 1] = ',';
            }
            i++;
            j++;
            length = length - 1;
            cnt++;
         }
        if (length % 2 != 0)         //for odd length putting one element in tmpCurrency and then subtracting length to make it even.
        {
            tmpCurrency[j] = currency[i];
            i++;
            j++;
            length = length - 1;
        }
    }

    tmpCurrency[j] = '\0';
    strcpy_s(currency,20, tmpCurrency);
    puts(currency);
    free(tmpCurrency);

    return 0;
}

Allocation woes. char *tmpCurrency needs enough memory for a null character terminated string.  This implies a +1 to the length + commas addition.  The cast of malloc() is not needed.  Multiplying by the size of a pointer sizeof(char*) is wrong.  It should be by the size of an element of tmpCurrency, which is a char.  length,commas should be size_t rather than int, but that make s little difference  with small strings.

// tmpCurrency = (char*) malloc((length + commas) * (sizeof(char*)));
tmpCurrency = malloc(sizeof *tmpCurrency * (length + commas + 1));

Separate out functional code from the test driver.  I'd expect some stand-alone function Devious_ICF(char *dest, const char *src) and then code in main() testing it.
Questionable space size and incorrect use of gets_s(char *s, rsize_t n).  Avoid coding naked magic numbers like 20

// char        currency[20];
// gets_s(currency);

#define CURRENCY_SIZE 20
...
char currency[CURRENCY_SIZE];
gets_s(currency, sizeof currency);

stdout is not certainly output as a prompt before asking for input.  To maintain portability, flush.

puts("Enter the Currency");
fflush(stdout);  // add
gets_s(...);

Curious use of static.  static not needed here.

// static int  i = 0, cnt = 0;
int  i = 0, cnt = 0;

Accessing code outside array bounds.

// memmove(currency, currency + n, strlen(currency));
memmove(currency, currency + n, strlen(currency) - n);
currency[n] = '\0';

Use of ?: does not certainly use a consistent type as the types of currency[0]='-' and NULL may differ.  An if() would be better anyways.

// signCount==1?currency[0]='-':NULL;
if (signCount==1) {
  currency[0] = '-';
}

Format to presentation width.  Use an auto formatter.  Weak formatting employed significantly detracts from code's presentation.

          if (length % 2 == 0)                            //When length is even comma comes after 1 digit eg 1,23,456. so what i have done adjusting length ann checking when it is becoming even and putting comma.

versus

          // When length is even, comma comes after 1 digit e.g. 1,23,456.
          //   so what i have done adjusting length and checking when it is
          //   becoming even and putting comma.
          if (length % 2 == 0)

Incorrect use of errno_t strcpy_s(char * restrict s1, rsize_t s1max, const char * restrict s2);.  Missing parameter.

strcpy_s(currency, tmpCurrency);`

Missing #include headers.
Overall - approach used is excessively complex.

Given above issues that a compiler should have reported, it implies compilation is not done with warning fully enabled or code is using non-standard C.  Recommend fully enabling all warnings.


Hackerrank Sparse Arrays in Swift I solved this problem and was looking for feedback on my code. More efficient, cleaner, more verbose variable names, anything helps.

  There are N strings. Each string's length is no more than 20  characters. There are also Q  queries. For each query, you are given a string, and you need to find out how many times this string occurred previously.

import Foundation

func readIntegers() -> Int {
    return Int(readLine()!)!
}

func sparseArrays() {
    let numOfStrings = readIntegers()
    var arrayOfStrings = [String]()
    for _ in 0..<numOfStrings {
        let string = readLine()!
        arrayOfStrings.append(string)
    }
    let numOfQueries = readIntegers()
    for _ in 0..<numOfQueries {
        let singleString = readLine()!
        let arrayOfSingleString = arrayOfStrings.filter {$0 == singleString}
        print(arrayOfSingleString.count)
    }
}

sparseArrays()

func readIntegers() -> Int {
    return Int(readLine()!)!
}

Now this is confusing! A function called  readIntegers which returns
a single integer? readInteger() or readInt() would be an
appropriate name.

The forced unwrapping is acceptable because the input comes from a
programming challenge with well-defined input. However, finding
errors in your code is easier if you check the optionals explicitly
and print an descriptive error message if the input does not
match the expectations.

And with a short description (using the "markdown format" which is
recognized by the Xcode "Quick Help inspector") it
becomes a reusable function for this and future programming
challenges:

/// Returns an integer read from one line of standard input.
func readInteger() -> Int {
    guard let line = readLine() else {
        fatalError("Unexpected end of input")
    }
    guard let i = Int(line) else {
        fatalError("Invalid integer in input")
    }
    return i
}

The same for reading a single string:

/// Returns a string read from one line of standard input.
func readString() -> String {
    guard let line = readLine() else {
        fatalError("Unexpected end of input")
    }
    return line
}

I am (usually) not a fan of adding type information to a variable
name, arrayOfStrings should just be named strings (as in the
description of the challenge).

Instead of starting with an empty array and adding elements one
can create the array with map(). This also allows us to declare the array as a constant:

let numOfStrings = readInteger()
let strings = (0..<numOfStrings).map {
    _ in readString()
}

singleString is better named query, and arrayOfSingleString
is the array with all strings matching the query:

let numOfQueries = readInteger()
for _ in 0..<numOfQueries {
    let query = readString()
    let matchingStrings = strings.filter { $0 == query }
    print(matchingStrings.count)
}

We are only interesting in the count of matching elements.
To improve the performance, determine the count without creating
an intermediate array:

    let matchingCount = strings.reduce(0) { (accum, str) in
        accum + (str == query ? 1 : 0)
    }
    print(matchingCount)

Another speed-up could be achieved by

Sorting the given strings, this needs to be done only once.
Use binary search to find a query string in the array.
If the string is found, traverse forward and backward through
the array to determine how many strings are equal to the query string.


jQuery dynamic elements like TR and TD, add to HTML table Is it possible to create dynamic TR and TD elements in an HTML table?  Something similar but better than this:

jQuery(document).ready(function() {
  $('button[name="new-title"]').on('click', function(){
    var table = $('table[name="errortitle"]');
    var tr = $('<tr />');
    var td = $('<td />');
    var input = $('<input />').attr({'class' : 'form-control'})
    var button = $('<button />').attr({'class' : 'btn'});
    var checkbox = input.clone().attr({'type' : 'checkbox', 'name' : 'check'});

    var tdId = td.clone().html('-');
    var tdTitle = td.clone().append(input.addClass('formInput').attr({'type': 'text'}));
    var tdCheckBox = td.clone().append(checkbox);
    var tdAction = td.clone().html(button.addClass('btn-danger').html('Usuń'));

    tr.append(tdCheckBox);
    tr.append(tdId);
    tr.append(tdTitle);
    tr.append(tdAction);

    table.append(tr);
  });
});

Is it possible to make this code nicer or more efficient?

You could create a function that will add a row to the table based on the number of elements of data you pass to it (which will become columns):

function newRow($table,cols){
    $row = $('<tr/>');
    for(i=0; i<cols.length; i++){
        $col = $('<td/>').append(cols[i]);
        $row.append($col);
    }
    $table.append($row);
}

You can then call this function for every new row you want to make. $table expects a jQuery object, cols expects an array of elements (DOM or jQuery) to append to the row. I've also tidied up the jQuery that creates your elements. Did you know you can use the second parameter in the jQuery function to set element properties?:

jQuery(document).ready(function() {
  $('button[name="new-title"]').on('click', function(){
    var table = $('table[name="errortitle"]');

    var tdId = '-';
    var tdTitle = $('<input />', {'class' : 'form-control formInput', 'type': 'text'});
    var tdCheckBox = $('<input />', {'class' : 'form-control', 'type' : 'checkbox', 'name' : 'check'});
    var tdAction = $('<button />', {'class' : 'btn btn-danger', html:'Usuń'});

    newRow(table,[tdCheckBox,tdId,tdTitle,tdAction]);
  });
});

JSFiddle

Try it with any amount of column data:

newRow(table,[tdCheckBox,tdId,tdTitle,tdAction,'another col','and another','a final col']);

JSFiddle


32-bit factorial calculator in x86 NASM assembly This is how it handles input:

if negative, terminate program
if 0-2, display input without calculating
if greater than or equal to 3, calculate and display result

I have also used some macros from an included file:

nwln: outputs a newline
PutStr: displays a label
GetLInt: receives a 32-bit integer
PutLInt: displays a 32-bit integer

My questions:

Is my input validation reducing readability, or is that not important as long as the program proceeds with only the correct input?
Should I need to check for non-numerical values, or does it not really matter?
Since the result register (EAX) is 32-bit, should I take into account a possible result that may not fit in 32 bits?  If I get a larger result, it won't display properly, and the user may be unaware.
Is doing a bunch of adds still faster than using mul?  I'm also avoiding mul because I still have trouble getting all the bits in the right place.
Should any of these procedures be organized differently?  I'm still not sure how important this is in assembly when most procedures have jumps or branches.  In this program, it makes sense to have check_input after get_input and with no jump in between them.

Other than that, I'd like a review on anything you may find.

%include "macros.s"

.DATA

input_lbl:       DB   "Number: ", 0
bad_input_lbl:   DB   "Number must be positive!", 0
result_lbl:      DB   "Total: ", 0

input:    DD   0
result:   DD   0

.CODE
.STARTUP

; EAX - input
; EBX - input (for counter)
get_input:
   PutStr    input_lbl
   GetLInt   [input]
   mov       EAX, [input]
   mov       EBX, [input]

; EBX - input
check_input:
   ; terminate if input is negative
   cmp   EBX, 0
   jl    report_bad_input

   ; no need to calculate if below 3
   cmp    EBX, 3
   jge    calc_factorial

   ; input is 0, 1, or 2
   jmp    display_result

report_bad_input:
   PutStr   bad_input_lbl
   .EXIT

; EAX - result
display_result:
   nwln
   mov       [result], EAX
   PutStr    result_lbl
   PutLInt   [result]
   .EXIT

; EAX - current value
; EBX - counter
; ECX - current total (for multiplication)
; EDX - counter (for multiplication)
calc_factorial:
   ; no need to multiply by 1
   dec   EBX
   cmp   EBX, 1
   je    display_result

   ; perform addition instead of multiplication
   mov   ECX, EAX
   mov   EDX, EBX
   jmp   mult_with_addition

   jmp   calc_factorial

; EAX - current total
; ECX - current total (for multiplication)
; EDX - counter (for multiplication)
mult_with_addition:
   ; determine if addition should stop
   dec   EDX
   cmp   EDX, 0
   je    calc_factorial

   add   EAX, ECX

   jmp   mult_with_addition

Sample tests:

  Input: 3
  Output: 6

  Input: 2
  Output: 2

  Input: 1
  Output: 1

  Input: 0
  Output: 0

  Input: -1
  Output: "Number must be positive!"

There are a couple of things that may be of use to you for this code, and some general comments that should be useful for any assembly language programs you write in the future.

Make it modular

The code that implements the factorial routine does a je display_result to exit, but that needlessly ties it to the display routine.  In assembly language, it's generally better to use either a ret to return (implying it's written as a subroutine) or to have the routine "fall through" the end.  Either is acceptable, but implementing as subroutines is more common.

Prefer subroutines to macros

The macros you've got might, underneath, result in a subroutine call to some code other than that which you've posted, or more likely, it implements directly the code used to perform those functions.  There might be a slight performance advantage to replicating the code multiple times via a macro invocation, but more often, it just leads to code bloat.  (Although even with this "bloat" it's probably smaller than the corresponding C program!)

Comment fully

If we look at the comment for calc_factorial it currently reads this way:

    ; EAX - current value
    ; EBX - counter
    ; ECX - current total (for multiplication)
    ; EDX - counter (for multiplication)

This may be accurate, but which are inputs?  Which are outputs?  What does the routine do?  I'd recommend a comment style something more like this:

    ;****************************************************************************
    ;
    ; PrintString
    ; prints the string at DS:DX with length CX to stdout
    ;
    ; INPUT:     ds:dx ==> string, cx = string length
    ; OUTPUT:    CF set on error, AX = error code if carry set
    ; DESTROYED: ax, bx
    ;
    ;****************************************************************************

Use mul rather than add

The mul instruction is definitely going to be faster than the corresponding loop with add instructions.  The mul instruction as a 32-bit instruction multiplies EAX by some other register, yielding a result in EDX:EAX meaning that the high part of the result will be in EDX and the low 32 bits of the result will be in EAX.

Structure code to avoid unconditional jumps

If we look at the code, (with comments removed for clarity) we can see that the code does a jmp mult_with_addition but it's jumping over an instruction that cannot be reached!  For that reason, you could eliminate both jmp instructions.

    calc_factorial:
       dec   EBX
       cmp   EBX, 1
       je    display_result

       mov   ECX, EAX
       mov   EDX, EBX
       jmp   mult_with_addition

       jmp   calc_factorial

    mult_with_addition:
       dec   EDX
       cmp   EDX, 0
       je    calc_factorial

       add   EAX, ECX

       jmp   mult_with_addition

Minimize register moves

As much as practical, try to minimize register moves, preferring instead to have the data already in the register you need for instructions or subroutines which have register-specific needs.  For example, because the mul instruction uses EDX and EAX, it makes sense to try to make sure that the number being multiplied is already in that register pair.

Don't fail silently

As with any code in any language, failing silently must be avoided.  In the case of a factorial operation, an overflow condition should be checked and passed back to the calling code.  If you use the mul instruction, and return a 32-bit result in EAX, checking for overflow is as simple as checking EDX for a zero value.

Look carefully at instruction encodings

In one place in your code, you have the instruction cmp EDX,0 followed by a conditional jump based on the Z flag.  In 32-bit mode, that instruction is probably coded as 83FA00 but if you had used or EDX,EDX it is likely to be coded as 09D2 which is one byte shorter.  However, neither are really needed because the dec EDX instruction before it (encoded as 4A) already sets the Z flag. There may be no reason to care about one or two bytes one way or the other, but if that's the case, why code in assembly at all?  If you're going to do it, do it well.


C program to write 5 lines to a file then print the lines except the second line This will read 5 lines into a file from stdin, then print all the lines except the second one.

I used an array because I'm not sure how to do dynamic memory allocation.

#include <stdio.h>
#include <stdlib.h>
#define MAX_SIZE 1024

void write_to_file(const char *s);
void print_file(const char *s);

int main(void){
    const char file_name[] = "text";
    write_to_file(file_name);
    print_file(file_name);

    return 0;
}

void write_to_file(const char *s){
    FILE *in_file;
    if((in_file = fopen(s, "w")) == NULL){
        perror(s);
        exit(EXIT_FAILURE);
    }

    char buffer[MAX_SIZE];
    for(int i = 0; i < 5; ++i){
        fgets(buffer, sizeof(buffer), stdin);
        fputs(buffer, in_file);
    }

    fclose(in_file);
}

void print_file(const char *s){
    FILE *in_file;
    if((in_file = fopen(s, "r")) == NULL){
        perror(s);
        exit(EXIT_FAILURE);
    }
    int c;
    int flag = 0;
    while((c = fgetc(in_file)) != EOF){
        printf("%c", c);
        if((flag == 0) && (c == '\n')){
            while((c = fgetc(in_file)) != '\n'){
                /*skip the second line*/
            }
            flag = 1;
        }
    }
}

You have the right idea in write_to_file(), but print_file() is awkward.

There is no need to work a character at a time in print_file(). You can read a line at a time using fgets(buffer, sizeof(buffer), in_file).

Avoid flag variables. Here, you could use a for loop to track the line number instead.

char buffer[MAX_SIZE];
for (int i = 0; fgets(buffer, sizeof(buffer), in_file) != EOF; ++i) {
    if (i != 1) puts(buffer);
}


User controller for a .net core WebAPI dating app I have this user controller that follows the repository pattern.  It works perfect, but is it easy to understand? Is this good quality work?

using System;
using System.Collections.Generic;
using System.Security.Claims;
using System.Threading.Tasks;
using AutoMapper;
using DatingApp_API.Data;
using DatingApp_API.DataTransferObjects;
using DatingApp_API.Helpers;
using DatingApp_API.Models;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Mvc;

namespace DatingApp_API.Controllers
{
    [ServiceFilter(typeof(LogUserActivity))]
    [Authorize]
    [Route("api/[controller]")]
    [ApiController]
    public class UsersController : ControllerBase
    {
        private readonly IDatingRepository _repo;
        private readonly IMapper _mapper;
        public UsersController(IDatingRepository repo, IMapper mapper)
        {
            _mapper = mapper;
            _repo = repo;
        }

        [HttpGet]
        public async Task<IActionResult> GetUsers([FromQuery]UserParams userParams)
        {
            var currentUserId = int.Parse(User.FindFirst(ClaimTypes.NameIdentifier).Value);

            var userFromRepo = await _repo.GetUser(currentUserId);

            userParams.UserId = currentUserId;

            if(string.IsNullOrEmpty(userParams.Gender))
            {
                userParams.Gender = userFromRepo.Gender == "male" ? "female" : "male";
            }

            var users = await _repo.GetUsers(userParams);
            var usersToReturn = _mapper.Map<IEnumerable<UserForListDTO>>(users);

            Response.AddPagination(users.CurrentPage, users.PageSize, users.TotalCount, users.TotalPages);
            return Ok(usersToReturn);
        }

        [HttpGet("{id}", Name = "GetUser")]
        public async Task<IActionResult> GetUser(int id)
        {
            var user = await _repo.GetUser(id);
            var userToReturn = _mapper.Map<UserForDetailedDTO>(user);

            return Ok(userToReturn);
        }

        [HttpPut("{id}")]
        public async Task<IActionResult> UpdateUser(int id, UserForUpdateDTO userForUpdateDTO)
        {
            if(id != int.Parse(User.FindFirst(ClaimTypes.NameIdentifier).Value))
            return Unauthorized();

            var userFromRepo = await _repo.GetUser(id);

            _mapper.Map(userForUpdateDTO, userFromRepo);

            if(await _repo.SaveAll())
                return NoContent();

            throw new Exception($"Updating user {id} failed on save");
        }

        [HttpPost("{id}/like/{recipientId}")]
        public async Task<IActionResult> LikeUser(int id, int recipientId)
        {
            if(id != int.Parse(User.FindFirst(ClaimTypes.NameIdentifier).Value))
                return Unauthorized();

            var like = await _repo.GetLike(id, recipientId);

            if(like != null)
                return BadRequest("You already like this user");

            if(await _repo.GetUser(recipientId) == null)
                return NotFound();

            like = new Like
            {
                LikerId = id,
                LikeeId = recipientId
            };

            _repo.Add<Like>(like);

            if(await _repo.SaveAll())
                return Ok();

            return BadRequest("Failed to like user");
        }

    }
}

Errors

An assumption: you never ever want to return a 500 HTTP status code (Internal server error). It may leak information you do not want to expose and it's not informative enough.  Do not throw any exception: return the correct HTTP status code and eventually supplement it with the an appropriate error message. For example if SaveAll() failed because a concurrent request you may return 409 (Conflict). This leads to two more points:

In your code you should almost never catch Exception and never throw it. There are many, more informative, exceptions to use which will play well with your exception handling policy (do you have one, right?).
SaveAll() returning a boolean just to throw an exception is, TBH, sub-optimal. If it's an error condition then it should be an exception (and caller MUST handle it, if it can). A boolean return value can too easily be ignored (and you don't want this to happen if data has not been saved).

You're not catching any exception, if anything happens with the repository then API calls will fail with an internal server error.

Reuse

You parse the current user ID in many functions, move it out to a separate function. Also do not use Int32.Parse() without specifying the culture you want to use (probably CultureInfo.InvariantCulture).

Security

Do not use UserParams both in your repository and in your controller. It smells because:

You can't change them independently.
You have unused values you can specify in the query.
If you add a new field and you forget to handle it in a controller method you might end up with a vulnerability.

ALWAYS validate and translate request parameters (and the easiest way is to have two separate objects). For example userParams.Gender is directly exposed to your repository as an unvalidated string.

UpdateUser() (and LikeUser()) does not actually need the user ID as parameter. Do not require useless and redundant data from the client (which must be validated) when you already have it server-side.

Gender

Do not compare strings using == unless you exactly know what it means. In this case an even simpler and faster ordinal comparison is enough however...

Do not store the gender as a string. If your app is limited to binary genders (at birth) then you may use an enum, however...

Gender is much more complex than a simple binary identity:

Gender at birth is not necessarily binary.
Gender at birth may not be the current gender.
The assigned gender at birth may not be what a person identifies with.
Especially in a dating app it's also important the sexual orientation (which is obviously independent from gender).

In short: do not use an hard-coded list and do not assume gender equals sexual orientation.

Performance

If your app will be successful then you definitely want to avoid to search the likes table (which will grow much faster than the users table) for an entry and to retrieve it only to return Bad request. Best case scenario is to have a simpler function to determine if the entry exists instead of reading, transmitting and mapping the entry itself. See also the next section...

Design

I'm generally against the indiscriminate use of the Repository pattern. It's an invaluable tool but the price you pay in complexity to use it (properly) must be justified. I do not see the big picture here but you should seriously consider if directly using your ORM is enough. From what I see you really need a domain model, much more than a repository.

You have an instance field _repo, you do not show that code but I do not see any synchronization mechanism and I do not see how you dispose its resources when controller is re-created. Generally, in your controllers, you should avoid instance fields as much as possible.

Your API should never ever work directly with the repository to perform any business logic. It should interact with an high-level model (or a Service layer on the top of the Data layer exposed by your ORM). For example I'd imagine the Like feature like this:

public async Task<IActionResult> LikeUser(int recipientId)
{
    using (var service = _serviceFactory.Create())
        await service.AddLike(GetCurrentUserId(), recipientId);

    return Ok();
}

Yes, controller methods should not contain much more than this. All the logic must be TESTABLE then it is much easier to have it elsewhere. Note that in this fictional example I used a factory to create the service object: you may get it directly from your DI framework in the controller ctor but do not create it here (...new MyService()) because you want to test the controller with a mocked service.

Note that the AddLike() is probably implemented with a stored procedure, you really need performance here. Given that user should not be able to click Like for someone he already liked then when it happens it's definitely an error condition and as such it should trigger an exception. Code is probably more complex than this because of error handling but I think you now got the point:

try
{
    ...
}
catch (ItemNotFoundException e)
{
    return NotFound(...);
}
catch (ConcurrencyException e)
{
    // TODO: try again?
}

If you structure well your service layer then you can probably move all this boilerplate code into a function method and reuse it in all controller methods.


Basic number property checker I made a small program that tests numbers against some properties:

Welcome to Numbers!
Is it odd? 4
No
Is it a multiple of 18? 38
No
Is it big? 100
Yes

That's basically it :)

My code is:

import System.IO
import Text.Read

askUser :: String -> IO Double
askUser str = do
  putStr str
  num <- fmap readMaybe getLine

  case num of
    Nothing -> putStrLn "That is not a number!" >> askUser str
    Just num -> return num

numberCheck :: [Double] -> String -> IO ()
numberCheck list text = do
  num <- askUser ("Is it " ++ text ++ "? ")

  if num `elem` list
    then putStrLn "Yes"
    else putStrLn "No"

main :: IO ()
main = do
  hSetBuffering stdout NoBuffering
  putStrLn "Welcome to Numbers!"
  let alot = 99999

  let oddNum = take alot [x | x <- [0..], odd (round x)]
  numberCheck oddNum "odd"

  let multiples18 = take alot [x | x <- [0..], round x `mod` 18 == 0]
  numberCheck multiples18 "a multiple of 18"

  let bigNumbers = take alot [99..]
  numberCheck bigNumbers "big"

Is there anything that can be improved? I was especially thinking of the alot number, as it is not a "clean" way of solving this. But if I don't, Haskell will continue checking the list infinitely.

Is there any nicer way of achieving this?

Note: I used lists instead of comparisons because that's what I just learned, and so you can ignore me using lists if you want to :)

Integer not Double

Double is not a reasonable type because checking if a number is big does not work with Doubles, you should use Integer

First class functions

Using a list is not a clean procedure, it does not work for very big numbers and it is slow for big numbers.

It is better to pass in a validation function:

numberCheck :: (Integer -> Bool) -> String -> IO ()
numberCheck predicate text = do
  num <- askUser ("Is it " ++ text ++ "? ")

  if predicate num
    then putStrLn "Yes"
    else putStrLn "No"

Usage is:

main = do
  hSetBuffering stdout NoBuffering
  putStrLn "Welcome to Numbers!"

  numberCheck odd "odd"
  numberCheck (\x -> x `mod` 18 == 0) "multiple of 18"
  numberCheck (> 100) "big (bigger than 100)"


Imitate randomness of a dice roll I have written some code in Java that consists of two classes. I have tried my best and I want you to tell me if I can improve and how good this is.

Also, how important is OOP? Java is an object oriented language, so I have also tried my best to use it that way (although I don't know whether I have succeeded or not).

MainClass

import java.util.Scanner;
class MainClass{
public static void main(String args[]){
    RandomDice dice = new RandomDice();
    Scanner scan = new Scanner(System.in);

    System.out.print("Please enter the number of times you want to roll the dice:");
    int arg0 = scan.nextInt();

    dice.roll(arg0);
    }
}

RandomDice

/*This program will help you to imitate the randomness of a dice roll,
 *and to represent the frequency of each face appearing in a tabular form.
 */
import java.util.Random;

public class RandomDice{
private int diceRoll;
Random rand = new Random();

    /// the method below takes an argument arg0 which indicates the amount of times the         dice has to be rolled.
    public void roll(int arg0){
    diceRoll = arg0;
    int ObservationArray[] = {0,0,0,0,0,0};
    ObserveRandom(diceRoll,ObservationArray);
    DisplayResult(ObservationArray);
}

///this method creates random numbers from 0 to 5 and respectively stores them in the     array index stimulating a dice being rolled.
public void ObserveRandom(int arg1,int ObservationArray[]){
for(int counter=0;counter<arg1;counter++){
    ++ObservationArray[rand.nextInt(6)];
    }
}
///this method displays the data collected in a tabular form.
public void DisplayResult(int arg1[]){
    System.out.println("face\tfrequency");
    for(int counter=0;counter<arg1.length;counter++){
        System.out.println(1+counter+"\t"+arg1[counter]);
        }
    }
}

Thanks for submitting your code for review — it shows that you care about code quality and you're willing to improve and learn.
Note that OOP stands for object-oriented programming and does not have a plural form ;-)

Alexandre has given you some good advice and one possible implementation. I'd like to critique your code a bit more thoroughly and offer another, more lightweight possibility.

Criticism

Concept

An important idea in software development is that code should be reusable. Your code seems like a specific application of a general idea that applies not only to throwing the dice but just as well to tossing a coin or drawing a card (and gathering statistics).

So you should consider designing your code in a way that allows it to work for all these (and other) scenarios. That is to say, the number of elements in the input sequence (currently 6) and the element names themselves (currently {1,2,3,4,5,6}) should be configurable by the caller of the code.

Separation of concerns

I'll keep this part short since Alexandre has already elaborated on it. Just remember a few key points:

Never put any user interface related code into your core business logic (RandomDice class)
If you mix up these concerns, you'll find it hard to reuse your code for applications based on other UI frameworks than the Console (for instance, Swing, SWT or AWT or Servlets).
Instead of outputting data, use return values
Just replace occurrences of System.out.println with an appropriate return value (e.g. String)
Choose one responsibility per object and refactor others into new objects
The class that calculates the statistics should not be in charge of formatting the output —  you need to introduce a new Collaborator (an object that encapsulates that responsibility)

Comments

Use them sparingly or they will cause clutter and be ignored
Check your spelling and grammar

amount of times should be number of times
stimulating should be simulating

Format them properly
For summaries above class and method definitions (don't put them between package and import but directly above the relevant code!) always use the same (Javadoc) syntax:

/**
 * This program will help you to imitate the randomness of a dice roll,
 * and to represent the frequency of each face appearing in a tabular form.
 */

Choose better names to avoid writing a comment

Naming

Be expressive and avoid junk names such as arg0 or arg1
Instead of the following obsolete comment:

// / the method below takes an argument arg0 which indicates the amount of
// times the dice has to be rolled.
public void roll(int arg0) {
    diceRoll = arg0;

Why not get rid of the field diceRoll and write

public void roll(int numberOfTimes) {

Avoid stating the obvious (that applies to comments, too!)
Dice can reasonably be assumed to be random, so RandomDice is unnecessary
MainClass also states the obvious, just use Main
Follow conventions (methods and variables are camelCase, classes are PascalCase)

Also

Always specify access modifiers
Use the most restrictive access modifier possible (in your code, rand was accessible from other classes!):
Don't declare your arrays C-Style (keep the data type before the variable instead)

int ObservationArray[] = { 0, 0, 0, 0, 0, 0 }; // don't do this
int[] ObservationArray = { 0, 0, 0, 0, 0, 0 }; // this form is clearer

Alternative implementation

By refactoring step-for-step and slowly introducing changes, I arrived at the implementation shown below that aims to illustrate the points made in this answer. RandomDice was renamed to DistributionCalculator and is now generic (it can work with any type of elements, as illustrated in the Usage section). It accepts an Array of elements in the static convenience method with. After initialization, the frequencies can be calculated by calling calculateDistribution.

That method returns a Distribution<K,V> which is the class that encapsulates the string formatting (I went ahead and made the output a bit more pretty).

The naming could probably be more accurate from a mathematical perspective — go ahead and rename things if you feel that these terms aren't quite correct (probably UniformDistributionCalculator or something would have been more exact).

The frequencies are now stored as longs to allow for bigger calculations, though that can be changed easily if you don't agree with the decision.

If something requires further explanation, go ahead and ask in the comments.

Usage

public class Main {
    private static final Scanner scan = new Scanner(System.in);
    private static long iterations;

    public static void main(String args[]) {
        System.out.print("Please enter the number of times you want to iterate: ");
        iterations = scan.nextLong();
        test(DistributionCalculator.with(1, 2, 3, 4, 5, 6));
        test(DistributionCalculator.with("hearts", "spades", "clubs", "diamonds"));
        test(DistributionCalculator.with(true, false, null));
    }

    private static <T> void test(DistributionCalculator<T> calculator) {
        System.out.println(calculator.calculateDistribution(iterations));
    }
}

Output

 Please enter the number of times you want to iterate:  200000

 element             frequency
 1                   33129
 2                   33156
 3                   33539
 4                   33469
 5                   33346
 6                   33361

 element             frequency
 hearts              50100
 diamonds            49857
 spades              49752
 clubs               50291

 element             frequency
 null                66560
 false               66726
 true                66714

Calculation of frequencies

public class DistributionCalculator<T> {

    private static final Random random = new Random();
    private final Map<T, Long> map = new HashMap<T, Long>();
    private final T[] keys;

    public static <T> DistributionCalculator<T> with(T... elements) {
        return new DistributionCalculator<T>(elements);
    }

    private DistributionCalculator(T... elements) {
        keys = elements;
        for (T key : keys) {
            map.put(key, Long.valueOf(0));
        }
    }

    public Distribution<T, Long> calculateDistribution(long numberOfIterations) {
        for (long i = 0; i < numberOfIterations; i++) {
            T key = keys[random.nextInt(keys.length)];
            map.put(key, map.get(key) + 1);
        }
        return new Distribution<T, Long>(map);
    }
}

Result formatting

public class Distribution<K, V> {
    private final Map<K, V> map;
    private final String table;

    public Distribution(Map<K, V> distribution) {
        map = distribution;
        table = generateTable();
    }

    public Map<K, V> asMap() {
        return map;
    }

    @Override
    public String toString() {
        return table;
    }

    private String generateTable() {
        final String line = "%n%-20s%s";
        String result = String.format(line, "element", "frequency");
        for (K key : map.keySet()) {
            // it would be better to use a StringBuilder if you have a lot of keys
            result += String.format(line, key, map.get(key));
        }
        return result;
    }
}


Base Conversion in Node.js I am doing a program for my APCS class in Node.js to practice node, but I am unsure if I did everything the best way possible.

The assignment is to take the number and base as user input and then perform the conversion and print it out.

Here is the code:

const readline = require('readline');

const rl = readline.createInterface({
    input: process.stdin,
    output: process.stdout
});

const BASE_CHARACTERS = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz/=";

exports.pa11 = function() {
    rl.question("Enter number: ", number => {
        rl.question("Enter base: ", base => {
            if (base <= 1) throw "Base must be greater than 1";
            else if (base > 64) throw "Base must be less than or equal to 64";

            const numberOriginal = number, baseOriginal = base;

            const negative = number < 0;
            if (negative) number = -number;

            let x = "";

            while (number > 0) {
                let digit = number % base;
                number -= digit;
                number /= base;
                x = BASE_CHARACTERS.charAt(digit) + x;
            }

            if (negative) x = "-" + x;

            console.log(numberOriginal, "in base", baseOriginal, "is", x);

            rl.close();
            process.exit();
        });
    });
};

Testing, vetting, and redundancy.

First you have not clearly defined the requirement. Base64 is an encoding used to convert bit/byte streams for safe transport. It is not the same as n-based number systems.

I will assume you want n-based number systems

Your code does not do a conversion that makes any sense at all, you should test your code before you release it.

Anyways, the easy solution up to base 36 is:

if(base <= 36){
    value = parseInt(number, base);
}

but I don't think that is what the lesson is designed to teach. So here is my assessment, and it would be a fail as it stands.

Throw Error objects not strings.

When you throw you should always throw an error object. One of the appropriate Error object types see the MDN documentation for Error.

You may not be handling the thrown errors and if the error handler does not expect a string it to may throw an error attempting to access non existing properties.

Compound declaration with assignment.

Generally I don't like compound declarations that have assignments, though I do make exception to last item assignment. I find it even less readable if you are inconsistent in the use.

Example of compound declaration and assignment:

// Bad
var a = 0, b = 1, c;
// or
var a = 0,
    b = 1,
    c;

// Good
var a, b, c;
a = 0;
b = 1;

You have:

// bad one line is compound and then you have a single
const numberOriginal = number, baseOriginal = base;

const negative = number < 0;

You should have:

const numberOriginal = number;
const baseOriginal = base;
const negative = number < 0;

Dont create needless noise.

You create the const baseOriginal and assign it the value of base yet at no point do you modify base before you use baseOriginal making it redundant. This just adds complexity where it is not needed making code harder to read and modify.

Show you understand let, var, and const

There is much debate about the use of let, var, and const with some schools of though opting for the use of only let and const.

The problem with this is that when I look at code I am in part assessing the writer's level of expertise and if I see a declaration out of place I wonder if that coder knows what he/she is doing. Once I lose trust in the code the marks fall off much quicker.

I understand that some teach the use of let instead of var - no harm done, right? No!!! var and let create very different behaviours and you should use them appropriately. Creating a block-scoped variable in function scope is incorrect. If you declare in-function scope then you should use var to show you know you are using function scope.

If the variable is never modified you should use const and you should place it as high as possible in the scope that it is needed.

Use let only inside blocks (With the exception of for loops)

Vars are hoisted and thus you should put var declarations where they are declared, first lines after the function declaration. If you don't then it is hard to know what has been defined, and the person reading the code must search for the var. This leads to errors and bugs.

So loud in here.

Noise is anything that adds to the source but serves not purpose. Good code has zero noise (with the exception of style dictated noise, though a note should always provide details on uncommon style and reason for use)

You have:

if (base <= 1) throw "Base must be greater than 1";
else if (base > 64) throw "Base must be less than or equal to 64";

If the first statement is true then the code is terminated at the throw. That makes the else redundant and just noise, so don't use it.

if (base <= 1) { throw new Error("Base must be greater than 1") }
if (base > 64) { throw new Error( "Base must be less than or equal to 64") }

Don't use blockless blocks.

Blockless blocks are a major source of bugs. Not only are they bugs waiting to happen, they are particularly hard to spot. There is a special place in the hell of a billion bugs for the person that introduced that in to languages.

// bad, no worse than bad.
if (something) doThat();
// or even worse than worse than bad.
if (something)
    doThat();
// and an extra notch of bad
if (something)
doThat()

// Good
if (something) { doThat(); }
// or
if (something) { doThat() }
// or
if (something) {
    doThat();
}

Vet your input

The two inputs from the question are strings. You neglect to test if base is NaN, and you do not check whether number contains only valid characters.

if(isNaN(base)) throw new TypeError("Base must be a number from 2-64 inclusive.");

The number should also be tested but as that will depend on the value of base, and to do a proper test means iterating all the characters that you will do in the function logic you should add the error in the while loop when you are doing the conversion.

Be type aware

The argument number is a string and as it is variable base, attempting to coerce it to a number can fail.

You have:

const negative = number < 0;
if (negative) number = -number;

This will not work if number = "-a"

Check for the sign as the first character of the string:

const negative = number[0] === "-";
if (negative) { number = number.substr(1) } // remove the minus sign

Did you test your code?

Looking at the function I instantly see that it will not work.  Your logic is completely wrong. You should have known that because you should have tested it and seen the wrong output for the given input. Always test your code before you let it out of your sight. You don't write half sentences nor should you write broken code. Code is broken until you have tested it not before.

The function as you have it does not allow easy testing. You should separate out the conversion so that you can automatically test it without having to modify your code.

Dont blindly follow the lead.

There is a tendency for node programmers to use nondescript variable names. This is a very bad trend and should be stopped. Out of context what is rl, or fs, only those in the know will understand. If I did not know better, the persistence of these abbreviations in nodeJS code would suggest a form of deliberate elitism.

Use descriptive names for variable. Just because everyone uses a particular abbreviation does not make it a good choice of name.

// bad
const rl = ...

// good
const readLineInterface = ...

This allows everyone to understand what the variable is, not just those in the know.

The rewrite

// Style note. Single line blocks do not include a ; before the closing } this is to comply with jsLint rule
// semicolons should be followed by a new line.

const readline = require('readline');

exports.pa11 = function () {
    const readLineInterface = readline.createInterface({
        input: process.stdin,
        output: process.stdout
    });
    function convertNumber(number, base){
        var value, digit, charIndex;
        const BASE_CHARACTERS = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz/=";

        if (isNaN(base)) { throw new TypeError("Base must be a number from 2 - 64") }
        if (base < 2 || base > 64) { throw new RangeError("Base must be a number from 2 - 64") }

        const numberOrg = number;
        const negative = number[0] === "-";
        if (negative) { number = number.substr(1) } // remove the minus sign

        charIndex = number.length;  // start from least significant digit
        digit = 0;  // the current place of digit
        value = 0;  // the base ten value

        while (charIndex--) {
            const index = BASE_CHARACTERS.indexOf(number[charIndex]);
            if (index === -1 || index >= base) {
                throw new TypeError(`Number ${numberOrg} is not a base '${base}' number.`);
            }
            value += Math.pow(base, digit++);
        }
        value *= negative ? -1 : 1; // if negative negate result

        console.log(`The number ${numberOrg}, in base ${base} has the value ${value} in base 10.`);
        return value;
    }

    readLineInterface.question("Enter number: ", number => {
        readLineInterface.question("Enter base: ", base => {
            convertNumber(number, base);
            readLineInterface.close();
            process.exit();
        });
    });
};

Note

Many markers use automatic plagiarism systems that search the web for copied content. If you copy the above code as-is it is highly likely that your code will be flagged by such systems. You will have to modify it rather than copy and past it.

I have left one missing error check. If the input number string evaluates to a value that can not be accurately expressed as a Javascript Number then the result will be incorrect. You should test the result and ensure it is in safe bounds. Hint: look up Number.MAX_SAFE_INTEGER and its negative counterpart.


Josephus Problem with cyclic iterator Given the Josephus Problem.

  Josephus Problem

  N people (numbered 1 to N) are standing in a circle. Person 1 kills Person 2 with a sword and gives it to Person 3. Person 3 kills Person 4 and gives the sword to Person 5. This process is repeated until only one person is alive.

  Task:
  (Medium) Given the number of people N, write a program to find the number of the person that stays alive at the end.
  (Hard) Show each step of the process.

  (The description from Sololearn application)"

This is my code. The forEachRemaining method solution is correct? I have to do something with this inherited method, but it has no meaning.

import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.function.Consumer;

class CyclicIterator implements Iterator {

private final List list;
private Iterator iterator;

public CyclicIterator(List list) {
    this.list = list;
    initIterator(list);
}

private void initIterator(List list) {
    this.iterator = list.iterator();
}

@Override
public boolean hasNext() {
    return !list.isEmpty();
}

@Override
public Object next() {
    if (!this.iterator.hasNext())
        initIterator(list);
    return this.iterator.next();
}

@Override
public void remove() {
    this.iterator.remove();
}

@Override
public void forEachRemaining(Consumer action) {
    throw new UnsupportedOperationException("This method has no meaning in CyclicIterator class!");
}
}

public class JosephusProblem {

public static void main(String[] args) {
    execution(0);
    execution(1);
    execution(2);
    execution(4);
    execution(6);
}

private static void execution(int members) {
    if (members < 1) {
        System.out.println("The parameter (members) has to be bigger than 0!");
        return;
    }
    if (members == 1) {
        System.out.println("There is olny one person, so he is the survivor. Peaceful version! :)");
        return;
    }
    LinkedList<Integer> list = new LinkedList();
    for (int index = 0; index < members; index++)
        list.add(index + 1);
    Iterator<Integer> it = new CyclicIterator(list);
    System.out.println("For " + members + " members: ");
    while (members-- > 1) {
        System.out.print(it.next() + " kills " + it.next() + ", ");
        it.remove();
    }
    System.out.println("\n The survivor: " + it.next());
}

}

Your indentation is off: there should be a level also after class, not only in methods:

class CyclicIterator {
    // Class members here
}

Always try to instantiate to an interface, not an implementation:
This:

LinkedList<Integer> list = new LinkedList();

Should be:

List<Integer> list = new LinkedList<>();

Note there the diamond operator <> to actually get a genericized list. Without it, you should have got a compiler warning about using raw lists. The diamond operator is shorthand for giving the generic type to the implementation: List<Integer> list = new ArrayList<Integer>();

I would also prefer using System.out.format to just smashing strings together in a println call: System.out.format("For members %s:%n");


Creating a new marker on a map Is this code good practice? What kind of design pattern should I use for this specific situation?

The following function creates a new marker on a map. It sets a new image for it, except when the marker references the user location.

$scope.addMarker = function (lat, lng, isUserLocation) {

    if (typeof(isUserLocation) === undefined || isUserLocation === false)
        var icon = "img/marker-map.png";

    $scope.markers.push({
        latitude: parseFloat(lat),
        longitude: parseFloat(lng),
        icon: icon
    });
}

Maybe just (!isUserLocation) is more readable - is it error prone?

Here's a call to the function to create the "user location" marker:

$scope.addMarker(position.coords.latitude, position.coords.longitude, true);

If someone reads that, he/she wouldn't be able to say what that true is for and would have to look for the addMarker() function, for example. But maybe that's no big deal or is it?

Here's the function that creates all other markers:

$scope.populateMarkers = function(locations) {
    for (var i = 0; i < locations.length; i++) {
        $scope.addMarker(locations[i].Latitude, locations[i].Longitude);
    }
}

This is the HTML / Angular directives that references the markers inside the map:

  <markers>
    <marker ng-repeat="marker in markers" coords="marker" icon="marker.icon">
    </marker>
  </markers>

It works, but since I'm very new to AngularJS and I also thought about many different ways to do just that, I'd like to know how people would do it and/or whether there is a recommended design pattern for it.

Interesting question,

I haven't found an obvious best way to do this.

For sure, sending true as a parameter should always be avoided, I'd rather see

var IS_USER_LOCATION = true;
$scope.addMarker(position.coords.latitude, position.coords.longitude, IS_USER_LOCATION);

though probably I would create 2 functions, 1 called addMarker and 1 called addUserMarker.

Finally, it amused me that the first 2 parameters in both calls are properties of the same object, so why not just ask for a coordinates object ?

$scope.addMarker = function ( coordinates ) {
    $scope.markers.push({
        latitude: parseFloat(coordinates.latitude),
        longitude: parseFloat(coordinates.longitude),
        icon: "img/marker-map.png";
    });
}

$scope.addUserMarker = function ( coordinates ) {
    $scope.markers.push({
        latitude: parseFloat(coordinates.latitude),
        longitude: parseFloat(coordinates.longitude),
    });
}

$scope.addUserMarker(position);

$scope.populateMarkers = function(locations) {
    for (var i = 0; i < locations.length; i++) {
        $scope.addMarker(locations[i]);
    }
}


Shuffle data simulation Is the following procedure good for shuffling data in general (not just for JavaScript)?

function Shuffle(array) {
  var temp = null,
    len = array.length - 1,
    rnd = null;
  for (var i = len; i > 0; i--) {
    // Generate a random number in which does not exceeds the length of array.
    rnd = Math.floor(Math.random() * len);
    // Assign last item in array to "temp" variable.
    temp = array[len];
    // Assign a random number to last item in array.
    array[len] = array[rnd];
    // Assign value of "temp" variable which may we added previously to the array last random item which has been generated previously.
    array[rnd] = temp;
  }
  return array;
}

// Implementation
var arr = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i'];
document.getElementById('result').innerHTML = Shuffle(arr);

Your algorithm is a variant of Fisher-Yates shuffle,
and it's a good algorithm to shuffle a collection.
A more common form is instead of using the last element to swap,
you could have used the decreasing index i to swap,
like this:

  for (var i = len; i > 0; i--) {
    rnd = Math.floor(Math.random() * len);
    temp = array[rnd];
    array[rnd] = array[i];
    array[i] = temp;
  }

It doesn't matter much though, your version is fine too.

Something to improve is a bit cosmetic:
it would be better to name your function with lowercase letters, shuffle,
because it's customary to start with a capital letter only functions that are used to create objects with the new keyword.

Lastly, this is a bit tedious to type:

var arr = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i'];

You could use this shorter form instead:

var arr = "abcdefghi".split("");


Assembler PIC programming, is this correct? The question is:

Write the following pseudo in assembler code:

if portC bit 3 == 0
(switch content of var1 and var2)
else
(add var1 with var2 and place the result in var1)

My answer is:

btfsc portC,3
goto add
movf var1,w
movwf temp
movf var2,w
movwf var1
movf temp,w
movwf var2
return

add movf var2,w
addwf var1,f

Since I dont know the right answer, can you please help me and tell me if I wrote it correct?

A general disclaimer to this answer should be that I don't know the hardware you're working with, but most of this should be applicable to many architectures.

Potential optimization #1

Your code seems correct, but you can make it shorter and potentially more efficient by moving the first memory load before the branch:

movf var1, w     ; We're going to load one of the variables anyway

btfsc portC, 3
goto add

movwf temp
movf var2, w
movwf var1
movf temp, w
movwf var2
return

add addwf var2, f

Performing the load before the branch might mean the branch instruction could be executed at the same time as the load, since they don't share any data.

Potential optimization #2

Using a XOR swap should be more efficient, like so:

movf  var1, w   ; load from var1 to w
xorwf var2, w   ; w = var1 xor var2
xofwf var2, f   ; var2 = w xor var2 = (var1 xor var2) xor var2 = var1
xorwf var1, f   ; var1 = w xor var1 = (var1 xor var2) xor var1 = var2

This should have several advantages:

There is no temporary variable and therefore no reads or writes are associated with it.
The first XOR doesn't perform a memory write so it should be completed faster.
No moving memory around until the variables are assigned their final values, so if the hardware is able to cache the values of var1 and var2 from the first two instructions, the last two could gain a performance boost.

I should state again that I don't know the hardware, so this is mostly educated speculation, but my code should be faster even without assumptions 2 and 3.

Furthermore, there's nothing stopping you from combining both ideas, of course. You probably should.


Computing the standard deviation of a Number array in Java - follow-up (See the first and initial iteration.)

Now I make only one pass over the data in order to compute the standard deviation of the input Numbers, and that's how:

\begin{align}
\sum_{i = 1}^n (x_i - \mu)^2 &= \sum_{i = 1}^n (x_i^2 - 2\mu x_i + \mu^2) \\
                             &= \sum_{i = 1}^n x_i^2 - 2\mu \sum_{i = 1}^n x_i + n \mu^2 \\
                             &= \sum_{i = 1}^n x_i^2 - 2 \Bigg( \frac{\sum_{i = 1}^n x_i}{n} \Bigg) \sum_{i = 1}^n x_i + n \Bigg( \frac{\sum_{i = 1}^n x_i}{n} \Bigg)^2 \\
                             &= \sum_{i = 1}^n x_i^2 - 2 \Bigg( \frac{\sum_{i = 1}^n x_i}{n} \Bigg) \sum_{i = 1}^n x_i + \frac{1}{n}\Bigg( \sum_{i = 1}^n x_i \Bigg)^2 \\
                             &= \sum_{i = 1}^n x_i^2 - \frac{2}{n} \Bigg( \sum_{i = 1}^n x_i \Bigg) \sum_{i = 1}^n x_i + \frac{1}{n}\Bigg( \sum_{i = 1}^n x_i \Bigg)^2 \\
                             &= \sum_{i = 1}^n x_i^2 - \frac{2}{n} \Bigg( \sum_{i = 1}^n x_i \Bigg)^2 + \frac{1}{n}\Bigg( \sum_{i = 1}^n x_i \Bigg)^2 \\
                             &= \sum_{i = 1}^n x_i^2 - \frac{1}{n} \Bigg( \sum_{i = 1}^n x_i \Bigg)^2.
\end{align}

My code is as follows:

StandardDeviation.java:

package net.coderodde.util;

import java.util.Arrays;
import java.util.Collection;
import java.util.Objects;
import java.util.function.Consumer;

public final class StandardDeviation {

    public static double computeStandardDeviation(final Number... array) {
        Objects.requireNonNull(array, "The input number array is null.");

        if (array.length == 0) {
            return Double.NaN;
        }

        MyConsumer myConsumer = new MyConsumer();

        Arrays.stream(array)
              .map(Number::doubleValue)
              .forEach(myConsumer);

        int n = array.length;
        double sum = myConsumer.getSum();
        double sumOfSquares = myConsumer.getSumOfSquares();
        double intermediate = sumOfSquares - sum * sum / n;

        return Math.sqrt(intermediate / (n - 1));
    }

    public static double
            computeStandardDeviation(final Collection<Number> collection) {
        Objects.requireNonNull(collection,
                               "The input number collection is null");
        return computeStandardDeviation(
                collection.toArray(new Number[collection.size()]));
    }

    private StandardDeviation() {}

    private static final class MyConsumer implements Consumer<Double> {

        private double sum;
        private double sumOfSquares;

        @Override
        public void accept(Double t) {
            sum += t;
            sumOfSquares += t * t;
        }

        double getSum() {
            return sum;
        }

        double getSumOfSquares() {
            return sumOfSquares;
        }
    }

    public static void main(String[] args) {
        // Mix 'em all!
        double sd = computeStandardDeviation(Arrays.asList((byte) 1,
                                                           (short) 2,
                                                           3,
                                                           4L,
                                                           5.0f,
                                                           6.0));
        System.out.println(sd);
    }
}

Any critique much appreciated.

Boxing
Your current class MyConsumer implements Consumer<Double>. This means that every time an element is accepted, it will unboxed into a double.
@Override
public void accept(Double t) {
    sum += t;  // <-- unboxed into double
    sumOfSquares += t * t;   // <-- here also
}

On large datasets, this can have a huge performance impact. Instead, it would be preferable to implement the primitive DoubleConsumer interface, and thus change your accept method into
@Override
public void accept(double t) {
    // ...
}

This way, there won't be any overhead involving autoboxing.
This will also involve a change inside your main method. You will need to use mapToDouble instead of map.
Arrays.stream(array)
      .mapToDouble(Number::doubleValue)

This is also a good change to do: currently, each Number would be converted to a double, only to be boxed into a Double by map, then unboxed multiple times inside the consumer.
collect instead of forEach
You aren't using properly the Stream API in your main pipeline:
MyConsumer myConsumer = new MyConsumer();

Arrays.stream(array)
      .map(Number::doubleValue)
      .forEach(myConsumer);

The problem with this code is that it mixed both the functional approach of the Stream API with the traditional iterative forEach. This can generally break in multiple ways when you're using a parallel pipeline.
The Stream API was designed so that parallel capabilities can be introduced effectively. The case of forEach is even mentioned in the documentation:

A small number of stream operations, such as forEach() and peek(), can operate only via side-effects; these should be used with care.
 [...]
the forEach() can simply be replaced with a reduction operation that is safer, more efficient, and more amenable to parallelization.

This is what collect (and the more general reduce) is for.
So let's make that parallel-friendly and take a look at collect(supplier, accumulator, combiner) method.

The supplier is a function returning the new result container.
The accumulator is a function that takes the current result container and the current Stream element and incorporates it into the container.
The combiner is a function that takes two result containers and merges them into one. This argument is only used in case of a parallel pipeline.

To adhere with that contract, we need to change MyConsumer:

MyConsumer isn't really a consumer anymore, it will be a class responsible for collecting the values into statistics having notably a sum and sumOfSquares. Let's rename it StatCollector (for lack of a better name). It does consume double as input so it makes sense to let it implement DoubleConsumer.
As seen by the above signature, it needs to handle a case of merging two containers, so we need to add a method public void combine(StatCollector other) that will be responsible for that.

This is what we end up with:
private static final class StatCollector implements DoubleConsumer {

    private double sum;
    private double sumOfSquares;

    @Override
    public void accept(double t) {
        sum += t;
        sumOfSquares += t * t;
    }

    public void combine(StatCollector other) {
        sum += other.sum;
        sumOfSquares += other.sumOfSquares;
    }

    double getSum() {
        return sum;
    }

    double getSumOfSquares() {
        return sumOfSquares;
    }
}

used with
StatCollector stat =
    Arrays.stream(array)
          .mapToDouble(Number::doubleValue)
          .collect(StatCollector::new, StatCollector::accept, StatCollector::combine);


Downsampling n-dimensional data from bins in one dimension So I'm writing some code to perform a quite specific task given a large numpy array with N rows and 3 columns representing points in 3D. The 3D points are to be binned along one of the dimensions between specified bin edges. For each of these bins, there is a set fraction by which I must reduce the number of points in that bin, perfectly (pseudo)randomly.

Here is the code I have written to perform this task. I found myself spending a lot of time trying to figure out the most 'pythonic' way to achieve this. It still seems very clunky, so I'm sure there must be a more elegant way that capitalises on numpy's array performance. Any tips?

# Initialise the array of 3D points
radecz = np.zeros((N, 3))

# grab the point data from elsewhere
points[:, 0] = ...
points[:, 1] = ...
points[:, 2] = ...    # this is the dimension we bin in, call it z

# Create an array of M + 1 elements for the edges of M bins
binedges = ...    # (they do not span all of the space of points by the way)

# Find the counts per bin
H = np.histogram(points[:, 2], bins=binedges)

# The number to downsample to in each bin is already known
num_down = ("""some M-dimensional array of fractions""") * H[0]

# initialise a mask for the final array for my analysis with dimension N
finmask = np.array(points.shape[0] * [False])

# loop over bins (do I really need to do this??)
for i, nd in enumerate(num_down):

    # First get the array ids of the points in each bin
    zbin_ids = np.where( ( (binedges[i] < points[:, 2]) & \
                           (points[:, 2) <= binedges[i + 1]) ) == True )

    # Choose ids at random without replacement
    keep = np.random.choice(zbin_ids[0], size=cn, replace=False)

    # What's left is turned on in the mask for the final array
    finmask[keep] = True

points = points[(finmask,)]

You could use numpy.digitize to determine which bin each point belongs in, allowing for a simpler expression inside where.


Blocking and non-blocking queue I came up with the following for a blocking and non-blocking queue implementation. Please suggest any changes/improvements/gotchas.

Blocking queue:

public class BlockingQueue<T> {
    List<T> buffer;
    private static final int MAX_QUEUE_SIZE = 100;

    public BlockingQueue() {
        buffer = new ArrayList<T>();
    }

    public void enqueue(T value) throws InterruptedException {
        synchronized(buffer) {
            while(buffer.size() >= MAX_QUEUE_SIZE) {
                buffer.wait();
            }
            buffer.add(value);
            buffer.notifyAll();
        }
    }

    public T dequeue() throws InterruptedException {
        synchronized(buffer) {
            while(buffer.size() < 1) {
                buffer.wait();
            }
            T value = buffer.remove(0);
            buffer.notifyAll();
            return value;
        }
    }

}

Non-blocking queue:

public class NonBlockingQueue<T> {
    List<T> buffer;
    private static final int MAX_QUEUE_SIZE = 100;
    private AtomicBoolean mutex = new AtomicBoolean(false);

    public NonBlockingQueue() {
        buffer = new ArrayList<T>();
    }

    public void enqueue(T value) throws InterruptedException {
        while(true) {
            while(!mutex.compareAndSet(false, true)) {
                Thread.sleep(100);
            }
            if(buffer.size() < MAX_QUEUE_SIZE) {
                buffer.add(value);
                mutex.set(false);
                return;
            } else {
                mutex.set(false);
            }
        }

    }

    public T dequeue() throws InterruptedException {
        T value = null;
        while(true) {
            while(!mutex.compareAndSet(false, true)) {
                Thread.sleep(100);
            }
            if(buffer.size() > 0) {
                value = buffer.remove(0);
                mutex.set(false);
                return value;
            } else {
                mutex.set(false);
            }
        }
    }
}

I think by non-blocking it means that the thread should not be blocked, so I used the sleep method. Please let me know if it has any issues.

You asked for critique ....

Why are you trying to use an AtomicBoolean as a mutex?  Use a primitive lock, or a Lock if you need more sophisticated behavior.
(Almost) any time you use sleep in an algorithm, you are doing it the wrong way.  In this case, the "wrong way" is a direct consequence of point 1. above.
If you want good performance in a situation where you have multiple producers and consumers, you should avoid doing a notifyAll.  For instance if there are N consumers waiting, and a producer adds a queue entry, then all N consumers will be woken up.  Ideally, only one consumer should be woken.


My Brainfuck interpreter in F# I'm very new to functional world. I've written a simple brainfuck interpreter as my first F# program.

What I would like to know:

Am I using the right data structure for each situation?
Is my code easy to understand?
Is my code style similar to common F# codes (naming, organization etc)?
Is my code "very declarative" or I'm still thinking in the imperative way?

What is not so relevant:

Performance (but if I've made something very bad, let me now!)
More features

Obs.: I assume input is always a valid brainfuck code and that's ok to me.

open System
open System.IO

let getValue (position:int) (memory:Map<int, int>) =
    match Map.tryFind position memory with
    | None  -> 0
    | value -> value.Value

let calcAccumulator (acc:int) (instruction:char) (search:char) (miss:char) =
    match instruction with
    | x when x = search -> acc+1
    | x when x = miss   -> acc-1
    | _ -> acc

let rec findMatch (code:string) (search:char) (miss:char) (inc:int) (current:int) (acc:int) =
    let instruction = code.[current]

    match instruction, acc with
    | x, 0 when x = search -> current+1
    | _    -> findMatch code search miss inc (current+inc) (calcAccumulator acc instruction search miss)

let updateMemory (instruction:char) (position:int) (memory:Map<int, int>) =
    let oldValue = getValue position memory
    let newValue =
        match instruction with
        | '+' -> oldValue + 1
        | '-' -> oldValue - 1
        | ',' -> Console.ReadKey().KeyChar |> Convert.ToInt32
        | _   -> oldValue

    Map.add position newValue memory

let updateOutput (instruction) (value:int) =
    if instruction = '.'
    then Console.Write ((char) value)

let updatePosition (instruction:char) (position:int) =
    match instruction with
    | '>' -> position+1
    | '<' -> position-1
    | _   -> position

let updateIndex (code:string) (index:int) (value:int) =
    match code.[index], value with
    | '[', 0 -> findMatch code ']' '[' 1 (index+1) 0
    | ']', x when x <> 0 -> findMatch code '[' ']' -1 (index-1) 0
    | _      -> index+1

let rec interpretHelper (code:string) (index:int) (memory:Map<int, int>) (position:int) =
    match code.Length with
    | x when x = index  -> memory
    | _ ->
        let instruction = code.[index]
        let newPosition = updatePosition instruction position
        let newMemory = updateMemory instruction position memory
        let newIndex = updateIndex code index (getValue position memory)
        updateOutput instruction (getValue position memory)
        interpretHelper code newIndex newMemory newPosition

let interpret (code:string) =
    interpretHelper code 0 Map.empty 0

let options (args:string array) =
    let pathToFile = args.[0]
    let showMemory =
        match args with
        | [| _ ; "-memory" |] -> true
        | _                   -> false

    pathToFile, showMemory

let onlyCode (text:string) =
    let validInstructions = "+-.,][<>".ToCharArray()

    String.filter (fun x -> Array.contains x validInstructions) text

[<EntryPoint>]
let main args =
    let pathToFile, showMemory = options args
    let code = onlyCode (IO.File.ReadAllText pathToFile)
    let memory = interpret code

    if showMemory
    then Map.iter (fun a b -> printf "\n%3d: %d" a b) memory

    0

Let me make a few comments:

F# has a good system of type inference. You don't need to specify the type explicitly, the compiler in many cases will do it for you. You can read more about it here.
In function getValue. Usually, matched with None|Some value:

match Map.tryFind position memory with
    | None  -> 0
    | Some value -> value

Although you can rewrite this code with using defaultArg

let getValue position memory =
    defaultArg (Map.tryFind position memory) 0

In function updateIndex you can check the value with 0 in match:

let updateIndex (code:string) index value =
    match code.[index], value = 0 with
    | '[', true  -> findMatch code ']' '['  1 (index + 1) 0
    | ']', false -> findMatch code '[' ']' -1 (index - 1) 0
    | _      -> index + 1

The helper functions better write as nested:

let interpret (code:string) =
    let rec interpretHelper index memory position =

You can read more about it here
In function interpretHelper, use if-then-else instead of PM:

let rec interpretHelper index memory position =
    if code |> String.length = index  then
        memory
    else ...


Superior improved stack implementation using a linked list This is a followup to

Improved stack implementation using a linked list
Simple stack implementation using linked list

Please review my hopefully improved stack implementation

As suggested by some answers to my previous code, some of the functions can be overloaded to provide versions of return by value, return by reference and const or pass by value, pass by reference and const, but the function definitions are almost the same so I did not add it here.
Regarding pointers, I know std::unique_ptr will be better and I already saw an implementation through an answer to my previous code, but I like to play with normal pointers right now.

template<class T>
class Stack {
    using stacksize = std::size_t;
public:
    Stack() : first{nullptr}, n{0} {}
    stacksize size() const { return n; }
    bool empty() const { return n == 0; }
    Stack(const Stack&);
    Stack(Stack&&);
    Stack& operator=(Stack);
    Stack& operator=(Stack&&);
    T& operator[](const stacksize& i) {
        Node* traverse = first;
        stacksize x = 0;
        while (x < i) {
            traverse = traverse->next;
            ++x;
        }
        return traverse->item;
    }
    void push(const T&);
    void pop();
    T& peek() const;
    ~Stack() {
        while (!empty()) {
            pop();
        }
    }
private:
    struct Node {
        T item;
        Node* next;
        Node(const T& t, Node* link) :item{t}, next{link} { }
    };
    Node* first;
    stacksize n;
};

template<class T>
Stack<T>::Stack(const Stack& s) :first{nullptr}, n{0}{
    for (auto t = s.first; t != nullptr; t = t->next) {
        push(t->item);
    }
}

template<class T>
Stack<T>& Stack<T>::operator=(Stack s) {
    std::swap(first,s.first);
    std::swap(n,s.n);
    return *this;
}

template<class T>
Stack<T>::Stack(Stack&& s) :first{s.first}, n{s.n} {
    s.first = nullptr;
    s.n = 0;
}

template<class T>
Stack<T>& Stack<T>::operator=(Stack&& s) {
    std::swap(s.n,n);
    std::swap(s.first,first);
    return *this;
}

template<class T>
void Stack<T>::push(const T& t) {
    first = new Node{t,first};
    ++n;
}

template<class T>
void Stack<T>::pop() {
    Node* oldfirst = first;
    first = first->next;
    delete oldfirst;
    --n;
}

template<class T>
T& Stack<T>::peek() const {
    return first->item;
}

Code Review
I find your placement of & inconsistent with *
    Node(const T & t, Node* link) :item{t}, next{link} { }

And r-value reference declaration stranger still
    Stack(Stack & & );  // I am surprised that compiled. As each `&` is a separate token
                        // While `&&` is a single token.

This is all part of the type information. Put it with the type.
    Node(const T& t, Node* link) :item{t}, next{link} { }
    Stack(Stack&& rhs);

You provide a push by copy.
void push(const T & );

and you are familiar with move semantics. Why not provide a push by move?
void push(T&& val);

Very Minor personal preference.
Personally I prefer const on the right of the type. The rule is that const binds left unless it is on the very left hand side then it binds right. There is one obsecure corner case were this makes a difference. But it is obscure so don't worry.
    Node(T const& t, Node* link) :item{t}, next{link} { }

I just find it easier when reading types (as you read them right to left).
    char const * const  x;
    // x is "const pointer" to a "const char"

    char const *   x;
    // x is "pointer" to a "const char"

    char * const  x;
    // x is "const pointer" to a "char"