Showing activity indicator (loading image) while processing in background I have a login screen on my iOS app. When I click login button I want to make sure that user should be displayed a loading image in center of the screen.

I found lots of different methods online but I am after a simplest method. I found a way to display activity Indicator with minimum code:

First of all, import your UI Kit:

import UIKit;

Then you have to initiate activity indicator:

let activityIndicator:UIActivityIndicatorView = UIActivityIndicatorView();

Then just copy paste this functions below which are self explainatory and call them whereever you need them:

func startLoading(){
    activityIndicator.center = self.view.center;
    activityIndicator.hidesWhenStopped = true;
    activityIndicator.activityIndicatorViewStyle = UIActivityIndicatorViewStyle.gray;
    view.addSubview(activityIndicator);

    activityIndicator.startAnimating();
    UIApplication.shared.beginIgnoringInteractionEvents();

}

func stopLoading(){

    activityIndicator.stopAnimating();
    UIApplication.shared.endIgnoringInteractionEvents();

}

Although, I am still not convinced that it's the best way to do it. Could it still be reduced?

Here is how my screen looks with the code:

There are a few things I would recommend to do.

Usually, instead of disabling the interaction events there is an extra layer on top of all views that contains the activity indicator. So, instead of adding the activity indicator directly into the viewController's view, add it in another view first with clear background color and with 0.4 alpha.
Right now the startLoading() function also sets up the activity indicator, what i would rather separate into a different function, like setupLoading()
If you want to reuse the solution on other viewControllers, create a class or a protocol at least ecapsulating the logic. Also, it is better to present it on the UIApplication's keyWindow property, than just simply adding it to viewContollers view's.
Prepare for the stopLoading() and startLoading() functions to be called from a background thread.

Finally, there are plenty of very good solutions already on the interwebs, that you could integrate into your project, like SwiftSpinner.

If you need further help, code examples, let me know and I will edit my answer.


Convert *.VCF contact file for cellphone import My new mobile phone is capable of importing contact information from a renamed *.VCF file stored on an SD memory card (https://www.quora.com/How-do-I-sync-contacts-on-Nokia-130). Annoyingly, it does not work very well for contacts with multiple phone numbers or anything not tagged as TEL: or TEL;TYPE=CELL:.

The following script takes an *.vcf file, parses the stored contact information as needed - first name, last name and phone numbers - and outputs the information in a way that the phone imports it correctly. Therefore, each contact may only contain one number. For multiple numbers, a contact will be duplicated for each phone number. The resulting string is simply printet out.

#!/usr/bin/python

import re

class Contact(object):
    """
    Contains the contact infromation,
    including a list of phone numbers.

    Arguments:
        object {[type]} -- [description]
    """

    def __init__(self):
        self.FirstName = ""
        self.LastName = ""
        self.Phonenumbers = []

def extract_number(line):
    """
    Extracts the phone number from a vCard-file line,
    by removing everything but numbers and '+'

    Arguments:
        line {string} -- the line to extract the phone number from

    Returns:
        string -- the phone number
    """

    line = line[line.index(":")+1:].rstrip()
    line = re.sub('[^0-9+]', '', line)
    return line

def generate_vcard_contact_string(contact):
    """
    Generates the vCard string for this contact.
    Will generate a sperate vCard for each phone number of the contact.

    Arguments:
        contact {Contact} -- the contact to generate the vCard string from

    Returns:
        string -- the generated vCard string
    """

    base = f"BEGIN:VCARD\n"
    base += f"N:{contact.LastName};{contact.FirstName}\n"
    base += f"TEL:{{phone_number}}\n"  # use '{phone_number}' as a placeholder.
    # Will be replaced by actual number.
    base += f"END:VCARD\n"

    result = ""
    for number in contact.Phonenumbers:
        # Add the base string for this contact,
        # replace {phone_number} with actual number
        result += base.replace("{phone_number}", number)

    return result

contacts = []

with open(r'contacts.vcf') as f:
    current_contact = Contact()
    for line in f:
        # Some lines are build like "item1.TEL;...",
        # remove "item1.", "item2.", to ease parsing
        if line.startswith("item"):
            line = line[line.index(".")+1:]

        if "BEGIN:VCARD" in line:
            # Marks the start of a vCard,
            # create a blank contact to work with
            current_contact = Contact()

        elif line.startswith("N:"):
            # Line contains a name in the format N:LastName;FirstName;...;...;
            # Only LastName and FirstName will be used
            line = line.replace("N:", "")  # remove "N:" from line
            chunks = line.split(';')
            current_contact.LastName = chunks[0].strip()
            current_contact.FirstName = chunks[1].strip()

        elif line.startswith("TEL"):
            # Line contains a phone number
            # phone number type may be specified by "TYPE=...",
            # currently, TYPE will ne omitted
            # One contact may contain multiple phone numbers.
            number = extract_number(line)
            current_contact.Phonenumbers.append(number)

        elif "END:VCARD" in line:
            # Marks the end of a vCard,
            # append contact to list
            contacts.append(current_contact)

result = ""
for contact in contacts:
    # Generate a string containing a vCard for each of the contacts phone
    # numbers, append those string to create one big text containing everything
    result += generate_vcard_contact_string(contact)

print(result)

Your function generate_vcard_contact_string obviously belongs to the Contact class. It even takes a Contact instance as the first argument.

I would actually elevate this to be the magic __str__ method, which is for example called when you do print(contact):

class Contact(object):
    """
    Contains the contact information,
    including a list of phone numbers.

    Arguments:
        object {[type]} -- [description]
    """

    def __init__(self):
        self.FirstName = ""
        self.LastName = ""
        self.Phonenumbers = []

    def __str__(self):
        """
        Generates the vCard string for this contact.
        Will generate a separate vCard for each phone number of the contact.

        Returns:
            string -- the generated vCard string
        """

        base = "\n".join(["BEGIN:VCARD",
                          f"N:{self.LastName};{self.FirstName}",
                          "TEL:{}",
                          "END:VCARD"])

        return "".join(base.format(number) for number in self.Phonenumbers)

You should also place your calling code under a if __name__ == "__main__": guard to allow importing from other scripts without executing the code. It is also complex enough to be encapsulated in its own function, IMO:

def parse_contacts(file_name):
    with open(file_name) as f:
        contact = None
        for line in f:
            # Some lines are build like "item1.TEL;...",
            # remove "item1.", "item2.", to ease parsing
            if line.startswith("item"):
                line = line[line.index(".")+1:]

            if "BEGIN:VCARD" in line:
                # Marks the start of a vCard,
                # create a blank contact to work with
                contact = Contact()

            elif line.startswith("N:"):
                # Line contains a name in the format N:LastName;FirstName;...;...;
                # Only LastName and FirstName will be used
                chunks = line[2:].split(';')
                contact.LastName = chunks[0].strip()
                contact.FirstName = chunks[1].strip()

            elif line.startswith("TEL:"):
                # Line contains a phone number
                # phone number type may be specified by "TYPE=...",
                # currently, TYPE will ne omitted
                # One contact may contain multiple phone numbers.
                number = extract_number(line)
                contact.Phonenumbers.append(number)

            elif "END:VCARD" in line:
                # Marks the end of a vCard,
                yield contact

def main():
    for contact in parse_contacts('contacts.vcf'):
        print(contact)

if __name__ == "__main__":
    main()

Your extract_number function can also be simplified using a single regex:

def extract_number(line):
    """
    Extracts the phone number from a vCard-file line,
    by removing everything but numbers and '+'

    Arguments:
        line {string} -- the line to extract the phone number from

    Returns:
        string -- the phone number
    """
    return re.match(r'TEL:(\+?\d*)', line).groups()[0]


Interface for obtaining the bounding box for a collection of elements How could I improve this code?
I used a position interface to avoid code duplication, can it be done better?
Can I reduce the amount of code somehow?

interface PositionInterface
{
    double getPosition(PageAreaInterface pArea);
}

private double getMinPosition(Collection<PageAreaInterface> pAreas, PositionInterface pPosition)
{
    double lMinPosition = Double.MAX_VALUE;
    for (PageAreaInterface lArea : pAreas)
    {
        lMinPosition = Math.min(lMinPosition, pPosition.getPosition(lArea));
    }
    return lMinPosition;
}

private double getTop(Collection<PageAreaInterface> pAreas)
{
    return getMinPosition(
                pAreas,
                new PositionInterface()
                {
                    @Override
                    public double getPosition(PageAreaInterface pArea)
                    {
                        return pArea.getBoundingBox().getTop();
                    }
                }
    );
}

private double getLeft(Collection<PageAreaInterface> pAreas)
{
    return getMinPosition(
                pAreas,
                new PositionInterface()
                {
                    @Override
                    public double getPosition(PageAreaInterface pArea)
                    {
                        return pArea.getBoundingBox().getLeft();
                    }
                }
    );
}

private double getMaxPosition(Collection<PageAreaInterface> pAreas, PositionInterface pPosition)
{
    double lMaxPosition = Double.MIN_VALUE;
    for (PageAreaInterface lArea : pAreas)
    {
        lMaxPosition = Math.max(lMaxPosition, pPosition.getPosition(lArea));
    }
    return lMaxPosition;
}

private double getBottom(Collection<PageAreaInterface> pAreas)
{
    return getMaxPosition(
                pAreas,
                new PositionInterface()
                {
                    @Override
                    public double getPosition(PageAreaInterface pArea)
                    {
                        return pArea.getBoundingBox().getBottom();
                    }
                }
    );
}

private double getRight(Collection<PageAreaInterface> pAreas)
{
    return getMaxPosition(
                pAreas,
                new PositionInterface()
                {
                    @Override
                    public double getPosition(PageAreaInterface pArea)
                    {
                        return pArea.getBoundingBox().getRight();
                    }
                }
    );
}

Another alternative.

Nutshell

Don't put type into name (PageAreaInterface etc.)
Lose the Hungarian; appropriately-short methods remove its utility.
Create an enum and method to get specific coordinates.
Create getMinimum and getMaximum methods in a PageAreaCollection taking an enum.

Justifications (working backwards)

PageAreaCollection

Static utility methods strike me as un-OO, particularly when there are other options.

Create a type, with type-appropriate methods: code shrinks, and reads better:

min = PageAreaUtils.findMinimum(pageAreas, TOP); // Contrast with...
min = pageAreas.findMinimum(TOP);

Shorter is good, but which reads nicer? Which is more communicative?

With static imports, you're still left with:

min = findMinimum(pageAreas, TOP);    // Minimum pageAreas?! No...
min = findMinimumTop(pageAreas);      // Doesn't read right.
min = findMinimumTopIn(pageAreas);    // Better?
min = findMinimum(TOP).in(pageAreas); // Better?

IMO the amount of extra work/code to remove the method from its rightful place (a method of a pageArea collection) isn't worth the effort.

Enum in bounding box

The enum and utility method could live in the collection, too, if the bounding box class isn't yours to finagle.

// In bounding box, collection, or standalone.
public enum POSITION { TOP, LEFT, BOTTOM, RIGHT }

// In bounding box or collection.
public double getPosition(POSITION pos) {
    switch (pos) {
    case TOP:    return getTop();
    case LEFT:   return getLeft();
    case BOTTOM: return getBottom();
    case RIGHT:  return getRight();
    }
    throw new RuntimeException("Bad position provided: " + pos);
}

Min/max position locators in PageAreaCollection class

This is essentially the same as the previous suggestion to use Collections.min/max, but I'd still wrap it all up so the mainline code doesn't have to see how it's implemented. This way or that, it's significantly cleaner, with an appropriately-named Comparator.

Collection<PageArea> pageAreas;

public double getMinimumPosition(MyRect.POSITION pos) {
    double min = Double.MAX_VALUE;
    for (PageArea area : pageAreas) {
        min = Math.min(min, area.getBoundingBox().getPosition(pos));
    }
    return min;
}

public double getMaximumPosition(MyRect.POSITION pos) {
    double max = Double.MIN_VALUE;
    for (PageArea area : pageAreas) {
        max = Math.min(max, area.getBoundingBox().getPosition(pos));
    }
    return max;
}

Essentially the same if getPosition() needs to be in the collection. My quibble with having the method in the bounding box is that it makes getting the position a bit bulky; I'd actually prefer this:

area.getBoundingBox(pos) // or area.getBoundingBoxPosition(pos)?

Hungarian

Ew. A method that's a dozen lines long doesn't need differentiation between parameters and locals; it's obvious. At most I could see naming member variables, but even that... Meh.

Interface naming

A PageAreaInterface is just a PageArea. An implementation may deserve a special name, but it'd be a "special" PageArea in that it implements specific functionality likely worth naming. It's the same reason we don't name things IWhatever anymore. The interface is the Whatever, implementations provide specificity and deserve naming.


Continuous Shuffler with "linked loop" As an academic exercise, I decided to implement a continuous shuffler, as seen on casino blackjack tables.  It operates by storing a "linked loop" (a linked list with the last element linked to the first) and walking a random number of steps around the loop when an item is removed.

public abstract class BaseShuffler<T>
{
    ///<summary>Returns a random int in the range [0,max)</summary>
    protected abstract int GetNextRandom(int max);
    private Node _current = null;
    private int _count = 0;

    public int Count
    {
        get { return _count; }
    }

    public void Add(T value)
    {
        var newNode = new Node(value);

        if (_count <= 0)
        {
            //form the loop
            _current = newNode;
            _current.Next = _current;
        }
        else
        {
            //insert into loop after current node
            var next = _current.Next;
            _current.Next = newNode;
            newNode.Next = next;
        }

        _count++;
    }

    public T Pop()
    {
        if (_count <= 0)
            throw new InvalidOperationException("Shuffler is empty");

        //walk a random number of steps round the loop
        for (int i = 0; i < GetNextRandom(_count); i++)
        {
            _current = _current.Next;
        }

        //remove next node from loop
        var next = _current.Next;
        var nextnext = _current.Next.Next;
        _current.Next = nextnext;
        _count--;

        if (_count <= 0)
        {
            //remove circular reference so we don't break refcounting for GC
            _current.Next = null;
            _current = null;
        }

        return next.Value;
    }

    private class Node
    {
        public Node Next { get; set; }
        public T Value => _value;
        private readonly T _value;
        public Node(T value)
        {
            _value = value;
        }
    }

    public bool Any()
    {
        return _count > 0;
    }
}

public sealed class Shuffler<T> : BaseShuffler<T>
{
    private Random _random;
    public Shuffler()
    {
        _random = new Random();
    }
    public Shuffler(int seed)
    {
        _random = new Random(seed);
    }

    protected override int GetNextRandom(int max)
    {
        return _random.Next(max);
    }
}

However, it appears to have a bias towards the returning an item near the end of the list as its first item: using the following test code

void Main()
{
    var shuffler = new Shuffler<(Rank rank, Suit suit)>();
    foreach (Suit suit in Enum.GetValues(typeof(Suit)))
    foreach (Rank rank in Enum.GetValues(typeof(Rank)))
        {
            shuffler.Add((rank, suit));
        }

    while (shuffler.Any())
    {
        var card = shuffler.Pop();
        Console.Out.WriteLine($"{card.rank} of {card.suit}");
    }
}

enum Suit
{
    Clubs,
    Diamonds,
    Hearts,
    Spades
}

enum Rank
{
    Two = 2,
    Three,
    Four,
    Five,
    Six,
    Seven,
    Eight,
    Nine,
    Ten,
    Jack,
    Queen,
    King,
    Ace
}

I get a Spade most commonly as the first card, occasionally a Heart, and Clubs or Diamonds seemingly never.  Is there a way to remove the bias from my algorithm without entirely removing the limit on the random number generator, which could then produce a large number of steps to take, slowing the program down?  If I remove the limit, my test case changes from sub-millisecond runtime to around 0.15 seconds - but without the bias (I'm running in LinqPad as a test bed).

As for the immediate problem, changing the initial start position, after all the cards have been added, to a random position should remove the bias:

public void SetStart()
{
    int limit = GetNextRandom(_count)
    for (int i = 0; i < limit; i++)
    {
        _current = _current.Next;
    }
}

Use custom classes rather than anonymous classes.  This encapsulates your code better and it is easier to decode what you've done.  Also now if you want to test different shufflers you can re-use a Card class.

public class Card
{
    public enum Suit
    {
        Clubs,
        Diamonds,
        Hearts,
        Spades
    }

    public enum Rank
    {
        Two = 2,
        Three,
        Four,
        Five,
        Six,
        Seven,
        Eight,
        Nine,
        Ten,
        Jack,
        Queen,
        King,
        Ace
    }

    public Rank rank { get; set; }
    public Suit suit { get; set; }

    public Card() { }
    public Card(Suit suit, Rank rank)
    {
        this.rank = rank;
        this.suit = suit;
    }
    public Card(int deckPosition)
    {
        rank = (Rank)((deckPosition % 13) + 2);
        suit = (Suit)(deckPosition / 13);
    }
    public override string ToString()
    {
        return $"{rank} of {suit}";
    }
}


Making equal-sized tabs to fill the full width of a box How would you simplify the following block of jQuery code?

I use this in a tab-box to size the tabs to style the tabs to the full width of the box no matter if it's 2 or 10 tabs...

It works perfectly, just interested if there's a better/shorter way to achieve the goal.

$(document).ready(function() {
  $('ul.tabs-list').each(function () {
    if ($(this).children().length == 2)
  $(this).addClass("two");
    if ($(this).children().length == 3)
  $(this).addClass("three");
    if ($(this).children().length == 4)
  $(this).addClass("four");
    if ($(this).children().length == 5)
  $(this).addClass("five");
    if ($(this).children().length == 6)
  $(this).addClass("six");
    if ($(this).children().length == 7)
  $(this).addClass("seven");
    if ($(this).children().length == 8)
  $(this).addClass("eight");
    if ($(this).children().length == 9)
  $(this).addClass("nine");
    if ($(this).children().length == 10)
  $(this).addClass("ten");
  });
});

Use the length as an index for an array containing classes :

$(document).ready(function() {
    var classes = ['zero', 'one', 'two', 'three', 'four', 'five', 'six', 'seven', 'eight', 'nine', 'ten'];
    $('ul.tabs-list').each(function () {
        $(this).addClass(classes[$(this).children().length]);
    });
});


4x4 matrix multiplication I am very new to matrix math. I have the following that sets the rotation in a 4x4 matrix. It is pretty ugly, so can anyone suggest how I could clean this up?

I would like to not have to call MultiplyTwoMatrixes twice. A better way of copying the 3x3 matrix into the end 4x4 matrix would be nice as well.

void MultiplyTwoMatrixes(float aMatrix[9], float bMatrix[9], float product[9])
{
    for (int i = 0; i < 3; i++)
    {
        for (int j = 0; j < 3; j++)
        {
            product[j + i * 3] = 0;
            for (int k = 0; k < 3; k++)
            {
                product[j + i * 3] += aMatrix[k + i * 3] * bMatrix[j + k * 3];
            }
        }
    }
}

void setrotation(float x, float y, float z)
{
    const float* oldMatrix = node->GetTransform();

    float xRot[9] = { 1, 0, 0, 0, cos(x), sin(x), 0, -sin(x), cos(x) };
    float yRot[9] = { cos(y), 0 ,-sin(y), 0, 1, 0, sin(y), 0, cos(y) };
    float zRot[9] = { cos(z), sin(z), 0, -sin(z), cos(z), 0, 0, 0, 1 };

    float xyResult[9];
    float xyzResult[9];
    float newMatrix[16];
    MultiplyTwoMatrixes(xRot, yRot, xyResult);
    MultiplyTwoMatrixes(xyResult, zRot, xyzResult);

    newMatrix[0] = xyzResult[0]; newMatrix[1] = xyzResult[1]; newMatrix[2] = xyzResult[2];
    newMatrix[4] = xyzResult[3]; newMatrix[5] = xyzResult[4]; newMatrix[6] = xyzResult[5];
    newMatrix[8] = xyzResult[6]; newMatrix[9] = xyzResult[7]; newMatrix[10] = xyzResult[8];
    newMatrix[12] = oldMatrix[12]; newMatrix[13] = oldMatrix[13]; newMatrix[14] = oldMatrix[14];

    node->SetTransform(newMatrix);
}

Arrays as Arguments

When you write this:

void MultiplyTwoMatrixes(float aMatrix[9], float bMatrix[9], float product[9])

That is exactly equivalent to:

void MultiplyTwoMatrixes(float*, float*, float*);

That is, there's no size checking on any of the inputs whatsoever - I could call your function with three arrays of size 3, 1, and 72. Would compile fine. Which is a problem!

Encapsulation and Operators

We want type safety. That suggests a Matrix class. For simplicity, let's just make it 2d:

template <typename T, int Rows, int Cols>
struct Matrix;

And then we can write a multiply operator:

template <typename T, int R1, int C1, int C2>
Matrix<T, R1, C2> operator*(const Matrix<T, R1, C1>& lhs, const Matrix<T, C1, C2>& rhs);

Notice that this already handles dimensional analysis for you. You just have to write the internals, which isn't complicated. And then you would be able to write your rotation function naturally:

Matrix<float, 3, 3> xyzResult = xRot * yRot * zRot;

Type-safe and more readable!

newMatrix

I don't understand the construction of newMatrix. So I can't really offer a better solution? You're not setting lots of indices of newMatrix, for one thing...


Intersection of two lists in clojure context: this is somewhat related to project euler problem 5, which is:

What is the smallest positive number that is evenly divisible by all of the numbers from 1 to 20?

I apply prime number decomposition to the range of numbers from 1 to 20, and the product of all that should be the smallest number.

(absorb '(2 2 2) '(2 3 4)) -> '(2 2 2 3 4) ;; does not have to be sorted

(defn absorb [seq1 seq2]
  (loop [s1 (sort seq1)
         s2 (sort seq2)
         acc '()]
    (let [s1-curr (first s1)
          s2-curr (first s2)]
      (cond (and (empty? s1) (empty? s2)) acc
            (empty? s1) (into s2 acc)
            (empty? s2) (into s1 acc)
            (= s1-curr s2-curr) (recur (rest s1) (rest s2) (cons s1-curr acc))
            :else (recur (rest s1) s2 (cons s1-curr acc))))))

Is there a way to write this in more idiomatic Clojure, or a more optimized/clea er version?

My assumption is that you want to keep the maximal count so that if 2 occurs 4 times in one list and 2 occurs 3 times in another, 2 will appear 4 times in the result.

In this case, I would write the following:

(defn elem-counts [coll]
   (reduce (fn [acc val] (assoc acc val (inc (acc val 0)))) {} coll))

With this, I can get the map from values in the collection to the number of times it occurs.

Now, for a way to reverse this process (from a map of counts to the list that would create the map):

(defn seq-from-counts [count-map]
   (flatten (for [[k v] count-map] (repeat v k))))

Finally, your intersect becomes:

(defn absorb [list1 list2]
   (seq-from-counts (merge-with max
                                (elem-counts list1)
                                (elem-counts list2))))


Adjacency list graph in Java I am reading a text, that from a higher level explained adjacency list implementation of a graph.

I have now tried to implement this as simply as I could. I did not see any reason to have an actual vertex or node class.

Based on my implementation, does it look like I understood properly how an adjacency list graph works?

import java.util.*;

public class Graph {
    private HashMap<String, HashSet<String>> nodes;

    public Graph ()
    {
        nodes = new HashMap<String, HashSet<String>>();
    }

    public void addNode(String name)
    {
        if (!nodes.containsKey(name))
            nodes.put(name ,  new HashSet<String>());
    }

    public void addEdge(String src , String dest)
    {
        HashSet<String> adjList = nodes.get(src);

        if (adjList == null)
            throw new RuntimeException("addEdge - src node does not exist");

        adjList.add(dest);
    }
}

I think the code is incomplete. It's possible to build a Graph but there is no way to process it as there are no methods returning the state of a Graph.

To be able to evaluate the shown code I'm going to extend it by two methods:

/**
 * Returns true if a node with the given name was added.
 */
public boolean hasNode(String name) {
    return nodes.containsKey(name);
}

/**
 * Returns true if an edge form source node to destination node (identified by their names) was added.
 */
public boolean hasEdge(String src, String dest) {
    HashSet<String> adjList = nodes.get(src);
    return adjList != null && adjList.contains(dest);
}

Let's use a simple example to show some shortcomings:

public static void main(String[] args) {
    Graph g = new Graph();
    g.addNode("A");
    g.addEdge("A", "B");

    System.out.println(g.hasNode("A")); // true
    System.out.println(g.hasNode("B")); // false
    System.out.println(g.hasEdge("A", "B")); // true
}

egdes to missing nodes
The method addEdge verifies that a source node is already present. But it doensn't verify if the destination node is present, too. Thus it's possible to addEdge from an already added node to an unknown node.
Whereas hasNode("B") will state that there is no node "B" in the Graph g, hasEdgde("A", "B") will return true which is at least misleading. I think  it's inconsistent as I would expected this implies there is a node "B" also present in the Graph g.

directed graph
Since addEdge adds an edge from a source node to a destination node it's more precise to speak of a directed graph. Otherwise

g.hasEdge("B", "A");

needs to return true. But actually it isn't.

readding a node is possbile
Another point: It's possible to add already added nodes again.

g.addNode("A");

This results in replacing the current node by a new one. There is no exception and hasNode("A") will still return true. But all edges from the previous node to other nodes are lost.

System.out.println(g.hasEdge("A", "B")); // false

This should be prevented.

readding an edge
Readding an edge doesn't throw an exception. But it doesn't add an edge actually. It's impossible since a node is storing it's edges in a Set. A Set will not add an already present element once again. As long as there is no need to know how many times an edge from a node to another was add this isn't severe.

Some further hints:

You can declare nodes as Map<String, Set<String>>. Since you're using interface methods only, there is no need to use the implementation types.
When adding a node calling first nodes.containsKey(name) and then on success calling nodes.put(name, new HashSet<String>()) has double complexity as the element within the map has to be looked up twice. But sinnce adding a node several times shouldn't be possible we can write

instead:

public void addNode(String name) {
    Set<String> node = nodes.get(name);
    if (node != null)
        throw new RuntimeException("addNode - node already exist");
    nodes.put(name, new HashSet<String>());
}


Mean π: Archimedes vs. Gauss - π computation through generalized means I've written this simplified code to compute Pi for educational/demonstration purposes.

These methods are based upon the generalized means: see a presentation on Pi and the AGM.

Archimedes' method gives linear convergence, which means you get two extra bits of precision per iteration.

Gauss's method gives quadratic convergence, which means your precision doubles every iteration.

One can wrap these methods in timers or print current results to see how they converge.

Archimedes' method could have been used to disprove a millennia of false claims about Pi. Alas, history.

import decimal

def pi_arc():
    """Archimedes c. ~230 B.C.E."""
    a, b = D(3).sqrt() / D(6), D(1) / D(3)
    pi = 0
    while True:
        an = (a + b) / 2
        b = (an * b).sqrt()
        a = an
        piold = pi
        pi = 2 / (a + b)
        if pi == piold:
            break
    return D(str(pi)[:-3])

def pi_agm():
    """Gauss AGM Method c. ~1800 A.D. """
    a, b, t = 1, D(0.5).sqrt(), 1 / D(2)
    p, pi, k = 2, 0, 0
    while True:
        an = (a + b) / 2
        b = (a * b).sqrt()
        t -= p * (a - an)**2
        a, p = an, 2**(k + 2)
        piold = pi
        pi = ((a + b)**2) / (2 * t)
        k += 1
        if pi == piold:
            break
    return D(str(pi)[:-3])

if __name__ == "__main__":
    prec = int(input('Precision for Pi: '))
    """Plus 3 for error"""
    decimal.getcontext().prec = prec + 3
    D = decimal.Decimal
    print(pi_arc())
    print(pi_agm())

Convergence testing

    if pi == piold:
        break

This is not usually done, because float equality has a lot of gotchas. In this case it's possible due to the numbers being Decimal, but if you need to move away from Decimal you're going to encounter issues.

Usually, convergence is measured as the absolute error decreasing below a chosen epsilon, a very small positive number. One advantage is that if you start checking for convergence to epsilon, your code will be compatible with arbitrary-precision math, where the two numbers will never equal each other exactly but you still need sane termination criteria.

Formatting/rounding

D(str(pi)[:-3])

This looks troublesome. You're converting a float to a string, and then selecting a certain number of fixed digits. Don't do this. Instead, just use the built-in round, which works with Decimals just fine.

Order of Operations

pi = ((a + b)**2) / (2 * t)

Exponentiation takes precedence over division, so you can drop the first pair of outer parens.

Import assignment

D = decimal.Decimal

Usually you shouldn't do this, and instead you should use normal import syntax:

from decimal import Decimal as D


Use counter for ajax loader I was using a Boolean var for indication of loading so my code looked like this:
(this example is in angularjs but i think it can be relevant for any ajax call handlers)

//HTML:

...LOADING...

//JS:
//Init
$scope.isLoading = false;

//Executing http request:
$scope.isLoading = true;
mainService.post(alteredItems).then(function (response) {
init();
}).catch(function (err) {
  $scope.callBack(err);
}).finally(function () {
  $scope.isLoading = false;
});

function init() {
  $scope.isLoading = true; //another loading
  mainService.get().then(function (response) {
  if (response) {

  }
  $scope.isLoading = false;
  });
}

The problem I had was that if 1 ajax method will complete before the other one so the "isLoading" will be false, and the loader will not be displayed although the request has not yet ended.

I converted the $scope.isLoading to int. So now code looks like this:

//Init
$scope.isLoading = 0;

//Start any ajax call
$scope.isLoading++;

//Finish
$scope.isLoading--;

Which works great, also no need to change the html because of the casting.
I can't find a drawback in this approach and it seems better than the Boolean option.

What do you think?

You SHOULD consider migrating to Angular (aka Angular2, aka Angular5) from ANgularJS (version 1), as it's not developing and soon won't be supported anymore.
I guess nested promises would be much useful here (more details below).

Rewrite init() as follow

function init() {
  return mainService.get().then(function (response) {
    if (response) {

    }
    $scope.isLoading = false;
  });
}

And in main block instead of

mainService.post(alteredItems).then(function (response) {
  init();
})

Write:

mainService.post(alteredItems).then(function (response) {
  return init();
})

In this case your finally would be called AFTER bot async calls completed.

P.S. Consider using library cg-buisy https://github.com/cgross/angular-busy
It automates displaying of loading indicator and accepts promises. Used it a lot in production.


Generating and finding parent nodes in a post-order tree Given a height h and list q of integers, list the parent node of
  each element in q, if nodes are read in post-order sequence starting
  at 1. The tree could be read like this if h=3.

    7
  3   6
 1 2 4 5

  If h = 3 and q = [1,3,7] the output would be the list [3, 6, -1] where
  the parent of the root is always -1.

How can I make this code faster? It does okay until h=10, then it slows down since it has to check \$2^h-1\$ nodes.

#Generate a perfect binary tree of height h
#Find the parent nodes of the values in list g, return negative 1 for the
#root

node_list = []
solution = {}

class Node:
    def __init__(self):
        self.left = None
        self.right = None
        self.data = None
        self.parent = -1
        self.left_edge = True
        self.depth = 0

def answer(h, q):
    global node_list
    global solution
    final_answer = []
    root = int(pow(2, h))-1
    solution.update({root: -1})
    node_list = (list(range(root+1)))
    node_list.reverse()
    node_list.pop()
    node = Node()
    node.data = root
    node.left = left_branch(h, node)
    node.right = right_branch(h, node)
    for i in q:
        for key in solution:
            if i == key:
                final_answer.append(solution[key])
        return final_answer

def left_branch(h, parent: Node):
    global node_list
    global solution
    new_node = Node()
    new_node.depth = parent.depth + 1
    new_node.parent = parent.data

    try:
        if parent.left_edge:
            new_node.data = parent.data//2
        node_list.remove(new_node.data)
    else:
        new_node.left_edge = False
        new_node.data = parent.data - int(pow(2, h - new_node.depth))
        print(new_node.data)
        node_list.remove(new_node.data)

    except ValueError:
        if not(new_node.data in node_list):
            return new_node

    solution.update({new_node.data: parent.data})
    left = left_branch(h, new_node)
    right = right_branch(h, new_node)
    new_node.left = left
    new_node.right = right
    return new_node

def right_branch(h, parent: Node):
    global node_list
    global solution
    new_node = Node()
    new_node.left_edge = False
    new_node.depth = parent.depth + 1
    new_node.parent = parent.data
    new_node.data = parent.data - 1
    try:
        node_list.remove(new_node.data)
    except ValueError:
        return new_node
    left = left_branch(h, new_node)
    right = right_branch(h, new_node)
    new_node.left = left.data
    new_node.right = right.data

    solution.update({new_node.data: parent.data})

    return new_node

This was a challenge given by Google Foobar. The time window has expired. My code took too long to run. I'm wondering about the optimal way to solve this problem.

The problem of "what is the parent of one node in the tree" can be solved as a recurrence relation, either using arithmetic or using the bits in the binary representation of the number.

The problem of "what are the various parents of many nodes in the tree" is a group solution that cries out for some batch computation followed by speed-oriented lookups.

Look at the numeric properties of the nodes. You should not be computing a tree for this.

Also, look at the desired problem solution: you want to perform a computation or lookup for each number in a list. Thus, your information storage should be optimized for this.

Given that your tree represents a range of numbers, I'll suggest either a dictionary or a list. Since the numbers are a dense group of integers that start from 1, I'll suggest a list.

Parents = [0] * (n+1)

Given h, how can you predict n (above)? What's the number at the root of the tree?

Given R the number at the root of a tree, what are the values of the numbers at the root of the next lower subtrees? (Hint: easy for the left, harder for the right.)


Creating a simple unit of work with Entity Framework and no repository I've been trying to implement a .NET MVC Unit of Work API (rather than creating a separate repository), but it doesn't feel right. Am I going about this the correct way?

Following advice from here.

BaseController

public class BaseController : ApiController
    {
        protected DBEntities _dbEntities;

        protected override void OnActionExecuting(ActionExecutingContext filterContext)
        {
            _dbEntities = new DBEntities();
        }

        protected override void OnActionExecuted(ActionExecutedContext filterContext)
        {
            _dbEntities.SaveChanges();
        }
    }

MyController

public class MyController : BaseController
{
    public HttpResponseMessage PutMyObject(int id, int id2)
    {
        if (id != 0)
        {
            var myObject = _dbEntities.MyObjects.Where(x => x.id == id);
            if (myObject.Count() > 0)
            {
                MyObject temp = myObject.SingleOrDefault();
                temp.Processed = true;
                return Request.CreateResponse(HttpStatusCode.OK);
            }
            else
            {
                return Request.CreateErrorResponse(HttpStatusCode.NotFound);
            }
        }
        else
        {
            /* do some other stuff */
        }
    }

}

My thinking around this is that the controller action is a single Unit of Work. The database connection is opened when the controller action starts, and SaveChanges is called once the response is returned.

Am I going about this in the correct manner? Do I need to dispose of _dbentities after SaveChanges is called within the BaseController?

I can be very short about this:

Single responsibility: a controller shouldn't also have Unit of Work responsibilities. The context itself is the perfect Unit of Work.
You don't always need SaveChanges
Use dependency injection.

I like to see controllers as light-weight doorways to web-independent service methods. The services have their own DbContext and execute SaveChanges where necessary and, thus, can be used in other applications than just MVC/Web API applications. Controllers receive services by dependency injection.


Python program to take in two strings and print the larger string I have written a Python program to take in two strings and print the larger of the two strings.

Here is my code -

string1 = input("Enter first string: ")
string2 = input("Enter second string: ")
count1 = 0
count2 = 0
for i in string1:
      count1 = count1 + 1

for j in string2:
      count2 = count2 + 1

if (count1 < count2):
      print ("Larger string is:")
      print (string2)

elif (count1 == count2):
      print ("Both strings are equal.")
else:
      print ("Larger string is:")
      print (string1)

Here are some example outputs -

Enter first string: everything
Enter second string: nothing
Larger string is:
everything

Enter first string: cat
Enter second string: apple
Larger string is:
apple

I feel that my code is unnecessarily long. Therefore, I would like to know whether I could make this program shorter and more efficient.

Any help would be highly appreciated.

Python strings supports Python built-in len function. You don't need to iterate through them manually, as for lists/dicts/sets etc (it is not Pythonic):

def compare_strings_len(s1, s2):
    if len(s1) > len(s2):
        print('String 1 is longer: ', s1)
    elif len(s1) < len(s2):
        print('String 2 is longer: ', s2)
    else:
        print('Strings length are equal!')


Simplifying Cryptography Key Generation without Losing Security I am writing a helper class for encrypting string and other data types. My goal is to make it as easy as possible to use, while still keeping a substantial amount of security.

One issue I struggled with was simplifying all the different key and IV sizes. I've settled on having the caller provider a salt, and then generating my own key and IV data from the salt.

I assume this compromises the security very slightly, but not that much.

I would appreciate hearing other people's thoughts, especially those who have more experience with cryptography than I. Am I compromising my security too much?

public virtual byte[] EncryptBytes(byte[] data)
{
    using (SymmetricAlgorithm algorithm = TripleDES.Create())
    using (ICryptoTransform encryptor = algorithm.CreateEncryptor(GetKey(algorithm), GetIV(algorithm)))
    {
        MemoryStream ms = new MemoryStream();
        using (Stream cs = new CryptoStream(ms, encryptor, CryptoStreamMode.Write))
        {
            cs.Write(data, 0, data.Length);
        }
        return ms.ToArray();
    }
}

public byte[] GetKey(SymmetricAlgorithm algorithm)
{
    return DeriveBytes(algorithm.KeySize / 8);
}

public byte[] GetIV(SymmetricAlgorithm algorithm)
{
    return DeriveBytes(algorithm.BlockSize / 8);
}

protected byte[] DeriveBytes(int bytes)
{
    Rfc2898DeriveBytes derivedBytes = new Rfc2898DeriveBytes(Password, Salt, 1000);
    return derivedBytes.GetBytes(bytes);
}

Why are you using 3DES instead of AES?  Why is the algorithm hard-coded in EncryptBytes, but parameterized in GetKey() and GetIV()?

The last line looks suspicious — did you mean bits / 8?

protected byte[] DeriveBytes(int bits)
{
    Rfc2898DeriveBytes derivedBytes = new Rfc2898DeriveBytes(Password, Salt, 1000);
    return derivedBytes.GetBytes(bits / 8);
}

If so, then shouldn't it be…?

public byte[] GetKey(SymmetricAlgorithm algorithm)
{
    return DeriveBytes(algorithm.KeySize);
}


Expanding pixels in an image Custom class Image takes a 2-D array of 0's and 1's upon initialization. Method transform returns a modified array by modifying 0's adjacent to a 1, such that

[[0,0,0],
 [0,1,0],
 [0,0,0]]

returns

[[0,1,0],
 [1,1,1],
 [0,1,0]]

I've been working on method blur(n), which outputs an array that has been transformed n times, such that n=3 changes

[[0,0,0,0,0,0,0,0,0],
 [0,0,0,0,0,0,0,0,0],
 [0,0,0,0,0,0,0,0,0],
 [0,0,0,0,0,0,0,0,0],
 [0,0,0,0,1,0,0,0,0],
 [0,0,0,0,0,0,0,0,0],
 [0,0,0,0,0,0,0,0,0],
 [0,0,0,0,0,0,0,0,0],
 [0,0,0,0,0,0,0,0,0],
 [0,0,0,0,0,0,0,0,0],
 [0,0,0,0,0,0,0,0,0],
 [0,0,0,0,0,0,0,0,0]]

into

[[0, 0, 0, 0, 0, 0, 0, 0, 0]
 [0, 0, 0, 0, 1, 0, 0, 0, 0]
 [0, 0, 0, 1, 1, 1, 0, 0, 0]
 [0, 0, 1, 1, 1, 1, 1, 0, 0]
 [0, 1, 1, 1, 1, 1, 1, 1, 0]
 [0, 0, 1, 1, 1, 1, 1, 0, 0]
 [0, 0, 0, 1, 1, 1, 0, 0, 0]
 [0, 0, 0, 0, 1, 0, 0, 0, 0]
 [0, 0, 0, 0, 0, 0, 0, 0, 0]
 [0, 0, 0, 0, 0, 0, 0, 0, 0]
 [0, 0, 0, 0, 0, 0, 0, 0, 0]
 [0, 0, 0, 0, 0, 0, 0, 0, 0]]

Right now my solution to blur(n) works, by first creating a new Image object, transforming and then transforming additional instances of Image an additional (n-1) times. I've been getting errors if I try to apply blur without instantiating a new Image and transforming it — the transform method is not available to class Array. How could I go about applying transform iteratively without having to instantiate Image inside the class?

class Image
  attr_accessor :arr

  def initialize(arr)
    @arr = arr
  end

  def transform
    cloned = self.arr.map(&:clone)

    #scan original array for 1; map crosses into clone if found
    self.arr.each.with_index do |row, row_index|
      row.each.with_index do |cell, col|
        if cell == 1
          cloned[row_index][col+1] = 1 unless col+1 >= row.length #copy right
          cloned[row_index+1][col] = 1 unless row_index+1 >= cloned.length # copy down
          cloned[row_index][col-1] = 1 unless col.zero? # copy left
          cloned[row_index-1][col] = 1 unless row_index.zero? #copy up
        end
      end
    end
    cloned
  end

  def blur(n)
    blurred = Image.new(self).arr.transform
    (n-1).times do
      blurred = Image.new(blurred).transform
    end
    blurred
  end

end

image = Image.new([[0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,1,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0]])

image.blur(3)

#returns...
# [[0, 0, 0, 0, 0, 0, 0, 0, 0],
#  [0, 0, 0, 0, 1, 0, 0, 0, 0],
#  [0, 0, 0, 1, 1, 1, 0, 0, 0],
#  [0, 0, 1, 1, 1, 1, 1, 0, 0],
#  [0, 1, 1, 1, 1, 1, 1, 1, 0],
#  [0, 0, 1, 1, 1, 1, 1, 0, 0],
#  [0, 0, 0, 1, 1, 1, 0, 0, 0],
#  [0, 0, 0, 0, 1, 0, 0, 0, 0],
#  [0, 0, 0, 0, 0, 0, 0, 0, 0],
#  [0, 0, 0, 0, 0, 0, 0, 0, 0],
#  [0, 0, 0, 0, 0, 0, 0, 0, 0],
#  [0, 0, 0, 0, 0, 0, 0, 0, 0]]

I assume you are trying to do something like:

def blur(n)
  blurred = Image.new(self).arr.transform
  (n-1).times do
    blurred = blurred.transform
  end
  blurred
end

And getting an error like:

`block in blur': undefined method `transform' for #<Array:0x00000002d51ba8>

The reason is that transform is a instance method of Image, which is why you need to convert back into an image before calling it.

There are a couple of way this can be handled.  I've chosen to make transform a static method that only works on arrays.  The instance methods transform and blur call the static method as needed

class Image
  attr_accessor :arr

  def initialize(arr)
    @arr = arr
  end

  def transform
    result = Image.arr_transform(@arr)
    Image.new(result)
  end

  def self.arr_transform(array)
    cloned = array.map(&:clone)

    #scan original array for 1; map crosses into clone if found
    array.each.with_index do |row, row_index|
      row.each.with_index do |cell, col|
        if cell == 1
          cloned[row_index][col+1] = 1 unless col+1 >= row.length #copy right
          cloned[row_index+1][col] = 1 unless row_index+1 >= cloned.length # copy down
          cloned[row_index][col-1] = 1 unless col.zero? # copy left
          cloned[row_index-1][col] = 1 unless row_index.zero? #copy up
        end
      end
    end
    cloned
  end

  def blur(n)
    blurred = @arr
    (n).times do
      blurred = Image.arr_transform(blurred)
    end
    Image.new(blurred)
  end

end

Testing:

image = Image.new([[0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,1,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0],
                   [0,0,0,0,0,0,0,0,0]])

require 'pp'
pp image.blur(3)

Returns:

#<Image:0x00000002e9fb40
 @arr=
  [[0, 0, 0, 0, 0, 0, 0, 0, 0],
   [0, 0, 0, 0, 1, 0, 0, 0, 0],
   [0, 0, 0, 1, 1, 1, 0, 0, 0],
   [0, 0, 1, 1, 1, 1, 1, 0, 0],
   [0, 1, 1, 1, 1, 1, 1, 1, 0],
   [0, 0, 1, 1, 1, 1, 1, 0, 0],
   [0, 0, 0, 1, 1, 1, 0, 0, 0],
   [0, 0, 0, 0, 1, 0, 0, 0, 0],
   [0, 0, 0, 0, 0, 0, 0, 0, 0],
   [0, 0, 0, 0, 0, 0, 0, 0, 0],
   [0, 0, 0, 0, 0, 0, 0, 0, 0],
   [0, 0, 0, 0, 0, 0, 0, 0, 0]]>


A WinForm application that filters users I built a small WinForm application that has checkboxes to filters users with different properties.

Each user has a name, color and number.
With the program you can filter by color or name the users that you will see:

Inside the application there is a static list of users:

// SomeUser(<name>, <color>, <number>);
SomeUser("user1", "red", "one");
SomeUser("user2", "blue", "one");
SomeUser("user3", "red", "two");
SomeUser("user4", "blue", "two");
SomeUser("user4", "yellow", "two");
SomeUser("user4", "blue", "three");

The application works as expected.
If you are not specified any filters, it will show you all.
There is OR inside each groupbox.
There is AND bettwen the groupboxes.
Meaning:
If you checked red and blue, it will search for users that have red color OR blue color.
If you checked red, blue and one, it will search for users that have red OR blue color AND one number.
I hope you understand from these two small examples.

This is the filter function:

void countFinding(CheckBox checkbox, string userField, ref int found)
{
    if (checkbox.Checked)
    {
        if (userField == checkbox.Text)
        {
            found++;
        }
    }
}

private bool isFilter(SomeUser user)
{
    bool isAllowed = false;

    int shouldBeFound = 0;
    int found = 0;
    if(this.checkBoxRed.Checked || this.checkBoxBlue.Checked)
    {
        shouldBeFound++;
        countFinding(this.checkBoxRed, user.color, ref found);
        countFinding(this.checkBoxBlue, user.color, ref found);
    }

    if (this.checkBoxOne.Checked || this.checkBoxTwo.Checked)
    {
        shouldBeFound++;
        countFinding(this.checkBoxOne, user.number, ref found);
        countFinding(this.checkBoxTwo, user.number, ref found);
    }

    if (shouldBeFound == found)
    {
        isAllowed = true;
    }

    return isAllowed;
}

Inside the isFilter() function I am checking twice the same checkbox.
In the first if I am checking:
if(this.checkBoxRed.Checked || this.checkBoxBlue.Checked)
and if one of them is true it goes (twice) to an inner function (countFinding())that also checks if the checkbox is checked.

So, it appear them I am checking at least one checkbox twice to see if it checked.

Is it possible to make it better?  what do you think?

I don't think that checking the state of the box twice is an issue.  However, I do think this could be written in a more general way that would be easier to add more boxes / fields to later.  For example, sticking with the current class structure:

bool FieldMatchesFilterGroup(string field, params CheckBox[] group)
{
    return group.All(box => !box.Checked)
        || group.Any(box => box.Checked && box.Text == field);
}

bool UserMatchesFilters(SomeUser user)
{
    return FieldMatchesFilterGroup(user.color, checkBoxRed, checkBoxBlue)
        && FieldMatchesFilterGroup(user.number, checkBoxOne, checkBoxTwo);
}

This could be made considerably more general, but at the cost of also becoming more complex, so whether that's worth doing depends on how you plan to use this code.  The FieldMatchesFilterGroup method above is using LINQ, but you could do the same thing with a foreach loop:

bool FieldMatchesFilterGroup(string field, params CheckBox[] group)
{
    bool any = false, found = false;
    foreach(var box in group)
    {
        if(box.Checked)
        {
            any = true;
            found |= (box.Text == field);
        }
    }
    return !any || found;
}

Also, just as a style thing, the typical C# naming convention is UpperCamelCase for methods.


Calculating student and class averages I am currently going through Codecademy's Python course (I don't come from a coding background but I'm thoroughly enjoying it) and I got to the end of one of the sections and thought, "Well it worked, but that seemed wildly inefficient."  I tried shortening unnecessary parts and came up with this:

lloyd = {
    "name": "Lloyd",
    "homework": [90.0, 97.0, 75.0, 92.0],
    "quizzes": [88.0, 40.0, 94.0],
    "tests": [75.0, 90.0]
}
alice = {
    "name": "Alice",
    "homework": [100.0, 92.0, 98.0, 100.0],
    "quizzes": [82.0, 83.0, 91.0],
    "tests": [89.0, 97.0]
}
tyler = {
    "name": "Tyler",
    "homework": [0.0, 87.0, 75.0, 22.0],
    "quizzes": [0.0, 75.0, 78.0],
    "tests": [100.0, 100.0]
}

classlist = [lloyd, alice, tyler]

# Add your function below!

def get_average(student):
    average_homework = sum(student["homework"]) / len(student["homework"])
    average_quizzes = sum(student["quizzes"]) / len(student["quizzes"])
    average_tests = sum(student["tests"]) / len(student["tests"])
    average_total = average_homework * 0.1 + average_quizzes * 0.3 + average_tests * 0.6
    return average_total

def get_letter_grade(score):
    if score >= 90:
        return "A"
    if score < 90 and score >= 80:
        return "B"
    if score < 80 and score >= 70:
        return "C"
    if score < 70 and score >= 60:
        return "D"
    else:
        return "F"

def get_class_average(classlist):
    a = []
    for student in classlist:
        a.append(get_average(student))
    return sum(a) / len(a)

get_class_average(classlist)

Now this bit works, but I was wondering if there are more places I could trim the code down without losing the functionality (I'm guessing there is in the get_average function but the things I tried came back as errors because of the "name" field in the dictionary). Codecademy seems good for learning the fundamentals, but I wanted to get some insight into the "best" way to do things.  Should I not worry about reworking functional code to be more efficient at first and just learn the fundamentals, or should I keep trying to make the "best" code as I learn?

In the function get_letter_grade you were checking more conditions then necessary. Try to follow the logical difference between your function and my code. It is simple math which I used to optimize it. Also structuring it as an if-elif-...-else is better for readability.
In get_average and get_class_average I use a thing called list comprehension. A simple google search will tell you what it is and why it is better. Google it in python tutorials if you have problem.

Edited Functions

def average(values):
    return sum(values) / len(values)

def get_average(student):
    keys = ['homework', 'quizzes', 'tests']
    factors = [0.1, 0.3, 0.6]
    return sum([ average(student[key]) * factor for key, factor in zip(keys, factors)])

def get_letter_grade(score):
    if score >= 90:
        return "A"
    elif score >= 80:
        return "B"
    elif score >= 70:
        return "C"
    elif score >= 60:
        return "D"
    else:
        return "F"

def get_class_average(classlist):
    return average([get_average(student) for student in classlist])

get_class_average(classlist)


Picking the smallest value in a list using recursion This is the code I wrote in order to find the smallest value in a list using recursion.However I feel like there would be a better way to do this?

def minimum(lst):
    """
    parameters : lst of type list
    return : the value of the smallest element in the lst
    """
    if len(lst) == 1:
        return lst[0]

    if lst[0] < lst[1]:
        lst.append(lst[0])
        return(minimum(lst[1:]))
    else:
        return(minimum(lst[1:]))

Instead of adding back to the list to maintain an accumulator (the current minimum in this case), I'd just add a second parameter to the function, and allow it to default to None:

# acc, the accumulator, represents the current lowest found
def my_minimum(lst, acc = None):
   if not lst: # If the list is empty, return the current lowest
      return acc

   # Head is the first element, and the tail is the rest of the list
   # This is a common pattern when recursively iterating a list
   head, *tail = lst

   # The first time this is run, "not acc" fails, and it defaults to head
   new_acc = head if not acc or head < acc else acc

   return my_minimum(tail, new_acc)

lst = [2, 3, 4, 9, 2, -2]

print(my_minimum(lst))
-2

Note though that head, *tail = lst, is quite inefficient, as it requires making an entire copy to construct tail. I'm using it here for brevity.


Validating an input string against some enum fields I have this enum below:

public enum TestEnum {
    h1, h2, h3, h4;

    public static String forCode(int code) {
    return (code >= 0 && code < values().length) ? values()[code].name() : null;
    }

    public static void main(String[] args) {
        System.out.println(TestEnum.h1.name());
        String ss = "h3";

        try {
           TestEnum.valueOf(ss); // but this validates with all the enum values
           System.out.println("valid");
        } catch (IllegalArgumentException e) {
           System.out.println("invalid");
        }
    }
}

I need to check if the enum value represented by my string ss is one of my enum values h1, h2 or h4. So if h3 is being passed as a string, I would like to return false or throw IllegalArgumentException. I won't need to validate ss with h3 in the enum.

I came up with the below code to do this with the enum, but I believe there is a more elegant solution.

public enum TestEnum {
    h1, h2, h3, h4;

    public static boolean checkExcept(String el, TestEnum... except){
        boolean results = false;
        try{
            results = !Arrays.asList(except).contains(TestEnum.valueOf(el));
        }catch (Exception e){}
        return results;
    }

    public static String forCode(int code) {
        return (code >= 0 && code < values().length) ? values()[code].name() : null;
    }

    public static void main(String[] args) {

        String ss = "h3";

        if(TestEnum.checkExcept(ss,TestEnum.h1, TestEnum.h2, TestEnum.h3)){
           System.out.println("valid");
        }else{
           System.out.println("invalid");
        }
    }
}

Is there any better way of solving this problem?

I would pass a TestEnum instead of a String to your method, forcing the exception handling to be outside the method. I don't see why you are using Strings at all actually (although I suspect that this is only a small part of your code, and that you have your reasons for needing this).

I would do some refactoring and use EnumSet, which could make your code something like this: (untested code, but I believe it should work)

public static void main(String[] args) {
    String enumName = "h3";
    EnumSet<TestEnum> except = EnumSet.of(TestEnum.h1, TestEnum.h2, TestEnum.h3);

    boolean valid;
    try {
       valid = !except.contains(TestEnum.valueOf(enumName));
    } catch (IllegalArgumentException e) { valid = false; }
    System.out.println(valid ? "valid" : "invalid");
}

Note that I have gotten rid of your entire method here as I think that when you use EnumSet like this, you don't need the method. But of course you can put the relevant parts of the above code into a method.


Take input of delimited file of integers and output result I wanted to learn some new language so I developed a really simple vb.net "console application" (was that the right type?) that takes text input, looks at it and multiplies it if it's even and then reads each result into a msgbox.

test.txt = 1 2 3 4 5 6 7 8 9
result should be an array {1 4 3 8 5 12 7 16 9}

This would be my first vb.net programming attempt

Module MyFirstModule
    Sub Main()
        HelloNumbers()
    End Sub

    Private Sub HelloNumbers()
        Dim inputFile As String
        inputFile = "c:\temp\test.txt"
        Dim i As Integer
        Dim readResult As String() = Split(System.IO.File.ReadAllText(inputFile))
        Dim multipliedResult As Integer()
        Dim lower As Integer = LBound(readResult)
        Dim upper As Integer = UBound(readResult)
        ReDim multipliedResult(0 To upper)
        For i = lower To upper
            multipliedResult(i) = MultiplyEvenNumbers(CInt(readResult(i)))
        Next

        For i = lower To upper
            If multipliedResult(i) Mod 2 = 0 Then
                MsgBox(readResult(i) & " was multiplied by two for " & multipliedResult(i))
            Else : MsgBox(readResult(i) & " is original")
            End If
        Next
    End Sub

    Private Function MultiplyEvenNumbers(ByVal inputNumber As Integer) As Integer
        If inputNumber Mod 2 = 0 Then inputNumber = inputNumber * 2
        MultiplyEvenNumbers = inputNumber
    End Function
End Module

This is a good start. You created two functions to separate parts of the logic but there is more that can be refactored. The HalloNumbers sub is too big and does too much. Let's try to improve.

First the path shouldn't be hardcoded inside it so we pass it as parameter:

Sub Main()
    HelloNumbers("c:\temp\test.txt")
End Sub

Now we take parts of the big HalloNumbers and move them into fuctions that do only one thing - we always strive for this because it's easier to maintain when you only have to care about one thing at a time (see SRP).

Private Sub HelloNumbers(ByVal inputFile As String)
    Dim numbers As Integer() = ReadNumbers(inputFile)
    Dim multipliedNumbers = numbers.Select(Function(number) MultiplyEvenNumbers(number))
    PrintNumbers(multipliedNumbers)
End Sub

So we can extract the following functions:

We need to read the numbers. They are in a file. So let's require a path and after reading them we convert each number-string into an Integer with linq:

Private Function ReadNumbers(ByVal inputFile As String) As IEnumerable(Of Integer)
    ReadNumbers = Split(File.ReadAllText(inputFile)).Select(Function(s, i) CInt(s))
End Function

Then we need to multiply them. This one was already there so we leave it as is.

Private Function MultiplyEvenNumbers(ByVal inputNumber As Integer) As Integer
    If inputNumber Mod 2 = 0 Then inputNumber = inputNumber * 2
    MultiplyEvenNumbers = inputNumber
End Function

Finaly you want to print the results. Let's do it in PrintNumbers. If you decide to write the output to the console you now just have to adjust this small function.

Private Sub PrintNumbers(ByVal numbers As IEnumerable(Of Integer))
    For Each number As Integer In numbers
        If number Mod 2 = 0 Then
            MsgBox(String.Format("{0} was multiplied by two for {1}", number, number / 2))
        Else : MsgBox(String.Format("{0} is original", number))
        End If
    Next
End Sub


Avoid redundant code in this simple method I am wondering what would be an efficient way to simplify this very simple PHP method and I really lack inspiration:

public function formatToFullDate($countryCode, $hideTime = false)
{
    $format = '';
    if($hideTime === true)
    {
        switch($countryCode)
        {
            case 'us':
            case 'ca':
                $format = '%A %e %B %Y';
                break;
            case 'de':
                $format = '%A, den %e. %B %Y';
            break;
            default:
                $format = '%A %e %B %Y';
        }
    }
    else
    {
        switch($countryCode)
        {
            case 'us':
            case 'ca':
                $format = '%A %e %B %Y - %I:%M %P';
                break;
            case 'de':
                $format = '%A, den %e. %B %Y - %H:%M';
            break;
            default:
                $format = '%A %e %B %Y - %H:%M';
        }
    }

    return gmstrftime($format, $this->getTimestamp());
}

There is still some redundant code in there(' - %H:%M' and '%A %e %B %Y'). You could put these in variabels, but i like it so more. Sory for my bad English.

public function formatToFullDate($countryCode, $hideTime = false)
{
    $format = '';
    $time_format = '';
    switch($countryCode)
    {
        case 'us':
        case 'ca':
            $time_format = ' - %I:%M %P';
            $date_format = '%A %e %B %Y';
            break;
        case 'de':
            $time_format = ' - %H:%M';
            $date_format = '%A, den %e. %B %Y';
        break;
        default:
            $date_format = '%A %e %B %Y';
            $time_format = ' - %H:%M';
    }
    if($hideTime)$time_format = '';

    return gmstrftime($date_format.$time_format, $this->getTimestamp());
}


Removing Values From a Vector I am currently writing some generic functions to make working with vectors a little "cleaner". Working with the "erase-remove idiom" can be a little confusing sometimes, and make the code look a little dirty, so why not throw it into a function? I have four individual function I wish to have reviewed and made better. I know that there are tons of examples online (trust me, I've looked at most of them).

1. Simply Remove All Instances of a Value From a Vector

template<class T>
void Vec_RemoveAll(vector<T>& vec, T val)
{
    vec.erase(remove(vec.begin(), vec.end(), val), vec.end());
}

This function is the simplest of all four.

2. Remove All Instances of a Container of Values From a Vector

template<class Container, class T>
void Vec_RemoveAll(vector<T>& vec, const Container& vals)
{
    for (const T& val : vals) vec.erase(remove(vec.begin(), vec.end(), val), vec.end());
}

Simply loops through the container checking the vector for occurrences of each value.

3. Remove All Elements That Meet Unary Predicate's Condition

template<class T, class UnaryPredicate>
void Vec_RemoveAll_If(vector<T>& vec, UnaryPredicate* predicate)
{
    vec.erase(remove_if(vec.begin(), vec.end(), predicate), vec.end());
}

I'm not familiar with remove_if, I'm worried I may have not implemented this correctly.

4. Remove Elements That Meet Any of the Predicates' Conditions

template<class T, class UnaryPredicate>
void Vec_RemoveAll_If(vector<T>& vec, const vector<UnaryPredicate> predicates)
{
    for (const UnaryPredicate& predicate : predicates)
        vec.erase(remove_if(vec.begin(), vec.end(), predicate), vec.end());
}

How would one generalize the type of container that the unary predicates are stored in?

Before getting into each function, your last question is important -- how do we generalize the type of container? By passing iterators instead of containers, of course! When we first encounter the C++ Standard Library, the ubiquity of iterators can seem confusing -- why not pass containers around instead? The reason is that different programs have many different needs, and passing a particular kind of container can be very restricting. For example, if my function takes a parameter of type const vector<const Foo>&, I cannot pass a const vector<Foo>& or a const vector<const Foo*>&. If instead I take a pair of iterators with the requirement that *it is of type const Foo&, I can use a wider range of container types; vector<Foo>'s and vector<const Foo>'s iterators work directly, and vector<const Foo*> can be made to work with iterator adaptors.

OK, function by function.

1. Simply Remove All Instances of a Value From a Vector

template<class T>
void Vec_RemoveAll(vector<T>& vec, T val)
{
    vec.erase(remove(vec.begin(), vec.end(), val), vec.end());
}

First I'll include my standard disclaimer about using namespace std;: Don't do it, it can lead to subtle bugs, and you'll get used to seeing std:: everywhere.

Second, pass const T&, not T. What if T is a large type? std::remove takes const T&.

Third, I don't think this is a particularly wise function to write. It's a common C++ idiom; once you learn it it should become automatic.

2. Remove All Instances of a Container of Values From a Vector

template<class Container, class T>
void Vec_RemoveAll(vector<T>& vec, const Container& vals)
{
    for (const T& val : vals) vec.erase(remove(vec.begin(), vec.end(), val), vec.end());
}

First point is a question: Is this the performance characteristic you want? Intuitively, I would think that vals would tend to be smaller than vec, so you probably want to iterate through vals in the inner loop, not the outer loop. You could achieve this by using std::remove_if instead and writing a function that determines whether an element is in vals. You could get even better performance if you required vals to be sorted.

Second: for (const T& val : vals) is dangerous. Container isn't required to contains Ts, just things that are comparable to T. You would want to write this as for (const auto& val : vals). For example, consider that vec is a vector<string> and vals is a vector<const char*>. Each of the strings in vals would be copied into a string while iterating using your version.

3. Remove All Elements That Meet Unary Predicate's Condition

template<class T, class UnaryPredicate>
void Vec_RemoveAll_If(vector<T>& vec, UnaryPredicate* predicate)
{
    vec.erase(remove_if(vec.begin(), vec.end(), predicate), vec.end());
}

Almost right, except that you should pass UnaryPredicate, not UnaryPredicate*. Your version works fine for function pointers but not function objects. But I raise the same objection as to function 1 -- this is a standard idiom, and hiding it behind a function call is more likely to confuse than enlighten experience C++ programmers who read your code.

4. Remove Elements That Meet Any of the Predicates' Conditions

template<class T, class UnaryPredicate>
void Vec_RemoveAll_If(vector<T>& vec, const vector<UnaryPredicate> predicates)
{
    for (const UnaryPredicate& predicate : predicates)
        vec.erase(remove_if(vec.begin(), vec.end(), predicate), vec.end());
}

I recommend against writing this function in the first place; it's probably best for any potential user of this function to compose their predicates manually into a single predicate, or in the alternative to write a library to help them do so. That is more generally applicable and removes the requirements that users of your library learn this idiosyncratic API. In the alternative, you can do as you did in function 2, and pass const Container& instead of trying to specify a specific container.


React Smart and Dumb Components separation query I have been working on React and would like to know best practices for separating smart and dumb components.

This example is a Parent controls state, but I have put the button UI in render. Should these go into child and implemented back to parent via callback or is that overkill?

class Child extends React.Component {
      constructor(props){
      super(props);
      }
      render() {
      return (

I said {this.props.greeting} {this.props.count} times

        );
      }
    }

    class Parent extends React.Component {
      constructor() {
        super();
        this.state = {
          count: 0,
          greeting: "Hello"
        };
      }

      sayHello() {
        this.setState((prevState, props) => {
        return {
          count: prevState.count + 1,
              greeting: "Hello"
               }
        }
      )};
        sayGoodBye() {
        this.setState((prevState, props) => {
        return {
          count: this.count = 1,
          greeting: "Goodbye"
               }
        }
      )};

      render() {
        return (

            <button onClick={() => this.sayHello() }>Say Hello</button>
            <button onClick={() => this.sayGoodBye() }>Say Goodbye</button>
          <Child count={this.state.count} greeting={this.state.greeting} />

        )

      }
    }

    ReactDOM.render(<Parent />, document.getElementById('app'));

Seems like you got a good start.

Consider refactoring components that do not keep state into a "functional component". More about functional components here. This is a simple JavaScript arrow function that returns what you would like the component to render, based on your props. The function will take the prop-object as an argument. In your case it would become:

const Child = (props) => (

I said {props.greeting} {props.count} times

)

Or even simpler using destructuring from Ecmascript 2015. Notice that we can retrieve the attributes of the prop-object directly in the function parameters.

const Child = ({greeting, count}) => (

I said {greeting} {count} times

)

I think your parent component is fine. You are separating the concerns of the buttons and text output neatly and there is no need to move the buttons into the Child component. This is obviously a simple example, in more complex cases it might make sense to encapsulate the elements inside another component in order to prevent re-rendering or because of functional cohesion. In this case it would perhaps be overkill.


Join - order by vote-sum and check if user has voted I have a vote and a comment table. Now I want to display all comments and order them by votes-sum. Sum because some votes can have a higher value than others.

Therefore I want to display if the user has voted already on a comment. So I need to get that value too.

This is my actual query:

SELECT *, SUM(v.`value`) as votesum,

(
     SELECT 1
     FROM votes
     WHERE user_id = 123 and id = v.id

) as user_voted_already

FROM comments as c
LEFT JOIN votes as v
ON c.id = v.`foreign_id` and v.`foreign_type` = 'comment'
WHERE c.`network_id` = 3183
GROUP BY ( v.`foreign_id`)
ORDER BY votesum DESC

It's working but I'm just wondering if this is the right way (or: the fastest way).

These are the table definitions:

votes (polymorphic):
    id
    user_id
    guest_ip (for guest votes)
    foreign_id
    foreign_type
    category
    value
    comment
    created_at
    updated_at

comments
    id
    network_id
    network_name

… and many others that I don't think are needed for the question.

Schema

Your schema isn't well normalized: you shouldn't have network_name in the comments table when you already have network_id.

Non-standard GROUP BY

Standard SQL requires that when the query has a GROUP BY, all columns being selected must either be mentioned in the GROUP BY clause or be an aggregate function.  However, MySQL is sloppy, and allows non-sensical queries like yours that violate that rule, unless you enable the standard behaviour explicitly by setting the ONLY_FULL_GROUP_BY option on the server.  (When you write SELECT *, …, which created_at timestamp would you expect to appear in the results, for example?)

You wrote GROUP BY ( v.`foreign_id`), but GROUP BY c.id would make it more obvious that you want to group by comment.

Subselect

Subselects within the SELECT column list are a bit nasty, and can nearly always be expressed better.  In this case, I think that a COUNT() of votes whose user_id is 123 would do the job — and it doesn't require another join.

Indentation

More meaningful indentation would make the query easier to read.  You don't have to indent it the way I suggest, but you should do something better than nearly no indentation at all.

Suggested solution

SELECT c.id AS comment_id
     , SUM(v.value) AS votesum
     , COUNT(WHEN v.user_id = 123 THEN 1 END) > 0 AS user_voted_already
    FROM comments as c
        LEFT JOIN votes as v
            ON v.foreign_type = 'comment'
            AND v.foreign_id = c.id
    WHERE c.network_id = 3183
    GROUP BY c.id
    ORDER BY votesum DESC;


Finding collisions of the first few bits of a SHA-1 hash My objective is to find a hash collision of my modified hash function. Assuming my modified hash only outputs the first 36 bits of SHA-1. As we know, SHA-1 is a 160-bit hash value, hence, we only need to output 9 characters out of 40 characters for comparison.

How i begin my program is by hashing a string (I have a SHA-1 algorithm running and i will name it as sha1. I have also ensured that the output of the SHA-1 algorithm is correct.)

Firstly, i would hardcode 2 string and extract out 9 characters out of 40 characters since i require only the first 36 bits of SHA-1. The function below basically return a true if a collision is found and false if a collision is not found

public static boolean findCollision(int x1, int x2) {

    String message1 = "I tried " + x1 + " iteration to find a collision";
    String message2 = "I tried " + x2 + " iteration to find a collision";

    //hashing my string and extracting 9 characters
    String message_hash1 = sha1(message1);
    String message_hash2 = sha1(message2);

    String modified_hash1 = message_hash1.substring(0, 9);
    String modified_hash2 = message_hash2.substring(0, 9);

    if (modified_hash1.equals(modified_hash2))
        return true;
    else
        return false;
}

Lastly, i will have a main function that will random Integers up to MAX_VALUE in a infinite loop and will break out if and only if a hash is found.

public static void main(String[] args) {

    Random random = new Random();
    int x1 = 0;
    int x2 = 0;
    int counter = 0;

    while (true) {

        while(true){

            x1 = random.nextInt(Integer.MAX_VALUE);
            x2 = random.nextInt(Integer.MAX_VALUE);

            if (x1 != x2)
                break;
        }

        if (findCollision(x1, x2) == true) {
            break;
        }
        counter++;
    }

    System.out.println("\nNumber of trials: " + counter);
}

If i tried taking only the first 24 bits of SHA-1, i could easily find a collision. However, i'm unable to find a collision for 36 bits instead despite running it for hours. Hence, I'm wondering what is other alternative way for me to find a collision with just 36 bits of SHA-1.

I realized that while typing this, you should look into the Birthday Attack, which will probably much more elaborated than my answer, lol.

Considering your have 36 bits of data, it means you have a total number of possibilities of \$2^{36} = 68719476736\$. Based on the Pigeonhole principle, if you compared the hashed of 68719476736 different strings, you'd get a collision.

So that's easy! All you need to do is run this (pseudocode):

hashset = (new hashset that uses your hashing algorithm)
for(i = 0; i < 68719476736 + 1; i++) {

    if hashset.contains(string(i)) break;

    hashset.add(string(i));
}

print("This took " + i + " tried!")

With this, you're guaranteed to get a collision. But there's a problem. If every iteration took 100 milliseconds, you'd need about 217 years to get a solution. Tell your teacher your great-great-grandchildren will get back to you with a solution. You could also buy 2000 computers to run this in ~40 days.

My algorithm is pretty much guaranteed to finish (assuming none of the computers crash in any way), yours isn't. This is more of a lesson on "how to test things", which is valuable for developers. When you want to test something, try not to be random. The thing with your algorithm is that maybe you wouldn't ever get collisions. I understand that you kind of need randomness if you want this to finish running this one day.

So the question is, what could we do to make this better?

We could check about how many different strings we'd need to get a good enough chance there's a collision. It's kind of the Birthday Problem which gives us an easy way to calculate what are the chances n different persons have the same birthday in a group of m persons.

We can use the formula under Approximation of number of people and adapt it to our problem to understand that if we had 68719476736 + 1 strings in our possession (for example, every number between 1 and 68719476736 + 1), you'd need to pick 308652 of those to have about 50% chances of having a collision.

What you could try is randomly take 308652 numbers between 1 and 68719476736 + 1 and hash them to find a collision. Repeat this as long as you don't have a collision.

The pseudocode would look like this :

generator = RandomBigIntGenerator()
numbers = []

for(i = 0; i < 308652; i++) {
    numbers.add(generator.random(1,68719476736+1))
}

hashset = {} (With your hashing function)
for n in numbers {
    if hashset.contains(n) {
        print("yay done");
        break;
    }
    hashset.add(m);
}

All in all, you can hope to have collisions, but you need computing power.

In a code review point of view :

You need to keep track of the strings you already tried, they give you valuable information that can actually help you find collision.
Don't be too random when you create your strings.
Understand that 68719476736 is a biiggg number, but still so much smaller than the 1461501637330902918203684832716283019655932542976 possible values the SHA1 have :)


Ruby on Rails: refactor big search method I have big search method in my model that corresponds to search a proper Car.

It looks like this:

  def self.search(params)
    cars = joins(:reservations).where.not("reservations.reception_time <= ? AND reservations.return_time >= ?",
      params[:return_date], params[:handover_date])
    cars = joins(:car_class).where("car_classes.id= ?", params[:car_class])
    cars = cars_at_both_locations(params[:handover_location], params[:return_location])
    cars = params[:car_body_style] == [""] ? cars : joins(:car_configuration).
       where("car_configurations.body_style_id = ?", params[:car_body_style])
    cars = params[:car_fuel] == [""] ? cars : where(fuel: params[:car_fuel])
    cars = params[:car_transmission] == [""] ? cars : where(transmission: params[:car_transmission])
    cars = params [:car_seats] == [""] ? cars : car_seats(params[:car_seats])
    cars = Car.joins(:prices).where('prices.to_days >= ?',
      (Date.parse(params[:return_date]) - Date.parse(params[:handover_date])).to_i)
end

It is very unreadable now.

Does anyone have an idea how to refactor this method to be more readable?

Some notes on your code:

As you know, in Rails, params usually refers to query strings in a request. In a model I'd use something like attributes or simply options to avoid confussion.
Variable cars reused multiple times: yes, it's a very usual pattern, but IMO it's not right. I think different values deserve different names.
You have a pattern to abstract here, use inject.

I gave an answer in SO a while ago with exactly this kind of problem, that's the approach I'd recommend: Approach post in SO


Set discounted ticket price, with a floor of 0 Is there a shorter way to write this?

discounted_price = original_ticket_price - discount_value
if discounted_price < 0:
    discounted_price = 0

Use the built-in max() function:

discounted_price = max(0, original_ticket_price - discount_value)


What is wrong with my Genetic Algorithm? I am trying to understand how genetic algorithms work. As with everything, I learn by attempting to write something on my own;however, my knowledge is very limited and I am not sure if I am doing this right.

The purpose of this algorithm is to see how long it will take half of a herd to be infected by a disease if half of that population are already infected. It is just an example I came up with in my head so I am not sure if this would even be a viable example.

Some feedback on how I can improve my knowledge would be nice.

Here is the code:

import random

def disease():
    herd = []
    generations = 0
    pos = 0
    for x in range(100):
        herd.append(random.choice('01'))
    print herd
    same = all(x == herd[0] for x in herd)
    while same == False:
        same = all(x == herd[0] for x in herd)
        for animal in herd:
            try:
                if pos != 0:
                    after = herd[pos+1]
                    before = herd[pos-1]
                    if after == before and after == '1' and before == '1' and animal == '0':
                        print "infection at", pos
                        herd[pos] = '1'
            #print herd
                pos += 1
            except IndexError:
                pass
        pos = 0
        generations += 1
        random.shuffle(herd)
        #print herd
    print "Took",generations,"generations to infect all members of herd."
if __name__ == "__main__":
    disease()

I am looking for comments based on the logic of this algorithm. What can be improved, and what has to be improved to make this a true genetic algorithm if it isn't one already.

your algorithm is not a genetic algorithm, it is a simple model of infection.

genetic algorithm normally uses at least the following concepts: inheritance, mutation, selection (or fitness).
see, e.g. here

if - for the sake of introductory understanding - you want to apply these concepts to your example:

inheritance:
conceptual: you need a genetic identity of your unit (the animal).
code: i.e. you need animals with state, different instances.
how: each animal inherits this genetic identity to the next generation when it is reproduced.
mutation:
conceptual: each generation is slightly different from the other.
code: each animal in a new generation has slightly different values than its parent.
how: apply some random modification.
selection of fitness:
conceptual: you select which units are reproduced for the next generation by the differences between the units. choose the fittest.
code: when assembling the new generation, choose only those units for reproduction that satisfy a certain fitness limit (e.g. the best 90%).
how: define a fitness-function which returns a value indicating relative fitness for each unit.

to put it together, you could make e.g. different herds of animals that have different starting values for the 'genetic' attributes (e.g. preferred_proximity_to_next_animal, skin_color, height, aggressiveness, etc
.).
Then, in your fitness function, you define how each of the attributes is connected to fitness, i.e. how they are related to the probability to get infected. then let the program run for a some of generations and see how at each iteration the global probability of infection decrease in relation to the genetic setup of your herd.

Like that you might find favourable combinations of attributes.
Generally, genetic algorithms are used in search-related problems.


bash script to bulk decode files I have written script that does the following. It uses cbl-log program to decode several log files in binary format. cbl-log only decodes 1 file at a time, so the script decodes files in bulk. If no arguments are given, it assumes log files (with extension .cbllog) are in the current directory, and it'll output files to decoded directory. Otherwise you need to give 2 arguments. The first is the directory with all cbllog files, and the second is the output directory.

#!/usr/bin/env bash

if (( $# == 1 || $# > 2 )); then
    echo "Usage:    cbldecode"
    echo "          cbldecode <source directory> <target directory>"
    exit 1
fi

cmd="$(command -v {./,}cbl-log | head -n1)"
if [[ -z "$cmd" ]] ; then
   echo "cbl-log should be in PATH or current directory"
   exit 1
fi

log_dir=${1:-$PWD}
out_dir=${2:-"decoded"}
count=$(find "$log_dir" -maxdepth 1 -name "*.cbllog" | wc -l)

if (( "$count" == 0 )); then
    echo "no cbllog files in source directory"
    exit 1
fi

if ! [[ -d "$out_dir" ]]; then
    mkdir -p "$out_dir"
elif ! [[ -w "$out_dir" ]]; then
    echo "no write permission to $out_dir"
    exit 1
fi

for path in "$log_dir"/*.cbllog; do
    file=${path//$log_dir/}
    out_file="${file%.cbllog}"
    "$cmd" logcat "$path" "$out_dir"/"$out_file"
done

While this works, I'm looking for ways to make the code more compact and optimize for speed.

Use the -u and -e switches to bash, when possible, because they help to find bugs.  Your log decoder may return errors that you want to ignore (making -e unhelpful) but -u is safe here.

Move the echo … exit pattern into a function:

die() { printf "%s\n" "$@" >&2; exit 1; }

Quotes aren't needed on the left-hand side of [[ tests.  For "die unless foo" assertions, consider writing them in the form foo || die "test failed":

[[ -z $cmd ]] && die "…"

(( $# == 1 || $# > 2 )) is more clearly written in the affirmative, as (( $# == 0 || $# == 2 ))

Collect the file list once instead of twice.  Use globbing instead of find to get proper handling of filenames with spaces in them.  Include contributing variables in error messages.  Do a cd first so that you don't need to decompose filenames later.  (( expressions return false when zero so there's usually no need to explicitly write x == 0:

shopt -s nullglob
pushd "$log_dir" || die "can't chdir to $log_dir"
declare -a in=( *.cbllog )
popd || die "can't return to original working directory"
(( ${#in} )) || die "no cbllog files in $log_dir"
…
for path in "${in[@]}"; do

Your command detector has a bug if cbl-log is an alias or function.  Use type -P instead.  If you truncate using read instead of head, the assignment doubles as an emptyness test:

read cmd < <( type -P {./,}cbl-log ) || die "cbl-log should be in PATH or current directory"

Your basename implementation has a bug if log_dir contains globbing characters.  This is obsoleted by the pushd above but something like file=${path##*/} or file=$( basename "$path" ) would work.

mkdir -p is a no-op when the directory exists; no need to test for that yourself.

You'll need execute permission on $out_dir along with write.

Putting it all together:

    #!/bin/bash -u
    die() { printf "%s\n" "$@" >&2; exit 1; }

    (( ${#} == 0 || ${#} == 2 )) || die "Usage:    cbldecode"  "          cbldecode <source directory> <target directory>"
    log_dir=${1:-$PWD}
    out_dir=${2:-"decoded"}
    read cmd < <( type -P {./,}cbl-log ) || die "cbl-log should be in PATH or current directory"

    pushd "$log_dir" || die "can't chdir to $log_dir"
    shopt -s nullglob
    declare -a in=( *.cbllog )
    popd || die "can't return to original working directory"
    (( ${#in} )) || die "no cbllog files in $log_dir"

    mkdir -p "$out_dir"
    [[ -w $out_dir && -x $out_dir ]] || die "insufficient permissions on $out_dir"

    for basename in "${in[@]}"; do
        "$cmd" logcat "$log_dir/$basename" "$out_dir/${basename%.cbllog}"
    done

For faster conversion, add & to the cbl-log invocation, or adapt the script to send basenames to xargs -P.


Handle Java Process outputs without extra-threads Usually, people would create two extra-threads in order to read the standard output and error respectively. However, the following code would allow to handle a Process outputs without those threads (it is arguably a little bit more difficult to decode the characters if you are using something else than a single-byte encoding and if you can't buffer the output in memory).

Although the following code seems to work, it is different enough from the "standard" solution, that I'm wondering if anyone can find am issue with it.

public static void main(String[] args) throws IOException, InterruptedException {
  ProcessBuilder processBuilder = new ProcessBuilder(args);
  Process process = processBuilder.start();

  InputStream outputStream = null, errorStream = null;
  ByteArrayOutputStream outputBuffer = new ByteArrayOutputStream();
  ByteArrayOutputStream errorBuffer = new ByteArrayOutputStream();
  try {
    outputStream = process.getInputStream();
    errorStream = process.getErrorStream();

    byte[] tmp = new byte[1024];

    while (true) {
      int outputBytes = readAvailablOnce(outputStream, outputBuffer, tmp);
      int errorBytes = readAvailablOnce(errorStream, errorBuffer, tmp);
      if (outputBytes == 0 && errorBytes == 0) {
        try {
          process.exitValue();
          break;
        } catch (IllegalThreadStateException e) {
          // keep on looping
        }
      }
    }
    readAvailableAll(outputStream, outputBuffer, tmp);
    readAvailableAll(errorStream, errorBuffer, tmp);

  } finally {
    closeQuietly(outputStream);
    closeQuietly(errorStream);
  }

  System.out.println(outputBuffer.toString("ASCII"));
  System.err.println(errorBuffer.toString("ASCII"));
  System.err.println("exit code: " + process.exitValue());
}

private static void closeQuietly(InputStream in) {
  if (in != null) {
    try {
      in.close();
    } catch (IOException e) {
      // ignored
    }
  }
}

private static int readAvailablOnce(
  InputStream inputStream, OutputStream outputStream, byte[] buffer)
throws IOException {
  int bytesRead = 0;
  if (inputStream.available() > 0) {
    bytesRead = inputStream.read(buffer);
    outputStream.write(buffer, 0, bytesRead);
  }
  return bytesRead;
}

private static void readAvailableAll(
  InputStream inputStream, OutputStream outputStream, byte[] buffer)
throws IOException {
  if (inputStream.available() > 0) {
    int bytesRead = 0;
    while ((bytesRead = inputStream.read(buffer)) >= 0) {
      outputStream.write(buffer, 0, bytesRead);
    }
  }
}

Is inputStream.available() a blocking method?

The process might send an error message while your are blocked (possibly forever) in the standard output available() waiting for something that will never arrive.


identifying pairwise groups from dictionary I have a dictionary that looks like this

masterdict = {'a':['b','c','d','e'],
         'b':['c','d','e'],
         'c':['d','e'],
         'd':['e']
         'e':[]}

where the list has all the values that are 'compatible' with the key and vice-versa. Since the dictionary is non-redundant, repeated compatiblities are not reported in the dictionary. For eg. 'e' is compatible with 'a','b','c' and 'd' but masterdict['e'] is empty since it is reported in the other lists. I would like to identify all groups of elements, such that all the elements are mutually compatible. Hence for the masterdict given above, the solution would be [['a', 'b', 'c', 'd'], ['a', 'b', 'c', 'd', 'e'], ['a', 'b', 'c', 'e'], ['a', 'b', 'd', 'e'], ['a', 'c', 'd', 'e'], ['b', 'c', 'd', 'e']]. Not showing lists of smaller size.

masterdict = {'a':['b','d','e'],
         'b':['c','d'],
         'c':['e'],
         'd':['e'],
         'e':[] }

then the solution would be [['a', 'b', 'd'], ['a', 'd', 'e']]. I did write a recursive function to do this, but it is slow for larger dictionaries (len(list) > 1000). The code is given below. I would like some suggestions on how to write a faster version, maybe using itertools or some other module. Thanks alot

import sys
import os

def recursive_finder(p1,list1,masterdict,d,ans):
        # if lenght of d is more than 2, store that list in ans
        if len(d) >2 :
                ans.append(d)
        nextlist = []
        # if lenght of list is 0, do not do anythin
        if len(list1) == 0:
                pass
        else:
                other = []
                for i in list1:
                        if i in masterdict[p1]:
                                new = []
                                new.extend(d)
                                new.append(i)
                                other.append(new)
                                nextlist.append(i)
                for i in range(len(nextlist)):
                        p1 = nextlist[i]
                        dnew = other[i]
                        recursive_finder(p1,nextlist[i+1:],masterdict,dnew,ans)

masterdict = {'a':['b','d','e'],
         'b':['c','d'],
         'c':['e'],
         'd':['e'],
         'e':[] }

final = []
ans = []
for mainkey in masterdict:
        if len(masterdict[mainkey]) > 1:
                for i in range(len(masterdict[mainkey])):
                        p1 = masterdict[mainkey][i]
                        recursive_finder(p1,masterdict[mainkey][i+1:],masterdict,[mainkey,p1],ans)

print(ans)

Indentation

You should use four (4) spaces for indentation, not eight (8).

Logic

This

if len(list1) == 0:
    pass
else:

should be changed to

if list:
    ... code here ...

This removes the need for the else, as you instead check if the list contains anything. Check out the Python Docs on Truth Testing.

Enumerate

This

for mainkey in masterdict:
    if len(masterdict[mainkey]) > 1:
        for i in range(len(masterdict[mainkey])):
            p1 = masterdict[mainkey][i]
            recursive_finder(p1,masterdict[mainkey][i+1:],masterdict,[mainkey,p1],ans)

can be changed to this

for mainkey in masterdict:
    if len(masterdict[mainkey]) > 1:
        for i, value in enumerate(masterdict[mainkey]):
            p1 = value
            recursive_finder(p1,masterdict[mainkey][i+1:],masterdict,[mainkey,p1],ans)

Use enumerate since you are using both the index and the value.