PrimeFactorize method for int I have written this PrimeFactorize utility method in C# (using LINQPad) which works fine, but feels like it is a bit slow, I was wondering what might be improved. Also looking for any criticism not related to efficiency to make the code better.

// Generate prime factors for a number, e.g., input 12 returns 2 2 3
public static IEnumerable<int> PrimeFactorize(int number)
{
    if(IntUtils.IsPrime(number))
    {
        yield return number;
    }
    else
    {
        List<int> primes = IntUtils.GetPrimesBetween(2, number);
        while(!IntUtils.IsPrime(number))
        {
            foreach(var p in primes)
            {
                if(number % p == 0)
                {
                    yield return p;
                    number /= p;
                    break;
                }
            }
        }
        yield return number;
    }
}

As a note, the IntUtils.IsPrime and IntUtils.GetPrimesBetween are two other utility methods I wrote. I am including GetPrimesBetween below. IsPrime has already been reviewed and it is quite fast by itself and can be found on GitHub.

public static List<int> GetPrimesBetween(int min, int max)
{
    var primes = new List<int>();
    for(var i = min; i <= max; i++)
    {
        if(IntUtils.IsPrime(i) && i != 1)
        {
            primes.Add(i);
        }
    }
    return primes;
}

Here is a test run, along with the execution times:

void Main()
{
    var N = 12;
    for(var i = 1; i <= 10; i++)
    {
        Console.Write("Run # {0} | Number to factorize: {1}\nResult: ", i, N);
        foreach(var P in IntUtils.PrimeFactorize(N))
        {
            Console.Write(P + " ");
        }
        // mix up results a bit
        N += N*3 + i;
        Console.WriteLine("\nTime elapsed since start: {0}", Util.ElapsedTime);
    }
}

Results:

Run # 1 | Number to factorize: 12
Result: 2 2 3
Time elapsed since start: 00:00:00.0130129
Run # 2 | Number to factorize: 49
Result: 7 7
Time elapsed since start: 00:00:00.0132697
Run # 3 | Number to factorize: 198
Result: 2 3 3 11
Time elapsed since start: 00:00:00.0133637
Run # 4 | Number to factorize: 795
Result: 3 5 53
Time elapsed since start: 00:00:00.0139543
Run # 5 | Number to factorize: 3184
Result: 2 2 2 2 199
Time elapsed since start: 00:00:00.0183778
Run # 6 | Number to factorize: 12741
Result: 3 31 137
Time elapsed since start: 00:00:00.0528457
Run # 7 | Number to factorize: 50970
Result: 2 3 5 1699
Time elapsed since start: 00:00:00.2867861
Run # 8 | Number to factorize: 203887
Result: 31 6577
Time elapsed since start: 00:00:01.9602979
Run # 9 | Number to factorize: 815556
Result: 2 2 3 7 7 19 73
Time elapsed since start: 00:00:13.5247897
Run # 10 | Number to factorize: 3262233
Result: 3 13 233 359
Time elapsed since start: 00:01:35.3379070

As you can see, starting at Run #6 the times start getting a lot longer very quickly, and by Run # 10 it takes over a minute to get the results.

First of all, the PrimeFactorize() does never terminate for number = 1
because the check IntUtils.IsPrime(number) will never return true.
The call PrimeFactorize(1) should return an empty list.

To compare the performance of various implementations, I used the following simple
test program which computes all prime factors of the numbers up to 10,000. The sum
is calculated to prevent the compiler from removing function calls whose result is
not used, and to verify that all implementations give the same results.

using System.Diagnostics; // for StopWatch

public static void Main()
{
    Stopwatch stopWatch = Stopwatch.StartNew();
    var sum = 0;
    for(var N = 2; N <= 10000; N++)
    {
        foreach(var P in IntUtils.PrimeFactorize(N))
        {
            sum += P;
        }
    }
    stopWatch.Stop();
    Console.WriteLine("Sum: {0}, Time: {1} ms", sum, stopWatch.Elapsed.TotalMilliseconds);
}

The tests were done on a MacBook, using
 Mono.

For your code, the result is

 Sum: 10243046, Time: 2945.7256 ms

Your code creates a list of primes in order to find the lowest factor of the given number,
and the GetPrimesBetween() function seems be the performance bottleneck.

The IsPrime() function can be improved slightly. Since divisibility by 2, 3, 5 is
checked upfront, the loop can start with i = 7. Inside the loop, divisibility by
both i and i+2 is checked, so that the next possible divisor is i+4. Therefore
the loop should be

    // iterate with trial division
    int i = 7;
    while (i * i <= n)
    {
        if (n % i == 0 || n % (i + 2) == 0)
        {
            return false;
        }
        i += 4;
    }

This reduces the execution time to

Sum: 10243046, Time: 1193.3498 ms

To improve the PrimeFactorize() function, note that every composite number \$ n \$ necessarily
has a factor \$ p \le \sqrt n \$. Also, if a factor is found,
you can repeatedly check that factor instead of breaking out of the
loop and starting with the first prime again.

This leads to the following implementation:

public static IEnumerable<int> PrimeFactorize(int number)
{
    List<int> primes = IntUtils.GetPrimesBetween(2, (int)Math.Sqrt(number));
    foreach (var p in primes)
    {
        while (number % p == 0)
        {
            yield return p;
            number /= p;
        }
    }
    // Now either number == 1 or number is a prime:
    if (number > 1) {
        yield return number;
    }
}

which reduces the execution time considerably:

Sum: 10243046, Time: 15.6476 ms

A faster way to determine all prime numbers in a given range are sieving methods.
A simple implementation of the sieve of Eratosthenes is
(taken from https://codereview.stackexchange.com/a/62158/35991, but
as an enumerator):

public static IEnumerable<int>GetPrimesUpto(int number) {
    bool[] isComposite = new bool[number + 1];
    for (int x = 2; x <= number; x++)
    {
        if (!isComposite[x])
        {
            yield return x;
            for (int y = x * x; y <= number; y = y + x)
            {
                isComposite[y] = true;
            }
        }
    }
}

which can then be used as:

public static IEnumerable<int> PrimeFactorize(int number)
{
    var primes = IntUtils.GetPrimesUpto((int)Math.Sqrt(number));
    foreach(var p in primes)
    {
        while (number % p == 0)
        {
            yield return p;
            number /= p;
        }
    }
    if (number > 1) {
        yield return number;
    }
}

Execution time:

Sum: 10243046, Time: 9.6804 ms

But actually you don't need to compute a list of primes first
(and I am essentially repeating the arguments from https://codereview.stackexchange.com/a/64795/35991).

The smallest factor of a number is necessarily a prime. You already
divide the number by any factor found, but this means that the lowest
factor of the updated number is again a prime. So by repeatedly
searching for the lowest factor you'll find all prime factors without
doing any extra test for primality.

Again it suffices to check for factors up
to the square root of the (remaining) number. After that,  number is
either equal to one or a prime.

This leads to the following function:

// Generate prime factors for a number, e.g., input 12 returns 2 2 3
public static IEnumerable<int> PrimeFactorize(int number)
{
    // Check divisibility by 2:
    while (number % 2 == 0) {
        yield return 2;
        number /= 2;
    }
    // Check divisibility by 3, 5, 7, ...
    for (var i = 3; i * i <= number; i += 2) {
        while (number % i == 0) {
            yield return i;
            number /= i;
        }
    }
    if (number > 1) {
        yield return number;
    }
}

Execution time:

Sum: 10243046, Time: 2.6631 ms

(C# is not my primary language, but I hope that the code demonstrates the idea.)


Form that reveals more fields as each step has been completed I'm new to JavaScript and jQuery and need input about how to optimize my script:

(document).ready(function() {

    var inputs_1 = $("form#myForm .case1 input, form#myForm .case1 select");
    var inputs_2 = $("form#myForm .case2 input, form#myForm .case2 select");
    var inputs_3 = $("form#myForm .case3 input");

    $("form#myForm .case2").addClass("disabled");
    $("form#myForm .case3").addClass("disabled");
    $("form#myForm .case2 input, form#myForm .case2 select").prop('disabled', true);
    $("form#myForm .case3 input").prop('disabled', true);

    var validateInputs = function validateInputs(inputs) {
      var validForm = true;
      inputs.each(function(index) {
        var input = $(this);
        if (!input.val()) {
            $("#subnewtide").prop('disabled', true);
            validForm = false;
        }
      });
      return validForm;
    }

    inputs_1.change(function() {
      if (validateInputs(inputs_1)) {
          $("form#myForm .case2").removeClass('disabled');
          $("form#myForm .case2 input, form#myForm .case2 select").prop('disabled', false);
      }
    });

    inputs_2.change(function() {
      if (validateInputs(inputs_2)) {
          $("form#myForm .case3").removeClass('disabled');
          $("form#myForm .case3 input").prop('disabled', false);
      }
    });

    inputs_3.change(function() {
      if (validateInputs(inputs_3)) {
          $("#subnewtide").prop('disabled', false);
      }
    });

});

It is a registration form. After a user has inserted all inputs from step1 the next step should be shown. so far so good. My solution works, but I guess it is not the best way to solve my issue.

How can I abstract this more to increase maintainability?

I need a way to use one XY.change(function) handler, not for everyone a single solution.

Code is pretty neat, though you might consider a couple of additional objectives :

ensuring that elements are re-disabled if the user revisits an earlier form field and makes it invalid.
allowing the validate/disable/enable system to work with as many cases as might appear in the form, without needing to amend the javascript.

I guess there are many ways these objectives could be met.

Here's one way, in which :

the key actions ('validate' and 'disable') are written as custom event handlers. That proves to be very convenient as it allows elements to be given behaviours that can be triggered en bloc without needing to write jQuery plugins or loop manually in the high level application code.
several assumptions are made about the HTML/DOM_tree, in particular that .case1, .case3, .case3 select one element each.

HTML

Add class name case to all "case" containers.

JavaScript

jQuery(function($) {
    var $cases = $('.case'),
        $finalElement = $('#subnewtide'),
        $casesPlus = $cases.add($finalElement)
        fieldSelector = 'input, select'; // a reusable selector string

    // attach event handlers to $cases
    $cases.on('validate', function(event) { // custom event
        var $this = $(this),
            n = $this.index($cases), // index of this within $cases
            valid = true;
        $casesPlus.slice(n+1).trigger('disable', [true]); // unconditionally disable everything below `this`
        $(this).find(fieldSelector).each(function() {
            if (!$(this).val()) {
                valid = false;
                return false; // break out of .each() iteration
            }
        });
        if (valid) {
            // conditionally enable next case and trigger 'validate' on it ... and so on in cascade ...
            $casesPlus.slice(n+1, n+2).trigger('disable', [false]).trigger('validate');
        }
    }).on('disable', function(event, bool) { // custom event
        if (bool) {
            $(this).addClass('disabled').find(fieldSelector).prop('disabled', true);
        } else {
            $(this).removeClass('disabled').find(fieldSelector).prop('disabled', false);
        }
    }).on('change', fieldSelector, function(event) {
        $(this).closest('.case').trigger('validate');
    });

    // attach event handlers to $finalElement
    // (slightly different from $cases)
    $finalElement.on('disable', function(event, bool) {
        $(this).prop('disabled', bool);
    });

    // initially, enable the first case and trigger 'validate' to disable everything else.
    $cases.eq(0).trigger('disable', [false]).trigger('validate');
});

untested

This style of programming tends to suit form validation and spreadsheet-like applications.


How can I make this WHERE clause more readable? Without changing the semantics and performance, how can I make this where clause more readable?

where
(@Category = 'all' or
    (@Category = 'omitted' and Category is null) or
    (@Category = Category and (
        @SubCategory = 'all' or
        (@SubCategory is null and SubCategory is null) or
        (@SubCategory = SubCategory and (
            @SubSubCategory = 'all' or
            (@SubSubCategory is null and SubSubCategory is null) or
            @SubSubCategory = SubSubCategory
        ))
    ))
)

At a high level, this clause represents a user query where the user has optionally specified a category, subcategory, and sub-subcategory on a table.  For compatibility reasons, I cannot change any semantics (e.g., I cannot improve consistency by using @Category is null instead of @Category = 'omitted')

Add comments, they're the usual way of making complicated source code easier to understand.

There's not a lot to add in your case because the meaning of the code is simple even if the implementation isn't easy to take in at a glance, but adding a few short comments may make it much more readable, especially for someone who's reading through the whole thing trying to understand the flow of logic. I would just do this:

where
    -- optional category
    (@Category = 'all' or
    (@Category = 'omitted' and Category is null) or
    (@Category = Category and (
        -- optional subcategory
        @SubCategory = 'all' or
        (@SubCategory is null and SubCategory is null) or
        (@SubCategory = SubCategory and (
            -- optional subsubcategory
            @SubSubCategory = 'all' or
            (@SubSubCategory is null and SubSubCategory is null) or
            @SubSubCategory = SubSubCategory
        ))
    ))
)

Now I can quickly see the purpose of the code, and even if the detailed logic is awkward to follow it may not matter at all because I may not need to change it, just understand what it's doing. If I do need to modify it, I'll have no choice but to work through it in detail anyway, in which case the comments will help to point me in the right direction.

You didn't mention if you're using SQL Server or Sybase, but if it's SQL Server and if you haven't read it already, this article is well worth reading. I wouldn't be too dogmatic about avoiding dynamic SQL, it certainly has its issues but it has its uses too.


String hash generator There is a lot of code duplication because it generates hashes using multiple cryptographic hash algorithms.

How can I improve this code?

package main;
import java.util.Scanner;
import java.math.BigInteger;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;

public class String_Hash_Generator {
    public static void main(String args[]) throws NoSuchAlgorithmException {
        Scanner inputScanner = new Scanner(System.in);
        System.out.print("String: ");
        String input = inputScanner.next();

        /* MD2 */
        MessageDigest objMD2 = MessageDigest.getInstance("MD2");
        byte[] bytMD2 = objMD2.digest(input.getBytes());
        BigInteger intNumMD2 = new BigInteger(1, bytMD2);
        String hcMD2 = intNumMD2.toString(16);
        while (hcMD2.length() < 32) {
            hcMD2 = "0" + hcMD2;
        }

        /* MD5 */
        MessageDigest objMD5 = MessageDigest.getInstance("MD5");
        byte[] bytMD5 = objMD5.digest(input.getBytes());
        BigInteger intNumMD5 = new BigInteger(1, bytMD5);
        String hcMD5 = intNumMD5.toString(16);
        while (hcMD5.length() < 32) {
            hcMD5 = "0" + hcMD5;
        }

        /* SHA-1 */
        MessageDigest objSHA1 = MessageDigest.getInstance("SHA-1");
        byte[] bytSHA1 = objSHA1.digest(input.getBytes());
        BigInteger intNumSHA1 = new BigInteger(1, bytSHA1);
        String hcSHA1 = intNumSHA1.toString(16);
        while (hcSHA1.length() < 40) {
            hcSHA1 = "0" + hcSHA1;
        }

        /* SHA-256 */
        MessageDigest objSHA256 = MessageDigest.getInstance("SHA-256");
        byte[] bytSHA256 = objSHA256.digest(input.getBytes());
        BigInteger intNumSHA256 = new BigInteger(1, bytSHA256);
        String hcSHA256 = intNumSHA256.toString(16);
        while (hcSHA256.length() < 64) {
            hcSHA256 = "0" + hcSHA256;
        }

        /* SHA-384 */

        MessageDigest objSHA384 = MessageDigest.getInstance("SHA-384");
        byte[] bytSHA384 = objSHA384.digest(input.getBytes());
        BigInteger intNumSHA384 = new BigInteger(1, bytSHA384);
        String hcSHA384 = intNumSHA384.toString(16);
        while (hcSHA384.length() < 96) {
            hcSHA384 = "0" + hcSHA384;
        }

        /* SHA-512 */
        MessageDigest objSHA512 = MessageDigest.getInstance("SHA-512");
        byte[] bytSHA512 = objSHA512.digest(input.getBytes());
        BigInteger intNumSHA512 = new BigInteger(1, bytSHA512);
        String hcSHA512 = intNumSHA512.toString(16);
        while (hcSHA512.length() < 128) {
            hcSHA512 = "0" + hcSHA512;
        }

        System.out.println("\nMD2: " + hcMD2
                        + "\nMD5: " + hcMD5
                        + "\nSHA-1: " + hcSHA1
                        + "\nSHA-256: " + hcSHA256
                        + "\nSHA-384: " + hcSHA384
                        + "\nSHA-512: " + hcSHA512);
    }
}

Your code looks promising but has some code smells.

When you have a big method you should consider splitting the method
up into methods/classes.
When you have a lot of code duplication you
should consider extracting common logic into methods/classes.
Your code is hard to test, consider extracting related logic into classes.

Example of a refactoring that could help you clean this code:

public class String_Hash_Generator  {

    public static void main(String args[]) throws NoSuchAlgorithmException {
        Scanner inputScanner = new Scanner(System.in);
        System.out.print("String: ");
        String input = inputScanner.next();

        CryptographyGenerator generator = new CryptographyGenerator();

        System.out.println("\nMD2: " + generator.generateMD2(input)
                + "\nMD5: " + generator.generateMD5(input)
                + "\nSHA-1: " + generator.generateSHA1(input)
                + "\nSHA-256: " + generator.generateSHA256(input)
                + "\nSHA-384: " + generator.generateSHA384(input)
                + "\nSHA-512: " + generator.generateSHA512(input));
    }
}

public class CryptographyGenerator {

    public String generateMD2(String input) throws NoSuchAlgorithmException {
        return generateString(input, "MD2", 32);
    }

    public String generateMD5(String input) throws NoSuchAlgorithmException {
        return generateString(input, "MD5", 32);
    }

    public String generateSHA1(String input) throws NoSuchAlgorithmException {
        return generateString(input, "SHA-1", 40);
    }

    public String generateSHA256(String input) throws NoSuchAlgorithmException {
        return generateString(input, "SHA-256", 64);
    }

    public String generateSHA512(String input) throws NoSuchAlgorithmException {
        return generateString(input, "SHA-512", 128);
    }

    public String generateSHA384(String input) throws NoSuchAlgorithmException {
        return generateString(input, "SHA-384", 96);
    }

    private static String generateString(String input, String algorithm, int minLength) throws NoSuchAlgorithmException {
        MessageDigest messageDigest = MessageDigest.getInstance(algorithm);
        byte[] bytes = messageDigest.digest(input.getBytes());
        BigInteger integer = new BigInteger(1, bytes);
        String result = integer.toString(16);
        while (result.length() < minLength) {
            result = "0" + result;
        }
        return result;
    }
}


Project Euler module I use Project Euler to teach me programming and not to submit any results. As such I look up the expected return values to double check my solutions.

To organise my files I use the following folder structure:

main.py
\euler # The problem files
     __init__.py # empty
     e001.py
     e002.py
     ...
\input # Additional input files
     8.dat
     11.dat
     ...

My main.py file is the common entry point. It can either run all the solved examples so far or a specific one. This second option is added that I don't need to add an if __name__ == '__main__' guard in every file. The file looks as follows:

TOP_LEVEL = "euler"

def run_module(num):
    """Run specific Problem"""
    mod = importlib.import_module('%s.e%0.3i' % (TOP_LEVEL, num))

    start = time.time()
    ist = mod.run()
    print("  %5i | %6.3f |  %s  | %i" % \
(num, time.time() - start, "ox"[ist == mod.SOLL], ist))

if __name__ == '__main__':
    N_MAX = 67

    # Pre Header
    print('Problem |  Time  | x/o | Solution')
    print("--------+--------+-----+---------")

    global_time = time.time()

    # Run over all problems
    if len(sys.argv) == 2:
        run_module(int(sys.argv[1]))
    else:
        for num in range(1, N_MAX + 1):
            run_module(num)

    # End Header
    print("--------+--------+-----+---------")
    print("Total: %.3f s" % (time.time() - global_time))

I'll show now two example files to show the source files and how old code can be reused. e018.py:

"""By starting at the top of the triangle below and moving to adjacent numbers on the row below, the maximum total from top to bottom is 23.

3
7 4
2 4 6
8 5 9 3
That is, 3 + 7 + 4 + 9 = 23.

Find the maximum total from top to bottom of the triangle below"""

SOLL = 1074

def run(file = "input/18.dat"):
    # Parse File
    with open(file) as fid:
        tri = [[int(num) for num in line.split(' ')] for line in fid.read().split('\n')]

    # From bottom's up find the maximal value
    for row in range(len(tri) - 2, -1, -1):
        for col in range(row + 1):
            tri[row][col] += max(tri[row + 1][col], tri[row + 1][col + 1])

    return tri[0][0]

and e067.py

"""By starting at the top of the triangle below and moving to adjacent numbers on the row below, the maximum total from top to bottom is 23.

3
7 4
2 4 6
8 5 9 3
That is, 3 + 7 + 4 + 9 = 23.

Find the maximum total from top to bottom of the triangle below"""

import e018

SOLL = 7273

def run(file = "input/67.dat"):
    # problem has been solved in set 18
    return e018.run(file = file)

Since this is the first time I tried to structure such a project, I'm quite sure there is plenty of room for optimization. I'm happy for any feedback I can get.

Use .format

Using % is considered old style, so:

print("  %5i | %6.3f |  %s  | %i".format(
    (num, time.time() - start, "ox"[ist == mod.SOLL], ist)))

Be generous with long variables names

For example ist is impossible to understand for me, solution is more natural

Don't abuse Code-Golf techniques

The line:

"ox"[ist == mod.SOLL]

is a well known Code-Golf trick that relies on implicit boolean to integer conversion that is equivalent to:

"x" if ist == mod.SOLL else "o"

please use the latter.

Use argparse

C-Style arguments such as sys.argv[1] should be avoided, I suggest argparse (help to get started here: https://stackoverflow.com/questions/7427101/dead-simple-argparse-example-wanted-1-argument-3-results)

Don't shadow built-ins

def run(file = "input/18.dat"):
    # Parse File
    with open(file) as fid:

file is a built-in, you should use file_ in your code.

Explode long lines

Divide the following lines in two please.

 tri = [[int(num) for num in line.split(' ')] for line in fid.read().split('\n')]


Settings JAVA_HOME with Powershell Previously in the Settings JAVA_HOME with batch post, I had created a script in bash to change my JAVA_HOME env variable. Since then, I have been trying to use more and more PowerShell so I recreated the script with PowerShell.

The script has several options that provide different functionalities:

e or echo : output the current value of JAVA_HOME.
s or set : option to change the current value of JAVA_HOME. You must provide the key you want to change to.
a or add : add a key value pair to the store file that you can later on changed to.
l or list : list the current value in the file.
r or remove : remove the key-value pair from the store file.

This is still a script that I use for my personal use so there are still things that are not robust. You can add a key with an empty value or an invalid one and the script will not stop you. I've tested every option and all the "happy paths" are working.

This is my first script in PowerShell, so there were a lot of googling and patching all around the script to make it work.

<#
    Name: javaenv
    Date: 12 September 2016
    Author: Marc-Andre Girard
    Purpose: Modify the JAVA_HOME env variable based on Java installations on the computer.
#>

Param(
  [Parameter(Mandatory=$True)]
  [string]$action,
  [string]$key,
  [string]$value
)

<#
    Show the JAVA_HOME variable to the screen
#>
function Output() {
    Get-ChildItem Env:JAVA_HOME
}

<#
    Set the JAVA_HOME variable in the Process scope to only influence the current process.
#>
function SetJavaHome([string]$value_to_set) {
    [Environment]::SetEnvironmentVariable("JAVA_HOME", $value_to_set, "Process")
}

<#
    Get a value from the version file, based on a key provided.
#>
function GetValueFromFile([string]$key_in_file) {
    $javaVersions = GetVersionsFromFile
    if($javaVersions.ContainsKey($key_in_file)) {
        $javaVersions.$key_in_file
    } else {
        Write-Host "The key does not exit in this file"
        exit
    }
}

<#
    Add a version to the version file, based on a key-value provided.
#>
function AddVersionToFile([string]$key_in_file, [string]$value_to_add) {
    $javaVersions = GetVersionsFromFile
    if (!$javaVersions.ContainsKey($key_in_file)) {
        Add-Content $dir/java_versions.store "$key_in_file=$value_to_add"
    } else {
        Write-Host "Key already exist in the file"
        exit
    }
    Write-Host "$key_in_file=$value_to_add"

}

function GetVersionsFromFile() {
    ConvertFrom-StringData (Get-Content $dir/java_versions.store | Out-String)
}

function RemoveVersionFromFile([string]$key_to_remove) {
    Write-Host $key_to_remove
    $javaVersions = GetVersionsFromFile
    $javaVersions.remove($key_to_remove)
    $content = HashConvertToString $javaVersions
    $content | Out-File $dir/java_versions.store
}

function HashConvertToString($ht) {
    $content_file = ""
    foreach($pair in $ht.GetEnumerator()) {
        $content_file += "{0}={1}`n" -f $($pair.key),$($pair.Value)
    }

    $content_file
}

Set-StrictMode -Version Latest

$scriptpath = $MyInvocation.MyCommand.Path
$dir = Split-Path $scriptpath

if($action -eq "e" -or $action -eq "echo") {
    Output
}

if($action -eq "s" -or $action -eq "set") {
    if(![string]::IsNullOrEmpty($key)) {
        $value_from_file = GetValueFromFile($key)
        SetJavaHome($value_from_file)
        Output
    } else {
        Write-Host "You must provide the key to set with -key or provide the second argument"
    }

}

if($action -eq "a" -or $action -eq "add") {
    AddVersionToFile $key $value
}

if($action -eq "l" -or $action -eq "list") {
    GetVersionsFromFile
}

if($action -eq "r" -or $action -eq "remove") {
    RemoveVersionFromFile $key
}

This is an example of what a store file should look like :

JAVA6=C:/Program Files/Java/jdk1.6.0_43
JAVA7=C:/Program Files/Java/jdk1.7.0_79
JAVA8=C:/Program Files/Java/jdk1.8.0_73

For a beginner script there are some There might be more to cover but there are a few things here that could improve the script.

Variable and function naming

In general functions and cmdlets should follow the Verb-Noun naming convention. You have the verb and noun part down but the hyphens are not there. Not a must but I would expect you to see those on other scripts.

Variables generally follow camelCase (with first work in lower) or PascalCase. I use camelCase myself. In either case PowerShell were prefer if you use one of those over the joined by underscore method.

Parameter in functions / cmdlets you will always see use PascalCase.

PowerShell script help

You have a multiline command block that contains some information about the script and its creation. I used the same sort of thing for my old VBS scripts. PowerShell has a robust help system you could(should?) be using for this information.. Stealing the simple example from TechNet

<#
.SYNOPSIS

The synopsis goes here. This can be one line, or many.
.DESCRIPTION

The description is usually a longer, more detailed explanation of what the script or function does. Take as many lines as you need.
.PARAMETER computername

Here, the dotted keyword is followed by a single parameter name. Don't precede that with a hyphen. The following lines describe the purpose of the parameter:
.PARAMETER filePath

Provide a PARAMETER section for each parameter that your script or function accepts.
.EXAMPLE

There's no need to number your examples.
.EXAMPLE
PowerShell will number them for you when it displays your help text to a user.
#>

This way you can give more explanation to what your script is doing to the users that might be running it. (I realize this is a personal script). Also is an easy way to explain how each parameter works and give some examples.

The power this gives you is using Get-Help in the shell against your script.

Single line comments

# You can also just use the number sign for single line comments instead of <##>

Too many ifs

If you find yourself with several if blocks there is probably a better way. You should be using a switch for this.

switch ($action){
    {$_ -eq "e" -or $_ -eq "echo"}{
        "Do Something."
    }
    {"s","set" -contains $_}{
        "Do Something Else."
    }
    default {
        "You typed '$_' and it is has no associated action"
    }
}

The above covers a couple of ways that you can work with multiple values of $action that result in the same code being executed. There is more you can do with switches so I encourage to read up on them. You can use regex as well for your cases but I opted not to include it here for brevity.

Remember that

  If the value matches multiple conditions, the action for each condition is executed. To change this behavior, use the Break or Continue keywords.

So it would not be uncommon to see break being used in examples on the web.

Advanced function parameters

This is another large topic that is covered in about_functions_advanced_parameters but one example to call out would be your $action parameter. You only support 10 values and the script will do nothing if the one of those values was not entered.

Param
(
    [parameter(Mandatory=$true)]
    [ValidateSet("e", "echo","s","set","a","add","l","list","r","remove")]
    [String]$Action
    # Continue with other params
)

So now if you don't provide one of those values for $action PowerShell will throw an error stating just that.

  Cannot validate argument on parameter 'Action'. The argument "bagel" does not belong to the set "e,echo,s,set,a,add,l,list,r,remove" specified by the ValidateSet attribute. Supply an argument that
  is in the set and then try the command again.

Parameter Sets

A continuation of advanced functions... Your $action is looking one of a set of mutually exclusive choices. Why not convert those actions into parameter switches? Then, using parameter sets, you can define their use with the $key $value parameters. Ensuring that nothing will be called or used if the combination was not meant to be. Consider the following (ignore the name. I had to call it something) function:

function Set-JavaHome{
    [CmdletBinding(DefaultParametersetName="List")]
    Param
    (
        [Parameter(ParameterSetName="Echo")]
        [switch]$Echo,
        [Parameter(ParameterSetName="Set")]
        [switch]$Set,
        [Parameter(ParameterSetName="Add")]
        [switch]$Add,
        [Parameter(ParameterSetName="List")]
        [switch]$List,
        [Parameter(ParameterSetName="Remove")]
        [switch]$Remove,
        [Parameter(ParameterSetName="Add",Mandatory=$true)]
        [Parameter(ParameterSetName="Remove",Mandatory=$true)]
        [Parameter(ParameterSetName="Set",Mandatory=$true)]
        [string]$Key,
        [Parameter(ParameterSetName="Add",Mandatory=$true)]
        [string]$Value
    )

    switch($pscmdlet.ParameterSetName){
        "Echo" {"Echo echo echo...."}
        "List" {"Show me the list!"}
        "Remove" {"Removing $key"}
        "Set"{"Setting the value to $key"}
        "Add"{"The new key:-$key- will be set to -$value-"}
    }
}

This gives better control over what is used when calling the function (or script). Running Get-Command against this function will help explain.

PS C:\Windows\system32> Get-Command Set-JavaHome -Syntax

Set-JavaHome [-List] [<CommonParameters>]

Set-JavaHome [-Echo] [<CommonParameters>]

Set-JavaHome -Key <string> [-Set] [<CommonParameters>]

Set-JavaHome -Key <string> -Value <string> [-Add] [<CommonParameters>]

Set-JavaHome -Key <string> [-Remove] [<CommonParameters>]

This shows the acceptable ways to call this function. There is no need to check, for example, if $key has been populated with calling -Add since the cmdlet would throw an error or prompt the user otherwise. $key is now mandatory for the parameter sets it is assigned to. PowerShell now pro

PS C:\Windows\system32> Set-JavaHome -Add -Value "C:\bagel"
cmdlet Set-JavaHome at command pipeline position 1
Supply values for the following parameters:
Key:

This is not a feature of parameter sets but since we have each "action" as its own parameter we also can take advantage of calling the parameters by their shortest unambiguous string. In your care that would accept the first letter.

Set-JavaHome -l

Depending on your function this could collide with common cmdlet parameters () e.g. verbose, whatif. In this case I do not think there is any worry.

Test-Path

You are adding paths to your environment. It might be worth testing them before the get used. A simple code block might help prevent hair pulling down the line

If (Test-Path $value){
   "Valid Path"
} else {
   "Not Valid Path"
}

Hopefully that will give you a little something to work with.


Fill a container from several threads I want to generate some random data and store it in a container. I split the container into disjoint intervals using iterators and pass them to a thread function (I hope its body isn't important here).

There are several design-related questions here:

Is it a good practice to solve such task in this manner?
If it is, do I optimally pass and receive parameters in thread_func?

#include <iostream>
#include <thread>
#include <random>
#include <utility>

template <typename Iterator, typename Distributions>
void thread_func(Iterator b, Iterator e, Distributions& ds)
{
    // *b++ = something, while b != e
}

int main()
{
    auto nthreads = std::thread::hardware_concurrency() ?: 1;
    std::cout << nthreads << " concurrent threads possible" << std::endl;

    const int nevents = 1'000'000;
    std::vector<std::pair<int, int>> events(nevents);

    int nparts = nevents / nthreads;

    std::vector<std::poisson_distribution<>> distributions;
    for (int i = 0; i < 10; i++)
        distributions.emplace_back(5);

    std::vector<std::thread> threads;
    for (int i = 0; i < nthreads - 1; i++) {
        auto left  = begin(events) + nparts * i;
        auto right = begin(events) + nparts * (i+1);

        auto f = [l=std::move(left), r=std::move(right), &d=distributions] { thread_func(l, r, d); };

        threads.emplace_back(f);
    }
    auto f = [l=begin(events)+nparts*(nthreads-1), r=end(events), &d=distributions] { thread_func(l, r, d); };
    threads.emplace_back(f);

    for (auto&& t : threads)
        t.join();
}

Questions

Is it a good practice to solve such task in this manner?

Yes. Sort of.
Distributing work across threads will get the work done quicker usually (especially when there is no interactions between threads).
BUT: The cost of spawning threads is relatively high. Doing it for such a simple problem may not be worth the cost (you should time it and try). So I am going to assume this is a trivial example just for review.
Normally you would spin up your threads into a generic pool. Then send the job to the pool for processing. The threads will continue to live after they have done the job waiting for more jobs to be added to the pool
Secondly this is a very low level interface.
C++ has added the concept of future and promise as a higher level interface to threading.

If it is, do I optimally pass and receive parameters in thread_func?

Sure that looks fine.
Code Review
So here ds is a shared resource used by all threads.
void thread_func(Iterator b, Iterator e, Distributions& ds)

If there is any mutation in this object then any gains can immediately be lost because of contention. So either the usage of this object has to be very well known or it needs to be const. To prevent contention you should make that a read only object that is shared across the threads.
So even if there is only 1 for concurrency you are spinning up threads?
    auto nthreads = std::thread::hardware_concurrency() ?: 1;

If you want to use this I would check that you have more than one core available.
    auto nthreads = std::thread::hardware_concurrency();
    if (nthreads < 2) {
        thread_func(std::begin(events), std::end(events), distributions);
    }
    else // Do it in parallel.
    {
    }

Don't rely on statement size scope:
    for (int i = 0; i < 10; i++)
        distributions.emplace_back(5);

It is just as easy to write:
    for (int i = 0; i < 10; i++) {
        distributions.emplace_back(5);
    }

This will protect you from accidental errors.
You don't need to re-calculate these every time.
        auto left  = begin(events) + nparts * i;
        auto right = begin(events) + nparts * (i+1);

Also if you update your code to use another container (or any generic container) the operator+ may not work. You should be using std::advance()
auto  segmentBegin = begin(events);
auto  segmentEnd   = segmentBegin;
std::advance(segmentEnd, nparts);

for (int i = 0; i < nthreads - 1; i++) {
   threads.emplace_back([l=segmentBegin, r=segmentEnd, &d=distributions] { thread_func(l, r, d); });

    segmentBegin = segmentEnd;
    std::advance(segmentEnd, nparts);
}
threads.emplace_back([l=segmentBegin, r=std::end(events), &d=distributions] { thread_func(l, r, d); });

Iterators are very cheap objects.
 l=std::move(left), r=std::move(right)

don't see the need to move them.


Is this the optimal way to write "find these IDs in a list of objects" using linq? I've got a situation where I have a list of Organizations (code to follow), and a user has a list of OrganizationIds (ints). I want to filter down the full list of Organizations using the User's OrganizationId list for a dropdown list. The query works, but it feels like there may be a more optimal way to write it. Is there, or am I already pretty much optimized?

public class Organization
{
    public int Id { get; set; }
    public string Division { get; set; }
    public bool IsActive { get; set; }
    public bool IsDefault { get; set; }
    public string Name { get; set; }
}

Query

vm.locations = _organizations.GetOrganizations().Where(o => ((ApplicationUser)User).OrganizationIdList.Any(u => u == o.Id)).Select(org =>
    new SelectListItem
    {
        Text = org.Division + " - " + org.Name,
        Value = org.Id.ToString()
    }).OrderBy(item => item.Text);

Use a Dictionary<int, Organization> to look up organizations by their IDs quickly.

Your code will then look like this: OrganizationIdList.Select(id => organizations[id])....

Given your clarification in the comments that your are getting organizations from a database, you should let the database do the hard work for you. The code will depend upon the ORM you're using, but a relatively SQL-agnostic version of the query will look like this:

SELECT organization_id, division, name FROM organizations WHERE organization_id IN (?)

or this:

SELECT organization_id, division, name FROM organizations JOIN user_organizations
USING (organization_id) WHERE user_id = ?


reinterpret_cast vs bit shifts for extracting 32-bit integers from bytes, with either endianness I've written some simple helper functions that read data types from a vector of bytes (binary files) and having a tough time deciding which route to go.

Method 1 : reinterpret_cast

int32_t read_32s(const std::vector<uint8_t> &buf, const unsigned offset,
                 const bool bswap = false) {
  // Check for out of bounds
  if (offset > buf.size() - sizeof(int32_t)) {
    // error handling
  }
  // Cast the pointer to int32_t and get the first value
  const auto val = reinterpret_cast<const int32_t *>(&buf[offset])[0];
  // Swap bytes if necessary
  if (bswap) {
    return (((val & 0xFF000000) >> 24) | ((val & 0x00FF0000) >> 8) |
            ((val & 0x0000FF00) << 8) | ((val & 0x000000FF) << 24));
  }
  return val;
}

The biggest advantage with this method is probably performance, readability is debatable. This basically compiles down to a single instruction, two if you need to swap the bytes. The problem I have with this is casting like this in modern C++ is frowned upon, and I am unsure how safe it actually is despite bounds checking first. How does alignment work?

Method 2 : classic bit shifts

int32_t read_32s(const std::vector<uint8_t> &buf, const unsigned offset,
                 const bool bswap = false) {
  // Check for out of bounds
  if (offset > buf.size() - sizeof(int32_t)) {
    // error handling
  }
  // Swap bytes if necessary
  if (bswap) {
    return (buf[offset] << 24) | (buf[offset + 1] << 16) |
           (buf[offset + 2] << 8) | buf[offset + 3];
  }
  return (buf[offset + 3] << 24) | (buf[offset + 2] << 16) |
         (buf[offset + 1] << 8) | buf[offset];
}

Very simple bit shifting, about double the instructions as the first method, and can get a bit messy with larger data types. I presume this is the safer solution though, and perhaps more readable.

Is the first method OK, or would you consider it to be poor code in comparison to the second method? Or maybe there's just a better method all together (std::byte?).

Here is a compiler explorer link with both examples.

You are correct to be concerned about the first method. It generates UB if buf[offset] doesn't happen to be at the right alignment boundary for a 32-bit value.

The second method is the way to go, even if you don't care about portability (but as a bonus, it's portable, except for the use of int32_t).

As mentioned by @AJNeufeld you need to fix up your out-of-bound checks. Once that's done, you could simplify the function quite a bit by using std::swap(). For example:

if (bswap) {
    swap(buf[offset    ], buf[offset + 3]);
    swap(buf[offset + 1], buf[offset + 2]);
}
return (buf[offset + 3] << 24) | (buf[offset + 2] << 16) |
    (buf[offset + 1] << 8) | buf[offset];

That's even pretty easy to generalize to any size with a couple of loops (that will almost certainly be unrolled).

EDIT: As @hoffmale notes in the comments, the return line is still possibly UB. The reason it technical.

auto b = int8_t{};
auto r = b << 8;

The above code is legal because in every arithmetic operation, signed chars and shorts are always promoted to int. So b is promoted to int, which must be at least 16 bits, and the shift is cool, and r is int.

The problems arise when you shift by more than 8 bits. The types will always be promoted to int, but int is only guaranteed to be 16 bits. That means if you did:

auto r = b << 16; // or 24

now you have a problem.

To solve it, you need to make sure the value being shifted is at least 32 bits before the shift:

auto r = uint_fast32_t{b} << 16; // or 24

This works portably.

So you'd need to add this cast before each shift in the return line.

But a better way to handle this - one that scales - is to use a loop:

auto ret = int32_t{};
for (auto i = 0; i < 4; ++i)
{
    ret << 8;
    ret |= buf[offset + (3 - i)];
}

or even:

auto ret = int32_t{};
for (auto i = 0; i < 4; ++i)
{
    ret << 8;
    if (bswap)
        ret |= buf[offset + i];
    else
        ret |= buf[offset + (3 - i)];
}

and now you don't have to worry about promotions. Any decent optimizing compiler will hoist the invariant conditional and offset out of the loop and unroll the loop (and eliminate the unnecessary shift). And you can even template the loop on the size and reuse it.

Shifting and OR-ing is the only portable way to get the little-endian data in buf into an integer for all machines.

@juhist describes another option: using memcpy(). That's not a portable solution for a number of technical reasons. First, you'll get different behaviour depending on the endianness of the machine. More technically, the machine may not always store integers the way you expect, and I'm not referring to endianness by that. You're dealing with signed values, but the type of signedness of the data may not be the same as the machine (for example, the data may be twos-complement, the machine may be signed magnitude). There may be padding bits and other weirdness going on that ends up meaning that a 32 bit value of 0xFFFFFFFF may not necessarily be represented in memory as 4 bytes of 0xFF. Or if your input data is "0x00 0x00 0x00 0xA0" which if memcpy()ed on a little endian machine will give you 0xA0000000 (that's its value, not its memory representation), which you assume is −2147483648... but that's assuming bit 31 is the sign bit, which is not necessarily true. Could be bit 0, for example, meaning this value is actually 1342177280. (Granted, if your data has a sign bit, you really need to handle it specifically in any case. That is, you'd need to calculate the high byte as something like buf[offset + 3] & 0x7Fu, and multiply the result by −1 if buf[offset + 3] & 0x80u... or you could dump the whole thing into an unsigned value, and then deal with the sign bit. But the point about the sign bit not being where you expect it still stands: 0x01 0x00 0x00 0x00, if memcpy()ed on a little-endian machine, could produce −2147483648 rather than 1... or anything else really.)

If performance really matters - and, I'll be blunt, if you're using this part of a serialization routine, it almost certainly doesn't; whatever time you spend decoding integers is going to be dwarfed by the I/O - you could write system-specific code. For example, if your system is little-endian, and you know the representation is the same as what's in the buffer, you could just memcpy(). If it's big-endian, maybe memcpy() and a swap intrinsic, or a loop that's basically reverse memcpy() (create the integer, then copy the bytes in reverse order). If it's other-endian or non-twos-complement or has some other quirck... well, you'd know how to deal with that in that case.

The bottom line is that shifting and OR-ing is the only portable solution (and you'd need to handle the sign bit properly). If you don't care about portability and you want speed, well, then the "right" answer depends entirely on 1) the data representation, and 2) your machine.


Concatenating an array of optional NSAttributedStrings Imagine you had to join together attributed strings with a separator in between them.  Which of the following methods would you use?

An extension on SequenceType with a function that takes in a separator.   Can't use any optionals in the array.

extension SequenceType where Generator.Element: NSAttributedString {
    func join(withSeparator separator: NSAttributedString) -> NSAttributedString {
        var shouldAddSeparator = true
        return self.reduce(NSMutableAttributedString()) {(element, sequence) in
            if shouldAddSeparator {
            shouldAddSeparator = false
            }
            else {
                element.appendAttributedString(separator)
            }
        element.appendAttributedString(sequence)
        return element
        }
    }
}

A private function that takes in an array of optional NSAttributedString and a separator.

private func join(attributedStrings strings: [NSAttributedString?], withSeparator separator: NSAttributedString) -> NSAttributedString? {
    let unwrappedStrings = strings.flatMap{$0} as [NSAttributedString]
    guard unwrappedStrings.count == strings.count else { return nil }
    let finalString = NSMutableAttributedString()
    for (index, string) in unwrappedStrings.enumerate() {
        if index == 0 {
            finalString.appendAttributedString(string)
        }
        else {
            finalString.appendAttributedString(separator)
            finalString.appendAttributedString(string)
        }
    }

    return finalString
}

Extension on _ArrayType that can take in an array of options NSAttributedString

extension _ArrayType where Generator.Element == NSAttributedString? {
    private func join(withSeparator separator: NSAttributedString) -> NSAttributedString? {
        let unwrappedStrings = flatMap{$0} as [NSAttributedString]

        var shouldAddSeparator = false
        return unwrappedStrings.reduce(NSMutableAttributedString(), combine: { (string, element) in
            if shouldAddSeparator {
                string.appendAttributedString(separator)
            }
            else {
                shouldAddSeparator = true
            }
            string.appendAttributedString(element)
            return string
        })
    }
}

The second and third approach combine two tasks into one:

Extract the non-nil elements from the given array of optional
attributed strings, and
concatenate these with a given separator.

My suggestion is to separate these concerns. This rules out #2 and #3
and leaves us with your #1 approach, the extension method on SequenceType.

Another disadvantage of the extension _ArrayType  is that it uses an internal type,
so that might break in a future version of Swift.

The flatMap() method from the Swift standard library already provides a method for the first task:

let attributedStrings = optionalAttributedStrings.flatMap { $0 }

and this can be combined with your extension method by the caller:

let joined = optionalAttributedStrings.flatMap { $0 }.join(withSeparator: separator)

A user might also want to concatenate non-optional strings, which
works with the extension method:

let joined = attributedStrings.join(withSeparator: separator)

but not with the other two approaches.

Another argument for approach #1 is that it resembles
the existing method to join strings:

extension SequenceType where Generator.Element == String {
    public func joinWithSeparator(separator: String) -> String
}

The method itself can be improved. The reduce() method creates a new
NSMutableAttributedString in each iteration. Better create only one
and append all elements (similar as on your join() function):

extension SequenceType where Generator.Element: NSAttributedString {

    func join(withSeparator separator: NSAttributedString) -> NSAttributedString {
        let finalString = NSMutableAttributedString()
        for (index, string) in enumerate() {
            if index > 0 {
                finalString.appendAttributedString(separator)
            }
            finalString.appendAttributedString(string)
        }
        return finalString
    }
}

Update: The Swift language changes constantly. For the convenience
of future readers, here is an update to Swift 4 of the above code:

extension Sequence where Element: NSAttributedString {

    func join(withSeparator separator: NSAttributedString) -> NSAttributedString {
        let finalString = NSMutableAttributedString()
        for (index, string) in enumerated() {
            if index > 0 {
                finalString.append(separator)
            }
            finalString.append(string)
        }
        return finalString
    }
}

Also flatMap has been renamed to compactMap:

let attributedStrings = optionalAttributedStrings.compactMap { $0 }


Integer Log2 implemented using binary search I saw a simple method to calculate Log2(N), where N is the power of 2:

unsigned int Log2(unsigned int N)
{
    unsigned int n = 0;
    while (N >>= 1)
    {
        ++n;
    }
    return n;
}

It would be not likely very necessary, but I try to improve it in a simple way, the idea comes from binary search.

unsigned int Log2New(unsigned int N)
{
    unsigned int bits = sizeof(N) * 4;
    unsigned int n = 0;
    while (N > 1)
    {
        if (N >> bits)
        {
            N >>= bits;
            n += bits;
        }
        bits >>= 1;
    }
    return n;
}

I run the test in Visual Studio 2013, 32 bits release build and fully optimized.  When I look into generated disassembly, both methods are inlined, and there are nothing special in both function.  There is only one shr command as expected in

if (N >> bits) { N >>= bits; ... }

I test both of the method by sending 1 << n, n from 0 to 31 to them, and repeat for 1000000 times, and count the time separately.

The new method is about 1.66 times faster than the old one,

Then I make a further change, expand the while loop in the new method.  (There was a word for this 'expand', but I don't remember it exactly 8-| )
To make it clear, I make a new define, and the method become:

#define CountShift(bits)  if ((N)>>(bits)) { (N)>>=(bits); (n) += (bits); }
unsigned int Log2NewExpand(unsigned int N)
{
    unsigned int n = 0;
    CountShift(16);
    CountShift(8);
    CountShift(4);
    CountShift(2);
    CountShift(1);

    return n;
}

Looking into the disassembly again, as expected, it's fully serialized, only 5 test, je shr and add commands.

Timing again, the expanded method is again about 1.66 times faster then the new one, and about 2.8 times faster than the original method.

I think there must be some more space for improvement.  Any suggestion are welcome.

I was initially skeptical of your benchmarking results because of your testing methodology:

  I test both of the method by sending 1 << n, n from 0 to 31 to them, and repeat for 1000000 times, and count the time separately.

When you're benchmarking code that contains conditional branches, it is generally not a good idea to use a predictable sequence of inputs, since this tends to ensure that branch prediction will be successful, mitigating something that is potentially one of the most significant costs in performance-sensitive code. Unless you're actually going to be using this function in a setting where the inputs (and thus the direction of the branches) are predictable, you run the risk of skewing the benchmark results. And even if the real-world inputs aren't purely random, you're testing here in a tight loop, which improves branch prediction even more.

To verify for myself, I used the outstanding Google Benchmark library. This automates most of the dirty work of creating a good benchmark. In my test harness, I just defined the necessary functions and prepared some arrays of randomly-generated numbers. The random number generation is done "off the clock" so it won't affect the measurement. Despite my earlier admonitions, though, it appears that your findings were valid; here are the results from my machine, using inputs randomly selected from a large array of randomly-generated values:

Benchmark                     Time           CPU Iterations
-----------------------------------------------------------
Log2                         21 ns         21 ns   29866667
Log2New                      10 ns         10 ns   74666667
Log2NewUnrolled               6 ns          6 ns  112000000

(As you can see from my function names, the word you were trying to think of when composing the question is "unroll". You've unrolled (or unwound) the while loop, which attempts to gain execution speed in exchange for code size, primarily by reducing the overhead of the loop itself. In this case, you've completely unwound the loop, allowing it to be eliminated entirely. This is a very common low-level optimization, although the customary caveat is that it significantly bloats the size of the code. That is true here as well, so it rarely makes sense to go to this extreme when optimizing unless the code is going to be called in a tight loop.)

I see essentially the same thing when compiling for 64-bit. Each improvement roughly corresponds to a doubling in overall performance, which is actually pretty impressive. Although it is almost certainly possible to squeeze more speed out of your code by iterative improvements, it will become increasingly difficult to do so. The magnitude of improvement will become a lot less significant, and you'll very quickly hit up against the limitations of writing code in a compiled language, where you can't manually tweak the generated assembly.

I would also advise you to consider whether it is really necessary. The benchmarks above suggest that on a mobile Haswell processor, you can compute the base-2 logarithm of in ~6 nanoseconds. Do you really need an implementation that is faster than that? Is it really worth the time it takes to develop one, test it, and maintain it?

Well, let's suppose either that you do need something faster, or perhaps that it's worth the time simply because it's fun. One possible tack is to seek out a way to completely eliminate branches. Rather than doing the shift-and-test-and-maybe-add dance each time, we'll simply do all of the shifts and bitwise-OR the results together. Then, the final answer is obtained by doing a population count (i.e., computing the Hamming weight). The code looks like this:

unsigned int Log2Fast(unsigned int N)
{
   N |= (N >> 1);
   N |= (N >> 2);
   N |= (N >> 4);
   N |= (N >> 8);
   N |= (N >> 16);
   return (PopCount(N) - 1);
}

The nice thing about a population count is that there are several highly-efficient ways to do it. The accepted answer to the linked question shows one possibility, a clever bit-twiddling trick that counts in parallel, also shown on Sean Eron Anderson's Bit-Twiddling Hacks page. This is a good general-purpose implementation. If you have a processor that supports it, you can use the POPCNT instruction (available on Intel Nehalem, AMD Barcelona, and later generations). On MSVC, the __popcnt intrinsic allows you to force this instruction to be emitted.

This gets compiled to beautiful, efficient, completely branchless code:

Log2Fast PROC
    mov ecx, DWORD PTR [N]

    mov    eax, ecx       // N |= (N >> 1)
    shr    eax, 1
    or     ecx, eax

    mov    eax, ecx       // N |= (N >> 2)
    shr    eax, 2
    or     ecx, eax

    mov    eax, ecx       // N |= (N >> 4)
    shr    eax, 4
    or     ecx, eax

    mov    eax, ecx       // N |= (N >> 8)
    shr    eax, 8
    or     ecx, eax

    mov    eax, ecx       // N |= (N >> 16)
    shr    eax, 16
    or     eax, ecx

    popcnt eax, eax       // return (PopCount(N) - 1)
    dec    eax
    ret    4
Log2Fast ENDP

Benchmark                            Time           CPU Iterations
------------------------------------------------------------------
Log2                                21 ns         20 ns   34461538
Log2New                             10 ns         10 ns   64000000
Log2NewUnrolled                      6 ns          6 ns  112000000
Log2Fast                             5 ns          5 ns   89600000
Log2Fast_Intrinsic                   4 ns          4 ns  179200000

Notice that the POPCNT instruction (the _Intrinsic variant shown here) is slightly faster than the hand-optimized bit-twiddling code. In fact, this single snapshot of the results is a bit of a lie. Log2Fast takes anywhere from 5–8 ns, varying slightly each time I run the benchmark. The version that uses the POPCNT instruction is consistently fast. If you can't guarantee that your target system supports this instruction, this implementation may not be any better than what you already have. It almost certainly would not be worth it except in a tight loop, when all of the required code is guaranteed to be hot in the cache. The POPCNT version, on the other hand, is actually shorter than your previous implementation, and that, combined with its lack of conditional branches, would make it a better general-case algorithm and possibly faster.

Can we do even better? Maybe. It's extremely hard to tell at this point, because our performance is already so good that the benchmark has virtually reached its limits. The performance differences we'll see will be almost exclusively due to noise. You'd say we need to improve our benchmark, but I'd say that we need to stop optimizing because we're done!

Some people have proposed an approach that uses a look-up table. The look-up table is implemented as an array, filled statically and compiled directly into the executable, with the intention of saving us from having to do expensive computations at runtime, since we can just index into the array and retrieve the precomputed value. The same solution also appears on the previously-linked Bit Twiddling Hacks page. What you'll notice, though, is that the look-up table is only being used to do the population count. Specifically, the look-up table contains a DeBruijn sequence, which is a well-known way to speed up various bit-counting operations, including finding the MSB, finding the LSB, and, of course, finding the total number of set bits.

We cannot reasonably use a look-up table for the shifting operations, because it would be ginormous. And, unfortunately, the population-count code we have is already so fast that it cannot be sped up with a look-up table. This is actually so interesting (and counter-intuitive) that it warrants a bit of further commentary. I tested this extensively a while back when I needed to find the position of the least-significant set bit (as part of an optimized implementation for strlen). I assumed that a look-up table would be the fastest solution, but it wasn't. Even as far back as a Pentium III, it turned out to be faster (or at least equally as fast) to simply compute the value at runtime. Sometimes, modern processors are like this—they are so fast, that it can be faster to recompute something than to store it and/or look it up. It is also a good demonstration of the fact that (micro-)optimization is often counter-intuitive, and can only be done correctly with the aid of a good benchmark test case!

I know I already said that we're pushing the limits of performance and that the code is already fast enough, but maybe there is a way to push it just a little bit faster? Or at least, maybe we can gain some style points for a truly efficient algorithm!

Let's stop and think for a moment about what we're actually trying to do. What is the log base-2 of a number? Well, it is the binary logarithm—the power to which you must raise 2 in order to obtain that number. The lightbulb might have already flashed on here, after seeing that word "binary". Computers work in binary. More specifically, numbers are stored using a binary representation. That's why your bit-shifting algorithm worked. Since binary is fundamentally based on log2, it turns out that the position of the most-significant set bit is the same as the binary logarithm of the represented value. For example, consider the number 21658123. Its binary logarithm is 24, and its binary representation is:

0001 0100 1010 0111 1010 0000 1011
↑  ↑                             ↑
|  └————— bit 24                 |
bit 31                           bit 0

The index of the first set bit, when searching from most-significant bit (MSB) to least-significant bit (LSB), is 24—exactly the same as the binary logarithm. This hints at an exceptionally simple algorithm (at least conceptually): scan through the binary representation of the number looking for the most significant set bit! As luck would have it, there is an instruction on x86 to do precisely what we want to do, and it's been available since the 386 way back in 1986. That is the BSR instruction (Bit Scan Reverse), usable in MSVC via the _BitScanReverse intrinsic and on Gnu compilers (i.e., GCC and Clang) via the __builtin_clz intrinsic (although you'll need to XOR the result with 32). And even if you're not targeting the x86, the intrinsic will likely still work for you, causing the compiler to use an architecture-provided instruction if available, or emit code to generate the equivalent result.

Here's our updated function:

unsigned int Log2_ViaBSR(unsigned int value)
{
    unsigned long result;
    _BitScanReverse(&result, static_cast<unsigned long>(value));
    return result;
}

(Yes, code that should be a single line looks a little bit bloated here because MSVC's _BitScanReverse intrinsic works only with DWORD values, which is a typedef for unsigned long, not unsigned int, so although on Windows they are both 32-bit types, they are nevertheless different types according to the language standard, so you need to declare and use an additional temporary for the output, instead of simply reusing the input parameter. Fortunately, this does not affect the object code generated by the compiler. Note, also, that the intrinsic returns a Boolean value indicating whether it was successful. This allows you to handle the case where the input is 0 and the result is undefined—see "caveats" below.)

And the updated leaderboard:

Benchmark                            Time           CPU Iterations
------------------------------------------------------------------
Log2                                22 ns         21 ns   32000000
Log2New                             10 ns         10 ns   64000000
Log2NewUnrolled                      6 ns          6 ns  112000000
Log2Fast                             7 ns          6 ns   89600000
Log2Fast_Intrinsic                   4 ns          4 ns  186666667
Log2_ViaBSR                          2 ns          2 ns  298666667

Stop and think about this for a second. We went from an O(log n) algorithm to what is effectively an O(1) algorithm, just by thinking about the problem in a different way, and figuring out how we can leverage the computer's internal binary representation to our advantage. It reminds me of a lesson straight out of Michael Abrash's Zen of Assembly Language. We now have the most efficient algorithm and have earned those style points!

Now, there are a couple of caveats. First off, log2(0) is mathematically undefined, so you have to decide how you want to handle that. I've simply ignored it in all of the code I've written, making it undefined there as well. The BSR instruction and its corresponding intrinsics take the same tack, formally defining the result as "undefined" when the input is 0. And although there is actually a lot more that I could say about this (including possible workarounds for "undefined" inputs, inefficient code generation when this intrinsic is used in MSVC, the similar LZCNT instruction introduced on Intel Haswell, etc.), I probably shouldn't make this answer any longer. Instead, I'll refer you to Peter Cordes's recent answer on the subject, and my comments below it.


Importing tab delimited file into array I am needing to import a tab delimited text file that has 11 columns and an unknown number of rows (always minimum 3 rows).

I would like to import this text file as an array and be able to call data from it as needed, throughout my project. And then, to make things more difficult, I need to replace items in the array, and even add more rows to it as the project goes on (all at runtime).

Hopefully someone can suggest code corrections or useful methods. I'm hoping to use something like the array style sMyStrings(3,2), which I believe would be the easiest way to control my data.

Imports System.IO
Imports Microsoft.VisualBasic.FileIO

Public Class Main
    Dim strReadLine As String

Private Sub Form1_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load
    Dim sReader As IO.StreamReader = Nothing
    Dim sRawString As String = Nothing
    Dim sMyStrings() As String = Nothing
    Dim intCount As Integer = -1
    Dim intFullLoop As Integer = 0

    If IO.File.Exists("C:\MyProject\Hardware.txt") Then     ' Make sure the file exists
        sReader = New IO.StreamReader("C:\MyProject\Hardware.txt")
    Else
        MsgBox("File doesn't exist.", MsgBoxStyle.Critical, "Error")
        End
    End If

    Do While sReader.Peek >= 0  ' Make sure you can read beyond the current position
        sRawString = sReader.ReadLine()     ' Read the current line
        sMyStrings = sRawString.Split(New Char() {Chr(9)})      ' Separate values and store in a string array

        For Each s As String In sMyStrings      ' Loop through the string array
            intCount = intCount + 1             ' Increment
            If TextBox1.Text <> "" Then TextBox1.Text = TextBox1.Text & vbCrLf ' Add line feed
            TextBox1.Text = TextBox1.Text & s   ' Add line to debug textbox
            If intFullLoop > 14 And intCount > -1 And CBool((intCount - 0) / 11 Mod 0) Then
                cmbSelectHinge.Items.Add(sMyStrings(intCount))
            End If
        Next
        intCount = -1
        intFullLoop = intFullLoop + 1
    Loop
End Sub

First I'd like to mention a style point. Ditch the hungarian notation. You don't need it. It often isn't maintained, so they tend to lie. when they are, it's a bit of a pain to find/replace your variable names. sReader should just be reader, intCount should just be count and so on.

I like that you're taking advantage of the single line variable declaration & assignment.

You should be using a few constants for your data file though. This reduces code duplication and makes it easier to change down the road.

const dataFilePath As String = "C:\MyProject\"
const dataFileName As String = "Hardware.txt"
const dataFile As String = dataFilePath & dataFileName

Now your code would look like this.

If IO.File.Exists(dataFile) Then     ' Make sure the file exists
    reader = New IO.StreamReader(dataFile)
Else
    '....

I don't see anything inherently wrong about your method of processing the file line by line. It's essentially no different from working in a 2 dimensional array. You may want to find out if you can read the whole file at once and use one anyway. I bet there would be a performance benefit.

What I do notice about your loops is the heavy reliance on Magic Numbers.

Go ahead and declare another constant TabChar and set it equal to Chr(9).
And here:

        If intFullLoop > 14 And intCount > -1 And CBool((intCount - 0) / 11 Mod 0) Then
            cmbSelectHinge.Items.Add(sMyStrings(intCount))
        End If

What is 14? What is 11? Find appropriate names for them. It makes your code easier to read and easier to change/maintain later.

Further Reading on Naming Conventions


Getting better speed from adjusting rows in a DockableToolBar I've been using FlexLib for its dockable toolbar feature.  When you shrink the window, it will try to wrap rows if the stage width is less than the row width.  Unfortunately, it won't reverse the process when you expand the window.  To get around that, I wrote the following method to add to the class (the caller is responsible for looping through all current toolbar rows and calling this method on them).

It does the job, but it's not quite as fast as I'd like.  I'm hoping for suggestions to improve both performance and code style, if possible.

/**
 *  @private
 *  Moves ToolBars to the previous row if there is sufficient space in that row to display
 *  another ToolBar properly.
 */
private function unwrapRow(row:HBox, rowIndex:int, toolbar:VBox):void
{
    if (rowIndex == 0) {
        return;
    }
    var prevRow:HBox = HBox(toolbar.getChildAt(rowIndex-1));
    var prevRowW:int = 0;
    var totalW:int = 0;

    //First, get the width of the previous row
    for (var j:int = 0; j < prevRow.numChildren; j++)
    {
        var ch:DockableToolBar = DockableToolBar(prevRow.getChildAt(j));
        prevRowW += ch.measuredWidth;
    }
    //Second, cycle through the children of the "row of interest", moving them
    //up to the previous row if there's room.
    for (var k:int = 0; k < row.numChildren; k++)
    {
        var child:DockableToolBar = DockableToolBar(row.getChildAt(k));
        if ((prevRowW + child.measuredWidth) >= toolbar.width) {
            break;
        }
        //Move child.
        child.parent.removeChild(child);
        prevRow.addChild(child);
        prevRowW += child.measuredWidth;
        --k;
    }
    if (row.numChildren == 0) {
        toolbar.removeChild(row);
    }
}

You can see the rest of this class here (that listing doesn't include my code; I plan to submit this as a patch once it's in good enough shape).

I have one little suggestion with regard to style. It is much better to invert your conditions to have the simpler code run first when it involves yielding control. It also helps you avoid excessive indentation.

var child:DockableToolBar = DockableToolBar(row.getChildAt(k));

//Skip the rest of the row if we can't move the first child up.
if (prevRowW + child.measuredWidth >= toolbar.width) {
    break;
}

//Move child.
child.parent.removeChild(child);
prevRow.addChild(child);
prevRowW += child.measuredWidth;
--k;


UPenn CIS 194 Homework 2: Log file parsing I am working through UPenn CIS 194: Introduction to Haskell (Spring 2013). Since I am not able to take the course for real I am asking for CR (feedback) as it could be from teacher in that course.

HW2 - Log file parsing - Full description

{-# OPTIONS_GHC -Wall #-}
module LogAnalysis where

import Log

parseMessage :: String -> LogMessage
parseMessage x = case words x of
                      "E":s:t:m -> LogMessage (Error (read s::Int)) (read t::Int) (unwords m)
                      "W":t:m -> LogMessage Warning (read t::Int) (unwords m)
                      "I":t:m -> LogMessage Info (read t::Int) (unwords m)
                      r -> Unknown (unwords r)

parse :: String -> [LogMessage]
parse x = case lines x of
                f:xs -> [parseMessage f] ++ parse (unlines xs)
                [] -> []

insert :: LogMessage -> MessageTree -> MessageTree
insert (Unknown _) t = t
insert m Leaf = (Node Leaf m Leaf)
insert message@(LogMessage _ t1 _) (Node l root@(LogMessage _ t2 _) Leaf)
  | t1 > t2 = (Node l root (Node Leaf message Leaf))
insert message@(LogMessage _ t1 _) (Node Leaf root@(LogMessage _ t2 _) r)
  | t1 <= t2 = (Node (Node Leaf message Leaf) root r)
insert message@(LogMessage _ t1 _) (Node l root@(LogMessage _ t2 _) r)
  | t1 > t2 = (Node l root (insert message r))
  | otherwise = (Node (insert message l) root r)

build :: [LogMessage] -> MessageTree
build [] = Leaf
build (x:xs) = insert x (build xs)

inOrder :: MessageTree -> [LogMessage]
inOrder Leaf = []
inOrder (Node left message right) = (inOrder left) ++ [message] ++ (inOrder right)

whatWentWrong :: [LogMessage] -> [String]
whatWentWrong log = (filterEmpty (errorsWithSeverity50 (inOrder (build log))))

errorWithSeverity :: LogMessage -> Int -> String
errorWithSeverity (LogMessage (Error s) _ m) severity
  | s >= severity = m
  | otherwise = ""
errorWithSeverity _ _ = ""

errorsWithSeverity50 :: [LogMessage] -> [String]
errorsWithSeverity50 [] = []
errorsWithSeverity50 (x:xs) = [(errorWithSeverity x 50)] ++ errorsWithSeverity50 xs

filterEmpty :: [String] -> [String]
filterEmpty [] = []
filterEmpty ("":xs) = filterEmpty xs
filterEmpty (x:xs) = [x] ++ filterEmpty xs

parseMessage

The main problem here is that you are calling unwords on the result of words ..., and this doesn't preserve the original whitespace.

For instance, the last case should just be:

case words x of
   ...
   _   -> Unknown x   -- use the original message text

and you have a similar problem with unwords m.

Unfortunately, to preserve whitespace in these cases I don't think you will be able to use the words function here.

It is customary to have parens around the list patterns, e.g.:

parseMessage x = case words x of
                      ("E":s:t:m) ->  LogMessage (Error (read s::Int)) (read t::Int) (unwords m)

Even though they are not needed, they aid in reading the code.

Also, aligning the -> in the case patterns helps with readability.

The type signatures on your read calls are not needed due to Haskell's type inference.

insert

You should be getting this warning from ghc and ghci:

LogAnalysis.hs:19:1: Warning:
    Pattern match(es) are non-exhaustive
    In an equation for ‘insert’:
        Patterns not matched:
            (LogMessage _ _ _) (Node (Node _ _ _) (Unknown _) _)
            (LogMessage _ _ _) (Node Leaf (Unknown _) _)

This means that your code doesn't cover all of the possible cases.

Append to your question an updated version of insert which doesn't produce any warnings, and then I might have some further comments about it.

(Please append to your question instead of modifying it so it doesn't invalidate any existing comments.)

Let me know if you are not seeing this warning from ghc / ghci so we can figure out what's going on with your environment.

filterEmpty

I don't think the logic is right here. What if you have this message:

LogMessage (Error 62) 23 ""

Shouldn't the empty message be included in the message list?


how to allow user to input results in Array using java? I want to allow the user to enter a number of race results (A minimum of 2 participants and a maximum of 7) I feel that I could use a much shorter code, (I am a beginner of Java only 2 months so any recommendation would be highly useful! thank you
Code below:

int entrants = 0;
String[][] runners = new String[2][2];
boolean valid = false; //this is for the initial number of runners question - it will stay false and keep us in the loop until a number between 2-7 is entered
System.out.println("Please enter the number of runners for this race (2 to 7 entrants permitted)");
entrants = input.nextInt();
input.nextLine();

while(valid == false){

    if(entrants < 2) {
        System.out.println("You need at least 2 runners for a race, please re-enter the number of runners");
        entrants = input.nextInt();
        input.nextLine();
    } else if (entrants > 7) {
        System.out.println("You have entered too many runners for this race (max runners is 7). Please re-enter the number of runners");
        entrants = input.nextInt();
        input.nextLine();
    } else {
        System.out.printf("You have entered %d runners\n", entrants);
        runners = new String[entrants][2];
        valid = true;
    }
}

You have a lot of lines that do the same thing and which with a little rearranging can be consolidated:

int entrants = 0;
String[][] runners = null; //no need to create an array that you'll be throwing away
boolean valid = false; //this is for the initial number of runners question - it will stay false and keep us in the loop until a number between 2-7 is entered
System.out.println("Please enter the number of runners for this race (2 to 7 entrants permitted)");

do {
  //read the input
  entrants = input.nextInt();
  input.nextLine();

  //validate (we'll keep your messages)
  if(entrants < 2) {
    System.out.println("You need at least 2 runners for a race, please re-enter the number of runners");
  } else if (entrants > 7) {
    System.out.println("You have entered too many runners for this race (max runners is 7). Please re-enter the number of runners");
  } else {
    System.out.printf("You have entered %d runners\n", entrants);

    //create the array and set the exit condition
    runners = new String[entrants][2];
    valid = true;
  }
} while (!valid);

Note the use of a do-while which is like a while-loop but evaluates the condition after an iteration, i.e. the loop body is run at least once.

Further improvements could include putting the constraints into constants (i.e. min = 2 and max = 7) instead of having them in all over your code.


Python file reader/writer, with processing pipeline and context manager The following code does three things:

The filename is determined by the RawData class
The reader and writer send data through a processing pipeline
(Hopefully) makes changes and extensions easy

Regarding (2), the processing pipeline consists of "processors", that can process and deprocess data.  For example, if you run the code below and look in ortho_animals.txt, you'll see the JSON version of a dict with (keys, values) reversed.

I left comments out to keep the size of the code to a minimum.  Please let me know what you think, with a particular eye on the class designs and interactions.

import contextlib
import json

class Processor(object):

  @classmethod
  def process(cls, data):
    raise NotImplementedError

  @classmethod
  def deprocess(cls, data):
    raise NotImplementedError

class JsonProcessor(Processor):
  @classmethod
  def process(cls, data):
    return json.dumps(data)

  @classmethod
  def deprocess(cls, data):
    return json.loads(data)

class SwapProcessor(Processor):
  @classmethod
  def process(cls, data):
    return cls.swap(data)

  @classmethod
  def deprocess(cls, data):
    return cls.swap(data)

  @classmethod
  def swap(cls, data):
    return {v: k for k, v in data.iteritems()}

class AggregateProcessor(Processor):
  def __init__(self, processors):
    self.processors = processors

  def process(self, data):
    return reduce(lambda d, kls: kls.process(d), self.processors, data)

  def deprocess(self, data):
    return reduce(lambda d, kls: kls.deprocess(d), self.processors[::-1], data)

class RawReader(object):
  def __init__(self, file_obj, processor):
    self._file = file_obj
    self._processor = processor

  def read(self):
    data = self._file.read()
    return self._processor.deprocess(data)

class RawWriter(object):
  def __init__(self, file_obj, processor):
    self._file = file_obj
    self._processor = processor

  def write(self, data):
    new_data = self._processor.process(data)
    self._file.write(new_data)

class RawData(object):
  processors = [SwapProcessor, JsonProcessor]
  aggregator = AggregateProcessor(processors)

  def __init__(self, obj_type, key):
    self.obj_type = obj_type
    self.key = key

  def _build_filename(self):
    return self.obj_type + '_' + self.key + '.txt'

  @contextlib.contextmanager
  def writer(self):
    filename = self._build_filename()
    with open(filename, 'w') as f:
      yield RawWriter(f, self.aggregator)

  @contextlib.contextmanager
  def reader(self):
    filename = self._build_filename()
    with open(filename, 'r') as f:
      yield RawReader(f, self.aggregator)

if __name__ == '__main__':
  data = {'cats': 'suck', 'dogs': 'rule'}
  rd = RawData('ortho', key='animals')

  with rd.writer() as writer:
    writer.write(data)

  with rd.reader() as reader:
    new_data = reader.read()

  assert(new_data['cats'] == data['cats'])

This code looks very nice! I admire how clean and organized your code is. It appears that you tried to think through how to arrange it, and I really appreciate that.

There are really only a few areas that I see things could be improved: the speed of execution, the robustness of the code when a user attempts to supply invalid data, and the verbiage for readability when others must see this code.

Efficiency

I had to look it up, but I remember reading about how reducing all of your code that follows if __name__ == '__main__': into a single main() function is good practice. After searching around I remember where I read it, it was in the Python Cookbook (3rd edition) by O'Reilly Media. The authors David Beazley and Brian K. Jones wrote:

  A little-known fact is that code defined in the global scope like this runs slower than code defined in a function. The speed difference has to do with the implementation of local versus global variables (operations involving locals are faster) ... The speed difference depends heavily on the processing being performed, but in our experience, speedups of 15-30% are not uncommon

Robustness

I can see in a couple places where someone using this code may (whether accidentally or with malicious intent) break it and cause an error, or perhaps not cause an error but make it do something not designed. You may want to consider adding checks throughout your code, so that if say a value in a dict is of a type other than a string, then you know something is wrong and you shouldn't continue with the swap function. I realize adding Exception handling throughout or if/then branches will make for less clean and readable syntax, but it is an area where you may need to take this next if you plan to put this into a production environment.

Readability

Think about how this code will look if someone else has to get up in the middle of the night when something breaks, and half conscious and under duress, go through and figure out where a problem might be. (I think my friend and QA Guru Jon Gluck called it "Drunk Proofing" your code, so even the intoxicated who get a late night emergency call can fix the problem.)

As an example, consider the word "processing." This is a general enough word that you could use it almost anywhere in your code, and across multiple classes and functions even though different uses of this word may be completely unrelated. It reminds me of "college speak" for the word "do." Imagine replacing the word "process" (including "processing" and "processor" and "deprocessor" with the words "doing", "doer" and "undoer", respectively). I don't have a suggestion for what to replace the word process with, and maybe it truly is the best and only word to use here (just as 'handler' is often the best word to use in many cases), but just be careful. Be especially careful if you have used these same terms in other parts of your code that could cause significantly more confusion if someone attempts to wade through a stack trace to troubleshoot an error.

Caveat

Please understand that I think your code is quite beautiful, and frankly I'm not sure my code would look this good if I were tasked with the same project. Regardless, since you posted in code review asking for suggestions, I am trying to poke some holes in it, and make suggestions of where to go from here.

Great job, and keep it up!


Is the new version of this code more difficult to read and understand than the original? How can it be improved? I was asked to do a code review for the following block of code. It implemented a bug fix to prevent some values being added to some drop down lists depending on the user's domain, in an ASP.NET WebForms app. The three lines adding items to the lists were from the pre-bug-fix code (not part of the fix), so I didn't review it.

        //Only add corporate AD domain if the user is a member of it.
        string domainName = "";
        if (Thread.CurrentPrincipal == null)
        {
            //"Missing the thread identity";
        }
        else if (Thread.CurrentPrincipal.Identity.Name.IndexOf("\\") < 0)
        {
            //"No domain detected leave domain name blank.";
        }
        else
        {
            domainName = Utils.GetUserDomain(Thread.CurrentPrincipal).ToUpper();
        }

        if (domainName == Config.AppSettings.Domain.ToUpper())
        {
              domainUserDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain, Config.AppSettings.Domain));
              domainOfficeDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain, Config.AppSettings.Domain));
              domainTypeDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain, Config.AppSettings.Domain));
        }

I reviewed the code and suggested changing the code to the following:

        //Only add corporate AD domain if the user is a member of it.
        var currentPrincipal = Thread.CurrentPrincipal;
        var hasDomain = currentPrincipal != null && currentPrincipal.Identity.Name.Contains("\\");
        var domainName = hasDomain ? Utils.GetUserDomain(currentPrincipal) : null;

        if (string.Equals(domainName, Config.AppSettings.Domain, StringComparison.OrdinalIgnoreCase))
        {
              domainUserDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain, Config.AppSettings.Domain));
              domainOfficeDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain, Config.AppSettings.Domain));
              domainTypeDropDownList.Items.Add(new ListItem(Config.AppSettings.Domain, Config.AppSettings.Domain));
        }

When the original developer saw the changes, he agreed with some of it (change of IndexOf to Contains, and ToUpper to String.Equals for case insensitive search), but not all of it. His main complaint was that the new code was more difficult to read and understand, and that the if blocks were clearer than the ternary operator. We weren't able to come to a point of agreement.

Could you please review both the original and new code and suggest improvements to either. Also, please point out any problems in the new code, especially where you think it is worse than the original code.

Disclaimer: I'm not too familiar with C#.

The modified form has expressions that are a little too complex for me, but either would be ok. My only comment is that the initial comment line is screaming for some notice:

// Only add corporate AD domain if the user is a member of it.

Why not take all the tests and put them into a function?

if ( IsMemberOfCorporateDomain( Thread.CurrentPrincipal ) ) ...

Then, depending on how you both feel about early exits:

private bool IsMemberOfCorporateDomain( IPrincipal currentPrincipal )
{
    if ( currentPrincipal == null ) return false;
    if ( !currentPrincipal.Identity.Name.Contains("\\") ) return false;

    var domainName = Utils.GetUserDomain(currentPrincipal);
    return string.Equals( domainName, Config.AppsSettings.Domain, StringComparison.OrdinalIgnoreCase )
}


Calling stored procedure with a sqlCommand I have the following code as part of an sqlCommand which is calling a stored procedure:

Dim groupObj

If groupId = 0 Then
    groupObj = DBNull.Value
Else
    groupObj = groupId
End If

Dim siteObj

If siteId = 0 Then
    siteObj = DBNull.Value
Else
    siteObj = siteId
End If

sqlCommand.Parameters.Add(New SqlClient.SqlParameter("GroupID", SqlDbType.Decimal)).Value = groupObj
sqlCommand.Parameters.Add(New SqlClient.SqlParameter("SiteID", SqlDbType.Decimal)).Value = siteObj

I basically want to check for a condition and (if true) send a DBNull instead.  This code looks awful however - is there a way to succinct this up?

You can use the ternary operator If():

sqlCommand.Parameters.Add(New SqlClient.SqlParameter("GroupID", SqlDbType.Decimal)).Value = If(groupID = 0, DBNull.Value, groupID)
sqlCommand.Parameters.Add(New SqlClient.SqlParameter("SiteID", SqlDbType.Decimal)).Value = If(siteId = 0, DBNull.Value, siteId)

Be aware that this can cause a compiler error if you have Option Strict turned on. The following is Option Strict compatible...

sqlCommand.Parameters.Add(New SqlClient.SqlParameter("GroupID", SqlDbType.Decimal)).Value = If(groupID = 0, DBNull.Value, CObj(groupID))
sqlCommand.Parameters.Add(New SqlClient.SqlParameter("SiteID", SqlDbType.Decimal)).Value = If(siteId = 0, DBNull.Value, CObj(groupID))

The If ternary operator was added in .Net 3.5/Visual Basic 2008. If you're using and older version you'll need to use the IIF function. The syntax is almost the same, just replace if with iif

sqlCommand.Parameters.Add(New SqlClient.SqlParameter("GroupID", SqlDbType.Decimal)).Value = IIF(groupID = 0, DBNull.Value, groupID)
sqlCommand.Parameters.Add(New SqlClient.SqlParameter("SiteID", SqlDbType.Decimal)).Value = IIF(siteId = 0, DBNull.Value, siteId)


Maintain sync of select list My objective has been to build a custom dropdown like the one on Amazon's search scope selector. I do this by positioning a styleable element on top of a standard select list. I then use jQuery to keep the styleable element in sync with the select list. My code works fine, but I would love to learn how to do it cleaner. Currently, I do the syncing twice - on page load and on change. Is it possible to refactor my code using on()? (or similar).

prev() is the styleable element.

// Sync is performed on page load
var $select = $('select');
$select.each(function(index, value) {
  var optionText = $(this).find('option:selected').text();
  $(this).prev().text(optionText);
});

// Sync is performed on change event
$('select').change(function() {
  var optionText = $(this).find('option:selected').text();
  $(this).prev().text(optionText);
});

Instead of rewriting the code you can just trigger the change event after you bind it which will cause it to sync on page load

$('select').change(function() {
  var optionText = $(this).find('option:selected').text();
  $(this).prev().text(optionText);
}).trigger('change'); // <-- trigger change


Optimizing Java SHA-512 String-to-Hash Generator In an attempt to learn Java and SHA-2 I created a very simple String-to-SHA512 generator. Here is the code:

package main;
import java.util.Scanner;
import java.math.BigInteger;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;

public class SHA512_String_Hash {
    public static void main(String args[]) throws NoSuchAlgorithmException {
        Scanner inputScanner = new Scanner(System.in);
        System.out.print("Text Input: ");
        String input = inputScanner.next();

        MessageDigest objSHA = MessageDigest.getInstance("SHA-512");
        byte[] bytSHA = objSHA.digest(input.getBytes());
        BigInteger intNumber = new BigInteger(1, bytSHA);
        String strHashCode = intNumber.toString(16);

        while (strHashCode.length() < 128) {
            strHashCode = "0" + strHashCode;
        }
        System.out.println("SHA-512: \n" + strHashCode);
    }
}

Since I am new to SHA-512 in Java, I do not know the optimal (for performance and more importantly security) way to create a SHA-512 hash generator.

TLDR— How could the performance and security of this code be improved?

You should extract the hex encoding into its own method.

See How to convert a byte array to a hex string in Java? for various implementations.
input.getBytes() uses the locale dependent legacy encoding. So you won't get reproducible results on different systems. Specify an encoding explicitly, I recommend utf-8.


Building an efficient bubble sort function I've been asked to create a bubble sort function that checks whether the array has been filled sorted before all passes are complete, thereby avoiding unnecessary passes. This is to involve "remembering" the last sort.

Below is the code I have for just a normal bubble sort that goes through all passes, and is quite inefficient. The parameter variables are 'a' for the generic array itself and n, which is the number of passes. In this case we're doing a full set of passes for the array, which is 10 integers in length.

public static void main(String[] args) {

    Integer[] myArray = new Integer[10];
    System.out.println("Iterative Bubble Sort - Ch 8, Ex 8a on page 265");
    generateRandonNumbers(myArray);
    display(myArray);
    bubbleSort(myArray,10);
    display(myArray);
    System.out.println("Recursive Bubble Sort - Ch 8, Ex 8b on page 265");
    generateRandonNumbers(myArray);
    display(myArray);
    recursiveBubbleSort(myArray,10);
    display(myArray);
    System.out.println("Better Bubble Sort - Ch 8, Ex 10 on page 266");
    generateRandonNumbers(myArray);
    display(myArray);
    betterBubbleSort(myArray,10);
    display(myArray);

}
public static void display(Integer[] array)
{
    for(int i=0; i<array.length; i++)
    {
        System.out.print(array[i] + " ");
    }
    System.out.println();
}
public static void generateRandonNumbers(Integer[] array)
{
    Random randomNum = new Random();
    for(int i=0; i<array.length; i++)
    {
        array[i] = randomNum.nextInt(100);
    }
}
public static <T extends Comparable<? super T>> void bubbleSort(T[] a, int n)
{
    int k = 0;
    while (k <= n)
    {
        k++;

    for (int i = 0; i < a.length - k; i++)

            {
                if (a[i].compareTo(a[i+1]) > 0)
                {
                    T data = a[i];
                    a[i] = a[i + 1];
                    a[i + 1] = data;

                }
            }
    }
}

The test results are:

9 36 59 60 40 28 71 95 90 56

(unsorted, displaying all values from the array)

9 28 36 40 56 59 60 71 90 95

(now sorted from the bubbleSort function)

How do I go about turning this into a more efficient sort function by avoiding unnecessary passes? I'm supposed to get the program to remember the last sort and I'm not sure how to go about doing this.

Instead of using a regular for loop to loop through array:

for(int i=0; i<array.length; i++)
{
    System.out.print(array[i] + " ");
}

you can use a nicer foreach loop:

for (int i : array)
{
    System.out.print(i + " ");
}


Process monitoring script I wrote a script which logs memory usage of a process (pid in file), and reacts in case a limit is reached. I run the script nightly. In my case, the process concerned propably has a memory leak and locks up the whole system after a couple of days. Hence, the script should reboot the system if excess memory is used. Logged memory usage values are formated to have 1 decimal place.

The code works, but I'd like to know if and how I could optimize it, and if there are no mistakes.

#!/bin/sh

# process_monitor.sh
#
# monitor specific process
# customized for memory usage monitor (can easily be changed)
# customized to perform system reboot if limit is exceeded (can easily be changed)
# by geohei <xxxx@xxxx.xx>
# created : 14.10.2016
# revised : n/a
#
# use cron to trigger (e.g. nightly at 04:15 '15 4 * * * ~/process_monitor.sh'
# /bin/bc is used (possibly not installed by default)
#
# $pidfile must include path
# $logpath is used to save 'process_monitor.log' file (omit trailing '/')
# $pidlimit can be floating point (e.g. 14.1)

pidfile='/tmp/process.pid'
logpath='~'
pidlimit=20.0

pidvalue=$(ps -p $(eval "cat $echo $pidfile") -o %mem --no-headers)

if [ "$(echo "${pidvalue} > ${pidlimit}" | bc)" -eq 1 ]; then
    # log $pidvalue, $pidlimit and reboot system
    echo "$(date '+%Y-%m-%d %H:%M:%S')   pidvalue: $pidvalue, pidlimit: $(echo $pidlimit | awk '{printf "%.1f\n", int($1)}') - reboot" >> $logpath/process_monitor.log
    shutdown -r -t 10
    exit 1
else
    # log $pidvalue, $pidlimit
    echo "$(date '+%Y-%m-%d %H:%M:%S')   pidvalue: $pidvalue, pidlimit: $pidlimit" >> $logpath/process_monitor.log
fi

exit 0

That's how the log looks like:

...
2016-10-14 04:15:04   pidvalue: 19.2, pidlimit: 20.0
2016-10-15 04:15:03   pidvalue: 20.4, pidlimit: 20.0 - reboot
...

Any remarks, comments or suggestions?

Don't write custom scripts for common sysadmin tasks. Using the proper tools will get the job done better, and the solution will be more maintainable — especially by any colleagues you might have.

Using monit, for example, this would just be a matter of writing a simple configuration file.

set logfile /root/process_monitor.log
check process myproc with pidfile /tmp/pidfile.pid
    restart program = "shutdown -r -t 10"  # draconian!
    every "15 4 * * *"
    if memory > 20 MB then restart myproc