Is there any downside to define a function that use one dictionary as an argument instead of several (+10) arguments? I tried to refactoring someone else's code and I found that there is alot of this pattern

def train(a, b, c, d, e, f, g, h, i, j, k): #Real code has longer name and around 10-20 arguments
    x = Dostuff()
    y = DoMoreStuff(b,c,d,e,f,g,h,x)
    z = DoMoreMoreStuff(a,b,d,e,g,h,j,y)

def DoMoreStuff(b,c,d,e,f,g,h):
    DoPrettyStuff(b,c,d,e,f,g,h) # The actually depth of function call is around 3-4 calls

I though that it would it be better to collect all of that a,b,c,d into one dictionary. I got this idea when I was reading about rail rack's middleware.

env = {"a":1, "b", 2.......}
def train(env):
    x = Dostuff()
    y = DoMoreStuff(env.update({"x":x}))
    z = DoMoreMoreStuff(env.update({"y":y}))

def DoMoreStuff(env)
    DoPrettyStuff(env)

But I am not sure if there is any downside about this approach ? If it does help, every function here has a side-effect.

EDIT: For more clarification. I think most of my problem are about the propagation of variables. The call pattern has very deep and wide hierarchy. Any further suggestion about this one ?

def main(a,b,c,d,e,f,g,h,i,j,k,l,m....z):
    Do(...)
    #maybe other function()

def Do(a,b,c,d,e,f,g,h,i,j,k,l):
    Do1(d,e,f,g,h,i,j,k,l)

def Do1(d,e,f,g,h,i,j,k,l)
    Do2(f,g,h,i,j,k,l)

def Do2(f,g,h,i,j,k,l)
    Do3(f,g,h,i)
    Do4(j,k,l)

EDIT 2:  Well, my intention is that I want the code more easier to read and followed. Performance in python code does not matter much since the program spend most of the time on SVM and machine learning stuff. If anyone interest and don't mind a very dirty code that I still working on it. Here is the real code https://github.com/yumyai/TEES/tree/30b3aeaab011e30dcdc3c1151062697861ddcae1

The code start with train.py (training model), classify.py (classify using model). In this case I'll use train.py as an example.
https://github.com/yumyai/TEES/blob/30b3aeaab011e30dcdc3c1151062697861ddcae1/train.py: line 136 where it start to call everything under it and pass a gigantic arguments to. These arguments are pass down to https://github.com/yumyai/TEES/blob/30b3aeaab011e30dcdc3c1151062697861ddcae1/Detectors/EventDetector.py: line 49. And then EventDetector pass these argument to it members (line 24-28).

Some argument is reused throughout the code, for example, def train(.......exampleStyles). exampleStyles is passed around to several member that is called by train.

Well, the answer depends on the meaning of the variables. If they are related (and they should be, since the same functions uses them!), then it's a good idea to group them in a dictionary since it makes the code more readable.

However, even if it makes sense to group all variables into a dict, your env.update() solution is not satisfying because it is unlikely that x and y really belong to env. What have you gained here? Function definitions may be shortened, but you only moved the problem to make it less visible. The next developer will be very surprised to see that you simply replaced function arguments by a dict without thinking about readability. This does not respect the Principle of least astonishment and probably others too.

You should try to group variables that really belong together, make use of *args and **kwargs whenever it makes sense, and otherwise split/reorganize functions that take too much unrelated arguments: they probably break the Single responsibility principe anyway.

Now, be careful when refactoring such functions, make sure you really understand them, and write unit tests or integration tests before changing them to make sure you don't make the code worse than before.


RAII and code duplication in constructor/destructor I have a class hierarchy similar to this:

struct VideoCapture { virtual open(const string& uri) = 0; virtual close() = 0; };
struct VideoCapture1 : public VideoCapture {...};
struct VideoCapture2 : public VideoCapture {...};
struct VideoCapture3 : public VideoCapture {...};

I added the following constructor and destructor in each of these classes:

VideoCaptureN(const string& uri) { open(uri); }
~VideoCaptureN() { if (!isOpen()) { close(); } }

Though, providing RAII this did not pass the code review as I had to duplicate the same exact code in every classes.

What would be the solution to get RAII and avoid code duplication?

You can't call virtual functions in constructors and should not call them in destructors (you may have already destroyed state that the virtual function relies on).

You can use the PIMPl pattern to help you out though (like the c++ stream, they use a wrapper class and a buffer class that does the work).

class VidCaptureInterface
{
    public:
    virtual ~VidCaptureInterface() {}
    virtual open(string const& uri) = 0;
    virtual close()                 = 0;
};

// When C++17 comes along then
// You may be able to force T to be a VidCaptureInterface
// This is one solution. But you basically want interface
// to be derived from `VidCaptureInterface`. In C++ stream
// the buffer is held as a pointer and can be re-assigned
// So alternatives include using some form of re-assignable pointer.
template<typename T>
class VideoCapture
{
     T   interface;
     public:
         VideoCapture(std::string const& uri)
         {
             interface.open(uri);
         }
         ~VideoCapture()
         {
             if (isOpen())
             {
                 interface.close();
             }
         }
};

typedef VideoCapture<VidCaptureInterface1>  VideoCapture1;
typedef VideoCapture<VidCaptureInterface2>  VideoCapture2;
typedef VideoCapture<VidCaptureInterface3>  VideoCapture3;


The quicksort algorithm in Haskell I am learning Haskell programming language mainly from this source. And there I have encouraged with "an elegant" realization of the quicksort sorting algorithm (the Quick, sort! section). Here it is:
Book's implementation
quicksort :: ( Ord a ) = > [ a ] -> [ a ]
quicksort [] = []
quicksort ( x : xs ) =
    let smallerSorted = quicksort [ a | a <- xs , a <= x ]
        biggerSorted = quicksort [ a | a <- xs , a > x ]
    in smallerSorted ++ [ x ] ++ biggerSorted

The problem with this implementation (as I think) is doubled number of comparisons inside the let binding every time. Isn't the fact that those of a's that already in the smallerSorted list cannot be in the biggerSorted list, so we don't need to compare x with them anymore.
In order to "improve" the above approach I have written my own implementation, where I use auxiliary local function split that splits a list into two parts: less than (or equal to) x and greater than x.
My implementation
quick_sort :: (Ord a) => [a] -> [a]
quick_sort []       = [] --edge condition
quick_sort (x : xs) =    --general condition
    let (lt, gt) = split x xs
    in  (quick_sort lt) ++ [x] ++ (quick_sort gt)
    where
        --split function is used to split a list
        --into two sublists: one for elements
        --less or equal (lt) than some value - x
        --and one for those that greater (gt) than x
        --NOTE: split function is also recursive
        split x [] = ([], []) --edge condition
        split x (h : hs)      --general condition
            | h <= x    =
                let (lt, gt) = split x hs
                in ( (h : lt), gt )
            | otherwise =
                let (lt, gt) = split x hs
                in ( lt, (h : gt) )

P.S.
For a while I am not able to compare two approaches, but on the worst cases (when list to sort is already sorted) it seems that my implementation is a bit slower.

First of all, great that you used type annotations, as they're sometimes missing. However, there are some remarks:

"edge" and "general"` condition are usually called "base case" and "recursive case"
"base case" and "recursive case" aren't commented in Haskell code, as they are everywhere
split is generic enough to be defined at top-level
split has a little bit of duplication that we can remove with a single where clause
functions are usually named in camelCase instead of snake_case, so we'd call it quickSort.

If we apply that we end up with

quickSort :: (Ord a) => [a] -> [a]
quickSort []       = []
quickSort (x : xs) =
    let (lt, gt) = split x xs
    in  (quickSort lt) ++ [x] ++ (quickSort gt)

split :: (Ord a) => a -> [a] -> ([a], [a])
split x [] = ([], [])
split x (h : hs)
    | h <= x    = ( h : lt,     gt)
    | otherwise = (     lt, h : gt)
    where
        (lt, gt) = split x hs

That being said, split seems so helpful that there should be some function already in the standard library. And it turns out there is, namely partition :: (a -> Bool) -> [a] -> ([a],[a]) from Data.List:

split :: (Ord a) => a -> [a] -> ([a], [a])
split x = partition (<= x)

That's a lot cleaner than our own implementation. So how do we find partition if we don't know about it yet? We use Hoogle. However, a query for Ord a => a -> [a] -> ([a],[a]) does not yield partition. It's sometimes a good idea to generalize the query a little bit, for example to [a] -> ([a],[a]), as Hoogle also scans parts of the type signature. That way, we find partition.

If we're going for brevity, we can then inline split to gain

-- | Sorts the given list in an ascending order.
quickSort :: (Ord a) => [a] -> [a]
quickSort []       = []
quickSort (x : xs) =
    let (lt, gt) = partition (<= x) xs
    in  (quickSort lt) ++ [x] ++ (quickSort gt)

but that's up to personal preference, as the compiler will inline split anyway.


Generic Min Heap Implementation in Java There is a Min-Heap implementation in Java. BubbleUp() and BubbleDown() could have been coded using recursion but I prefer without recursion. Would there be a major difference if had used recursion?

package heap;

import interfaces.queue.PriorityQueue;
import java.util.Arrays;
import java.util.stream.Collectors;

public class Heap<T extends Comparable<T>> implements
PriorityQueue<T>
{
    private T[] elements;
    private int size;
    private int capacity;

    public Heap()
    {
        this(500);
    }

    public Heap(int capacity)
    {
        this.capacity = capacity;
        size = 0;
        elements = (T[]) new Comparable[this.capacity];
    }

    public int size()
    {
        return size;
    }

    public boolean isEmpty()
    {
        return size() == 0;
    }

    @Override
    public void add(T data) throws Exception
    {
        if(size() >= capacity) throw new Exception("Heap is full");
        elements[size++] = data;
        bubbleUp();
    }

    private void bubbleUp()
    {
        int child = size() - 1;
        int parent = (child-1)/2;

        while(parent >= 0 && elements[child].compareTo(elements[parent]) < 0)
        {
            swap(child, parent);
            child = parent;
            parent = (child-1)/2;
        }
    }

    @Override
    public T removeMin() throws Exception
    {
        if(isEmpty()) throw new Exception("Empty heap");
        T root = elements[0];
        swap(size()-1, 0);
        elements[size()-1] = null;
        size--;
        bubbleDown();
        return root;
    }

    private void bubbleDown()
    {
        int parent = 0;
        int leftChild = 2*parent + 1;
        int rightChild = 2*parent + 2;

        int choice = compareAndPick(leftChild, rightChild);

        while(choice != -1)
        {
            swap(choice, parent);
            parent = choice;
            choice = compareAndPick(2*choice+1, 2*choice+2);
        }
    }

    private int compareAndPick(int leftChild, int rightChild)
    {
        if(leftChild >= capacity || elements[leftChild] == null) return -1;
       if((elements[leftChild].compareTo(elements[rightChild]) <= 0)|| (elements[rightChild] == null))
            return leftChild;
        return rightChild;
    }

    private void swap(int child, int parent)
    {
        T t = elements[child];
        elements[child] = elements[parent];
        elements[parent] = t;
    }

    @Override
    public String toString()
    {
         return Arrays.stream(elements)
                 .filter(element -> element != null)
                 .collect(Collectors.toList()).toString();
    }
}

Exception handling

Throwing Exception is not recommended because it's too generic.
It doesn't give a clue to the caller as to what went wrong.
It's recommended to use specific exceptions.

When implementing a collection abstract data type, it's good to take a look at a similar interface in the standard library, for example Deque:

How do they handle when an element cannot be added due to capacity restrictions? They throw IllegalArgumentException.
How do they handle when an element is requested but the collection is empty? They throw NoSuchElementException.

As you see, suitable specific exceptions already exist.
Also notice that these exceptions are unchecked.
It means that callers of these methods don't have to catch them.
And that seems an appropriate decision,
since the situations in which these exceptions can be thrown are quite unexpected, and should not happen under normal circumstances.

Redundant capacity variable

The member variable capacity is redundant.
The same information already exists in elements.length.

Make members final when possible

Since elements is never reassigned, it would be good to make it final,
so that you cannot reassign by mistake.

Overriding toString

Keep in mind that toString is not intended for "pretty-printing".

And for printing the content of the heap,
this implementation doesn't look useful to me.
With the null values removed,
the structure of the heap is not visible,
and without the structure, the ordering of the elements is meaningless,
which can be misleading.

For printing the content of the heap I would suggest adding a dedicated method,
keep the nulls, and print values of the first size elements.


Abstract Data Type: Stack implementation in C++ While trying to get better at C++ I tried my hand at making a stack implementation. This is the result:

#include<iostream>

template<typename T>
struct Node {
    T data;
    Node *next;
    Node(T data, Node *next) : data(data), next(next) {}
};

template<typename T>
class Stack {
    public:
        Stack() : top(nullptr), level(0), max_size(10) {}
        Stack(int max_size) : top(nullptr), level(0), max_size(max_size) {};
        void push(T data);
        T pop();
        void display();
        bool isEmpty() { return level == 0; }
        bool isFull() { return level == max_size; }
        int geLevel() { return level; }
        int getMaxSize() { return max_size; }

    private:
        Node<T> *top;
        int level, max_size;
};

template<typename T>
void Stack<T>::push(T data) {
    if(!isFull()) {
        Node<T> *newNode = new Node<T>(data, top);
        top = newNode;
        level++;
    } else {
        std::cerr << "stack overflow";
    }

}

template<typename T>
T Stack<T>::pop() {
    if(!isEmpty()) {
        Node<T> *temp = top;
        T data = temp->data;
        top = top->next;
        level--;
        delete temp;
        return data;
    }

    std::cerr << "stack underflow";

}

template<typename T>
void Stack<T>::display() {
    Node<T> *curr = top;
    while(curr != nullptr) {
        std::cout << curr->data << std::endl;
        curr = curr->next;
    }
}

Any help would be appreciated to make this code mor C++'ish.

It seems a stack is the HelloWorld of C++ classes and memory management :).

First, your class has no destructors. It means that when your instance will be destroyed, it will only deallocate the stack attributes : top, level and max_size. Notice that it deallocate the pointer top, but not the object pointed by top ! You must define a destructor : ~Stack(), which will likely consists in a loop executing pop() while the stack is not empty.

Remember that when you are encapsulating pointers, you must respect the Rule of Three (known also as Rule of Five) : it is a simple but accurate rule which can be interpreted as :

  If you need to specify one of destructor/Copy Constructor/Copy Assignment Op; then you probably have to define all three.

Here, it applies because you are holding a pointer, whose value is deleted in the destructor. So if you do the default copy, for instance Stack<T> s2 = s1;, it will copy the data members of s1 into s2. It means that s2.top == s1.top. So when one get destroyed, for instance s1, it will delete s1.top, but then s2.top points to deleted memory : ERROR.

While you are defining copy-constructor and copy-assignment, you may want to define move-constructor Stack(Stack&& other) and move-assignment Stack& operator=(Stack&& other). T&& means "Movable T" and signifies "This value is temporary, so there is no problem if it is altered". For example, here, it means you can only swap other.top and this->top, so you do not need to copy each node of the stack.
Both move-constructor and move-assignment are parts of the Rule of Five, which is an extent of the Rule of Three above.

You use std::cerr to showing errors. It is not the right way to do this : for example, if push() results in an error, the code calling push has no way to know it. The program will continue to run as usual.

C++ has a specific way to handle errors : exceptions. I will not detail here but follow the link and give it a try.

Moreover, in your pop() function, there is a problem. If the stack is empty, there is no return value ! Your compiler should at least warn you (if not yelling at you) that one conditional-branch has no return value. Instead, you should throw an exception, or, if it is intended, use std::optional<T>. std::optional can represent two states : empty, or full. If it is full, you can access to T value.

Do you know the keyword const in method declaration ? It means it will not modify the object during its execution. Examples :

bool method_A() const; //declaration
int method_B() const { /* definition */ }

You need the keyword const in both declaration and definition. Currently, the functions which can be marked const are display(), isEmpty(), isFull(), getLevel() and getMaxSize().

One of the purpose is the const-ref argument. For example :

template<typename T>
void printLevel(Stack<T> const& stack) {
    std::cout << "Nb levels = " << stack.getLevel() << std::endl;
}

The argument type is a const-ref (one & means reference). It means that, because it is a reference, the argument is not copied, but you can not modify it : "read-only" (because of const). Having a const-ref variable means you can only access to const methods.

Also, you can optimize memory management by using smart pointers. I will not explain this here, but please check out my answer to another stack implementation, which details how can smart pointers be used.

Something you should do is defining max_size constant :

public:
    const int max_size;

Notice that this is ok to set max_size public, because it is a constant and it won't be modified by external code.


Clojure "Game of Life" I'm a Clojure beginner and I implemented Game of life to learn how I should code things in Clojure.

(def state (ref [
    [false false false false false]
    [false false true  false false]
    [false false true  false false]
    [false false true  false false]
    [false false false false false]]))

(defn get-state [state x y]
  (nth (nth state y) x))

(defn count-true-neighbours [state x y]
  (let [size (count state)]
    (->>
      (for [p (range (- x 1) (+ x 2))
            q (range (- y 1) (+ y 2))
            :when (not (and (= x p) (= y q)))]
        [p q])
      (map (fn [coord]
        (let [[p q] coord]
          (if (or (< p 0) (>= p size) (< q 0) (>= q size))
            false
            (get-state state p q)))))
      (filter true?)
      (count))))

(defn next-state-of [state x y]
  (let [alive (get-state state x y)
        alive-neighbours (count-true-neighbours state x y)]
    (cond
      (and (not alive) (= alive-neighbours 3)) true
      (and alive (or (= alive-neighbours 2) (= alive-neighbours 3))) true
      :else false)))

(defn next-state [state]
  (let [size (count state)]
    (mapv
      (fn [row]
        (mapv
          #(next-state-of state %1 row)
          (range 0 size)))
      (range 0 size))))

(defn pretty-print [state]
  (let [size (count state)]
    (loop [i 0]
      (if (< i size)
        (do
          (println
            (->>
              (nth state i)
              (mapv #(if %1 "■ " "□ "))
              (reduce str)))
          (recur (inc i)))))))

(defn tick
  ([times] (tick 0 times))
  ([i times]
    (if (< i times)
      (do
        (dosync
          (println (str i ":"))
          (pretty-print @state)
          (ref-set state (next-state @state)))
        (recur (inc i) times))
      nil)))

(defn -main [& args]
  (tick 5))

I used ref to learn how to manage state although it seems like an overkill for this tiny program.

As you see my code is terrible, but I couldn't improve this by myself.

I already checked this answer, but I think I did this in a quite different way and need a different advice.

Your code actually isn't terrible, but there are a few places where it can be improved.

First, the ref. It's entirely unnecessary in this case. What I'd change to keep everything immutable:

Change your declaration at the top to just:

(def initial-state [[false false false false false]
                    [false false true  false false]
                    [false false true  false false]
                    [false false true  false false]
                    [false false false false false]])

This now defines, as the name suggests, the initial state of the game. This will no longer be used to store the running state.

Now, you can change tick to get rid of the need for ref-set:

(defn tick [state times]
  (loop [i 0 ; Moved the counter into a loop
         acc-state state] ; Keeping track of the running state
   (when (< i times)
     (println (str i ":"))
     (pretty-print acc-state)

     ; Recur into the loop instead of the function itself
     (recur (inc i) (next-state acc-state)))))

(defn -main [& args]
  (tick initial-state 5))

Note how I just made the running state an accumulator of a loop. I think the whole design should be rethought though. Really, you could just get the successive states by using iterate, and take as many as you want. Lets say you want to get the first five states after the initial-state:

(take 5 (iterate next-state initial-state))

Now, your entire tick function can basically be written as:

; Obviously, change 5 to however many states you want to produce
(doseq [state (take 5 (iterate next-state initial-state))]
  (pretty-print state))

I would change pretty-print to a format style function that returns a string instead of printing directly. That way, the caller can make use of your prettying, but decide how/when they want to display it.

Your count-true-neighbors function is huge. I'd break it down into a few discreet parts:

You have a part that produces a list of neighbors to check
You have a part that does bounds checking

I'd change it to something more like:

(defn neighbors-of [x y]
  (for [p (range (- x 1) (+ x 2))
        q (range (- y 1) (+ y 2))
        :when (not (and (= x p) (= y q)))]
    [p q]))

(defn inbounds? [x y size]
  (and (<= 0 x (dec size)) ; Comparison operators can be chained!
       (<= 0 y (dec size))))

(defn count-true-neighbours [state x y]
  (let [size (count state)]
    (->> (neighbors-of x y)
      (map (fn [[p q]] ; You can deconstruct here directly
             (when (inbounds? p q size)
               (get-state state p q))))
      (filter true?)
      (count))))

Your mapping function could be changed to:

(map (fn [[p q]]
       (and (inbounds? p q size)
            (get-state state p q))))

Although the benefit here is subjective.

Things to note:

<= and other comparison operators can be used to check more than two arguments! I'm making use of that in inbounds? to neaten it up a bit.
I reduced your bounds check in the map down to (when (inbounds? p q size) ...). when returns nil (falsey) if the condition fails, so you don't need to explicitly return false.

(or (= alive-neighbours 2) (= alive-neighbours 3)))

Can also be written as:

(#{2 3} alive-neighbors)

It's a matter of style which you prefer. I prefer the latter personally.

(nth (nth state y) x))

Can also be written as:

(get-in state [y x])

There's a few things to note in pretty-print:

(reduce str strings)

Isn't wrong, but a more idiomatic way of writing it would be:

(apply str strings)

With:

(mapv #(if %1 "■ " "□ "))

You add explicit spaces after the display characters. It may be cleaner to use join to add the spaces. This limits the repetition, and gets rid of the trailing space. This also lets you just use the character literals directly.

And when you use threading macros (->>), you put the expression being threaded on the next line. Personally, I prefer to put the threaded expression on the same line as the ->>. This makes it clearer what's being threaded.

Taking that all together, I'd write that whole bit as:

(->> (nth state i)
     (mapv #(if %1 \■ \□))
     (s/join " ")
     (apply str)))

Where s is an alias for clojure.string.


Do I always have to return something in an anonymous function? I have inherited this snippet of jQuery JavaScript and am currently brushing up on my jQuery. NetBeans IDE complains that Anonymous function does not always return a value. So there is not always an explicit exit point.

I am aware that the $() shortcut is not being used. This is because it is running within a WordPress environment and needs to be in no-conflict mode.

function init() {

    jQuery("#post").submit(function(_e) {
        // Make sure question is supplied
        var contents;
        if(window.tinyMCE && document.getElementById("content").style.display=="none") { // If visual mode is activated.
            contents = tinyMCE.get("content").getContent();
        } else {
            contents = document.getElementById("content").value;
        }

        if(!contents) {
            alert(msg_enter_question);
            _e.preventDefault();
            _e.stopPropagation();
            return true;
        }

        // We must have atleast 2 answers.
        var answer_count = 0
        jQuery(".answer").each(function() {
            if(this.value) answer_count++;
        });
        //if(answer_count < 2) {
        //  alert("<?php //_e("Please enter atleast two answers"); ?>");
        //  _e.preventDefault();
        //  _e.stopPropagation();
        //  return true;
        //}

        //A correct answer must be selected.
        var correct_answer_selected = false;
        jQuery(".correct_answer").each( function(_e) {
            if(this.checked) {
                correct_answer_selected = true;
                return true;
            }
        });
        if(!correct_answer_selected) {
            alert(msg_correct_answer);
            _e.preventDefault();
            _e.stopPropagation();

        }
    });
}
jQuery(document).ready(init);

My question is - do I always have to return something in an anonymous function? I.e. is NetBeans right to complain?

While I understand how it may seem a pointless little nitpick for such simple code to have to have a return something from all code paths if it has one from any, pretty much every thing that a reviewer could say something about can be seen the same way. Individually the rules like the one Netbeans wants to enforce here don't do much, but when all followed together they make for much more maintainable code (especially after letting it sit there until you have forgotten about it for 16 months).

Further recommendations

Wrap the code in an IIFE (and add 'use strict';)

On top of other things this allows you to use a $ for jQuery here and not worry about messing with any conflict issues.
There is no reason to name the init function that you are just passing to $.ready; inline it.
Cache the jQuery elements.
Pass in global variables used to the IIFE (window, alert, msg_enter_question, msg_correct_answer).
document.getElementById("content").style.display has too many . I'd rewrite it as $content.css('display') after caching the element. Better still:

!$content.is(':visible')

_e is an awful variable name; that underscore adds nothing to it but makes it more annoying to type. Rename to e.
This:

var answer_count = 0
$(".answer").each(function() {
    if(this.value) answer_count++;
});

is better written as:

var answer_count = $answers.filter(function () { return this.value !== ''; }).length;

But it is even better to be commented out or otherwise removed since you aren't using it anywhere.
Similarly:

var correct_answer_selected = false;
$(".correct_answer").each( function(_e) {
    if(this.checked) {
        correct_answer_selected = true;
        return true;
    }
});

should be:

var correct_answer_selected = $correctAnswers.is(':checked');

Move variables to the top of their respective functions (personal preference in agreement with JSLint).
Returning false from a handler is equivalent to calling both .preventDefault() and .stopPropagation() on the event object. Thus:

if(!contents) {
    alert(msg_enter_question);
    e.preventDefault();
    e.stopPropagation();
    return true;
}

may as well be:

if(!contents) {
    alert(msg_enter_question);
    return false;
}

Most obvious there is that the return true statement is exactly not what is actually happening and thus is wrong. Since I would prefer to be explicit I would probably do this:

if(!contents) {
    alert(msg_enter_question);
    e.preventDefault();
    e.stopPropagation();
    return false;
}

As this same if statement is used twice (with different values) I would prefer to pull it out into a function:

function check(item, e, msg) {
    if (!item) {
        alert(msg);
        e.preventDefault();
        e.stopPropagation();
        return false;
    }
    return true;
}

This leaves:

    //...
    if(!check(contents, e, msg_enter_question)) {
        return false;
    }
    correct_answer_selected = $correctAnswers.is(':checked');
    check(correct_answer_selected, e, msg_correct_answer);
}

The problem of not all paths returning something is still here, so change the last line to return check....
If you inline correct_answer_selected then the pattern if (!x) { return false; } return y; logic can be replaced with return x && y:

return check(contents, e, msg_enter_question) &&
    check($correctAnswers.is(':checked'), e, msg_correct_answer);

The Full Monty then becomes (after some very minor spacing changes to pass jslint):

(function ($, window, alert, msg_enter_question, msg_correct_answer) {
    'use strict';

    $(function () {
        var $correctAnswers = $(".correct_answer"),
            $content = $('#content');

        function check(item, e, msg) {
            if (!item) {
                alert(msg);
                e.preventDefault();
                e.stopPropagation();
                return false;
            }
            return true;
        }
        $("#post").submit(function (e) {
            // Make sure question is supplied
            var contents;
            if (window.tinyMCE && !$content.is(':visible')) { // If visual mode is activated.
                contents = window.tinyMCE.get("content").getContent();
            } else {
                contents = $content.val();
            }

            return check(contents, e, msg_enter_question) &&
                check($correctAnswers.is(':checked'), e, msg_correct_answer); //A correct answer must be selected.
        });
    });
}(jQuery, window, alert, msg_enter_question, msg_correct_answer));


Extending prototype of given instance from factory method I created a rubber ball by extending instance of b2Body prototype of Box2D.js. I get the instance from factory method b2World#CreateBody.

var ball = world.CreateBody(bodyDef);

I extended the instance in the constructor of my RubberBall prototype. In result, I made the constructor very long.

function RubberBall(position) {
  var ball = world.CreateBody(bodyDef);
  :
  :
  very long implementation to extend `ball` to make it a rubber ball
}

How can I extract the extending ball part and make the constructor simple?

http://jsfiddle.net/suzuki/XXmeR/

Prototypal Inheritance
Now, the very first problem you'll face in your implementation is memory usage. Each RubberBall instance will be creating an internal function - that's a bad thing:
function RubberBall(){
  this.someFn = function(){...}
}

In this sample code above, each RubberBall instance will create its own someFn. Functions are also objects in JS and will take up memory.
The proper way to do it in JS is to place those methods in the prototype, so that they are shared in all instances of RubberBall. This is what we call prototypal inheritance Do note that we are only placing/sharing the methods. Instance data remains in the instance:
function RubberBall(){
  // ballData gets created *per instance*
  this.ballData = 'I am bouncy';
}

// Assign a prototype object, shared accross all instances of RubberBall
RubberBall.prototype = {
  getData : function(){
    // If called as `[instance].getData()`, `this` is the instance.
    return this.ballData;
  }
}

As far as I've read (but I may be wrong), the V8 engine optimizes objects via hidden classes, and are more optimized if you declare all instance properties in advance. That way, it does not do some "run-time rebuilding". So have your rubber ball properties declared in advance.
Now, you wanted to extend Box2D's b2Body (I'm not well versed in Box2D so pardon my vocabulary). The previous example does not yet inherit it. So [we use the more proper Object.create][2] to create an object that has b2Body in its prototype. Let's modify the previous example:
function RubberBall(){
  // Attach to this instance all properties of b2Body
  b2Body.call(this);

  this.ballData = 'I am bouncy';
}

// Now create our prototype object, with its prototype pointing to b2Body's prototype
RubberBall.prototype = Object.create(b2Body.prototype);

// Then attach methods to it
RubberBall.prototype.getData = function(){...}
RubberBall.prototype.anotherSharedMethod = function(){...}

Private members
JS wasn't designed to have private members, but there are a lot of ways to emulate it (and I think ES6 has something more formal for it). You can create closures, which are fast, but if not handled properly, might eat memory.
So let's stick to prototypal inheritance and public members. If you need to indicate that it's private, by convention you can prefix the property or method with a _.
requestAnimationFrame
Modern browsers are now equipped with requestAnimationFrame which is a special timer that executes a given function as fast as possible, aiming 60fps. It's also optimized to throttle it's speed in different situations, like slowing down when the tab is not focused to save battery.
There are polyfills that automatically fill in the gap, especially for older browsers that don't have it.
Standard Reminders

Don't use globals
Place everything you do in a namespace or an enclosed scope
Keep your code short and readable
Use proper indentation


Min-Heap in JavaScript As part of my study on fundamentals, I have implemented the following code in JavaScript for a MinHeap data structure.

Please have a look and I look forward to your feedback for any improvements.

const swap = require('../swap');

function Heap(){
  this.heap = [];
}

Heap.prototype.getParent = function(index){
  return (index-1)/2;
}

Heap.prototype.getLeft = function(index){
  return (2*index)+1;
}

Heap.prototype.getRight = function(index){
  return (2*index)+2;
}

//inserts a new value to the min-heap
Heap.prototype.insert = function(value){
  this.heap.push(value);
  this.heapifyUp(this.heap.length-1);
  return;
}

//deletes any node at given index
Heap.prototype.delete = function(index){
  if(index === 0){
    this.heap[0] = this.heap[this.heap.length-1];
    this.heap.splice(this.heap.length-1, 1);
    this.heapifyDown(0);
    return;
  }

  this.heap[index] = this.heap[this.heap.length-1];
  this.heap.splice(this.heap.length-1, 1);
  var parent = this.getParent(index);
  (this.heap[index]<this.heap[parent])? this.heapifyUp(index) : this.heapifyDown(index);
  return;
}

//extracts min value and recalibrates minheap
Heap.prototype.extractMin = function(){
  var temp = this.heap[0];

  swap(this.heap, 0, (this.heap.length-1));
  this.heap.slice((this.heap.length-1), 1);
  this.heapifyDown(0);

  return temp;
}

//decreases the value of node at given index by given value
Heap.prototype.decrease = function(index, value){

  if((this.heap[index]-value)<0) return false;

  this.heap[index] = this.heap[index]-value;
  (index === 0) ? this.heapifyDown(0) : this.heapifyUp(index);
  return;
}

Heap.prototype.heapifyUp = function(index){

  var parent = this.getParent(index);

  while(parent>=0 && this.heap[parent]>this.heap[index]){
    swap(this.heap, parent, index);
    index = parent;
    parent = this.getParent(index);
  }

}

Heap.prototype.heapifyDown = function(index){

  var left = this.getLeft(index);
  var right = this.getRight(index);

  while(this.heap[index]>this.heap[left] || this.heap[index]>this.heap[right]){
    if(this.heap[index]>this.heap[left]){
      swap(this.heap, left, index);
      index = left;
      left = this.getLeft(index);
    }
    if(this.heap[index]>this.heap[right]){
      swap(this.heap, right, index);
      index = right;
      right = this.getLeft(index);
    }
  }

}

The code seems good to me. You have one bug and some unnecessary code.

First of all, it's not clear what types of values can be stored in the heap. It looks like it's supposed to be positive numbers, but the only place you check for this is in the decrease-key function. But there's actually not anything stopping callers from attempting to store strings, negative numbers, classes, etc. This will lead to bizarre behavior. I think you need to validate the input and make sure only the right type of data is going in.
It's not necessary to end your functions with return
The line

(this.heap[index]<this.heap[parent])? this.heapifyUp(index) : this.heapifyDown(index);

should be written as a normal if, not a ternary.
This line

this.heap.slice((this.heap.length-1), 1);

is repeated several times; perhaps it should be brought into its own helper method with an explanatory name (suggest deleteLastElement()).
In extractMin(), this line:

swap(this.heap, 0, (this.heap.length-1));

performs unnecessary work. The last element is about to be deleted, so you don't need to put anything there. Just say

this.heap[0]=this.heap[this.heap.length-1]`

A conceptual problem is that you only allow accessing nodes by index. How is someone using your code supposed to know what the index is? An item could move around the heap a lot due to inserts and removals and heapify calls. This applies to decrease() and delete()
heapifyDown() has a bug. Consider the heap represented by [4,6,5,7] and what happens if you extractMin().
decreaseKey() has a bug, but I'm not going to find this one for you.


Maximum depth or height of a Binary tree The Following function is supposed to calculate the maximum depth or height of a Binary tree
        the number of nodes along the longest path from the root node down to the farthest leaf node.

please comment as if it was a 15 minute coding interview. coding style and efficiency.

this was given to you

public class BinaryTreeNode
{
    public BinaryTreeNode Left { get; set; }
    public BinaryTreeNode Right { get; set; }
    public int Value { get; set; }

}

please comment on the unit tests as well.

using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace TreeQuestions
{
    [TestClass]
    public class MaxTreeDepth
    {
        //     0
        //   1    2
        //  3
        [TestMethod]
        public void GetMaxTreeDepthTest()
        {
            BinaryTreeNode root = new BinaryTreeNode
            {
                Value = 0,
                Left = new BinaryTreeNode {Value = 1},
                Right = new BinaryTreeNode {Value = 2}
            };
            root.Left.Right = new BinaryTreeNode {Value = 3};

            int result = GetMaxTreeDepth(root);
            Assert.AreEqual(3,result);
        }

        //     0

        [TestMethod]
        public void GetMaxTreeDepthTestOneNode()
        {
            BinaryTreeNode root = new BinaryTreeNode
            {
                Value = 0,
            };

            int result = GetMaxTreeDepth(root);
            Assert.AreEqual(1, result);
        }

        [TestMethod]
        [ExpectedException(typeof(ArgumentNullException))]
        public void GetMaxTreeDepthTestNull()
        {
            BinaryTreeNode root = null;
            int result = GetMaxTreeDepth(root);
        }

        private int GetMaxTreeDepth(BinaryTreeNode root)
        {
            if (root == null)
            {
                throw new ArgumentNullException();
            }
            int depth = 0;
            return GetMaxHelper(root, depth);
        }

        private int GetMaxHelper(BinaryTreeNode root, int depth)
        {
            if (root == null)
            {
                return depth;
            }

            return Math.Max( GetMaxHelper(root.Right, depth+1),  GetMaxHelper(root.Left, depth+1));
        }
    }
}

(I won't comment on the prescribed BinaryTreeNode definition)

API

Both GetMaxTreeDepth and GetMaxHelper can, and arguably should, be static, as they have no logical dependencies. GetMaxTreeDepth should of course be in some way public (i.e. expose an API), and GetMaxHelper should remain private/internal as an implementation detail.

There is little documentation here, and inline documentation (///) of public APIs is invaluable for discoverability and usability. In particular, I'd expect the throw-on-null documented, and a precise statement of the return value (e.g. I might expect that null would return 0, and a tree with only one node and no children return 1, but I'd be wrong). If I was feeling helpful, I might add note clarifying that it is a tree search, and that passing a graph with cycles is not supported (it will eventually crash with a stack-overflow); it is stated that you are processing a binary tree, but the definition you are given is clearly not going to prevent anyone building a graph.

I don't know if it was clarified in the question statement, but throwing on null is a design decision, and isn't something you can assume as an implementer (certainly not without documentation to go with the assumption). This is also not the most helpful exception ever:

throw new ArgumentNullException();

Which argument? Granted there is only one, but this could have bubbled up from an internal method which takes some parameter which is a function of root. Everyone will be happier if you tell them exactly how it was they violated your API's contract:

throw new ArgumentNullException(nameof(root), "Ideally a helpful message here, unless it's just going to state the obvious and create confusion");

Recursion

Recursion is always dodgy*: you don't know where you might end up, and it could well be a stack overflow. As a 'quick and easy' implementation this looks fine (and is easy to understand), but for production code I would consider an iterative solution using an explicit stack, as this can be considerably more space efficient and cope with much deeper trees. A reasonable iterative solution will almost certainly also be faster for larger trees.

GetMaxHelper

I'd break that last line of GetMaxHelper out a bit, making it clearer and easy to read in a narrow window. I'd be inclined to change root to node or something less important sounding also, since root sounds like it should be at depth 0 (or 1?), and these guys are not necessarily so.

int leftDepth = GetMaxHelper(node.Left, depth + 1);
int rightDepth = GetMaxHelper(node.Right, depth + 1);
return Math.Max(leftDepth, rightDepth);

We could probably argue all day about where the depth++ bit should go, so I won't.

Tests

It's good that you are testing boundary conditions, but you really need more comprehensive tests than this (I guess this is where the 15min time-limit shows up). An implementation where you 'forgot' to look down the right-hand branches, or made a C&P error (like I did when writing the above snippet) to the same effect would pass your tests.

Sorry, just noticed that you do use a right-hand branch; it's just that your diagram for GetMaxTreeDepthTest is misleading, only showing left-hand branches (which is obviously not ideal). There is value, however, in having tests which explicitly test different bits of logic (e.g. if you have one which only worries about left-branches, and another which only worried about right-branches, you can single out some class of problems more quickly).

Boring Stuff

The int depth = 0 is clearly redundant, and though that wouldn't bother me, you might consider a named argument or default (= 0) parameter to avoid leaving a variable floating around or to avoid misusing the method by accident. (That said, I seem to spend most of my life regretting using default parameters, so I wouldn't be tempted)

This spacing is inconsistent:

 return Math.Max( GetMaxHelper(root.Right, depth+1),  GetMaxHelper(root.Left, depth+1));

As is this:

Assert.AreEqual(3,result);

* unless it's a tail-call in an explicitly tail-call optimising language, of course


Getting the list of people who RT'd a particular Twitter account (using Twitter4J) I'm looking for feedback (both general Java, and Twitter specific) on how I'm getting the Twitter handles of everyone who retweeted me over the last 200 Tweets.  Comments welcome.

package twitone;

import java.util.ArrayList;
import twitone.structure.BaseTwitterClass;
import twitone.structure.TwitApplicationFactory;
import twitter4j.Paging;
import twitter4j.Status;
import twitter4j.Twitter;
import twitter4j.TwitterException;

public class getmyretweeters extends BaseTwitterClass {

    public static void main(String[] args) throws TwitterException {

        Twitter twitter = TwitApplicationFactory.getjoereddingtonTwitter();
        String temp[] = getRetweeters(twitter);
        for (String string : temp) {
            System.out.println(string);
        }
    }

    public static String[] getRetweeters(Twitter twitter) {
        ArrayList<String> names = new ArrayList<String>();
        try {
            for (Status status : twitter.getUserTimeline(new Paging(1, 200))) {
                System.out.println(status.getText());
                if (status.getRetweetCount() > 0) {
                    Thread.sleep(120000);// Because I don't want to breach
                                        // Twitter's rate limits
                    for (Status rt : twitter.getRetweets(status.getId())) {
                        names.add(rt.getUser().getScreenName());
                    }
                }
            }
        } catch (TwitterException e) {
            e.printStackTrace();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        return names.toArray(new String[names.size()]);
    }
}

GOing through some general style issues first:

getmyretweeters is a class, and should have Camel-Case capitalization -> GetMyRetweeters. I would prefer MyRetweeters with a method get().
Your class should not have the static methods you have, but shouild rather have a constructor that takes a twitter name, and then initializes itself to that: public MyRetweeters(Twitter account) {...}
your getRetweeters(...) method is static, but should really be an instance method, and the name is implied by the class-name (Retweeters). get() is just fine. Since the class should have been constructed with a Twitter handle, there is no need for any arguments either...

Note about the twitter-API rate limits. Sleeping for 2 minutes is a bad idea in your code. If you really have to be doing this sleeping, then you should change your code to do a background mechanism for accessing Twitter, and have a callback to your application with the results of your query. Also, you should have some sort of caching mechanism so that you do not need to re-query data you already have from Twitter. A limit of 1 query every 2 minutes is a really tight limit.

You should also consider reading up on the Twitter API rate limits... they are not as harsh as you have allowed for. Also, there are headers returned as part of tyour request that shows you how many requests are available in the current window, and when the window ends. Bottom line is that you should be smarter about your rate limiting.

Finally, your factory that gets Twitter sessions should not have a method TwitApplicationFactory.getjoereddingtonTwitter() but should instead have something like: TwitApplicationFactory.getTwitter("joereddington");

Your main method becomes:

public static void main(String[] args) throws TwitterException {
    Twitter twitter = TwitApplicationFactory.getTwitter("joereddington");
    Retweeters retweets = new Retweeters(twitter);
    String temp[] = retweeters.get();
    for (String string : temp) {
        System.out.println(string);
    }
}


Python Linked List implementation I am looking for some comments on an implementation of a singly-linked list in Python. What could I do to make it more reliable and readable?

class LinkedList(object):
    class Node(object):
        """
        Inner class of LinkedList. Contains a blueprint for a node of the LinkedList
        """
        def __init__(self, v, n=None):
            """
            Initializes a List node with payload v and link n
            """
            self.value=v
            self.next=n

    def __init__(self):
        """
        Initializes a LinkedList and sets list head to None
        """
        self.head=None

    def insert(self, v):
        """
        Adds an item with payload v to beginning of the list
        in O(1) time
        """
        Node = self.Node(v, self.head)
        self.head = Node

    def size(self):
        """
        Returns the current size of the list. O(n), linear time
        """
        current = self.head
        count = 0
        while current:
            count += 1
            current = current.next
        return count

    def search(self, v):
        """
        Searches the list for a node with payload v. Returns the node object or None if not found. Time complexity is O(n) in worst case.
        """
        current = self.head
        found = False
        while current and not found:
            if current.value == v:
                found = True
            else:
                current = current.next
        if not current:
            return None
        return current

    def delete(self, v):
        """
        Searches the list for a node with payload v. Returns the node object or None if not found. Time complexity is O(n) in worst case.
        """
        current = self.head
        previous = None
        found = False
        while current and not found:
            if current.value == v:
                found = True
            else:
                previous = current
                current = current.next
        # nothing found, return None
        if not current:
            return None
        # the case where first item is being deleted
        if not previous:
            self.head = current.next
        # item from inside of the list is being deleted
        else:
            previous.next = current.next

        return current

    def __str__(self):
        """
        Prints the current list in the form of a Python list
        """
        current = self.head
        toPrint = []
        while current != None:
            toPrint.append(current.value)
            current = current.next
        return str(toPrint)

If readability is a concern, why use single-character variable names? Replace them with something a bit more meaningful: v -> value, n -> next_, etc.

If you followed Python's data model your class could be used more easily and conventionally. For example, implementing size as a method seems a bit odd - if you called it __len__, then it would behave correctly with len and in a boolean context. Looking at the documentation for other, similar data structures can help with picking sensible names (I would expect an insert to take an index, for example, like the other sequences); in your case, maybe start with the deque's interface.

If you follow the conventional implementation, any of the following should work in a predictable way:

if thing in linked_list:
for thing in linked_list:
list(linked_list) (which would really simplify your __str__ implementation)
if linked_list:

You could also leverage Python's abstract base classes; if you inherit from Sequence and implement the required methods, for example, you get some additional behaviour for free.

In general you're following the style guide, but Node as a temporary variable name sticks out. It shadows the class you just instantiated and naming a variable in CamelCase is not the convention, it should be snake_case. In fact, it's entirely pointless; why not just self.node = Node(v, self.head)?


Python automate job ads search I recently started to work with python and came to idea to automate process of looking for new jobs. There is nice website and I managed to get content, because they use angular to render webpage and using just requests gives no results, because page is rendered using javascript.

Below code is functional, but a bit slow, it take about 10-15 seconds to get rendered page. Full process include going through all job ads, get title,description,link, etc, and check local database to see whether this ad already exists. If not, new mail is sent through mailgun. Any suggestion is welcomed.

#!/usr/bin/python
# -*- coding: utf-8 -*-
import sys
from PyQt4.QtGui import *
from PyQt4.QtCore import *
from PyQt4.QtWebKit import *
from bs4 import BeautifulSoup
import pdfkit
import requests
from db import DB

# Variables
base_url = "https://www.helloworld.rs"
links = []

def send_mail(title, body, attach):
    return requests.post(
        "https://api.mailgun.net/v3/MY_URL/messages",
        auth=("api", "MY-API-KEY"),
        files=[("attachment", open(attach))],
        data={"from": 'Job Bot <my@mail>',
              "to": ["another@mail"],
              "subject": "New job: " + title,
              "text": body})

class Render(QWebPage):

    def __init__(self, url):
        self.app = QApplication(sys.argv)
        QWebPage.__init__(self)
        self.loadFinished.connect(self._loadFinished)
        self.mainFrame().load(QUrl(url))
        self.app.exec_()

    def _loadFinished(self, result):
        self.frame = self.mainFrame()
        self.app.quit()

# full url of jobs of interests
url = 'https://www.helloworld.rs/oglasi-za-posao/#/page=0&tag=&cat=&show_more=0&senioritet=1&vreme_postavljanja=&rok_konkursa=&jezik='
try:
    r = Render(url)
except:
    print "[!] Connection error!"
    exit(0)

result = r.frame.toHtml()
formatted_result = str(result.toAscii())
bs = BeautifulSoup(formatted_result, 'lxml')
jobs = []
for job in bs.find_all('div', class_="job-item"):
    _company = job.find('strong')
    try:
        company = _company.a.text
    except:
        company = _company.text
    company = company.replace('\n', '')
    company = company.replace('\r', '')
    company = company.replace('\t', '')

    title = job.h3.a.string
    description = job.find('p', class_="description").contents[0]
    link = base_url + job.find('a', class_="job-link")["href"]
    jobs.append({"title": title, "description": description, "company": company, "link": link})

db = DB(db="DATABASE-NAME")

#get all links from db to check if new result exists
db_links = db.execute("SELECT link FROM jobs")
for u in db_links.fetchall():
    links.append(u[0])

for job in jobs:
    job_title = job['title'].encode("utf-8")
    job_company = job['company'].encode("utf-8")
    job_description = job['description'].encode("utf-8")
    job_link = job['link']
    if job_link not in links:
        db.execute(
            "INSERT INTO jobs (title,description,company,link) VALUES ('{}','{}','{}','{}','{}')".format(
                job_title,
                job_description,
                job_company,
                job_link))
        mail_body = "{}\n\n{}".format(job_description, job_link)
        print "[+] New ad, sending mail."
        generated_pdf_location = "/tmp/" + job_title + ".pdf"
        pdfkit.from_url(job_link, generated_pdf_location)
        print send_mail(job_title, mail_body, generated_pdf_location)
    else:
        print "[-] Already exists, skipping."

Speeding up HTML Parsing

You can make some quick wins on the HTML parsing side by applying a SoupStrainer class allowing to parse only a part of the document containing the desired jobs:

parse_only = SoupStrainer('div', class_='job-item')
bs = BeautifulSoup(formatted_result, 'lxml', parse_only=parse_only)

jobs = []
for job in bs.select("div.job-item"):
    _company = job.strong
    try:
        company = _company.a.get_text()
    except AttributeError:
        company = _company.get_text()

    company = company.replace('\n', '').replace('\r', '').replace('\t', '')

    title = job.h3.a.string
    description = job.select_one("p.description").contents[0]
    link = base_url + job.select_one("a.job-link")["href"]
    jobs.append({
        "title": title,
        "description": description,
        "company": company,
        "link": link
    })

Don't forget to import SoupStrainer from bs4.

Speeding up Database Interaction

First of all, you should not be using str.format() to parameterize your query - it is highly discouraged. You are opening your code to SQL injections and have to deal with type conversions and quote balancing and escaping manually. Instead, properly parameterize your query via your database driver.

As far as improving the speed - you can actually use executemany() to insert jobs into the database in one go:

query = """
    INSERT INTO
        jobs (title, description, company, link)
    VALUES
        (%(title)s, %(description)s, %(company)s, %(link)s)
"""
db.executemany(query, jobs)

Other performance improvements

I would probably enforce the uniqueness of a link on the database level and handle/skip constraint violations on insert instead of doing it manually via grabbing the existing links first. If you still gonna continue with this approach, make sure to make links a set to improve the lookup speed.


Plugin for WordPress and Foundation Reveal I figure that someone here probably knows a much better way to do this. I'm still figuring out AJAX and jQuery, so I consider that I still need to master quite a bit of knowledge.

My function extends Foundation 4 Reveal's functionality in a few ways:

Uses WordPress AJAX to dynamically pull in content to populate a modal div.
Centers the modal div and allows it to be variable width.
Adds paging navigation from the modal window that when triggered will close the open modal window, then open the previous/next modal content.

While I feel like I've managed to figure out a lot on my own, my code isn't optimal; I would greatly appreciate any feedback or insights on ways to improve it, or things that I should avoid.

(function($) {

$.fn.displayPost = function() {

event.preventDefault();

var post_id = $(this).data("id");
var id = "#" + post_id;

// Check if the reveal modal for the specific post id doesn't already exist by checking for it's length
if($(id).length == 0 ) {
    // We'll add an ID to the new reveal modal; we'll use that same ID to check if it exists in the future.
    var modal = $('
').attr('id', post_id ).addClass('reveal-modal').appendTo('body');
    var ajaxURL = MyAjax.ajaxurl;
     $.ajax({
        type: 'POST',
        url: ajaxURL,
        data: {"action": "load-content", post_id: post_id },
        success: function(response) {
            modal.empty().html(response).append('&#215;').foundation('reveal', 'open');
            modal.bind('opened', function() {
                // Reset visibility to hidden and set display: none on closed reveal-modal divs, for some reason not working by default when reveal close is triggered on .secondary links
                $(".reveal-modal:not('.reveal-modal.open')").css({'visibility': 'hidden', 'display' : 'none'})
                // Trigger resize
                $(window).trigger('resize');
            return false;
            });
        }
    });
}
 //If the div with the ID already exists just open it.
 else {
     $(id).foundation('reveal', 'open');
 }

 // Recalculate left margin on window resize to allow for absolute centering of variable width elements
 $(window).resize(function(){
     var left;
        left = Math.max($(window).width() - $(id).outerWidth(), 0) / 2;
        $(id).css({
            left:left + $(window).scrollLeft()
        });
 });
}

 })(jQuery);

 // Apply the function when we click on the .reveal link
 // (document).ready won't work on any reveal-modal divs added subsequently
 // after page load via AJAX so use .on instead.
 jQuery(document).on("click", ".reveal,.secondary", function() {
  jQuery(this).displayPost();

 });

  // Close open modal via secondary paging links; target parent div id.
  jQuery(document).on("click", ".secondary", function() {
    var id = jQuery(this).closest("div").attr("id");
  jQuery(id).foundation('reveal', 'close');
  });

L15, L26, L31-60: Inconsistent indentation levels.
L4-43: Consider indenting this block.
L04: I don't see the point in storing the ID rather than the jQuery object itself. Instead of var id = "#" + post_id;, go ahead and store the object: var $id = $("#" + post_id);. Modify the references as appropriate on L11 and L33.
L13: Your code for appending the modal is nice in terms of the builder pattern, but it might be better just as:

var modal = $("

").appendTo("body")

L14: There's no point in declaring the ajaxURL variable. Instead, just go ahead and use { ... url: MyAjax.ajaxurl, ... } on L17,
L20: The jQuery API documentation on .html() notes a specific case in which it's important to use .empty().html(). I don't believe this applies in your case; you don't need the call to .empty() since .html() will effectively empty the container first.
L21: Since jQuery 1.7, .on() is preferred.
L58-59: This part is at best senseless and wrong, and at worst unintuitive and potentially deceptive. By extracting the ID attribute and using it as a selector, you're effectively matching against the ID without the #. This either means that it doesn't do what you think it does, in which case you should use:

jQuery(this).closest("div").foundation("reveal", "close");

or you for some reason have a construct like
 and you really mean to match all span elements instead of #span. In that case, I strongly suggest you restructure your HTML to be more semantically sensible, and to leave a comment explicitly detailing that that's your intention.


Storing global styles and utilizing them throughout view controllers Assuming an app has many display styles, fonts, colors, etc., and that we never want to hardcode values, I've created an AppVariables object that houses properties for all of the necessary styles in my app:

class AppVariables: NSObject {

    /**
     * Fonts
     */
    let UIFontPrimary10 = UIFont(name: "Heiti TC", size: 10)
    let UIFontPrimary12 = UIFont(name: "Heiti TC", size: 12)
    let UIFontPrimary14 = UIFont(name: "Heiti TC", size: 14)

    var attribsFont10: NSDictionary!
    var attribsFont12: NSDictionary!
    var attribsFont14: NSDictionary!

    var paragraphStyleFont10 = NSMutableParagraphStyle()
    var paragraphStyleFont12 = NSMutableParagraphStyle()
    var paragraphStyleFont14 = NSMutableParagraphStyle()

    /**
     * Colors
     */
    let UIColorWhiteOff = UIColor(red: 250.0/255.0, green: 250.0/255.0, blue: 250.0/255.0, alpha: 1)
    let UIColorPurple = UIColor(red: 134.0/255.0, green: 129.0/255.0, blue: 204.0/255.0, alpha: 1)
    let UIColorPurpleLight = UIColor(red: 178.0/255.0, green: 176.0/255.0, blue: 217.0/255.0, alpha: 1)
    let UIColorBlueDark = UIColor(red: 40.0/255.0, green: 35.0/255.0, blue: 61.0/255.0, alpha: 1)
    let UIColorBlueDarkAlpha6 = UIColor(red: 40.0/255.0, green: 35.0/255.0, blue: 61.0/255.0, alpha: 0.6)
    let UIColorBlueDarkAlpha95 = UIColor(red: 40.0/255.0, green: 35.0/255.0, blue: 61.0/255.0, alpha: 0.95)

    /**
     * Dimensions and margins
     */
    var screenWidth: CGFloat!
    var screenHeight: CGFloat!
    var viewPadding: CGFloat!
    var viewPaddingSmall: CGFloat!
    var viewWidth: CGFloat!

    init() {
        super.init()

        self.paragraphStyleFont10.lineSpacing = 4
        self.paragraphStyleFont12.lineSpacing = 5
        self.paragraphStyleFont14.lineSpacing = 6

        self.attribsFont10 = [NSFontAttributeName: self.UIFontPrimary10, NSParagraphStyleAttributeName: self.paragraphStyleFont10]
        self.attribsFont12 = [NSFontAttributeName: self.UIFontPrimary12, NSParagraphStyleAttributeName: self.paragraphStyleFont12]
        self.attribsFont14 = [NSFontAttributeName: self.UIFontPrimary14, NSParagraphStyleAttributeName: self.paragraphStyleFont14]

        self.screenWidth = UIScreen.mainScreen().bounds.width
        self.screenHeight = UIScreen.mainScreen().bounds.height
        self.viewPadding = (screenWidth * 0.1) / 2
        self.viewPaddingSmall = 10.0
        self.viewWidth = screenWidth - (viewPadding * 2)
    }
}

Assuming I've instantiated a copy of AppVariables into a property of AppDelegate, I can use them like so:

class Foo {

    func bar() {

        var someLabel = UILabel()
        someLabel.textColor = self.appDelegate.appVariables.UIColorPurpleLight
    }

}

Is there a better way of doing this? It seems like the simplest approach, since these are more than mere constants, but I'm open to thoughts and opinions.

I think that you would be better off using static methods on a struct or class. This removes the need for you to access your style through the App Delegate. The App Delegate should only be concerned with handling the delegate callbacks from the OS, storing globally accessible values in it messes with the principle of separation of concerns and creates a spider web of dependencies hurting the reusability and understandability of your code.
Lumping Fonts, Colors, Dimensions, Paragraph styles, etc. all into a single level is not very discoverable. I think it would be better to separate them into their own types.
In my experience a single color is not enough to define a style. Colors almost always come associated with other colors like foreground v.s. background, or normal v.s. highlighted.
It doesn't make sense to create multiple constants for a single font of different sizes. Better to have a method that takes in a size and returns the special font with the given size.

I recently started creating a collection of "style" classes for myself. This is what I did for colors:

extension UIColor {
    convenience init(hex : Int) {
        let blue = CGFloat(hex & 0xFF)
        let green = CGFloat((hex >> 8) & 0xFF)
        let red = CGFloat((hex >> 16) & 0xFF)
        self.init(red: red / 255.0, green: green / 255.0, blue: blue / 255.0, alpha: 1)
    }
}

struct DepthColor {
    enum Depth {
        case Foreground, Background
    }

    let foreground: UIColor
    let background: UIColor

    init(foreground: Int, background: Int) {
        self.foreground = UIColor(hex: foreground)
        self.background = UIColor(hex: background)
    }

    init() {
        self.foreground = UIColor(hex: 0xFFFFFF)
        self.background = UIColor(hex: 0)
    }

    func colorForDepth(depth : Depth) -> UIColor {
        switch(depth) {
            case .Foreground:
                return self.foreground
            case .Background:
                return self.background
        }
    }
}

struct StyleColor {
    enum Purpose {
        case Normal, Highlighted, Disabled
    }

    let normal : DepthColor
    let highlighted : DepthColor
    let disabled : DepthColor

    init(normal: DepthColor = DepthColor(), highlighted: DepthColor = DepthColor(), disabled: DepthColor = DepthColor()) {
        self.normal = normal
        self.highlighted = highlighted
        self.disabled = disabled
    }

    func depthColorForPurpose(purpose : Purpose) -> DepthColor {
        switch(purpose) {
            case .Normal:
                return self.normal
            case .Highlighted:
                return self.highlighted
            case .Disabled:
                return self.disabled
        }
    }

    func color(_ purpose : Purpose = .Normal, _ depth: DepthColor.Depth = .Foreground) -> UIColor {
        return self.depthColorForPurpose(purpose).colorForDepth(depth)
    }

    static func positiveAction() -> StyleColor {
        return StyleColor(
            normal: DepthColor(foreground: 0x45A332, background: 0xFFFFFF),
            highlighted: DepthColor(foreground: 0x45A332, background: 0xFFFFFF),
            disabled: DepthColor(foreground: 0x888888, background: 0xFFFFFF)
        )
    }

    static func deleteAction() -> StyleColor {
        return StyleColor(
            normal: DepthColor(foreground: 0xF31717, background: 0xFFFFFF),
            highlighted: DepthColor(foreground: 0xD91414, background: 0xFFFFFF),
            disabled: DepthColor(foreground: 0x888888, background: 0xFFFFFF)
        )
    }

    static func neutralAction() -> StyleColor {
        return StyleColor(
            normal: DepthColor(foreground: 0x666666, background: 0xFFFFFF),
            highlighted: DepthColor(foreground: 0x555555, background: 0xFFFFFF),
            disabled: DepthColor(foreground: 0x888888, background: 0xFFFFFF)
        )
    }
}

I can then get a color from a style like so:

var color = StyleColor.positiveAction().color()
color = StyleColor.positiveAction().color(.Highlighted)
color = StyleColor.positiveAction().color(.Highlighted, .Background)

This also allowed me to create subclasses of common UIKit components that allow me to simply set a StyleColor on them to fully configure them to a style:

class StyleButton: UIButton {
    var style: StyleColor = StyleColor() {
        didSet {
            self._configureView()
        }
    }

    init(coder aDecoder: NSCoder!, style : StyleColor) {
        self.style = style

        super.init(coder: aDecoder)

        self._configureView()
    }

    init() {
        super.init(frame: CGRect())

        self._configureView()
    }

    func _configureView() {
        self.showsTouchWhenHighlighted = false
        self.adjustsImageWhenHighlighted = false
        self.adjustsImageWhenDisabled = false
        self.reversesTitleShadowWhenHighlighted = false

        self.setTitleColor(style.color(.Normal, .Background), forState: .Normal)
        self.setTitleColor(style.color(.Disabled, .Background), forState: .Disabled)
        self.setTitleColor(style.color(.Highlighted, .Background), forState: .Highlighted)

        self.setBackgroundImage(UIImage(color: style.color()), forState: .Normal)
        self.setBackgroundImage(UIImage(color: style.color(.Disabled)), forState: .Disabled)
        self.setBackgroundImage(UIImage(color: style.color(.Highlighted)), forState: .Highlighted)
    }
}

As I need it, I will probably build similar structures for other types of styles and potentially some higher level style classes like StyleText that includes a StyleColor, StyleFont, and potentially StyleDimensions for padding.


McDonald's food order system Is there any way I could make this code more efficient?

def processOrder(quantity, item_list):
    global total
    if quantity > item_list[2]:
        print("There is not enough stock!")
        pass
    else:
        total += item_list[1] * quantity
        item_list[2] -= quantity

total = 0
A = ["Big Mac", float(2.50), 50], ["Large Fries", float(0.50), 200], ["Vegetarian Burger", float(1.00), 20]

print("Welcome to McDonald's")
print("[1]", A[1][0:2],
      "\n[2]", A[1][0:2],
      "\n[2]", A[1][0:2])

while True:
    choice, quantity = (input("\nWhat would you like?\n")).upper(), int(input("\nHow many would you like?\n"))

    if choice == "BIG MAC":
        processOrder(quantity, A[0])
    elif choice == "LARGE FRIES":
        processOrder(quantity, A[1])
    elif choice == "VEGETARIAN BURGER":
        processOrder(quantity, A[2])
    else:
        print("Invalid Item")

    more_items = (input("Do you want to order more items?")).lower()
    if more_items == "yes":
        pass
    else:
        break

print("Thank you for ordering!\nYour total cost is: £" +  str(total))

PEP 8

Line length

E501:11:80:line too long (107 > 79 characters)
E501:19:80:line too long (110 > 79 characters)

Odd spacing

E222:36:58:multiple spaces after operator

No newline at end

W292:36:71:no newline at end of file

General comments

Main

You want to use __name__ to allow importing without running your code. This explains it better

def main():
    # my code here

if __name__ == "__main__":
    main()

Storing food items

A = ["Big Mac", float(2.50), 50], ["Large Fries", float(0.50), 200], ["Vegetarian Burger", float(1.00), 20]

This may be better represented as either instances of a class, or just namedtuples, making the code more readable, as you access them by name a dictionary may clean up the code for this, rather than a list


Is this a good encryption algorithm? I've never had a serious attempt at writing an encryption algorithm before, and haven't read much on the subject. I have tried to write an encryption algorithm (of sorts), I would just like to see if anyone thinks it's any good. (Obviously, it's not going to be comparable to those actually used, given that it's a first attempt)

Without further adieu:

def encode(string, giveUserKey = False, doubleEncode = True):
    encoded = [] #Numbers representing encoded characters
    keys = [] #Key numbers to unlonk code
    fakes = [] #Which items in "encoded" are fake
    #All of these are stored in a file

    if giveUserKey:
        userkey = random.randrange(2 ** 10,  2 ** 16)
        # Key given to the user, entered during decryption for confirmation
        # Could change range for greater security
    else:
        userkey = 1

    for char in string:
        ordChar = ord(char)
        key = random.randrange(sys.maxsize)
        encoded.append(hex((ordChar + key) * userkey))
        keys.append(hex(key))

        if random.randrange(fakeRate) == 0:
            fake = hex(random.randrange(sys.maxsize) * userkey)
            encoded.append(fake)
            fakes.append(fake)

    if doubleEncode:
        encoded = [string.encode() for string in encoded]
        keys = [string.encode() for string in keys]
        fakes = [string.encode() for string in fakes]

    hashValue = hex(hash("".join([str(obj) for obj in encoded + keys + fakes]))).encode()

    return encoded, keys, hashValue, fakes, userkey

def decode(encoded, keys, hashValue, fakes, userkey = 1):
    if hash("".join([str(obj) for obj in encoded + keys + fakes])) != eval(hashValue):
        #File has been tampered with, possibly attempted reverse-engineering
        return "ERROR"

    for fake in fakes:
        encoded.remove(fake)

    decoded = ""

    for i in range(len(keys)):
        j = eval(encoded[i]) / userkey
        decoded += chr(int(j - eval(keys[i])))

    return decoded

Firstly, your encryption is useless. Just run this one line of python code:

print reduce(fractions.gcd, map(eval, encoded))

And it will tell you what your user key is (with a pretty high probability).

Your algorithm is really obfuscation, not encrytion. Encryption is designed so that even if you know the algorithm, but not the key, you can't decryt the message. But your algorithm is basically designed to make it less obvious what the parts mean, not make it hard to decrypt without the key. As demonstrated, retrieving the key is trivial anyways.

def encode(string, giveUserKey = False, doubleEncode = True):

Python convention is lowercase_with_underscores for local names

    encoded = [] #Numbers representing encoded characters
    keys = [] #Key numbers to unlonk code
    fakes = [] #Which items in "encoded" are fake
    #All of these are stored in a file

    if giveUserKey:
        userkey = random.randrange(2 ** 10,  2 ** 16)
        # Key given to the user, entered during decryption for confirmation
        # Could change range for greater security

Why isn't the key a parameter?

    else:
        userkey = 1

    for char in string:
        ordChar = ord(char)
        key = random.randrange(sys.maxsize)
        encoded.append(hex((ordChar + key) * userkey))
        keys.append(hex(key))

These just aren't keys. Don't call them that.

        if random.randrange(fakeRate) == 0:
            fake = hex(random.randrange(sys.maxsize) * userkey)
            encoded.append(fake)
            fakes.append(fake)

    if doubleEncode:
        encoded = [string.encode() for string in encoded]
        keys = [string.encode() for string in keys]
        fakes = [string.encode() for string in fakes]

Since these are all hex strings, why in the world are you encoding them?

    hashValue = hex(hash("".join([str(obj) for obj in encoded + keys + fakes]))).encode()

    return encoded, keys, hashValue, fakes, userkey

def decode(encoded, keys, hashValue, fakes, userkey = 1):
    if hash("".join([str(obj) for obj in encoded + keys + fakes])) != eval(hashValue):
        #File has been tampered with, possibly attempted reverse-engineering
        return "ERROR"

Do not report errors by return value, use exceptions.

    for fake in fakes:
        encoded.remove(fake)

What if coincidentally one of the fakes looks just like one of the data segments?

    decoded = ""

    for i in range(len(keys)):

Use zip to iterate over lists in parallel

        j = eval(encoded[i]) / userkey

Using eval is considered a bad idea. It'll execute any python code passed to it and that could be dangerous.

        decoded += chr(int(j - eval(keys[i])))

It's better to append strings into a list and join them.

    return decoded


Improve on Regex replacing classnames I am working on a small script for a mobile site.
Is there any way to improve on the regex here? or remove it completely?

function fn_page_change(event) {
    event.preventDefault();
    var activePage = $(".ui-active");
    var newPage = $(event.target.getAttribute("href"));
    activePage.className = activePage.className.replace(/(?:^|\s)ui-active(?!\S)/, " ui-inactive ");
    newPage.className = newPage.className.replace(/(?:^|\s)ui-inactive(?!\S)/, " ui-active");
}

I'm not using jQuery, the $ is myown

function $(selector, el) {
    if (!el) {
        el = document;
    }
    var tmp = el.querySelectorAll(selector);
    if (tmp.length === 1) {
        return tmp[0];
    }
    return tmp;
}

Here is an example

You will have less code if you use a regex because you need the word boundary detection that is built into regex.  I'd suggest putting it in a utility function to make it cleaner and more readable:

function replaceClass(el, oldClass, newClass) {
    el.className = el.className.replace(new RegExp("\\b" + oldClass + "\\b"), newClass);
}

function fn_page_change(event) {
    event.preventDefault();
    var activePage = $(".ui-active");
    var newPage = $(event.target.getAttribute("href"));
    replaceClass(activePage, "ui-active", "ui-inactive");
    replaceClass(newPage, "ui-inactive", "ui-active");
}

If you wanted a version of replaceClass() that would handle either of the types of return values you get out of your selector function (single DOM object or array of DOM objects), then you could use this:

function replaceClass(el, oldClass, newClass) {
    var re = new RegExp("\\b" + oldClass + "\\b");
    if (el.nodeType) {
        el.className = el.className.replace(re, newClass);
    } else {
        for (var i = 0; i < el.length; i++) {
            el[i].className = el[i].className.replace(re, newClass);
        }
    }
}


Repositioning images in a gallery based on the current image I'm working on extending the functionality of a photo gallery by adding a thumbnail display, I have multiple if statements set up to adjust the position of the thumbnails. Obviously this is not ideal, I'm trying to put my head around how to use a for loop to iterate the width multiplier and curSlide position.

Ideally I would like the curSlide to increment after every 5 slide and the thumbGalwidth*i to increment as well.

var thumbGalwidth = 795;

if(currSlide < 6) {
jQuery('#view-all-container').css('margin-left', '-' + thumbGalwidth*0 + 'px');
}
if(currSlide >= 6) {
jQuery('#view-all-container').css('margin-left', '-' + thumbGalwidth*1 + 'px');
}
if (currSlide >= 11) {
jQuery('#view-all-container').css('margin-left', '-' + thumbGalwidth*2 + 'px');
}
if (currSlide >= 16) {
jQuery('#view-all-container').css('margin-left', '-' + thumbGalwidth*3 +     'px');
}
if (currSlide >= 21) {
jQuery('#view-all-container').css('margin-left', '-' + thumbGalwidth*4 +    'px');
}
if (currSlide >= 26) {
jQuery('#view-all-container').css('margin-left', '-' + thumbGalwidth*5 +    'px');
}

Because you have nice, predictable ranges, you don't need a loop at all. Just do a little math.

var thumbGalwidth = 795;

var n = Math.floor(((currSlide || 1)-1) / 5);

jQuery('#view-all-container').css('margin-left', '-' + thumbGalwidth*n + 'px');

If the largest n should be 5, then change the assignment to this:

var n = Math.min(5, Math.floor(((currSlide || 1)-1) / 5));


Project Euler 23 in Scala (non-abundant sums) I recently overhauled my solution to Project Euler Problem 23 (which asks for the sum of all positive integers which cannot be written as the sum of two abundant numbers) to move it from an imperative style to a functional style and tried to increase its speed. However, the performance is still slow. I have clocked this solution at 58 seconds, and the older imperative solution at 64 seconds.

I am mainly hoping for feedback on how I could speed up the performance of this solution. The bottleneck appears to be at the notSumAbundant function. Is there a reliable way to reduce the number of values I need to check? Can this portion of notSumAbundant be made faster in any other way?

if(!abundantList.exists(x => abundantList.contains(num - x))) {
  notSumAbundant(num + 1, num :: currentList) }

I am also hoping for feedback if my code falls short of functional programming best practices.

val t0 = System.nanoTime()

//function to find factors
def factors(num: Int) = {
    val max = (num / 2) + 1
    val factors = for(i <- 1 to max if num % i == 0) yield { i }
    factors
}

//function to find sums
def sumList(numList: IndexedSeq[Int]) = {
  val sum = numList.reduceLeft( _ + _ )
  sum
}

//function to recursively form list of abundant numbers
def abundance(num: Int, currentList: List[Int]): List[Int] = {
  if(num <= 20161){
  val numFactors = factors(num)
  val factorSum = sumList(numFactors)

  //if the number is abundant, prepend it to the running list of abundant numbers in the function call
  if(factorSum > num) { abundance(num + 1, num :: currentList) }
  else { abundance(num + 1, currentList) }
  } else return currentList
}

//all abundant numbers
val abundant = abundance(3, List())

//function to create a list of numbers which cannot be formed as the sum of two abundant numbers
def notSumAbundant(num: Int, currentList: List[Int]): List[Int] = {
  val abundantList = abundant.filter(_ < num)
  if(num <= 20161) {
    //if it cannot be made as the sum of abundant numbers, prepend it to the current list when calling on the next number
    if(!abundantList.exists(x => abundantList.contains(num - x))) {
      notSumAbundant(num + 1, num :: currentList)
      } else { notSumAbundant(num + 1, currentList) }
 }  else return currentList
}

//store the values and then sum them
val answerList = notSumAbundant(1, List())
val answer = answerList.reduceLeft(_ + _)

val t1 = System.nanoTime()
println("Elapsed time: " + (t1 - t0) / 1000000000 + " seconds")
println(answer)

Update:

Changing the nonAbundantSum function to use a Set instead of a List worked to get this down to 2 seconds, as t. fochtman suggested:

val abundant = abundance(3, List()).toSet
//function to create a list of numbers which cannot be formed as the sum of two abundant numbers
def notSumAbundant(num: Int, currentSet: Set[Int]): List[Int] = {
  val abundantList = abundant.filter(_ < num)
  if(num <= 20161) {
    //if it cannot be made as the sum of abundant numbers, prepend it to the current list when calling on the next number
    if(!abundantList.exists(x => abundantList.contains(num - x))) {
      val updatedSet = (num :: currentSet.toList).toSet
      notSumAbundant(num + 1, updatedSet)
      } else { notSumAbundant(num + 1, currentSet) }
 }  else return currentSet.toList
}

//store the values and then sum them
val answerList = notSumAbundant(1, Set())

I was able to reduce the run time of your code to a few seconds (2 seconds on my computer) by changing the type of abundant from a List to an immutable Set.

You were correct that !abundantList.exists(x => abundantList.contains(num - x)) was the major bottleneck in your code.

This is a bottleneck because checking that an item exists in a List means inspecting every item in the List until we find it or reach the end of the List. Fortunately we can use a Set (which under the hood is supported by the immutable.HashMap class) and reduce the search time for objects from linear to effectively constant.

Notice that in the code below I simply applied the method toSet to your abundance function converting the returned List to a Set.

val abundant = abundance(3, List()).toSet

def notSumAbundant(num: Int, currentList: List[Int]): List[Int] = {
  if(num <= n) {
    val abundantSet: Set[Int] = abundant.filter(_ < num)
    if (!abundantSet.exists(x => abundantSet.contains(num - x))) {
      notSumAbundant(num + 1, num :: currentList)
    } else
      notSumAbundant(num + 1, currentList)
  } else
    currentList
}

For more information check out this nice write up on Scala collections that is co-written by the creator of Scala, Martin Odersky.


Too many JavaScript sort functions to accomplish the same type of sorting You can see each list item has multiple attributes data-whatever. The user will choose from a dropdown box on how things should be sorted and the appropriate method is called. Is eval() the most appropriate choice for this or is there a better way?

$('#sort-options').change(function() {
    var sortType = $(this).val();
    var $selectedList = $($(".drop-zone.ui-tabs-selected a").attr('href')).find('.connectedSortable');
    var $listItems = $('li', $selectedList);

    //Sort
    var sortFunction = null;
    switch (sortType) {
    case 'custom':
        $listItems.sort(sortCustom);
        break;
    case 'breed':
        $listItems.sort(sortBreed);
        break;
    case 'hatch':
        $listItems.sort(sortHatch);
        break;
    case 'laid':
        $listItems.sort(sortLaid);
        break;
    default:
        $listItems.sort(sortCustom);
    }

    //Empty UL
    $selectedList.empty();
    //Repopulate UL
    $.each($listItems, function(index, $listItem) {
        $selectedList.append($listItem);
    });
    });

    function sortCustom(a, b) {
    return $(a).attr('data-sort-order') - $(b).attr('data-sort-order');
    }

    function sortBreed(a, b) {
    return $(a).attr('data-breed') - $(b).attr('data-breed');
    }

    function sortHatch(a, b) {
    return $(a).attr('data-hatch') - $(b).attr('data-hatch');
    }

    function sortLaid(a, b) {
    return $(a).attr('data-laid') - $(b).attr('data-laid');
    }

    //Force initial sort
    $('#sort-options').trigger('change');
}

function makeSorter(sortType) {
    sortType = "data-"+sortType;
    return function (a, b) {
        return $(a).attr(sortType) - $(b).attr(sortType);
    };
}

$('#sort-options').change(function () {
    var sortType = $(this).val();
    var $selectedList = $($(".drop-zone.ui-tabs-selected a").attr('href')).find('.connectedSortable');
    var $listItems = $('li', $selectedList);

    $listItems.sort(makeSorter(sortType));

Note that if val is "custom" it won't work, so I recommend replacing "custom" with "sort-order".


Need a better way to create subclass instances from a base class reference FeatureMap contains features, derived classes have some mandatory features that the map should have, say keys (the mandatory part) are added and all the features are fetched from somewhere. Since clients interact with FeatureMap, a featuremap needs to be returned and the type is part of the actual object. Is there a better way to create instead of createFeatureMapWithMandatoryValues

import lombok.NonNull;

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

public abstract class FeatureMap {
    private Map<String, Object> features;
    public FeatureMap() {
        features = new HashMap<>();
    }
    void addFeature(@NonNull String name, @NonNull Object value) {
        features.put(name, value);
    }

    abstract FeatureMap createFeatureMapWithMandatoryValues(Map<String, Object> values);
}

class FoodFeatureMap extends FeatureMap {
    public static final String foodType = "foodType";

    public FoodFeatureMap(@NonNull String foodTypeValue) {
        addFeature(foodType, foodTypeValue);
    }

    @Override
    FeatureMap createFeatureMapWithMandatoryValues(Map<String, Object> values) {
        return new FoodFeatureMap((String) values.get(foodType));
    }
}

class HumanFeatureMap extends FeatureMap {
    public static final String name = "name";
    public static final String origin = "origin";

    public HumanFeatureMap(@NonNull String nameValue, @NonNull String originValue) {
        addFeature(name, nameValue);
        addFeature(origin, originValue);
    }

    @Override
    FeatureMap createFeatureMapWithMandatoryValues(Map<String, Object> values) {
        return new HumanFeatureMap((String)values.get(name), (String) values.get(origin));
    }
}

class Main {
    public static void main(String[] args) {
        FeatureMap featureMap = new FoodFeatureMap("Chinese");
        Map<String, Object> map = Collections.singletonMap(FoodFeatureMap.foodType, "Chinese");
        //assume this map contains more features fetched from somewhere given the earlier map having keys
        featureMap.createFeatureMapWithMandatoryValues(map);
    }
}

There sure is. You created a redundant way to create a FeatureMap and that is why you are having so much trouble with it. Your createFeatureMapWithMandatoryValues is pretty much useless.
When you do something such as:

FeatureMap featureMap = new FoodFeatureMap("Chinese");

You already have everything that you would ever want. By looking at FoodFeatureMap ctor we can see that you already correctly add the values that are mandatory because you have to create an instance of FoodFeatureMap by calling it.

public FoodFeatureMap(@NonNull String foodTypeValue) {
    addFeature(foodType, foodTypeValue);//this already correctly adds the value to the map
}

And thus the resulting code becomes this.

public abstract class FeatureMap {
    private Map<String, Object> features;
    public FeatureMap() {
        features = new HashMap<>();
    }

    protected void addFeature(@NonNull String name, @NonNull Object value) {
        features.put(name, value);
    }
}

class FoodFeatureMap extends FeatureMap {
    private static final String foodType = "foodType";

    public FoodFeatureMap(@NonNull String foodTypeValue) {
        addFeature(foodType, foodTypeValue);
    }
}

class HumanFeatureMap extends FeatureMap {
    private static final String name = "name";
    private static final String origin = "origin";

    public HumanFeatureMap(@NonNull String nameValue, @NonNull String originValue) {
        addFeature(name, nameValue);
        addFeature(origin, originValue);
    }
}

class Main {
    public static void main(String[] args) {
        FeatureMap featureMap = new FoodFeatureMap("Chinese");
        //do something with FeatureMap
    }
}

Now what comes ahead maybe for example turn your FeatureMap into an Iterable so you can get all the values that are on it


Brainfuck interpreter (with emphasis on robustness) While writing a review of @MotokoKusanagi's Brainfuck interpreter, I decided to write my own implementation to illustrate a few points.  In particular, I'd like it to be robust to malformed programs, and reasonably efficient (short of JITting the Brainfuck code).

Please point out any deficiencies you might find in this implementation.

#include <fcntl.h>
#include <libgen.h>
#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/stat.h>
#include <unistd.h>

typedef struct {
    char *begin,            // Pointer to beginning of program
         *end,              // Pointer just beyond the end of the program
         **jumps;           // Pointer to jump table
} program;

/**
 * Fills in p.jumps, the jump table.  For each '[' or ']' character
 * in the code, the pointer at the corresponding offset in p.jumps
 * is set to the matching ']' or '[' (or NULL if it is mismatched).
 * This function calls itself recursively to handle nesting.
 */
static char *build_jump_table(program p, char *begin) {
    char *c = begin;
    while (c < p.end) {
        ptrdiff_t i = c - p.begin;
        switch (*c) {
          case '[':
            p.jumps[i] = NULL;      // In case no matching ']' is ever found
            p.jumps[i] = c = build_jump_table(p, c + 1);
            if (!c) return NULL;    // Error: no closing bracket
            break;
          case ']':
            p.jumps[i] = (begin > p.begin) ?
                begin - 1 :         // Normal case
                NULL;               // Error: no opening bracket
            return c;
        }
        c++;
    }
    return NULL;
}

/**
 * Loads the program from the specified path.  On failure, all members of
 * the returned program will be NULL, and errno is set.
 */
program load_program(const char *path) {
    const program FAILURE = { .begin = NULL, .end = NULL, .jumps = NULL };
    int fd;
    struct stat stat_buf;
    if ( -1 == (fd = open(path, O_RDONLY)) ||
         -1 == fstat(fd, &stat_buf) ) {
        return FAILURE;
    }
    int size = stat_buf.st_size;
    char *text = malloc(size);
    if ( (text == NULL) || (size != read(fd, text, size)) ) {
        free(text);
        close(fd);
        return FAILURE;
    }
    close(fd);
    program p = { .begin = text, .end = text + size, .jumps = malloc(size) };
    if (p.jumps == NULL) {
        free(text);
        return FAILURE;
    }
    build_jump_table(p, p.begin);
    return p;
}

void free_program(program p) {
    free(p.jumps);
    free(p.begin);
}

int execute_program(program p) {
    unsigned char mem[30000] = { 0 };
    unsigned char *ptr = mem;
    char *ip = p.begin;
    while (ip < p.end) {
        ptrdiff_t i = ip - p.begin;
        switch (*ip) {
            case '<': if (--ptr < mem) return i + 1;                    break;
            case '>': if (++ptr >= mem + sizeof mem) return i + 1;      break;
            case '-': (*ptr)--;                                         break;
            case '+': (*ptr)++;                                         break;
            case '.': putchar(*ptr); fflush(stdout);                    break;
            case ',': *ptr = getchar();                                 break;
            case '[': if (!*ptr) ip = p.jumps[i];                       break;
            case ']': if ( *ptr) ip = p.jumps[i];                       break;
        }
        if (!ip++) {
            return i + 1;
        }
    }
    return 0;
}

void usage(FILE *f, char *argv[]) {
    fprintf(f, "Usage: %s PROGRAM.bf\n", basename(argv[0]));
}

int main(int argc, char *argv[]) {
    if (argc <= 1) {
        usage(stderr, argv);
        return 1;
    }
    program prog = load_program(argv[1]);
    if (!prog.begin) {
        perror("Error: Could not load program");
        return 2;
    }
    int error_offset = execute_program(prog);
    if (error_offset > 0) {
        fprintf(stderr, "Error at offset %d\n", error_offset);
    }
    free_program(prog);
}

Deficiencies? I don't really see any. Improvements? Maybe :)

You don't handle at all the return values of putchar, getchar and fflush. The Wikipedia article has some hints about how different implementations handle an EOF from the user input. Yours works as well, but is this really what you want?
In build_jump_table, the switch used to check whether *c is a [ or a ] seems a bit overkill. It would probably be easier to get away with a simple if .. else statement.
That's highly speculative, but I guess that if you replace the characters <, >, -, +, ., ,, [ and ] by values ranging from 0 to 7, or at least by contiguous values during the program load, it might help your compiler to generate a better jump table for your switch, which might help increasing the speed of the programs.

The assumption here is that your care more about the speed during the execution and that a little slowdown during the program load isn't a problem if it can otherwise improve performance.
I guess that you want your programs to be fast while they run and that a small slowdown when it terminates isn't one of your concerns if you can otherwise speed your program up. Therefore, if your compiler supports it (I guess it does), you could use __builtin_expect to influence branch prediction while the program runs:

switch (*ip) {
    case '<':
        if (__builtin_expect(--ptr < mem, false)) {
            return i + 1;
        }
        break;
    case '>':
        if (__builtin_expect(++ptr >= mem + sizeof mem, false)) {
            return i + 1;
        }
        break;
    // ...
}

It would however be a bad idea to influence branch prediction for the conditions in the cases of [ and ]. The results are too prone to change to risk a pessimization.

I would have some other small remarks but I will keep them for myself since they would be considered a matter of style and you know what you're doing anyway :)