Reading in a file and ranking result to output file I am reading in a file which consists of scores between two teams.  The team that wins gets 2 points, if the teams tie both teams get one point.  The result is ranked by points and if there is a tie ranked by alphabetic order.  Then this is written to an output file.  Any suggestions to improve the code?

Input file:
Blues 3, Coyotes 0

Output
Blues 2 pts
Coyotes 0 pts

using System;
using System.Collections.Generic;
using System.Linq;

namespace skateboard
{
    class MainClass
    {
        //If team does not exists add to library and award 0 points
        static void DoesTeamExist(Dictionary<string, int> dictionary, string team)
        {
            if (!dictionary.ContainsKey(team))
            {
                dictionary.Add(team, 0);
            }
        }

        //If draw add one point to each team, else awards the winner 2 points
        static void AwardPoints(Dictionary<string, int> dictionary, string teamOne, string teamTwo, int teamOneScore, int teamTwoScore)
        {
            if (teamOneScore == teamTwoScore)
            {
                dictionary[teamOne] += 1;
                dictionary[teamTwo] += 1;
            }
            else if (teamOneScore > teamTwoScore)
            {
                dictionary[teamOne] += 2;
            }
            else
            {
                dictionary[teamTwo] += 2;
            }
        }

        public static void Main(string[] args)
        {

            int counter = 0;
            string line;
            System.IO.StreamReader file =
                new System.IO.StreamReader(@"/Users/aaronmk2/Projects/skateboard/skateboard/sample-input.txt");
            Dictionary<string, int> dictionary = new Dictionary<string, int>();

            while ((line = file.ReadLine()) != null)
            {
                string []lines = line.Split(',');
                string teamOne = lines[0].Substring(0, lines[0].Length - 2);
                int teamOneScore = Convert.ToInt32(lines[0][lines[0].Length - 1] - '0');
                DoesTeamExist(dictionary, teamOne);
                string teamTwo = lines[1].Substring(1, lines[1].Length - 2);
                int teamTwoScore = Convert.ToInt32(lines[1][lines[1].Length - 1] - '0');
                DoesTeamExist(dictionary, teamTwo);
                AwardPoints(dictionary, teamOne, teamTwo, teamOneScore, teamTwoScore);
                counter++;
            }

            file.Close();

            var items = from pair in dictionary
                orderby pair.Value descending, pair.Key ascending
                select pair;

            System.IO.StreamWriter write = new System.IO.StreamWriter(@"/Users/aaronmk2/Projects/skateboard/skateboard/test-output.txt");

            // Display results.
            foreach (KeyValuePair<string, int> pair in items)
            {
                Console.WriteLine("{0}: {1}", pair.Key, pair.Value);
                if (pair.Value == 1)
                {
                    write.WriteLine("{0}: {1} pt", pair.Key, pair.Value);
                }
                else
                {
                    write.WriteLine("{0}: {1} pts", pair.Key, pair.Value);
                }

            }
            write.Close();
        }
    }
}

In this console application it might not be that important but later you should always take care of any resources and dispose them propertly. Closing it isn't enough so you should wrap the streams in usings:

using (var file = new StreamReader(@"/Users/aaronmk2/Projects/skateboard/skateboard/sample-input.txt"))
{
    ...
}

Be consistent. Either use var everywhere or nowhere. You also don't always need the entire namespace. Add a using System.IO; to other usings to avoid lengthy lines.

string teamOne = lines[0].Substring(0, lines[0].Length - 2);
int teamOneScore = Convert.ToInt32(lines[0][lines[0].Length - 1] - '0');
DoesTeamExist(dictionary, teamOne);
string teamTwo = lines[1].Substring(1, lines[1].Length - 2);
int teamTwoScore = Convert.ToInt32(lines[1][lines[1].Length - 1] - '0');

You should define a class for the results. It would help to explain all the magic indexes [0] above and encapsulate some logic. It's extremely hard to follow in its current form.

lines[0].Substring(0, lines[0].Length - 2)

Assuming that the score is always a space and a number like 1 is a very bad idea. Use regex instead. The TeamScore class could look like this:

class TeamScore
{
    public string Name { get; set; }
    public int Points { get; set; }

    public static TeamScore Parse(string value)
    {
        var scoreMatch = Regex.Match(value, @"(?<name>.+?)(?<points>\d+)");
        return new TeamScore
        {
            Name = scoreMatch.Groups["name"].Value.Trim(),
            Points = Convert.ToInt32(scoreMatch.Groups["points"].Value)
        };
    }
}

This has now huge implications as it will affect the entire application and make it much easier to understand:

The new Main becomes:

public static void Main(string[] args)
{
    var counter = 0;
    var teamScores = new Dictionary<string, TeamScore>();

    // create constants for indicies
    const int teamOneIndex = 0;
    const int teamTwoIndex = 1;

    using (var file = new StreamReader(@"/Users/aaronmk2/Projects/skateboard/skateboard/sample-input.txt"))
    {
        // define the variable as close to the usage as possible
        string line;
        while ((line = file.ReadLine()) != null)
        {
            // name it appropriately, it's "teams" not just a "line"
            var teams = line.Split(',').Select(TeamScore.Parse).ToList();

            // add both teams to the dictionary if not already there
            foreach (var team in teams)
            {
                if (!teamScores.ContainsKey(team.Name))
                {
                    teamScores.Add(team.Name, team);
                }
            }

            AwardPoints(teamScores, teams[teamOneIndex], teams[teamTwoIndex]);
            counter++;
        }
    }

    // don't name it just items, write what items they are
    var sortedTeamScores =
        from teamScore in teamScores
        orderby teamScore.Value.Points descending, teamScore.Key ascending
        select teamScore;

    using (var write = new System.IO.StreamWriter(@"/Users/aaronmk2/Projects/skateboard/skateboard/test-output.txt"))
    {
        // Display results.
        foreach (var teamScore in sortedTeamScores)
        {
            Console.WriteLine("{0}: {1}", teamScore.Key, teamScore.Value);
            if (teamScore.Value.Points == 1)
            {
                write.WriteLine("{0}: {1} pt", teamScore.Key, teamScore.Value.Points);
            }
            else
            {
                write.WriteLine("{0}: {1} pts", teamScore.Key, teamScore.Value.Points);
            }

        }
    }
}

and the helper method is:

static void AwardPoints(Dictionary<string, TeamScore> dictionary, TeamScore teamOne, TeamScore teamTwo)
{
    if (teamOne.Points == teamTwo.Points)
    {
        dictionary[teamOne.Name].Points += 1;
        dictionary[teamTwo.Name].Points += 1;
    }
    else if (teamOne.Points > teamTwo.Points)
    {
        dictionary[teamOne.Name].Points += 2;
    }
    else
    {
        dictionary[teamTwo.Name].Points += 2;
    }
}

There still other improvements possible like:
- separate the saving from the console output into two methods
- encapsulate the file reading in another method

The final version should look like this:

void Main(string[] args)
{
    var fileName = @"...";
    var teamScores = ImportTeamScores(fileName);
    var sortedTeamScores = SortTeamScores(teamScores);
    SaveTeamScore(sortedTeamScores);
    PrintTeamScore(sortedTeamScores);
}

It will make the maintenace much easier as you can now adjust only one feature at a time witout affecting the others.

I hope I have the logic correct after all the refactoring.


Find all needles in the haystack and return their sum This is based on a programming challenge I recently solved. However, since the writer of the challenge asked me not to disclose the question and/or it's answer, I modified it a little. If it doesn't make sense: my mistake.

The target is to find all numbers in a file and sum those. So if the following is found:

Sed ut 11 perspiciatis, unde omnis 11 iste natus 20 error sit

The result should be 42.

Floats like 0.42 will be considered equal to 42 in this challenge.

To generate example input, I took the revised version of my 'n x on the y' generator and made it run from 999. That's almost 5k lines of code, a nice amount to test against. The old code produces the same output, for those interested.

I was quite content with the performance. Now I'm wondering whether I could've written it better. Readability and maintainability are key, as usual.

The next iteration of this code will likely try to match keywords instead of arbitrary numbers. More on that in a later question.

haystack.py

# Sum all numbers in a haystack
import re

def getNumbers(inp):
    with open(inp, 'r') as f:
        return re.findall('[0-9]+', str(f.readlines()))

def calculateSum(results):
    total = 0
    for result in results:
        total += int(result)
    return total

def main(input_file='haystack.txt'):
    print calculateSum(getNumbers(input_file))

if __name__ == "__main__":
    main()

First, you should have a look at PEP8 again, specifically the part about function names. It recommends using lower_case_with_underscores. It also recommends leaving two blank lines between function definitions on the module level.

Next, open already opens the file read-only by default, there is no need for 'r'. Also, f.readlines() returns a list, which you then convert into a string representation of that list. However, you can just use f.read() here, which directly returns the full string. Note that reading in the full file content at once might be quite memory-consuming, so don't do it with files larger than your RAM.

Last but not least, your function calculate_sum could just be the built-in sum, if the elements of results were already numbers. So I would change get_numbers to return actual numbers (like the name implies), by using map:

import re

def get_numbers(file_name):
    with open(file_name) as f:
        return map(int, re.findall('[0-9]+', f.read()))

def main(file_name='haystack.txt'):
    print sum(get_numbers(file_name))

if __name__ == "__main__":
    main()

In Python 3.x, you could use the yield from keyword to make your get_numbers into a generator and avoid the memory issue, because the file is read one line at a time (at the cost of executing the regexp for every line):

def get_numbers(file_name):
    with open(file_name) as f:
        for line in f:
            yield from map(int, re.findall('[0-9]+', line))

In Python 2.x this could be implemented like this:

def get_numbers(file_name):
    with open(file_name) as f:
        for line in f:
            for item in map(int, re.findall('[0-9]+', line)):
                yield item


Cryptographic range based random number generator class 2.0 I've corrected the bug in the first version and fixed up the error messages.

As for using floating arithmetic, since i'm not using more than 10 digits of the generated double, which, to my understanding, has at least 14 significant digits I can't see how the rounding will effect the outcome.

As for the randomness of the results, I can't seem to find any bias in the results of this algorithm.  Perhaps someone has a better test and hard numbers, instead of blind theory.

        public static class CryptoRandom
        {
            const double MAX_RANGE = (double)UInt64.MaxValue + 1;

            /// <summary>
            /// Internal algorithm to generate the range based integers.
            /// </summary>
            static int Next(UInt64 min, UInt64 max)
            {
                if (max < min)
                {
                    throw new ArgumentException($"max is less than min.  The values are, min = {min}, max = {max}");
                }
                if(min < 0)
                {
                    throw new ArgumentException($"min is negative.  The value is min = {min}");
                }
                if (min == max)
                {
                    throw new ArgumentException($"min equals max.  The values are, min = {min}, max = {max}");
                }

                using (RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider())
                {
                    byte[] randomNumber = new byte[8];

                    rng.GetBytes(randomNumber);
                    double baseNum = BitConverter.ToUInt64(randomNumber, 0) / MAX_RANGE;

                    UInt64 range = max - min;

                    return (int)((baseNum * range) + min);
                }

            }
            /// <summary>
            /// Get a cryptographic random 32-bit integer in the range from
            /// min(inclusive) to max(exclusive)
            /// </summary>
            public static int Next(int min, int max)
            {
                return (int)Next((UInt64)min, (UInt64)max);
            }
            /// <summary>
            /// Get a cryptographic random 32-bit integer in the range from
            /// 0 to max(exclusive)
            /// </summary>
            public static int Next(int max)
            {
                return (int)Next(0, (UInt64)max);
            }

            /// <summary>
            /// Get a cryptographic random 32-bit integer
            /// </summary>
            public static int Next()
            {
                return (int)Next(0, (UInt64)Int32.MaxValue+1);
            }

        }

First of all somehing trivial:

When max < min or max == min you correctly throw ArgumentException but then for min < 0you should throw ArgumentOufOfRangeException however...ulong cannot be less than zero then this check is meaningless.
There is a needless cast in the other overloads of Next(): return value is already an int.
If you cast int to ulong (signed to unsigned) you may want to do it in a checked environment to, at least, have a run-time error instead of wrong but undetected values (I assume that you do not want to penalize distribution simply discarding ms bits).
You may replace new byte[8] with new byte[sizeof(ulong)].

Now more serious things...

Using double for such big range you effectively break the distribution of your random numbers (because above a threshold even integer numbers cannot be represented correctly in floating point). I understand that you're doing that to keep an apparent upper limit equal to UInt64.MaxValue but you're sacrificing randomness. It may be an issue or not, in your case, but be aware of it.

As you already know RNGCryptoServiceProvider generates random bytes and Convert.ToUInt64() is not enough to have a true randomly generated number (because each byte, not their composition, is supposed to be randomly distributed). There is a technique you're somehow applying to compensate this issue and it's described by Stephen Toub and Shawn Farkas in Tales from the CryptoRandom. I suggest to pick their code and to use it as-is until you completely understand their implementation (and the implication of any change). Not that this will  introduce any big bias in your calculations but unless you statistically validate your algorithm I think you should stick to well-known and reviewed implementations.

As Adam already noted in his answer you should not have a static class and RNGCryptoServiceProvider should be a class member. Let the caller decide how your random number generator should be used to have a better distribution. Don't forget to implement IDisposable also for your class. Also consider to accept ctor parameter of type RandomNumberGenerator (which can be mocked in your tests), you may want to test your code with a well-known set of inputs, not with random ones:

public sealed class CryptoRandom : IDisposable
{
    public CryptoRandom(RandomNumberGenerator generator)
    {
        _generator = generator ?? throw new ArgumentNullException(nameof(generator));
    }

    public CryptoRandom()
        : this(new RNGCryptoServiceProvider())
    {
    }

    // ...

    private readonly RandomNumberGenerator _generator;
}

There is, finally, an even more serious problem. If you initialize RNGCryptoServiceProvider without a seed then it will use Environment.TickCount and its resolution is limited to the system timer resolution (typically 10 ms), this alone will practically vanish most of your effort.


PyQt5 GroupBox Layouts Feedback I will import given code for my main QMainWindow.(I only add import sys etc. to run it)
Is there a more compact way to code this lines.
Output is correct for my expectation.

from PyQt5.QtWidgets import *
import sys

class ButtonWidget(QWidget):

    def __init__(self):
        super(ButtonWidget, self).__init__()

        # Function
        button1 = QRadioButton("Sinus")
        button2 = QRadioButton("Cosines")
        # Color
        button3 = QRadioButton("Red")
        button4 = QRadioButton("Green")
        # Line
        button5 = QRadioButton("Solid")
        button6 = QRadioButton("Dashed")
        # Left Group
        left_group = QGroupBox("Left Group")
        left_group_layout = QVBoxLayout()
        left_group_layout.addWidget(button1)
        left_group_layout.addWidget(button2)
        left_group.setLayout(left_group_layout)
        # Middle Group
        middle_group = QGroupBox("Middle Group")
        middle_group_layout = QVBoxLayout()
        middle_group_layout.addWidget(button3)
        middle_group_layout.addWidget(button4)
        middle_group.setLayout(middle_group_layout)
        # Right Group
        right_group = QGroupBox("Right Group")
        right_group_layout = QVBoxLayout()
        right_group_layout.addWidget(button5)
        right_group_layout.addWidget(button6)
        right_group.setLayout(right_group_layout)
        # Main Group
        main_group = QGroupBox("Main Group")
        main_group_layout = QHBoxLayout()
        main_group_layout.addWidget(left_group)
        main_group_layout.addWidget(middle_group)
        main_group_layout.addWidget(right_group)
        main_group.setLayout(main_group_layout)
        # Widget
        main_widget = QWidget()
        main_widget_layout = QVBoxLayout()
        main_widget.setLayout(main_widget_layout)
        main_widget_layout.addWidget(main_group)
        # Layout Set
        self.setLayout(main_widget_layout)

        self.show()

if __name__ == "__main__":
    app = QApplication(sys.argv)
    ui = ButtonWidget()
    sys.exit(app.exec_())

Yes there is a solution. The obvious solution is of course to use loops.
Here is my try using a composite dictionary. It is functionally equivalent to your code and will save you about a dozen lines but hopefully adds flexibility. This was a quick job, so maybe you can take it further.

from PyQt5.QtWidgets import *
import sys

class ButtonWidget(QWidget):

    def __init__(self):
        super(ButtonWidget, self).__init__()

        groups = {"Left Group": ("Sinus", "Cosines"),
            "Middle Group": ("Red", "Green"),
            "Right Group": ("Solid", "Dashed")
        }

        # Main Group
        main_group = QGroupBox("Main Group")
        main_group_layout = QHBoxLayout()

        # loop on group names
        for group, buttons in groups.items():
            group_box = QGroupBox(group)
            group_layout = QVBoxLayout()
            for button_text in buttons:
                group_layout.addWidget(QRadioButton(button_text))

            group_box.setLayout(group_layout)
            main_group_layout.addWidget(group_box)

        main_group.setLayout(main_group_layout)

        # Widget
        main_widget = QWidget()
        main_widget_layout = QVBoxLayout()
        main_widget.setLayout(main_widget_layout)
        main_widget_layout.addWidget(main_group)
        # Layout Set
        self.setLayout(main_widget_layout)

        self.show()

if __name__ == "__main__":
    app = QApplication(sys.argv)
    ui = ButtonWidget()
    sys.exit(app.exec_())

NB: you'll probably have to add names to the generated controls (the buttons at least).

PS: personally I use the QT designer to build forms unless I need a highly dynamic layout. Adding controls in code is tedious and less visual.


Singly linked list queue class I have a Queue class that is basically a single linked list. It has 2 remove methods. What would be the cleanest way to avoid duplication of the code? Only the comparisons in methods are different.

int Queue::remove(const string& substring)
{
    // Loop through the queue list
    size_t found;
    found = current_job->description.find(substring)
    if (found != string::npos) {
        // Remove job.
    }
    // Return the amount of removed jobs.
}

int Queue::remove(int priority)
{
    // Loop through the queue list
    if (current_job->priority == priority) {
        // Remove job.
    }
    // Return the amount of removed jobs.
}

I thought a template solution as answered here, but in this case, the comparison function has different arguments.

You can abstract out the thing that changes, the comparison expression, into a predicate for a template function.

class PriorityMatch
{
    int priority_;
public:
    explicit PriorityMatch(int priority)
        : priority_(priority) {}

    bool operator()(const Job& job)
    {
        return job->priority == priority_;
    }
};

class DescriptionMatch
{
    std::string substring_;
public:
    explicit DescriptionMatch(const std::string& substring)
        : substring_(substring) {}

    bool operator()(const Job& job)
    {
        return job->description.find(substring_) != std::string::npos;
    }
};

template<class Predicate>
int Queue::remove(Predicate predicate)
{
    // Loop through the queue list
    if (predicate(current_job)) {
        // Remove job.
    }
    // Return the amount of removed jobs.
}

int Queue::remove(const string& substring)
{
    return remove(DescriptionMatch(substring));
}

int Queue::remove(int priority)
{
    return remove(PriorityMatch(substring));
}

Or in C++11:

template<class Predicate>
int Queue::remove(Predicate predicate)
{
    // Loop through the queue list
    if (predicate(current_job)) {
        // Remove job.
    }
    // Return the amount of removed jobs.
}

int Queue::remove(const string& substring)
{
    return remove([](const Job& job){
        return job->description.find(substring) != std::string::npos;
    });
}

int Queue::remove(int priority)
{
    return remove([](const Job& job){
        return job->priority == priority;
    });
}


Return the next power of two Given an unsigned integer \$N\$, I want to return \$P\$ with \$P\$ being a power of two such that \$P \ge N\$.

Examples:

1 -> 1
2 -> 2
3 -> 4
5 -> 8

The goal is to provide a very fast implementation. This code is supposed to target a specific platform and we can make the assumption that the intrinsics used are supported.

uint32_t nextPowerOfTwo(uint32_t n) {
    uint32_t msb_index = fls(n);
    uint32_t trailing_zeros = std::__ctz(n);
    bool is_pot = (msb_index - 1 == trailing_zeros);
    uint32_t p = (is_pot ? n : 1 << msb_index);
    return p;
}

If you really care about performance you may also give a chance to a branchless version. Note that compiler may optimize code with explicit branches (if and ?:) if target architecture supports CMOV (& similar) instructions, check generated assembly output. In these examples I will use X86 mnemonics but I suppose meaning is clear whichever your architecture is.

Also you're using non-standard fls() and std::__clz(), they may be implemented with instrinsics or not. If not then performance will greatly decrease because they need a loop or a lookup table instead of BSR (or LZCNT) and BSF (or TZCNT).

uint32_t nextPowerOfTwo(uint32_t n)
{
    --n;

    n |= n >> 1;
    n |= n >> 2;
    n |= n >> 4;
    n |= n >> 8;
    n |= n >> 16;

    return n + 1;
}

Note that 0 gives a wrong result then if you expect 0 as input and you don't want 0 as output you have to handle this special case (just return 1 or add 1 if your compiler will generate a branch for that.) AFAIK also implementation with BSx (or xZCNT) have the same problem (they just set ZF or CF) then special case has to be handled also there (if your implementation specific function does not already do it).

Code is equivalent to 1 << (log2(n - 1) + 1). I don't have a link to original code (algorithm is not mine!) but in my comments authors are W. Lewis and P. Hart) and (thanks Martin) there also is another author who arrived to the same code (with link to their original discussion/implementation!)

Which implementation is faster? It depends (see comments for a discussion with CodeRodde.) Target architecture (possibly revision), compiler vendor, version and settings will all affect results. Exact numbers are meaningless without details about your specific scenario, the point is that CodeRodde's answer is faster than this with MSVC++ but slower with GCC, the absolutely great answers from Jerry and JS1 also add more doubts about which method is best.

The only thing I can suggest is to implement them all and pick the fastest one in your specific case. If this is truly performance critical, because this function is so small, I'd also feel to suggest is to put some raw performance comparison in your unit tests: when current version will be slower than expected (compared to all the others) you will need to inspect again...


HTTP Load test with Golang I am a golang novice... I am trying to write an application that takes in many requests - up to a sustained 10000 HTTP posts/sec and post the payload to two back-ends in parallel (5 second timeout).  A canned message is returned to the client, it doesn't matter whether the backed end returns a response or errors out, the same response text is sent to the client.

I have a first crack at it below.  I'm not worried about error handling yet.  My thinking is that I can make each back-end call asynchronous using goroutines and let the go runtime deal with queuing/threads for the go routines.  I simply need to perform the post and log the result, there is no communication between goroutines. I have had others guide me to using channels, but given that I don't need to return any data back from the post I'm not sure I need them.

Here is my first very simple crack at it.  Please make any suggestions on the code and approach!

package main

import (
    "fmt"
    "github.com/go-chi/chi"
    "net/http"
    "io/ioutil"
    "bytes"
    "time"
)

func main() {
    r := chi.NewRouter()
    r.Post("/", func(w http.ResponseWriter, r *http.Request) {
        handlePost(w, r);
    })

    http.ListenAndServe(":3000", r)

}

func handlePost(w http.ResponseWriter, r *http.Request) {
    body, _ := ioutil.ReadAll(r.Body)
    fmt.Fprintf(w, "dummy response will go here");
    go callBackend("http://backend1", body)
    go callBackend("http://backend2", body)
}

func callBackend(url string, body []byte) {

    req, err := http.NewRequest("POST", url, bytes.NewBuffer(body))
    req.Header.Set("Content-Type", "application/xml")

    client := &http.Client{
        Timeout: time.Second * 5,
    }
    resp, err := client.Do(req)
    if err != nil {
        panic(err)
    }
    defer resp.Body.Close()

    fmt.Sprintf("backend [%s] response status: %s", url, resp.Status)
    fmt.Println("backend response Headers:", resp.Header)
    responsebody, _ := ioutil.ReadAll(resp.Body)
    fmt.Println("backend response Body:", string(responsebody))

}

r.Post("/", func(w http.ResponseWriter, r *http.Request) {
    handlePost(w, r)
})

can be simplified to

r.Post("/", handlePost)

since both functions have same signature.

fmt.Fprintf(w, "dummy response will go here")

No need to use printf family functions where you don't need formatting. You can write directly with

w.Write([]byte("dummy response will go here"))

or with

io.WriteString(w, "dummy response will go here")

responsebody, _ := ioutil.ReadAll(resp.Body)
fmt.Println("backend response Body:", string(responsebody))

This one reads the whole response into memory for no reason. Use io.Copy to write it more efficiently:

fmt.Print("backend response Body: ")
_, err := io.Copy(os.Stdout, resp.Body)

  I'm not worried about error handling yet.

Then everything else is totally fine.


Defining a zip code string I'm following this

let StringOfLengthConstructor<'c> (input:string, length:int, defaultConstructor:string->'c) =
    match input with
    | null -> None
    | x when x.Length = length -> Some(defaultConstructor(input))
    | _ -> None
type String3 = | String3 of string
let String3 input = StringOfLengthConstructor<String3>(input, 3, String3)
type String4 = | String4 of string
let String4 input= StringOfLengthConstructor<String4>(input, 4, String4)

type String5 = | String5 of string
let String5 input = StringOfLengthConstructor<String5>(input, 5, String5)
type String6 = | String6 of string
let String6 input = StringOfLengthConstructor<String6>(input,6, String6)

and am trying to make more of these with hopefully less repetition. Can this be done without having to repeat type String3 =| String3 of string followed by this?

let String3 input = StringOfLengthConstructor<String3>(input, 3, String3)

See all the repetition?  For the purpose of defining a zip code, a US zip code could be defined as

type ZipCode = | Us of String5

or

type ZipCode = | Us of String5*(String4 option) | Canadian of String3*String3

The solution can be made cleaner by re-arranging the order of the arguments and using pointfree style.

let StringOfLengthConstructor<'c> (length : int) (defaultConstructor : string -> 'c) (input : string) =
    match input with
    | x when x <> null && x.Length = length -> Some (defaultConstructor input)
    | _ -> None

type String3 = String3 of string
let String3 = StringOfLengthConstructor 3 String3

However, I don't think that String5 * (String4 option) is a good definition of a zip code. The code should be validating on more than just the string length.


Rewrite Amazon s3 key I've create a function that rewrites the key or "path" of an object in s3.

By default, Amazon Web Services Firehose writes to s3 in the format of YYYY/MM/DD/HH/foo.json. We have a AWS Lambda function listening for putObjects on l1/source/event_type/fh/, and when a new file is added to s3, the Lambda is invoked and the key or 'path' to that file is rewritten to a flat structure of l1/source/event_type/daily/dt=YYYY-MM-DD/foo.json - yes, I purposefully left-off the fh and HH paths.

input key: l1/source/event_type/fh/YYYY/MM/DD/HH/foo.json

output key: l1/source/event_type/daily/dt=YYYY-MM-DD/foo.json

def create_date_parition_from_key(key):
    '''creates new date parition prefix
    '''

    try:
        key_split = re.split(r'(/\d{4})', key)
        start_path = (key_split[0].split('/')[0], key_split[0].split('/')[1])
        remove_fh_path = '/'.join(start_path)
        default = key_split[2].split('/')
        year = key_split[1][1:]

        s3_prefix = remove_fh_path + '/'# /l1/foo/bar/baz/
        date_partion = ('daily/dt=' +
                        '-'.join([year, default[1], default[2]]) +
                        '/') # dt=YYYY-MM-DD/
        file_name = default[-1] # foo.json

        new_key = s3_prefix + date_partion + file_name

        print ('New partition key created: {}.'.format(new_key))

        return new_key
    except Exception as ex:
        print(ex)
        print('Error paritioning key {}.'.format(key))
        raise ex

I'm newer to python and am just looking for ways to improve my code as it seems fragile.

EDIT: The input key can vary in its number of paths:

l1/source/event_type/fh/YYYY/MM/DD/HH/foo.json
l1/app/source/event_type/fh/YYYY/MM/DD/HH/foo.json
l1/event_type/fh/YYYY/MM/DD/HH/foo.json

Simplicity

Tuple unpacking and str.format can simplify the function so much:

def create_date_parition_from_key(key):
    a,b,c,_,year, month, day, _, name = key.split('/')
    return "{}/{}/{}/daily/dt={}-{}-{}/{}".format(\
            a, b, c, year, month, day, name)

If input key can change in the number of paths:

def create_date_parition_from_key(key):
    *_, year, month, day, _, name = key.split('/')
    return "{}/daily/dt={}-{}-{}/{}".format(\
            key[:key.index("/fh/")], year, month, day, name)

Exception handling

try:
    # Things
except Exception as ex:
    print(ex)
    print('Error paritioning key {}.'.format(key))
    raise ex

This is against re-use. I cannot re-use this function because it inevitably prints to the console on error, I have no way of working around it throwing an exception with try - except.

This kind of error notification should be done in the main function that has exactly the job of communicating errors / successes of the other functions to the end user.

Finally Exception is too vague and too much code is inside the try block. Please reduce the code inside try as much as possible and specify a precise Exception kind.

I just removed all exception handling in my version as in my opinion it was just making the code worse while there was no need for it.


Adding punctuation to US and European patent numbers I want to add custom punctuations in a given string.

eg:

US 8012999 B2 --> US 8,012,999 B2
US 20170107206 A1 --> US 20170107206 A1
EP 2795605 B1 --> EP 2 795 605 B1
US 09700674 --> US 09/700,674

This is what I wrote

def punctuate(text, punctuations):
    """Given text and list of tuple(index, puncuation)
    returns punctuated text"""

    char_list = list(text)
    k = 0
    for i, char in punctuations:
        char_list.insert(i + k, char)
        k += 1
    return "".join(char_list)

In [53]: punctuate('US 8012999 B2', [(4, ','), (7, ',')])
Out[53]: 'US 8,012,999 B2'

In [54]: punctuate('US 20170107206 A1', [(7, '/')])
Out[54]: 'US 2017/0107206 A1'

In [55]: punctuate('US 09700674', [(5, '/'), (8, ',')])
Out[55]: 'US 09/700,674'

In [56]: punctuate('EP 2795605 B1', [(4, ' '), (7, ' ')])
Out[56]: 'EP 2 795 605 B1'

This works fine. Is this the best way to do it?
The punctuations list will always be sorted one starting from lower to higher index.

Here is a solution using a generator, which gives you linear time. Your current solution is not linear, because list.insert is already \$\mathcal{O}(n)\$.

def interweave(x, y):
    """
    Given an iterable `x` and an iterable `y` of indices and items,
    yield from `x`, but interweave the items from `y` at the given indices.
    """
    y = iter(y)
    next_i, next_y = next(y, (-1, ""))
    for i, next_x in enumerate(x):
        if i == next_i:
            yield next_y
            next_i, next_y = next(y, (-1, ""))  # default value for when y is exhausted
        yield next_x

def punctuate(text, punctuations):
    return "".join(interweave(text, punctuations))

It passes all the given testcases:

In [98]: punctuate('US 8012999 B2', [(4, ','), (7, ',')])
Out[98]: 'US 8,012,999 B2'

In [99]: punctuate('US 20170107206 A1', [(7, '/')])
Out[99]: 'US 2017/0107206 A1'

In [100]: punctuate('US 09700674', [(5, '/'), (8, ',')])
Out[100]: 'US 09/700,674'

In [101]: punctuate('EP 2795605 B1', [(4, ' '), (7, ' ')])
Out[101]: 'EP 2 795 605 B1'


Caesar Cipher implementation - Odin Project I am going through the Odin Project Ruby section, and just finished implementing a Caesar cipher interpretation.  Should I use classes, or more than one method to distribute the flow?  Is there really a need for initialization in this?

def caesar_cipher(string, shifter)
    ciphered = []
    string.split("").each do |char|
        if char == " " then ciphered << char
            next
        end

        char = char.to_s.ord

        if ("a".ord.."z".ord).include? char
            if char + shifter < "z".ord
                ciphered.push((char + shifter).chr)
            else
                ciphered.push((shifter - ("z".ord - char) + "a".ord).chr)
            end
        elsif ("A".ord.."Z".ord).include? char
            if char + shifter < "Z".ord
                ciphered.push((char + shifter).chr)
            else
                ciphered.push((shifter - ("Z".ord - char) + "A".ord).chr)
            end
        else
            next
        end

    end
    print ciphered.join + "\n"
end

#test: caesar_cipher("Hello friend my Name is Sam",10)

The code can be simplified, but you're on the right track.

A function is sufficient for a simple string filter like this; a class would be overkill.  I would rename the first parameter to plaintext and the result to ciphertext because that is the standard terminology.

The function should return the ciphertext rather than printing it.  The caller should have the flexibility to print it, show it in a GUI, store it in a database, or whatever.

Instead of split("").each, you can iterate over the plaintext using plaintext#each_codepoint.  You can also build the result by appending to a string rather than joining a list.

The if…elseif would be slightly better as a case expression.  You don't need the special case for " "

def caesar_cipher(plaintext, shift)
  ciphertext = ''
  plaintext.each_codepoint do |asc|
    ciphertext << case asc
      when 'a'.ord..'z'.ord
        'a'.ord + (asc - 'a'.ord + shift) % 26
      when 'A'.ord..'Z'.ord
        'A'.ord + (asc - 'A'.ord + shift) % 26
      else
        asc
    end
  end
  ciphertext
end


Sequential and parallel addition of 2 vectors I have done sequential and parallel addition of 2 vectors that I have filled with some dummy values. I am also measuring the time of execution for both, sequential and parallel execution.

#include <iostream>
#include <vector>
#include <iterator>
#include <algorithm>
#include <functional>
#include <chrono>
#include <cassert>
#include <thread>

#define NUM_OF_THREADS 8
#define VECTOR_SIZE    512

using time_point = std::chrono::high_resolution_clock::time_point;

template<typename T>
std::vector<T> operator+(std::vector<T> const& first, std::vector<T> const& second)
{
    assert(first.size() == second.size());

    std::vector<T> result;
    result.reserve(first.size());

    std::transform(first.begin(), first.end(), second.begin(), std::back_inserter(result), std::plus<T>());
    return result;
}

template<typename T>
void parallel_addition(std::vector<T>& result,
                       std::vector<T> const& first,
                       std::vector<T> const& second,
                       size_t lower_bound,
                       size_t upper_bound)
{
    for (size_t i = lower_bound; i < upper_bound; i++)
    {
        result[i] = first[i] + second[i];
    }
}

int main(int argc, char *argv[])
{
    std::vector<int> first_vec, second_vec;

    /* Filling up the vectors with dummy values */
    for (int i = 1; i <= VECTOR_SIZE; i++)
    {
        first_vec.push_back(i);
        second_vec.push_back(i);
    }

    /* Sequential addition of vectors */
    time_point t1_seq = std::chrono::high_resolution_clock::now();
    std::vector<int> sequential_result = first_vec + second_vec;
    time_point t2_seq = std::chrono::high_resolution_clock::now();
    auto duration_seq = std::chrono::duration_cast<std::chrono::microseconds>(t2_seq - t1_seq).count();
    std::cout << "Sequential addition of vectors - execution time: " << duration_seq << "us" << std::endl;

    /* Parallel addition of vectors */
    std::vector<std::thread> threads;
    std::vector<int> parallel_result(VECTOR_SIZE);

    time_point t1_par = std::chrono::high_resolution_clock::now();

    for (int i = 0; i < NUM_OF_THREADS; i++)
    {
        threads.push_back(std::thread(parallel_addition<int>,
                                      std::ref(parallel_result),
                                      first_vec,
                                      second_vec,
                                      i * (VECTOR_SIZE / NUM_OF_THREADS),
                                      (i + 1) * (VECTOR_SIZE / NUM_OF_THREADS)));
    }

    for (auto &thread : threads)
    {
        thread.join();
    }

    time_point t2_par = std::chrono::high_resolution_clock::now();
    auto duration_par = std::chrono::duration_cast<std::chrono::microseconds>(t2_par - t1_par).count();
    std::cout << "Parallel (" << NUM_OF_THREADS << " threads) addition of vectors - execution time: " << duration_par << "us" << std::endl;

    return 0;
}

Despite style review, can you also tell me if there is another, better, approach to addition of two vectors in multiple threads?
My intention was to write in C++17.

#define NUM_OF_THREADS 8
#define VECTOR_SIZE    512

Don't use #define for constants or "functions" (⧺ES.31)

using time_point = std::chrono::high_resolution_clock::time_point;

Good!  bring down just the stuff you need, and give it a friendly name.  I think

using std::chrono::high_resolution_clock::time_point;

would be the same thing, and is the usual way of importing symbols from a library’s namespace.

assert(first.size() == second.size());

That will not do anything in a release build, you know.

If you really want checking, make a precondition that always works.  The GSL support library has Expects that can be programmed at compile time to assert, throw, or die.

In general operator+ looks good!  You are using a value return, and you ensure NVRO takes place.  You reserve the vector size.  You use STL algorithms.

for (int i = 1; i <= VECTOR_SIZE; i++)
{
    first_vec.push_back(i);
    second_vec.push_back(i);
}

There is a standard algorithm for that.

time_point t1_seq = std::chrono::high_resolution_clock::now();
⋮

Use auto (almost everywhere).

since there is no code-movement barrier, your benchmark might not be accurate.

for (auto &thread : threads)

Good!  You use the range-for where you can.


Strongly-typed reading values from CSV DataTable Is there a way to do this using parameters so the value is automatically converted to whatever datatype the keyfield has in the datatable?

This code should be reusable for future bulk update applications hence the constant and the check on multiple datatypes.

private const string DBKEYFIELDNAME = "CustomerNr";

...
for (int i = 1; i <= csvLines.Length - 1; i++) // i=1 => skip header line
{
    string[] csvFieldsArray = csvLines[i].Split(';');
    int indexKeyField = csvHeaders.IndexOf(CSVKEYFIELDNAME.ToLower());

    object csvKeyValue = csvFieldsArray[indexKeyField];

    // ... some more code here that is not relevant

    // Find the matching row for our csv keyfield value
    Type keyType = parameters.DataTableOriginal.Columns[DBKEYFIELDNAME].DataType;
    DataRow[] rowsOriginal = null;
    if (keyType.IsAssignableFrom(typeof(string)))
        rowsOriginal = parameters.DataTableOriginal.Select(DBKEYFIELDNAME + "='" + csvKeyValue.ToString() + "'");
    else if (keyType.IsAssignableFrom(typeof(Int16)) || keyType.IsAssignableFrom(typeof(Int32)) || keyType.IsAssignableFrom(typeof(Int64)) || keyType.IsAssignableFrom(typeof(bool)))
        rowsOriginal = parameters.DataTableOriginal.Select(DBKEYFIELDNAME + "=" + csvKeyValue);

    if (rowsOriginal != null && rowsOriginal.Length == 1)
    {
        // Do some processing of the row here
    }
}

A few comments:

This method does way too much.  It is difficult to understand and will be difficult to debug and maintain.  Break it down into smaller methods that each have a single responsibility.
Use curly braces after your if and else statements.  This improves readability of the code and makes it less likely for other code to sneak in there in the future.
Can't your else statement just be a plain else with no if after it?  It seems like you want to put quotes around a string, and use the plain value for everything else.  Are there other requirements here?
The type of csvKeyValue could just be string, since it's pulling a value out of a string[]
No need for the call to .ToString() in your if branch
I would try to write the call to parameters.DataTableOriginal.Select only once.  Consider using your if/else to set a delimeter variable to either string.Empty or ', then write your query once like so:

DataRow[] rowsOriginal = parameters.DataTableOriginal.Select(
    DBSLEUTELVELDNAAM + "=" + delimeter + csvKeyValue + delimeter);


Dropdown bubble for a menu item I am a tyro with OOCSS and think I bit off more than I can chew in creating a dropdown bubble for a menu item.

I removed as much element specific code as I could, but still think I don't quite have it down.

I know my class names are all messed up, but I'm really just messing around right now.

For example: should the dropdownContain have the positioning, or more like set the margin that gives the position  .drop .dropdownContain {margin-left: -100px} that way the dropdownContain can be re-used later?

Any suggestions?

CSS:

li {
    list-style-type: none;
}

ol, ul {
    list-style-type: none;
    font: 13px Verdana, 'Lucida Grande';
    cursor: pointer;
}
ol>li, ul>li {
    margin: .2em 0
}
.navDropDown .drop {
    position: relative;
}
.triangle {
    width: 0;
    height: 0;
    position: absolute;
    border-left: 8px solid transparent;
    border-right: 8px solid transparent;
    border-bottom: 8px solid white;
}
.dropOut .triangle {
    top: -8px;
    left: 50%;
    margin-left: -8px;
}
.dropdownContain {
    position: absolute;
    z-index: 2;
    left: 50%;
    margin-left: -100px;
    top: -400px;
 }
.dropOut {
    width: 200px;
    background: white;
    float: left;
    position: relative;
    margin-top: 0px;
    opacity: 0;
    border-radius: 4px;
    box-shadow: 0 1px 6px rgba(0, 0, 0, .15);
    transition: all .1s ease-out;
}
.dropOut .dropDown {
    float: left;
    padding: 10px 5px;
}
.dropList {
    width: 155px;
    padding: 5px 0 5px 15px;
    margin: 0px 10px;
    color: #777;
    border-radius: 4px;
    transition: background .1s ease-out;
}
.dropDown .dropList:not(:first-child):hover {
    background: #f6f6f6;
    width: 155px;
}
.drop:hover .dropdownContain {
    top: 18px;
}
.drop:hover .dropOut {
    opacity: 1;
    margin-top: 8px;
}

HTML:

E-Mail

Settings

Account

Download Software

Help

Sign Out

http://jsfiddle.net/mHvYQ/5/

To be honest I don't quite understand what you are asking in your question, so I'll just do a general review:

You said your class names are temporary (you should only be posting "finished" code here), however currently they are very confusing because they are all very generic or don't actually say what they are for.

Apart from that you have far too many class names and possibly too many unnecessary DIVs. Use class names only in exceptional cases. For example, having all list items have the same class name is useless. Also use other HTML elements other than div and lists. header, 'nav' and h# would help here a lot.

The triangle DIV is unnessary. Use the CSS pseudo element :before instead.

And have the title element ("E-Mail") should not be identified by be the first element of the list, but should either have a class, or possible moved out of the list all together and marked up with a heading element (h3 or so).

The HTML could look something like this:

E-Mail

Settings

Account

Download Software

Help

Sign Out


The FizzBuzz challenge in Java 8 written in a short, readable and interesting way I decided to take on the FizzBuzz challenge with as twist that I would use Java 8 concepts to make it a bit modular, yet still let it be a short, readable and understandable program.

This in contrary to some gem I found on the net: FizzBuzzEnterpriseEdition

The problem description:

  Write a program that prints the numbers from 1 to 100. But for multiples of three print "Fizz" instead of the number and for the multiples of five print "Buzz". For numbers which are multiples of both three and five print "FizzBuzz"

Here's my code:

public class FizzBuzz {
    private static Stream<String> fizzBuzz(final int min, final int max) {
        if (min < 0) {
            throw new IllegalArgumentException("min is negative: min = " + min);
        }
        if (min > max) {
            throw new IllegalArgumentException("min > max: min = " + min + " / max = " + max);
        }
        return IntStream.rangeClosed(min, max)
                .mapToObj(FizzBuzz::fizzBuzzify);
    }

    private static String fizzBuzzify(final int value) {
        StringBuilder stringBuilder = new StringBuilder();
        boolean toDefault = true;
        if (value % 3 == 0) {
            stringBuilder.append("Fizz");
            toDefault = false;
        }
        if (value % 5 == 0) {
            stringBuilder.append("Buzz");
            toDefault = false;
        }
        return (toDefault) ? String.valueOf(value) : stringBuilder.toString();
    }

    public static void main(String[] args) {
        fizzBuzz(1, 100).forEach(System.out::println);
    }
}

I'm still looking for a nicer way to write fizzBuzzify, my intention however is to not hardcode the if (value % 15 == 0) similarly if (value % 3 == 0 && value % 5 == 0), because it creates a sort of illogical operation precedence, being that you absolutely need to write the if (value % 15 == 0) case up front, followed by the 3-case and the 5-case (or vica versa).

A guideline that I use for deciding on a StringBuilder or not is that it depends on whether I know beforehand how many times it will be used or not. I believe I've come across this recommendation on some MSDN page once but I'm not entirely sure.

In your case, you know that there are only two possible uses of your StringBuilder, and that there are no values added inside a loop, so I would use normal string concatenation:

Now you can also change your code a little to what I consider more easily interpretable:

private static String fizzBuzzify(final int value) {
   String result = "";

   if (value % 3 == 0) {
       result += "Fizz";
   }
   if (value % 5 == 0) {
       result += "Buzz";
   }
   return result.length() > 0 ? result : Integer.toString(value);
}


Reversing the bits of a 32-bit integer Here is my code:

noTest=int(input())
reverse=[]
nums=[]
for i in range(noTest):
    nums.append(int(input()))

for num in nums:
    binary=bin(num)
    binNum=binary[2:]
    rev=binNum[::-1]
    rev=binary[:2]+rev+'0'*(32-len(rev))
    #print int(rev,2)
    reverse.append(int(rev,2))

for r in reverse:
    print(r)

Input:
The first line of input consists number of the test cases. Each test case
contains a single 32 bit integer.

Output:
Print the reverse of integer.

Input:00000000000000000000000000000001 =1
Output:10000000000000000000000000000000 =2147483648

You can avoid building a list of results to print them after, you can just print them as you calculate them.

Also you assign a lot of variables and:

binNum=binary[2:]

is not explicit in removing the '0b' that Python prefixes, better is

.replace('0b','')

Also the padding with 0 logic that you write is already implemented in the zfill function, so you can avoid re-writing it yourself.

Finally, you should write a function to read the input to separate concerns and maybe reuse it in a similar problem.

Here are all my suggestions implemented.

def read_numbers(n):
    return [int(input()) for _ in range(n)]

for i in read_numbers( int(input()) ):
    binary_padded = bin(i).replace('0b','').zfill(32)
    print( int( binary_padded[::-1], 2) )


Project Euler - Problem 3 The description of the problem is:

  The prime factors of 13195 are 5, 7, 13 and 29.
  What is the largest prime factor of the number 600851475143 ?

Here is my solution:

import heapq
from math import sqrt

def find_max_prime_factor(n: int) -> int:
    '''
    returns the largest prime factor of the non negative integer n
    :param n: the number we want to find the max prime factor of
    :return: the largest prime factor of n
    '''
    max_heap = []
    # Checking for 2 as prime factor
    if n % 2 == 0:
        heapq.heappush(max_heap, -2)
        # dividing by 2 until n becomes odd
        while n % 2 == 0:
            n = n/2
    # dealing with odd numbers
    for i in range(3,int(sqrt(n)),2):
        if n % i == 0:
            heapq.heappush(max_heap,-1*i)
            while n % i == 0:
                n = n/i

    # dealing with case where n is a prime by itself
    if n > 2:
        return n

    return heapq.heappop(max_heap) * (-1)

Here's some examples and test-cases:

from ProjectEuler.problem3 import find_max_prime_factor

if __name__ == "__main__":
    # 13195 prime factors are 5,7,13,29 -> returns 29
    print(find_max_prime_factor(13195))
    # 600851475143  prime factors are 6857,1471,839,71 -> returns 6857
    print(find_max_prime_factor(600851475143))
    # 7 is prime therefor -> returns 7
    print(find_max_prime_factor(7))
    # 3452656 prime factors are 2,31,6961 -> returns 6961
    print(find_max_prime_factor(3452656))
    # 896776435 prime factors are 5,17,2549,4139 -> returns 4139
    print(find_max_prime_factor(896776435))

Would love for feedback on my code.Thanks!

max_heap = []

I think the essentially Hungarian naming here is actually fairly helpful, but it would be very useful to have a comment explaining that the library support is only for min heaps, so everything must be negated when pushed or popped.

    # Checking for 2 as prime factor
    if n % 2 == 0:
        heapq.heappush(max_heap, -2)
        # dividing by 2 until n becomes odd
        while n % 2 == 0:
            n = n/2
    # dealing with odd numbers
    for i in range(3,int(sqrt(n)),2):
        if n % i == 0:
            heapq.heappush(max_heap,-1*i)
            while n % i == 0:
                n = n/i

Reasonable use of a special case: I see you've given some thought to optimisation.

It's better to use -i than -1*i, and n // i (since you want integer division) than n / i.

    # dealing with case where n is a prime by itself

That's open to misinterpretation. Is it talking about the original value of n or the current value? I think I would phrase it

    # if n > 1 here then it's a prime

    return heapq.heappop(max_heap) * (-1)

See previous point about unary minus.

I wanted to address minor improvements before addressing the big one. Why use a heap at all? The largest prime is the last prime encountered, and there's no need to store the smaller ones. Removing the heap would make the code simpler and faster.


Fastest way to count non-zero pixels using Python and Pillow I have a Python script that creates a diff of two images using PIL. That part works fine. Now I need to find an efficient way to count all the non-black pixels (which represent parts of the two images that are different). The diff image is in RGB mode.

My initial cut was something like this:

return sum(x != (0, 0, 0) for x in diffimage.getdata())

Then I realized that the diffs were usually constrained to a portion of the image, so I used getbbox() to find the actual diff data:

bbox = diffimage.getbbox()
return sum(x != (0, 0, 0) for x in diffimage.crop(bbox).getdata()) if bbox else 0

This has the advantage of being VERY fast when the image is all black since bbox is None in that case and no pixel counting need be done.

I still wasn't satisfied, so I decided to try using more of the built-in PIL methods to avoid the generator expression with the Python conditional that needed to be evaluated for each pixel. I came up with:

bbox = diffimage.getbbox()
if not bbox: return 0
return sum(diffimage.crop(bbox)
                    .point(lambda x: 255 if x else 0)
                    .convert("L")
                    .point(bool)
                    .getdata())

This is about five times faster than the previous version. The basic steps are:

Crop to the bounding box to avoid counting black pixels
Convert all non-zero values in each channel to 255. This way, when we later convert it to grayscale, all non-black pixels are guaranteed to have non-zero values. (Because a pixel might differ in only one channel, and only by a small amount, some pixels that are not actually black might end up as black in grayscale mode, because only a fraction of that channel's value makes its way to grayscale.) BTW, the function isn't evaluated for each pixel but only once for each possible pixel value to make a lookup table, so it's very fast.
Convert to grayscale.
Convert all non-zero pixels to 1 using bool.
Sum all the pixel values.

Can I do better?

Here's your implementation using Pillow:

def count_nonblack_pil(img):
    """Return the number of pixels in img that are not black.
    img must be a PIL.Image object in mode RGB.

    """
    bbox = img.getbbox()
    if not bbox: return 0
    return sum(img.crop(bbox)
               .point(lambda x: 255 if x else 0)
               .convert("L")
               .point(bool)
               .getdata())

And here's an implementation using Numpy:

def count_nonblack_np(img):
    """Return the number of pixels in img that are not black.
    img must be a Numpy array with colour values along the last axis.

    """
    return img.any(axis=-1).sum()

(We will need scipy.ndimage.imread to load the image.)

Here's a quick performance comparison using timeit.timeit:

>>> from PIL import Image
>>> import scipy.ndimage
>>> from timeit import timeit
>>> img1 = Image.open(filename)
>>> timeit(lambda:count_nonblack_pil(img1), number=10)
5.4229461060022
>>> img2 = scipy.ndimage.imread(filename)
>>> timeit(lambda:count_nonblack_np(img2), number=10)
2.3291947869875003

So Numpy is about two and a half times as fast on my example.


Processing image blocks using a task runner This code works, but I'm sure it isn't written according to "best practices" for closures.  On the other hand, at least it's intuitive to me...

the taskRunner object runs functions on setTimeout, so as not to block the UI for too long.  I'm breaking this function into chunks and running one chunk at a time.  I'm passing i into my function so it will be part of the closure, but I know I've seen this done more elegantly - I just can't wrap my head around how to do it right now.

var tr = new taskRunner();
        //I'm sure there's a better way to do a closure, but works
        for(var i=0;i<blocks;i++){
            var blockFunc = (function(i){
                    var innerFunc = function(){
                    var blockLen=Math.min(32768,len-(i*32768));
                    stream.push(i==(blocks-1)?0x01:0x00);
                    Array.prototype.push.apply(stream, blockLen.bytes16sw() );
                    Array.prototype.push.apply(stream, (~blockLen).bytes16sw() );
                    var id=imageData.slice(i*32768,i*32768+blockLen);
                    Array.prototype.push.apply(stream, id );
                }
            return innerFunc;
            }(i));
            tr.AddTask(blockFunc);
        }

IMO, inlined functions add clutter. I think it's clearer and cleaner to use a named function.

function makeBlockFunc(i) {
    return function(){
        var blockLen=Math.min(32768,len-(i*32768));
        stream.push(i==(blocks-1)?0x01:0x00);
        Array.prototype.push.apply(stream, blockLen.bytes16sw() );
        Array.prototype.push.apply(stream, (~blockLen).bytes16sw() );
        var id=imageData.slice(i*32768,i*32768+blockLen);
        Array.prototype.push.apply(stream, id );
    };
}

Then invoke the function in the loop, passing i.

var tr = new taskRunner();

for(var i=0;i<blocks;i++){
    tr.AddTask(makeBlockFunc(i));
}

Also, you didn't really need an inner and outer function. You could have done the .AddTask() directly. Here the returned function is added directly into the tr.


Iterating through a collection of objects Subject

I have to iterate through a collection of objects (in this case IGrouping<string, SupplyDemand>). So I could do this easyly using a query with LINQ.

Code

    public IEnumerable<string> getParentPegs(IGrouping<string, SupplyDemand> data)
    {
        IEnumerable<string> query =
            from d in data
            where d.Source.ToLower() == "make"
            select d.Part;

        return query;
    }

Problem

I'm using this function inside a loop in a VSTO Excel project and it's pretty slow, besides it stops running (memory leak). So I came up with the following code:

Improved Code

    public List<string> getParentPegs(IGrouping<string, SupplyDemand> data)
    {
        List<string> result = new List<string>();

        var enumerator = data.GetEnumerator();
        while (enumerator.MoveNext())
        {
            SupplyDemand obj = enumerator.Current;
            if (obj.Source.ToLower() == "make")
            {
                result.Add(obj.Peg);
            }
        }

        return result;
    }

Question

Is there any other way to improve this?

You can do the same with just a foreach loop instead of reading the enumerator.

You can use the String.Compare method to do a case insensetive compare instead of creating a lowercase version of each string that you compare.

Improved improved code:

public List<string> getParentPegs(IGrouping<string, SupplyDemand> data) {

  List<string> result = new List<string>();

  foreach (SupplyDemand obj in data) {
    if (String.Compare(obj.Source, "make", true) == 0) {
      result.Add(obj.Peg);
    }
  }

  return result;
}

Note: In your first code you are getting the property Part, but in the second you are getting the property Peg instead.


LeetCode: Best Time to Buy and Sell Stock II https://leetcode.com/problems/best-time-to-buy-and-sell-stock-ii/

Please review for performance.

  Say you have an array for which the i-th element is the price of a
  given stock on day i.

  Design an algorithm to find the maximum profit. You may complete as
  many transactions as you like (i.e., buy one and sell one share of the
  stock multiple times).

  Note: You may not engage in multiple transactions at the same time
  (i.e., you must sell the stock before you buy again).

Example 1:

Input: [7,1,5,3,6,4]
Output: 7
Explanation: Buy on day 2 (price = 1) and sell on day 3 (price = 5), profit = 5-1 = 4.
             Then buy on day 4 (price = 3) and sell on day 5 (price = 6), profit = 6-3 = 3.
Example 2:

Input: [1,2,3,4,5]
Output: 4
Explanation: Buy on day 1 (price = 1) and sell on day 5 (price = 5), profit = 5-1 = 4.
             Note that you cannot buy on day 1, buy on day 2 and sell them later, as you are
             engaging multiple transactions at the same time. You must sell before buying again.
Example 3:

Input: [7,6,4,3,1]
Output: 0
Explanation: In this case, no transaction is done, i.e. max profit = 0.

using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace ArrayQuestions
{
    /// <summary>
    /// https://leetcode.com/explore/featured/card/top-interview-questions-easy/92/array/564/
    /// </summary>
    [TestClass]
    public class BestTimetoBuyandSellStockII
    {
        [TestMethod]
        public void AnswerIs7()
        {
            int[] prices = {7, 1, 5, 3, 6, 4};
            Assert.AreEqual(7, MaxProfit(prices));
        }

        public int MaxProfit(int[] prices)
        {
            int max = 0;
            for (int i = 0; i < prices.Length-1; i++)
            {
                if (prices[i] < prices[i + 1])
                {
                    max += prices[i + 1] - prices[i];
                }
            }
            return max;

        }
    }
}

Performance

Your solution is a single pass with time complexity \$O(n)\$. It's a small variation of the proposed solution.

Conventions

use static when no instance state/operations are used
use a clear, non-abbreviated method name CalculateMaximumProfit
always guard against bad user input in public methods
prefer the use of var when the type is obvious when reading the code
prefer whitespace between operands and operators prices.Length - 1
clean up waisted whitespace (in this case, after the return statement)

applied to your code:

 public static int CalculateMaximumProfit(int[] prices)
 {
     prices = prices ?? throw new ArgumentNullException(nameof(prices));
     var max = 0;
     for (var i = 0; i < prices.Length - 1; i++)
     {
         if (prices[i] < prices[i + 1])
         {
             max += prices[i + 1] - prices[i];
         }
     }
     return max;
 }


Exiting a Promise early I'm using a Promise chain to write some data to a file, execute a command on it, inspect its results, and send the results, using Express.js. The thing about my function is that I need to be able to exit early from multiple conditions.

The following code comes from an Express.js handler. my-command is a command that takes one input file, and outputs either resultWithBadName.txt, when the output is only one "page," and result-{id}.txt, when the output has multiple pages.

var cp = require('child_process'),
    Promise = require('bluebird');

var results,
    id = req.param.id; // from user

promisifiedWriteFile('src.txt', 'foobar' /* real content from user */).catch(function (err) {
    return Promise.reject({ text: 'Cannot write file', err: err });
}).then(function() {
    return exec(
        'my-command src.txt'
    ).catch(function (err) {
        // Considered a user error.
        res.send({
            error: err.stderr,
            id: id,
            pages: 0
        });
        return Promise.reject('DONE');     // UGLY
    });
}).then(function (ret) {
    results = ret;
    // Check if output only has one page
    return promisifiedStat(
        'resultWithBadName.txt'
    ).catch(function () {
        // Nope, has multiple pages. Coutn how many pages there are.
        return countPagesInOutput(id).then(function (pages) {
            res.send({
                output: results.stderr,
                id: id,
                pages: pages
            });
            return Promise.reject('DONE'); // UGLY
        });
    });
}).then(function () {
    // Yep, only has one page. Let's make the result have standard naming scheme.
    return promisifiedRename(
        'resultWithBadName.txt',
        'result' + id + '.txt'
    ).catch(function (err) {
        return Promise.reject({ text: 'file rename failed', err: err });
    });
}).then(function () {
    res.send({
        output: results.stderr,
        id: id,
        pages: 1
    });
}).catch(function (err) {
    if (err === 'DONE') return;         // UGLY
    res.status(500).send('Internal server error: ' +
        (err.text || err.message || '')
    );
    console.error(err.err || err);
}).catch(console.error);

function exec() {
    var args = [].slice.apply(arguments);
    return new Promise(function (fulfill, reject) {
        args.push(function (err, stdout, stderr) {
            if (err) {
                err.stderr = stderr;
                return reject(err);
            }
            fulfill({
                stdout: stdout,
                stderr: stderr
            });
        });
        cp.exec.apply(cp, args);
    });
};

I have read somewhere that one can chain the thens inside a onFulfilled handler, but that adds even more pyramids to my code, which Promises are supposed to prevent…

Now my question is, is there a way to remove the ugly 'DONE' hack?

I realize this question is old now, but it's intriguing and still getting a number of views, so I though I would give it a shot.
Now days there's a very easy solution to this: Just use async/await syntax instead so that you can use normal flow-control statements to get the job done. However, I'll write this under the assumption that this task needs to be done in legacy es5.
Lets take a simplified example. A lot of the links in your promise chain tended to have a structure such as this:
firstTask()
    // before...
    .then(function () {
        return asyncTask()
            .catch(function (err) {
                // handle catch
                return Promise.reject('DONE'); // This gets caught at the end of the promise-chain and ignored
            });
    })
    // after...

You're right in assuming that it's not such a great idea to be rejecting with a sentinel like that in order to skip over future .then()s. The proper solution is to restructure this in order to make it so there's nothing that needs to be skipped over. The following would have the same effect, but without needing to reject with a sentinel.
firstTask()
    // before...
    .then(asyncTask)
    .then(
        function () {
            // after ...
        },
        function () { /* handle catch */ }
    )

While this does fix the issue, you'll notice an important problem: The // after ... part is now nested a couple levels deep. Each time we need this skip-to-the-end behavior, we will have to add even further nesting, and that's just gross  - we just got ourselves right back into the callback hell that promises were designed to prevent. This is an unfortunate limitation to promises, however, even callback hell can be tamed with some good coding practices. If we split up the logic into multiple functions, then we can avoid having too much nesting happening in one place.
firstTask()
    // before...
    .then(asyncTask)
    .then(
        doAfter,
        function () { /* handle catch */ }
    )

function doAfter() {
    // after ...
}

And now we have a recipe to fix up this code! Below I have an (untested) version of the original code that's been fixed up so it doesn't need to break out of a promise chain anymore. I did make one change that simplified things a bit: I took out the places where you would catch an error, then re-reject it like this Promise.reject({ text: 'file rename failed', err: err }), which you later re-caught, logged the error, and returned the text to the user. The end-user doesn't need to know what specific internal detail went wrong, simply giving back "Internal Server Error" should be good enough.
var id = req.param.id;

promisifiedWriteFile('src.txt', 'foobar')
    .then(function () { return exec('my-command src.txt') })
    .then(onExecute, onExecuteError)
    .then(function (response) { res.send(response) })
    .catch(function (err) {
        res.status(500).send('Internal server error');
        console.error(err.message);
        console.error(err.stack);
    });

function onExecute (ret) {
    var fileHasBadName;
    return isFileNameBad()
        .then(function (fileHasBadName_) {
            fileHasBadName = fileHasBadName_;
            if (fileHasBadName) return fixBadFileName(id);
        })
        .then(function () {
            return fileHasBadName ? 1 : countPagesInOutput(id);
        })
        .then(function (pages) {
            return { output: ret.stderr, id: id, pages: pages };
        });
}

function onExecuteError (err) {
    return { error: err.stderr, id: id, pages: 0 };
}

function isFileNameBad() {
  return promisifiedStat('resultWithBadName.txt')
    .then(function () { return true }, function () { return false })
}

function fixBadFileName(id) {
    return promisifiedRename('resultWithBadName.txt', 'result' + id + '.txt')
}


Project Euler Problem #7 in Python (10001st prime number) I have managed to solve the 7th Project Euler problem, however I think it can be improved by a lot, I am by no means a professional programmer or even consider myself really good at it. Any improvements / suggestions would be really helpful.
Problem statement:

By listing the first six prime numbers: 2, 3, 5, 7, 11, and 13, we can see that the 6th prime is 13.
What is the 10 001st prime number?

This is my solution.
counter = 2
n = 10001
for i in range(3, 1000000, 2):
 k = 1
 while k < i:
  k += 2
  if i % k == 0:
   break
  if k + 2 == i:
   counter += 1
  if counter == n:
   print(i)
   raise ZeroDivisionError

The program does skip 2 and 3, in an attempt of mine to make it faster. At the end I raise an error in order to stop the program from looping.

Upper bound for p_n

There is a known upper bound for the n-th prime.

It means that you don't need to guess how large it could be. upper_bound_for_p_n(10001) tells us in less than a micro-second that the desired number cannot be larger than 114320.

You just need to apply the Sieve of Erathosthenes up to 114320 and you're done:

from math import log, ceil

def find_primes(limit):
    nums = [True] * (limit + 1)
    nums[0] = nums[1] = False

    for (i, is_prime) in enumerate(nums):
        if is_prime:
            yield i
            for n in range(i * i, limit + 1, i):
                nums[n] = False

def upper_bound_for_p_n(n):
    if n < 6:
        return 100
    return ceil(n * (log(n) + log(log(n))))

def find_n_prime(n):
    primes = list(find_primes(upper_bound_for_p_n(n)))
    return primes[n - 1]

It calculates the 10001th prime in 15ms on my computer, compared to 35s for your code.


Code for finding repeated entries with different data project is the data frame. For the purpose of the code, HOUSE.NO is a column of the type character, and NO..OF.FAMILY.MEMBER is another column of the type integer. My aim was to find out the house numbers which repeated, and then find out if the no of family members reported for each of the house matched, and identify the sets for which it didn't.

x<-1
matr<-NULL
matr2<-NULL
matr3<-NULL
r<-NULL
index<-NULL
repeat{

  y<-project$HOUSE.NO[-x]==project$HOUSE.NO[x]
  if (any(y)){
    r<-which(grepl(project$HOUSE.NO[x],project$HOUSE.NO))
    if(length(r)==2){
    check<-project$NO..OF.FAMILY.MEMBER[r[1]]!=project$NO..OF.FAMILY.MEMBER[r[2]]
    if(check){matr<-c(matr,r)}
    }
    if (length(r)==3){
      check2<-length(levels(factor(project$NO..OF.FAMILY.MEMBER[c(r[1],r[2],r[3])])))>1
      if(check2){matr2<-c(matr2,r)}
    }
    if (length(r)==4){
      check3<-length(levels(factor(project$NO..OF.FAMILY.MEMBER[c(r[1],r[2],r[3],r[4])])))>1
      if(check3){
      matr3<-c(matr3,r)}}
    if (length(r)>4&project$HOUSE.NO[x]!=""){index<-c(index,r)
    }
  }
  x<-x+1
  if(x>392){
    m1<-matrix(matr, ncol=2, byrow = TRUE)
    m2<-matrix(matr2, ncol=3, byrow = TRUE)
    m3<-matrix(matr3, ncol=4, byrow=TRUE)
    break
  }
}

The extra argument while computing index is to avoid a false entry when the HOUSE.NO is "", which is true in my data frame for 3 entries. There are 393 entries, hence the final caveat before break.

The concerns are:

I am an absolute beginner in R, and the functions used here are almost all I know.
This code only finds if in the case of the same number repeated more than twice, only if the entire set has the same family members. I couldn't find the row indices of only the cases which mismatched. Currently, the output includes the entire set.
Do let me know tips on how to make this simpler. As it stands, I found this code to be quite a bit complicated.

(let me know if more details specific to the data frame/variables I am working with are needed. Or if the question is not suited to the site)

ADDENDUM

 HOUSE.NO NO..OF.FAMILY.MEMBER
1    14/274                    6
2    14/259                    6
3    14/217                    5
4    14/258                    4
5    14/306                    5
6    14/300                    8
7     14/96                    4
8    14/166                    4
9     14/69                    5
10    14/68                    2

And the expected output is just the row numbers/house.no. which fulfill the aforementioned criteria. Currently, the matrix outputs are as below. The same set is repeated in the matrix again (twice in m1, thrice in m2..etc).

 m1
      [,1] [,2]
 [1,]   20  380
 [2,]   36   68
 [3,]   37  340
 [4,]   64  191
 [5,]   36   68
 [6,]   72  329
 [7,]   88  218
 [8,]  103  199
 [9,]  111  278
[10,]  125  214
[11,]  135  387
[12,]  149  196
[13,]   64  191
[14,]  149  196
[15,]  103  199
[16,]  125  214
[17,]  215  320
[18,]   88  218
[19,]  248  317
[20,]  111  278
[21,]  310  350
[22,]  248  317
[23,]  319  324
[24,]  215  320
[25,]  319  324
[26,]   72  329
[27,]   37  340
[28,]  310  350
[29,]   20  380
[30,]  135  387
> m2
      [,1] [,2] [,3]
 [1,]   43  258  354
 [2,]   65  219  269
 [3,]  169  322  323
 [4,]   65  219  269
 [5,]   43  258  354
 [6,]   65  219  269
 [7,]  169  322  323
 [8,]  169  322  323
 [9,]   43  258  354
> m3
      [,1] [,2] [,3] [,4]
 [1,]    2   84  211  347
 [2,]    2   84  211  347
 [3,]   99  100  101  363
 [4,]   99  100  101  363
 [5,]   99  100  101  363
 [6,]  180  185  260  263
 [7,]  180  185  260  263
 [8,]    2   84  211  347
 [9,]  180  185  260  263
[10,]  180  185  260  263
[11,]    2   84  211  347
[12,]   99  100  101  363

Your code uses several constructs that have a bit of smell in R.

Foremost is how you write the loop. An immediate replacement would be to replace the repeat with a for(i in seq_len(nrow(project))) (especially the hard coded 394 reeks)

Also you don't need to initialize the variables you will not use outside the loop - that will just prevent them from getting cleaned up after the loop.

A more R like way would be to use some higher level verbs that operate on the whole table. Such as provided by dplyr. Supposing you have something like an id in the lines you would write something like

left_join(project, project, on=c('HOUSE.NO')) %>% filter(id.1<id.2)

such commands are usually easier to read and usually much faster than looping.


Project Euler 10: find the sum of all the primes below two million This is question #10 from Project Euler:

  The sum of the primes below 10 is 2 + 3 + 5 + 7 = 17. Find the sum of all the primes below two million.

I just started programming and read that a Sieve of Eratosthenes is the fastest way to find primes however this code is taking forever to run. How can I make this go faster?

end = False
all = list(range(2, 2000000))
delnum = 2
while all.index(delnum)+1 <= len(all) and end == False:
    for x in all:
        if x % delnum == 0 and delnum != x:
            all.remove(x)
    if all.index(delnum)+1 == len(all):
        end = True
    if all.index(delnum)+1 <= len(all) and end == False:
        delnum = all[all.index(delnum) + 1]
print(sum(all))

Most of the time the line all.remove(x) is running, and it is executed for all non-prime numbers, which is most of the numbers.

Moreover, your remove items by value, which means that each time you remove it, a searching by value is performed on the list, making it even slower.

You can avoid this if, instead of removing the non-primes, you

set zero in their place and access the list by index
build a list for collecting the identified primes

But I would suggest a completely different approach:

Instead of starting with list of all numbers all = list(range(2, 2000000)), and then removing the non-primes, it is better just start with empty list and adding the number if it is prime. Or if you need only the sum, don't save the primes at all; just sum them.

if you want to check if the number \$n\$ is prime, you don't need to check division for all the numbers. Check only from 2 to \$\sqrt{n}\$.


Insertion of a node in M-way search tree I am currently studying data structures and i am looking to see if my implementation of the insertion method in an M-way search tree is good and i am looking forward to improving my coding approach.

There are some basic cases in inserting a node in an M-way search tree:-

Basic Case: If there is still space in the node for new key.

Otherwise,

Case 1: If the data is smaller than the first key of the node then we move towards the first child.

Case 2: If the data is greater than the last node then we move towards the last child.

Case 3: According to the property we check accordingly if the value ranges between the values of two keys.

Code:

#include<iostream>
using namespace std;

struct mWay{
    int index=-1;
    static const int m=3;
    int k[m-1];
    mWay *child[m]={NULL};
};

mWay* createNode(int value){
    mWay *node=new mWay;
    (node->index)++;
    node->k[node->index]=value;
    return node;
}

void sorting(mWay *s){      //using bubble sort..
    int temp;
    for(int i=0;i<(s->index);i++){
        for(int j=0;j<(s->index);j++){
            if((s->k[j])>(s->k[j+1])){
                temp=(s->k[j]);
                (s->k[j])=(s->k[j+1]);
                (s->k[j+1])=temp;
            }
        }
    }
}

mWay* Insert(int data,mWay *root){
    if(root==NULL){         //creating a new mWay tree...
        root=createNode(data);
        return root;
    }
    else{                   //updating the mWay tree...
        mWay *ptr=root;
        mWay *par=NULL;
        int store;          //we will store the index of next child node in this variable (in the last case)...
        while(ptr!=NULL){
            par=ptr;
            if((ptr->index)<(ptr->m)-2){    //checking to see if we can insert more keys in the current node...
                (ptr->index)++;
                ptr->k[ptr->index]=data;
                sorting(ptr);               //sorting keys..
                return root;
            }
            else{       //we have to move to the next node..
                if(data<ptr->k[0]){     //if the data is smaller than the first key of the node then we move towards the first child...
                    ptr=ptr->child[0];
                }
                else if(data>ptr->k[(ptr->m)-2]){       //if the data is greater than the last node then we move towards the last child...
                    ptr=ptr->child[(ptr->m)-1];
                }
                else{       //according to the property we check accordingly if the value ranges between the values of two keys ...
                    for(int i=1;i<(ptr->m)-1;i++){
                        if(data>(ptr->k[i-1]) && data<(ptr->k[i])){
                            store=i;
                            ptr=ptr->child[i];
                            break;
                        }
                    }
                }
            }
        }   //end of while loop which means we just have to insert the node now...
        if(data<(par->k[0])){
            par->child[0]=createNode(data);
        }
        else if(data>(par->k[(par->m)-2])){
            par->child[(par->m)-1]=createNode(data);
        }
        else{
            par->child[store]=createNode(data);
        }
        return root;
    }
}

This looks more like C than C++.
The only features of C++ I see used are #include <iostream>, using namespace std; and new.
Since this is flagged c++, I will review it as such, but if it is intended to use with C, a review for that language might give better insights.

Don't reinvent the wheel
The C++ standard library provides many features that would help. Need a sorting algorithm? Use std::sort. Need to find something in a container? Use std::find, std::lower_bound, std::upper_bound or some other algorithm that suits your needs!
Naming
Many variables have very short and cryptic names. This makes the code hard to read.
Example: What does mWay::k stand for?
Issues

Elements of <iostream> aren't used anywhere.

using namespace std; is discouraged, especially in headers. If someone else includes that header, it might cause ambiguities in their code!

Design
C++ has classes. Why not use them to define the tree, instead of a loose set of free standing functions?
For example, a rough outline could look like this:
#include <array>
#include <memory>
#include <algorithm>

template<typename ValueType, size_t Arity>
class nary_search_tree {
    static_assert(Arity >= 2u, "Tree needs an arity greater than or equal to 2!");

    class node {
        size_t size = 0;
        std::array<ValueType, Arity-1> values;
        std::array<std::unique_ptr<node>, Arity> children;

        void insert_in_order(ValueType value) {
            auto insert_index = find_child_index(value);

            if(insert_index == size) {
                values[insert_index] = value;
                ++size;
                return;
            }

            std::copy_backwards(&values[insert_index], &values[size-1], &values[size]);
            values[insert_index] = value;
            ++size;
        }

        void find_child_index(ValueType value) const {
            for(auto i = 0u; i < size; ++i) {
                if(values[i] > value) return i;
            }

            return size;
        }

    public:
        void insert(ValueType value) {
            if(index < values.size()) {
                insert_in_order(value);
                return;
            }

            auto index = find_child_index(value);
            if(!children[index]) {
                children[index] = std::make_unique<node>();
            }

            children[index]->insert(value);
        }
    };

    std::unique_ptr<node> root = nullptr;

public:
    void insert(ValueType value) {
        if(!root) root = std::make_unique<node>();

        root->insert(value);
    }
};

Makes reasoning much easier, doesn't it? And as a bonus, it's much more generic, too!
Note: This code requires C++11 or later to work.

Of course, this is just the beginning... This code could be improved, e.g. by using const ValueType& to prevent copies and by using overloads for ValueType&& to support movable types. Also, other search tree functionality is missing, like looking up an element


Extract unique terms from a PANDAS series Background

I have process tons of DataFrames with shapes of ~230 columns x ~2000-50000+ rows. Here is an extremely simplified example;

 numbers                colors
0    0.03620894806802     1xYellow ; 2xRed
1  0.7641262315308163  2xYellow ; 1xOrange
2  0.5607449770945651   3xYellow ; 2xGreen
3  0.6714547913365702     1xYellow ; 1xRed
4  0.8646309438322237     2xYellow ; 1xRed

Problem

I need to break the colors column down to a set that looks like this;
{'Green', 'Orange', 'Red', 'Yellow'}. The example code below can do this but it is painfully slow on huge DataFrames.

import re
import pandas as pd
import numpy as np
# Generating example data
color = ["1xYellow ; 2xRed ",
         "2xYellow ; 1xOrange ",
         "3xYellow ; 2xGreen ",
         "1xYellow ; 1xRed ",
         "2xYellow ; 1xRed "]
numbers = np.random.rand(len(color))
ex_df = pd.DataFrame(np.array([numbers,color]).T,
                     columns = ["numbers","colors"])
# Compile the regex to apply with findall
rx = re.compile("x(\w+)\s")
just_colors = ex_df.colors.apply(rx.findall)
# Below is the painfully slow operation that needs optimization.
present_colors = set(sum(just_colors,[]))

Question

Is there a better method out there for pulling unique terms out of a pandas series?

It doesn't look like you really need regular expressions.  This construct just using basic string operations is about 10x faster than the construct with the regular expressions:

present_colors = set()
for value in ex_df['colors'].values:
    for color in [x.strip() for x in value.split(';')]:
        present_colors.add(color.split('x')[-1])

And a bit faster yet, go with the same code as a generator using itertools:

import itertools as it
present_colors = set(it.chain.from_iterable(
    ([color.split('x')[-1].strip() for color in value.split(';')]
     for value in ex_df['colors'].values)))


Performance and Readability Improvements for HTML Parser with BeautifulSoup This function takes as an argument a json file (could contain anything in json format, since I scrap hundreds of random pages) and returns a list of dictionaries where a URL is mapped to its corresponding headers, based on the extraction of headers using beautifulsoup and a regex pattern.

I'm looking for suggestions regarding performance readability and clarity.

Following  my 1st iteration I improved my code and here is the result:

import json
from tqdm import tqdm
import re
from bs4 import BeautifulSoup
import csv
import string

"Load HTML body, and fetch headers"
def get_headers_from_json(local_path):
    """
    The function takes a json file with html_body and returns a list of headers.
    It parses the titles, based on tags starting with 'h' + num.
    """
    data = json.loads(open(local_path).read())
    pattern = re.compile(r"^h[0-9]$")
    headers_urls = []
    printable = set(string.printable)
    for dict in tqdm(data):
        headers = []
        for val in dict.values():
            soup = BeautifulSoup(val, 'html.parser')
            url = dict.values()[0]
        for element in soup.find_all(pattern):
            element = element.get_text().strip().encode('utf-8')
            element = filter(lambda word: word in printable, element)
            headers.append(element)
        cleaned_data = {"url": url, "headers": headers}
        headers_urls.append(cleaned_data)
    return headers_urls

Example of json input:

[["<body class=\" catalog-category-view categorypath-sale-html category-sale\">\n<script type=\"text/javascript\">\n//<![CDATA[\nif (typeof(Varien.searchForm) !== 'undefined') {\n    Varien.searchForm.prototype._selectAutocompleteItem = function(element) {\n        var link = element.down();\n        if (link && link.tagName == 'A') {\n            setLocation(link.href);\n        } else {\n            if (element.title){\n                this.field.value = element.title;\n            }\n            this.form.submit();\n        }\n    };\n    Varien.searchForm.prototype.initAutocomplete = function(url, destinationElement) {\n        new Ajax.Autocompleter(\n            this.field,\n            destinationElement,\n            url,\n            {\n                paramName: this.field.name,\n                method: 'get',\n                minChars: 2,\n                frequency: .3,\n                updateElement: this._selectAutocompleteItem.bind(this),\n                onShow : function(element, update) {\n                    if(!update.style.position || update.style.position=='absolute') {\n                        update.style.position = 'absolute';\n                        Position.clone(element, update, {\n                            setHeight: false,\n                            offsetTop: element.offsetHeight\n                        });\n                    }\n                    Effect.Appear(update,{duration:0});\n                }\n\n            }\n        );\n    };\n    Autocompleter.Base.prototype.markPrevious = function() {\n        if (this.index > 0) {\n            this.index--;\n        } else {\n            this.index = this.entryCount - 1;\n        }\n        var entry = this.getEntry(this.index);\n        if (entry.select('a').length === 0) {\n            this.markPrevious(); // Ignore items that don't have link\n        }\n    };\n    Autocompleter.Base.prototype.markNext = function() {\n        if (this.index < this.entryCount - 1) {\n

There are multiple things you can do to speed things up:

faster JSON parsing - try out ujson or simplejson; or even simplejson on PyPy
use lxml instead of html.parser (requires lxml to be installed)
do not parse the complete HTML, parse only what you need with SoupStrainer:

pattern = re.compile(r"^h[0-9]$")
parse_only = SoupStrainer(pattern)
soup = BeautifulSoup(data, "lxml", parse_only=parse_only)

You can then do simple soup.find_all() (or via a shortcut - soup()) since you'll only have header elements in your HTML soup.

Here is a demonstration of what SoupStrainer would do:

In [1]: import re

In [2]: from bs4 import BeautifulSoup, SoupStrainer

In [3]: data = """
   ...:
   ...:
Some Paragraph 1

   ...:

   ...:
Some Paragraph 2

   ...:

   ...:
   ...:
   ...:

   ...:
Some Paragraph 3

   ...:

   ...:
Some Paragraph 4

   ...:
   ...:
Some Paragraph 2

   ...:
   ...: """

In [4]: pattern = re.compile(r"^h[0-9]$")

In [5]: parse_only = SoupStrainer(pattern)

In [6]: soup = BeautifulSoup(data, "lxml", parse_only=parse_only)

In [7]: print(soup.prettify())

 Some Paragraph 1

 Some Paragraph 2

 Some Paragraph 3

 Some Paragraph 4

 Some Paragraph 2


Constructing query conditions from answers that may be null I was thinking about this after recently reading about the new Null Conditional Operators in C# 6.0. So here is my code that I am currently working on...

    if (!string.IsNullOrWhiteSpace(question2Answer))
    {
        stringbuild.Append($" and ProdUsers = {Regex.Match(question2Answer, @"\d+").Value}");
    }
    if (!string.IsNullOrWhiteSpace(question3Answer))
    {
        stringbuild.Append($" and ProdFullyOnline = '{(int)Enum.Parse(typeof(YesNo), question3Answer, true)}'");
    }
    if (!string.IsNullOrWhiteSpace(question4Answer))
    {
        stringbuild.Append($" and ProdMultiCurrency = '{(int)Enum.Parse(typeof(YesNo), question4Answer, true)}'");
    }
    if (!string.IsNullOrWhiteSpace(question5Answer))
    {
        stringbuild.Append($" and ProdPrice <= {question5Answer}");
    }

As you can see, it is fairly repetitive. So my question, is would there be a better way of checking whether questionXAnswer is null or not and then using the not null value on the string builder. And it would be nice to use the new null conditional operators with C# 6.0, if possible.

The main reason for me posting this however, is to solve the repetition problem if possible. And obviously, any other ways I could improve this would be appreciated too!

Since you are doing the same operation over and over, you can put your strings in an array or list and use a mapping function.

For example:

//this struct makes it easy to initialize the list of values
public struct Tuple
{
  public String Value;
  public String InterpolatedValue;
  public Tuple(string val, string def)
  {
    Value = val;
    InterpolatedValue = def;
  }
}

//dummy answers to the questions
string question2Answer = "aaa";
string question3Answer = "bbb";
string question4Answer = "";
string question5Answer = "ddd";

//initialize a list of answer/interpolated answer strings
List<Tuple> list = new List<Tuple>()
{
  new Tuple(question2Answer, $" and ProdUsers = {question2Answer}"),
  new Tuple(question3Answer, $" and ProdFullyOnline = '{question3Answer}'"),
  new Tuple(question4Answer, $" and ProdMultiCurrency = '{question4Answer}'"),
  new Tuple(question5Answer, $" and ProdPrice <= {question5Answer}"),
};

//loop over list, checking for non empty answer
//this line and the next takes the place of almost all of your original code
IEnumerable<string> tempList= list.Select(t => (string.IsNullOrWhiteSpace(t.Value) ? string.Empty : (t.DefaultValue)));

//concat results into a single string
string text = string.Join("", tempList);

Console.WriteLine(text);

Outputs: " and ProdUsers = aaa and ProdFullyOnline = 'bbb' and ProdPrice <= ddd"

I had to add some boilerplate code to make the example work, but I think one gets the idea...


Breakable Toy: Go CLI for Gitlab I started to learn programming in Go and came up with a Breakable Toy, which is a CLI for Gitlab. To get something up and running fast, I used some Go libraries:

Ginkgo for testing
Cobra for basic CLI stuff
Sling for HTTP requests and JSON mapping

The project can be found in this repository on Github.

I am looking for any feedback / code review that could help me to better understand the concepts and conventions of Go. I come from the Java world and have the impression that I try to stick too much on the object oriented world. So any feedback concerning code style and best-practices is highly appreciated.

Besides general feedback, here are some specific questions:

1. Error Handling in Go

Being used to the try catch mechanism in Java, the _, err := mechanism in Go seems a little unhandy to me. To me it looks as if I have to implement the "bubble up" of exceptions all by myself. Example:

// HTTP client
func (client *GitlabClient) Do(req *http.Request, value interface{}) (*http.Response, error) {
    resp, err := client.sling.Do(req, value, nil)
    return resp, err
}

// service
func (service *ProjectsService) List() (*[]model.Project, error) {
    // ...
    _, err = service.Client.Do(req, projects)
    if err != nil {
        return nil, err
    }
    return projects, nil
}

// CLI command
func(cmd *cobra.Command, args []string) error {
    projects, err := gitlabClient.Projects.List()
    if err != nil {
        return err
    }
    err = OutputJson(projects)
    return err
}

so there are really many LOCs only for error handling - is that the "way to go"?

2. Global Variables

The Cobra library seems to store values for CLI flags in global variables:

var id string

// ...

projectGetCmd.PersistentFlags().StringVarP(&id, "id", "i", "", "(required) Either ID of project or 'namespace/project-name'")
viper.BindPFlag("id", projectGetCmd.PersistentFlags().Lookup("id"))

This makes it really hard to figure out which flags belong to which command and it's easy to re-initialize the same variable. See group.go, line 31 for the full example.

Is there any rule of thumb / best practice of how to namespace variables in Go besides structs?

3. Mocking in Unit Tests

I wrote some integration tests with Ginkgo that also simulate an HTTP server. This works quite nicely. But now I wanted to test, whether my CLI commands are calling the expected service methods and therefore I wanted to mock the services classes in the Unit tests for the commands. I couldn't find a convincing way for how to mock objects in Go. Can you give me any hint / resources for that?

Disclaimer

I am not expecting a full code review of my application. But if you have any suggestions for improvement, I'd be glad the get them.

Yes, this is the way to do error handling in Go. By design, you can't let exceptions propagate to the callers "silently", you have to consider what makes sense at each step. I found that on large-scale projects, it makes it significantly easier to predict and test the error handling behavior of your code. It also forces you to put the error handling first, which (imho) results in more readable, less nested code.
Truly global variables don't exist in Go — if a package foo exports a variable Bar, other packages will have to call foo.Bar to access it. That being said, it pretty much never makes sense to do that. In the example you gave, cmd.go and project.go are in the same package, and all package-level variables are shared, so identifiers conflict. If you want this variable to be shared, it's more readable to put all functions that use it in the same file.
When you want to mock some of your functions, it's the sign that there should probably be an associated interface. You can then "mock" an interface easily, by re-implementing your interface in your tests and making it do whatever you want. I find this more readable and idiomatic than using complex frameworks.

Note on the last question: if your interface has many functions:

type Fooer interface {
    Foo1(string) (string, error)
    Foo2(int) (string, int)
    ...
    // Foo42() []byte
}

and you only want to test Foo1, you can use the following syntax

type fakeFooer struct {
    Fooer
    ret string
}

func (fi fakeFooer) Foo1(_ string) (string, error) {
     return fi.ret, nil
}

and not implement any other functions. fakeFooer{"bar"} will "implement" Fooer, calling Foo1 will do what you expect, and trying to call Foo2 on it will panic (nil pointer dereference).