Parse a search condition into an expression tree I had to implement a parser according to a given grammar as part of a school project. However, only the program execution was graded and I want to know if my code adheres to best practices. I would really appreciate it if you could give me pointers on how to improve my code.

Search conditions are of the form: a + b < 10 OR c = 1. The following code parses the search condition into an expression tree. The input to the function is a vector of the tokens in the search condition.

void recursiveSplit(node *parent, vector<string> const &tokens, string delimiter) {
    /* Given a string, parse it into an expression tree with nodes as the
     * operators and the children as the operands */
    if (tokens.size() == 1) {
        /* Base case for the recursion, the leaf nodes are either strings or
         * integers */
        node *mathNode;
        mathNode = new node(tokens[0]);
        parent->subTree.push_back(mathNode);
        return;
    } else {
        vector<string>::const_iterator it;
        for (it = tokens.begin(); it != tokens.end(); it++) {
            if (*it == delimiter) {
                break;
            }
        }
        if (it != tokens.end()) {
            /* Case 1 :delimiter was found */
            node *delimNode;
            delimNode = new node(delimiter);
            parent->subTree.push_back(delimNode);
            vector<string> leftTokens(tokens.begin(), it), rightTokens(it+1, tokens.end());
            string newDelimiter = switchDelimiter(delimiter);
            recursiveSplit(delimNode, leftTokens, newDelimiter);
            /* In rightTokens, we still need to search for the current
             * delimiter since we may have multiple instances of the delimiter
             * */
            recursiveSplit(delimNode, rightTokens, delimiter);
        } else {
            // delimiter is not found
            string newDelimiter = switchDelimiter(delimiter);
            recursiveSplit(parent, tokens, newDelimiter);
        }
    }
}

string switchDelimiter(string delimiter) {
    /* Return the next delimiter based on the priority. The priority of
     * operations is : OR, AND, <, >, =, +, -, * */
    char delim;
    if (delimiter == "OR") {
        delim = '|';
    } else if (delimiter == "AND") {
        delim = '&';
    } else {
        delim = delimiter[0];
    }

    switch(delim) {
        case '|':
            return "AND";
        case '&':
            return "<";
        case '<':
            return ">";
        case '>':
            return "=";
        case '=':
            return "+";
        case '+':
            return "-";
        case '-':
            return "*";
        case '*':
            return " ";
    }
}

node *createTree(const string& searchStrBuf) {
    string buf;
    stringstream ss(searchStrBuf);
    vector<string> tokens;
    while(ss >> buf) {
        if ((buf != ")") && (buf != "(")) {
            tokens.push_back(buf);
        }
    }
    node *root = new node("searchTreeRoot");
    recursiveSplit(root, tokens, "OR");
    return root;
}

class node {
    public:
    string nodeType;
    vector<node *> subTree;

    node(string strNodeType) {
        nodeType = strNodeType;
    }
};

You don't show how this is being called. So we have to make some assumptions:

I assume that the search conditions have been parsed into tokens as one-token-per-string in the vector? That is, for search conditions like "a + b < 10 OR c = 1" (your example), the tokens vector looks like { "a", "+", "b", "<", "10", "OR", "c", "=", "1" }.
I assume that the node type contains a vector<node *> called subTree.
You also don't specify if you expect the resulting tree to be a binary tree or not. Your code is written that way, but it doesn't seem to be a requirement. Other than the relational operators, the rest of your operators could easily handle more than two operands: a OR b OR c, 1 + 2 + 3, etc. I assume that the tree must be binary, since you coded it that way.

With those assumptions, I observe:

You are not handling the case of a zero-length tokens vector. This would be an empty input, or an error of some kind.
You are not handling syntax errors at all, really. What should your code be returning for something like a + * b?
You are using recursion when it is not necessary. Specifically, your switchDelimiter function is really just a way to hide iteration. It would be better, IMO, to write your code to loop over the delimiters instead of calling a function and then recursing with the same tokens and a new delimiter.
You are using recursion when it is not necessary. Specifically, your "right subtree" case recurses with the same delimiter, and it + 1. That's a sign that you can just continue your existing loop, after updating the parent node to a new value. (You will have to move your "new node" code inside the loop, though.)
 3.


sed to fix a messy nginx.conf I have a few servers to fix that repeatedly get their nginx.conf file messed up due to a rogue deploy script.

These are the three sed commands I'm using to fix at present:

sed -i '' 's/^\([[:blank:]]*\)#\([[:blank:]]*\)server_names_hash_bucket_size/\1server_names_hash_bucket_size/g' nginx.conf

The first finds any kind of blank space at the start of a line followed by a pound symbol and the string server_names_hash_bucket_size. Then it replaces it with the original blank space (to honour any indentations) and the string without the pound.

sed -i '' '/^\([[:blank:]]*\)server_names_hash_bucket_size/s/[0-9]//g' nginx.conf

The second gets rid of any numbers:

sed -i '' '/^\([[:blank:]]*\)server_names_hash_bucket_size/s/;/64;/g' nginx.conf

The third finds the semi colon and replaces it with the necessary value 64;

I'm currently testing this in terminal OSX, but ultimately will be used on Linux machines.

Does anyone know how to tidy this up and can point out any potential Linux gotchas with sed?

In the general case, the answer to any question regarding sed is "use something else."

Not that sed is not useful! It definitely is!  It's just that it's best for simple tasks that have compact, obvious solutions.

In particular, Perl is good at replacing sed scripts, thanks to its more expressive regular expressions and uncomplicated access to conventional programming constructs.   It even comes with an automated tool to do the translation from sed to Perl, called s2p.

perl -pli.bak -e'
    next unless s/^(\s*)(#\s*)?(?=server_names_hash_bucket_size)/$1/;
    tr/\d//d;
    s/;/64;/;
' nginx.conf

The newlines are optional and included for readability.  Line by line:

-p means "print each line after running this code against it."  This should be familiar since sed works exactly the same way.
-l means "remove newlines before running the code and reinstate them before printing."
-i.bak means "edit in-place and save the original as original.bak".
-e means the same thing as in sed.

We're only interested in lines with server_names_hash_bucket_size, so skip to the next line if the replace fails (the unmodified line still prints; we're skipping the rest of this code but not the printing).
We need to include lines that had no #, so make the # match optional with ? after (#\s*).
(?=...) is a lookahead; the text must exist but is not included in the left side of the substitution, so we don't need to include it on the right side.   Only the (optional) leading whitespace ($1) needs to be reinstated on the RHS.

tr works like y in sed.  This line is equivalent to s/[0-9]//g (as far as we're concerned.  Technically, there are digits other than 0-9 but they aren't going to appear in your nginx.conf.)
Same as sed.

You can simplify even further by recognizing that the only significant parts of a server_names_hash_bucket_size line, after you throw away the number, are the leading whitespace and terminal comments.

Preserve the indent as before, throw away as many non-comment characters as possible ([^#]*) and rewrite the line in one replace operation:

perl -pli.bak -e' s/^(\s*)(?:#\s*)?(server_names_hash_bucket_size)[^#]*/$1$2 64;/ ' nginx.conf

(?: makes that group non-capturing, so the second capture ($2) will be server_names_hash_bucket_size.

At this point, we're back in "simple enough for sed" territory:

s/^\([[:blank:]]*\)\(#[[:blank:]]*\)*\(server_names_hash_bucket_size\)[^#]*/\1\3 64;/


Throttle/debounce implementation performance I'm trying to create a good way to manage throttling and debouncing  a lot of functions. What I have allows you to group, add and remove functions to instances of Booster. Each instance runs it's own timers and can be manipulated by methods.

Are there any major performance concerns with the approach below? I'm brand new to ES6, scope/context issues, and come from a mostly jQuery background—so I don't know much about performance, memory leaks, etc. Any tips are appreciated.

// booster.js

export default class Booster {

  constructor(method, type, amount, ...fn) {
    this.view = window;
    this.type = type;
    this.amount = amount;
    this.fn = fn ? fn : [];
    this.paused = false;
    this.method = method;
    this.init();
  }

  raf() {
    if (this.view.requestAnimationFrame) {
      this.view.requestAnimationFrame(this.run.bind(this));
    } else {
      this.run().bind(this);
    }
  }

  run() {
    if (0 < this.fn.length) {
      this.fn.forEach(function(fn) {
        fn();
      });
    }
  }

  throttle() {
    this.active = true;
    this.view.addEventListener(this.type, () => {
      if (this.active && !this.paused) {
        this.active = false;
        this.raf();
        this.timeout = setTimeout( () => {
          this.active = true;
        }, this.amount);
      }
    }, false);
  }

  debounce() {
    this.view.addEventListener(this.type, () => {
      if (!this.paused) {
        clearTimeout(this.timeout);
        this.timeout = setTimeout( () => {
          this.raf();
        }, this.amount);
      }
    }, false);
  }

  add(...fn) { this.fn.push(...fn); }
  remove(n) {
    if (n < this.fn.length && n >= 0) {
      this.fn.splice(n, 1)
    } else {
      throw new Error('There are only ' + this.fn.length + ' functions in this Booster.');
    }
  }

  pause()    { this.paused = true;}
  resume()   { this.paused = false; }
  toggle()   { this.paused = !this.paused; }

  init() {
    if      ('throttle' === this.method) { this.throttle(); }
    else if ('debounce' === this.method) { this.debounce(); }
    else { throw new Error('Invalid Booster method. Must be "throttle" or "debounce".'); }
  }
}

usage:

// main.js

import Booster from './classes/booster.js';

let scrollDebouncer1 = new Booster('debounce', 'scroll', 100, myFunction());

scrollDebouncer1.add(myOtherFunction());

scrollDebouncer1.pause();

// etc.

Problems

There was a bug in Booster.raf in the else block you have this.run().bind(this) and I assume you mean this.run.bind(this)() though the binding is not needed because you are in the context of this so this.run() will do just fine.

You throw in two places yet ignore many problems that can be introduced by not vetting at the interface. There should be more checking when setting the Booster state. Eg amount should be a Number, should not be less than 0. Booster.remove(n) n could be a fraction, you should ignore or truncate floats.

It is possible for a function to execute even after Booster is paused. This will happen if pause() is called after the event is triggered. The run function should make sure that you do not run in paused state.

If any of the given functions throw an error then all remaining function on the fn array will not be executed. Nor do you check if any of the functions given are actually functions. This will also prevent functions after the bad function from being called.

You do not check if Booster.view has the function addEventListener. In Booster.throttle this will result in the Booster object incorrectly setting the active state to true.

It is possible to have more than one event listener if Booster.init is called more than once. Because you only have one Booster.timeout property the Booster.throttle function can overwrite the Booster.debounce timeout property preventing debounce from working correctly (and debounce can stop throttle timeouts breaking throttling)

Design
The logic is not fully thought out. It is always important to test your code for behaviour that are not expected.
I can not tell if you expect init to be called again after construction. The fact that it is outside the constructor suggests so. If this is the case, why are you not providing a way to remove listeners. Calling init will just keep adding listeners and because the added listeners are anonymous there is no way to remove them.
I do not know how many events you are expecting to intercept or delay, but any more than a few and you would be much better of running a continuous rAF loop and handling all the needed events in that one function.
Review by comments.
All comments within /* ... */ are added as I went over the code, and other comments are me modifying your code by removal. The comments are generally unrelated suggestion and were created before I wrote the review above.
export default class Booster {

  constructor(method, type, amount, ...fnArray) {
    this.view = window;
    this.type = type;     /* type of what? bad naming maybe eventType */
    this.amount = amount; /* amount? bad naming maybe bebounceDelay */
    /* fn will always be an array as you have defined it as so in the arguments
       this.fn = fn ? fn : [];  // bad naming use plural or indicate its an array
       Use existing methods */
    // this.fnArray = fnArray;
    this.add(fnArray);
    /* Use existing methods */
    //this.paused = false;
    this.pause();
    this.method = method; /* method? bad naming maybe methodType */
    this.runBound = this.run.bind(this);  /* Only need to bind once */

    /* calling init in the constructor always seams such a silly thing to
        do especially when init does not actually init the class */
    /* Use a setter to set the method */
    // this.init();
  }
  /* Use a setter to set method and remove init as it is not needed */
  set method(type) {
    if      (type === 'throttle') { this.throttle(); }
    else if (type === 'debounce') { this.debounce(); }
    /* Why throw, You are not in control of the calling function so how do you know it
        will be caught. Throwing will stop executing code and in production if
       not caught end up in a non functioning page.
       Degrade gracefully, don't fall in a heap at the slightest fault. */
    else  { console.warn("Invalid Booster method. Must be \"throttle\" or \"debounce\".") }
    //  else { throw new Error('Invalid Booster method. Must be "throttle" or "debounce".'); }
  }
  raf() {
    if (this.view.requestAnimationFrame) {
      this.view.requestAnimationFrame(this.runBound);
    } else {
      /* Run then bind ?????   you had this.run().bind(this);
         maybe you meant this.run.bind(this)(); */
      this.run();
    }
  }

  run() {
     /* Don't need to check if zero. For each will do that for you */
    // if (0 < this.fn.length) {
    /* try to avoid callback iterators as they are slower than for loops */
      //this.fn.forEach(function(fn) {
      //  fn();
      //});
    /* a better way to iterate
    for(const fn of this.fnArray){ fn() }  */
  }

  throttle() {
    this.active = true;
    this.view.addEventListener(this.type, () => {
      if (this.active && !this.paused) {
        this.active = false;
        this.raf();
        this.timeout = setTimeout( () => {
          this.active = true;
        }, this.amount);
      }
    }, false);
  }

  debounce() {
    this.view.addEventListener(this.type, () => {
      if (!this.paused) {
        clearTimeout(this.timeout);
        this.timeout = setTimeout( () => {
          this.raf();
        }, this.amount);
      }
    }, false);
  }

  add(...fnArray) { this.fnArray.push(...fnArray); }
  remove(n) {
    if (n >= 0 && n < this.fn.length) {
      this.fn.splice(n, 1)
    }
    /* Again you are not in control of calling code and can not know
      if this will be caught. I dont think this is even worth a warning  */
  //    else {
  //  throw new Error('There are only ' + this.fn.length + ' functions in this Booster.');
    //}
  }

  pause()    { this.paused = true;}
  resume()   { this.paused = false; }
  toggle()   { this.paused = !this.paused; }

  /* removed as not needed, see setter set method */
  //init() {
  //  if      ('throttle' === this.method) { this.throttle(); }
  //  else if ('debounce' === this.method) { this.debounce(); }
  //  else { throw new Error('Invalid Booster method. Must be "throttle" or "debounce".'); }
  //}
}


Encrypting and decrypting passwords in PHP I am wanting to encrypt a password and decrypt a password using PHP. Is this a safe method?

$pass = "password"

//encrypt password
$salt = strtr(base64_encode(mcrypt_create_iv(16, MCRYPT_DEV_URANDOM)), '+', '.');
$salt = sprintf("$2a$%02d$", 10) . $salt;
$hash = crypt($pass, $salt);

//decryption in second program
if(crypt($pass, $hash) == $hash){
    echo "you are in";
}

The name of the function crypt() is somewhat misleading, but you're not encrypting and decrypting. You create a hash and you check it, that's it.

It's not the best I have seen, but it is not uncommon to do it this way.

In the PHP crypt() documentation it says: Use of password_hash() is encouraged. You didn't see that? It allows you to choose an algorihtm. The PASSWORD_BCRYPT algorithm should be used. See: http://codahale.com/how-to-safely-store-a-password

I also see that you're not really using a salt in the traditional manner. Here's a nice introduction: http://www.martinstoeckli.ch/hash/en/index.php

So you're not using a salt on a per-password basis. See: http://blog.codinghorror.com/youre-probably-storing-passwords-incorrectly

There's much to say about this topic. Password security is a hot topic.

One of the mistakes most programmers make is that they rely on the user to select a password. In general users are just plain bad at choosing a secure password. You can try and force them to choose one, which is probably secure, but by doing that you will only annoy the hell out of the user. The solution is quite simple: You choose the password for the user. For a site where security is not that paramount you can use simple password. A random five digit number, in combination with a email address and brute force protection, is already reasonable secure; there are a million different possible combinations. Most developers however stick to what they know.


Unloading external DLL library after 10 seconds in order to release file locks In my web application, I'm utilizing a PDF library which is using a native DLL.

I'm using it for two very different purposes in two completely separate (and isolated by NuGet) parts of the web application.

Upon application start, I load the library DLL and thus lock the file. Then, rebuild requires completely terminating the application which takes much time and disrupts our workflow.

We cannot load and unload the DLL on every operation, as the operation will be executed lots of times in bursts. This will also not be thread safe.

So in order to alleviate this, I have decided to make a small wrapper to the PDF library that:

for every operation (it will only have 2) it will check if the library is loaded.
If not, it loads it. After the operation, it sets a timer that will unload the DLL in, let's say 10 seconds.
If another operation is executed within this timeframe, the timer is reset.

This is the code I have written so far to achieve this, appreciate any pointers.

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Timers;

namespace PdfLibrary
{
    public static class PdfLibrary
    {
        static object Lock = new object();
        static TimeSpan Timeout = new TimeSpan(0, 0, 10);
        static bool IsLoaded = false;
        static Timer UnloadTimer = new Timer();

        static PdfLibrary()
        {
            UnloadTimer.Interval = Timeout.TotalMilliseconds;
            UnloadTimer.Elapsed += (_, __) => Unload();
        }

        private static void Unload()
        {
            UnloadTimer.Stop();

            lock (Lock)
            {
                // ... unloads pdf library ...
            }
        }

        static void DoWhenLoaded(Action action)
        {
            if (!IsLoaded)
            {
                lock (Lock)
                {
                    if (!IsLoaded)
                    {
                        // ... loads pdf library ...
                    }
                }
            }

            action();

            UnloadTimer.Stop();
            UnloadTimer.Start();
        }

        public static IEnumerable<string> ExtractText(Stream inputStream)
        {
            IEnumerable<string> result = null;

            DoWhenLoaded(() => {
                // ... sets result ...
            });

            return result;
        }
    }
}

Another, perhaps simpler, option is to locally copy the dll and load the copy.

Then regularly check the original dll for an updated last modified time and then unload the copied dll, make a fresh copy and load the new copy.

Do keep in mind that when debugging a dll has a pdb file associated with it and loading the dll also locks the pdb. You can work around that by making the compiler emit the pdb with a different name each time.

However as is your implementation isn't thread safe. If the timer is about to trigger then action could still be running. Instead you will want to use a ReaderWriterLock, take a read lock when doing action() and a write lock when actually loading and unloading.

Lock.AcquireReaderLock(1000);
try{
    if (!IsLoaded)
    {
        LockCookie cookie = Lock.UpgradeToWriterLock(1000);
        try {
            //someone else may have gotten ahead in the queue
            if (!IsLoaded)
            {
                // ... loads pdf library ...
            }
        } finally {
            Lock.DowngradeFromWriterLock(cookie);
        }
    }
    action();

} finally {
    Lock.ReleaseReaderLock();
}


Printing the PID of a program immediately before it runs I need to know (for monitoring purposes) the PID of a program immediately before I start it. There could be multiple of the same program launched at the same time, so monitoring ps or top isn't really an option. It dawned on me as I was exploring various bash-related options that I could make use of C's exec functions to try and pull this off. With that in mind, I whipped up this little piece of code:

#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <unistd.h>

int main(int argc, char **argv)
{
    char **argv_copy;
    int i;

    if (argc >= 2)
    {
        argv_copy = malloc(argc * sizeof *argv_copy);

        for (i = 1; i < argc; i++)
        {
            argv_copy[i-1] = argv[i];
        }
        argv_copy[argc] = (char *) NULL;

        printf("%lu\n", (long unsigned) getpid());

        execvp(argv[1], argv_copy);
    }
    return 0;
}

It works as expected - it immediately prints out the PID of the process and then loads the actual process I want to run. What I'm not convinced of is the security or robustness of this little hack. I don't need it to be 100% bullet-proof security-wise, but if you can drive an SUV through it I'd like to know.

Could I get some advice on these two areas specifically?

Bug
This line:

    argv_copy[argc] = (char *) NULL;

should be:
    argv_copy[argc-1] = NULL;

You are removing one argument so you need to terminate the array at the right place.
Copy unneeded
Instead of:

    execvp(argv[1], argv_copy);

you could do:
    execvp(argv[1], &argv[1]);

and avoid making a copy.


Simple magic 8 ball program I would just like some input on how I may improve my code(in terms of content not formatting) without changing the output. It's just a simple magic 8 ball program using dialog boxes.

import java.security.SecureRandom;
import javax.swing.ImageIcon;
import javax.swing.JOptionPane;

public class MagicEightBall {

    public static void main(String[] args) {

        String answers[] = {"It is certain", "It is decidedly so", "Without a doubt",
            "Yes - definitely", "You may rely on it", "As I see it, yes",
            "Most likely", "Outlook good", "Signs point to yes",
            "Yes", "Reply hazy, try again", "Ask again later",
            "Better not tell you now", "Cannot predict now", "Concentrate and ask again",
            "Don't count on it", "My reply is no", "My sources say no",
            "Outlook not so good", "Very doubtful"};

        //constructs a random number generator
        SecureRandom randomNumber = new SecureRandom();
        ImageIcon image = new ImageIcon("8ball_small.jpg");
        int counter = 0;

        //while loop allows the user to continue asking questions until they click no
        while(counter != 1) {
            //prompts the user to enter a question
            String answer = JOptionPane.showInputDialog(null,
                    "PLease enter a yes or no question:",
                    "WELCOME: Have your questions answered!", JOptionPane.INFORMATION_MESSAGE);

            //displays the answer
            if(answer != null)
                JOptionPane.showMessageDialog(null, answer+ "\n" + answers[randomNumber.nextInt(answers.length)],
                "The Magic-8 Ball has responded.", JOptionPane.PLAIN_MESSAGE, image);

            //gives the user the option to click yes or no to continue asking questions
            counter = JOptionPane.showConfirmDialog(null, "", "Would you like to ask again?",
                    JOptionPane.YES_NO_OPTION, 0, image);
        }

        //displays name after user enters no
        JOptionPane.showMessageDialog(null, "Programmed by my name", "Goodbye! Your answers have been answerd.",
                JOptionPane.PLAIN_MESSAGE, image);
    }
}

Your program looks nice and is easy to understand. I only could find a single thing that you should change.

The variable counter is wrong, since it doesn't count anything. You should remove that variable completely. Instead, write while (true), and check the return value of the "again" dialog whether it is something else than JOptionPane.YES. If so, break out of the loop.


Basic Java statistics class I noticed there's some code on StackOverflow that is, to put it mildly, suboptimal. I'm posting here to see if we can provide a better solution.

Here's a proposal for fixing it.

The approach is to provide a small utility class with static methods, so it can be used just like Math.method(...).

The standard deviation implements this formula, which applies to a sample.

The median function is taken from this answer, which is based on the reasonable assumption that the array can be sorted in memory.

import java.util.Arrays;

public class Statistics {

    static public double getMean(double[] nums) {
        double total = 0;
        for (double value : nums) {
            total += value;
        }
        return total / nums.length;
    }

    // sample version
    static public double getVariance(double[] nums) {
        if (nums.length <= 1) {
            return 0;
        }

        double sum = 0;
        double mean = getMean(nums);

        for (double value : nums) {
            sum += Math.pow(value - mean, 2);
        }

        return sum / (nums.length - 1); // notice the -1
    }

    // sample version
    static public double getStdDev(double[] nums) {
        return Math.sqrt(getVariance(nums));
    }

    static public double getMedian(double[] nums) {
        Arrays.sort(nums);

        if (nums.length % 2 == 0) {
            return ((double) nums[nums.length / 2] + (double) nums[nums.length / 2 - 1]) / 2;
        } else {
            return (double) nums[nums.length / 2];
        }
    }
}

Is there something you would add?

Assuming this works well, it only works for double (and float) arrays.

It does not work for int arrays, nor does it work for Container classes, although all it would require to get it to work would be a copy-paste and some minimal changes.

Could Java 8 lambdas help make the code more generic and reduce the need for copy-paste?

Feel free to comment.

First of all, I object to the get… naming convention.  "Get" implies that you are retrieving something that already exists (usually, though not always, paired with "set").  You wouldn't call Math.getCos(theta), would you?

The problems stem from accepting arrays in the first place.  The class would be much more useful as an accumulator, like this:

Statistics stats = new Statistics();
stats.datum(3);
stats.datum(4);
stats.datum(5);
System.out.println(stats.mean());
System.out.println(stats.stdDev());

A simple way to calculate the mean, variance, and standard deviation is to keep running totals \$\sum x_i^0\$ (i.e., the count), \$\sum x_i^1\$ (i.e., the sum), and \$\sum x_i^2\$ (i.e., the sum of the squares).  However, see Algorithms for calculating variance for a discussion of the merits of this method compared to others.

You could write

import java.util.OptionalDouble;

public class Statistics {
    private int sum0;
    private double sum1, sum2;

    public void datum(double x) {
        this.sum0++;
        this.sum1 += x;
        this.sum2 += x * x;
    }

    public int count() {
        return this.sum0;
    }

    public double sum() {
        return this.sum1;
    }

    public OptionalDouble mean() {
        if (this.count() == 0) {
            return OptionalDouble.empty();
        } else {
            return OptionalDouble.of(this.sum() / this.count());
        }
    }

    public OptionalDouble variance() {
        if (this.count() == 0) {
            return OptionalDouble.empty();
        } else {
            return OptionalDouble.of(
                   (this.sum2 - this.sum() * this.sum() / this.count())
                   / //////////////////////////////////////////////// /
                                     this.count()
            );
        }
    }

    public OptionalDouble stdDev() {
        if (this.count() == 0) {
            return OptionalDouble.empty();
        } else {
            return OptionalDouble.of(Math.sqrt(this.variance().getAsDouble()));
        }
    }
}

The median is trickier to calculate, as you would have to keep a list of all of the values.

Note that DoubleStream already gives you count(), sum(), and average().


C - 2D peak finder in This is an implementation of a 2D peak finder from MIT's Intro to Algos.
I'm a C beginner and I would really like some feedback on coding style, documentation and any other general C bad practices.
Additionally have I correctly freed the memory of the recursive calls?
#include <stdio.h>
#include <stdlib.h>

typedef struct max_vector {
    unsigned int col_index;
    unsigned int row_index;
    int index_value;
} max_vector;

max_vector * find_col_peak(int *arr, int rows, int col){
    /* Find the peak of a column.

    This function treats a 2D array like a 1D array and
    traverses using some basic pointer arithmetic.

    next item = *((i * row + arr) + j)
    where:
        - i == current row number (the y axis in matrix i.e. up and down)
        - j == column number in column space (the x axis i.e left to right)
        - row == the number of items in each row
        - *arr == the first item in the array
    */

    // we want a vector to save the max number and its index
    max_vector *vector = malloc(sizeof(max_vector));

    if (vector) {
        // set max to the first item of the column
        vector->col_index = 0;
        vector->index_value = *(col + arr);

        // loop through the rest of the column
        for (int i=1; i < rows; i++){
            int next_number = *((i * rows + arr) + col);
            if (next_number > vector->index_value){
                vector->col_index = i;
                vector->index_value = next_number;
            }
        }
        printf("the max value is %d and it is at index %d\n",
            vector->index_value, vector->col_index);
        return vector;
    }
    return NULL;
}

max_vector * peak_finder(int *matrix, int rows, int first, int last){
    /* Find a local 2D peak.

    NOTE: Expects a matrix of size row x col

    A peak is defined as:
    (i, j) >= (i, j + 1), (i, j - 1), (i + 1, j) (i - 1, j)
    */

    max_vector *vector;

    int middle = last / 2;
    vector = find_col_peak(matrix, rows, middle);

    if (vector){
        if (middle == first || middle == last){
            vector->row_index = middle;
            return vector;
        }

        if (last > first) {
            int current = *((vector->col_index * rows) + matrix + middle);
            int one_less = *((vector->col_index * rows) + matrix + middle - 1);
            int one_more = *((vector->col_index * rows) + matrix + middle + 1);

            if (one_less <= current >= one_more) {
                vector->row_index = middle;
                return vector;
            }

            // if current is not a peak we can free the vector
            if (one_less > current){
                free(vector);
                return peak_finder(matrix, rows, first, middle - 1);
            } else {
                free(vector);
                return peak_finder(matrix, rows, middle + 1, last);
            }
        }
        vector->row_index = middle;
        return vector;
    }
    return NULL;
}

int main(void) {
  int arr [5][5] = {
    {10, 8, 11, 10, 16},
    {14, 13, 12, 11, 24},
    {15, 9, 11, 21, 6},
    {16, 21, 19, 20, 12},
    {100, 6, 13, 19, 9},
  };

  max_vector *ptr = peak_finder(*arr, 5, 0, 5);
  printf("the local peak is at %d, %d", ptr->row_index, ptr->col_index);
  free(ptr);
  return 0;
}

Use standard array notation
When you are accessing array elements, the idiomatic way in C is to write foo[bar] instead of *(foo + bar). So instead of:
int next_number = *((i * rows + arr) + col);

Write:
int next_number = arr[i * rows + col];

It is both shorted, and now it is clear what part the array pointer is and what part the index is.
Avoid unnecessary memory allocations
It's perfectly possible in C to return a struct by value. This avoids having to call malloc() and free(), and you no longer have to worry about its memory leaking, nor have to check whether malloc() succeeded. You do it like so:
max_vector find_col_peak(int *arr, int rows, int col) {
    max_vector vector;

    // set max to the first item of the column
    vector.col_index = 0;
    vector.index_value = *(col + arr);

    ...

    return vector;
}

About recursion
With the above you don't have to worry about freeing memory for vector anymore. However, recursive calls themselves could have some memory overhead, since each call needs some amount of space on the stack. With this algorithm, you only need log2(cols) recursion levels, so that won't be an issue even for huge inputs. Also, the compiler can turn these recursive calls into tail calls.


jquery script for inserting and deleting text Unfortunately I get 3 warnings at jslint. Could someone please help?

I already tried a lot, but didn't get it yet.

Here's a jsfiddle: http://jsfiddle.net/KYjF9/21/

JSlint options:

/*jslint browser: true indent: 2 */ /*global $ jQuery alert*/

JavaScript:

var Typing = function (el, toType, period) {
  "use strict";
  this.toType = toType;
  this.el = el;
  this.loopNum = 0;
  this.period = parseInt(period, 10) || 2000;
  this.txt = '';
  this.tick();
  this.isDeleting = false;
};

Typing.prototype.tick = function () {
  "use strict";
  var fullTxt = this.toType[this.loopNum % this.toType.length];

  if (this.isDeleting) {
    this.txt = fullTxt.substring(0, this.txt.length - 1);
  } else {
    this.txt = fullTxt.substring(0, this.txt.length + 1);
  }

  $(this.el).html('' + this.txt + '');

  var delta = 300 - Math.random() * 100;

  if (this.isDeleting) { delta /= 2; }

  if (!this.isDeleting && this.txt === fullTxt) {
    delta = this.period;
    this.isDeleting = true;
  } else if (this.isDeleting && this.txt === '') {
    this.isDeleting = false;
    this.loopNum = this.loopNum + 1;
    delta = 500;
  }

  setTimeout(this.tick.bind(this), delta);
};

var data = {
  "text": {
    "list": ["hello", "hallo", "hola", "what", "ever"],
    "period": "3000"
  }
};

$(document).ready(function () {
  "use strict";
  var toType = data.text.list;
  var period = data.text.period;
  $(".js").each(function () {
    if (toType) {
      new Typing(this, toType, period);
    }
  });
});

I suppose you're talking about these 3 JSLint warnings:
Combine this with the previous 'var' statement.
  var interval = 300 - Math.random() * 100;

Combine this with the previous 'var' statement.
  var interval = 3000;

Do not use 'new' for side effects.
      new Typing(this, toType, interval);

The first 2 are suggestions to combine multiple var declarations into one. For example, instead of:

var x = 3;
var y = 4;

The suggestion is to do like this instead:
var x = 3, y = 4;

The 3rd one is about instantiating something without storing the reference somewhere. This page explains why this is not recommended, in particular:

By not assigning the return value of a constructor to something you will lose the reference to that instance. Generally, by constructing an instance you would want to keep that reference, whether to use again later or for "internal" use as part of a comparison. What's the point of constructing something you are going to throw away as soon as it's been created?
If you have a constructor function that performs work beyond simply setting up an instance, and you are calling that constructor just for these "side effects", consider reworking your code to allow you to call the function normally, without the new operator.

Personally I disagree with the suggestion to combine the var statements, but agree with the 3rd warning.


Add/Remove class on scroll depending on scroll length I am a jQuery beginner and would like some pointers on whether the following code could be made shorter, or there is a better way to get the same result.

jQuery(document).ready(function(){
     var aa=jQuery('#navigation-wrapper');
     var bb=jQuery('#top');
     jQuery(window).scroll(function(){

        if(jQuery("body").hasClass("home")){
            if(jQuery(this).scrollTop()>510){
                aa.addClass("navbar-fixed-top");
                //bb.css('marginTop', aa.height());
             }
            else{
                aa.removeClass("navbar-fixed-top");
                //bb.css('marginTop', 0);
            }
        }
        else {
           if(jQuery(this).scrollTop()>293){
             aa.addClass("navbar-fixed-top");
           //bb.css('marginTop', aa.height());
            }
            else{
                aa.removeClass("navbar-fixed-top");
                //bb.css('marginTop', 0);
            }
        }
     });
  });

The code can made very short.

$(window).scroll(function () {
    var scrollTop = $(document.body).hasClass('home') ? 510 : 293;
    $('#navigation-wrapper').toggleClass('navbar-fixed-top', $(this).scrollTop() > scrollTop);
});

There is no need of wrapping the code in $(document).ready() for binding events on window.
Use ternary operator
Use toggleClass() with second parameter to add/remove class based on a condition.


Langton's ant in Clojurescript The weekly coding challenge in Eric Normand's newsletter this week involves implementing Langton's ant.

I wanted something on my screen so I put together a simple re-frame app. The core functions relating to the ant:

world.cljs

(ns ant.world)

(defn init-world [width height]
  {:width width
   :height height
   :grid (into [] (repeat height (into [] (repeat width 0))))
   :ant-pos [(quot width 2) (quot height 2)]
   :ant-dir :up
   :stopped? false})

(defn within-bounds? [[row col] width height]
  (and (>= row 0) (>= col 0)
       (< row width) (< col height)))

(defn turn-right [dir]
  (case dir
    :up :right
    :right :down
    :down :left
    :left :up))

(defn turn-left [dir]
  (case dir
    :up :left
    :left :down
    :down :right
    :right :up))

(defn move [[x y] dir]
  (case dir
    :up    [x (dec y)]
    :right [(inc x) y]
    :down  [x (inc y)]
    :left  [(dec x) y]))

(defn iter-world [{:keys [width height grid
                          ant-pos
                          ant-dir
                          stopped?]
                   :as world}]
  (if (and (not stopped?) (within-bounds? ant-pos width height))
    (let [cell-color (get-in grid ant-pos)
          is-white? (= cell-color 0)
          next-dir (if is-white? (turn-right ant-dir) (turn-left ant-dir))
          next-pos (move ant-pos next-dir)
          next-grid (assoc-in grid ant-pos (if is-white? 1 0))]
      {:width width
       :height height
       :grid next-grid
       :ant-pos next-pos
       :ant-dir next-dir
       :stopped? false})
    (assoc world :stopped? true)))

And the view:

views.cljs

(ns ant.views
  (:require
   [re-frame.core :as re-frame]
   [ant.subs :as subs]
   [ant.events :as ev]
   [cljs.pprint :refer [pprint]]
   ))

(defn main-panel []
  (let [grid (re-frame/subscribe [::subs/grid])]
    (into [:table {:style {:border "1px solid red"}}]
          (for [row @grid]
            (into [:tr]
                  (for [cell row]
                    [:td {:width "5px" :height "5px"
                          :style {:backgroundColor (if (= cell 1) "black" "white")}}]))))))

(defn dispatch-timer-event
  []
  (let [now (js/Date.)]
    (re-frame/dispatch [::ev/timer now])))  ;; <-- dispatch used

;; Call the dispatching function every second.
;; `defonce` is like `def` but it ensures only one instance is ever
;; created in the face of figwheel hot-reloading of this file.
(defonce do-timer (js/setInterval dispatch-timer-event 50))

subscriptions:

subs.cljs

(ns ant.subs
  (:require
   [re-frame.core :as re-frame]))

(re-frame/reg-sub
 ::grid
 (fn [db]
   (:grid db)))

Events:

events.cljs

(ns ant.events
  (:require
   [re-frame.core :as re-frame]
   [ant.db :as db]
   [ant.world :as aw]
   ))

(re-frame/reg-event-db
 ::initialize-db
 (fn [_ _]
   db/default-db))

(re-frame/reg-event-db
 ::timer
 (fn [db _]
   (aw/iter-world db)))

DB:

db.cljs

(ns ant.db
  (:require [ant.world :as aw]))

(def default-db
  (aw/init-world 100 100))

This works. But is quite slow. Though the timer is supposed to trigger an event every 50ms, the drawing rate is not so high.

What can I do to improve the performance here? Is it possible without resorting to mutable data structures?

(Any general code improvement suggestions are also most welcome)

Full code is here:
https://github.com/nakiya/langton-ant

I don't have any performance related suggestions unfortunately. I do have a couple cleanup suggestions though.

The bounds checking function can be cleaned up a bit by making use of "comparison chaining" similar to what you'd use in Pytbon:

(defn within-bounds? [[row col] width height]
  (and (< -1 row width)
       (< -1 col height)))

In the iteration function, you have

{:width width
 :height height
 :grid next-grid
 :ant-pos next-pos
 :ant-dir next-dir
 :stopped? false}

This is less than ideal for a few reasons

You're reassociating entries that already exist in the map. This is unlikely to effect performance, but causes unneeded code bloat. It also creates a potential source of bugs if you add a new entry into the game state. Will you remember to update the map retuned by the function? I'd use assoc here instead.
You're associating false with the :stopped? key. Following the logic of the function though, it must already be falsey there.

I'd just write

(assoc world :grid next-grid
             :ant-pos next-pos
             :ant-dir next-dir}

In init-world, you're using (into []) to convert to vectors. This can be written slightly neater using vec.

:grid (vec (repeat height
                   (vec (repeat width 0))))

I also typically space this code out like I have above (I actually write this exact code fairly often).

This is entirely subjective, but instead of maintaining a :stopped? flag, I prefer to use a :running? one instead (the logic is flipped). I find it tends to lead to easier to read code.


Suggestion for a lot of chained Task I have a lot of Task continuation that depend on .Result of previous Task -> for example: I have a command that should:

Stop previous instance
Copy files elsewhere
Delete original files
Start instance again

So for example the command function:

public Task<CommandResult> Execute(ICommandArguments arguments)
{
    TaskCompletionSource<CommandResult> tcs = new TaskCompletionSource<CommandResult>();

    // 1. Stop
    var tskStop = this.stopCommand.Execute(arguments);
    tskStop.ContinueWith(stoppingTask =>
    {
        if (stoppingTask.Result == CommandResult.Success)
        {
           // 2. Copy files
           var tskCopy = this.copyCommand.Execute(arguments);
           tskCopy.ContinueWith(copyingTask =>
           {
               if (copyingTask.Result == CommandResult.Success)
               {
                  // 3. Delete
                  ... skip for brevity...
               }
               else
               {
                  // Could not 'delete'
                  tcs.SetResult(CommandResult.Fail);
                }
            }, TaskContinuationOptions.OnlyOnRanToCompletion | TaskContinuationOptions.AttachedToParent);

            tskCopy.ContinueWith(tskCopyBad =>
            {
               //Error in 'copy'
               tcs.SetException(tskCopyBad.Exception.Flatten());
             }, TaskContinuationOptions.NotOnRanToCompletion | TaskContinuationOptions.AttachedToParent);
         }
         else
         {
            // Could not 'stop' for some reason
            tcs.SetResult(stoppingTask.Result);
          }
      }, TaskContinuationOptions.OnlyOnRanToCompletion | TaskContinuationOptions.AttachedToParent);

      tskStop.ContinueWith(tskSTopBad =>
      {
           // Exception in 'stop'
           tcs.SetException(tskStopBad.Exception.Flatten());
      }, TaskContinuationOptions.NotOnRanToCompletion | TaskContinuationOptions.AttachedToParent);
}

and you see that only with 2 'commands' code blown to so many lines. Is there any design pattern, shortcut, or extension that I can add/use to shorten code lines? I depend on the .Result from the previous Task and its Status (to know whether an exception was thrown or not).

Note that we currently develop our app in .NET 4 so we are limited to Task, and unfortunately cannot use async/await.

P.S. All those copyCommand, deleteCommand is also have a public Task<CommandResult> Execute() function

You may not be able to get away from chaining a ton of ContinueWith invocations together. However, you might be able to create a builder class with methods that you pass parameters to so you don't have to call ContinueWith from calling code.

It looks like you might be able to remove a lot of duplication by extracting your anonymous methods into one or more stateless (probably static) methods that you can reuse by passing the method signature in, e.g.

private static void SetFlattenedException(Task stoppedTask, TaskContinuationOptions options)

You can also start going down the route of wrapping continuations in method calls with additional Action parameters to "inject" code to execute, but given that it tends to lead to a lot of "inside-out" code that is hard to follow, I'd use sparingly.


Find the corresponding closing parenthesis Problem:

  "Sometimes (when I nest them (my parentheticals) too much (like this
  (and this))) they get confusing."

Write a function that, given a sentence like the one above, along with the position of an opening parenthesis, finds the corresponding closing parenthesis. For the above input string and position 10 the algorithm should output 76.

After some trial and error I am able to come up with an algorithm.

function findClosingBracketMatchIndex(str, pos) {
  let openBracketCount = 0;
  let givenBracketPosition = -1;
  let index = -1;
  for (let i = 0; i < str.length; i++) {
    let char = str[i];
    if (char === '(') {
      openBracketCount++;
      if (i === pos) {
        givenBracketPosition = openBracketCount;
      }
    } else if (char === ')') {
      if (openBracketCount === givenBracketPosition) {
        index = i;
        break;
      }
      openBracketCount--;
    }
  }
  return index;
}

console.log(findClosingBracketMatchIndex('a (bc)', 2)); // 5
console.log(findClosingBracketMatchIndex('a (b ())', 2)); // 7
console.log(findClosingBracketMatchIndex('a (b ())', 5)); // 6
console.log(findClosingBracketMatchIndex('(a (b ()))', 0)); //9

Expert advice:

Apart from code review I would like to know if there are some tips to check the correctness of such algorithms, including off-by one error and conditions.

You might want to confirm that the character at the given position is indeed a (.

There is no reason to care about what appears before the given position.  You can start examining the string starting at pos + 1.  Once you have found the the position of the matching closing parenthesis, you can just return it right away; there is no need to ever set index.

function findClosingBracketMatchIndex(str, pos) {
  if (str[pos] != '(') {
    throw new Error("No '(' at index " + pos);
  }
  let depth = 1;
  for (let i = pos + 1; i < str.length; i++) {
    switch (str[i]) {
    case '(':
      depth++;
      break;
    case ')':
      if (--depth == 0) {
        return i;
      }
      break;
    }
  }
  return -1;    // No matching closing parenthesis
}

console.log(findClosingBracketMatchIndex('a (bc)', 2)); // 5
console.log(findClosingBracketMatchIndex('a (b ())', 2)); // 7
console.log(findClosingBracketMatchIndex('a (b ())', 5)); // 6
console.log(findClosingBracketMatchIndex('(a (b ()))', 0)); //9


Writing and saving scraped documents in several sheets in an Excel file I've written some code in Python for the purpose of scraping names and URLs from several links found in the left sided bar in a webpage and populate the data in several sheets (also giving each sheet a new name taking a customized portion from URL) in an Excel workbook so that things do not get messy and the data can be located separately. I tried to do the whole thing accurately and found it working great. It would be very nice if I get any better idea other than what I did here.

Here is what I tried with:

import requests
from lxml import html
from pyexcel_ods3 import save_data

web_link = "http://www.wiseowl.co.uk/videos/"
main_url = "http://www.wiseowl.co.uk"

def get_links(page):

    response = requests.Session().get(page)
    tree = html.fromstring(response.text)
    data = {}
    titles = tree.xpath("//ul[@class='woMenuList']//li[@class='woMenuItem']/a/@href")
    for title in titles:
        if "author" not in title and "year" not in title:
            get_docs(data, main_url + title)

def get_docs(data, url):

    response = requests.Session().get(url)
    tree = html.fromstring(response.text)
    for item in tree.xpath("//p[@class='woVideoListDefaultSeriesTitle']"):
        title = item.findtext('.//a')
        link = item.xpath('.//a/@href')[0]
        sheetname = url.split("videos/")[1].split("/")[1].split(".")[0]
        if sheetname in data:
            data[sheetname].append([title, str(link)])
        else:
            data[sheetname] = [[title, str(link)]]
    save_data("documents.ods", data)

if __name__ == '__main__':
    get_links(web_link)

Overall I don't think it's terrible. Suggestions:

Add a shebang to the top of your script so that it's executable in Posix-like environments: #!/usr/bin/env python
You create a session every time you call get_docs. Don't do this unless absolutely necessary; instead, pass the session created in get_links to be reused.
Put this through a PEP8 linter, such as that built into PyCharm, for minor formatting suggestions, such as there being two newlines before every top-level function.
Your sheetname (which should be called sheet_name by Python standard) smells like it should be a regex instead of a series of splits.
Condense your if ... in ... else to a setdefault(); something like:

data.setdefault(sheet_name, []).append([title, str(link)])


Getting a decryptor object I need to lock on my AesManaged instance _Aes to call CreateDecryptor() however the CreateDecryptor() method is not thread safe and this function could be called by multiple threads. Is this the correct way to create a disposable object while needing to use a lock on the parent creating the object?

byte[] encryptedTextBlob = GetEncryptedTextBlob();

ICryptoTransform decryptor = null;
lock(_Aes)
{
    decryptor = _Aes.CreateDecryptor();
}

using (decryptor)
using (var encryptedStream = new MemoryStream(encryptedTextBlob))
using (var decrypedStream = new CryptoStream(encryptedStream, decryptor, CryptoStreamMode.Read))
using (var textReader = new StreamReader(decrypedStream))
{
    return textReader.ReadToEnd();
}

I would say you're not using AES correctly - don't hold on to an instance of it in your class (guessing by the underscore in the name) but rather create it as needed, while holding AES's parameters (the key and IV) in your class:

using (var aes = new AesManaged() { IV = _IV, Key = _Key })
using (var decryptor = aes.CreateDecryptor())
using (var encryptedStream = new MemoryStream(encryptedTextBlob))
using (var decrypedStream = new CryptoStream(encryptedStream, decryptor, CryptoStreamMode.Read))
using (var textReader = new StreamReader(decrypedStream))
{
    return textReader.ReadToEnd();
}


Basic abstract graph implementation in Java I am trying to write a graph implementation in Java. I would appreciate it if someone can please review the code for me and give constructive feedback.

import java.util.Arrays;
import java.util.HashMap;
import java.util.LinkedList;

public class Graph {
    private int V;  // Number of vertices

    private int E;  // Number of edges

    private HashMap<Integer, LinkedList<Integer>> adj;  // adjacency list

    Graph() {
        adj = new HashMap<>();
    }

    /**
     * Returns the number of vertices
     * @return Number of vertices in the graph
     */
    public int vertices() {
        return this.V;
    }

    /**
     * Number of edges in the graph
     * @return Returns the number of edges in the graph
     */
    public int edges() {
        return this.E;
    }

    public void addEdge(int v, int w) {
        if(adj.containsKey(v)) {
            adj.get(v).add(w);
            ++E;
        } else if(adj.containsKey(w)) {
            adj.get(w).add(v);
            ++E;
        } else {
            LinkedList<Integer> llOne = new LinkedList<>();
            LinkedList<Integer> llTwo = new LinkedList<>();
            llTwo.add(v);
            llOne.add(w);
            adj.put(v, llOne);
            adj.put(w, llTwo);
            ++V; ++E;
        }
    }

    @Override
    public String toString() {
        StringBuilder returnString = new StringBuilder();
        for (int key : adj.keySet()) {
            returnString.append(key).append(": ");
            LinkedList<Integer> nodesList = adj.get(key);
            returnString.append(Arrays.toString(nodesList.toArray()));
        }
        return returnString.toString();
    }
}

Thinking about naming...

When I see a single capital letter, it makes me think of generic type.  I find your usage misleading.
Single letter variable names can be OK when it's obvious what they are, such as indexers in loops, I would suggest avoiding them for anything with larger scope, particularly at the field level.
int V;  // Number of vertices, why not just call it numberOfVertices?  You don't need the comment and the rest of the code is that little bit easier to interpret?
edges(): Typically in Java I'd expect this type of method to be called getEdges, it's really just returning the field.  In fact, when I first saw it, I thought it would be returning some collection of edges, but it doesn't it just returns a count.  Consider renaming it to match what it returns.
LinkedList<Integer> llOne = new LinkedList<>();.  Generally, avoid embedding the type information (ll) in the variable name.  Think about what the list is representing and call it that, rather than 'one' and 'two'.


Recreating merge-with Just working through the questions on 4Clojure as I learn the language. This is my solution for #69. Would appreciate feedback as this one gave me a little trouble.

(fn merge-with2 [f m & ms]
   (let [update-or-add (fn update-or-add [m1 k f arg]
                          (let [x (get m1 k)]
                            (if (nil? x)
                              (conj m1 {k arg})
                              (update-in m1 [k] f arg))))]
   (loop [m1 m m2 (first ms) ms ms
          k (keys m2)]
     (if-not (map? m2)
       m1
       (recur (reduce #(update-or-add %1 %2 f (get m2 %2)) m1 k)
              (first (next ms))
              (next ms)
              (keys (first (next ms))))))))

I suspect that it would be possible to replace the loop/recur with a nested reduce, but I'm not quite sure how to implement it.

Here are my thoughts:

letfn instead of let

You could use letfn instead of let, which would be a little more concise:

(letfn [(update-or-add [m1 k f arg]
          ...

conj vs. update-in

conj and update-in are very similar functions in that they both take a map and "update" the value for a key that may or may not exist. Because of this, they are sometimes exchangeable. update-in is fine here, but I think using conj for both scenarios could make it easier to see at a glance what your function is doing. (See the next example.)

if-let

The (let [x (get m1 k)] (if (nil? x) ... part of your code is a good example of where if-let comes in handy.

(if-let [x (m1 k)]
  (conj m1 {k (f x arg)})
  (conj m1 {k arg}))

Which you could even simplify further by moving the if-let inside of a single conj function call:

(conj m1 {k (if-let [x (m1 k)] (f x arg) arg)})

As an aside, I tend to prefer assoc, although it's really just as good as conj:

(assoc m1 k (if-let [x (m1 k)] (f x arg) arg))

Reducers work best as functions taking 2 arguments

The first thing I noticed about your loop is that its arguments can be simplified:

(loop [m1 m, ms ms]
  ...

Essentially what you're doing is starting with m1 and reducing update-or-add over each key-value pair in each map in ms. The tricky thing here is that, as you pointed out, this is a nested reducing operation, in that each map in ms can have multiple key-value entries. You have the right general idea in that you have a reduce within a loop, but a better/simpler way to do this would be to make your update-or-add function take the two arguments that a reducing function expects -- an accumulator and the next item in the collection being reduced over. Once you have update-or-add working that way, you can do the same thing with your loop and at that point you'll be able to represent your solution as a simple nested reduce.

Let's start with update-or-add. This function takes a working "map in progress" and a key-value pair and either adds it to the map if that key isn't already there, or it updates the value to be the result of calling f with the value already there and the new value as arguments. Note that we don't need to make f an argument because it's already available within the context of the entire function merge-with2. You can also use destructuring to easily access the key and the value of the key-value pair.

(letfn [(update-or-add [m1 [k v]]
          (assoc m1 k (if-let [x (m1 k)]
                        (f x v)
                        v)))]
  ...

Now you can use update-or-add "as-is" within a reduce.

You can do something similar with your main loop. To simplify things, let's make it its own function, and call it something like merge-with-one. This function will take one of the maps supplied to merge-with2 and reduce update-or-add over its key-value pairs. Defining this function is actually super easy, because it's just a call to reduce. So, here is my suggested refactoring of your code:

(fn merge-with2 [f m & ms]
  (letfn [(update-or-add [m1 [k v]]
            (assoc m1 k (if-let [x (m1 k)]
                          (f x v)
                          v)))
          (merge-with-one [m1 m2]
            (reduce update-or-add m1 m2))]
    (reduce merge-with-one m ms)))

It turns out that this is actually pretty close to the actual implementation of merge-with in clojure.core.


Simple Open Dynamics Engine (ODE) I created the following code to solve a simple ODE. It is clear enough? Is it efficient code? I really want to read opinions because I want to make all my exercise solutions the same way (same structure of the program), so I need to make sure that this code is really clear and efficient.

#include <iostream>
#include <fstream>
#include <string>
#include <vector>

class step
{
public:
    step()
        : time(0.), velocity(0.) {}
    step(const double& t, const double& v)
        : time(t), velocity(v) {}

    double time;
    double velocity;
};

void calculate(
    std::vector<step>& steps,
    double g,
    double t_max,
    double dt)
{
    int iterations(t_max / dt + 1);

    steps.resize(iterations);

    for(int i(0); i <= iterations; i++)
    {
        steps[i+1].time = steps[i].time + dt;
        steps[i+1].velocity = steps[i].velocity - g * dt;
    }
}

void save(
    const std::vector<step>& steps,
    const std::string& filename)
{
    std::ofstream out(filename);

    for(int i(0); i < steps.size(); i++)
    {
        out << steps[i].time << ' ' << steps[i].velocity << std::endl;
    }
}

int main()
{
    double g(9.81);
    double t_max(10.);
    double dt(0.01);

    step initial_conditions(0.,0.);
    std::vector< step > steps({initial_conditions});

    calculate(steps, g, t_max, dt);
    save(steps, "ex_1.1.dat");

    return 0;
}

Although I'm not a C++ programmer I once held a course in computations with Matlab for physicists. So I can only give hints and thoughts on your code but I won't answer your question on efficiency.

As you mentioned that this code will be the basic of following exercises, I'm missing all sorts of documentation. If you are planning to teach someone (solving) ODEs with this code, you should at least refer to a physical or mathematical problem, which will be solved by your code and you should comment every function and what it does. However, if you want to teach writing clean code, you should separate your definitions from the implementation.
I'm the opinion that variables should have meaningful names even if you are referring to the standard variable names in physics (like g for gravity). This will ease the burden of understanding the code if you are not familiar with standard variable names.

Lastly, there is the call to the resize method of the vector. As I am no C++ programmer, so I can't say for sure, but I feel uncomfortable with this function. I searched in a C++ reference and found out about the method reserve which might be more appropriate in a safer way.


Arabic numbers to Roman numerals conversion The following converts Arabic numbers to Roman numerals.

I decided to factor each digit into its place value, then use a mapping to convert it to its Roman equivalent.

For example, 1997 is factored to:

\$(1 \times 10^3) + (9 \times 10^2) + (9 \times 10^1) + (7 \times 10^0) = 1000 + 900 + 90 + 7\$

The Roman numerals for those numbers are then retrieved from the mapping.

class Integer
  def to_roman
    mappings = {
         1 => 'I',
         2 => 'II',
         3 => 'III',
         4 => 'IV',
         5 => 'V',
         6 => 'VI',
         7 => 'VII',
         8 => 'VIII',
         9 => 'IX',
        10 => 'X',
        20 => 'XX',
        30 => 'XXX',
        40 => 'XL',
        50 => 'L',
        60 => 'LX',
        70 => 'LXX',
        80 => 'LXXX',
        90 => 'XC',
       100 => 'C',
       200 => 'CC',
       300 => 'CCC',
       400 => 'CD',
       500 => 'D',
       600 => 'DC',
       700 => 'DCC',
       800 => 'DCCC',
       900 => 'CM',
      1000 => 'M',
      2000 => 'MM',
      3000 => 'MMM'
    }

    roman = []
    to_s.chars.reverse.each.with_index(0) do |digit, i|
      roman.unshift(mappings[digit.to_i * 10**i])
    end

    roman.join
  end
end

I like using exponents to factor each digit into its value place. I don't like having to define a mapping for each number from 1 to 3 and 6 to 8, and repeat that for the 10s, 100s, and 1000s digits.

Similar to dankohn's comment, there are really only a handful of operations going on, just with different character sets.

Here is my quick implementation.  It only handles the base 10 digits, and dynamically swaps out character sets depending on the power of the number.

def to_roman(digits)
    numeral_sets = {  1=>['I', 'V', 'X'],
                     10=>['X', 'L', 'C'],
                    100=>['C', 'D', 'M'],
                   1000=>['M', "\u2181", "\u2182"] }

    str = ''
    digits.reverse.each.with_index do |d, ix|
        set = numeral_sets[10**ix]
        case d
            when 0 then roman = ''
            when 1..3 then roman = set[0] * d
            when 4 then roman = "%s%s" % [set[0], set[1]]
            when 5 then roman = set[1]
            when 6..8 then roman = "%s%s" % [set[1], set[0] * (d-5)]
            when 9 then roman = "%s%s" % [set[0], set[2]]
        end
        str = str.insert(0, roman)
    end
    str
end

And a few tests:

puts to_roman [1,9,9,7] # expected MCMXCVII
puts to_roman [1,9,0,7] # expected MCMVII
puts to_roman [5,6,7,8] # expected ↁDCLXXVIII


NHibernate (UoW) + cCommand pattern I'm building an ASP.NET MVC app and would like some feedback for my way to query/execute statements to the ORM.

I'm using Fluent NHibernate, and have mixed the UoW provided by NHibernate with a Command pattern.

ICommand.cs (Core layer)

using NHibernate;

namespace MyDemon.Core.Commands
{
    public interface ICommand<out TResult>
    {
        TResult Execute(ISession session);
    }
}

ISessionExtensions.cs (Core layer)

using NHibernate;

namespace MyDemon.Core.Commands
{
    public static class ISessionExtensions
    {
        public static TResult Execute<TResult>(this ISession session, ICommand<TResult> unitOfWork)
        {
            return unitOfWork.Execute(session);
        }
    }
}

GetUserById.cs (Core layer)

using NHibernate;

namespace MyDemon.Core.Commands.User
{
    using Entities;

    public class GetUserById : ICommand<User>
    {
        public int UserId { get; set; }

        public GetUserById(int userId)
        {
            UserId = userId;
        }

        #region Implementation of IUnitOfWork<out User>

        public User Execute(ISession session)
        {
            return session.Get<User>(UserId);
        }

        #endregion
    }
}

AccountController.cs (Web layer)

[AjaxOnly]
[Authorize]
public ActionResult Details(int id)
{
    User userToGet = _session.Execute(new GetUserById(id));

    if (userToGet == null)
    {
        return PartialView("Partials/UserNotFound");
    }

    DetailsUserViewModel userToViewModel = Mapper.Map<User, DetailsUserViewModel>(userToGet);

    return PartialView("Partials/Details", userToViewModel);
}

What do you think? A clever design, or just another "too-much code" approach?

This seems to be a lot of code to say the same as this:

[AjaxOnly]
[Authorize]
public ActionResult Details(int id)
{
    User userToGet = _session.Get<User>(id);

    if (userToGet == null)
    {
        return PartialView("Partials/UserNotFound");
    }

    DetailsUserViewModel userToViewModel = Mapper.Map<User, DetailsUserViewModel>(userToGet);

    return PartialView("Partials/Details", userToViewModel);
}

What problem are you trying to solve here?

(or am I missing something?)


Process bulk update actions It's using four sections of code block, I just want it to be reduced to one contiguous set that does either a or b with the two sections not a copy/pasted approach as done here:

  public function process_bulk_action() {

    //Detect when a bulk action is being triggered...
     if ( 'delete' === $this->current_action()){

      // In our file that handles the request, verify the nonce.
      $nonce = esc_attr( $_REQUEST['_wpnonce'] );

      if ( ! wp_verify_nonce( $nonce, 'sp_delete_customer' ) ) {
        die( 'Go get a life script kiddies' );
      }
      else {
        self::delete_customer( absint( $_GET['customer'] ) );
        wp_redirect( esc_url( add_query_arg() ) );
        exit;
      }

}
    //Detect when a Reset is being triggered...
     if ( 'reset' === $this->current_action()){

      // In our file that handles the request, verify the nonce.
      $nonce = esc_attr( $_REQUEST['_wpnonce'] );

      if ( ! wp_verify_nonce( $nonce, 'sp_reset_payouts' ) ) {
        die( 'Go get a life script kiddies' );
      }
      else {
        self::reset_record( absint ( $_GET['customer']));
        wp_redirect( esc_url( add_query_arg() ) );
        exit;
      }
}

    // // If the delete bulk action is triggered
    if ( ( isset( $_POST['action'] ) && $_POST['action'] == 'bulk-delete' )
         || ( isset( $_POST['action2'] ) && $_POST['action2'] == 'bulk-delete' )
    ) {

      $delete_ids = esc_sql( $_POST['my_CheckBoxes'] );

      // loop over the array of record IDs and delete them
      foreach ( $delete_ids as $id ) {
        self::delete_customer( $id );
      }
      wp_redirect( esc_url( add_query_arg() ) );
      exit;
    }

    //If the delete bulk action is triggered
    if ( ( isset( $_POST['action'] ) && $_POST['action'] == 'bulk-reset' )
         || ( isset( $_POST['action2'] ) && $_POST['action2'] == 'bulk-reset' )
    ) {

      $reset_ids = esc_sql( $_POST['my_CheckBoxes'] );

      // loop over the array of record IDs and delete them
      foreach ( $reset_ids as $id ) {
        self::reset_record( $id );

      }

      wp_redirect( esc_url( add_query_arg() ) );
      exit;
    }

  }

}

I have found myself doing code duplication from time to time, and often will find them in the code. It screams at you "fix me!".

I would recommend a book, Refactoring by Martin Fowler on Amazon. It is an excellent resource and has step to step guides to refactoring code as the example you provided here.

Using Martin Fowlers techniques, I refactored the second half of your code step by step. The first thing was to simplify the conditionals:

if ( ( isset( $_POST['action'] ) && $_POST['action'] == 'bulk-reset' )
     || ( isset( $_POST['action2'] ) && $_POST['action2'] == 'bulk-reset' )
) {

Became a query instead:

if($this->isBulkReset()) {
...

protected function isBulkReset() {
    return $this->isBulkAction('bulk-reset');
}

protected function isBulkDelete() {
    return $this->isBulkAction('bulk-delete');
}

protected function isBulkAction($action) {
    return (isset($_POST['action']) && $_POST['action'] == $action)
    || (isset($_POST['action2']) && $_POST['action2'] == $action);
}

Next I looked at some similarities in the functions, like these two:

$delete_ids = esc_sql( $_POST['my_CheckBoxes'] );
$reset_ids = esc_sql( $_POST['my_CheckBoxes'] );

I changed them in both functions:

$recordIds = $this->getRecordIds();

foreach ($recordIds as $id) {
...
protected function getRecordIds() {
    return esc_sql($_POST['my_CheckBoxes']);
}

Next I joined the two functions:

 if($this->isBulkDelete() || $this->isBulkReset()) {
        $recordIds = $this->getRecordIds();

        foreach ($recordIds as $id) {
            if($this->isBulkDelete()) {
                self::delete_customer($id);
            } else {
                self::reset_record($id);
            }
        }

        wp_redirect(esc_url(add_query_arg()));
        exit;
    }

Then I noticed that $recordIds is a totally unnecessary temporary variable, so I removed it and just did the query directly in the foreach:

if($this->isBulkDelete() || $this->isBulkReset()) {
    foreach ($this->getRecordIds() as $id) {
        if($this->isBulkDelete()) {
            self::delete_customer($id);
        } else {
            self::reset_record($id);
        }
    }
    wp_redirect(esc_url(add_query_arg()));
    exit;
}

This is how it typically progresses, small steps. But in the end, you will have more separation and cleaner code. The big secret here is that as you progress, you will see new ways in organising your code that you could not see before.

Heads up! This code is untested, simply example refactoring techniques.


Effective fetching data algorithm I'm fetching some data by using bs4, and want to know why access speed gets slower as my program progresses.

I'm using thread, and I know threading isn't so fast because of GIL.

My problem is that my program gets slower gradually.
When the number of word is over 300,it become much slower than that of word is about 100 words.

GIL causes this?

(Multiprocessing can solve GIL problem,however,I want to find other causes,if any.)

What is the better web-scraping code to download at once?

Thanks.

import os,gc,queue
import requests, bs4
from threading import Thread, BoundedSemaphore
class add_line:
    def __init__(self,semaphore,q):
        self.q = q # queue
        self.semaphore = semaphore

    def put_queue(self,li): #insert
        self.q.put(li)

    def fetch_word(self,w_list):
        fetched_text = ""# targeted text
        path = "https://ejje.weblio.jp/content/"+ w_list[1]
        res = requests.get(path)
        res.raise_for_status()
        no_starch_soup = bs4.BeautifulSoup(res.text)
        fetched_text = no_starch_soup.select('.conjugateRowR > table > tr > td')
        if len(fetched_text)== 0: #if get nothing
            fetched_text = no_starch_soup.select('.conjugateRowR > a')
            if len(fetched_text) ==0:# if get nothing too
                self.put_queue(w_list)# return original conjurate
            else :
                li_ = list(map(lambda x:x.contents[0], fetched_text))
                li_.insert(0,w_list[0])
                self.put_queue(li_)
        else:
            li_=list(map(lambda x:x.contents[0], fetched_text))
            li_.insert(0,w_list[0])
            self.put_queue(li_)
        gc.collect()
        self.semaphore.release()
if __name__ == "__main__":
    # creating thread
    maxconnections = 5
    path = "./target_index1900.txt"
    """
    the part of this text data is below(the number of words is over 1000)

    510 abandon
    510 abandonment
    1807 abide
    1318 abolish
    1318 abolition
    1167 abortion
    1167 abortive
    1096 abound
    1683 abrupt
    1683 abruptly
    1199 absolute
    1199 absolutely
    507 absorb
    507 absorption
    899 abstract
    899 abstraction

    """
    #set semaphore
    pool_sema = BoundedSemaphore(value=maxconnections)
    q = queue.Queue()# put word  in queue
    al = add_line(pool_sema,q)
    path_w = "./threading_test_result.txt"
    with open(path) as f:
        while True:
            lines = f.readline()
            if not lines:
                break
            pool_sema.acquire()
            Thread(target=al.fetch_word, args=(lines.split(" "),)).start()
    with open(path_w, mode='a') as f:
        while not q.empty():
            for x in q.get():
                f.write(x)
                f.write(" ")
            f.write("\n")
        f.close()
        gc.collect()
    print("Done!")

Whitespace formatting

Apply a linter that will give you PEP8 suggestions. Among other things, it will suggest the following:

import os,gc,queue

There should be spaces after those commas. It'll also suggest that there be one line per import.

class add_line:

should be

class AddLine:

and have a couple of newlines before it.

else :

shouldn't have a space before the colon.

f-strings

"https://ejje.weblio.jp/content/"+ w_list[1]

can be

f'https://ejje.weblio.jp/content/{w_list[1]}'

Falsiness

if len(fetched_text)== 0:

can be

if not fetched_text:

Don't repeat yourself

            li_ = list(map(lambda x:x.contents[0], fetched_text))
            li_.insert(0,w_list[0])
            self.put_queue(li_)

appears twice. Put it in a function or restructure your if blocks.

File iteration

with open(path) as f:
    while True:
        lines = f.readline()
        if not lines:
            break

First of all, your variable name is confusing. This isn't lines, it's line.

Also: just change your iteration to

with open(path) as f:
    for line in f:

As for this:

with open(path_w, mode='a') as f:
    # ...
    f.close()

Get rid of the close; the file is already closed at the end of the with.

main

You do check __name__, but you don't give all of that code function scope. You should move it into a main function. Otherwise, every one of those variables will be visible from other functions.


Group by two lists in one map using lambda I've got two collections: collection of rules, and collection of keys that can handle specific rules. Key have method boolean canHandle(Rule) to check whether specific key can handle specific rule or not.

I need to gather these two collections into single map Map<Key, List<Rule>> bycanHandle` predicate method. I've done this so far:

Map<Key, List<Rule>> keyToRulesMapping = rules.parallelStream()
        .collect(Collectors.groupingBy(rule -> keys.stream()
                .filter(key -> key.canHandle(rule))
                .findFirst().get()));

The thing I don't like in this approach is number of traverse in key list to get the right key. In best case I have only one key that handles all rules (\$O(N)\$ where N is the number or rules). In worst case I have each key associated with it's own unique rule (becomes \$O(N^2)\$). I'm asking is there a better way to match these two lists or my approach is good enough?

This does not look horribly wrong to me. There are some issues though:

groupingBy I presume is a static import from Collectors? You should make these things more obvious. In fact, I dislike the static imports for most things.....
you don't show much about ruleKeys.

The style otherwise looks good.

The concept you are solving, given the information you have given, is being solved in what amounts to a reasonably efficient manner, all things considered. It is an \$O(nm)\$ solution, if you double either the number of rules, or number of keys, your solution will take twice as long, and if you double both, the solution will take 4 times as long.

If you do this just once and then reuse the Mapping, then it should be fine. If you have fewer than say 100 members in each of the rules/keys, then you should be fine.

If you have more data than that then you should consider building up the data directly in to the map at the same time as you initially populate the rules/keys.

So, you are right to not like the solution because it scales badly, but, if your data is small, be pragmatic and not worry about it too much in this instance.


Limiting the number of text fields created It's a script that I've ripped off elsewhere, and have modified it via help from Stack Overflow on a couple of other questions, so that it tops out at 10 fields.

My knowledge of JS/jQuery is barely there.  I'm terrified that I did it badly or wrongly. Could you please let me know if this is the correct way of doing it, or if there's another way that I should have done it differently?

I need to make sure that the max number of text fields created is limited to 10 at most.

Here's a demo and a JSFiddle.

jQuery(document).ready(function($) {
    var max_fields = 10;
    var x = 1;

    $('.my-form .add-box').click(function() {
        var n = $('.text-box').length + 1;

        if(x < max_fields) {
            var box_html = $('
Name ' + n + ' <input type="text" name="on' + (n - 1) + '" value="" id="box' + n + '" /> Remove
');
            box_html.hide();
            $('.my-form p.text-box:last').after(box_html);
            box_html.fadeIn('slow');
            x++;
        }
    });

    $('.my-form').on('click', '.remove-box', function() {
        $(this).parent().css('background-color', '#FF6C6C').fadeOut("slow", function() {
            $(this).remove();
            $('.text-box').each(function(index) {
                var label = $(this).children('label');
                label.attr('for', 'box' + (index + 1));
                label.children('.box-number').text(index + 1);
                $(this).children('input').attr({
                    name: 'on' + index,
                    id: 'box' + (index + 1)
                });
            });
        });
        return false;
    });
});

        Name 1
        <input type="text" name="on0" value="" id="box1" />
        Add More Names

Bored, so I made a few improvements as I mentioned (in the comments that have vanished when this was moved to Code Review):

Use the jQuery shortcut DOM-ready handler

A handy short-cut for your DOM ready, with locally scoped $ is just jQuery(function($){...});

Store templates in HTML, not JavaScript strings:

and access like this:

var box_html = $('#template').html().replace(/{n}/g, n).replace(/{-1}/g, n-1);
var $box = $(box_html);

Count the current items when you need them

var n = $('.text-box').length + 1;
if (n <= max_fields) {

Restrict to the current "group" of inputs:

var $form = $(this).closest('.my-form');
$('p.text-box:last', $form).after($box);

This also means it can now handle multiple sets of inputs, each with different name/id prefixes.

Combine operations when you can:

$box.hide().fadeIn('slow');

Putting it all together:

JSFiddle: http://jsfiddle.net/TrueBlueAussie/q2emvzdd/4/

Notes:

A prefix was needs to be supplied by the parent form (e.g. a data attribute) so that each set of inputs has unique IDs and names. This is not applied to the first entry yet, but I have to go now, so something for you to finish :)


Designing a dynamic array of Memory Streams I'm working on a feature for work that requires me to combine a set number of memory streams based on specific conditions.

Previous Code

Before based on documentation/examples online. In order to create a PDF to print, you have to create a Memory Stream and save that document into a Memory Stream. Here lies my old code.

    PdfDocument testDocument1 = new PdfDocument(testParametersToDesignUniquePDF1);
    PdfDocument testDocument2 = new PdfDocument(testParametersToDesignUniquePDF2);
    PdfDocument testDocument3 = new PdfDocument(testParametersToDesignUniquePDF3);
    MemoryStream testDocument1Stream = new MemoryStream();
    MemoryStream testDocument2Stream = new MemoryStream();
    MemoryStream testDocument3Stream = new MemoryStream();
    testDocument1.Save(testDocument1Stream);
    testDocument2.Save(testDocument2Stream);
    testDocument3.Save(testDocument3Stream);

    Stream[] source = { testDocument1, testDocument2};

    if (testCondition)
    {
        source = { testDocument1Stream, testDocument2Stream, testDocument3Stream}
    }

As you can see its only a matter of time before my code gets chunkier as for every PdfDocument, I create I have to create a new memory stream. There's also a limitation of having a static array that I have to keep initializing a new array as more condition applies. I fixed this code with the following below.

New Code

My Code Refactor is to create a dynamic List that I can add x number of streams based on x conditions. Then convert back to a static size array after I'm done. It saves a lot of extra code creating memory streams and saving my documents on it.

    List<MemoryStream> memoryStreams = new List<MemoryStream>();

    memoryStreams.Add(ConvertToMemoryStream(new PdfDocument(testParametersToDesignUniquePDF1)));
    memoryStreams.Add(ConvertToMemoryStream(new PdfDocument(testParametersToDesignUniquePDF2)));

    if (testCondition)
    {
       memoryStreams.Add(ConvertToMemoryStream(new PdfDocument(testParametersToDesignUniquePDF3)));
    }

    //Convert dynamic linked list back to an array of steam.
    Stream[] source = memoryStreams.ToArray();

Method to convert PdfDocument to a stream.

 private MemoryStream ConvertToMemoryStream(PdfDocument document)
    {
        MemoryStream stream = new MemoryStream();

        document.Save(stream);

        return stream;
    }

I tested this code to have the same performance and output as the original code. However, the code has more flexibility adding more conditions and PDF's. It is also a lot more readable than the previous code. However, I am worried about a few things.

Deallocation - I might not be taking care of disposing my streams if I manipulate it this way.
Time of Deallocation - I'm worried that since I don't have a reference to my streams that it might get disposed on its own before actually using it when I Print the PDF's showing a blank PDF.

You might need to create a class handler that would handle the PdfDocument and MemoryStream, which will make things easier to handle and disposed as well. Then, you can just adjust the handler to your application requirements. here is an example :

public class PdfDocumentManager
{
    // Use Dictionary to keep both PdfDocument and MemoryStream paired.
    // Also, this means, you'll only save unique PdfDocuments and work on them.
    // if you want to save a PdfDocument multiple times (say different copy of the original copy)
    // use List<KeyValuePair<PdfDocument, MemoryStream>> instead.
    private readonly Dictionary<PdfDocument, MemoryStream> _storage = new Dictionary<PdfDocument, MemoryStream>();

    public PdfDocumentManager() { }

    public void Add(PdfDocument document)
    {
        // don't forget to validate

        _storage.Add(document, ToMemoryStream(document));
    }

    public void AddRange(IEnumerable<PdfDocument> documents)
    {
        foreach(var document in documents)
        {
            // validate each document before adding it.
            Add(document);
        }
    }

    public void Remove(PdfDocument document)
    {
        // code to remove the saved stream.
        // choose MemoryStream or PdfDocument or Both if you want.
    }

    private MemoryStream ToMemoryStream(PdfDocument document)
    {
        using (MemoryStream stream = new MemoryStream())
        {
            document.Save(stream);
            return stream;
        }
    }

    private PdfDocument ToPDFDocument(MemoryStream stream)
    {
        // code to convert back to memory stream.
        // just search the values to return its key.
        // if there is no match, then create a new PdfDocument along with MemoryStream, then return it back.
    }

    public IEnumerable<PdfDocument> GetAvailableDocuments()
    {
        return _storage.Keys;
    }

    public IEnumerable<MemoryStream> GetAvailableStreams()
    {
        return _storage.Values;
    }

}

simple add usage :

// initiate
var pdfManager = new PdfDocumentManager();

// add single document
pdfManager.Add(new PdfDocument(testParametersToDesignUniquePDF1));
pdfManager.Add(new PdfDocument(testParametersToDesignUniquePDF2));

// add multiple PdfDocument
var pdfDocumentList = new List<PdfDocument>
{
    new PdfDocument(testParametersToDesignUniquePDF1),
    new PdfDocument(testParametersToDesignUniquePDF2)
}

pdfManager.AddRange(pdfDocumentList);

The methods in this class are just for demonstration purpose only. You can bind the idea to your actual needs. With this handler, you can even make it a singleton class or make _storage static to have only one storage for the duration of the application ..etc. You can also move some related conditions inside the class to validate the documents (for instance, if you check if that document existed or not ..etc). This would give you also an easier way to handle the disposable objects.


Enforcing correct input/output of integers The problem:

Some types, like cstdint aliases or, at least purely theoretically, even std::size_t, may be (but don’t have to be) bound to char, signed char or unsigned char. In practice, std::int_least8_t seems to be often bound to signed char and std::uint_least8_t – to unsigned char. This seems odd, since semantically, all those types are supposed to represent integers rather than characters.
In case of variables of a character type, the >> operator writes the character code of the first non-whitespace character of the input stream into the variable, while the << operator outputs a character of the character code equal to the value of the variable.
Therefore it may happen that if we, for example, declare a variable as int_least8_t and use the input operator or the output operator on it, then the results will be far from expected. This seems to be even more awkward since whether such aliases are bound to character types or not depends only on the implementation and is a priori unknown.

An example of the above problems: http://ideone.com/JgyFhR A proof that purely theoretically, this can even be the case for std::size_t: https://stackoverflow.com/questions/32915434

I’ve attempted to write a generic solution to all those problems:

#ifndef INTIO_H
#define INTIO_H

#include <istream>
#include <ostream>
#include <type_traits>
#include <limits>

template
<class T, typename = typename std::enable_if<std::is_integral<T>::value>::type>
class integer_IO
{
  T &val;

public:

  integer_IO(T &arg) : val(arg) {}
  integer_IO(T &&arg) : val(arg) {}

  friend std::istream &operator>> (std::istream &is, integer_IO<T> &&i)
  {
    using TT = decltype(+i.val);
    TT hlp;
    is >> hlp;
    TT constexpr minval = static_cast<TT>(std::numeric_limits<T>::min());
    TT constexpr maxval = static_cast<TT>(std::numeric_limits<T>::max());
    i.val = static_cast<T>(hlp > maxval ? maxval : hlp < minval ? minval : hlp);
    if(hlp > maxval || hlp < minval)
      is.setstate(std::ios::failbit);
    return is;
  }

  friend std::ostream &operator<< (std::ostream &os, integer_IO<T> const &&i)
  {
    os << +i.val;
    return os;
  }
};

template
<class T, typename = typename std::enable_if<std::is_integral<T>::value>::type>
integer_IO<T> intIO(T &arg)
{
  return integer_IO<T>(arg);
}

template
<class T, typename = typename std::enable_if<std::is_integral<T>::value>::type>
integer_IO<T> intIO(T &&arg)
{
  return integer_IO<T>(arg);
}

#endif

The usage of this wrapper is simple:

#include <iostream>
#include <cstdint>
#include "intIO.hpp"
using namespace std;

int twoplustwo()
{
  return 2+2;
}

int main()
{
  int_least8_t i;
  cin >> intIO(i);
  cout << intIO(i) << '\n';
  cout << intIO(twoplustwo()) << '\n';
}

What is your opinion of my code?

Avoid name collisions

#ifndef INTIO_H
#define INTIO_H

You should ideally prefix the include guard by something that is unlikely to cause a collision with any one using your library. As it stands now it is at risk of collision if some one else unluckily choose to use INTIO_H as their include guard.

Hide implementation details

To me it looks like the integer_IO class is not directly intended to be used by the user. In that case you should wrap it in an internal namespace so that the primary namespace that the user sees isn't cluttered with internal functions.

Naming

I think that the name integer_IO is kind of mixing two different case standards. To my knowledge there are kind of two major standards emerging, one is to use CameCase in which you would name your class IntegerIO and the other one is to use inderscore instead of space and all lower case so you would have integer_io. If you're going to allow upper case in names, then I would at least suggest that you use leading upper case like so Integer_IO. Any of the above are easier on my eyes than the current name.

Also I think that the intIO functions would be better named asInt:

std::cin>>asInt(x);

Universal references

This here is a universal reference, the parameter arg will inherit whatever qualifiers the constructor was called with.

  integer_IO(T &&arg) : val(arg) {}

If the constructor was called with a temporary like so auto iio = integer_IO(x + y) where x and y are int arguments, then arg would deduce to int&&. Which is an r-value reference, you are then storing the pointer to this temporary into val which refers to... you guessed it something that might be dangling as soon as the constructor returns.

The other constructor:

  integer_IO(T &arg) : val(arg) {}

will not bind to temporaries because it must be able to take the address of the argument. Other than that it will bind to reference to const because T will be deduced to const T. So this constructor covers all legal use cases that the one with universal reference does, so you can remove either one. I suggest you nuke the universal one because it is slightly harder to understand.

Stream operators

Your two stream operators:

friend std::istream &operator>> (std::istream &is, integer_IO<T> &&i)
friend std::ostream &operator<< (std::ostream &os, integer_IO<T> const &&i)

are using r-value references (NOTE: They are not universal references). Note that const r-value reference doesn't make any sense.

Anyway, I assume that this is because you only want to allow usage on the form:

std::cout<<intIO(x);

and prohibit some one from doing:

auto foo = intIO(x);
std::cout<<foo;

However I do not see the point of prohibiting this use at the cost of making the implementation harder to read (I had to reason about those functions for quite some while before I was sure they work and why they work). You could just as well have

friend std::istream &operator>> (std::istream &is, integer_IO<T> i)
friend std::ostream &operator<< (std::ostream &os, const integer_IO<T>& i)

and your code would be easier to follow and would generate the same code. And allow the above usage syntax.

Reading into a temporary??

This tickled my funny bone:

template
<class T, typename = typename std::enable_if<std::is_integral<T>::value>::type>
integer_IO<T> intIO(T &&arg)
{
  return integer_IO<T>(arg);
}

As you are using universal references this means that arg will bind to anything and retain it's qualifiers. Even r-value references... which means that the following will compile and run doing some manner of undefined behaviour:

std::cin>>intIO(3);

Jolly.

In summary

I can see how this can have some uses in template code but I believe that your implementation is too complicated. My advice is to stop trying to be fancy with && and just make something that works.

I would probably have implemented it like this:

#include <istream>
#include <ostream>
#include <iostream>
#include <limits>
#include <type_traits>
#include <cstdint>

#define ENABLE_IF_INTEGRAL(T)\
typename = typename std::enable_if<std::is_integral<std::decay_t<T>>::value>::type

namespace detail{
    template<class T, ENABLE_IF_INTEGRAL(T)>
    struct wrapper{
        T& x;
        wrapper(T& xx) : x(xx){};

        friend std::istream& operator >> (std::istream& is, wrapper<T> w){
            auto read = +w.x;
            is>>read;
            w.x = static_cast<std::decay_t<T>>(read);
            if(w.x != read){
                is.setstate(std::ios::failbit);
                //C++11 behaviour
                constexpr auto max = std::numeric_limits<std::decay_t<T>>::max();
                constexpr auto min = std::numeric_limits<std::decay_t<T>>::min();
                w.x = static_cast<T>(read < min ? min : max);
            }
            return is;
        }
    };
}

template<class T, ENABLE_IF_INTEGRAL(T)>
auto intoInt(T& x){ return detail::wrapper<T>(x);}

template<class T, ENABLE_IF_INTEGRAL(T)>
auto asInt(T x){ return +x; }

#undef ENABLE_IF_INTEGRAL

int oneplusone(){return 1 + 1;}

int main() {
    char foo;

    // Shouldn't compile
    //std::cin>>asInt(foo);
    //std::cin>>asInt(3);
    //std::cin>>intoInt(3);
    //std::cin>>intoInt(std::move(foo));
    //std::cout<<intoInt(std::move(foo));
    //std::cout<<intoInt(3);
    //std::cout<<intoInt(foo);

    // Readable into
    std::cin>>intoInt(foo);

    // Check status
    std::cout<< (std::cin.fail() ? "failed":"success")<<std::endl;

    // Binds to reference
    auto& ref = foo;
    std::cout<<asInt(ref)<<std::endl;

    // Binds to const reference
    const auto& cref = foo;
    std::cout<<asInt(cref)<<std::endl;

    // Binds to r-value reference
    std::cout<<asInt(std::move(foo))<<std::endl;

    // Binds to value returned function call (r-value really)
    std::cout<<asInt(oneplusone())<<std::endl;

    // Binds to literal
    std::cout<<asInt(31)<<std::endl;

    return 0;
}

In addition to being shorter and easier to comprehend, it also prohibits: std::cin<<asInt(3); from happening.


Check winner in a Tic Tac Toe game I have the below code to check for a winner in a Tic Tac Toe game.  I'm wondering if this is a good approach, and if there is a better way of doing this (maybe by monitoring the state of the board).

public void CheckWinner()
{
    var board = GameBoard.Content;//stores data as dictionary
    for (int i = 0; i < 8; i++)
    {
        int a = winningIndex[i, 0], b = winningIndex[i, 1], c = winningIndex[i, 2];

        if (board.ContainsKey(a) && board.ContainsKey(b) && board.ContainsKey(c))
        {
            Func<KeyValuePair<int, string>, bool> containsO = input => (input.Key == a || input.Key == b || input.Key == c) && input.Value == "O";

            Func<KeyValuePair<int, string>, bool> containsX = input => (input.Key == a || input.Key == b || input.Key == c) && input.Value == "X";

            var oResult = board.Where(containsO).ToDictionary();
            if (oResult.Count == 3)
            {
                Winner = Player.Computer;
                WinnerIndex = oResult.Keys.ToArray();
                _gameOver = true;
            }

            var xResult = board.Where(containsX).ToDictionary();
            if (xResult.Count == 3)
            {
                Winner = Player.Human;
                WinnerIndex = xResult.Keys.ToArray();
                _gameOver = true;
            }
        }
    }
    if (board.Count == 9)
    {
        _gameOver = true;
        Winner = null;
    }
}

General advice:

Use enums instead of strings for board squares. This will protect you against mistakes like board[i] = "x".
A dictionary is overkill for a 3x3 board, you can just use an array. One benefit is that a bad index will throw an IndexOutOfRangeException, while a dictionary will not complain.
Board logic should be encapsulated in a class, so that you can make changes to your implementation (like switching from a dictionary to an array) without the rest of your code having to change. It will also allow you to guard against errors such as overwriting a non-empty board square.

To make the function easier to test:

Pass the board to the function, or make this a method of your Board class, instead of referring to GameBoard.Content.
Return an object representing the game state, instead of modifying private fields.

Together, it might look like this:

public static GameState GetGameState(Board board)
{
    foreach (var winningLine in WinningLines)
    {
        var line = new BoardState[]
        {
            board[winningLine[0]],
            board[winningLine[1]],
            board[winningLine[2]]
        };

        if (line.All(state => state == BoardState.Human))
        {
            return GameState.HumanWon(winningLine);
        }

        if (line.All(state => state == BoardState.Computer))
        {
            return GameState.ComputerWon(winningLine);
        }
    }

    return board.Any(state => state == BoardState.Empty)
        ? GameState.Unfinished
        : GameState.Tie;
}