Trie Tree match string containing ? and *

1. Review

The implementation with defaultdict is very elegant. I like it.
The code is not portable to Python 3 because of the use of the print statement. It would be easy to make it portable using from __future__ import print_function.
Set-like data structures contain elements, so element would be a better variable name than source.
It's inconvenient to create a trie from some other data structure like a list of strings: you have to call the insert method for each element you want to add. It would be better if the constructor took an optional iterator and inserted all the elements, just like the constructors for other collections in Python.
Instead of self.isEnd == True, just write self.isEnd, and similarly for other comparisons against True.
The attributes children and isEnd are only intended for use by the TrieNode class itself. It's conventional to give such attributes names starting with _.
The code for insert starts like this:

if not source:
    return

which means that if you try to add the empty string to the trie, it apparently succeeds, but in fact the empty string was not added. This is misleading. If you really want to prevent the caller adding the empty string, then raise an exception:

if not source:
    raise ValueError("empty string not supported")

But this seems inconvenient to me, so instead I suggest removing these two lines and allowing the empty string as an element. But this reveals a problem with the search method:

>>> root = TrieNode()
>>> root.insert('')
>>> root.search('*')
False

To fix this, revise the search method as follows:

elif source[0] == '*':
    if node.search(source[1:]):
        return True
    for (ch, node) in self.children.items():
        if node.search(source) == True:
            return True
    return False

(but see below for further improvements to this code).
In this code:

for (ch,node) in self.children.items():
    if node.search(source[1:]) == True:
        return True
return False

you don't make any use of ch, so it would be simpler to write:

for node in self.children.values():
    if node.search(source[1:]):
        return True
return False

which is the same as:

return any(node.search(source[1:]) for node in self.children.values())

Instead of computing source[1:] on each iteration of the loop, compute it once and remember it in a local variable:

rest = source[1:]
return any(node.search(rest) for node in self.children.values())

But better still, reorganize the search so that it remembers the current index into the search term, then you don't have to construct all these substrings. See below for how this might be implemented.
The interface to the search method makes it impossible to tell if strings containing wildcards are elements of the trie. Consider:

>>> import random
>>> root = TrieNode()
>>> root.insert(random.choice(['?', 'x']))
>>> root.search('?')
True

Is '?' a member of the trie or not? It would make sense to provide an alternative search method that doesn't use wildcards: for example the caller could write 'element' in trie instead of trie.search('element'). This requires a __contains__ method, which is straightforward to implement since it doesn't need to consider wildcards:

def __contains__(self, element):
    node = self
    for k in element:
        if k not in node._children:
            return False
        node = node._children[k]
    return node._end

Now you can accurately determine membership for strings containing wildcards:

>>> '?' in root
False

The search method only tells you whether some element in the trie matched the search term, and doesn't tell you which element or elements match. When the search term contains wildcards, it would be more useful if the search generated the matching elements. For example, if you have added the words in a dictionary to a trie, then you'd like to be able to query 'p?t' and get out 'pat', 'pet', 'pit', 'pot', 'put'. See the revised code in §2 below, and further discussion in §3.
The data structure presents a set-like interface (it represents a collection of distinct strings where you can add new elements and test elements for membership). It would therefore make sense to design the interface so that it uses the same method names as Python's built-in sets, that is, add instead of insert, and to implement more of the set interface, for example __iter__, __len__, __or__, __and__ and so on.

The easiest way to do this is to inherit from collections.abc.Set. The idea is that your class implements the __contains__, __iter__ and __len__ methods, and collections.abc.Set class implements the other set methods in terms of these. See the revised code in §2 below.
The code has test cases, but it's hard to check that they are correct: you have to carefully compare the output against the expected output. It would be better to get the computer to do the work here, using the features in the unittest module.

2. Revised code

from __future__ import print_function
from collections import defaultdict
from collections.abc import Set

class TrieNode(Set):
    """A set of strings implemented using a trie."""

    def __init__(self, iterable=()):
        self._children = defaultdict(TrieNode)
        self._end = False
        for element in iterable:
            self.add(element)

    def add(self, element):
        node = self
        for s in element:
            node = node._children[s]
        node._end = True

    def __contains__(self, element):
        node = self
        for k in element:
            if k not in node._children:
                return False
            node = node._children[k]
        return node._end

    def search(self, term):
        """Return the elements of the set matching the search term, which may
        include wildcards ? (matching exactly one character) and *
        (matching zero or more characters).

        """
        results = set() # Set of elements matching search term.
        element = []    # Current element reached in search.
        def _search(m, node, i):
            # Having just matched m, search for term[i:] starting at node.
            element.append(m)
            if i == len(term):
                if node._end:
                    results.add(''.join(element))
            elif term[i] == '?':
                for k, child in node._children.items():
                    _search(k, child, i + 1)
            elif term[i] == '*':
                _search('', node, i + 1)
                for k, child in node._children.items():
                    _search(k, child, i)
            elif term[i] in node._children:
                _search(term[i], node._children[term[i]], i + 1)
            element.pop()
        _search('', self, 0)
        return results

    def __iter__(self):
        return iter(self.search('*'))

    def __len__(self):
        return sum(1 for _ in self)

3. Efficiency

The Set class requires implementations of __iter__ and __len__ so in the code above I implemented them as simply as possible to avoid making the answer too complicated. But if this is going to be used for anything serious then it would be a good idea to provide specialized implementations with better performance. The __iter__ method could be implemented like this, using the stack of iterators pattern:

def __iter__(self):
    element = ['']
    stack = [iter([('', self)])]
    while stack:
        for k, node in stack[-1]:
            element.append(k)
            if node._end:
                yield ''.join(element)
            stack.append(iter(node._children.items()))
            break
        else:
            element.pop()
            stack.pop()

And the __len__ method can be made \$O(1)\$ if you maintained in each node a count of the number of elements in the trie rooted at that node:

def __init__(self, iterable=()):
    self._children = defaultdict(TrieNode)
    self._end = False
    self._len = 0
    for element in iterable:
        self.add(element)

def add(self, element):
    node = self
    for s in element:
        node = node._children[s]
    if not node._end:
        node._end = True
        node._len += 1
        node = self
        for s in element:
            node._len += 1
            node = node._children[s]

def __len__(self):
    return self._len

I made the search method return a set of elements matching the search term. It would be more general if this method generated the matching elements one at a time. This would more efficiently support use cases such as finding any element matching a search term (by stopping the iteration after the first result), or determining if there is a unique element matching a search term (by stopping the iteration after the second result).

In Python 3 it's easy to implement this using the yield from statement, but in Python 2 you have to write:

for result in _search(...):
    yield result

which is rather verbose. But see below for an alternative.
The implementation of the search method is inefficient in the way it handles the * wildcard. The problem is that when the current character in the search term is *, the search descends the trie twice at each node (once having consumed the *, once not). This means that each node in the trie may be visited many times in the course of the search.

It is possible to revise the code so that each node in the trie is visited at most once, by modifying the search method so that instead of considering a single index i in the search term, it considers a set of indexes.

Here's one way to implement this, again using the stack of iterators pattern:

def search(self, term):
    """Generate the elements of the set matching the search term, which
    may include wildcards.

    """
    def _add(indexes, i):
        # Add i to set of indexes into term, taking account of wildcards.
        while i < len(term) and term[i] == '*':
            indexes.add(i)
            i += 1
        indexes.add(i)

    indexes = set()
    _add(indexes, 0)
    if self._end and len(term) in indexes:
        yield ''

    indexes_stack = [indexes] # Stack of sets of indexes into term.
    element = ['']            # Current element reached in search.
    iter_stack = [iter(self._children.items())] # Stack of iterators.

    while iter_stack:
        for k, node in iter_stack[-1]:
            new_indexes = set()
            for i in indexes_stack[-1]:
                if i >= len(term):
                    continue
                elif term[i] == '*':
                    _add(new_indexes, i)
                elif term[i] == '?' or term[i] == k:
                    _add(new_indexes, i + 1)
            if new_indexes:
                element.append(k)
                if node._end and len(term) in new_indexes:
                    yield ''.join(element)
                indexes_stack.append(new_indexes)
                iter_stack.append(iter(node._children.items()))
                break
        else:
            element.pop()
            indexes_stack.pop()
            iter_stack.pop()


99 Haskell problems, problem 9: Pack function

Let's improve the function step by step, focusin on readability as well as on the algorithm itself.

I believe the type of the function is not what it should be. We get a list of characters, and we should produce a list of lists:

pack1 :: [Char] -> [[Char]]
pack1 xs = foldl (\acc x -> if not(null acc) && [x] `isPrefixOf` (last acc)
                            then (init acc) ++ [(last acc) ++ [x]]
                            else acc ++ [[x]]) [] xs

The function doesn't require that we compare Chars. In such a case, it's better to make its type as generic as possible. Not only it makes it usage more generic, it also helps us ensure that we restrict to the specific problem of packing, not introducing anything particular about Chars. So the generalized type would be (Eq a) => [a] -> [[a]].
The major problem is that the function has O(n^2) time complexity. This is because calling last on a list, or appending someting to its end using ++ is O(n), and doing it n-times gets us to O(n^2). This is definitely something we'd like to avoid. The solution is easy: Instead of appending to the list, let's just prepend, and reverse it at the end. Since reverse is O(n), the result will be O(n) as well. Let's apply this idea to both the "big" list as well as its sublists:

pack2 :: (Eq a) => [a] -> [[a]]
pack2 xs = reverse . map reverse $
           foldl (\acc x -> if not (null acc) && [x] `isPrefixOf` (head acc)
                            then (x : (head acc)) : (tail acc)
                            else [x] : acc) [] xs

Most likely we could also omit map reverse, as the sublists always contain equal elements. This assumes that if two elements are equal, they're indistinguishable, which is usually a reasonable assumption. But I kept it there for completeness, and it doesn't affect the asymptotic complexity.
In Haskell it's customary to avoid if/then/else in favor of pattern matching. Not only it's often shorter, but it provides stronger guarantees and/or less required reasoning about the code. In the if condition, the reader needs to infer that using last or head on acc is allowed because we checked it's non-empty. If we instead factor out the inner function and use patternmatching,

pack3 :: (Eq a) => [a] -> [[a]]
pack3  = reverse . map reverse . foldl f []
  where f acc@(as@(a:_):ass) x | a == x     = (x : as) : ass
        f acc                x              = [x] : acc

we don't need to use any potentially dangerous functions such as head/tail and do any checking.

Here we also introduced another small improvement called η-conversion (eta). The idea is that \x -> g x is equivalent to g.
We'd like to avoid calling reverse.

The problem is that we're using foldl to produce the final list. By its nature, if we use foldl for converting lists, it reverses the order of the elements (or causes O(n^2) time complexity). Although we want to consume the list from left to right, it's more appropriate to use foldr here. (I'd say that the only reason to use foldl is if the result needs the whole list - for example when summing a list.)

Let's start with implementing map using foldr. (I'll hide the example, if you want to try it yourself. If you don't, just hover the mouse over it.)

   map' f = foldr (\x r -> f x : r) []

 The idea is to express how to process a single element, knowing how the rest of the list is processed.

Let's apply the same principle to pack:

pack4 :: (Eq a) => [a] -> [[a]]
pack4  = foldr f []
  where f x acc@(as@(a:_):ass) | a == x     = (x : as) : ass
        f x acc                             = [x] : acc

This isn't very different from the previous example. The main difference is that we (seemingly!) consume elements from the right, so we have no problem with reversing the order.
And still ... we can ask for more. The problem is that the function isn't lazy enough (even though we're using foldr). We'd like for example that

head $ pack4 ('a' : 'c' : repeat 'b')

produced "a", instead of looping forever - the required information is available. The reason for this is that we inspect the accumulator (by matching it with the pattern acc@(as@(a:_):ass) which is not irrefutable) at each iteration, which means that in order to produce a single element we first force the processing of the rest of the list.

Instead, we can create the result structure first to hold the current element without forcing the results of processing the rest of input list, so that this structure will be filled with more values only when/if it is so demanded:

pack5 :: (Eq a) => [a] -> [[a]]
pack5  = foldr f []
  where
    f x r = (x:u):w
            where
              (u,w) = case r of
                        []                 -> ([],[])
                        (g:gs) | head g==x -> (g ,gs)
                        gs                 -> ([],gs)

See also the source code for group, which is also implemented with the lazy behavior.


I would suggest not to recreate the instance of XmlSerializer each time, because it creates an assembly in memory that is not reused / unloaded. To avoid that, just create the instance as static field of the class or use something like a XmlSerializerCache.

see also: XmlSerializer class may result in a memory leak and poor performance

The value of procedure could be returned by SetProcedureXmlData and passed to SaveProcedureSummary. That makes it more clear the the methods have to be called in that order.
The instance variable procedureDataManager could be readonly
You could drop the isSuccess variable if you return the result of SaveProcedureSummary or false in the catch block.
It would be more readable if the line in SaveProcedureSummary is splitted to 2 or 3 lines.
The using is OK, but ther is no need to close the reader because it will be closed when the reader is disposed by the using.


k-means clustering algorithm implementation

N.b. I'm pretty sure you can find optimised replacements of many of
these functions in either the NumPy or SciPy library.

Edit: I don't immediately see a reason for a slowdown except the
addition of new clusters.  A bigger sample set that shows the behaviour
would be nice, together with some timings and probably running a
profiler on it.

In general you should likely use a NumPy array or matrix instead of
lists of lists as they are optimised for storing numeric data(!).  That
would likely also eliminate the need for some of these functions or
considerably reduce their length.  Also store all data in the same
containers - that way you don't have to create new functions like
equals and contains yourself to compare between different
representations.

The X/Y definitions at the start are more of a WTF for me.  I'd
get rid of that by writing code that's either independent on the
number of dimensions, or just use 0/1 - IMO it's not magic enough
to warrant separate constants.
get_first isn't neccessary - the pattern is obvious enough to not
require encapsulation in a function.  The zero is also optional.
Typo in cost signature, should be centroids.
The pattern for i in range(len(...)): appears a couple of times and
should be replaced with proper iteration over some helper generator.
E.g. in cost the parallel iteration over both centroids and
clusters should be done by zip (itertools.izip in Python 2.7 if
used in the same way).  It can also be simplified by using another
SciPy function,
cdist.
If you still need the index, use enumerate.
You can also often get away with using the squared euclidean if the
exact value isn't relevant, just the relation between different
distances.
The pattern to initialise a list of empty lists should probably use
_ instead of i to make it obvious that the variable serves to
further purpose.
print should be used as a function to make it more uniform.  Also,
use one of the various formatting options (% or format) to get rid
of the additional str calls.
compute_centroids can be simplified with a list comprehension.

Looks like this now:

from scipy.spatial import distance
import numpy as np
import random
from itertools import izip

def cost(centroids, clusters):
    return sum(distance.cdist([centroid], cluster, 'sqeuclidean').sum()
            for centroid, cluster in izip(centroids, clusters))

def compute_centroids(clusters):
    return [np.mean(cluster, axis=0) for cluster in clusters]

def kmeans(k, centroids, points, method):
    clusters = [[] for _ in range(k)]

    for point in points:
        clusters[closest_centroid(point, centroids)].append(point)

    new_centroids = compute_centroids(clusters)

    if not equals(centroids, new_centroids):
        print("cost [k={}, {}] = {}".format(k, method, cost(new_centroids, clusters)))

        clusters = kmeans(k, new_centroids, points, method)

    return clusters

def closest_centroid(point, centroids):
    min_distance = float('inf')
    belongs_to_cluster = None
    for j, centroid in enumerate(centroids):
        dist = distance.sqeuclidean(point, centroid)
        if dist < min_distance:
            min_distance = dist
            belongs_to_cluster = j

    return belongs_to_cluster

def contains(point1, points):
    for point2 in points:
        if point1[0] == point2[0] and point1[1] == point2[1]:
        # if all(x == y for x, y in izip(points1, points2)):
            return True

    return False

def equals(points1, points2):
    if len(points1) != len(points2):
        return False

    for point1, point2 in izip(points1, points2):
        if point1[0] != point2[0] or point1[1] != point2[1]:
        # if any(x != y for x, y in izip(points1, points2)):
            return False

    return True

if __name__ == "__main__":
    data = [[-19.0748,     -8.536       ],
            [ 22.0108,      -10.9737    ],
            [ 12.6597,      19.2601     ],
            [ 11.26884087,  19.90132146 ],
            [ 15.44640731,  21.13121676 ],
            [-20.03865146,  -8.820872829],
            [-19.65417726,  -8.211477352],
            [-15.97295894,  -9.648002534],
            [-18.74359696,  -5.383551586],
            [-19.453215,   -8.146120006],
            [-16.43074088,  -7.524968005],
            [-19.75512437,  -8.533215751],
            [-19.56237082,  -8.798668569],
            [-19.47135573,  -8.057217004],
            [-18.60946986,  -4.475888949],
            [-21.59368337,  -10.38712463],
            [-15.39158057,  -3.8336522  ],
            [-40.0,          40.0       ]]
    k = 4

    # k-means picking the first k points as centroids
    centroids = data[:k]
    clusters = kmeans(k, centroids, data, "first")


You can use built-in functions instead. Like this for example:

function array_mask(array $source, array $filter)
{
    $source = array_intersect_key($source, $filter);

    foreach ($source as $key => $value)
        if (is_array($value))
            $source[$key] = array_mask($source[$key], $filter[$key]);

    return $source;
}

$source = array(
    'page' => array(
        'q' => 'alkflsdj',
        'x' => 1,
        9   => 'm'
    ),
    'news' => 7
);
$filter = array(
    'page' => array(
        9 => ''
    ),
    'news' => ''
);

var_dump (array_mask($source, $filter));


If you are going to go to the trouble of creating Rank and Suit enum then use them.

Card( int rank, int suit ) throw(std::range_error)

I would have done

Card(Rank rank,Suit suit )

That way you can't accidentally go: Card(Card::Hearts,2) The compiler type checking will catch this at compiletime. Just use the enums all through your code. C++ is a strongly typed language use the compiler to catch your errors.

The exception specifications have been deprecated. They were mostly useless anyway (if you fail and throw an illegal exception it causes your application to terminate without unwinding the stack). So in general don't use them. For the one case where they are usfull (no throw specifications) they are good but I use them more as documentation to remind me that I should make sure the method does not throw.

But yes the constructor should throw an exception if it gets a value outside the expected range. There is no point in allowing an invalid object to propagate threw your code it will just cause problems with your invariants.

Prefer to use the initializer list rather than initializing variables in the constructor.

Card(Rank rank, Suit suit)
    : m_rank(rank)
    , m_suit(suit)

Again another place that it would have been nice to use the enum to make the code more eadable:

        if( rank < 1 || rank > 13 ) {

Try:

        if( rank < Ace || rank > King ) {

Hate getters/setter they spoil encapsulation. Do you really need to get the values of the card? Any operation that manipulates the card is usually a method on the card or a friendly class.

int rank() const { return m_rank; }
int suit() const { return m_suit; }

Don't try and write C++ like Java. We don;t need a string function. It is much more preferable (and flexable) to write an output (and probably input) stream operator.

std::string str() const
{
        return std::string( Ranks[m_rank] + Suits[m_suit] );
}

I would have done:

friend std::ostream& operator<<(std::ostream& stream, Card const& card)
{
    return stream << card.Ranks[card.m_rank] << card.Suits[card.m_suit];
}

Adding Cards to a deck. (Based on comment below)

#include <vector>

class Card
{
    public:
        enum Ranks { Ace = 1, Ten = 10, Jack, Queen, King };
        enum Suits { Spades, Hearts, Diamonds, Clubs };

        Card(Ranks rank, Suits suit){}
};

// Need to add the ++ operators for Ranks/Suits
Card::Ranks& operator++(Card::Ranks& r)
{
    return r = Card::Ranks(static_cast<int>(r)+1);
}
Card::Suits& operator++(Card::Suits& s)
{
    return s = Card::Suits(static_cast<int>(s)+1);
}

int main()
{
    std::vector<Card>   deck;

    // Does not look that verbose to me.
    for(Card::Ranks rank=Card::Ace;rank <= Card::King;++rank)
    {
        for(Card::Suits suit=Card::Spades;suit <= Card::Clubs;++suit)
        {
            deck.push_back(Card(rank, suit));
        }
    }

    // Compared to
    for(int rank=Card::Ace;rank <= Card::King;++rank)
    {
        for(int suit=Card::Spades;suit <= Card::Clubs;++suit)
        {
            deck.push_back(Card(rank, suit));
        }
    }
}


Is this a "pythonic" method of executing functions based on the values of a tuple?

In short, a dictionary of functions IS the Python equivalent of the switch statement.

However, your implementation is awful. Start by dropping the "switch" in your variable names. It is not a switch, it is a dictionary. And there is no point in using "dictionary" in the name either. Make a real name like "act_on_msg" but even that is weak in my opinion.

And don't even explain that you are using a dictionary as a switch in your comments. At most, mention that  actOnMsg[combomsg]() runs the action method just to help those who are not used to seeing parentheses in that context.


Trim certain characters from a string in JavaScript

There are many different ways to tackle trimming strings, a regex is probably the most popular, and there are lots of blogs and performance tests out there, (one such blog), that you can look at and decide which is best.

So continuing along the lines that you have demonstrated, then this may be of some help to you.

Javascript

/*jslint maxerr: 50, indent: 4, browser: true */

var trim = (function () {
    "use strict";

    function escapeRegex(string) {
        return string.replace(/[\[\](){}?*+\^$\\.|\-]/g, "\\$&");
    }

    return function trim(str, characters, flags) {
        flags = flags || "g";
        if (typeof str !== "string" || typeof characters !== "string" || typeof flags !== "string") {
            throw new TypeError("argument must be string");
        }

        if (!/^[gi]*$/.test(flags)) {
            throw new TypeError("Invalid flags supplied '" + flags.match(new RegExp("[^gi]*")) + "'");
        }

        characters = escapeRegex(characters);

        return str.replace(new RegExp("^[" + characters + "]+|[" + characters + "]+$", flags), '');
    };
}());

/*jslint devel: true */

console.log(trim(" - hello** +", "+*- "));

On jsfiddle

Of course, you can customise which characters you wish to escape, or flags to test for, as per your own needs. Regular Expressions


Decomposing bitfield with Haskell: is recursion the only way to do this?

A more common way of "initializing" values on the first call is to define an inner worker function.

findPower :: Int -> Int
findPower bitfield = go 0
    where go i = ... -- Note that the bitfield argument is still in scope

Recursion is the standard way to loop in Haskell (so much so that in lots of code you will find locally defined recursive functions named loop), but your code might be more terse without explicit recursion.

Here's my first pass at a rewrite of the findPower function using higher order functions.

findPower :: Int -> Int
findPower i = last . takeWhile (<= i) $ map (2^) [0..]

Note that this function still isn't total, i.e. it will fail when passed Ints <= 0. Your version would recurse until 2^power overflows, whereas mine will fail fast with an empty list exception due to the use of last. It's considered good style to make your Haskell functions total, so here's my second pass.

findPower :: Int -> Maybe Int
findPower 0 = Just 0
findPower i | i < 0     = Nothing
            | otherwise = Just . last . takeWhile (<= i) $ map (2^) [0..]

Using Maybe here is probably overkill, and in fact you probably want your program to crash when given bad input! Here's a final version.

findPower :: Int -> Int
findPower i | i < 0     = error "findPower: Can't find power of negative number"
            | i == 0    = 0
            | otherwise = last . takeWhile (<= i) $ map (2^) [0..]

On this line | found = [flag] ++ ..., if you're adding a single element to the front of a list, this is just regular cons! Use | found = flag : ... instead.

You've defined a lot of constants in where clauses for your two functions which only get used once. Sometimes it can be helpful to give a name to something that might be non-obvious, but in this case it makes your code longer and arguably adds a bit of cognitive overhead to what are very simple transformations. For instance, I would consider this more readable.

getFlags currentBits flag | currentBits == flag = [flag]
                          | currentBits > flag  = flag : getFlags (currentBits - flag) (flag `div` 2)
                          | otherwise           = getFlags currentBits (flag `div` 2)

In your definition of main, you can omit the type annotation from read line and the compiler will deduce you want an Int due to the type signature of bithelp. On its own this is a small change, but leveraging the type system in this way is entirely natural and reduces the number of changes you would have to make if, say, you changed your functions to operate on Integers.

See if this isn't more clear.

main = do putStrLn "Enter a positive integer:"
          nstr <- getLine
          putStrLn "List of flags:"
          print $ bithelp (read nstr)

The last thing to do would be to eliminate the bithelp function by rewriting the getFlags function with a call to findPower inside of it, passed to an inner worker function. I'll leave that as an exercise to the reader.


It looks pretty good and definitely works, but I would approach this a little bit differently and try to avoid that closure - it's a little bit difficult to read and it's also unnecessary. You can use Object.keys if you want to be able to use an object for your routes parameter.

module.exports = function(app, routes) {
    // loop through routes
    var len = routes.length,
        keys = Object.keys(routes),
        // Define path and stream outside of the loop, at the top
        // of the module as private vars and reuse (they are
        // hoisted by JavaScript anyway.
        url,
        path,
        stream;

  // Use a while loop here because it doesn't matter which
  // order you setup these routes in. This is the least verbose.
  while(len--){
    url = keys[len];
    app.get(url, function(req, res) {
        path = routes[url];
        // create a readable stream
        stream = fs.createReadStream(path, {encoding: 'utf8'});

        stream
        .once('error', function() {
            res.status(500).end('Server error.');
            console.error('ERROR (staticize): Could not open readable stream to ' + path);
        })
        .once('readable', function() {
            // create headers
            var headers = {
                'Content-Type': mime.lookup(path)
            };

            // write head then pipe response
            res.writeHead(200, headers);
            stream.pipe(res);
        });
    });
  }
};

Personally, I would use an array with named properties instead of an Object for my routes, but key/value is OK. Something like this would allow you to just do a routes.forEach() and have direct access without having to use Object.keys:

staticize(app, [
    {
        url: '/foo',
        path: '../test/foo/bar.txt'
    }, {
        url: '/foo/more',
        path: '../test/foo-extra/bar.txt'
    }
]);

module.exports = function(app, routes) {
  // Use a forEach here instead of a loop
  routes.forEach(function(route){
    app.get(route.url, function(req, res) {
        // Save the route since we use it three times in here
        var path = route.path;

        // create a readable stream
        var stream = fs.createReadStream(path, {encoding: 'utf8'});

        stream
        .once('error', function() {
            res.status(500).end('Server error.');
            console.error('ERROR (staticize): Could not open readable stream to ' + path);
        })
        .once('readable', function() {
            // create headers
            var headers = {
                'Content-Type': mime.lookup(path)
            };

            // write head then pipe response
            res.writeHead(200, headers);
            stream.pipe(res);
        });
    });
  });
};


User input validation that might be unbreakable for C++

The #define NOMINMAX issue is easy to fix. To do so we need to remove(or move internally to our input function) the using namespace std; call as this pollutes our global namespace with max/min calls. See this for why it is bad practice to include the namespace.
Seeing as we only want to output our message string and do not care about copying, it is best to pass this to our function as const& std::string
I would change the function signature to make it easier to use in client code. Instead of passing in a temp var for input, just let the function return type T.
The type of low and high should be of the same type as T, not always int. For types such as double and float the comparisons may not always return expected results and may also need to be compared with some small epsilon value.
Finally we do not wish to modify low or high and should also pass them by const.

The final function might then look something like

#include<iostream>
#include<string>
#include<limits>

template<class T>
T ValidateInput(const T low, const T high, const std::string& message)
{
    using namespace std;

    T input = low - 1;

    while (input < low || input > high)
    {
        cout << message;
        while ((cin >> input).fail() || cin.peek() != '\n')
        {
            cin.clear();
            cin.ignore(numeric_limits<streamsize>::max(), '\n');
            cout << message;
        }
        cin.clear();
        cin.ignore(numeric_limits<streamsize>::max(), '\n');
    }

    return input;
}


Pairwise swap elements of a given linked list by changing links

It's weird that you picked append as your primitive for building test cases, when prepend would be so much simpler and faster — O(1) instead of O(n).

auto newNode = new Node{data, nullptr};

This is a very "modern" way of writing what would be more idiomatically written as

Node *newNode = new Node(data, nullptr);

I would weakly recommend the latter. And I would strongly recommend, if you do nothing else, at least calling out explicitly when you're working with raw (non-owning) pointers:

auto *newNode = new Node{data, nullptr};  // the asterisk means watch out!

auto temp{head};

Again, I'd write simply

Node *temp = head;

or at least

auto *temp = head;

The * signals the reader to watch out for pointer pitfalls (aliasing, memory leaks); the = signals the reader that an initialization is happening here. You might be surprised how easy it is to glance over auto temp{head}; surrounded by other lines of code and not even recognize that it's introducing the name temp!

//  pairwise swap of the elements in the given linked lists
void pairwiseSwap(Node **head_ref) {

Some coding guidelines tell you to pass the inout parameter head_ref by pointer here, instead of by reference. I'm going to assume you're following one of those guidelines.

return ;

is an unidiomatic whitespace style; most programmers would write

return;

You actually put an extra space before a lot of semicolons in this function (but not consistently). Are you French? ;)

You should definitely factor out the "swap two nodes" functionality into a named function. I somewhat suspect that this would do, but you'd have to draw it out on paper...

void swap_two_nodes(Node *&p, Node *&q) {
    assert(p->next == q);
    std::swap(p, q);
}

Alternatively — and since I've confused myself ;) — you could just write a recursive version of the whole thing:

Node *pairwise_swap(Node *head) {
    if (head && head->next) {
        Node *first = head->next;
        Node *second = head;
        Node *tail = pairwise_swap(head->next->next);
        head = first;
        first->next = second;
        second->next = tail;
    }
    return head;
}

Turning this into tail-recursion is left as an (easy) exercise for the reader.

// for even number of nodes
a = append(a, 15);

Appending an even number of nodes to an odd-length list does not result in an even-length list. Did you try running your test code? Did you look at the output and verify that it was correct? You should have!


You're over-complicating it.

First the static variable isn't required, all the variables in the scope of include/require statements are available in the document being included or required.  For example:

FileA.php

<?php
$variable1 = "Something";
require_once __DIR__ . "/FileB.php";

FileB.php

<?php
echo $variable1; // Outputs "Something"

So the $view variable will just be accessible in the template.php file.  That said I think your code could be changed up just a bit and be a lot cleaner.  I went through and tweaked it some with comments below.  Feel free to ask any questions you may have after reading through it:

View.php

<?php
class View {
    const DIR_VIEW = "/views/";

    function make ($view, $data = array()) {
        extract($data, EXTR_SKIP); // Extracts key/value pairs from data as variables

        ob_start();

        // Just require header here, later on you could add another option to make for layout,
        // to use different headers footers if needed.  Just include everything up to
        //
 in the header.php file.
        require __DIR__ . Views::DIR_VIEW . 'header.php';

        // Do this better, it's open to a directory traversal attack: http://en.wikipedia.org/wiki/Directory_traversal_attack
        require __DIR__ . Views::DIR_VIEW . $view . '.php';

        require __DIR__ . Views::DIR_VIEW . 'footer.php'; // Just require footer here

        return ob_get_clean();
    }
}

HomeController.php

<?php
class HomeController extends BaseController // Capitalize the H
{

    public function index()
    {
        // Add a return, have your router take the action method's return value and echo it.
        // This way your view class isn't just arbitrary printing things out, and will give you
        // flexibility later to have different types of returns (like for redirects you could
        // do Redirect::to('/'), similarly to laravel).

        return $this->view->make("common/home");
    }
}

common/home.php

Bam! <?php echo $someVariable; ?> Wow! Such Templates!

header.php

<!DOCTYPE html>

    <title><?php echo isset($title) ? $title : 'Your site'; ?></title>

        Home

footer.php

        &copy; Copyright 2014


Selecting kids for a Christmas play with similar heights

Time is mostly spent in I/O

I played with your program and found that most of the time was spent doing I/O.  However, from your comments, it appears you already improved your I/O so that your best solution is now 0.02 seconds instead of 0.05 seconds.

I took your improved I/O code and took it one step further.  What I did was read the entire input into a buffer using a single fread(), and then instead of using getchar_unlocked(), I just advanced a pointer through the buffer using *p++.

Why is *p++ faster than getchar_unlocked()?  Here is what getchar_unlocked() does (not exactly but something close to this):

char getchar_unlocked()
{
    if (stdin.ptr >= stdin.buffer_len) {
        refill_stdin_buffer();
        stdin.ptr = 0;
    }
    return stdin.buffer[stdin.ptr++];
}

So for every character you read, it does a check to see if it needs to reload the buffer.  If you read the entire input from stdin to a local buffer, you are essentially skipping and end of buffer check for each character you read.

Update: Later I found that my I/O changes didn't matter.  Both yours and mine were able to achieve 0.00 time, as long as radix sort was used (see next section).

Radix sort

Radix sort is 0.02 seconds faster than std::sort for this test.  I know because I tried it both ways.  First I tried radix sort, which took 0.00 seconds overall.  Then I replaced radix sort with std::sort and got 0.02 seconds.

In general, radix sort should be faster than quicksort for large arrays of integers.  The radix sort I used operates on bytes at a time, so for 32-bit integers it uses exactly 4 rounds.  On each round it does a counting pass followed by a rearranging pass.  So in total, it does approximately \$8n\$ operations to sort an array of \$n\$ integers, plus a fixed overhead of 1024 operations to deal with the buckets.

From my own testing, I found that radix sort is 5x faster than std::sort for sorting arrays of 10 million integers.  For 20000 integers, it is around 4x faster.

The final program, which achieved 0.00 seconds

Here is my program.  You can see in the SPOJ status that I got 0.00 seconds with it.  FYI, my first entry was a C program that got 0.00 seconds, but I changed it to the following C++ program:

#include <bits/stdc++.h>
#include <cstdio>

#define MAX                20000

// 30 testcases, 11 characters per number, 20000 numbers
char buf[MAX*11*30];
int array    [MAX];
int arrayTemp[MAX];

static inline int readInt(char **pPtr)
{
    char *p   = *pPtr;
    int   ret = 0;
    char  c   = *p++;

    while (c < '0')
        c = *p++;
    do {
        ret = (ret << 3) + (ret << 1) + c - 48;
        c   = *p++;
    } while (c >= '0');

    *pPtr = p;
    return ret;
}

void radixsort(int *a, int len)
{
    int *space       = arrayTemp;
    int *current     = a;
    int *scratch     = space;
    int *tmp         = NULL;
    int  radixByte   = 0;
    int  i           = 0;
    int  bucket[256];

    // Iterate once per byte.
    for (radixByte=0;radixByte<4;radixByte++) {
        int shift = (radixByte << 3);
        memset(bucket, 0, sizeof(bucket));

        // Count how many of each byte.
        for (i=0;i<len;i++)
            bucket[(current[i] >> shift) & 0xff]++;

        // Change bucket to be cumulative count.
        for (i=1;i<256;i++)
            bucket[i] += bucket[i-1];

        // Copy from current to scratch, using bucket counts.
        for (i=len-1;i>=0;i--)
            scratch[--bucket[(current[i] >> shift) & 0xff]] = current[i];

        // Switch arrays
        tmp     = current;
        current = scratch;
        scratch = tmp;
    }
}

int main(void)
{
    char *p = buf;
    fread(buf, 1, sizeof(buf), stdin);

    int t = readInt(&p);
    while (t-- > 0) {
        int n = readInt(&p);
        int k = readInt(&p);

        for (int i = 0; i < n; i++)
            array[i] = readInt(&p);

        if (k == 1) {
            puts("0");
            continue;
        }

        radixsort(array, n);

        int diff = INT_MAX;
        n -= k;
        k--;
        for (int i = 0; i <= n; i++) {
            int newDiff = array[i+k] - array[i];
            if (newDiff < diff)
                diff = newDiff;
        }
        printf("%d\n", diff);
    }
    return 0;
}


The number of combinations of size k from a set of size n can be computed with choose(n, k) in R, and if you have multiple k values you can just pass in a vector for k. Therefore your code can be simplified to:

choose(12, 0:12)
#  [1]   1  12  66 220 495 792 924 792 495 220  66  12   1


One of the important traits of Scala is that it very well supports the functional programming, and of the advantages of functional programming is that is makes the reasoning of the functions easier. This is easier if the functions do not have side effects and if the used data structures are immutable.

Immutable data structures and pure functions are not always the best option, but in a graph library I thing they might be suitable.

So in your code I'd change the Graph to be an immutable class (see sample below).

Another problem with your code you have notices yourself. It is the strange coupling of the graph and the node classes. A graph is a structure that contains the nodes and the edges. In your code however the graph does not have any information about the edges itself, the nodes are responsible for holding the information about the edges. This could become problematic if you add only the node a to the graph and the node a has edges to other nodes, not known to the graph. Also, with the current design, you cannot reuse the same node object in multiple graphs. Also in your code there is no guarantee that when the node a has an outgoing edge to b that there is a corresponding edge on b incoming from a.

Here a sample of immutable classes for the Node and the Graph. Note that this structure is not optimal at all, as the information about the incoming and outgoing edges is not stored efficiently, but this can be done easily with some maps, depending on the algorithms you want to develop. Another note: The graph in my sample stores the nodes and the edges a bit redundantly. It would be enough to store just the edges, if isolated nodes are not needed.

case class Node[+T](i: T)

class Graph[N <: Node[T], +T](someNodes:Set[N], val edges:Set[(N,N)]) {

  // someNodes does not necessarily contains all nodes from the edges, nodes does
  val nodes: Set[N] = someNodes ++ edges.map(_._1) ++ edges.map(_._2)

  def addVortex(node:N) = new Graph(nodes + node, edges)

  def addEdge(edge: (N, N)) = new Graph(nodes + edge._1 + edge._2, edges + edge)

  // Redundant, but more efficient data structure for the outgoing edges
  lazy val outgoingEdges: Map[N, Set[N]] = edges.groupBy(_._1).mapValues(s => s.map(_._2))

  // Here come your algorithms
}


Parse atom RSS feed with xml.dom.minidom

Nope, nope, nope.

feed = type('Feed', (object,), {})
feed.entries.append(type('Entry', (feed,), entry_dict))

The entire point of OOP is to have pre-defined classes, as contracts to follow. Since your classes are always the same, you should just define them with the class keyword.  I recommend attrs to make it look nicely.

In a good design, classes are never created on the fly, out of thin air.  They’re always defined in code, with a set of attributes that should also never change. (I’m not a fan of Python’s lenient style — Java, for example, makes it hard/impossible to create classes and new attributes at runtime.)

Or alternatively, you could make those regular lists of regular dicts.  Not everything needs to be a class.

More complaints:

Entry should not inherit from Feed. They’re two separate, unrelated things.

dom = minidom.parseString(res.read().decode('latin-1'))

99% of feeds in the wild are in UTF-8, and you should check the encoding in the <?xml ?> declaration.


As already stated you don't need to set it to true multiple times.
There is cleaner syntax for this.

static bool IsPalidrone(string word)
{
    for (int i = 0, j = word.Length - 1; i < j; i++, j--)
    {
        if (word[i] != word[j])
        {
            return false;
        }
    }
    return true;
}


There are a few small improvements I can see:

Your get_current_user() function is slightly misleading. Currently, you return None if there is no current user, or True if there is. This function isn't returning the current user, its returning if there is a current user.

If I assume the return value of get_secure_cookie() is a basic string, the method can actually be reduced to this single line:

# On a valid return, the return value will be a non-empty string == True.
# Otherwise, it will either be None or '', both of which == False.
return self.get_secure_cookie('trakr')

Since, this function is just passes through of the return value of another function, it really isn't necessary. Unless you wanted to keep it to describe what the return value of self.get_secure_cookie() could mean, I would suggest removing it entirely.
In your get() functions you have this if structure:

if get_current_user():
    self.redirect('/product')
    return

followed by:

if self.get_argument('code', None):
    ...
elif self.get_secure_cookie('trackr'):
    self.redirect('/products')
    return

First you check for a current user; if there is none you continue on. Then, if you get a False return value from self.get_argument('code', None), you immediately check again. From what I can tell from your snippet, unless you expect a user to get logged in during the time it takes to evaluate two if statements, the second user check is redundant.
I would change your naming conventions a bit. The G and F at the beginnings of your classes can be understood when looking at the source code. However, away from the source the F and G can be ambiguous.

I would recommend using the names: FacebookLoginHandler and GoogleLoginHandler.

Also, instead of trackr I would use the full work tracker. Saving 1 character isn't worth it.
I would use str.format() instead of string concatenation. You only do this once and with only two strings. However, string formatting is more Pythonic and it, personally, describes better what the final string will look like:

'Hello' + ' ' + 'world' + '!'   vs.   '{} {}!'.format('Hello', 'world')

My final point is more personal preference than a sure-fire Python convention. Whenever you have to split up a single statement onto multiple lines, you indent the next line 4 spaces in. As alluded to earlier, this fine:

yield self.authorize_redirect(
    redirect_uri=settings.fb_redirect_url,
    client_id=self.settings['facebook_api_key'],
    extra_params={'scope': 'email'}
)

  As a small aside, I don't like having the ending param on a line by itself; its too C/Java-like.

However, my personal preference is to indent them to line up (or as close to in line) with the section of the statement they are related to. Take this 'simple' example:

# Technically OK.
some_long_variable_name = [foobar for foobar in map(ord, 'Hello World!')
    if foobar < 97]

# My preference.
some_long_variable_name = [foobar for foobar in map(ord, 'Hello World!')
                           if foobar < 97]

In the first statement above, its slightly difficult to tell at a glance the if statement is in the list comprehension. Even though they are related syntactically, they currently aren't related spatially.

Now in the second statement, the if statement and the rest of the comprehension are related spatially which, personally, helps the code make more sense at a glance.

Here is how I would structure your code:

yield self.authorize_redirect(redirect_uri=settings.fb_redirect_url,
                              client_id=self.settings['facebook_api_key'],
                              extra_params={'scope': 'email'})

Otherwise, your code looks pretty good. I really wanted to look more into a single Login class because your two classes are so similar. However, a solution did not come quickly to mind and creating one would go past the bounds of typical code review.


The small improvement I would like to suggest you is related to performance in terms of speed.

In flatten_unicode_keys(dic), you use for key in dic.keys(): which is less fast than for key in dic:. This is mentioned in PEP 234:

  Dictionaries implement a tp_iter slot that returns an efficient
  iterator that iterates over the keys of the dictionary. [...] This
  means that we can write

for k in dict: ...

  which is equivalent to, but much faster than

for k in dict.keys(): ...

An other side note: it is a better coding habit not to encode the container type into the name. So instead, for example, of typing dic, use a name which actually better describes what that dictionary consists of.


Better way to code this code snippet (copy paste)

Let me give you the following answer/advise:
As far as your first piece of code is concerned, this can be re-written as:
Sheets("DataExport").Range("A1").CurrentRegion.ClearContents

As far as the second piece of code is concerned (about the copying), I'd like to refer you to this very complete answer. (I might give you the answer but I believe you'd better try to create it, based on this URL. If you have any issues, please edit your question, we'll have another look).


The only thing that I see is that the files are not closed with fclose after the data is read/written.  Especially using the same variable (fileID) as the file handle for the second while the first is still open (effectively losing the first one). Although you can still always do fclose('all') and I think it would close even those files still open that don't have a handle still assigned.


Compress numbers in a string array (#3, #1, #2, #4) to range (#1:#4)

In addition to my answer on your other (related) post which is still valid here (with the proper modifications obviously) I'd suggest to use brackets even in case of if blocks with only 1 instructions. For example:

if (IDs.Contains(less1) && hasPlus1) continue;

should become something like:

if (IDs.Contains(less1) && hasPlus1)
{
    continue;
}

Also, var id = item; can be safely removed and you can work directly on item.

By applying all changes the method becomes something like the following:

static string Compress(IEnumerable<string> IDs)
{
    var result = new List<string>();
    var ignore = new HashSet<string>();
    foreach (var item in IDs)
    {
        if (ignore.Contains(item))
        {
            continue;
        }
        if (!item.StartsWith("#"))
        {
            result.Add(item);
            continue;
        }
        int fromInt;
        if (!(int.TryParse(item.Substring(1), out fromInt)))
        {
            result.Add(item);
            continue;
        }
        var less1 = $"#{fromInt - 1}";
        var plus1 = $"#{fromInt + 1}";
        var hasPlus1 = IDs.Contains(plus1);
        if (IDs.Contains(less1) && hasPlus1)
        {
            continue;
        }
        var plus2 = $"#{fromInt + 2}";
        if (!(hasPlus1 && IDs.Contains(plus2)))
        {
            result.Add(item);
            continue;
        }
        ignore.Add(plus1);
        ignore.Add(plus2);
        var toInt = fromInt + 2;
        while (IDs.Contains($"#{toInt + 1}"))
        {
            toInt += 1;
            ignore.Add($"#{toInt}");
        }
        result.Add($"#{fromInt}:#{toInt}");
    }
    return string.Join(",", result);
}


probably a bit too late to help OP, but in case anyone else stumbles upon this question, here's a nice succinct way of doing it, using the frequencies function:

(doseq [[word freq] (frequencies
                      (map
                        #(re-find #"^[^\t]+" %) ;; just get the first non-tab characters
                        (line-seq (java.io.BufferedReader. *in*))))]
  (println (str word "\t" freq)))


Parse a CSV file and return an object or array

Concept

CSV is a somewhat loosely defined concept. The closest thing there is to a specification is RFC 4180.

This parser is rather naïve: it doesn't handle quoting, and as a result it cannot return data that contain strings with literal commas. Maybe that is good enough for your own use, but in my opinion it's not good enough for a publicly published library, at least not without a giant disclaimer.

For an example of a good CSV library that handles multiple dialects, take a look at Python's built-in library.

Implementation

The purpose of the option is not self-evident. A better name might be headerRow.  Also, the way you have nearly duplicated the two cases, you might as well write it as two functions instead.

The code crashes on empty input, instead of returning an empty data structure as I would expect.

Why do you catch an exception and convert it to a return value? That defeats the purpose of the exception mechanism, and forces the caller to handle the possibility of very weird "data" resulting from the parsing.

I'm not a fan of var declarations that span multiple lines, since it is easy to unintentionally write something different if you screw up the punctuation. ("use strict" would be a good practice for helping to detect such errors.) Your use is particularly bad because the intervening comment lines and the lack of additional indentation on the subsequent lines make it hard to read correctly.

l <= lines.length - 1 is not idiomatic: the standard way to write a counting for-loop in JavaScript is:

for (var i = 0; i < array.length; i++)

Why do you "reset" your temporary variables at the end of the loop for reuse in the next iteration? Why not just create it at the top of the loop?

Commenting every line of code is annoying. Most of your comments can just be eliminated.

Use semicolons consistently. You missed one at return records.


My first question is what does your graph represent? It sounds like you want it to represent the things which need to be rendered. Does the graph encode the order of the rendering? Or is it just a container that holds all the objects to be rendered? It's not clear from what you've shown here why you want things in a graph. (Which isn't to say it's a bad idea. It's just not clear what your reasons are.)

The code you've written is pretty straightforward and seems mostly reasonable. It's very readable, which is great.

What is the purpose of a Node? You've made it an Entity, but it's not clear to me that it should be. It looks like you want it to be an entity so that it can have a Render() method which calls its components Render() methods. From what you're showing here, it's not clear that a Node ever uses its transforms. (If I'm misunderstanding, then let me know.) If that's the case, I'd make the Node class be a free class that doesn't inherit from Entity. It would look mostly the same. To render, you would either give it a Render() method like it has now, or create a class or function that walks the graph, gets each node's list of children and calls each child's Render() method. I'd probably also rename it to EntityNode since it only ever keeps track of entities.

I notice that Entity's Render() method is pure virtual, but its Update() method is not, though it's empty. Will most subclasses of Entity implement it? If not, then it's fine to leave it as an empty method. But if each subclass will implement its own version, then it should probably also be pure virtual, too.

Speaking of virtual methods, if you're using C++11, you should add override after each virtual method in the subclasses, like this:

class Spatial : public Entity {
    //... constructors, etc.
    virtual void Render (const Shader& shader) override {
        //... method contents ...
    }
    // ... other methods...
};

This can help you catch some hard-to-understand errors as your class hierarchy grows. For example, if you ever change the function prototype for the Render() method, it will remind you to fix each subclass's implementation.

In your main method, you're reading the same object twice, though it's not clear why. That might make sense if you're creating 2 instances of the same object. But if not, it probably indicates a need to structure things differently. Since I don't know your goals there, I can't say which it is.


Created a serializable object that can handle primitive types

Don’t say using namespace std;.

And you were qualifying names in std anyway!  What gives?

std::function<void(std::string, std::string)> &changed)

Why must you pass the strings by value to the callback?

You have this std::function signature spelled out twice — you might want to make an alias for that.

using cbfunc = std::function<void(std::string, std::string)> changed)
  ⋮
⋯Prop(const std::string& key, const cbfunc& changed)

In C++, the style is to put the & modifier with the type, not the declared name.  This has been true since Bjarne’s original book, where it is pointed out explicitly.

this->Key = key;
   this->changed = changed;

No…
Write your constructor initializers, not assignment on top of default-initialized members.  Learn the difference between initialization and assignment.

: Key{key}, changed{changed}

and note that having the parameter with the same name as the data member is not a problem and is in fact idiomatic.  If you ever need to write this-> a lot, you are doing something wrong.

In this particular case, take advantage of move semantics.  In full:

Prop<T>::Prop(std::string key, cbfunc changed)
: Key{std::move(key)}, changed{std::move(changed)}
{ }

static_cast<ostringstream&>(ostringstream() << value).str()

What’s with the static_cast?  Don’t do that.

Since you are modelling a value that does things behind the scenes when written to, using operator= for the name is probably OK in this case.  But it should not be void.  Either return *this or value.

Again, don’t use this-> to access members.

Prop& operator= (const T& value)
    {
    set(value);
    return *this;
    }

void set (const T& value)
    {
    Value= value;
    std::ostringstream& fmt;
    fmt << value;
    changed (Key, fmt.str());
    }

But, there is a handy package that already does the formatting exactly like this.  So just write

    Value= value;
    changed (Key, boost::lexical_cast<std::string>(value));

Note that I made a normal-named function set and then defined operator= to call it.  That will be much better for people who read your code later, since operator= carries certain expectations.

You did not provide the matching get function!  Your values are write-only!

In keeping the callbacks as invisible as possible, the design would be to have a function get and then an operator T that calls it.

 int x;
 Prop<int> px {"px", some_function};
 px = 5;  // invisible in
 x = px;  // and invisible out.

std::bind is deprecated since C++11.  Sometimes it is indeed the simplest way to write something.  But with placeholders, not so much.

Since all your props use the exact same function, you should just set it up once.

// member of class User
const auto binderCB = [this](std::string left, std::string right)
    { Update (left, right); }

Then the constructor※ is

User::User()
: Name{cb("Name")}, Age{cb("Age")}, ⋯

(just be sure cb is the first member so it is initialized first!)

(almost always) use auto.
You don’t have any uses at all, so I’ll point it out with an example in your main:

auto json = user.ToJson();

The main might be a throw-away example, but I’d rather initialize the user object in one go, not assign to each prop individually.

User user { "Pamela", 25, "USA" };

for a plain struct, this would just work as an aggregate initializer.  You need a constructor that takes three arguments (with defaults) to provide the same functionality.

Notes

※ I’d normally write all those as default initializers in-line and not give a constructor at all.  But for this specific example, I worry about referring to other members.  I’d need to look that up to see if it’s OK.

Rather than updating a map every time you change a value, you can use the same effort of setting up each member with an identifier and callback but provide a general way to explore the current values and types when you are ready to do something with them.  That is, pull rather than push.

There are libraries that do this, most notably Boost::Fusion.  But there are more modern ways — I think Boost::Hanna has that, but I don’t recall for sure.  An any case, the thing to do is to supply a non-member get template so the object can be used just like a tuple`.  This will let it interoperate with other code that follows this approach, including the built-in structured bindings.

Since your callback turns everything into a string representation, your Json rendering will not know what things were originally strings and what are numbers.  Your code will output

"Age" : "25"

instead of

"Age" : 25

In your design (where everything is handed over as a string) you can include the quoting as part of the update callback.  Use one for strings that add quotes and also escapes out needed characters, and another for numbers that does not.  Then, don’t add the quotes in your outputing function.


I think you are overthinking it.  Or maybe this is just not the right problem for TDD? This seems to be a typical interview / Project Euler (kind of) question. Something that doesn't require more testing than running it several times in console.

Why do I say this?--Tests are typically needed when the borderline conditions for the input are not trivially found and the algorithm being tested may take on multiple ways, not easily seen from its definition.  In this case we have a very clearly stated borderline condition for inputs (our inputs are characters A-Z) and the algorithm may only take on two paths (a base-case, when 'A' is given) and the "other" cases (given any letter other than 'A'). The algorithm isn't defined for any other input.

Below is a simple version of this problem:

function diamondKata(character) {
    var a = 'A'.charCodeAt(), frame, line,
        width = character.toUpperCase().charCodeAt() - a,
        i = 0, lines = [];

    function blanks(n) { return new Array(n).join(' '); }

    function makeLine() {
        return blanks(width + 1 - i) + frame +
            blanks(i * 2) + frame;
    }
    while (i++ < width) {
        frame = String.fromCharCode(a + i);
        lines.push(makeLine());
    }
    if (lines.length) {
        lines.unshift(blanks(width + 1) + 'A');
        lines = lines.concat(
            lines.slice(0, lines.length - 1).reverse());
    } else {
        lines.push('A');
    }
    return lines.join('\n');
}

Some points about your code: while operations with strings are generally very fast in JavaScript (highly optimized C code), it is still better to create long strings by first pushing all chunks into an array, and then to join it all in one go.  The reason for this is that creating strings by consequent concatenation will make a lot of intermediate memory allocations, which would be avoided if the string is created at once.

More on your code: type checking is frowned upon, unless its some library code, where the algorithm may take on multiple paths depending on what kind of object it works with.  When you check types only to throw a type error, you are working against the benefits you get from dynamically type-checked language: with dynamically type-checked languages you get these type errors for free (the environment already does that for you).

Compare this to C-like languages, where after dereferncing a pointer and making wrong assumption about the type of the object pointed by the derefernced pointer will spell disaster for the program, unless checked.

Hardcoding the codepoint assigned to characters would be a bad thing to do in any language, unless it's an optimization justified by a profiling session.


Can you replace a REPLACE statement, or 9?

This strikes me as best being done by using a temp table, or with clause.

Also, the DISTINCT part worries me.... why is that needed? Are your joins not good?

Still, I think you may have some more contentment with:

SELECT DISTINCT
    CAH.CaseNbr,
    (SELECT CASE WHEN Degree in ('F1','F2','F3','F4','F5','F6','FA','FB','FC')
                 THEN 'F'
                 ELSE Degree
            END
     FROM SDUJS.dbo.fnWorstDegreeCharge(CB.CaseID)) AS Degree,
  dbo.fncasecounty(CB.CaseID) AS County
....

You should still consider a better mechanism for joining. The procedure route seems 'klunky'


Optimizing code: longest repeating substring

You may want to roll your own comparison method to compare two pieces of a string in-place, instead of using substring(), which as I understand creates an unnecessary temporary copy. It may look like this:

bool CompareInPlace(String text, int from1, int len1, int from2, int len2)
{
    // ...
}

Furthermore, instead of keeping a copy of the longest duplicat found, keep its indices from and len.
Then in the end you can return text.substring(longestFrom, longestLen);.


Python's built-in pow already has this functionality.

class Solution(object):
    def superPow(self, a, b):
        """
        :type a: int
        :type b: List[int]
        :rtype: int
        """
        digit = int("".join(map(str, b)))
        return pow(a, digit, 1337)

The bottleneck then might be parsing the list of digits into the integer. To remedy that, you can try to use the fact that

\$
\begin{align}
a^{123} ~\text{mod}~ 1337 &= (a^{100}\cdot a^{20}\cdot a^3)~\text{mod}~ 1337& \\
              &= (a^{100}~\text{mod}~ 1337 \cdot a^{20}~\text{mod}~ 1337\cdot a^3  ~\text{mod}~ 1337) ~\text{mod}~ 1337& \\
\end{align}\$

class Solution(object):
    def superPow(self, a, b):
        """
        :type a: int
        :type b: List[int]
        :rtype: int
        """
        ret = 1
        for i, digit in enumerate(reversed(b)):
            ret = (ret * pow(a, digit * 10**i, 1337)) % 1337
        return ret

And here are some timings for inputs of increasing length (from 1 to 10 digits, because your function would take way too long otherwise), with a = 2 and where super_pow_op is your function, super_pow_built_in is my first function and super_pow_expanded is my second function.

And here for 3 to 1000 digits, but only with my two functions:


Probabilistic Matrix Inspection - Suggested by a paper, implemented by me

In F# we prefer match over if:

if thereIsZeroProbabilityIn column then false
else
    column
    |> List.filter (fun (_, q) -> q < 100)
    |> thereIsZeroIn
    |> not

Should, instead, be:

match thereIsZeroProbabilityIn column with
| true -> false
| false ->
    column
    |> List.filter (fun (_, q) -> q < 100)
    |> thereIsZeroIn
    |> not

In the case where you're only piping to one other function, you should stay consistent with whether you inline the pipe or not:

 System.Console.ReadKey() |> ignore

and

column
|> List.sortWith comparator

Are inconsistent, generally speaking I prefer to inline the pipe when there is only one pipe happening. (It seems to make be more readable that way.)

column |> List.sortWith comparator

You reduce LoC and make use of the horizontal spacing.

let comparator column1 column2 =
    let sum1 = sumProbabilities column1
    let sum2 = sumProbabilities column2

    sum2 - sum1

In that case you should not be using the local constants, they only create overhead on the stack, they're not used more than once, and they're not cached, so in reality they provide no additional value.

let comparator column1 column2 =
    (sumProbabilities column2) - (sumProbabilities column1)

Again, reduce vertical space a little.

Other than this, I have nothing else to say about the code except maybe to shorten some of the names. It's awful verbose. It's good F# already, my modifications are merely recommendations. Good work. :)


Your code looks generally good (with two real problems explained below).

Instead of an empty default array

private var tuples: [KeyValueType] = []

which is overwritten in two of the three init methods, I would initialize the
array explicitly:

private var tuples: [KeyValueType]

init() {
    tuples = []
}

but that is perhaps a matter of taste.

The problem with the other two init methods is that they don't check
for duplicate keys, e.g. if you call

let od = OrderedDictionary(("foo", 1), ("bar", 2), ("foo", 3))

You either have to add an explicit check (and throw an error or fail
if duplicate keys are given), or add the key/value pairs sequentially
so that duplicate keys are silently overwritten with the new value:

init(_ tupleArgs: [KeyValueType]) {
    self.init()
    for (key, value) in tupleArgs {
        self[key] = value
    }
}

There is a problem in your subscript getter method:

return tuples.filter({ $0.key == key })[0].value

will crash if there is no key/value tuple for the given key, instead of returning
nil, because accessing [0] requires that there is at least one element.
This can be fixed as

return tuples.filter({ $0.key == key }).first?.value

because first returns an optional. But this is not very effective. The
filter method always traverses the entire array, but only the first (and only)
matching tuple is needed. This can be achieved with indexOf:

get {
    if let index = tuples.indexOf({ $0.key == key }) {
        return tuples[index].value
    } else {
        return nil
    }
}

Similarly, in the subscript setter method, it is not effective to call
contains first and then map the entire tuple array to a new array.
I would implement that as

set(newValue) {
    if let index = tuples.indexOf({ $0.key == key }) {
        if let val = newValue {
            // Update existing value
            tuples[index].value = val
        } else {
            // Remove existing value
            tuples.removeAtIndex(index)
        }
    } else if let val = newValue {
        // Add new value
        tuples.append((key, val))
    }
}

The generate method can be simplified considerably, since you are only
forwarding the enumeration to the tuples array:

func generate() -> AnyGenerator<KeyValueType> {
    return anyGenerator(tuples.generate())
}

The

func toArray<T>(transform:((KeyValueType) -> T)) -> [T]

is not really needed, you'll get the same result already with your
map() and toArray() method.

You should make your type conform to CustomStringConvertible so that
printing an ordered dictionary gives a nice looking result. A simple
implementation would be

extension OrderedDictionary : CustomStringConvertible {
    var description: String {
        return "[ "
            + tuples.map({ (key, value) in "\(key) : \(value)"}).joinWithSeparator(", ")
            + "]"
    }
}

Creating a new (ordered) dictionary by mapping both keys and values
can lead to unexpected results if two keys are mapped to the same
new key. E.g. (with the fixed init method from above):

let od1 = OrderedDictionary(("foo", 1), ("bar", 2), ("FOO", 3))
print(od1) // [ foo : 1, bar : 2, FOO : 3]

let od2 = od1.map { (key, value) in (key.uppercaseString, value) }
print(od2) // [ FOO : 3, BAR : 2]

You have to decide if that is wanted. Alternatively, define a
map() method which only maps the values but preserves the keys.

If you plan to add the code to a framework then you have to annotate the struct
itself and all its public methods as public.

Some remarks concerning the performance: Each key lookup is done by a linear
traversal of the tuples array. For large dictionaries this can become a problem.

The project from the referenced blog post solves this by maintaining both
a dictionary (for the key/value pairs) and an array (for the ordered keys).
 The advantage is that a
lookup is fast. But adding a new key or removing a key still requires a
linear traversal of the keys array. Whether this is acceptable or not
depends on how often keys are added/removed compared to key/value lookups.

One could store the key/value pairs in a tree structure to improve the
performance (e.g. AVL tree or a red-black tree). Of course that increases
the complexity of the code.


Two things:

Always specify the visibility. Don't use the old var keyword, use public, private or protected. Since the class-member $url contains internal information, you'll probably want to hide that information (you've already implemented a getter method so that the information can be read but not written):

class website {
    protected $url;

    public function __construct($url) {
        $this->url = $url;
    }

    // ...

Every instance of this class seems to be meant to represent one website. But your method "get_httpcode()" works for every website (it expects a URL; this is exactly what you've already noticed by yourself). It would be more natural to skip the parameter and return the HTTP-Statuscode for the url that is bound to the instance (you've "remembered" the URL in the class-member variable $this->url):

class website {
    // ...

    public function get_httpcode() {
        // ...
        curl_setopt($ch, CURLOPT_URL, $this->url);
        // ...
    }
}

// Usage:
$google = new website("http://www.google.com");
$statusCode = $google->get_httpcode();


Shopping cart logic for authenticated users and guests

There are two primary concerns I have.

First, rather than accessing [[UserManager sharedManager]isUserLoggedIn] twice, let's access it just once:

BOOL loggedIn = [[UserManager sharedManager] isUserLoggedIn];

Now just use the loggedIn variable in place of calling these methods.

In this case, it's not going to make a huge difference, but it's a good habit to get into.  If you're going to use the result of a method call multiple times within a method, call the original method once and save the result to a local variable.  This can make the code more clear (with appropriately named local variables) and will be more efficient with more complex code.

Second, APPDELEGATE, I'm going to assume, is a #define-ed macro.  You should eliminate uses of this macro.  For one, you should just generally be avoiding #define-ing stuff, and for two, this runs into the same trouble as calling that isUserLoggedIn method repeatedly, but masks it behind APPDELEGATE.

Given that you're cleaning up someone else's code, I recommend just eliminating uses of the macro, but don't actually delete the macro definition until you've cleaned them all up eventually.

Instead, use a function that returns the app delegate reference.  Call that function once and store a local reference to it for future uses within the method.

A function would look very similar to the macro:

MyAppDelegate * MyAppDelegate() {
    return [[UIApplication sharedApplication] delegate];
}

Now...

MyAppDelegate *appDelegate = MyAppDelegate();

And use appDelegate as your reference to app delegate.


Using a Subject to trigger a music list to reload

There is no rule like

  one should generally avoid Subjects

You should just be careful around them, like you have. Just as a rule of thumb, keep Subjects private (inside your class), and if you want the outside world to be able to subscribe to it, just call subject.asObservable.

As for your code, you can perform the setupObservable() inside an obervable.onSubscribe() so that you have a cold observable and not a hot one.

If you really have to remove the Subject, you can do a Observable.create(subsriber -> ...). This subscriber variable would be the replacement for your mManualRefreshesSubject.

Frankly, there is not much difference between either pattern. The only thing you should do is check whether the subject.isUnsubscribed() / subscriber.isSubscribed. One benifit of using a subscriber is that you make sure to do stuff onSubscribe and not before.


I do not have an Azure account, so I cannot test this, but does this work in general? The linked question uses a MemoryCache which is in-memory and probably references the added objects directly. As Azure cache is a distributed system, that will probably not work and the values need to be serialized in some way to be passed to the remote cache.

As I was writing this, I stumbled upon this SO question which clearly shows that Lazy<T> is serializable and thus can probably be stored in Azure, but the value will be evaluated on serialization. Basically, using Lazy<T> does not give you any advantage here, because of the aforementioned side-effect of putting it in the cache.

            // We know for sure that the key exists at this point:
            // Two concurrent callers tried to get it, and this thread
            // happened to be deadlock victim.

That state is probably not true, because the cache item with that key might have expired between the exception and the call to Get. Granted, that this is relatively unlikely and heavily depends on how small the cache timeout is, but it could happen. This scenario could be avoided if you retried the whole method upon receiving a DataCacheErrorCode.KeyAlreadyExists error code.


def spacing(a, b):
    score = 0
    for x, y in zip(a, b):
        if x != y:
            score += 1
    return score - 1

can be simplified to:

def spacing(a, b):
    """Returns numerical spacing between a and b on the word ladder"""
    return sum(1 for x, y in zip(a, b) if x != y) - 1

I suggest writing the comments as a string inside the function (a docstring) like above, then you can later write

help(spacing)

and get the documentation.  It also helps your IDE to find the documentation.


Help optimizing this query with multiple where exists

SQL allows you to join the same table multiple times, so what you need here is:

SELECT p.id FROM products AS p
JOIN attributes_products AS ap1
  ON ap1.product_id = p.id AND ap1.attribute_id = 27 AND ap1.value = '...'
JOIN attributes_products AS ap2
  ON ap2.product_id = p.id AND ap2.attribute_id = 28 AND ap2.value = '...'
JOIN attributes_products AS ap3
  ON ap3.product_id = p.id AND ap3.attribute_id = 29 AND ap3.value = '...'

Here's the toy MySQL database that I'm using to answer this question:

CREATE TABLE products (
    id INTEGER PRIMARY KEY AUTO_INCREMENT
);
CREATE TABLE attributes_products (
    product_id INTEGER NOT NULL,
    attribute_id INTEGER NOT NULL,
    value CHAR(40)
);
CREATE INDEX ap_product ON attributes_products (product_id);
CREATE INDEX ap_attribute ON attributes_products (attribute_id);

INSERT INTO products VALUES (1);
INSERT INTO products VALUES (2);
INSERT INTO attributes_products VALUES (1, 27, 'a');
INSERT INTO attributes_products VALUES (1, 28, 'b');
INSERT INTO attributes_products VALUES (1, 29, 'c');

With my query above, MySQL reports the following query plan:

+----+-------------+-------+--------+-------------------------+--------------+---------+---------------------+------+--------------------------+
| id | select_type | table | type   | possible_keys           | key          | key_len | ref                 | rows | Extra                    |
+----+-------------+-------+--------+-------------------------+--------------+---------+---------------------+------+--------------------------+
|  1 | SIMPLE      | ap1   | ref    | ap_product,ap_attribute | ap_attribute | 4       | const               |    1 | Using where              |
|  1 | SIMPLE      | ap2   | ref    | ap_product,ap_attribute | ap_attribute | 4       | const               |    1 | Using where              |
|  1 | SIMPLE      | ap3   | ref    | ap_product,ap_attribute | ap_attribute | 4       | const               |    1 | Using where              |
|  1 | SIMPLE      | p     | eq_ref | PRIMARY                 | PRIMARY      | 4       | temp.ap3.product_id |    1 | Using where; Using index |
+----+-------------+-------+--------+-------------------------+--------------+---------+---------------------+------+--------------------------+

See the MySQL documentation for an explanation of the EXPLAIN output.

This looks better than the plan for the OP's query:

+----+--------------------+---------------------+-------+-------------------------+--------------+---------+-------+------+--------------------------+
| id | select_type        | table               | type  | possible_keys           | key          | key_len | ref   | rows | Extra                    |
+----+--------------------+---------------------+-------+-------------------------+--------------+---------+-------+------+--------------------------+
|  1 | PRIMARY            | p                   | index | NULL                    | PRIMARY      | 4       | NULL  |    2 | Using where; Using index |
|  4 | DEPENDENT SUBQUERY | attributes_products | ref   | ap_product,ap_attribute | ap_attribute | 4       | const |    1 | Using where              |
|  3 | DEPENDENT SUBQUERY | attributes_products | ref   | ap_product,ap_attribute | ap_attribute | 4       | const |    1 | Using where              |
|  2 | DEPENDENT SUBQUERY | attributes_products | ref   | ap_product,ap_attribute | ap_attribute | 4       | const |    1 | Using where              |
+----+--------------------+---------------------+-------+-------------------------+--------------+---------+-------+------+--------------------------+

But results will vary from one database to another: a good query planner might be able to make something efficient out of the OP's query.


Your code looks good and is well documented.

A few details :

instead of having is_composite function, I'd rather have a is_prime function : it is more common and it reuses the terms used to describe the problem. (You just have to swap True/False).
I wouldn't define a is_divisable function but it may be a good idea if you think it is necessary. However, the proper English word is "divisible" and not "divisable". Also, I wouldn't call the second argument "modulo" to avoid mixing argument name and operator name. Wikipedia suggests "modulus". Finally, the docstring should tell more about what is intended and less about how it is implemented. """Returns whether number is divisable by modulus""".
Because you know an upper bound for the number you'll consider during primality check, it may be a good idea to use the Sieve of Eratosthenes.


Is there a better/cleaner way to assign values based on the result of radio buttons?

Just a side note for you.  It would probably be more efficient to use the Checked_Change handler.  The main caveat is that both the checked and the unchecked will trigger this event.  but since the unchecked is handled first, it's a simple matter of returning if the sender isn't checked.

Since the radiobuttons are actually numbered you don't need the switch block once you have the checked radiobutton identified.

Something like this would work:

private void radioButton_CheckedChanged(object sender, EventArgs e)
{
    RadioButton rb = (RadioButton)sender;
    if(!rb.Checked)
    {
        return;
    }
    selectedButton = rb.Name.Last() - '0';
    selectedButtonText = rb.Text;
}


De-bouncing a (Gray) Rotary Encoder Switch in C

I don't see why bother with clipping averages. If something was less than ROTSW_MIN, it is also less than ROTSW_LOWER.
count is useless, you may operate directly on rotsw_count.

The moving averages could be computed a bit faster in assembly. The rotsw_samples array effectively contains 8 2-bit entries, and hence could be stored in a 16-bit register, say R. Now consider something along the lines of

            rlc R             ; Shift the ra2 of the "old sample" into carry
            sbc avg_ra2, $0   ; Subtract carry
            rlc R             ; Same for ra3
            sbc avg_ra3, $0

            or R, sample      ; New sample is added to the "array"

            rrc sample        ; Shift ra3 of the "new sample" into carry
            adc avg_ra3, $0   ; Add carry
            rrc sample        ; Same for ra2
            adc avg_ra2, $0

The exact syntax depends on the architecture and an assembler; it also assumes that the sample is already shifted right by 2.

Your target architecture may allow even faster variants.

Note that the similar tricks could be used to speed up the final rotsw_count computation.

There is not enough context to address your worries about the switch being too speedy. In any case, the code is already quite fast. If doesn't keep up with the peripheral, the best answer is to upgrade the CPU.


Condition bug Your current implementation has a bug. Your lines like these have the problem:
(secondaryStatus === 'payment1' || 'payment2' || 'payment3')

JavaScript only has unary operators, binary operators, and a single ternary operator. All the operators used above are binary operators; two expressions will be evaluated into a single expression until a single one is left. Since === has higher operator precedence than ||, your code is equivalent to:
(secondaryStatus === 'payment1' || 'payment2' || 'payment3')

(trueOrFalse || 'payment2' || 'payment3')

The payment2 and payment3 are not compared against secondaryStatus, and if secondaryStatus is not payment1, the whole expression will evaluate to 'payment2', a truthy value (because || will evaluate to the second value if the first is falsey):
// || operates left-to-right:

(trueOrFalse || 'payment2' || 'payment3')

((false || 'payment2') || 'payment3')

(('payment2') || 'payment3')

// payment2 is truthy, so the `||` evaluates to it:
('payment2')

To fix the logic, use an array instead, and check the status against each element in the array.
Typo? You have (secondaryStatus === 'payment2' || 'payment5' || 'payment5'), with payment5 repeated twice. Did you mean something else, like payment9?
Names carStatus does not contain the car's status, as it would sound to contain; it's a function that, when called, returns the car's status. It also takes a confusingly-similarly named status parameter. Call them something else, if at all possible: perhaps getCarStatus and whatever more specific thing the status parameter represents. Maybe call secondaryStatus: paymentType.
DRY You also want to make the code more DRY, which fits in well with the idea of using arrays instead. You might use an object, whose keys are the status that's required, and whose values are the possible secondaryStatus values as well as the return value if the secondaryStatus is found.
Also, since all the payment strings end in a number, and that number is the only thing that changes, use that number to set up the config object instead of the full payment strings:
const statusOptions = {
    OPEN:      { paymentNumbers: [1, 2, 3], status: 'CONFIRMED' },
    CANCELLED: { paymentNumbers: [4, 5, 6], status: 'REMOVED' },
    REVIEW:    { paymentNumbers: [2, 5], status: 'CHECKED' },
};
// ...
getCarStatus = (status, paymentType) => {
    const possibleOption = statusOptions[status];
    if (possibleOption) {
        const paymentNumber = Number(paymentType.match(/\d+$/)[0]);
        if (possibleOption.paymentNumbers.includes(paymentNumber)) {
            return possibleOption.status;
        }
    }
}

Live snippet:

const statusOptions = {
    OPEN:      { paymentNumbers: [1, 2, 3], status: 'CONFIRMED' },
    CANCELLED: { paymentNumbers: [4, 5, 6], status: 'REMOVED' },
    REVIEW:    { paymentNumbers: [2, 5], status: 'CHECKED' },
};
// ...
getCarStatus = (status, paymentType) => {
    const possibleOption = statusOptions[status];
    if (possibleOption) {
        const paymentNumber = Number(paymentType.match(/\d+$/)[0]);
        if (possibleOption.paymentNumbers.includes(paymentNumber)) {
            return possibleOption.status;
        }
    }
};

console.log(
  getCarStatus('OPEN', 'payment3'),
  getCarStatus('REVIEW', 'payment2')
);

Component type It looks like you're using a class component. React recommends that you try out using functional components in new code; they say they're a bit easier to work with and understand than class-based components in most circumstances, and I agree. Try using functional components instead, if you haven't already, you might like them.


WebAPI Handling fire and forget exceptions

ContinueWith() using TaskContinuationOptions.OnlyOnFaulted is right way to do it, especially when you do not have control over first task code (for example because it comes from an unknown method as Task).

However in your example you do not need a continuation task to handle errors, you may (and should) enclose your code inside a try/catch block and handle exceptions in-place:

Task.Factory.StartNew(() =>
{
    try
    {
        var client = (HttpWebRequest)WebRequest.Create(_url);

        // ...
    }
    catch (Exception e)
    {
         _logger.LogException(LogLevel.Error, e.Message, e);
    }
});

Note that this is just first step. First of all you're catching a generic Exception, if it's just for logging purposes it may be fine but you should consider if you have better options (error recovering? retrying?) handling more specific exceptions. For example you  may want to handle WebException and check if it's a timeout, there may be a network connection issue and you may want to wait and retry like in System.Net.WebException: The remote name could not be resolved.

Now you should correctly dispose resources you're using. If an error occurs, for example, after you acquired request stream but before you close it then connection will be immediately released. To simplify this you have a nice C# statement, using:

using (var requestStream = client.GetRequestStream())
{
    requestStream.Write(bytes, 0, bytes.Length);
    // ...
}

Also note that you should close request stream before asking for response stream, even if it's not an issue it's easier to do it right if you have using in-place.


Several things we can improve:

don't repeat the multi words and the same multi words with tags - just have a list of multi-words and, if it is in the corp, replace each word with a tagged word
use with context manager to read the file contents
use a list comprehension for tagging all other words
generalize the function a bit and provide it with data as an argument

Improved version:

MWE = ['prime minister', 'new delhi', 'reserve bank']

def tag_words(text):
    # tag all MWEs
    for multi_word in MWE:
        if multi_word in text:
            for word in multi_word.split():
                text = text.replace(word, word + "/MWE")

    # tag all NMWEs and return
    return " ".join([word if "/" in word else word + "[NMWE]"
                     for word in text.split()])

with open("/python27/MWETagtext1.txt") as f:
    corp = f.read().lower()

print(tag_words(corp))

Since you are doing natural language processing, I think you should also be using word_tokenize() instead of the naive str.split().


The function is very long.

Functions should do one thing, do it completely, and do it well. Long functions are difficult to reason about.

Move the subject and pupil data entry into separate functions to isolate the functionality, and streamline the mainline code.

I'm assuming the presence of String getSubjectCode() and Pupil getPupilInformation() methods, each containing the code currently in the mega-function.

Naming conventions

Minor, but you call an instance of the Pupil class Pupil, which is confusing at best, misleading at worst. Non-static-final variables should always begin with lowercase letters.

Use typed collections

School.getSubjects() should return a List<Subject> (or Collection). (Or at least a properly-typed array; nothing with Objects.)

Reduce the amount of code in conditionals

There's a lot of code in block that loops over the school's subjects, most of which is the block for when the subject is found.

In order to understand what happens if the subject isn't found, I have to scroll down to the end of that block, make sure I'm reading the indentation correctly (assuming the indentation is correct), only to find there's nothing there.

In cases like that, might as well do a negative test and continue, slightly "flattening" the loop's guts. (Code turned on its side is not a graph of how awesome it is ;)

That said, this functionality is available in a contains method--looping in the mainline code isn't necessary. See next.

Localize functionality

Deciding if a school has a given subject should be moved into the School class, for example:

public class School {
    // ... existing functionality elided ...

    /** Returns subject with given code, or null if not found. */
    public Subject findSubjectByCode(String code) {
        for (Subject s : subjects) {
            if (s.getCode().equals(code)) {
                return s;
            }
        }
        return null;
    }
}

The refactored mainline code

All the above turns the mainline code into the following:

System.out.println("Adding a Pupil...");

String subjectCode = getSubjectCode();
Subject schoolSubject = school.findSubjectByCode(subjectCode);
if (schoolSubject == null) {
    System.out.printf("A Subject with code, %s, doesn't exist!%n", subjectCode);
    return;
}

Pupil pupil = getPupilInformation();
if (schoolSubject.addPupil(pupil)) {
    System.out.printf("%nPupil, %s, has been added to the %s Subject.%n", pupil.getName(), subj.getName());
} else {
    System.out.printf("Pupil #%s is already in the Subject!%n", pupil.getID());
}

IMO this is more communicative, cleaner, and easier to understand.


Your code is pretty neat, and well structured. I like the variable names. There are a number of problems to point out, though...

splitting the string is a huge "cheat".... if you're going to use regex, you may as well just match for the substring.
splitting the strings is also slow. Just use an index in to the string and the string[indx] operator. You can remove the h and n and h_len and n_len variables as well then.
your outer loop can terminate at h_len - n_len - there's no need to loop to the end when a match would be impossible

You have some bugs.... like:

your needle cannot contain the same letter as the start letter in a another place... For example, your code will fail to find cec. This is because you do weird logic here: if (h[i] == n[0]) ... I am not sure why that's needed.... other than to reset the search?
related to the above, you cannot backtrack and reset a search in the event that the search pattern starts part way through a partial match....

Even when you resolve the above issues, your algorithm is considered 'naive'. There are two commonly used algorithms that are considered better. The Rabin–Karp algorithm is simpler to implement because you just create a hash of the needle, and then a matching rolling-hash in the haystack. If you find the hash in the haystack you can confirm a match, and return, in what is effectively \$O(n+m )\$. A more complicated, but also more efficient algorithm is the Boyer-Moore algorithm which creates a "jump table" from the needle, which allows you to 'skip' values in the haystack based on the details in the needle. It is efficient, and, the longer the needle, the more you can skip.


Using method c will have an side-effect if $params is modified. This is because arrays are passed by reference, and so any changes to $params will be visible to the caller when the function returns. Use b if you need to manipulate id or name, unless you intend to return the modifications to the caller.

Example:

$params = array('id' => 1, 'name' => 'b');
$this->d($params);
echo $params['id']; // outputs 2
...

function d($params) {
    $params['id']++;
}