Zip the contents of subfolders, conditionally I've written a script in Powershell for zipping files into new folders.

This script performs the following steps:

Check whether each subfolder contains non-zip files which are older than 31 days
If so, create a new folder within, labelled with the date
Move the old files to the newly-created folder
Create a zipped copy of said folder
Delete the unzipped source folder

If I could get some feedback, that'd be great!

$initpath = get-location #determine initial location, in order to return later
foreach($path in get-childitem -recurse) { #get all subfolders
if ($path.Attributes -eq "Directory") {
    set-location $path.FullName
    $date = "$((Get-Date).ToString('yyyy-MM-dd_hh-mm'))"
    $newpath = [io.path]::combine("$path$date","")

    #get all items older than 31 days, exclude zip files and folders
    $items = Get-ChildItem -Exclude *.zip | ? {$_.LastWriteTime -lt (Get-date).AddDays(-31) -and -not $_.psIsContainer}

    #verify that there are such items in this directory
    if ( $(Try { Test-Path $items } Catch { $false }) ) {
        $newfld = New-Item -ItemType Directory -name $newpath

        Move-Item $items -destination $newpath #move items to newly-created folder

        #the following block zips the folder
        $src = $newfld.FullName
        $dest = "$src.zip"
        [Reflection.Assembly]::LoadWithPartialName( "System.IO.Compression.FileSystem" )
        [System.IO.Compression.ZipFile]::CreateFromDirectory($src, $dest)

        Remove-Item $src -force -recurse #remove the unzipped source folder
    }
    Else {
    Continue #continue to the next folder if there are no relevant files present
    }

    }
}
set-location $initpath #return to the initial location

Path Location

I see that you're trying to save the current location and then restore it later.

There are actually built-in cmdlets for this, Push-Location and Pop-Location. They work the same way as pushd and popd if you're familiar with those from the regular Windows command prompt for from unix-like systems (and in fact those terms are aliases in PowerShell and still work).

So you can actually just do this:

Push-Location  # saves the current location
# do some stuff
Pop-Location   # restores the original location

You can also give a location to change to, so for example instead of doing Set-Location later, you can do something like this:

$dirToChangeTo = 'C:\Windows'
Push-Location $dirToChangeTo
# do stuff in the new location
Pop-Location   # original location restored

Handling Errors

There's a small problem with any of these methods though: if your script doesn't execute all the way through for any reason, your original location will not be restored. For that you can wrap your entire script in a try/finally block:

Push-Location
try {
    # all your code here
} finally {
    Pop-Location
}

The idea with this is that the code in finally {} should execute always (if a fatal exception occurs, if you stop the script, press CTRL C, etc.), so you can be fairly certain that your original location will get restored. I do this all the time when dealing with the SQLPS module because it always changes the location to the SQLPS drive.

Avoiding this altogether

Of course you really don't have to change the current location. The only place you're using it is where you're finding the zip files, but you could have just passed the current directory into the -Path or -LiteralPath parameter of Get-ChildItem and then you would avoid the need to worry about changing it. I tend to prefer this method personally.

Iterating Directories

The Pipeline

foreach($path in get-childitem -recurse) { #get all subfolders

Get-ChildItem is a pipeline cmdlet, so I would typically recommend you stay with the pipeline, as it tends to be a bit more natural in PowerShell:

Get-ChildItem -Recurse | ForEach-Object {
    $path = $_
}

Getting Directories Only

if ($path.Attributes -eq "Directory") {

You don't need to test for a directory after the call. If you're using PowerShell 3 or higher, you can just do:

Get-ChildItem -Directory -Recurse | ForEach-Object {
    $path = $_  # this will be a directory
}

If you're using v2 you can still keep it in the pipeline but it's a bit more verbose:

Get-ChildItem -Recurse | Where-Object { $_.PSIsContainer } | ForEach-Object {
    $path = $_  # this will be a directory
}

Date Formatting

$date = "$((Get-Date).ToString('yyyy-MM-dd_hh-mm'))"

There's no need to embed this in a string; calling .ToString() will already give you one!

Get-Date has a -Format parameter that takes the same format string as .ToString() so you could just do:

$date = Get-Date -Format 'yyyy-MM-dd_hh-mm'

If you prefer .net objects, you could do it without any cmdlets:

$date = [DateTime]::Now.ToString('yyyy-MM-dd_hh-mm')

12 vs 24 hour clock?

This may be culture dependent and I'm not sure where you are, but on my system 'hh-mm' will give me the 12-hour time, and you're not including AM/PM in your string.

So right now for me it's 8:14 PM and Get-Date -Format 'hh-mm' gives me 08-14. It would give me the same thing in 12 hours though. 'HH-mm' will give me 20-14 which is probably what you want to do here instead of embedding am/pm, but if you did I believe the format code is tt:

Get-Date -Format 'hh-mm tt'

Path Combining

$newpath = [io.path]::combine("$path$date","")

I'm not sure what you're doing here. This is the same thing as:

$newpath = "$path$date"

[System.IO.Path]::Combine() is a way to combine multiple path components without having to concatenate them yourself (and deal with leading/trailing path separators). Am empty string as one of the parameters effectively gets ignored.

It's a great function to know though. There is also the Join-Path cmdlet which does the same thing but in a more "PowerShell" style of course.

Aliases and Item Type

$items = Get-ChildItem -Exclude *.zip | ? {$_.LastWriteTime -lt (Get-date).AddDays(-31) -and -not $_.psIsContainer}

Aliases

? is an alias for Where-Object. Try to avoid aliases in scripts you intend to reuse, especially if they'll be shared or read by others.

Item Type

As above, if you're using PS3+ you can use Get-ChildItem -File to get only files, otherwise you're already doing the right thing with .PSIsContainer.

Test-Path already returns [bool]

if ( $(Try { Test-Path $items } Catch { $false }) ) {

There's no need to use try/catch here; Test-Path already returns $true or $false so you can just use it directly:

if (Test-Path $items) {

else { continue } is Unnecessary

That's what was going to happen anyway!

Adding Parameters

I definitely get the idea of this operating on the current directory, but it would be nice to be able to launch it from anywhere and tell it what directory to operate on. To do that, you add a param() block at the top of the script:

# very top of script, pretend this isn't here
[CmdletBinding()]
param(
    [ValidateScript( { $_ | Test-Path -Type Container } )]
    [String]
    $InitPath = (Get-Location)
)

# rest of code

Now you can call your script with -InitPath C:\some\directory or just leave it blank and it will use the current working directory anyway. And I threw in a parameter validation attribute to make sure that the passed in value exists and is actually a directory.

Parameterizing a script is something I tend to recommend a lot; it's something good to think about in general because it tends to make your code more versatile, testable, etc.


ResettableLazy: a resettable version of .NET Lazy I implemented a resettable version of .NET Lazy (context: mostly to use it in a MemoryCache for auto refresh purposes on item expiration)

This is a very naive implementation and I am pretty sure there are couple of things that are not okay. In particular, I am not really sure whether the reset is fine. I think the assignment is atomic but I am not sure what can happen when there is an attempt to get the Value while it is resetting at the same (e.g. I am not sure whether the fact that the assignment is atomic is enough to guarantee proper operations).

public class ResettableLazy<T>
{
    private Lazy<T> _lazy;

    public bool IsValueCreated => _lazy.IsValueCreated;

    public T Value => _lazy.Value;

    public LazyThreadSafetyMode LazyThreadSafetyMode { get; }

    private readonly Func<T> _valueFactory;

    public ResettableLazy(Func<T> valueFactory, LazyThreadSafetyMode lazyThreadSafetyMode = LazyThreadSafetyMode.ExecutionAndPublication)
    {
        _valueFactory = valueFactory;
        LazyThreadSafetyMode = lazyThreadSafetyMode;
        _lazy = new Lazy<T>(_valueFactory, LazyThreadSafetyMode);
    }

    public ResettableLazy(Func<T> valueFactory, bool isThreadSafe)
        : this(valueFactory, isThreadSafe ? LazyThreadSafetyMode.ExecutionAndPublication : LazyThreadSafetyMode.None)
    {
    }

    public void Reset()
    {
        _lazy = new Lazy<T>(_valueFactory, LazyThreadSafetyMode);
    }
}

And played a bit around with:

public static class Program
{
    public static void Main(params string[] args)
    {
        var lazy = new ResettableLazy<int>(() =>
        {
            Thread.Sleep(2000);
            return new Random().Next();
        });

        for (var i = 0; i < 1000; i++)
        {
            if (i % 2 == 0)
            {
                Thread.Sleep(200);
            }

            Task.Run(() => Console.WriteLine(lazy.Value));

            if (i % 3 == 0)
            {
                Task.Run(() => lazy.Reset());
            }
        }

        Console.ReadKey();
    }
}

Is there any red flags in my implementation?

Overall, it looks good to me.  There aren't any real threading concerns with simultaneously resetting and accessing the value.  The body of the Value property getter will either use the old _lazy or the new _lazy, but those are the only options.  Since there isn't any locking it is possible for one thread to continue to see the old value after another thread resets (due to caching a value of the _lazy reference), but this is likely not a problem in most use cases.

One thing to note is that if Lazy<T>.IsValueCreated ever becomes true it stays true for the lifetime of the object, and this is not the case with your class.  It's worth calling out that if the object is shared between threads than many of the reasons to use IsValueCreated become less reliable (e.g. accessing .Value may be expensive).


Verifying a person's age When coding I noticed I started to get into a habit of not using if-else statements when the else block only has one line of code. For example, if I have code that can be solved like this:

public Person(int initialAge) {
    if(initialAge < 0){
        System.out.println("Age is not valid, setting age to 0.");
        age = 0;
    }
    else{
        age = initialAge;
    }
   }

I will instead remove the else statement entirely to cut down on a few lines of code. This results in my code looking more like this:

public Person(int initialAge) {
    age = initialAge;
    if(initialAge < 0){
        System.out.println("Age is not valid, setting age to 0.");
        age = 0;
    }
   }

I'm wondering if this is a bad coding habit to get into and if I should break out of this coding habit or if it'll be fine to continue doing this. Would doing this be a bigger issue down the line when I get onto more complex programs?

Edit: Just wanted to give some more information for why part of the code is written the way it is. The snippet provided is part of a coding challenge where one of the requirements is to set any negative number inputted to 0.

The biggest problem I see here is that you are printing a warning message to System.out and then using a default value for something which sounds like an Exception.

Why allow negative values at all?

if (initialAge < 0) {
    throw new IllegalArgumentException("Initial age cannot be negative, was specified as " + initialAge);
}
age = initialAge;

This way it is always up to the caller to pass a valid age instead of the method defaulting to zero which will bring other problems. It is better to have your program break early because of a bug than to have a warning message be printed out which may indicate a bug.


Approximate string search in Python I have a long text (about 6,000,000 chars) and a short text (about 6,000 chars).  The long text contains the short text, but not exactly - a small number of words are missing, abbreviated or just erroneous. So I want to do an approximate search.

There are several Python libraries that do fuzzy string matching. For example, FuzzySet accepts a list of candidates and a string and returns the candidate that is most similar to the string. However, here I do not have a list of candidates - I have just a very long text.

A naive solution is to take, as the list of candidates, all substrings of the long text. However, the number of candidates will be huge. So, now I use make several heuristic assumptions:

I assume that the first several characters at the start and end of the short text are not erroneous in the long text.
I assume that the length of the best match of the short text inside the long text is at least half and at most twice the length of the short text.

These assumptions allow me to reduce the number of candidates dramatically, so all of them can be checked in a reasonable time. Here is the current solution:

import re
from fuzzyset import FuzzySet

def fuzzyFind(needle:str, haystack:str, needleStart:str, needleEnd:str)->str:
    """
    needle: short text to look for.
    haystack: long text to look in.
    needleStart: first few chars in needle,
    needleEnd  : last few chars in needle - assumed to exist verbatim in haystack.

    returns a subset of haystack that is a best match for needle.
    """
    minPossibleLength = len(needle)//2
    maxPossibleLength = len(needle)*2
    possibleStarts = [m.start() for m in re.finditer(needleStart, haystack)]
    possibleEnds =   [m.end()   for m in re.finditer(needleEnd,   haystack)]
    possibleMatches = FuzzySet()
    for iStart in possibleStarts:
        for iEnd in possibleEnds:
            possibleLength = iEnd-iStart
            if minPossibleLength <= possibleLength and possibleLength <= maxPossibleLength:
                possibleMatch = haystack[iStart:iEnd]
                print("possible match from {} to {}".format(iStart,iEnd))
                possibleMatches.add(possibleMatch)
    matches = possibleMatches.get(needle)
    bestMatch = matches[0]
    bestMatchText = bestMatch[1]
    return bestMatchText

This heuristic solution is quick and quite accurate in practice, but of course it is not guaranteed to work. I would like to get rid of the assumptions, especically assumption #1. Is there a quick solution that does not need to assume anything about the first and last characters of the needle?

Note: before writing this function I tried to use the fuzzysearch library. However, this function requires an upper bound on the Levenshtein distance between the short text and its variant inside long text. Even with modest upper bounds (e.g, 60-70) it becomes quite slow.

Since there is no answer yet - what I would try:

I would search words rather than characters. make a dictionary of words (bag of words) and count the frequency. your test bag of words should be a subset of the source bag of words mostly. by comparing to other sources you could analyze relevance of words and other measures.

The big advantage of words vs. characters is that the comparison resyncs with every whitespace. an insertion/deletion of a character will result in a single changed word. you are now talking in edit distance of words, rather than characters. so translate your strings to arrays of words (whatever representation you use for a word (sequential number, hash, reference, ...).

Now slide your test words over the source words and count the number of word matches. for your parameters this will be around 1,000 word over 1,000,000 words, which sounds reasonable. you get an array of match counts with length 1,000,999. it will have the following properties

some noise floor of matches. the most frequent words of your language match by accident
some (at least one) entries greater or equal to the longest matching subsequence
some 'distribution' (near gaussian?) caused by inserted/deleted words indicating where to find your test file

this would be less heuristic than your approach, although you probably will again apply some more algorithms/heuristics to find the 'exact' match start/end.

EDIT: I simply had to try the bag of words approach

I simply slide a window of the probe words length ofer the text words in single word steps. for this text window i calculate the bag of words and compare it to the probe and sum the absolute difference values for every entry.

e. g.: 'so the the moon' vs. 'the moon moon is' resulting (so:1 the:2 moon:1) vs. (the:1 moon:2 is:1) wich results in a diff (so:1 the:1 moon:-1 is:-1) in absolute values (so:1 the:1 moon:1 is:1) summing up to the value 4.

This measure for my test set

text size 100,000 chars (~ 16,000 words)
probe size 6,000 chars (~ 1,000 words)
probe edits (change, insert, delete) 300 chars (letters, digits, punctuation, whitespace)
bag size for a 1000 words window from text ~ 500 words with frequencies from 20+ down to 1
bag size of the (edited) probe ~ 600 words

gives the following result

This looks pretty robust to me. We see

for the best match a word-edit-distance of 460 (inserting a whitespace may results in a distance of 3)
for a non-matching region a word-edit-distance of 1200
a near linear transition double as wide as the probe size

See the transition scaled up


Find most significant bit without bitshifts I am trying to find the most significant bit of unsigned number without using any bitshifts. This is what I have come up with, which looks very naive but it's very simple:

template<typename T>
constexpr uint8_t find_msb(const T& a_variable)
{
    static_assert(std::is_unsigned<T>::value, "Variable must be unsigned");

    constexpr uint8_t number_of_bits = std::numeric_limits<T>::digits;
    const std::bitset<number_of_bits> variable_bitset{a_variable};

    for (uint8_t msb = number_of_bits - 1; msb >= 0; msb--)
    {
        if (variable_bitset[msb] == 1)
            return msb + 1;
    }
    return 0;
}

In C++17 this is a valid constexpr function and it appears it just vanishes into thin air with gcc, even with -O0: https://godbolt.org/z/pKI8fj.

But the non constexpr version is more involved: https://godbolt.org/z/eyj2bO.

In my use case I can use the constexpr version so it's not a big deal. How can I improve this code while maintaining readability and also without using any "smart" tricks and/or bitshifting?

use std::uint8_t, to avoid relying on something that may or may not exist in the global namespace.
An unsigned type will always be >= 0. So if a_variable is 0, this will result in undefined behaviour by accessing something out of bounds of the bitset (and either crash or loop forever).
If we're finding the "most significant bit", I don't think avoiding bitshifts makes it more readable. (This requires understanding of both numeric_limits::digits, and std::bitset instead).


Nsum problem: find all n elements in an array whose sum equals a given value I am pretty new to Python, so I want to listen to any advice that improve my coding style in a "pythonic" way, even about naming style of variables. The following code reflects a very general paradigm that formulates a problem as depth-first search in a tree, which I find interesting and powerful.

Suppose arr = [3, 5, 2, 1, 1, 7, 10, 2], we want to find all n=2 elements whose sum equals val=8. The algorithm should return a list of tuples, where each tuple is INDICES of n=2 elements (at different positions of the array) whose sum is val=8: [(3,5), (4,5), (0,1)], so we have,

arr[3]+arr[5] = 1+7 = 8;
arr[4]+arr[5] = 1+7 = 8;
arr[0]+arr[1] = 3+5 = 8.

Here is my code:

def nsum(arr, n, val):
    if arr is None or len(arr) < n:
        return []

    # first sort the array by indice, so that we can search sequentially
    # with early stopping
    sorted_indice = sorted(xrange(len(arr)), key=lambda i: arr[i])
    size = len(arr)

    def nsum_recursive(arr, prev_indice, prev_sum, result):
        ''' This is the main algorithm that is a recursive function.
        Think about the problem as tree search using depth-first search,
        Each node/state is determined by (prev_indice, prev_sum), where
        prev_indice is a list of currently explored indice, whose sum is
        prev_sum. When len(prev_indice)==n, we reach a leaf node. Then we
        can check whether the sum equals val.
        '''
        # termination condition (leaf nodes)
        if len(prev_indice) == n:
            if prev_sum == val:
                result.append(tuple(sorted_indice[i] for i in prev_indice))
        else:
            current_count = len(prev_indice)
            # depth-first search
            for idx in range(
                    0 if not prev_indice else prev_indice[-1]+1,
                    size-(n-current_count-1)):
                current_sum = prev_sum + arr[sorted_indice[idx]]
                # early stopping (pruning of branches)
                if current_sum>val and idx+1<size and arr[sorted_indice[idx+1]]>=0:
                    break
                nsum_recursive(arr, prev_indice+[idx],
                               current_sum, result)

    # start DFS from the root node
    result = []
    nsum_recursive(arr, [], 0, result)
    return result

def nsum(arr, n, val):

A documentation string here is important. The names of the variables are so vague that even knowing the task in advance is difficult to understand their meaning.

    if arr is None or len(arr) < n:
        return []

This check is non very useful. It does not decrease the algorithm complexity. In particular the check arr is None is wrong... you want the function to fail if the arr is not a list, otherwise you hide possible errors in the caller's code.

    # first sort the array by indice, so that we can search sequentially
    # with early stopping
    sorted_indice = sorted(xrange(len(arr)), key=lambda i: arr[i])
    size = len(arr)

invert the two lines above, so that you can use size in the previous line. Why to use xrange instead of range? The name of the variable sorted_indice is not very good for my taste: it is not "sorted" (maybe it is "sorting") and what does "indice" means? Maybe you mean "indices"?

    def nsum_recursive(arr, prev_indice, prev_sum, result):

Here the complexity is too much... we have four variables plus the three of the enclosing function: very difficult to keep track of all of them. arr is repeated. result should be the return value.

        ''' This is the main algorithm that is a recursive function.
        Think about the problem as tree search using depth-first search,
        Each node/state is determined by (prev_indice, prev_sum), where
        prev_indice is a list of currently explored indice, whose sum is
        prev_sum. When len(prev_indice)==n, we reach a leaf node. Then we
        can check whether the sum equals val.
        '''

The explanation is not very clear. You describe what are the nodes of the tree but what are the children of a node?

        # termination condition (leaf nodes)
        if len(prev_indice) == n:
            if prev_sum == val:
                result.append(tuple(sorted_indice[i] for i in prev_indice))
        else:

The two nested if are better replaced by a single if and the program should return at once, so that else is not required. This would reduce the indentation of your code. Also it seems that the check for equality and the check for early stopping are quite similar and could be done together.

            current_count = len(prev_indice)
            # depth-first search
            for idx in range(
                    0 if not prev_indice else prev_indice[-1]+1,
                    size-(n-current_count-1)):

here it is very difficult to understand if the range of numbers is correct.

                current_sum = prev_sum + arr[sorted_indice[idx]]
                # early stopping (pruning of branches)
                if current_sum>val and idx+1<size and arr[sorted_indice[idx+1]]>=0:
                    break

You can make the early stopping better (and maybe simpler) by checking that the current_sum plus the product of the remaining terms by the smallest term is larger that the target sum. You could also make the opposite check: if summing the larger possibile terms you cannot reach the target sum.

                nsum_recursive(arr, prev_indice+[idx],
                               current_sum, result)

    # start DFS from the root node
    result = []
    nsum_recursive(arr, [], 0, result)
    return result


Merit of a "Search" class similar to a "Builder" class I have a class something like the following:

class ItemManager {

    List<Item> items;

    public ItemManager() {

        items = Database.retrieveItems();

    }
}

An item has several attributes, so much so that I used a Builder class instead of a typical constructor(Like the builder classes found in Joshua Bloch's Effective Java).  I want to use ItemManager to keep track of a large number of items and I want to be able to search for Items that match one or more given attributes.  My question is, is the following method a reasonable approach, where can I improve it, and what is a common method of tackling this problem?

class ItemManager {

    ... // constructor etc.

    public Search getSearch() {
        return new Search();
    }

    public class Search {

        // Set default search parameters
        AttrA attrA = null;
        AttrB attrB = null;
        AttrC attrC = null;

        public Search setAttrA(AttrA a) {
            attrA = a;
            return this;
        }

        ... // set method for all attributes

        public List<Item> search() {

            List<Item> results = new ArrayList<Item>();

            for(Item i : items) {

                // If the search parameter is still null OR if the attribute
                // matches the search parameter, add it to the result set
                if ( (attrA==null || i.getAttrA().equals(attrA) )
                    && (attrB==null || i.getAttrB().equals(attrB) )
                    && (attrC==null || i.getAttrB().equals(attrC) ) {

                    results.add(i);

                }
            }

            return results;
        }
    }
}

The implementation would look somethign like this (bear in mind the actual Item class would have much more than three attributes):

ItemManager itemManager = new ItemManager();

List<Item> items =
    itemManger.getSearch()
    .setAttrA(a)
    .setAttrB(b)
    .search();

There's a number of ways I can think of to improve this, but I didn't want to include them all (so they're welcome).

You can generify your ItemManager

class ItemManager<T extends Item> {

    List<T> items;

    public ItemManager() {

        items = Database.retrieveItems();

    }
}

The search method can be refactored with this help of CollectionUtils like this

public Collection<Item> search() {
    return getMatchedItems();
}

And her is getMatchedItems()

public Collection<Item> getMatchedItems() {
    CollectionUtils.select(items, new Predicate() {
    public boolean evaluate(Object o) {
        Item item = (Item)o;
        return containsA(item) && containsB(item) && containsC(item) //explain below
    }
    });

    return items
}

IMO its better to create seperate methods like containsA() , containsB()....

Because as you mentioned you have many attributes(let presume 50), then one big if will sooner or later lend you in problems like readability, maintainability and testing.
You may say that creating separate methods may degrade your performance but jvm handles method inlining pretty well.


VBA Macro to generate a report of mean values for each date My Macro that I wrote is running really slowly and I'm not sure why. The Macro deletes the area, then reproduces one column of locations as a row, deleting any duplicates, then copy's across a column of dates, then puts the corresponding Mean value for each date under the correct functional location.

Sub Macro1()
'
' Macro1 Macro

' Delete Previous Data
Range("J1:AQ1000").Clear

' Functional Location

Dim toAdd As Boolean, uniqueTag As Integer, i As Integer, j As Integer
Cells(1, 11).Value = Cells(2, 2).Value
uniqueTag = 11
toAdd = True
For i = 3 To 1000
For j = 11 To uniqueTag
If Cells(i, 2).Value = Cells(1, j).Value Then
toAdd = False
End If
Next j
If toAdd = True Then
Cells(1, uniqueTag + 1).Value = Cells(i, 2).Value
uniqueTag = uniqueTag + 1
End If
toAdd = True
Next i

' Date

Dim k As Integer
For k = 2 To 1000
Cells(k, 10).Value = Cells(k, 6).Value
Next k

' Mean Value
Dim l As Integer, m As Integer
For l = 2 To 1000
For m = 11 To 100
If Cells(l, 2).Value = Cells(1, m).Value Then
Cells(l, m).Value = Cells(l, 5).Value
End If
Next m
Next l

End Sub

Are there any optimizations that would make it run faster, or something I haven't included?
The first parts all work pretty speedily, its only the copying of the mean values which appears to take time.

In my example the iteration needs 2 seconds. How long is your code running?

Note 1:
Reduce the number of iterations. You are iterating 100.000 times

Note 2:
You are accessing the same cell Cells(l, 2).Value very often in the second for loop. But the value does not change. It will change in the first for loop.

You can save the value in a local variable in the first loop:

Dim l2 As Variant
l2 = Cells(l, 2).Value

Note 3:
You are accessing the same 90 cells Cells(1, m).Value very often too (1000 times).

You can save the 90 values in an array before looping:

Dim Arr() As Variant
Arr = Range("K1:CV1")

My complete code looks like this:

' Mean Value
Dim Arr() As Variant
Arr = Range("K1:CV1")
Dim l As Integer, m As Integer
For l = 2 To 1000
    Dim l2 As Variant
    l2 = Cells(l, 2).Value
    For m = 1 To 90
        If l2 = Arr(1, m) Then
            Cells(l, m).Value = Cells(l, 5).Value
        End If
    Next m
Next l


Showing a certain screen depending on a particular status In this iOS app I'm working on, there are two landing screens shown to the user depending on a particular status. The screens are Login screen and Activate screen.

Let me explain what this Activate screen is. There is something called activating users. It's done by a web backend and the app has no control over it. Each device the app runs has a unique device ID. This unique ID is generated in the device. A person manually activates this ID from the backend. And there are two ways this ID gets to the backend. Either it's sent from the device at first launch, or it can be obtained by some other method and entered to the backend database manually.

When the app launches, you must get that ID and send it over to the backend to check if it's activated or not. You get two statuses, 100 - Activated, 101 - Not Activated. I keep a boolean flag called DeviceIsActive in NSUSerDefaults to store the result.

When the app launches first it checks if the DeviceIsActive is true, if it is, the user is directed to the Login screen. If its false, you're directed to the Activate screen where you're informed to contact the person at the backend to activate it.

Here is the code I have in the didFinishLaunchingWithOptions in the AppDelegate file which is the method that runs first when the app is launched.

var window: UIWindow?
private enum Screen {
    case ActivateScreen
    case LoginScreen
}
let api = API()

func application(application: UIApplication!, didFinishLaunchingWithOptions launchOptions: NSDictionary!) -> Bool {

    if Preferences.getDeviceIsActive() == true {
        // Checks if deviceIsActive flag is true.
        redirectToView(.LoginScreen)
    } else {
        // If there is not deviceID, generate one.
        if Preferences.getDeviceID() == nil {
            var deviceID = NSUUID.UUID().UUIDString
            deviceID = deviceID.stringByReplacingOccurrencesOfString("-", withString: "") // e,g: 70788AEB8586513C46B0599AEA4F82BF
            Preferences.setDeviceID(deviceID)

            // Send the ID to the backend to check.
            register()
        } else {
            // If there is a deviceID already, still send it to the server to check and make sure.
            register()
        }
    }

    return true
}

private func register() {
    api.registerDevice(Preferences.getDeviceID(), completionHandler: { (response, error) -> Void in
        let (status, code) = response

        if status == Status.DeviceIsActive.toRaw() { // Status is an enum. It has all the statuses (100, 101).
            Preferences.setDeviceIsActive(true)
            self.redirectToView(.LoginScreen)
        } else {
            self.redirectToView(.ActivateScreen)
        }
    })
}

private func redirectToView(screen: Screen) {
    self.window = UIWindow(frame: UIScreen.mainScreen().bounds)
    let storyboard: UIStoryboard = UIStoryboard(name: "Main", bundle: nil)

    switch screen {
    case .ActivateScreen:
        let activateVC: ActivateViewController = storyboard.instantiateViewControllerWithIdentifier("ActivateController") as ActivateViewController
        let navigationVC = UINavigationController(rootViewController: activateVC)
        self.window?.rootViewController = navigationVC
    case .LoginScreen:
        let loginVC: LoginViewController = storyboard.instantiateViewControllerWithIdentifier("LoginController") as LoginViewController
        let navigationVC = UINavigationController(rootViewController: loginVC)
        self.window?.rootViewController = navigationVC
    default:
        println("No view controller specified")
    }

    self.window?.makeKeyAndVisible()
}

Now the code works fine. But it's just so ugly and messy with all the if else statements all over the place. Is there any way to make it cleaner?

if Preferences.getDeviceIsActive() == true {
    // Checks if deviceIsActive flag is true.
    redirectToView(.LoginScreen)
} else {
    // If there is not deviceID, generate one.
    if Preferences.getDeviceID() == nil {
        var deviceID = NSUUID.UUID().UUIDString
        deviceID = deviceID.stringByReplacingOccurrencesOfString("-", withString: "") // e,g: 70788AEB8586513C46B0599AEA4F82BF
        Preferences.setDeviceID(deviceID)

        // Send the ID to the backend to check.
        register()
    } else {
        // If there is a deviceID already, still send it to the server to check and make sure.
        register()
    }
}

This seems to be the chunk of code you're primarily concerned with, so it's where I'll focus on for now.

First, == true is basically always redundant.  Our first if could simply be:

if Preferences.getDeviceIsActive() {

If the nested else were truly necessary, I'd recommend unnesting the nested if else and turning this into an if else if else, but the nested else is unnecessary.  We're calling register() unconditionally inside the else.  We can just write:

if Preferences.getDeviceID() == nil {
    var deviceID = NSUUID.UUID().UUIDString
    deviceID = deviceID.stringByReplacingOccurrencesOfString("-", withString: "")
    Preferences.setDeviceID(deviceID)
}

register()

api.registerDevice()

You haven't included any details about what API is or what the registerDevice method does specifically, but it looks like it most likely is asynchronous, and it probably calls to a web service of some sort.

This sort of logic would be fine in say a standard view controller that loads after your app delegate was finished.  But we don't know how long we may be waiting for this return.

If the call is asynchronous, then your application(application:, didFinishLaunchingWithOptions launchOptions:) -> Bool will most likely return before we figure out where to navigate to.  Best case scenario, your project is set up to have a root view controller via the storyboard without the redirectToView method being called and it just looks a little weird when it then later redirects you.  Worst case scenario, you don't have a default view controller set up and we have to rely on redirectToView which means we may sometimes not have a view before we're done with the didFinishLaunchingWithOptions returns and the app will crash.

And heaven forbid this line of code ever get called without a default view already in place:

default:
    println("No view controller specified")

Because this might also leave the window without a view, which again, is a crash.

If the call is synchronous, now we're causing the didFinishLaunching to hang out for an unnecessarily long amount of time because we're performing network operations when we're supposed to be loading up resources and initializing our app.

Ultimately the best solution to this is to move this sort of logic into a base view controller.  For now, let's call it the "Home" view controller.  Our app will be set up to always load to "Home".  We want to have somewhere to go to in order to get out of didFinishLaunchingWithOptions as quickly as possible.

Once we're at "Home", now we perform the logic for determining whether we need to login or activate.

Or perhaps, given that the activation is a one-time process, perhaps we should default to the login screen.  The login screen can check if the device is activated or not.  If it determines it's not activated, we can give the user a pop up.  "This device is not activated.  Would you like to activate it now?" and answering yes to this pop up takes us to the activation screen.


Multiple object instantiation prevention I have a class with a private variable used to store an object.
I have a function that checks first if that variable already contains an object or not; if not, it instantiates the needed object and sets it to that variable, otherwise it just returns the content of that variable.

I was wondering if the getSessionCustomer() here is an overkill/unnecessary or if it has real benefits. I simply based this on the Album tutorial by Zend, but I haven't fully tested it out yet to really see the advantages (or disadvantages).

class JobController extends AbstractActionController
{
    private $SessionCustomer;

    public function saveJobAction()
    {
        $SessionCustomer = $this->getSessionCustomer();

        if(empty($SessionCustomer->offsetGet('customer_id'))) {
            return $this->redirect()->toRoute('login');
        } else {
            $JobService = $this->getServiceLocator()->get('Job\Factory\JobServiceFactory');
            $job_id     = $JobService->saveJob();

            return $this->redirect()->toUrl('/job/' . $job_id);
        }
    }

    public function viewJobAction()
    {
        $sm                 = $this->getServiceLocator();
        $SessionCustomer    = $this->getSessionCustomer();

        if(empty($SessionCustomer->offsetGet('customer_id'))) {
            return $this->redirect()->toRoute('login');
        } else {
            $JobTable       = $sm->get('Job\Model\JobTable');
            $JobItemTable   = $sm->get('Job\Model\JobItemTable');

            $jobId          = $this->params()->fromRoute('id');
            $Job            = $JobTable->getJobById($jobId);
            $JobItems       = $JobItemTable->getJobItemsByJobId($jobId);

            $this->layout()->setVariable('title', 'Order #' . $jobId);

            $viewModel = new ViewModel();
            $viewModel->setVariables(array(
                'Job' => $Job,
                'JobItems' => $JobItems
            ));

            return $viewModel;
        }
    }

    private function getSessionCustomer()
    {
        if(!$this->SessionCustomer) {
            $this->SessionCustomer = $this->getServiceLocator()->get('Session\Customer');
        }

        return $this->SessionCustomer;
    }
}

Using the check if (!$this->SessionCustomer) is not unusual even if a bit clunky. I would still be more explicit about it and use if ($this->SessionCustomer === null). The private $SessionCustomer; member will be initiated to null.

More generally however, what you are using here is called an anti-pattern. PHP Singletons are almost universally considered an anti-pattern. Singleton really is not something you should be using, as it's really unnecessary, what you really want is Dependency Injection.

When you want to use a single class instance during the lifetime of an object, you simply request the \Session\Customer type in your object's constructor, and use a Dependency Injection Container to make sure that the same instance is passed to all the instances of the JobController class:

class JobController
{
    private $sessionCustomer;
    public function __construct(\Session\Customer $sessionCustomer)
    {
        $this->sessionCustomer = $sessionCustomer;
    }
    public function saveJobAction()
    {
        // just use the private property
        if (empty($this->sessionCustomer->offsetGet('customer_id'))) {
            // do stuff
        }
    }
}

At this point, I would just like to comment that otherwise, there is really no reason to use OOP if you are not using constructor injection and just globally access things with a service locator (which is also a very bad pattern of its own, but that's for another topic). That code could be procedural.

Back to the topic: defining your requirements in the constructor allows for the calling code to decide that instance to pass to the JobController's constructor, reuse that instance, pass in a mock instance (because hopefully, you would be testing that class, which may necessitate for you to mock instances).

I generally use Auryn as a Dependency Injection Container, but the usage generally consists on instructing a container to share an instance of a class:

$injector = new \Auryn\Injector;
// this is how you actually use a single instance of a class, through sharing it with a container
$injector->share($sessionCustomer);
$jobController = $injector->make('JobController'); // uses the shared instance


Saving WPF DataGrid changes to Entity Framework I feel like I had to invent a whole new way to do this because I couldn't find anything that did this the way I wanted to...

The basic process involves reading changes to cells and capturing information about those changes as they relate to the actual Entities in question and then persisting them to the database.

Here's the XAML. I didn't use databinding because I don't quite understand it yet and I'm on a  bit of a deadline for this one so that's item number 1 on my to do list when I get a break.

Note that this project has been my first every foray into WPF. I've dabbled a bit in the past but I've really pushed myself to do things properly here... As properly as I know how anyway.

<DataGrid Name="dgUsers" Height="150"
    CellEditEnding="dgUsers_CellEditEnding">
</DataGrid>

<StackPanel Name="spActions" Background="#2d2d30" Width="500" Height="50" Orientation="Horizontal" FlowDirection="RightToLeft">
    <Button Name="btnCancel" Click="btnClose_Click" Content="Cancel" Style="{StaticResource CancelButton}" Height="30" Width="66" Margin="5,3,5,5"></Button>
    <Button Name="btnSave" Click="btnSave_Click" Content="Save" Style="{StaticResource RoundedButtonGreen}" Height="30" Width="66" Margin="5,3,5,5"></Button>
    <Label Name="lblResult"></Label>
</StackPanel>

Here's the C# codebehind:

Generate an entity from the changes that were made:

private List<User> UpdatedUsers { get; set; }
/// <summary>
/// Maintains a list of all changes made to all entities on the datagrid
/// </summary>
/// <param name="sender"></param>
/// <param name="e"></param>
private void dgUsers_CellEditEnding(object sender, DataGridCellEditEndingEventArgs e)
{
    if (UpdatedUsers == null) UpdatedUsers= new List<User>();

    var _User = (User)e.Row.Item;
    var Element = (TextBox)e.EditingElement;
    if (String.Equals((string)e.Column.Header, "EmailAddress"))
    {
        User.EmailAddress = Element.Text;
    }
    else if (String.Equals((string)e.Column.Header, "Password"))
    {
        User.Password = Element.Text;
    }

    UpdatedUsers.Add(User);
}

From here the user clicks on a save button which loops through the changes captured in this list and updates the relevant records in the database:

private void btnSave_Click(object sender, RoutedEventArgs e)
{
    try
    {
        ApplicationDbContext Context = new ApplicationDbContext();
        var Users = Context.Users.Where(x => x.CompanyId == (int)cmbCompanies.SelectedValue).ToList();

        foreach (User User in Users)
        {
            foreach (User U in UpdatedUsers)
            {
                if (U.Id == User.Id)
                {
                    User.EmailAddress = U.EmailAddress;
                    User.Password = U.Password;
                }
                Context.SaveChanges();
            }
        }
    }
    catch (Exception ex)
    {
        lblResult.Content = ex.Message;
    }
}

Other issues...

C# variable naming convention is camelCase. And definitely don't start a local's name with an underscore, as underscores are understood to prefix instance variables.
There's no separation of concerns here. Your UI code is talking to the database. That's not the responsibility of the UI.
I assume ApplicationDbContext inherits from DbContext which is implements IDisposable. You're not disposing it. Use it in a using block. (I know nothing about Entity Framework so don't know if there's a good reason not to do so.)


Human class implementation I just want an indication as to whether or not I'm on the right track regarding PHP OOP, at least on a basic level.  Positive criticism welcome.

P.S. Excuse the visuals of the code.  This is how I usually remember code.

<html>
<?php

//Blueprint

class Human{

    // properties of human
    private $gender;
    private $name;
    private $age;
    private $talk;

    public function __construct($gender, $name, $age, $talk){
        $this->gender   = $gender;
        $this->name     = $name;
        $this->age      = $age;
        $this->talk     = $talk;
    }

    public function getGender(){
        return $this->gender;
    }

    public function getName(){
        return $this->name;
    }

    public function getAge(){
        return $this->age;
    }

    public function getTalk(){
        return $this->talk;
    }
}

class female extends Human{

    //properties of female
    private $bum;
    private $feet;

    public function __construct($gender, $name, $age, $talk, $bum, $feet){
        parent::__construct($gender, $name, $age, $talk);

        $this->bum  = $bum;
        $this->feet = $feet;
    }

    public function  getBumSize(){
        return $this->bum;
    }

    public function getFeetSize(){
        return $this->feet;
    }
}

class male extends Human{

    //properties of male
    private $facialHair;
    private $adamsApple;

    public function __construct($gender, $name, $age, $talk, $facialHair, $adamsApple){
        parent::__construct($gender, $name, $age, $talk);

        $this->beard    = $facialHair;
        $this->throat   = $adamsApple;
    }

    public function getBeard(){
        return $this->beard;
    }

    public function getThroat(){
        return $this->throat;
    }
}

//instantiate objects
$malePerson     = new male('male', 'John Doe', '32', 'clear', 'beard', 'adams apple in my throat');
$femalePerson   = new female('female', 'Jane Doe', '28', 'loud', 'big and round', 'small');

echo "Hi, My name is {$malePerson->getName()}, {$malePerson->getAge()} years of age, I talk {$malePerson->getTalk()}
        so i guess that makes me {$malePerson->getGender()} a.k.a man....I also have
        a {$malePerson->getThroat()} and a {$malePerson->getBeard()} which is very very long .lol";
echo "<br>";
echo "Hi, My name is {$femalePerson->getName()}, {$femalePerson->getAge()} years of age, I talk {$malePerson->getTalk()} so i
        guess that makes me {$femalePerson->getGender()} a.k.a mamalicious....I also have
         {$femalePerson->getFeetSize()} feet and a {$femalePerson->getBumSize()} bum.lol";
?>
</html>

First of all: Extract your classes into extra files and require_once them as necessary. I found the Java style of having one class per file a very good one.

Second: Learn and start to use PHPDoc to document your code.

//Blueprint

That's not a helpful comment...only write a comment if it is helpful.

// properties of human
private $gender;
private $name;
private $age;
private $talk;

Such structural comments are unnecessary if your class is correctly structured and indented.

$this->gender   = $gender;
$this->name     = $name;
$this->age      = $age;
$this->talk     = $talk;

Personally I don't like this kind of intending. It can make code easier to read, but in a pure "assign all parameters to all fields" constructors it seems kinda unnecessary.

class female extends Human{

Is it a typo that female is lower case?

private $bum;

If it holds the size of the bum, then it should be called $bumSize. The same goes for $feet.

$malePerson     = new male('male', 'John Doe', '32', 'clear', 'beard', 'adams apple in my throat');

Apart from the unnecessary intending, this is a good example stringly typed programming. You're using a string for everything even though the age should be an integer.

Let's have a look at the basic variables a Human consists of:

$gender Should an enumeration with two values, Male and Female.
$name Names are strings, so it's okay.
$age Is an integer and should therefor be an integer.
$talk Is also a string.

And let's go forth with the Male:

$facialHair Should be an enum, or an integer to indicate strength.
$adamsApple Not sure what this is.

Also forget what I just said about $gender, Human should be an abstract class and getGender() should be an abstract method that is implemented by the next class.

<html>
     <?php Some code with echo... ?>
</html>

That's not a well-formed HTML document.

<html>
    <head>
        <title>Title of your page</title>
    </head>
    <body>
        Content goes here.
    </body>
</html>

Ideally you would perform initializations before the html tag and then just use everything prepared in the body.


Custom exception for each invalid input value I have backend project in which there's own "parent-to-all exception" exists, something like this (also I have InvalidArgumentException derived from this exception):

class CaliException extends \Exception {
    private $caliCode;

    public function __construct(string $message = "", string $caliCode = self::UNDEFINED_CODE,
                                int $code = 0, Throwable $previous = null) {
        parent::__construct($message, $code, $previous);

        $this->caliCode = $caliCode;
    }

    function getCaliCode(): string {
        return $this->caliCode;
    }
}

It's clear from code above, that purpose of custom class is ability to keep string codes. Why do I need string codes? Because I have HTTP API which works in the following way:

Get JSON request
Do something
Produce JSON output with 'state' field which contains either code of a thrown exception or the "200" response. These state codes are quite useful for client which can distinguish different problems and react to them accordingly.

So there's a problem. System above produces code like this:

class UsernameFormatException extends InvalidArgumentException {
    public const USERNAME_FORMAT_UNDEFINED_CODE = "DM:USERCA:0001";

    public function __construct(string $message = "", string $calistoCode = self::USERNAME_FORMAT_UNDEFINED_CODE,
                                int $code = 0, \Throwable $previous = null) {
        parent::__construct($message, $calistoCode, $code, $previous);
    }
}

class PasswordFormatException extends InvalidArgumentException {
    public const PASSWORD_FORMAT_UNDEFINED_CODE = "DM:USERCA:0002";

    public function __construct(string $message = "", string $calistoCode = self::PASSWORD_FORMAT_UNDEFINED_CODE,
                                int $code = 0, \Throwable $previous = null) {
        parent::__construct($message, $calistoCode, $code, $previous);
    }
}

class InvalidLastActivityTimestampException extends InvalidArgumentException {
    public const INVALID_TIMESTAMP = "DM:USERCA:0003";

    public function __construct(string $message = "", string $calistoCode = self::INVALID_TIMESTAMP,
                                int $code = 0, \Throwable $previous = null) {
        parent::__construct($message, $calistoCode, $code, $previous);
    }
}

class InvalidCreationTimestampException extends InvalidArgumentException {
    public const INVALID_TIMESTAMP = "DM:USERCA:0004";

    public function __construct(string $message = "", string $calistoCode = self::INVALID_TIMESTAMP,
                                int $code = 0, \Throwable $previous = null) {
        parent::__construct($message, $calistoCode, $code, $previous);
    }
}

As seen, for each invalid-argument-case I create new CaliException-derived exception. I consider it as not cool. Is it good? How can I improve the code?

Bonus question: I read that I should throw \InvalidArgumentException when it's a programmer's mistake, so an exception must be not caught. But in my code there's no \InvalidArgumentException only my own version of this. Is it acceptable in the context of best practices and so on? And how to distinguish when it's a programmer's mistake and when it's mistake of user (invalid user input)? (After all, any invalid values passed to a function are invalid inputs relatively to this function)

With regard to if you need multiple exceptions or not:
I'd argue not in this particular situation, multiple exceptions are great for if you want to do different logic to handle them(so you plan to catch PasswordFormatException because that needs to be handled differently to your other validation errors).

To solve this you can add multiple constants on the same class if you want to return the code for what's invalid at the point you throw it:

class CaliException extends \Exception {
    const INVALID_USERNAME = "DM:USERCA:0001";
    const INVALID_PASSWORD = "DM:USERCA:0002";
    const INVALID_LAST_ACTIVITY_TIME = "DM:USERCA:0003";
    const INVALID_CREATION_TIME = "DM:USERCA:0004";

    private $caliCode;

    // Note that caliCode is now required, forcing you to pass in a constant
    // to say what the error is
    public function __construct(string $caliCode, string $message = "",
                                int $code = 0, Throwable $previous = null) {
        parent::__construct($message, $code, $previous);

        $this->caliCode = $caliCode;
    }

    function getCaliCode(): string {
        return $this->caliCode;
    }
}

// ....
// And then when you use it you now say it's a username or password field that failed validation

throw new CaliException(CaliException::INVALID_USERNAME, 'Username must not contain special characters');

As for if this should extend InvalidArgumentException:
I'd argue not, this feels more like you should have a class for UserInputValidationError or similar and extend this for your purpose. Failing validation of user input is not a fundamental problem with the program which InvalidArgumentException would imply but instead a natural consequence of having to handle user inputs.


Counting pairs of relatively prime numbers Problem from Hacker Earth:

  Inverted GCD:

  Given an array a of \$N\$ numbers , you have to find the number of pair of indices \$i\$ and \$j\$ that satisfy the following relation:

  \$i < j\$
  \$a_i > a_j\$
  gcd\$(a_i , a_j) = 1\$

  Input:

  The first line of the input contains a single integer N - denoting the size of the array.
  The next line contains N space separated integers denoting the array a.

  Output:
  Output a single integer - the answer to the problem stated above.

  Constraints:
  \$1 \le N \le 10^5\$
  \$1 \le a_i \le 10^5\$

For this problem I've submitted the following solution in Python:

from fractions import gcd
n = int(input())
count = 0;
a = [int(x) for x in input().split()]
for i in range(n):
    for j in range(n):
        if(a[i]>a[j] and i<j and gcd(a[i] , a[j]) == 1):
            count +=1;
print(count)

I'm not sure what is the complexity of gcd() function in Python, but it clear due to two for loop that the time complexity is minimum \$O(n^2)\$.I can't understand how to reduce it. Because here we have to check these conditions between every pair of integers. I've seen such question here but there is no condition for gcd(), so can't relate these two questions. I'm newbie to Python.

tl;dr TLE beyond input size 40000 to 50000 (10 seconds for input size 100000)

Conditions (1) and (2) are transitive but independent and one-sided. This means they can only prune half of all candidates on average each, and so their combination only reduces the average candidate set to one fourth (i.e. half of a half). Such a small constant factor is not worth it to invest in (as in 'devising a special data structure'), unless overall performance is already close to a passing grade. Asymptotic performance doesn't change at all, of course. Hence it is best to take the doubling in performance that comes from index pruning (condition (2), which is essentially free) and test condition (1) as pairs are processed.

Condition (3) induces a squarish relation all on its own already, and it is not transitive. This means that there is no efficiency gain from precomputing this relation, and memoisation is likely to be completely ineffective in the face of random inputs.

The evaluation of the GCD for a pair of numbers takes time logarithmic in the smaller of the two inputs. This time can be cut by a considerable constant factor with a bit of precomputation, by computing integer-sized 'bit vectors' representing the presence of a handful of the smallest primes in the (radical) factorisation of a number.

This way a simple binary AND can prove the absence of shared small factors and thus avoid the computation of the actual GCD in about 89% of all cases (assuming factorisation by the first 32 primes, i.e. up to 131). In the remaining 11%, only the factorisation 'remainders' need to be GCDed, i.e. what remains after dividing out the small factors/radicals represented by the bit vectors. This saves some iterations of Euclid's algorithm.

This can give you an overall speed boost of an order of magnitude or more. On top of the factor 2 speed gain from index pruning mentioned by coderodde and some general efficiency tuning this might just be enough to beat the time limit...

Note: one might be tempted to extend the 'bit vectors' from 32 bits to 64 bits to utilise the bigger machine words on 64-bit machines, but this is not worth it. It doubles the up-front factorisation effort but it improves the rejection rate only from 88.7% to 90.3%, i.e. by a completely negligible 1.8%.

Here's a small table with the pass/reject rates for bit masks involving the given number of the first primes:

 8 primes: pass 17,102%, rej. 82,898%                 # up to 4 extra radicals
12 primes: pass 14,872%, rej. 85,128% // boost 2,690% # up to 3 extra radicals
16 primes: pass 13,609%, rej. 86,391% // boost 1.484%
32 primes: pass 11,300%, rej. 88,700% // boost 2,673%
64 primes: pass  9,683%, rej. 90,317% // boost 1,823% # up to 1 extra radical

The 'up to X extra radicals' bit says how many different prime factors can remain in the factorisations of numbers not exceeding 100,000 after the the given number of the first small primes has been factored out.

P.S.: the bounds on the number of numbers is the same as the bound on the size of the numbers, which means that the factorisations can be pre-computed wholesale without any divisions (by borrowing the logic behind the Sieve of Eratosthenes). The partial factoring for the bit vectors takes less than a millisecond, and factoring the input remainders takes less than 10 milliseconds for 100,000 inputs. Hence the stuff can always be computed wholesale and it isn't even worth it to have different code paths for small and big input batches.

--- UPDATE ---

For getting a better handle on the problem I needed a closer fix on some numbers, and so I coded things up in C++. It turned out that computing the gcd of the remainders (after factoring out the small primes used in the 'bit vectors') is orders of magnitude too slow, even though it occurs only for 0.35% of all candidates. This necessitated replacing the partial factorisation remainders with lists of the (up to two) remaining radicals.

'Radical' Optimisation

For numbers less than 107113 = 43 * 47 * 53, factoring out the 13 smallest primes leaves at most two distinct prime factors ('radicals'). Moreover, only factors up to half the limit need be considered since the only numbers with bigger prime factors are the primes, which are coprime to all numbers but themselves and self-pairings get excluded via criteria (1) and (2) both. This means that 2 x 16 bits = 32 bits are sufficient for storing the 'list' of remaining radicals for a number.

This size reduction is beneficial for L1 cache utilisation and it would allow to stuff both radicals into a single 32-bit integer; however, it collides with an important optimisation of the intersection operation. This optimisation is the reduction of branchings to the absolute minimum of two to three comparisons, by always treating the two-element lists as full and by replacing all but one of the remaining non-comparison branches with arithmetic (poor man's CMOV). This makes things easier for the CPU's branch prediction logic and reduces the amount of incurred branch mis-prediction penalties, but it requires the use of sentinel values for 'missing' radicals. With the only safe sentinel for a number a[i] being a[i] itself this means that the slots for the radicals must be able to hold full-sized numbers.

Elimination of one particularly nasty branch required adding a third radical slot (which always holds a sentinel value) but it resulted in a speedup of 10%. Without this explanation it might seem odd that the code uses three slots even though there can be at most two radicals. First, the data:

uint16_t sieve[1 + LIMIT];  // bit masks of partial factorisations of 1 .. LIMIT
uint16_t v[1 + LIMIT];      // v[i] = sieve[a[i]]
uint32_t f[1 + LIMIT][3];   // remaining factors (radicals) for each input (3rd is sentinel)
uint8_t skip[1 + LIMIT];    // for each a[j]: how many successors are not less

The code for generating these tables is elementary and of little interest here (especially as it takes less than 10 milliseconds even in C#). Hence I'm skipping it in the interest of brevity. Here is the meat of the code, the function that does the counting as specified by the task:

uint64_t count_matches (uint32_t const a[], uint32_t count)
{
    uint64_t result = 0;

    prepare_tables(a, count);

    for (uint32_t i = 0; i < count; ++i)
    {
        uint32_t a_i = a[i];
        unsigned v_i = v[i];

        for (uint32_t j = i + 1; j < count; ++j)
        {
            if ((v_i & v[j]) == 0)
            {
                if (a_i > a[j])
                {
                    if (uint32_t d = f[i][0] - f[j][0])
                    {
                        uint32_t di = (d >> 31) & 1;
                        uint32_t ci = di;
                        uint32_t cj = di ^ 1;

                        if ((d = f[i][ci] - f[j][cj]) != 0)
                        {
                            di = (d >> 31) & 1;
                            ci += di;
                            cj += di ^ 1;

                            if ((d = f[i][ci] - f[j][cj]) != 0)
                                ++result;
                        }
                    }
                }
                else j += skip[j];
            }
        }
    }

    return result;
}

On my laptop this clocks a bit less than 19 seconds for random inputs of size 100,000, compared to 22 seconds for a 'branchy' version of the intersection test (which has no use for the third slot):

                    int32_t d = int32_t(f[i][0] - f[j][0]);

                    if (d > 0)
                    {
                        d = int32_t(f[i][0] - f[j][1]);

                        result += d > 0 || d && f[i][1] != f[j][1];
                    }
                    else if (d != 0)
                    {
                        d = int32_t(f[i][1] - f[j][0]);

                        result += d < 0 || d && f[i][1] != f[j][1];
                    }

Of course, in Python the optimisation is bound to get killed by the interpreter overhead, because the code contains a lot of explicit little operations instead of pushing processing into the engine. But in any case it should be beneficial to utilise the fact the 'radical factor set intersection' involves only lists of at most two elements here instead of general sets.

Skip Counts

The skip count thing hasn't been mentioned yet - it's a small optimisation that costs virtually nothing and improves speed by about 10%. It utilises the fact that a[i] <= a[j] && a[j] <= a[j + k] implies a[i] <= a[j + k], and storing/precomputing this is cheap because it involves only a[j] and its context in the input sequence instead of depending on pairings of a[i] and a[j].

Half of all skip counts are 0, of course, and half of the rest is 1. The rest peters off quickly because the probability of a skip count having value k is 1 / ((k+1)*(k+2)), which means that skip counts of four bits or so already realise most of the performance gain that can be gotten this way.

Four bits would probably be optimum if one wanted to stuff several different values - like input number, small factor bit vector and skip count - into a single integer-sized variable, especially in languages where array indexings carry a heavy cost (Python, Java, C#). In my sample code I used a separate array for simplicity, and hence allocated one whole byte for each skip count.

Numbers and Summary

The reason I sat down and got coding was that I wanted to get a fix on some numbers, in order to get a better handle on the problem and a better feeling for the lay of the land. Here are those numbers.

For a random input vector of 100,000 numbers - the limit given in the task description - and enumerating only the 4,999,950,000 pairs of numbers with i < j, it took the following amounts of time to count the following things:

11.0 s for 2512386875 inversions
21.5 s for 3029211382 coprime pairs
17.3 s for 1527936260 candidates (a[i] > a[j] and no common factors among the first 16 primes)
15.7 s for 1527936260 candidates (dito, with byte-sized skip counts)
85.7 s for 1522549277 matches (with gcd applied to remainders)
18.6 s for 1522549277 matches (with list intersection for remainder radicals)

The most important message here is that the number of inversions (i.e. criterion (2) in the task description), the number of coprime pairs (criterion (3)) and the number of results to be counted are all the same order of magnitude as the number of pairs with i < j. The percentages are 50.2%, 60.6% and 30.5% respectively.

Also, just counting the number of inversions takes more than five times as long as the two seconds admissible as per the task description, even though my laptop is likely quite a bit faster than the hardware used by the robot judge.

Even if I found some fiendishly clever algorithm or data structure which can prune everything that doesn't fulfil the task conditions then the time would still be 30.5% * 19 s which is about 6 seconds. In other words, on a maximum input file the code would still take three times as long as the time limit.

For comparison, counting the matches for a vector of 1000 random inputs - as per coderodde's 'lightened' specification - takes less than 2 milliseconds with this code. I'd hate to think how long coderodde's solution would take on a maximum input file as per the actual task description; extrapolation from the given results puts it at more than half an hour.

In the first two paragraphs of this post I argumented why the intersection of the task criteria must leave a number of pairings that is still square in the number of inputs; now we know that the actual number is about 30% of 'j-pruned' pairs or 15% of all input pairs. Moreover, the C++ timings suggest that counting each result pair individually is simply not an option, since the optimisation potential that's still left is far less than an order of magnitude (which is why I didn't bother to create an optimised version).

Any admissible solution must count whole swathes of pairs in one go instead of processing each pair individually, but at the moment I cannot offer any specific advice as to how one might accomplish that. One seedling of an idea might lie in the fact that any prime must a priori be coprime to any input number other than itself, so that only criteria (1) and (2) need be applied for counting it. Similar considerations apply to sets of input numbers that have disjoint sets of prime factors.

I took a peek at the submission table at HackerEarth, and the only successful submissions I could see were in C++ with timings between 3.3 seconds and 12 seconds, with codes sizes ranging from 10KB to several hundred KB. This suggests that real solutions might be quite involved, compared to the simple code that has been discussed in this topic so far. I did not look at any of the code, however, because I intend to have a stab at the problem myself first...

UPDATE #2

250,000,000 factor set intersections per second

For an input vector of 10^5 values the current algorithm has to filter 0.5 * (10^5)^2 pairs of numbers - that's about 5 billion (American-style). Performance can be roughly doubled by looking only at inversions and by removing all branches but one from the set intersection code.

The standard algorithm for inversion counting by merge sorting divides the input into non-inverted and inverted runs and tallies the latter:

long count_inversions_m1 (List<int> values)
{
    long inversions = 0;
    int n = values.Count;
    var a = values.ToArray();
    var b = new int[n];

    for (int w = 1; w < n; w <<= 1)
    {
        for (int beg = 0; beg < n; beg += w + w)
        {
            int mid = Math.Min(beg + w, n);
            int end = Math.Min(mid + w, n);

            for (int i = beg, j = mid, k = beg; k < end; ++k)
            {
                if (i < mid && (j >= end || a[i] <= a[j]))
                {
                    b[k] = a[i++];
                }
                else // a[k] > a[j] for all k in [i .. mid - 1]
                {
                    inversions += mid - i;
                    b[k] = a[j++];
                }
            }
        }

        var t = a;
        a = b;
        b = t;
    }

    return inversions;
}

In the a[i] > a[j] branch, the run from a[i] to a[mid - 1] is inverted with respect to a[j]; all inversions in the input pass that branch eventually. Replacing the line

 inversions += mid - i;

with a small loop that performs factor set intersection on the values involved gives an algorithm that is a tad faster than the earlier one, because it never has to look at any non-inverted pairs. Simply dropping the intersection code from the earlier version in there would work, but it can be made faster by using branch-less comparison.

That is, instead of branching based on the result of comparing factors, a value is computed that is 0 if the two lists of non-bitmapped prime factors (at most two elements each) have some value in common, and 1 if they do not. I chose to abuse the fact that, given a pair of unequal values a and b, either a - b or b - a must be negative but no difference is negative if the values are equal. Hence the high bit of (a - b) | (b - a) is 1 if the values are different, and 0 if they are equal (as long as the difference does not exceed INT32_MAX).

A further speedup comes from ‘compressing’ factors down to 16 bits. This is possible because the factors are only ever compared for equality, and any unique mapping of values will do just as well for that. There are only 9592 primes up to 100000, and so there are many viable schemes for assigning each prime a unique 16-bit proxy value. I chose p < UINT_MAX ? p : (ushort)(p + 3) for simplicity. This ‘compression’ from 32 bits to 16 bits leads to better cache utilisation and reduces register pressure for 32-bit architectures (since a whole pair fits into a single machine word).

Here's the final set intersection code, ready for replacing the inversion branch in the above procedure (C++ version shown, but the C# is similar):

uint32_t a_j = b[k] = a[j++];                // x 815,024
unsigned f_j = factor_bits[a_j];
unsigned bj0 = big_factors[a_j][0];
unsigned bj1 = big_factors[a_j][1];

for (uint32_t l = i; l < mid; ++l)
{
    uint32_t a_i = a[l];                     // x 2,512,386,875

    if ((f_j & factor_bits[a_i]) == 0)
    {
        unsigned bi0 = big_factors[a_i][0];  // x 1,527,936,260
        unsigned di0 = ((bi0 - bj0) | (bj0 - bi0)) & ((bi0 - bj1) | (bj1 - bi0));
        unsigned bi1 = big_factors[a_i][1];
        unsigned di1 = ((bi1 - bj0) | (bj0 - bi1)) & ((bi1 - bj1) | (bj1 - bi1));

        result += (di0 & di1) >> 31;         // 1,522,549,277 x 1 vs 5,386,983 x 0
    }
}

The 16-bit factor bitmaps are kept in a separate array to improve cache efficiency, because the bitmap AND must occur for every inversion pair whereas comparison of the up to two extra factors occurs less often. I've annotated some code lines with execution counts for an input of 100,000 random values, to put things a bit into perspective.

The resulting code comes closer to the goal, but not close enough. The C++ version still needs 10 seconds for a maximum-size (non-degenerate) input, and for the C# edition it's 12.5 seconds. I submitted the C# but, as expected, it failed all (non-degenerate) test cases where the input size exceeded 40,000 significantly. The performance target seems almost within reach, but it seems hopeless to try squeezing another five-fold performance increase out of the current approach.

I've chosen to complete the write-up despite the failure because some of the techniques may be useful for other challenges. Thanks to Kaushal28 for bringing this neat problem to our attention - I had a lot of fun trying to solve it and I learnt a lot along the way.


Asynchronously wait for a task to complete and do some async action while waiting I have a long-running task. My goal is to create a method that will allow me to:

Asynchronously wait for this task to complete
While waiting on a task, do some async action once in a while. This
'action' basically tells some remote service that task is not dead
and still executing.

I've written the following code to solve this problem. Please have a look and share your thoughts!

public delegate Task AsyncAction();

public static class TaskExtensions
{
    public static async Task<bool> TimeoutAfter(this Task task, TimeSpan timeout)
    {
        var timeoutCancellationTokenSource = new CancellationTokenSource();

        var completedTask = await Task.WhenAny(task, Task.Delay(timeout, timeoutCancellationTokenSource.Token)).ConfigureAwait(false);
        if (completedTask == task)
        {
            timeoutCancellationTokenSource.Cancel();
            await task.ConfigureAwait(false);
            return true;
        }
        else
        {
            return false;
        }
    }

    public static async Task AwaitWithTimeoutCallback(this Task task, TimeSpan timeout, AsyncAction onTimeout)
    {
        while (true)
        {
            var completed = await task.TimeoutAfter(timeout).ConfigureAwait(false);
            if (completed)
            {
                break;
            }

            await onTimeout().ConfigureAwait(false);
        }

        await task.ConfigureAwait(false);
    }
}

Usage:

var task = Task.Delay(TimeSpan.FromSeconds(15));

task.AwaitWithTimeoutCallback(TimeSpan.FromSeconds(5), () =>
{
    Console.WriteLine("Waiting...");
    return Task.FromResult(true);
})
.Wait();

public delegate Task AsyncAction();

Instead of creating a custom delegate type, you could just reuse Func<Task> (even though logically, it is an action and not a function).

return Task.FromResult(true);

Consider adding an overload that takes a non-async onTimeout delegate, so that this confusing code wasn't required.

Or, if you do something like this often, have some TaskEx.Completed, or something like that.

Otherwise, good job. I especially like that you remembered to use ConfigureAwait(false) everywhere.


Generate probabilities around a numpad Currently, I'm working on a game with directional movement influenced by the numpad. The player AI should intelligently pick a direction to move (so it's not entirely random, and can be somewhat coherent).

For reference, this is the numpad I'm using:

7 | 8 | 9
- - - - -
4 | x | 6
- - - - -
1 | 2 | 3

(5 is ignored as moving to your current location is considered invalid).

Essentially, I want to take the players prior movement direction and prioritize it where possible, making it "fan out" into lesser probabilities the more "unlikely" the movement is. Suppose the players prior direction was 8, the probability distribution should look like this:

60 | 80 | 60
-- | -- | --
30 | xx | 30
-- | -- | --
20 | 10 | 20

Just to provide another example, a prior movement of 1 would look like this:

30 | 20 | 10
-- | -- | --
60 | xx | 20
-- | -- | --
80 | 60 | 30

The algorithm I've come up with looks like this:

let neighbors = {
  1: [4, 2],
  2: [1, 3],
  3: [2, 6],
  4: [7, 1],

  6: [9, 3],
  7: [4, 8],
  8: [7, 9],
  9: [8, 6]
};

let probabilities = [80, 60, 30, 20, 10];

let recurse = (obj, pos, level = 0) => {
  if(!probabilities[level] || obj[pos]) return;

  obj[pos] = probabilities[level];
  recurse(obj, neighbors[pos][0], level+1);
  recurse(obj, neighbors[pos][1], level+1);
};

export default (firstDir) => {
  var ret = {};
  recurse(ret, firstDir);
  return ret;
}

The implementation spits out a hash with the probabilities, so, the output from the 8 situation above would look like this:

{
  1: 20,
  2: 10,
  3: 20,
  4: 30,
  6: 30,
  7: 60,
  8: 80,
  9: 60
}

I'm not really sold on this implementation - it requires a map of neighbors for recursion. I can't quite figure out if there's some fancy math I can use to derive this, so I'm instead using recursion.

Could this be done better?

There’s no need for recursion here.  You have some base probability map, and you want to rotate it such that the maximum element points in some particular direction.  You could implement that fairly simply:

var INDICES = [1, 2, 3, 6, 9, 8, 7, 4];
var PROBABILITIES = [80, 60, 30, 20, 10, 20, 30, 60];

function generateProbabilities(lastNumpadDirection) {
    var lastIndexDirection = INDICES.indexOf(lastNumpadDirection);
    var shifted = PROBABILITIES.slice(PROBABILITIES.length - lastIndexDirection)
        .concat(PROBABILITIES.slice(0, PROBABILITIES.length - lastIndexDirection));
    var result = {};
    for(var i = 0, l = INDICES.length; i < l; i++) {
        result[INDICES[i]] = shifted[i];
    }
    return result;
}

Much of the complexity here comes from needing to deal with directions represented as arrows on the numpad–here, I convert them to 45-degree-offsets going clockwise from northwest, shift the probabilities array, and then convert back to your desired numpad format. Illustrated:


Ruby script to create git repos I wrote my first ruby script, by this tutorial. It creates a local and remote git repo.

I have been working for about 3 years with PHP, and somehow I find that my ruby script looks incredibly ugly.

Is it correct to use a class for that?
What can I make better?
Are there any security gaps in it?
Is the usage in for true and false in the empty_name? and folder_exists? methods correct?

#!/usr/bin/env ruby

require 'fileutils'
#require 'colorize'

class Repo

  LOCAL_PATH = 'C:\xampp\htdocs'
  REMOTE_PATH = 'C:\Users\Windows\Desktop\Dropbox\repos'
  UPSTREAM_NAME = 'dropbox'

  def initialize
      print 'Name: '
      @name = gets.chomp

      if ! folder_exists?

          create_local_repo
          create_files_and_commit
          create_remote_repo

      end
  end

  private

  # Check if name is emtpy.
  def empty_name?
      if @name.empty?
          print "You need to pass a name: "
          @name = gets.chomp
          empty_name?

          true
      end

      false
  end

  # Check if folder exists on local and remote path.
  def folder_exists?

      empty_name?

      if File.exists? "#{LOCAL_PATH}/#{@name}"
          print "Directory `#{@name}` exists in #{LOCAL_PATH}. Pass a new name: "
          @name = gets.chomp

          folder_exists?
      elsif File.exists? "#{REMOTE_PATH}/#{@name}.git"
          print "Directory `#{@name}.git` exists in #{REMOTE_PATH}. Pass a new name: "
          @name = gets.chomp

          folder_exists?
      end

      false
  end

  # Create local folder and initialize git.
  def create_local_repo
      Dir.chdir "#{LOCAL_PATH}" do `git init #{@name}` end
  end

  # Create .gitignore and commit.
  def create_files_and_commit
      Dir.chdir "#{LOCAL_PATH}/#{@name}" do
          File.open ".gitignore", 'w+' do |f|
              f.write ".idea\n/vendor\n/node_modules\n.Thumbs.db\n.DS_Store"
          end

          `git add .`
          `git commit -m "initialize commit"`
      end
  end

  # Create empty remote repo.
  def create_remote_repo
      `git init --bare #{REMOTE_PATH}/#{@name}.git`

      Dir.chdir "#{LOCAL_PATH}/#{@name}" do
          `git remote add #{UPSTREAM_NAME} #{REMOTE_PATH}/#{@name}.git`
          `git push -u #{UPSTREAM_NAME} master`
      end
  end

end

Repo.new

The structure is kinda confusing. empty_name? does a lot more than just tell you if the name's empty, and so does folder_exists?. Meanwhile, the constructor is the first place you ask for a name - but it's not checked there. create_remote_repo doesn't just create a repo, it also sets it as upstream for the local one. Which, in turn, means that create_local_repo must be called before create_remote_repo, but that isn't made explicit in the code.

In all, you methods just seem to be doing more, or doing something different than what they claim.

There's also a straight-up bug: empty_name? will never return true. If the string's empty it'll just call itself repeatedly until you finally give in and enter a string. At that point, it'll return all the way back through the stack, and... return false in the end. The true line does nothing by itself.
folder_exists? does something similar; it'll also always return false.

You class is also called Repo, though it doesn't actually model a repository. A name like RepoGenerator would be more accurate, though I'm on the fence about whether a class is even necessary in this case. You basically just call Repo.new and it does everything and the script exits; there's little need to keep state.

I'd also avoid the hardcoded file path constants. They make the script unusable for anyone but you - and perhaps even for yourself if you switch computer.

However, regardless of implementation, I find the function of this script somewhat more suspicious.

Your remote repo isn't actually "remote". It's just as local as your other one, it just happens to get copied by Dropbox. And since it's copied from computer to computer there's no guarantee it's in sync. If you have 2 computers, you basically end up with 2 "remote" repos, and 2 local ones.

Moreover, you don't want Dropbox messing with git's own files. If something gets corrupted by Dropbox, your "remote" repo may break. Or worse: It breaks silently. You then pull from that repo, and now you've broken your local repo too.

You might as well simply place your "local" repo in Dropbox, and call it a day. If you want a proper remote repo, use GitHub, Bitbucket, or Beanstalk - or set up your own server.

Secondly, you could just set up a ~/.gitignore_global file on your computer(s) to make sure files like .DS_Store etc. are always ignored. No need for a local .gitignore in every repo that just lists the same files every time.

If you do want to list some files, use a heredoc:

ignore_list = <<-LIST
# System files
.Thumbs.db
.DS_Store

# IDE files
.idea

# Generated/downloaded files
/vendor
/node_modules

LIST

Other stuff:

You don't handle special characters (e.g. whitespace) in your paths. A name like "my awesome repo" would probably pass all your checks, but fail when it has to actually create a repo. Take a look at the Shellwords module and do something like: %x{ git init #{name.shellescape} }
In fact, if a command fails, your script will probably just keep going. For instance, let's say that git init fails: Your script will happily continue with git add . (which fails) and git commit (which fails too). Kinda pointless.
In other words, if you run a shell command, you'll probably want to check its return value (e.g. $?.success? - $? gets set when you execute a shell command). You can also consider combining a few commands and let the shell handle things, e.g. git add . && git commit -m 'Initial commit'
In git parlance, the first commit is the initial commit. Not "initialize commit" - in fact, that doesn't really make much sense.

To sum up, and to answer your questions directly:

  Is it correct to use a class for that?

You can, although it's probably not called for in this case. Your class, as it is right now, is just a collection of methods. You can't really use a Repo instance for anything. A module would be fit the bill better, though for a simple script like this, you might as well just write some methods.

  What can I make better?

Well, see above :)
Basically, refactor it so your methods only do what they say, and only say what they do; don't repeat yourself; etc. etc..

  Are there any security gaps in it?

"Security gaps" is debatable. I doubt this script will somehow get you hacked, or something. There are safety gaps though, such as not escaping strings before using them in commands, and not checking that previous commands actually worked.
And there's the idea of have two local repos, where one just happens to get copied by Dropbox, which is sort of odd.

  Is the usage in for true and false in the empty_name? and folder_exists? methods correct?

Well, no, not quite. As mentioned, those methods can't actually return anything but false every time. ?-methods are basically yes/no questions, but yours do a bunch of stuff - seemingly to avoid answering "yes". Just make them "honest", and just have them say true or false.


State logger for optimization problems I am working on a C++ project for solving a class of optimization problems. The project will be header-only, and for now, I would like to stay with C++11 (so, not fold-expressions).

To be able to support some basic logging functionality, I have started writing the below logger class. Any comment/feedback is welcome. However, I would like to ask specifically about your suggestions on how to restructure the code, if at all, to make it more efficient.

#ifndef LOGGER_HPP_
#define LOGGER_HPP_

#include <chrono>
#include <cstdint>
#include <type_traits>
#include <vector>

namespace utility {
namespace logger {

template <class float_t, bool log_x_v, bool log_g_v> struct logger_t {
  template <class InputIt1, class InputIt2>
  void operator()(const std::size_t k, const float_t fval, InputIt1 xbegin,
                  InputIt1 xend, InputIt2 gbegin, InputIt2 gend) {
    tend = std::chrono::high_resolution_clock::now();
    const auto telapsed =
        std::chrono::duration<float_t, std::chrono::milliseconds::period>(
            tend - tstart);
    iterations.push_back(k);
    times.push_back(telapsed.count());
    fvalues.push_back(fval);
    log_x(xbegin, xend, std::integral_constant<bool, log_x_v>{});
    log_g(gbegin, gend, std::integral_constant<bool, log_g_v>{});
    tstart = std::chrono::high_resolution_clock::now();
  }

private:
  template <class InputIt>
  void log_x(InputIt xbegin, InputIt xend, std::true_type) {
    xvalues.emplace_back(xbegin, xend);
  }
  template <class InputIt> void log_x(InputIt, InputIt, std::false_type) {}
  template <class InputIt>
  void log_g(InputIt gbegin, InputIt gend, std::true_type) {
    gvalues.emplace_back(gbegin, gend);
  }
  template <class InputIt> void log_g(InputIt, InputIt, std::false_type) {}

  std::vector<std::size_t> iterations;
  std::vector<float_t> times;
  std::vector<float_t> fvalues;
  std::vector<std::vector<float_t>> xvalues, gvalues;
  std::chrono::time_point<std::chrono::high_resolution_clock> tstart{
      std::chrono::high_resolution_clock::now()},
      tend;
};

template <class float_t> using value = logger_t<float_t, false, false>;
template <class float_t> using decision = logger_t<float_t, true, false>;
template <class float_t> using gradient = logger_t<float_t, false, true>;
template <class float_t> using full = logger_t<float_t, true, true>;

} // namespace logger
} // namespace utility

#endif

Basically, the class is not complete, yet. I will be adding iterator support, read/write csv/binary functionality and some locking mechanism, later on, to be able to support thread-safe logging of values inside the logger.

Specifically, what I would like to ask is if I need to have some level of abstraction to make the unused private member variables disappear. The tag dispatch on log_x and log_g member functions seem to be OK when it comes to optimizing out the empty function calls, I hope. But then, when there is no logging needed for x and g, private member variables xvalues and gvalues are redundant. Should I care about them, or should I leave it as is for the sake of maintainability and ease of reading the code?

You could use a better name for the template argument float_t, maybe value_t

Maybe you could combine both log functions as they are very similar:

template <typename Container, typename InputIt>
void log(Container &c, InputIt begin, InputIt end, std::true_type) {
    c.emplace_back(begin, end);
  }


Three ways to sum the values of letters in words I've created these 3 functions to count the value of letters in a word.

All these functions are functionally the same.

Apparently List comprehensions (count_word3) are usually seen as the most pythonic in these situations. But to me it is the least clear of the 3 examples.

Have I stumbled across something that is so simple that lambdas are acceptable here or is "the right thing to do" to use the list comprehension?

Or is there something better?

def count_word1(w):
    def count(i, a):
        return i + ord(a) - 64
    return reduce(count, w, 0)

def count_word2(w):
    return reduce(lambda a,b : a + ord(b) - 64, w, 0)

def count_word3(w):
    vals = [ord(c) - 64 for c in w]
    return sum(vals)

how's example 3 the least readable? I would write it a little better but you've got it right already:

def count_word3(word):
    return sum(ord(c) - 64 for c in word)


Basic financial calculation library I'm trying to build a JavaScript library that does basic financial calculations, such as NPV, IRR, PV, FV, etc. So far I've added the NPV functionality only, and it's passed my test cases. But I want to know if the code overall can be written better.

(function () {
    /*
This library depends on underscore.js
*/
    'use strict';
    var root = this, previousFin = root.Fin, Fin = root.Fin = {}, _ = root._;

    Fin.VERSION = '0.0.1';
    Fin.PRECISION = 4; // floating point precision

    /*
Runs Fin.js in noConflict mode and
returns reference to this Fin object
*/
    Fin.noConflict = function () {
        root.Fin = previousFin;
        return this;
    };

    /*
NPV - Net Present Value
rate = the periodic discount rate
example: If the discount rate is 10% enter 0.1, not 10.
payments = an array or object (keys are the year numbers) of payments
example: [-100, 50, 60] means an initial cash outflow of 100 at time 0,
then cash inflows of 50 at the end of the period one, and 60 at
the end of the period two.
If you pass {0: -100, 2:50}, then the payment at the end of the
year one is assumed to be 0.
*/
    Fin.npv = function (rate, payments, precision) {
        if (isNaN(rate)) {
            /* rate needs to be a number */
            return null;
        }
        if (_.isArray(payments)) {
            /* all elements of the array need to be numbers */
            if (!_.all(payments, function (elem) { return !isNaN(elem); })) {
                return null;
            }
        } else if (_.isObject(payments)) {
            /* all key, value pairs of the object need to be numbers */
            if (!_.all(payments, function (key, value) { return !isNaN(key) && !isNaN(value); })) {
                return null;
            }
        } else {
            /* payment needs to be either an array or an object */
            return null;
        }
        if (typeof (precision) === 'undefined' || isNaN(precision)) {
            precision = this.PRECISION;
        }
        var i, npv = 0;
        for (i in payments) {
            if (payments.hasOwnProperty(i)) {
                npv += payments[i] / Math.pow((1 + rate), i);
            }
        }
        return npv.toFixed(precision);
    };
}).call(this);

Avoid getting a reference to the global object ("window") using this; it won't work in strict mode. If this is only meant to run in the browser, just use window; otherwise you can do this:

var root = (1,eval)('this');

Try to declare variables at the top of the function.
Regarding isArray: Favor feature detection. Does it need to have a length property? A push function?
Regarding return null: Just return. This will return undefined, which should be close enough 99.9% of the time.
typeof (precision) === 'undefined' can simply be !precision in this case.
all those early isNaN checks seem redundant, why not just do the checks in the main loop?  Or, better yet, leave out the checks entirely, let the calculation fail, and the function will return NaN by itself, which makes more sense than null or undefined.

Incidentally, these changes will remove the underscore.js dependency.

(function () {

    'use strict';

    var root = (1,eval)('this'),
        previousFin = root.Fin,
        Fin;

    root.Fin = Fin = {};

    Fin.VERSION = '0.0.1';
    Fin.PRECISION = 4; // floating point precision

    /*
    Runs Fin.js in noConflict mode and
    returns reference to this Fin object
    */
    Fin.noConflict = function () {
        root.Fin = previousFin;
        return this;
    };

    /*
    NPV - Net Present Value
    rate = the periodic discount rate
    example: If the discount rate is 10% enter 0.1, not 10.
    payments = an array or object (keys are the year numbers) of payments
    example: [-100, 50, 60] means an initial cash outflow of 100 at time 0,
    then cash inflows of 50 at the end of the period one, and 60 at
    the end of the period two.
    If you pass {0: -100, 2:50}, then the payment at the end of the
    year one is assumed to be 0.
    */
    Fin.npv = function (rate, payments, precision) {
        var i, npv = 0;
        if (!precision || isNaN(precision)) {
            precision = this.PRECISION;
        }
        for (i in payments) {
            if (payments.hasOwnProperty(i)) {
                // do the NaN check here if you want.
                // if (isNaN(payments[i])) return;
                npv += payments[i] / Math.pow((1 + rate), i);
            }
        }
        return npv.toFixed(precision);
    };
}());


Conditional merge with Ruby to include a parameter if it hasn't been included I have the following function, and I have a hunch it can be written more concisely in Ruby.

def template_params
  filtered_params = params[:template].permit!
  filtered_params[:published] ||= false
  filtered_params
end

The end result is that filtered_params has published: false if the params hash doesn't have published as a key.

I'm wondering if there is some kind of conditional merge w/ Ruby to turn that code into a 1-liner.

Essentially, the params hash includes "what has been changed" and when :published becomes false (it's a checkbox that gets unchecked), it naturally doesn't get included in the params hash. I wanted to pass in the fact that :published became false when it got unchecked.

I do not need to pass in published: false by default, because whatever the default is, is fine. It is the changed state that I need, but from false to true. The alternative works: if the checkbox is checked, it becomes true and gets passed through without issue.

This is more an exercise of how can I turn this bit of code into something more "elegant" and maybe a 1-liner.  As it stands, it does exactly what I need it to do.

You want to merge params with one or more default values. How about, well, #merge?

{"published" => false}.merge(params[:template].permit!)

Only trick, if you can call it a trick, is that the default hash must use string keys, not symbols, despite the params object accepting either. The resulting hash will also be keyed with strings only.

Alternatively, you can use HashWithIndifferentAccess to get around this:

defaults = HashWithIndifferentAccess.new({ published: false })
filtered = defaults.merge(params[:template].permit!)

And filtered will be a HashWithIndifferentAccess much like params produces.

All that said, the better way to handle all this is to send the right parameters in the first place. With the code above you're always assuming that published should be set to false unless told otherwise. But there could be all sort of reasons for why only a subset of parameters are sent; if published is intentionally left out, it'll suddenly be filled in - perhaps wrongly - by the controller.

Relatedly, if the parameter is required, raise an exception if it's missing and tell the client. It's not the controller's job to fill in the blanks when faced with incomplete information. Conversely, it's the client's job to send the parameters that are necessary or required.

As I said in a comment, if you use Rails' regular checkbox helpers, they'll produce a hidden input with the checkbox's "unchecked" value, in addition to the checkbox input itself. If the checkbox is checked, it'll override the former, unchecked value. This is the typical (and better) way to solve this.

So in short, if you want to merge to hashes, use #merge. But in this case, you probably don't want to do that.