Nucleotide count in Scala This is my second day in learning Scala and I still need to develop a taste of functional programming, I often find myself doing imperative coding. Below is the result of my TDD practice.

Code

class DNA(dnaSequence: String) {
  var count = collection.mutable.Map('A' -> 0, 'T' -> 0, 'C' -> 0, 'G' -> 0)

  dnaSequence.foreach { (i: Char) =>
    validate(i)
    count = count.updated(i, count.get(i).getOrElse(0) + 1)
  }

  def nucleotideCounts() = count

  def nucleotideCounts(nucleotide: Char) = {
    validate(nucleotide)
    count.get(nucleotide).getOrElse(0)
  }

  private def validate(chr: Char) = {
    if (!count.contains(chr)) throw new RuntimeException()
  }
}

Test Suite

import org.scalatest._

class NucleotideCountSpecs extends FlatSpec with Matchers {
  "empty dna string" should "have no adenine" in {
    new DNA("").nucleotideCounts('A') should be (0)
  }

  it should "have no nucleotides" in {
    val expected = Map('A' -> 0, 'T' -> 0, 'C' -> 0, 'G' -> 0)
    new DNA("").nucleotideCounts should be (expected)
  }

  "a repetitive sequence" should "count cytosine" in {
    new DNA("CCCCC").nucleotideCounts('C') should be (5)
  }

  it should "have only guanine" in {
    val expected = Map('A' -> 0, 'T' -> 0, 'C' -> 0, 'G' -> 8)
    new DNA("GGGGGGGG").nucleotideCounts should be (expected)
  }

  "a mixed dna string" should "count only thymine" in {
    new DNA("GGGGGTAACCCGG").nucleotideCounts('T') should be (1)
  }

  it should "count a nucleotide only once" in {
    val dna = new DNA("CGATTGGG")
    dna.nucleotideCounts('T')
    dna.nucleotideCounts('T') should be (2)
  }

  it should "not change counts after counting adenine" in {
    val dna = new DNA("GATTACA")
    dna.nucleotideCounts('A')
    val expected = Map('A' -> 3, 'T' -> 2, 'C' -> 1, 'G' -> 1)
    dna.nucleotideCounts should be (expected)
  }

  it should "validate nucleotides" in {
    a [RuntimeException] should be thrownBy new DNA("GACT").nucleotideCounts('X')
  }

  it should "validate dna not rna" in {
    a [RuntimeException] should be thrownBy new DNA("ACGU")
  }

  it should "validate dna" in {
    a [RuntimeException] should be thrownBy new DNA("John")
  }

  it should "count all nucleotides" in {
    val s = "AGCTTTTCATTCTGACTGCAACGGGCAATATGTCTCTGTGTGGATTAAAAAAAGAGTGTCTGATAGCAGC"
    val dna = new DNA(s)
    val expected = Map('A' -> 20, 'T' -> 21, 'G' -> 17, 'C' -> 12)
    dna.nucleotideCounts should be (expected)
  }
}

Source

Encapsulation and information hiding

The parameterless nucleotideCounts method exposes the map of counts.
If later you wanted to change the way you store the counts,
for example you wanted to store in a database instead of a map in memory,
you won't be able to do that,
because you did not hide this information.

Exposing the map is especially suspicious since the map is mutable.
Users of this class could circumvent the validation logic,
for example delete nucleotides or add invalid nucleotides.

In any case, it would be better to declare the count map private.

Unnecessary getOrElse

Thanks to the validate method,
and if we assume that users of the parameterless nucleotideCounts method will not abuse the class and delete nucleotides,
it's guaranteed that count.get(c).get will always find a value,
so you can replace all occurrences of count.get(c).getOrElse(0).

Mutable or not

It's not clear why you use a mutable map.
Your current implementation and tests would work with an immutable map too.

If you really want to use a mutable map, then you can replace this:

count = count.updated(i, count.get(i).getOrElse(0) + 1)

with:

count.update(c, count.get(c).get + 1)

Naming

count is a not a great name for a collection type.
For example the plural counts would be a more fitting name,
which would also match the naming of your parameterless nucleotideCounts method.

You used multiple names for a character in a DNA string: i, chr, nucleotide. It would be easier to read the code if you used a consistent name. i is especially a bad name, as it's typically used in counting loops.


Graphs and Kruskal's Algorithm - Python OOP This is my implementation of a graph and Kruskal's algorithm in Python. I wanted to design the graph module myself and I would like some feedback on the design. I tried to follow SOLID throughout. I am not sure if the separate Vertex object is wise, but I feel it could be useful as I expand this module.

I had a copy of a flowchart of Kruskal's algorithm from a textbook (not my current course) and I decided to implement it, I am wondering how Pythonic my code is.

I have also programmed Prim's algorithm in the same file but I will split it over two questions.

class Vertex:
    def __init__(self, name):
        self.name = name

    def __str__(self):
        return f"Vertex {self.name}"

class Edge:
    def __init__(self, start, end, weight):
        self.start = start
        self.end = end
        self.weight = weight

    def __str__(self):
        return f"{self.start}{self.end}"

class Graph:
    def __init__(self, v, e):
        self.vertices = v
        self.edges = e

    def vertex_from_name(self, name):
        """ Return vertex object given vertex name. """
        return next((v for v in self.vertices if v.name == name), None)

    def add_edge(self, start, end, weight):
        """ Add an edge connecting two vertices. Arguments can either be vertex name or vertex object. """
        if isinstance(start, str):
            start = self.vertex_from_name(start)
        if isinstance(end, str):
            end = self.vertex_from_name(end)

        self.edges.append(Edge(start, end, weight))

    def edge_on_vertex(self, v):
        """ Return edges connected to given vertex v."""
        return [e for e in self.edges if (e.start == v) or (e.end == v)]

    def connected_vertices(self, v):
        """ Return the vertices connected to argument v."""
        if isinstance(v, str):
            v = self.vertex_from_name(v)

        return [e.start for e in self.edges if e.end == v] + [e.end for e in self.edges if e.start == v]

    def union(self, lst, e1, e2):
        """ Given a list of lists, merges e1 root list with e2 root list and returns merged list."""
        xroot, yroot = [], []
        # Find roots of both elements
        for i in lst:
            if e1 in i:
                xroot = i
            if e2 in i:
                yroot = i
        # Same root, cannot merge
        if xroot == yroot:
            return False
        xroot += yroot
        lst.remove(yroot)
        return lst

    def is_cycle(self):
        """ Return if the graph contains a cycle. """
        self.sets = [[v] for v in self.vertices]
        self._edges = sorted(self.edges, key=lambda x: x.weight)
        for e in self._edges:
            _temp = self.union(self.sets, e.start, e.end)
            if _temp == False:
                return True
            else:
                self.sets = _temp
        return False

    def Kruskals(self):
        """ Return MST using Kruskal's algorithm. """
        self.tree = Graph([], [])
        self.tree.vertices = self.vertices
        self.sorted_edges = sorted(self.edges, key=lambda x: x.weight)
        self.tree.edges.append(self.sorted_edges.pop(0))

        for edge in self.sorted_edges:
            self.tree.edges.append(edge)
            if self.tree.is_cycle():
                self.tree.edges.remove(edge)

        return self.tree

if __name__ == "__main__":
    v = [Vertex(x) for x in ["A", "B", "C", "D", "E", "F"]]

    g = Graph(v, [])

    g.add_edge("A", "B", 9)
    g.add_edge("A", "C", 12)
    g.add_edge("A", "D", 9)
    g.add_edge("A", "E", 11)
    g.add_edge("A", "F", 8)
    g.add_edge("B", "C", 10)
    g.add_edge("B", "F", 15)
    g.add_edge("C", "D", 8)
    g.add_edge("D", "E", 14)
    g.add_edge("E", "F", 12)

    print(g.Kruskals().edges)

Type hints

def __init__(self, start, end, weight):

can be

def __init__(self, start: Vertex, end: Vertex, weight: float):

depending on a few things, including the order of declaration of your classes, Vertex might need to be 'Vertex' here.

For another example, this

def vertex_from_name(self, name):

can turn into

def vertex_from_name(self, name: str) -> Vertex:

Efficient lookups

To make this more efficient:

    return next((v for v in self.vertices if v.name == name), None)

Consider maintaining a string-to-Vertex dictionary to reduce this lookup from O(n) to O(1) in time.

Premature materialization

These:

    return [e for e in self.edges if (e.start == v) or (e.end == v)]

    return [e.start for e in self.edges if e.end == v] + [e.end for e in self.edges if e.start == v]

require that the entire results be stored to an in-memory list. To return the generator directly and reduce this memory requirement, the first one can be

    return (e for e in self.edges if v in {e.start, e.end})

and the second one can be

yield from (e.start for e in self.edges if e.end == v)
yield from (e.end for e in self.edges if e.start == v)

Set-membership tests

This:

""" Given a list of lists, merges e1 root list with e2 root list and returns merged list."""

is probably better-expressed as accepting a list of set, not a list of lists. That will make these two tests:

        if e1 in i:
            xroot = i
        if e2 in i:
            yroot = i

faster. This:

    self.sets = [[v] for v in self.vertices]

would then become

    self.sets = [{v} for v in self.vertices]

Strings as iterables

This

v = [Vertex(x) for x in ["A", "B", "C", "D", "E", "F"]]

can be

v = [Vertex(x) for x in 'ABCDEF']

Convenience functions

Consider making a convenience function to turn this

g.add_edge("A", "B", 9)
g.add_edge("A", "C", 12)
g.add_edge("A", "D", 9)
g.add_edge("A", "E", 11)
g.add_edge("A", "F", 8)
g.add_edge("B", "C", 10)
g.add_edge("B", "F", 15)
g.add_edge("C", "D", 8)
g.add_edge("D", "E", 14)
g.add_edge("E", "F", 12)

into

g.add_edges(
    ("A", "B", 9),
    ("A", "C", 12),
    ("A", "D", 9),
    ("A", "E", 11),
    ("A", "F", 8),
    ("B", "C", 10),
    ("B", "F", 15),
    ("C", "D", 8),
    ("D", "E", 14),
    ("E", "F", 12),
)


Paginating API with "next" Pythonically I'm wondering if there's a more Pythonic way to keep paginating an API endpoint until there's no next key in the response. The thing that bothers me the most is the for item in json_resp.get("data") (I know this can be extracted as a function).

# some code is omitted, some obfuscated
# attachment of token in API calls omitted for code brevity
def get_object_list_for_user(user_service_token):
    endpoint = f"{HOSTNAME}/me/library/objects/"
    params = {"limit": 100}

    response = _call_service_api(endpoint, params=params)
    json_resp = response.json()

    lists_of_objects_to_append = []

    # get first batch
    for item in json_resp.get("data"):
        object_dict = {"library_id": item["id"], "name": item["attributes"]["name"]}
        lists_of_objects_to_append.append(object_dict)

    offset = 0
    while json_resp.get("next"):
        offset += 100
        params.update({"offset": offset})
        response = _call_service_api(endpoint, params=params)
        json_resp = response.json()

        for item in json_resp.get("data"):
            object_dict = {"library_id": item["id"], "name": item["attributes"]["name"]}
            lists_of_objects_to_append.append(object_dict)

    return lists_of_objects_to_append

I attempted to reduce redundancy by simulating a do-while loop so that the first condition initializes the sequence and then the code can be factored out.

Instead of having a temporary list, it returns a generator which can be appended to the list.

I couldn't test this so I don't know if it works:

def get_object_list_for_user(user_service_token):
    endpoint = f"{HOSTNAME}/me/library/objects/"
    params = {"limit": 100}
    offset = 0

    lists_of_objects_to_append = []

    while True:
      json_resp = _call_service_api(endpoint, params=params).json()
      lists_of_objects_to_append.extend(extract_items(json_resp))

      offset += 100
      params.update({"offset": offset})

      if not json_resp.get("next"):
          return lists_of_objects_to_append

def extract_items(json_resp):
    for item in json_resp.get("data"):
        yield({"library_id": item["id"], "name": item["attributes"]["name"]})


JavaScript Colour Converter I've made a little colour conversion app, and below is my JavaScript.

The app converts HEX values to RGB values and vice versa, as well as using a simple colour picker to get colours and convert them to both HEX and RGB.

Here is a picture so it is easier to understand what the code does. Basically you enter a HEX or RGB value into HTML input fields, and then the converted RGB/HEX value gets printed out.

You can also choose a colour from the 'Pick A Colour' section, and the colour selected also gets converted into HEX/RGB:

Does anyone have any suggestions to shorten this?

var svgPath = document.getElementById('path');
var colorInput = document.getElementById('color-input');
var hexOutput = document.getElementById('hex-output');
var rgbOutput = document.getElementById('rgb-output');
var rgb = document.getElementById("rgb");
var rgb2 = document.getElementById("rgb2");
var rgb3 = document.getElementById("rgb3");

// HEX to RGB
function hexToRGB(hex) {
    var bigint = parseInt(hex, 16);
    var r = (bigint >> 16) & 255;
    var g = (bigint >> 8) & 255;
    var b = bigint & 255;
    return r + ', ' + g + ', ' + b;
}

// RGB to HEX
function rgbToHEX(red, green, blue) {
    var rgb = blue | (green << 8) | (red << 16);
    return '#' + (0x1000000 + rgb).toString(16).slice(1);
}

// Gets HEX input
document.getElementById("hex").addEventListener("input", function() {
    // Sets colours for SVG, picker, converts HEX to RGB and prints out HEX
    svgPath.style.fill = '#' + this.value;
    colorInput.value = '#' + this.value;
    hexOutput.innerHTML = 'HEX: #' + this.value.toUpperCase();
    rgbOutput.innerHTML = 'RGB: rgb(' + hexToRGB(this.value) + ')';
}, false);

// Selects elements under .rgb class
var rgbInput = document.querySelectorAll('.rgb');
for(var i = 0; i < rgbInput.length; ++i){
    rgbInput[i].addEventListener("input", rgbCalc);
}

function rgbCalc() {
    // Skips to next input box if 3 characters have been entered
    if (rgb.value.length >= this.maxLength) {
        rgb2.focus();
    }
    if (rgb2.value.length >= this.maxLength) {
        rgb3.focus();
    }

    // Stores the r, g, b values
    var r = rgb.value;
    var g = rgb2.value;
    var b = rgb3.value;

    // Sets colours for SVG, picker, converts RGB to HEX and prints out RGB
    svgPath.style.fill = rgbToHEX(r, g, b);
    colorInput.value = rgbToHEX(r, g, b);
    rgbOutput.innerHTML = 'RGB: rgb(' + r + ', ' + g + ', ' + b + ')';
    hexOutput.innerHTML = 'HEX: ' + rgbToHEX(r, g, b).toUpperCase();
}

// Gets colour picker input value
colorInput.addEventListener("input", function() {
    // Sets colours for SVG, converts Hex to RGB and prints out HEX
    svgPath.style.fill = colorInput.value;
    hexOutput.innerHTML = "HEX: " + colorInput.value.toUpperCase();
    rgbOutput.innerHTML = "RGB: rgb(" + hexToRGB(colorInput.value.replace('#', ''))  + ')';
}, false);

Nice code, only a few comments to offer here:

.1.

var rgb = document.getElementById("rgb");
var rgb2 = document.getElementById("rgb2");
var rgb3 = document.getElementById("rgb3");

This way of keeping the r, g, and b is not the best. Consider having to keep six of them, or more. You would not do this that way. You cannot run a loop with this and this gets obvious in

    if (rgb.value.length >= this.maxLength) {
        rgb2.focus();
    }
    if (rgb2.value.length >= this.maxLength) {
        rgb3.focus();
    }

So what I would do is:

var getEl = document.getElementById;
var rgbElements = {
                   r:getEl("rgb"),
                   g:getEl("rgb2"),
                   b:getEl("rgb3")
                  }

This way you could run a loop for these.

.2.
This code

document.getElementById("hex").addEventListener("input", function() {
    // Sets colours for SVG, picker, converts HEX to RGB and prints out HEX
    svgPath.style.fill = '#' + this.value;
    colorInput.value = '#' + this.value;
    hexOutput.innerHTML = 'HEX: #' + this.value.toUpperCase();
    rgbOutput.innerHTML = 'RGB: rgb(' + hexToRGB(this.value) + ')';
}, false);

should be like this one

document.getElementById("hex").addEventListener("input", function() {
    // Sets colours for SVG, picker, converts HEX to RGB and prints out HEX
    var hexVal = this.value;
    svgPath.style.fill = '#' + hexVal;
    colorInput.value = '#' + hexVal;
    hexOutput.innerHTML = 'HEX: #' + hexVal.toUpperCase();
    rgbOutput.innerHTML = 'RGB: rgb(' + hexToRGB(hexVal) + ')';
}, false);

You should not 'ask' for values from 'this' or higher scope when you can have it closer.


Angular, sort objects by their keys I had to sort a list of objects - {name:'name', email:'email} by their keys, ex. by email, so they all be rendered with email key/value pair as a first key/value pair and I was surprised about the solution i came up to - Object.assign() when i loop through array and assign email key/value pair to each object:

objects = [{name: 'name1', phone: 'phone1', email: 'email1'},
            {name: 'name1', phone: 'phone1', email: 'email1'},
            {name: 'name1', phone: 'phone1', email: 'email1'}]

sort(val) {
  for (let i=0; i < this.objects.length; i++) {
    let temp = {};
    temp[val] = null;
    this.objects[i] = Object.assign(temp, this.objects[i])
  }
}

The rest is done by Angular's ChangeDetection. Are there better ways or can you tell any possible issues with this? (I still don't feel very proficient in Javascript, so just wondering)

Here is a StackBlitz's DEMO

Object assign via ... operator
There is a sorter way to do the property order if you use the spread operator to assign properties.
function propOrderFirst(key,objects){
     for(var i = 0; i < objects.length; i++){
          objects[i] = {[key] : null, ...objects[i]};
     }
     return objects;
}

The problem is
const objs = [{name: 'name1', phone: 'phone1', email: 'email1'},
        {name: 'name1', phone: 'phone1', email: 'email2'},
        {name: 'name1', phone: 'phone1', email: 'email3'}];

const obj1 = obj[0];

objs = propOrderFirst("email",objs);

if(obj1 !== objs[0]) { // true as you have created a new instance

Reorder in place
But you do lose the object reference that may not be the best option. You can reorder the object properties and still keep the object reference by using delete to remove properties.
A little more complex as you need to delete all the properties you want to move the new property position in-front of
function propOrderFirst(key, objects){
    const keys = Object.keys(objects[0]);  // assuming all objects have same properties
                                           // If not move this inside loop
    for (const object of objects) {
        const temp = {...object};     // get a copy of object
        for (const k of keys) { delete object[k] };  // remove all properties
        object[key] = temp[key];      // add the key you want first
        Object.assign(object, temp);  // assign the objects properties back
    }
    return objects;
 }

So now
const objs = [{name: 'name1', phone: 'phone1', email: 'email1'},
        {name: 'name1', phone: 'phone1', email: 'email2'},
        {name: 'name1', phone: 'phone1', email: 'email3'}];

const obj1 = obj[0];

objs = propOrderFirst("email",objs);

if(obj1 === objs[0]) { // true as you have reordered in place }


Determine the winner of a coin flip game While I was writing this I had some thoughts and questions going through my head about if I was doing this in a good manner. For instance, is it good to use a StringTokenizer to loop through the words in the line I copied from the Scanner's input?

  Write a method named coinFlip that accepts as its parameter a Scanner for an input file. Assume that the input file data represents results of sets of coin flips that are either heads (H) or tails (T) in either upper or lower case, separated by at least one space. Your method should consider each line to be a separate set of coin flips and should output to the console the number of heads and the percentage of heads in that line, rounded to the nearest tenth. If this percentage is more than 50%, you should print a "You win" message.

public static void coinFlip(Scanner input) {

    int countHeads = 0;
    int countTails = 0;
    double count = 0.0;
    double ratio = 0.0;
    String line = "";

    while(input.hasNextLine()) {
        countHeads = 0;
        countTails = 0;

        line = input.nextLine();
        StringTokenizer lineTokens = new StringTokenizer(line);
        while (lineTokens.hasMoreTokens()) {
            String token = lineTokens.nextToken();
            if (token.toLowerCase().equals("h")) {
                countHeads++;
            } else {
                countTails++;
            }
        }

        count = countTails + countHeads;

        ratio = Math.abs(1 - (countTails/count)) * 100;

        System.out.printf(countHeads + " heads (%1.1f%%)\n", ratio);

        if (countHeads > countTails) {
            System.out.println("You win!\n");
        }
        else {
            System.out.println();
        }
    }
}

The biggest simple improvement you could make would be to declare your variables in a more limited scope.  As you have written it, I cannot easily tell whether the statistics are cumulative, or each line is being treated as an independent problem.

count being a double is weird; it should be an int.

I suggest calling .toLowerCase() once per line instead of once per word.

It's probably worthwhile to do an extra simple check to ensure that every word that is not an "H" is a "T".

The calculation for ratio is more complicated than it needs to be.

You aren't using printf() effectively — the whole point of printf() is to take advantage of the format string rather than concatenation.

public static void coinFlip(Scanner input) {
    while (input.hasNextLine()) {
        int heads = 0, tails = 0;

        StringTokenizer tokens =
            new StringTokenizer(input.nextLine().toLowerCase());
        while (tokens.hasMoreTokens()) {
            String token = tokens.nextToken();
            if ("h".equals(token)) {
                heads++;
            } else if ("t".equals(token)) {
                tails++;
            }
        }

        double ratio = 100.0 * heads / (heads + tails);
        System.out.printf("%d heads (%1.1f%%)\n", heads, ratio);
        System.out.println((heads > tails) ? "You win!\n" : "");
    }
}

One alternative to StringTokenizer would be to use Scanner.findInLine().

public static void coinFlip(Scanner input) {
    while (input.hasNextLine()) {
        int heads = 0, tails = 0;
        String token;
        while (null != (token = input.findInLine("\\b[HhTt]\\b"))) {
            switch (token) {
                case "H": case "h": heads++; break;
                case "T": case "t": tails++; break;
            }
        }

        double ratio = 100.0 * heads / (heads + tails);
        System.out.printf("%d heads (%1.1f%%)\n", heads, ratio);
        System.out.println((heads > tails) ? "You win!\n" : "");
    }
}


Product of all but one number in a sequence - Follow Up This question is a follow up question to the Product of all but one number in a sequence.
I am posting a new code here with taking into consideration the suggestions of [Edward], [CiaPan], [chux], [superb rain] and others. Please advise how the code efficiency can be improved.
#include <stdio.h>
#include <stdlib.h>

//without division, with O(n) time, but extra space complexity as suggested
//return new array on the heap
int *find_product_arr(const int *nums, int arr_size)
{
    int *new_arr = (int *)malloc(sizeof(int)*arr_size);

    int mult_prefix=1; //product of prefix elements
    int mult_suffix=1; //product of suffix elements

    //left most element special handling
    new_arr[0]=1;

    //swipe up
    for(int i=1; i<arr_size; i++) {
        mult_prefix *= nums[i-1];
        new_arr[i] = mult_prefix;
    }

    //swipe down
    for(int j=arr_size-2; j>=0; j--) {
        mult_suffix *= nums[j+1];
        new_arr[j] *= mult_suffix;
    }

    return new_arr;
}

int main(void)
{
    /*Given an array of integers, return a new array such that each element at index i of the
    new array is the product of all the numbers in the original array except the one at i.
    For example, if our input was [1, 2, 3, 4, 5], the expected output would be
    [120, 60, 40, 30, 24] */
    int nums[] = {1, 2, 2, 4, 6};
    int size = sizeof(nums)/sizeof(nums[0]);

    int *products = find_product_arr(nums, size); //get a new array

    for (int i = 0; i < size; i++)
        printf("%d ", *(products+i) );

    free(products); //release heap memory

    return 0;
}

You could eliminate the special case here:

//left most element special handling
new_arr[0]=1;

//swipe up
for(int i=1; i<arr_size; i++) {
    mult_prefix *= nums[i-1];
    new_arr[i] = mult_prefix;
}

by assigning before multiplying, and bringing index 0 into the loop:
//swipe up
for(int i=0; i<arr_size; i++) {
    new_arr[i] = mult_prefix;
    mult_prefix *= nums[i];
}

A similar transformation also applies to the downward sweep (so that each iteration only accesses nums[i], making it easier to reason about).
There is a cost associated with the simplification: an extra multiply, and risk of overflow (technically Undefined behaviour, even though we don't use the final value).


Rewrite of Array.join While rewriting Array so I can practice my javascript, I looked at my implementation of .join(). It seems a little clunky to me.

I've tried to cover all the edges, like making sure separator isn't null before joining, not adding the separator at the end of the string, and writing some documentation.

What I'm looking for is a smoother way to write this function. Any recommendations about this function are appreciated and considered.

/**
 * Converts each element of the array to string then concats them.
 *
 * @param separator Value to separate the elements by.
 *
 * @return String of joined elements.
 */
ArrayRewrite.prototype.join = function(separator) {
    separator = separator || "";
    let string = "";
    for (let i = 0; i < this.content.length; i++) {
        string += (i !== this.content.length - 1)
            ? this.content[i] + separator
            : this.content[i]
        ;
    }
    return string;
}

A test case is below

// This is the main function that other functions are built onto //
function ArrayRewrite(...elements) {
    this.content = elements || [];
}

myArray = new ArrayRewrite("Hello", "Testing", "Testing Again");
console.log(myArray.join(", "))

// Output:
// Hello, Testing, Testing Again

A short review;

You probably want to cache this.content.length
That check for the end of array is clunky
separator will change from 0 to "", an unlikely edge case, but it could happen
Why are array elements in this.content ? I would have gone for this[i] instead
string is not very evocative, I went with out

Obligatory rewrite;

Array.prototype.myJoin = function stringMyJoin(separator) {
    separator = separator || "";
    let out = this[0];
    const length = this.length;
    for (let i = 1; i < length; i++) {
        out += separator + this[i];
    }
    return out || "";
}

console.log([].myJoin(","), [].myJoin(",") === "");
console.log(["apple"].myJoin(","));
console.log(["apple","bear","bananers"].myJoin(","));


Project Euler #2: sum of even Fibonacci numbers, using generators I am currently working on Project Euler problem #2 which requires you to print the sum of all even fibonacci numbers below 4 million.

I just recently learned about generators and from my understanding they are used to decrease memory consumption by generating values on the fly (and not storing them in a huge list).

Does this still work when used in a for loop or will it just generate all values, store them in a list and then loop through them?

for n in some_generator(): pass

In my specific case the code looks like this:

def fibonacci(n): # yields all fibonacci numbers below n
    second_last_n = 0
    last_n = 1
    while (last_n < n):
        current_n = second_last_n + last_n
        second_last_n = last_n
        last_n = current_n
        yield current_n

def filter_even(nums): # yields all numbers in nums (a generator) which are even
    for n in nums:
        if (n % 2 == 0):
            yield n

It's weird that the fibonacci() generates a sequence that starts with 1, 2, 3, 5, 8, ….  Conventionally, the Fibonacci sequence starts with either 0, 1, 1, 2, 3, 5, 8, … or 1, 1, 2, 3, 5, 8, ….

A purer approach would be to use [itertools.takewhile()] in conjunction with an infinite generator.

You should also take advantage of parallel assignment.

The fibonacci() generator would look like this:

def fibonacci():
    a, b = 0, 1
    while True:
        yield a
        a, b = b, a + b

filter_even() is fine.  You could also just write a generator expression instead.

from itertools import takewhile

print(sum(n for n in takewhile(lambda n: n < 4000000, fibonacci()) if n % 2 == 0))


Simple list comprehension As far as I can tell, there is no network access happening at this stage in the code. I am accessing Reddit's API via the PRAW module. However, it is crawling, and I think it should be faster, considering I am hardly doing any work (unless Python heavily penalizes object access?).

This is the entire short script:

import sys, os, pprint, praw

class Scanner(object):
    ''' A scanner object. '''
    def __init__(self):
        self.user_agent = 'debian.blah8899889.agent'
        self.r = praw.Reddit(user_agent=self.user_agent)
        self.nsfw = ('funny', 'nsfw')
        self.nsfw_posters = set()
        self.content = []

    def getSub(self, subreddit):
        ''' Accepts a subreddit. Adds subreddit posts object to self.content'''
        url = 'http://www.reddit.com/r/{sub}/'.format(sub=subreddit)
        print 'Scanning:', subreddit
        subreddit_posts = self.r.get_content(url, limit=5)
        self.addContent(subreddit_posts)

    def addContent(self, subreddit):
        print 'Adding subreddit posts to content.'
        self.content.append(subreddit)

    def addNSFWPoster(self, post):
        print 'Parsing author and adding to posters.'
        self.nsfw_posters.add(str(post.author))

    def scanNSFW(self):
        ''' Scans all NSFW subreddits. Makes list of posters. '''
#       Get content from all nsfw subreddits
        print 'Executing map function.'
        map(self.getSub, self.nsfw)
#       Scan content and get authors
        print 'Executing list comprehension.'
        [self.addNSFWPoster(post) for sub in self.content for post in sub]

def main():
    scan = Scanner()
    scan.scanNSFW()

main()

All of the network access should happen at map(self.getSub, self.nsfw). This actually runs quite fast considering I am rate limited by Reddit's servers.

I cannot work out why the list comprehension is so slow. All it should be doing is iterating through some objects and extracting a simple attribute: it should merely get str(post.author).

 [self.addNSFWPoster(post) for sub in self.content for post in sub]

Can someone help me understand why it seems so slow to execute?

I am new to the OOP approach, and I can only assume I have made a big mistake somewhere.
It is slower than the network access portion of the code!

Well for starters with

[self.addNSFWPoster(post) for sub in self.content for post in sub]

you are constructing a list of

[None, None, ... , None]

with a length equal to

len(self.content) * len(sub)

that you are immediately throwing away because you aren't assigning it to a variable. In other words you should only use a list comprehension for building a list, not for calling a method that is doing work elsewhere.

An alternative implementation is as you might expect a nested for loop:

for sub in self.content:
    for post in sub:
        self.addNSFWPoster(post)

To speed this up further you could reduce your function call overhead with the following:

for sub in self.content:
    for post in sub:
        print 'Parsing author and adding to posters.'
        self.nsfw_posters.add(str(post.author))


Dynamically-sized stack - follow up Follow up of - Which is a better implementation of a stack data structure (based on an array)?

Please review my stack implementation from before.  I've made it dynamically sized if needed and made a few tweaks.

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

typedef struct
{
    int *elementData;
    unsigned int stackSize;
    unsigned int capacityIncrement;
    unsigned int elementCount;
} Stack;

void initializeStack(Stack*, unsigned int, unsigned int);
void stackPush(Stack*, int);
int stackPop(Stack*);
int stackPeek(Stack*);
bool isStackEmpty(const Stack*);
void setCapacityIncrement(Stack*, unsigned int);
int getNumberOfElements(const Stack*);
int getStackSize(const Stack*);

int main()
{
    Stack s1;
    initializeStack(&s1, 4, 10);
    for(int i = 0; i < 25; i++)
    {
        stackPush(&s1, i + 1);
    }
    stackPop(&s1);
    stackPush(&s1, 88);
    stackPush(&s1, 25);
    printf("The top of the stack is %d.\n", stackPeek(&s1));
    while(!isStackEmpty(&s1))
    {
        int top = stackPop(&s1);
        printf("Popping %d from the top of the stack.\n", top);
    }
    return 0;
}

void initializeStack(Stack *p, unsigned int stackSize, unsigned int capacityIncrement)
{
    p->elementData = calloc(stackSize, sizeof(int));
    p->stackSize = stackSize;
    p->capacityIncrement = capacityIncrement;
    p->elementCount = 0;
}

void stackPush(Stack *p, int value)
{
    if (p->elementCount == p->stackSize)
    {
        p->stackSize *= p->capacityIncrement;
        p->elementData = (int*)realloc(p->elementData, sizeof(int) * p->stackSize);
    }
    p->elementData[p->elementCount] = value;
    p->elementCount++;
}

int stackPop(Stack *p)
{
    if (!isStackEmpty(p))
    {
        p->elementCount--;
        return p->elementData[p->elementCount];
    }
    fputs("ERROR : Stack is empty!", stderr);
    return 0;
}

int stackPeek(Stack *p)
{
    if (!isStackEmpty(p))
    {
        p->elementCount--;
        int topValue = p->elementData[p->elementCount];
        p->elementCount++;
        return topValue;
    }
    fputs("ERROR : Stack is empty!", stderr);
    return 0;
}

bool isStackEmpty(const Stack *p)
{
    return p->elementCount == 0;
}

void setCapacityIncrement(Stack *p, unsigned int capacityIncrement)
{
    p->capacityIncrement = capacityIncrement;
}

int getNumberOfElements(const Stack *p)
{
    return p->elementCount;
}

int getStackSize(const Stack *p)
{
    return p->stackSize;
}

First of all, bad naming.

Try prefixing everything with stack,  this makes it a bit more useful as a library where you won't name clash

stack_count
stack_top
stack_push
stack_pop
stack_is_empty

etc

A lot of stack implementations now don't let pop both return a value and change the stack.  It goes against CQS (Command Query Separation). Instead, just use top to get the the top of the stack, and pop simply removes one if one exists.

Your reallocs need to be tested to see if they work, if not, handle the situation nicely rather than the havoc that currently will happen.

Don't use fputs().  A stack should have no dependencies on a "UI"

you should have a stack_destroy  which free's up your stack memory.

Bad things will happen if your capacity increment is 1.

Having a multiplier as a capacity increment will make for a super expensive stack for larger amounts of memory.

Changing your elementcount-- then ++ again in peek is just weird.  Just change all this:

p->elementCount--;
int topValue = p->elementData[p->elementCount];
p->elementCount++;
return topValue;

to

return p->elementData[p->elementCount-1]


Accessing a list of dictionaries in a list of dictionaries I have 4-deep JSON data structure. Basically its a list of dictionaries nested in a list of dictionaries. This is my first time working with this kind of nested data. Below I have done my best to reproduce the structure:

   data = [
    0 : {"name": "hotel1",
         "overall_rating": 4,
         "reviews": [{"rating": 5, "review" : "best place ever"},
                     {"rating": 1, "review" : "toilet was blocked"}],
        }

    1 : {"name": "hotel2",
         "overall_rating": 4,
         "reviews": [{"rating":4, "review" : "welly nice"},
                      {"rating":3, "review" : "I couldn't bring my dog"}],
        }
    ]

What i want to do is write a function that can iterate over all the nested dictionaries and retrieve "rating" and "review". I have written the following program:

reviews = []
ratings = []

for index in range(len(data)):
    for key in data[index]:
        for key in range(len(data[index]['reviews'])):
            reviews.append(data[index]['reviews'][key]['review'])
            ratings.append(data[index]['reviews'][key]['rating'])

However, this loop only retrieves the ratings and reviews of the first nested.
Additionally, it does this len(data) x len(dict) x len(reviews) times. Besides getting help with the loop I it would also be nice with some pointers to libraries that make dealing with this kind of data structure easier (if such a library exists).

Your data is slightly wrong - you're using list [] but then putting in keys (0, 1) - I'm going to assume it's a dict of dicts in my examples. However it wouldn't be that different if it was a list of dicts.

data = {
  0: {"name": "hotel1",
         "overall_rating": 4,
         "reviews": [{"rating": 5, "review" : "best place ever"},
                     {"rating": 1, "review" : "toilet was blocked"}],
        },
  1:  {"name": "hotel2",
         "overall_rating": 4,
         "reviews": [{"rating":4, "review" : "welly nice"},
                      {"rating":3, "review" : "I couldn't bring my dog"}],
        }
    }

Your code makes one of the common mistakes that people new to python make, which is to not realise that you can iterate directly over things like lists and dictionaries - rather than having to get the length and then ask for the index of each item.

You can dramatically simplify your code by just iterating over the values in data directly, and then getting the reviews key of each 'child' dict:

ratings = []
reviews = []
for hotel in data.values():
    for review in hotel['reviews']:
        reviews.append(review['review'])
        ratings.append(review['rating'])

Note that this isn't really what most people would think of as processing a nested dict - since although the data is nested, it is all one level deep - so you can just ask for it at that level.

More complex examples are where you want the same data from multiply nested dictionaries, and have to resort to recursion to get it, e.g. getting all 'name' fields from the following family tree:

tree = {
  'name': 'Alice',
  'relation': 'grandmother',
  'child': {
    'name': 'Bob',
    'relation': 'father',
    'child': {
      'name': 'Charlie',
      'relationship': 'me'
    }
  }
}


Pointers to polymorphic classes saved in vector that reference data in a list as source I have a list called data with strings in it. I have a class String that derives from class Item. A class Scene to which I can add Items that are put in a vector. Scene has a print function to show the current Items.

If I change an element in the data list it also changes what it prints in the Scene.print().

When I remove an item from a Scene with function del it also removes the element in data (list).

Is this a good way to do it am I missing something? The code is actually for a game where I want to add items with coordinates in a scene. Each item has its own function to draw itself etc. and when I remove an item from the scene the data it relates to should be removed too.

#include <iostream>
#include <string>
#include <list>
#include <vector>

using namespace std;

list<string> data;

class Item {
public:
    Item() {
    }

    virtual void print() = 0;
};

class String : public Item {
public:
    String(string &s) {
        _s = &s;
    }

    virtual void print() override {
        cout << *_s << endl;
    }

    string  *_s;

    string ret() {
        return *_s;
    }
};

class Scene {
public:
    void add(shared_ptr<Item> item) {
        _items.push_back(item);
    }

    void del(int n) {
        shared_ptr<Item> pa = _items[n];
        shared_ptr<String>pb = dynamic_pointer_cast<String>(pa);

        if (pb) {
            data.remove(pb->ret());
        }

        _items.erase(_items.begin()+n);
    }

    void print() {
        for(int i = 0; i < _items.size(); i++) {
            _items[i]->print();
        }
    }

    vector<shared_ptr<Item>> _items;
};

void print (list<string> l) {
    for (std::list<string>::iterator it = l.begin(); it != l.end(); ++it) {
        cout << "data: " << *it << endl;
    }
}

int main() {
    Scene s;

    data.push_back("first");
    data.push_back("seconed");
    data.push_back("third");
    data.push_back("fourth");

    std::list<std::string>::iterator it = data.begin();

    for (std::list<string>::iterator it = data.begin(); it != data.end(); ++it) {
        s.add(make_shared<String>(*it));
    }

    s.print();

    *it++;
    *it = "$$$";

    s.print();

    print(data);

    s.del(1);
    s.del(1);

    s.print();
    print(data);

    return 0;
}
```

Avoid using namespace std

While it is nice to save some typing, using namespace std should be avoided. See this post for more details: https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice

Have class Scene take ownership of the list

At the moment, the list data lives outside class Scene. This means other code can modify the list without Scene knowing about it. If you only want to allow items being added via Scene's add() and del() member functions, then make the list a private member variable. But in fact you already have a copy of the list in Scene, named _items. So make that the sole container of strings.

If you really need to have a std::vector<shared_ptr<Item>>, and a std::list<std::string>, then maybe they can be both member variables of class Scene?

Add virtual destructors to polymorphic classes

Whenever you inherit from a base class, you want the right destructor to be called. Therefore, especially if you have any non-trivial member variables, you need to declare a virtual destructor, even if it's empty:

class Item {
    public:
    Item() {}
    virtual ~Item() {}
    ...
};

class String: public Item {
    public:
    String(...) {...}
    virtual ~String() override {}
};

Consider:

Item *item = new String("foo");
delete item;

Without the virtual destructor, the base classes destructor would be called, which doesn't clean up String's member variable _s.

Consider making some member variables private

If a member variable should only ever be modified via a member function, then you should make that variable private. If other code needs to access it anyway, but only for reading, then add a function to get a const pointer or reference to the data. For example, in Scene, you don't want something to remove items directly from _items, because it would bypass the removal of strings from data. So make it private and add const access to it:

class Scene {
    std::vector<std::shared_ptr<Item>> _items;

public:
    ...
    const std::vector<std::shared_ptr<Item>> &get_items() const {
        return _items;
    }
}

Maybe the same can be applied to String as well.

Avoid reinventing the wheel

Perhaps there is a reason for it, but if I just look at the code you posted, I wonder why you create your own class String. If Scene is just a container for strings, then why not have it have std::vector<std::string> _items?

Use auto where appropriate

You can avoid repeating types by using auto in several places. For example, in del() you could write:

auto pa = items[n];
auto pb = dynamic_pointer_cast<String>(pa);

Use range-based for-loops

Whenever you are iterating over the items in a container, use range-based for loops. They are easier to write, and there is less chance of errors. For example:

for (auto item: _items) {
    item->print();
}

Consider using if-statements with initializers

C++17 made if-statements a bit more powerful. For example, especially when doing dynamic-casts, you can write shorter code:

if (auto pb = dynamic_pointer_cast<String>(pa); pb) {
    data.remove(pb->ret());
}

Prevent out-of-bounds access

In Scene::del(), you don't check whether the supplied parameter n is smaller than the size of _items. If this function is called with a too large value, this will result in an out-of-bounds access, which, in the best case, causes a segmentation fault, but in the worst case results in weird behavior that is hard to debug.

If it's something that could happen due to wrong input to the program, you should add a run-time check, and for example throw an exception if n is too large. If the only time n would be too large is due to a programming error somewhere else in the code, then consider adding an assert() statement.

Avoid using std::endl

Use \n instead. See this post for the rationale:
https://stackoverflow.com/questions/213907/c-stdendl-vs-n

Consider overloading operator<<()

Instead of writing a print() function that always prints to std::cout, you could rewrite this to an overload of operator<<(), so that you could print your objects to any output stream. There is a slight problem when having derived classes though, the solution is to modify your print() functions to take an output stream object as a parameter, and then only overload operator<<() in the base class:

class Item {
public:
    ...
    virtual void print(std::ostream &out) const = 0;

    friend std::ostream& operator<<(std::ostream& out, const Item &item) {
        item.print(out);
    }
};

class String: public Item {
public:
    ...
    virtual void print(std::ostream &out = std::cout) const override {
        out << *_s << '\n';
    }
};

Now you can write:

String s;
...
std::cout << s;

I recommend you don't print the newline character in your print() functions though, so code that wants to print items can decide itself whether they should be on separate lines or not.

Make member functions that don't modify anything const

As I already did in the above example, functions that don't change any of the member variables should be made const. This will allow the compiler to perform more optimizations, and will cause it to generate errors when you accidentily do change something in const functions.

Another thing to consider is that if you have a const variable or const reference to a variable, then only member functions of that variable that are marked const can actually be called.

Don't move an iterator past the end

In your main(), you iterator over data. After the for loop, it is equal to data.end(). Then you write:

*it++;

Depending on the implementation of std::list<>, this might cause the iterator to become invalid, and either that statement itself will cause some error, or the subsequent *it = "$$$" will.


Express routing with a login action using SQLite I'm new to Express and SQL, so I don't know the conventional ways of combining the two. Right now I have done it the following way:

app.get('/login', function (req, res) {
    res.render('login');
});

app.get('/home', function (req, res) {
    res.render('home');
});

app.post('/login', function (req, res) {
    db = new sqlite3.Database(file);
    db.serialize(function () {
        [...]
        db.all(query, function (err, rows) {
            if(rows.length == 1) {
                [...]
                res.render('home', {
                    username: rows[0].username
                });
            }
            else {
                res.render('login', {
                    message: "Login not successful!"
                });
            }
        });
    });
    db.close();
});

However, I feel like the routing should be separated from the database stuff. What should I do different? Or is this normal?

For larger projects this can get very complicated and nasty. You can refactor the db access into functions that take a callback as a parameter. Example:

// db_helper.js
module.exports.isUserAuthenticated = function(query, callback) {
  [db code here]
  callback(err, rows);
};

And then in your routes file:

// app.js
var db_helper = require('./path/to/db_helper.js');

....

app.post('/login', function(req, res) {
  var query = [parse from request object]
  db_helper.isUserAuthenticated(query, function(err, rows) {
    if (rows.length == 1) {
      [...]
      res.render(...);
    } else {
      [...]
      res.render(...);
    }
  });
});

Node-style callbacks typically have an error as the first parameter (null if there was no error), and associated data in other parameters.

Nesting callbacks can be a pain if this gets more complicated, so I suggest you look into Promises or async


Better ways of making a scroller http://jsfiddle.net/xkuZF/6/

function func() {
    document.body.scrollTop++;
}

window.onmouseover = function() {
    clearInterval(interval);
};

window.onmouseout = function() {
    interval = setInterval(func);
};

var interval =  setInterval(func);

Do you think that there are any better ways of doing this code?

Better:

More options
Less code

To your questions:

More options; sure you could provide a wait time to setInterval to control the speed of the scrolling down, you could also increase the scrollTop increment to make it scroll down faster.
Less code; I think this is pretty much the bare minimum, a little too bare really.

I think func and interval are very generic names, I wasnt even sure if the code works / what it does until I clicked the fiddle link.

I would counter propose the following code, it is longer, but far more readable.

var scrollDownInterval;

function scrollDown() {
  document.body.scrollTop++;
}

function startScrollingDown(){
  scrollDownInterval = setInterval( scrollDown );
}

function stopScrollingDown(){
  clearInterval( scrollDownInterval );
}

window.onmouseover = function() {
  stopScrollingDown();
};

window.onmouseout = function() {
  startScrollingDown();
};

//Start scrolling down immediately
startScrollingDown();


Switch statement for adding/removing operands to the text part of a label I have a switch statement with five cases, and four of them are really similar to the others. How could I properly refactor this code to eliminate the duplication of code?

func appendOperand(operand: Operand) {
    let displayedString = amountLabel.text!
    if displayedString.last != decimalSeparator {
        switch operand {
        case Operand.Div:
            if division {
                dropLastChar()
            } else {
                if multiplication || subtraction || addition {
                    dropLastChar()
                }
                appendOperand(buttonDivision, operand: &division, operandName: "/")
            }
        case Operand.Mult:
            if multiplication {
                dropLastChar()
            } else {
                if division || subtraction || addition {
                    dropLastChar()
                }
                appendOperand(buttonMultiplication, operand: &multiplication, operandName: "x")
            }
        case Operand.Sub:
            if subtraction {
                dropLastChar()
            } else {
                if division || multiplication || addition {
                    dropLastChar()
                }
                appendOperand(buttonSubtraction, operand: &subtraction, operandName: "-")
            }
        case Operand.Add:
            if addition {
                dropLastChar()
            } else {
                if division || multiplication || subtraction {
                    dropLastChar()
                }
                appendOperand(buttonAddition, operand: &addition, operandName: "+")
            }
        default:
            break
        }
    }
}

func appendOperand(button: UIButton, inout operand: Bool, operandName: String) {
    setAllOperatorsToFalse()
    operand = true
    wasDecimalAmount = isDecimalAmount
    isDecimalAmount = false
    setDisplayWith(operandName)
}

func dropLastChar() {
    isDecimalAmount = wasDecimalAmount
    setAllOperatorsToFalse()
    let displayedString = dropLast(amountLabel.text!)
    amountLabel.text! = displayedString
    calculatorMode = displayedString.rangeOfString("[x/+-]", options: .RegularExpressionSearch) != nil
}

func setAllOperatorsToFalse() {
    division = false
    multiplication = false
    subtraction = false
    addition = false
    equal = false
}

enum Operand {
    case Div, Mult, Sub, Add, Equal
}

I thought at first making a method that would do everything that is in each case but then I would end up with a lot of arguments for the method.

Another problem I'm facing is the multiplication || subtraction || addition part. If addition == true, multiplication, subtraction and addition are false for example. I am sure there is a more elegant way to do this.

EDIT:

So here is my calculator:

Every time one of the operand (x, +, / or -) is pressed, appendOperand(operand: Operand) is called:

@IBAction func buttonPressed(sender: UIButton) {
    switch sender.tag {
        // operands
    case Tag.Add.rawValue:
        appendOperand(Operand.Add)
    case Tag.Sub.rawValue:
        appendOperand(Operand.Sub)
    case Tag.Mult.rawValue:
        appendOperand(Operand.Mult)
    case Tag.Div.rawValue:
        appendOperand(Operand.Div)
    case Tag.Equal.rawValue:
        appendOperand(Operand.Equal)

    default:
        break
}

It adds the operand at the end of the string displayed in the label ("96x" in my example on the screenshot).

I use these flags for each operator to manage if whether or not the operand button has been pressed before. For example, if I press x, then x will appear on the label. If / was pressed right before, / will be removed from the label before adding x. Or if I pressed x and press it again, x will be removed from the label as if I never pressed it.

These operator are declared on top of the UIViewController:

var addition: Bool = false
var subtraction: Bool = false
var multiplication: Bool = false
var division: Bool = false
var equal: Bool = false

So, as a start, I'd take my enum, which you've stuck at the bottom, and move it to the top.  It's great to use custom types, but sticking it at the bottom of the file does a disservice to readers.  And the fact that it has been placed at the bottom makes me think that the author may have undervalued this custom type (and judging by the code, I'd say the enum certainly appears undervalued).

So:

Move the enum to the top so that future readers can start there.

Next, what are we gaining by abbreviating the operand types?  Not a whole lot.  We only have to fully type it out once, and the rest of the time it will autocomplete for us, right?  Right.  By fully typing out the name the first time, we're going to increase the readability throughout everywhere that uses the enum.  And writing readable code is important.

So:

Move the enum to the top so that future readers can start there.
Don't abbreviate.

Finally, look at what we're doing.  We call our appendOperand and pass in one of our Operand enum values, and that does some work to eventually call an overloaded appendOperand method that takes a string.  A string which we're magically associating with the operand, and only doing so in this very weird way.  But why?  Swift enums can have a backing value associated with them (and there's no performance hit either for storing them or for passing them around... they still perform the same).  We can access that backing value using the rawValue property of enums.

So:

Move the enum to the top so that future readers can start there.
Don't abbreviate.
Use a backing value rather than magically associated strings.

Applying these three points gives us the following enum at the top of the file:

enum Operand: String {
    case Multiply = "x"
    case Divide = "/"
    case Add = "+"
    case Subtract = "-"
    case Evaluate = "="
}

Now, we take a close inspection of the appendOperand method, and I notice a few oddities...

We take a UIButton argument, but we don't use it.
We take an inout argument and return void (hint: don't take the argument and just return a value).
Now that we've fixed our enum, we can take the enum as an argument rather than a String.

There are more issues that can and should be addressed, but getting the enum right is so fundamental to this code that we need to take a breather after fixing that up, see what we can do after fixing that up, and reassess where we're at before we start making more changes.


Finding alternating sequence in a list of numbers Please be brutal and treat this as me coding this up for an interview.

A sequence of numbers is called a zig-zag sequence if the differences between successive numbers strictly alternate between positive and negative. The first difference (if one exists) may be either positive or negative. A sequence with fewer than two elements is trivially a zig-zag sequence.

For example, 1,7,4,9,2,5 is a zig-zag sequence because the differences (6,-3,5,-7,3) are alternately positive and negative. In contrast, 1,4,7,2,5 and 1,7,4,5,5 are not zig-zag sequences, the first because its first two differences are positive and the second because its last difference is zero.

Given a sequence of integers, sequence, return the length of the longest subsequence of sequence that is a zig-zag sequence. A subsequence is obtained by deleting some number of elements (possibly zero) from the original sequence, leaving the remaining elements in their original order.

More examples:

{ 1, 7, 4, 9, 2, 5 }

Returns: 6

The entire sequence is a zig-zag sequence.
{ 1, 17, 5, 10, 13, 15, 10, 5, 16, 8 }

Returns: 7

There are several subsequences that achieve this length. One is 1,17,10,13,10,16,8.
{ 44 }

Returns: 1
{ 1, 2, 3, 4, 5, 6, 7, 8, 9 }

Returns: 2
{ 70, 55, 13, 2, 99, 2, 80, 80, 80, 80, 100, 19, 7, 5, 5, 5, 1000, 32, 32 }

Returns: 8
{ 374, 40, 854, 203, 203, 156, 362, 279, 812, 955, 600, 947, 978, 46, 100, 953, 670, 862, 568, 188, 67, 669, 810, 704, 52, 861, 49, 640, 370, 908, 477, 245, 413, 109, 659, 401, 483, 308, 609, 120, 249, 22, 176, 279, 23, 22, 617, 462, 459, 244 }

Returns: 36

Worst case: \$O(n^2)\$
Space Complexity: \$O(n)\$

private static int longestAlternatingSequence(int[] values){
        if(values.length == 1){
            return 1;
        }
        int[] difference = new int[values.length-1];
        for(int i = 1; i < values.length; i++){
            difference[i-1] = values[i] - values[i-1];
        }
        int[] calculationsCache = new int[difference.length];
        calculationsCache[0] = 1;
        int max = Integer.MIN_VALUE;
        for(int i = 1; i < difference.length; i++){
            if(difference[i] > 0){
                for(int j = 0; j < i; j++){
                    if(difference[j] < 0){
                        max = Math.max(max, calculationsCache[j]);
                    }
                }
            }else if(difference[i] < 0){
                    for(int j = 0; j < i; j++){
                        if(difference[j] > 0){
                            max = Math.max(max, calculationsCache[j]);
                        }
                    }
             }else{
                   max = 0;
            }
            calculationsCache[i] = max + 1;
         }
        max = Integer.MIN_VALUE;

        for(int value : calculationsCache){
            max = Math.max(max, value);
        }
        return max + 1;
     }

Basically, you approach solves the problem adequately, and I have nothing to say about the existing code in terms of style.

However, I think the general approach might be too costly, in terms of time and space requirements (even though I admit that there is no spec).
But since we should treat it as an interview question, then a potential interviewer could ask you
how to approach the problem more efficiently: for example, how could you process the array in one pass only?

So here is a possible answer.

Algorithm's complexity

As you said, your current implementation requires:

linear storage requirement
quadratic execution time

I have an alternative approach, using constant memory and linear time, which is based on an incremental algorithm: for each element of the array, you can determine the longuest zig-zag subsequence that can be built so-far.

I prototyped the solution using Common Lisp, so here is my version (this could be translated easily as a simple for loop in Java, but this is left as an exercise ;-))

(defun max-zig-zag (sequence)
  (loop
     for last = nil then current
     for current in sequence
     for sign   = 0 then (signum (- current last))
     for match  = t then (or (eql await 0) (eql await sign))
     for await  = 0 then (if match (- sign) await)
     count match))

This approach makes all your test examples pass:

(loop for (expected sequence) in
     '((6 (1 7 4 9 2 5))
       (7 (1 17 5 10 13 15 10 5 16 8))
       (1 (44))
       (2 (1 2 3 4 5 6 7 8 9))
       (8 (70 55 13 2 99 2 80 80 80 80 100 19 7 5 5 5 1000 32 32))
       (36 (374 40 854 203 203 156 362 279 812 955 600 947 978 46 100 953 670 862 568 188 67 669 810 704 52 861 49 640 370 908 477 245 413 109 659 401 483 308 609 120 249 22 176 279 23 22 617 462 459 244)))
     always (eql expected (max-zig-zag sequence)))

=> T

I acknowledge that this might not be easy to follow through, so I'll try to write a step-by-step explanation.

Main idea

The difficulty in your question is that you allow some elements to be deleted (or, ignored). We are looking for sequences with maximum length, however.
The elements that can be ignored are those that do not alternate from previous ones: when you encounter a 2, and then the number 4, you can discard all the following elements that are greater than, or equal to, 4, without reducing the potential length of the subsequence: all numbers above 3 are just intermediate values.

The suggested approach is a greedy count of all the alternations of slopes in the array (*).
It is built around a fixed number of variables that are updated at each element, during a unique iteration of the array; in fact, the input array could be an infinite stream of inputs, whereas the count value could be an infinite stream of integers that is updated for each input (and growing when needed).

Derivative

So, the main idea is to look at the shape, or the derivative of your numbers: you take the current value, and the last value, when iterating over the array, and you compute the difference: that gives you the slope.

However, what is interesting is only the sign of this slope: the sign variable always contains the sign of the difference between current and last element, so that a 1 means that current value is higher, and -1 mean that is is lower than the last one.

Matching zig-zags

Initially, you await for any of those sign, either -1 or 1 (which is encoded by zero in the await variable).
This first time that sign differs from zero you have a match: you encountered a positive or negative variation of value; so, the new value of await is now the opposite of the current sign: if we saw a positive slope, like 1, we must read until there is a negative one.

The match variable is initially true, and is otherwise true only when the sign variable equals the await variable (or, if await is zero, during the initialization phase).

When match is false, we skip entries and let await keep its previous value.
When true, await takes the opposite of current sign, which means that we await for a change in the slope.

Then, match is true whenever we go up after having gone down, and respectively down after having gone up, discarding all continuous sequences of decreasing (or increasing) elements.

Finally, we count the number of matches to have the longuest subsequence.

(*) It is based on a data-flow approach (see Lustre, Signal, ...). In fact, the Common Lisp code above could probably easily be rewritten using the SERIES package.


Retrying a request using 'request' library without extra modules I am using the request module and wanted to implement some retries for robustness. It seems excessive to use a whole new or extra module just to do a retries so I've put together the following as an exercise to retry requests using only the request library:

var checkUrl = function(url = 'http://localhost', retriesLeft = 0, timeoutMS = 3000) {
  var request = require('request');
  request(url, async function (error, response, body) {
    if (error) {
      if (retriesLeft > 0) {
        console.log(url + ' : not available yet');
        await sleep(timeoutMS);
        checkUrl(url, --retriesLeft);
      }
    } else {
      console.log(url + ' : URL is available');
    }
  });

}

function sleep(ms) {
  return new Promise(resolve => setTimeout(resolve, ms));
}

checkUrl('http://www.example.com/', 3, 5000);

Is this a reasonable approach? It seems to work. It's just a basic example - I've not considered redirects for instance.

Of course this can be wrapped up to make it easier to use and then, ironically, becomes a sort of module but it would not require pulling anything external.

It's not "excessive" (your term) to use a pre-made module that does what you want. In fact, if it's solid code, then it's smart to use something that's already written and tested and maintained by someone else. There's little cost on a server to using a small module that does what you want.

I didn't do an exhaustive search, but I came across requestretry on NPM that seems to be pretty much what you might want (there are several other similar options on NPM too).  Using it, reduces your code to this:

const requestRetry = require('requestretry');

function checkUrl(url = 'http://localhost', retriesLeft = 0, timeoutMS = 3000) {
    return requestRetry({url: url, maxAttempts: retriesLeft + 1, retryDelay: timeoutMS});
}

checkUrl('http://www.example.com/', 3, 5000).then(data => {
    console.log(data);
}).catch(err => {
    console.log(err);
});

This particular library even has ways for you to plug in your own retry strategy or delay strategy (if, for example, you wanted to implement a back-off strategy to the retries so the retry interval gets longer after more failures which is sometimes desirable).


C++ instance counter I have created template class to easily get the current number of instances a class has. It's threadsafe and I tried to make it as hard as possible to abuse.

#include <atomic>
#include <cstdint>
#include <type_traits>

template<typename t_derived>
struct countable
{
    friend t_derived;

    struct counter
    {
        friend countable<t_derived>;

    private:
        static std::atomic<uint32_t> s_counter;
    };

    static uint32_t instances()
    {
        static_assert(std::is_base_of<countable<t_derived>, t_derived>::value,
                      "Trying to get count of non counted class");
        return counter::s_counter;
    }

private:
    countable()
    {
        ++counter::s_counter;
    }

    countable(const countable&)
    {
        ++counter::s_counter;
    }

    ~countable()
    {
        --counter::s_counter;
    }
};

template<typename t_derived>
std::atomic<uint32_t> countable<t_derived>::counter::s_counter(0);

Edit:

There seem to be some misunderstandings about this class. I'm sorry, I should've explained it a bit. Here is an example main.cpp. It requires at least C++11:

#include <iostream>

// paste implementation here

struct counted1 : countable<counted1>
{
    counted1()
    {
        std::cout << "counted1::counted1()\n";
    }
    ~counted1()
    {
        std::cout << "counted1::~counted1()\n";
    }
};

struct counted2 : countable<counted2>
{
    counted2()
    {
        std::cout << "counted2::counted2()\n";
    }
    ~counted2()
    {
        std::cout << "counted2::~counted2()\n";
    }
};

int main()
{
    std::cout << "counted1 [" << counted1::instances() << "]\n";
    std::cout << "counted2 [" << counted2::instances() << "]\n";

    counted1 s1;

    std::cout << "counted1 [" << counted1::instances() << "]\n";
    std::cout << "counted2 [" << counted2::instances() << "]\n";

    {
        counted1 s2;
        counted2 s3;

        std::cout << "counted1 [" << counted1::instances() << "]\n";
        std::cout << "counted2 [" << counted2::instances() << "]\n";
    }
    std::cout << "counted1 [" << counted1::instances() << "]\n";
    std::cout << "counted2 [" << counted2::instances() << "]\n";

    return 0;
}

Why did I make the ctors private & not protected? Simply if they were protected every class could inherit from it. Making them private & declaring the template parameter a friend enforces the correct use of the crtp pattern:

struct my_counted_struct : countable<some_other_struct>{};   // error!

This has the side effect that all actual private data (s_counter) is also exposed to the derived class, so this would be possible:

struct my_struct : countable<my_struct>
{
    void do_something_nasty()
    {
        countable<my_struct>::s_counter = 0;
    }
};

That's why the actual private data is in the inner class counter which declares countable a friend, giving it access to its private data.

Nice use of the "Curiously Recurring Template Pattern".

The trouble with your design, is that any user of your class needs to declare their own version of the s_counter. Not strictly a problem but it does make it harder to use.

I would use static function variable as the counter. That way you don't actually need to declare your own version.

    struct counter
    {
        private:
            // Should not be used by friends.
            static std::atomic<uint32_t>& getCounterInstance() {
                static std::atomic<uint32_t>  counter = 0;
                return counter;
            }
    };

As counter is a private member of countable. Nobody else can see it (let alone access it). So you don't need to make countable a friend. Just use some standard public functions to get you accesses.

template<typename t_derived>
class countable
{
    struct counter
    {
        private:
            // Should not be used by friends.
            static std::atomic<uint32_t>& getCounterInstance() {
                static std::atomic<uint32_t>  counter = 0;
                return counter;
            }

       public:
            // Handy access methods:
            static void incrementCounter() {
                ++getCounterInstance();
            }
            static void decrementCounter() {
                --getCounterInstance();
            }
    };
};

The only person that has access to incrementCounter() and decrementCounter() are members of countable.

You got the default constructor and the copy constructor.

    countable()
    {
        ++counter::s_counter;
    }

    countable(const countable&)
    {
        ++counter::s_counter;
    }

But you forgot the move constructor.

    countable(countable&&)
    {
        counter::incrementCounter();
    }


Converting JSON to CSV using powershell I have a very large JSON Lines File with 4.000.000 Rows, and I need to convert several events from every row. The resulted CSV File contains 15.000.000 rows. How can I optimize this script?

I'm using Powershell core 7 and it takes around 50 hours to complete the conversion.

My Powershell script:

$stopwatch =  [system.diagnostics.stopwatch]::StartNew()
$totalrows = 4000000

$encoding = [System.Text.Encoding]::UTF8
$i = 0
$ig = 0
$output = @()

$Importfile = "C:\file.jsonl"
$Exportfile = "C:\file.csv"

if (test-path $Exportfile) {
    Remove-Item -path $Exportfile
}

foreach ($line in [System.IO.File]::ReadLines($Importfile, $encoding)) {
    $json = $line | ConvertFrom-Json

    foreach ($item in $json.events.items) {
    $CSVLine = [pscustomobject]@{
    Key = $json.Register.Key
    CompanyID = $json.id
    Eventtype = $item.type
    Eventdate = $item.date
    Eventdescription = $item.description
    }
    $output += $CSVLine
    }

    $i++
    $ig++
    if ($i -ge 30000) {
        $output | Export-Csv -Path $Exportfile -NoTypeInformation -Delimiter ";" -Encoding UTF8 -Append
        $i = 0
        $output = @()

        $minutes = $stopwatch.elapsed.TotalMinutes
        $percentage = $ig / $totalrows * 100
        $totalestimatedtime = $minutes * (100/$percentage)
        $timeremaining = $totalestimatedtime - $minutes

        Write-Host "Events: Total minutes passed: $minutes. Total minutes remaining: $timeremaining. Percentage: $percentage"
    }
}

$output | Export-Csv -Path $Exportfile -NoTypeInformation -Delimiter ";" -Encoding UTF8 -Append

$stopwatch.Stop()

There's a good article about performance to get your started: Slow Code: Top 5 ways to make your Powershell scripts run faster; at first sight, here are applicable both parts of Problem #2 & #3: Appending stuff:

Appending to files: Export-CSV -Append repeats roughly 133 times (= 4E+6/30E+3 i.e. 4E+6 rows written in chunks of 30E+3 rows each) which could considerably worsen performance for big output file;
Appending to arrays: (heart of the matter) when the += operator is used, it's actually destroying the array and creating a new one (source).

To eliminate appending to arrays, you can use .NET array list object as follows (merely a hint): replace

$output = @() with $output = [System.Collections.ArrayList]::new(), and
$output += $CSVLine with [void]$output.Add($CSVLine).

Note: do not compute one-purpose variable $CSVLine at all; instead, use

[void]$output.Add(
    [pscustomobject]@{
        Key = $json.Register.Key
        CompanyID = $json.id
        Eventtype = $item.type
        Eventdate = $item.date
        Eventdescription = $item.description
    })

Read the remark about the generic List<T> class and Performance Considerations as well:

  In deciding whether to use the List<T> or ArrayList class, both of
  which have similar functionality, remember that the List<T> class
  performs better in most cases and is type safe. If a reference type
  is used for type T of the List<T> class, the behavior of the two
  classes is identical.


What would be the idiomatic F# way to write optional parameters? In the context of a library to generate random credit card, it would be nice to let the user the possibility to set some optional options on some cards. Lets take Visa. Two lengths are available: 13 or 16 digits.
In C# a common implementation would be:
enum VisaLengthOptions
{
    Thirteen,
    Sixteen
}

public static string GenerateVisa(VisaLengthOptions length = VisaLengthOptions.Sixteen) {
    if (length == VisaLengthOptions.Thirteen) return "4000000000001";
    if (length == VisaLengthOptions.Sixteen) return "4000000000000001";
    return "";
}

Since F# does not support optional parameter (for currying and simplicity reason (note that it could be done since ocaml did it)). What would be the most idiomatic way to write this in F#?
Here is a guess:
type VisaLengthOptions =
    | Thirteen = 0
    | Sixteen = 1

let constructVisa length =
    match length with
    | VisaLengthOptions.Thirteen -> "4000000000001"
    | VisaLengthOptions.Sixteen -> "4000000000000001"
    | _ -> ""

let generateVisa () = constructVisa VisaLengthOptions.Sixteen

So there are no optional parameter and have two functions cant have the same name even with different signatures. What wrong with this snippet?
We want our library to have a Truly Discoverable API. So we would like to avoid having two different name for basically the same logic. Should we follow the map and map2 and goes generateVisa and generateVisa2? or like map and mapi and goes generateVisa and generateVisaL? Is there a one true F# way?
A good example in the .net world is Split.

I would argue that if you need optional parameters your interface is different hence you need another function.
With that said, if you really want optional parameters you must always define a default value, an you could then use partial application for that.
let f op1 op2 nonOpt = op1 + op2 + nonOpt

let fWithDefaults = f 1 2

Then you can use your fWithDefaults whenever you want the defaults.
For your particular example I would write it like this :
type VisaLengths =
     | Thirteen
     | Sixteen

let generateVisa (length : VisaLengths option) =
     let length = Option.defaultValue Sixteen length
     match length with
     | Thirteen -> "4000000000001"
     | Sixteen -> "4000000000000001"

It would be the most "idiomatic", even if it can be a litter cumbersome but it does what you want to do.
Agree, you still have to passe None... But at least your intention about a optional value is explicit, which is very valuable for others reading your code.


A conditional active record query I have a conditional active record query that checks to see if a relationship of type X exists (.count > 0) where attribute x and attribute y match specific input or if they match when attribute x and attribute y are switched.

I currently use an || operator with two long active record queries to cover both situations.

def coach_of?(user)
  if UserRelation.where(relation_type: 'coach-student').where(user_id: self.id).where(with_user_id: user.id).count > 0 ||
     UserRelation.where(relation_type: 'coach-student').where(user_id: user.id).where(with_user_id: self.id).count > 0
    true
  else
    false
  end
end

Is there a more clever way to do this or is this kind of query acceptable?

Some notes:

if boolean then true else false end -> boolean.
UserRelation.where(user_id: self.id).where(with_user_id: user.id) -> UserRelation.where(user_id: self.id, with_user_id: user.id)
active_record_collection.count > 0 -> active_record_collection.exists?
I don't understand why you have to check both ways, a "coach - student" relationship looks unidirectional.

Anyway, if you set up the coach_students association, you may write something like this:

def coach_of?(user)
  coach_students.where(with_user_id: user).exists? ||
    user.coach_students.where(with_user_id: self).exists?
end


Review the code to copy List of custom class to another list of Custom class Thanks for the Reply.

Here is the actual Scenario
My application received the data from a backend in the form of a class object ie.

public class FlowObject {
private String id;
private List<FlowStep> flowlSteps;
private List<String> audRefs;

}

Now FlowStep class as mentioned above is

public class FlowStep {

private String id;
public String getId() {
return id;
}
public void setId(String id) {
this.id = id;
}

}

Now since Backend code is NOT ready Im writing the dummy implementation of backend FlowObject Class at my Side whose data gets copied to my local Class FlowCopy
Thats why I need to write both FlowObject and FlowCopy Classes

Now My Backend Dummpy Implemetation of FlowObject is as below

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
public class FlowObject {
    private String id;
    private List<FlowStep> flowlSteps;
    private List<String> audRefs;
    public String getId() {
            return id;
        }
        public void setId(String id) {
            this.id = id;
        }
        public List<FlowStep> getFlowSteps() {
            return flowlSteps;
        }
        public void setFlowSteps(List<FlowStep> flowlSteps) {
            this.flowlSteps= flowlSteps;
        }
        public List<String> getAudRefs() {
            return audRefs;
        }
        public void setAudRefs(List<String> audRefs) {
            this.audRefs = audRefs;
        }
}

And public class FlowStep {

private String id;

 public String getId() {
    return id;
}
public void setId(String id) {
    this.id = id;
}

}

and my local FlowCopy Class is as below

public class FlowCopy {
    private String id;
    private List<FlowStep> flowSteps;
    private List<String> audRefs;

    public String getId() {
        return id;
    }
    public void setId(String id) {
        this.id = id;
    }
    public List<FlowStep> getFlowSteps() {
        return flowSteps;
    }
    public void setFlowlSteps(List<FlowStep> flowSteps) {
        this.flowSteps= flowSteps;
    }
    public List<String> getAudRefs() {
        return audRefs;
    }
    public void setAudRefs(List<String> audRefs) {
        this.audRefs = audRefs;
    }
}

Now my intention is to Copy the Custom FlowObject Class to a list of FlowCopy which will be used in application

public List<FlowCopy> getFlow(  List<FlowObject > obj  ){
       List<FlowCopy> flowRes = new ArrayList<FlowCopy>(obj.size());
       for (FlowObject object:obj)
       {
           FlowCopy  callFlow = new FlowCopy();
           callFlow.setId(object.getId());
           callFlow.setAudioRefs(object.getAudRefs());
           callFlow.setCallSteps(object.getFlowSteps());
           flowRes .add(callFlow);

       }
     return flowRes ;
   }

Please review the above implemetation and suggest a suitable approach

One thing I guess you can consider is to add a constructor for class FlowCopy that handles (or encapsulates) the copy task to release this burden from function getFlow():

public class FlowCopy {
    public FlowCopy(FlowObject obj) {
        setId(obj.getId());
        setAudioRefs(obj.getAudRefs());
        setCallSteps(obj.getFlowSteps());
    }
   ...
}


Loading data async with queue. Proper write only locking and scheduling I have to store and return data on multitasking requests. If data is missing, I schedule loading and return null. I'm using some nubie scheduling by saving current jobs in a list. Also I need data request to be locked during writing. .NET 4.0

How can I schedule and lock properly?

private ConcurrentDictionary<string, IDataSet> data;
private List<string> loadingNow;

public IDataSet GetData(string dataId)
{
    if (loadingNow.Contains(dataId))
        return null; // Currently loading. Return null

    if (data.ContainsKey(dataId))
        return data[dataId]; // Return data

    // Schedule loading async. Return null.
    loadingNow.Add(dataId);
    dataIoAsync.LoadDataAsync(dataParams);
    return null;
}

private void DataIoAsync_DataFileLoaded(object sender, DataFileLoadedAsyncEventArgs e)
{
    loadingNow.Remove(e.DataId);
    data.TryAdd(e.DataId, e.DataSet);
    OnDataFileLoaded(e.DataId);
}

EDIT: Dictionary replaced with ConcurrentDictionary

Instead of keeping track of what's being loaded, you can represent that as a state by wrapping IDataSet into a class:

private sealed class DataSetResult
{
    public volatile IDataSet Result;
}

When an instance of this class is created, start loading the data. You can then just return the value of the Result field - if it is null, the data is still being loaded:

private readonly ConcurrentDictionary<string, Lazy<DataSetResult>> _data;

public IDataSet GetData(string dataId)
{
    var dataSet = _data.GetOrAdd(
        dataId,
        _ => new Lazy<DataSetResult>(
            () =>
            {
                var result = new DataSetResult();
                dataIoAsync.LoadDataAsync(dataParams);
                return result;
            }
        )
    );

    return dataSet.Value.Result;
}

private void DataIoAsync_DataFileLoaded(object sender, DataFileLoadedAsyncEventArgs e)
{
    _data[e.DataId].Value.Result = e.DataSet;
    OnDataFileLoaded(e.DataId);
}

The Lazy<DataSetResult> above ensures that data is retrieved at most once for a given id.

Personally, however, I would prefer to return Task<IDataSet> from this method, as it is really up to the caller to decide whether they want to wait for the result to become available or not. Using Task also allows you to report any exceptions to the client:

private readonly ConcurrentDictionary<string, Lazy<TaskCompletionSource<IDataSet>>> _data;

public Task<IDataSet> GetDataAsync(string dataId)
{
    var dataSet = _data.GetOrAdd(
        dataId,
        _ => new Lazy<TaskCompletionSource<IDataSet>>(
            () =>
            {
                var result = new TaskCompletionSource<IDataSet>();
                dataIoAsync.LoadDataAsync(dataParams);
                return result;
            }
        )
    );

    return dataSet.Value.Task;
}

private void DataIoAsync_DataFileLoaded(object sender, DataFileLoadedAsyncEventArgs e)
{
    _data[e.DataId].Value.TrySetResult(e.DataSet);
    OnDataFileLoaded(e.DataId);
}


Showing that an integer function's value and the EOF value are not the same I was doing Exercise 1.6 and 1.7 in 'The C Programming Language' by K&R.

The questions state: "Verify that the experssion getchar() != EOF is 0 or 1" (Exercise 1.6) and "Write a program to print the value of EOF" (Exercise 1.7)

Here is my attempt at trying to do both in one program:

#include <stdio.h>

int main()
{
    printf("%d \t This is the value of EOF \n", EOF);
    int c = getchar() != EOF;
    printf("%d \t This is the value of getchar(), our variable \n", c);
}

I was wondering if there is a more concise or preferred way to do this?

The original code from this question improperly assumes that c still has the numeric value of a character, saying "This is the value of getchar()".  This is not actually true based upon that code.  The value being printed out in the last line is not the value of getchar() but the value of the comparison of getchar against != EOF.

To clarify things, I wrote the below code, with comments.

#include <stdio.h>

int main( void )
{
  /* result variables, with initial value for cleanliness */
  int getchar_result = 0;
  int compare_result = 0;

  /* print out the value of EOF as an integer */
  printf( "%d \t This is the value of EOF \n", EOF );

  /* obtaining the result of getchar */
  getchar_result = getchar();

  /* obtaining the result of comparison */
  compare_result = EOF != getchar_result;

  /* print out the actual value of the getchar_result, which may or may not be EOF */
  printf( "%d \t This is the value of getchar()\n", getchar_result );

  /* print out the value of the comparison result against EOF */
  printf( "%d \t This Is the result of (getchar()!=EOF)\n", compare_result);
}

This produced the results, when ctrl+D was entered during the execution the first round, and 1 was entered during the second.  Notice that the comparison value is zero (not true) in the first, and 1 (true) in the second.

UbuntuPrompt>./a.out
-1       This is the value of EOF
-1       This is the value of getchar()
0        This Is the result of (getchar()!=EOF)
UbuntuPrompt>./a.out
-1       This is the value of EOF
1
49       This is the value of getchar()
1        This Is the result of (getchar()!=EOF)

Hope this helps.


Showing values from different fields in an array with ternary This small piece of code shows the value of different fields of an array.
I'm looking for a way to improve this nested ternary function. An if statement solution would be obvious, so I'm wondering if there is something more elegant to solve it.
<button type="button" onclick="myFunction()">Check</button>

Output
Abba: 1 Mars: 4 Nexus: 6

I'm looking for a way to improve this nested ternary function

They keys to check could be iterated over - e.g.

function getArray(value) {
  var showValue = 0;
  for (let key of ['value1', 'value2', 'value3']) {
    if (value[key] > 0) {
      showValue = value[key];
      break;
    }
  }

  document.getElementById("demo").innerHTML += value.system + ": " + showValue + " ";
}

function myFunction() {
  var array = [
    {system:"Abba", value1:0, value2:1, value3:0},
    {system:"Mars", value1:0, value2:4, value3:0},
    {system:"Nexus", value1:0, value2:0, value3:6},
    {system:"Zeroes", value1:0, value2:0, value3:0}
  ];

  array.forEach(getArray);
}
<button type="button" onclick="myFunction()">Check</button>

And this could be simplified using the array method find() combined with logical OR - i.e. || for a fall-back value using Short-circuit evaluation for the case when no values are greater than zero:

function getArray(value){
  //find the key that has a value greater than zero
  const key = ['value1', 'value2', 'value3'].find(key => value[key] > 0);
  //return the value that is greater than zero, or else zero in case no value is greater than zero
  const showValue = value[key] || 0;
  document.getElementById("demo").innerHTML += value.system + ": "+ showValue + " ";
}

function myFunction() {
  var array = [
    {system:"Abba", value1:0, value2:1, value3:0},
    {system:"Mars", value1:0, value2:4, value3:0},
    {system:"Nexus", value1:0, value2:0, value3:6},
    {system:"Zeroes", value1:0, value2:0, value3:0}
  ];

  array.forEach(getArray);
}
<button type="button" onclick="myFunction()">Check</button>

Other review points
Improper use of map method
The map() method "creates a new array populated with the results of calling a provided function on every element in the calling array"1. For this code forEach() can be used (as it is in the two snippets above) since the return value of each iteration is not used after the callback is executed. .map() could be used to return a string for each object and then update the DOM element only once, which would be better for multiple reasons:

fewer DOM lookups - remember those aren't cheap
Array.join() can be used to separate each string with a space, without adding an excess space at the end of the output
the function would only have one responsibility - in-line with the Single Responsibility principle

function getArray(value){
  const key = ['value1', 'value2', 'value3'].find(key => value[key] > 0);
  const showValue = value[key] || 0;
  return value.system + ": "+ showValue;
}

function myFunction() {
  var array = [
    {system:"Abba", value1:0, value2:1, value3:0},
    {system:"Mars", value1:0, value2:4, value3:0},
    {system:"Nexus", value1:0, value2:0, value3:6},
    {system:"Zeroes", value1:0, value2:0, value3:0}
  ];

  document.getElementById("demo").innerHTML = array.map(getArray).join(" ");
}
<button type="button" onclick="myFunction()">Check</button>

Keep HTML and JavaScript separate
The HTML has an onclick attribute:

<button type="button" onclick="myFunction()">Check</button>

Instead of using that attribute, the event handling can be setup in Javascript - e.g. using document.addEventListener():
document.getElementById('checkBtn').addEventListener('click', myFunction);

this would require an id attribute be added to the element - e.g.
<button type="button" id="checkBtn">Check</button>

though other ways to detect it are possible (e.g. class name, elements of document.forms, etc. With this separation the JavaScript (logic) can be modified without ever needing to alter the HTML (markup) - especially helpful in larger projects where one person might update HTML and another the JS.


Dice roller in Python I'm trying to get better in Python and decided to rewrite my dice roller in Perl into Python.

#!/usr/bin/env python
"""roll D&D dice"""

from sys import argv
from random import randrange
import re

if len(argv) < 2:
    raise ValueError("args are dice in D&D format like 2d6")

for roll in argv[1:]:
    matched = re.match(r'^(\d*)d(\d+)$', roll)
    if matched:
        print roll + ':'
        roll_sum = 0
        count = 0
        if len( matched.group(1) ):
            count = int( matched.group(1) )
        else:
            count = 1
        sides = int( matched.group(2) )
        for z in range(count):
            rolled = randrange(1, sides+1)
            print "\troll " + str(z) + ': ' + str(rolled)
            roll_sum += rolled

        possible = str( sides * count )
        print "\ttotal: " + str(roll_sum) + "/" + possible
    else:
        print roll + ' is not a dice in 3d8 format'

I've fixed most of the complaints from Pylint.  I'm looking for ways to make this more pythonic.  If that means more succinct or less, so be it, but I am surprised that the Python version is longer than the Perl version.  Is this the cleanest way to handle the implicit roll count as in d6 instead of 1d6?

Input validation

A 0-sided dice for example 3d0 will raise an exception with an ugly stack trace. Since technically it's in "3d8 format", perhaps an additional validation will be a good idea.

Printing error messages

It's a common practice to print error messages to stderr instead of stdout.

Following the pythonic way

A good start will be to follow PEP8, notably:

the spaces in int( matched.group(1) ) should be removed
if len( matched.group(1) ) can be simplified to if matched.group(1)

The only thing that limits this script to Python 2.x is the print statements. You can easily make this work in Python 3.x by changing the print statements to print(...) function.

String concatenation is a bit awkward, for example you have to manually convert integers to strings. Instead of this:

    print "\troll " + str(z) + ': ' + str(rolled)

The recommend way:

    print("\troll {}: {}".format(z, rolled))

(Nice pun with the "troll" btw :-)

User-friendliness

An example output looks like this:

2d6:
  roll 0: 6
  roll 1: 2
  total: 8/12

"roll 0" huh... D&D is a nerdy thing, but not really in that way... The roll indexes would be better 1-based instead of 0-based in the output.

Variable span

The span of a variable is the lines between two statements that use it.
In this code, roll_sum has a large span. It's initialized early on, but not used until much later.

    print roll + ':'
    roll_sum = 0
    count = 0
    if len( matched.group(1) ):
        count = int( matched.group(1) )
    else:
        count = 1
    sides = int( matched.group(2) )
    for z in range(count):
        rolled = randrange(1, sides+1)
        print "\troll " + str(z) + ': ' + str(rolled)
        roll_sum += rolled

It would be better to move the initialization further down, right before the variable is actually used.

I would also move the print roll + ':' down closer to the statements that do the printing, so as to group similar operations together.

And some empty lines will be nice to create a visual grouping of closely related statements.

Misc

Instead of randrange, you can use randint for an inclusive range.
That is, randrange(1, sides+1) can be written equivalently as randint(1, sides).

Although it's "intelligent" to combine the rolling, summing and printing steps, the code would be slightly cleaner if these were separated. Something like this:

rolls = [randint(1, sides) for _ in range(count)]

for i, rolled in enumerate(rolls):
    print("\troll {}: {}".format(i + 1, rolled))

possible = sides * count
print("\ttotal: {}/{}".format(sum(rolls), possible))

Inside the main for loop for the arguments,
the if branch has a long body and the else branch just one line.
By the time you read the else branch,
the reader might have forgotten what the if was about.
In situations like this it might more readable to flip the branches.
Actually, once there, eliminating a branch becomes an option too, like this:

if not matched:
    print roll + ' is not a dice in 3d8 format'
    continue

# valid case, no more "else" branch, flatter code


How can I optimize this Monte Carlo simulation running at 10,000,000 iterations? I am writing this Monte Carlo simulation and I am facing this issue when running the code at 10,000,000 iterations. here is the code:

import random as rnd
from time import time

#get user input on number of iterations
numOfIterations = raw_input('Enter the number of iterations: ')
numOfIterations = int(numOfIterations)

start = time()

#initialize bag (44 green, 20 blue, 15 yellow,  11 red, 2 white, 1 black
#a counter
#and question counter
bag = 44*'g'+ 20*'b' + 15*'y' + 11*'r' + 2*'w' + 'k'
counter = {'g':0, 'b':0,'y':0 ,'r':0,'w':0,'k':0}
question = {'one':0,'two':0,'three':0,'four':0,'five':0}

for i in range(0,numOfIterations):
  for j in xrange(0,5):
    draw = rnd.sample(bag,5)
    for x in draw: counter[x]+=1
  if counter['w'] >0 and counter['k'] >0: question['one']+=1
  if counter['b'] > counter['r']: question['two']+=1
  if counter['b'] > counter['y']: question['three']+=1
  if counter['y'] > counter['r']: question['four']+=1
  if counter['g'] < (counter['b']+counter['y']+counter['r']+counter['w']+counter['k']): question['five']+=1
  for k in counter: counter[k] = 0

p1 = float(question['one'])/float(numOfIterations)
p2 = float(question['two'])/float(numOfIterations)
p3 = float(question['three'])/float(numOfIterations)
p4 = float(question['four'])/float(numOfIterations)
p5 = float(question['five'])/float(numOfIterations)

print 'Q1 \t Q2 \t Q3 \t Q4 \t Q5'
print str(p1)+'\t'+str(p2)+'\t'+str(p3)+'\t'+str(p4)+'\t'+str(p5)

end = time()

print 'it took ' +str(end-start)+ ' seconds'

any suggestions/criticism would be appreciated.

import random as rnd

I dislike abbreviation like this, they make the code harder to read

from time import time

#get user input on number of iterations
numOfIterations = raw_input('Enter the number of iterations: ')
numOfIterations = int(numOfIterations)

Any reason you didn't combine these two lines?

start = time()

#initialize bag (44 green, 20 blue, 15 yellow,  11 red, 2 white, 1 black
#a counter
#and question counter
bag = 44*'g'+ 20*'b' + 15*'y' + 11*'r' + 2*'w' + 'k'
counter = {'g':0, 'b':0,'y':0 ,'r':0,'w':0,'k':0}
question = {'one':0,'two':0,'three':0,'four':0,'five':0}

Looking up your data by strings all the time is going to be somewhat slower. Instead, I'd suggest you keep lists and store the data that way.

for i in range(0,numOfIterations):

Given that numOfIterations will be very large, its probably a good idea to use xrange here.

  for j in xrange(0,5):

You should generally put logic inside a function. That is especially true for any sort of loop as it will run faster in a function.

    draw = rnd.sample(bag,5)
    for x in draw: counter[x]+=1

I dislike putting the contents of the loop on the same line. I think it makes it harder to read.

  if counter['w'] >0 and counter['k'] >0: question['one']+=1
  if counter['b'] > counter['r']: question['two']+=1
  if counter['b'] > counter['y']: question['three']+=1
  if counter['y'] > counter['r']: question['four']+=1
  if counter['g'] < (counter['b']+counter['y']+counter['r']+counter['w']+counter['k']): question['five']+=1
  for k in counter: counter[k] = 0

p1 = float(question['one'])/float(numOfIterations)
p2 = float(question['two'])/float(numOfIterations)
p3 = float(question['three'])/float(numOfIterations)
p4 = float(question['four'])/float(numOfIterations)
p5 = float(question['five'])/float(numOfIterations)

Don't create five separate variables, create a list. Also, if you add the line from __future__ import division at the beginning of the file then dividing two ints will produce a float. Then you don't need to convert them to floats here.

print 'Q1 \t Q2 \t Q3 \t Q4 \t Q5'
print str(p1)+'\t'+str(p2)+'\t'+str(p3)+'\t'+str(p4)+'\t'+str(p5)

See if you had p1 a list, this would be much easier

end = time()

print 'it took ' +str(end-start)+ ' seconds'

For speed improvements you want to look at using numpy. It allows implementing efficient operations over arrays.

In this precise case I'd use a multinomial distribution and solve the problem analytically rather then using monte carlo.