Sum of Subset of an Array equals a given number The exact question that I am solving is given here:  Codechef - MARCHA1

Problem Statement

Basically, we have an array of n integers, say {1, 5, 6, 3, 12} where n = 5.

Then we have a given number m = 10, and we have to check if the sum of any subset of the array is equal to m or not.

In this case, we have 1+6+3 = 10, so we print Yes.

Proposed Solution

I proposed the following solution:

We create another array of n integers which only hold either 0 or 1. For example, in the case with n = 5, we can have this new array as {0, 0, 0, 0, 1}, {0, 0, 0, 1, 0} .. etc

In this manner, each time we check for the sum of numbers of original array which are at position 1. So there are 2^n such cases.

After checking, if none of them match, we print No, else we print Yes.

Here's the code in C:

#include <stdio.h>

int main()
{
    unsigned int n, m;
    scanf("%u%u", &n, &m);

    unsigned int note[n];
    unsigned long j;

    for (j = 0; j < n; ++j) {
        scanf("%u", note+j);
    }

    char teller[n], flag = 0;

    for (j = 0; j < n; ++j) {
        teller[j] = 0;
    }

    unsigned long two_pow_n = 1 << n;

    for (j = 0; j < two_pow_n; ++j)
    {
        unsigned int sum0 = 0, sum1 = 0, k;

        for (k = 0; k < n; ++k)
        {
            if (teller[k] == 0) {
                sum0 += note[k];
            }
            else {
                sum1 += note[k];
            }
            if (j % (1 << k) == 0) {
                teller[k] = teller[k] == 0 ? 1 : 0;// swap 0 and 1
            }
        }

        if (sum0 == m || sum1 == m)
        {
            flag = 1;
            break;
        }
    }

    flag ? printf("Yes\n") : printf("No\n");
    return 0;
}

The problem with this approach is that it took too much time and too many comparisons. On the codechef website, I found my solution was among the slowest.

Please help me by suggesting a better algorithm, I couldn't think of any optimisation either. Thank you!

Let's start with a subtle bug in your code:

unsigned long two_pow_n = 1 << n;

According to the problem description, n is at most 20, so using
unsigned long is good, an int is only guaranteed to have
16 bits according to the C standard. But 1 is an int and if n
exceeds the number of bits in an int then the behaviour of 1 << n
is undefined. So

Either you know that int has (at least) 32 bit on your platform. Then
you don't need to use long at all.
Or you don't want to make that assumption: Then it must be

unsigned long two_pow_n = 1UL << n;

using an unsigned long int literal.

Now some remarks to your current implementation. First, I would
put the computation into its own function and separate it
from the I/O:

bool hasSubsetSum(unsigned numbers[], unsigned n, unsigned targetSum) {
    // ... return true or false
}

That makes the program structure more organized, and allows to add
test cases easily. I have used bool from <stdbool.h>. If that
is not available with your compiler, replace it by int.

Next, your code to update the teller array looks a bit obfuscated
to me. Actually you don't need that array at all: You already
test all bit positions of j in

if (j % (1 << k) == 0) { ...

so you can use the same test to update either sum1 or sum0.

You compute both a sum and a complementary sum at once, which is a
good idea to cut the (maximal) number of iterations into half. But the loop

for (j = 0; j < two_pow_n; ++j)

does not reflect that, you only need to iterate over 2^(n-1) combinations.

Finally, a variable should be declared at the narrowest possible scope.
In particular, loop variables should be declared in the loop statement:

for (unsigned int j = 0; j < two_pow_n; ++j)

Putting it all together, you code could look like this:

bool hasSubsetSum(unsigned numbers[], unsigned n, unsigned targetSum) {

    unsigned long two_pow_n1 = 1UL << (n - 1);

    for (unsigned long j = 0; j < two_pow_n1; ++j) {
        unsigned sum0 = 0, sum1 = 0;
        for (unsigned k = 0; k < n; ++k) {
            if ((j & (1UL << k)) == 0) {
                sum0 += numbers[k];
                if (sum0 == targetSum) {
                    return true;
                }
            } else {
                sum1 += numbers[k];
                if (sum1 == targetSum) {
                    return true;
                }
            }
        }
    }

    return false;
}

Alternatively you could compute the totalSum of all numbers in advance
and then compare both sum1 and totalSum - sum1 with the target sum.

This should be a bit faster than your original approach, but
it is still the "brute-force" algorithm to solve the
"subset sum problem"
and has complexity O(2^n).

For positive integers (as in your case) there is a better algorithm
using "dynamic programming".
The rough idea is to incrementally compute a list of all sums which
are achievable with the given numbers, and then check if the target
sum is in that list.

One possible implementation is to use a boolean array hasSum with indices from
0 up to targetSum. Initially, only hasSum[0] is true.
Then for each number x in the given array and all
possible indices j in hasSum,
hasSum[j + x] is set to true if hasSum[j] == true.

Here is a sample implementation of that algorithm:

bool hasSubsetSum(unsigned numbers[], unsigned n, unsigned targetSum) {

    bool hasSum[targetSum + 1];
    for (unsigned i = 0; i <= targetSum; ++i) {
        hasSum[i] = false;
    }
    hasSum[0] = true;

    unsigned maxIndex = 0; // Highest index in `hasSum` which is `true`

    // For all numbers in the given array ...
    for (unsigned k = 0; k <= n; ++k) {

        // For all `j` for which `hasSum[j] == true` ...
        for (unsigned j = maxIndex; j != (unsigned)(-1); --j) {
            if (hasSum[j]) {

                unsigned sum = j + numbers[k]; // New achievable sum
                if (sum == targetSum) {
                    return true;
                } else if (sum < targetSum) {
                    hasSum[sum] = true;
                    if (sum > maxIndex) {
                        maxIndex = sum;
                    }
                }
            }
        }

    }
    return false;
}

The inner loop is traversed in decreasing order, so that updating the boolean array
does not influence the computation for the current number[k].


Have I prepared this prepared statement well? This is a prepared statement (I think).  Have I done this well?

function wyswietl($rekord, $tabela) {
    $lacz = lacz_bd();

    if (!$zap_preparowane = $lacz->prepare("SELECT {$rekord} FROM {$tabela} where email = ?"))                 {echo "Prepare nieudane: (" . $zap_preparowane->errno . ") " . $zap_preparowane->error;}
    if (!$zap_preparowane->bind_param("s",$_SESSION['prawid_uzyt']))                                           {echo "BindPar nieudane: (" . $zap_preparowane->errno . ") " . $zap_preparowane->error;}
    if (!$zap_preparowane->execute())                                                                          {echo "Execute nieudane: (" . $zap_preparowane->errno . ") " . $zap_preparowane->error;}
    if (!$zap_preparowane->store_result())                                                                     {echo "StoreRe nieudane: (" . $zap_preparowane->errno . ") " . $zap_preparowane->error;}
    if (!$zap_preparowane->bind_result($binded_result))                                                        {echo "BindRes nieudane: (" . $zap_preparowane->errno . ") " . $zap_preparowane->error;}
        while ($zap_preparowane->fetch()) {
             echo $binded_result;     //htmlspecialchars? printf("%s ?
        }
    $zap_preparowane->free_result();
    $zap_preparowane->close();

    $lacz->close();
}

Single line if statements combined with column aligned spacing:

Never ever do this. Never. A quick glance at the code you posted looks like this:

function wyswietl($rekord, $tabela) {
    $lacz = lacz_bd();

    if (!$zap_preparowane = $lacz->prepare("SELECT {$rekord} FROM {$tabela} where email = ?"))
    if (!$zap_preparowane->bind_param("s",$_SESSION['prawid_uzyt']))
    if (!$zap_preparowane->execute())
    if (!$zap_preparowane->store_result())
    if (!$zap_preparowane->bind_result($binded_result))
        while ($zap_preparowane->fetch()) {
             echo $binded_result;     //htmlspecialchars? printf("%s ?
        }
    $zap_preparowane->free_result();
    $zap_preparowane->close();

    $lacz->close();
}

My first thought was "why are all these if statements chained together with no indentation?". Then I noticed the horizontal scrollbar. Now, I see that the code does something much different. You should not make someone reading your code have to work this hard to know what the code is doing.

On top of that, I don't like the use of single line if statements and column aligned spacing, in general. Single line if statements lead to people missing the executed line because a line for the if clause and a line for the true clause is so much more prevalent. Column aligned spacing is easy enough to write the first time. But when you have to come back and edit the code, you might have to make changes to lines just to maintain the spacing.

Repeated code:

The code that prints the error message is repeated code. You can easily make it a function.

function log_error($zap_preparowane, $header) {
  echo $header ." nieudane: (" . $zap_preparowane->errno . ") " . $zap_preparowane->error;
}

Now if you want to change how the errors are handled, it is easily done in one place.

The error handling doesn't stop execution of the code. If you fail to properly prepare the SQL statement, it is unlikely that anything else will execute correctly.


Optimizing code block using Switch Case I have a implementation of following code block,

public class FileFormatConversion {

public static enum File_TYPES {

  PDF, PNG, OTHERS, ALL
}

public static void processOtherFileType(String arg1) {
    System.out.println("Other file output -- " + arg1 + " file");
}

public static void processPNGPDFALL(String arg1, String arg2) {
    System.out
            .println("General block for processing PDF/PNG or ALL other file format -- "
                    + arg2
                    + "\\"
                    + arg1
                    + " parameter validation/check file availability");
}

public static void processPDF(String arg1, String arg2) {
    System.out.println("Block for processing PDF file -- " + arg2 + "\\"
            + arg1 + ".pdf");
}

public static void processALL(String arg1, String arg2) {
    System.out.println("Block for processing ALL file types -- " + arg2
            + "\\" + arg1 + ".all");
}

public static void processPNGALL(String arg1, String arg2) {
    System.out
            .println("Common block for manipulating file formats such as PNG or ALL files");
}

/**
 * @param args
 */
public static void main(String[] args) {

    args = new String[3];

    args[0] = "ALL";
    args[1] = "fileName";
    args[2] = "C:\\filePath";

    File_TYPES fileFormatInput = File_TYPES.valueOf(args[0]);

    if (File_TYPES.OTHERS == fileFormatInput) {
        processOtherFileType(args[1]);
    } else {
        processPNGPDFALL(args[1], args[2]);
        if (File_TYPES.ALL == fileFormatInput
                || File_TYPES.PNG == fileFormatInput)
            processPNGALL(args[1], args[2]);
        if (File_TYPES.ALL == fileFormatInput)
            processALL(args[1], args[2]);
        if (File_TYPES.PDF == fileFormatInput)
            processPDF(args[1], args[2]);
    }

    System.out.println("Completed");
}

I might need to add more options in ENUM in future. so using if-else will make it complex while debugging/adding. Is there any better alternative way to improve this block - improve understandability/maintenance.

To avoid the messy switch block, you should probably create an interface or abstract class called FileConversionHandler:

public abstract class FileConversionHandler {
    public abstract void process(File f);
}

Then, for each type of file, make a subclass that implements process() differently.


Pairwise summation in Haskell I'm new to Haskell, and I figured that pairwise summation is something I could do easily. I wrote this:
pairwiseSum :: (Num a) => [a] -> a
pairwiseSum [] = 0
pairwiseSum [x] = x
pairwiseSum (x:xs) = (pairwiseSum (everyOther (x:xs))) + (pairwiseSum (everyOther xs))

everyOther :: [a] -> [a]
everyOther [] = []
everyOther [x] = [x]
everyOther (x:_:xs) = x : everyOther xs

Given [0,1,2,3,4,5,6,7], this computes (((0+4)+(2+6))+((1+5)+(3+7))), whereas my C(++) code computes (((0+1)+(2+3))+((4+5)+(6+7))). How does this affect optimization?
How easy is it to read? Could it be written better?
How can I test it? I have three tests in C++:

add the odd numbers [1,3..(2*n-1) and check that the sum is n²;
add 100000 copies of 1, 100000 copies of 0.001, ... 100000 copies of 10^-18 and check for the correct sum;
add 0x36000 terms of a geometric progression with ratio exp(1/4096).

To test execution time, I use 1000000 copies and 16 times as many terms of exp(1/65536).

Your code doesn’t have great complexity. I think it’s \$\mathcal{O}(2^n)\$, but I’m rusty so maybe somebody can come in with an assist. The thing to remember is that Haskell lists are linked lists and laziness doesn’t get you out of paying the cost of walking them. Each call to everyOther is \$\mathcal{O}(n)\$, and you pay that twice with every recursive call.
There are a bunch of ways we could solve this. If you actually want to process lists like a binary tree for some reason, then I’d recommend just turning the list into a binary tree and processing that if you’re favoring clarity. If you do that you could get the compiler to fuse away the intermediate structure, but it might not happen automatically without some pragmas.
You could also decompose your problem into smaller, easier to solve bits, then reassemble them into your ultimate solution.
-- Do a single pass through a list, summing elements pairwise
sumPairs :: Num a => [a] -> [a]
sumPairs [] = [0]
sumPairs [n] = [n]
sumPairs (m:n:xs) = m + n : sumPairs xs

-- Identify when there's only a single element left in the list
isLengthOne :: [a] -> Bool
isLengthOne [_] = True
isLengthOne _ = False

-- Repeatedly apply sumPairs until you find a one element list and return that value
pairwiseSum :: Num a => [a] -> a
pairwiseSum = head . find isLengthOne . iterate sumPairs

If you’re looking for testing and benchmarking libraries, check out QuickCheck for testing and criterion for benchmarking.
QuickCheck does what’s known as property testing, instead of constructing elaborate unit tests the library helps you to test random inputs to see if some property holds. Usually that’s things like testing that a function to insert an element into a collection increases that collections size by one, or that after a random sequence of inserts the collection contains all of the elements that were inserted. In this case you can use it to prove equivalence between two versions of functions that should have identical results. I.e.—
import Test.QuickCheck

prop_sumsEqual :: [Int] -> Bool
prop_sumsEqual xs = pairwiseSum xs == sum xs

main :: IO ()
main = quickCheck prop_sumsEqual

criterion handles re-running benchmarks for you and gives you all sorts of fancy statistical output. You might use it like—
import Criterion.Main

main :: IO ()
main = defaultMain [bench "pairwiseSum" $ nf pairwiseSum [1 .. 10000]]

You could bench sum by adding another list element there and compare the difference. Or you could take advantage of your benchmarks being values to generate a series, that might help you understand how your execution time grows with input size.
mkBench_pairwiseSum :: Int -> Benchmark
mkBench_pairwiseSum n = bench ("pairwiseSum:" ++ show n) $ nf pairwiseSum [1 .. n]

main :: IO ()
main = defaultMain $ map mkBench_pairwiseSum [10000, 20000 .. 100000]


Project Euler # 35 Circular primes in Python The number, 197, is called a circular prime because all rotations of the digits: 197, 971, and 719, are themselves prime.
There are thirteen such primes below 100: 2, 3, 5, 7, 11, 13, 17, 31, 37, 71, 73, 79, and 97.
How many circular primes are there below one million?

Here's my implementation in Python 3. Awaiting feedback.
from time import time

def is_prime(number):
    """returns True for a prime number, False otherwise."""
    factor = 2
    while factor * factor <= number:
        if number % factor == 0:
            return False
        factor += 1
    return True

def circulate(number):
    """returns all circulations of a number."""
    for n in str(number):
        if not int(n) % 2:
            return False
    circulations = []
    digits = list(str(number))
    for i in range(len(str(number))):
        last = digits.pop()
        circulations.append(last + ''.join(digits))
        digits = [last] + digits
    return [int(x) for x in circulations]

def circular_primes(limit):
    """returns all circular primes below limit."""
    all_circulations = [2]
    for num in range(3, limit, 2):
        if is_prime(num):
            check = 0
            if circulate(num):
                circulations = circulate(num)
                for circulation in circulations:
                    if not is_prime(circulation):
                        check += 1
                if not check:
                    all_circulations.extend(circulations)
    return all_circulations

if __name__ == '__main__':
    start_time = time()
    print(len(set(circular_primes(200000))))
    print(f'Time: {time() - start_time} seconds.')

Let's start with a quick analysis of your current algorithm. For each number less than n (1000000 in your case) it does sqrt(n)log(n) operations (sqrt for is_prime*logcycles). However, some fairly simple things about this problem make it much easier to solve.

As you probably know, any 2 digit or greater number ending in 2,4,6,8 or 5 is not prime since it is divisible by 2 or 5. Thus when constructing circular primes, you don't even have to check numbers that have these digits in them (and add 2 to the result for 2,5)

Thus by changing the main bit of code to only generate reasonable candidates, you do significantly less work.

def digit_combinations(digit_list, n):
    if n == 1:
        for digit in digit_list:
            yield digit
        return
    for digit in digit_list:
        for part in digit_combinations(digit_list, n-1):
            yield digit + 10*part

def circular_primes(digits):
    if digits == 1:
        return 4 # 2,3,5,7

    all_circulations = [2,3,5,7]
    for i in range(2, digits):
        for num in digit_combinations((1,3,7,9), i):
            if is_prime(num):
                check = 0
                if circulate(num):
                    circulations = circulate(num)
                    for circulation in circulations:
                        if not is_prime(circulation):
                            check += 1
                    if not check:
                        all_circulations.extend(circulations)
    return all_circulations

By my timings, this code can return the 8 digit or less ones in 3 seconds, where yours took 7 seconds to do only up to 1 million.


What's the most idiomatic way to call a function an arbitrary number of times with the previous result? Basically I have a function of the type 'a -> 'a (an optimization function on a AST) and I want to call it (passing the previous result) until it returns the same thing as the input.

Right now I have this:

let performOptimizations ast =
  Seq.initInfinite (fun _ -> 0)
  |> Seq.scan (fun last _ -> optimizeAst last) ast
  |> Seq.pairwise
  |> Seq.find (fun (first, second) -> first = second)
  |> fst

This seems really convoluted because it is created an infinite sequence of ints just so that the scan will continue until the ast is no longer modified. I realized that I could also do something like this:

let performOptimizations ast =
  let mutable newAst = optimizeAst ast
  let mutable lastAst = ast
  while newAst <> lastAst do
    lastAst <- newAst
    newAst <- optimizeAst newAst

  newAst

However, I would prefer to avoid mutation.

What's the functional, idiomatic way of doing this in F#?

I general, I agree with what mavnn said in his answer, I just think the function can be written in a simpler way:

let rec performOptimizations ast =
    let optimized = optimizeAst ast
    if ast = optimized then
        ast
    else
        performOptimizations optimized

This method also sounds like a good candidate for making it more generic, something like:

let rec doWhileNotSame seed getNextValue =
    let nextValue = getNextValue seed
    if seed = nextValue then
        seed
    else
        doWhileNotSame nextValue getNextValue


Project Euler #4 Largest Palindrome Product I'm kinda new to Java (<1 year) but have some background in C and Python.

I've come up with a working solution to Project Euler Problem 4 - Largest Palindrome Product. However, I'm not confident this is "Effective Java"...

All suggestions for improvement are welcome. I'd especially like feedback on the following:

Java language usage
Java library usage
Algorithm performance

/**
 * Palindrome Integer is a whole number which reads the same both ways.
 */
public class PalindromeInteger {

    /**
     * Predicate to determine whether the given number is a palindrome.
     * @param integer
     * @return true if palindrome, false otherwise.
     */
    public static boolean isPalindrome(int integer) {
        String integerStr = String.valueOf(integer);
        StringBuilder sb = new StringBuilder();
        sb.append(integerStr);
        String integerStrReversed = sb.reverse().toString();
        return integerStrReversed.equals(integerStr);
    }

    /**
     * Returns a Set of the palindrome integers which can be made from the product of the given integers.
     * @param multiplicand
     * @param multiplier
     * @return a Set of palindrome integers.
     */
    public static Set getPalindromeProducts(int multiplicand, int multiplier) {
        Set palindromes = new HashSet();
        for (int i = multiplicand; i >= 0; i--) {
            for (int j = multiplier; j >= 0; j--) {
                int product = i * j;
                if (isPalindrome(product))
                    palindromes.add(product);
            }
        }
        return palindromes;
    }

    /**
     * Returns the largest Palindrome integer possible from the product of the two given integers.
     * @param multiplicand
     * @param multipler
     * @return palindromic integer e.g. 9009
     */
    public static int getLargestPalindromeProduct(int multiplicand, int multipler) {
        Set palindromes = getPalindromeProducts(multiplicand,
                multipler);
        return (int)Collections.max(palindromes);
    }
}

Your method isPalindrome produces wrong results for negative numbers. (which is not important for Project Euler #4, but still)
It is also inefficient to convert all numbers to String. I would only do that when using BigInteger. With int, you can just use a little math:

public static boolean isPalindrome(int number) {
    return number == reverse(number);
}

public static int reverse(int number) {
     //negative numbers: reverse the positive and negate it again.
    if (number < 0) return -reverse(-number);
    //a 1-digit number is its own reverse
    if (number < 10) return number;
    int reversed = 0;
    while (number > 0) {
        //take the last digit of number and paste it to reversed.
        reversed = reversed * 10 + number % 10;
        //cut off the last digit
        number /= 10;
    }
    return reversed;
}

You are not using the method getPalindromeProducts(int multiplicand, int multiplier) correctly. You run both loops all the way down to zero.
The meaning of the names multiplicant and multiplier isn't clear imo. In a multiplication, both numbers are just factors. One of them is the minimum (100) and the other is the maximum (999). You're looking for numbers between 100 and 999, not between 100 and 0 or between 999 and 0.
You also do a lot of the work twice by running both loops all the way. For example, you will get a * b AND b * a. You can skip a lot of iterations by breaking out of the loop early.

Another thing to think about is the HashSet. The problem with Java Collections is that you cannot use them with primitive types. It will convert all numbers to Object, which will hurt performance. (You cannot have a HashSet<int> for example)
However, in this case, you don't need a HashSet at all. You don't care about duplicates, you just want to find the largest product. It is also not necessary to store all the palindromes you found; you only need the largest one.

public static int getLargestPalindromeProduct(int minFactor, int maxFactor) {
    int largest = 0;
    for (int a = maxFactor; a >= minFactor; --a) {
        for (int b = maxFactor; b >= a; --b) {//Note the b >= a, not b >= minFactor
            int product = a * b;
            if (product <= largest) break; //We can stop this iteration, because factors will only get smaller.
            if (isPalindrome(product)) {
                largest = product;
                break;
            }
        }
    }
    return largest;
}

Then call getLargestPalindromeProduct(100, 999);


String to Array formatting I'm tying to convert from this formatted string:

$filtersStr = "id:1||name:alex";

to:

['id' => 1,'name' => 'alex']

My solution for now:

foreach(explode('||', $filtersStr) as $filter){
            list($k, $v) = explode(':', $filter);
            $filters[ $k ] = $v;
        }

I'm looking for a more elegant way.

You can use the amazing parse_str() function. It does everything you need. But you are using different delimiters for key => value pairs. If you change those to & and =, you can do like this:

$filtersStr = 'id=1&name=alex';

parse_str($filtersStr, $filters);

And $filters will have what you want. But that's not the case, so, one could do this:

parse_str(
    str_replace(
        array('%', '=', '&', '||', ':'),
        array('%26', '%3D', '%26', '&', '='),
        $filtersStr
    ),
    $filters
);

The characters %, = and & will have a special meaning. They have to be replaced with their URL-codes. parse_str() works on URL-encoded strings and leaving those characters unchanged would cause all sorts of bugs. The order of the elements in the array is VERY important.

Here's how it works:

Replace % by %26
It must be the first of you will be replacing % everywhere and break it
Replace = by %3D
This guarantees that no meaningful = will be replaced
Replace & by %26
Replace || by &
Replace : by =
At this point, your 'id:1||name:alex' will be 'id=1&name=alex'
Hand it over to parse_str()

And done! I prefer the first method instead of this kludge. Use this second one with extreme care!

In a comment, it was explained that it's usage was like this:

/arena?filter=id:1||content_name:alex&anotherParam=smth

If you can change the URL, throw this whole code away and do like this:

/arena?filter[id]=1&filter[content_name]=alex&anotherParam=smth

This will have the exact structure you wish. Simply use $_GET['filter'] to access all values. And done! An alternative with 0 lines of code!


Class responsibilities OOP design I'm having some questions on creating a design for simple approval workflow. Some arbitrary thing (ItemToApprove) needs approval process. Approval process begins with creating ApprovalRoute which contains several approval steps (ApprovalStep). ItemToApprove goes sequentially through all steps until gets status Approved. On each step there are several Approvers with equal priority. If one approver approves item, step becomes Approved.

public class ApprovalRoute
{
    private LinkedList<ApprovalStep> _approvalSteps;
    private ApprovalStep _currenStep;

    public bool? Approved { get; private set; }

    public void Approve(Approver approver)
    {
        // ... some cheks if already approved etc
        _currentStep.Approve(approver);

        if (_currentStep.Approved == true)
        {
            MoveToNextStep(); // changes _currentStep
        }

        if (AllStepsApproved()) {
           Approved = true;
        }
    }
}

public class ApprovalStep
{
    public bool? Approved { get; private set; }
    private List<Approver> _approvers;

    public ApprovalStep(List<Approver> approvers)
    {
        _approvers = approvers;
    }

    public void Approve(Approver approver)
    {
        if (_approvers.Contains(approver))
        {
            Approved = true;
        }
    }
}

public class ItemToApprove
{
    public bool? Approved { get; private set; }
    private ApprovalRoute _approvalRoute;

    public void StartApprovalWorkflow(ApprovalRoute route)
    {
        _approvalRoute = route;
    }

    public void Approve(Approver approver)
    {
        _approvalRoute.Approve(approver);
        if (_approvalRoute.Approved == true)
        {
            Approved = true;
        }
    }
}

public class Approver
{
    public string Name { get; private set; }
    public Approver(string name)
    {
        Name = name;
    }

    // ... ovveriding Equals etc
}

So some console client would be like this

      var itemToApprove = new ItemToApprove();
      var aprovalRoute = new ApprovalRoute(new ApprovalStep(new List<Approver>
      {
          new Approver("One"),
          new Approver("Neo"),
      });
      itemToApprove.StartApprovalWorkflow(aprovalRoute);

      while (itemToApprove.Approved == null)
      {
          Console.Write("Your name: ");

          string approverName = Console.ReadLine();
          itemToApprove.Approve(new Approver(approverName));

      }

I thik this design has some smell because calling Approve delegates further down to ApprovalStep. The second smell I feel more naturally call Approve on Approver passing ItemToApprove

// ....
var approver = new Approver();
approver.Approve(itemToApprove);

What do you think?

Random thoughts

In this scenario, there's few implementation details making me ask "why"?

In ApprovalRoute what benefits LinkedList<T> gives over any other IEnumerable<T> ?
In ApprovalStep and ItemToApprove classes is it necessary to make Approved property    nullable ?
Won't it, at least, hurt coder's eyes if bool type is compared using equality operator == instead of someBool or !someBool ?

And to anwer myself right-away

I think it could be e.g. Queue<T> since

  ItemToApprove goes sequentially through all steps until gets status
  Approved

No, because default value will always be false, it is never checked if property  HasValue and there's no separate logic for nulls.
Yes, it hurts the eyes and makes you say nonconstructive things like "WTF dude". Just don't do it.

On the higher level then

  I think this design has some smell because calling Approve delegates further down to
  ApprovalStep.
  itemToApprove.Approve(new Approver(approverName));

You are right. Now it feels like the ItemToApprove is actually approving something. This method shouldn't exist.

  The second smell I feel more naturally call Approve on Approver passing ItemToApprove
  approver.Approve(itemToApprove);

Again you are right. But in this case it would seem than approver is approving the item itself, not a step along the route.

Suggestions

I'd remove the Approve method from ItemToApprove and ApproveStep classes and make it ApprovalRoute's responsibility to dequeue ApprovalSteps as they are approved. Dequeued step is the current step and when queue is empty, item is fully approved.

Better yet I'd pass the ItemToApprove along the ApprovalRoute, not the other way around. You could also have multiple items going down the same route at the same time, all having their own "states".

Something like below.

Related dotnet Fiddle can be found here https://dotnetfiddle.net/6BlA30 (web page "console" seems little buggy though).

public class Program
{
    // console app for testing
    public static void Main()
    {
        // some items
        var item1 = new ItemToApprove("item1");
        var item2 = new ItemToApprove("item2");

        // approvers
        var approver1 = new Approver("app1");
        var approver2 = new Approver("app2");

        // steps w/ authorized approvers
        var step1 = new ApprovalStep("step1", approver1);
        var step2 = new ApprovalStep("step2", approver1, approver2);

        // create route
        var route = new ApprovalRoute("route1", new[] { step1, step2 });
        // add items to current route
        route.AddItems(item1, item2);

        // start manual testing
        Console.WriteLine("** {0} has {1} items **", route.Name, route.Items.Count());
        while (route.Items.Any())
        {
            Console.Write("> ");
            string input = Console.ReadLine() ?? string.Empty;
            var parts = input.Split(' ');

            if (parts.Length == 2)
                route.ApproveStep(new ItemToApprove(parts[0]), new Approver(parts[1]));
            else if (input.Equals("?"))
                route.Status();
        }
        Console.WriteLine("No more items in route");
        Console.ReadLine();
    }
}

Implementation

For examples sake all classes are simplistic and inherit abstract ModelBase.

public abstract class ModelBase : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;

    public string Name { get; set; }

    [NotifyPropertyChangedInvocator]
    protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null)
    {
        var handler = PropertyChanged;
        if (handler != null)
            handler(this, new PropertyChangedEventArgs(propertyName));
    }

    public override bool Equals(object obj)
    {
        if (obj == null || GetType() != obj.GetType())
            return false;

        return (Name == ((ModelBase)obj).Name);
    }

    public override int GetHashCode()
    {
        return Name.GetHashCode();
    }
}

Actual "business objects" below

public class ItemToApprove : ModelBase
{
    private bool _isApproved;

    public ItemToApprove(string name)
    {
        Name = name;
    }

    public bool IsApproved
    {
        get { return _isApproved; }
        set { _isApproved = value; OnPropertyChanged(); }
    }
}

public class Approver : ModelBase
{
    public Approver(string name)
    {
        Name = name;
    }
}

public class ApprovalStep : ModelBase
{
    public ApprovalStep(string name, params Approver[] approvers)
    {
        Name = name;
        Approvers = approvers;
    }

    public IEnumerable<Approver> Approvers { get; private set; }
}

public class ApprovalRoute : ModelBase
{
    // sequential steps in this route
    private readonly List<ApprovalStep> _steps;
    // items and step indexes
    private readonly Dictionary<ItemToApprove,int> _itemsInRoute =
        new Dictionary<ItemToApprove,int>();

    public ApprovalRoute(string name, params ApprovalStep[] steps)
    {
        Name = name;
        _steps = new List<ApprovalStep>(steps);
    }

    public void AddItems(params ItemToApprove[] items)
    {
        foreach (var item in items)
        {
            item.PropertyChanged += (sender, args) =>
                {
                    if (item.IsApproved)
                        Console.WriteLine("Item '{0}' fully approved", item.Name);
                };
            _itemsInRoute.Add(item, 0);
        }
    }

    public IEnumerable<ItemToApprove> Items { get { return _itemsInRoute.Keys; } }

    public void ApproveStep(ItemToApprove item, Approver approver)
    {
        if (!_itemsInRoute.ContainsKey(item))
        {
            Console.WriteLine("There is no '{0}' on '{1}'", item.Name, Name);
            return;
        }

        int index = _itemsInRoute[item];
        var step = _steps[index];

        if (step.Approvers.Contains(approver))
        {
            Console.WriteLine("{0} for {1} approved by {2}",
                step.Name, item.Name, approver.Name);

            index++;
            if (index > _steps.Count - 1)
            {
                _itemsInRoute.Single(i => i.Key.Equals(item)).Key.IsApproved = true;
                _itemsInRoute.Remove(item);
            }
            else
            {
                _itemsInRoute[item] = index;
            }
        }
        else
        {
            Console.WriteLine("{0} for {1} can only be approved by {2}",
                step.Name, item.Name, string.Join(",", step.Approvers.Select(a => a.Name)));
        }
    }

    public void Status()
    {
        foreach (var entry in _itemsInRoute)
            Console.WriteLine("{0} at {1}", entry.Key.Name, _steps[entry.Value].Name);
    }
}

Hope this made any sense.


Depth First Search & Breadth First Search implementation I've implemented DFS and BFS implementations.  I want to check if the code is readable, contains any issues, and can be improved.

GraphImplementation

package graphs;

import java.util.*;
import graphs.State;
public class GraphImplementation
{
    public void dfs(Node root)
    {
        //Avoid infinite loops
        if(root == null) return;

        System.out.print(root.getVertex() + "\t");
        root.state = State.Visited;

        //for every child
        for(Node n: root.getChild())
        {
            //if childs state is not visited then recurse
            if(n.state == State.Unvisited)
            {
                dfs(n);
            }
        }
    }

    public void bfs(Node root)
    {
        //Since queue is a interface
        Queue<Node> queue = new LinkedList<Node>();

        if(root == null) return;

        root.state = State.Visited;
         //Adds to end of queue
        queue.add(root);

        while(!queue.isEmpty())
        {
            //removes from front of queue
            Node r = queue.remove();
            System.out.print(r.getVertex() + "\t");

            //Visit child first before grandchild
            for(Node n: r.getChild())
            {
                if(n.state == State.Unvisited)
                {
                    queue.add(n);
                    n.state = State.Visited;
                }
            }
        }

    }

    public static Graph createNewGraph()
    {
        Graph g = new Graph();
        Node[] temp = new Node[8];

        temp[0] = new Node("A", 3);
        temp[1] = new Node("B", 3);
        temp[2] = new Node("C", 1);
        temp[3] = new Node("D", 1);
        temp[4] = new Node("E", 1);
        temp[5] = new Node("F", 1);

        temp[0].addChildNode(temp[1]);
        temp[0].addChildNode(temp[2]);
        temp[0].addChildNode(temp[3]);

        temp[1].addChildNode(temp[0]);
        temp[1].addChildNode(temp[4]);
        temp[1].addChildNode(temp[5]);

        temp[2].addChildNode(temp[0]);
        temp[3].addChildNode(temp[0]);
        temp[4].addChildNode(temp[1]);
        temp[5].addChildNode(temp[1]);

        for (int i = 0; i < 7; i++)
        {
            g.addNode(temp[i]);
        }
        return g;
    }

    public static void main(String[] args) {

        Graph gDfs = createNewGraph();
        GraphImplementation s = new GraphImplementation();

        System.out.println("--------------DFS---------------");
        s.dfs(gDfs.getNode()[0]);
        System.out.println();
        System.out.println();
        Graph gBfs = createNewGraph();
        System.out.println("---------------BFS---------------");
        s.bfs(gBfs.getNode()[0]);

    }

}

Graph.java:

package graphs;

public class Graph {

    public int count; // num of vertices
    private Node vertices[];

    public Graph()
    {
        vertices = new Node[8];
        count = 0;
    }

    public void addNode(Node n)
    {
        if(count < 10)
        {
            vertices[count] = n;
            count++;
        }
        else
        {
            System.out.println("graph full");
        }
    }

    public Node[] getNode()
    {
        return vertices;
    }
}

Node.java:

package graphs;
import graphs.State;
public class Node {

    public Node[] child;
    public int childCount;
    private String vertex;
    public State state;

    public Node(String vertex)
    {
        this.vertex = vertex;
    }

    public Node(String vertex, int childlen)
    {
        this.vertex = vertex;
        childCount = 0;
        child = new Node[childlen];
    }

    public void addChildNode(Node adj)
    {
        adj.state = State.Unvisited;
        if(childCount < 30)
        {
            this.child[childCount] = adj;
            childCount++;
        }
    }

    public Node[] getChild()
    {
        return child;
    }

    public String getVertex()
    {
        return vertex;
    }

}

State.java:

package graphs;

public enum State {

    Unvisited,Visiting,Visited;

}

Data Structure

Your terminology is a bit off.  Trees have roots and children.  Arbitrary graphs, on the other hand… I think "origin" and "neighbors" would be more appropriate.

Visited flag: Storing the visited/unvisited flag in the Node hurts flexibility.  Once you perform a dfs() or bfs(), that graph is "ruined".  You won't be able to reset all nodes to the unvisited state.  (Well, you could do it manually, since Node.state is a public field, after all.  But that's nasty too.)  Instead, I suggest that dfs() and bfs() keep a HashSet of visited nodes.  Once the traversal is done, just discard the set.
State.Visiting: That value is never used.
Node.getNode(): The name suggests that it would return a single node, but it doesn't.  Also, by returning the entire array, and the original of the array rather than a copy, you would be letting the caller alter the graph's connections in unapproved ways.  Better to offer ways to iterate through all neighbours and to fetch a specific neighbour.
Child vertex array: The Node constructor says: vertices = new Node[8];  However, addNode() checks if (count < 10).  You should test against vertices.length instead.

If the capacity is exceeded, you should't print to System.out as a side-effect.  Throw an exception instead, so that the caller can decide how to handle it.

Better yet, use an expandable data structure so that you don't have to deal with capacity limits.  A simple substitution would be to use an ArrayList<Node>, but read on…
Graph.vertices vs. Node.child: Those arrays seem to serve the same purpose, redundantly.
Graph.createNewGraph(): It's cumbersome.  Wouldn't it be nice to be able to write

Graph g = new Graph();
g.addEdge("A", "B");
g.addEdge("B", "C");
…
return g;

My suggestion:

public class Graph {
    // Alternatively, use a Multimap:
    // http://google-collections.googlecode.com/svn/trunk/javadoc/com/google/common/collect/Multimap.html
    private Map<String, List<String>> edges = new HashMap<String, List<String>>();

    public void addEdge(String src, String dest) {
        List<String> srcNeighbors = this.edges.get(src);
        if (srcNeighbors == null) {
            this.edges.put(src,
                srcNeighbors = new ArrayList<String>()
            );
        }
        srcNeighbors.add(dest);
    }

    public Iterable<String> getNeighbors(String vertex) {
        List<String> neighbors = this.edges.get(vertex);
        if (neighbors == null) {
            return Collections.emptyList();
        } else {
            return Collections.unmodifiableList(neighbors);
        }
    }
}

Traversal

Your dfs() and bfs() methods can only ever print the node names.  You can't reuse the code for anything else, since the System.out.print() calls are mingled with the graph traversal code.  It would be better to implement an Iterator so that the caller can decide what to do with each node.

Also, DFS and BFS are two different strategies for accomplishing a similar task.  Therefore, they should be implemented in two classes with a shared interface.  I suggest an Iterator<String>.

The breadth-first iterator is a pretty straightforward translation of your original code, with the main difference being that the iterator is now responsible for keeping track of which vertices have been visited.

public class BreadthFirstIterator implements Iterator<String> {
    private Set<String> visited = new HashSet<String>();
    private Queue<String> queue = new LinkedList<String>();
    private Graph graph;

    public BreadthFirstIterator(Graph g, String startingVertex) {
        this.graph = g;
        this.queue.add(startingVertex);
        this.visited.add(startingVertex);
    }

    @Override
    public void remove() {
        throw new UnsupportedOperationException();
    }

    @Override
    public boolean hasNext() {
        return !this.queue.isEmpty();
    }

    @Override
    public String next() {
        //removes from front of queue
        String next = queue.remove();
        for (String neighbor : this.graph.getNeighbors(next)) {
            if (!this.visited.contains(neighbor)) {
                this.queue.add(neighbor);
                this.visited.add(neighbor);
            }
        }
        return next;
    }
}

Unfortunately, you'll find that you can no longer use recursion for depth-first traversal.  Instead, you'll have to rewrite it as an iterative solution using an explicit stack, which makes the code more complicated.  (Alternatively, if you abandon the idea of making an Iterator and use the visitor pattern instead, then you could keep the same recursive code structure).

public class PreOrderDFSIterator implements Iterator<String> {
    private Set<String> visited = new HashSet<String>();
    private Deque<Iterator<String>> stack = new LinkedList<Iterator<String>>();
    private Graph graph;
    private String next;

    public PreOrderDFSIterator(Graph g, String startingVertex) {
        this.stack.push(g.getNeighbors(startingVertex).iterator());
        this.graph = g;
        this.next = startingVertex;
    }

    @Override
    public void remove() {
        throw new UnsupportedOperationException();
    }

    @Override
    public boolean hasNext() {
        return this.next != null;
    }

    @Override
    public String next() {
        if (this.next == null) {
            throw new NoSuchElementException();
        }
        try {
            this.visited.add(this.next);
            return this.next;
        } finally {
            this.advance();
        }
    }

    private void advance() {
        Iterator<String> neighbors = this.stack.peek();
        do {
            while (!neighbors.hasNext()) {  // No more nodes -> back out a level
                this.stack.pop();
                if (this.stack.isEmpty()) { // All done!
                    this.next = null;
                    return;
                }
                neighbors = this.stack.peek();
            }

            this.next = neighbors.next();
        } while (this.visited.contains(this.next));
        this.stack.push(this.graph.getNeighbors(this.next).iterator());
    }
}

Tests

This problem deserves better testing.  With the original code, which always printed its output to System.out, there was no good way to write unit tests.  Now, you can do anything you want with the results, so it is possible to write proper unit tests.

import static org.junit.Assert.*;

import org.junit.BeforeClass;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

import java.util.*;

// javac -cp .:junit.jar GraphTest.java
// java -cp .:junit.jar:hamcrest-core.jar org.junit.runner.JUnitCore GraphTest

@RunWith(JUnit4.class)
public class GraphTest {

    public static Graph graph1;

    @BeforeClass
    public static void makeGraphs() {
        Graph g = graph1 = new Graph();
        g.addEdge("A", "B");
        g.addEdge("B", "C");
        g.addEdge("B", "D");
        g.addEdge("B", "A");
        g.addEdge("B", "E");
        g.addEdge("B", "F");
        g.addEdge("C", "A");
        g.addEdge("D", "C");
        g.addEdge("E", "B");
        g.addEdge("F", "B");
    }

    private void expectIteration(String answer, Iterator<String> it) {
        StringBuilder sb = new StringBuilder();
        while (it.hasNext()) {
            sb.append(' ').append(it.next());
        }
        assertEquals(answer, sb.substring(1));
    }

    @Test
    public void preOrderIterationOfIsolatedVertex() {
        expectIteration("Z", new PreOrderDFSIterator(graph1, "Z"));
    }

    @Test
    public void preOrderIterationFromA() {
        expectIteration("A B C D E F", new PreOrderDFSIterator(graph1, "A"));
    }

    @Test
    public void preOrderIterationFromB() {
        expectIteration("B C A D E F", new PreOrderDFSIterator(graph1, "B"));
    }

    @Test
    public void BreadthFirstIterationOfIsolatedVertex() {
        expectIteration("Z", new BreadthFirstIterator(graph1, "Z"));
    }

    @Test
    public void BreadthFirstIterationFromA() {
        expectIteration("A B C D E F", new BreadthFirstIterator(graph1, "A"));
    }

    @Test
    public void BreadthFirstIterationFromB() {
        expectIteration("B C D A E F", new BreadthFirstIterator(graph1, "B"));
    }
}


Leeetcode - Best time to buy and sell stock to get maximum profit Question

  Say you have an array for which the ith element is the price of a
  given stock on day i.

  If you were only permitted to complete at most one transaction (i.e.,
  buy one and sell one share of the stock), design an algorithm to find
  the maximum profit.

  Note that you cannot sell a stock before you buy one.

Example 1:

Input: [7,1,5,3,6,4]
Output: 5
Explanation: Buy on day 2 (price = 1)
             and sell on day 5 (price = 6), profit = 6-1 = 5.
             Not 7-1 = 6, as selling price needs to be larger than
             buying price.

Example 2:

Input: [7,6,4,3,1]
Output: 0
Explanation: In this case, no transaction is done, i.e. max profit = 0.

My Approach is brute force.(Naive approach)
Take each element and iterate through right to see how much profit can be made.

public int maxProfit(int[] prices) {
        int size = prices.length;

        if(size == 0 || size == 1) {
            return 0;
        }

        int maxProfit = 0;
        // Iterate through each right element.
        for(int i =0; i<size; i++) {
            for(int j = i+1; j<size; j++) {
                if(prices[j] > prices[i]) {
                    int diff = prices[j] - prices[i];
                    if(diff > maxProfit) {
                        maxProfit = diff;
                    }
                }
            }
        }

        return maxProfit;
    }

How can I improve this from O(n2) . Also, what if more than 1 transactions are allowed to get the profit.

Your use of whitespace is inconsistent, which makes the code harder to read and understand. There should be whitespace between control flow statements and open parentheses, and there should be whitespace on both sides of operators.

Your comment is noise and should be deleted.

Unless leetcode promises you'll never get a null input, you should check to make sure the prices array is not null.

It's cleaner to check if the array length is < 2 rather than enumerating the cases. Also, your algorithm works correctly without the check, since the loop falls through correctly.

size should be marked as final since it does not change. There's also no real value in storing this in a variable, since it's obvious what prices.length is, and it's not a computed value.

You can use Math.max() instead of doing subtraction and int comparison yourself. It makes the code easier to read.

You can save a comparison in some cases by always doing the subtraction. It's easier to read, and in some cases will be faster.

If you were to make all these changes, your code might look more like:

public int maxProfit(final int[] prices) {
    if (prices == null) {
        return 0;
    }

    int maxProfit = 0;

    for (int i = 0; i < prices.length; i++) {
        for (int j = i + 1; j < prices.length; j++) {
            maxProfit = Math.max(maxProfit, prices[j] - prices[i]);
        }
    }

    return maxProfit;
}

As far as algorithmic performance, you can do this in O(n) time and O(1) space. Walk through the input array one time, tracking the minimum value seen so far and the current best profit. At each step, update those two values.

public int maxProfit(final int[] prices) {
    if (prices == null) {
        return 0;
    }

    int minPrice = Integer.MAX_VALUE;
    int maxProfit = 0;

    for (int i = 0; i < prices.length; i++) {
        maxProfit = Math.max(maxProfit, prices[i] - minPrice);
        minPrice = Math.min(minPrice, prices[i]);
    }

    return maxProfit;
}


The end-of-list patterns can be combined into one:

zip4 (b:bs) (c:cs) (d:ds) (e:es) = (b,c,d,e) : zip4' bs cs ds es
zip4 _ _ _ _ = []

There's no need for a separate zip4' helper.

This definition is correct, but it's unnecessarily complex. List functions like this are usually defined in terms of more general list functions like map, rather than by explicit recursion. The actual definition of zip4 is:

zip4 = zipWith4 (,,,)

Doing recursion exercises like reimplementing standard list functions can teach you the bad habit of using recursion even when you don't need it. Good Haskell code does almost all list manipulation with high-order functions or list comprehensions. It's better practice to use them than reimplement them.


Basic C++ Circle Class I'm trying to improve on my basic C++ skills and create really nice, concise and clean code that is free from artifacts.

The program can add and subtract radius values from each circle, assign one circle to another and furthermore give the area.

Any tips on how this program can be improved, whether considering readability issue or anything else would be great.

//  Basic circle exercise 23rd March

#include <cstdio>
#include <math.h>

using namespace std;

//==============================================================================
/*                               interface                                    */
//==============================================================================
namespace corey
{
    class Circle
    {
    public:
        Circle(const int & r = 1.0);
        ~Circle();

        const double & getRadius() const;
        const double getArea() const;

        Circle & operator = (const Circle &);
    private:
        double radius;
    };
};

//==============================================================================
/*                              implementation                                */
//==============================================================================
/*I'd probably included using namespace corey if
    i'd have bothered with seperate files*/

corey::Circle::Circle(const int & r) : radius(r){}

corey::Circle::~Circle(){}

const double & corey::Circle::getRadius() const
{
    return radius;
}

const double corey::Circle::getArea() const
{
    const double area = (M_PI * radius * radius);
    return area;
}

corey::Circle operator + ( const corey::Circle & lhs, const corey::Circle & rhs)
{
    return corey::Circle(lhs.getRadius() + rhs.getRadius());
}

corey::Circle operator - ( const corey::Circle & lhs, const corey::Circle & rhs)
{
    return corey::Circle(lhs.getRadius() - rhs.getRadius());
}

corey::Circle & corey::Circle::operator = (const Circle & lhs)
{
    if(&lhs != this)
    {
        radius = lhs.radius;
    }
    return (*this);
}

//==============================================================================
/*                                   useage                                   */
//==============================================================================
int main( int argc, char ** argv )
{
    //this stuff is just some info printed out so not super important...
    corey::Circle c1(2);
    printf("Circle made with radius %lf & area %lf\n", c1.getRadius(), c1.getArea());

    const corey::Circle c2(6);
    printf("Circle made with radius %lf & area %lf\n", c2.getRadius(), c2.getArea());

    corey::Circle c3;
    c3 = c2 + c1;
    printf("Circle made with radius %lf & area %lf\n", c3.getRadius(), c3.getArea());

    c1 = c3;
    printf("Circle made with radius %lf & area %lf\n", c1.getRadius(), c1.getArea());

    return 0;
}

Be consistent with #include
We have C++ <cstdio> (which defines the C names in the std namespace), but the C-style <math.h> (which puts the identifiers in the global namespace).  For new C++ code, prefer <cmath>.  And don't use using namespace std; - that can be harmful.
Allow the compiler to define assignment
We don't need to do anything in operator=() that's different from the compiler-generated (member-by-member copy) assignment.  Declaring our own actually prevents some optimizations that can be done for so-called "trivial" types (without the operator, the compiler knows it can simply copy the object's memory, but with one, it must assume that copying is more involved than that; it also inhibits the generation of a move assignment operator).
Similarly, we don't need to define a destructor - let the compiler generate that, too.
Pass by value for primitive types
There's no benefit to passing a reference to const int in the constructor - just pass the int by value.  That's simpler and (before optimization) faster.
Actually, why is this an int, when we could accept any double value?  And why is the default 1.0?  I think that 0.0 (an empty circle) would be less surprising (it's then more like the numbers, where default initialization uses zero).  Consider also some validation - is a negative radius meaningful for your code?
It's better to return double by value from getRadius() for much the same reasons.
Mark the constructor explicit
Unless you like surprises, it's better if you don't allow numbers to convert automatically to circles.
Don't return 'const' values
The const double return type of getArea() is no different to plain double - the const will be ignored (and that's a good thing).  So it's useless clutter to write it (and GCC will warn you, assuming you turn on a reasonable set of warnings).
Operators
What does it mean to subtract one circle from another?  In a geometry library, I'd want to get an annulus back (i.e. a circle with a hole cut out) or not be allowed to subtract at all.  Similarly for addition.
If you do choose to provide operators, you'll want to declare them with your interface (so this could be moved to a header).  And you should consider whether to provide the corresponding assignment operators (+= and -=).
On the positive, you have the correct argument and return types here.
Portability
Your environment clearly provides you with M_PI for a value of π.  That's not a standard identifier, and although it's a common extension, it's more portable to define your own.  Popular expressions producing π include 4 * std::atan(1) and std::acos(-1)  These are both compile-time constants, so cost nothing to the program's users.
A thing I like in the getArea() is that you write radius * radius and didn't get seduced by std::pow(radius, 2) - which is generally slower and less accurate than multiplication (it's more versatile, because it supports fractional powers, but we don't need that here).
Prefer C++ formatted output
The <iostream> header provides I/O with better type-checking than the C-style <cstdio> functions.  Consider providing a << operator to stream to a standard library stream.

Modified code

#include <cmath>
#include <iosfwd>

namespace corey
{
    class Circle
    {
        static constexpr double pi = 4 * std::atan(1);
        double radius;

    public:
        explicit Circle(double r = 0);

        double getRadius() const;
        double getArea() const;
    };

    Circle operator+(const Circle&, const Circle&);
    Circle operator-(const Circle&, const Circle&);

    std::ostream& operator<<(std::ostream&, const Circle&);
}

#include <ostream>

namespace corey {
    Circle::Circle(double r)
        : radius(r)
    {}

    double Circle::getRadius() const
    {
        return radius;
    }

    double Circle::getArea() const
    {
        return pi * radius * radius;
    }

    Circle operator+(const Circle& lhs, const Circle& rhs)
    {
        return Circle(lhs.getRadius() + rhs.getRadius());
    }

    Circle operator-(const Circle& lhs, const Circle& rhs)
    {
        return Circle(lhs.getRadius() - rhs.getRadius());
    }

    std::ostream& operator<<(std::ostream& os, const Circle& c)
    {
        return os << "Circle made with radius " << c.getRadius()
                  << " & area " << c.getArea();
    }
}

#include <iostream>

int main()
{
    using corey::Circle;

    Circle c1(2);
    std::cout << c1 << '\n';

    const Circle c2(6);
    std::cout << c2 << '\n';

    Circle c3;
    c3 = c2 + c1;
    std::cout << c3 << '\n';

    c1 = c3;
    std::cout << c1 << '\n';
}


Add Strings in-between index of ArrayList if not already exists So as the title states, I am trying to add a specific Items to an ArrayList if they not already exist. So here is how the Array starts.

[a, b, c, d, e, f]

I have this items inside the ArrayList

and I'm gonna add some items after the item d if it doesn't exist already in a pattern.

[a, b, c, d, x, y, z, e, f]

I had a successful run of the items I have provided resulting with the output

[a, b, c, d, e, f]
Adding string x to index 3
Adding string y to index 4
Adding string z to index 5
[a, b, c, d, x, y, z, e, f]
String already exists x skipping...
String already exists y skipping...
String already exists z skipping...
[a, b, c, d, x, y, z, e, f]

here is the code that I have created to get this result

public static void addStringIfNotAlreadyExists(List<String> list, int index, String... strings) {
    for (String s : strings) {
        if (!list.get(index + 1).equals(s)) {
            System.out.println("Adding string " + s + " to index " + index);
            list.add(index + 1, s);
        } else {
            System.out.println("String already exists " + s + " skipping...");
        }
        index++;
    }
}

It's not the best code I have written but it works.

and here is how I call it for the test.

public static void main(String[] args) {
    List<String> list = new ArrayList<>();
    list.add("a");
    list.add("b");
    list.add("c");
    list.add("d");
    list.add("e");
    list.add("f");
    System.out.println(list);
    addStringIfNotAlreadyExists(list, 3, "x", "y", "z");
    System.out.println(list);
    addStringIfNotAlreadyExists(list, 3, "x", "y", "z");
    System.out.println(list);
}

What I am curious in if this code can fail in certain tests, Or how I can improve the code.

A few things come to mind:

First of all, as you access the list at index + 1 without any bounds check, the code will fail if a user attempts to use the method to add elements to the end of the string. You should probably handle that gracefully.
Changing the input parameter's value index is not expressedly forbidden, but it makes my skin crawl. Furthermore, you have to coordinate two values in a loop (s and index) which makes the code error prone. In this case, I'd go for an index based loop instead for(int i = 0; i < strings.length; i++) and use strings[i] and list.get(index + i). This way, you have only a single variable changing in the loop.
If there's nulls in the list, the code will throw an NPE. Preferably use Objects.equals(x, y) instead of x.equals(y) to stay null-safe.


Java 8 streams - restructure entity classes for UI Grid I have the following domain model:

class Holding
------------
Account account;
DataSourceProduct dataSourceProduct;

class Account
-------------
String name;

class DataSourceProduct
-------------
DataSource dataSource;
Product product;
// other product data specific to the data source

class Product
--------------
String id;

class DataSource
----------------
String name;

I get a list of Holding data from three different data sources. I need to show the data in a grid. Each row in the grid has the same product and account and will have columns to show the product data as provided by the three different data sources. Note: One of the data sources may have a holding with a product id of null.

To accomplish this, I currently break up the list of holdings into several lists where each list contains only holdings with the same product id and account. Also, I key each list by data source. So, in the end, I have the following:

List<Map<DataSource,Holding>>

So, we have something like this:

index 0 in the list
    Map:Key       Map:Value
    datasource1   holding1AccordingToDs1  ----|
    datasource2   holding1AccordingToDs2  ----|---> all three holdings have same product and account
    datasource3   holding2AccordingToDs3  ----|
index 1 in the list
    Map:Key       Map:Value
    datasource1   holding2AccordingToDs1  ----|
    datasource2   holding2AccordingToDs2  ----|---> all three holdings have same product and account
index 2 in the list
    Map:Key       Map:Value
    datasource1   holding3AccordingToDs1  ----|
    datasource3   holding3AccordingToDs3  ----|---> all three holdings have

To get from List<Holding> to List<Map<DataSource,Holding>> I do the following:

    // these holdings could have an null instrument id
    // and this data is returned from a different db than other
    // holdings
    List<Holding> aSourceHoldings = holdingService.getHoldings(date);

    List<Holding> unMatchedHoldings = aSourceHoldings .stream()
            .filter(h -> h.getDataSourceProduct().getProduct().getId() == null)
            .collect(Collectors.toList());

    // if null product id, no way to match to other instruments
    aSourceHoldings.removeAll(unMatchedHoldings);

    // get other data source holdings
    List<Holding> toBeMatchedHoldings = holdingRepository.getHoldings(date);
    toBeMatchedHoldings.addAll(aSourceHoldings);

    // grouping by account and product
    Map<Account, Map<Product, List<Holding>>> matchedHoldings =
            toBeMatchedHoldings.stream()
            .collect(Collectors.groupingBy(Holding::getAccount,
                    Collectors.groupingBy(h -> h.getDataSourceProduct().getProduct())));

List<Map<DataSource, Holding>> unifiedHoldings = new ArrayList<Map<DataSource, Holding>>();
    Map<DataSource, Holding> tmp = new HashMap<DataSource, Holding>();
    for (Map<Product, List<Holding>> productHoldings : matchedHoldings.values()) {
        for (List<Holding> holdings : productHoldings.values()) {
            for (Holding holding : holdings) {
                tempMap.put(holding.getDataSourceProduct.getDataSource(), holding);
             }
            unifiedHoldings.add(tempMap);
        }
    }
    return unifiedHoldings;

Like data sources, the number of accounts is small (4-5) and will remain fairly static over the years. Is it possible to reduce the number of manipulations to group and get the data that I need for the UI grid?

Type inference for generic instance creation
You can rely on type inference for generic instance creation when declaring unifiedHoldings:
List<Map<DataSource, List<Holding>>> unifiedHoldings = new ArrayList<>();

Mutable operations on collections
List<Holding> aSourceHoldings = holdingService.getHoldings(date);
List<Holding> unMatchedHoldings = /* ... */ ;
aSourceHoldings.removeAll(unMatchedHoldings);
List<Holding> toBeMatchedHoldings = holdingRepository.getHoldings(date);
toBeMatchedHoldings.addAll(aSourceHoldings);

Calling mutable operations such as removeAll(Collection) and addAll(Collection) may throw UnsupportedOperationException if the underlying List does not return a mutable implementation. As such, in these scenarios, you should be creating your own List given the resulting elements, and then do the necessary removal/addition calls on the new instance.
Account and Product usage
Interestingly, it looks like Account and Product themselves aren't used in the processing, after kind of using them as 'keys' for grouping. The nested Maps looks too peculiar, which brings us to the next point...
Grouping your Holding objects

I currently break up the list of holdings into several lists where each list contains only holdings with the same product id and account. Also, I key each list by data source.
(emphasis mine)

This looks like a good use case for a supplementary 'key' class that will allow us to grouping Holding objects with the same account, product and data source. For example:
private static final class HoldingKey {
    private final Account account;
    private final Product product;

    HoldingKey(Account account, Product product) {
        this.account = Objects.requireNonNull(account);
        this.product = Objects.requireNonNull(product);
    }

    Account getAccount() {
        return account;
    }

    Product getProduct() {
        return product;
    }

    @Override
    public boolean equals(Object o) {
        return o instanceof HoldingKey
                && getAccount().equals(((HoldingKey) o).getAccount())
                && getProduct().equals(((HoldingKey) o).getProduct());
    }

    @Override
    public int hashCode() {
        return Objects.hash(getAccount(), getProduct());
    }

    static HoldingKey toKey(Holding holding) {
        return new HoldingKey(holding.getAccount(),
                                holding.getDataSourceProduct().getProduct());
    }
}

Using the method reference HoldingKey::toKey to group Holding objects by, with a dash of Stream.concat(Stream, Stream) to combine two streams together, we can combine the processing on both the repository and service results with relative ease:
    Stream<Holding> fromRepository = holdingRepository.getHoldings(date).stream();
    Stream<Holding> fromService = holdingService.getHoldings(date).stream()
            .filter(h -> h.getDataSourceProduct().getProduct().getId() != null);
    Map<HoldingKey, List<Holding>> map = Stream.concat(fromRepository, fromService)
            .collect(Collectors.groupingBy(HoldingKey::toKey));

This simplifies the nested Maps we have by flattening that multi-key structure into a single HoldingKey object. Achieving what you require is a simple continuation on map's entrySet(), with a mapping function to turn List<Holding> into your required Map<DataSource, Holding>:
private static final Function<List<Holding>, Map<DataSource, Holding>> MAPPER =
        x -> x.stream().collect(Collectors.toMap(
                y -> y.getDataSourceProduct().getDataSource(), Function.identity()));

private static List<Map<DataSource, List<Holding>>> getUnifiedHoldings(Date date) {
    Stream<Holding> fromRepository = holdingRepository.getHoldings(date).stream();
    Stream<Holding> fromService = holdingService.getHoldings(date).stream()
            .filter(h -> h.getDataSourceProduct().getProduct().getId() != null);
    return Stream.concat(fromRepository, fromService)
            .collect(Collectors.groupingBy(HoldingKey::toKey))
            .entrySet()
            .stream()
            .map(MAPPER.compose(Map.Entry::getValue))
            .collect(Collectors.toList());
}

Here, we compose() our MAPPER function with the 'extractor' function Map.Entry::getValue to derive our resulting list elements.


Computing intersections of a Polyline and a line I wrote a script to compute / interpolate the nodes of a Polyline at a given latitude, input_lat. It works for my purpose but I am wondering if there is a better, more optimized way of doing this.

The idea is to first check if there is any exact match, i.e. if there are nodes at the exact given latitude so I subtract the input latitude to the list of nodes (the latitudes list) and if there are any zeros, I can just read the longitude of these.

Most of the time the crossing will be between 2 consecutive nodes, with one being below (or above) and the next one being on the other side of the input_lat (the node above will retain a positive value after subtracting input_lat and the node below a negative value) so in my latitudes list I detect that by checking for a change of sign between two consecutive elements, so I multiply each element with the next: if the result is negative, there is a change of sign.
I do this with np.multiply(latitudes[1:], latitudes[:-1]) but I wonder if there is a better way.

The script is meant to run from within QGIS so all the qgis.* libraries that are imported and the Qgs commands are specific to QGIS.

"""Script to compute coordinates of a Polyline at a given Latitude"""

#import QGIS specific libraries
from qgis.core import *
from qgis.gui import *
import qgis.utils

#import numpy
import numpy as np

"""Assuming the following :
- a Line/Polyline Layer is selected"""

#the latitude we want to compute intersections with
input_lat = 28.54456111
ray = [0, input_lat]
longitudes = []

def getIntersectLon(a, b, lat):
    """Compute the Longitude of the intersection between two nodes"""
    return a[0] - (a[1] - lat) * (a[0] - b[0]) / (a[1] - b[1])

layer = qgis.utils.iface.activeLayer()
if layer is None:
    iface.messageBar().pushMessage("Select a Layer", level=QgsMessageBar.CRITICAL, duration=2)
else:
    for feature in layer.getFeatures():
        geom = feature.geometry()
        if geom.type() == QGis.Line:
            # get the nodes coordinates in an array :
            #   geom.asPolyline() will produce an array of tuples
            #   that we will convert to an array of arrays so we
            #   can mutate the values
            nodes = np.asarray(geom.asPolyline())
            #
            # THIS IS THE CORE ROUTINE
            #
            # get the list of the difference between latitudes and the input
            latitudes = (nodes - ray)[:,1]
            # if there are zeros, we have points at the exact given latitude
            exact = np.where(latitudes == 0)[0]
            for e in exact:
                longitudes.append(nodes[e][0])
            # where line crosses between nodes, there will be a change of sign
            # one node being below, and the next above the input
            xing = np.multiply(latitudes[1:], latitudes[:-1])
            # get the indexes of the sign changes
            crossing = np.where(xing < 0)[0]
            for c in crossing:
                longitudes.append(getIntersectLon(nodes[c], nodes[c+1], input_lat))
            #
            # THIS IS THE END OF THE CORE ROUTINE
            #
            # we will now create points the found latitudes (if any)
            #   that we will load into a new Point layer for display
            #
            if longitudes:
                #create a Point layer to store the intersections found
                newlayer =  QgsVectorLayer("Point", "Intersections", "memory")
                pr = newlayer.dataProvider()
                feat = QgsFeature()
                for lon in longitudes:
                    feat.setGeometry(QgsGeometry.fromPoint(QgsPoint(lon, input_lat)))
                    pr.addFeatures( [ feat ] )
                    print lon
                QgsMapLayerRegistry.instance().addMapLayers([newlayer])
            qgis.utils.iface.mapCanvas().refresh()
        else:
             iface.messageBar().pushMessage("Select a Line layer", level=QgsMessageBar.CRITICAL, duration=2)

In getIntersectLon, I didn't find the docstring very helpful. For y = m*x + b, I think of m as rise over run, but in (a[0] - b[0]) / (a[1] - b[1]) I think I'm seeing run over rise. Ok. So we're multiplying 1/m by some delta_latitude, then we subtract, which seems like it could be related to the docstring. Maybe the docstring should talk about how the line described by two points intersects a line of latitude. Given that (lat,lng) are spherical coordinates, the approximation seems to have accuracy that falls off with increasing distance. Maybe this function should be named getLongitudeAt, with 1st parameter being the line of latitude.

It's not obvious to me that a[1] - b[1] shall be non-zero.

The latitudes == 0 test is maybe more stringent than you would like. Consider using abs(latitudes) < epsilon.

If you're mostly interested in sign changes, you might want to define sgn() and then work with small signum values.

def sgn(n):
    return math.copysign(1, n)

The comments "THIS IS THE {|END OF} THE CORE ROUTINE" suggest that you may want to break out a helper routine.


Snoozy: A lazy reevaluation upon get I would appreciate critique and comments in regards of thread-safety on a basic type I'm trying to create.

In C#, the Lazy<T> type is initialized only once, and only when needed the first time.

My type, called Snoozy<T>, is similar, but differs in one important aspect:

The value can be reinitialized when necessary (but only when read).

The purpose is to not perform the value factorization unless someone needs the new value. This could prevent e.g. unnecessary heavy calculations.

Example: A bank account stores a long list of events describing withdrawals and deposits. To get the current balance, all transactions need to be summed up. This could potentially be a very CPU-intensive operation, but the adding of new transactions is rare whereas reading the balance is frequent. When adding a new transaction, there is really no need to recalculate the balance until someone explicitly wants to read it.

This is the code I've come up with so far:

public class Snoozy<T>
{
    private readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();
    private readonly Func<T> _factory;
    private T _value;
    private bool _isStale;

    public Snoozy(Func<T> valueFactory)
    {
        _factory = valueFactory;
        _isStale = true;
    }

    public T Value
    {
        get
        {
            _lock.EnterUpgradeableReadLock();

            if (_isStale)
            {
                _lock.EnterWriteLock();

                _value = _factory();
                _isStale = false;

                _lock.ExitWriteLock();
            }

            _lock.ExitUpgradeableReadLock();

            return _value;
        }
    }

    // This is how the consumer signals that the underlaying value has changed.
    public void Poke()
    {
        _lock.EnterWriteLock();
        _isStale = true;
        _lock.ExitWriteLock();
    }

    public static implicit operator Snoozy<T>(T value)
    {
        return new Snoozy<T>(() => value);
    }

    public static implicit operator T(Snoozy<T> snoozy)
    {
        return snoozy.Value;
    }
}

I have some bad news for you... According to MSDN only one thread can enter an upgradeable read lock:

  Only one thread can enter upgradeable mode at any given time. If a thread is in upgradeable mode, and there are no threads waiting to enter write mode, any number of other threads can enter read mode, even if there are threads waiting to enter upgradeable mode.

That means your current implementation is basically just exclusive locking.

You should validate your parameters:

public Snoozy(Func<T> valueFactory)
{
    if (valueFactory == null)
    {
        throw new ArgumentNullException(nameof(valueFactory));
    }

and likewise in your conversions.

On the whole, this feels like a cached value to me - there are probably existing constructs which would suit your needs better depending on what framework/type of app you're building. Rolling your own cache is hard.
Remember, there are only 2 hard problems in programming: naming, cache invalidation and off by one errors.


Comparing two arrays by index in python in a cleaner way I have a simple task I want to do, I want to compare two arrays like this:
array 1 of data:  [0.0, 92.6, 87.8, 668.8, 0.0, 86.3, 0.0, 147.1]
array 2 of data:  [16.7, 0.0, 0.0, 135.3, 0.0, 205.5, 0.0, 93.8]

I want to find the difference between each value by index, and if the difference is lower than 50%, keep the value of the second array in a third array.
array 3 of data : [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 93.8]

Now I believe using a for loop would be the best option, iterating through each array since both have the same length, the code below is part of my code, array1 and array2 are arrays like above.
for scan in sensor:
        #here is some code where we build the Array1 and Array 2 from serial data which is inside the loop.
    if specialcount ==1: #build the first array
      #get data from dict and build first array
      datadict={ 0:[], 1:[], 2:[], 3:[], 4:[], 5:[], 6:[], 7:[], }
    if specialcount ==2: #build the second array.
        #get data from dict and build second array
      datadict={ 0:[], 1:[], 2:[], 3:[], 4:[], 5:[], 6:[], 7:[], }
    if specialcount ==3: #after we build 2 arrays, specialcount goes to 3, and we now compare the arrays
            indexsizeofArray1 = len(array1)
            indexsizeofArray2 = len(array2)
            if indexsizeofArray1 != indexsizeofArray2:
                print ("array mismatch, try again")
                break
            elif indexsizeofArray1 == indexsizeofArray2:
                print("array match, proccessing")
                array3 = [0,0,0,0,0,0,0,0]
                for X in range (indexsizeofArray2):
                    difference = array2[X]-array1[X]
                    fiftypercent = array1[X]-(array1[X]/2)
                    lowerrange = int(array1[X]-fiftypercent)
                    upperrange = int(array1[X]+fiftypercent)
                    if array2[X] in range (lowerrange,upperrange):
                        array3[X]=array2[X]
                    if X == indexsizeofArray2:
                        print("final array: ",array3)
    specialcount +=1

Above is my code ,I do get the result I want of course, but I believe I could make it cleaner mostly because I am in the code transforming float to int as they work with the range function. I am sure there is a cleaner way of doing this , that specialcount==3 is my condition since I build the 2 arrays inside the forloop before as soon as the second array is built, then we, this is inside a loop where I am polling constantly a sensor, the sensor is special in the sense that if I stop polling it and go out of the loop, it will not work again unless I restart it , as such I must be inside this loop all the time.
I am sure I can improve the speed of it if slightly, mostly on the reinitialization of the datadict, since I initialized it before , however inside the loop I populate it with the data up until a certain point, in which I then create the first array of data, and clear it so I can rebuild it.  The goal is to build two arrays of 2 different variables that must be related, once those 2 lists  are built, I need to compare them, ensure that they are not that different and if they are not , pass it to another function , if its too different, the value for that index in the list is 0 .

Don't use the loop-switch antipattern
If you have three different things that have to be done in order, just write them in that order. Don't put a loop around them with if-statements inside to check what has to be done that iteration; loops are for doing the same task over and over. What you are doing has a name: it is the loop-switch antipattern. Instead, just write:
#get data from dict and build first array
datadict={ 0:[], 1:[], 2:[], 3:[], 4:[], 5:[], 6:[], 7:[], }

#get data from dict and build second array
datadict={ 0:[], 1:[], 2:[], 3:[], 4:[], 5:[], 6:[], 7:[], }

#we now compare the arrays
...

I assume that's not the real code. If each of the three tasks is of course much longer than what you wrote, then it helps to put them in functions.
Split the problem up into smaller sections
It always helps to break up a problem into smaller sections, and for example you can then handle each sub-problem in a separate function. In this case, looping over the arrays can be split from deciding whether or not to keep a value. So:
keep_within_fifty_percent(a, b):
    fiftypercent = a // 2
    lower = a - fiftypercent
    upper = a + fiftypercent

    if b in range(lower, upper):
        return b;
    else:
        return 0

...

#we now compare the arrays
if len(array1) != len(array2):
    # handle error here
    ...

array3 = [keep_within_fifty_percent(a, b) for a, b in zip(array1, array2)]
print("final array: ", array3)


Check for timeout in wait() loop I want to check for a condition in a guarded wait() loop with a user-specified total timeout, similar to how LinkedBlockingQueue.poll() works.

For example collection can be appended to by different threads, but only when the collection contains a value equating to myItem within timeout, should the loop terminate successfully. If the user-specified timeout elapses, it should throw.

This is my current best approach:

long startTime = System.currentTimeMillis();
synchronized (collection) {
    while (!collection.contains(myItem)) {
        long remaining = System.currentTimeMillis() - startTime - timeout;
        if (remaining < 0)
            throw new TimeoutException();

        collection.wait(remaining);
    }
}

The problem here is that System.currentTimeMillis() (or System.nanoTime()) is called inside a synchronized context. IntelliJ's warning states:

  While not necessarily representing a problem, such calls cause an expensive context switch, and are best kept out of synchronized contexts, if possible.

I wonder how much impact this 'expensive context switch' will have in practice, with Oracle JVM 8?

This is one alternative I came up with, but I don't like this as the actual timeout becomes ⌊timeout/10⌋*10 + delta. and the error will probably be much worse than the context switch.

synchronized (collection) {
    int loops = 0;
    while (!collection.contains(myItem)) {
        if(loops++ > timeout / 10)
            throw new TimeoutException();

        collection.wait(10);
    }
}

System.currentTimeMillis() is the system clock, and can change if someone changes the current date / time.  It's not really a good way to accurately measure elapsed time.  Use System.nanoTime() instead, which is invariant to system clock changes.


How I can short my code when request have many parameters Brief:

I've create simple filter in my laravel project. I'll send parameters with ajax request.

Code:

public function specialitiesAjaxLoadMore(Request $request)
{
    $city = null;
    if(isset($request->city_id)) {
        $city = City::find($request->city_id);
        if($city !== null) {
            $city = $city->id;
        }
    }

    $sex = null;
    if((isset($request->sex) && is_numeric($request->sex)) && $request->sex == 1 || $request->sex == 2) {
        $sex = $request->sex;
    }

    $ageFrom = null;
    if((isset($request->ageFrom) && is_numeric($request->ageFrom)) && $request->ageFrom > 7) {
        $ageFrom = $request->ageFrom;
    }

    $ageTo = null;
    if((isset($request->ageTo) && is_numeric($request->ageTo)) && $request->ageTo > 7) {
        $ageTo = $request->ageTo;
    }

    if(isset($city)) {
        $candidates = Candidate::where('city_id', $city)->get();
    }

    if(isset($sex)) {
        $candidates = Candidate::where('sex', $sex)->get();
    }

    if(isset($ageFrom)) {
        $candidates = Candidate::where('birthday', '<=', date('Y-m-d', strtotime("-$ageFrom years")))->get();
    }

    if(isset($ageTo)) {
        $candidates = Candidate::where('birthday', '>=', date('Y-m-d', strtotime("-$ageTo years")))->get();
    }

    if(isset($city, $sex)) {
        $candidates = Candidate::where('city_id', $city)->where('sex', $sex)->get();
    }

    if(isset($city, $sex, $ageFrom)) {
        $candidates = Candidate::where('city_id', $city)
                               ->where('sex', $sex)
                               ->where('birthday', '<=', date('Y-m-d', strtotime("-$ageFrom years")))
                               ->get();
    }

    if(isset($city, $sex, $ageFrom, $ageTo) && $ageTo < $ageFrom) {
        $candidates = Candidate::where('city_id', $city)
                               ->where('sex', $sex)
                               ->where('birthday', '<=', date('Y-m-d', strtotime("-$ageFrom years")))
                               ->where('birthday', '>=', date('Y-m-d', strtotime("-$ageTo years")))
                               ->get();
    }

}

In my case there will be a lot of parameters from the request and if I continue to write everything in such a way as seisai then there will be a lot of combinations. So is it possible to improve or shorten my current code?

https://laravel.com/docs/5.7/queries#where-clauses

$where = [];
if($city) {
    $where[] = ['city_id', '=', $city];
}
if($sex) {
    $where[] = ['sex', '=', $sex];
}
if($ageFrom) {
    $where[] = ['birthday', '<=', date('Y-m-d', strtotime("-$ageFrom years"))];
}
if($ageTo) {
    $where[] = ['birthday', '>=', date('Y-m-d', strtotime("-$ageTo years"))];
}
$candidates = Candidate::where($where)->get();