Compare two CSV files In Ruby I wrote my first program in Ruby that compares two CSV files, but I'm sure there are more efficient ways to do it. I tried using the Ruby CSV library at first, but it was unproductive.  Please let me know if you see a better way of doing it. Specifically, I am asking if anyone knows a simpler way to compare two CSV lists other than loading each first and last name into an array and comparing them, then joining them.

Here is a sample name list that I am comparing to:

Kanye,West
Jay,Z
Chance,TheRapper
Two,Chains

# A program to compare two CSV lists
# and return data that is on both lists

# Files must be present in the directory the
# program is in and the two files must
# match the naming conventions in the program

# open an array for each list of names
people = Array.new
people1 = Array.new

# load each list into a variable
first_list = File.open("namelist.csv", "r")
second_list = File.open("namecomplist.csv", "r")

# split the file up by line and push
# the contents of the line to an array
# each element is delineated by the new line
# character "\n"
first_list.each_line { |line|
    fields = line.split('\n')
    people.push(fields)
}

# split the second file up by line and push
# the contents of the line to an array
second_list.each_line { |line|
    fields = line.split('\n')
    people1.push(fields)
}

# There has to be a cleaner way to do the folowing
# Put the data that is in both files in a variable
final_list = people & people1

# convert the array to a string by joing it
final_list1 = final_list.join

# print the matching data and substitute a comma
# for a space for formatting purposes
# the result should be the names of the people
# who are on both lists
puts final_list1.gsub(',', ' ')

Parsing CSV data

First, let me point out that what you are doing is not proper CSV parsing. Recommended reading: How to parse CSV data with Ruby. Carving up CSV files manually is not often ideal, as it can lead to mistakes, such as what I will explain below.

Using a well established library is most often a better choice. This also holds true of other types of data files, such as XML and JSON. Those libraries help improve consistency and reduce bugs.

Once you've chosen a library to use, read further into how to use that library effectively. You mentioned that you are pretty new, so I will leave that here as something to study when you feel ready to tackle it. =)

Bugs

Your code "works" for your simple use case, but only because you are comparing identical CSV files, rather than because the CSV files contain the same data. What your code is doing is splitting up a string (the characters from your CSV files) into lines, and comparing those. If the lines are identical, but are not in the same order, then you start getting incorrect results. For demonstration, you can try this:

sample_data1 = \
'Kanye,West
Jay,Z
Chance,TheRapper
Two,Chains'

sample_data2 = \
'Jay,Z
Kanye,West
Two,Chains
Chance,TheRapper'

# open an array for each list of names
people = Array.new
people1 = Array.new

# load each list into a variable
first_list = sample_data1
second_list = sample_data2

# rest of the code stays the same

I would expect the result to be the same either way (if it was working correctly), however this is what I get back:

Kanye West
Jay Z

Interesting. Let's see why this is happening, by adding some print and puts instructions in your each_line statements. (note that I converted from curly brackets to do...end syntax since it makes it simpler for statements which aren't one-liners)

puts '- first_list'
first_list.each_line do |line|
    fields = line.split('\n')
    print fields
    people.push(fields)
    puts
end

Did the same thing to second_list. Here is what the console shows now. I added the comments to show you what is happening.

- first_list
["Kanye,West\n"]
["Jay,Z\n"]
["Chance,TheRapper\n"] # has newline
["Two,Chains"] # no newline
- second_list
["Jay,Z\n"]
["Kanye,West\n"]
["Two,Chains\n"] # has newline
["Chance,TheRapper"] # no newline

The issue that is causing those errors is as follows. In Ruby, single-quoted strings and double-quoted strings do not work the same way.

When you use single-quoted strings, they are string literals, so '\n' is not the newline escape character; rather it is a backslash character and the lowercase letter n. If you want to be able to use backslash escaping, you need to use double-quoted string "\n" instead. (this also holds true of things like string interpolation)

You will also notice that you have an array of arrays (or 2D array), which is fine if that's what you need, but I suspect that's possibly not what you expected.

The reason each value is an array is because you are splitting them in a loop with each_line, and then "splitting" (incorrectly due to single quote) each line again with .split('\n') and pushing that into the array.

Implementing the above fixes:

Splitting on "\n" after the file is read, not while iterating;
Using double-quotes to look for newline characters rather than string literals of '\n';
Pushing split strings into the destination array, rather than into sub-arrays/

# open an array for each list of names
people = Array.new
people1 = Array.new

# load each list into a variable
first_list = File.open("namelist.csv", "r").split("\n")
second_list = File.open("namecomplist.csv", "r").split("\n")

# split the file up by line and push
# the contents of the line to an array
# each element is delineated by the new line
# character "\n"
first_list.each do |line|
    people.push(line)
end

# split the second file up by line and push
# the contents of the line to an array
second_list.each do |line|
    people1.push(line)
end

This now functions "correctly", at least to the extent you want to compare for the same lines of comma-separated characters. Note that this is still a naive solution, because if there were spaces or other unexpected characters between the commas and words, it would also not work correctly.

Review

With the nasty bug out of the way, here are some things I would do to improve your code.

First, I would suggest to make a method to read the CSV file. Flambino's answer goes into more detail on that matter, so I will just post a naive implementation that works OK. I recommend to heed Flambino's advice on this file-reading matter.

# Converts a CSV raw character string into
# an array of newline-separated strings.
def load_list_from_csv(csv_file_raw_str)
  list = csv_file_raw_str.split("\n")
end

Then we can just update this:

# load each list into a variable
first_list = load_list_from_csv(sample_data1)
second_list = load_list_from_csv(sample_data2)

Or in your case...

# load each list into a variable
first_list = load_list_from_csv("namelist.csv", "r")
second_list = load_list_from_csv("namecomplist.csv", "r")

While we're at it, let's make a method to compare the 2 files. Basically just combining a few bits of code into one method. By the way, the operation you are making on those data sets is called an intersection.

Here is a working code example on repl.it showing the changes:

sample_data1 = \
'Kanye,West
Jay,Z
Chance,TheRapper
Two,Chains'

sample_data2 = \
'Jay,Z
Kanye,West
Two,Chains
Chance,TheRapper'

# Converts a CSV raw character string into
# an array of newline-separated strings.
def load_list_from_csv(csv_file_raw_str)
  list = csv_file_raw_str.split("\n")
end

# Intersect two arrays to return identical strings/lines
def intersect_csv_names_lists(file1, file2)
  people1 = load_list_from_csv(file1)
  people2 = load_list_from_csv(file2)
  [people1 & people2].join("\n")
end

# Replace commas with spaces in a string
def format_replace_commas_with_spaces(arr)
  arr.gsub(',', ' ')
end

# Print the matching data and substitute a comma
# for a space for formatting purposes.
# The result should be the names of the people
# who are on both lists.
puts format_replace_commas_with_spaces(intersect_csv_names_lists(sample_data1, sample_data2))

Output:

ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-linux]

 Kanye West
 Jay Z
 Chance TheRapper
 Two Chains
=> nil


Abstract session value manager I've been working in C# for a while, but I'm just recently starting to force myself to use better abstracted and generic code. I recently encountered a situation where I was juggling a lot of session values on a page, so I made a wrapper class for the session to enable easy and safe get/sets of session values.

I'm not entirely sure if this is the best way to go about accomplishing what I want, but I would love some feedback on ways this code can be improved. I'm aware that it can currently only be used for string values, but I'm just using it right now for storing form values between multiple postbacks in a very complex page, so that's not a huge concern for me at the moment.

Base class

abstract public class SessionManager
{
    public List<string> PossibleKeys;

    public string GetValue(HttpSessionState pSession, string pKey)
    {
        if (this.PossibleKeys.Contains(pKey))
            return ((SessionManager)pSession[this.SessionKey]).Values[pKey].ToString();
        else
            return "";
    }

    public bool SetValue(HttpSessionState pSession, string pKey, string pValue)
    {
        if (this.PossibleKeys.Contains(pKey))
        {
            var tParameters = (SessionManager)pSession[this.SessionKey];
            tParameters.Values[pKey] = pValue.Trim();
            pSession[this.SessionKey] = tParameters;
            return true;
        }
        return false;
    }

    abstract class Keys { }

    private Dictionary<string, string> Values { get; set; }
    private string SessionKey { get; set; }
    private static string DefaultKey = "parameters";

    protected void Construct(HttpSessionState pSession, string pSessionKey = null)
    {
        if (pSessionKey.IsNullOrWhitespace())
            this.SessionKey = SessionManager.DefaultKey;
        else
            this.SessionKey = pSessionKey.Trim();

        this.Values = new Dictionary<string, string>();
        foreach (var tKey in this.PossibleKeys)
        {
            this.Values.Add(tKey, "");
        }

        SessionManager.Save(pSession, this, this.SessionKey);
    }

    private static void Save(HttpSessionState pSession, SessionManager pSelf, string pSessionKey)
    {
        pSession[pSessionKey] = pSelf;
    }
}

Implemented class on the page codebehind

public class DataManager : SessionManager
{
    public class Keys
    {
        public static string MemberID = "MemberID";
        public static string ProviderID = "ProviderID";
        public static string ReferredToID = "ReferredToID";
        public static string SpecialtyID = "SpecialtyID";
        public static string DiagnosisID = "DiagnosisID";
        public static string CPTServiceID = "CPTServiceID";
        public static string FromDate = "FromDate";
        public static string ToDate = "ToDate";
    }

    public DataManager(HttpSessionState pSession, string pSessionKey = null)
    {
        this.PossibleKeys = new List<string>()
        {
              DataManager.Keys.MemberID
            , DataManager.Keys.ProviderID
            , DataManager.Keys.ReferredToID
            , DataManager.Keys.SpecialtyID
            , DataManager.Keys.DiagnosisID
            , DataManager.Keys.CPTServiceID
            , DataManager.Keys.FromDate
            , DataManager.Keys.ToDate
        };

        base.Construct(pSession, pSessionKey);
    }
}

Actual use in the page codebehind

    public DataManager Parameters { get; set; }
    protected void Page_Load(object sender, EventArgs e)
    {

        if (!IsPostBack)
        {
            this.Parameters = new DataManager(this.Session);
        }

        this.Parameters.SetValue(this.Session, DataManager.Keys.DiagnosisID, "Blah blah, your value goes here");

        string test = this.Parameters.GetValue(this.Session, DataManager.Keys.DiagnosisID);
    }

I don't get what your solution brings that something like this doesn't:

public class DataManager
{
    private HttpSessionState session
    public String MemberID
    {
        get {return session["MemberID"];}
        set {session["MemberID"] = value;}
    }
}

Managing the collections Keys and PossibleKeys in two places seems error-prone.
Your naming convention of prefixing everything with 'p' is unusual (in C#).


Reading and writing binary data in C++ I have written a small container class which groups a 3D position, a normal vector and a texture coordinate into one object. It uses the glm library for the actual data types (vec2 and vec3). This class is named Vertex. Here is the source:

Vertex.h

#ifndef CPP3D_VERTEX_H
#define CPP3D_VERTEX_H

#include <iostream>
#include <glm/glm.hpp>

namespace cpp3d {

class Vertex {

public:
    glm::vec3 position;
    glm::vec3 normal;
    glm::vec2 texCoord;
    Vertex();
    Vertex(const glm::vec3& position);
    Vertex(const glm::vec3& position, const glm::vec3& normal);
    Vertex(const glm::vec3& position, const glm::vec3& normal, const glm::vec2& texCoord);
    bool operator==(const Vertex& other);
};

std::ostream& operator<<(std::ostream &stream, const Vertex& vertex);
std::istream& operator>>(std::istream &stream, Vertex& vertex);

}

#endif

Vertex.cc

#include "Vertex.h"

namespace cpp3d {

using namespace glm;
using namespace std;

Vertex::Vertex() {
}

Vertex::Vertex(const vec3& position) :
        position(position) {
}

Vertex::Vertex(const vec3& position, const vec3& normal) :
        position(position), normal(normal) {
}

Vertex::Vertex(const vec3& position, const vec3& normal, const vec2& texCoord) :
        position(position), normal(normal), texCoord(texCoord) {
}

bool Vertex::operator ==(const Vertex& other) {
    return position == other.position && normal == other.normal && texCoord == other.texCoord;
}

ostream& operator<<(ostream &stream, const Vertex& vertex) {
    stream.write(reinterpret_cast<const char *>(&vertex), sizeof(vertex));
    return stream;
}

istream& operator>>(istream &stream, Vertex& vertex) {
    stream.read(reinterpret_cast<char *>(&vertex), sizeof(vertex));
    return stream;
}

}

As you can see I've overloaded the stream operators to write and read the data the object contains. For this I simply dump the whole object which results in writing eight float numbers so the output will be 32 bytes. I know that this is a very simple file format but I want to read the data as fast as possible so I don't want to parse text files. I also don't really care about different endiannesses (unless it can be implemented without speed impact). And I know the file format will change when I add more members to the class (This will most likely not happen but when it happens then I also want to change the file format anyway)

My main concern is if this code guarantees that it always writes and reads 32 bytes. I'm pretty new to C++ and my ancient C knowledge tells me that there is (or was) some kind of packed structs and aligned structs where the compiler could decide to align 8 bit values to 16 bits for some reason. I don't want some compiler (current or future ones) to store the 32 bit floats in 64 bit values because this obviously changes the binary data format. Can this happen with my code when compiled on specific platforms? If yes, what's the best way to write binary floats to a stream then? Shall I write each coordinate in each vector with a separate call to write()? Or are there other/better techniques to write and read the binary data?

If you see other issues with my code which are not related to my question please feel free to comment anyway. As a C++ newbie I'm eager to learn.

Both std::istream and std::ostream are actually not defined in <iostream>, despite the fact that they look similar.  They're respectively defined in <istream> and <ostream>.

You should instead include these in the implementation file and include <iosfwd> (the iostream declaration forwarding library) in the header.
These data members should be private:

glm::vec3 position;
glm::vec3 normal;
glm::vec2 texCoord;

Otherwise, they will be accessible outside of the class.  If they are not meant to be this way, then you should use a struct instead, which are public by default.
If you don't need a default constructor, then leave it out.  The compiler will provide one for you.
Regarding operator==:

Conditional operators overloads, such as operator==, should be const as they should not modify any data members:

bool operator==(const Vertex& other) const;

Its definition could also be split into multiple lines, which is easier to read than one long line:

bool Vertex::operator==(const Vertex& other) {
    return position == other.position
        && normal == other.normal
        && texCoord == other.texCoord;
}


Simple unit test in C of input using freopen Here is an initial attempt at unit test of user input in C.  The thing that feels unusual is the use of freopen to send the test data to stdin.  Are there better ways to implement this kind of test?

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

static size_t get_num(size_t tmpnum) {
    if (scanf("%zu", &tmpnum) != 1) {
        fprintf(stderr, "ERROR: system error: failed to read input, exiting.\n");
        exit(1);
    }
    return tmpnum;
}

int main(void)
{
    int return_status = 0;

    /* Test 1 */
    if (freopen("test-data.txt", "r", stdin) != NULL) {
        size_t tmpnum = 0;
        tmpnum = get_num(tmpnum);
        assert(tmpnum >= 1 && tmpnum <= 20);
        printf("ok: tmpnum == %ld\n", tmpnum);
        freopen("/dev/stdin", "r", stdin);
    } else {
        printf("ERROR: failed to open test-data.txt\n");
        return_status = EXIT_FAILURE;
    }

    return return_status;
}

The thing that feels unusual is the use of freopen to send the test data to stdin.
Are there better ways to implement this kind of test?

C has a freopen() footnote

The primary use of the freopen function is to change the file associated with a standard text stream (stderr, stdin, or stdout), as those identifiers need not be modifiable lvalues to which the value returned by the fopen function may be assigned.

This looks like a good direct way to test code, although I'd expect stdout, strderr being re-opened to capture output.

Are there better ways

Enable all compiler warnings - save time.
The mismatch of specifier and type implies code is not efficiently using the 1st round of code improvement:  Compiler warnings.
// printf("ok: tmpnum == %ld\n", tmpnum);
printf("ok: tmpnum == %zu\n", tmpnum);

Code is strange in that it passes in tmpnum for no good reason.
//static size_t get_num(size_t tmpnum) {
//  if (scanf("%zu", &tmpnum) != 1) {

static size_t get_num(void) {
  size_t tmpnum;
  if (scanf("%zu", &tmpnum) != 1) {


Code to make elements fit within their container while keeping their aspect ratio This function receives the id of an element (frame) and calculates the correct height and width so it fits inside its parent element, while keeping its aspect ratio. Before I had a way to calculate the frame ratio dynamically, but I decided it was easier to simply hard code it, it calculates it through naturalHeight and naturalWidth if it's an image, and use 0.5625 for the rest which are iframes, and that ratio makes sense for all of those. It works, tried it in different sizing scenarios, what I'm not 100% sure is if the math involved to calculate the correct width and height in all cases is correct.

In all cases the element should fill as much as possible.

function fix_frame(frame_id)
{
    var id = `#${frame_id}`

    var frame = $(id)

    if(frame_id === "media_image_frame")
    {
        var frame_ratio = frame[0].naturalHeight / frame[0].naturalWidth
    }

    else
    {
        var frame_ratio = 0.5625
    }

    var parent = frame.parent()
    var parent_width = parent.width()
    var parent_height = parent.height()
    var parent_ratio = parent_height / parent_width

    var frame_width = frame.width()
    var frame_height = frame.height()

    if(parent_ratio === frame_ratio)
    {
        frame.width(parent_width)
        frame.height(parent_height)
    }

    else if(parent_ratio < frame_ratio)
    {
        frame.width(parent_height / frame_ratio)
        frame.height(parent_height)
    }

    else if(parent_ratio > frame_ratio)
    {
        frame.width(parent_width)
        frame.height(parent_width * frame_ratio)
    }
}

Although you posted your code it would help if you told us what you want help with?

This is possible to do using CSS. I've seen multiple solitions, here is the first one I found on Google: http://www.mademyday.de/css-height-equals-width-with-pure-css.html


Best way to optimize this code so it takes up less space? CRITICAL = 0
MEDIUM = 1
LOW = 0

test = {
    "version": "0",
    "id": "85fc3613-e913-7fc4-a80c-a3753e4aa9ae",
    "detail-type": "ECR Image Scan",
    "source": "aws.ecr",
    "account":
        "123456789012",
    "time": "2019-10-29T02:36:48Z",
    "region": "us-east-1",
    "resources": [
        "arn:aws:ecr:us-east-1:123456789012:repository/my-repo"
    ],
    "detail": {
        "scan-status": "COMPLETE",
        "repository-name": "my-repo",
        "finding-severity-counts": {
            "CRITICAL": 10,
            "MEDIUM": 9
        }
    }
}
resource = test["resources"][0]

finding_severity_counts = test["detail"]["finding-severity-counts"]

if "CRITICAL" in finding_severity_counts:
    critical_count = finding_severity_counts["CRITICAL"]
    if critical_count > CRITICAL:
        print("Resource {} has {} critical findings".format(resource, critical_count))

if "MEDIUM" in finding_severity_counts:
    medium_count = finding_severity_counts["MEDIUM"]
    if medium_count > MEDIUM:
        print("Resource {} has {} medium findings".format(resource, medium_count))

if "LOW" in finding_severity_counts:
    low_count = finding_severity_counts["LOW"]
    if low_count > LOW:
        print("Resource {} has {} low findings".format(resource, low_count))

I have got this code and am trying to optimize it so it is smaller and takes up less lines of code. Right now the code is kind of ugly and not at all efficient. Is there any way to make this more efficient?

There is indeed a great deal of repetition in your code. A straight-forward solution to this would be putting your severity levels into a dictionary and iterate over it instead:
test = {
    "version": "0",
    "id": "85fc3613-e913-7fc4-a80c-a3753e4aa9ae",
    "detail-type": "ECR Image Scan",
    "source": "aws.ecr",
    "account":
        "123456789012",
    "time": "2019-10-29T02:36:48Z",
    "region": "us-east-1",
    "resources": [
        "arn:aws:ecr:us-east-1:123456789012:repository/my-repo"
    ],
    "detail": {
        "scan-status": "COMPLETE",
        "repository-name": "my-repo",
        "finding-severity-counts": {
            "CRITICAL": 10,
            "MEDIUM": 9
        }
    }
}

resource = test["resources"][0]
finding_severity_counts = test["detail"]["finding-severity-counts"]

THRESHOLDS = {
    'CRITICAL' : 0,
    'MEDIUM'   : 1,
    'LOW'      : 0
}

for severity in THRESHOLDS:
    if severity in finding_severity_counts:
        count = finding_severity_counts[severity]
        if count > THRESHOLDS[severity]:
            print("Resource {} has {} {} findings".format(resource, count, severity.lower()))

This should produce the exact same result as your code does, by reusing the logic (that's the same for every severity. Note that we can reuse the count, since we're only interested in one value at a time.
That is, assuming you're only interested in the amount of severities if they reach a certain threshold. Hence the name.
It would probably be prettier to put the for-loop into a function though. An even better solution would probably involve one of Python's 'collections' functions, like Counter.


Better way of clone and then replace some attributes with jQuery I have HTML code like this :

Add Text

            <input class="form-control" id="project_components_attributes_32943600_name" name="project[components_attributes][32943600][name]" type="text">

              <input id="project_components_attributes_32943600__destroy" name="project[components_attributes][32943600][_destroy]" type="hidden" value="false">

If I click link "Add Text", It will clone div with class form-inline and append to div#nF and then replacing all attribute of input tag in form-inline (id and name) with new value from new date and time.

For Illustration :

  id => project_components_attributes_32943600_name => replace
  "32943600" with new value

  name => project[components_attributes][32943600][_destroy] => replace
  "32943600" with new value

And this javascript :

$(document).ready(function(){
    $('#addTxt').click(function(){
        var time = new Date().getTime();
        var $target = $('#nF').find('div.form-inline:first');
        $target.clone().appendTo('#nF');
        $target.find('input').each(function(){
           $(this).val('');
           var tID = $(this).attr("id").split(/_/);
           var re = new RegExp(tID[3],"g");
           var newID = $(this).attr('id').replace(re, time);
           var newName = $(this).attr('name').replace(re, time);
           $(this).attr('id', newID);
           $(this).attr('name', newName);
        });
    });
});

Live Demo : http://jsfiddle.net/gjj8mzya/

Is there any way to clone element and replace attributes?

The current code does essentially this:

Clone the dirtied first item and append to the end
Modify the dirtied first item to make it clean again

It would be better to use a clean prototype / template from which you can easily create clean new instances. Then the operations can become:

Move the dirtied first item to the end
Recreate the first item from the clean template

The first approach is not so good, because:

Cloning is always a suspicious operation. For example, you have to make sure that:

all fields correctly copied
no references accidentally leaked
deep objects correctly deep-copied: although this is not a concern in this specific example, but I'm adding it anyway as a reminder of general concerns about the concept of cloning

Reseting an object to a clean state is always a suspicious operation. For example, you have to make sure that:

all fields are correctly reset: often duplicating the same logic that must exist (explicitly or implicitly) in the initializer / constructor

The new approach doesn't have any of these tricky issues.
It's conceptually much cleaner, with much fewer hidden traps and bugs waiting to happen.


Creating a border using ncurses The biggest question is if it's worth it to use a useless if statement to make the x and y variables local away from the main statement.

    //Unconventional part, make it so that x and y are local variables
if(true){
        int x,y;
        //set x and y to be the width + height of the terminal
        getmaxyx(stdscr,y,x);
        for(int i = 0; i < x; i++){
                //top border
                mvaddch(0,i,'#');
                //bottom border
                mvaddch(y - 1,i,'#');
        }
        for(int i = 0; i < y; i++){
                //left border
                mvaddch(i,0,'#');
                //right border
                mvaddch(i,x - 1,'#');
        }
}

You can create a block without an if statement.
For example, these are equivalent:

{
    // some code
}

if (true) {
    // some code
}

But it's not a good practice to create blocks like this.
It's a code smell,
suggesting to wrap // some code in a dedicated function instead.

Anytime you think some neat trick is "unconventional",
it's most probably a bad practice. Don't do it, fix it.


Filtering units by a dynamic Lambda expression I have a simple if statement which, based on user input, determines which expression is going to be utilized. It looks like it can be done better.

Func<Unit, bool> exp;
if (onlyChildren)
    exp = u => u.PhaseId == id;
else
    exp = u => u.PhaseId != id;

var unitGroups = _db.Units.Where(exp).GroupBy(m => m.UnitTemplate, (m, group) => new UnitTemplateZoneVm()
{
    Id = m.Id,
    Units = group.ToList()
}).ToList();

Don't omit braces. Ever. It's such a bad idea (I learned this the hard way).

As far as your lambda, your method is not necessarily a bad thing, though there are always other ways to write it.

var unitGroups = _db.Units
                    .Where(u => (onlyChildren && u.PhaseId == id) || (!onlyChildren && u.PhaseId != id))
                    .GroupBy(m => .UnitTemplate, (m, group) => new UnitTemplateZoneVm()
                    {
                        Id = m.Id,
                        Units = group.ToList()
                    })
                    .ToList();

Whitespace is free, use it. It makes it clearer as to what steps belong together.

I would personally prefer this method, as you can extract this out into LINQ-SQL style, instead of using the methods. Personally, I prefer the LINQ-SQL style rather than the methods, but the choice is up to you.


Optimization for calculating statistics I have collected a large amount of data about the train network in my country. Now, I am writing some code that calculates the average amount of delay on the entire train network, for each day, in a given period of time. Important to note: every train has a unique train-number. Each train-number only occurs once each day.

The average amount of delay is calculated in two ways:

Worst case: For every train, the maximum amount of delay that train suffers on the given day is used for the computation.
Average case: For every train, the average amount of delay is computed over all the stops of the train and used for the computation.

To further understand my code, I have to explain a portion of my database design. Each train has train_id, origin, destination. Each route has route_id, train_id, date. As mentioned before, each train_id occurs only once each day. So the combination of train_id and date is unique. Each stop has stop_id, arrival_delay, departure_delay, route_id. Each stop is associated with only one route.

start = date(2014, 10, 27)
stop = date(2014, 12, 14)
diff = stop - start
period = pd.date_range(start, stop)
zeros = np.zeros((diff.days+1, 2))

#DataFrame with worst-case and avg delay on entire network, per day
#Delay is computed in a worst-case scenario, i.e. max delay of a train
#and in avg-case scenario, i.e. avg of avg of arrival and avg of departure delay
delays = pd.DataFrame(zeros, index = period, columns = ['Worst case', 'Avg case'])

while(start <= stop):
    t = time(0, 0, 0)
    dt = datetime.combine(start, t)

    #Get all routes for a specified date.
    #Every trainID rides only once each day, so there is no point in asking all the trainIDs first
    routes = #Gets all routes for given date out of database

    worst_train_delays = []
    avg_train_delays = []

    for routeRow in routes:
        routeID = routeRow[0]
        stops = #Gets all stops for given routeID out of database
        max_delay = 0
        arrivals = []
        departures = []

        for stopRow in stops:
            arrival_delay = stopRow[2]
            departure_delay = stopRow[5]
            arrivals.append(arrival_delay)
            departures.append(departure_delay)
            if max(arrival_delay, departure_delay) > max_delay:
                max_delay = max(arrival_delay, departure_delay)

        worst_train_delays.append(max_delay)
        avg_arrival = np.mean(arrivals)
        avg_departure = np.mean(departures)
        avg_train_delays.append(np.mean([avg_arrival, avg_departure]))

    key = start.isoformat()
    delays['Worst case'][key] = np.mean(worst_train_delays)
    delays['Avg case'][key] = np.mean(avg_train_delays)

    delta = timedelta(days=1)
    start = start + delta

The above code computes the average delay, per day, for a period of 49 days. There are about 4000 trains on the network, with each on average 10 stops. These equates to reading approx. 2.000.000 values.

Could I improve this code in terms of speed, performance, memory usage?

One problem I see with your code is that you first collect all the data points you want to average, and then you take the average. This has O(n) complexity and extra storage requirements, as you have to append all the data points into a list.

An easy way to fix this is to keep track of the sum and the number of data points, and then divide to get the average. This uses O(1) extra memory instead of O(n), as for each data point, you increment the number of data points and add the value to the accumulated sum, and then continue on to the next one.

For example, instead of:

for stopRow in stops:
    # some code
    arrivals.append(arrival_delay)
    # more code

avg_arrival = np.mean(arrivals)

you should use:

total_arrival_delay = 0
number_arrival_delays = 0

for stopRow in stops:
    # some code
    total_arrival_delay += arrival_delay
    number_arrival_delays += 1
    # more code

avg_arrival = total_arrival_delay / number_arrival_delays

While this approach makes your code a bit longer, it will allow it to work on larger datasets, as you no longer need to store lots and lots of data points while processing, and instead need only to store a few integers


Generics: Is this improper use? I have a class library in which I'm using generics as below. I'm not sure if it's an improper use or not. So aside from the fact that it works and everything "depends", id like some specific critique on how this usage might fall apart. In particular In wondering about the shadowing of non generic properties with generic counterparts. Is this an accepted practice?

 public interface IPickOrder
    {
        IPickOrderHeader Header { get; }

        ERPListBase<IPickOrderLine> Lines { get; }
    }

 public interface IPickOrder<TPickOrderHeader, TPickOrderLine> : IPickOrder
        where TPickOrderHeader : class, IPickOrderHeader
        where TPickOrderLine : class, IPickOrderLine
    {
        new TPickOrderHeader Header { get; }

       new ERPListBase<TPickOrderLine> Lines { get; }
    }

 public class PickOrder : IPickOrder
    {
        public IPickOrderHeader Header { get; protected set; }

        public ERPListBase<IPickOrderLine> Lines { get; protected set; }

        public PickOrder() { }
    }

   public class PickOrder<TPickOrderHeader, TPickOrderLine> : PickOrder, IPickOrder<TPickOrderHeader, TPickOrderLine>
        where TPickOrderHeader : class, IPickOrderHeader, new()
        where TPickOrderLine : class, IPickOrderLine, new()
    {
        public new TPickOrderHeader Header
        {
            get { return base.Header as TPickOrderHeader; }
            protected set { base.Header = value; }
        }

        public new ERPListBase<TPickOrderLine> Lines { get; protected set; }

        /// <summary>
        ///
        /// </summary>
        public PickOrder() : base() { }
    }

I feel you on the "My reasons are more due to preference than need." comment but the inheritance from PickOrder and public new ERPListBase<TPickOrderLine> Lines { get; protected set; }  will confuse somebody best case and really mess things up worst case. While it may still give you that feeling of impending doom here are some suggestions to make this safer while keeping the look and feel you prefer.

Consider implementing the property like so (also for Header)

public new ERPListBase<TPickOrderLine> Lines {
    get {
        // note that you can't control base.Lines so some values may not be of TPickOrderLine
        return new ERPListBase<TPickOrderLine>(base.Lines.OfType<TPickOrderLine>());
    }
    protected set { base.Lines = new new ERPListBase<IPickOrderLine>(value.Cast<IPickOrderLine>()); }
}

Consider not reusing the base class and using explicit interface implementation:

public class PickOrder<TPickOrderHeader, TPickOrderLine> :
    IPickOrder<TPickOrderHeader, TPickOrderLine>
    where TPickOrderHeader : class, IPickOrderHeader, new()
    where TPickOrderLine : class, IPickOrderLine, new()
{
    public TPickOrderHeader Header { get; protected set; }

    IPickOrderHeader IPickOrder.Header { get { return Header; } }

    public ERPListBase<TPickOrderLine> Lines { get; protected set; }

    ERPListBase<IPickOrderLine> IPickOrder.Lines { get { return new ERPListBase<IPickOrderLine>(Lines.Cast<IPickOrderLine>()); } }
}

Whatever you do, avoid storing the same data twice as you may be doing (depending on use) in your current code.


Python Script: Converts .tsv to TFRecord The python code below converts a tab spaced values file (.tsv) into a TFRecord in hopes that this approach expedites future time spent loading the data. Is the performance of this task correct and optimal, specifically in terms of run-time?

The .tsv file is of the following format, including spaces to make it more readable (SEMEVAL STS'17 training dataset): float_value_from_0_to_5 \t string1 \t string2. The desired TFRecord's entries should optimally store the previous values as float rate, string sent1, and string sent2 for proper implementation with any arbitrary Neural Network that will embed the words once loaded from the TFRecord.

import tensorflow as tf
import nltk
import os
import csv

def tsv_to_tfrec(file_path):
  with open(file_path, 'rb') as tsv, tf.python_io.TFRecordWriter(
        os.path.splitext(file_path)[0] + ".tfrecords") as tfrec:
    read_tsv = csv.reader(tsv, delimiter = '\t', quoting = csv.QUOTE_NONE)

    for row in read_tsv:
      if len(row) != 3:
        print("\nERROR in # of indicies: ", len(row))
        for i in range(len(row)):
          print(i, " =", row[i])
      else:
        example = tf.train.Example(features = tf.train.Features(feature = {
            "rate": tf.train.Feature(float_list=tf.train.FloatList(
                value = [float(row[0])])),
            "sent1": tf.train.Feature(bytes_list=tf.train.BytesList(
                value = nltk.word_tokenize(row[1]))),
            "sent2": tf.train.Feature(bytes_list=tf.train.BytesList(
                value = nltk.word_tokenize(row[2])))
            }))

        tfrec.write(example.SerializeToString())

I am no expert in TensorFlow (even though started to grasp), but here are some things I noticed:

if this is a proper valid TSV file, you should not be worried about checking the length of each row - each row should have the same amount of columns
you may also try to use TextLineReader to read features from a csv file like noted here, this may lead to "unpacking" the features and avoiding getting them by index:

rate, sent1, sent2 = tf.decode_csv(value)

if, for some reason, TextLineReader is not applicable, you may unpack in the for loop:

for rate, sent1, sent2 in read_tsv:

you can also look into applying some micro-optimizations, like avoiding name lookups in the loop by pre-lookuping things before the loop - e.g. setting Example = tf.train.Example and then using Example in place of tf.train.Example
there is also a possibility of that classic trading off memory for the speed. Depending on how unique your features are, you, for instance, can "cache" the word tokenization results in a dictionary to avoid re-tokenizing the same sentences over again

Also check out the TensorFlow Performance Guide and Performance Tips.

There are also some minor PEP8 code style violations, like extra spaces around = in the keyword argument definitions - run your code through a static code analyzer like flake8 or pylint.


Removing untagged docker images using Python I have written a piece of code in Python for removing the docker images which are not tagged only:

# docker images
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
release             3.1                 b6bf9d19cc6c        5 hours ago         869MB
release             3.2                 b6bf9d19cc6c        5 hours ago         869MB
<none>              <none>              3dfdfcb0769d        6 hours ago         433MB
<none>              <none>              d505190470fd        6 hours ago         433MB
<none>              <none>              979a42368814        7 hours ago         433MB
<none>              <none>              f8bcf895ffce        7 hours ago         433MB
<none>              <none>              8c1ed97822da        7 hours ago         433MB
release             1.0                 36e9ea407082        7 hours ago         433MB
ubuntu              16.04               6a2f32de169d        6 days ago          117MB
vault               latest              144fecac962b        3 weeks ago         64.4MB

Python code:

import subprocess

output = subprocess.Popen(["docker", "images"], stdout=subprocess.PIPE)

result = output.communicate()[0].split("\n")
image_list = []

for line in result[1:]:

    if ("<none>" == line.split("  ")[0]):
        image_list.append(filter(None,line.split("  "))[2])

for image in image_list:
     #Piping both stdout and stderr to stdout.
     output = subprocess.Popen(["docker", "rmi", "-f", image], stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
     print(output.communicate()[0])

Final output:

docker images

REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
release             3.1                 b6bf9d19cc6c        5 hours ago         869MB
release             3.2                 b6bf9d19cc6c        5 hours ago         869MB
release             1.0                 36e9ea407082        7 hours ago         433MB
ubuntu              16.04               6a2f32de169d        6 days ago          117MB
vault               latest              144fecac962b        3 weeks ago         64.4MB

Please let me know if the code can be optimized or can be written in a more pythonic way.

I would split the line of the output once and re-use that. Also, if you don't pass a separator to str.split, it will split on whitespace, so "A    B C".split() == ['A', 'B', 'C'].

import subprocess

output = subprocess.Popen(["docker", "images"], stdout=subprocess.PIPE)
result = output.communicate()[0].split("\n")

image_list = []
for line in result[1:]:
    line = line.split()
    if line[0] == "<none>":
        image_list.append(line[2])

for image in image_list:
     #Piping both stdout and stderr to stdout.
     output = subprocess.Popen(["docker", "rmi", "-f", image], stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
     print(output.communicate()[0])

I would probably change if line[0] == "<none>" to if line[1] == "<none>" to adhere more closely to what you want to do. I'm not sure if it can happen that the repository is not none, but the tag is.

I would also change the grouping slightly, so that things which logically belong together are grouped. This is the first step towards putting them into their own functions, which could be slightly overkill here, but would make it a lot easier to re-use parts of this:

import subprocess

def get_docker_images():
    output = subprocess.Popen(["docker", "images"], stdout=subprocess.PIPE)
    return map(str.split, output.communicate()[0].split("\n")[1:-1])

def no_tag(images):
    for image in images:
        if image[1] == "<none>":
            yield image

def delete_images(images):
    for image in images:
        output = subprocess.Popen(["docker", "rmi", "-f", image[2]],
                                  stdout=subprocess.PIPE,
                                  stderr=subprocess.STDOUT)
        print(output.communicate()[0])

if __name__ == '__main__':
    delete_images(no_tag(get_docker_images()))


Is this queue properly implemented? I tried to implement a queue which stores it's elements in a resizing array. Seems to work fine. Can you tell me if i did something wrong?

public class Queue<T>
{
    private T[] array;

    public Queue()
    {
        array = new T[16];
    }

    private void Enqueue(T item)
    {
        if(Count == array.Length/2)
            Grow();

        array[Count++] = item;
    }

    public void Grow()
    {
        var temp = array;
        array = new T[array.Length * 2];
        Array.Copy(temp,startingPos,array,0,Count);
        startingPos = 0;
    }

    public void Shrink()
    {
        var temp = array;
        array = new T[array.Length / 2];
        Array.Copy(temp,startingPos,array,0,Count);
        startingPos = 0;
    }

    public T Dequeue()
    {
        if(Count == array.Length/4)
            Shrink();

        var item = array[startingPos];
        array[startingPos++] = default(T);
        Count--;
        return item;
    }

    int startingPos = 0;

    public int Count {get;set;}
}

This is a partial review about some things I saw reading through your code fast.

The setter of your Count property shouldn't be public, because I could set it to one million when there's only one element in the queue.

You should specify the accessibility modifier for your member field, I guess it is private int startingPos, and I think you should put it at the top of your class, it is easier to read this way.

I don't think you should expose methods Shrink and Grow as they aren't part of a Queue but more of its implementation. Exposing these methods imply that you use an Array behind and the users of your class shouldn't know about this.

Also, in your Enqueue method, you grow your array every time your array is half filled, it is overkill. You should expand it only when the current array is full, otherwise you will always have half your array wasted.


Generated serialization code CGbR project

I am currently working on a project that generates code from classes. The idea is to achieve maximum performance by replacing reflection use cases with fast, generated code.

Current results look good, but I want to take this chance to find room for further improvement.

Class under test:

[DataContract]
public partial class Partial
{
    [DataMember]
    public float Price { get; set; }

    [DataMember]
    public string Name { get; set; }

    [DataMember]
    public List<double> DecimalNumbers { get; set; }
}

Generated JSON

One of the generator creates methods to read and write JSON. Code is available in full at the benchmark project.

To JSON:

writer.Write(",\"Price\":");
writer.Write(Price.ToString(CultureInfo.InvariantCulture));

writer.Write(",\"Name\":");
writer.Write(string.Format("\"{0}\"", Name));

writer.Write(",\"DecimalNumbers\":");
if (DecimalNumbers == null)
    writer.Write("null");
else
{
    writer.Write('[');
    foreach (var value in DecimalNumbers)
    {
        writer.Write(value.ToString(CultureInfo.InvariantCulture));
        writer.Write(',');
    }
    writer.Write(']');
}

From JSON:

while (reader.Read())
{
    // Break on EndObject
    if (reader.TokenType == JsonToken.EndObject)
        break;

    // Only look for properties
    if (reader.TokenType != JsonToken.PropertyName)
        continue;

    switch ((string) reader.Value)
    {
        case "Price":
            reader.Read();
            Price = Convert.ToSingle(reader.Value);
            break;

        case "Name":
            reader.Read();
            Name = Convert.ToString(reader.Value);
            break;

        case "DecimalNumbers":
            reader.Read(); // Read token where array should begin
            if (reader.TokenType == JsonToken.Null)
                break;
            var decimalnumbers = new List<double>();
            while (reader.Read() && reader.TokenType != JsonToken.EndArray)
                decimalnumbers.Add(Convert.ToDouble(reader.Value));
            DecimalNumbers = decimalnumbers;
            break;

Generated binary conversion

Another generator maps the class directly onto a byte[].

Converter methods:

public static unsafe void Include(ushort value, byte[] bytes, int index)
{
    fixed (byte* b = bytes)
        *((ushort*)(b + index)) = value;
}

To binary:

// Convert Price
GeneratorByteConverter.Include(Price, bytes, index);;
index += 4;
// Convert Name
// Two bytes length information for each dimension
GeneratorByteConverter.Include((ushort)(Name == null ? 0 : Name.Length), bytes, index);
index += 2;
if (Name != null) Buffer.BlockCopy(_encoder.GetBytes(Name), 0, bytes, index, Name.Length);
index += Name == null ? 0 : Name.Length;
// Convert DecimalNumbers
// Two bytes length information for each dimension
GeneratorByteConverter.Include((ushort)(DecimalNumbers == null ? 0 : DecimalNumbers.Count), bytes, index);
index += 2;
if (DecimalNumbers != null)
for(var i = 0; i < DecimalNumbers.Count; i++)
{
    var value = DecimalNumbers[i];
    GeneratorByteConverter.Include(value, bytes, index);;
    index += 8;
}

From binary:

// Read Price
Price = BitConverter.ToSingle(bytes, index);
index += 4;
// Read Name
var nameLength = BitConverter.ToUInt16(bytes, index);
index += 2;
Name = _encoder.GetString(bytes, index, nameLength);
index += nameLength;
// Read DecimalNumbers
var decimalnumbersLength = BitConverter.ToUInt16(bytes, index);
index += 2;
var tempDecimalNumbers = new List<double>(decimalnumbersLength);
for (var i = 0; i < decimalnumbersLength; i++)
{
    var value = BitConverter.ToDouble(bytes, index);
    index += 8;
    tempDecimalNumbers.Add(value);
}
DecimalNumbers = tempDecimalNumbers;

I couldn't post all the code here, but maybe one of you already sees something I am missing.

Avoid single statement control structures

if (DecimalNumbers == null)
    writer.Write("null");
else

The one real argument in favor of the single statement version of control statements is that they save typing.  However, in this case, it increases the amount of typing, as you have to add special logic to handle the single statement case.

if (DecimalNumbers == null)
{
    writer.Write("null");
}
else

If anyone ever edits the generated code directly, the single statement form can be actively harmful.  Someone not paying attention can add a second statement and not realize that it is not part of a block.  So a statement that is only supposed to run some of the time runs all the time.

Indentation?

if (DecimalNumbers != null)
for(var i = 0; i < DecimalNumbers.Count; i++)
{
    var value = DecimalNumbers[i];
    GeneratorByteConverter.Include(value, bytes, index);;
    index += 8;
}

Shouldn't most of these lines (all but the first) have another level of indent?

Note that this is an example of when the single statement form can be confusing.  It would be more obvious that the for loop was inside a then block if it were in block form.

foreach vs. for

In .NET, which loop runs faster, 'for' or 'foreach'?

Most answers agree that for is faster than the equivalent foreach but not all.  See here.  Note that that answer is newer than others.

There is no reason why a foreach has to be slower than the equivalent for.  The compiler can certainly make that conversion for you.  Even if it is true for some case now, it may not be true in the future.

Another answer also points out that you would gain as much performance by switching from List to an array.

If performance is that important to you, then you should benchmark the various alternatives.  You might even find it useful to have the code generator run benchmarks for you.  Because apparently some situations are optimized and others not.


Custom jQuery Slider Can someone please help to rewrite / tidy up this?

// News Article Slideshow
var periodToChangeSlide = 5000;
var pp_slideshow = undefined;
var currentPage = 0;

$('#news-feature-img-wrap li').css('display', 'list-item').slice(1).css('display', 'none');
$('#news-items li:first').addClass('active');

$("#news-feature-wrap #news-items li").click(function () {
    $(this).parent().addClass('active');
    $(this).parent().siblings().removeClass('active');

    var index = $(this).parent().index();
    var toShow = $("#news-feature-wrap #news-feature-img-wrap li").eq(index);
    toShow.show();
    toShow.siblings().hide();
    currentPage = index;
    $.stopSlideshow();
});

$.startSlideshow = function () {
    if (typeof pp_slideshow == 'undefined') {
        pp_slideshow = setInterval($.startSlideshow, periodToChangeSlide);
    } else {
        $.changePage();
    }
}

$.stopSlideshow = function () {
    clearInterval(pp_slideshow);
    pp_slideshow = undefined;
}
$.changePage = function () {
    var numSlides = $('#news-feature-wrap #news-feature-img-wrap li').length;
    currentPage = (currentPage + 1) % numSlides;
    var menu = $('#news-feature-wrap #news-items li').eq(currentPage);
    menu.addClass('active');
    menu.siblings().removeClass('active');

    var toShow = $("#news-feature-wrap #news-feature-img-wrap li").eq(currentPage);
    toShow.show();
    toShow.siblings().hide();
}

$.startSlideshow();

I've got a few suggestions for you:

Wrap your entire thing in an IIFE, you don't need any external access to any of your vars.

;(function(){
   ...your code goes here
}());

Now your period to change slide isn't in the global name space.

You don't need to attach your startSlideSnow and stopSlideShow to the jQuery namespace (e.g. $.startSlideShow) you are just polluting jQuery's namespace which isn't goot.

Use setTimeout() instead of setInterval.  The setTimeout function will only run after its callback is done, setInterval runs at the interval no matter what, so you can run into some strange behavior.

Try to not repeat selectors, and use chaining:

$(this).parent().addClass('active');
$(this).parent().siblings().removeClass('active');

could become:

$(this).parent().addClass('active').siblings().removeClass('active');

You could optimize your selectors as well, remember jQuery goes right to left, so in a lot of cases you're getting all li items on the page and filtering those.  It might be a marginal speed boost doing $( 'selector' ).find( 'li' );.

There are a few more things you could do, like pluginize it, but these are the biggest things I see right away.


Replace nested property for an interview During an interview I was given this interesting JavaScript task:

Given the code:
const testStr = "bar.baz.foo:111222",
      testObj = {
        bar: {
          baz: {
            foo: 333444,
            foo2: 674654
          },
          boo: {
            faa: 11
          }
        },
        fee: 333
      };

function replaceInObj(obj, str) {
  // your code
}

write the implementation of the replaceInObj function

I was not allowed to use a 3rd-party library. I kinda solved the task, but I'm not satisfied with my solution and I'm struggling with my complex implementation. At the same time I cannot find another solution by myself.
My implementation of the replaceInObj function

const testStr = 'bar.baz.foo:111222';
const testObj = {
  bar: {
    baz: {
      foo: 333444,
      foo2: 674654,
    },
    boo: {
      faa: 11,
    },
  },
  fee: 333,
};

console.log('[before] testObj.bar.baz =', testObj.bar.baz);

// run
replaceInObj(testObj, testStr);

console.log('[after] testObj.bar.baz =', testObj.bar.baz);

function replaceInObj(obj, str) {
  const [path, valueToChange] = str.split(':');

  path.split('.').reduce((acc, prop, index, arr) => {
    const isLastProp = arr.length === index + 1;
    if (isLastProp) {
      acc[prop] = valueToChange;
    }

    return acc[prop];
  }, obj);
}

Neither of your solutions handles the type of the replacement correctly. For instance "bar.baz.foo:111222" replaces foo:333444 with foo:"111222" where I would expect it to result in foo:111222.
Further they can handle if the replacement is an array, but not if it is an object due to the use of split(':'). You should use str.indexOf(':') together with str.substr() instead:
function replaceInObj(obj, str) {
  let splitIndex = str.indexOf(':');
  let [path, replacement] = [str.substr(0, splitIndex), str.substr(splitIndex + 1)];
  let sections = path.split('.');
  let sub = obj;
  for (var i = 0; i < sections.length - 1; i++) {
    sub = sub[sections[i]];
  }

  sub[sections[i]] = JSON.parse(replacement);
  return obj;
}

Here JSON.parse() is used to interpret the value to the correct type. I don't know if the JSON api is considered as "third-party" in this context.

There is a more "quick and dirty" approach using eval:
function replaceInObj(obj, str) {
  let splitIndex = str.indexOf(':');
  let [path, replacement] = [str.substr(0, splitIndex), str.substr(splitIndex + 1)];
  eval("obj." + path + "=" + replacement);
  return obj;
}

Some test cases:

  let testObj = {
    bar: {
      baz: {
        foo: 333444,
        foo2: 674654
      },
      boo: {
        faa: 11
      }
    },
    fee: 333
  };

    let replacements = [
      "bar.baz.foo:111222",
      "bar.baz.foo:\"05-31-2020\"",
      "bar.baz.foo:false",
      "bar.baz.foo:\"abcdefg\"",
      "bar.baz.foo:[111222, 21341234, 234243]",
      "bar.baz.foo:{ \"aaa\": 1234, \"bbb\": \"hello world\" }"
    ];

    for (let replacement of replacements) {
      let result = replaceInObj(testObj, replacement);
      console.log(result.bar.baz.foo);
    }

function replaceInObj(obj, str) {
  let splitIndex = str.indexOf(':');
  let [path, replacement] = [str.substr(0, splitIndex), str.substr(splitIndex + 1)];
  eval("obj." + path + "=" + replacement);
  return obj;
}


PHP OOP Repository overview I already learned JAVA, a few design patterns (Controllers, etc.) and OOP. I'm trying too build some PHP scripts in OOP (a blog). I want to collect some articles from the DB and store in an array. I'm doing this with a repository (like I do this is JAVA). Is this correct?

Artikel.php :

class Artikel {
private $id;
private $naam;

function __construct($id, $naam) {
    $this->id = $id;
    $this->naam = $naam;
}
function getId() {
    return $this->id;
}

function getNaam() {
    return $this->naam;
}

function setId($id) {
    $this->id = $id;
}

function setNaam($naam) {
    $this->naam = $naam;
}
}

ArtikelMapper.php

    require_once '../Classes/Artikel.php';
    class ArtikelMapper {
    private $database;

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

    function getArtikels(){
        /* become a mysql query, this is just for showing */
        $array = array();
        $art1 = new Artikel(1, "name1");
        array_push($array, $art1);
        $art2 = new Artikel(2, "name2");
        array_push($array, $art2);
        return $array;
    }
}

ArtikelRepository.php

require_once '../Classes/Artikel.php';
require_once '../Mappers/ArtikelMapper.php';
class ArtikelRepository {
    private $artikelMapper;
    private $artikelArray;

    function __construct($database) {
        $this->artikelMapper = new ArtikelMapper($database);
        $this->artikelArray = $this->artikelMapper->getArtikels();
    }

    function getArtikels(){
        return $artikelArray;
    }

    function addArtikels($artikel){
        array_push($this->artikelArray, $artikel);
    }

}

At first glance, a couple of things I'd change about your code are:

Follow coding standards, PSR being the most commonly adopted standard out there. Class definitions and method definitions have the opening { on a separate line.
Specify the visibility of properties (as you do) and methods. Relying on the implied public will work for now (might change in the future), but it's best to be explicit: self-documenting code is what you should be aiming for
use type-hints as much as you can. In particular your ArtikelRepository class: the constructor takes a $database argument, but what is it supposed to be? You're not performing any checks to see whether or not the value that was passed was indeed the expected one. I could've passed a string, null or anything in between. Same goes for the addArtikels method, and the ArtikelMapper class.
Method names should make sense. addArtikels is plural, so going by the name, I expect to pass an array of Artikel instances, but you're using array_push on a property that is a 1-dimensional array of Artikel instances. The correct name for that method would be addArtikel.
The ArtikelMapper class should not depend on the DB. It doesn't even look like a mapper. A mapper maps a set of data in a particular format (arrays, class instances, ...) onto another format (eg query results returned as arrays can be mapped onto Artikel instances). That's its job, exposing the DB to this class does not make sense.
This suggests the mapper class you have is violating the Single Responsibility Principle (SRP).
The use of require_once all over the place seems to suggest you're not using an autoloader, which is fine for quick, small tests or proof-of-concept type work, but for actual applications, use namespaces and a PSR compliant autoloader.
Personal preference perhaps: but entity setters are, IMHO, best fluent (return $this;) so they can be used in a single statement without having to repeat the variable to which you've assigned the instance.

Basically, here's how I'd write the Artikel entity:

class Artikel
{//new line
    /**
     * @var int
     */
    private $id;

    /**
     * @var string
     */
    private $naam;

    /**
     * @param int $id
     * @param string $naam
     */
    public function __construct($id, $naam)
    {//new line
        $this->id = $id;
        $this->naam = $naam;
    }

    /**
     * @return int
     */
    public function getId()
    {
        return $this->id;
    }

    /**
     * @return string
     */
    public function getNaam()
    {
        return $this->naam;
    }

    /**
     * @param int $id
     * @return $this
     */
    public function setId($id)
    {
        $this->id = $id;
        return $this;
    }

    /**
     * @param string $naam
     * @return $this
     */
    public function setNaam($naam)
    {
        $this->naam = $naam;
        return $this;
    }
}

The setters could perform some basic validation on the arguments passed (eg make sure $id is an integer), in which case, the constructor should not assign the properties directly, but rather call the setters, too.

Along with the (mainly style) changes to the entity class, I'd refactor the repository class, too:

class ArtikelRepository
{
    /**
     * @var ArtikelMapper
     */
    private $artikelMapper;

    /**
     * @var Database <-- what class is this?
     */
    private $database;

    /**
     * @var Artikel[]
     */
    private $artikelArray = [];//initialize to array already

    /**
     * Do NOT create depenendencies in constructors, INJECT dependencies!
     * In a true Respositor, the database connection is a dependency of the Repository, not the mapper
     * @param Database $database
     * @param ArtikelMapper $mapper
     */
    function __construct(Database $database, ArtikelMapper $mapper)
    {
        $this->database = $database;
        $this->artikelMapper = new ArtikelMapper($database);
        //Don't use a constructor to query the DB, only load data if we need it
        //$this->artikelArray = $this->artikelMapper->getArtikels();
    }

    public function getArtikels()
    {
        if (!$this->artikelArray) {
            $results = $this->database->getArtikels();//load from DB
            //use mapper to convert results into Artikel instances
            $this->artikelArray = $this->artikelMapper->mapResults($results);
        }
        return $this->artikelArray;
    }

    /**
     * @param Artikel $artikel
     * @return $this <-- fluent interface
     */
    function addArtikel(Artikel $artikel)
    {
        if (!$this->artikelArray) {
            $this->getArtikels();//initialize artikels if needed
        }
        $this->artikelArray[] = $artikel;//don't use array_push
        return $this;
    }
}

I don't know what it is you're trying to do exactly, but I'm not sure why you're actually loading all existing artikel instances and assigning them to your repository property. A repository fetches data from a persistence layer in a format that the rest of the application understands. Nothing more, or less. You can add some caching mechanisms to avoid hitting the DB for data you already loaded, but in that case, it'd make more sense to use $artikelArray as an associative array (using the ID as key). To be honest, I wouldn't bother with that property at all for now, and perhaps consider adding a specialized caching layer later on.


Fast prototype code to hook up a webcam to the Azure Emotion API Last week I had a 5 hours school project deadline based on the Azure Emotion API. Right now I am trying to rewrite the same code in a more clean/elegant way. I am quite new to coding so I will need all the help I can get. I am not really looking for a shorter way to write the same code, but I am looking for a clean/understandable way.

(function () {
  let tempEmotion = ''
  let lastEmotion = ''

  function ShowCam () {
    Webcam.set({
      width: 480,
      height: 360,
      image_format: 'jpeg',
      jpeg_quality: 100
    })
    Webcam.attach('#myCamera')
  }

  function TakeSnapshot () {
    Webcam.snap(function (dataUri) {
      document.getElementById('results').innerHTML = '<img id="base64image" src="' + dataUri + '"/>'
    })
    SaveSnapshot()
  }

  function SaveSnapshot () {
    let file = document.getElementById('base64image').src.substring(23).replace(' ', '+')
    let img = Base64Binary.decodeArrayBuffer(file)
    let ajax = new XMLHttpRequest()

    ajax.addEventListener('load', function (event) {
      UploadComplete(event)
    }, false)
    ajax.open('POST', 'https://api.projectoxford.ai/emotion/v1.0/recognize', 'image/jpg')
    ajax.setRequestHeader('Content-Type', 'application/octet-stream')
    ajax.setRequestHeader('Accept', 'text/html,application/xhtml+xml,application/xml')
    ajax.setRequestHeader('Ocp-Apim-Subscription-Key', 'EXTREMELY_HIDDEN_KEY')
    ajax.send(img)
  }

  function UploadComplete (event) {
    let xmlDoc = event.target.responseXML
    let result = xmlDoc.getElementsByTagName('scores')
    GiveUserFeedback(result)
  }

  function GiveUserFeedback (result) {
    let emotionArray = []
    let tempValue = 0
    let htmlBody = $('body')
    if (result[0]) {
      for (let i = 0; i < result[0].childNodes.length; i++) {
        emotionArray.push(Math.floor((Number(result[0].childNodes[i].textContent) * 100) + 1))
        if (Number(result[0].childNodes[i].textContent).toFixed(2) > tempValue) {
          tempValue = Number(result[0].childNodes[i].textContent).toFixed(2)
          tempEmotion = String(result[0].childNodes[i].nodeName)
          tempEmotion = tempEmotion.substr(0, 1).toUpperCase() + tempEmotion.substr(1)
        }
      }
      for (let i = 0; i < emotionArray.length; i++) {
        $('ul li:nth-child(' + (i + 1) + ') span').css('height', emotionArray[i])
      }
      switch (tempEmotion) {
        case 'Angry':
          htmlBody.css('background-color', '#F44336')
          htmlBody.css('color', '#FFFFFF')
          $('span').css('background-color', '#FFFFFF')
          break
        case 'Contempt':
          htmlBody.css('background-color', '#9C27B0')
          htmlBody.css('color', '#FFFFFF')
          $('span').css('background-color', '#FFFFFF')
          break
        case 'Disgust':
          htmlBody.css('background-color', '#673AB7')
          htmlBody.css('color', '#FFFFFF')
          $('span').css('background-color', '#FFFFFF')
          break
        case 'Fear':
          htmlBody.css('background-color', '#FF9800')
          break
        case 'Happiness':
          htmlBody.css('background-color', '#4CAF50')
          break
        case 'Neutral':
          htmlBody.css('background-color', '#FFFFFF')
          break
        case 'Sadness':
          htmlBody.css('background-color', '#03A9F4')
          break
        case 'Surprise':
          htmlBody.css('background-color', '#FFEB3B')
      }
      lastEmotion = tempEmotion
    }
  }

  window.onload = ShowCam
  window.setInterval(function () {
    TakeSnapshot()
  }, 3000)
})()

Dealing with DOM

document.getElement[s]By*(...) competes with $(...). Do not mix DOM accessing styles. That hurts readability. Either use jQuery everywhere, or native API everywhere. jQuery is kind of out of fashion nowadays, mainly because major browsers got much better in exposing DOM accessing APIs.

Dealing with Http

Again, since your code already depends on jQuery, I don't see why prefer XMLHttpRequest over $.ajax(). Inconsistency of the code is one of the biggest issues when it comes to maintenance, especially in case of new hires.

Separate Data from Logic

Your mood to color scheme relationship is data that can be encapsulated in a separate place (apart from the places where it's being used). Say, you define color scheme like this:

  var schemeOfMood = {
      'Angry': { htmlBackground: '#F44336', htmlColor: '#FFFFFF', spanBackground: '#FFFFFF' },
      'Contempt': { htmlBackground: '#9C27B0', htmlColor: '#FFFFFF', spanBackground: '#FFFFFF' },
      // ...
  };

then the coloration logic becomes trivial, and no ugly switch is needed at all.

  var scheme = schemeOfMood[tempEmotion];
  htmlBody.css('background-color', scheme.htmlBackground);
  htmlBody.css('color', scheme.htmlColor);
  $('span').css('background-color', scheme.spanBackground);

The other good effect is the reduced code repetition.

Move Magic Code into Functions

When I see lines like that they look Magic to me.

let file = document.getElementById('base64image').src.substring(23).replace(' ', '+')

What is subsctring(23)? Why 23 and not 32 (I like powers of 2 more) -- "magic numbers"? Why replace ' ' with '+'?

If you extract this one-liner into a function, you can give it a good name, thus expressing your intent. At the moment, I didn't even try to understand why that manual transformation of a base64 image is needed.

Unclear Intent

When giving user a feedback, there's this if (result[0]) { ... } that makes the code look only into the first result. Why is that?

Put it into a variable, give it a name, and you will avoid the hard-to-read result[0] and explain what's going on.

var lastChronologicalResult = result[0];
if (lastChronologicalResult) {
  // work against lastChronologicalResult
}

Very similarly, the code loops through multiple results' child nodes (for (let i = 0; i < result[0].childNodes.length; i++) { ... }) and does two things:

A) populates emotionArray based on each child;

B) calculates the tempEmotion based on the last child which has .textContent.toFixed(2) > tempValue. Nobody knows what textContain holds and what's the tempValue.

So, better naming things and function extraction would clarify what is being done in that part of the code.


Am I using Angular directives correctly? I'm coming from Backbone and trying to wrap my head around Angular for a new project and would like to get some feedback on whether I'm using it correctly or not.

Plunker

What I'm trying to accomplish is this: I have a list of items that can be very easily rendered with an ngRepeat. Each item starts off in an inactive state. When a user selects one of the items it enters into an expanded active state that shows some extra details and controls for working with the currently selected item. The markup for these two states is different enough that adding & removing some CSS classes won't cut it, so I have separate templates for each state.

The approach I've taken is encapsulating this functionality into a directive that can render the two templates. When the item is initially rendered the inactive template is used. When an item is selected (in the example, by checking a checkbox) then the active template is compiled and the resulting markup put into the element's HTML. When the checkbox is unchecked the same happens for the inactive template.

The ngShow/ngHide directives won't work for me in this case because there could potentially be lots of these things on the page and my understanding is that ngShow/ngHide will render the markup for both states and then show/hide based on some model property. Ultimately only one item will be active at any given time so rendering all that markup for each item only to display one at a time seems wasteful. I also wanted more control over the transition between the inactive & active states.

Is this solution something that would be considered idiomatic Angular or an egregious abuse of directives? Am I exposing myself to any bugs?

The directive:

module.directive("myDirective", ['$compile', '$http', '$templateCache', function($compile, $http, $templateCache) {

  function getTemplate(isActive) {
    var templateLoader,
        baseUrl = 'templates/',
        templateMap = {
          active: 'checked.html',
          inactive: 'unchecked.html'
        },
        templateUrl = baseUrl + templateMap[isActive ? 'active' : 'inactive'];

    return $http.get(templateUrl, { cache: $templateCache });
  }

  function render(scope, element) {
    getTemplate(scope.isActive)
      .success(function(html) {
        element.hide().html(html);
      })
      .then(function(response) {
        element.html($compile(element.html())(scope)).fadeIn();
      });
  }

  function linker(scope, element, attrs) {
    render(scope, element);

    scope.onSelected = function() {
      render(scope, element);
    };
  }

  function controller($scope) {
    $scope.isActive = false;
  }

  return {
    restrict: 'A',
    scope: {
      item: '='
    },
    controller: controller,
    link: linker
  };

}]);

The active template:

{{item.description}}

{{item.details}}

  <input type="checkbox" ng-model="isActive" ng-change="onSelected()">

The inactive template:

  <input type="checkbox" ng-model="isActive" ng-change="onSelected()"> {{item.description}}

And the view:

If the only reason is to avoid a few more DOM elements, this sounds like an overkill to me. It is so much easier to use ng-show or hiding/showing depending on the class like it is done in TodoMVC:

    ...

 ...

    <input class="edit"> ...

Then in CSS:

#todo-list li.editing .view {
    display: none;
}
#todo-list li.editing .edit {
    display: block;
}

Also it does not feel right to do DOM manipulation with .html() jQuery-style, maybe use dynamic template property instead.

If having additional elements in DOM is really a concern,
there are also ng-if and ng-switch.
Though I personally prefer ng-show/ng-hide exactly because
this way I don't have to rebuild the DOM.

To put this into perspective, I have tried using ngRouter on my site 33hotels.com, which does rebuild the view every time the route is changed. That felt in considerable delay. Instead I have all my Views inside my DOM at all times and only hide/show them depending on the route. The performance difference is dramatic - it now feels instant.

Finally, both templates have common fragment, which does not make code feel DRY. Upon any change you'll have to edit each of them. I would simply keep the common part outside any directive, and put the dynamic part inside ng-whatever.

Also note that the HTML tag
 has semantic meaning, not presentational. That is, it marks your main header. Any presentation adjustment should be done via CSS.


Externalize to a function I have the following code block which I'd like to refactor into a method, I just don't see a good way to get the following code into a method which doesn't have multiple responsibilities.  Here is the code block:

int maxResults = pageable.getMaxResults(userPrefs);
int firstResult;

if (pageData.isLastPage()) {
    int count = getCountFromDb(); //'Heavy' cost associated with this, so we only
                                  //determine the count if they are on the last page...
    firstResult = getFirstResult(count, maxResults);
    if (count > 0) {
        boolean isFinalPageFull = (count % maxResults) == 0;
        int pageNum = count / maxResults;
        pageData.setPageNumber((isFinalPage) ? pageNum - 1 : pageNum;
    }
} else {
    firstResult = maxResults * pageNumber;
}

I'd like to move this out into a method or two, but I need to  only do this count one time.  This limitation is clouding my vision, as I can't think of any function which follows the Single Responsibility Principle.  Anything I can do to achieve my goal?

You could make count a field, then have a function to determineFirstResult().  Then the setPageNumber block can be separated out.

if (pageData.isLastPage()) {
    count = getCountFromDb();
    if (count > 0) {
        boolean isFinalPageFull = (count % maxResults) == 0;
        int pageNum = count / maxResults;
        pageData.setPageNumber((isFinalPage) ? pageNum - 1 : pageNum;
    }
}
firstResult = determineFirstResult();

...

int determineFirstResult(int maxResults) {
    if (pageData.isLastPage()) return getFirstResult(count, maxResults);
    return maxResults * pageNumber;
}

But now you've got a dependency; count has to be determined before calling determineFirstResult() as written.  So maybe make count a lazy-loaded field; if it's requested and null, then (and only then) do you getCountFromDb.


Error handling for an Erlang 2D point class I am working on a math library for an online game. I want to prevent as many errors as possible and I want to catch all errors as early as possible to simplify debugging in the long run. However, I feel like I drown in details. This is quite different from "let it crash" motto.

point has {number(), number()} type or better say {0, 0} is a point. I started with Dialyzer specs and guard statements which work as a poor man's assertions.

Here is my point class.

-module(point).
-author("nt").

-export([is_point/1, distance/2, translate/2, pointToMap/1]).

-type point() :: {number(), number()}.
-export_type([point/0]).

is_point({X, Y}) when is_number(X), is_number(Y) -> true;
is_point(_) -> false.

distance({X1, Y1} = A, {X2, Y2} = B) ->
  case is_point(A) of false -> error(badarg); _ -> ok end,
  case is_point(B) of false -> error(badarg); _ -> ok end,
  math:sqrt(math:pow((X2 - X1), 2) + math:pow(Y2 - Y1, 2)).

translate({X1, Y1} = A, {X2, Y2} = B) ->
  case is_point(A) of false -> error(badarg); _ -> ok end,
  case is_point(B) of false -> error(badarg); _ -> ok end,
  {X1 + X2, Y1 + Y2}.

pointToMap({X, Y} = A) ->
  case is_point(A) of false -> error(badarg); _ -> ok end,
  #{
    x => X,
    y => Y
  }.

%% Spec
-spec is_point(P :: point()) -> boolean().
-spec distance(A :: point(), B :: point()) -> float().
-spec translate(A :: point(), B :: point()) -> point().
-spec pointToMap(A :: point()) -> #{x:= number(), y := number()}.

{{0, 0}, {0, 0}} is a rect.
rect "class" relies on point.

-module(rect).
-author("nt").

%% API
-export([is_rect/1, contains/2]).

-type rect() :: {point:point(), point:point()}.
-export_type([rect/0]).

is_rect({{OriginX, OriginY}, {W, H}}) when is_number(OriginX), is_number(OriginY), is_number(W), is_number(H) -> true;
is_rect(_) -> false.

-spec contains(Rect, Point) -> boolean() when Rect :: rect(), Point :: point:point().
contains({{OriginX, OriginY}, {W, H}} = R, {X, Y} = P) ->
  case is_rect(R) of false -> error(badarg); _ -> ok end,
  case point:is_point(P) of false -> error(badarg); _ -> ok end,
  ((OriginX < X) and (X < (OriginX + W))) and ((OriginY < Y) and (Y < (OriginY + H))).

Things are getting really massive and daunting. Given the fact there is almost no business logic I wonder how fast codebase will become a complete mess if I continue to check all the required preconditions in all functions.

Update:

I think about removing all the specs and "assertions" from the code. Functions like pointToMap will be also removed as I don't need to rely on concrete types anymore. Something like tupleToMap should be used instead.

point.erl:

-module(point).
-author("nt").

-export([distance/2, translate/2]).

distance({X1, Y1}, {X2, Y2}) ->
  math:sqrt(math:pow((X2 - X1), 2) + math:pow(Y2 - Y1, 2)).

translate({X1, Y1}, {X2, Y2}) ->
  {X1 + X2, Y1 + Y2}.

rect.erl

-module(rect).
-author("nt").

-export([contains/2]).

contains({{OriginX, OriginY}, {W, H}}, {X, Y}) ->
  ((OriginX < X) and (X < (OriginX + W))) and ((OriginY < Y) and (Y < (OriginY + H))).

I think the concerns about redundancy and verbosity are probably well placed.
andalso
The short circuiting logical AND is andalso. It can be used to simplify the code:
is_point({X, Y}) -> is_number(X) andalso is_number(Y).

Generally, the pattern:
// pseudo code
if f(x)
  return true
else
  return false

can be replaced with return f(x). Or in languages like Erlang where everything is an expression, f(x) alone is sufficient.
Atoms
In Erlang, it is idiomatic to use atoms as the first level of data validation:
is_point({point, X, Y}) -> is_number(X) andalso is_number(Y).

More importantly, it aids in readability and debugging. Atoms let points be distinguished from football scores:
1> point:is_point({point, 3, 2}).
true
2> point:is_point({football_score, 3, 2}).
** exception error: no function clause matching
point:is_point({football_score,3,2})
(/erlang/point.erl line 4)

Note that using atoms and good function names provides most of what we want to start debugging this type of problem. Football scores are not points and "letting it crash" provides better information than badarg.
Message passing
Labeling data with atoms lets us distinguish {rhombus, P1, P2} from {rectangle, P1, P2} and {circle, P1, P2}.
Try-catch
Taking error handling code out of point:is_point is consistent with the "unwritten code is bug free" heuristic. If crashes are problematic, the caller should wrap the call to point:is_point/1 in a try-catch-finally block. The caller can handle the exception at whatever granularity it needs. And we can avoid the close coupling that coordinated error handling requires whenever the defaults are good enough.


Android Java Splash Screen code Here's my android splash screen. I am trying to figure out a better way to write it. It just moves the logo up to the center of the screen. It's my third time creating a splash screen for an android app. I am just trying to figure out the best way to write more optimized code.

import androidx.appcompat.app.AppCompatActivity;

import android.content.Intent;
import android.os.Bundle;
import android.os.Handler;
import android.view.animation.Animation;
import android.view.animation.AnimationUtils;
import android.widget.ImageView;

final public class Splash extends AppCompatActivity {
    private final static int timeout = 3000;
    final static Handler handler = new Handler();
    static ImageView logo;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);
        logo = findViewById(R.id.companylogo);
        Animation animation = AnimationUtils.loadAnimation(Splash.this, R.anim.myanim);
        logo.startAnimation(animation);

        handler.postDelayed(new Runnable() {
            @Override
            public void run() {
                Intent splashscreen = new Intent(Splash.this, Home.class);
                startActivity(splashscreen);
            }

        }, timeout);

    }

}

This code looks reasonably clean and seems to accomplish what it's intended to do.
Some minor nitpicks can still be improved here and there:

You're not consistent in visibility modifier ordering between class declaration and member declarations. It should either be public final class or final private static int timeout.
Most Java conventions that I've seen default to the public static final ordering.
handler and logo are currently package-private. It's very unusual for that visibility to be necessary and I think setting these to private if at all possible would be better. (I'm not recommending protected, because the class can not be extended.)
static final members are usually named in SNAKE_CAPS (also known as shout-case).
the id companylogo might be better off named companyLogo to follow camel case conventions.
myanimation doesn't tell us anything about the animation. A better name would be splashScreenAnimation or logoAnimation


Handy tool that provides easy syntax for specifying multi-dimensional types in C++ The syntax for specifying multi-dimensional types in C++ is messy. For example, to specify a two-dimensional std::vector<int>, one has to write std::vector<std::vector<int>>. The syntax doesn't scale. So, I wrote a handy tool that provides easy syntax for this. Any kinds of comments are welcome ^_^ (original version of the code can be found here with unit test code here)

template <template <typename...> class T, typename Base, std::size_t n>
struct multi {
  static_assert(n > 0, "");
  using type = T<typename multi<T, Base, n - 1>::type>;
};

template <template <typename...> class T, typename Base>
struct multi<T, Base, 1> {
  using type = T<Base>;
};

/// Provides easy syntax for specifying multi-dimensional types.
/// For example, `multi_t<std::vector, int, 2>` is equivalent to
/// `std::vector<std::vector<int>>`.
template <template <typename...> class T, typename Base, std::size_t n>
using multi_t = typename multi<T, Base, n>::type;

There's so little code here (and none of it anything that actually executes, just typedefs) that it's difficult to review the code as a thing in itself.

That leaves only the question of whether the basic idea of the code is a good one. At least in my opinion, the answer to that is a (heavily qualified) "no", at least as a general rule.

If you really need the equivalent of a muti-dimensional array, nested containers are rarely the best way to achieve that. Instead, you're usually better off with a single container and multi-dimensional indexing into that single container.

There are a couple of different ways of providing that. One is to overload operator() for the correct number of parameters (and optionally provide overloads for fewer parameters, which return entire collections rather than a single element). Another is to use multiple overloads of operator[], so indexing looks like a multi-dimensional array in C (e.g., x[i][j][k]), but the higher-order operators return proxy objects, so [i][j][k] ends up doing the multiplication necessary to find the correct element in the collection.

Relatively straightforward code for that (supporting only 3 dimensions) could look something like this:

#include <vector>
#include <iostream>

template<class T>
class matrix3 {
    size_t a, b, c;
    std::vector<T> data;

    friend class proxy;
    friend class proxy2;

    class proxy {
        matrix3 &m_;
        int index1_, index2_;
    public:
        proxy(matrix3 &m, int i1, int i2)
            : m_(m), index1_(i1), index2_(i2) { }

        T &operator[](int index3) {
            return m_.data[index1_ * m_.b * m_.c + index2_ * m_.c + index3];
        }
    };

    class proxy2 {
        matrix3 &m_;
        int index_;
    public:
        proxy2(matrix3 &m, int d) : m_(m), index_(d) { }

        proxy operator[](int index2) {
            return proxy(m_, index_, index2);
        }
    };
public:
    matrix3(size_t a, size_t b, size_t c) : a(a), b(b), c(c), data(a * b * c) { }

    proxy2 operator[](int index) {
        return proxy2(*this, index);
    }
};

Simple demo code:

int main() {
    matrix3<double> m(3, 3, 2);

    for (int x = 0; x < 3; x++)
        for (int y = 0; y < 3; y++)
            for (int z = 0; z < 2; z++)
                m[x][y][z] = x * 100 + y * 10 + z;

    for (int x = 0; x < 3; x++) {
        for (int y = 0; y < 3; y++) {
            for (int z = 0; z < 2; z++)
                std::cout << m[x][y][z] << "\t";
            std::cout << "\n";
        }
        std::cout << "\n\n";
    }
}

I haven't tried to work through really doing it, but I believe you should be able to shorten (and possibly even simplify) that by defining the proxy classes using a non-type parameter for the stored index, and recursion to a proxy<n-1>, where n is the dimension. Likewise, you'd need to define the operator[] to multiply and accumulate across N dimensions instead of the fixed number employed here.

This gives a number of advantages. First, improved locality of reference--all the data is allocated and accessed as a single, contiguous block. Second, reduce memory access--instead of a pointer to a pointer to a pointer to a block of data, you have only a single level of pointer to access, regardless of the number of dimensions. You do have to use some multiplications to avoid those memory accesses, but on a modern CPU that's a really good trade off.

Hope I haven't gotten too far afield from the original topic--this is quite a bit different code (really code for the entirety of a multi-dimensional array, not just typedefs for declaring/defining one), but it seems pretty obvious that if you're doing to declare/define one, you also want to use it...


regular expression for telephone/mobile per country and variable naming would like to know if there is a better approach in doing a telephone/mobile number regular expression for specific country. this includes naming variables like how to know if format is international format (is it with + sign?), a national format (is it the area code enclosed in parenthesis) or a local format (no + sign, and no area code), currently they are labelled as country_name_telephone/country_name_mobile or country_name_telephone_N_digit, etc...

below are the regular expression for some country.

/** matches the following pattern:
 * - 01-111-11-11
 * - 01 111 11 11
 * - 011-11-11-11
 * - 011 11 11 11
 * - 011111111
 */
public final static String CTRY_BELGIUM_TELEPHONE = "^0(\\d{8}|\\d\\s\\d{3}\\s\\d\\d\\s\\d\\d|\\d-\\d{3}-\\d\\d-\\d\\d|\\d\\d\\s\\d\\d\\s\\d\\d\\s\\d\\d|\\d\\d-\\d\\d-\\d\\d-\\d\\d)$";

/** matches the following pattern:
 * - 0412-34-56-78
 * - 0412 34 56 78
 * - 0412-345-678
 * - 0412 345 678
 * - 0412345678
 */
public final static String CTRY_BELGIUM_MOBILE = "^04\\d\\d(\\d{6}|\\s\\d\\d\\s\\d\\d\\s\\d\\d|\\s\\d{3}\\s\\d{3}|-\\d\\d-\\d\\d-\\d\\d|-\\d{3}-\\d{3})$";

/** matches the following pattern:
 * - 0000-0000
 * - 0000 0000
 * - 00000000
 */
public final static String CTRY_HONG_KONG_TELEPHONE = "^(\\d{4}[-\\s]?\\d{4})$";

/** matches the following pattern:
 * - +852-0000-0000
 * - +852 0000 0000
 * - +85200000000
 */
public final static String CTRY_HONG_KONG_MOBILE = "^\\+852(\\d{8}|-\\d{4}-\\d{4}|\\s\\d{4}\\s\\d{4})$";

/** matches the following pattern:
 * - 212-0000
 * - 212 0000
 * - 2120000
 */
public final static String CTRY_UNITED_STATES_TELEPHONE_7_DIGIT = "^[2-9]((?!11)\\d{2})[-\\s]*\\d{4}$";

/** matches the following pattern:
 * - 200 212 0000
 * - 200-212-0000
 * - 2002120000
 */
public final static String CTRY_UNITED_STATES_TELEPHONE_10_DIGIT = "^[2-9]\\d{2}((-[2-9]((?!11)\\d{2})-)|(\\s[2-9]((?!11)\\d{2})\\s)|([2-9]((?!11)\\d{2})))\\d{4}$";

/** matches the following pattern:
 * - all matches in CTRY_UNITED_STATES_TELEPHONE_7_DIGIT and CTRY_UNITED_STATES_TELEPHONE_10_DIGIT
 */
public final static String CTRY_UNITED_STATES_TELEPHONE = "("+CTRY_UNITED_STATES_TELEPHONE_7_DIGIT+")|("+CTRY_UNITED_STATES_TELEPHONE_10_DIGIT+")";

/** matches the following pattern:
 * - +1 200 212 0000
 * - +1-200-212-0000
 * - +12002120000
 */
public final static String CTRY_UNITED_STATES_MOBILE = "^\\+1((-[2-9]\\d{2}-[2-9]((?!11)\\d{2})-)|(\\s[2-9]\\d{2}\\s[2-9]((?!11)\\d{2})\\s)|([2-9]\\d{2}[2-9]((?!11)\\d{2})))\\d{4}$";

For CTRY_BELGIUM_TELEPHONE:

First, to understand what the regex is doing, I'll add the railroad diagram

Demo on RegEx101

What it matches:

Zero followed by

Eight Digits
Digit-Space-Three_Digits-Space-Two_Digits-Space-Two_Digits
Digit-Hyphen-Three_Digits-Hyphen-Two_Digits-Hyphen-Two_Digits
Two_Digits-Space-Two_Digits-Space-Two_Digits-Space-Two_Digits
Two_Digits-Hyphen-Two_Digits-Hyphen-Two_Digits-Hyphen-Two_Digits

Now, looking at above, we can easily spot that many of the things are use multiple times in the same sub-pattern.

Can we combine/reuse them?

For simplicity, I'll use single backslash from here onward. You need to escape it by prepending another backslash to escape it in string.

When I say nth road, it'll refer to the nth path in the above diagram.

Review:

The first road is straight forward. Match eight digits. Simple!

The second road, \d\s\d{3}\s\d\d\s\d\d. As you've used \d{3}, similarly we can write \d\d as \d{2}. Cool!

It'll now become \d\s\d{3}\s\d{2}\s\d{2}.

The third road similarly can be written as \d-\d{3}-\d{2}-\d{2}.

For fourth and fifth road, same can be used.

Second and Third roads are similar except the separator used between digits. Is there any way to combine them?

Back-reference.

How can back-reference be used here?

I'll show you: The character should be captured in capturing group and refer it with back-reference whenever needed.

\d([\s-])\d{3}\1\d{2}\1\d{2}

The regex here can be used to match both second and third road.

\d: Match a digit
([\s-]): Match either a space character or hyphen and add it into first capturing group
\d{n}: Match digits n times.
\1: Match whatever is in first captured group(see #2 above)

There still is repetition in \1\d{2}\1\d{2}. This can be added in group and repeated twice (?:\1\d{2}). Since, back-reference is used, it's not better to use non-capturing group.(You'll know why in the 4th and 5th road).

Similarly, for fourth and fifth road, the regex will be

\d{2}([\s-])\d{2}(?:\1\d{2}){2}

At this stage, the regex will become

^0(?:\d{8}|\d([\s-]?)\d{3}(?:\1\d{2}){2}|\d{2}([\s-])\d{2}(?:\2\d{2}){2})$

The first part \d{8} can also be removed by using ? quantifier on the first captured group. By doing this, we're making the [\s-] optional and thus \1 will be empty and will match eight digits with no spaces between them.

So, Final RegEx is

/^0(?:\d([\s-]?)\d{3}(?:\1\d{2}){2}|\d{2}([\s-])\d{2}(?:\2\d{2}){2})$/

Here's railroad diagram for this

Note that 1 and 2 in the diagram refer to respective capturing group.

Demo on RegEx101

Although, this answer only explains about one RegEx, other RegExes can be modified in the similar way.


Finding many paths in a graph I have a graph which we can think of is representing a network of train tracks. Each node is a station and each edge is a piece of track connecting two stations. Each station node may be connected by more than one track and all tracks are one way (i.e the graph is directed).

My job is to find as many routes as possible connecting one node with a set of destination nodes. Routes may contain cycles to the train track analogy breaks down here, but whatever. I have Python code to perform this task, but it is to slow so I would love to get some help in making it faster. Below is my traversal function:

from collections import deque
from itertools import product

def travel(graph, start, dests, count):
    queue = deque([[(start, None)]])
    paths = []
    while queue:
        path = queue.popleft()
        at = path[-1][0]
        if at in dests:
            pp = [p[1] for p in path[1:]]
            for real_path in product(*pp):
                paths.append(''.join(real_path))
                count -= 1
                if count == 0:
                    return paths
        adj = graph.get(at, {})
        for item in adj.items():
            queue.append(path + [item])
    return paths

Called with a sample graph:

G = {
    1 : {2 : 'mqr', 3 : 't'},
    2 : {3 : 'q'},
    3 : {1 : 'S', 4 : '_'},
    4 : {3 : 't', 1 : 'xUI', 5 : '=+/'},
    5 : {4 : 'q', 6 : '1'},
    6 : {1 : '0'}
}
print(len(set(travel(G, 1, [6], 1000))))

1000 unique strings representing paths through the graph will be generated. Ideas on how to improve the travel function would be greatly appreciated. Also on the format I'm using for representing the graph. It is usually very efficient to store a graph as a dict whose values are themselves dicts, but perhaps there is a better format.

There's no docstring. What does travel do? What arguments does it take? What does it return?
The meaning of the data structure queue is far from clear. It seems to be a deque whose elements represent paths, each path being represented by a list of pairs (v, e) where v is the vertex on the path, and e is None if this is the first vertex on the path, or an iterable of edges by which v could be reached from the previous vertex of the path. This kind of complex data structure is hard to understand, and would be clearer if it had its own class, for example using collections.namedtuple.
It would be a good idea to generate the paths one at a time using the yield statement, so that if a consumer is processing the paths one at a time then the whole list of paths does not have to be stored in memory.
Generating the paths would also allow us to omit the count argument and keep generating paths forever. If the caller wants at most 1000 paths, they can use itertools.islice:

islice(travel(graph, start, dests), 1000)

The test if at in dests: takes time proportional to the number of destinations. This would run in constant time if dests were converted to a set.
It would be reasonable to insist on the graph having a key for every vertex (whose corresponding value is an empty dictionary if the vertex is a sink); this would allow you to write graph[at] instead of graph.get(at, {}) which creates and throws away an empty dictionary.
The implementation stores path + [item] in the queue, taking time and space proportional to the length of the path, on each iteration. This is unnecessary: many paths will share the same initial prefix, so a data structure which stored only the last item on the path, together with a reference to the previous data structure on the path, would take constant time on each iteration. The whole path could be reconstructed when the destination is reached by following the references.

2. Revised code

from collections import deque, namedtuple

# Node in the breadth-first search with fields:
# vertex -- the current vertex in the search
# edges -- iterable of edges from previous vertex (or None)
# prev -- previous Search node (or None)
SearchNode = namedtuple('SearchNode', 'vertex edges prev')

def travel(graph, start, dests):
    """Generate paths in graph beginning at the vertex start and ending at
    any of the vertices in dests.

    The graph must be represented as a mapping from vertex to a
    mapping from neighbouring vertex to an iterable of edges.

    """
    dests = set(dests)
    queue = deque([SearchNode(start, None, None)])
    while queue:
        at = queue.popleft()
        if at.vertex in dests:
            edges = []
            prev = at
            while prev.edges is not None:
                edges.append(prev.edges)
                prev = prev.prev
            for path in product(*reversed(edges)):
                yield ''.join(path)
        for v, e in graph[at.vertex].items():
            queue.append(SearchNode(v, e, at))