OrderedList class template I called my template OrderedList, but I don't like the name. Is there a better one?

It stores a vector of values of type T, and a map from each value to its index in the vector.

Just for information, the code should be pure C++03, so please don't suggest how it could be improved with C++11/14/17 extensions, or with Boost.

template<class T> class OrderedList {
    public:
        typedef typename std::map<T, int> MapInd;
    public:
        int Insert(const T& val); // insert new value, return its index
        int Find(const T& val) const; // find index for the value, or -1 if not found
        const T& operator[](int i) const {return vValues[i];}
        int Size() const {return vValues.size();}
    private:
        MapInd mapIndices; // index for each element
        std::vector<T> vValues; // element for each index
};

template<class T> int OrderedList<T>::Insert(const T& val) {
    if (Size() < 0)
        return -1;
    int ind = Find(val);
    if (ind >= 0)
        return ind;
    ind = Size();
    mapIndices[val] = ind;
    vValues.push_back(val);
    return ind;
}

template<class T> int OrderedList<T>::Find(const T& val) const {
    typename MapInd::const_iterator it = mapIndices.find(val);
    if (it != mapIndices.end())
        return it->second;
    return -1;
}

Name: I would call it an "Unordered Set"
You have a container that you can add items to. Duplicates are not accepted and return the index of the existing value. New values are added to the container in the order they are inserted.
Because items can be inserted in any order iterating through the container returns the values in insert order (somewhat random) so the container is not sorted in any way.
Issues: You keep two copies of each value.
There is a copy in the map and a copy in the vector. So your container uses twice as much spaces it needs too (especially for big objects).
What you have implemented looks like  std::set
But it has worse space requirements.
Comparison to std::set
std::set<int>   c1;                  OrderedList c1;

auto p1 = c1.insert(5);              auto p1 = c1.Insert(5);
auto p2 = c1.insert(5);              auto p2 = c1.Insert(5);
if (p1.first == p2.first) {          if (p1 == p2) {
    std::cout << "Equal\n";              std::cout << "Equal\n";
}                                    }

auto p3 = c1.find(6);                auto p3 = c1.find(6);
if (p3.first == c1.end()) {          if (p3 == -1) {
    std::cout << "Not Found\n";          std::cout << "Not Found\n"
}                                    }

auto p4 = c1.find(5);                auto p4 = c1.find(5);
if (p4 == p1.first) {                if (p4 == p1) {
    std::cout << "Already there\n";      std::cout << "Already there\n"
}                                    }

std::cout << c1.size() << "\n";      std::cout << c1.Size() << "\n";

std::cout << *(p1.first) << "\n";    std::cout << c1[p1] << "\n";


Using AutoMapper in ASP.Net MVC I'd like to check my understanding and usage of the AutoMapper in my ASP.Net MVC app.

I setup the maps in Application_Start:

 Mapper.CreateMap<Customer, CustomerViewModel>();
 Mapper.CreateMap<CustomerViewModel, Customer>();
 Mapper.AssertConfigurationIsValid();

To return a list of users associated with the logged in user, in my Customer Controller I have:

    //
    // GET: /Customer/

    public ActionResult Index()
    {
        var customer = db.Customers.Where(x => x.UserName == User.Identity.Name).ToList();
        IList<CustomerViewModel> customerVM = Mapper.Map<IList<Customer>, IList<CustomerViewModel>>(customer);
        return View(customerVM);
    }

To Edit a specific customers details I am using:

    //
    // GET: /Customer/EditPartial
    public ActionResult EditPartial(int id)
    {
        var customer = db.Customers.Where(x => x.UserName == User.Identity.Name && x.CustomerId == id).FirstOrDefault();
        CustomerViewModel customerVM = Mapper.Map<Customer, CustomerViewModel>(customer);
        return PartialView(customerVM);
    }

Where I Post back updates after making changes in the EditPartial view:

    //
    // POST: /Customer/EditPartial
    [HttpPost]
    [ValidateAntiForgeryToken]
    public ActionResult EditPartial(CustomerViewModel customerVM)
    {
        if (ModelState.IsValid)
        {
            Customer customer = db.Customers.Where(x => x.UserName == User.Identity.Name && x.CustomerId == customerVM.CustomerId).FirstOrDefault();
            if (customer == null)
            {
                return HttpNotFound();
            }
            customer.CustomerName = customerVM.CustomerName;

            // How do I use AutoMapper here, instead of updating the domain model directly
            customer.Email = customerVM.Email;
            db.Entry(customer).State = EntityState.Modified;
            db.SaveChanges();
            /////////////////

            // Now I want to return a customer list again
            // Is this the best way to do it again
            var customer2 = db.Customers.Where(x => x.UserName == User.Identity.Name).ToList();
            IList<CustomerViewModel> customerVM2 = Mapper.Map<IList<Customer>, IList<CustomerViewModel>>(customer2);

            return PartialView("CustomerListPartial", customerVM2);
        }
        this.Response.StatusCode = 400;
        return PartialView("EditPartial", customerVM);
    }

The code above works, I'm just not sure about using AutoMapper in the Post, and if my way of returning a partial list of customers is the best method.

If you were not using AutoMapper, your POST method would either have a block of manual mapping, or it would call out to a custom mapper class. In the latter case, you would have a line (or two) calling that helper class instead of a line calling AutoMapper.

Whether returning a PartialView is the best method depends on your UI; if that's what your View needs, then that's what you need to to. (I personally don't use them in my markup.)

I wager the reason you think the mapping doesn't belong in your POST method is because your Controller is not just concerned with Control, but also Persistence and Construction. If the rest of your application is indeed this simple, then you might as well stick with it, but as complexity grows, you should consider hiding the persistence concerns behind an interface.


Removing an object which is wrapped in another object from a collection I have Clips that can be added to or removed from any number of Lines. When I add the Clip, I put it into a ClipHolder so I can keep some bounds information with it which is specific to the Line / Clip relationship.

The trouble comes when I want to remove a Clip from the Line. I need to remove the ClipHolder in the associatedClips Set which holds the Clip, but I cannot just call associateClips.remove(Clip) (obviously as the set is holding ClipHolders, not Clips) so I am stuck iterating through the set, and searching one by one for it (see the removeClip method below.)

Within the Line class the ClipHolder class is simply an implementation detail that the user should not have to worry about. They should only have to worry about their Clips which are a separate class.

I would like to avoid burdening the users of this class with requiring them to choose a unique token to identify each instance of Clip. I think that  having a reference to the Clip should be enough. (Is this reasonable?)

Does anyone have a good way of cleaning this up?

class Line {

    .....

    private class ClipHolder{
        Clip c;
        Bounds bounds;
    }

    private TreeSet<ClipHolder> associatedClips = new TreeSet<ClipHolder>();

    public void addClip(Clip c){

        ClipHolder holder = new ClipHolder();
        holder.c = c;
        calculateBounds(holder);

        associatedClips.add(holder);
    }

    public void removeClip(Clip c){

        for (ClipHolder ih: associatedClips ) {
            if(ih.c == c){
                associatedClips.remove(ih);
                break;
            }
        }
    }
}

All you need is a (hash) map for mapping a Clip to its ClipHolder. That way you can remove Clips in constant time. I have added a couple of comments directly in the code snippet:

Line.java:

import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;

class Line {

    private static class Bounds {
        //...
    }

    // Clip is immutable. It is a good idea to override 'hashCode' and 'equals'
    // in order to make sure that we can safely use 'Clip's as keys in a hash
    // map.
    private static class Clip {
        private final String name;

        Clip(String name) {
            this.name = name;
        }

        public String getName() {
            return name;
        }

        @Override
        public int hashCode() {
            return name.hashCode();
        }

        @Override
        public boolean equals(Object o) {
            if (!(o instanceof Clip)) {
                return false;
            }

            return ((Clip) o).getName().equals(this.getName());
        }

        @Override
        public String toString() {
            return "[Clip: " + name + "]";
        }
    }

    // Keyword 'static' removes an implicit reference to Line from each
    // 'ClipHolder', thus saving some space. Declare your ClipHolder a
    // 'Comparable'.
    private static class ClipHolder implements Comparable<ClipHolder> {
        private final Clip clip;
        private final Bounds bounds;

        ClipHolder(Clip clip, Bounds bounds) {
            this.clip = clip;
            this.bounds = bounds;
        }

        @Override
        public int compareTo(ClipHolder o) {
            return this.getClip().getName().compareTo(o.getClip().getName());
        }

        public Clip getClip() {
            return clip;
        }

        public Bounds getBounds() {
            return bounds;
        }
    }

    // 1: Program to interface, not implementation.
    // 2: Since Java 7, you can use diamond inference "<>".
    private final Set<ClipHolder> associatedClips = new TreeSet<>();
    private final Map<Clip, ClipHolder> map = new HashMap<>();

    public void addClip(Clip clip) {
        Bounds bounds = null; // Compute your bounds.
        ClipHolder holder = new ClipHolder(clip, bounds);
        associatedClips.add(holder);
        // Map a clip to its holder.
        map.put(clip, holder);
    }

    public void removeClip(Clip c) {
        associatedClips.remove(map.get(c));
    }

    @Override
    public String toString() {
        StringBuilder sb = new StringBuilder("[Line: ");

        int i = 0;

        for (ClipHolder ch : associatedClips) {
            sb.append(ch.getClip().toString());

            if (i < associatedClips.size() - 1) {
                sb.append(", ");
                ++i;
            }
        }

        return sb.append("]").toString();
    }

    public static void main(String[] args) {
        Line line = new Line();
        line.addClip(new Clip("A"));
        line.addClip(new Clip("B"));
        line.addClip(new Clip("C"));
        line.addClip(new Clip("D"));
        System.out.println(line);

        line.removeClip(new Clip("B"));
        System.out.println(line);
    }
}


An installer for shell scripts, written in pure sh I wrote a tiny shell script that basically installs a script into your system by copying it to someplace like /usr/local/bin, chmodding it, and adding it to your PATH. It works on both Unix and Windows (via an emulation layer like MSys or Cygwin). Its source code is available on GitHub. Here is the script in its entirety:

#!/bin/sh

set -e

help()
{
    echo 'sinister is a simple installer for shell scripts.'
    echo
    echo 'Usage: sinister -u url [options]'
    echo
    echo 'Options:'
    echo
    echo '-?,-h,--help: Print this help and exit.'
    echo '-c,--chmod: The mode to chmod your script. Defaults are a+x and u+x.'
    echo '-d,--debug: sinister should be run in debug mode.'
    echo '-l,--local: The script should be saved for the current user, instead of the entire machine.'
    echo '-n,--name: The name of the file the script should be saved to. Mandatory if no URL is given.'
    echo '-o,--output: The output directory the script should be saved to.'
    echo 'By default this is ~/bin on Unix systems, and C:\Users\you\AppData\Local\script on Windows.'
    echo '-u,--url: The location of the script online. e.g: https://github.com/you/repo/raw/master/script'
    echo 'If no URL is given the script is read from stdin.'
}

on_windows()
{
    uname | grep -q '[CYGWIN|MINGW|MSYS]'
}

per_user()
{
    test $LOCAL = 'true'
}

run_powershell()
{
    powershell -NoProfile -ExecutionPolicy Bypass "$1"
}

getpath_windows()
{
    run_powershell "[Environment]::GetEnvironmentVariable('PATH', '$1')"
}

setpath_windows()
{
    run_powershell "[Environment]::SetEnvironmentVariable('PATH', '$1', '$2')"
}

pwd_windows()
{
    run_powershell 'Get-Location'
}

CHMOD=
LOCAL='false'
NAME=
OUTPUT=
URL=

test $# -ne 0
while [ $# -gt 0 ]
do
    case "$1" in
        '-?'|-h|--help) help; exit 0 ;;
        -c|--chmod) CHMOD="$2"; shift ;;
        -d|--debug) set -x ;;
        -l|--local) LOCAL='true' ;;
        -n|--name) NAME="$2"; shift ;;
        -o|--output) OUTPUT="$2"; shift ;;
        -u|--url) URL="$2"; shift ;;
        *) help; exit 1 ;;
    esac
    shift
done

if [ -z "$CHMOD" ]; then
    if per_user; then
        CHMOD='u+x' # Executable for the current user
    else
        CHMOD='a+x' # Executable for all users
    fi
fi

if [ -z "$URL" ]; then
    test ! -z "$NAME"
    SCRIPT=$(< /dev/stdin) # read from standard input
else
    NAME=${URL##*/} # Grab everything after the last /
    if which curl > /dev/null 2>&1; then
        SCRIPT=$(curl -sSL "$URL")
    else # Assume wget is installed
        SCRIPT=$(wget -q -O - "$URL")
    fi
fi

if [ -z "$OUTPUT" ]; then
    if per_user; then
        if on_windows; then
            OUTPUT=~/AppData/Local/"$NAME"
        else
            OUTPUT=~/bin
        fi
    else
        if on_windows; then
            OUTPUT='/C/Program Files/$NAME'
        else
            OUTPUT='/usr/local/bin'
        fi
    fi
fi

# Use sudo on Unix
! on_windows && SUDO='sudo'

# Where the magic happens
$SUDO mkdir -p "$OUTPUT"
cd "$OUTPUT"
echo "$SCRIPT" | $SUDO tee "$NAME" > /dev/null
$SUDO chmod $CHMOD "$NAME"

# Add $OUTPUT to PATH if it's not in it
if ! which "$NAME" > /dev/null 2>&1; then
    if on_windows; then
        if per_user; then
            TARGET='User'
        else
            TARGET='Machine'
        fi
        CURRENT_PATH=$(getpath_windows $TARGET)
        CURRENT_DIRECTORY=$(pwd_windows)
        setpath_windows "$CURRENT_PATH;$CURRENT_DIRECTORY" $TARGET
    else # Unix
        CONTENTS="export PATH=\$PATH:$OUTPUT"
        if per_user; then
            echo "$CONTENTS" >> ~/.profile
        else
            sudo echo "$CONTENTS" >> /etc/profile
        fi
    fi
fi

Any feedback would be appreciated; I haven't tested this on Windows yet so some of the PowerShell snippets might be untested.

I see a number of things that may help you improve this program.  I don't have a Windows machine handy, so these will all be Linux-based observations.

For local installation, don't use sudo

Right now, when the program is created for a local user, the use of sudo to copy it means it ends up owned by root.  That means that although it's in the local user's bin, they can't run it.  For local installation, sudo shouldn't be needed at all and would be a lot safer.

Don't duplicate entries in .profile

Right now, each invocation of your program will cause an additional possibly duplicate line to be added to .profile.  Better would be to only add a line if it doesn't yet exist there.

Use only POSIX features

Right now the code includes this line:

SCRIPT=$(< /dev/stdin) # read from standard input

However, that's not necessarily portable.  Instead, use cat as in:

SCRIPT=$(cat /dev/stdin) # read portably from standard input


Find the minimum path sum in a binary tree I am working on the problem of finding the minimum path sum in a binary tree, and printing the path. The path can be from the root node to any leaf node.

Here is my code in Python 2.7. I am looking for advice for bugs, performance improvement ideas or general code style advice.

One specific question is: I am using a list result to track minimal sum so far, because a simple integer in Python 2.7 is not reference type (I mean change the value of an integer in one function call, does not impact the value of same variable in another function). Are there any more elegant ways to track the minimal sum so far?

class TreeNode:
    def __init__(self, value, left, right):
        self.value = value
        self.left = left
        self.right = right
    def min_path(self, sum_so_far, prefix, result, result_path):
        if self.left:
            prefix.append(self.value)
            self.left.min_path(sum_so_far + self.value, prefix, result, result_path)
            prefix.pop(-1)
        if self.right:
            prefix.append(self.value)
            self.right.min_path(sum_so_far + self.value, prefix, result, result_path)
            prefix.pop(-1)
        if not self.left and not self.right: # leaf node
            prefix.append(self.value)
            if sum_so_far + self.value < result[0]:
                result[0] = sum_so_far + self.value
                if len(result_path) > 0:
                    result_path.pop(0)
                result_path.append(prefix[:])
            prefix.pop(-1)
if __name__ == "__main__":
    root = TreeNode(1, TreeNode(2, TreeNode(-4, None, None), TreeNode(5, None, None)), TreeNode(3, TreeNode(6, None, None), TreeNode(-7, None, None)))
    result = [float('inf')]
    result_path = []
    root.min_path(0, [], result, result_path)
    print result, result_path

There's no docstring. What kind of thing is a TreeNode? What does the min_path method do? What should I pass for each of the parameters?
The code is written in Python 2.7, which is obsolescent (it gets bug fixes but no new features). We'll see later that this causes inconvenience because of the lack of the nonlocal statement. I suggest switching to Python 3.
The duplicated code for pushing and popping the prefix list, and updating sum_so_far, can be avoided, like this:

def min_path(self, sum_so_far, prefix, result, result_path):
    prefix.append(self.value)
    sum_so_far += self.value
    if self.left:
        self.left.min_path(sum_so_far, prefix, result, result_path)
    if self.right:
        self.right.min_path(sum_so_far, prefix, result, result_path)
    if not self.left and not self.right: # leaf node
        if sum_so_far < result[0]:
            result[0] = sum_so_far
            if len(result_path) > 0:
                result_path.pop(0)
            result_path.append(prefix[:])
    prefix.pop(-1)

The duplicated code for visiting the children can be avoided, like this:

children = [c for c in (self.left, self.right) if c]
if children:
    for child in children:
        child.min_path(sum_so_far, prefix, result, result_path)
else: # leaf node
    # etc.

But a better approach would be to use a locally defined function, like this:

def min_path(root):
    """Return list of values on the minimum path from root to a leaf."""
    min_path = []
    min_sum = [float('inf')]
    prefix = []

    def visit(node, sum_so_far):
        prefix.append(node.value)
        sum_so_far += node.value
        children = [c for c in (node.left, node.right) if c]
        if children:
            for child in children:
                visit(child, sum_so_far)
        elif sum_so_far < min_sum[0]:
            min_sum[0] = sum_so_far
            min_path[:] = prefix[:]
        prefix.pop(-1)

    visit(root, 0)
    return min_path

This still uses the trick of updating the first element of a list in order to update a result inside a possibly nested function call, but at least here the trick is hidden inside the implementation, and not exposed to the caller.

(In Python 3, you could write nonlocal min_sum and so avoid the need for the trick.)
Alternatively, you could use the stack of iterators pattern. This avoids making recursive function calls, and so there's no difficulty in maintaining the running minimum.

def min_path(root):
    """Return list of values on the minimum path from root to a leaf."""
    min_path = []
    min_sum = float('inf')
    current_path = [0]
    current_sum = 0
    stack = [iter([root])]
    while stack:
        for node in stack[-1]:
            current_path.append(node.value)
            current_sum += node.value
            children = [n for n in (node.left, node.right) if n]
            stack.append(iter(children))
            if not children and current_sum < min_sum:
                min_sum = current_sum
                min_path = current_path[1:]
            break
        else:
            current_sum -= current_path.pop()
            stack.pop()
    return min_path


Should this Point class remain a class, or become a struct? I am just beginning to dabble in C++. Would you implement this as a class like I did, or a struct? What are the pros and cons?

class Point
{
private:
    double _x;
    double _y;

public:
    Point();
    Point(double x, double y);
        ~Point();
        Point(Point& other);
        double GetX();
        void SetX(double x);
        double GetY();
        void SetY(double y);
};

Point::Point() : _x(0.0), _y(0.0)
{
}

Point::Point(double x, double y) : _x(x), _y(y)
{
}

Point::Point(Point& other) : _x(other._x), _y(other._y)
{
}

Point::~Point()
{
}

double Point::GetX()
{
    return _x;
}

void Point::SetX(double x)
{
    _x = x;
}

double Point::GetY()
{
    return _y;
}

void Point::SetY(double y)
{
    _y = y;
}

If you are using point as a property bag (like this).

Then just make it a struct (leave the members public). If you think there is a slight possibility of modifying the implementation then use get/set (ers) to access the memebrs but this class is so simple it seems over kill.

If this is part of a school class then your professor is going to say use member accessors so that the internal implementation can change and the internal implementation can change without the class interface being changed.

Though true in this simplistic case its just not going to happen.

Though I would add constructors to make sure the members are correctly initialized.

PS. Try not to use a leading underscore on identifiers. Though in this case it is not a problem the rules are non trivial and not well known so best avoided. see https://stackoverflow.com/a/228797/14065


Run-aware merge sort The concept is very, very simple: let's do merging run by run (a run is a range without decrement).

template <class RandomIt>
void RunAwareMergeSort(RandomIt first, RandomIt last) {
    RandomIt m_first(first); // the begin of first run of merge
    RandomIt i(first); // the locater
    while (true) {
        while (++i < last && !(*i < *(i-1))); // locate the first run
        if (!(i < last)) { // i has located the end of the last run
            if (m_first == first) return; // the range has only one run; sorting completed
            else { // the range has odd number of runs
                m_first = i = first; // start over
                continue;
            }
        }
        RandomIt m_middle(i); // the end of first run of merge; the begin of second run of merge
        while (++i < last && !(*i < *(i-1))); // locate the second run
        std::inplace_merge(m_first, m_middle, i); // merge
        if (i < last) m_first = i; // next
        else m_first = i = first; // start over
    }
}

Best-case time complexity: \$O(n)\$ (which is better than typical merge sort)
Average-case time complexity: \$O(n \log n)\$
Worst-case time complexity: \$O(n \log n)\$
Worst-case space complexity: \$O(n)\$
Stability: true

This is slower than Timsort, right?

The algorithms in the standard library which care about order usually have an additional overload that takes a comparison function to replace operator<. It can be useful to compare instances of types that do not have an overloaded operator< or to compare things differently (for example, passing an instance of std::greater<> to std::sort will sort a collection in reverse order). Transforming your algorithm to take such a comparison function is more or less trivial:

template <class RandomIt, class Compare>
void RunAwareMergeSort(RandomIt first, RandomIt last, Compare compare) {
    RandomIt m_first(first); // the begin of first run of merge
    RandomIt i(first); // the locater
    while (true) {
        while (++i < last && !compare(*i, *(i-1))); // locate the first run
        if (!(i < last)) { // i has located the end of the last run
            if (m_first == first) return; // the range has only one run; sorting completed
            else { // the range has odd number of runs
                m_first = i = first; // start over
                continue;
            }
        }
        RandomIt m_middle(i); // the end of first run of merge; the begin of second run of merge
        while (++i < last && !compare(*i, *(i-1))); // locate the second run
        std::inplace_merge(m_first, m_middle, i, compare); // merge
        if (i < last) m_first = i; // next
        else m_first = i = first; // start over
    }
}

From a style point of view, your code could use more braces to avoid a potential Apple goto fail; bug and to let it breathe a bit more. Also, when you have to comment almost every line, it's almost always better to put the full explanation along the algorithm and not into it.

You also seemed to be concerned about the performance of your algorithm. I tested it with several patterns and compared it to several stable sorting algorithms that I had in a library. It always sorts an std::vector<int> with \$10^6\$ elements. Here are the results (Stable sort is std::stable_sort, yours is the natural mergesort, the other algorithms are not in the standard library):

As you can see, you algorithm is a bit better than Timsortfor its worst case (random data) and beats every other algorithm for the very specific "ascending sawtooth" pattern, which corresponds to a succession of ascending data (more or less log2(std::distance(first, last)) in our case). However, it slower than almost any other sorting algorithm whenever the data isn't mostly sorted in ascending order and it really doesn't like descending patterns.


Critique my JSON-optimizing Python script Here's a Python script to optimize JSON documents:

#!/usr/bin/python3

"""jopo - Optimize JSON documents."""

from collections import defaultdict

def optimize(data):
    """Optimize a JSON-like data structure."""
    if isinstance(data, list):
        if len(data) == 1:
            return optimize(data[0])
        digest = defaultdict(lambda: defaultdict(set))
        for element in data:
            if not isinstance(element, dict) or len(element) != 2:
                break
            for key, value in element.items():
                digest[key][type(value)].add(repr(value))
        else:
            if len(digest) == 2:
                pool = {k for k in digest if len(digest[k][str]) == len(data)}
                if pool:
                    if len(pool) > 1:
                        meta = input("Which key (%s)? " % "|".join(pool))
                    else:
                        meta = next(iter(pool))
                    infra = (digest.keys() - {meta}).pop()
                    return {d[meta]: optimize(d[infra]) for d in data}
        return [optimize(x) for x in data]
    if isinstance(data, dict):
        if len(data) == 1:
            return optimize(next(iter(data.values())))
        else:
            return {k: optimize(v) for k, v in data.items()}
    return data

if __name__ == "__main__":
    import json
    import sys
    for arg in sys.argv[1:]:
        with open(arg) as f:
            doc = json.load(f)
        print(json.dumps(optimize(doc), indent=2))

... which, given a JSON document recipes.json like this:

{
  "recipes": [
    {
      "name": "pizza",
      "ingredients": [
        {
          "quantity": "100g",
          "ingredient": "cheese"
        },
        {
          "quantity": "200g",
          "ingredient": "tomato"
        }
      ]
    },
    {
      "name": "pizza 2",
      "ingredients": [
        {
          "quantity": "300g",
          "ingredient": "ham"
        },
        {
          "quantity": "300g",
          "ingredient": "pineapple"
        }
      ]
    }
  ]
}

... is used like this:

$ ./jopo.py recipes.json
Which key (ingredient|quantity)? ingredient
{
  "pizza 2": {
    "ham": "300g",
    "pineapple": "300g"
  },
  "pizza": {
    "tomato": "200g",
    "cheese": "100g"
  }
}

It does two things:

Collapse 1-item objects and arrays.
Reduce arrays of congruent 2-item objects to a single object, if possible.

I think the optimize() function is both too long and too hard to follow.

Obviously I could separate out the if isinstance(data, <type>): # ... blocks into separate functions, but even then, an optimize_list() function along those lines is still going to be quite long and, IMO, somewhat impenetrable.

How can I improve the legibility of this code?

digest = defaultdict(lambda: defaultdict(set))

It's pretty rare that we have a good reason for such a nested data structure in python.

    for element in data:
        if not isinstance(element, dict) or len(element) != 2:
            break
        for key, value in element.items():
            digest[key][type(value)].add(repr(value))

Why are we adding the repr of the value? I assume you do it to avoid taking the hash of unhashable objects. But what if somebody puts some strange object in your list that has a repr which always returns the same thing. Besides you are only interested in strings.

    else:
        if len(digest) == 2:

This is an unintuive way to check for your requirement. Your requirement is actually that all elements have the same keys. Check for that instead.

            pool = {k for k in digest if len(digest[k][str]) == len(data)}

Your actually checking to see if that key was always a string. Perhaps you should check that more directly.

            if pool:
                if len(pool) > 1:
                    meta = input("Which key (%s)? " % "|".join(pool))

Its deeply suspicious if you need to ask the user for instructions.

                else:
                    meta = next(iter(pool))
                infra = (digest.keys() - {meta}).pop()
                return {d[meta]: optimize(d[infra]) for d in data}
    return [optimize(x) for x in data]

Here is my reworking of it.

def optimize(data):
    """
    Dispatcher, figure out which algorithm to use to compress the json
    """
    if isinstance(data, list):
        if len(data) == 1:
            return optimize(data[0])
        else:
            keys = determine_key(data)
            if keys is None: # unable to determine tkeys
                return [optimize(element) for element in data]
            else:
                return optimize_list(data, keys)
    elif isinstance(data, dict):
        if len(data) == 1:
            return optimize(next(iter(data.values())))
        else:
            return optimize_dict(data)
    else:
        return data

def optimize_dict(data):
    """
    Optimize a dict, just optimizes each value
    """
    return {key : optimize(value) for key, value in data.items()}

def can_be_key(data, key):
    """
    return true if key could be a key
    """
    if not all( isinstance(element[key], str) for element in data):
        return False
    return len(set(element[key] for element in data)) == len(data)

def determine_key(data):
    """
    Determine the key, value to compress the list of dicts
    or None if no such key exists
    """
    for element in data:
        if not isinstance(element, dict):
            return None
        if len(element) != 2:
            return None
        if element.keys() != data[0].keys():
            return None

    key1, key2 = data[0].keys()
    key1_possible = can_be_key(data, key1)
    key2_possible = can_be_key(data, key2)

    if key1_possible and key2_possible:
        meta = input("Which key (%s|%s)? " % (key1, key2))
        if meta == key1:
            return key1, key2
        else:
            return key2, key1
    elif key1_possible:
        return key1, key2
    elif key2_possible:
        return key2, key1
    else:
        return None

def optimize_list(data, keys):
    key_key, key_value = keys
    return {element[key_key]: optimize(element[key_value]) for element in data}

But... I must question the wisdom of the entire approach. The function is actually rather useless. Suppose that you use it on web service that returns a json object. It gives you

{"results" : [
     {"name" : "foo", "age": 42}
     {"name" : "bar", "age": 36}
}

Which optimize to

{"foo": 42, "age": 36}

So you write:

for name, age in optimize(the_json).items():
    print(name, age)

But then in another request, the server returns

{"results" : [
     {"name" : "foo", "age": 42}
}

which optimizes to:

42

So when you run:

for name, age in optimize(the_json).items():
    print(name, age)

You get an error.

Or the server adds extra data to the json:

{
 "salinity" : "high",
 "results" : [
     {"name" : "foo", "age": 42, "status" : "sad"}
     {"name" : "bar", "age": 36, "status" : "happy"}
}

Which should be backwards compatible using json, but will break your code or anything doing the same job.

This is not to say that can't or shouldn't simplify the json. But don't try to do it by analyzing the data like this. You should do it by some external record of the schema.

class Fetch(object):
    """
    Fetch one element from a dictionary
    """
    def __init__(self, name, inner):
        self._name = name
        self._inner = inner

    def __call__(self, data):
        return self._inner(data[self._name])

class KeyValue(object):
    """
    Given a list of dicts, convert to a dictionary
    with each key being taken from the `key` on each element
    and each value being taken from the `value`
    """
    def __init__(self, key, value, inner):
        self._key = key
        self._value = value
        self._inner = inner

    def __call__(self, data):
        return {element[self._key] : self._inner(element[self._value]) for element in data}

class Literal(object):
    def __call__(self, data):
        return data

schema = Fetch("recipes",
    KeyValue("name", "ingredients",
        KeyValue("ingredient", "quantity", Literal())
    )
)

That's actually simpler then what you were doing, and its much more robust. Its also very flexible as you can write code to perform just about any transformation on the data.


Haskell Dynamic Programming on a Tree Here is my shot at Homework 8 of CIS-194 (from Spring '13). The problem is about a hierarchy of employees at a company (a tree), each node being of type Employee, each described by a label (empName :: String) and an integer value (empFun :: Integer). We need to choose a subset of employees so that no two chosen employees have a parent-child relationship within the tree and the sum of empFuns of the chosen employees is maximised.
Some of the outline and structure of this code is based on what was suggested by the document, but I am curious to know if the rest of it is consistent and idiomatic Haskell. I'm not sure what I'm expecting as feedback, but any is welcome.
module Party where

import Data.List ( sort )
import Data.Tree ( Tree(Node) )
import Employee ( Employee(empFun, empName), Fun, GuestList(..) )

instance Semigroup GuestList where
    (GL a b) <> (GL c d) = GL (a ++ c) (b + d)

instance Monoid GuestList where
    mempty = GL [] 0

glCons :: Employee -> GuestList -> GuestList
glCons emp (GL a b) = GL (a ++ [emp]) (empFun emp)

moreFun :: GuestList -> GuestList -> GuestList
moreFun = max

treeFold :: (a -> [b] -> b) -> Tree a -> b
treeFold f (Node label subtree) = f label (map (treeFold f) subtree)

nextLevel :: Employee -> [(GuestList, GuestList)] -> (GuestList, GuestList)
nextLevel emp subList = (glCons emp (foldMap snd subList), foldMap (uncurry max) subList)

maxFun :: Tree Employee -> GuestList
maxFun tree = uncurry moreFun $ treeFold nextLevel tree

getFormattedGL :: GuestList -> String
getFormattedGL gl = unlines (("Total fun " ++ show (fun gl)) : sort (map empName (emps gl)))
    where fun (GL _ fun) = fun
          emps (GL emps _) = emps

work :: [Char] -> String
work = getFormattedGL . maxFun . read

main :: IO ()
main = readFile "company.txt" >>= putStrLn . work

Unfortunately, there is a bug in glCons. The employee's fun should get added, not replace the current fun level:
glCons :: Employee -> GuestList -> GuestList
glCons emp (GL a b) = GL (a ++ [emp]) (empFun emp)
          --     ^

But let's stay on glCons. We can expect glCons to be used via foldr. However, the ++ operator leads to quadratic behaviour. When we add a single element, we should therefore use (:) (and reverse, if the order matters):
glCons :: Employee -> GuestList -> GuestList
glCons emp (GL a b) = GL (emp : a) (empFun emp + b)

Other than that the code seems fine.


Thread-safe stack in Java with fixed capacity Implement a multi-threaded stack, with freedom to use existing
  implementations of stack. On top of being thread-safe, it must block
  (not busy-wait) a pushing thread when stack is full and a popping
  thread when stack is empty. It must signal the threads out of sleep
  when stack has space again or elements to pop. Fairness is optional in this question.

How can I implement such a stack in Java? I came up with below implementation. Is this the correct way to solve above problem?

public class MyStack<E> {
  private final Stack<E> stack;
  private int max = 16;
  private final ReentrantLock lock = new ReentrantLock(true);
  private final Condition notEmpty = lock.newCondition();
  private final Condition notFull = lock.newCondition();

  public MyStack(int size) {
    this.stack = new Stack<>();
    this.max = size;
  }

  public void push(E e) {
    lock.lock();
    try {
      while (stack.size() == max) {
        notFull.await();
      }
      stack.push(e);
      notEmpty.signalAll();
    } catch (InterruptedException e1) {
      e1.printStackTrace();
    } finally {
      lock.unlock();
    }
  }

  public E pop() {
    lock.lock();
    try {
      while (stack.size() == 0) {
        notEmpty.await();
      }
      E item = stack.pop();
      notFull.signalAll();
      return item;
    } catch (InterruptedException e) {
      e.printStackTrace();
    } finally {
      lock.unlock();
    }
  }
}

Looks good to me on a cursory glance. It's pretty much the exact same code as in java.util.concurrent.locks.Condition JavaDoc.

The only definitive problem I see is the catching and logging of InterruptedException. The code acts towards the caller as if the operation succeeded while nothing was done.

Not providing a way to set the maximum wait time will be a limitation in a library intended for generic use.

Stylistically, private int max should be final to emphasize that it must not be changed after initialization (as changing it when threads are waiting would cause serious problems).

Another thing, which may be a matter of taste, is the stack.size() == max comparison. I always like to use >= where a size is compared to a maximum limit (with a comment, of course). This gives an extra safeguard against infinite loops in the case where a programming error causes the collection to exceed maximum size.


Android Thread and Runnable This is my code for running a background thread.  I believe it is very poor in naming and code structure.

package com.ocs.socialshare.bloggershare;

import android.app.Activity;
import android.os.Bundle;
import android.view.Menu;
import android.view.View;
import android.view.View.OnClickListener;
import android.widget.Button;
import android.widget.EditText;

import com.google.gdata.util.AuthenticationException;

public class MainActivity extends Activity implements OnClickListener{

    private Thread doinBackground;

    private EditText username,password,blogurl;
    private Button login;
    private String user,pass,bloglink;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);
        findViewById();
        setOnclickListenerRegister();
    }

    private void findViewById() {
        // TODO Auto-generated method stub
        username        =   (EditText) findViewById(R.id.username);
        password        =   (EditText) findViewById(R.id.password);
        blogurl         =   (EditText) findViewById(R.id.blogurl);
        login           =   (Button) findViewById(R.id.login);
    }

    private void setOnclickListenerRegister() {
        // TODO Auto-generated method stub
        login.setOnClickListener(this);
    }
    private void blogShare(final String user,final String pass,final String bloglink) {
        // TODO Auto-generated method stub

            doinBackground =new Thread(new Runnable()
            {
                @Override
                public void run()
                {
                // TODO Auto-generated method stub
                BloggerClient blog = new BloggerClient(getApplicationContext());
                try {
                    blog.oAuthBlogger(user,pass,bloglink);
                    }
                    catch (AuthenticationException e)
                    {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                    }
                }
            });
        doinBackground.start();

    }

    @Override
    public boolean onCreateOptionsMenu(Menu menu) {
        // Inflate the menu; this adds items to the action bar if it is present.
        getMenuInflater().inflate(R.menu.main, menu);
        return true;
    }

    @Override
    public void onClick(View view) {
        // TODO Auto-generated method stub
        int viewid  = view.getId();

        if(viewid == R.id.login)
        {
        validateFeild();
        blogShare(user,pass,bloglink);
        }
    }

    private void validateFeild() {
        // TODO Auto-generated method stub
        user        =    username.getText().toString();
        pass        =    password.getText().toString();
        bloglink    =    blogurl.getText().toString();

    }

}

Please remove all // TODO Auto-generated method stub comments. Your methods are implemented. You don't need those comments any more.
Indentation. If you're using Eclipse, select all code and press Ctrl + Shift + I to ident your code properly. If you're using NetBeans, press Alt + Shift + F. Your try-catch inside the blogShare method is incorrectly indented.
The correct spelling is validateField
user, pass and bloglink are only set within validateField and only read directly after that method invocation. Get rid of the validateField method (which currently doesn't do any actual validation so it's a poorly named method). Use local variables within the onClick method.
int viewid  = view.getId(); is IMO not needed to set as a variable, as you only check it once, use view.getId() directly.
setOnclickListenerRegister is only called once from onCreate and this method only contains one line, so what you have done is to extract one statement to it's own method and then call that method which were only called once. This extraction is not worth it. Remove this method and put login.setOnClickListener(this); back to the onCreate method where it belongs.
I totally agree with Marco that you should use AsyncTask. If you encounter any problems with orientation changes, deal with those correctly. Don't avoid using AsyncTask just because you're afraid that it will cause problems.


Checking if switch is checked and doing something with the data How can I optimize this code?

for(var i = 0; i<people_to_save_parse.length;i++)
{
                if($(".ID-"+people[i].tdid).is(':checked')==true)
                {
                    people_to_save_parse[i].id = $(".ID-text-"+people[i].tdid).text();
                }else{
                    people_to_save_parse[i].id = "";
                }
                if($(".Name-"+people[i].tdid).is(':checked')==true)
                {
                    people_to_save_parse[i].firstname = $(".Name-text-"+people[i].tdid).text();
                }else{
                    people_to_save_parse[i].firstname = "";
                }
                if($(".Surname-"+people[i].tdid).is(':checked')==true)
                {
                    people_to_save_parse[i].lastname = $(".Surname-text-"+people[i].tdid).text();
                }else{
                    people_to_save_parse[i].lastname = "";
                }
}

With a function:

var checkedValue = function (tdid, field) {
    if ($("." + field + "-" + tdid).is(':checked')) {
        return $("." + field + "-text-" + tdid).text();
    }
    return "";
}

and you could then simply call that with:

for(var i = 0; i < people_to_save_parse.length; i++) {
    people_to_save_parse[i].id = checkedValue(people[i].tdid, "ID");
    people_to_save_parse[i].firstname = checkedValue(people[i].tdid, "Name");
    people_to_save_parse[i].lastname = checkedValue(people[i].tdid, "Surname");
}

This removes much of the code duplication.

Note also that convention puts the { at the end of the line in JavaScript, not at the start of the next line. Also, you should add space between your operators and expressions to make them more readable. Finally, you don't need to add the == true as it is not useful in the if condition since the is(":checked") function returns a boolean already.

A concern I have is that you have two arrays, people_to_save_parse and people. You index them both with the same i index, and there is no apparent control that ensures they are 'in sync'. Are they a 1-to-1 matching pair of arrays?


Count overlapping substrings I'm looking for feedback on the actual logic of this simple Python 2.7 script.  The automated grader passed it, but I expect it could be make more 'pythonic'.  It was written for homework, and some of the constraints included basic looping constructs only and no function declarations.  This differs from a simple String.find() because it must count substrings with common characters. For example, "bobob" must match "bob" twice.

#! /usr/bin/python
s = 'azcbobobegghakl'  #2 bobs
s = 'auoeunbobnoeubobob,ntonethbobobo'  #5 bobs

seek = 'bob'
matches = 0

for i in range(0,len(s)):
    if s[i] == seek[0]:  #if any input char matches seek string
        if abs(i-len(s)) < len(seek):  #checks not closer to end of input than seek
            break
        match_maybe = True
        match = False
        seek_index = 0
        while (match_maybe):  #look ahead to see if subsequent chars match seek
            seek_index += 1
            if seek_index == len(seek):  #successful match each char of seek
                match = True
                break
            if s[i+seek_index] != seek[seek_index]: #does not match
                match_maybe = False
                break
        if match:
            matches += 1

print 'Number of times bob occurs is: ' + str(matches)

First things first, make this a function.
If you then add a doctest to the function you can test if it works correctly, without having to change the code.
This is good, as it also describes how to use it to your users.
Whilst this is a homework assignment, but it's better to get used to using these things.

At the moment you should have the following code.
The import doctest is so that you don't have to use the command line to test the code.
This is good if you don't know how to use the command line or if there are problems with Python and the system PATH.

def overlapping_count(s, seek):
    """
    >>> overlapping_count('azcbobobegghakl', 'bob')
    2
    >>> overlapping_count('auoeunbobnoeubobob,ntonethbobobo', 'bob')
    5
    """
    matches = 0
    # Rest of code...
    return matches

if __name__ == "__main__":
    import doctest
    doctest.testmod()

The biggest improvement in clarity that you can make with the code you have is to use slicing.
Python allows you to index from a beginning to an end of a sliceable object.
To use this is quite simple.

>>> s = 'Lorem ipsum dolor sit amet, consectetur adipiscing elit.'
>>> s[0]
'L'
>>> s[1]
'o'
>>> s[0:1]
'L'
>>> s[0:5]
'Lorem'

As you can probably guess, using this for your match check will greatly decrease the amount of lines, and clarity.
Now the inside of your for-loop can be:

if s[i] == seek[0]:
    if s[i:i + len(seek)] == seek:
        matches += 1

If you use some (premature?) optimizations, you can make your code faster.
It keeps roughly the same amount of complexity in the function.
But mostly, you don't want to waste CPU cycles on things you can remember.
Therefore you would want to store both len(seek) and seek[0] in variables.
Finally enumerate can add a small speed boost to the function,
and is also generally preferred in this scenario.

This solution is twice as fast as your solution:

def overlapping_count(string, seek):
    """
    >>> overlapping_count('azcbobobegghakl', 'bob')
    2
    >>> overlapping_count('auoeunbobnoeubobob,ntonethbobobo', 'bob')
    5
    """
    matches = 0
    seek_len = len(seek)
    seek_c = seek[0]
    for i, c in enumerate(string):
        if c == seek_c and string[i:i + seek_len] == seek:
            matches += 1
    return matches


Generate random 10% of file to be used in testing with Ruby I'm new to Ruby, but not to programming.  I just need a simple script that, given a text file, will pull out around 10% of the lines at random.  Below is what I came up with based upon a Python script I wrote.  What do I need to do differently to make it more Ruby-like?

prng = Random.new

File.open('english-words-partial.txt', 'w') {  |f|
  File.readlines('english-words-full.txt').each do |line|
    if prng.rand >= 0.9
      f.write(line)
    end
  end
}

Here's my shot at it:

File.open('english-words-partial.txt', 'w') do |file|
    File.foreach('english-words-full.txt') do |line|
        file.puts(line) if rand(10) == 0
    end
end

In Ruby, you can add an if statement to the end of a line, to conditionally execute that line. This also works with the unless statement.

Also, the rand function can take a number, and produce that many possible numbers from it. So rand(10) will give you a random number from 0 to 9

In Ruby, you usually want to use { } for 1 line blocks, and do/end for multiline blocks, but they both act slightly different. Theres more on that here: https://stackoverflow.com/questions/5587264/do-end-vs-curly-braces-for-blocks-in-ruby

File.foreach('...') is just a bit shorter and neater then File.readlines('...').each (thanks to @Flambino for this)

And lastly, you can use IO methods on a file object like print and puts, so I liked that better but file.write works too. Thats really personal preference.


Combine and condense PowerShell arrays I need to combine 3 sets of arrays that contain PsCustomObject elements, such that the unique elements of each array is only listed once, but has properties that indicate the source.

# create test cases
$a = [PsCustomObject]@{Id='AAA';Name='Dev'}
$b = [PsCustomObject]@{Id='BBB';Name='Dev/Test'}
$c = [PsCustomObject]@{Id='CCC';Name='Dev/Prod'}
$d = [PsCustomObject]@{Id='DDD';Name='Dev/Test/Prod'}
$e = [PsCustomObject]@{Id='EEE';Name='Test'}
$f = [PsCustomObject]@{Id='FFF';Name='Test/Prod'}
$g = [PsCustomObject]@{Id='GGG';Name='Prod'}

# add to arrays
$dev=@()
$dev += $a
$dev += $b
$dev += $c
$dev += $d

$test=@()
$test += $b
$test += $d
$test += $e
$test += $f

$prod=@()
$prod+=$c
$prod+=$d
$prod+=$f
$prod+=$g

# array to contain the results
$result=@()

# process dev list; decorate w/ additional properties
$dev | % {
    Add-Member -InputObject $_ -NotePropertyName "DEV" -NotePropertyValue "Y"
    Add-Member -InputObject $_ -NotePropertyName "TEST" -NotePropertyValue $null
    Add-Member -InputObject $_ -NotePropertyName "PROD" -NotePropertyValue $null
    # add to results
    $result += $_
}

# process test list
$test | % {

    $match = $result -match $_.Id

    # if a test element matches an element in the results
    if ($match) {
        # set the column
        $match[0].TEST='Y'
    }
    # if this element is not in the results
    else {
        # additional properties
        Add-Member -InputObject $_ -NotePropertyName "DEV" -NotePropertyValue $null
        Add-Member -InputObject $_ -NotePropertyName "TEST" -NotePropertyValue "Y"
        Add-Member -InputObject $_ -NotePropertyName "PROD" -NotePropertyValue $null
        # add to results
        $result += $_
    }
}

# process test list
$prod | % {

    $match = $result -match $_.Id

    # if a test element matches an element in the results
    if ($match) {
        # set the column
        $match[0].PROD='Y'
    }
    else {
        # additional properties
        Add-Member -InputObject $_ -NotePropertyName "DEV" -NotePropertyValue $null
        Add-Member -InputObject $_ -NotePropertyName "TEST" -NotePropertyValue $null
        Add-Member -InputObject $_ -NotePropertyName "PROD" -NotePropertyValue 'Y'
        # add to results
        $result += $_
    }
}

# display the results
$result | format-table -AutoSize

the results of the script:

Id  Name          DEV TEST PROD
--  ----          --- ---- ----
AAA Dev           Y
BBB Dev/Test      Y   Y
CCC Dev/Prod      Y        Y
DDD Dev/Test/Prod Y   Y    Y
EEE Test              Y
FFF Test/Prod         Y    Y
GGG Prod                   Y

I realize that I could refactor the search logic into a single function, but my concern is the search logic itself.

Is there a more-efficient way to perform this logic?

Assuming your objects have a unique property, such as the Id property in your example, you can expand each array in a single pipeline and use Select -Unique to create a combined array, and add your tracking in the same command as such:

$result = $dev,$test,$prod|%{$_}|Select *,@{l='Dev';e={$_.Id -in $dev.id}},@{l='Prod';e={$_.id -in $prod.id}},@{l='Test';e={$_.id -in $test.id}} -Unique

That cuts the time down to a bit under half what you are seeing. I ran Measure-Command 100 times against your code vs my code (using the same test build in each, the only difference being how $result was generated), and your code on my machine took about 16.9ms on average, and my code took 7.4ms on average.

1..100|%{Measure-Command {# create test cases
$a = [PsCustomObject]@{Id='AAA';Name='Dev'}
$b = [PsCustomObject]@{Id='BBB';Name='Dev/Test'}
$c = [PsCustomObject]@{Id='CCC';Name='Dev/Prod'}
$d = [PsCustomObject]@{Id='DDD';Name='Dev/Test/Prod'}
$e = [PsCustomObject]@{Id='EEE';Name='Test'}
$f = [PsCustomObject]@{Id='FFF';Name='Test/Prod'}
$g = [PsCustomObject]@{Id='GGG';Name='Prod'}

# add to arrays
$dev = $a,$b,$c,$d

$test = $b,$d,$e,$f

$prod = $c,$d,$f,$g

$results = $dev,$test,$prod|%{$_}|Select *,@{l='Dev';e={$_.Id -in $dev.id}},@{l='Prod';e={$_.id -in $prod.id}},@{l='Test';e={$_.id -in $test.id}} -Unique

$Results|FT -Auto
}}|Measure-Object -Average -Property TotalMilliseconds|% Average

7.408363


ActiveRecord model for upvotes and downvotes How can I make my model DRY and KISS?

class Rating < ActiveRecord::Base
  attr_accessible :item_type, :item_id, :rating, :voters_up, :voters_down
  serialize :voters_up, Hash
  serialize :voters_down, Hash
  belongs_to :ranks, :polymorphic => true

  def self.up(item, user, iteration = 1)
    @rating = Rating.where(item_type: item.class.name, item_id: item.id).first
    @rating = Rating.create(item_type: item.class.name,
                            item_id: item.id,
                            rating: 0,
                            voters_up: {users:[]}, voters_down: {users:[]}) unless @rating

    changed = nil
    if !@rating.voters_up[:users].include?(user.id) && !@rating.voters_down[:users].include?(user.id)
      if changed.nil?
        @rating.increment(:rating, 1)
        @rating.voters_up[:users] << user.id
        changed = true
      end
    end

    if @rating.voters_up[:users].include?(user.id) && !@rating.voters_down[:users].include?(user.id)
      if changed.nil?
        @rating.decrement(:rating, 1)
        @rating.voters_up[:users].delete user.id
        changed = true
      end
    end

    if @rating.voters_down[:users].include?(user.id) && !@rating.voters_up[:users].include?(user.id)
      if changed.nil?
        @rating.voters_up[:users] << user.id
        @rating.voters_down[:users].delete user.id
        @rating.increment(:rating, 2)
        changed = true
      end
    end

    @rating.save
    item.update_attribute(:rating_value, @rating.rating)
  end

  def self.down(item, user, iteration = 1)
    @rating = Rating.where(item_type: item.class.name, item_id: item.id).first
    @rating = Rating.create(item_type: item.class.name,
                            item_id: item.id,
                            rating: 0,
                            voters_up: {users:[]}, voters_down: {users:[]}) unless @rating

    changed = nil
    if !@rating.voters_down[:users].include?(user.id) && !@rating.voters_up[:users].include?(user.id)
      if changed.nil?
        @rating.decrement(:rating, 1)
        @rating.voters_down[:users] << user.id
        changed = true
      end
    end

    if @rating.voters_down[:users].include?(user.id) && !@rating.voters_up[:users].include?(user.id)
      if changed.nil?
        @rating.increment(:rating, 1)
        @rating.voters_down[:users].delete user.id
        changed = true
      end
    end

    if @rating.voters_up[:users].include?(user.id) && !@rating.voters_down[:users].include?(user.id)
      if changed.nil?
        @rating.voters_down[:users] << user.id
        @rating.voters_up[:users].delete user.id
        @rating.decrement(:rating, 2)
        changed = true
      end
    end

    @rating.save
    item.update_attribute(:rating_value, @rating.rating)
  end

end

Here's a 2-part answer: First, a direct answer to your question, and then an alternative approach to the whole thing.

DRYing the current implementation
Firstly, I'd make sure that every "item" has_one :rating. Right now, you're doing a lot of manual record creation, and Rating has to leave all of its attributes accessible to mass-assignment. It'd also be nicer to simply say question.upvote or answer.upvote directly, instead of "manually" calling the Rating.up method and having to supply both user and item.

That can be done, provided you can store the current user somewhere that's accessible to the models. But let's keep it a little simpler and just go for a syntax like question.upvote(user)

To DRY out the "vote-enabled" models, try this:

# in app/concerns/votable.rb
require 'active_support/concern'

module Votable
  extend ActiveSupport::Concern

  included do
    has_one :rating, :as => :item
  end

  module ClassMethods
    def upvote(user)
      # Build the Rating record if it's missing
      build_rating if rating.nil?
      # Pass the user on
      rating.upvote user
    end

    def downvote(user)
      build_rating if rating.nil?
      rating.downvote user
    end
  end
end

Then, in a record that users can vote on, include that concern:

class Question < ActiveRecord::Base
  include Votable
  # ...
end

class Answer < ActiveRecord::Base
  include Votable
  # ...
end

Now the Question and Answer records will automatically build their own Rating record, if they don't have one already, and then pass the upvote/downvote calls on to that rating record.

As for Rating itself, here's what I'd do (I've changed the attribute names a little, but you can of course keep using yours):

class Rating < ActiveRecord::Base
  belongs_to :item, :polymorphic => true
  # Serialize stuff as straight-up Array
  serialize :upvoters, Array
  serialize :downvoters, Array
  # Create arrays after record initialization, so they're not nil on new records
  after_initialize :initialize_voters
  # Update the "owner's" rating cache after every save
  after_save :update_item

  # Note: No attr_accessible at all; it's not needed.

  # I'll skip the comments, as this should all be very easily readable.

  def upvote(user)
    if upvoted_by?(user)
      decrement :rating
      remove_upvoter(user)
    elsif downvoted_by?(user)
      increment :rating, 2
      add_upvoter user
      remove_downvoter user
    else
      increment :rating
      add_upvoter user
    end
    save
  end

  def downvote(user)
    if downvoted_by?(user)
      increment :rating
      remove_downvoter user
    elsif upvoted_by?(user)
      decrement :rating, 2
      remove_upvoter user
      add_downvoter user
    else
      decrement :rating
      add_downvoter user
    end
    save
  end

  def upvoted_by?(user)
    upvoters.include? user.id
  end

  def downvoted_by?(user)
    downvoters.include? user.id
  end

  private

    def add_upvoter(user)
      upvoters << user.id
    end

    def add_downvoter(user)
      downvoters << user.id
    end

    def remove_upvoter(user)
      upvoters.delete user.id
    end

    def remove_downvoter(user)
      downvoters.delete user.id
    end

    def initialize_voters
      upvoters ||= []
      downvoters ||= []
    end

    def update_item
      if item.present?
        item.update_attribute :rating_value, rating
      end
    end

end

And that's pretty much it. Now you can do Question.find(23).upvote(current_user) or whatever, and it should work out.

Going straight to the database
Alternatively, I'd suggest using "raw" database queries instead of models with serialized attributes and all that. All you really need is a simple table. It doesn't need an id column or a model-layer abstraction. All it needs to is have 4 columns: user_id, vote, item_type, and item_id. You can skip ActiveRecord models and such, since all you need to do is check for a certain user/item combo.

Make user_id, item_id and item_type the primary key (or add a uniqueness constraint) and you can do the entire logic in SQL (at least MySQL). If, for instance, user 23 votes on answer 42, it'd look like this:

INSERT INTO `votes` (`user_id`, `item_id`, `item_type`, `vote`)
VALUES (23, 42, "Answer", X)
ON DUPLICATE KEY UPDATE `vote`=IF(`vote`=X, 0, X);

where X is 1 for an upvote, or -1 for a downvote. That's actually all the upvote/downvote logic in one query.

Right after that, update the total rating for the post:

UPDATE `answers`
SET `rating_value`=(
  SELECT IFNULL(SUM(`vote`), 0)
  FROM `votes`
  WHERE `item_id`=42 AND `item_type`= "Answer"
)
WHERE id=42;

From time to time, you may want to do some clean-up to keep the table a little leaner (i.e. delete rows where the vote is zero, or when an associated item is deleted), but otherwise you should have a pretty robust solution with none of the overhead of Rails and ActiveRecord. And it can all be abstracted into a mixin like the concern above, and you can easily add the upvoted_by?(user)/downvoted_by?(user) methods there too with some simple SQL:

SELECT * FROM `votes` WHERE `vote`<>0 AND `user_id`=23 AND `item_id`=42 AND `item_type`="Answer";


Best practise way to handle array of pairs I am trying to randomly select 4 items out of an array, each item has an index, a name and a price attached to it. Currently I have the following code segment to select 4 at random:

function SelectItems()
{
     $arrayItems = array(
          1 => "Name of Item 1",
          2 => "Name of Item 2",
          3 => "Name of Item 3",
          4 => "Name of Item 4",
     );

     $arrayItemPrices = array(
          1 => "$6.00",
          2 => "$4.00",
          3 => "$5.00",
          4 => "$15.00",
     );

     $randomKeys = array_rand( $arrayItems, 4 );
     foreach( $randomKeys as $key )
     {
          $name = $arrayItems[$key];
          $price = $arrayItemPrices[$key];

          /* Process item... */
     }
}

However, this is not how I would approach it in C++, I would only use a std::map<int, std::pair<std::string, std::string>>, this would map directly in PHP to an array where the value consisted of a pair of strings, however as there is no pair class in PHP I have done it with two arrays. Is this best practise in PHP?

I would work with a two dimensional array, in stead of two separate arrays. You could then shuffle your array to get the items in a random order. Something like this:

$items = [
    ['id' => 1, 'name' => 'item 1', 'price' => '$1.00'],
    ['id' => 2, 'name' => 'item 2', 'price' => '$2.00'],
    ['id' => 3, 'name' => 'item 3', 'price' => '$3.00'],
    ['id' => 4, 'name' => 'item 4', 'price' => '$4.00'],
];

shuffle($items);

foreach($items as $item) {
    echo $item['id'] . ': ' . $item['name'] . ' ' . $item['price'];
    // or whatever you want to do, just trying to demonstrate you
    // won't need those $name and $price variables anymore...
}

I find it makes more sense to keep your item's properties together, in stead of spreading them over different arrays. You know when you pick an element from the items array you will have all it's properties. And to me the code is better readable as well.


Replace enums by inheritance I have a class Call and 3 types of calls. Each type of call has a specific rate. So I have created an CallType enum and I will apply the right rate when making a call.

Here is my code so far:

public partial class Devices : Form
{
    public Devices()
    {
        InitializeComponent();
    }

    private void button1_Click(object sender, EventArgs e)
    {
        var call = new Call(CallType.Regular);
        call.MakeCall();
    }

    private void button2_Click(object sender, EventArgs e)
    {
        var call = new Call(CallType.Important);
        call.MakeCall();
    }

    private void button3_Click(object sender, EventArgs e)
    {
        var call = new Call(CallType.Critical);
        call.MakeCall();
    }
}

public enum CallType
{
    None,
    Regular,
    Important,
    Critical
};

public class Call
{
    private CallType callType;
    const decimal regularRate = 0.25M;
    private decimal ratePerMinute;

    public Call(CallType callType)
    {
        this.callType = callType;
    }

    public void MakeCall()
    {
        switch (callType)
        {
            case CallType.Regular:
                ratePerMinute = regularRate;
                break;
            case CallType.Important:
                ratePerMinute = regularRate * 1.5M;
                break;
            case CallType.Critical:
                ratePerMinute = regularRate * 2.5M;
                break;
        }
    }
}

One of my colleague said that this code is bad: not loosely coupling and not scalable. He suggested to use inheritance instead.

How can I transform this code to respect loose coupling and scalabilty?

public void MakeCall()
{
    switch (callType)
    {
        case CallType.Regular:
            ratePerMinute = regularRate;
            break;
        case CallType.Important:
            ratePerMinute = regularRate * 1.5M;
            break;
        case CallType.Critical:
            ratePerMinute = regularRate * 2.5M;
            break;
    }
}

Seeing this method I would say the same. You cannot easily add new call-types and you cannot easily change their rate because they are all in the same switch. You have to open it every time. The Open-Closed-Priciple is violated. Even worse, the rate is not a part of the call-type but some magic number inside the switch.

What you can do is to create a class for each call-type. This means forget about the enum. Remove everything but the basic stuff from the Call and give it a protected constructor:

public abstract class Call
{
    protected Call(decimal rate) => Rate = rate;

    public decimal Rate { get;}

    public void MakeCall()
    {
        // do something...
    }
}

Use it to derive other calls (I did it for one of them);

public class RegularCall : Call
{
    private const decimal DeafultRate = 0.25m;

    public RegularCall() : base(DeafultRate ) {}
}

Alternatively you can make the property abstract and return the value here instead of passing it via the constructor:

public class RegularCall : Call
{
    public override decimal Rate => 0.25m;
}

And use some other service to create an instance of a call and do some other stuff:

public class Caller
{
    public static void MakeCall<TCall>() where TCall : Call, new
    {
        var call = new TCall();
        call.MakeCall();
    }
}

Each button would the just do this:

private void button1_Click(object sender, EventArgs e)
{
    Caller.MakeCall<RegularCall>();
}

Now you can add as many calls as you like without modifying anything.


Is there a way to optimise my Perlin noise? I just started using Perlin noise a few days ago, and the results look quite good. But it takes over 3 seconds to calculate and draw a 1024x1024 bitmap of said noise. I use an array of 1024x1024 to store int32s ranging from -1 to 1.

Please note that the variable tileSize equals 1024 / resolutieX. This is in case I want to use a lower resolution Perlin noise.

 private void button1_Click(object sender, EventArgs e)
    {
        sw.Start();
        LibNoise.Perlin perlinMap = new LibNoise.Perlin();
        perlinMap.Lacunarity = lacunarity + 0.01d;
        perlinMap.NoiseQuality = LibNoise.NoiseQuality.High;
        perlinMap.OctaveCount = octaveCount;
        perlinMap.Persistence = persistence;
        perlinMap.Frequency = frequency;
        perlinMap.Seed = 1024;

        if (radioButton1.Checked)
            perlinMap.NoiseQuality = LibNoise.NoiseQuality.Low;
        else if (radioButton2.Checked)
            perlinMap.NoiseQuality = LibNoise.NoiseQuality.Standard;
        else if (radioButton3.Checked)
            perlinMap.NoiseQuality = LibNoise.NoiseQuality.High;

        double sample = trackBar6.Value * 10;

        double[,] perlinArray = new double[resolutieX, resolutieY];
        for (int x = 0; x < resolutieX; x++)
        {
            for (int y = 0; y < resolutieY; y++)
            {
                perlinArray[x, y] = perlinMap.GetValue(x / sample, y / sample, 1d);
            }
        }
        draw(perlinArray);
        textBox12.Text = sw.ElapsedMilliseconds.ToString() + "ms";
        sw.Reset();
    }

public void draw(double[,] array)
    {
        Bitmap afbeelding = new Bitmap(1024, 1024);
        Color color;
        int tileSize = 1024 / resolutieY;
        for (int y = 1; y < resolutieY; y++)
        {
            for (int x = 1; x < resolutieX; x++)
            {
                if (array[x, y] <= -0.2)
                    color = Color.DarkBlue;
                if (array[x, y] <= 0)
                    color = Color.DarkBlue;
                else if (array[x, y] <= 0.1)
                    color = Color.Blue;
                else if (array[x, y] <= 0.2)
                    color = Color.Beige;
                else if (array[x, y] <= 0.22)
                    color = Color.LightGreen;
                else if (array[x, y] <= 0.40)
                    color = Color.Green;
                else if (array[x, y] <= 0.75)
                    color = Color.DarkGreen;
                else if (array[x, y] <= 0.8)
                    color = Color.LightSlateGray;
                else if (array[x, y] <= 0.9)
                    color = Color.Gray;
                else if (array[x, y] <= 1)
                    color = Color.DarkSlateGray;
                else
                    color = Color.DarkSlateGray;

                //  color = Color.FromArgb(255);

                for (int i = 0; i < tileSize; i++)
                {
                    for (int j = 0; j < tileSize; j++)
                    {
                        afbeelding.SetPixel(((x - 1) * tileSize) + i, ((y - 1) * tileSize) + j, color);
                    }
                }

            }
        }

        pictureBox1.Image = afbeelding;
    }

Should I start using Simplex noise? Or am I just missing something obvious here?

Your check

if (array[x, y] <= -0.2)
    color = Color.DarkBlue;

serves no purpose, if your number is indeed lessthan or equal -.2 then it will also hit the next check if it is lessthan or equal to 0.

A prettier way to write if-else blocks like yours would have been to use the ternary operator...

color = array[x, y] <= 0.0  ? Color.DarkBlue       :
        array[x, y] <= 0.1  ? Color.Blue           :
        array[x, y] <= 0.2  ? Color.Beige          :
        array[x, y] <= 0.22 ? Color.LightGreen     :
        array[x, y] <= 0.40 ? Color.Green          :
        array[x, y] <= 0.75 ? Color.DarkGreen      :
        array[x, y] <= 0.8  ? Color.LightSlateGray :
        array[x, y] <= 0.9  ? Color.Gray           :
        array[x, y] <= 1.0  ? Color.DarkSlateGray  :
                              Color.DarkSlateGray  ;

Your method draw is a public method, and should be PascalCase not camelCase

Your worst case scenario for the if-else chain is that you have to hit every single one of those IFs.
If you structured those in binary search design, ie. start from the middle and eliminate half of the spectrum at a time and work your way out, worst case scenario is that you have to hit half of your IFs. The code for this may look ugly, but it would help.

The most inefficient thing about your code though, and the reason it is taking so long to process is most likely your quadruple nested for-loop

for (int y = 1; y < resolutieY; y++)
{
    for (int x = 1; x < resolutieX; x++)
    {
        //color calculations

        for (int i = 0; i < tileSize; i++)
        {
            for (int j = 0; j < tileSize; j++)
            {
                afbeelding.SetPixel(((x - 1) * tileSize) + i, ((y - 1) * tileSize) + j, color);
            }
        }
    }
}

I would recommend instead, to create the 2d array of colors in that first double for-loop, then, loop over that Color[,]  and set the pixels.

If possible, instead just create a max resolution version of your image, and rely on the PictureBox's ability to scale the image for you.


Compile-time-fixed templated integer range This is a follow-up of an old question by @LokiAstari, modified for the current community challenge. The idea is to provide a compile-time integer range. I applied all the modifications that I proposed to Loki at the time and tried to write a class as close as possible to the standard library class std::integer_sequence:

This is a [begin, end) range instead of a [begin, end] one.
The range can be ascending or descending.
The class is templated so that it is possible to choose the integer type to use.
In order to match the standard library utilities, I also provide the template index_range which is an alias of integer_range for the type std::size_t.

Here is the implementation:

#include <cstddef>
#include <tuple>
#include <type_traits>
#include <utility>

namespace details
{
    ////////////////////////////////////////////////////////////
    // implementation details

    template<typename Int, typename, Int Begin>
    struct increasing_integer_range;

    template<typename Int, Int... N, Int Begin>
    struct increasing_integer_range<Int, std::integer_sequence<Int, N...>, Begin>:
        std::integer_sequence<Int, N+Begin...>
    {};

    template<typename Int, typename, Int Begin>
    struct decreasing_integer_range;

    template<typename Int, Int... N, Int Begin>
    struct decreasing_integer_range<Int, std::integer_sequence<Int, N...>, Begin>:
        std::integer_sequence<Int, Begin-N...>
    {};
}

////////////////////////////////////////////////////////////
// integer_range

template<typename Int, Int Begin, Int End,
         bool Increasing=(Begin<End)>
struct integer_range;

template<typename Int, Int Begin, Int End>
struct integer_range<Int, Begin, End, true>:
    details::increasing_integer_range<
        Int,
        std::make_integer_sequence<Int, End-Begin>,
        Begin>
{};

template<typename Int, Int Begin, Int End>
struct integer_range<Int, Begin, End, false>:
    details::decreasing_integer_range<
        Int,
        std::make_integer_sequence<Int, Begin-End>,
        Begin>
{};

template<std::size_t Begin, std::size_t End>
using index_range = integer_range<std::size_t, Begin, End>;

I don't ask for a specific kind of review. If you see anything that can be improved, do not hesitate.

And as a bonus, take this test cases. Since integer_range uses some heavy template wizardry, I had troubles coming with decent test cases that are easy enough to read, write and understand. They are not really beautiful, but they work as expected; that may help you if you want to try modifications:

template<std::size_t N>
void test(std::integer_sequence<int>)
{
    static_assert(N == 0, "");
}

template<std::size_t N>
void test(std::integer_sequence<int, 0, 1, 2, 3, 4>)
{
    static_assert(N == 1, "");
}

template<std::size_t N>
void test(std::integer_sequence<int, 5, 4, 3, 2, 1>)
{
    static_assert(N == 2, "");
}

template<std::size_t N>
void test(std::integer_sequence<int, -3, -2, -1, 0, 1>)
{
    static_assert(N == 3, "");
}

template<std::size_t N>
void test(std::integer_sequence<int, -1, -2, -3>)
{
    static_assert(N == 4, "");
}

template<std::size_t N>
void test(std::integer_sequence<int, 3, 2, 1, 0, -1, -2, -3, -4, -5, -6, -7>)
{
    static_assert(N == 5, "");
}

int main()
{
    using namespace polder;

    test<0>(integer_range<int, 8, 8>());
    test<1>(integer_range<int, 0, 5>());
    test<2>(integer_range<int, 5, 0>());
    test<3>(integer_range<int, -3, 2>());
    test<4>(integer_range<int, -1, -4>());
    test<5>(integer_range<int, 3, -8>());
}

Note: if you're interested, I wrote a follow-up to this question which incorporates most of the suggestions from the accepted answer and adds the possibility to specify a Step parameter.

IMO, your test cases would be vastly more readable if you wrote them simply in terms of std::is_same. If you were going to write a lot of test cases, you could wrap all that boilerplate up in a macro such as

#define TEST(type, start, end, ...) \
    static_assert(std::is_same< \
        polder::integer_range<type, start, end>, \
        std::integer_sequence<type ,##__VA_ARGS__ > >::value)

which reduces your test cases to the tiny amount of code they actually are.

TEST(int, 8,8);
TEST(int, 0,5, 0,1,2,3,4);
TEST(int, 5,0, 5,4,3,2,1);
TEST(int, -3,2, -3,-2,-1,0,1);
TEST(int, -1,-4, -1,-2,-3);
TEST(int, 3,-8, 3,2,1,0,-1,-2,-3,-4,-5,-6,-7);

This shorter formulation incidentally makes it more obvious that you didn't test any corner cases or types other than int — which is why I'm a strong proponent of "shorter code is better code." We've now made the code short enough that we can see where the bugs are.

Okay, let's move on to looking at the actual code. It actually looks very clean and simple. Admittedly I did think at first that you were doing the bad O(n) approach where integer_range<N> inherits from integer_range<N-1>; it took me a moment to see that you were in fact doing the right thing. I see only two issues with

template<typename Int, Int... N, Int Begin>
struct increasing_integer_range<Int, std::integer_sequence<Int, N...>, Begin>:
    std::integer_sequence<Int, N+Begin...>
{};

The first problem is that our new test cases don't pass, because your increasing_integer_range is not actually a synonym for std::integer_sequence; instead it's merely derived from std::integer_sequence. This is good enough for the average codebase, but for a library implementation we'd want to make sure that the two types are actually equal. Fortunately, C++11 template typedefs make this easy. Rewrite it as

namespace details
{
    template<typename Int, typename, Int Begin, bool Increasing>
    struct integer_range_impl;

    template<typename Int, Int... N, Int Begin>
    struct integer_range_impl<Int, std::integer_sequence<Int, N...>, Begin, true> {
        using type = std::integer_sequence<Int, N+Begin...>;
    };

    template<typename Int, Int... N, Int Begin>
    struct integer_range_impl<Int, std::integer_sequence<Int, N...>, Begin, false> {
        using type = std::integer_sequence<Int, Begin-N...>;
    };
}

template<typename Int, Int Begin, Int End>
using integer_range = typename details::integer_range_impl<
    Int,
    std::make_integer_sequence<Int, (Begin<End) ? End-Begin : Begin-End>,
    Begin,
    (Begin<End) >::type;

template<std::size_t Begin, std::size_t End>
using index_range = integer_range<std::size_t, Begin, End>;

The second problem is that this implementation does not work for some obvious corner cases, such as integer_range<int, INT_MIN, INT_MAX>, because in general the result of End-Begin is not guaranteed to be representable in Int.

Fixing the second problem is left as an exercise for the reader, because I believe it's pretty darn hard. I think you might have to abandon the (really clever) idea of using std::integer_sequence to do your heavy lifting, and resign yourself to reimplementing integer_sequence something like Xeo did here except harder.

EDIT TO ADD a third "problem" I just thought of: The name integer_range could be considered misleading. If I were proposing this for the standard library, I'd name it make_integer_range, by analogy with make_integer_sequence.


find if one string is a valid anagram of another , in scala Question is taken from leet code.

Problem

Given two strings s and t , write a function to determine if t is an anagram of s.

Examples

Input: s = "anagram", t = "nagaram"
Output: true

Input: s = "rat", t = "car"
Output: false

Here is my implementation in scala

import scala.collection.immutable.HashMap

object ValidAnagram extends App {
  def validAnagram(s1: String, s2: String): Boolean = {
    if (s1 == s2) true
    else if (s1.length != s2.length) false
    else {
      val lookupTable = s1.foldLeft(HashMap[Char,Int]()) ((m,c) => m ++ HashMap(c -> (if (m contains c) m(c) + 1 else 1)))
      println(lookupTable)
      (s2.foldLeft(lookupTable) {(m,c) =>
        if (m contains c) {
          val count = m(c)
          if (count > 1) {m + (c -> (count-1))} else m - c
        } else m  // we can return here if not functional
      }).isEmpty
    }
  }
    println(validAnagram(args(0), args(1)))
  }

Short circuit!

Check the lengths first. In the case that they aren't the same, you don't need to incur a linear pass for an equality check.

Don't check the hashmap for an entry!

It's good that you know m(c) can throw if there's a missing key; luckily,
 you can use Map#getOrElse and provide a default -- in this case, it makes sense to provide a 0 default since you haven't seen the character.

Compare the counts from each string directly!

I find the subtracting of the counts to be very confusing. You'll need to remove keys from the counter if any of the values go to 0, which lowers the signal to noise ratio of the code. It's better to just build a second counter.

If you decide to build two counters and compare them, you'll need to do the same fold logic twice. This is really messy to write, so you won't want to do this twice. You may want to pimp out the String class. You can do this in the following way:

implicit class Counter(s: String) {
  def countCharacters: Map[Char, Int] =
    s.foldLeft(Map.empty[Char, Int])({ case (acc, c) =>
      acc + (c -> (acc.getOrElse(c, 0) + 1))
    })
}

You can see it in action here:

scala> "hello".countCharacters
res0: Map[Char,Int] = Map(h -> 1, e -> 1, l -> 2, o -> 1)

So now you can compare the counts with a clean, non-repeated API.

Pulling this all together, you can write:

object ValidAnagram extends App {

  // when I see a string, implicitly construct a `Counter` instance
  implicit class Counter(s: String) {
    // this will be "added" to the String API via an implicit class construction
    def countCharacters: Map[Char, Int] =
      s.foldLeft(Map.empty[Char, Int])({ case (acc, c) =>
        acc + (c -> (acc.getOrElse(c, 0) + 1))
      })
  }

  def validAnagram(s1: String, s2: String): Boolean =
    if (s1.length != s2.length) false
    else if (s1 == s2) true
    else s1.countCharacters == s2.countCharacters

  println(validAnagram(args(0), args(1)))
}


Streamline code for checking if a directory exists and saving a file I hate this code.  What is the slickest way to write the following:

MyFile = f;
SaveFolder = Server.MapPath("\\") + "returns\\";
if(!System.IO.Directory.Exists(SaveFolder) )
{
    System.IO.Directory.CreateDirectory(SaveFolder);
}
MyFile.SaveAs(SaveFolder + "2011" + "000-00-0000" + ".xlsx");

Since CreateDirectory does nothing if a directory already exists, you can do this:

MyFile = f;
SaveFolder = Path.Combine(Server.MapPath("\\"), "returns");
System.IO.Directory.CreateDirectory(SaveFolder);
MyFile.SaveAs(SaveFolder, "2011000-00-0000.xlsx");


Error checking in function that returns reference to static variable This is a function I just wrote with the goal to only load the desired information once, and if an error occurs, save it for later so that it can always be reported to a calling function.

It does what I want but seems quite clumsy.

std::wstring const& GetMuiFilePath()
{
    static DWORD error = NO_ERROR;
    static std::wstring muiPath;
    static bool hasTriedLoadingPath = false;
    if (!hasTriedLoadingPath) {
        try {
             muiPath = Internal::zGetSpecialFolder(zCsidlSystem)
                     + L"\\" + Internal::zGetSystemLocale()
                     + L"\\" + L"wbadmin.exe.mui";
        }
        catch (AutoWinError const& e) {
            error = e.m_error;
        }
        hasTriedLoadingPath = true;
    }

    if (muiPath.empty()) {
        throw AutoWinError(error);
    }

    return muiPath;
}

The only things I would change are:

    // Move this boolean to the top.
    // So if you throw an exception that is not caught
    // you then stil know that you have tried.
    hasTriedLoadingPath = true;

    try {
         // No change.
         muiPath = Internal::zGetSpecialFolder(zCsidlSystem)
                 + L"\\" + Internal::zGetSystemLocale()
                 + L"\\" + L"wbadmin.exe.mui";
    }
    catch (AutoWinError const& e) {
        error = e.m_error;

        // Curious why you did not re-do the throw here?
        throw;  // Note. Not  `throw e;`
                // Rethrows the current exception.
    }


Shopping cart application in Spring MVC I am creating a shopping cart application to learn Spring MVC. I want to display a list of all categories and subcategories in more than one page. i.e; Home Page, Products page, and Category page.

In the Home Page controller I am retrieving the list of categories, subcategories and featured products and passing them to the home page:

@Controller
public class CatalogController {

    @Autowired
    private CategoryConfigService categoryConfigurationService;
    @Autowired
    private ProductConfigService productConfigurationService;
    private static final Logger logger = LoggerFactory
            .getLogger(CatalogController.class);

    /**
     * Catalog Controller method which retrieves the information required in the
     * application home page(Categories,SubCategories)
     *
     * @return Home Page View
     */
    @RequestMapping(value = "/home", method = RequestMethod.GET)
    public String returnHomePage(Model model) {
        logger.info("Processing information for home page");
        List<Category> categoriesList = categoryConfigurationService
                .getAllCategories();
        Map<Category, List<SubCategory>> categoryMap = new HashMap<Category, List<SubCategory>>();
        for (Category category : categoriesList) {
            List<SubCategory> subCategoryList = categoryConfigurationService
                    .getAllSubCategoriesByCategoryId(category.getId());
            categoryMap.put(category, subCategoryList);
        }
        model.addAttribute("categoryMap", categoryMap);
        model.addAttribute("featProd",
                productConfigurationService.getFeaturedProducts());

        return "home";
    }

}

What is the best way to store the object categoryMap in Spring to access it in the header page?

The header page is common for all the JSPs in my web application. So I have used the application context scope to store and retrieve the categoryMap.

What are the drawbacks of this approach? And are there any better ways of doing it?

I want to display the header page as shown in this image:

I think you need different approach to this. It's good practice when you don't have much of business logic in your controller. Maybe you should try creating a different class that will fill pages with content, based on something, like a template.

Now answering to your question:

You can create method with annotation @PostConstruct, which Spring will execute after done with his setup. In this method initialize field that you need, or in your case set application scope attribute with data that you need.

Also, I don't know if you knew it, you can create jpsf - jsp parts, that you can include in any of your jsp. As an example, you can create part with menu, CSS link or other parts, that share all pages, and just include it in your pages - makes jps look much more cleaner.

Here how u can bind objects to any response of your controller:

@ModelAttribute(value = "foo")
public String addAttribute(){
return "string";
}

Or:

@ModelAttribute()
public void addAttribute(Model model){
   model.addAttribute("roo", "one");
   model.addAttribute("loo", "two");
   model.addAttribute("moo", "some");
}

this how you bind for every response of your application:

@ControllerAdvice
public final class GlobalController {
    @ModelAttribute
    public void addAttributes(Model model) {
       model.addAttribute("msg", "Welcome to My Country");
    }
}


Retrieve Data from TOC I aim to convert the TOC of The Python Language Reference — Python 3.6.3 documentation to a structured data with the following steps:

Copy contents to a plr.md file:

In [1]: with open('plr.md') as file:
   ...:     content = file.read()
In [2]: content
Out[2]: '\n\n- \\1. Introduction\n  - [1.1. Alternate Implementations]
(https://docs.python.org/3.6/reference/introduction.html#alternate-implementations)\n  - [1.2. Notation](https://docs.python.org/3.6/reference/introduction.html#notation)\n- \\2. Lexical analysis\n  - [2.1. Line structure]
(https://docs.python.org/3.6/reference/lexical_analysis.html#line-structure)\n  - [2.2. Other tokens](https://docs.python.org/3.6/reference/lexical_analysis.html#other-tokens)\n

Get chapters:

In [47]: chapters = content.split('\n- \\')
    ...: #subtract the unqualified part
    ...: chapters = chapters[1:]
In [50]: chapters[0]
Out[50]: '1. Introduction\n  - [1.1. Alternate Implementations](https://docs.python.org/3.6/reference/introduction.html#alternate-implementations)
\n  - [1.2. Notation](https://docs.python.org/3.6/reference/introduction.html#notation)'

Separate chapter name and section name in each chapters:

chapter_details = chapters[0].split('\n  -')
sections = chapter_details[1:]
chapter = chapter_details[0]
In [54]: chapter
Out[54]: '1. Introduction'
In [55]: sections
Out[55]:
[' [1.1. Alternate Implementations](https://docs.python.org/3.6/reference/introduction.html#alternate-implementations)',
 ' [1.2. Notation](https://docs.python.org/3.6/reference/introduction.html#notation)']

Convert section:

def convert_section(s):
    start = s.index('[') + 1
    end = s.index(']')
    return s[start:end]
In [57]: print(convert_section(' [1.1. Alternate Implementations](https://docs.python.org/3.6/reference/i
    ...: ntroduction.html#alternate-implementations)'))
1.1. Alternate Implementations

sections = map(convert_section, sections)
sections = list(sections)

Create a dict:

key = chapter
{key:sections}
 {'1. Introduction':['1.1. Alternate Implementations', '1.2. Notation']}

Encapsulate code in a class and get the result:

class TOC:
    def __init__(self, filename):
        self.filename = filename

    def read(self, filename):
        with open (filename) as file:
            content = file.read()
        return content

    def convert_section(self, s):
        start = s.index('[') + 1
        end = s.index(']')
        return s[start:end]

    def get_chapters(self, filename):
        content = self.read(filename)
        chapters = content.split('\n- \\')
        #subtract the unqualified part
        chapters = chapters[1:]
        return chapters

    def create_chapter_dict(self, chapter):
        chapter_details = chapter.split('\n  -')
        sections = chapter_details[1:]
        key = chapter_details[0]
        value = map(self.convert_section, sections)
        return {key: list(value)}

    def get_chapters_dict(self):
        chapters = self.get_chapters(self.filename)
        chapters_dict = {}
        for chapter in chapters:
            chapter_dict = self.create_chapter_dict(chapter)
            chapters_dict.update(chapter_dict)
        return chapters_dict

Run and get the result:

In [89]: TOC('plr.md').get_chapters_dict()
Out[89]:
{'1. Introduction': ['1.1. Alternate Implementations', '1.2. Notation'],
 '2. Lexical analysis': ['2.1. Line structure',
  '2.2. Other tokens',
  '2.3. Identifiers and keywords',
  '2.4. Literals',
  '2.5. Operators',
  '2.6. Delimiters'],
 '3. Data model': ['3.1. Objects, values and types',
  '3.2. The standard type hierarchy',
  '3.3. Special method names',
  '3.4. Coroutines'],

This solution is a bit too much for a daily common operation. Is there a standard or easy method for such a task?

I think you are overcomplicating the problem. I would probably go with a proper Markdown parser (like mistune), or parse the generated HTML instead. Here is how I would do it using BeautifulSoup parser:

from pprint import pprint

from bs4 import BeautifulSoup
import requests

response = requests.get("https://docs.python.org/3.6/reference/index.html")

soup = BeautifulSoup(response.content, "html.parser")
contents = soup.select_one("#the-python-language-reference ul")

pprint({
    li.a.get_text(): [li.a.get_text() for li in li("li")]
    for li in contents.find_all("li", recursive=False)
})

Prints:

{'1. Introduction': ['1.1. Alternate Implementations', '1.2. Notation'],
 '10. Full Grammar specification': [],
 '2. Lexical analysis': ['2.1. Line structure',
                         '2.2. Other tokens',
                         '2.3. Identifiers and keywords',
                         '2.4. Literals',
                         '2.5. Operators',
                         '2.6. Delimiters'],
 '3. Data model': ['3.1. Objects, values and types',
                   '3.2. The standard type hierarchy',
                   '3.3. Special method names',
                   '3.4. Coroutines'],
 '4. Execution model': ['4.1. Structure of a program',
                        '4.2. Naming and binding',
                        '4.3. Exceptions'],
 '5. The import system': ['5.1. importlib',
                          '5.2. Packages',
                          '5.3. Searching',
                          '5.4. Loading',
                          '5.5. The Path Based Finder',
                          '5.6. Replacing the standard import system',
                          '5.7. Special considerations for __main__',
                          '5.8. Open issues',
                          '5.9. References'],
 '6. Expressions': ['6.1. Arithmetic conversions',
                    '6.2. Atoms',
                    '6.3. Primaries',
                    '6.4. Await expression',
                    '6.5. The power operator',
                    '6.6. Unary arithmetic and bitwise operations',
                    '6.7. Binary arithmetic operations',
                    '6.8. Shifting operations',
                    '6.9. Binary bitwise operations',
                    '6.10. Comparisons',
                    '6.11. Boolean operations',
                    '6.12. Conditional expressions',
                    '6.13. Lambdas',
                    '6.14. Expression lists',
                    '6.15. Evaluation order',
                    '6.16. Operator precedence'],
 '7. Simple statements': ['7.1. Expression statements',
                          '7.2. Assignment statements',
                          '7.3. The assert statement',
                          '7.4. The pass statement',
                          '7.5. The del statement',
                          '7.6. The return statement',
                          '7.7. The yield statement',
                          '7.8. The raise statement',
                          '7.9. The break statement',
                          '7.10. The continue statement',
                          '7.11. The import statement',
                          '7.12. The global statement',
                          '7.13. The nonlocal statement'],
 '8. Compound statements': ['8.1. The if statement',
                            '8.2. The while statement',
                            '8.3. The for statement',
                            '8.4. The try statement',
                            '8.5. The with statement',
                            '8.6. Function definitions',
                            '8.7. Class definitions',
                            '8.8. Coroutines'],
 '9. Top-level components': ['9.1. Complete Python programs',
                             '9.2. File input',
                             '9.3. Interactive input',
                             '9.4. Expression input']}

Note that this would work for the limited depth of the nested lists. If it would be needed, you can generalize parsing the nested lists using something like this dictify() function.