Monte Carlo pi animation I have created a program that calculates pi using a Monte Carlo method. It also animates the process and displays the value as it is updated to show how it gets closer and closer to the actual value as more time passes. It is made with HTML canvas. I would like some feedback on my program.

"use strict";

function hypot(a, b) {
  return Math.sqrt(a * a + b * b);
}

function randInt(min, max) {
  return Math.floor(Math.random() * (max - min)) + min
}

class Circle {
  constructor(x, y, r) {
    this.x = x;
    this.y = y;
    this.r = r;
  }

  draw() {
    ctx.strokeStyle = "#000";
    ctx.lineWidth = "5px";
    ctx.beginPath();
    ctx.arc(this.x, this.y, this.r, 0, Math.PI * 2);
    ctx.stroke();
  }

  collidePoint(pt) {
    var dx = this.x - pt.x;
    var dy = this.y - pt.y;
    return hypot(dx, dy) <= this.r;
  }
}

class Point {
  constructor(x, y) {
    this.x = x;
    this.y = y;
  }

  draw() {
    ctx.fillStyle = "rgba(0, 0, 0, 16)";
    ctx.fillRect(this.x, this.y, 1, 1);
  }
}

const canvas = document.getElementById("disp");
const ctx = canvas.getContext("2d");
canvas.width = window.innerWidth;
canvas.height = window.innerHeight;
var width = parseInt(canvas.width);
var height = parseInt(canvas.height);
var trys = 0;
var lands = 0;
var area = width * height;
var points = [];
var ball = new Circle(width / 2, height / 2, Math.min(width, height) / 2);
setInterval(step, 10);

function drawAll() {
  ctx.clearRect(0, 0, width, height);
  ball.draw();
  points.forEach((point) => {
    point.draw();
  });
}

function step() {
  drawAll();
  var pnt = new Point(randInt(0, width), randInt(0, height));
  trys++;
  if (ball.collidePoint(pnt)) {
    lands++;
  }
  var prob = lands / trys;
  var pi = (prob * area) / Math.pow(ball.r, 2);
  document.getElementById("out").innerText = pi.toString();
  points.push(pnt);
}
html, body {
  height: 100%;
  overflow: hidden;
}

#disp {
  width: 100vw;
  height: 100vh;
  margin: 0;
}

#box {
  position: absolute;
  z-index: 1;
  width: 200px;
  height: 20px;
  left: 5%;
  top: 5%;
  background-color: whitesmoke;
}
<!DOCTYPE html>

    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width">
    <title>Monte Carlo Pi</title>
    <link href="style.css" rel="stylesheet" type="text/css" />

    <canvas id="disp"></canvas>
    Pi &#x2248;

The jungle
OO design tends to create unneeded eco systems when the focus is the objects not the task at hand.
In this case Point, Circle, points and supporting functions have nothing to do with the problem. They exist to serve each other.
You clear the canvas so you can draw all the points, seams to me that the only reason you keep an array of points is so you can draw them after you clear the canvas.
Storing the points means no machine today using your function will ever be able to solve PI to over 6 digits as none have either the memory to hold all the points, nor the time to draw them.
Having an open setInterval (no handle to stop the interval)  constitutes a memory leak because you push to the points array each time.
The task is to calculate Pi via Monte Carlo method and show the points, it can be done without the overhead,of the OO eco system and supporting scope.
General point

Use const for constants.

ECMAScripts Math has a hypot method already, however it is much quicker to do the calculation inline. As the calculation requires the radius squared, you may as well store the radius as such and then test if inside with x * x + y * y < radiusSqr

window is the default object, you don't use it for window.parseInt so why use it for innerWidth?

canvas.width, and canvas.height represent the resolution of the canvas. They can only be integer values. parseInt is thus redundant.

You are moving the ball to the center of the canvas and with it all the points. As such need to move each point back to the origin to calculate its distance from the ball. If you moved the canvas origin to the canvas center you only need the radius, and you don't need to move the points to the ball origin to get the distance.

The calculation works for random distribution of points. You introduce a very non uniform distribution by rounding the points to the nearest pixel.

Every 10 ms you redraw the canvas content. All browsers have a MAX display rate of 60 frame per second. you draw 100 so 40 of the frames you draw are never seen. Never render to the DOM or to canvas using setInterval at rates above a few frames per second. Use requestAnimationFrame

You Search the DOM for an element with id "out". every 10 ms. Better to get it once at the start. better yet use direct reference via the id See example

You call the Number.toString when you set out.textContent ECMAScript does type coercion automatically you don't need to call toString when assign a number to a string

Your DOM element ids are rather poor and could be improved

You should set the body margin and padding to "0px" so that the canvas fits without needing to have overflow clip the canvas.

If you don't need to locate an element uniquely use a CSS class to set its style rather than id

Use the setTimeout rather than setInterval to give a interval between rather than an interval over. Many devices need time to cool or they will force the issue and reduce the clock speed. setInterval can result in back to back calls to the function giving no rest for the CPU/GPU, the result will be slower performance for all services the machine is running.

Example
The example removes the overhead of storing points.
The constant pointsPerInterval  defines how many points to calculate each interval. The points are added to the current 2D context path and only rendered when it can be displayed using requestAnimationFrame
Having high rated of points quickly results in a black canvas. So the number of points drawn per frame is only a sub set of the points used. Thus it draws only ~1000 points per second and uses ~10million points per second to calculate PI.
The example also exposes a simple interface to reset the animations, or stop.
Also shows number of points use. "Mp" represents Mega points (1,000,000 points)
Click the canvas to reset.

"use strict";
const calculatePi = (() => {
    const ctx = canvas.getContext("2d");
    const width = canvas.width = innerWidth, halfWidth = width / 2;
    const height = canvas.height = innerHeight, halfHeight = height / 2;
    const radiusSqr = Math.min(halfWidth, halfHeight) ** 2;
    const areaScale = width * height / radiusSqr;
    const pointsPerInterval = 100000, interval = 10, showPointCount = 10;
    var pointCount, inCircleCount, timeoutHandle;

    ctx.setTransform(1, 0, 0, 1, halfWidth, halfHeight); // Move origin to cent of canvas

    resetCanvas();
    requestAnimationFrame(render);
    function resetCanvas() {
        pointCount = 0;
        inCircleCount = 0;
        ctx.clearRect(-halfWidth, -halfHeight, width, height);
        ctx.lineWidth = 5;
        ctx.beginPath();
        ctx.arc(0, 0, radiusSqr ** 0.5, 0, Math.PI * 2);
        ctx.stroke();
        ctx.beginPath();
        addPoints();
    }
    function render(){
        ctx.fill();
        ctx.beginPath();
        currentPiId.innerText = inCircleCount / pointCount * areaScale;
        pointCountId.innerText = pointCount / 1000000 | 0;
        requestAnimationFrame(render);
    }
    function addPoints() {
        var i = pointsPerInterval;
        pointCount += pointsPerInterval;
        while (i --) {
            const x = -halfWidth + Math.random() * width;
            const y = -halfHeight + Math.random() * height;
            i < showPointCount && (ctx.rect(x, y, 1, 1));
            inCircleCount += x * x + y * y < radiusSqr ? 1 : 0;
        }
        timeoutHandle = setTimeout(addPoints, interval);
    }
    return Object.freeze({
        stop() { clearTimeout(timeoutHandle) },
        restart() {
            this.stop();
            resetCanvas();
        },
    });
})();

canvas.addEventListener("click", () => calculatePi.restart());
#canvas  {
  position: absolute;
  top: 0px;
  left: 0px;
}

.box {
  position: absolute;
  z-index: 1;
  /*width: 200px;*/
  height: 20px;
  left: 5%;
  top: 5%;
  background-color: whitesmoke;
}
<canvas id="canvas"></canvas>
Mp = Pi &#x2248;


Program that prints out the initials of a given name #include <string.h>
#include <cs50.h>
#include <stdio.h>
#include <ctype.h>

int main(void)
{
    // store user input
    char * name = get_string();

    // print out the first character of firts string
        printf("%c", toupper(name[0]));

    // iterate through name
    for (int i = 0, n = strlen(name); i < n; i++)
    {
        // if i'th character is a space print next character as uppercase
        if (name[i - 1] == ' ')
        {
            printf("%c", toupper(name[i]));

        }

    }
    // print new line following initials
    printf("\n");

}

It does what it's supposed to do but I'm concerned about the style of the program.

Your code is buggy. It might seem to work, but accessing name[-1] invokes undefined behavior. You do that in the first iteration of the for loop. To fix it, start at i = 1 instead of i = 0.

The next instance of undefined behavior is when you call toupper(name[i]). The functions from <ctype.h> must not be called with a char as their argument. It must always be an unsigned char or the value of the macro EOF. To fix this, call toupper((unsigned char) name[i]).

The proper type for array indexes is not int, but size_t. Therefore the loop should start with for (size_t i = 1, n = strlen(name).

You have a typo: firts instead of first.

Other than that, the code looks good. In some months you will be able to remove all the comments because they are obvious to you. But until then, they should stay.

To make your program even more correct, you could change the condition name[i - 1] == ' ' to be more precise: !isalpha((unsigned char) name[i - 1]) && isalpha((unsigned char) name[i]). Then you won't print an "uppercase space" if the input contains several spaces in a row.


Single value computation I'm writing code to compute a single value answer to a given numbers in integer array such that step sum is calculated till only 1 number is left.

For example:

Input:

3,5,2,6,7

Output is computed as:

(3+5)+(5+2)+(2+6)+(6+7)

It remains like this till I only have a single number left.

3   5   2   6   7
  8   7   8  13
   15  15  21
     30  36
       66

int inputarray={3,5,2,6,7};
int inputlength=inputarray.length;
   while(inputlength!=0)
   {
   for(int i=0;i<inputlength-1;i++)
   {
      inputarray[i]=inputarray[i+1]+inputarray[i];
   }
   inputlength--;
   }
    System.out.println(inputarray[0]);

It computes the result in quadratic growth.  Is there any way to compute my result more efficiently? What else can I do to improve it?

First of all it is always helpful to format your code properly (I now used the default code convention for Java):

int inputarray={3,5,2,6,7};
int inputlength=inputarray.length;
while (inputlength != 0) {
    for (int i = 0; i < inputlength-1; i++) {
        inputarray[i] = inputarray[i+1] + inputarray[i];
    }
    inputlength--;
}
System.out.println(inputarray[0]);

Also it is better to test for inputlength < 0 because then the termination of the loop is much more likely...

For performance tuning:

Try to create a formula for your problem to calculate the result. Your example could be reduced to 3 + 4*5 + 6*2 + 4*6 + 7...

Find a formula such like Gauss did for summing up the parts...


Program to find the largest odd number among three variables def greatest(a,b):
    if a>b:
        return a
    return b
def odd_check(a):
    return not(a%2 == 0)
def go(x,y,z):
    a = odd_check(x)
    b = odd_check(y)
    c = odd_check(z)
    if a and b and not c:
        print greatest(x,y)
    elif a and not b and c:
        print greatest(x,z)
    elif not a and b and c:
        print greatest(y,z)
    elif not a and not b and c:
        print z
    elif not a and b and not c:
        print y
    elif a and not b and not c:
        print x
    elif a and b and c:
        print greatest(x,greatest(y,z))
    else:
        print "None of them are odd"
go(int(input()),int(input()),int(input()))

Doing the problem it felt like I am going over all the eight combinations \$2^3\$ for three variables. Is there any way to avoid this?

And for those who love comprehensions...

def max_odd(*args):
     return max(x for x in args if x%2)


Python 3 multi-threaded pinger My goal: I want to ping every single IPv4 address and record whether or not they responded.

The way I have it set up is every IP address corresponds to an index. For example 0.0.0.0 is index 0 and 0.0.1.0 is index 256. So if 0.0.0.0 responded, then the 0th element of the bitarray is true. At the end I write the bitarray to a file.

Here is the code:

import subprocess
from bitarray import bitarray
import threading
import time

response_array = bitarray(256 * 256 * 256 * 256)
response_array.setall(False)

def send_all_pings():
    index = 0
    for f1 in range(256):
        for f2 in range(256):
            for f3 in range(256):
                for f4 in range(256):
                    thread = PingerThread(".".join(map(str, [f1, f2, f3, f4])), index)
                    thread.start()
                    index += 1

    time.sleep(30)
    print("Writing response array to file")
    with open('responses.bin', 'wb') as out:
        response_array.tofile(out)

class PingerThread(threading.Thread):
    def __init__(self, address, index):
        threading.Thread.__init__(self)
        self.address = address
        self.index = index

    def run(self):
        if subprocess.call(["ping", "-c", "1", "-w", "1", self.address]) == 0:
            response_array[self.index] = True
        else:
            response_array[self.index] = False

What can I do to make this run faster? Any optimisations at all, even if very small, are welcome!

Some suggestions:

IP addresses are only formatted as octets (0-255) for human readability - they actually just represent integers. Instead of for example 127.0.0.1 you can use 2130706433 (127*2^24+1). In other words, range(2^32-1) represents the entire range of IPv4 addresses.
Using a Python library to ping hosts is very likely going to be much faster than starting a shell command.
Use multiprocessing rather than Python threads to avoid running into the global interpreter lock
response_array will end up taking many gigabytes of memory. If you really need the kind of detail you're logging you should be writing each entry to disk ASAP (keeping the file open all the while). You could also look into simplifying your reporting, such as only saving the IP addresses which don't respond, or saving to two files, one with responding IPs and the other with non-responding ones. You'll have to store a file (or a pair of files) per process, to avoid them clobbering each other.


Asynchronously loading a point in chart on drag I have some code that asynchronously loads some point in a chart when a users drag it:

private void UpdateHistoryAndReSizeWhenDrag(string param, long duration)
    {
        lock (Lock)
        {
            if (Boxes.Count() > 0)
            {
                BoxStruct temp = Boxes.Where(key => key.id == param).FirstOrDefault();
                if (temp != null)
                {
                    int index = Boxes.IndexOf(Boxes.Where(key => key.id == param).First());
                    if (index >= 0)
                    {
                        double ChartLastPoint = Boxes[index].Series.Points[0].X;
                        double LastShownPoint = (from x in Boxes where x.id == param select x.PlotModel.Axes[0].ActualMinimum).Min();
                        double PMActualMinimumY = (from x in Boxes where x.id == param select x.PlotModel.Axes[0].ActualMinimum).Min();
                        double PMActualMaximumY = (from x in Boxes where x.id == param select x.PlotModel.Axes[0].ActualMaximum).Max();
                        if (LastShownPoint < ChartLastPoint)
                        {
                            string StartTime = (ConvertToUnixTime(DateTime.FromOADate(LastShownPoint)) - (10800 + duration * 4)).ToString();
                            string EndTime = (ConvertToUnixTime(DateTime.FromOADate(LastShownPoint)) - 10800).ToString();
                            List<DataPoint> LoadedPoints = LoadPointsRange(param, StartTime, EndTime).Points;
                            Boxes[index].Series.Points.InsertRange(0, LoadedPoints);
                        }
                        if (Boxes.Count > 0 & index < Boxes.Count)
                        {
                            var temp1 = Boxes[index].Series.Points.Where(key => key.X > PMActualMinimumY && key.X < PMActualMaximumY);

                            if (temp1 != null & temp1.Count() > 0)
                            {
                                double max = temp1.Select(p => p.Y).Max();
                                double min = temp1.Select(p => p.Y).Min();
                                Boxes.Where(k => k.id == param).First().PlotModel.Axes[1].Reset();
                                Boxes.Where(k => k.id == param).First().PlotModel.Axes[1].Zoom(min, max);
                            }
                        }
                    }
                }
            }
        }
    }

This function calls something like:

                UpdateHistoryAsyc UHA = new UpdateHistoryAsyc(UpdateHistoryAndReSizeWhenDrag);
            IAsyncResult result = UHA.BeginInvoke(item, CurrentDuration, null, null);

Where boxes looks like:

        private ObservableCollection<BoxStruct> boxes;
    public ObservableCollection<BoxStruct> Boxes
    {
        get { return boxes; }
        set { boxes = value; RaisePropertyChanged("Boxes"); }
    }

How can I improve this LINQ code?

You don't follow capitalization conventions: ChartLastPoint, LastShownPoint, PMActualMinimumY, PMActualMaximumY, StartTime, EndTime, LoadedPoints should all be camelCase.

I suspect Lock must be camelCase as well, since I doubt it is a public property.

Use proper variable names, not temp or temp1. Same problem with param: it doesn't tell me anything, only through the code I know it's some kind of ID.

Your code suffers from the Arrow anti-pattern. You can easily reduce some of the nesting by returning earlier:

private void UpdateHistoryAndReSizeWhenDrag(string param, long duration)
{
    lock (Lock)
    {
        if (Boxes.Count() < 1)
        {
            return;
        }

        BoxStruct temp = Boxes.Where(key => key.id == param).FirstOrDefault();
        if (temp == null)
        {
            return;
        }

        int index = Boxes.IndexOf(Boxes.Where(key => key.id == param).First());
        if (index < 0)
        {
            return;
        }

        /* rest of your code */
    }
}

I don't know what Boxes is, but I suspect an ICollection<T> or an IList<T> of sorts, so why then use Boxes.Count() when you can use Boxes.Count (as you do later on in your code)?

I don't see why you're using Boxes[index], since the temp contains the BoxStruct you need to work with. Later on again you do Boxes.Where(k => k.id == param).First() twice, even though you could simply use temp.

I also don't see the point of this check: if (Boxes.Count > 0 & index < Boxes.Count): the second part is always true, and the first part you checked right at the start of your code: if (Boxes.Count() > 0).

I find it a bit odd that you carefully check that Boxes contains a value you can work with, but then never check if that object's Series isn't null, and if Series's Points isn't null etc. Also, you don't do anything if you can't find an appropriate BoxStruct -- shouldn't you inform the user?

I see the magic number 10800 appear twice. This should be stored in a constant and have a descriptive name.

Why do you retrieve the same value twice, yet store it in different variables?

    double LastShownPoint = (from x in Boxes where x.id == param select x.PlotModel.Axes[0].ActualMinimum).Min();
    double PMActualMinimumY = (from x in Boxes where x.id == param select x.PlotModel.Axes[0].ActualMinimum).Min();

Moreover, since you need to use the result of from x in Boxes where x.id == param select x.PlotModel.Axes[0] three times, store it in a variable and use that variable.

PMActualMinimumY and PMActualMaximumY both end in Y, yet you compare them to a value named X.

I'm a bit worried to see a method named LoadPointsRange which seemingly returns some kind of object, from which you then take its property Points.


FizzBuzz without hard-coded logic checks I'm a self-taught (on the job) programmer, always looking for ways to expand my skills. Seeing all the FizzBuzz examples with hard-coded logic checks made my head hurt, so I thought I'd try my hand at it. Leaning heavily on KISS, I'm ignoring exception handling and such, and all that's required to add new fizzbuzzers is a call to Catalog.Add(Divisor,Output).

I'd appreciate your feedback, whether on style or substance. This is part of my journey to adopting SOLID principles for better OO development, but I felt that not much could be applied here without over-complicating it.

using System;
using System.Collections.Generic;

public class Program
{
    public static void Main()
    {
        var catalog = new Catalog();
        catalog.Add(Divisor: 3, Output: "Fizz");
        catalog.Add(Divisor: 5, Output: "Buzz");
        catalog.Add(Divisor: 10, Output: "Pozz");

        var counter = new Counter(Min: 1, Max: 100, Catalog: catalog);
        counter.Output();
        Console.ReadLine();
    }
}

internal class Counter
{
    private int min;
    private int max;
    private Catalog catalog;

    internal Counter(int Min, int Max, Catalog Catalog)
    {
        this.min = Min;
        this.max = Max;
        this.catalog = Catalog;
    }
    internal void Output()
    {
        for(int i = min; i <= max; i++)
        {
            Console.WriteLine(catalog.ToString(i));
        }
    }
}

internal class Catalog
{
    private List<Spec> specs;
    internal class Spec
    {
        internal int divisor;
        internal string output;
    }
    internal Catalog()
    {
        this.specs = new List<Spec>();
    }
    internal void Add(int Divisor, string Output)
    {
        this.specs.Add(new Spec() { divisor = Divisor, output = Output });
    }
    internal string ToString(int Number)
    {
        string outputstring = "";
        foreach (var x in specs)
        {
            outputstring += Number % x.divisor == 0 ? x.output : "";
        }
        return String.IsNullOrWhiteSpace(outputstring) ? Number.ToString() : outputstring;
    }
}

.NetFiddle

The code looks well structured and seems to do its job (I have just copy-pasted it in VS2015 and worked directly), so many of the above are rather superficial:

1) Function (including constructor) parameter naming. In C#, usually all function parameters are named using CamelCase.

internal Counter(int min, int max, Catalog catalog)
{
    this.min = min;
    this.max = max;
    this.catalog = catalog;
}

2) Non-private members naming. Here, I would also favor PascalCase over CamelCase, although this targeted question and answers do not seem to clarify this.

internal class Spec
{
    internal int Divisor;
    internal string Output;
}

3) ToString usage - ToString(int Number) function has exactly the same name as Object.ToString() function (no parameters). Although it is perfectly legal (becomes an overload), I would choose a different name, just not to create any confusion (your are not getting a string representation of the object using its state only, but also providing some input).

Also, since strings are immutable, concatenation is faster and allocates less memory if StringBuilder is used.

internal string GetString(int Number)
{
    var builder = new StringBuilder();
    foreach (var x in specs)
    {
        builder.Append(Number % x.Divisor == 0 ? x.Output : "");
    }
    string outputString = builder.ToString();

    return String.IsNullOrWhiteSpace(outputString) ? Number.ToString() : outputString;
}

}

The whole code:

using System;
using System.Collections.Generic;
using System.Text;

public class Program
{
    public static void Main()
    {
        var catalog = new Catalog();
        catalog.Add(divisor: 3, Output: "Fizz");
        catalog.Add(divisor: 5, Output: "Buzz");
        catalog.Add(divisor: 10, Output: "Pozz");

        var counter = new Counter(Min: 1, Max: 100, Catalog: catalog);
        counter.Output();
        Console.ReadLine();
    }
}

internal class Counter
{
    private int min;
    private int max;
    private Catalog catalog;

    internal Counter(int min, int max, Catalog catalog)
    {
        this.min = min;
        this.max = max;
        this.catalog = catalog;
    }

    internal void Output()
    {
        for (int i = min; i <= max; i++)
        {
            Console.WriteLine(catalog.GetString(i));
        }
    }
}

internal class Catalog
{
    private List<Spec> specs;

    internal class Spec
    {
        internal int Divisor;
        internal string Output;
    }

    internal Catalog()
    {
        specs = new List<Spec>();
    }

    internal void Add(int divisor, string Output)
    {
        specs.Add(new Spec() { Divisor = divisor, Output = Output });
    }

    internal string GetString(int Number)
    {
        var builder = new StringBuilder();
        foreach (var x in specs)
        {
            builder.Append(Number % x.Divisor == 0 ? x.Output : "");
        }
        string outputString = builder.ToString();

        return String.IsNullOrWhiteSpace(outputString) ? Number.ToString() : outputString;
    }
}


Generate a sequence of coin flips until pattern is hit A book on probability says that a sequence of heads and tails is generated by a fair coin asking what is the probability that the pattern T, H, H, H occurs before the pattern H, H, H, H. I have made a generator of such strings. Is it right (I have heard that return statement is not appreciated in Scala)?

def l(s: String): Boolean = {
    if (s.endsWith("hhhh")) return true;
    if (s.endsWith("thhh")) return false;
    l(s + (if (Math.random > .5) "h" else "t"))
}

To avoid return statements:

def l(s: String): Boolean = {
  if (s.endsWith("hhhh")) true
  else if (s.endsWith("thhh")) false
  else l(s + (if (Math.random > .5) "h" else "t"))
}

Although I don't like if - else if sequences, in this case it remains simple and readable, but it still can be simplified like this

def l(s: String): Boolean = {
  if (s.length > 3) s.endsWith("hhhh")
  else l(s + (if (Math.random > .5) "h" else "t"))
}

or even

def l(s: String): Boolean = {
  s.endsWith("hhhh") || l(s + (if (Math.random > .5) "h" else "t"))
}

Randomness

I think that there is a minor issue with random values distribution.

As per Scaladoc for Math.random:

  Returns a Double value with a positive sign, greater than or equal to 0.0 and less than 1.0.

For the current condition, this means that 0.5 < h < 1.0 and 0 <= t <= 0.5.

0.5 is not included in the lower bound for generation of h values, which favors t very slightly.

The condition for equal distribution should be if (Math.random < 0.5) "t" else "h".


React programming pattern I create https://github.com/storenth/react-redux-todo and ask to review my Todo-react-app for programming pattern: dividing components into presentational components and container components.
Here’s the basic idea behind it: if a component has to have state, make calculations based on props, or manage any other complex logic, then that component shouldn’t also have to render HTML-like JSX.
Instead of rendering HTML-like JSX, the component should render another component. It should be that component’s job to render HTML-like JSX.
So, my question about the /containers/TodosContainer that contains /components/Todos and /components/AddForm. Does it match the pattern? I didn't get where to place the AddForm, inside /components/Todos (to complex I guess)?
import React, { Component, Fragment } from 'react';
import { Todos } from "../components/Todos";
import { AddForm } from "../components/AddForm";

export class TodosContainer extends Component {
    constructor(props) {
        super(props);
        this.state = { items: [], text: '' };
        this.handleChange = this.handleChange.bind(this);
        this.handleSubmit = this.handleSubmit.bind(this);
        this.removeItem = this.removeItem.bind(this);
    }
    handleChange = (e) => {
        this.setState({
            text: e.target.value
        })
    }
    handleSubmit = (e) => {
        e.preventDefault();
        if (this.state.text.length === 0) {
            return;
        }
        const newItem = {
            text: this.state.text,
            id: Date.now()
        };
        this.setState({
            items: this.state.items.concat(newItem),
            text: ''
        })
    }
    removeItem = (id) => {
        const items = this.state.items.filter(item => {
            return item.id !== id;
        })
        this.setState({
            items
        })
        console.log(this.state);
    }

    render() {
        return (
            <>
                <Todos items={this.state.items} removeItem={this.removeItem} />
                <AddForm items={this.state} onSubmit={this.handleSubmit} onChange={this.handleChange} />
            </>
        )
    }
}
```

I'd say, if it's the Presentation and Container Components pattern as described by Dan Abramov, then yes, you've pretty effectively nailed it. (Though Dan no longer advocates this pattern!!)
Suggestions/Review
AddForm
items prop is really the text state in the parent component, it doesn't need the entire state object. (If it does then name it more accurately!) What's used is just the input value, so value is a better descriptor.
export const AddForm = ({ value, onSubmit, onChange }) => {
  console.log(value);
  return (

      <form onSubmit={e => onSubmit(e)}>
        Add new todo:
        <input
          type="text"
          id="name"
          onChange={e => onChange(e)}
          value={value}
        />
        <button
          className="btn waves-effect waves-light"
          type="submit"
          name="action"
        >
          Add item
        </button>
      </form>

  );
};

Usage: <AddForm value={this.state.text}  ...  />
TodosContainer
The handlers are all arrow functions, they already have the this of the class bound to them, so there isn't need to bind them in a constructor. In fact, if all the constructor is doing is setting the initial state, then you don't really need the constructor either as you can simply set initial state with a state instance variable.
Instead of
constructor(props) {
  super(props);
  this.state = { items: [], text: '' };
  this.handleChange = this.handleChange.bind(this);
  this.handleSubmit = this.handleSubmit.bind(this);
  this.removeItem = this.removeItem.bind(this);
}

Simply
state = { items: [], text: '' };

State updates
When any next state depends upon current state, like incrementing counters, or adding values to arrays, you should get into the habit of using functional state updates, and also not referencing this.state within the state update.
State and Lifecycle
setState
handleSubmit can be improved
handleSubmit = (e) => {
  e.preventDefault();
  const newItem = {
    text: this.state.text,
    id: Date.now(),
  };
  if (this.state.text) { // <-- if text is truthy, i.e. non-empty string
    this.setState(prevState => ({
      items: prevState.items.concat(newItem),
    }));
  }
}

State updates are also asynchronous, so don't try to console.log it after requesting the state update. Use a component lifecycle like componentDidUpdate to log state after the update (or for class-based components, you can use the setState 2nd parameter callback function, but for simplicity sake stick to the lifecycle functions.).
removeItem handler can be improved
removeItem = (id) => {
  this.setState(prevState => ({
    items: prevState.items.filter(item => item.id !== id),
  }));
}


Leetcode 17 : Letter Combinations of a Phone Number Problem Statement

Given a string containing digits from 2-9 inclusive, return all possible letter combinations that the number could represent. Return the answer in any order. A mapping of digit to letters (just like on the telephone buttons) is given below. Note that 1 does not map to any letters.

Example 1:

Input: digits = "23"
Output: ["ad","ae","af","bd","be","bf","cd","ce","cf"]

Example 2:
Input: digits = ""
Output: []

Example 3:
Input: digits = "2"
Output: ["a","b","c"]

My Solution
A simple DFS based approach. Ignore some comments that are for my learning purposes (for similar problems to able to code it fast).
#include <array>
#include <string>
#include <vector>

class Solution {
private:
  // any auxilliary fields to help us (the "fluff" in the problem)

  std::array<std::string_view, 10> mappings = {
      "0",   "1",    "abc", "def", "ghi", "jkl",
      "mno", "pqrs", "tuv", "wxyz"}; // note that 0 and 1 have the digits since
                                     // that makes the most sense

  // private functions that (may) do the heavy lifting
  std::string_view getLettersMapping(char digit) {
    return mappings[digit - '0'];
  }

  void dfs(std::vector<std::string> &combinations, std::string_view digits,
           std::string &s, size_t curr_idx, const size_t &N) {
    //    base case
    if (curr_idx == N) {
      // last digit's combination has already been explored, so simply append it
      // to our set of combinations and return
      combinations.push_back(s);
      return;
    }

    // determine current mapping digit
    auto letters = getLettersMapping(digits[curr_idx]);

    // for that particular digit, generate all the possible combinations by
    // recursing for each possible digit in the next index.
    for (const auto &ch : letters) {
      // note that passing the string s by reference saves space,
      // but now we need to modify it by adding the letter and then removing the
      // latest added later after the  recursive call
      s.push_back(ch);
      dfs(combinations, digits, s, curr_idx + 1, N);
      s.pop_back();
    }
  }

public:
  std::vector<std::string> letterCombinations(std::string digits) {
    //   in the main function, we develop the answer, call the "magic"
    //   recursion/DFS function, and return the answer
    std::vector<std::string> ans;
    if (digits.empty() || digits.size() == 0) {
      return ans;
    }
    std::string s;
    dfs(ans, digits, s, 0, digits.size());
    return ans;
  }
};

Any suggestions for my code? Also, I'm still learning the more "modern" features of C++ (anything from C++17 onwards), so any such advice would be very welcome :)

Make mappings static constexpr
This mapping should never be changed, so ideally it should be constexpr. You also need only one instance of it, so it should be marked static as well (also, constexpr requires this). So:
static constexpr std::array<const std::string_view, 10> mappings = {
    ...
};

This also means getLetterMappings() can be made static constexpr, and in fact you can make dfs() static constexpr as well, although the constexpr part doesn't do much there.
Redundant check for empty input
Why do you check both digits.empty() and digits.size() == 0? Those expressions are equivalent. I would just check for digits.empty().
Unnecessary parameters curr_idx and N
Since you are passing the input digits to dfs() as a std::string_view by value, you don't need the parameters curr_idx and N. Just pass a modified version of digits to recursive calls to dfs(), like so:
void dfs(std::vector<std::string> &combinations, std::string_view digits, std::string &s) {
    if (digits.empty()) {
        combinations.push_back(s);
        return;
    }

    auto letters = getLettersMapping(digits.front());

    for (const auto &ch : letters) {
        s.push_back(ch);
        dfs(combinations, digits.substr(1), s);
        s.pop_back();
    }
}


Stack Array implementation of Java This is my array implementation on stack in Java. It involves push, pop, get and growth when limit of array size is reached.

I am a self-taught programmer starting to learn data structures and algorithms.

I am looking for

any recommendation on conventions
an honest opinion on how bad this piece of code if this is an interview.
anything wrong and room for improvement.

I wrote this code myself, not looking at any other similar questions on Code Review.

public class stackAlist{
    int[] data;
    int size;
    static int growthmultipler = 1;

    public stackAlist(){
        data = new int[2*growthmultipler];
        size = 0;
    }

    public void push(int value){
        if(this.size == 0){
            this.data[0] = value;
            this.size += 1;
            this.growthmultipler = 1;
            }
        else if(this.size == 2*growthmultipler){
            growthmultipler += 1;
            stackAlist newlist = new stackAlist();
            newlist.data = new int[2*growthmultipler];
            System.arraycopy(this.data, 0, newlist.data, 0, this.size);
            this.data = newlist.data;
            this.data[size] = value;
            this.size += 1;
        }
        else{
        this.data[size] = value;
        this.size += 1;
        }
    }

    public void pop(){
        this.data[size-1] = 0;
        this.size = this.size-1;
    }

    public void get(){
        int i;
        for(i =0; i < this.size; i++){
            System.out.println(data[i]);
        }
    }

    public static void main(String[] args){
        stackAlist a = new stackAlist();
        a.push(1);
        a.push(2);
        a.get();
        a.pop();
        a.get();
        a.push(3);
        a.push(4);
        a.get();
    }
}

First of all, you are using a static variable (growthmultipler) inside the instance of a class. This is fatal if you have more than one instance of this class. Because you always use 2*growthmultipler, you don't need it. Use +=2  instead.

You don't need to prealloc data in the constructor. It will be done if it's needed.

You don't need to handle "size == 0" separately. You only have to handle "size >= length".

You don't have to create a new class if you want to grow data.

In pop() you should test to not get negative (and throw an exception). And normally a pop function returns the value.

In summary it might look like this:

public class stackAlist{
    int[] data;
    int size;

    public stackAlist(){
        size = 0;
        data = new int[size];
    }

    public void push(int value){
        if(size>=data.length) {
            int [] ndata = new int[data.length+2];
            System.arraycopy(data, 0, ndata, 0, size);
            data = ndata;
        }
        data[size] = value;
        size += 1;
    }

    public int pop() {
        int ret=0;
        if(size>0) {
            size -= 1;
            ret = data[size];
            data[size] = 0;
        }
        return ret;
    }
    .....


A shell script to mount a disk image file I'm trying to create a bash shell script which mounts a disk image file. Not only that but checks to see if the disk image already exists. Is there anyway I could improve my script? Currently my script looks like this:

#!/bin/bash

MOUNTPOINT="/myfilesystem"

if [ ! -d "${MOUNTPOINT}" ]; then
    if [ -e "${MOUNTPOINT}" ]; then
        echo "Mountpoint exists, but isn't a directory..."
        exit 1
    else
        sudo mkdir -p "${MOUNTPOINT}"
    fi
fi

if [ "$(echo ${MOUNTPOINT}/*)" != "${MOUNTPOINT}/*" ]; then
    echo "Mountpoint is not empty!"
    exit 1
fi

if mountpoint -q "${MOUNTPOINT}"; then
    echo "Already mounted..."
    exit 0
fi

The first conditional group can be reorganized to make it flatter and simpler:

if [ ! -e "${MOUNTPOINT}" ]; then
    sudo mkdir -p "${MOUNTPOINT}"
elif [ -d "${MOUNTPOINT}" ]; then
    echo "Mountpoint exists, but isn't a directory..."
    exit 1
fi

The exit 0 is pointless in the last condition,
and as such, you could replace it with a one-liner using &&:

mountpoint -q "${MOUNTPOINT}" && echo "Already mounted..."


Selenium Framework using a lot of statics I am writing a Selenium framework that should be really easy for a tester with relatively little Java knowledge to use and write tests for. In order to keep the framework as user friendly as possible, I've noticed that I am writing a lot of statics so that the tester doesn't have to instantiate the page objects and methods every time. This of course goes against the principles of OOP. Before I get in too deep with this I'd like to see if there are viable alternatives to my approach. I'm including below the login page class that instantiates a webdriver Singleton, has a username/password field, and a login button. There are 2 tests for this page that are run using TestNG. All code to follow:

LoginPage.java

public class LoginPage {

    public static WebDriver driver;
    private static String username_selector = "username";
    private static String password_selector = "password";
    private static String login_button_selector = "#loginbutton > input";
    private static String fail_message_selector = "error";

    public static void goTo(String environment_url){
        driver = Driver.getDriver();
        driver.get(environment_url);
    }

    public static void loginAs(String username, String password){
        WebElement user = driver.findElement(By.name(username_selector));
        user.clear();
        user.sendKeys(username);
        WebElement pass = driver.findElement(By.name(password_selector));
        pass.clear();
        pass.sendKeys(password);
        WebElement loginBtn = driver.findElement(By.cssSelector(login_button_selector));
        loginBtn.click();
    }

    public static boolean loginErrorDisplayed(){
        WebElement failMessageContainer = driver.findElement(By.className(fail_message_selector));
            if(failMessageContainer.isDisplayed()){
                return true;
            }
            else return false;
    }

    public static String getLoginErrorMessage(){
        WebElement failMessageContainer = driver.findElement(By.className(fail_message_selector));
        String failMessage = failMessageContainer.getText();
        return failMessage;
    }

}

Driver.java

public class Driver {
    public static WebDriver driver = null;

    public static WebDriver getDriver(){
        if(driver == null){
            driver = new FirefoxDriver();
        }

        return driver;
    }

    public static void killDriver(){
        driver.quit();
    }
}

LoginTest.java

public class LoginTest {

@Test(priority = 0)
public void loginfail() {
    LoginPage.goTo("http://127.0.0.1:8080/login");
    LoginPage.loginAs("wrong username", "wrongpassword");
    boolean didLoginFail = LoginPage.loginErrorDisplayed();
    Assert.assertTrue(didLoginFail == true, "Bad login was successful");
    if (didLoginFail){
        LoginPage.getLoginErrorMessage();
    }
}

@Test(priority = 1)
public void loginsuccess() {
    LoginPage.loginAs("correct_username", "correctpass");
    boolean didLoginFail = LoginPage.loginErrorDisplayed();
    Assert.assertTrue(didLoginFail == false, "Valid Login was unsuccessful");
}

}

if(failMessageContainer.isDisplayed()){
            return true;
        }
        else return false;

Why not simply return failMessageContainer.isDisplayed()?

@Test(priority = 0)
public void loginfail() {
    LoginPage.goTo("http://127.0.0.1:8080/login");
    LoginPage.loginAs("wrong username", "wrongpassword");
    boolean didLoginFail = LoginPage.loginErrorDisplayed();
    Assert.assertTrue(didLoginFail == true, "Bad login was successful");
    if (didLoginFail){
        LoginPage.getLoginErrorMessage();
    }
}

What about Assert.assertTrue((((didLoginFail == true) == true) == true, ...). I mean, if writing a==true was better than a, then doing it more often must be even better.

The following if is superfluous, the getLoginErrorMessage doesn't get used.

  I've noticed that I am writing a lot of statics so that the tester doesn't have to instantiate the page objects and methods every time.

I'd start by dropping all the static modifiers and making it fluent. With a proper API the tester will happily do the instantiation as it leads them to what they want to do.

I don't find something like

public void loginfail() {
    LoginPage loginPage = new LoginPage()
        .goTo("http://127.0.0.1:8080/login")
        .loginAs("wrong username", "wrongpassword");
    Assert.assertTrue(loginPage.loginErrorDisplayed());
    Assert.assertTrue(loginPage.getLoginErrorMessage().contains("WTF?"));
}

harder to use than the static version. You could provide even more, but this is most probably not worth the effort (you'd sort of do the tester's job by providing fluent versions of all asserts they may need; this goes too far, but might be useful for the most commonly used assertions):

public void loginfail() {
    LoginPage loginPage = new LoginPage()
        .goTo("http://127.0.0.1:8080/login")
        .loginAs("wrong username", "wrongpassword")
        .assertThatLoginErrorDisplayed()
        .assertThatLoginErrorMessageContains("WTF?");
}


Open addressed hash table I'm attempting to implement various data structures and algorithms to better learn C.  I would appreciate comments on style, correctness of the implementation, memory management, etc.

One specific question is how to address an error case where either the table or key is null in the hash function.  In get or rm I can use an unsigned return of 0 or 1 representing a boolean value.

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

typedef struct HashTable {
  size_t size;
  char ** buckets;
} HashTable;

HashTable * create_table(size_t size);
int hash_code(char * key, HashTable * ht);
char * get_val(char * key, HashTable * ht);
unsigned int set_val(char * key, char * val, HashTable * ht);
unsigned int rm_val(char * key, HashTable * ht);

//Bernstein's function uses INITIAL_VALUE of 5381 and M of 33;
unsigned int INITIAL_VALUE = 5381;
unsigned int M             = 33;

/*
 * Function which allocates a Hash table and
 * bucket array of specified size
 * @param size the size of the bucket array
 *
 */
HashTable * create_table(size_t size)
{
  HashTable * ht = (HashTable*)malloc(sizeof(*ht));
  ht->size    = size;
  ht->buckets = (char **)malloc(sizeof(char*) * size);
  return ht;
}

/*
 * Function which returns multiplicative hash of key, which is used as bucket
 * @param key pointer to key
 * @param ht HashTable structure
 *
 */
int hash_code(char * key, HashTable * ht)
{
  if (ht == NULL || key == NULL)
    return -1;

  unsigned int hash = INITIAL_VALUE; //seed
  unsigned int i;
  for (i=0;i<strlen(key);i++)
    hash = M * hash + key[i];

  return hash % ht->size;
}

/*
 * Function which gets a value from the hashtable, computes hash of key
 * and queries the buckets at idx of hash
 * @param key pointer to key
 * @param ht HashTable structure
 *
 */
char * get_val(char * key, HashTable * ht)
{
  if (ht == NULL || key == NULL)
    return NULL;

  unsigned int hash = hash_code(key,ht);
  char * val = ht->buckets[hash];
  if (val != NULL)
    return val;
  return NULL;
}

/*
 * Function which sets a value in the hashtable, computes hash of key
 * and queries the buckets at idx of hash, if key is already set
 * it fails
 * @param key pointer to key
 * @param val pointer to val
 * @param ht HashTable structure
 *
 */
unsigned int set_val(char * key, char * val, HashTable * ht)
{
    if (ht == NULL || val == NULL || key == NULL)
      return 0;

  int hash = hash_code(key, ht);
  if (hash < 0)
    return 0;
  if (ht->buckets[hash] == NULL)
    {
      ht->buckets[hash] = val;
      return 1;
    } else
    {
      return 0;
    }
}

/*
 * Function which removes a value from the hash table
 * based upon key
 * @param key pointer to key
 * @param ht HashTable structure
 *
 */
unsigned int rm_val(char * key, HashTable * ht)
{
  if (ht == NULL || key == NULL)
    return 0;

  unsigned int hash = hash_code(key, ht);
  if (ht->buckets[hash] != NULL)
    {
      ht->buckets[hash] = NULL;
      return 1;
    } else
    {
      return 0;
    }
}

int main(void) {

    char * key = "foo";
    char * val = "bar";

    HashTable * ht = create_table(1000);
    unsigned int set = set_val(key, val, ht);
    if (set)
      {
    unsigned int rm = rm_val(key, ht);
    if (rm)
      {
        return 0;
      }
    }
    return 1;
}

Regarding the problem of the error case where either the table or key is null in hash_code, you might pass in ht->size instead of ht, thus halving the problem.  If you redefine the 'contract' so that a NULL key is treated the same as an empty string "", then you no longer have an error case.

Some comments on the code:

I'd expect a short description of the nature of the hashtable and how it is used at the start.
INITIAL_VALUE and M should be const
function parameters should be const where possible
return values not stated in headers
I would omit things like 'Function which'... from comments.  If it is a function then this can be assumed.
hash_code uses -1 as an error code while set/rm_val use 0.  Inconsistent.
Generally, unsigned should be used for values that are not numeric, such as bitmaps, not for numbers that you don't intend to go negative (although size_t contradicts that).

create_table()

don't cast malloc return value
check malloc return
ht->buckets not initialised to 0; use calloc instead?
you could avoid the first malloc by passing in a pointer to a statically allocated HashTable or by defining the buckets array as a flexible member and then allocate the necessary total size

typedef struct HashTable {

 size_t size;
 char *buckets[];

} HashTable;

hash_code():

parameter ht: I'd pass in the size (ht->size) instead of ht.  In general, it is easier to test a function that takes simple scalar inputs than one that receives structures.
use of strlen in loop control is inefficient; key size does not change but will be evaluated each loop.  Perhaps for (; *key; ++key) {...}
prefer spaces after ';' in for-loop conditions
return hash % ht->size; loses precision (compiler warning due to type conversion).  Just use int throughout unless your hash is ever going to be big enough for the extra bit to matter.  If you thought it might get big enough (eg if using a processor where int is small) then you would have to check for overflow of an unsigned int too (I'm not suggesting you do that here).

get_val()

ignores -ve return from hash_code on error
last 4 lines reduce to return ht->buckets[hash];

set_val()

Better to return int, not unsigned int
hash is signed. Other uses are unsigned.
should val be duplicated (allocated)?  If not you have to tell the  caller that the string must be persistent (ie. the caller must not free it or pass a stack value).
else { return 0; } at the end of function can be reduced to return 0;
comment could be said to be excessive - describing what is obvious from the code; its first ',' should be '.'  I would say just: "Set hashtable value unless already set.  Returns 1 on success, 0 on failure (value already set)."

rm_val()

Better to return int, not unsigned int
ignores -ve return from hash_code on error
else { return 0; } at the end of function can be reduced to return 0;

main()

use EXIT_SUCCESS and EXIT_FAILURE


performance of modulus operator vs if statement The following code

if (i >= N)
    i = 0;

has been corrected to:

i %= N;

for performance reasons. Does it makes sense?

More context: i is guaranteed to be non negative. The code is used to fill a circular buffer of length N, i is first incremented and then the check is performed. N depends on user input. The code is an interview exercise so there is no clear context.

EDIT
I missed the fact that this question was about C instead of C#. This likely invalidates my performance argument, so I removed that part of the answer. However, I do think that the readability argument (which is 90% of my answer) remains to be a relevant consideration.

You didn't really explain what the code does, and you really should have. We need the surrounding context to make sense of why each snippet is relevant.

However, if I can rely on the fact that whoever corrected your code is sure that the alternative works the same way, then I think I understand the scenario:

You are incrementing i, e.g. i++;
You're trying to have i increment from 0 up to N, multiple times. E.g. if N = 5, you want 0,1,2,3,4,0,1,2,3,4,... For the rest of the answer, I will refer to this behavior as "cyclical values".

The rest of the answer works under this assumption.

I agree with the corrected version.
The modulo operator is a fairly niche operator, compared to other C# operations. By that, I mean that when you encounter the modulo operator, you can almost immediately assume that you're dealing with cyclical values.
This cyclical behavior is exactly the case for you, so I do advocate the use of the modulo operator. It makes the cyclical nature of the algorithm easily discernible.

Distinguishing between the logical and the mathematical.
There are other cases where you can choose between a logical approach and a mathematical one. A simple example is for (mathematical) and foreach (logical):
for(int i = 0 ; i < myArray.Length; i++)
{
    var item = myArray[i];

    // ...
}

foreach(var item in myArray)
{
    //...
}

These two are functionally equivalent. However, more importantly, observe the intention of a for/foreach: it is used to direct the logical flow, it's not used (by itself) for calculating and storing values.
In the majority of cases, I would advocate the use of a foreach instead of a for, because it is easier to read a logical approach when the intention of the code is to direct the logical flow.
Note that for loops have their uses, e.g. when your iterations need a counter value to keep track of which iteration you're in. This is where the mathematical side of things becomes more important. But for the majority of use cases, you'll simply be iterating over an entire list and will not need a counter.
If you boil this down, you get to the following rule of thumb:

In cases where the mathematical and logical approaches are sufficiently equivalent (!) you should use the logical approach when you're directing the algorithm's logic, and you should use the mathematical approach when making calculations.
This closes the gap between the code and the intention of the code. The smaller the gap, the more readable the code will be.

The general idea here is that when you use a mathematical approach for a logical flow, then the developer needs to understand the mathematics to understand how the code will flow.
A sufficiently experienced developer will of course not get hindered by something he's familiar with, e.g. the simple for that I used. But as the logic gets more and more complex, the mathematical approach will compromise more and more of the code readability, faster than the logical approach will.

Let's go back to comparing your two options:
if (i >= N)
    i = 0;

i %= N;

Observe the intention of the code: You are trying to find the next value of i. That is inherently a mathematical operation, and therefore warrants the use of a mathematical approach over a logical one.


Merge entities which have the same children The C# code below, using LINQ to Entities, aims to merge all Class entities which have the same SubjectId and the same Students.

var sb = new StringBuilder();
var counter = 0;

using (var ctx = new Ctx())
{
  var allClasses = ctx.Classes.Where(o => o.SubjectId > 0).OrderBy(o => o.Id).ToList();
  foreach (var c in allClasses)
    ctx.LoadProperty(c, o => o.Students);

  // For each class...
  for (var i = 0; i < allClasses.Count; i++)
  {
      var c = allClasses[i];
      var duplicates = allClasses
             .Where(o => o.SubjectId == c.SubjectId && o.Id != c.Id &&
                         o.Students.OrderBy(s => s.Id).Select(s => s.Id)
                              .SequenceEqual(c.Students.OrderBy(s => s.Id)
                                                  .Select(s => s.Id)))
             .ToList();

      // Does it have any duplicates?
      if (duplicates.Count == 0)
          continue;

      ctx.LoadProperty(c, o => o.PeriodsTimetable);
      // For each duplicate...
      for (var j = 0; j < duplicates.Count; j++)
      {
          var d = duplicates[j];
          // If the duplicate class has a timetable slot that's not also allocated to
          // the class we are keeping, then allocate it (otherwise MySql will delete it).
          ctx.LoadProperty(d, o => o.PeriodsTimetable);
          var dt = d.PeriodsTimetable.ToList();
          for (var k = 0; k < dt.Count; k++)
          {
              var dtp = dt[k];
              var tpIsDuplicate = false;
              foreach (var ctp in c.PeriodsTimetable)
              {
                  if (dtp.TeacherId == ctp.TeacherId &&
                      dtp.PeriodTime == ctp.PeriodTime)
                  {
                      tpIsDuplicate = true;
                  }
              }

              if (!tpIsDuplicate)
                  dtp.ClassId = c.Id;
      }

      // Now update all other entities which reference the duplicate to instead
      // reference the class we are keeping.
      var dPeriods = ctx.Periods.Where(o => o.ClassId == d.Id).ToList();
      for (var k = 0; k < dPeriods.Count; k++)
          dPeriods[k].ClassId = c.Id;

      var dAssessments = ctx.Assessments.Where(o => o.ClassId == d.Id).ToList();
      for (var k = 0; k < dAssessments.Count; k++)
          dAssessments[k].ClassId = c.Id;

      var dSeatingPlans = ctx.SeatingPlans.Where(o => o.ClassId == d.Id).ToList();
      for (var k = 0; k < dSeatingPlans.Count; k++)
          dSeatingPlans[k].ClassId = c.Id;

      var dMerits = ctx.Merits.Where(o => o.ClassId == d.Id).ToList();
      for (var k = 0; k < dMerits.Count; k++)
          dMerits[k].ClassId = c.Id;

      sb.Append(d.LongName).Append(" merged with ").Append(c.LongName).Append(".
");
      // Finally, delete the duplicate
      ctx.Classes.DeleteObject(d);
      allClasses.Remove(d);
      counter++;
    }
  }
  ctx.SaveChanges();
  lblOutput.Text = "Merged " + counter + " duplicate classes.

" + sb.ToString();
}

For an idea of scale, there are about 1600 classes with 30 students each, and counter comes out at just over 300.

I'd really appreciate any suggestions.

Your code is very difficult to read.  (To give you an idea of how difficult it is, I've been looking at this for the past 2 hours so far)

You are using traditional for loops when you instead should be using foreach loops.
Your "loop variables" are poorly named.  Look toward the end of the code.  What is a c?  What is a d?  Leave the shorter names within smaller lambdas, index varialbes and possibly single instances of "utility" objects (e.g., StringBuilder or Regex).
You are trying too hard to write everything within a single line.  for loops, if conditions, large queries... all of them should span multiple lines.  With perhaps an exception to the if (duplicates.Count == 0) continue; line.  I'd be okay with that but you should try to keep those to a minimum (it might be possible that you could code it so that line is not needed).
You have a long chain of Append()s to the string builder when you really should use a single call to AppendFormat().
There are some snippets of your code which would work better either as a separate method or a LINQ query.  Use it.
LINQ to Entities should have made associations between your objects so your joins could be replaced by using the navigation properties.  If you don't have those set up, you should.
You also have some useless comments.  // For each class...  Do we really need a comment to tell us that?  Well, maybe since we were using for loops but that should have been avoided in the first place.

You have a lot of performance issues too.

You are calling ToList() on every query when you are simply just enumerating over it most of the time.
You have a lot of repeated loops on the same collection too.  You should be able to do a lot within a single loop.
Your query for finding duplicates will be a huge hit.  I don't have much to say on it right now but it's already making this O(n^2).
I have a feeling that you don't need the calls to LoadProperty().  From what I can tell, the majority of your code is filling in data manually when you probably don't need to.  Unfortunately I don't know the entity framework enough to know better (though I am looking into it right now).

Applying what I've explained to your code, I'd start with this (hopefully I didn't break anything).

var sb = new StringBuilder();
var counter = 0;
using (var ctx = new SchoolModel())
{
    var allClasses = ctx.Classes
        .Where(o => o.SubjectId > 0)
        .OrderBy(o => o.Id)
        .ToList();

    foreach (var aClass in allClasses)
    {
        ctx.LoadProperty(aClass, o => o.Students);

        // a properly crafted query could remove this completely and not use LINQ to Objects
        var students = new HashSet<int>(aClass.Students.Select(s => s.Id));
        var dupClasses = allClasses
            .Where(o => o.SubjectId == aClass.SubjectId
                     && o.Id != aClass.Id
                     && students.SetEquals(o.Students.Select(s => s.Id)))
            .ToList();

        if (dupClasses.Count == 0) continue;

        ctx.LoadProperty(aClass, o => o.PeriodsTimetable);

        foreach (var dupClass in dupClasses)
        {
            // If the duplicate class has a timetable slot that's not also allocated to the
            // class we are keeping, then allocate it (otherwise MySql will delete it).
            ctx.LoadProperty(dupClass, o => o.PeriodsTimetable);

            foreach (var dupTimetable in dupClass.PeriodsTimetable)
            {
                var isDuplicate = aClass.PeriodsTimetable
                    .Any(ctp => dupTimetable.TeacherId == ctp.TeacherId
                             && dupTimetable.PeriodTime == ctp.PeriodTime);
                if (!isDuplicate)
                    dupTimetable.ClassId = aClass.Id;
            }

            // Now update all other entities which reference the duplicate to instead reference
            // the class we are keeping.
            foreach (var dupPeriod in dupClass.Periods)
                dupPeriod.ClassId = aClass.Id;
            foreach (var dupAssessment in dupClass.Assessments)
                dupAssessment.ClassId = aClass.Id;
            foreach (var dupSeatingPlan in dupClass.SeatingPlans)
                dupSeatingPlan.ClassId = aClass.Id;
            foreach (var dupMerit in dupClass.Merits)
                dupMerit.ClassId = aClass.Id;

            sb.AppendFormat("{0} merged with {1}.
", dupClass.LongName, aClass.LongName);

            // Finally, delete the duplicate
            ctx.Classes.DeleteObject(dupClass);

            // not sure this is really needed
            allClasses.Remove(dupClass);
            counter++;
        }
    }
    ctx.SaveChanges();
    Output = "Merged " + counter + " duplicate classes.

" + sb.ToString();
}

That's my initial analysis so far.  I'll update some more once I've gained understanding on what you are doing.  As I've said, it is very difficult to read.  I'll probably offer more improvements once I've had a chance to look at it more.


Converting an object to an array of objects I've written a method that takes an object whose keys are arrays and converts it to an array of objects.

I feel like the code is fairly complex, after waiting a week or so and revisiting it, I don't think anyone would be able to really understand what is going on, especially if they aren't used to this style of coding. [read: I had no idea what I did there.]

function zipObj(obj) {
  // First get keys from object.
  var keys = _.keys(obj);

  // For each key, grab the data
  var propertyArray = _.map(keys, function(k, i) { return obj[k] || undefined; });

  // "Shift" propertyArray around, so we can make into array of objects
  var zippedArray = _.zip.apply(_, propertyArray);

  // Set up function that will take an array,
  // Then use the keys variable to re-create.
  var createObject = function(d) {
    var rObj = {};

    _.each(keys, function(k, i) { rObj[k] = d[i]; });
    return rObj;
  };

  // For each item in zipped array, return an object
  return _.map(zippedArray, createObject);
}

From what I understand, a lot of people writing in a functional style would reduce this down to smaller functions, however I'm not 100% sure the best way of doing that would be in this example.  I don't think reducing each step of this into its own functionality would necessarily increase readability, especially with how many little things that are going on here.

(If you open this and click Run, it will alert you with the function's inputs/ outputs.)

var input = {
  key1: [ 'val1', 'val2', 'val3' ],
  key2: [ 'val4', 'val5', 'val6' ],
  key3: [ 'val7', 'val8', 'val9' ]
}

window.alert("INPUT:\n" + JSON.stringify(input, null, '  '));

function zipObj(obj) {
  var arr = _.zip.apply(_, _.map(_.keys(obj), function(k, i) { return obj[k] || undefined; }));
  return _.map(arr, function(d) {
    var rObj = {};

    _.each(_.keys(obj), function(k, i) { rObj[k] = d[i]; });
    return rObj;
  });
}

window.alert("OUTPUT:\n" + JSON.stringify(zipObj(input), null, '  '));
<script src="//cdnjs.cloudflare.com/ajax/libs/underscore.js/1.7.0/underscore-min.js"></script>

When defining propertyArray, you don't need to handle undefined explicitly.  (It would have been nice to use _.pluck(), but unfortunately it is designed to take arguments in the wrong order.)

_.object() is too delicious to pass up!  You basically reimplemented it as createObject().  The function that is passed to map() can be further simplified by currying.

var input = {
  key1: [ 'val1', 'val2', 'val3' ],
  key2: [ 'val4', 'val5', 'val6' ],
  key3: [ 'val7', 'val8', 'val9' ]
};

window.alert("INPUT:\n" + JSON.stringify(input, null, '  '));

function zipObj(obj) {
  var keys = _.keys(obj);
  var values = _.map(keys, function(k) { return obj[k]; });

  // Transpose the values matrix
  var valueSlices = _.zip.apply(_, values);
  return _.map(valueSlices, _.partial(_.object, keys));
}

window.alert("OUTPUT:\n" + JSON.stringify(zipObj(input), null, '  '));
<script src="//cdnjs.cloudflare.com/ajax/libs/underscore.js/1.7.0/underscore-min.js"></script>

I suspect that var values = … could be simplified as _.values(obj), but I don't see anything in the Underscore.js documentation that guarantees that _.values(obj) will list the values in the same order as _.keys(obj), so I hesitate to recommend it.


Error solution: Uncaught TypeError Uncaught TypeError: document.getElementById(...) is null
I have a single JavaScript file that is connected to multiple pages. The below snippet is a function that is used for a single page. The above error appears when the user navigates to one of the pages that does not use the below function, and it references the document.getElementById('button01').addEventListener('click', newThing); line at the bottom. I've gathered that the error comes up due to the fact that button01 does not exist on these pages.
function newThing() {
const output = document.getElementsByTagName('output')[0];
if (!(document.forms.thingSelection2.type.value in options)) {
    return false;
}
const list = options[document.forms.thingSelection2.type.value];
const method = document.forms.thingSelection1.mode.value + 'Item';
const item = list[method]();
output.innerHTML = item;

}
document.getElementById('button01').addEventListener('click', newThing);

Solution
My solution for this is simple; place the line in an if statement like so:
if(document.getElementById('button01')) {
    document.getElementById('button01').addEventListener('click', newThing);
}

This removes the error from the pages that don't use it.
Questions
Does this open up the possibility for any buggy behavior that I don't know about? Is there a better way to achieve the same results?
HTML
I'm thankful for the feedback this post has received thus far. Since some of the answers contain recommendations based on assumptions of my HTML, I've decided to add the respective HTML in the below snippet. One thing to note is that I use two forms. This is required for the functionality of the code. I'm unaware of any unintended bugs this may cause. If it does, please let me know below.

<output></output>

<button id="button01">New Thing</button>

   <form name="thingSelection1">
      <input type="radio" name="mode" value="random" id="mode1">&nbsp;Random

<input type="radio" name="mode" value="forward" id="mode2">&nbsp;Old&nbsp;-&nbsp;New

<input type="radio" name="mode" value="reverse" id="mode3">&nbsp;New&nbsp;-&nbsp;Old
   </form>

   <form name="thingSelection2">
       Doodle&nbsp;<input type="radio" name="type" value="doodle" id="doodleCheck">

       Video&nbsp;<input type="radio" name="type" value="video" id="videoCheck">

       Audio&nbsp;<input type="radio" name="type" value="audio" id="audioCheck">

       Photo&nbsp;<input type="radio" name="type" value="photo" id="photoCheck">

       Text&nbsp;<input type="radio" name="type" value="text" id="textCheck">
   </form>

Furthermore, I'd like to point out that the original snippet is only part of my full JavaScript. Based on the information provided by this post, I've concluded that adding the remaining JavaScript, while possibly beneficial and relevant to some of the answers below, could possibly be out of the scope of the original topic of this post. Please note your thoughts on this in the comments below.

Does this open up the possibility for any buggy behavior that I don't know about?

It's not likely to. The if statement is fine, though it can be made cleaner:

Rather than selecting the element twice (once to check to see if it exists, another time to call addEventListener on it), save it in a variable:
const button = document.getElementById('button01');
if (button) {
  button.addEventListener('click', newThing);
}

Or, if you're writing JS which gets transpiled for production (which, in a professional or larger project, you really should be), use optional chaining:
document.getElementById('button01')?.addEventListener('click', newThing);

But the fundamental issue remains - the HTML layout is completely disconnected from the JavaScript. Having to check to see if a button exists in the first place is something that ideally shouldn't be an issue to worry about, at least in a larger or more professional project. What if you had not 1, but 3 or 5 or 10 elements with handlers on different pages, all of which may or may not exist? The codebase would be more difficult to maintain than it should be.
There are a few solutions for this:

One option is to have a separate

where thingSelection.js adds the event listener. But that'll require a separate request to the server, which may be a problem on a large page on HTTP 1.1 - if you have lots of different scripts like this, the sheer number of parallel requests could slow things down, especially for users with high latency. (The HTTP/2 protocol doesn't have issues with additional connections to the same server IIRC)
(You could also inline the script, eg </form>but I prefer putting scripts in separate files to permit caching)

But if it were me, I'd strongly prefer to fully integrate the creation of the HTML with the event listeners for that HTML, to make it completely inconceivable of one existing without the other, using a framework. For example, with React, you could do something along the lines of:

const Things = () => {
    const [item, setItem] = useState('');
    const [selectedOption, setSelectedOption] = useState('foo');
    const clickHandler = (e) => {
        const fn = options[selectedOption];
        if (fn) {
          setItem(fn());
        }
    };
    return (

            <select value={selectedOption} onChange={e => setSelectedOption(e.currentTarget.value)}>
                <option value="foo">foo</option>
                <option value="bar">bar</option>
            </select>
            <button onClick={clickHandler}>click</button>
            <output>{item}</output>

    );
};

const options = {
  foo: () => 'foo',
  bar: () => 'bar',
};

const Things = () => {
    const [item, setItem] = React.useState('');
    const [selectedOption, setSelectedOption] = React.useState('foo');
    const clickHandler = (e) => {
        const fn = options[selectedOption];
        if (fn) {
          setItem(fn());
        }
    };
    return (

            <select value={selectedOption} onChange={e => setSelectedOption(e.currentTarget.value)}>
                <option value="foo">foo</option>
                <option value="bar">bar</option>
            </select>
            <button onClick={clickHandler}>click</button>
            <output>{item}</output>

    );
};
ReactDOM.render(<Things />, document.querySelector('.react'));

Then when you're on a page where the Things are needed, you just render <Things />. With this approach, the HTML its associated JS handlers are all completely self-contained in the Things. There's no need to use selectors that may have unintentional collisions. (For example, going with your original code of document.getElementById('button01'), what if some other completely separate section of the HTML on one of the pages had an element which accidentally used the same ID? Then you'll have problems.)
Using a framework like this is admittedly a lot to learn, but it makes codebases significantly more maintainable. It's well worth it for medium-size or larger projects, IMO.

On a different note, your current code could also be improved a bit:
Prefer selectors Selector strings are usually more easily understood and more flexible than other methods of selecting elements (like getElementsByTagName and document.forms.someFormName.someFormElement). The selector string that matches an element will line up with the CSS selector that styles the element. For example, I'd replace:
document.forms.thingSelection2.type.value

with
document.querySelector('#thingSelection2 [name=type]').value

and
const output = document.getElementsByTagName('output')[0];

with
const output = document.querySelector('output');

(no need to select a collection when you only need the single matching element)
Save the value Instead of selecting and extracting the value twice, write DRY code; put it into a variable:
const { value } = document.querySelector('#thingSelection2 [name=type]');
if (value in options) { // Or, use `!options[value]
  return false;
}


Mounting a disk with the DiskArbitration framework I have the following code that makes use of the DiskArbitration framework to mount a disk to its default location:

#import <CoreFoundation/CoreFoundation.h>
#import <DiskArbitration/DiskArbitration.h>

void MountCallback(DADiskRef disk, DADissenterRef dissenter, void *context);

int main(int argc, const char *argv[]) {
  if (argc != 2) {
    fprintf(stderr, "Usage: %s <disk>\n", argv[0]);
    return EXIT_FAILURE;
  }

  const char *deviceName = argv[1];

  DASessionRef session = DASessionCreate(kCFAllocatorDefault);
  DADiskRef disk = DADiskCreateFromBSDName(kCFAllocatorDefault, session, deviceName);

  // Pass NULL for a "standard" mount path.
  DADiskMount(disk, NULL, kDADiskMountOptionWhole, MountCallback, (void *)deviceName);

  DASessionSetDispatchQueue(session, NULL);
  CFRelease(session);
  session = NULL;

  return EXIT_SUCCESS;
}

void MountCallback(DADiskRef disk, DADissenterRef dissenter, void *context) {
  const char *mountedDisk = context;
  fprintf(stderr, "Device mounted: %s\n", mountedDisk);
  fflush(stderr);
  return;
}

To compile the code:

clang -Wall -Werror -g -v main.m -lobjc -framework DiskArbitration -framework Foundation -o mount

To run the program:

./mount diskN

The program works fine, however as a developer without much knowledge on the Apple platform, I'm interested in the following points:

The MountCallback function doesn't appear to be called. I don't see the line I'm printing to stderr.
In case the mount operation fails, how do I access the error, and use it return a different exit status on main? (I assume the error is caught from the callback, but can't find documentation about it).
Am I releasing the session and other resources I'm using for the task correctly?

A few years later, and not being a C expert;
As far as I know (using Pascal myself), your application needs to wait for the callback to happen. By the looks of it though (again: not an expert) your application may already closed/finished before the callback actually happened.


Convert list of dictionaries to iterable list Due to another module coupled to my function, I can only receive the input to my part in the form of a JSON object structured roughly like this:

[{'id':0, 'y':4, 'value':25},{'id':0, 'y':2, 'value':254}]

Note that I do know that the data will arrive in exactly that format. Now, I need to cast this to lists such that I can pass it to the constructor of scipy.sparse.coo_matrix().
Since I have frequent incoming calls, I want to perform this cast as quickly as possible, which is why I am concerned with the optimization of this operation. Below are three different approaches that come up with this type of question. Note that the casting itself has been frequently addressed on Stackoverflow, even with respect to performance in some cases (mostly in terms of list comprehension), but I could not find anything that would give me an optimal solution.

To quickly address the three different methods I use:

evaluate each line as a tuple of the dictionary values. Comparable in speed with something like [[el['id'], el['y'], el['values']] for el in x].
Let pandas do the casting for you. Extremly slow.
Cast three separate lists. Since the allocation of lists in list comprehensions is way slower (compare [[el['id']] for el in x] to [el['id'] for el in x]), this seems to be the currently best-performing solution.

According to the articles I found, list comprehensions outperform any python-native method using .append(), but I might add an example and timing for that later.

The benchmarks are as follows:

import timeit as ti
# 2.107 seconds
print(ti.timeit("z = [tuple(el.values()) for el in x]",
          setup="import random;  x = [{'id':random.randint(0,5),'y':random.randint(0,5), 'value':random.randint(0,500)}]*60000",
          number=100))

# 8.93 seconds
print(ti.timeit("z = pd.DataFrame(x)",
          setup="import pandas as pd; import random;  x = [{'id':random.randint(0,5),'y':random.randint(0,5), 'value':random.randint(0,500)}]*60000",
          number=100))

# 0.717 seconds
print(ti.timeit("z1 = [el['id'] for el in x]; z2 = [el['y'] for el in x]; z3 = [el['value'] for el in x]",
          setup="import random;  x = [{'id':random.randint(0,5),'y':random.randint(0,5), 'value':random.randint(0,500)}]*60000",
          number=100))

I also include the "raw code" for the three snippets:

import random
import pandas as pd

if __name__ == "__main__":
    # ignore the fact that this actually isn't random for individual values
    x = [{'id':random.randint(0,5),'y':random.randint(0,5), 'value':random.randint(0,500)}]*60000
    # first method
    z1 = [tuple(el.values()) for el in x]
    # second method
    z2 = pd.DataFrame(x)
    # third method
    z_3a = [el['id'] for el in x]
    z_3b = [el['y'] for el in x]
    z_3c = [el['value'] for el in x]

The question is whether there is any significant improvement on this (maybe by using a specialized library I don't know of, or any trick with numpy, etc.) to easily improve the speed on this. I'm currently assuming that, following the 80/20 principle, it is unlikely I'll get more performance out of this without spending a lot more effort on it...

Does the scipy.sparse.coo_matrix() function accept three different lists as parameters?

Maybe I'm missing something but your "fastest" method is not actually the fastest, since you'd still need some conversion to parse it to the function anyhow.
I think your timing is off

  For this one z = [tuple(el.values()) for el in x] you say:

  evaluate each line as a tuple of the dictionary values. Comparable in speed with something like [[el['id'], el['y'], el['values']] for el in x].

I have tested this myself, and it seems the second suggestion is 2 times as fast as the first method.

Setup: x = [{'id':random.randint(0,5),'y':random.randint(0,5), 'value':random.randint(0,500)}]*100000

z = [tuple(el.values()) for el in x]
3.070572477

z1 = [[el['id'], el['y'], el['value']] for el in x]
1.5449943620000006

This could also be a generator

def gen_vals(d):
    for el in d:
        yield [el['id'], el['y'], el['value']]

However this did not improve speed, but just to bounce some ideas around

Setup: x = [{'id':random.randint(0,5),'y':random.randint(0,5), 'value':random.randint(0,500)}]*100000

z1 = [el['id'] for el in x]; z2 = [el['y'] for el in x]; z3 = [el['value'] for el in x]
1.244481049000001

z1 = [[el['id'], el['y'], el['value']] for el in x]
1.5449943620000006

z1 = list(gen_vals(x))
1.8347137390000015

z1 = [a for a in gen_vals(x)]
2.018111815000001

Timing code for reference

import timeit as ti
import random

def gen_vals(d):
    for el in d:
        yield [el['id'], el['y'], el['value']]

# SETUP
x = [{'id':random.randint(0,5),'y':random.randint(0,5), 'value':random.randint(0,500)}]*100000
print("Setup: x = [{'id':random.randint(0,5),'y':random.randint(0,5), 'value':random.randint(0,500)}]*100000")

print("z = [tuple(el.values()) for el in x]")
print(ti.timeit("z = [tuple(el.values()) for el in x]",
          setup="from __main__ import x",
          number=100))

print("z = pd.DataFrame(x)")
print(ti.timeit("z = pd.DataFrame(x)",
          setup="import pandas as pd; from __main__ import x",
          number=100))

print("z1 = [el['id'] for el in x]; z2 = [el['y'] for el in x]; z3 = [el['value'] for el in x]")
print(ti.timeit("z1 = [el['id'] for el in x]; z2 = [el['y'] for el in x]; z3 = [el['value'] for el in x]",
          setup="from __main__ import x",
          number=100))

print("z1 = [[el['id'], el['y'], el['value']] for el in x]")
print(ti.timeit("z1 = [[el['id'], el['y'], el['value']] for el in x]",
          setup="from __main__ import x",
          number=100))

print("z1 = list(gen_vals(x))")
print(ti.timeit("z1 = list(gen_vals(x))",
          setup="from __main__ import x, gen_vals",
          number=100))

print("z1 = [a for a in gen_vals(x)]")
print(ti.timeit("z1 = [a for a in gen_vals(x)]",
          setup="from __main__ import x, gen_vals",
          number=100))


Python Character Creator Program Q1. Can the following code be made more efficient and simple?

Q2. How is the general structure of the code?

Q3. Would this be better written in def/classes? If yes, what would the code look like?

Any other opinions/ tips are completely welcome. Thank you.

# Challenge 3 // Chapther 5 Python for The Absolute Beginner Book
# Python 3.4.3 // MAC 0SX (Latest) // 01.08.2017
from time import sleep

pool_points = 30
menu_option = None
attribute_words = ("Strength, Health, Wisdom & Dexterity")
attributes = {"strength": 0,
              "health": 0,
              "wisdom": 0,
              "dexterity": 0}
GENERAL_MENU = ("""\t\tMENU
    0 = Exit and Final Result
    1 = Add Points to Attributes
    2 = Remove Points from Attributes
    3 = Print Menu
    """)

print("Welcome to Character Builder!")
print()
print(GENERAL_MENU)

while menu_option != "0":
    print()
    sleep(1) #provides user friendly reading
    menu_option = input("Please select an option: ")
    print()

    #exit
    if menu_option == "0":
        print("Creating Character Profile... ")

    #add
    elif menu_option == "1":
        print("ADD Points To An Attribute!")
        print("Attributes Available:", attribute_words)
        print("Points Available:", pool_points)
        print()
        attribute_name = input("Attribute Name: ").lower()
        if attribute_name in attributes:
            value_points = int(input("Value: "))
            if value_points <= pool_points and value_points >= 0:
                pool_points -= value_points
                attributes[attribute_name] += value_points
                print("Points ADDED to,", attribute_name.title())
            else:
                print(value_points, "is an incorrect value. Try again...")
        else:
            print("Unfortunately,", attribute_name, "doesn't exist!")

    #remove
    elif menu_option == "2":
        print("REMOVE Points From An Attribute!")
        print("Attributes Available:", attribute_words)
        print("Points Available:", pool_points)
        print()
        attribute_name = input("Attribute Name: ").lower()
        if attribute_name in attributes:
            value_points = int(input("Value: "))
            if value_points <= int(attributes[attribute_name]) and value_points >= 0:
                pool_points += value_points
                attributes[attribute_name] -= value_points
                print("Points REMOVED from,", attribute_name.title())
            else:
                print(value_points, "is an incorrect value. Try again...")
        else:
            print("Unfortunately,", attribute_name, "doesn't exist!")

    #print menu
    elif menu_option == "3":
        print(GENERAL_MENU)

    #if all else fails!
    else:
        print("INVALID! Select an option from the menu... ")
        print()
        print(GENERAL_MENU)

#final results
sleep(1)
print("-----------------------")
print("Points Remaining", "=>", pool_points, "|")
print("-----------------------")
for i in attributes.keys():
    print(i.title(), "=>", attributes[i], "\t       |")
    print("-----------------------")

input()

As for the overall structure of the code, I would try to avoid the if-elif-else chains. Instead you can use either a dictionary with a keys "0", "1", etc. and with functions such as exit_menu, add_points etc. or a list that stores just stores exit_menu add_points etc. in the order and in a particular order.

To go even beyond this, you could add a function attribute to exit_menu, add_points, etc. called display_name and generate the start menu by iterating through the list and loading each display_name.

Here is an (incomplete) example of what it should look like:

def create_profile():
    print("Creating Character Profile... ")

option_dispatch = [create_profile]

while menu_option != 0:
    menu_option = int(input("Please select an option: "))
    option_dispatch[menu_option]()


Importing data from spreadsheet Not really an issue I am stuck on (the code works) but I just really don't like some code I wrote, if you're bored in lockdown maybe you might like to help.

Context:
I am import some data from a spreadsheet, it contains data about retail stores (grocery stores). As the data comes in from the spreadsheet, each row is stored as a StoreData object. Each store has upto five tills (point of sale devices) and I want the StoreData class to be able to return an array of information about each till. Here is the class showing the code relevant to the tills (the class is much bigger).

How can I clean up the function getTills()?

class StoreData
{
    private $spreadsheetData;

    private $tillModel;
    private $tillVersion;
    private $tillOneName;
    private $tillOneIpAddress;
    private $tillTwoName;
    private $tillTwoIpAddress;
    private $tillThreeName;
    private $tillThreeIpAddress;
    private $tillFourName;
    private $tillFourIpAddress;
    private $tillFiveName;
    private $tillFiveIpAddress;

    public function __construct($spreadsheetData, String $storeStatus)
    {
        $this->spreadsheetData = $spreadsheetData;
        $this->setStoreProperties();
    }

    private function setStoreProperties()
    {
        $this->tillModel = $this->spreadsheetData[17];
        $this->tillVersion = $this->spreadsheetData[7];
        $this->tillOneName = $this->spreadsheetData[40];
        $this->tillOneIpAddress = $this->spreadsheetData[41];
        $this->tillTwoName = $this->spreadsheetData[42];
        $this->tillTwoIpAddress = $this->spreadsheetData[43];
        $this->tillThreeName = $this->spreadsheetData[44];
        $this->tillThreeIpAddress = $this->spreadsheetData[45];
        $this->tillFourName = $this->spreadsheetData[46];
        $this->tillFourIpAddress = $this->spreadsheetData[47];
        $this->tillFiveName = $this->spreadsheetData[48];
        $this->tillFiveIpAddress = $this->spreadsheetData[49];
    }

    public function getTills()
    {
        $tillList = [];

        if ($this->tillOneName !== null)
        {
            $tillList[] = [
                'model' => $this->tillModel,
                'name' => $this->tillOneName,
                'ip_address' => $this->tillOneIpAddress,
                'version' => $this->tillVersion,
            ];
        }
        if ($this->tillTwoName !== null)
        {
            $tillList[] = [
                'model' => $this->tillModel,
                'name' => $this->tillTwoName,
                'ip_address' => $this->tillTwoIpAddress,
                'version' => $this->tillVersion,
            ];
        }
        if ($this->tillThreeName !== null)
        {
            $tillList[] = [
                'model' => $this->tillModel,
                'name' => $this->tillThreeName,
                'ip_address' => $this->tillThreeIpAddress,
                'version' => $this->tillVersion,
            ];
        }
        if ($this->tillFourName !== null)
        {
            $tillList[] = [
                'model' => $this->tillModel,
                'name' => $this->tillFourName,
                'ip_address' => $this->tillFourIpAddress,
                'version' => $this->tillVersion,
            ];
        }
        if ($this->tillFiveName !== null)
        {
            $tillList[] = [
                'model' => $this->tillModel,
                'name' => $this->tillFiveName,
                'ip_address' => $this->tillFiveIpAddress,
                'version' => $this->tillVersion,
            ];
        }
        return $tillList;
    }
}

For info, this is used to populate an eloquent model on a controller:

           foreach($storeData->getTills() as $till)
            {
                $store->tills()->create($till);
            }

You might want to create a service that accepts the spreadsheet data as method argument instead.

And maybe the columns could be configured to make it more flexible.

Also no need to do 5 times the same again, instead use a loop.

class SpreadsheetTillsConverter
{
  private int $modelColumn;
  private int $versionColumn;

  /** @var array<int, int> Map nameColumn => ipAddressColumn */
  private array $columns;

  public function __construct(int $modelColumn, int $versionColumn, array $columns)
  {
    $this->modelColumn = $modelColumn;
    $this->versionColumn = $versionColumn;
    $this->columns = $columns;
  }

  public function getTills(array $data): array
  {
     if (!isset($data[$this->modelColumn])) {
        throw new \InvalidArgumentException('Model column not found.');
     }
     $model = $data[$this->modelColumn];

     if (!isset($data[$this->versionColumn])) {
        throw new \InvalidArgumentException('Version column not found.');
     }
     $version = $data[$this->versionColumn];

     $result = [];
     foreach ($this->columns as $nameColumn => $ipAddressColumn) {
        if (isset($data[$nameColumn], $data[$ipAddressColumn])) {
          $result[] = [
            'model' => $model,
            'name' => $data[$nameColumn],
            'ip_address' => $data[$ipAddressColumn],
            'version' => $version,
          ];
        }
     }
     return $result;
  }
}

$converter = new SpreadsheetTillsConverter(17, 7, [
  40 => 41,
  42 => 43,
  44 => 45,
  46 => 47,
  48 => 49
]);

$tills = $converter->getTills($spreadsheetData);


Am I implementing BDD correctly? I'm writing a utility that validates fields. I decided to try my hand at Behaviour Driven Development (BDD). The validator utilises "rules" to determine if field is valid.

Three different types of rules exist:

RegexRule A field conforms to specific pattern(s)
RequireRule A value is present
EqualityRule A field's value equals another's value

A rule is tested against a value. If the value does not conform to the rule, a flag is set which marks the rule as erroneous. Calling getError() on an erroneous rule returns a string that details the error. Furthermore, the rule is reset (no longer erroneous).

Is this the correct manner of implementing BDD on this class? I'd greatly appreciate advice on how I could improve on this.

describe('RequireRule', function()  {
    var nameErr = 'The name field is required';

    beforeEach(function()  {
        this.r = new RequireRule('name', nameErr);
    });

    describe('test()', function()  {
        it('should return false when value provided is null or undefined', function()  {
            expect(this.r.test('asd')).to.equal(true);
        });
        it('should return true when value provided is not null or undefined', function()  {
            expect(this.r.test(null)).to.equal(false);
        });
    });

    describe('isErroneous()', function()  {
        it('should become erroneous when value tested is null or undefined', function()  {
            r.test(null);
            expect(this.r.isErroneous()).to.equal(true);
        });
        it('should not be erroneous when value tested is not null or undefined', function()  {
            r.test('asd');
            expect(this.r.isErroneous()).to.equal(false);
        });
    });

    describe('getError()', function()  {
        it('should return null when the test() succeeded', function()  {
            r.test('asd');
            expect(this.r.getError()).to.equal(null);
        });
        it('should return the error string when the test() failed, and set erroneous to false', function()  {
            r.test(null);
            expect(this.r.getError()).to.equal(nameErr);
            expect(this.r.isErroneous()).to.equal(false);
        });
    });
});

The test framework is Mocha.

This is roughly what BDD should look like.  You're definitely on the right track in that you have a specific set of postconditions and expected results for every "public" method.  However, the tests for these postconditions are incomplete in some respects that I think will probably come up as edge conditions and/or browser quirks (assuming your system is going to run in a browser).  In addition, you're testing behaviors in this suite that are shared between your subclasses - even if they don't actually share a prototype, they do have a common interface and common expectations.

For incompleteness, I'm referring to 'should return false when value provided is null or undefined' when the corresponding test only tests null.  This should be two tests, one for null and another for undefined.  In addition, you should define and test the function's behavior for other values that are commonly mistaken for null and undefined, like 0, false, and ''.  The empty string is of particular importance because it's unclear from your tests whether an empty string is acceptable as long as it's specified - the source of your data might determine whether that's applicable.  I would go with two or three tests per native type: one that is acceptable, one that is not (if both are applicable), and one that is falsy (if the falsy value for the type is not already covered).  If you really want to go all out, define behavior for the parameters '0', 'false', 'undefined', and NaN.

This sounds like a pain to do - especially when you see that you need to define said behavior for all of the other functions whose preconditions are the same.  The root of the problem here is that your preconditions are not very DRY.  Each function has two test cases: null and 'asd' as inputs to test.  Since null and 'asd' are not really the only two inputs you can expect to get, your tests shouldn't be coupled to those two inputs.  Further, when you go to write your tests for your other rule subclasses, you will be unable to re-use your isErroneous and getError tests because they have specifically named null as erroneous and 'asd' as acceptable.

I'm not a mocha expert, so I don't know how helpful or cooperative it will be, but you can refactor your suites to test states rather than classes, and it will be far DRYer.  From what I can tell, the rule subclasses all have the following features:

A (theoretically unbounded) set of inputs to the test method that will:

return false
leave the object in an erroneous state

A (theoretically unbounded) set of inputs to the test method that will:

return true
leave the object in a validated state

An initial state before test is ever called, whose behavior remains unspecified.

Each of your subclasses has a different set of erroneous and valid inputs, but you can easily define the behavior of those three states independently:

A rule object in an erroneous state should:

return true from isErroneous()
return non-null from getError()
(become | not become) valid when a valid input is provided to test

A rule object in a validated state should:

return false from isErroneous()
return null from getError()
(become | not become) erroneous when an invalid input is provided to test

A rule object in its initial state should:

probably have defined behavior for getError and isErroneous, too.
become valid when a valid input is provided to test
become erroneous when an invalid input is provided to test

Now, your tests for all of your classes are reduced to the task of generating the input test cases and expected states they will generate.  For the RequiredRule, that could be easily expressed in two arrays; you might want something different for your more complicated rules, since the criteria are presumably individualized per object for those subclasses.  I might go with something like a generator.

The concept of a class is helpful for BDD when you have specific class invariants, but with javascript you are by no means limited to a one-to-one correspondence between a class (constructor/prototype) and its defined behaviors.  Like anything else in software design, you can tell your unit test is wrong as soon as you type the same thing 2 or 3 times.  Usually, you can fix the repetition by making the object of your suite (parameter to describe) something more applicable than the name of the constructor.