Handling exceptions with FileInputStream Could you please tear my code apart? It's an exercise from CodeGym. Recently I changed a few things within my coding practices, namely:

Not writing all my solutions on main. Instead, trying to create methods to make the code more organised.
Instantiating the class on main to call the methods.
Using try with resources
Pay close attention to indentation

I have on specific question. What types of comments would you write on that code, if any.

Thanks!

/*
ABCs
The first parameter of the main method is a file name.
Count the letters of the English alphabet in the file.
Display the number of letters.
Close the streams.

Requirements:
1. You don't need to read anything from the console.
2. Create a stream to read from the file passed as the first argument of the main method.
3. You need to count the letters of the English alphabet in the file and display the number.
4. You must count both uppercase and lowercase letters.
5. The stream used to read the file must be closed.

*/

import java.io.FileInputStream;
import java.io.IOException;

public class Solution {
    private void countLetters (String args) {
        char[] array = new char[] {'a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r','s','t','u','v','w','x','y','z',
                'A','B','C','D','E','F','G','H','I','J','K','L','M','N','O','P','Q','R','S','T','U','V','W','X','Y','Z'};

        int count = 0;
        try (FileInputStream in = new FileInputStream(String.valueOf(args))) {
            int ch;
            while ((ch = in.read()) != -1) {
                for (int i = 0; i < array.length; i++) {
                    if (ch == array[i]) {
                        count++;
                    }
                }
            }
        } catch (IOException e) {
            System.out.println("General I/O exception: " + e.getMessage());
        }

        printCount(count);
    }

    private void printCount (int count) {
        System.out.println(count);
    }

    public static void main(String[] args) {
        Solution solution = new Solution();
        solution.countLetters(args[0]);
    }
}

Naming

Class name Solution does not describe the purpose of the class beyond it being a solution to some random assignment. For the sole purpose of archiving exercises I would name the package with the identifier of the assignment and use a more descriptive class name: your.domain.codegym.exercise42.LetterCounter and put the URL to the assignment in the JavaDoc (maybe package.html). BTW: If you haven't done already, I recommend setting up a GitLab repo for your exercises. One of my regrets is that I've lost so much code I wrote when I was young because broken hard disks and obsoleted hardware. We didn't have on-line repos then...

The parameter name args in the countLetters method does not tell what it is supposed to contain and it is misleading as the plural suffix 's' implies that the parameter would contain multiple values. The parameter should be named as fileName.

Unnecessary object initialization

The method countLetters does not rely on any member variable from the Solution class and the counter does not hold any state between invocations to countLetters. There is no need for the method to require a the Solution class to be instantiated. It could very well be a public static void countLetters(...). If the assignment required that an object is to be initialized, then the requirement is a bit artificial.

Inefficient algorithm

The array that contains the english alphabet letters never changes. It should be a static field in the Solution class. As it is now, your code allocates a new array for the alphabet every time your method is called. But an array like this is also a very inefficient way to look for a letter. As the US-ASCII charset fits in 7 bits and the upper and lower case letters form two consecutive blocks in the character table. You can simply compare the character to first and last letter:

if ((ch >= 'A' && ch <= 'Z') || (ch >= 'a' && ch <= 'z')) {
    count++;
}

Note that this approach does not work if the text in the file is in a character set that does not inherit US-ASCII. That would be very very rare these days.

Multiple responsibilities

The countLetters method is a utility method. It should have exactly one purpose so that it does not unnecessarily limit it's usability (also maintaining the single responsibility principle helps in maintainability and testability). You should not implement exception handling beyond closing the resources you have acquired in the method. So instead of printing out the error message, you should just let the exception escape and let caller decide how to handle the error.

So the countLetters has three responsibilities: it counts the letters in a file, handles error logging and also implements output handling. Instead of printing the letter count the method should just return the number of letters and let the caller handle the output. Use long instead int. Files can be large.

private long countLetters (String args) throws IOException {
    ...
    return count;
}

Error handling

For future reference, it is almost never correct to write errors to System.out. The correct stream for errors is System.err. And logging n exception simply with the exeption message causes trouble when you're trying to debug problems, because all the important information in the exception is in the stack trace. If you have to log an exception to the console, simply write ex.printStackTrace();. In the real world you would use a loging framework but still you would log the full stack trace, never just the message.

Add a check to see if the args array in the main method contains exactly one element. If you do not provide parameters to your program now you get an ArrayIndexOutOfBoundsExceltion.

Monolithic code

You've tried to not implement all your logic in the main method, but the attempt falls a bit short because the methods in the Solution class are all private. The only way to access the counter is is still via the main method only. Instead of implementing the main method to the Solution-class you should have two classes: one that implements the letter counting and one that has the main method, which invokes the letter counter class. I.e. separate the letter counting responsibility from the command line processing responsibility. This again follows the single responsiblity principle: the Solution-class is only responsible for counting. All other responsibilities are put into separate modules.


Rock Paper Scissors game in Python I'm new to programming and I'm trying to teach myself Python through some online books.

One of the exercises has me making a Rock Paper Scissors game. The one I have written works well enough I think, but I was wondering if there were more efficient ways of writing the code, perhaps something shorter.

I'm using Python 3.4.3 IDLE GUI.

    import random

#variables
done = False
comp = random.randrange(1,4)

#start
print("Lets play rock, paper, scissors! Choose numbers 1-3 only please.")
print()

#begin loop

while not(done):
#computer vs input
    guess = int(input("1. Rock 2. Paper 3. Scissors "))
    if comp == 1 and guess == 3:
        print("Rock beats scissors, too bad!")
    elif comp == 2 and guess == 1:
        print("Paper beats rock, tough luck!")
    elif comp == 3 and guess == 2:
        print("Scissors me timbers! Scissors cuts paper! You lose.")
    elif guess == 1 and comp == 3:
        print("Rock beats scissors, you beat me!")
    elif guess == 2 and comp == 1:
        print("You win! Paper beats rock.")
    elif guess == 3 and comp == 2:
        print("Scissor cuts paper, you win!")
    else:
        print("It's a draw!")

#continue
    stop = input("Do you want to quit? ")
    if stop.lower() == "y" or stop.lower() == "yes":
        done = True

Looping

Rather than setting a done flag and looping while not(done): (note: this would usually be written as simply while not done:), you could use a formulation more like:

while True:
    # do things
    if loop_should_end:
        break

This removes the need for the flag and makes the loop easier to follow.

Logic

At present, comp is set outside the loop, so the computer will always guess the same thing. This makes it rather too easy to win - you should move comp = random.randint(1, 4) (note whitespace) inside the loop.

Comparisons

The most obvious tweak is that the canonical replacement for lots of elifs in Python is a dictionary. In this case, you could look up a tuple (comp, guess) and print the result:

RESULTS = {
    (1, 3): "Rock beats scissors, too bad!",
    (2, 1): "Paper beats rock, tough luck!",
    ...
}

then access this inside the loop as easily as:

print(RESULTS.get((comp, guess), "It's a draw!"))

You could also simplify:

stop = input("Do you want to quit? ")
if stop.lower() == "y" or stop.lower() == "yes":

to:

if input("Do you want to quit? ").lower() in {"y", "yes"}:

Validation

You currently have no input validation, so the program could crash with a ValueError if the user types 'scissors' instead of 3. See e.g. "Asking the user for input until they give a valid response" for guidance on this.

Structure

Finally, it is bad practice to have all of your code running at the top level of the script - it makes it much more difficult to re-use later. Instead, I would do something like:

import random

RESULTS = {...}  # possible outcomes

def get_int_input(...):
    """Take valid integer input from user."""
    ...

def game_loop(...):
    """Main game loop."""
    ...

if __name__ == '__main__':
    game_loop()

This will work exactly the same if you run the script directly (see "What does if __name__ == “__main__”: do?") but allows you to use e.g. from wherever import get_input_int elsewhere, simplifying reuse and encouraging modular design.


Simple Java calculator with two operands I am taking a lynda dot com course online to start off. The course asks you to write a simple program:

  Ask the user to input 2 values
  Ask the user to input an operation
  Use the operation on the 2 values and print the result

This was very different from the solution given by the course. Could anyone take a quick look at it and tell me if there's something I'm doing fundamentally wrong or inefficient? My suspicion is that I'm using too much memory.

package com.example.java;

import java.util.Scanner;

public class Main {

    public static void main(String[] args) {

        Scanner scanner = new Scanner(System.in);

        System.out.print("Enter a numeric value: ");
        String input1 = scanner.nextLine();
        System.out.print("Enter a numeric value: ");
        String input2 = scanner.nextLine();

        double double1 = Double.parseDouble(input1);
        double double2 = Double.parseDouble(input2);

        System.out.print("Choose and operation (+ - * /): ");
        String input3 = scanner.nextLine();

        double resultAdd = double1 + double2;
        double resultSub = double1 - double2;
        double resultMul = double1 * double2;
        double resultDiv = double1 / double2;

        switch (input3) {
            case "+":
                System.out.println("The answer is " + resultAdd);
                break;
            case "-":
                System.out.println("The answer is " + resultSub);
                break;
            case "*":
                System.out.println("The answer is " + resultMul);
                break;
            case "/":
                System.out.println("The answer is " + resultDiv);
                break;
        }
    }
}

Here is what the instructor wrote, for the record:

package com.example.java;

import java.util.Scanner;

public class Calculator2 {

    public static void main(String[] args) {
        String s1 = getInput("Enter a numeric value: ");
        String s2 = getInput("Enter a numeric value: ");
        String op = getInput("Choose an operation (+ - * /):");

        double result = 0;

        try {
            switch (op) {
                case "+":
                    result = addValues(s1, s2);
                    break;
                case "-":
                    result = subtractValues(s1, s2);
                    break;
                case "*":
                    result = multiplyValues(s1, s2);
                    break;
                case "/":
                    result = divideValues(s1, s2);
                    break;
                default:
                    System.out.println("Unrecognized operation!");
                    return;
            }

            System.out.println("The answer is " + result);

        } catch (Exception e) {
            System.out.println("Number formatting exception " + e.getMessage());

        }
    }

    private static double addValues(String s1, String s2) {
        double d1 = Double.parseDouble(s1);
        double d2 = Double.parseDouble(s2);
        return d1 + d2;
    }

    private static double subtractValues(String s1, String s2) {
        double d1 = Double.parseDouble(s1);
        double d2 = Double.parseDouble(s2);
        return d1 - d2;
    }

    private static double multiplyValues(String s1, String s2) {
        double d1 = Double.parseDouble(s1);
        double d2 = Double.parseDouble(s2);
        return d1 * d2;
    }

    private static double divideValues(String s1, String s2) {
        double d1 = Double.parseDouble(s1);
        double d2 = Double.parseDouble(s2);
        return d1 / d2;
    }

    private static String getInput(String prompt) {
        System.out.print(prompt);
        Scanner sc = new Scanner(System.in);
        return sc.nextLine();
    }

}

Your code

Simple is nice. But it would have been good to decompose your solution to multiple methods, for example at least:

a method to read a double
a method to perform the calculation: take the numbers and an operator as parameter and return the result

Other issues:

Don't pre-calculate the result of all possible calculations when you will only use one of them
The print statements are repetitive. If you decomposed the solution to multiple methods, naturally you would have factored out duplicated elements.
Naming, as @Caridorc already covered.

The instructor's code

Sort of nicely written, decomposed, but this is bad code:

Delaying the validation of input by carrying around the String values of supposed numeric values is a terrible idea. As @Caridorc said it too, the numeric input should have been read into the desired types (double in this example) and lift the burden of uncertainty from the rest of the code.
The getInput method is a good idea, but poorly executed. Creating a new Scanner for every piece of input is a waste. Just like there is one System.in, a single Scanner could be used and passed to the method. The method is a huge missed opportunity for actually validating the input, and returning the desired type, instead of a String.
Large try blocks are also better to avoid, and the large try block in the middle is just another consequence of delaying the input validation, that could have been done much better earlier.


Efficient way of BinaryGap A binary gap within a positive integer N is any maximal sequence of consecutive zeros that is surrounded by ones at both ends in the binary representation of N.

For example, number 9 has binary representation 1001 and contains a binary gap of length 2. The number 529 has binary representation 1000010001 and contains two binary gaps: one of length 4 and one of length 3. The number 20 has binary representation 10100 and contains one binary gap of length 1. The number 15 has binary representation 1111 and has no binary gaps. The number 32 has binary representation 100000 and has no binary gaps.

Write a function:

class Solution { public int solution(int N); }

that, given a positive integer N, returns the length of its longest binary gap. The function should return 0 if N doesn't contain a binary gap.

For example, given N = 1041 the function should return 5, because N has binary representation 10000010001 and so its longest binary gap is of length 5. Given N = 32 the function should return 0, because N has binary representation '100000' and thus no binary gaps.

Write an efficient algorithm for the following assumptions:

N is an integer within the range [1..2,147,483,647].

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

import javax.swing.plaf.basic.BasicInternalFrameTitlePane.MaximizeAction;

public class Main {

    public static void main(String[] args) {
        // TODO Auto-generated method stub
        Solution s = new Solution();
        int x = s.solution(1041);

        System.out.println(x);
    }

}

class Solution {

    public int solution(int N)
    {
        int countZero = 0;
        Integer maxCountZeroList = 0;

        List<Integer> countZeroList = new ArrayList<Integer>();

        String nBin = Integer.toBinaryString(N);
        for (int i=1;i<nBin.length();i++) {

            if(nBin.charAt(i) == '1')
            {
                countZeroList.add(countZero);
                countZero = 0;
            }
            else
            {
                countZero++;
            }

        }

        if(countZeroList.size() > 0)
        {
            maxCountZeroList = Collections.max(countZeroList);
        }
        return maxCountZeroList;
    }
}

I am looking for more efficient way of solution of this problem

Unused Imports

import javax.swing.plaf.basic.BasicInternalFrameTitlePane.MaximizeAction;

This is never used, so why have it?

Useless Comments

// TODO Auto-generated method stub

Clearly, you've implemented the method.  Remove the TODO comment.

Singletons

Solution s = new Solution();
int x = s.solution(1041);

Solution is a class with no data members, just one function.  There is no reason to make an instance of this class, just make the method static.   Actually, there is no obvious reason to have the Solution class; you can make solution() a static member function in Main.

Pointless Box

Integer maxCountZeroList = 0;

There is no reason to have maxCountZeroList an object.  An int would be sufficient.  Since solutions() returns an int, and ends with return maxCountZeroList;, you are unboxing this integer anyway.

One Pass

You are doing two passes over your data.  The first pass you are counting non-'1' characters, and storing the counts in a List.  The second pass, this time over the list, you are looking for the max() value.

This can be done in one pass.

int countZero = 0;
int maxCountZero = 0;

String nBin = Integer.toBinaryString(N);
for (int i=1;i<nBin.length();i++) {

    if(nBin.charAt(i) == '1')
    {
        if (countZero > maxCountZero)
            maxCountZero = countZero;

        countZero = 0;
    }
    else
    {
        countZero++;
    }

}
return maxCountZero;

Enhanced For Loop

Instead of looping over the indices of nBin, and then calling .charAt() at each index (the only place the index is used), you could instead loop over the characters of the string, eliminating the need for the .charAt() call.

for (char ch : nBin.toCharArray()) {

    if(ch == '1')
    {
        if (countZero > maxCountZero)
            maxCountZero = countZero;

        countZero = 0;
    }
    else
    {
        countZero++;
    }

}

Looking for the wrong thing

This code is not counting "zeros".  It is counting "not ones".  It would be clearer written as:

for (char ch : nBin.toCharArray()) {

    if(ch == '0')
    {
        countZero++;
    }
    else
    {
        if (countZero > maxCountZero)
            maxCountZero = countZero;

        countZero = 0;
    }
}

Efficiency Improvement

@Harshal's improved solution is a bit obfuscated, but it does run in \$O(gap)\$ time.  Here is a less obfuscated implementation which runs in \$O({numGaps})\$ time.

int max_gap = 0;

n >>>= Integer.numberOfTrailingZeros(n);   // Get rid of ending 0's.
n >>>= Integer.numberOfTrailingZeros(~n);  // Get rid of ending 1's.

while (n > 0) {
    int gap = Integer.numberOfTrailingZeros(n);
    if (gap > max_gap)
        max_gap = gap;

    n >>>= gap;
    n >>>= Integer.numberOfTrailingZeros(~n);
}

return max_gap;

Assuming Integer.numberOfTrailingZeros(x) executes in constant time, the above while loop will execute once for each "gap", computing exactly how long each gap is.  With 0x40000001, Harshal's method must loop 31 times, where as this algorithm loops only once.  In contrast, with 0x55555555, this algorithm loops 15 times, finding each gap of one '0', where as Harshal's algorithm loops once; the maximum gap size.

Profiling, and knowledge of the distribution of values, will be important in choosing between the two optimizations.


Install Ruby using rbenv in a bash script I want to write an automated bash script for installing Ruby and Rails using rbenv, but I am getting session reload issues in the terminal.

#!/bin/bash
echo "Installing rbenv.."
git clone https://github.com/rbenv/rbenv.git ~/.rbenv
git clone https://github.com/rbenv/ruby-build.git ~/.rbenv/plugins/ruby-build
echo 'export PATH="$HOME/.rbenv/bin:$PATH"' >> ~/.bashrc
echo 'eval "$(rbenv init -)"' >> ~/.bashrc
source ~/.bashrc
reset
echo "Finished installing rbenv"

#!/bin/bash
rbenv -v
echo "Installing ruby.."
sudo rbenv install 2.2.3
rbenv global 2.2.3
echo "Finished installing ruby"
ruby -v

echo "gem: --no-document" > ~/.gemrc

echo "Installing rails.."
sudo apt-get install ruby-dev
sudo gem install bundler
gem install rails -v 4.2.3
rbenv rehash
echo "Finished installing rails"
rails -v
rbenv rehash

Not a lot can be improved on a simple installer script like this.
I have a few tips but nothing really significant.

The script doesn't check if it has already been executed.
If you run it again by mistake, it will append to ~/.bashrc lines that were already there.

As a simple workaround, you could add -e flag to the shebang:

#!/bin/bash -e

This will cause the script to exit on the first error.
Which will happen on the first git clone command,
which will fail because the local clone already exists.
The script will stop there and not reach the lines that append again to .bashrc.

Speaking of shebang, you have a second #!/bin/bash in the middle of the script, which was probably a copy-paste mistake and has no purpose there, so should be removed.


Indeterminate-length big-endian to little-endian conversion in Python I have a need to convert wire encoding (big-endian) to little-endian in Python.

The conversion needs to happen as a separate function; I am not allowed to change the function that reads the byte stream because it will break a lot of other functions.

The best I can think of is this:

def be_to_le(value: int) -> int:
    assert isinstance(value, int)
    numbytes = len(f"0{value:x}") >> 1
    if numbytes == 1:
        return value
    rslt = 0
    for b in value.to_bytes(numbytes, byteorder="little"):
        rslt <<= 8
        rslt |= b
    return rslt

Is there a better way?

To calculate number of bytes, better use:

numbytes = math.ceil(value.bit_length() / 8)

And then you can just:

def be_to_le(value: int) -> int:
    numbytes = math.ceil(value.bit_length() / 8)
    return int.from_bytes(value.to_bytes(numbytes, byteorder="little"), byteorder="big")


Is there a better way to test for the exception conditions? This method assigns the value of a specified column in a DataRow to specified property in an object.

I have 3 conditionals handling exceptions, and I want these conditions to throw exceptions.

The final condition just allows the use of a conversion method if it exists. Is there a better way to test for and throw the exceptions?

public class ColumnToPropertyMap<colType, propType>
{
    public string ColumnName { get; private set; }
    public string PropertyName { get; private set; }
    private Func<colType, propType> conversion;

    public virtual void Map<T>(T obj, DataRow row)
    {
        if (!row.Table.Columns.Contains(this.ColumnName))
        {
            throw new ArgumentException(
                String.Format(@"Column ""{0}"" does not exist in the row.", this.ColumnName));
        }
        PropertyInfo property = obj.GetType().GetProperty(this.PropertyName);
        if (property == null)
        {
            throw new ArgumentException(String.Format(@"Property ""{0}"" does not exist for object.", this.PropertyName));
        }
        var value = row.IsNull(this.ColumnName) ? row[this.ColumnName].GetDefaultValue(typeof(colType)) : row[this.ColumnName];
        if (conversion == null && property.PropertyType != row.Table.Columns[this.ColumnName].DataType)
        {
            throw new InvalidCastException(
                string.Format(@"Mapping for {0} to {1} failed. Unable to convert {2} to {3}.", this.ColumnName, this.PropertyName, value.GetType(), property.PropertyType));
        }
        if (conversion != null)
        {
            value = conversion((colType)value);
        }
        property.SetValue(obj, value, null);
    }
}

Incorporating @Leonid and @SmartLemons comments/answers into consideration, I changed it to this:

    public virtual void Map<T>(T obj, DataRow row)
    {
        PropertyInfo property = obj.GetType().GetProperty(this.PropertyName);
        ValidateArguments(property, row);
        var value = row.IsNull(this.ColumnName) ? row[this.ColumnName].GetDefaultValue(typeof(colType)) : row[this.ColumnName];
        if (conversion != null)
        {
            value = conversion((colType)value);
        }
        property.SetValue(obj, value, null);
    }
    private void ValidateArguments(PropertyInfo property, DataRow row)
    {
        if (!row.Table.Columns.Contains(this.ColumnName))
        {
            throw new ArgumentException(
                String.Format(@"Column ""{0}"" does not exist in the row.", this.ColumnName));
        }
        if (property == null)
        {
            throw new ArgumentException(String.Format(@"Property ""{0}"" does not exist for object.", this.PropertyName));
        }
        if (conversion == null && property.PropertyType != row.Table.Columns[this.ColumnName].DataType)
        {
            throw new InvalidCastException(
                string.Format(@"Mapping for {0} to {1} failed. Unable to convert {2} to {3}.", this.ColumnName, this.PropertyName, row.Table.Columns[this.ColumnName].DataType.Name, property.PropertyType));
        }
    }

Takes the argument validation out of the method and makes the intent of the Map method clearer.

Your code is as simple as it can get... but if you are wanting to clean up the method then you could do something like this to make it look nicer.

public string ColumnName { get; private set; }
    public string PropertyName { get; private set; }
    private Func<colType, propType> conversion;

    public virtual void Map<T>(T obj, DataRow row)
    {
        Conditions.Check1(row, this.ColumnName).argerror(String.Format(@"Column ""{0}"" does not exist in the row.", this.ColumnName));
        PropertyInfo property = obj.GetType().GetProperty(this.PropertyName);
        Conditions.Check2(property).argerror(String.Format(@"Property ""{0}"" does not exist for object.", this.PropertyName));
        var value = row.IsNull(this.ColumnName) ? row[this.ColumnName].GetDefaultValue(typeof(colType)) : row[this.ColumnName];
        Conditions.Check3(conversion, property, row, this.ColumnName).casterror(string.Format(@"Mapping for {0} to {1} failed. Unable to convert {2} to {3}.", this.ColumnName, this.PropertyName, value.GetType(), property.PropertyType));
        if (conversion != null)
        {
            value = conversion((colType)value);
        }
        property.SetValue(obj, value, null);
    }

    public static class Conditions
    {
        public class Error
        {
            private bool error = false;
            public bool isError { get {return error;}}

            public Error(bool r)
            {
                error = r;
            }
            public bool argerror(string message)
            {
                if (error)
                    throw new ArgumentException(message);
                return error;
            }
            public bool casterror(string message)
            {
                if (error)
                    throw new InvalidCastException(message);
                return error;
            }

        }
        public static Error Check1(dynamic a, string cn)
        {
            return new Error(a.Table.Columns.Contains(cn));
        }
        public static Error Check2(dynamic a)
        {
            return new Error(a == null);
        }
        public static Error Check3(dynamic a, dynamic b, dynamic c, string cn)
        {
            return new Error(a == null && b.PropertyType != c.Table.Columns[cn].DataType);
        }
    }


Check if a tree is a proper BST I have a function that takes the root node as input and needs to return if the tree is a proper BST as per the definition below:

The data value of every node in a node's left subtree is less than
the data value of that node.
The data value of every node in a node's
right subtree is greater than the data value of that node
It cannot contain duplicate values.

Here's my implementation

  boolean checkBST(Node root) {
        if(root==null) return false;
        Queue<Node> q = new LinkedList<>();
        Set<Integer> s = new HashSet<>();
        q.add(root);
        while(q.size()>0){
            Node t = q.poll();
            if(s.contains(t.data))
                return false;
            s.add(t.data);
            if(t.left!=null)
                {
                    if(t.data<=t.left.data)
                        return false;
                q.offer(t.left);
            }
            if(t.right!=null){
                if(t.data>=t.right.data)
                    return false;
                q.offer(t.right);
            }
        }
        return true;
    }

Things to discuss:

I've pursued a Breadth first approach. Is there a better approach?
The above procedure fails for some test inputs (I don't know what the inputs are that breaks it). Trying to find out what they are
Issues with the above implementation

Since @Imus has already pointed out the technical problems with your code, and how a depth-first approach is more reasonable, I'll just note on code style and give a few bits of advice.
Advice
If you expect balanced BSTs of height > 11 (more than 6500 nodes), to avoid stack overflow errors you can still use a DFS with an explicit Stack and while loop, like you do for your BFS - it'll even be less code.
Style
Note that here all my points can be seen in example in @Imus' answer.

Spaces around operators make expressions easier to read.

Consistent bracing style and indentation. I prefer the opening brace to be on the same line as the statement, with a space between the closing parenthesis and the opening brace. The closing brace should be on its own line.

Horrible variable names - one-letter names are reserved for loop indices, and then too not always. Variable names should be meaningful and convey the purpose of the variable. Use visitedNodes instead of q, processedContents instead of s, and currentNode instead of t. Doesn't it make it that much more understandable at first glance?

!visitedNodes.isEmpty() looks a bit better than q.size()>0 in your while loop.

My recommendation is to always use the block form instead of the statement form for if/else statements or for loops (use braces around even single-line bodies). This makes code addition later easier, and also contributes to readability.

Everything in points 1 to 3 can be done automatically by a proper IDE (you'll have to choose the good names, the IDE will suggest them to you after that so you don't need to type as much).


Run loop for a text-based adventure game I am programming a graphical text adventure using python. The game is playable, but I am not sure whether the code is well-suited for newcomers (code smells / quirks / general readability).

Here's the main script:

import time
import sys

import printer
from rooms import *
from _player import Player

VER = 0.01

SCENES = {0 : Lobby(),
          1 : LivingRoom(),
          2 : Bathroom()}

player = Player()

def run(start_at=None):
    """Starts the game"""

    if not start_at:
        scene = SCENES[0]
    else:
        scene = SCENES[start_at]
    scene.init_player(player)
    while 1:
        new_scene = scene.run()
        if new_scene != None:
            scene = SCENES[new_scene]
            scene.init_player(player)

if __name__ == "__main__":
    run()

Avoid wildcard imports. Import from rooms just what you need, named explicitly.

while 1 is an unnatural way of writing while True

Don't use != or == with None. Use is not None and is None.

The scene.run call that returns another scene object is not very intuitive. Functions are best named after what they do and/or what they return. In this example "run" is a pretty vague term, it could mean anything. If it transitions the game state to the next scene, I think scene.next_scene would be more intuitive.

Do you really need SCENES ? How about just passing around scene instances? This approach using a global dictionary of index to instance mappings seems fishy.


Split a long integer into eight 4-bit values It's an exercise from a book. There is a given long integer that I should convert into 8 4-bit  values. The exercise is not very clear for me, but I think I understood the task correctly.

I googled how to return with an array in a function. In the splitter function I add the last 4 bits into the values array, then cut the them from the original number.

Is this a good way to do this?

#include <iostream>

int * splitter(long int number)
{
    static int values[8];

    for (int i = 0; i < 8; i++)
    {
        values[i] = (int)(number & 0xF);
        number = (number >> 4);
    }

    return values;
}

int main()
{
    long int number = 432214123;
    int *values;
    values = splitter(number);
    for (int i = 7; i >= 0; i--)
        std::cout << values[i] << " ";
    return 0;
}

There are only three issues I take with your code...

The caller of the splitter function has the possibility of trying to access a value outside of the returned array and could cause a memory access violation if they did so. For this reason alone, it would be better to go with the vector approach that @Jamal posted.
You should use the C++ style casting (e.g. static_cast<int>(number & 0XF)) instead of the C style casting (e.g. (int)(number & 0xF)).
You should use std::int32_t instead of long or std::uint32_t instead of unsigned long. long is not gauranteed to be 32 bits, especially on 64 bit platforms. I don't take as big of an issue with this as some do though because on most systems, it will be at least 32 bits.

Here is a solution that only returns one nybble at a time and throws an exception if the caller tries to access an invalid portion of the of number. Since your book hasn't talked about pointers or vectors yet, this might actually be the type of approach it is looking for (possibly minus the exception throwing if it hasn't talked about exceptions yet either). If you take away the exception throwing, the caller still cannot cause a memory access violation though. If they passed something lower than 0 or greater than 7 as the value of part they would just get a return value of 0.

#include <iostream>
#include <stdexcept>
#include <cstdint>

unsigned short get_nybble( std::uint32_t number, const unsigned short part )
{
    if( part > 7 )
        throw std::out_of_range("'part' must be a number between 0 and 7");
    return (number >> (4 * part)) & 0xF;
}

int main( int argc, char* argv[] )
{
    std::uint32_t number = 432214123;
    try {
        for( short i = 7; i >= 0; i-- )
            std::cout << get_nybble(number, i) << " ";
    } catch( std::exception& ex ) {
        std::cerr << "Error: " << ex.what() << std::endl;
    }
    return 0;
}

This may not be the best solution, but another approach I find interesting is an anonymous union combined with a bit field struct. This works with the GNU G++ compiler and should also work with Visual C++. Note that as I understand it this is potentially unsafe on non x86 systems due to the structure packing which I'd bet your book has not talked about yet either.

#include <iostream>
#include <cstdint>

#pragma pack(push, 1)
typedef union
{
    std::uint32_t value;
    struct
    {
        std::uint32_t part1: 4;
        std::uint32_t part2: 4;
        std::uint32_t part3: 4;
        std::uint32_t part4: 4;
        std::uint32_t part5: 4;
        std::uint32_t part6: 4;
        std::uint32_t part7: 4;
        std::uint32_t part8: 4;
    };
} nybbles;
#pragma pack(pop)

int main( int argc, char* argv[] )
{
    nybbles num;
    num.value = 432214123;
    std::cout << "Parts = "
        << num.part8 << ", "
        << num.part7 << ", "
        << num.part6 << ", "
        << num.part5 << ", "
        << num.part4 << ", "
        << num.part3 << ", "
        << num.part2 << ", "
        << num.part1 << std::endl;
    return 0;
}


Purchase tickets calculation with JavaScript I'm creating a page where users can purchase an adult, child or senior ticket, each with it's own pricing.

Here's the code for review. Please let me know if there are ways to improve upon this code.

// declare all variables
   var adultQty = document.getElementById('quantityAdult');
   var childQty = document.getElementById('quantityChild');
   var seniorQty = document.getElementById('quantitySenior');
   var submitBtn = document.getElementById('submitButton');
   var outputPara = document.getElementById('totalPrice');

   // generic function that takes in quantity and multiplies with appropriate price
   function calcPrice(qty, price){
     return qty * price;
   }

   // generic function that outputs final price and amout it tickets purchased
   function getMessage(qty, total){
     return outputPara.innerHTML = 'You purchased ' + qty + ' ticket(s) and your total price is $' + total + '

' + '<button>Proceed To Checkout</button>';
   }

   submitBtn.addEventListener('click', function() {

    if(adultQty.value === '0' && childQty.value === '0'  && seniorQty.value === '0'){
      alert('Please purchase at least 1 ticket');
    } else {
      var totalAdult = calcPrice(adultQty.value, 49);
      var totalChild = calcPrice(childQty.value, 20);
      var totalSenior = calcPrice(seniorQty.value, 30);

      var totalPrice = totalAdult + totalChild + totalSenior;
      var totalTix = parseInt(adultQty.value) + parseInt(childQty.value) + parseInt(seniorQty.value);

      getMessage(totalTix, totalPrice);
    }

   });

Purchase your tickets online!

$49 - Adult

$20 - Child

$30 - Senior

Quantity: <input type="text" id="quantityAdult" value="0"> Adult

Quantity: <input type="text" id="quantityChild" value="0"> Child

Quantity: <input type="text" id="quantitySenior" value="0"> Senior

<button type="submit" id="submitButton">Submit</button>

General advices

Separate pure functions from impure ones.
Pure functions don't have side effects and they are easy to test and reuse. If you have function, which
contain pure (some calculations) and impure (DOM access/modify) parts — split it on two functions.

Create functions, which will do only thing.
Do not force reader (primarily you) to keep in mind function side effects.

To improve code quality start by writing functions calls instead of functions definitions.
This will force you to think more about function signature and its usability.

Solve more general problem after solving few particular ones.
General solutions are more reusable, although could be more complex.

Line by line review

Comment // declare all variables is not very useful. This comment contains obvious information.
Comment // generic function that takes in quantity and multiplies with appropriate price
discloses inner working of function calcPrice().
Reader should not understand how function work in order to use it.
Your function name is calcPrice() and its second parameter is price. This is confusing.
You actually calculate total price.
Comment // generic function that outputs final price and amout it tickets purchased is redundant and contains typo.
Function getMessage() has two responsibilities.
First one is creating string message, second one is assigning it to innerHTML of HTML element. Split it.
You will get NaN if one of input fields is empty.
Don't define var variables in block scope. var has function scope. Use IIFE if you need to have a scope.
You duplicate function call calcPrice() three times, which is not really bad, but can be improved.

Suggested solution

function calcTotalPrice(quantity, price) {
    return quantity * price;
}

function getMessage(quantity, totalPrice) {
    return 'You purchased ' + quantity + ' ticket(s) and your total price is $' + totalPrice
}

function parseQuantity(val) {
    return parseInt(val, 10) || 0;
}

// Sum numbers in given list
function sum(list) {
    return list.reduce(function(acc, x) {
        return acc + x;
    }, 0)
}

(function() {
    // We use IIFE here to define a scope to initialize some variables here
    var submitBtn = document.getElementById('submitButton');
    var outputPara = document.getElementById('totalPrice');

    var config = [
        [document.getElementById('quantityAdult'), 49],
        [document.getElementById('quantityChild'), 20],
        [document.getElementById('quantitySenior'), 30]
    ];

    submitBtn.addEventListener('click', function() {
        var totalPrices;
        var quantities = config.map(function (data) { // In ES6 we could use array destructing: [el, price]
            var el = data[0];
            return parseQuantity(el.value);
        });

        if ( sum(quantities) > 0 ) {
            totalPrices = config.map(function(data) {
                var el = data[0], price = data[1];
                return calcTotalPrice(parseQuantity(el.value), price);
            });

            outputPara.innerHTML = getMessage( sum(quantities), sum(totalPrices) );
        } else {
            alert('Please purchase at least 1 ticket');
        }
    });

}());
<!DOCTYPE html>

    <meta charset="UTF-8">
    <title>Title</title>

Purchase your tickets online!

$49 - Adult

$20 - Child

$30 - Senior

    Quantity: <input type="text" id="quantityAdult" value="0"> Adult

    Quantity: <input type="text" id="quantityChild" value="0"> Child

    Quantity: <input type="text" id="quantitySenior" value="0"> Senior

    <button type="submit" id="submitButton">Submit</button>

    <button>Proceed To Checkout</button>


Using VBA Copying from one ,master sheet to other workbooks based on the file names provided in master book Pretty new to this hence taking it as challenge. I have a master sheet in which Column A have all the file names( complete file name with .xls extensions). I need to copy data of column b in this sheet and paste it in all the files in a particular folder in a particular tab. There can be multiple items for one name. Sample Data

Australia.xlsx  XX
Austria.xlsx    YY
Austria.xlsx    DD
Belgium.xlsx    OO
Brazil.xlsx     MM
Brazil.xlsx     NN

Here is the code i tried to write:

Sub Macro1()
    Application.DisplayAlerts = False
    Dim Country As String
    Dim i As Integer
    Dim j As Integer
    Dim k As Integer

    k = 2
    For i = 2 To 50

        Windows("try 1 split to combines").Activate
        Country = Worksheets("Sheet3").Cells(i, 1).Value
        Workbooks("try 1 split to combines").Sheets("Sheet3").Range(Cells(i, i + 1)).Copy

        Workbooks.Open Filename:= _
        "C:\Users\abhayl.INDIA\Desktop\For Primary Team\For Primary Team\" & Country & " "
        'Workbooks("Name").Activate
        ActiveWorkbook.Sheets("Specific Questions").Activate
        ActiveWorkbook.Sheets("Specific Questions").Cells(k, 2).xlPasteValues
        finalrow2 = 42 + k
        k = finalrow2 + 1
        Workbooks(Country).Sheets("Specific Questions").Activate
        Workbooks(Country).Close SaveChanges:=False
    Next i
End Sub

This is pretty basic but need some improving, can anyone help me out to better rewrite this code?

Your code and problem description is confusing. It seems as if you're halfway debugging the posted code. However, I've set up an example below that you can adapt to your situation. Some notable points:

Always use Option Explicit
Using Activate or Select is almost never necessary
Use clear variables to indicate the source and destination data, it
will help you keep things straight when writing your code and also
later when you have to maintain it.
An internal set of variables can be used to copy the data from one workbook to another. Using the Excel Copy is not necessary.
The "clean up" code at the bottom of my example is not strictly necessary in VBA (the interpreter will detect those objects as out of scope and will destroy them automatically). But for me it's a good habit to keep track of what I'm using.
Saving the previous state of the DisplayAlerts flag is a good idea when your code will change it. You don't necessarily know what the user's current setting will be and, aside from the changes your macro creates, you don't want the user to experience sudden unexpected changes.

Option Explicit

Sub Macro1()
    Dim previousAlertsFlag As Boolean
    Dim masterWB As Workbook
    Dim masterWS As Worksheet
    Dim destWB As Workbook
    Dim destWS As Worksheet
    Dim lastRow As Long
    Dim filepath As String
    Dim fullpath As String
    Dim country As String
    Dim countryData As Variant
    Dim r As Integer

    '--- save the current state of Alerts display, so it can
    '    be restored later
    previousAlertsFlag = Application.DisplayAlerts
    Application.DisplayAlerts = False

    '--- establish connections to the master data
    Set masterWB = ThisWorkbook
    Set masterWS = masterWB.Worksheets("Sheet3")
    lastRow = masterWS.Cells(masterWS.Rows.Count, "A").End(xlUp).Row
    filepath = "C:\Users\abhayl.INDIA\Desktop\For Primary Team\For Primary Team\"

    For r = 2 To lastRow
        country = masterWS.Cells(r, 1).Value
        countryData = masterWS.Cells(r, 2).Value

        fullpath = filepath & country
        Set destWB = Workbooks.Open(fullpath)
        Set destWS = destWB.Sheets("Specific Questions")
        destWS.Cells(1, 2).Value = countryData
        destWB.Close SaveChanges:=True
    Next i

    '--- clean up
    Set destWS = Nothing
    Set destWB = Nothing
    Set masterWB = Nothing
    Set masterWS = Nothing

    '--- restore the previous state of the Alerts flag
    Application.DisplayAlerts = previousAlertsFlag
End Sub


Return "support count" for a given itemset after searching a large list of transactions Not having found a satisfactory association rules implementation in Python, I set out to code my own. The code below runs reasonably quickly, but the groceries dataset used for testing is peanuts compared to the actual transactions dataset I'll be using in production (think hundreds of thousands of transactions). I'm looking for any performance improvements. Most of the objects iterated over for creating itemset frequencies are sets, but is there anything else in Python I should be thinking of, speed-wise, that's relevant to the code below? I think the final list comprehension maybe could use some improvement, but I haven't had any luck with multiprocessing.Pool yet.

import operator
from itertools import combinations
from bs4 import BeautifulSoup
import urllib2

# separate into support_count and support
def support_count(items):
    '''
    Let transactions be a list of transactions, n_transactions be the number of elements in that list, and X be an itemset.

    In pseudocode, support_count(X) - support count for an itemset of length 1:

    count = 0
    For t in transactions:
        if X is an element of t:
            count += 1
    return count

    support_count((X,Y)) - support count for an itemset of length two:
    count = 0
    For t in transactions:
        if X or Y is an element of t:
            count += 1
    return count
    '''
    # if str, then we know we're looking for the support of one item
    if type(items) is str:
        # if type(items) is str then we know we're searching
        # for support_count for one item and can use rows_indiv (list of sets of words)
        spec_rows = rows_indiv
        count = sum([1 for row in spec_rows if items in row])
    # if tuple, then we know we're looking for the support rule between two items
    elif type(items) is tuple:
        if items[0] == items[1]:
            return None
        else:
            # use rows_all_type_combos to satisfy or condition...
            # either the whole tuple is in a given transaction t, or we see if at least one of the elements is in t
            spec_rows = rows_all_type_combos
            # if any of the elements in items are in a row, count that row
            count = sum([1 for row in spec_rows if any([item in row for item in items])])
    return count

def vocab(tuples_set):
    return {word for t in tuples_set for word in t}

url = 'https://raw.githubusercontent.com/stedy/Machine-Learning-with-R-datasets/master/groceries.csv'
open_url = urllib2.urlopen(url)
soup = BeautifulSoup(open_url).encode("ascii")

# list of strings
rows = [row for row in soup.split('\n')]

# global
n_rows = len(rows)

# set of all items in transactions
items = {item for trans in rows for item in trans.split(',')}

# create all possible combinations of elements in items
# should be factorial(len(items))/(factorial(r) * factorial(len(items) - r)) number of elements in pairs list below
pairs = [(a,b) for (a,b) in combinations(items, 2)]

#### two options ####
# One: one support function for tuples and string. support_master()
# used to look up tuples and individual items
rows = [set(row.split()) for row in rows]

# Two: keep one support function for tuples, and one support function for individuals.... support_tuple() and support_indiv()
# used for looking up tuples
rows_tuple = [set((a, b) for a in row for b in row) for row in rows]

# each row is set of elements... used for looking up quickly an individual word
# used for looking up individual items
rows_indiv = [set(a) for a in rows]

rows_all_type_combos = []
for row in rows_tuple:
    row_vocab = vocab(row)
    row_to_append = list(row)
    for word in row_vocab:
        row_to_append.append(word)
    rows_all_type_combos.append(row_to_append)

for idx, row in enumerate(rows_all_type_combos):
    rows_all_type_combos[idx] = set(rows_all_type_combos[idx])

# support for all pairs of items
res = [(p, support_count(p)) for p in pairs]

One simple improvement will be to realize that sum can take a generator expression. It does not need an intermediate list which will take up a lot of memory if the number of transactions becomes large. Same goes for any, which will even use short-circuit evaluation in that case. So you can write:

count = sum(1 for row in spec_rows if items in row)

and

count = sum(1 for row in spec_rows if any(item in row for item in items))

Secondly, your specs rows_indiv seem to be used exactly once (and have the length of the number of entries). You could make it a generator to save a lot of space (and time for memory allocation).

rows_indiv = (set(a) for a in rows)

On the other hand, this is not true for rows_all_type_combos, because some of the list's members are being re-assigned in the for loop after its construction.


Printing text based on divisibility of number The following piece of code will print a text based on the divisibility of number.
If the number is not divisible by 3, 5 and 7, then the number gets printed.

Does making use of a f-string make the type cast of integers to string implicit?

def convert(number):
    result = ""
    if (number % 3 == 0):
        result += "Pling"
    if (number % 5 == 0):
        result += "Plang"
    if (number % 7 == 0):
        result += "Plong"
    if(number % 7 != 0 and number % 5 != 0 and number%3 != 0):
        result = f{'number'}
    return result

In programming, there are only three useful numbers, 0, 1, and many.

If you write an if-condition that repeats all the tests already done, you're doing something wrong.

If you write similar ad hoc code for three special cases, you're doing something wrong.

E.g. adding "11 Plung" will require two more lines for the special case and the "no match" case will have to be made even longer and uglier.

Write simple code for a general case, and then supply specific data as needed:

def convert(number):
    names = [ [3, "Pling"], [5, "Plang"], [7, "Plong"] ]
    result = ""
    for value,message in names:
        if not number % value: result += message
    if message == "": message = f"{number}"
    return result

for index in range(1,25): print(index, convert(index))

That way, if you later need to add the "11" case, it's a simple change, and more importantly it happens in only one place.


Java Manual-Reference-Counting I have a Java game development framework. Certain classes, such as Texture, need to be disposed as soon as you don't need them - we cannot depend on the Garbage Collector.

For this, I have decided to enforce reference counting like Objective-C. You call retain() when you want to own something, and release() when not anymore. This increases/decreases a counter. When the counter hits 0, dispose() is called - at which point, Texture will gladly free its resources such as pixel data etc.

NOTE

The reason I can't depend on the Garbage Collector, is because things like the pixel data aren't stored in Java, but rather by OpenGL in some bindings. Similar things happen with audio data. So it is necessary to call OpenGL to free that data manually.

On my first attempt, I made an interface:

public interface Retainable {
    void retain();
    void release();
    void dispose();
}

So a class that wants to be part of this MRC scheme would be like

class Texture implements Retainable {
    int retainCount = 1;    // 1 because I want it to begin retained
    public void retain() {
        retainCount += 1;
    }
    public void release() {
        if (--retainCount == 0) {
            dispose();
        }
    }
    public void dispose() {
        // Free pixel data
    }
}

This works and is OK.

However, every class that wants to do be part of the MRC scheme would need to:

Create an int field
Implement retain()
Implement release()
Implement dispose()

From the four points, the only non-redundant point is the last one. Obviously dispose() will differ from one class to another. But all the other points won't differ from class to class - I am repeating code, and I don't like that.

On an attempt to improve the simplicity of this scheme, I decided to make a manager class to keep track of all Retainable objects. The class will have a map, where the keys are the Retainable objects and the values are their integer counts. The class will have retain(obj) and release(obj), which increase/decrease the count in the map. When the count hits 0 it will call dispose() and then remove the object from the map.

public abstract class Manager {
    private static HashMap<Object, Integer> data = new HashMap<Object, Integer>();
    public static void retain(Retainable object) {
        if (data.containsKey(object)) {
            data.put(object, data.get(object) + 1);
        }else{
            data.put(object, 1);
        }
    }
    public static void release(Retainable object) {
        if (data.containsKey(object)) {
            int count = data.get(object);
            if (count == 1) {
                data.remove(object);
                object.dispose();
            }else{
                data.put(object, count - 1);
            }
        }
    }
}

While I understand that "global" stuff isn't likeable, I decided to sacrifice that for the sake of simplifying this scheme as much as possible.

I really like this approach, because now my classes don't need to implement so much redundant stuff. They only need to implement dispose().

There is one thing that still bugs me

I would like that, when you construct a Retainable object, it will automatically have a retain count of 1 (so you don't need to call retain() every time you make a new object).

Then, this means that a Retainable class needs to call

Manager.retain(this);

In their constructor... for all classes. Ahh, if only there was a way to construct objects already retained without having to add this line in their constructor (I know this is a super small and almost meaningless thing, but heck...).

Do you have any ideas to improve this? And, very hopefully, a solution to the annoyance above?

You should not be manually counting references. Instead, you should be letting the garbage collector do the reference management for you, and then hook in to the garbage collection framework when the reference is cleared in Java.
The right tool for the job is the finalize() method. This often-misused method, is actually purpose-designed for this exact case:

the usual purpose of finalize, however, is to perform cleanup actions before the object is irrevocably discarded. For example, the finalize method for an object that represents an input/output connection might perform explicit I/O transactions to break the connection before the object is permanently discarded.

public class Texture {
    ....
    @Override
    protected void finalize() {
        // clear resources kept in external areas to Java....
        .....
    }
}

Note:

The Following 'Old' answer is based on the first version of the question that made no reference to non-Java memory....

I believe you have a basic misunderstanding of Java memory management. What you say you are doing, you are not actually doing, and this statement 'This works and is OK' is not what you think is happening.
You cannot possibly tell java to release the memory for an object. What you can do though, is remove all references to an object, and, at some point Java GC will scan through and identify that memory as no longer used, and available for reuse.
In your class, you have the 'pixel data', perhaps an array of private byte[] pixels = ..., and in your dispose() method you call pixels = null, which removes the reference to the byte[] array. When the Garbage collector sweeps the heap, it will identify that the byte[] array is no longer referenced, and it will release that space.
This is what you are seeing, and you think 'it works'. But, that is not the solution you should be using.
What you should be doing, is not referencing the texture when it is no longer needed!
In other words, there's a bigger picture here... there is somewhere in you code where you have done:
Texture mytexture = new Texture(pixeldata);

and then, you have a reference to mytexture that you are not releasing.
If you say mytexture = null, then the garbage collector will release the pixel data as well as the texture object.
Instead of finding all the places in your code that use the texture, and calling retain() and release() they should just set their Texture variable to null, and move on if they no longer need the Texture.
If you are not releasing your pixels after doing that, it means you have some place in your code where you are still holding references to the Texture, perhaps some value in a HashMap, an array, or somewhere else where you are holding a reference. Remove those entries, and you will be fine.
Note: it is possible that your program is part of a very, very, tiny, small proportion of programs that may need finer memory control, and need to keep a Texture cached even though no-one is using it at a particular moment in time. Java has Soft-References that may help, but that is not likely to be your solution, and is more complicated than this post warrants....


Social network broadcast message algorithm Working on below problem and post problem and my current code. My idea is to always iterate from the minimal in-degree node which has not been visited before. Then based on this node to do a BFS. I am not sure if there are better ways to implement in terms of performance. If you also find any other code issues or bugs, please point them out as well.

Problem,

Let's suppose that I'd like to spread a promotion message across all people in Twitter. Assuming the ideal case, if a person tweets a message, then every follower will re-tweet the message.

You need to find the minimum number of people to reach out (for example, who doesn't follow anyone etc) so that your promotion message is spread out across entire network in Twitter.

Also, we need to consider loops like, if A follows B, B follows C, C follows D, D follows A (A -> B -> C -> D -> A) then reaching only one of them is sufficient to spread your message.

Input: A 2x2 matrix like below. In this case, a follows b, b follows c, c follows a.

   a b c
a  1 1 0
b  0 1 1
c  1 0 1

Output: List of people to be reached to spread out message across everyone in the network.

My code ,

# below two connectMatrix are just two test cases, you can use either one for the testing, or use new ones
connectMatrix=[
    [1,1,0,0],
    [1,1,1,0],
    [0,0,1,1],
    [1,0,0,1]
]

connectMatrix=[
    [1,1,0],
    [0,1,1],
    [1,0,1]
]
inDegree = [0] * len(connectMatrix)
visited = [0] * len(connectMatrix)
allVisited = 0

# if (x,y) == 1, it means x follows y
# if (x,y) == 1, it means y can send message to x

def BFS(connectMatrix, visitedPersons, personID):
    global allVisited
    visitQueue = []
    result = []
    visitQueue.append(personID)
    result.append(personID)
    visitedPersons[personID]=1
    allVisited += 1
    while len(visitQueue) > 0:
        personID = visitQueue.pop(0)
        # downstream: check what other person this person can reach to
        for i in range(len(connectMatrix)):
            if connectMatrix[i][personID] == 1 and visitedPersons[i] == 0:
                visitedPersons[i] = 1
                visitQueue.append(i)
                allVisited += 1
                result.append(i)

    return result

if __name__ == "__main__":

    for j in range (len(connectMatrix)):
        degree = 0
        for i in range(len(connectMatrix)):
            if connectMatrix[i][j] == 1:
                degree += 1
        inDegree[j] = degree

    results = []

    # find non-visited person with minimal in-degree
    while allVisited < len(connectMatrix):
        minDegree = max(inDegree) + 1
        nextIndex = -1
        # find next person to iterate
        for i in range(len(connectMatrix)):
            if (visited[i] == 0) and inDegree[i] < minDegree:
                nextIndex = i
                minDegree = inDegree[i]
        results.append(nextIndex)
        print BFS(connectMatrix, visited, nextIndex)

    print 'persons to send message, ', results

1. Bug

The algorithm does not always find the minimum number of messages! Suppose that we have the following follower graph (an arrow from A to B means that A follows B).

To reach everyone, only one message is needed (to C or D; either will do). But when I represent this graph as an adjacency matrix:

connectMatrix = [
    [1, 1, 1, 0],
    [1, 1, 0, 0],
    [0, 0, 1, 1],
    [0, 0, 1, 1],
]

and run the code from the post, I get the following output:

[0, 1]
[3, 2]
persons to send message,  [0, 3]

2. Review

The code is not portable to Python 3 because it uses the print statement. It would be easy to make it portable by using from __future__ import print_function.
The code is awkward to test because it only runs as a script and so it can't be run from the interactive interpreter, or from a unit test case. It would be better to organize the code into functions. For example, I would write:

def min_sinks(connectMatrix):
    """Return the smallest list of sinks such that each node in the graph
    has a path to at least one of the sinks.

    """
    inDegree = [0] * len(connectMatrix)
    visited = [0] * len(connectMatrix)
    allVisited = 0
    # ... etc ...

But this doesn't quite work because BFS uses the global variable allVisited. The way to fix that is to observe that BFS adds one to allVisited for each node it adds to result, and so in min_sinks we can update allVisited as follows:

visited_nodes = BFS(connectMatrix, visited, nextIndex)
allVisited += len(visited_nodes)

The name allVisited doesn't describe the variable very well: better would be something like visited_count.
The code in BFS uses the name persons for the nodes in the graph. But nothing in this code depends on the fact that the nodes represent persons. So it would be better to use a neutral name like nodes.
Popping the leftmost item of a list takes time proportional to the length of the list (see the time complexity page on the Python wiki). It's necessary to use a collections.deque to ensure that popping is efficient.
The data structure visitedNodes is a list whose value at index i is 1 if the node i has been visited, and 0 if it has not. It would be clearer to use True and False instead of 1 and 0.
The code in the post uses an adjacency matrix to represent the graph. But this is a poor representation for this algorithm, for two reasons. First, an adjacency matrix takes \$Θ(n^2)\$ space to store a graph on \$n\$ nodes, and this is only efficient if the graph is dense, whereas a graph of Twitter followers is (most likely) sparse. Second, at each iteration of the breadth-first search, the code needs to iterate over the followers of a node, and that takes \$Θ(n)\$ in the adjacency matrix represention, which is inefficient if the graph is spare.

It would be better to use a different representation, for example adjacency list.

3. Correct algorithm

The correct algorithm for finding the minimum number of sinks so that every node has a path to at least one sink is as follows:

Find the strongly connected components in the graph, for example using Tarjan's algorithm. (A strongly connected component is a group of nodes such that there is a path from every node in the component to every other node in the component. For example, a cycle.)
In the graph, replace each strongly connected component with a new single node (updating the edges so that if there was an edge between a node \$n\$ and any node in the component, there is a corresponding edge between \$n\$ and the new node).
If the graph has no nodes, stop.
Find a node of out-degree zero and add it to the set of sinks. (If the node replaced a strongly connected component in step 2, then add any node from the original component.)
Remove the sink from the graph, along with every node that has a path to the sink.
Go to step 3.

For example, suppose that we start with this graph:

At step 1 we find the strongly connected components:

At step 2 we replace each component with a new single node:

At step 4 we find that there is one node of out-degree zero (the node CD) so we add either C or D to the set of sinks (it doesn't matter which). At step 5 we remove CD from the graph together with every node that has a path to CD. This leaves the graph empty, so we are done.

4. Answers to questions

You asked in comments whether "out-degree zero" in step 4 of the algorithm is a mistake for "in-degree zero".

The answer is no: "out-degree" is correct. Remember that the graph is a follower graph. So a node with out-degree zero represents someone who doesn't follow anyone (or a strongly connected component nones of whose members follows anyone outside of the component). The only way that someone who doesn't follow anyone can receive the message is if they receive it directly. So that person needs to be added to the set of sinks.
You asked for clarification about the direction of edges in the graph. I took this directly from the problem statement:

  Also, we need to consider loops like, if A follows B, B follows C, C follows D, D follows A (A → B → C → D → A) then reaching only one of them is sufficient to spread your message.

This says that "A follows B" implies "A → B" which implies that there is an edge from A to B. I stated this explicitly in §1 above, where I wrote:

  (an arrow from A to B means that A follows B)

Of course you are free to represent the graph the other way round, so that if A follows B then B → A. If you do this then this doesn't affect the strongly connected component part of the algorithm (since a component remains strongly connected if you reverse all the edges), but you will need to swap "out-neighbour" and "in-neighbour" in the remainder of the algorithm.


Elixir - Points bot using Ecto I'm making a bot that will be used for "points" tracking and such, like a reward system and I'm using Ecto with postgrex, this is my approach so far, it works but I'm not sure if I'm doing it in an "Elixir", functional way or if I can improve this a bit ^-^

defmodule Cord.Commands do
  ...
  def add_me(msg) do
    case Repo.get_by(Rewards, user: msg.author.id) do
      # Adds user if it doesn't exist
      nil ->
        case Repo.insert(%Rewards{user: msg.author.id}) do
          {:ok, _struct} ->
            Api.create_message(msg.channel_id, ":white_check_mark:")
          {:error, _changeset} ->
            Api.create_message(msg.channel_id, ":x:")
        end
      _ ->
        Api.create_message(msg.channel_id, "You're already registered")
    end
  end

  # Maps to each id in the list and updates each one
  def thanks(msg) do
    Enum.map msg.mentions, fn user ->
      repo_user = Repo.get_by(Rewards, user: user.id)
      points = repo_user.quantity
      repo_user = Ecto.Changeset.change(repo_user, quantity: points + 1)
      Repo.update!(repo_user)
      Api.create_message(msg.channel_id, "#{msg.author.username} gave a :cookie: to #{user.username}")
    end
  end

  def not_found(_), do: IO.inspect :ignore
end

Here are a few suggestions...

You could DRY up your code in add_me by returning the string and then making the API call in one place. Also, you should call Repo.insert/1 with a changeset. This will ensure your validation is checked. You can also remove the nesting with the with special form. Its a really handy function that I recommend you learn if haven't already.

  def add_me(msg) do
    message =
      with nil <- Repo.get_by(Rewards, user: msg.author.id),
           changeset <- Rewards.changeset(%Rewards{}, %{user: msg.author.id}),
           {:ok, _} <- Repo.insert(changeset) do
        ":white_check_mark:"
      else
        {:error, _} -> ":x:"
        _ -> "You're already registered"
      end
    Api.create_message(msg.channel_id, message)
  end

You can reduce some of the one-time variables in thanks like this:

  def thanks(msg) do
    Enum.map msg.mentions, fn user ->
      repo_user = Repo.get_by(Rewards, user: user.id)

      repo_user
      |> Ecto.Changeset.change(quantity: repo_user.points + 1)
      |> Repo.update!
      Api.create_message(msg.channel_id, "#{msg.author.username} gave a :cookie: to #{user.username}")
    end
  end

Finally, it appears that you don't care about the type of error on the Repo.insert. Keep in mind that the changeset returned with {:error, changeset} contains the validation errors.

NOTE:

I see 4 common style / issues with programmers learning Elixir.

error Calling a side affect free function without using the return value. Data in Elixir is immutable, so you need need to either capture the results of a function, call it as the last statement so it returned to the caller, or create a side effect in the function (send an event, write to database, file, etc)
warning Use a variable outside the scope of the block it was set in. In other words, don't set a variable inside an if, case, or cond, then use it later
style Use lots of single use variables. This is typically a sign that the pipe |> operator may clean up the code.
style Use logs of conditional statements where multi-clause function heads with pattern matching and guards may make the code cleaner, with less nesting


Fastest way to create a nested treeview I am trying to create a dynamic Treeview in HTML using jQuery with following code,

Code with data and working example : https://jsfiddle.net/8jt0crv7/

var Menu = function(data) {
    this.data = data;
};

Menu.prototype.render = function(root) {
    var ul = $("

");
    var li = $("

");
    li.append($("", {
        href: this.data.url,
        text: this.data.title
    })).appendTo(ul);
    ul.appendTo(root);
    if (this.data.menu) {
        Menu.renderMenus(this.data.menu, $("

").appendTo(ul));
    }
};

Menu.renderMenus = function(menus, root) {
    $.each(menus, function(key, val) {
        var m = new Menu(val);
        m.render(root);
    });

console.time("Tree");

Menu.renderMenus(children, $("#container"));

console.timeEnd("Tree");

Some tests I ran on latest Chrome on Windows 10 are followings :

Tree: 129.593ms
Tree: 145.519ms
Tree: 179.777ms
Tree: 146.314ms
Tree: 174.272ms

Is there anyway I could improve performance of this code as I have trimmed it from a very big setup into example code. I am open to not use jQuery or any other plugin to make it faster.

All modern browsers support ES5 which means array.forEach will exist. You can use that to replace $.each. The overhead of $.each comes from checking if it's an array or object and using the appropriate loop operation.

The same goes for the creation of li and ul. You can simply replace it with document.createElement. The overhead of $() is because it's a multi-purpose function. It can create an element, alias document.ready, wrap an array or object, grab elements from the DOM, an alias to jquery.find if given a context, and more.

You don't really need to go OOP on this one. A simple recursive function is all you need for this to work. Going the OOP route means you're creating instances of an object, in this case, Menu - an unnecessary overhead. If you want structure, the data is already your structure.

Try to batch the render, preferably all at once to the very end of the process. This is because DOM manipulation is slow. In your case, you already appended the list to the container before processing the descendants nodes. This means every time you append a descendant, the browser has to render that in the DOM.

Lastly, that <div> is unnecessary.

Here's a rewritten version using a simple recursive function. It runs 3~6ms depending on the browser's mood.

function createMenu(nodes, container){
  var list = document.createElement('ul');

  // Use native array.forEach
  nodes.forEach(function(node){
    var listItem = document.createElement('li');
    listItem.textContent = node.Name;
    list.appendChild(listItem);

    // If no children, return early to skip code below.
    if(!node.Children) return;

    // Process descendants next.
    var childList = document.createElement('li');
    createMenu(node.Children, childList);
    list.appendChild(childList);
  });

  // Only when the list is fully formed do we append it to the parent
  container.appendChild(list);
}

createMenu(children, document.getElementById('container'));


Java Crypto library number of padding bytes using PKCS5Padding I have implemented AES encryption in Java using the following function. What puzzles me is the number of padding bytes added using the PKCS5Padding method. For example, when calling the following function with a text byte of length 12, the output is a 32 byte array. I would expect a 16 byte output of which 4 bytes are padding.
How can the initialization of the cipher be changed to do this?

public byte[] do_decryption(byte[] text) {

        String key_val = new String("1234567890123456");
        byte[] key = key_val.getBytes();

        try {
            Cipher cipher = Cipher.getInstance("AES/ECB/PKCS5Padding");
            SecretKeySpec secretKey = new SecretKeySpec(key, "AES");
            cipher.init(Cipher.ENCRYPT_MODE, secretKey);
            System.out.println("Block size: " + cipher.getBlockSize());

            byte[] output = cipher.doFinal(text);
            return output;

        } catch (Exception e) {
            e.printStackTrace();
        }
    }

"PKCS5Padding" in Java is really PKCS#7 compatible padding. It adds 1 to 16 bytes for a 16-byte-block cipher like AES. If you view those bytes as hexadecimals, e.g. in a debugger, then it may look like 32 characters instead.

ECB mode is almost never secure enough to be used. Use GCM or another authenticated mode (e.g. CCM or EAX).

public byte[] do_decryption(byte[] text) {

For Java we'd use CamelCase starting with a lower case letter (doDecryption or performDecryption).

text is not a good name for a byte array. plaintext for encryption or ciphertext for decryption could be acceptable though.

String key_val = new String("1234567890123456");
byte[] key = key_val.getBytes();

Obviously the key should not be in the source code.

Furthermore, you should almost never use getBytes without specifying the character encoding (e.g. use StandardCharsets.US_ASCII as parameter for the overloaded function.

Variable names except constants should not contain underscores.

cipher.init(Cipher.ENCRYPT_MODE, secretKey);

Already noted in the comments, either the method is badly named or this is plain wrong. Encryption != decryption.

e.printStackTrace();

Please handle exceptions properly.


TLE in APS - Amazing Prime Sequence in Python at Spoj This series is similar to Fibonacci Series defined as : a[0] = a[1] = 0  and For n > 1, a[n] = a[n - 1] + f(n), where f(n) is smallest prime factor of n.
for N in (1 < N < 10^7). Spoj Link for this problem .

I have solved this problem in c & Python. But due to long range, I am getting Time Limit Exceeded in Python. For small range this is working fine. Is there any alternative way do this in Python or can this be more optimized.

lst = []
lst.append(0)
lst.append(0)
i = 2
while i <10000000 :
    t = 2
    while t <= i :
        if i % t == 0 :
            break
        t +=1
    lst.append(lst[i-1]+t)
    i +=1
for _ in range(int(input())):
    print(lst[int(input())])

The best way to solve this is to precompute the prime factors of n for n in range(1:10**7) I would suggest using a sieve of Eratosthenes.

def sieve5(n):
"""Return a list of the primes below n."""
prime = [True] * n
result = [2]
append = result.append
sqrt_n = (int(n ** .5) + 1) | 1    # ensure it's odd
for p in range(3, sqrt_n, 2):
    if prime[p]:
        append(p)
        prime[p*p::2*p] = [False] * ((n - p*p - 1) // (2*p) + 1)
for p in range(sqrt_n, n, 2):
    if prime[p]:
        append(p)
return result

Once you have the list of primes, you can just run the same type of algorithm you would for the fibs.


Delete all virus-affected files with certain filenames Purpose of the code is to delete all virus affected files from your pc.

   private static void SearchAndDeleteFiles(String directoryName, List<File> files) {

        File directory = new File(directoryName);

        File[] fList = directory.listFiles();

        if (fList != null) {

            for (File file : fList) {
                if (file.isFile() && (file.getName().endsWith(".ccc") || file.getName().startsWith("_how_recover_anj")
                        || file.getName().startsWith("_how_recover_jav"))) {
                    files.add(file);
                    System.out.println((new StringBuilder()).append(file).toString());

                    boolean isSuccess = file.delete();

                    System.out.println(
                            (new StringBuilder()).append(file).append(" delete status: ").append(isSuccess).toString());

                } else if (file.isDirectory()) {
                    SearchAndDeleteFiles(file.getAbsolutePath(), files);
                }

            }
        }

    }

For what you are trying to achieve, Java has a very nice solution since Java 7: java.nio.Files.

The Files class comes with a lot of useful methods that do what you did more expressively and easier to understand.

Instead of listing all files in a directory and looping over them, you can actually walk the entire file tree, including sub-directories, if you want.

You can do this the "callback-style" or using Java Streams. I like streams, so I will try and rebuild what you did, but with java.nio.Files and java.util.stream.Stream. Also I am going to use Java 11's var keyword, because I don't like typing out all the types unless absolutely necessary:

/**
 * Deletes all files with their names ending in ".ccc" or starting with
 * "_how_recover_anj" or "_how_recover_jav".
 *
 * @param directoryName The directory to search in.
 * @return A map of all files which were attempted to be deleted and their
 *         deletion status.
 */
private static Map<File, Boolean> SearchAndDeleteFiles(String directoryName) throws IOException {
    try (var fileStream = Files.walk(Paths.get(directoryName))) {
        return fileStream.map(path -> path.toFile())
                .filter(file ->
                        file.isFile()
                        && (file.getName().endsWith(".ccc")
                        || file.getName().startsWith("_how_recover_anj")
                        || file.getName().startsWith("_how_recover_jav")))
                .peek(file -> System.out.println(file))
                // Actually delete the file and record the status.
                .map(file -> Map.entry(file, file.delete()))
                .peek(entry -> System.out.printf("%s delete status: %s%n", entry.getKey(), entry.getValue()))
                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
    } catch (IOException e) {
        throw new IOException(e);
    }
}

First you see, that I actually provide some documentation. Tell the user (even your future self), what this method is supposed to do, what is wants as parameters and returns as a result.

Also notice, that I removed the files argument. You changed that list and added the files that you traversed (not the ones that were successfully deleted) to that list. This is a code smell as you are modifying the incoming collection which could very possibly be immutable without checking.
In my solution, this is no longer necessary, as we don't need any recursion and actually return which files we traversed and attempted to delete.

Then I am following the recommendation written in the documentation and walk the file tree in a try-with-resources block. This makes sure, that in any case, the file handles to the directories are cleanly closed.

The stream is expressive and pretty self-explanatory, it first maps all the file paths to actual Java File objects, filters the ones matching your criteria, then prints the file name. You don't need to construct StringBuilders for every string you print, just more complex expressions and actually constructed strings.

Then it maps the file to itself and it's deletion status. This status is then printed, just like in your method and collected into a map.
The map is then returned.

If any exception is thrown, it is re-thrown and the stream of paths is cleanly closed.

The Files.walk method is implicitly recursive and lazy, so you don't need to worry about performance or traversing into sub-directories.

You could make this method even more flexible, if you want, but I hope this answers your question.


Problem: Traffic Flow I have the following task:

  The Shallow Alto city council has organized a special committee to
  review the traffic situation in the city. Despite the fact that there
  is nothing whatsoever to do in the city, citizens still write
  complaints about the amount of time it takes them to get from one
  unexciting location to the next. The committee has been asked to
  figure out how long it takes someone to travel down the streets of
  Shallow Alto given the current traffic light programming. To do this,
  they want you to write an algorithm which, given the speed a car
  travels down a road and the timing of the traffic lights, returns the
  amount of time it takes the car to travel down the street. The traffic
  lights will be in an int[], with each element representing the amount
  of time in seconds between signal changes. Initially, all traffic
  lights have just turned green, and are at the beginning of their
  cycle. The order of the traffic lights in the int[] will be the order
  in which a car encounters them. The speed you will be given is in
  meters per second. Assume that the car starts 150 meters before the
  first traffic light, that there are 150 meters between each traffic
  light, and that the car stops 150 meters after the last traffic light.
  Disregard all acceleration and deceleration; a car is either at its
  given speed or entirely stopped, and it takes no time to go from one
  state to the other. If a car reaches a traffic light just as it turns
  red, it immediately stops and waits for it to turn green again (at
  which point it starts moving immediately). If the light just turned
  green, there is no wait and the car drives on through. Return the
  time, in seconds, that it takes the car to travel the entire distance.
  Round down any fractional parts, (for example 55.5 becomes 55 and 44.9
  becomes 44), but do not do this until returning.

This is my first draft:

public class Car
{
    public double Speed { get;set; }

}

public class Light
{
    public int Number { get; set; }

    public double SwitchTime { get; set; }
    public bool IsGreen(double time)
    {
        double num = Math.Floor(time / SwitchTime);

            return num % 2 == 0;
    }
    public double TimeToWait(double time)
    {
        if (!IsGreen(time))
        {
            return ((time % SwitchTime) < 0.01) ? SwitchTime : (time % SwitchTime);
        }
        else
        {
            return 0;
        }
    }

}

class Traffic
{
    public Car mycar;
    public IList<Light> LightList;
    public static double lengthbetweenLights { get; set; }

    public double getTravelTime()
    {
        double currenttime = lengthbetweenLights / mycar.Speed;

        foreach (Light light in LightList)
        {
            currenttime += lengthbetweenLights / mycar.Speed + light.TimeToWait(currenttime);
        }

        return currenttime;
    }
}

class Program
{
    static void Main(string[] args)
    {
        var car1 = new Car
        {
            Speed = 20
        };
        List<Light> lights1 = getLightList();

        var mytraffic = new Traffic
        {
            mycar = car1,
            LightList = lights1

        };

        Traffic.lengthbetweenLights = 150;

        var traveltime = mytraffic.getTravelTime();

        Console.WriteLine(string.Format("Time for car: {0}", traveltime));
        Console.ReadLine();
    }

    private static List<Light> getLightList()
    {
        List<Light> lstProducts = new List<Light>();
        //input 1
        lstProducts.Add(new Light { Number = 0, SwitchTime = 10 });
        lstProducts.Add(new Light { Number = 1, SwitchTime = 20 });
        lstProducts.Add(new Light { Number = 2, SwitchTime = 30 });
        return lstProducts;
    }

}

You can also see my code on GitHub.

I need to improve my code, make it more OOP and use Design Patterns.

Question is: which design patterns I can use here?

Naming

I strongly recommend you read through the official C# naming conventions. Here are a few rules to keep in mind:

Properties are UpperCamelCase
Private fields are lowerCamelCase, sometimes prepended with an underscore
Methods are UpperCamelCase
Local variables are lowerCamelCase
Test methods should be descriptive [more information]

Applied to your code, here are a few changes to make:

TestMethod1 -> TimeToWait_WithFiveMinutes_ShouldReturnZero
mycar -> currentCar
lengthbetweenLights -> lengthBetweenLights
getTravelTime -> GetTravelTime

Also keep in mind that your naming should make sense in light of the contest: MyCar does not really make sense in the sense of a Traffic analysis: what if I want to calculate it with someone else's car? Therefore I suggest making it CurrentCar instead.

This is part of the encapsulation I talk about next, but keep in mind that hiding the implementation details by not putting them in the variable's name, is also a form of encapsulation.

Right now you have a variable named LightList which tells me that it concerns a list of lights. Except, I already know it's a list because I can see the type. But what if you want to go more general and make it a ICollection instead? Or you want to group them with the amount of dogs that peed against it: Dictionary<Light, int>. Now your name doesn't fit anymore and you'll have to change it (and make version control ugly).

Methods are actions: you perform an action on the instance. What kind of action is TimeToWait? I would have expected it to be a property from the way it's worded. GetTimeToWait makes it clear that I will receive a value (and just that).

Encapsulation

Definitely read more on the topic of encapsulation, a cornerstone of OO-Programming. Your Traffic class has public fields which are open for modification. By forcing to do this through setters (an auto-generated property is a setter!) you can satisfy encapsulation and force whoever wants to modify it to go through your setter first. This means that you can add validation logic before touching the backing variable.

Instead what you should do is work with a public property like

public Car CurrentCar { get; set; };

[More information on fields and properties]

Events

Traffic lights that get their status updated every second sounds like a good option to put events in here. You could keep counters elapsedTime and switchTime and a flag isGreen so your light can articulate its state to callers. This would make your methods IsGreen() and GetTimeToWait() more intuitive: you don't have to call them with an argument to signify the current time.

Although this might be overdoing the assignment a little bit.


Area of an irregular polygon """
Provides a way to caculate the area of an arbitrary
n-sided irregular polygon.
"""
import doctest
import math

def heron(a,b,c):
    """
    Uses the heron formula to calculate the area
    of the triangle where `a`,`b` and `c` are the side
    lengths.

    >>> heron(3, 4, 5)
    6.0
    >>> heron(7, 10, 5).__round__(2)
    16.25
    """
    s = (a + b + c) / 2
    return math.sqrt(s * (s - a) * (s - b) * (s - c))

def pytagoras(a, b):
    """
    Given the cathets finds the hypotenusas.

    >>> pytagoras(3, 4)
    5.0
    """
    return math.sqrt(a**2 + b**2)

def distance(point_1, point_2):
    """
    Computes the cartesian distance between two points.

    >>> distance((0,0), (5,0))
    5.0
    """
    delta_x = point_1[0] - point_2[0]
    delta_y = point_1[1] - point_2[1]
    return pytagoras(delta_x, delta_y)

def triangle_area(triangle):
    """
    Wraps `heron` by allowing points inputs instead
    of sides lengths inputs.

    >>> triangle_area([(0,0), (0,3), (4,0)])
    6.0
    """
    side_1 = distance(triangle[0], triangle[1])
    side_2 = distance(triangle[1], triangle[2])
    side_3 = distance(triangle[2], triangle[0])
    return heron(side_1, side_2, side_3)

def triplets(list_):
    """
    Yields items from a list in groups of 3.

    >>> list(triplets(range(6)))
    [(0, 1, 2), (1, 2, 3), (2, 3, 4), (3, 4, 5)]
    """
    for index, item in enumerate(list_[:-2]):
        yield item, list_[index + 1], list_[index + 2]

def polygon_area(polygon):
    """
    Calculates the area of an n-sided polygon by
    decomposing it into triangles. Input must be
    a list of points.

    >>> polygon_area([(0,0), (0,5), (3,0), (3, 5)])
    15.0
    """
    return sum(triangle_area(triangle)
                   for triangle in triplets(polygon))

def _main():
    doctest.testmod()

if __name__ == "__main__":
    _main()

The "shoelace" formula finds the area of a simple polygon:

from __future__ import division

def polygon_area(points):
    """Return the area of the polygon whose vertices are given by the
    sequence points.

    """
    area = 0
    q = points[-1]
    for p in points:
        area += p[0] * q[1] - p[1] * q[0]
        q = p
    return area / 2

if __name__ == '__main__':
    square = [(0,0), (0,1), (1,1), (1,0)]
    irregular = [(1,1), (1,5), (2,5), (5,1)]
    print('Unit square area: {}'.format(polygon_area(square)))
    print('Irregular polygon area: {}'.format(polygon_area(irregular)))

This outputs:

Unit square area: 1.0
Irregular polygon area: 10.0


REST Web Api unit test endpoint validity with NUnit and Moq I am unit testing that the URL that I give to my IRestClient is valid. This client talks to a third party Web API.

The test is as below:

    [Test]
    [ExpectedException("System.UriFormatException")]
    public void MakeRequest_EndPointIsInValid_ThrowsUriFormatException()
    {
        var stub = new Mock<IRestClient>();
        stub.Setup(x => x.MakeRequest()).Throws<UriFormatException>();

        string results = new RestClient("example", "method=payment&params[Key1]=Value1&params[Key2]=Value2&params[Key3]=Value3").MakeRequest();
    }

The RestClient class under test is

public class RestClient : IRestClient
{
    /// <summary>
    /// Web Api Url End point.
    /// </summary>
    public string EndPoint { get; set; }

    /// <summary>
    /// The Post data.
    /// </summary>
    public string PostData { get; set; }

    /// <summary>
    /// Do Request.
    /// </summary>
    /// <returns></returns>
    public string MakeRequest()
    {
        using (WebClient client = new WebClient())
        {
            client.Headers[HttpRequestHeader.ContentType] = "application/x-www-form-urlencoded";

            if (string.IsNullOrWhiteSpace(PostData)) throw new ArgumentNullException("PostData");
            if (string.IsNullOrWhiteSpace(EndPoint)) throw new ArgumentNullException("EndPoint");

            // I want to test this Uri parsing.
            Uri uriResult;
            if (!Uri.TryCreate(EndPoint, UriKind.Absolute, out uriResult) || uriResult.Scheme != Uri.UriSchemeHttps) throw new UriFormatException("EndPoint");

            return client.UploadString(uriResult, PostData);
        }
    }
}

How is this test and class under test?
Is my fake object a stub or a mock? I believe it's a stub as I am
just forcing the return of a UriFormatException.
Any improvements?

I think you are confused about what a mock is actually supposed to be used for.  A mock is to create a fake object to pass into a parameter, and is very useful when testing systems that use dependency injection exclusively.

Take this for example:

public interface IFoo
{
     string SomeMethod(IBar bar);
}

public interfact IBar
{
    int CalculateSomething();
}

public class Foo : IFoo
{
     public string SomeMethod(IBar bar)
     {
         if (bar.CalculateSomething() > 10)
         {
              // insert more code

              return "Completed";
         }

         return string.Empty;
     }
 }

One of your tests is to ensure the // insert more code section works.  To get into that code you need the IBar.CalculateSomething() to return a value > 10.  You also don't want to create an instance of the Bar class, because then you are not testing one unit of code. This is where mocking comes into play.  The way to make sure you hit the code is to pass a mock into the method, that is set up to pass the if statement

[Test]
public void EnsureThatInserMoreCodeWorks()
{
    var bar = Mock.Create<IBar>();
    bar.SetUp(b => b.CalculateSomething()).Returns(15);

    var sut = new Foo();

    var result = sut.SomeMethod(bar.Object);

    Assert.That(result, Is.EqualTo("Completed");
}

This is really simple, and I hope there is more to the // insert more code section to test, but this gives an easy to follow example of using mocks.

To make your test pass, you would simply have to pass in an invalid uri string.  No Mock needed.


Showing a list of plugins to filter How can I refactor this function? I show a list of plugins in JSF, and I should filter them. I added function filterPlugins but I have some questions:

Should we simplify block with conditional expression?
I reassign global variable "plugins" to introduce my filter function (is it not bad practice?)

load()

@NotNull
@DataModel("plugins")
List<Plugin> plugins;

public void load() {
    plugins = pluginManager.getPlugins();

    plugins = filterPlugins(searchParam);

    SortingUtil.sort(plugins, SortingUtil.SortType.ID_ASC);
    if (plugins.size() > 0) {
        if (plugin != null && plugins.contains(plugin)) {
            selectPlugin(plugin);
        } else {
            selectPlugin(plugins.get(0));
        }
    } else {
        plugin = null;
    }
}

@SuppressWarnings("unchecked")
private List<Plugin> filterPlugins(final String searchParam ){
      return (List)Iterables.filter(plugins, new Predicate<Plugin>() {
        @Override
        public boolean apply(@Nullable Plugin plugin) {
            return  plugin.getName().contains(searchParam)
                    || plugin.getNetworkClasses().contains(searchParam)
                    || plugin.getClassName().contains(searchParam);
        }
    });
}

@Override
public void selectPlugin(Plugin p) {
    plugin = p;
}

Plugin.java

public class Plugin {

    private static final long serialVersionUID = -8424575107726996696L;

    @NotNull
    private Long id;

    @NotNull
    private String name;

    @NotNull
    private String networkClasses;

    + setters and getters
}

I want to pay your attention this code interacts with JSF page:

<a:support event="onRowClick" action="#{configPlugins.selectPlugin(p)}" reRender="pluginList"/>
<a:commandLink action="#{configPlugins.removePlugin(true)}" reRender="pluginList"/>

Early Return

You can return early to reduce the nesting of your if statements:

if (plugins.isEmpty()) {
    plugin = null;
    return;
}

Also note that I changed plugins.size() > 0 to !plugins.isEmpty(), I think it's more readable.

Plugins Field

Code like this:

plugins = pluginManager.getPlugins(); // assign field

plugins = filterPlugins(searchParam); // method expects plugins field to be assigned,
                                      // but doesn't work on it but returns instead

Can be a bit confusing. I would change it to:

plugins = filterPlugins(pluginManager.getPlugins(), searchParam);

And then change filterPlugins to:

private List<Plugin> filterPlugins(final List<Plugin> plugins, final String searchParam )

plugin field

I would still like to see the declaration of plugin as well as the selectPlugin method. As mentioned in the comments, it looks a bit like it might be bad design.