Code to measure the length of binary numbers in terms of English nomenclature "ONE" and "ZERO".

The basis of this project is in this
video -> links to his code can be found in description

In it the speaker asks you to check his math; I wrote my own code and then compared it against his.

It is also in the other GIST i am adding to stack-exchange -> Credit for that code goes to MATT PARKER.

TL;DR This code loops through all the numbers between 0 and the value in "maximum" it calculates how many letters are in the binary form of the word "One" or "Zero" -> 13 (1101) and 18 (10010) are the only two values if spelled with the number of letters it equals its value! In this all variables loop around until they reach either 13 or 18 (explained in video) I counted how many of them go to either 18 (99.9909%) or 13 (the rest).

Please rate my code and how efficient it is: especially as compared to the original code and also how it can be improved!
Code
My code is in main.py and his is in binarychain.py

#Creating Variables
Count18 = 0
Count13 = 0
#Change maximum if you don't want to break your computer
maximum = 1000000

#Iterates over each letter and adds to a variable called "letter" for each letter in "one" -> 3 or "Zero" -> 4
def letters(position=0):
    letter = 0
    binary = str('{0:b}'.format(position))

    for ch in binary:
        if ch == '1':
            letter += 3
        if ch == '0':
            letter += 4
    return int(letter)

#Checks if the number reaches 13 or 18
def check(number=0):
    global Count13
    global Count18
    if number == 13:
        Count13 += 1
        return 0
    if number == 18:
        Count18 += 1
        return 0
    if number != 13 or 18:
        return number

running = True
counter = 0
while running:
    checker = check(letters(counter))
    if checker == 0:
        pass
    else:
        #Loops through over and over again until a number reaches either 13 or 18
        while checker != 0:
            checker = check(letters(checker))

    #I could have used a "for" loop but it just wasn't working with this :(
    counter += 1
    if counter > maximum:
        running = False

#Calcualtions for percentage
PerCent18 = (Count18 / maximum)*100
PerCent13 = (Count13 / maximum)*100
print("\nCount 18:", Count18, PerCent18, "Count13:", Count13, PerCent13)

#Just incase it is not being run in an IDE
input()

Just some points:

This code here:

running = True
counter = 0
while running:
    checker = check(letters(counter))
    if checker == 0:
        pass
    else:
        #Loops through over and over again until a number reaches either 13 or 18
        while checker != 0:
            checker = check(letters(checker))

    #I could have used a "for" loop but it just wasn't working with this :(
    counter += 1
    if counter > maximum:
        running = False

is a bit repetitive and could be easily simplified. First, let's remove the duplicate checker = check(letters(counter)) by using a different loop:

running = True
counter = 0
while running:
    checker = 1 # really any number can be used here
    while checker != 0
        checker = check(letters(counter))
    counter += 1
    if counter > maximum:
        running = False

and then we can change the while loop into a for loop:

running = True
counter = 0
for counter in range(0, maximum + 1):
    checker = 1 # really any number can be used here
    while checker != 0:
        checker = check(letters(counter))

Here:

for ch in binary:
    if ch == '1':
        letter += 3
    if ch == '0':
        letter += 4

Can't you use an else?

for ch in binary:
    if ch == '1':
        letter += 3
    else:
        letter += 4


Implementing joinWithSeparator on a Character array I recently answered a Stack Overflow question about transforming an array of Characters into a String in Swift.  Looking at the question and the Swift standard lib, it appears that there is a method joinWithSeparator, but the current implementation only supports SequenceType instances where the Element is a String.  As an exercise to myself, I wanted to write an extension on SequenceType that could flatten an array of Characters into a String:

extension SequenceType where Generator.Element == Character {

    @warn_unused_result
    public func joinWithSeparator(separator: String) -> String {

        var str = ""

        self.enumerate().forEach({
            str.append($1)

            if let arr = self as? [Character], endIndex: Int = arr.endIndex {
                if $0 < endIndex - 1 {
                    str.append(Character(separator))
                }
            }
        })

        return str
    }
}

How can I further optimize this code?  Would it be possible to replace the forEach loop with flatMap?  Or is a map call inappropriate since I need to append each character to a String, rather than create a new array?

There are two problems with your method:

Problem #1: The method takes a separator: String parameter, but actually it is
expected that a single character string is passed, and it crashes
at Character(separator) if a multi-character string is passed:

let myChars: [Character] = ["a", "b", "c"]
let joined = myChars.joinWithSeparator("==")
print(joined)
// fatal error: Can't form a Character from a String containing more than one extended grapheme cluster

The solution is easy: change the parameter to separator: Character.
Btw, this makes the method faster.

Problem #2: The method is an extension of SequenceType, but it
actually works only for Arrays. For arbitrary sequences of characters,
the optional cast as? [Character] fails. This is silently ignored
and the separator not inserted:

let charSeq = Repeat(count: 4, repeatedValue: Character("x"))
let joined = charSeq.joinWithSeparator(",")
print(joined) // Output: xxxx

Two more remarks before I suggest possible solutions:

The name of the local variable var str = "" is quite non-descriptive.
I would call it result or joinedString (but that may be
opinion based).
Your loop

self.enumerate().forEach {
    /* do something with `$0` (index) and `$1` (char)
}

is correct, but I would write it as

for (index, char) in self.enumerate() {
    /* do something with `index` and `char`
}

So how can problem #2 be solved? You cast the sequence to an array in
order to get the index of the last element, so that you can insert the
separator after all elements but the last.

It becomes simpler if you think the other way around: Insert the
separator before all elements but the first. No need to determine
the endIndex anymore:

public func joinWithSeparator(separator: Character) -> String {

    var result = ""
    for (idx, char) in self.enumerate() {
        if idx > 0 {
            result.append(separator)
        }
        result.append(char)
    }
    return result
}

This has about the same speed as your method when applied to an
array, but works for arbitrary character sequence.

You asked:

  Would it be possible to replace the forEach loop with flatMap?

and the answer is "yes":

public func joinWithSeparator(separator: Character) -> String {

    let joinedChars = self.enumerate().flatMap {
        (idx, char) in
        idx == 0 ? [ char ] : [ separator, char ]
    }
    return String(joinedChars)
}

The shorted implementation (but not the fastest) would be to convert all characters
to strings, and use the existing method to join strings:

public func joinWithSeparator(separator: Character) -> String {

    return self.map { String($0) }.joinWithSeparator(String(separator))
}

Performance comparison

Here is my test code (compiled in Release
mode on a 3.5Ghz iMac):

let myChars = Array(Repeat(count: 1_000_000, repeatedValue: Character("x")))
let d1 = NSDate()
let j = myChars.joinWithSeparator(",")
let d2 = NSDate()
print(d2.timeIntervalSinceDate(d1))

Results:

Your original code: 0.163 sec.
Your code, with the separator parameter changed to Character: 0.104 sec.
My first suggestion: 0.095 sec.
Using flatMap: 0.276 sec.
Convert all characters to strings: 0.305 sec.


A simple python3 calculator Made a very simple calculator in Python. The program has the following features:

All the basic operators can be used. '+', '-', '*', '**', '/', and '//'.
The user can start the program again.
The previous output is available for the user to use again.

def start(out='temp'):
    # Get the input from the user and pass all the values to verify.
    # Explicit test is used because the output can be zero.
    if out != 'temp':
        x = out
        print('\nFirst number: ', x)
    else:
        x = input('\nFirst number: ')
    op = input('Operator: ')
    y = input('Second number: ')
    verify(x, op, y)

def verify(x, op, y):
    # Check if the operator and the numbers entered are valid.
    # If any input is invalid call start and get input again.
    # If inputs are valid pass all the values to calc.
    ops = ['+', '-', '*', '**', '/', '//']
    if op in ops:
        try:
            x, y = int(x), int(y)
        except ValueError:
            print('Numbers are not valid.\n')
            start()
        else:
            calc(x, op, y)
    else:
        print('Please enter a valid operator.\n')
        start()

def calc(x, op, y):
    # Use eval to calculate the output and pass the output to
    # restart.
    out = eval(f'x {op} y')
    print('Output:', out)
    restart(out)

def restart(out):
    # User can start the process again. The previous output can be used as
    # the first number.
    re = input('Start again? (y/n): ')
    if re == 'y':
        use_out = input(
            'Use the previous output as the first number? (y/n): ')
        if use_out == 'y':
            start(out=out)
        else:
            start()
    else:
        print('Calc is now closed.')

start()

Here's a sample output.

First number: 5
Operator: *
Second number: 9
Output: 45
Start again? (y/n): y
Use the previous output as the first number? (y/n): y

First number:  45
Operator: //
Second number: 2
Output: 22
Start again? (y/n): n
Calc is now closed.

I am looking for ways to make the code more readable, minimize redundancy, improving the overall design, etc. Any help would be appreciated!

Your program is easy to read and does all the necessary input validation. That's good.

The error messages should be more helpful. If the user enters an invalid operator, you should tell them which operators are valid:

print(f'Please enter a valid operator: {' '.join(ops)}\n')

Every use of eval is dangerous. If you pass unvalidated input to it, users might be able to run arbitrary Python code. You currently do the validation in verify and the actual calculation in calc. That's nicely separated, but it can also lead to a situation where you later call calc by accident with unvalidated input. To avoid this, most calculator programs use a dictionary of operators:

binops = {
    '+': lambda a, b: return a + b,
    '-': lambda a, b: return a - b,
    # and so on
}

try:
    return binops[op](x, y)
except KeyError, e:
    print(f'Invalid operator {op!r}, valid operators are {sorted(ops.keys())}')

One unfortunate thing about the above code is that ops.keys() returns the operators in an unspecified order. Therefore I had to add the sorted call.

Each of your functions calls the continuation function at the end. When you try a long session with the calculator (about 5000 calculations), it will raise an exception. Whenever a function is called, Python remembers where it was called from, and it can remember only a few nested function calls. Therefore it is more common to use while loops for representing repetition. See https://stackoverflow.com/q/1359197 for more details.

As a user of the calculator, I don't like to enter the numbers and operators separately. As the next step the calculator should allow inputs like 3+5 and 7 - -9 and +7--9. You can do this by using regular expressions.

The current code asks many questions individually. Instead of asking whether to reuse the result from the last calculation, you could print the result in the form ans1 = 8 and allow the user to write expressions like ans4 * ans5. A calculator session might then look like:

> 123
ans1 = 123

> 456
ans1 = 456

> 3 + ans1
ans3 = 126

> ans3 + ans3
ans4 = 252

> result = ans4
result = 252

> result + result
ans5 = 504

This way the calculator remembers all previous results, and by using the = operator, you can name individual results and refer to them via that name, just like variables in Python. All you need for this is a dictionary and a counter (for the automatic variable names like ans4):

vars = {}
ans = 0

def store_var(name, value):
    vars[name] = value

def store_auto_var(value):
    global ans
    ans += 1
    name = 'ans' + ans
    vars[name] = value

These are the basic building blocks for building a really powerful calculator.


Trie structure with three classes I have implemented a trie in Java and I would request you to review it and suggest improvements or new features.

The code is split into 3 files.

TrieNode (individual nodes in the trie)

import java.util.HashMap;

class TrieNode {

    private char value;
    private HashMap<Character, TrieNode> children ;

    public TrieNode() {
        // TODO Auto-generated constructor stub
        children = new HashMap<>();
    }

    public TrieNode(char value) {
        this.value = value;
        children = new HashMap<>();
    }

    public char getValue() {
        return value;
    }

    public void setValue(char value) {
        this.value = value;
    }

    public HashMap<Character, TrieNode> getChildren() {
        return children;
    }

    public void setChildren(HashMap<Character, TrieNode> children) {
        this.children = children;
    }
}

Trie (the trie itself)

import java.util.Set;

public class Trie {

    private TrieNode root;

    public Trie() {
        // TODO Auto-generated constructor stub
        root = new TrieNode();
        root.setValue('$'); //arbitary node value for root
    }

    /**
     * This method will build the trie with the given set of words
     * @param wordSet
     */
    public void buildTrie(Set<String> wordSet){

        for (String word : wordSet){
            int wordLength = word.length();
            TrieNode crawl = root;
            for (int i=0;i<wordLength ;++i){
                crawl = traverseTrie(crawl, word.charAt(i));
            }
        }
    }

    /**
     * This method will traverse the trie and find out the exact position to
     * insert a particular character
     * @param node
     * @param input
     * @return
     */
    public TrieNode traverseTrie(TrieNode node,char input){
        TrieNode newNode = null;
        //if no such node exists then create one
        if (node.getChildren().get(input)==null){
            node.getChildren().put(input, new TrieNode(input));
            newNode = node.getChildren().get(input);
        }else{
            //if exist then just traverse
            newNode = node.getChildren().get(input);
        }
        return newNode;
    }

    /**
     * This method searches the trie for the given word and returns a boolean value
     * indicating it's presence or absence
     * @param word
     * @return {@link boolean} value indicating it's presence or absence
     */
    public boolean searchWord(String word) {
        // TODO Auto-generated method stub
        boolean result = true;
        int wordLength = word.length();
        TrieNode crawler = root;
        for (int i= 0 ; i< wordLength ; ++i){
            char character = word.charAt(i);
            if (crawler.getChildren().get(character) == null){
                result = false ;
                break;
            }else{
                crawler = crawler.getChildren().get(character);
            }
        }
        return result;
    }
    //getters and setters
    public TrieNode getRoot() {
        return root;
    }

    public void setRoot(TrieNode root) {
        this.root = root;
    }
}

TrieDriver (the driver program to run the trie)

import java.util.HashSet;
import java.util.Set;

public class TrieDriver {

    public static void main(String[] args) {
        // TODO Auto-generated method stub
        Trie trie = new Trie();
        Set<String> dictionary = new HashSet<>();
        dictionary.add("CAT");
        dictionary.add("CATWOMAN");
        dictionary.add("CATERER");
        dictionary.add("DOT");
        dictionary.add("DOG");
        dictionary.add("DOC");
        //build the trie
        trie.buildTrie(dictionary);
        String wordToSearch = "CAX";
        //search the trie
        System.out.println("The word "+wordToSearch + " is present or not???"+
                (boolean)trie.searchWord(wordToSearch));
    }

}

Using a concrete class where an abstract one or an interface can be used is usually a bad practice. That is, the getChildren method of the TrieNode class should return a Map<Character, Integer>, not a HashMap. There is no need to get bound to a concrete implementation.
The TrieNode class has redundant fields and methods. The value field is not used anywhere, so there is no need to keep it. Getters and setters for it are not necessary, too. Moreover, this classes exposes its internal representation. The clients of this class(in this it is the Trie class) do not really need to know how the children of this node are represented. Instead of returning a Map and letting the client work with it, I would create two methods: getChildForCharacter and getChildForCharacterOrCreateIfAbsent. Here is my version of this class:

class TrieNode {

    private Map<Character, Integer> children = new HashMap<>();

    /**
     * Returns the child of this node for the given character or null
     * if it does not exist.
     */
    public TrieNode getChildForCharacter(char c) {
        return children.get(c);
    }

    /**
     * Returns the child of this node for the given character. If it does
     * not exist, a new child is created.
     */
    public TrieNode getChildForCharacterOrCreateIfAbsent(char c) {
        if (getChildForCharacter(c) == null) {
            children.put(c, new TrieNode());
        }
        return children.get(c);
    }
};

Now this class does not expose its implementation and we can easily change it in the future if it is necessary. You could also add a isTerminal and setTerminal methods to this class if you want to search for the entire words, not just their prefixes.
The Trie class, again, in my opinion, contains a couple of methods that should not be there.

I think that the getRoot and setRoot method should be simply deleted. Why? Setting a new root breaks the trie(in a sense that previously added strings might turn out not to be there any) and the purpose of a getter is not clear(why would I need a trie's root?). Moreover, they expose the internal representation, which is not good. I'd just remove them. The clients of the Trie do not need to know that it has a root in the first place because it is an implementation detail.
traverseTree should be private(or at least protected). It is a helper method. There is no need to expose it to the clients. They don't even know what a TrieNode is(because this class is not public).
buildTrie is not a very good name. It implies that this method should be called only once(then the question is: why isn't it a constructor?). But it makes perfect sense to add new words to a trie. That's why I'd rename it to addAll or something like this. I would also add a method add which takes one String argument(to avoid making the clients of this class create a Set even if they want to add only one String.

Spacing and indentation. It is inconsistent in your code. Sometimes whitespaces around binary operators are absent, sometimes there are arbitrary whitespaces before a semicolon. You should keep you style consistent and adhere to established guidelines. It is easy to fix it using almost any modern Java IDE.
Testing. If do not use unit-tests, start using them. Testing your code in a consistent, systematic manner is a very good practice.

 Summary

Avoid exposing the internal representation of objects. Do not create getters and setters for the sake of having them. It is a bad practice.
Keep your code style consistent and follow established guidelines unless you have very good reasons not to.
Keep a consistent level of abstraction in your API. Try not to mix concepts from different levels(like a node in a trie and a HashMap).


Recursive download articles and dump to MongoDB This is simple code that recursively downloads articles from the public WordPress API:

import requests

from pymongo import MongoClient
client = MongoClient()
db = client.wp

payload = {
    'number': 100
}

url = 'https://public-api.wordpress.com/rest/v1.1/sites/{site_id}/posts'
response = requests.get(url, params=payload).json()
found = response['found']
posts = response['posts']
print 'found {number} posts'.format(number=found)
db.posts.insert_many(posts)

def get_next_page(offset):
    print 'Inserted we have {left} posts left to go'.format(left=abs(found-offset))
    response = requests.get(url, params= {
        'number': 100,
        'offset': offset
    }).json()

    db.posts.insert_many(response['posts'])

    if int(response['found']) > offset:
        get_next_page(offset + 100)

get_next_page(100)

The logic for getting 100 posts is duplicated for the case of the initial load and subsequent loads with an offset. It would be better to refactor in a way to eliminate the duplicated logic:

params = {'number': 100}
offset = 0

while True:
    response = requests.get(url, params=params).json()
    found = response['found']
    posts = response['posts']
    db.posts.insert_many(posts)
    print 'Inserted {number} posts'.format(number=len(posts))
    offset += 100
    if offset >= found:
        break
    print 'We have {left} posts left to go'.format(left=found-offset)
    params['offset'] = offset


Improve search keyword I've been given the following PHP code:

if(empty($search)){
        $_SESSION['keyword_exists'] = "no";
    }else{
        if(isset($_SESSION['old_keyword']) && $_SESSION['old_keyword'] != $search){
            $_SESSION['keyword_exists'] = "no";
        }
    }
    if(isset($_REQUEST['limitstart']) && $_REQUEST['limitstart'] > 0){
        if (!empty($search)) {
            if(isset($_SESSION['keyword_exists']) && $_SESSION['keyword_exists'] === "yes"){
                //intentionally left blank?
            }else{
                $limitstart = "0";
                $_SESSION['keyword_exists'] = "yes";
                $_SESSION['old_keyword'] = $search;
            }
        }
    }else{
            $limitstart = "0";
    }

I'm not entirely sure what this code does, as I'm fairly new to PHP and webdev.  The one comment is mine.  Is there any way to clean this up so that the next guy to see this is less confused than I am?

Let's start with the basics:

$_SESSION: This is a global array that holds all your session information.
$_REQUEST: This is a global array that contains the contents of $_GET, $_POST and $_COOKIE. Read more on PHP's superglobals.

if( empty($search) ) {

    $_SESSION['keyword_exists'] = "no";

} else {

    if( isset($_SESSION['old_keyword']) && $_SESSION['old_keyword'] != $search ) {
        $_SESSION['keyword_exists'] = "no";
    }

}

The outer if depends on whether the $search variable is empty or not. Empty in context could mean:

  "" (an empty string)
  0 (0 as an integer)
  0.0 (0 as a float)
  "0" (0 as a string)
  NULL
  FALSE
  array() (an empty array)
  var $var; (a variable declared, but without a value in a class)

If $search is empty, $_SESSION['keyword_exists'] is set to "no" (can't read the author's mind, but my choice would be false, not a string). From the naming we can assume that $search holds (or not) the keyword(s) of a search. Now if that is not empty:

    if( isset($_SESSION['old_keyword']) && $_SESSION['old_keyword'] != $search ) {
        $_SESSION['keyword_exists'] = "no";
    }

The first clause, isset($_SESSION['old_keyword']) essentially checks if there's an "old_keyword" index in the $_SESSION array (and whether it's null or not), that's a pretty typical check for arrays. The second check, that executes if and only if the first one passes, checks whether what's in $_SESSION['old_keyword'] is not the same as what's in $search. I'm assuming that the author had some reason for that, but can't imagine what that reason is.

Summarizing what happens here, if:

$search is empty, or
$search is not the same as $_SESSION['old_keyword']

...then $_SESSION['keyword_exists'] = "no". A perhaps simpler way to write that would be:

if(
    empty($search)
    || ( isset($_SESSION['old_keyword']) && $_SESSION['old_keyword'] != $search )
) {
    $_SESSION['keyword_exists'] = "no";
}

Moving on to the next part:

if( isset($_REQUEST['limitstart']) && $_REQUEST['limitstart'] > 0 ) {

    ...

} else {
        $limitstart = "0";
}

The clause is very similar to the one discussed previously, only this time other than checking if $_REQUEST has a "limitstart" index, the author also checks that the value is larger than zero. That's an unsafe check, because at this point we don't now what the type of the value in $_REQUEST['limitstart'] is, and if it's anything other than a number, there will be automatic type juggling involved, and the check is completely unreliable.

From the name and context, I'm assuming the variable should hold an integer (if anything) that limits the search. If the variable doesn't hold anything, the limit is set to zero ($limitstart = "0"), curiously using a string form of zero. I'd rewrite that check as:

$limitstart = 0;

if(
    isset($_REQUEST['limitstart'])
    && ( is_int($_REQUEST['limitstart']) || ctype_digit($_REQUEST['limitstart']) )
    && $_REQUEST['limitstart'] > 0
) {

    ...

}

is_int() checks whether the value is an integer and ctype_digit() whether it's a string that only contains digits (thus a integer in string form), any of the two is acceptable for the following check, $_REQUEST['limitstart'] > 0. I've also moved $limitstart = 0; out of the check, I'm initializing it to zero and will override if and only if there's a need. But let's see what happens if the check is true:

    if ( !empty($search) ) {
        if( isset($_SESSION['keyword_exists']) && $_SESSION['keyword_exists'] === "yes" ) {
            //intentionally left blank?
        } else {
            $limitstart = "0";
            $_SESSION['keyword_exists'] = "yes";
            $_SESSION['old_keyword'] = $search;
        }
    }

This is obviously incomplete. The outer check is simple enough, it's whether $search is empty or not. If it's empty, nothing happens, if it's not, the inner check is on whether _SESSION has a "keyword_exists" index, and if it's value is "yes". If that's true, something happens, but who knows what? I'm assuming one of the things that would happen would be to set a proper value to $limitstart. Anyways, if the check is false:

$limitstart remains zero,
$_SESSION['keyword_exists'] for some reason becomes "yes", and
$_SESSION['old_keyword'] becomes $search.

Unfortunately, I have no idea why. Anyways, my full rewrite would be:

if(
    empty($search)
    || ( isset($_SESSION['old_keyword']) && $_SESSION['old_keyword'] != $search )
) {
    $_SESSION['keyword_exists'] = "no";
}

$limitstart = 0;

if(
    isset($_REQUEST['limitstart'])
    && ( is_int($_REQUEST['limitstart']) || ctype_digit($_REQUEST['limitstart']) )
    && $_REQUEST['limitstart'] > 0
) {

    if ( !empty($search) ) {
        if( isset($_SESSION['keyword_exists']) && $_SESSION['keyword_exists'] === "yes" ) {
            //intentionally left blank?
        } else {
            $_SESSION['keyword_exists'] = "yes";
            $_SESSION['old_keyword'] = $search;
        }
    }

}

The code is equivalent, and will work (?) if you replace it in your script. Hope it clarifies things a bit. The overall quality of the code is bad, there are some hints of an amateur developer there, and you shouldn't really worry that you didn't grasp what the code does, since you are unfamiliar with the language. It's an incomplete and mostly poorly written piece of code, good luck with it ;)


SE-like voting system I've created a voting system that's similar to Stack Exchange's in a Ruby on Rails web app. It's working great - as far as I can tell, all the edge cases are caught. This is the action to cast a vote.

There are some conditions I've had to satisfy:

must not allow duplicate voting, even from console requests
one vote per post (can't have an up and a down vote on the same post)
must be logged in

At the moment, I'm updating the record if there's already a vote by this user on this post, but I wonder if that's the most efficient strategy. Would destroying the old row and creating a new record be faster?

I'd particularly appreciate efficiency comments - at the moment, this is a very server heavy action, which I'd like to minimise.

def create
  post = (params[:post_type] == "a" ? Answer.find(params[:post_id]) : Question.find(params[:post_id]))
  existing = Vote.where(:post => post, :user => current_user)

  if existing.count > 0
    if existing.first.vote_type == params[:vote_type].to_i
      render :plain => "You have already voted.", :status => 409 and return
    else
      # There's already a vote by this user on this post, so we may as well update that instead of removing it.
      vote = existing.first
      vote.vote_type = params[:vote_type].to_i
      vote.save!

      state = { :status => "modified", :vote_id => vote.id }
      render :json => state and return
    end
  end

  vote = Vote.new
  vote.user = current_user
  vote.post = post
  vote.vote_type = params[:vote_type]
  vote.save!

  state = { :status => "OK", :vote_id => vote.id }
  render :json => state and return
end

(A quick note on vote types: the Vote.vote_type column has type integer, and the possible values are 0 (upvote) or 1 (downvote)).

First of all, I'd strongly encourage you to change your vote-scheme to use a value attribute of either 1 or -1, rather than a vote_type that can be 0 or 1.
A) because +1/-1 is what the votes actually mean, and B) because it makes it a heck of a lot easier to calculate the total. You just have to do:

score = some_post.votes.sum(:value)

Obviously, it'd still be a good idea to cache the score somewhere when appropriate. The database is fast, but retrieving a cache is faster.

In the following I've assumed it's set up like that, because it makes a ton more sense.

I would also recommend that you just have 1 post model; not an Answer and a Question as separate models. I'd wager the two are practically identical anyway, and it'd mean you don't have to find one or the other. You can just call Post.find(id) and done. For this especially, there's no difference between a question and an answer.

Second, you can use .find_or_initialize_by for this:

vote = post.votes.find_or_initialize_by(user: user)
if vote.value == params[:value].to_id
  # already voted
else
  # never voted (new vote record) or changing the vote
  vote.value = params[:value].to_id
  vote.save!
end

If you can skip loading the post, that'll save you some time too (however minor), since you don't actually need the post itself for anything in particular. You just need its ID for reference. Of course, it's safer to load the post, just so you know it exists (otherwise, I could preemptively up- or downvote future posts by passing a large post ID to the action). But even so, you can add a .select(:id) to the query, so you don't waste time loading all the attributes you don't need here and now - e.g. a large piece of text content or similar. And if you have eager-loading associations, nerf them. Again, you only need to post's ID.

Alternatively, you can set up a uniqueness constraint on the post_id/user_id columns, and - if your database supports it! - use a bit of raw SQL:

INSERT INTO votes (post_id, user_id, value) VALUES (?, ?, ?)
  ON DUPLICATE KEY UPDATE value = ?;

You'd want to safely insert the right values in the sql, maybe add values for created_/updated_at, etc..

The above would just insert a new row if the user hasn't voted before, but if he/she has, the constraint is triggered, and it performs an update on the existing row instead.

The major drawback is of course that you don't get any immediate feedback from the database about what happened. You can be sure that the database is correct, but you won't be able to send the 409 Conflict. And again, not all databases support this.

You can also use a uniqueness constraint to trigger a RecordNotUnique exception: Attempt to insert a new record, and if the exception is raised, rescue it and update instead. Sort a manual, Rails-side version of ON DUPLICATE KEY UPDATE.

In any case, I wouldn't worry too much about performance for this. A database is fast, and voting is relatively rare (a post is likely viewed many more times than votes are cast on it). You're probably spending 10x as much time in Rails code, as the database is spending doing all the selects, inserts and updates you throw at it.


Solving the knapsack problem with user provided input This question was given in my college assignment to be done in C. After doing that, I rewrote the program again in rust.
I come from a C/Python background and this is my first rust program. Please critique it.
The Program is given inputs in the following format

{num_items} {max_weight}
{item_weight_1} ... {item_weight_num_items}

The output is the number of items and the indices of the chosen items to be put in the bag

use std::io::{self, BufRead};

fn main() {
    let reader = io::stdin();
    let top_line: Vec<i32> = reader
        .lock()
        .lines()
        .next()
        .unwrap()
        .unwrap()
        .trim()
        .split(' ')
        .map(|s| s.parse().unwrap())
        .collect();
    let num_items: i32 = top_line[0];
    let max_weight: i32 = top_line[1];
    let items: Vec<i32> = reader
        .lock()
        .lines()
        .next()
        .unwrap()
        .unwrap()
        .trim()
        .split(' ')
        .map(|s| s.parse().unwrap())
        .collect();
    let s: (i32, i32) = (1..1 << num_items)
        .map(|i| {
            (
                items
                    .iter()
                    .enumerate()
                    .filter(|&(t, _)| (i >> t & 1 == 1))
                    .map(|(_, elem)| elem.clone())
                    .sum::<i32>(),
                i,
            )
        })
        .map(|(elem, i)| (max_weight - elem, i))
        .filter(|&(elem, _)| (elem >= 0))
        .min_by(|(a, _), (b, _)| a.cmp(&b))
        .unwrap();
    let ans: Vec<i32> = (0..num_items)
        .filter(|&t| (s.1 >> t & 1 == 1))
        .map(|t| t + 1)
        .collect();
    println!("{}", ans.len());
    println!(
        "{}",
        ans.iter()
            .fold(String::new(), |acc, &arg| acc + &arg.to_string() + " ")
    );
}

Example input

6 44
14 13 21 8 56 3

Example output

3
1 4 3

General

You don't have any comments. I recommend you document what basic blocks of code are doing
I recommend factoring out the parsing and algorithm parts of your code into different functions.
Why prompt the user for the number of input items? You can trivially detect it from the second line input, unless this specifies a different value.
Non-descriptive variable names: What does s contain? What does ans mean?
The itertools crate is super helpful whenever iterators are involved

Input Capture / Parsing

Read in both lines at once: let lines = io::stdin().lock().lines().take(2);
Map over those lines, parsing the integers:

let mut numbers = lines.map(|line| {
    line
    .trim()
    .split(' ')
    .map(|s| s.parse::<i32>().unwrap())
});

Split the first line:

let first_line = numbers.next().unwrap().take(2).collect::<Vec<_>>();
assert_eq!(first_line.len(), 2);

let num_items = first_line[0];
let max_weights = first_line[1];

And grab the items from the second line: let items = numbers.next().unwrap().collect::<Vec<_>>();

Algorithm Part 1 (s)

I'd specify the range as 1..(1 << num_items) to really make clear that second 1 is being shifted, not the whole range.
Personally, I prefer my tuples to be easily spotted. I'd set the iteration result a variable then use that in the tuple:

.map(|i| {
    let sum = items
        .iter()
        .enumerate()
        .filter(|&(t, _)| (i >> t & 1 == 1))
        .map(|(_, elem)| elem.clone())
        .sum::<i32>();
    (sum, i)
})

It's unclear what .filter(|&(t, _)| (i >> t & 1 == 1)) is doing here, but usually bitwise comparisons can be expressed more clearly with other operations.
Additionally, i and t should probably be renamed to be more clear.
Also, those extra parenthesis around the closure return value should instead be used to group whichever operation takes precedence for clarity of the expression
.map(|(_, elem)| elem.clone()) I think can be replaced with .map(|(_, &elem)| elem)
Recommend returning (i, sum) instead of (sum, i) to better match the pattern of enumerate
Extra parenthesis around the return value in .filter(|&(elem, _)| (elem >= 0))
I think .min_by(|(a, _), (b, _)| a.cmp(&b)) can be replaced with .min_by_key(|(x, _)| x)
Doesn't look like you ever use s.0 so you can ignore it with let (_, s): (i32, i32) = ...
In fact, it doesn't look like you use the i index at all after the first .map(|i| { so you don't need to pass it through the rest of the chain.

Algorithm Part 2 (ans)

You repeat (x >> t & 1 == 1) twice: here and above. Perhaps move it to a clearly-named function?
I'd store the ans.iter().fold(...) in a variable before printing the result. Makes the eventual format string output easier to predict.
Suggest using format! in the fold:

.fold(|out, &num| format!("{}{} ", out, num))

Alternatively, you could use join instead of the fold:

ans.into_iter().map(|x| x.to_string()).collect::<Vec<_>>().join(" ")


Searching a Join List for an index Attempt #2 Originally I posted this incorrect attempt - Searching a Join List for an index.

Given a JoinList:

data JoinList m a = Empty
                  | Single m a
                  | Append m (JoinList m a) (JoinList m a)
    deriving (Eq, Show)

where m represents the size of the JoinList, find the element (Maybe a) at the given index in the JoinList structure.

indexJ :: (Sized b, Monoid b) => Int -> JoinList b a -> Maybe a
indexJ _ Empty             = Nothing
indexJ i (Single _ x)
  | i == 0                 = Just x
  | otherwise              = Nothing
indexJ i (Append _ left right)
  | leftHt > i             = indexJ i left
  | (rightHt + leftHt) > i = indexJ (i-leftHt) right
  | otherwise              = Nothing
  where leftHt  = (getSize. size . tag) left
        rightHt = (getSize. size . tag) right

where tag =

tag :: Monoid m => JoinList m a -> m
tag Empty          = mempty
tag (Single x _)   = x
tag (Append x _ _) = x

Testing

Data

jlIndex2 :: JoinList Size String
jlIndex2 = Append (Size 3)
                       (Single (Size 1) "foo")
                       (Append (Size 2)
                             (Single (Size 1) "bar")
                             (Single (Size 1) "baz"))

jlIndex3 :: JoinList Size String
jlIndex3 = Append (Size 4) (Single 1 "biz") jlIndex2

Tests

*JoinList> indexJ 3 jlIndex3
Just "baz"
*JoinList> indexJ 0 jlIndex3
Just "biz"
*JoinList> indexJ 100 jlIndex3
Nothing

Note - I would've used QuickCheck, but I'm not sure how to use it with algebraic data types.

Your code is completely fine, although the suffix Ht for the sublist sizes is a little bit strange.

For the tests you can use

genJoinList :: Arbitrary a => Int -> Gen (JoinList Size a)
genJoinList 0 = pure Empty
genJoinList 1 = Single (Size 1) <$> arbitrary
genJoinList n = do
  left <- choose (0, n)
  Append (Size n) <$> (genJoinList left) <*> (genJoinList (n - left))

toList :: JoinList b a -> [a]
toList Empty          = []
toList (Single _ x)   = [x]
toList (Append _ l r) = toList l ++ toList r

You can now check whether indexJ n js returns the same value as toList js !! n:

prop_index :: (Eq a, Sized m, Monoid m) => Int -> JoinList m a -> Bool
prop_index n js = indexJ n js == indexI n (toList js)
  where
    indexI n xs = lookup n $ zip [0..] xs

main = quickCheck $ \n -> forAll (sized genJoinList) $ \js -> prop_index n js

An Arbitrary instance would need FlexibleInstances by the way.


Doubly circular linked list implementation I made this C++ doubly circular linked list implementation and it seems to work right, handle everything well, and not have any errors. However, I've been working with it for a while and would like some fresh eyes to take a look and see if I am missing anything large or if my code contains any 'gotchas'.

Side note: I ran valgrind on several tests of this implementation and never got any memory leaks so I suspect it is handling that end of this correctly.

CLL.h

#ifndef CLL
#define CLL

class CircularLinkedNode {
public:
    int info;
    CircularLinkedNode *next, *prev;

    CircularLinkedNode() {
        next = prev = this;
    }

    CircularLinkedNode(int num) {
        info = num;
        next = prev = this;
    }

    CircularLinkedNode(int nu, CircularLinkedNode *ne, CircularLinkedNode *pre) {
        info = nu;
        //auto put new node in the middle of next and previous
        next = ne;
        prev = pre;
        prev->next = this;
        next->prev = this;
    }

    ~CircularLinkedNode() {
        //when deleted, seal the gap
        //prev->next = next;
        //next->prev = prev;
    }
};

class CircularLinkedList {
private:
    CircularLinkedNode *head, *tail;
public:
    CircularLinkedList() {
        head = tail = 0;
    }

    ~CircularLinkedList();

    bool isEmpty() const {
        return head == 0;
    }

    void addToHead(int);
    void addToTail(int);
    int removeFromHead();
    int removeFromTail();
    void remove(int);
    bool isInList(int) const;

};

#endif

CLL.cpp

#include <iostream>
#include "CLL.h"

CircularLinkedList::~CircularLinkedList() {
if (isEmpty()) {
    return;
}
if(head == tail) {
    delete head;
    head = tail = 0;
    return;
}

    CircularLinkedNode *tmp = head->next;
    delete head;
    while(tmp != tail) {
        head = tmp;
        tmp = tmp->next;
        delete head;
    }
    delete tail;

}

void CircularLinkedList::addToHead(int n) {
    isEmpty() ? head = tail = new CircularLinkedNode(n) : head = new     CircularLinkedNode(n, head, tail);
}

void CircularLinkedList::addToTail(int n) {
    isEmpty() ? tail = head = new CircularLinkedNode(n) : tail = new CircularLinkedNode(n, head, tail);
}

int CircularLinkedList::removeFromHead() {
    if(isEmpty()) {
        return 0;
    }

    int el = head->info;
    if(head == tail) {
        delete head;
        head = 0;
        return el;
    }

    CircularLinkedNode *tmp = head;
    head->prev->next = head->next;
    head->next->prev = head->prev;
    head = head->next;
    std::cout << "delete" << std::endl;
    delete tmp;
    return el;
}

int CircularLinkedList::removeFromTail() {
    if(isEmpty()) {
        return 0;
    }

    int el = tail->info;
    if(head == tail) {
        delete tail;
        tail = 0;
        return el;
    }
    CircularLinkedNode *tmp = tail;
    tail->prev->next = tail->next;
    tail->next->prev = tail->prev;
    tail = tail->prev;
    std::cout << "delete" << std::endl;
    delete tmp;
    return el;
}

void CircularLinkedList::remove(int n) {
    if (!isEmpty()) {
        if (head->info == n) {
            removeFromHead();
            return;
        } else if (tail->info == n) {
            removeFromTail();
            return;
        } else {
            CircularLinkedNode *tmp;
            for (tmp = head; tmp != tail && !(tmp->info == n); tmp = tmp->next);
            if(tmp != tail) {
                tmp->prev->next = tmp->next;
                tmp->next->prev = tmp->prev;
                delete tmp;
            }
        }

    }
}

bool CircularLinkedList::isInList(int el) const {
    CircularLinkedNode *tmp;
    for (tmp = head; tmp != tail && !(tmp->info == el); tmp = tmp->next);
    return (tmp->info == el);
}

Heads and tails
I got confused reading this code because in my mind, a circular list doesn't have a tail.  Or at least, the tail is the same as the head.
The code appears to be correct, but you could have only kept a head pointer.  Anywhere where you use tail, you could essentially replace that with head->prev and get the same functionality.


Program to automate clicking on the new button on a chat website until the username matches certain conditions This is a python program that automates skipping through users on a chat website called
talkwithstrangers using the Selenium Webdriver and BeautifulSoup modules of python.

The first part uses selenium to open the browser

from selenium import webdriver
driver = webdriver.Chrome('C:\SeleniumDrivers\chromedriver.exe')
userName = 'null'
driver.get('https://talkwithstranger.com/talk-to-strangers')

The second part is a while loop that run while the username has not met the required conditions,

while 'female' not in userName or 'Female' not in userName:
    driver.get('https://talkwithstranger.com/talk-to-strangers')
    html = driver.execute_script("return document.documentElement.innerHTML;")

    from bs4 import BeautifulSoup
    soup = BeautifulSoup(html, 'html5lib')
    userVal = soup.find(class_='alert text-center alert-success')
    userNames =  userVal.find_all('b')

    def getName(userNames):
        for name in userNames:
            return name.prettify()

    userName = getName(userNames)

The code is running fine but the problem is the efficiency and runtime.

Your third part is wrong, I think.

nonCompatible = 'female' not in name

will only run against the value of name at the time of execution, so nonCompatible will end up either True or False, and won't change unless you reset it elsewhere in the while loop.  You may want to either inline it:

while 'female' not in name:
    ...

or abstract it:

def nonCompatible(nym):
    return 'female' not in nym

while nonCompatible(name):
    ...


modifying local environment in python I need to inject same variables into settings.py for multiple django apps.hence i wrote a module which takes in output of local() and modifies. This works but is it the right way to do things?

def enable_theme(theme_name, container):
"""
Enable the settings for a custom theme, whose files should be stored
in ENV_ROOT/themes/THEME_NAME (e.g., edx_all/themes/stanford).

The THEME_NAME setting should be configured separately since it can't
be set here (this function closes too early). An idiom for doing this
is:

   THEME_NAME = "stanford"
   enable_theme(THEME_NAME)
"""
    container["THEME_NAME"] = theme_name
    container['MITX_FEATURES']['USE_CUSTOM_THEME'] = True
    # Calculate the location of the theme's files
    theme_root = container['THEME_ROOT'] / "themes" / theme_name
    # Include the theme's templates in the template search paths
    container['TEMPLATE_DIRS'].append(theme_root / 'templates')
    container['THEME_SASS_DIRECTORY'] = theme_root / 'static/saas'
    container['MAKO_TEMPLATES']['main'].append(theme_root / 'templates')
    # Namespace the theme's static files to 'themes/<theme_name>' to
    # avoid collisions with default edX static files
    container['STATICFILES_DIRS'].append((u'themes/%s' % theme_name,
                                   theme_root / 'static'))
    container['FAVICON_PATH'] = 'themes/%s/images/favicon.ico' % theme_name

#Test case
from theming_utils.template_customizer import enable_theme
THEME_NAME = "sokratik"
PIPELINE = False
MITX_FEATURES['USE_DJANGO_PIPELINE'] = False
DEBUG = True
TEMPLATE_DEBUG = True
enable_theme(THEME_NAME, locals())

You're not trying to modify a local environment, you're trying to modify the module's dictionary, which you can access by calling globals().

Note: the dictionary returned by locals() isn't meant to be modified.


Am I on the right track? PHP/MySQL I have the following table called info:

info_name   |   info_value

name        |   Susan

desc        |   Human

I'm trying to print Susan without knowing that the value is Susan.

The following code works, but I want to know if what I wrote is correct.

I'm open to any improvements.

Config Class:

class Config {

    private $hostname = 'localhost';
    private $username = 'blah';
    private $password = 'blah';
    private $database = 'blah';

    public function __construct()
    {
        $this->connection = new mysqli($this->hostname,$this->username,$this->password,$this->database);

        if($this->connection->connect_errno)
        {
            die('Error: ' . $this->database->error);
        }
    }

    public function query($query)
    {
        return $this->connection->query($query);
    }

    public function __destruct()
    {
        $this->connection->close();
    }

}

Name Class:

class Name {

    protected $name;

    public function __construct()
    {
        $this->db = new Config;

        $r = $this->db->query('SELECT info_value FROM info WHERE info_name="name"');

        while($row = $r->fetch_row())
        {
            $this->name = $row['0'];
        }

    }

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

}

Print Results:

<?php $show_name = new Name(); echo $show_name->getName(); ?>

Thanks for any help!

Your config and name classes are good starts. Here are a few humble suggestions:

I highly suggest steering clear of what is called the entity-attribute-value pattern when structuring your MySQL tables. Here is an article that discusses ways of refactoring. Also, if you are interested in learning more about mysql (and sql in general) I highly recommend the book SQL Antipatterns by Bill Karwin.
In the future, if your projects grow and you want a more robust system, you may want to look into a few concepts such as dependency injection, which will help if and (hopefully) when you ever start unit testing with software like PHPUnit.

Hope this helps!


Compare and Process Common and Uncommon files separately I have the following files:

STUDENT_D19001.T103412_HU.txt
STUDENT_D19001.T103412_KU.txt
STUDENT_D19002.T113030_HU.txt
STUDENT_D19002.T115055_KU.txt
STUDENT_D19003.T115055_KU.txt
STUDENT_D19004.T135040_HU.txt

I want to process the common/pair files separately in this order (HU1 and then KU1; HU2 and then KU2) and the uncommon/unpaired files separately.

These two files (STUDENT_D19001.T103412_HU.txt and STUDENT_D19001.T103412_KU.txt) are common/pair files because they have same day of the year 001 in their name. However, these two files (STUDENT_D19003.T115055_KU.txt and STUDENT_D19004.T135040_HU.txt) are uncommon/unpaired because the do not have the same day of year with other files.

The output of the script is illustrated below for further clarification:

*** Uncommon/Unpaired files: ***
Processing './STUDENT_D19003.T115055_KU.txt'
Processing './STUDENT_D19004.T135040_HU.txt'

*** Common/Pair files: ***
1 of : 4 './STUDENT_D19001.T103412_HU.txt'
2 of : 4 './STUDENT_D19001.T103412_KU.txt'
3 of : 4 './STUDENT_D19002.T113030_HU.txt'
4 of : 4 './STUDENT_D19002.T115055_KU.txt'

Here is the script for review:

#!/bin/bash

SOURCE_DIR="."

#
# Create an array of all the HU files
#
HU_files=()
while IFS='' read -r -d '' file
do
    HU_files+=("$file")
done < <(find "$SOURCE_DIR" -maxdepth 1 -type f -iname "*HU*" -printf '%p\0' | sort -zn)

#
# Calculate HU total files
#
total_HU="${#HU_files[@]}"

#
# Create an array of all the BFV files
#
KU_files=()
while IFS='' read -r -d '' file
do
    KU_files+=("$file")
done < <(find "$SOURCE_DIR" -maxdepth 1 -type f -iname "*KU*" -printf '%p\0' | sort -zn)

#
# Calculate KU total files
#
total_KU="${#KU_files[@]}"

# Find common/pair HU and KU files considering partial part name 'STUDENT_D19001'
COMMON_files=()
i=0
k=0

while [ $i -lt $total_HU ]
do
    j=0
    while [ $j -lt $total_KU ]
    do
        if [[ "${HU_files[$i]/.T*/}" == "${KU_files[$j]/.T*/}" ]]
        then
            COMMON_files[$((k++))]="${HU_files[$i]}"
            COMMON_files[$((k++))]="${KU_files[$j]}"
            break
        else
            ((j++))
        fi
    done
    ((i++))
done

# Process uncommon/unpaired HU and KU files
echo "*** Uncommon/Unpair files: ***"
while IFS='' read -r -d '' file
do
    if [[ ! "${COMMON_files[@]}" =~ "${file}" ]]
    then
    echo "Processing '$file'"
    fi
done < <(find "$SOURCE_DIR" -maxdepth 1 -type f \( -iname "*HU*.txt" -o -iname "*KU*.txt" \) -printf '%p\0' | sort -zn)

#
# Process the common/pair HU and KU files
#
total_COMMOM="${#COMMON_files[@]}"
counter=0
echo "*** Common/Pair files: ***"
for file in "${COMMON_files[@]}"
do
    echo "$((++counter)) of $total_COMMOM: '$file'"
done

Your inputs are appreciated :-)

Per shell checkers feedback, I suggest the following changes:

Fix#1
To prevent globing and word splitting it is recommended to use double quote. Hence, update this while [ $i -lt $total_HU ] to while [ $i -lt "$total_HU" ], and this while [ $j -lt $total_KU ] to while [ $j -lt "$total_KU" ]. I am aware we are dealing with number rather than string but no harm to double quote variables.

Fix#2
Per SC2199 and SC2076 this line of code if [[ ! "${COMMON_files[@]}" =~ "${file}" ]] is problematic, therefore, I changed it to if [[ ! "${COMMON_files[*]}" =~ ${file} ]].

In this context I am sure that I want to match literally so I could also use a trick like this if [[ ! "${COMMON_files[*]}" =~ "${file}"* ]].


Factory for classes unknown at compile time I have a class called Machine. It contains a list of parts and a list of actions. The list of parts will contain instances of the Part class subclasses and the list of actions will contain instances of my Action class subclasses. Each Part subclass will have its corresponding Action subclass. None of the subclasses are available at compile time. When a Machine instance starts all its parts should perform their action.

An action will perform the same manipulation for each Part subclass so I am trying to create a factory class that will create a single instance for each Action subclass. My first thought was to use an interface for Action, but then I would have to create an Action instance for each Part instance I create, or make each class that implemented Action responsible for making itself a singleton. I am not sure if a factory is the best solution and if I am implementing the pattern correctly.

Neither Action nor Part subclasses are available to me so I am using generics. I am new to Java so I am not sure if I am using them correctly - Eclipse gives me several Raw type warnings (I indicated where). Here is my code:

import java.util.ArrayList;

public class GenericsTest
{
    public static void main(String[] args) throws InstantiationException,
            IllegalAccessException
    {

        ArrayList<Part> parts = new ArrayList<Part>();

        Rotate rotate = ActionFactory.getAction(Rotate.class);
        Move move = ActionFactory.getAction(Move.class);

        int i;
        for (i = 0; i < 1000; i++)
        {
            Gear gear = new Gear();
            gear.value1 = i;
            gear.actions.add(rotate);
            parts.add(gear);

            Lever lever = new Lever();
            lever.value2 = i + 2000;
            lever.actions.add(move);
            parts.add(lever);
        }

        Machine machine = new Machine();
        machine.parts.addAll(parts);
        machine.start();
    }

}

class Machine
{
    public void start()
    {
        for (Part part : parts)
        {
            // Raw type warning
            for (Action action : part.actions)
            {
                // Raw type warning
                action.execute(part);
            }
        }
    }

    public ArrayList<Part> parts = new ArrayList<Part>();
}

abstract class Part
{
    // Raw type warning
    public ArrayList<Action> actions = new ArrayList<Action>();
}

abstract class Action<T extends Part>
{
    abstract public void execute(T part);
}

class ActionFactory<T extends Action>
{

    public static <T> T getAction(Class<T> c) throws InstantiationException,
            IllegalAccessException
    {
        T returnAction = null;

        for (Action action : actions)
        {
            if (action.getClass() == c)
            {
                // Unchecked cast warning
                returnAction = (T) action;
            }
        }

        if (returnAction == null)
        {
            returnAction = c.newInstance();
            actions.add((Action) returnAction);
        }

        return returnAction;
    }

    private static ArrayList<Action> actions = new ArrayList<Action>();
}

/*
 * Subject1, Subject2, Action1 and Action2, somewhere else:
 */

class Gear extends Part
{
    public int value1;
}

class Lever extends Part
{
    public int value2;
}

class Rotate extends Action<Gear>
{

    @Override
    public void execute(Gear part)
    {
        System.out.println("rotate action executed, value 1: " + part.value1);
    }
}

class Move extends Action<Lever>
{

    @Override
    public void execute(Lever part)
    {
        System.out.println("move action executed, value 2: " + part.value2);
    }
}

I don't understand exactly the purpose of the code but having two similar object inheritance tree (Actions and Parts) smells a little bit. It also seems a sample code and I guess it violates the single responsibility principle (the behaviour of a part is separated to two classes) and it might also violates the Liskov substitution principle too (if you have an Action you can't use it with any kind of Part). I'd consider moving the action code to the Part class or creating generic Action classes which ignore unhandled Parts (by class type). Anyway, it's hard to say something more useful without the purpose of the code. See also: SOLID, Parallel Inheritance Hierarchies.

Other notes:

ArrayList<...> reference types should be simply List<...>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

List<Part> parts = new ArrayList<Part>();

Public fields usually leads to harder maintenance. See: What's the deal with Java's public fields?

Some other ideas to consider:

Create a separate Action instance for every part and action. Pass the Part instance to the constructor of your Action. For example:

Action rotate = new Rotate(gear);

Store the actions in a list and then you can call execute on the rotate instance:

for (final Action action: actions) {
    action.execute();
}

It's type-safe, clients can't create an Action with a wrong Part type.
I'd move the actions list from the Part object to somewhere else. Why should a part know what actions does a machine do with it? It seems to me that this responsibility should be stored somewhere else. The Macine is the closest object now (but it might deserve separate class).
If the Action classes do not have any state consider moving their code inside the Part object. (The basic idea of OOP is that the data and the logic which operates with the data should be together in a class.) You can mark the action method with an @Action annotation and parse it runtime.

public class Gear {

    ...

    @Action
    public void rotate() {
        ...
    }
}

I guess you still need a class which stores the actions which a machine should do:

public class Action {
    private Part part;
    private String actionMethodName;
}


Writing unit tests for exception should be thrown case Can anyone here comment on the quality of this test case? I am here testing for the exception should be thrown scenario. I mean it works but is it the correct way to unit test for the scenarios where the expectation is that exception should be thrown?

it('should throw exception if config.env.json is malformed', async () => {
  // Arrange: beforeEach
  const fakeFsStub = sinon.stub(File, 'readFileAsyc');
  fakeFsStub.withArgs('./src/configuration/config.json').returns(mockConfig);
  fakeFsStub.withArgs('./src/configuration/config.test.json').returns(FakePromise.resolve(`{"key"}`));

  try {
    // Act
    await Configuration.getConfiguration('test');
    chai.assert.fail('test case failed: [should throw exception if config.env.json is malformed]');
  } catch (e) {
    // Assert
    chai.assert.equal('SyntaxError: Unexpected token } in JSON at position 6', e + '');
  }
});

You should be using the Chai assert.throws operator instead (http://www.chaijs.com/api/assert/#method_throwsO so your code might look like:

it('should throw exception if config.env.json is malformed', async () => {
  // Arrange: beforeEach
  const fakeFsStub = sinon.stub(File, 'readFileAsyc');
  fakeFsStub.withArgs('./src/configuration/config.json').returns(mockConfig);
  fakeFsStub.withArgs('./src/configuration/config.test.json').returns(FakePromise.resolve(`{"key"}`));

  chai.assert.throws( () => {
      // Act
      await Configuration.getConfiguration('test');
    },
    SyntaxError,
    'test case failed: [should throw exception if config.env.json is malformed]'
  );

});

Also it is not generally a good idea to test the string returned by an exception. There is no guarantee that it will be the same between different browsers or different browser versions. If you need to check the position you should use the lineNumber and columnNumber properties of SyntaxError (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SyntaxError)

Lastly concating with '' to perform a string conversion is not the best option either use e.messaage attribute of an exception or the toString() method available on pretty much all objects.


Simple stopwatch program with tkinter I've self-taught myself Python and have recently started learning tkinter. I made a simple stopwatch program and I wanted to know if the code is clean and written in a way that the code works efficiently. I would highly appreciate any suggestions on how to improve my code! Thank you!

from tkinter import *
import time

root = Tk()

numberOfSeconds = 0

def counting():
    global numberOfSeconds
    global stopCounting

    if stopCounting == False:
        numberOfSeconds += 1
        seconds.config(text=str(numberOfSeconds))
        seconds.after(1000, counting)
    elif stopCounting == True:
        stopCounting = False
        numberOfSeconds = 0
        seconds.config(text=str(numberOfSeconds))

def start():
    global stopCounting
    stopCounting = False
    stopButton.config(state=NORMAL)
    seconds.after(1000, counting)

def stop():
    global stopCounting
    stopButton.config(state=DISABLED)
    stopCounting = True

seconds = Label(text=str(numberOfSeconds))
startButton = Button(text="Start", command=start)
stopButton = Button(text="Stop", command=stop, state=DISABLED)

seconds.grid(row=0, column=0, columnspan=2)
startButton.grid(row=1, column=0)
stopButton.grid(row=1, column=1)

root.mainloop()

Globals

Generally speaking it's not a good idea to use globals like this. It harms re-entrance. What if you want to support two stopwatches at once, either in one UI or as a web server? Having globals like this will prevent that.

It also harms testability. It is more difficult to test methods that rely on global state than it is to test methods that are self-contained, having state passed to them either in an object context (self) or as method parameters.

One way to get around this is make a class with attributes number_of_seconds and is_counting (which I find would be more intuitive than stop_counting).

Booleans

This block:

if stopCounting == False:
    numberOfSeconds += 1
    seconds.config(text=str(numberOfSeconds))
    seconds.after(1000, counting)
elif stopCounting == True:
    stopCounting = False
    numberOfSeconds = 0
    seconds.config(text=str(numberOfSeconds))

is more easily expressed as

if stopCounting:
    stopCounting = False
    numberOfSeconds = 0
    seconds.config(text=str(numberOfSeconds))
else:
    numberOfSeconds += 1
    seconds.config(text=str(numberOfSeconds))
    seconds.after(1000, counting)

Variable names

They should be lower_snake_case, i.e. start_button.


Longest increasing subsequence algorithm I'm just getting into Rust, and I've decided to implement the longest increasing subsequence algorithm suggested in the related Wikipedia page. Though this program compiles and returns the expected sequence for what's passed to lis, I am wondering if more experienced Rust programmers would have written this same algorithm in a different way?

 fn lis(x: Vec<i32>)-> Vec<i32> {
    let n = x.len();
    let mut m = vec![0; n];
    let mut p = vec![0; n];
    let mut l = 0;

    for i in 0..n {
        let mut lo = 1;
        let mut hi = l;

        while lo <= hi {
            let mut mid = (lo + hi) / 2;

            if x[m[mid]] <= x[i] {
                lo = mid + 1;
            } else {
                hi = mid - 1;
            }
        }

        let mut new_l = lo;
        p[i] = m[new_l - 1];
        m[new_l] = i;

        if new_l > l {
            l = new_l;
        }
    }

    let mut o = vec![0; l];
    let mut k = m[l];
    for i in (0..l).rev() {
        o[i] = x[k];
        k    = p[k];
    }

    o
}

fn main() {
    let v = vec![0, 8, 4, 12, 2, 10, 6, 14, 1, 9, 5, 13, 3, 11, 7, 15];
    let o = lis(v);
    println!("{:?}", o);
}

The parameter x doesn't have to be a Vec, it could be a slice instead (&[i32]). However, if you had used a non-Copy item type (i32 is Copy), then in order to return individual items from the input Vec, you'd either have to clone the items or move them out of the Vec (using remove).
You've implemented binary search by hand, but the Rust standard library provides a very flexible implementation via slice::binary_search_by.

I present you my own implementation of the algorithm, based on a slightly different C++ implementation, which is why there are so many differences compared to your version. I wrote this about one year ago, but sadly, I no longer use it. Note a few things though that are not necessarily relevant to the code review:

This is a very generic function: the selector parameter is used to extract a particular field from each item in the slice. I was using this function on a slice of objects and I was interested in finding increasing values of a particular field of these objects.
The function returns indices into the slice, not the values themselves. I needed to know the indices of the items in the longest increasing subsequence because I had to do something else with the items that were not in the subsequence (without destroying the original sequence), and using indices was the easiest way to do that. If I wanted to return the values without consuming the original sequence, I could have returned a Vec of references to the items instead (Vec<&T>).
Single letter variable names don't really help to understand the algorithm. Since I was using this algorithm for practical purposes, I took the time to understand and document it. If this is just an exercise to learn idiomatic Rust, I suppose this kind of detail doesn't matter much.
I like your use of a for loop on a reversed range iterator at the end. I should have done that myself!

/// Finds one of the [longest increasing subsequences][1]
/// from the subsequence created by applying `selector` on each item in `items`.
/// The result is a vector of indices within `items`
/// corresponding to one of the longest increasing subsequences.
///
/// [1]: https://en.wikipedia.org/wiki/Longest_increasing_subsequence
pub fn lis<T, I: Ord, F: Fn(&T) -> I>(items: &[T], selector: F) -> Vec<usize> {
    // This algorithm is adapted from
    // http://www.algorithmist.com/index.php?title=Longest_Increasing_Subsequence.cpp&oldid=13595

    let mut result = Vec::new();

    // If `items` is empty, then the result is also empty.
    if items.is_empty() {
        return result;
    }

    // This vector stores, for each item,
    // the index of the largest item prior to itself that is smaller than itself.
    // We'll use this vector at the end to build the final result.
    let mut previous_chain = vec![0; items.len()];

    // Initially, we assume that the first item is part of the result.
    // We will replace this index later if that's not the case.
    result.push(0);

    for i in 1..items.len() {
        // If the next item is greater than the last item of the current longest subsequence,
        // push its index at the end of the result and continue.
        if selector(&items[*result.last().unwrap()]) < selector(&items[i]) {
            previous_chain[i] = *result.last().unwrap();
            result.push(i);
            continue;
        }

        // Perform a binary search to find the index of an item in `result` to overwrite.
        // We want to overwrite an index that refers to the smallest item that is larger than `items[i]`.
        // If there is no such item, then we do nothing.
        let comparator = |&result_index| {
            use std::cmp::Ordering;

            // We don't return Ordering::Equal when we find an equal value,
            // because we want to find the index of the first equal value.
            if selector(&items[result_index]) < selector(&items[i]) {
                Ordering::Less
            } else {
                Ordering::Greater
            }
        };

        let next_element_index = match result.binary_search_by(comparator) {
            Ok(index) | Err(index) => index,
        };

        if selector(&items[i]) < selector(&items[result[next_element_index]]) {
            if next_element_index > 0 {
                previous_chain[i] = result[next_element_index - 1];
            }

            result[next_element_index] = i;
        }
    }

    // The last item in `result` is correct,
    // but we might have started overwriting earlier items
    // with what could have been a longer subsequence.
    // Walk back `previous_chain` to restore the proper subsequence.
    let mut u = result.len();
    let mut v = *result.last().unwrap();
    while u != 0 {
        u -= 1;
        result[u] = v;
        v = previous_chain[v];
    }

    result
}


Empty method in abstract class I have an abstract class which holds a hook method for a template method.  Not all classes which extend this class will need to implement this method (in fact most will not, but a few will).  As such I declared the method this way:

protected void applyExtraCriteria(Object extraCriteria) {

}

Should it instead be declared this way:

protected abstract void applyExtraCriteria(Object extraCriteria);

Which one would be considered better practice?

I would declare as required by all classes. It is easy enough to leave an empty method stub in the sub class.


Knight's Tour - Python Below is my recursive solution for the Knights Tour. The user can set the board size and the program will print the first legal knight's tour it finds.

As there are potentially millions of correct solutions to the knight's tour, only one solution is given.

def legal_moves(visited, position, sq):
    """Calculate legal moves"""
    legal_moves = [
        (-2, -1),
        (-2, 1),
        (-1, -2),
        (-1, 2),
        (1, -2),
        (1, 2),
        (2, -1),
        (2, 1),
    ]

    row = position // sq
    col = position % sq

    move_to = []
    for dx, dy in legal_moves:
        if 0 <= (row + dx) < sq and 0 <= (col + dy) < sq:
            new_position = (position + (sq * dx)) + dy
            if new_position not in visited and 0 <= new_position < (sq * sq):
                move_to.append(new_position)
    return move_to

def make_move(move_to, visited, current, route, sq):
    """Carry out the move"""
    for move in move_to:
        visited.add(current)
        new_route = route + f"{current}-"
        solution_found = next_move(visited, move, new_route, sq)
        visited.remove(current)
        if solution_found:
            return True
    return False

def next_move(visited, current, route, sq):
    """Find the next valid moves and instruct "make_move" to carry them out"""
    if len(visited) == (sq * sq) - 1:
        route += f"{current}"
        print(route)
        return True
    move_to = legal_moves(visited, current, sq)
    solution_found = make_move(move_to, visited, current, route, sq)
    if solution_found:
        return True

def start_tour(sq):
    """Calculate the knights tour for  grid sq*sq starting at all positions in range 0-sq"""
    for starting in range(sq * sq):
        visited = set()
        route = ""
        solution_found = next_move(visited, starting, route, sq)
        if solution_found:
            return
    print("No knights tour could be calculated")

if __name__ == "__main__":
    square_size = 8
    start_tour(square_size)

EDIT
I have added a print_route function which is called from inside next_move in place of the current print statement as follows:
print_route(route, sq)

def print_route(route, size):
    """Convert the 1D array into a 2D array tracking the movement on the knight"""
    import numpy as np

    steps = route.split("-")
    array = np.zeros(size * size)

    for index, item in enumerate(steps):
        array[int(item)] = index + 1
    array = array.reshape(size, size)

    print(array)

Starting position

If there is a route to be found, it will go through cell 0 and will be found at first iteration of start_tour. We can remove the loop and just have starting = 0.

Generating legal moves

Various details can be improved in the legal_moves function.

This is a good occasion to use a generator with the keyword yield.

We could compute row and col with a single call to divmod.

We could make computation of new position more straight-forward with intermediates variables for coordinates on each axis.

Because of the way new_position is computed, there is no need for the additional boundary check.

We'd get something like:

def generate_new_positions(visited, position, sq):
    """Yield legal moves"""
    generate_new_positions = [
        (-2, -1),
        (-2, 1),
        (-1, -2),
        (-1, 2),
        (1, -2),
        (1, 2),
        (2, -1),
        (2, 1),
    ]
    # position = row * sq + col
    row, col = divmod(position, sq)

    for dx, dy in generate_new_positions:
        x, y = row + dx, col + dy
        if 0 <= x < sq and 0 <= y < sq:
            new_pos = x * sq + y
            if new_pos not in visited:
                yield new_pos

Separation of concerns: printing vs returning a result

When a result is found, it is printed and a boolean (or None) is returned in the different functions. It would be easier to return either None or the result found and to have that result printed from a single point of the logic.

We'd have something like:

def make_move(move_to, visited, current, route, sq):
    """Carry out the move"""
    for move in move_to:
        visited.add(current)
        new_route = route + str(current) + "-"
        solution_found = next_move(visited, move, new_route, sq)
        visited.remove(current)
        if solution_found:
            return solution_found
    return None

def next_move(visited, current, route, sq):
    """Find the next valid moves and instruct "make_move" to carry them out"""
    if len(visited) == (sq * sq) - 1:
        route += str(current)
        return route
    move_to = generate_new_positions(visited, current, sq)
    return make_move(move_to, visited, current, route, sq)

def start_tour(sq):
    """Calculate the knights tour for grid sq*sq starting at all positions in range 0-sq"""
    starting = 0
    visited = set()
    route = ""
    return next_move(visited, starting, route, sq)

if __name__ == "__main__":
    for square_size in 3, 5, 6:
        ret = start_tour(square_size)
        print("No knights tour could be calculated" if ret is None else ret)

Also, formatting the string could be done in a single place as well. We could use lists for instance in all the logic.

def make_move(move_to, visited, current, route, sq):
    """Carry out the move"""
    for move in move_to:
        visited.add(current)
        new_route = route + [current]
        solution = next_move(visited, move, new_route, sq)
        visited.remove(current)
        if solution:
            return solution
    return None

def next_move(visited, current, route, sq):
    """Find the next valid moves and instruct "make_move" to carry them out"""
    if len(visited) == (sq * sq) - 1:
        return route + [current]
    move_to = generate_new_positions(visited, current, sq)
    return make_move(move_to, visited, current, route, sq)

def start_tour(sq):
    """Calculate the knights tour for grid sq*sq starting at all positions in range 0-sq"""
    starting = 0
    visited = set()
    route = []
    return next_move(visited, starting, route, sq)

if __name__ == "__main__":
    for square_size in 3, 5, 6:
        ret = start_tour(square_size)
        print("No knights tour could be calculated" if ret is None else "-".join((str(e) for e in ret)))

Reducing the duplicated information

We're maintaining a visited set and a route list: both containing roughtly the same data. Maybe we could recompute visited from the route when we need it.

def make_move(move_to, current, route, sq):
    """Carry out the move"""
    for move in move_to:
        solution = next_move(move, route + [current], sq)
        if solution:
            return solution
    return None

def next_move(current, route, sq):
    """Find the next valid moves and instruct "make_move" to carry them out"""
    if len(route) == (sq * sq) - 1:
        return route + [current]
    move_to = generate_new_positions(set(route), current, sq)
    return make_move(move_to, current, route, sq)

def start_tour(sq):
    """Calculate the knights tour for grid sq*sq starting at all positions in range 0-sq"""
    return next_move(0, [], sq)

Simplifying the logic

Having a function A calling a function B itself calling B can make things hard to understand properly because both A and B are hard to understand independently.

Here, we could get rid of make_move by integrating directly in next_move:

def next_move(current, route, sq):
    """Find the next valid moves and carry them out"""
    if len(route) == (sq * sq) - 1:
        return route + [current]
    for move in generate_new_positions(set(route), current, sq):
        solution = next_move(move, route + [current], sq)
        if solution:
            return solution
    return None

More simplification

In next_move, route is almost always used in the expression route + [current]. We could directly define this at the beginning of the function:

def next_move(current, route, sq):
    """Find the next valid moves and carry them out"""
    new_route = route + [current]
    if len(new_route) == (sq * sq):
        return new_route
    for move in generate_new_positions(set(new_route), current, sq):
        solution = next_move(move, new_route, sq)
        if solution:
            return solution
    return None

More importantly, it leads to the question: why do we provide a route AND an element to add to it instead of just having that element in the list.

def next_move(current, route, sq):
    """Find the next valid moves and carry them out"""
    if len(route) == (sq * sq):
        return route
    for move in generate_new_positions(set(route), current, sq):
        solution = next_move(move, route + [move], sq)
        if solution:
            return solution
    return None

def start_tour(sq):
    """Calculate the knights tour for grid sq*sq starting at all positions in range 0-sq"""
    start = 0
    return next_move(start, [start], sq)

Going further, we do not really the current argument anymore as we can compute it from route.

def next_move(route, sq):
    """Find the next valid moves and carry them out"""
    if len(route) == (sq * sq):
        return route
    current = route[-1]
    new_pos = generate_new_positions(set(route), current, sq)
    for move in new_pos:
        solution = next_move(route + [move], sq)
        if solution:
            return solution
    return None

def start_tour(sq):
    """Calculate the knights tour for grid sq*sq starting at all positions in range 0-sq"""
    start = 0
    return next_move([start], sq)

Final code

def generate_new_positions(visited, position, sq):
    """Yield legal moves"""
    generate_new_positions = [
        (-2, -1),
        (-2, 1),
        (-1, -2),
        (-1, 2),
        (1, -2),
        (1, 2),
        (2, -1),
        (2, 1),
    ]
    # position = row * sq + col
    row, col = divmod(position, sq)

    for dx, dy in generate_new_positions:
        x, y = row + dx, col + dy
        if 0 <= x < sq and 0 <= y < sq:
            new_pos = x * sq + y
            if new_pos not in visited:
                yield new_pos

def next_move(route, sq):
    """Find the next valid moves and carry them out"""
    if len(route) == (sq * sq):
        return route
    current = route[-1]
    new_pos = generate_new_positions(set(route), current, sq)
    for move in new_pos:
        solution = next_move(route + [move], sq)
        if solution:
            return solution
    return None

if __name__ == "__main__":
    for square_size in 3, 5, 6:
        ret = next_move([0], square_size)
        print("No knights tour could be calculated" if ret is None else "-".join((str(e) for e in ret)))


Delay and queue animation in a function - how can I optimise this by removing duplicate code? I have this script that moves a box around the screen and the loops the second part of the movement. I have altered the code to drop another three boxes (4 in total) into the animation so the boxes follow each other around the screen.

I want it to work exactly like it does, but I'm sure there's a much better way of doing this:

Here's a js fiddle: http://jsfiddle.net/NF6LU/

JS

function animateNode() {
    $('.node').animate({top: '425px'}, { duration: 1800, easing : 'linear', queue: true });
    $('.node2').animate({top: '425px'}, { duration: 1800, easing : 'linear', queue: true });
    $('.node3').animate({top: '425px'}, { duration: 1800, easing : 'linear', queue: true });
    $('.node4').animate({top: '425px'}, { duration: 1800, easing : 'linear', queue: true });
    $('.node').animate({marginLeft: '-284px'}, { duration: 2500, easing : 'linear', queue: true });
    $('.node2').animate({marginLeft: '-284px'}, { duration: 2500, easing : 'linear', queue: true });
    $('.node3').animate({marginLeft: '-284px'}, { duration: 2500, easing : 'linear', queue: true });
    $('.node4').animate({marginLeft: '-284px'}, { duration: 2500, easing : 'linear', queue: true });
    $('.node').animate({top: '157px'}, { duration: 1800, easing : 'linear', queue: true });
    $('.node2').animate({top: '157px'}, { duration: 1800, easing : 'linear', queue: true });
    $('.node3').animate({top: '157px'}, { duration: 1800, easing : 'linear', queue: true });
    $('.node4').animate({top: '157px'}, { duration: 1800, easing : 'linear', queue: true });
    $('.node').animate({marginLeft: '264px'}, { duration: 2500, easing : 'linear', queue: true });
    $('.node2').animate({marginLeft: '264px'}, { duration: 2500, easing : 'linear', queue: true });
    $('.node3').animate({marginLeft: '264px'}, { duration: 2500, easing : 'linear', queue: true });
    $('.node4').animate({marginLeft: '264px'}, { duration: 2500, easing : 'linear', queue: true });
}
$('.node').delay(1500).animate({top: '157px'}, { duration: 1000, easing : 'linear', queue: true });
$('.node2').delay(3000).animate({top: '157px'}, { duration: 1000, easing : 'linear', queue: true });
$('.node3').delay(4500).animate({top: '157px'}, { duration: 1000, easing : 'linear', queue: true });
$('.node4').delay(6000).animate({top: '157px'}, { duration: 1000, easing : 'linear', queue: true });
$('.node').animate({marginLeft: '264px'}, { duration: 1500, easing : 'linear', queue: true });
$('.node2').animate({marginLeft: '264px'}, { duration: 1500, easing : 'linear', queue: true });
$('.node3').animate({marginLeft: '264px'}, { duration: 1500, easing : 'linear', queue: true });
$('.node4').animate({marginLeft: '264px'}, { duration: 1500, easing : 'linear', queue: true });
$(function(){
    animateNode();
    setInterval(animateNode, 2000);
});

HTML

CSS

span.node, span.node2, span.node3, span.node4{
    height: 16px;
    width: 16px;
    background-color: black;
    position: absolute;
    top: 60px;
    left: 50%;
    margin-left: -9px;
}

You are repeating yourself a ton here, you should read up on DRY.

You could store the animations in an array and then execute those animations :

var PROPERTIES = 0 , OPTIONS = 1;
var animations =
[
  [ {top: '425px'}         , { duration: 1800, easing : 'linear', queue: true } ],
  [ {marginLeft: '-284px'} , { duration: 2500, easing : 'linear', queue: true } ],
  [ { top: '157px'}        , { duration: 1800, easing : 'linear', queue: true } ],
  [ {marginLeft: '264px'}  , { duration: 2500, easing : 'linear', queue: true } ]
]

function animateNode( $node )
{
  animations.forEach( function (animation)
  {
    $node.animate( animation[PROPERTIES] , animation[OPTIONS] );
  });
}

function animateNodes()
{
  animateNodes( $('.node') );
  animateNodes( $('.node2') );
  animateNodes( $('.node3') );
  animateNodes( $('.node4') );
}

Ideally of course, you would store the results of $(...) in a variable and use that variable instead of invoking $(...) every single time. Even more ideally, those variables would be in an array over which you can loop.

You can apply this technique to the rest of your code as well.


Function calls as parameters for other functions I am in two minds about function calls as parameters for other functions. The doctrine that readability is king makes me want to write like this:

br = mechanize.Browser()
raw_html = br.open(__URL)
soup = BeautifulSoup(raw_html)

But in the back of my mind I feel childish doing so, which makes me want to write this:

br = mechanize.Browser()
soup = BeautifulSoup(br.open(__URL))

Would it actually look unprofessional to do it the first way? Is there any serious reason to choose one method over the other?

I'm typically an advocate of liberal use of space, but in this situation, I'd go with the third option:

soup = BeautifulSoup(mechanize.Browser().open(__URL))

(I'm not actually sure if that's valid syntax or not. I'm not very familiar with Python [I think it's Python?].)

I find that just as readable. There are of course situations where you must separate it though. The first thing that comes to mind is error handling. I suspect that open throws exceptions, but if it were to return boolean false on failure rather than a string, then I would be a fan of option two. Option two would still be brief, but would allow for properly checking for the false return.

I think this really comes down to personal taste. There's no magical rule book for this type of thing (though I'm sure there are convoluted, borderline-dogmatic metrics somewhere that support one way or the other).

I tend to just eyeball things like this and see if they make me visually uncomfortable or not.

For example:

superLongVariableNameHere(someParamWithLongName, someFunctionLong(param1, param2), someFunc3(param, func4(param1, param2, func5(param)))

Makes me cry a little whereas:

func(g(x), y(z))

Seems perfectly fine. I just have some weird mental barrier of what length/nesting level becomes excessive.

While I'm at it, I'd also disagree with Juann Strauss' dislike of JavaScript's practically idiomatic use of anonymous and inline-defined functions. Yes, people go overboard sometimes with a 30 line callback, but for the most part, there's nothing wrong with a 1-10 line anonymous function as long as it's used in one and only one place and it doesn't overly clutter its context.

In fact, to have a closure, you often have to define functions in the middle of scopes (well, by definition you do). Often in that situation you might as well inline it within another function call rather than defining it separately (once again, provided the body is brief).

If he means this though, then yeah, I'd agree 95% of the time:

f((function(x) { return x * x; }(5))

That gets ugly fast.


VBA - If value in sheet1 found in sheet2, then delete data from sheet2 I have 2 sheets setup: Exclusions and Issues

Issues has a list of CASE ID's and Columns that list the "Issue"

Exclusions will be populated with CASE ID's that are to be excluded (and removed) from the Issues sheet.

My question is 2 fold:

Is my current code handling this correctly? Are there any ways to improve this?
Is there a way to have the code cycle through all columns dynamically? Or is it just easier to copy the FOR/NEXT loop for each column on the "Issues" sheet?

Code below:

Sub Exclusions()

'find exclusions and remove from issues sheet. once done delete any completely blank row

Dim i As Long
Dim k As Long
Dim lastrow As Long
Dim lastrowex As Long
Dim DeleteRow As Long
Dim rng As Range

On Error Resume Next
    Sheets("Issues").ShowAllData
    Sheets("Exclusions").ShowAllData
On Error GoTo 0

Application.ScreenUpdating = False

lastrowex = Sheets("Exclusions").Cells(Rows.Count, "J").End(xlUp).Row

    With ThisWorkbook

        lastrow = Sheets("Issues").Cells(Rows.Count, "A").End(xlUp).Row

    For k = 2 To lastrowex
        For i = 2 To lastrow
            If Sheets("Exclusions").Cells(k, 10).Value <> "" Then
                If Sheets("Exclusions").Cells(k, 10).Value = Sheets("Issues").Cells(i, 1).Value Then
                    Sheets("Issues").Cells(i, 11).ClearContents
                End If
            End If
        Next i
    Next k

    End With

On Error Resume Next
Sheets("Issues").Activate
For Each rng In Range("B2:P" & lastrow).Columns
    rng.SpecialCells(xlCellTypeBlanks).EntireRow.Delete
Next rng

Application.ScreenUpdating = True

End Sub

Data Format:

"Issues" sheet

CASE ID      Issue 1     Issue 2    Issue 3
ABC123       No address  No Name    No Number

"Exclusions" sheet

Issue 1    Issue 2    Issue 3
ABC123     DEF123     ABC123

My example you'll find below is based on most often working with large datasets and opts for speed in data handling. You didn't state the size of your Issues and Exclusions, so I worked with a large dataset in mind.

A couple quick things to get out of the way because these are good practices to make into consistent habits:

Always use Option Explicit
Avoid a "wall of declarations", plus the very useful other tips on that site
Establish specific object variables for the worksheets, instead of always using Sheets. Further, by only using Sheets you're implying that the code should operate on the currently ActiveWorksheet. This is quite often correct, but will trip you up at some point when you intend something different.

So I make a habit of defining exactly which workbook and worksheet I'm using by initializing variables with fully qualified references.

Dim exclusionsWS As Worksheet
Dim issuesWS As Worksheet
Set exclusionsWS = ThisWorkbook.Sheets("Exclusions")
Set issuesWS = ThisWorkbook.Sheets("Issues")

While I understand your rationale for handling the possible ShowAllData errors, I would much rather be clear about "why" you need to do this. So I'd avoid the On Error Resume Next by making it clear I'm checking for a possible AutoFilter:

With exclusionsWS
    If (.AutoFilterMode And .FilterMode) Or .FilterMode Then
        .AutoFilter.ShowAllData
    End If
End With
With issuesWS
    If (.AutoFilterMode And .FilterMode) Or .FilterMode Then
        .AutoFilter.ShowAllData
    End If
End With

Next, because there may be a large dataset, I would copy the data on the worksheet into a memory-based array. Working out of memory is MUCH faster than working with the Range object in Excel. Later, the process of checking to see if a value exists in another dataset is perfect for a Dictionary. So we'll loop through all the exclusions and create a dictionary item for each entry.

Dim exclusionData As Variant
exclusionData = exclusionsWS.UsedRange

Dim exclusion As Dictionary
Set exclusion = New Dictionary
Dim i As Long
For i = 2 To lastRow
    If Not exclusionData(i, 10) = vbNullString Then
        exclusion.Add exclusionData(i, 10), i
    End If
Next i

After that, my example shows checking each Issue against the Dictionary and clearing out any excluded Issues. In order to copy the remaining issues back to the worksheet, we have to clear ALL the issues first, then copy the array data to the worksheet.

Here's the whole routine in a single view:

Option Explicit

Public Sub RemoveExclusions()
    Dim exclusionsWS As Worksheet
    Dim issuesWS As Worksheet
    Set exclusionsWS = ThisWorkbook.Sheets("Exclusions")
    Set issuesWS = ThisWorkbook.Sheets("Issues")
    With exclusionsWS
        If (.AutoFilterMode And .FilterMode) Or .FilterMode Then
            .AutoFilter.ShowAllData
        End If
    End With
    With issuesWS
        If (.AutoFilterMode And .FilterMode) Or .FilterMode Then
            .AutoFilter.ShowAllData
        End If
    End With

    Dim lastRow As Long
    With exclusionsWS
        lastRow = .Cells(.Rows.Count, "J").End(xlUp).Row
    End With

    '--- move the exclusion data to a memory-based array
    '    for processing into a dictionary
    Dim exclusionData As Variant
    exclusionData = exclusionsWS.UsedRange

    Dim exclusion As Dictionary
    Set exclusion = New Dictionary
    Dim i As Long
    For i = 2 To lastRow
        If Not exclusionData(i, 10) = vbNullString Then
            exclusion.Add exclusionData(i, 10), i
        End If
    Next i

    '--- move all the issues into a memory-based array also
    '    and clear the data from exclusion matches
    Dim issuesData As Variant
    Dim excludedCount As Long
    issuesData = issuesWS.UsedRange
    For i = 2 To UBound(issuesData, 1)
        If exclusion.Exists(issuesData(i, 10)) Then
            issuesData(i, 10) = vbNullString
            excludedCount = excludedCount + 1
        End If
    Next i

    '--- now collapse all the empty rows by copying the remaining
    '    issues into a new array, then copy the array back to the
    '    worksheet
    Dim remainingIssues As Variant
    ReDim remainingIssues(1 To UBound(issuesData, 1) - excludedCount, _
                          1 To UBound(issuesData, 2))
    Dim newIssue As Long
    newIssue = 1
    Dim j As Long
    For i = 1 To UBound(issuesData, 1)
        If Not issuesData(i, 10) = vbNullString Then
            For j = 1 To UBound(issuesData, 2)
                remainingIssues(newIssue, j) = issuesData(i, j)
            Next j
            newIssue = newIssue + 1
        End If
    Next i
    issuesWS.UsedRange.ClearContents
    issuesWS.Range("A1").Resize(UBound(remainingIssues, 1), _
                                UBound(remainingIssues, 2)) = remainingIssues
End Sub