Detecting if the system is idling The following code detects if the system is idling. It approaches the problem by using the Robot class to take a screenshot, waits for a while, and then takes another screenshot. It then compares screenshot 1 with screenshot 2. If  a certain amount of change is detected, it presumes the system is idling, else the system is active.

Please suggest code changes or even a different solution!

package base;

import java.awt.AWTException;
import java.awt.DisplayMode;
import java.awt.GraphicsDevice;
import java.awt.GraphicsEnvironment;
import java.awt.Rectangle;
import java.awt.Robot;
import java.awt.Toolkit;
import java.awt.image.BufferedImage;

public class CheckIdle extends Thread {
    private Robot robot;
    private double threshHold = 0.05;
    private int activeTime;
    private int idleTime;
    private boolean idle;
    private Rectangle screenDimenstions;

    public CheckIdle(int activeTime, int idleTime) {
        this.activeTime = activeTime;
        this.idleTime = idleTime;

        // Get the screen dimensions
        // MultiMonitor support.
        int screenWidth = 0;
        int screenHeight = 0;

        GraphicsEnvironment graphicsEnv = GraphicsEnvironment
                .getLocalGraphicsEnvironment();
        GraphicsDevice[] graphicsDevices = graphicsEnv.getScreenDevices();

        for (GraphicsDevice screens : graphicsDevices) {
            DisplayMode mode = screens.getDisplayMode();
            screenWidth += mode.getWidth();

            if (mode.getHeight() > screenHeight) {
                screenHeight = mode.getHeight();
            }
        }

        screenDimenstions = new Rectangle(0, 0, screenWidth, screenHeight);

        // setup the robot.
        robot = null;
        try {
            robot = new Robot();
        } catch (AWTException e1) {
            e1.printStackTrace();
        }

        idle = false;
    }

    public void run() {
        while (true) {
            BufferedImage screenShot = robot
                    .createScreenCapture(screenDimenstions);

            try {
                Thread.sleep(idle ? idleTime : activeTime);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }

            BufferedImage screenShot2 = robot
                    .createScreenCapture(screenDimenstions);

            if (compareScreens(screenShot, screenShot2) < threshHold) {
                idle = true;
            } else {
                idle = false;
            }
        }
    }

    private double compareScreens(BufferedImage screen1, BufferedImage screen2) {
        int counter = 0;
        boolean changed = false;

        // Count the amount of change.
        for (int i = 0; i < screen1.getWidth() && !changed; i++) {
            for (int j = 0; j < screen1.getHeight(); j++) {
                if (screen1.getRGB(i, j) != screen2.getRGB(i, j)) {
                    counter++;
                }
            }
        }

        return (double) counter
                / (double) (screen1.getHeight() * screen1.getWidth()) * 100;
    }

    public static void main(String[] args) {
        CheckIdle idleChecker = new CheckIdle(20000, 1000);
        idleChecker.run();
    }
}

In

        if (compareScreens(screenShot, screenShot2) < threshHold) {
            idle = true;
        } else {
            idle = false;
        }

Isn't this better as

        idle = compareScreens(screenShot, screenShot2) < threshHold;

Here, why initialize robot to null first? Isn't this given in the constructor? And why do you continue if there is an AWT exception? Doesn't it mean no GUI? perhaps your constructor should throw that exception. Otherwise, perhaps it is better to have a larger block for try catch. The reason is that the exception handling code is better away from the main sequence. You use robot in the run code. So in order for that to run, robot has to be non null. So catching the exception here is not correct.

    // setup the robot.
    robot = null;
    try {
        robot = new Robot();
    } catch (AWTException e1) {
        e1.printStackTrace();
    }

You also set idle to false, which is not really needed because it is initialized to false by java.

Why do you take an extra screen shot? Isn't this what you mean?

public void run() {
 try {
    BufferedImage screenShot = null;
    BufferedImage screenShotPrev
         = robot.createScreenCapture(screenDimenstions);
    while (true) {
        Thread.sleep(idle ? idleTime : activeTime);
        screenShot = robot.createScreenCapture(screenDimenstions);
        idle = compareScreens(screenShotPrev, screenShot) < threshHold;
        screenShotPrev = screenShot;
    }
 } catch (InterruptedException e) {
    ...
 }

Here, where do you use the changed? (as noted in comment to the question.)

private double compareScreens(BufferedImage screen1, BufferedImage screen2) {
    int counter = 0;
    boolean changed = false;

    // Count the amount of change.
    for (int i = 0; i < screen1.getWidth() && !changed; i++) {
        for (int j = 0; j < screen1.getHeight(); j++) {
            if (screen1.getRGB(i, j) != screen2.getRGB(i, j)) {
                counter++;
            }
        }
    }

    return (double) counter
            / (double) (screen1.getHeight() * screen1.getWidth()) * 100;
}

The for loop may be better as

 int[] s1 = ((DataBufferInt) screen1.getRaster().getDataBuffer()).getData();
 int[] s2 = ((DataBufferInt) screen2.getRaster().getDataBuffer()).getData();
 for(int i = 0; i < s1.length; i++)
     if (s1[i] != s2[i]) counter++;

Since it avoids multiple calls to getRGB.

And is there any chance of getHeight or getWidth returning 0?
Also, I assume that you are calling .run instead of .start only because you are testing?


HtmlAgilityPack is being slow HtmlAgilityPack is being really slow pulling back results. I have seen similar tools that get results a lot faster, but it's taking over a minute just to get the viewcounts on YouTube, and that's just with the first page of results.

Ideally I want to loop through multiple elements, but a nested loop wouldn't work for this code.

private void button1_Click(object sender, EventArgs e)
{
    //webBrowser1.Navigate("www.youtube.com/results?search_query=grindtime");
    StringBuilder output = new StringBuilder();
    string raw = "http://www.youtube.com/results?search_query=grindtime";
    HtmlWeb webGet = new HtmlWeb();
    webGet.UserAgent = "Mozilla/5.0 (Macintosh; I; Intel Mac OS X 11_7_9; de-LI; rv:1.9b4) Gecko/2012010317 Firefox/10.0a4";
    var document = webGet.Load(raw);
    var viewcount = document.DocumentNode.SelectNodes("//*[@class='viewcount']");
    //var videotitle = document.DocumentNode.SelectNodes("//a[@class='yt-uix-tile-link']");
    //var browser = document.DocumentNode.SelectNodes("//*[@class='viewcount']");

    if (viewcount != null)
    {
        foreach (var v in viewcount)
        {
             output.AppendLine(v.InnerHtml);

             ListViewItem lvi = new ListViewItem("#1");
             lvi.SubItems.Add("video title here");
             lvi.SubItems.Add(b.InnerHtml);
             //views
             //desc..
             //lid = link in desc yes/no
             listView1.Items.Add(lvi);
         }
     }

First, the web request itself will take some time.

To be able to profile it, maybe do some test with opening saved HTML file instead, to make sure that the parsing is your actual bottleneck, maybe even commenting the lines that create the result UI as well.

I haven't used HTML Agility Pack much, but I see you are using an XPATH selector there, and it only uses a CSS Class. I'd assume the path will scan the entire document to check eevery element in it if it has the desired class or not.

So, it may be a good idea to try to add some parents to the selector, so that it only looks for elements inside a given parent, best selected by ID or a tag. Those are general rules for browsers, I don't expect them to work the same with HTML Agility Pack, but they may provide some gain still.
Also, check if there are any options in the library to match a single element, and if so, find that element and go through its children till you find your desired elements. This may improve things. Also, if there are any strict mode or less tolerant parsing options, try turning those on if they don't break the parsing.

If you expect the page to be well structured and HTML valid, and parsing speed isn't getting any better however you enhance the selector, you may consider reverting to classic old Regex matching.

Also, I recommend separating the Windows Forms rendering from the loop. Maybe the bottleneck is the drawing (unlikely, but maybe), try to add all your elements to a list, and then outside the loop, add the list to the windows forms control.


Reverse string by separator Here is problem:
There is string (domain) divided by dots (like www.part1.partN.com)
Need to build reversed by dot string (like com.partN.part1.www)
Both strings are utf-8 encoded

My solution:

std::string reverseHost(const std::string& host)
{
    std::string ret;
    ret.reserve(host.size());

    size_t tail = host.size();
    size_t head = host.find_last_of('.');

    while (head != std::string::npos)
    {
        ret.append(host.c_str() + head + 1, tail - head - 1);
        ret += '.';

        tail = head;
        head = host.find_last_of('.', tail - 1);
    }

    ret.append(host.c_str(), tail);

    return ret;
}

Need to improve speed (or advice how to improve), raw memory and other tricks allowed. ty.

You've misspelt std::size_t.

Apart from that, the code seems reasonably straightforward if you always need to return a copy.

A two-pass algorithm (reverse the whole string, then reverse each of the individual components) might be faster, because you can operate in-place, without having to create a new string.  Of course, you'll only get the speed benefit if the caller doesn't need to retain the original (pass by value, and use std::move to control copying).


Java - Something similar to Abstract Factory? I have many repositories stored in a map. User chooses some of them and then a downloading begins. Repository may be one of 4 types (see code below).

/**
 * Data class that stores settings of a connection to repository.
 * Lombok's annotations are used to generate some methods.
 */
@Getter
@EqualsAndHashCode
public class Repository {
    public enum Type {
        FILE, FOLDER, GITHUB, BITBUCKET
    }

    private Type type;
    private String username;
    private char[] password;
    /*...*/
}

/**
 * A class that contains common stuff for all repository tools (such as connection to remote repository, download, logging, etc.)
 */
public abstract class AbstractRepositoryTools {

    /**
     * This method is implemented in child classes and actually it does the job.
     */
    protected abstract void doDownloadZip(Repository repo, String branch, Path saveTo);

    public static void downloadZip(Repository repo, String branch, Path saveTo) {
        AbstractRepositoryTools tools = Resolver.getToolsFor(repo);
        tools.doDownloadZip(repo, branch, saveTo);
    }

}

There are package-private classes that implement AbstractRepositoryTools for GitHub, BitBucket, and local repositories (folder or zip file).

/**
 * Resolver of a RepositoryTools implementation (with caching)
 */
class Resolver {

    // cache
    private static volatile BitbucketTools bitbucketTools = null;
    private static volatile GitTools gitTools = null;
    private static volatile LocalFileTools localFileTools = null;
    private static volatile LocalFolderTools localFolderTools = null;

    public static synchronized AbstractRepositoryTools getToolsFor(Repository repo) {
        switch (repo.getType()) {
            case GITHUB:
                if (gitTools == null)
                    gitTools = new GitTools();
                return gitTools;
            case BITBUCKET:
                if (bitbucketTools == null)
                    bitbucketTools = new BitbucketTools();
                return bitbucketTools;
            case FILE:
                if (localFileTools == null)
                    localFileTools = new LocalFileTools();
                return localFileTools;
            case FOLDER:
                if (localFolderTools == null)
                    localFolderTools = new LocalFolderTools();
                return localFolderTools;
            default:
                throw new NotImplementedException("Repository.TYPE = " + repo.getType());
        }
    }
}

Then I can take a repository from the map and download it like:

Repository repo = getRepo();
Path output = Paths.get("C:\\out")
AbstractRepositoryTools.downloadZip(repo, "develop", output);

It works but...

Does this code smell?
If yes, how to improve it?
If no, is there a name for such architecture pattern?

A few points about your resolver.

Since you're using an enum to specify the repository's Type and resolve the tools depending on that type, an EnumMap seems to be the simplest solution to resolving the specific tool implementation necessary.

Also caching objects that have virtually no creation cost seems overkill to me. You can just eagerly initialize the objects and then don't even have to care about multithreading problems:

class Resolver {

    private static final Map<Repository.Type, AbstractRepositoryTools> cache =
        new EnumMap<>(Repository.Type.class);

    static {
        cache.put(GITHUB, new GitTools());
        cache.put(FILE, new LocalFileTools());
        cache.put(FOLDER, new LocalFolderTools());
        cache.put(BITBUCKET, new BitbucketTools());
    }

    public static AbstractRepositoryTools getToolsFor(Repository repo) {
        if (cache.containsKey(repo.getType()) {
           return cache.get(repo.getType());
        }
        throw new NotImplementedException("Repository.TYPE = " + repo.getType());
    }
}

Additionally I wouldn't call the class AbstractRepositoryTools, but just RepositoryTools.

You can simplify the resolver call by only giving it the type of the repo (since that's the only thing necessary). Furthermore I would extract the doDownloadZip into a separate interface and then seal the RepositoryTools class by declaring it final

Other than that your code is very clear and simple. I assume the javadoc comments are only explanatory notes for reviewers, and that you excluded the actual javadoc.

And since you asked about the name of the pattern... This is a Facade


Making a scroll function more efficient in jQuery I have the following code, though I'm not sure it is efficient as it could be.

 $(window).scroll(function() {
    var scrollT = $(document).scrollTop();
    if (scrollT >= 180) {
        $("#primary-nav-wrapper").addClass("scroll");
        $("#primary-nav-wrapper li.front").addClass("active");

        $("#primary-nav-wrapper .search-wrapper").removeClass("active");
        $("#primary-nav-wrapper .search-field").blur();
    }
    else {
        $("#primary-nav-wrapper").removeClass("scroll");
        $("#primary-nav-wrapper .search-wrapper, #primary-nav-wrapper li.front").removeClass("active");

        $("#primary-nav-wrapper .search-field").blur();
    }
    if (scrollT >= 400) {$("a#to-top-link").addClass("active");}
    else {$("a#to-top-link").removeClass("active");}
});

Basically, what I do here is checking two if-clauses every time I scroll, but is it more resource-friendly to only check every few milliseconds? If so, how is this done? Or is it a better idea to re-write the if-else structure, e.g. if.. else if... else if... else?

I don't know if you mean general performance, but one thing I've heard helps a lot is to put all of your different selectors in variables outside of the function as it is apparently pretty expensive to make the selector calls again and again. I mean something like this (and I do mean "something like this" because I haven't tested even the basics, but the general idea is described here: http://www.artzstudio.com/2009/04/jquery-performance-rules/#cache-jquery-objects)

var scroll = {
     primary  : $("#primary-nav-wrapper"),
     front    : $("#primary-nav-wrapper li.front"),
     search_w : $("#primary-nav-wrapper .search-wrapper"),
     search_f : $("#primary-nav-wrapper .search-field"),
     to_top   : $("a#to-top-link")
}
$(window).scroll(function() {
    var scrollT = $(document).scrollTop();
    if (scrollT >= 180) {
        scroll.primary.addClass("scroll");
        scroll.front.addClass("active");

        scroll.search_w.removeClass("active");
        scroll.search_f.blur();
    }
    else {
        scroll.primary.removeClass("scroll");
        scroll.front.removeClass("active");

        scroll.search_f.blur();
    }
    if (scrollT >= 400) {scroll.to_top.addClass("active");}
    else {scroll.to_top.removeClass("active");}
});


LINQ for generating all possible permutations Permutate() is supposed to generate all possible permutations of the source sequence:

foreach(var s in "abc".Permutate())
    Console.WriteLine(s); // abc
                          // acb
                          // bac
                          // bca
                          // cab
                          // cba

Where:

    public static IEnumerable<string> Permutate(this string source) =>
        source.AsEnumerable().Permutate().Select(a => new string(a));

    public static IEnumerable<T[]> Permutate<T>(this IEnumerable<T> source)
    {
        return permutate(source, Enumerable.Empty<T>());
        IEnumerable<T[]> permutate(IEnumerable<T> reminder, IEnumerable<T> prefix) =>
            !reminder.Any() ? new[] { prefix.ToArray() } :
            reminder.SelectMany((c, i) => permutate(
                reminder.Take(i).Concat(reminder.Skip(i+1)).ToArray(),
                prefix.Append(c)));
    }

Any optimizations? Could it be shorter?

English language

These are relatively minor issues, but fixing them might help other people to use / maintain your code.

The verb corresponding to permutation is permute.
I'm pretty sure that reminder is intended as remainder.

Code

    public static IEnumerable<T[]> Permutate<T>(this IEnumerable<T> source)
    {
        return permutate(source, Enumerable.Empty<T>());
        IEnumerable<T[]> permutate(IEnumerable<T> reminder, IEnumerable<T> prefix) =>
            !reminder.Any() ? new[] { prefix.ToArray() } :
            reminder.SelectMany((c, i) => permutate(
                reminder.Take(i).Concat(reminder.Skip(i+1)).ToArray(),
                prefix.Append(c)));
    }

To return a permutation of source it is necessary to find all of the elements of source, so I think this is a case where the first thing the method should do is to fully evaluate source (e.g. with ToList() or ToArray()), and then work with that list rather than source. Apart from the efficiency benefits, that guarantees that all of the permutations will be permutations of the same size and elements, even if source has side-effects.

There are a couple of things you can then do with a list to make it much more efficient. Either you can use a standard "next permutation" algorithm (see Wikipedia: for arbitrary inputs it can be done by permuting an array of integers and copying the operations on the array of T) or you can recursively select an element from the first k, swap it to position k, recurse on k-1, and then swap it back. When k==0 you instead copy the entire array and yield the copy. This avoids building up chains of Append and the overheads of Take/Skip/Concat. I expect that the most efficient would be the "next permutation" approach, because it is non-recursive and so doesn't wrap coroutine in coroutine.


Checking whether strings are permutations of each other in Swift I'm solving this problem on Hacker Earth, just for practice. The exercise is to determine whether two given equal-length strings are permutations of each other.

Except for this one test case, which has 100 very long strings to compare, all other test cases are fine. But that one is taking too long to execute. What are possible optimizations that can be done?

Also it would be great if you can tell me what expensive operations I have used.

import Foundation;

var response:String = readLine()!

func createDictionary(from arrayOfCharacters: [Character], _ charCount: Int) -> Dictionary<Character, Int>{
    var dict : [Character: Int] = [:];
    for index in 0...charCount-1{
        if(dict.index(forKey: arrayOfCharacters[index]) != nil){
            dict[arrayOfCharacters[index]] = dict[arrayOfCharacters[index]]!+1;
        }
        else{
            dict[arrayOfCharacters[index]] = 1;
        }
    }
    return dict;
}

func checkEqual(_ x:String) -> Bool{
    var flag = false;
    var names = x.components(separatedBy: " ");
    //let charCount = names[0].characters.count;
    let charCount = (x.characters.count-1)/2;
    var dictionaryOne: [Character:Int] = [:];
    var dictionaryTwo: [Character:Int] = [:];

    if(names[1].characters.count == charCount){
        var charsOfStringOne = Array(names[0].characters);
        var charsOfStringTwo = Array(names[1].characters);

        //print(charsOfStringOne);
        //print(charsOfStringTwo);

        dictionaryOne = createDictionary(from: charsOfStringOne, charCount);
        dictionaryTwo = createDictionary(from: charsOfStringTwo, charCount);

        for char in "abcdefghijklmnopqrstuvwxyz".characters {
            if(dictionaryOne[char] != dictionaryTwo[char]){
                return false;
            }

        }

        if(dictionaryOne != dictionaryTwo){
            return false;
        }else{
            flag=true;
        }

        return flag;
    }else{
        return flag;
    }
}

var debug = false;
var count = Int(response)!
 for _ in 1...count {
    var line = readLine()!;
    if(checkEqual(line)){
        if(debug){
            print(line, "YES")
        }
        else {
            print("YES");
        }
    }else{
        if(debug){
            print(line, "NO")
        }
        else {
            print("NO");
        }
    }
}

First some general remarks:

There is no need to terminate statements with a semicolon in Swift.
The condition in an if-statement does not require parentheses, e.g.

if debug { ... }

You should be consistent about whether to start else on a new line
or not, I prefer

if condition {
    // ...
} else {
    // ...
}

Many of your variables are not mutated and should be declared as
constants, e.g.

let line = readLine()!

The overall structure can be improved. Why is the first input line
read at the top of the program, and the remaining lines at the bottom?
I would do the complete input parsing in the "main" part of the
program, which would then look like this:

let numTestCases = Int(readLine()!)!
for _ in 1...numTestCases {
    let strings = readLine()!.components(separatedBy: " ")
    if checkEqual(first: strings[0], second: strings[1]) {
        print("YES")
    } else{
        print("NO")
    }
}

(I have omitted the "debug" feature for simplicity.)

About the createDictionary() function: A loop like

for index in 0...charCount-1 {
    // ... do something with `arrayOfCharacters[index]` ...
}

can always be simplified using enumeration:

for char in arrayOfCharacters {
    // ... do something with `char` ...
}

But actually you don't have to create an array with all characters at all: You can pass the string itself to the function, and enumerate
its characters:

func createDictionary(from string: String) -> [Character:Int] {
    var dict = ...
    for char in string.characters {
        // insert or update `dict[char]` ...
    }
    return dict
}

The nil-coalescing operator ?? can be used to simplify the
"insert or update". Also it might be advantageous to create the
dictionary with the required capacity, in order to avoid rehashing.

The function would then look like this:

func createDictionary(from string: String) -> [Character:Int] {
    var dict: [Character: Int] = Dictionary(minimumCapacity: 26)
    for char in string.characters {
        dict[char] = (dict[char] ?? 0) + 1
    }
    return dict
}

About the func checkEqual() function:

A better name might be checkPermutation().
The var flag is not really needed because you always "early return" if the comparison fails.
There is no need to assign empty dictionaries to var dictionaryOne/Two
first because they are overwritten later.
You also don't have to compare the strings lengths because they are
always equal according to the problem statement.
The

for char in "abcdefghijklmnopqrstuvwxyz".characters { ... }

loop is superfluous, it suffices to compare the dictionaries

if dictionaryOne != dictionaryTwo { ... }

Putting it all together, that function reduces to

func checkPermutation(first: String, second: String) -> Bool {

    let dictionaryOne = createDictionary(from: first)
    let dictionaryTwo = createDictionary(from: second)
    return dictionaryOne == dictionaryTwo
}

Some syntactic sugar: If you replace the global function by
an extension method of String:

extension String {
    func isPermutation(of other: String) -> Bool {
        // ...
    }
}

then the check can be more expressively done as

if strings[0].isPermutation(of: strings[1]) { ... }

For further performance improvement, note that the strings consist
of lowercase letters only, i.e. there are only 26 possible characters.
Therefore you can use a fixed-size array instead to count the number
of occurrences of each letter in the strings. Array subscripting is
much faster than dictionary lookups.

In order to compute a number (from 0 to 25) from each lowercase letter,
it is more convenient to enumerate the unicodeScalars of the string.
This leads to the following implementation:

extension String {
    func characterCounts() -> [Int] {
        let letterA: UnicodeScalar = "a"
        var counts = Array(repeating: 0, count: 26)
        for ucs in self.unicodeScalars {
            let offset = ucs.value - letterA.value
            assert(offset >= 0 && offset < 26, "only lowercase letter 'a'...'z' allowed")
            counts[Int(offset)] += 1
        }
        return counts

    }

    func isPermutation(of other: String) -> Bool {
        let counts1 = self.characterCounts()
        let counts2 = other.characterCounts()
        return counts1 == counts2
    }
}

let numTestCases = Int(readLine()!)!
for _ in 1...numTestCases {
    let strings = readLine()!.components(separatedBy: " ")
    if strings[0].isPermutation(of: strings[1]) {
        print("YES")
    } else{
        print("NO")
    }
}


Adding a node to a Binary Search Tree and printing the path to it I wrote BST insertion code with the book. The book uses recursion, but I just want to know how I can do it without recursion.

My code is working, but I don't know if it is correct.

public class BSearchTree {
    BNode root = null;

    public void add(BNode node){
        int depth = 0;

        if(root != null){
            if(node.data == root.data) return;

            BNode p = root;

            while(p != null){
                depth++;

                if(p.data < node.data){
                    if(p.right != null){
                        p = p.right;
                    }else{
                        p.right = node;
                        node.index = depth;
                        break;
                    }
                }else{
                    if(p.left != null){
                        p = p.left;
                    }else{
                        p.left = node;
                        node.index = depth;
                        break;
                    }
                }
            }
        }else{
            root = node;
            node.index = depth;
        }

        inOrder(root, "[ROOT]");
    }

    public void inOrder(BNode node, String direction){
        BNode p = node;
        if(p == null) return;

        inOrder(p.left, "[LEFT]");
        System.out.println(p.index+": "+direction+" "+p.data);
        inOrder(p.right, "[RIGHT]");
    }
}

if(root != null){
        if(node.data == root.data) return;

        BNode p = root;

        while(p != null){

p cannot be null on the first pass.

Convert your while-loop into a do-while loop.

            }else{
                if(p.left != null){
                    p = p.left;
                }else{
                    p.left = node;
                    node.index = depth;
                    break;
                }
            }

else block containing only a if-elseif chain.

Merge them, like so:

            }else if(p.left != null){
                p = p.left;
            }else{
                p.left = node;
                node.index = depth;
                break;
            }


Synchronous loop with internal asynchronous calls Below I've written a simple version of what I'm attempting to accomplish. I'm new to asynchronous functions, so it's difficult to wrap my head around this. My goal is to wait for the loop to finish all the callbacks before console.log() runs, but I want all the callbacks inside the loop to be asynchronous, so they don't wait for calls to complete one after another, taking way too much time to complete. How would I write this, the loop to be synchronous (wait for all internal calls to complete) but the internal calls to be asynchronous?

Much appreciated!

var addressList = [];

for (var i = 0; i < members.length; i++) {
  service.member.info({
    name: members[i],
    company: "Cerberus",
    // Callback function
    function(body) {
      addressList.push(body.address);
    }
  });
}

console.log(addressList);

You can do this manually with a counter along these lines (kind of messy):

function getAddresses(members, callback) {
  var addressList = [];
  var count = 0;

  for (var i = 0; i < members.length; i++) {
    service.member.info({
      name: members[i],
      company: "Cerberus",
    }, function(body) {
      addressList[i] = body.address;
      count++;

      if (count === members.length) {
        callback(addressList);
      }
    });
  }
}

There are also libraries for this sort of thing, like async:

var async = require('async');

function getAddresses(members, callback) {
  async.map(members, function (member, callback) {
    service.member.info({
      name: member,
      company: "Cerberus",
    }, function (body) {
      callback(null, body.address);
    });
  }, callback);
}

But promises are really the best option if you’re able to use them consistently throughout your project, with a library like bluebird:

var bluebird = require('bluebird');

// This only works if the callback is really (err, body)
bluebird.promisifyAll(service.member);

function getAddresses(members) {
  return bluebird.map(members, function (member) {
    return service.member.infoAsync({
      name: member,
      company: "Cerberus",
    }).then(function (body) {
      return body.address;
    });
  })
}

They could take some learning, but they’re the future of asynchronicity in JavaScript or something, since right now you can do this (or you could, if service.member.info returned a promise) – note how close it is to your original loop:

var bluebird = require('bluebird');

var getAddresses = bluebird.coroutine(function* (members) {
  var addressList = [];

  for (var i = 0; i < members.length; i++) {
    var body = yield service.member.info({
      name: members[i],
      company: "Cerberus",
    });

    addressList.push(body.address);
  }

  return addressList;
});

// or, to run the loop in parallel:

var getAddresses = bluebird.coroutine(function* (members) {
  var operations = [];

  for (var i = 0; i < members.length; i++) {
    operations.push(
      service.member.info({
        name: members[i],
        company: "Cerberus",
      })
    );
  }

  return (yield operations).map(body => body.address);
});

and one day, that might be simplified to this, but not yet:

async function getAddresses(members) {
  var addressList = [];

  for (var i = 0; i < members.length; i++) {
    var body = await service.member.info({
      name: members[i],
      company: "Cerberus",
    });

    addressList.push(body.address);
  }

  return addressList;
}


Implementing class equality/comparison in F# I'm working on some book material on classes and interfaces in F#. My example is a latitude/longitude class:

    open System

    type LatLon(latitude : float, longitude : float) =

        member __.Latitude = latitude
        member __.Longitude = longitude

        override this.GetHashCode() =
            hash (this.Latitude, this.Longitude)

        override this.Equals(thatObj) =
            match thatObj with
            | :? LatLon as that ->
                this.Latitude = that.Latitude
                && this.Longitude = that.Longitude
            | _ -> false

        interface IEquatable<LatLon> with
            member this.Equals(that : LatLon) =
                this.Equals(that)

        interface IComparable with
            member this.CompareTo(thatObj) =
                match thatObj with
                | :? LatLon as that ->
                    compare (this.Latitude, this.Longitude) (that.Latitude, that.Longitude)
                | _ ->
                    raise <| ArgumentException("Can't compare instances of different types")

    let landsEnd = LatLon(50.07, -5.72)
    let johnOGroats = LatLon(58.64, -3.07)
    let landsEnd2 = LatLon(50.07, -5.72)

    let demo() =
        // false
        printfn "%b" (landsEnd = johnOGroats)
        // true
        printfn "%b" (landsEnd = landsEnd2)

        // 50.070000, -5.720000
        // 58.640000, -3.070000
        [ landsEnd; johnOGroats; landsEnd2 ]
        |> Set.ofList
        |> Seq.iter (fun ll -> printfn "%f, %f" ll.Latitude ll.Longitude)

By the way, I'm well aware that in this case, a record type would give the necessary structural equality out of the box. But the material I want to cover with this example is specifically about classes.

My main concerns are:

The code works the same if I don't implement IEquatable, as it uses the Equals() override. Is it considered desirable to implement IEquatable anyway, and/or does it have advantages I haven't spotted?
What about the (currently very-lightly-documented) op_Equality? Should that be used as well/instead?
The code works fine (for example Set construction, requiring comparison) if I implement IComparable but not IComparable<'T>. Have I missed out on anything useful by not using IComparable<'T>?

Obviously I'm amenable to any other feedback on the quality/style of this code.

Many thanks!

There's a suggestion for [<AllowNullLiteral>]: I think this is a bad idea.

You wrote a class that is a lat/long pair, as a result, it's a standard GeoCoordinate, which is a regular value-type, and should be marked as such. I get that the book calls it a class, but as an F# developer, how many times in your life have you actually had to think about the difference?

As a result, I'd add the structure definition in some manner, either via struct/end, or [<StructAttribute>].

I also recommend implementing the generic IComparable<T>. It's cheap, and can offer more performance to avoid boxing/unboxing should the caller need it. (Reed also covered this.) Some of the newer API's will try to rely on both, preferring the IComparable<T>. You may also want to read this answer to another question, as it talks about a lot of performance intricacies with regard to the standard BCL stuff. (You may want to create an IEqualityComparer<T>, for example.)

Some might argue that the guidelines say to keep the instance size "under 16 bytes", I personally think that's rubbish, as I'm pretty sure that should read "at or under 16 bytes". These are not always law.

The .NET BCL contradicts itself here several times (Size structure, Guid structure, too lazy to find the others), and the Struct Design guidelines don't mention it, only the class vs. struct guidelines.

As a result, I think you're safe in that regard here. The item you're talking about is small (16-bytes), represents a value (physical location that doesn't change), and should not be directly changed. Just as well, the copying performance can often use native SIMD instructions, so it should not be a big deal to copy 16-bytes. (I had a source on this, but I lost it, going to try to dig it up again.)


Sieve of Eratosthenes in Rust This is a relatively simple implementation of the Sieve of Eratosthenes in Rust. The main objective is to find the \$n\$th prime quickly when \$n\$ might grow to huge numbers.

pub fn nth(n: u32) -> u32 {
    // A convenient variable that will help in simplifying a complicated expression
    let x = if n <= 10 { 10.0 } else { n as f64 };

    // The prime counting function is pi(x) which is approximately x/ln(x)
    // We need the inverse of that. A good estimate is ceil(x * ln(x * ln(x)))
    let limit: usize = (x * (x * (x).ln()).ln()).ceil() as usize;
    let mut sieve = vec![true; limit];
    let mut count = 0;

    // Exceptional case for 0 and 1
    sieve[0] = false;
    sieve[1] = false;

    for prime in 2..limit {
        if !sieve[prime] {
            continue;
        }
        if count == n {
            return prime as u32;
        }
        count += 1;

        for multiple in ((2 * prime)..limit).step_by(prime) {
            sieve[multiple] = false;
        }
    }
    return <u32>::max_value();
}

This entire function would be present in main.rs or lib.rs, as necessary.

Please feel free to nitpick on any aspect of the code. Suggestions to make the code cleaner or more efficient are welcome.

Nice use of the Prime Number Theorem there. However, there are some issues with your code as is:

The name nth does not convey the meaning of the function.
A return statement is used instead of an expression at the end of the function.
The composite marking can start at prime * prime, as all smaller composites are already marked by previous primes.
The \$\left(2^{32}-1\right)\text{th}\$ prime is larger than \$2^{32}-1\$, but less than \$2^{64}-1\$, so u64 is a better return type.
The function will use a lot of memory if we use <u32>::max_value() as input.
The first (\$1\text{st}\$) prime number \$p_1\$ is \$2\$, but your code returns \$3\$ on nth(1).
The behaviour is not documented at the moment, as the function is missing all documentation.
The function doesn't necessarily return a prime number.

The last issue is the greatest one, to be honest. If I use a nth_prime function, I expect the function to return a prime number and only a prime number, whereas <u32>::max_value() is a composite. If it's possible that your function does not return a valid result, use Option or Result instead:

// https://play.rust-lang.org/?gist=4d6abc78a8c0d205da57a17c02201d7c&version=stable&mode=release&edition=2015

/// Returns the nth prime.
///
/// It uses a sieve internally, with a size of roughly
/// `n * (n.ln() + n.ln().ln()` bytes. As a result, its
/// runtime is also bound loglinear by the upper term.
///
/// # Examples
///
/// ```
/// use <yourcrate>::nth_prime;
///
/// assert_eq!(nth_prime(0), None);
/// assert_eq!(nth_prime(1), Some(2));
/// assert_eq!(nth_prime(2), Some(3));
/// assert_eq!(nth_prime(3), Some(5));
/// ```
///
///
pub fn nth_prime(n: u32) -> Option<u64> {
    if n < 1 {
        return None;
    }

    // The prime counting function is pi(x) which is approximately x/ln(x)
    // A good upper bound for the nth prime is ceil(x * ln(x * ln(x)))
    let x = if n <= 10 { 10.0 } else { n as f64 };
    let limit: usize = (x * (x * (x).ln()).ln()).ceil() as usize;
    let mut sieve = vec![true; limit];
    let mut count = 0;

    // Exceptional case for 0 and 1
    sieve[0] = false;
    sieve[1] = false;

    for prime in 2..limit {
        if !sieve[prime] {
            continue;
        }
        count += 1;
        if count == n {
            return Some(prime as u64);
        }

        for multiple in ((prime * prime)..limit).step_by(prime) {
            sieve[multiple] = false;
        }
    }
    None
}

Note that the documentation's examples show that 0 isn't a viable index for primes, and that nth_prime(1) == Some(2).

Other than that, we can optimize the code a little bit further if we consider 2 as a special case and then step (3..limit).step_by(2), but that makes the code slightly harder to read. There are also several approaches to keep the memory usage limited, but they are harder to achieve.

Other than that, well done, although I'd link to the Prime Number Theorem for the approximation.


Member function state machine For various reasons, I wrote this. For one, I don't like to wrap every single function in a separate class, which is too verbose.

It's a state machine based on GoTW 57 function pointer wrapping and CRTP to make things readable, at least in client code parts.

template <class Derived>
struct StateMachine {
    typedef Derived Self;
    typedef StateMachine<Derived> Base;

    //http://www.gotw.ca/gotw/057.htm
    struct Func;
    typedef Func (Derived::*UpdateFuncPtr)();
    struct Func {
        UpdateFuncPtr updateFunc;
        Func(UpdateFuncPtr ptr) : updateFunc(ptr) {}
    };

    //actual statemachine .. can be as complex as needed
    Func currentState;
    StateMachine( Func init) : currentState(init) {}
    void update() {
        UpdateFuncPtr p = currentState.updateFunc;
        Self * pThis = (Self *)this;
        Func newPtr = (pThis->*p)();
        if(newPtr.updateFunc)
            currentState = newPtr;
        }
};

The client looks like this:

struct DemoSM : StateMachine<DemoSM> {
    int check; //just checking that *this is sane
    DemoSM() : Base( &Self::update1 ) {
        check = 42;
    }
private:
    Func update1() {
        log("update1 called, check %d\n\r",check);
        return &Self::update2;
    }
    Func update2() {
        log("update called\n\r");
        return NULL;
    }
};

And just checking:

DemoSM machine;
machine.update();
machine.update();

Any critique or suggestions for improvements or gotchas welcome.  The core principle is that the state machine functions themselves code, procedurally (not declaratively in a table), how the transitions get triggered - by returning the function that needs to be called next.

This can be made marginally more interesting with separate "update" and "state transition" functions maybe with different signatures.

I could not get GoTW suggested operator() to work for taking pointer to members.  C++ is insistent on &Class::member; syntax there.

CRTP helpers

When using CRTP, it is common to write derived methods in the base class to avoid having to deal with the this pointer and to use reference semantics instead:

Derived& derived() {
    return static_cast<Derived&>(*this);
}

Derived const& derived() const {
    return static_cast<const Derived&>(*this);
}

Thanks to these functions, your update method becomes:

void update() {
    UpdateFuncPtr p = currentState.updateFunc;
    Func newPtr = (derived().*p)();
    if(newPtr.updateFunc)
        currentState = newPtr;
    }

And it is now easier to have the base class call methods from the derived type without having to deal with the this pointer.

Improving Func

If you want to impose more responsibility on Func and less on StateMachine itself, you can still add operator UpdateFuncPtr() const to Func to allow easy conversions to and from the class:

operator UpdateFuncPtr() const {
    return updateFunc;
}

That will allow you simply again the method update:

void update() {
    UpdateFuncPtr p = currentState;
    Func newPtr = (derived().*p)();
    if(newPtr)
        currentState = newPtr;
    }

The implicit conversion to `UpdateFuncPtr` is also used for the boolean conversion needed to check whether `newPtr` is `NULL` or not. Be careful though, implicit conversions are often the source of many hidden errors. Implementing `operator()` in `Func` would be quite difficult since it would mean that `Func` must know the instance of `StateMachine` on which you wish to call the methods. All in all, it is easier to keep the `this` pointer out of `Func`.

Access modifiers

There are several parts of the base class that you do not need in the derived class. You could make them private:

UpdateFuncPtr
currentState
The derived methods if you choose to use them

Moreover, there are even more things that the final users of the state machine don't need to know. Actually, the only need to know about update, so almost everything else could be made protected.

Stylistic tidbits

There are a few more details that can be improved with your code:

Try to always use constructor initialization lists when you can (you may have forgotten it in this particular case because it's only an example):

DemoSM() :
    Base( &Self::update1 ),
    check( 42 )
{ }

Your brace indentation in update is really misleading. When modifying the code, I found it hard to read since the last brace seems to match the if while it actually closes the function block. To reflect that, this last brace should be aligned with the function signature, like every other function in your code.
If possible, try to avoid if blocks without braces. While it may be easier to write when there is only one line, your code proves that it can hide subtle bugs: at first, I thought that the last closing brace closed the if and I tried to add lines to it (to perform some checks) while I wasn't actually adding to the if. Had you used braces for the if, I wouldn't have had this problem.
You don't seem to be very consistent with regard to parenthesis and spaces. You might want to choose some guideline and apply it everywhere.


A Summation Function For Arbitrary Nested Vector Implementation In C++ I am trying to deal with some calculations on nested vector data in C++. The nested vector data may be like std::vector<long double>, std::vector<std::vector<long double>>, or std::vector<std::vector<std::vector<long double>>>. I want to focus on summation here, and the calculation of summation could be done with the Sum function implemented here. Is there any possible improvement of this code?
The function declaration part is as below.
template <class T>
static long double Sum(const std::vector<T> inputArray);
static long double Sum(long double inputNumber);

The function implementation part is as below.
template<class T>
inline long double Sum(const std::vector<T> inputArray)
{
    long double sumResult = 0.0;
    for (auto& element : inputArray)
    {
        sumResult += Sum(element);
    }
    return sumResult;
}

inline long double Sum(long double inputNumber)
{
    return inputNumber;
}

Test for this sum function:
std::vector<long double> testVector1;
testVector1.push_back(1);
testVector1.push_back(1);
testVector1.push_back(1);
std::cout << std::to_string(Sum(testVector1)) + "\n";

std::vector<std::vector<long double>> testVector2;
testVector2.push_back(testVector1);
testVector2.push_back(testVector1);
testVector2.push_back(testVector1);
std::cout << std::to_string(Sum(testVector2)) + "\n";

std::vector<std::vector<std::vector<long double>>> testVector3;
testVector3.push_back(testVector2);
testVector3.push_back(testVector2);
testVector3.push_back(testVector2);
std::cout << std::to_string(Sum(testVector3)) + "\n";

Oct 18, 2020 Update
Reference:

The follow-up question:
A Summation Function For Various Type Arbitrary Nested Iterable Implementation in C++

The further implementation with c++-concepts:
https://stackoverflow.com/q/64414869/6667035

The template seems reasonable, although inputArray is a misnomer. However, there is a major drawback that can be remedied with a single &: use call-by-reference instead of call-by-value (see guidelines):
template<class T>
inline long double Sum(const std::vector<T> &inputArray)
{
    ...
}

Other than that it's perfectly fine for summing arbitrary nested std::vector<double>.
That being said, there is some room for further experiments:

enable Sum for anything that has begin() and end()
enable Sum for other types than double (e.g. int)

Also, I'm a little bit concerned by the comment that declaration and definition were split. While it's possible, it's usually not intended.


Calculating the checksum digit with the Luhn-algorithm I'm doing some exercises in Python and this one is about using the Luhn-algorithm to calculate the checksum digit in Swedish personal identification numbers.

The description is the same as this question:

  Given a string of 9 digits, abcdefghi compute:

array = [a*2, b, c*2, d, e*2, f, g*2, h, i*2]

  Then you compute the sum of the digits in this array, so for example
  if a*2 is 16, then that part counts as 1 + 6

  Then the result is how much more you have to add to make it evenly
  divisible by 10. For example, if sum == 54 then the result is 6 as
  60 - 54 = 6.

def control_numb(nbr_string):
    cut_string = nbr_string[:9]  ## We need 9 numbers to calculate the 10th
    mult_numb = [2,1]            ## we should alternate multiplying 2 and 1 starting with 2
    i = 0                        ## holds the index of which mult_numt to use. starting with index 0

    sum_of_digits = 0
    for s in cut_string:         ## loop through the string of digits
        for d in str(int(s) * mult_numb[i]):  ## use s as int to do the multiplication. then turn back to str to loop through digit
            sum_of_digits += int(d)
        i = (i+1)%2

    return (10 - sum_of_digits % 10) % 10

I'm pretty new to Python and would very much like feedback, if there are any pythonic way to do things please spread the knowledge. Or just tear the code apart completely. Any comments are welcome!

Some comments:

Variable names could be improved a little bit. For example, I think nbr_string could be better as swedish_id
Please add a docstring (PEP257)
itertools.cycle is useful to iterate multiple times over the same sequence (in this case, the multiplication factors)
Use list comprehensions where possible instead of for loops

Things that might be different from the question's code:

I'm assuming the initial string has 9 digits, but if it has already 10, then go with your code to keep only the first 9 numbers, but note that you can use some assertions to be sure the data passed is correct.
I've calculated partial sums to apply the algorithm as in the explanation, but doing the sums as in the question should be fine.

Please find my solution based on your code below:

from itertools import cycle

def control_number(swedish_id):
    """Calculate control number in Swedish personal IDs

    :param swedish_id: A Swedish personal ID
    :type swedish_id: str

    """
    assert swedish_id.isdigit()
    assert len(swedish_id) == 9

    # Multiplication factors for each digit depending on its position
    mult_factors = cycle([2, 1])

    def partial_sum(number, mult_factor):
        """Calculate partial sum ofr a single digit."""
        quotient, remainder = divmod(number * mult_factor, 10)
        return quotient + remainder

    final_sum = sum(
        partial_sum(int(character), mult_factor)
        for character, mult_factor in zip(swedish_id, mult_factors))

    # Calculate control number based on partial sums
    control_number = -final_sum % 10

    return control_number

I hope this helps.


Unique elements, long runtime Haskell I have two functions. Both have to have unique elements, one with Integers and the other a String which returns a list of Char. My runtime is currently quite high and I'm not allowed to use imports, but I was wondering how to improve them:
uniqueInt:: [Int] -> [Int]
uniqueInt[] = []
uniqueInt xs = [x | (x,y) <- zip xs [0..], x `notElem` (take y xs)]

Example:
uniqueInt [1,2,1,3,1,2] = [1,2,3]
uniqueString :: String -> [Char]
uniqueString ""=[]
uniqueString xs = [x | (x,y) <- zip xs [0..], x `notElem` (take y xs)]

Example: uniqueString "hello"="helo"

Firstly you might want to make use of a Haskell feature to combine both functions into one:
unique :: Eq a => [a] -> [a]
unique [] = []
unique xs = [x | (x,y) <- zip xs [0..], x `notElem` (take y xs)]

Now let's talk about performance.  Obviously lists aren't great in this
respect, since take is quite expensive and needs to iterate through
the list, as well as notElem, which will, again iterate through the
list.  Plus this is also constructing new lists, which leads to GC,
which leads to decreased performance.
So best option would be to go through the list only once, putting each
element into a set while it's then only collecting the elements which
haven't been in the set yet at that moment.
That's of course a very imperative way of doing it.  And regardless, if
you're not allowed to use imports you'd have to create the respective
data structures yourself, which I'd say isn't worth the effort.

In any case, at least within the constraints set it's still possible to
make this a bit more
efficient, basically by
copying
nub's
implementation:
unique :: Eq a => [a] -> [a]
unique l = unique' l []
  where
    unique' [] _ = []
    unique' (x:xs) ls
           | x `elem` xs = unique' xs ls
           | otherwise   = x : unique' xs (x:ls)


C - Division from scratch I wrote this code where division can be done without using C's builtin operator:
#include <stdio.h>
#include <math.h>

int idiv(int a, int b) {
    return a * (pow(b, -1));
}

int main(void) {
    /* 15/3 is 5 */
    printf("%d\n", idiv(15, 3));
    return 0;
}

Is there anything I could do to make this code faster? I benchmarked it with the division operator and it was more than 10x faster.

It is unclear what toolchain do you use and what is target processor. However, I can guess about performance degradation. pow function usually works with floating point values. Moreover, most probably power with negitive exp will also use division.
I compiled following code with ARM gcc 8.2 with -O3 flag:
#include <math.h>

int idiv(int a, int b) {
    return a * (pow(b, -1));
}

int idiv2(int a, int b) {

    return a/b;
}

I've got following assebly listing:
idiv:
        push    {r4, r5, r6, lr}
        mov     r6, r0
        mov     r0, r1
        bl      __aeabi_i2d
        mov     r2, r0
        mov     r3, r1
        mov     r0, #0
        ldr     r1, .L4
        bl      __aeabi_ddiv
        mov     r4, r0
        mov     r0, r6
        mov     r5, r1
        bl      __aeabi_i2d
        mov     r2, r0
        mov     r3, r1
        mov     r0, r4
        mov     r1, r5
        bl      __aeabi_dmul
        bl      __aeabi_d2iz
        pop     {r4, r5, r6, pc}
.L4:
        .word   1072693248
idiv2:
        push    {r4, lr}
        bl      __aeabi_idiv
        pop     {r4, pc}

From assebly code the reson of performance problems is obvious. And we still dependent on ABI.
If you want to have code that uses only assebly instructions? then you can implement integer division with "shift and subtract". Or you can google for some open source implementation of it. For example I found one here. Here is slightly modified example from here:
void unsigned_divide(unsigned int dividend,
             unsigned int divisor,
             unsigned int *quotient,
             unsigned int *remainder )
{
  unsigned int t, num_bits;
  unsigned int q, bit, d;
  int i;

  *remainder = 0;
  *quotient = 0;

  if (divisor == 0)
    return;

  if (divisor > dividend) {
    *remainder = dividend;
    return;
  }

  if (divisor == dividend) {
    *quotient = 1;
    return;
  }

  num_bits = 32;

  while (*remainder < divisor) {
    bit = (dividend & 0x80000000) >> 31;
    *remainder = (*remainder << 1) | bit;
    d = dividend;
    dividend = dividend << 1;
    num_bits--;
  }

  /* The loop, above, always goes one iteration too far.
     To avoid inserting an "if" statement inside the loop
     the last iteration is simply reversed. */
  dividend = d;
  *remainder = *remainder >> 1;
  num_bits++;

  for (i = 0; i < num_bits; i++) {
    bit = (dividend & 0x80000000) >> 31;
    *remainder = (*remainder << 1) | bit;
    t = *remainder - divisor;
    q = !((t & 0x80000000) >> 31);
    dividend = dividend << 1;
    *quotient = (*quotient << 1) | q;
    if (q) {
       *remainder = t;
     }
  }
}  /* unsigned_divide */

#define ABS(x)  ((x) < 0 ? -(x) : (x))

void signed_divide(int dividend,
           int divisor,
           int *quotient,
           int *remainder )
{
  unsigned int dend, dor;
  unsigned int q, r;

  dend = ABS(dividend);
  dor  = ABS(divisor);
  unsigned_divide( dend, dor, &q, &r );

  /* the sign of the remainder is the same as the sign of the dividend
     and the quotient is negated if the signs of the operands are
     opposite */
  *quotient = q;
  if (dividend < 0) {
    *remainder = -r;
    if (divisor > 0)
      *quotient = -q;
  }
  else { /* positive dividend */
    *remainder = r;
    if (divisor < 0)
      *quotient = -q;
  }

} /* signed_divide */

You can take any, verify it and optimize it according to your needs. Generalized functions are usually more complicated than specialized.


Implementing graph search I am very new in Python, I would like to know if the style presented below attends a good implementation in Python.

import numpy as np
from collections import deque

def BFS(theGraph,source):
    n=len(theGraph)
    explored=np.empty(n,bool)
    for i in range(0,n):
        explored[i]=False
    explored[source]=True
    queue = deque([source])
    while len(queue)!=0:
        v=queue.popleft()
        print "theNode: ", v
        for i in theGraph[v]:
            if(not explored[i]):
                explored[i]=True
                queue.append(i)
    return explored

def DFS(theGraph,source):
    n=len(theGraph)
    explored=np.empty(n,bool)
    for i in range(0,n):
        explored[i]=False
    explored[source]=True
    stack = [source]
    while len(stack)!=0:
        v=stack.pop()
        print "theNode: ", v
        for i in theGraph[v]:
            if(not explored[i]):
                explored[i]=True
                stack.append(i)
    return explored

def DFSrec(theGraph,explored,source):
    explored[source]=True
    print "theNode: ",source
    for i in theGraph[source]:
        if(not explored[i]):
            DFSrec(theGraph,explored,i)
    return explored

n=6 #number of nodes

theGraph=[set() for i in range(n)]

theGraph[0].add(1)
theGraph[0].add(2)
theGraph[1].add(0)
theGraph[1].add(3)
theGraph[2].add(0)
theGraph[2].add(4)
theGraph[3].add(1)
theGraph[3].add(5)
theGraph[4].add(2)
theGraph[4].add(5)
theGraph[5].add(3)
theGraph[5].add(4)
print theGraph

print "DFS: "
print DFS(theGraph,0)

explored=np.empty(n,bool)
for i in range(0,n):
    explored[i]=False

print "DFSrec: "
print DFSrec(theGraph,explored,0)

print "BFS: "
print BFS(theGraph,0)

There are some things that you could do to improve your code:

First of all, your functions clearly lack documentation. Unless you know a little bit about graph theory, it is quite hard to guess that BFS and DFS stand for "Breadth-First Search" and "Depth-First Search". While the names may appear to be long, using the names breadth_first_search and depth_first_search would be really clearer.
Also, docstrings to document what your functions do would be great. For example, you could write what theGraph is supposed to be since there is no obvious Graph structure to be used around. Documenting your functions, what are the parameters, what is the return type and what may be the preconditions and postconditions really matters :)
Also, your code is too dense. You should add more spaces where it improves readability. My advice to follow the guidelines in the PEP 8, the Python style guide. These guidelines are really good to write readable code.
By the way, the PEP 8 says that you shouldn't check the len of a collections to know whether there are elements in or not. Therefore, you should change this line:

while len(queue)!=0:

...to this one:

while queue:

These lines:

explored=np.empty(n,bool)
for i in range(0,n):
    explored[i]=False

...can be reduced to a one-liner thanks to numpy.zeros:

explored=np.zeros(n,bool)

And since you don't use n except for constructing excepted, you may as well get rid of n and use numpy.zeros_like to construct expected:

explored = np.zeros_like(theGraph, bool)

While it is good for modules to have examples of how the module works, you generally do not want these examples to be run, unless when you explicitly run the module as a stand-alone. If you want your code to be a module, put the example part of the code in what we could call a "main block" (I don't know whether there is an official name):

if __name__ == '__main__':

    n=6 #number of nodes

    theGraph=[set() for i in range(n)]

    theGraph[0].add(1)
    theGraph[0].add(2)

   # Rest of your example
   # code goes here...


Permute and count between nested dictionaries Goal:

Permute inner values between two dictionaries with matching outer key,
Save permuatations in counter,
Move to next outer key, update counter with new permutations.

Problem:

Many lookups on large dictionary (60 outer keys x 18000 inner key:value pairs) make existing code correct but slowwwww.

import collections

cnv = {'x': {'a':0, 'b':1, 'c':-1}, 'y': {'a':1, 'b':1, 'c':1}, 'z': {'a':-1, 'b':-1, 'c':-1}}
rna = {'x': {'A':1, 'B':3, 'C':5}, 'y': {'A':1, 'B':3, 'C':5}, 'z': {'A':1, 'B':3, 'C':5}}

cnv_loss_total = {}
for cell in cnv:
    cnv_gene_loss = set()
    for gene in cnv[cell]:
        if cnv[cell][gene] < 0:
            cnv_gene_loss.add(gene)
    cnv_loss_total[cell] = cnv_gene_loss

total_link_counter = collections.Counter()
for cell in rna:
    rna_gene_gain = set()
    for gene in rna[cell]:
            rna_gene_gain.add(gene)
    cell_combo = itertools.product(cnv_loss_total[cell], rna_gene_gain)
    for comboC, comboR in cell_combo:
        total_link_counter[comboC+'+'+comboR]+=rna[cell][comboR]

print (total_link_counter)
print(total_link_counter.values())

The code provides the desired output:

Counter({'c+C': 10, 'c+B': 6, 'b+C': 5, 'a+C': 5, 'a+B': 3, 'b+B': 3, 'c+A': 2, 'b+A': 1, 'a+A': 1})

There is also a memory bottleneck here:

--> 23         total_link_counter[comboC+'+'+comboR]+=rna[cell][comboR]
     24
     25 print (total_link_counter)

MemoryError:

But I intend on using much larger data (although similarly formatted data: either as a dict as dict or pandas dataframe).  How can I optimize the above code to deal with the MemorError (and hopefully shorten runtime)?

Here's a first pass at compacting your code, it should provide some speed improvements too:

cnv_loss_total = {cell: set(gene for gene in genes if genes[gene] < 0) for cell,genes in cnv.iteritems()}

total_link_counter = collections.Counter()
for cell,genes in rna.iteritems():
    cell_combo = itertools.product(cnv_loss_total[cell], set(genes.keys()))
    for comboC, comboR in cell_combo:
        total_link_counter[comboC+'+'+comboR] += genes[comboR]

By turning the first loop into a list comprehension, it avoids temporary variables and is more efficient in creating a set.  Also, using iteritems() avoids generating the entire list of dictionary items and yields one pair at a time.

In the second loop, the rna_gene_gain variable is eliminated by using set(genes.keys), which is again more efficient in building the set.


Is this code is safe or not (img upload) ? [PHP] I'd like to know if this code is usable, because I'm not sure, thank you. The code is used to upload images to a predefined folder for users.
if(isset($_FILES['avatar']) AND !empty($_FILES['avatar']['name'])) {
$tailleMax = 6844685465456456;
$extensionsValides = array('jpg', 'jpeg', 'gif', 'png');

if($_FILES['avatar']['size'] <= $tailleMax) {

   $extensionUpload = strtolower(substr(strrchr($_FILES['avatar']['name'], '.'), 1));

   if(in_array($extensionUpload, $extensionsValides)) {

      $resultat = move_uploaded_file($_FILES['avatar']['tmp_name'], 'uploads/'.$_SESSION['id'].'.'.$extensionUpload);

      if($resultat) {

     echo "success";

          } else {

            echo "Erreur durant l'importation de votre photo de profil extension";
          }
       } else {

        echo "Votre photo de profil doit être au format jpg, jpeg, gif ou png";
       }
    } else {

       echo "Votre photo de profil ne doit pas dépasser 2Mo";
    }
 }else{
     echo ':/';
 }

Just a few remarks:
I am wondering how that magic value of 6844685465456456 was determined :) I don't think that is equivalent to 2 Mb.

You need to use proper indentation, especially when you have nested control blocks - readability is important. Lack of readability here can result in a greater likelihood of logic errors.

This code is not helpful at all:
 }else{
     echo ':/';
 }

It doesn't tell the user what the problem is. Don't you hate those forms that tell you 'invalid input, try again' but don't actually tell you the problem and leave you guessing ?

This code (borrowed from mickmackusa):
if (!move_uploaded_file($_FILES['avatar']['tmp_name'], "uploads/{$_SESSION['id']}.{$extensionUpload}")) {
    exit("Erreur durant l'importation de votre photo de profil extension");
}

You are showing an error message to the user but it should be treated like an exception. This code could fail for several reasons like:

invalid file name
disk full/not mounted
etc

But at this point it's not the user fault. It's a malfunction in your application or a server issue. There is nothing the user can do. So I would handle the exception and send an alert. If your application is broken you'll want to be notified and fix it asap, before someone reaches out to you.

This code is not foolproof:
$resultat = move_uploaded_file($_FILES['avatar']['tmp_name'], 'uploads/'.$_SESSION['id'].'.'.$extensionUpload);

You are expecting that the session variable ID will be set. It's perfectly possible that the user has the page opened already and the session expires after some time. When submitting the form the session variable could be unset, unless you have more code elsewhere (an include perhaps) that verifies that the session is active. If there is no active session you'll probably redirect the user to the login page.
So the resulting file name could be incorrect and not what you expected (eg: .jpg). It could be one reason why move_uploaded_file would fail in your script. In itself it doesn't present an obvious security risk though but it is a bug.


jQuery callbacks optimization I have a lot of repeatable blocks of code and want to optimize/simplify them:

    function animationInit() {
    content = $('#content')
        .bind('show',function(event, f) {
            $(this).animate({right:0}, lag + 200, function() {
                if (f) f();
            });
        })
        .bind('hide',function(event, f) {
            $(this).animate({right:-700}, lag + 200, function() {
                if (f) f();
            });
        });

    hotelnav = $('#hotel')
        .bind('show',function(event, f) {
            hotelnav.removeClass('small');
            $(this).animate({left:0}, lag + 200, function() {
                if (f) f();
            });
        })
        .bind('hide',function(event, f) {
            $(this).animate({left:isGallery()?-300:-300}, lag + 200, function() {
                hotelnav.addClass(isGallery()?'small':'');
                if (f) f();
            });
        });

    bottompanel = $('#bottompanel')
        .bind('show',function(event, f) {
            $(this).animate({bottom:40}, lag + 200, function() {
                if (f) f();
            });
        })
        .bind('hide',function(event, f) {
            $(this).animate({bottom:-120}, lag + 200, function() {
                if (f) f();
            });
        });

    booknow = $('#booknow')
        .bind('show',function(event, f) {
            $(this).fadeIn(lag + 200, function() {
                if (f) f();
            });
        })
        .bind('hide',function(event, f) {
            $(this).fadeOut(lag + 200, function() {
                if (f) f();
            });
        });
};

How i can optimize repeatable parts of code with callbacks?
Im trying to create separate function like this:

function cb(callback) {
    if (callback) callback();
};

... but just have a lot of asynchronous callbacks...

I don't understand the need for

function() {
    if (f) f();
}

put f instead of that whole thing. If it is undefined it won't get called.

e.g

.bind('show',function(event, f) {
    $(this).animate({right:0}, lag + 200, f);
});

Another thought came to me. The functions you use could be factored and used like:

function animateRight(event, f, rightValue, delay)
{
    $(this).animate({right: rightValue}, lag + delay, f);
}

(or if you can't pass that information in:

function animateRight(event, f, rightValue, delay)
{
    return new function()
    {
        $(this).animate({right: rightValue}, lag + delay, f);
    }
}


Compiler for grond, a language-in-the-making I'm working on a language called grond (see it on GitHub); the compiler is so small it is included in this post.

My gut tells me that this code is awful, but it works. I am hesitant to change things to make the code better because generally, I have found that bad things happen when I "fix" something that isn't necessarily broken.

How can I make this "ugly" code better? Note - If you don't think this code is ugly, please tell me!

String.prototype.reverse = function() {
  return this.split("").reverse().join("");
}

function replace(wc) {
  var lastWC;

  while (true) {
    lastWC = wc;
    wc = wc.reverse();
    wc = wc.replace("§", "");
    wc = wc.replace("Ś", "");
    wc = wc.replace(/\(a(?!\w)/, "(trela"); //alert ,a
    wc = wc.replace(/\(f(?!\w)/, "(rof"); // for, f
    wc = wc.replace(/\(w(?!\w)/, "(elihw"); //while,w
    wc = wc.replace(/\(p(?!\w)/, "(tpmorp"); //prompt,p
    wc = wc.replace(/\(vr\./, "(esrever."); //reverse, rv
    wc = wc.replace(/\(tm\./, "(chtam."); // match, mt
    wc = wc.replace(/\(nj\./, "(nioj."); //join, .jn
    wc = wc.replace(/\(pr\./, "(ecalper."); //replace, .rp
    wc = wc.replace(/\(ls\./, "(.ecils"); //slice, sl
    wc = wc.replace(/\(lt\./, "(esaCrewoLot."); //toLowerCase, tl
    wc = wc.replace(/\(ut\./, "(esaCreppUot."); //toUpperCase, tu
    wc = wc.replace(/\(ps\./, "(tilps."); //split, sp
    wc = wc.reverse();
    wc = wc.replace(/\.l(?!\w)/, ".length"); // .length, .l
    if (lastWC === wc)
      break;
  }
  return wc;
}

var source = prompt("Enter Source");
var sourceCopy = source;

var fullASCII = ' !#$%&()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~'.split('');
var y = (a) => {
  for (let char in fullASCII) {
    if (!source.contains(char)) {
      source.replace(a, char);
    }
  }
}
var OK = false;
var OK2 = false;
for (let char in fullASCII) {
  if (!source.contains(char)) {
    OK2 = OK ? true : false;
    OK = true;
  }
}
if (!OK2) {
  throw "Your program uses every ASCII charactor. Grond requires your program to leave 2 ASCII chars unused. This is stupid and will be fixed.";
}
y('\\\"');
y("\\\'");
var sourceSplit = sourceCopy.split("\'");
for (let i = 0; i < sourceSplit.length; i += 2) {
  var o = sourceSplit[i].split('\"');
  for (let e = 0; e < o.length; e += 2) {
    o[e] = replace(o[e]);
  }
  sourceSplit[i] = o.join('\"');
}
sourceCopy = sourceSplit.join("\'");

var append = "if(s != \"\"){alert(s)}";
var prepend = "var s = prompt();";
var oSS = source.split('');
if (oSS[0] == "§") {
  prepend = "var s = \"\"";
}
if (oSS[0] == "Ś" || oSS[1] == "Ś") {
  prepend = "with(math){" + prepend;
  append += "}";
}

sourceCopy = prepend + " String.prototype.la=function(h){var x=this.split(''); x[h] = x[h].toLowerCase(); return x.join('');};String.prototype.ca=function(h){var x=this.split(''); x[h] = x[h].toUpperCase(); return x.join('');};String.prototype.reverse=function(){return this.split('').reverse().join('');}; \n var h = ['hello, world', 'Hello, World', 'Hello, World!', 'hello, world!', 'hello world', 'hello world!', 'Hello World', 'Hello World!', 'Hello, world.', 'Hello, World.' ,'hello world.' , 'Hello World.'];  var q ='" + source + "'; " + sourceCopy + append;
alert(sourceCopy);
eval(sourceCopy);

String.prototype.reverse = function() {
  return this.split("").reverse().join("");
}

Firstly, don't modify objects you don't own. It could cause incompatibility problems if some other code had its own String.prototype.reverse function which had different functionality.

Secondly, this is a bad implementation of reversing a string. For example, calling this function on the following string:

'foo  bar'

returns:

'rab �� oof'

You use many meaningless identifiers, for example wc, y, OK, OK2. It's hard to tell what purpose they serve.

while (true)

Instead of using an infinite loop and break-ing it, I recommend you using a do-while loop. In your case it could look like that:

var condition = false
do {
  // the actual loop content

  condition = lastWC === wc
} while (!condition)

See Is while (true) bad programming practice?.

I highly recommend you wrapping your code in an IIFE (see Ben Alman's article for more details). In the current state, all your variables are global, which is a bad practice. See Why are global variables considered bad practice?.

I had to spend quite a while to work out how the replace function works. It seems that you wanted to re-invent the g regex modifier (read MDN RegExp docs). You could get rid of the loop at all if you used it.

I don't see any point in reversing the wc variable two times, except of making the code less readable.

Calling wc.replace() multiple times and assigning the result to wc looks ugly. You could at least chain the .replace() calls like that: wc.replace(...).replace(...).replace(...), or even better, make an array of matches and replacements and loop through them:

var matchesAndReplacements = [
  ["§", ""]
 ,["Ś", ""]
 ,[/\(a(?!\w)/g, "(trela"]
]

for (let i of matchesAndReplacements) {
  let [match, replacement] = i
  wc = wc.replace(match, replacement)
}

And most importantly, this doesn't work. Writing a programming language compiler using regex is a very naive idea, and IMHO even worse than trying to parse HTML with regex. A simple example: what if you had a .l fragment in a string? It would be incorrectly changed to .length.

var source = prompt("Enter Source");

prompt() is a pretty primitive way to make an I/O. Even if you decide so, it shouldn't be somewhere in the middle of the code. Create a function, named compile for example, which takes input as argument and returns output, and then at the end of the code place alert(compile(prompt("Enter Source"))).

var source = prompt("Enter Source");
var sourceCopy = source;

When you declare multiple variables at once, you can use the var keyword only once, and separate the variables declarations with a comma, like that:

var source = prompt("Enter Source")
   ,sourceCopy = source

However, in this case it would be better to use const for source, as you don't intend to modify it.

var y = (a) => {
  for (let char in fullASCII) {
    if (!source.contains(char)) {
      source.replace(a, char);
    }
  }
}

If your arrow function has only one parameter, you don't need to use parentheses, so it could be var y = a=> {}.

Also, this function seems to be no-op, because it doesn't return any value and doesn't assign any value to any variable.

var OK = false;
var OK2 = false;
for(let char in fullASCII){
  if(!source.contains(char)){
    OK2 = OK? true:false;
    OK = true;
  }
}

That looks ugly for me. You could rewrite it like that:

const isOK = fullASCII.map(source.contains.bind(char)).filter(x=>x).length >= 2

One liners FTW!

var sourceSplit = sourceCopy.split("\'");

You don't need to escape apostrophes if you use double quotes to make a string. Besides, I would name the variable sourceSplitted, because now it looks more like a function name.

if (oSS[0] == "§") {

Always use === instead of ==. See Does it matter which equals operator (== vs ===) I use in JavaScript comparisons?.

To conclude: yes, this code is ugly. You have a lot to learn.


Grouping methods of a Python class I have a class with numerous methods, and so I would like to split these up into groups. This would make it easier for the user to find the method they are after. Here is a minimal example of this functionality.

class Greeter():

    def __init__(self, name):
        self.name = name
        self.spanish = Spanish(self)

   def generic(self):
       return f"Hello, {self.name}"

class Spanish():

        def __init__(self, greeter):
            self.grtr = greeter

        def hola(self):
            return f"Hola, {self.grtr.name}"

        def buen_dia(self):
            return f"Buenos días, {self.grtr.name}"

greeter = Greeter("Ann Example")
greeter.generic()
#> "Hello, Ann Example"
greeter.spanish.hola()
#> "Hola, Ann Example"

This feels like a hacky approach. Are there places people could see this failing? Is there a better solution in general?

EDIT

As suggested, here is a reduced example of my actual need for this functionality:

class Matrix():

    def __init__(self, m, n, data):
        # checks on m, n, data
        self.m = m
        self.n = n
        self.data = data
        self.decomposition = Decomposition(self)

    # many generic methods some returning more matrices
    def __add__(self, other):
        # add matrices
        return Matrix(m, n, data)

    # some methods will affect m, n, data
    def transpose(self):
        # set up new attributes
        self.m = m
        self.n = n
        self.data = data

class Decomposition():

    def __init__(self, mtrx):
        self.mtrx = mtrx

    # NB: eventually these might change m, n, data
    def QR(self):
        # do stuff for QR decomposition using self.mtrx
        return Q, R

    def LU(self):
        # do stuff for LU decomposition using self.mtrx
        return L, U

This is a review of the initial code that was posted. Unfortunately, I don't have time to revise it now for the new code.

Wanting to lessen the number of methods in a class is on its own a poor reason to split up a class into multiple. Classes can have many methods, and we can use IDE functionality and good documentation to make sense of a class.

Having many methods can suggest though that the class is doing too much, and that would be a more logical justification to split the class up. A better thing to think about is not the size of the class, but what the job of the class is, and if it's doing too many things.

That said, if you feel like it's justified to split it, I wouldn't do it as you have here. You've given the entire state of the Greeter object to Spanish. If Spanish begins using a lot of the state in Greeter, you now have two tightly coupled classes that will begin to break as you make changes to either.

Just give Spanish the information that it explicitly needs:

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

    def hola(self):
        return f"Hola, {self.name}"

    def buen_dia(self):
        return f"Buenos días, {self.name}"

And

class Greeter:
    def __init__(self, name):
        self.name = name
        self.spanish = Spanish(name)  # And just pass the name
    . . .

This setup really doesn't make sense though. To get a greeting in Spanish, you need to access an object that greets in English? Even combined it doesn't make much sense. Having an object that greets in multiple languages seems odd. It would make more sense to have completely discrete classes that don't know about each other.

With a toy example like this though, it's really hard to make a concrete suggestion because the actual intent isn't clear. The textbox that I'm writing the answer in is also bugging out, so I'm going to submit this before I lose everything.


Extracting parts of filenames from an array I am very new to perl. So far I love it, my new favorite interpreted language. I am quickly learning that perl has MANY 'tricks' to it. The following code extracts file names out of one array and into another using a simple regex. Is there a shorter way of doing this?

It feels like there's a way to remove the for loop.

my @configs = glob("m4/config.*.m4");
my @outputs;

foreach(@configs)
{
    $_ =~ m/m4\/(config\..*)\.m4/;
    push @outputs, $1;
}

print join(' ',@configs) . "\n";
# m4/config.a2.m4 m4/config.mk.m4 m4/config.php.m4

print join(' ',@outputs) . "\n";
# config.a2 config.mk config.php

exit;

As said before, use map instead of a foreach loop and change the regex delimiter.
If you put the array in double quote when print, a space is inserted between each element of the array in fact it is the content of the special variable $", see http://perldoc.perl.org/perlvar.html

$LIST_SEPARATOR
$"
When an array or an array slice is interpolated into a double-quoted string or a similar context such as /.../ , its elements are separated by this value. Default is a space. For example, this:
   print "The array is: @array\n";

is equivalent to this:
   print "The array is: " . join($", @array) . "\n";

Mnemonic: works in double-quoted context.

Your code becomes:
# Use ALLWAYS these two lines in every script.
use strict;
use warnings;

my @configs = glob("m4/config.*.m4");
my @outputs = map{ m~m4/(config\..*)\.m4$~ } @configs;
print "@configs\n";
print "@outputs\n";

Output:
m4/config.a2.m4 m4/config.mk.m4 m4/config.php.m4
config.a2 config.mk config.php


Keep main content scrollable but not the body For a template such as:

I'm using the following approach

Get the height of the viewport via javascript (Let's say 500px)
Deduct the header and footer height from it (Let's say header and footer are 50px each. So the end result will be 400px)
Apply the end result as height of the main content div via javascript
Use overflow-y:scroll property on the main content div via CSS

This works as expected but I was hoping there might be a better way to achieve the same. I'm thinking only via CSS, if possible.

The code I'm using is:

<!-- javascript -->
var height = Math.max(document.documentElement.clientHeight, window.innerHeight || 0);
var to_apply_height = height - 50 -50; // header and footer height
document.getElementById('main_content').style.height = to_apply_height+'px';

<!-- css -->
#main_content{
overflow-y:scroll;
}

You can use flexbox. This works in all modern browsers, and will also make it responsive.

.wrapper {
  /* Makes the wrapper fill the entire screen */
  position: fixed;
  top: 0;
  height: 100%;
  left: 0;
  width: 100%;
  /* The important bit */
  display: flex;
  flex-direction: column;
}

.main {
  /* Makes the main content take up the remaining space */
  flex: 1;
  overflow-y: scroll;
}

.header, .footer {
  height: 50px;
}

JSFiddle


Git hook written on JavaScript As in many teams, me and my coworkers are putting ticket number into beginning of each commit message. If your branches are starting with ticket number it is easy to create prepare-commit-msg hook to automate this action, and since I'm a front-end deveoper, I decided to write it as a nodejs script. Look at what I came up with and tell me how to make it better if you will.

#.git/hooks/prepare-commit-msg

#!/usr/bin/env node

const fs = require('fs')
const { exec } = require('child_process')

const COMMIT_EDITMSG = process.argv[2]

exec('git symbolic-ref --short HEAD', (err, stdout) => {
    if (err) throw err

    fs.readFile(
        COMMIT_EDITMSG,
        'utf8',
        (err, message) => {
            if (err) throw err

            fs.writeFile(
                COMMIT_EDITMSG,
                `${stdout.replace(/(-|_).*/, '')} ${message}`.replace(/\n/, ''),
                err => {
                    if (err) throw err
                }
            )
        }
    )
})

Outwith the improved error handling as per the comments, another improvement on this be to write the hook using a more synchronous approach e.g.

#.git/hooks/prepare-commit-msg
#!/usr/bin/env node

const { readFileSync, writeFileSync } = require('fs')
const { execSync } = require('child_process')
const COMMIT_EDITMSG = process.argv[2];

const stdout = execSync('git symbolic-ref --short HEAD');
const message = readFileSync(COMMIT_EDITMSG, 'utf-8');
writeFileSync(COMMIT_EDITMSG, `${stdout.replace(/(-|_).*/, '')} ${message}`.replace(/\n/, ''));

This completely eliminates the need for callbacks and, naturally, if an error occurs the commit should be blocked - it's also a lot more succinct and easier to read.


unique_ptr usage too unwieldy I'm creating a MUD and am running into some design issues regarding Items.

Items are stored in an ItemDatabase after being read from a JSON file.

template <typename ItemType>
static void load(std::string const& filename)
{
    PropertyTree propertyTree;
    boost::property_tree::read_json(filename, propertyTree);

    for (auto& jsonItem : propertyTree)
    {
        Item *item = new ItemType();
        item->fromJson(jsonItem);

        database_[item->id()] = item;
    }
}

At first, every Database::find() returned a shallow copy of the item, however, this quickly becomes dangerous and erroneous when items have a finite usage, e.g. a life potion that is consumed after one charge.

Therefore, I want to start returning deep copies as std::unique_ptrs because each Item is unique and is possessed by only one entity. This avoids dangerous dangling pointers, if the Item is not maintained or destroyed by the caller:

template <typename DataType>
std::unique_ptr<DataType> EntityDatabase<DataType>::find(EntityId const& entityId)
{
    auto found = std::find_if(std::begin(database_), std::end(database_),
                              [&entityId](Container::value_type const& candidate)
                              {
                                  return (candidate.second->id() == entityId);
                              });

    if (found != std::end(database_))
        return std::unique_ptr<DataType>(new DataType(*found->second));
    else
        return nullptr;
}

However, the resulting code is worse than hideous. Consider turning an Item into a Weapon:

void Inventory::fromJson(PropertyTree::value_type const& properties)
{
    auto jsonInventory = *properties.second.find("Inventory");

    auto weaponId = getValue<EntityId>(jsonInventory, "Weapon");
    weapon_.reset(static_cast<Weapon*>(ItemDatabase::find(weaponId).release()));

    auto armorId  = getValue<EntityId>(jsonInventory, "Armor");
    armor_.reset(static_cast<Armor*>(ItemDatabase::find(armorId).release()));

    money_ = getValue<Money>(jsonInventory, "Money");

    for (auto& item : jsonInventory.second.get_child("Items"))
    {
        auto itemId = boost::lexical_cast<EntityId, std::string>(item.second.data());
        inventory_.push_back(std::move(ItemDatabase::find(itemId)));
    }
}

How can I improve the code or the design?

Let me restate the problem, because it wasn't 100% clear to me from what you've written. I personally think you've left out a bit too much code. I'll state my assumptions of what is going on, but ideally this should be clear from what is posted. Firstly, I assume EntityDatabase is a class, not a namespace. Secondly, I assume ItemDatabase is declared as something like:

EntityDatabase<Item> ItemDatabase;

Further, I assume you have some kind of inheritance hierarchy that looks something like:

class Item
{
    Item();
    virtual ~Item();
    //....
}

class Weapon : public Item
{
    //...
}

class Armor : public Item
{
    //...
}

The problem then is that ItemDatabase.find(...) (I assume it's meant to be ., not ::) will return a std::unique_ptr<Item> when you want a std::unique_ptr<Weapon> or some other derived class of Item.

This seems less a problem of unique_ptr and more the fact that you're kind of abusing polymorphism. Let's assume you didn't return a unique_ptr, just a normal Item*:

template <typename DataType>
DataType* EntityDatabase<DataType>::find(EntityId const& entityId)

The only thing that would change then is that armor_ would be declared as Armor* instead of std::unique_ptr<Armor>, so it would become:

 armor_ = static_cast<Armor*>(ItemDatabase::find(armorId));

Hence, the only things that really change are the calls to release and reset. This is not significantly less ugly than what is already there. If armour_ were declared as std::unique_ptr<Item> then you could of course do the following:

armor_ = std::move(ItemDatabase::find(armorId));

Again, this is all built on a lot of assumptions since there is quite a bit of context missing.

For fixes, I suppose the options are:

Leave it as is. It's fairly ugly, but not that much uglier than the code you'd have for raw pointers.
Fix your inheritance hierarchy. The fact that you're having to cast things back and forth suggests that perhaps polymorphism isn't the correct solution here. Does each item need to know its specific type? Is there a virtual function you can add to Item that will fix this? I don't know, there's not enough context from the code you've posted.
Finally, make a function to limit the uglyness to one spot:

template <typename T, typename U>
void recast(std::unique_ptr<T>& ptr1, std::unique_ptr<U>&& ptr2)
{
    static_assert(std::is_base_of<U, T>::value, "U is not a base type of T");
    ptr1.reset(static_cast<T*>(ptr2.release()));
}

....
auto armorId  = getValue<EntityId>(jsonInventory, "Armor");
recast(armor_, ItemDatabase::find(armorId));

This is still pretty ugly, but if used often, might be slightly less offensive.


Fermat Factorization Please review my implementation of Fermat's Factorization.

fermat :: Int -> Maybe (Int, Int)
fermat n = odd' n >> (go $ ceiling $ sqrt (fromIntegral n))
   where go a = case (try n a) of j @ (Just _) -> j
                                  Nothing      -> go (a + 1)

odd' :: Int -> Maybe Int
odd' x = if (odd x) then Just x else Nothing

try :: Int -> Int -> Maybe (Int, Int)
try n a = fmap (\b -> ((a + b), (a - b))) result
  where result = get_perfect_sq (a^2 - n)

get_perfect_sq :: Int -> Maybe Int
get_perfect_sq x = if (sq * sq == x) then Just sq else Nothing
  where sq = floor $ sqrt (fromIntegral x :: Float)

examples

ghci> fermat 5995
Just (109,55)

-- prime per http://www.bigprimes.net/cruncher/1300633/
ghci> fermat 1300633
Just (1300633,1)

odd' seems like a useless function, you never use its result, only its monadic context. Use a guard on fermat instead.

fermat n | odd n     = Just $ -- ...
         | otherwise = Nothing

Don't use snake_case in Haskell (get_perfect_sq). We're a camelCase language.

Instead of writing your recursive functions explicitly, try to use higher order functions out of Prelude. They more clearly express intent to readers of your code by highlighting common patterns. In this case, having read Wikipedia's page on Fermat's factorization method, the pseudo-code given in the Basic Method section shows that the primary operation of the function iterates on changing values of \$a\$ and \$b2\$. In Haskell we can use iterate to produce an infinite lazy stream of values, and find to grab the first that meets a condition.

-- given odd n
let a  = ceiling . sqrt $ fromIntegral n
    b2 = a * a - n
in find squareb2 $ iterate step (a, b2)

I'd also be sure to strap a comment to the top with the pseudo-code or algorithm you're trying to implement. It aids understanding and error recognition far easier.

{-
From https://en.wikipedia.org/wiki/Fermat%27s_factorization_method

FermatFactor(N): // N should be odd
  a ← ceil(sqrt(N))
  b2 ← a*a - N
  while b2 isn't a square:
    a ← a + 1 // equivalently: b2 ← b2 + 2*a + 1
    b2 ← a*a - N // a ← a + 1
  endwhile
  return a - sqrt(b2) // or a + sqrt(b2)
-}
fermat :: Integer -> Maybe (Integer, Integer)
fermat n | odd n     = Just . solutions $ find squareb2 (iterate step (a_0, b2_0))
         | otherwise = Nothing
  where
    a_0  = ceiling . sqrt $ fromIntegral n
    b2_0 = a_0 * a_0 - n

    step (a, b2) = (a + 1, b2 + 2 * a + 1)

    squareb2 = isSquare . snd

    solutions (a, b2) = let b = floor . sqrt $ fromIntegral b2 in (a - b, a + b)


Brute force Caesar Cipher decrypter I wrote the following that tries to crack a message that was encrypted via a simple Caesar cipher through brute force. I'm fairly new to Haskell, so any and all input is appreciated.
Because there are infinite ways that something could have been encrypted, it assumes very simple "offset" encryption; like what decShift uses. It also doesn't assume any "wrapping" if an offset character goes out of range.

It works fine (from my testing), I'd just like any input on how it could be made more readable, idiomatic, or generally better.

Use:

simpDec "Encrypted Message", which returns a list of tuples where the first element is the key used to obtain the decryption, and the second is the decrypted message (it returns a list because it's possible for it to return more then one possible decryption).

decUpper = 127 --Upperbound of normal characters
decLower = 32 --Lowerbound of normal characters
decRange = decUpper - decLower --Total range

type Message = String

--Checks if the char is in range
isInRange :: Char -> Bool
isInRange char = ichar <= decUpper && ichar >= decLower
    where ichar = ord char

--Makes sure all chars in the Message are in range
isPossible :: Message -> Bool
isPossible [] = False
isPossible m = null [letter | letter <- m, not $ isInRange letter]

--Returns a list > 0 if it finds consecutive symbols
conSymList :: Message -> Char -> [Char]
conSymList [] _ = []
conSymList (x:xs) lastChar
    | isSym x && isSym lastChar = x : conSymList xs x
    | otherwise = conSymList xs x
    where isSym c = isSymbol c || isPunctuation c

--Checks if a string has 2 symbols beside each other (meaning its improbable)
isProbable :: Message -> Bool
isProbable [] = False
isProbable m = null $ conSymList m 'a'

--Shifts all characters by n
decShift :: Message -> Int -> Message
decShift [] _ = []
decShift (x:xs) n
    | iNewChar > 0 = chr iNewChar : decShift xs n
    | otherwise = x : decShift xs n
    where iNewChar = ord x + n

--Returns a list of tuples of the offset used, and the shifted Message
getCombos :: Message -> [(Int,Message)]
getCombos m = [(off,decShift m off) | off <- [decRange * (-1)..decRange]]

--Returns a list of tuples of possible decryptions (KeyUsed,Message)
simpDec :: Message -> [(Int, Message)]
simpDec m = [(key,try) | (key,try) <- getCombos m, isPossible try && isProbable try]

import Data.Char (chr, isPunctuation, isSymbol, ord)

decUpper = 127 --Upperbound of normal characters
decLower = 32 --Lowerbound of normal characters
decRange = decUpper - decLower --Total range

type Message = String

-- | Checks if the char is in range
isInRange :: Char -> Bool
isInRange char = ichar <= decUpper && ichar >= decLower
    where ichar = ord char

-- Test if all chars in the Message are in range
isPossible :: Message -> Bool
isPossible = all isInRange

-- Turn a list into a list of pairs
mkPairs :: [a] -> [(a,a)]
mkPairs xs = zip xs (tail xs)

-- no consecutive symbols in Message
isProbable :: Message -> Bool
isProbable = not . any (\(x,y) -> isSym x && isSym y) . mkPairs
  where isSym c = isSymbol c || isPunctuation c

-- Shifts all characters by n
decShift :: Message -> Int -> Message
decShift []     _  = []
decShift (x:xs) n
    | iNewChar > 0 = chr iNewChar : decShift xs n
    | otherwise    = x : decShift xs n
  where iNewChar = ord x + n

-- Returns a list of tuples of the offset used, and the shifted Message
getCombos :: Message -> [(Int,Message)]
getCombos m = [(off,decShift m off) | off <- [negate decRange .. decRange]]

-- Returns a list of tuples of possible decryptions (KeyUsed,Message)
simpDec :: Message -> [(Int, Message)]
simpDec = filter ((\x -> isPossible x && isProbable x) . snd) . getCombos

Changes:

used more function composition
simpDec is really a filter on results. getCombos generates data so a list comprehension makes sense.
simplify isProbable with the definition of mkPairs
simplify isPossible using the all function.
added import line


Mobile webpage for a refugee organization For a school project, I've to create a website for a refugee foundation in Haarlem. But designing is one part, coding the actual design is a whole other world. The hamburger menu went pretty well and I'm very proud of that piece of code, but the mosaic tiles just don't feel right. It is designed mobile first, so the desktop version is completely forgotten at this point of the project. This is how it should look:

And well, it does look like it. But the code is a big mess in my opinion:

main {
  display: grid;
  grid-template-areas: 'one one one two two' 'three three four four four';
  grid-template-columns: repeat(5, 1fr);
  grid-gap: 45px;
  height: 70%;
  width: 87.5%;
  margin: 2.5% auto;
}

.one,
.two,
.three,
.four {
  display: grid;
  grid-template-rows: 1fr 1fr 1fr;
  font-family: functionbold;
  font-size: 4em;
  text-transform: uppercase;
  color: white;
  text-align: center;
}

.one {
  grid-area: one;
  background: url("../images/mozaik1.jpg") no-repeat center;
  background-size: 175%;
}

.text {
  grid-row: 2;
  align-self: center;
}

.two {
  grid-area: two;
  background: url("../images/mozaik2.jpg") no-repeat center;
  background-size: 250%;
}

.three {
  grid-area: three;
  background: url("../images/mozaik3.jpg") no-repeat left;
  background-size: 275%;
  background-position: -100px 0;
}

.four {
  grid-area: four;
  background: url("../images/mozaik4.jpg") no-repeat center;
  background-size: 150%;
}

Samen Praten

Samen Shoppen

Samen Doen

Samen Eten

Could someone please help me with simplifying and beautifying this piece of code?

This question is difficult to answer because art is subjective but if this was my site, I would limit the gutters between the images to be slightly closer together (perhaps 2 maybe 3 ems) and being sure to keep them extending out to the width of the boxed layout.

If I was a user viewing this site, I would be so confused because there is no indication of actions to use.  For example, the website doesn't tell the user anything about what actions to take.  I would add some content to the top, maybe some rollover or pop-overs on my images to encourage user interactions.

Last thing, if this site is a non profit, it is pretty common to re-enforce the mission and/or vision statement on the homepage for the user to understand the commitment of the nonprofit.  Foundations are typically trying to raise money for a cause and without the user knowing or understanding the cause, they will likely not take action. My college professors all said that the user should be able to find information immediately, if they have to look they are going to leave.

Without seeing the actual site, I am going off of my design knowledge so I hope this helps. If you want to share your HTML to show how you are placing your code and what is actually happening on the site, I will try my best to help you clean up the CSS.