Choosing between 2 Fibonacci alternatives Task:
Return the fibonacci value at a given index.
e.g: input: 6, return: 8.

Algorithm 1:

public static fibonacci(input: number): any {
    if (input <= 1) return input;
    return this.fibonacci(input - 1) + this.fibonacci(input - 2);
}

  Time complexity: \$O(n^2)\$,
  Space complexity: \$O(1)\$

Algorithm 2:

public static fibonacci2(input: number): any {
    if (input <= 1) return input;

    let a = 0;
    let b = 1;
    let n = 0;
    for (let i=2; i<=input; i++) {
        n = a + b;
        a = b;
        b = n;
    }
    return n;
}

  Time complexity: \$O(n)\$,
  Space complexity: \$O(1)\$

Am I right regarding the complexities?

Can you suggest any alternatives that achieve the same result, with different time/space complexity?

You can compute fibonacci numbers with both time and space complexity O(1).

(n) => ((Math.Pow(phi,n) - Math.Pow(1-phi, n)) / Math.Sqrt(5);

where phi is the golden ratio:

(1 + Math.Sqrt(5)) / 2

But if you needed to iterate fibonacci numbers one after the other, I would use an "iterator", then every other number would also be generated in O(1) and it would be better then the double formula.


Leap year check in Java I'm looking for 3 different answers:

Using Java predefined functions
Converting from string to number
Without the above two (any other types of answers are also welcome)

I am looking for improving my code (as per the above 3 conditions for 3 answers) taken from my repository here.

/*
 * Checks if the year passed is a leap year
 *
 * @param year; requires the year to have 4 digits
 * @return true if leap, else false
 *
 */

public static boolean isLeapYear(int year) {
    String testYear = String.valueOf(year);
    if (testYear.charAt(2) == '1' || testYear.charAt(2) == '3' ||
            testYear.charAt(2) == 5 || testYear.charAt(2) == '7' ||
            testYear.charAt(2) == '9') {
        //If the third digit is odd

        if (testYear.charAt(3)=='2'||testYear.charAt(3)=='6'){
            return true;
        }
        else{
            return false;
        }
    }
    else{
        if (testYear.charAt(2) == '0' && testYear.charAt(3) == '0') {
            if(testYear.charAt(0) == '1' || testYear.charAt(0) == '3' ||
                    testYear.charAt(0) == 5 || testYear.charAt(0) == '7' ||
                    testYear.charAt(0) == '9'){
                //If first digit is odd

                if(testYear.charAt(1)=='2'||testYear.charAt(1)=='6'){
                    return true;
                }
                else{
                    return false;
                }
            }

            //If second digit is not odd
            else if(testYear.charAt(1)=='0'||testYear.charAt(1)=='4'||
                    testYear.charAt(1)=='8'){
                return true;
            }
            else
                return false;
        }
        else if (testYear.charAt(3)=='0'||testYear.charAt(3)=='4'||
                testYear.charAt(3)=='8'){
            //If fourth digit is not odd
            return true;
        }
        else
            return false;
    }
}

Processing the number as a string

Don't. In general, converting a numerical value to a string and using some string processing on what was once a number is a code smell. You of course often need to convert numbers to strings for output and formatting, but mathematical operations should be done mathematically.

If you for instance wanted to check whether the second-last digit is even, you can divide your number by 10 and check the oddity of that result. That's an arithmetic check without involving any string processing.

The correct algorithm

Whenever you approach a problem, you should think about the correct algorithm to solve it, and do some research, if applicable. Leap years for instance can be detected by a much simpler algorithm. Leap years in the Gregorian calendars are years that are divisible by 4 but not 100, unless they are also divisible by 400. So:

If it's not divisible by 4, it's not a leap year.

If it's divisible by 4, it's a leap year if: it's not divisible by 100, or it's divisible by 400.

Now that looks almost like an algorithm in code. Which you can write along the lines of

if (year % 4 == 0) {
    if (year % 100 != 0) {
        return true; //Divisible by 4 and not by 100, leap year
    }
  return (year % 400 == 0); //Divisible by 4 and by 100. Leap year if divisible by 400.
}
return false; //Not divisible by 4 - not a leap year.

Of course that could also be a one-liner, but this perhaps makes the decision logic a bit more clear.

Java 8

In Java 8, a lot of functionality for handling calendars has been added, such as the Year class. The following would work:

return Year.isLeap(year);

General

It's hard not to notice that your formatting is inconsistent. Sometimes space before opening braces, sometimes not. Sometimes you surround a single statement with braces, sometimes not. Those are largely up to individual preference (although I think space before braces is a must, as is enclosing single statements in braces), but consistency is definitely very important.

Also, as the answer by RubberDuck notes, it's not needed or a good practice to write code of the if (something) return true; else return false type when you can just return the condition directly.


Is there a "code less" way to construct this JS loop? I just know there's a simpler, better way to do this. Will eventually append <select> options with these values (e.g. 01:30, 02:45, 10:15, etc.)

var arrHours=[]
var i=0, j=0, k=0;
for (i=0;i<=23;i++)
{
    for(j=0;j<=45;j=j+15)
    {
        var m,n;
        m=i.toString();
        n=j.toString();
        if(m=="0")
        {
            m="00";
        }
        if(m.length<2)
        {
            m="0"+m;
        }
        if(n=="0")
        {
            n="00";
        }
        arrHours.push(m+":"+n);
    }
}
for(k in arrHours)
{
    $("#preview4").append(arrHours[k]+"<BR>");
}

ps. I prefer the semicolon. ;)

I would do it like this. Note how the zero-padding is encapsulated in a function and how the double loops are turned into a single loop by using modulo and integer division. Also, the vars are pre-declared as is considered good style in JavaScript.

function zeroPad(number) {
    if (number <= 9) {
        return "0" + number;
    } else {
        return number;
    }
}

var i, hour, minute,
    arrHours = [];
for (i = 0; i < 60 * 24; i += 15) {
    hour = Math.floor(i / 60);
    minute = i % 60;
    arrHours.push(zeroPad(hour) + ":" + zeroPad(minute));
}


K&R 2.6 Exercise setbits(x,p,n,y) function Write a function setbits(x,p,n,y) that returns x with the n bits that begin at position p set to the rightmost n bits of y, leaving the other bits unchanged.

#include <stdio.h>

unsigned setbits(unsigned x, int p, int n,unsigned y);

int main(void){
    unsigned result=setbits(0, 4, 3, ~0);
    printf("%d\n",result);
    return 0;
}

unsigned setbits(unsigned x, int p, int n,unsigned y){
    unsigned ybits = y & ~(~0 << n);
    unsigned helper = ~(~0 << n);
    unsigned result = (x | (helper << (p+1-n))) & ((ybits << (p+1-n)) | (~(helper << (p+1-n))));
    return result;
}

Output:

  28

Your example, which uses x = 0 and y = ~0, is not a very useful demonstration, as those degenerate cases would tend to let bugs remain undetected.

You have some common expressions that reappear several times, which should be given variable names:

(p+1-n) appears three times in result.  I would name it shift.
~(~0 << n) appears in ybits, but it is the same as helper.  I would name it mask.

I would write it as (1 << n) - 1 instead.  It's probably not any more or less efficient than ~(~0 << n).  However, you use bitwise negation a lot.  When you write (~(helper << (p+1-n))), that's a negation of a negation of a negation, which I find headache-inducing.

Conversely, you gave variable names to ybits and result, neither of which provide any benefit.

Your strategy is, in summary:

(x | ForceRelevantBitsOn) & ((ybits << ShiftToRelevantPlace) | OnesEverywhereElse)

However, that strategy requires you to construct OnesEverywhereElse.  I recommend the following simpler technique instead:

(x & ForceRelevantBitsOff) | (ybits << ShiftToRelevantPlace)

Here's how I would write it:

unsigned setbits(unsigned x, int p, int n, unsigned y) {
    int shift = p - n + 1;
    unsigned mask = (1 << n) - 1;
    return (x & ~(mask << shift)) | ((y & mask) << shift);
}

The last line could be replaced with this expression, which might be more efficient (using five operations instead of six), but is more cryptic:

    return ((((x >> shift) ^ y) & mask) << shift) ^ x;

That works by determining which bits in x need to be toggled.


Lowest Common Multiple of 1 to n in Ruby Project Euler problem 5 asks:

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

I'm learning Ruby by going through Project Euler problems, and though I got the following code right (as in the answer given by it), I'm not sure if I'm doing things quite the "Ruby way". Hence, I wanted the community's opinion of my code to better understand alternatives to my code (using the same algorithm please!), so that I don't become used to writing "C++ using Ruby!"

def prime? x
  (2..x-1).each { |y| return false if x % y == 0 }
  true
end

def primes_upto n
   p = Array.new
   (2..n-1).each {|x| p.push(x) if prime? x}
   return p
end

upto = 20 # The upper limit of the LCM range
primes = primes_upto upto
range  = Array.new(upto) {|i| i + 1}
lcm = 1
primes.each do |i|
flag = true
while flag and range.length > 0
  range.each do |x|
    if x % i == 0
      flag = true
      break
    else
      flag = false
    end
  end
  if flag
    lcm *= i
    range.map! {|y| y % i == 0? y/i : y}
  end
  range.delete(1)
  range.compact!
end
end
print "\nLCM of 1..", upto, " = ", lcm

In particular, I would like to take advantage of ruby's built-in operators on Array, to reduce the size of the code, as well as make it less of an if-then, while structure that I've seen in C++ programs.

The algorithm I've used can be found here (if its not obvious from the code!)

Definitely your code looks like C directly translated to Ruby. Let's refactor the first two methods:

def prime?(n)
  (2..n-1).none? { |x| n % x == 0 }
end

def primes_upto(n)
  (2..n-1).select { |x| prime?(x) }
end

Of course prime? should be checking only 2 and odd numbers until sqrt(x) as candidates. Although you probably should be using the library in the core: Prime.

Calculating the lcd using a table is a possible way to do it, granted, but having the standard formula lcm(a, b) = a * b / gcd(a, b), and gcd being easily calculated with Euclides algorithm, well, I don't see why you'd choose that other way.

Anyway, note that to get the lcm of three numbers you can do lcm(a, b, c) = lcm(a, lcm(b, c)) (you get the idea for N numbers), and since Ruby >= 1.9 has Integer#lcm, you could simply write a left fold of the range:

(1..20).reduce(:lcm) #=> 232792560

BTW, that's how you could implement Integer#lcm in 1.8, a direct translation of the formulas:

class Integer
  def gcd(b)
    b == 0 ? self : b.gcd(self % b)
  end

  def lcm(b)
    (self * b) / self.gcd(b)
  end
end

A final advice: project-euler is about maths. When doing maths you should try to use expressions (functional programming) instead of statements and change of state (imperative programming). Try to do these problems without any of these: each, var = some_initial_value, push, bang_methods!, break, return, *=, +=, delete, while, ... Instead, you should be using: map, select, reduce, detect, zip, any?, all?, flatten, take_while, lazy (Ruby 2.0)... More on functional programming with Ruby.


Adding error messages I am new to PHP and was just wondering if there is a better way to write the following OR if it would make any difference to use elseif here instead of multiple if statements - especially if the options in questions might be more than in the example here (the current code is working).

Note: I cannot use switch here since the haystack string can contain multiple of the needle strings.

$errorList .= "Some text
";
    if(strpos($errorDetails, "emailInvalid") !== false){
        $errorList .= "
Some text
";
    }
    if(strpos($errorDetails, "passLength") !== false){
        $errorList .= "
Some text
";
    }
    if(strpos($errorDetails, "passLowerCase") !== false){
        $errorList .= "
Some text
";
    }
    if(strpos($errorDetails, "passUpperCase") !== false){
        $errorList .= "
Some text
";
    }
    if(strpos($errorDetails, "passNumberSymbol") !== false){
        $errorList .= "
Some text
";
    }
    if(strpos($errorDetails, "passMatch") !== false){
        $errorList .= "
Some text
";
    }
$errorList .= "
";
echo $errorList;

I would use an associative array (also known as  a "map" or "dictionary" in other languages).

$errorMessages = array('emailInvalid' => 'Invalid email',
    'passLength' => 'invalid password length',
    'passLowerCase' => 'missing lower case character in password'
    // and so on...
);

Then I would loop across this map:

foreach ($errorDetails as $key => $value) {
    if (strpos($errorDetails, $key) !== false) {
        $errorList .= '
' . $value . '
';
    }
}

strpos

Storing your error-message stubs in $errorDetails is inefficient. strpos loops through the string each time. I'd recommend to instead append your error messages to an array, which would open up the possibility of looking up the full error message from something like emailInvalid with a O(1) complexity.

How did I know it was a map that was needed?

Let's look at this code:

if(strpos($errorDetails, "emailInvalid") !== false){
    $errorList .= "
Some text
";
}
if(strpos($errorDetails, "passLength") !== false){
    $errorList .= "
Some (other) text
";
}

There are two things that differs here, these parts can be simplified to:

if (strpos($errorDetails, $xxxxxxx) !== false) {
    $errorList .= $yyyyyyy;
}

To clean up this code, we could extract a method and call that multiple times, such as:

addErrorList($errorDetails, 'emailInvalid', 'invalid e-mail');
addErrorList($errorDetails, 'passLength', 'invalid password length');
...

However, this is still not very nice. A map / associative array contains a key to value mapping, which is precisely what you have here. For each specific error detail key, there is an associated error message. So add those values to an associative array and then loop through it.


Compact ways to filter even numbers in Java int array I am writing some examples of Java programs on GitHub rsp/ppj for people to learn and I want them to be as simple and compact as possible, to work on Java 1.7.x (a.k.a. Java 7) and to not use any non-standard libraries that need installation.

This question is about my WhereEven example:

a function of an int[] array that returns an int[] array with only even numbers

(The method can internally use any other data types if it makes sense.)

Here is how my example looks like right now:

static int[] even(int[] t) {
    int i, j;
    for (i=j=0; i < t.length; i++)
        if (t[i] % 2 == 0) j++;
    int[] r = new int[j];
    for (i=j=0; i < t.length; i++)
        if (t[i] % 2 == 0) r[j++] = t[i];
    return r;
}

I am not satisfied with that code because it is much more complex than the same in JavaScript - even using the old ES5 syntax (few years older than Java 7) it is:

function even(t) {
    return t.filter(function(x){ return x % 2 == 0; });
}

It is even better in Scheme, which is amazing considering the fact that this is a language that's 40 years old:

(define (even t)
    (filter even? t))

But I can't get to the solution that is even remotely as simple and readable in Java. Of course I can add comments and more meaningful variable names, but the point is that in Scheme and JS the code is simple and the meaning is obvious as it is. What I am looking for is a way to write a better code, not to write the same code but with better names.

What in your opinion is "the best" way to improve the above code to be most useful for an educational example like this? Maybe the shortest way in terms of lines/characters, the most readable way, the most idiomatic way, the way using best practices, the most crazy way? I am looking forward to every suggestion of my code improvement that would help people learn something about Java and maybe I'll put few different approaches in my tutorial.

The only requirement is taking an int[] array, returning a new int[] array (but internally the method can use any other data types if it makes sense), working on Java 1.7.x (a.k.a. Java 7) and not using any non-standard classes.

First... choose the right problem or choose what you want them to learn

Let me first question your assumptions (pure Java 7, no "extra" libraries and array filtering). I do so under the premise that you want your students not just learn "pure Java 7" (ain't best idea neither commercially nor academically, nor will it make them greater programmers).

Also, why I keep typing "extra libraries" in "":

most of the projects now have Guava or Apache Commons
new Java has lambdas (for a reason and due to serious demand)
there are at least 2 popular libraries that enable Java 7 to use FP-like programming style: Javaslang, Lambda4Java (IIRC)

This is not "for fun", but because there was need for it. Among mainstream languages none treat "backward compatibility" so seriously as Java does. Despite deprecating APIs since version 1.1, they have yet to cut a thing. This means, there's lots of OLD stuff there.

Also, the problem you have (filtering data) is VERY FP friendly. FP languages among first operations have map and filter. Java at it's inception targeted C++ people, who wrote programs iteratively (for loops, step-by-step instructions to solve problems). FP works on streams and pipelines (data flows, and while it does, we do things).

So, yes, Scheme (FP language) is awesome for your problem. So is anything that has decent FP features (JS can be argued so).

Pure Java, without Apache Commons, Javaslang, Lambdas etc. will be hard-pressed for it:

you want to filter data
you want to use arrays (in Java objects are central concept, arrays are closer to primitives)
in Java, Collections are arrays equivalents of objects

Summarizing this long intro:

Filtering is FP-concept, FP-friendly languages or strict FP languages are  therefore better. If you choose examples like that and want to work with Java, work with Java Collections, libraries geared towards filtering/collections/data pipelines and Java 8 that introduces Lambdas.

Java 8 and streams

First proposition for you, uses streams, introduced in Java 8. There's a library that tries to make lambdas (and perhaps streams?) available in Java 7, so you may be able to show same code with it.

static void java8even(int[] t) {
    IntStream iWantEvens = IntStream.of(t);

    iWantEvens
        .filter(i -> i % 2 == 0)
        .forEach(new IntConsumer() {
            @Override
            public void accept(int value) {
                System.out.println(value);
            }
        });
}

I build a java.util.Stream with Stream.of(input array). Then I filter the stream and using forEach I on the fly create an IntConsumer that just prints the value. forEach performs an action that you define on every stream element. I could've used toArray and then function would - instead of having side effect - return an array, which I'd then have to print in main, iterating over it. It might look simpler and be 1-2 lines terser, but it's less memory efficient.

Answer within all your limits

With all that said, Java 7, no "extra" libs:

/**
 * Created by LAFK on 17.11.15.
 */
public class EvenFilter {

    static ArrayList evens = new ArrayList();

    public static void main(String[] args) {
        int[] simpleTable = new int[]{1, 2, 3, 4, 5, 6, 7, 8, 9, 10};

        for (int i: simpleTable) {
            if (i%2 == 0) {
                evens.add(i);
            }
        }
        System.out.println(evens);
    }
}

I used ArrayList without any type-safety. I then iterated over input array and added elements meeting criteria to ArrayList. Then, I printed out ("returned") ArrayList.

Why?

Java ain't that great with arrays (try printing an array without iterating over it's elements, weak API, fixed length of an array, while Java Lists can grow and shrink over time)
even more so with primitives, like int, try getting that into collections!
Java API for collections ain't that great as well, compared to modern languages or FP ones, but it's better than what you can do with just array
I don't need the fixed size with lists

I think similar thing can be done with a StringBuffer and were you to process something more text-oriented, that would be better choice.


Class with methods to retrieve data with Linq to sql I think here is something wrong with code. I use a class with methods to get tenants from DB:

    public List<CrmTenant> GetAllTenants()
    {
        List<CrmTenant> tenantsList = new List<CrmTenant>();

        try
        {
            var crmTenants =
                 from tenant in context.CrmTenant
                 select tenant;

            if (crmTenants != null)
            {
                foreach (var tenant in crmTenants)
                {
                    tenantsList.Add(tenant);
                }
            }

            return tenantsList;
        }
        catch (Exception ex)
        {
            this.logger.Write("Failed to get all tenants from database.", "Exceptions", TraceEventType.Error, ex);
            return null;
        }
    }

    /// <summary>
    /// Gets Tenant by name.
    /// </summary>
    /// <returns></returns>
    public CrmTenant GetTenantByName(string tenantName)
    {
        CrmTenant crmTenant = null;

        try
        {
            crmTenant =
                 (from customerCrm in context.CrmTenant
                  where customerCrm.TenantName == tenantName
                  select customerCrm).Single();
            return crmTenant;
        }
        catch (Exception ex)
        {
            this.logger.Write(String.Format("Failed to get tenant '{0}' from database.", tenantName), "Exceptions", TraceEventType.Error, ex);
            return null;
        }
    }

    /// <summary>
    /// Gets tenant by Username.
    /// </summary>
    /// <returns></returns>
    public CrmTenant GetTenantByUserName(string userName)
    {
        CrmTenant crmTenant = null;

        try
        {
            crmTenant =
                 (from customerCrm in context.CrmTenant
                  where customerCrm.Username == userName
                  select customerCrm).Single();
            return crmTenant;
        }
        catch (Exception ex)
        {
            this.logger.Write(String.Format("Failed to get tenant by username '{0}' from database.", userName), "Exceptions", TraceEventType.Error, ex);
            return null;
        }
    }

Here is call for methods

// get tenant
var crmTenant = CrmServiceHandlerFactory.GetInstance().Resolve<TenantManager>().GetTenantByUserName(userName);

I am new in programming, could you please help me to improve this code

I can see some room for improvement:

Most importantly, you shouldn't hide exceptions like this. You should most likely let the exceptions bubble up and let the called decide what to do when an exception happens. And if you return null, it means you will either have to add null checks everywhere, or you will get NullReferenceException instead of the exception that actually caused the problem, which makes it harder to debug.
You don't need to add items into the list one by one, you can do it all at once using AddRange(). Or even better, just use ToList():

return crmTenants.ToList();

This one is really minor, but the query from tenant in context.CrmTenant select tenant can be simplified to just context.CrmTenant, the LINQ doesn't do anything useful there.

#1 is by far the biggest issue, I think, because it shows an error in your design. #2 and #3 could just simplify your code a bit.


Circular buffer implementation for buffering TCP socket messages I have written my own for practice and also some tweaks for my specific requirement.  There is an insert which can copy a range (well array copy really) which I am hoping is more efficient than individual element insert.

Can anyone make any comments specifically as regards efficiency and code elegance.  I need to use this with Java 1.6 and am getting a warning on the array = newArray(size); line in the constructor.  But that is the only issue I am currently aware of.

import java.util.Arrays;  //copyOf

public class CircularBuffer<T> {
    private T[] array;  //storage
    private int put_idx, get_idx, count; //get/put & internal num elements counter
    private final int size;   //size of internal array storage

    public CircularBuffer(int size) {
        get_idx = 0;
        put_idx = 0;
        count   = 0;
        this.size = size;

        //Type safety: A generic array of T is created for a varargs parameter - investigate how to remove warning
        array = newArray(size);
    }

    //@SafeVarargs - ok in java 1.7
    static <T> T[] newArray(int length, T... array)
    {
        return Arrays.copyOf(array, length);
    }

    void reset(){
        get_idx=put_idx=count=0;
    }

    public boolean empty() { return count == 0; }

    public boolean full() { return count == size; }

    int get_count() { return count; }

    //inserts a single new element
    public void insert(T element)
    {
        if(full())
            throw new ArrayIndexOutOfBoundsException("buffer full");

        array[put_idx++] = element;
        put_idx %= size;
        ++count;
    }

    //insert array of specified length
    public void insert(T[] thearray) {
        if(thearray.length + count > size)
            throw new ArrayIndexOutOfBoundsException("insufficient space to insert array");

        //first copy to end - contiguous array copy
        int remaining = size - put_idx;
        if(remaining >= thearray.length) {
            System.arraycopy(thearray, 0, array, put_idx, thearray.length);
            put_idx += thearray.length;
            put_idx %= size;
        } else {
            System.arraycopy(thearray, 0, array, put_idx, remaining);
            int rest = thearray.length - remaining;
            System.arraycopy(thearray, remaining, array, 0, rest);
            put_idx = rest;
        }
        count += thearray.length;
    }

    //get the oldest element, move read pointer
    public T get()
    {
        if(empty())
            throw new ArrayIndexOutOfBoundsException("buffer empty");

        final T ret = array[get_idx++];
        get_idx %= size;
        --count;
        return ret;
    }
}

Code to exercise class:

public class TestCircularBuffer {
    static public void main(String args[]) {
        new TestCircularBuffer().go();
    }

    void go() {
        int SIZE = 1000;
        int arr[] = new int[SIZE];

        for(int i = 0; i < SIZE; ++i) {
            arr[i] = i;
        }

        CircularBuffer<Integer> cb = new CircularBuffer<Integer>(10);

        //exercise circ buffer
        int i = 0;
        while(i < SIZE) {
            while(!cb.full())
                cb.insert(arr[i++]);

            while(!cb.empty())
                System.out.print(cb.get() + " ");
        }

        //now exercise array insert
        System.out.println("\nrange insert test");
        Integer marr[] = {1,2,3,4};

        i = 0;
        while(i < 20) {
            //will only be able to fit 2 lots
            cb.insert(marr);
            cb.insert(marr);

            while(!cb.empty())
                System.out.print(cb.get() + " ");

            ++i;
        }
    }
}

A few minor notes:

Comments like the following are unnecessary:

private T[] array; // storage

You could call the field as storage and remove the comment.
A "solution" for the warning in the constructor:

@SuppressWarnings("unchecked")
private T[] newArray(int length) {
    return (T[]) new Object[length];
}

Reference: Java how to: Generic Array creation
camelCase variable and method names are more common in Java than underscored ones. (Code Conventions for the Java Programming Language, 9 - Naming Conventions)
From Code Complete, 2nd Edition, p761:

  Use only one data declaration per line

  [...]

  It’s easier to modify declarations because each declaration is self-contained.

  [...]

  It’s easier to find specific variables because you can scan a single
  column rather than reading each line. It’s easier to find and fix
  syntax errors because the line number the compiler gives you has
  only one declaration on it.

Setting default integer field values to zero is unnecessary in the constructor since zero is the default int value:

getIdx = 0;
putIdx = 0;
count = 0;

Does it make sense to create a CircularBuffer instance with zero size? If not check it and throw an IllegalArgumentException in the constructor.
The second insert method might be a premature optimization.
I'd throw IllegalStateException instead of ArrayIndexOutOfBoundsException. The clients of the class should not know that it uses an array (or anything else) for storage.
Here are a few unit tests:

@Test(expected = IllegalArgumentException.class)
public void testZeroSize() {
    new CircularBuffer<String>(0);
}

@Test
public void testIsEmptyOnNewBuffer() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    assertTrue(buffer.isEmpty());
}

@Test
public void testIsFullOnNewBuffer() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    assertFalse(buffer.isFull());
}

@Test
public void testIsEmpty() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    buffer.insert("a");
    assertFalse(buffer.isEmpty());
}

@Test
public void testIsFull() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    buffer.insert("a");
    buffer.insert("b");
    buffer.insert("c");
    assertTrue(buffer.isFull());
}

@Test
public void testIsEmptyAfterReset() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    buffer.insert("a");
    buffer.reset();
    assertTrue(buffer.isEmpty());
}

@Test
public void testIsFullAfterReset() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    buffer.insert("a");
    buffer.insert("b");
    buffer.insert("c");
    buffer.reset();
    assertFalse(buffer.isFull());
}

@Test
public void testIsEmptyOnUsedBuffer() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    buffer.insert("a");
    buffer.insert("b");
    buffer.insert("c");

    assertFalse("isEmpty 1", buffer.isEmpty());
    buffer.get();
    assertFalse("isEmpty 2", buffer.isEmpty());
    buffer.get();
    assertFalse("isEmpty 3", buffer.isEmpty());
    buffer.get();
    assertTrue("isEmpty 4", buffer.isEmpty());
}

@Test
public void testIsFullOnUsedBuffer() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    buffer.insert("a");
    buffer.insert("b");
    buffer.insert("c");
    buffer.get();

    assertFalse("isFull 1", buffer.isFull());
    buffer.get();
    assertFalse("isFull 2", buffer.isFull());
    buffer.get();
    assertFalse("isFull 3", buffer.isFull());
}

@Test
public void testInsert() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    buffer.insert("a");
    buffer.insert("b");
    buffer.insert("c");
}

@Test(expected = IllegalStateException.class)
public void testInsertFull() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    buffer.insert("a");
    buffer.insert("b");
    buffer.insert("c");
    buffer.insert("d");
}

@Test(expected = IllegalStateException.class)
public void testGetOnEmptyBuffer() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    buffer.get();
}

@Test(expected = IllegalStateException.class)
public void testGetOnEmptyUsedBuffer() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    buffer.insert("a");
    buffer.get();

    buffer.get();
}

@Test
public void testGet() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    buffer.insert("a");
    assertEquals("a", buffer.get());
    buffer.insert("b");
    buffer.insert("c");
}

@Test
public void testDelayedGet() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    buffer.insert("a");
    buffer.insert("b");
    buffer.insert("c");
    assertEquals("a", buffer.get());
}

@Test
public void testInsertAndGetTwice() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    buffer.insert("a");
    buffer.insert("b");
    buffer.insert("c");
    assertEquals("a", buffer.get());
    assertEquals("b", buffer.get());
    assertEquals("c", buffer.get());

    buffer.insert("d");
    buffer.insert("e");
    buffer.insert("f");
    assertEquals("d", buffer.get());
    assertEquals("e", buffer.get());
    assertEquals("f", buffer.get());
}

@Test
public void testGetAndInsert() {
    final CircularBuffer<String> buffer = new CircularBuffer<String>(3);
    buffer.insert("a");
    assertEquals("a", buffer.get());

    buffer.insert("b");
    assertEquals("b", buffer.get());

    buffer.insert("c");
    assertEquals("c", buffer.get());

    buffer.insert("d");
    buffer.insert("e");
    assertEquals("d", buffer.get());
    assertEquals("e", buffer.get());

    buffer.insert("f");
    assertEquals("f", buffer.get());

    assertTrue("empty", buffer.isEmpty());
}

(I've renamed a few methods.)


Using getopt parse and validate command line arguments I am learning getopt in Python to parse and validate command line inputs:

#!/usr/bin/python
'''\nMailing Script
Usage: python script.py -y <year> -c <type_user>"
Arguments:
 -y, --year       any value from 2008 to 2013
 -c, --campaign   any value in:
                    '80c', '80d', 'allowances', 'late_filing',
                    'exempt_income', 'other_sources'
Flags:
 -h      help
'''
import sys, getopt
first_yr, last_yr = (2008, 2013)
campaign_type = (
    '80c', '80d', 'allowances', 'late_filing',
    'exempt_income', 'other_sources',
)

def usage():
    print sys.exit(__doc__)

def parse_arguments(argv):
    try:
        opts, args = getopt.getopt(argv[1:], "y:c:", ["year=", "campaign="])
    except getopt.GetoptError:
        usage()
    for opt, arg in opts:
        if opt == '-h':
            usage()
        elif opt in ("-y", "--year"):
            target_year = arg
            # customized exception
            try:
                if not target_year in map(str, range(first_yr, last_yr+1)):
                    raise Exception()
            except:
                sys.exit("Argument Error: Invalid year passed. \n"
                         "One valid year is within (%s, %s)" % (first_yr, last_yr)
                )
        elif opt in ("-c", "--campaign"):
            campaign = arg.lower()
            try:
                if not campaign in campaign_type:
                    raise Exception()
            except Exception, e:
                sys.exit("Argument Error: Invalid 'Campaign type' passed. \n"
                         "A valid value can be any of %s" % str(campaign_type)
                )

    # to avoid 'UnboundLocalError' Exception when one argument not passed
    try:
        target_year, campaign
    except UnboundLocalError, u:
        usage()

    print 'Year : ', target_year
    print 'campaign: ', campaign

    # return argument values
    return (target_year, campaign)

if __name__ == "__main__":
    target_year, campaign = parse_arguments(sys.argv)

 Presently I didn't learn 16.4. argparse. I will learn it in the next step.

I want to improve my script as follows:

All arguments are mandatory. Presently if the user misses some argument then the script raises an UnboundLocalError exception. I then catch it and call usage(). Is it a correct way? Should I use len(argv) in some ways to handle this?
I check for correct values of arguments. If any argument value is wrong then I explicitly raise an exception and print an error message in the except clause. I am again not sure whether it's a preferable way to write this kind of code. I feel it adds one more level of nested blocks.

Some runs of this script:

$ python opt.py -y 2012 -c 80c
Year :  2012
campaign:  80c

Correct as I want:

$ python opt.py -year=2012
Argument Error: Invalid year passed.
One valid year is within (2008, 2013)

I think I should print message something like "insufficient arguments passed" or "use --year=2012 or -y 2012".

I think you have too many try/excepts.  More than one of them raise and catch their own exceptions. I've made some changes that improves readability.

For clarity the new code created a new exception class and raises it when appropriate.

Also using getopt's own handler for bogus input. The else in the main loop is redundant but it can also serve as a fallback in case you make a mistake by allowing an arg but forget to process it.

Pay attention to the wording:
if target_year not in map( ...

reads better than:
if not target_year in map( ...

#!/usr/bin/python
'''\nMailing Script
Usage: python script.py -y <year> -c <type_user>"
Arguments:
 -y, --year       any value from 2008 to 2013
 -c, --campaign   any value in:
                    '80c', '80d', 'allowances', 'late_filing',
                    'exempt_income', 'other_sources'
Flags:
 -h      help
'''
import sys
import getopt
first_yr, last_yr = (2008, 2013)
campaign_type = (
    '80c', '80d', 'allowances', 'late_filing',
    'exempt_income', 'other_sources',
)

class ArgumentError(Exception):
    pass

def usage():
    print sys.exit(__doc__)

def parse_arguments(argv):
    target_year = None
    campaign = None

    try:
        opts, args = getopt.getopt(argv[1:], "y:c:", ["year=", "campaign="])
    except getopt.GetoptError as err:
        # print help information and exit:
        print str(err)  # will print something like "option -a not recognized"
        usage()

    for opt, arg in opts:
        if opt == '-h':
            usage()

        elif opt in ("-y", "--year"):
            target_year = arg
            if target_year not in map(str, range(first_yr, last_yr + 1)):
                raise ArgumentError("Argument Error: Invalid year passed. \n "
                                    "One valid year is within (%s, %s)" % (first_yr, last_yr))

        elif opt in ("-c", "--campaign"):
            campaign = arg.lower()
            if campaign not in campaign_type:
                raise ArgumentError("Argument Error: Invalid 'Campaign type' passed. "
                                    "A valid value can be any of %s" % str(campaign_type))
        else:
            raise ArgumentError("Bad argument: I don't know what %s is" % arg)

    if target_year is None or campaign is None:
        raise ArgumentError("You need to supply both -y and -c")

    print 'Year : ', target_year
    print 'campaign: ', campaign

    # return argument values
    return target_year, campaign

if __name__ == "__main__":
    target_year, campaign = parse_arguments(sys.argv)


Binary search on string in alphabetical order The code for a binary search on an ordered string by alphabetical orders seems to work.  I am wondering if there is a better way to write this.

function binary_search(arr, letter){
    var first = 0;
    var middle = Math.floor(arr.length/2);
    var last = arr.length -1;

    while(true){
        var test = arr[last];
        if(arr[middle] === letter){
            return true
        }else if(arr[first] >= letter && letter < arr[middle]){
            last = middle -1;
            middle = Math.floor((first + last)/2);
        }else if (arr[middle] < letter && letter <= arr[last]){
            first = middle +1;
            middle = Math.floor((first + last)/2);
        }else {
            return false;
        }
    }
}

var str ="abcdefg"
console.log(binary_search(str, "g"));
console.log(binary_search(str, "a"));
console.log(binary_search(str, "e"));
console.log(binary_search(str, "z"));

Binary search is usually simpler:

function binarySearch(haystack, needle) {
    var a = 0;
    var b = haystack.length - 1;
    if (needle < haystack[0] || needle > haystack[b]) {
        return false;
    }
    while (a < b - 1) {
        var c = (a + b) / 2 |0;
        if (needle < haystack[c]) {
            b = c;
        } else {
            a = c;
        }
    }
    return haystack[a] === needle || haystack[a+1] === needle;
}


Basic Countdown Timer Below is a class,Countdown, that takes seconds as input from an entry, and starts counting down when the "Start" button is pressed, while displaying the time left on a label:

import tkinter as tk
import datetime

class Countdown(tk.Frame):
    '''A Frame with label to show the time left, an entry to input the seconds to count
    down from, and a start button to start counting down.'''
    def __init__(self, master):
        super().__init__(master)
        self.create_widgets()
        self.show_widgets()
        self.seconds_left = 0
        self._timer_on = False

    def show_widgets(self):

        self.label.pack()
        self.entry.pack()
        self.start.pack()

    def create_widgets(self):

        self.label = tk.Label(self, text="00:00:00")
        self.entry = tk.Entry(self, justify='center')
        self.entry.focus_set()
        self.start = tk.Button(self, text="Start", command=self.start_button)

    def countdown(self):
        '''Update label based on the time left.'''
        self.label['text'] = self.convert_seconds_left_to_time()

        if self.seconds_left:
            self.seconds_left -= 1
            self._timer_on = self.after(1000, self.countdown)
        else:
            self._timer_on = False

    def start_button(self):
        '''Start counting down.'''
        self.seconds_left = int(self.entry.get())   # 1. to fetch the seconds
        self.stop_timer()                           # 2. to prevent having multiple
        self.countdown()                            #    timers at once

    def stop_timer(self):
        '''Stops after schedule from executing.'''
        if self._timer_on:
            self.after_cancel(self._timer_on)
            self._timer_on = False

    def convert_seconds_left_to_time(self):

        return datetime.timedelta(seconds=self.seconds_left)

if __name__ == '__main__':
    root = tk.Tk()
    root.resizable(False, False)

    countdown = Countdown(root)
    countdown.pack()

    root.mainloop()

Review Concern(s):

My main concern is to code in an easy to read, efficient, and well-structured manner while still learning the language and concepts such as OOP. Feel free to mention the tiniest issue or improvement that comes to your mind, as I am a beginner and I probably need it.

Note: This was an answer in SO.

Very clean code indeed, there's not much to be said here.

PEP8

I'm sure you've been lectured on this before, but there's an official style guide for Python called PEP8. It states that module-level class definitions should be separated by two lines:

import tkinter as tk
import datetime

class Countdown(tk.Frame):
    ...

You also leave the first line of every method blank (or make it a docstring). If a function is undocumented, I wouldn't bother adding a blank line:

def foo():
    do_stuff()

If you decide to add a docstring, I'd recommend the following structure:

< function definition >
    < docstring (may span multiple lines) >
    < blank line >
    < function body >

For example:

def bar():
    """I'm a docstring!
    Blah, blah.
    """

    do_stuff()

Private attributes

It's good that you marked _timer_on as private. Why not do the same for seconds_left and the helper methods? This clearly tells other developers: 'I'm not part of the API. Don't rely on me for backwards compatibility.'. (Added bonus: you don't have to document private functions).

class Countdown(tk.Frame):
    ...

    def show_widgets(self):
        < docstring >

        ...

    def create_widgets(self):
        < docstring >

        ...

    def _countdown(self):
        ...

    def _start_button(self):
        ...

    def _stop_timer(self):
        ...

    def _convert_seconds_left_to_time(self):
        ...

Basically, any method, property, or attribute that isn't useful for end users, should be private.

OOP

convert_seconds_left_to_time could be a staticmethod. A static method does not receive an implicit first argument. You should make a method static if it doesn't interact with the instance of the class, but is still strongly related to the class.

I'd then rename it to _get_timedelta_from_seconds:

class Countdown(tk.Frame):
    ...

    def countdown(self):
        """Update the label based on the time left."""

        self.label["text"] = self._get_timedelta_from_seconds(self.seconds_left)

    @staticmethod
    def _get_timedelta_from_seconds(seconds):
        return datetime.timedelta(seconds=seconds)


Modern, C++ compliant, merge sort This is a kind of follow up to my previous sort implementation review with specific questions to merge sort, and Modern C++ idioms.

template<typename InputIt1,
         typename InputIt2,
         typename OutputIt,
         typename Comparator = std::less<typename std::iterator_traits<InputIt1>::value_type >>
void merge(InputIt1 first1,
           InputIt1 last1,
           InputIt2 first2,
           InputIt2 last2,
           OutputIt out,
           Comparator cmp = Comparator())
{
    while (first1 != last1 || first2 != last2)
    {
        if (first2 == last2 || first1 != last1 && cmp(*first1, *first2))
        {
            *out = *first1++;
        }
        else
        {
            *out = *first2++;
        }
        ++out;
    }
}

template<typename BidirIt,
         typename Comparator = std::less<typename std::iterator_traits<BidirIt>::value_type >>
void sort_merge(BidirIt first,
                BidirIt last,
                Comparator cmp = Comparator())
{
    typedef std::iterator_traits<BidirIt>::difference_type dt;
    typedef std::iterator_traits<BidirIt>::value_type vt;

    dt size = std::distance(first, last);
    if (size > dt(1))
    {
        auto middle = first + size / dt(2);

        std::vector<vt> left(first, middle);
        std::vector<vt> right(middle, last);

        auto left_begin = left.begin(), left_end = left.end();
        auto right_begin = right.begin(), right_end = right.end();

        sort_merge(left_begin, left_end);
        sort_merge(right_begin, right_end);

        ::merge(left_begin, left_end, right_begin, right_end, first);
    }
}

I am looking for any performance optimizations I may have missed in the algorithm itself, as well as Modern C++ constructs I may not be abiding by for this example.

I looked into in-place merging, however, all of the solutions I have found for in-place merging perform worse. Namely, substituting a sort_insertion for the merge operation. However, if I were to make an in-place sort_merge, I would start a separate topic as it's considered a completely separate algorithm.

Prefer to use move rather than copy semantics

If your object is an int then it makes no difference. But some objects are more expensive to copy (such as std::string). If you can move rather than copy you will get much better performance with more complex objects.

*out = *first1++;

// Just add a call to std::move
*out = std::move(*first1++);

Too many lines of parameters

This is bit difficult to say this is required or perfect and you can make arguments both ways. But one parameter per line seems a bit over verbose. I prefer to group my parameters into logical groups (tools don't like this as they can't apply logical as it is situation specific).

But I personally think that for readability it makes sense to put parameters in logical groups.

(see below)

Member types names

Technically this is illegal:

typedef std::iterator_traits<BidirIt>::difference_type dt;

The compiler can not tell if difference_type is a type or a member variable until it knows the type of BidirIt. So you need to tell it that it is a typename.

typedef typename std::iterator_traits<BidirIt>::difference_type dt;
 //     ^^^^^^^^  required for standards conformance

Type names

Types are important in C++. Try and use names that help identify the type (be a bit verbose). But also it is a sort of convention that user defined types have an initial upper case letter (so that they can be easily spotted from objects).

typedef std::iterator_traits<BidirIt>::difference_type dt;

// Here I did not like the short name:
typedef typename std::iterator_traits<BidirIt>::difference_type DiffType;

Don't pass comparator to merge by value

The merge operation is not supposed to be done in isolation. It is always called from the sort_merge() function. So there is no need to create a comparator for each call to merge() just create it once in sort_merge() and pass by const reference to all calls to merge():

void merge(InputIt1 first1, InputIt1 last1,
           InputIt2 first2, InputIt2 last2,
           OutputIt out,
           Comparator const& cmp)

Non optimal implementation

while (first1 != last1 || first2 != last2)
{
    if (first2 == last2 || first1 != last1 && cmp(*first1, *first2))
    {
        *out = *first1++;
    }
    else
    {
        *out = *first2++;
    }
    ++out;
}

Better to write as this:

// Test your constraints here.
while (first1 != last1 && first2 != last2)
{
    // The comparison test then becomes much simpler.
    *out = std::move(cmp(*first1, *first2)
                         ? *first1++
                         : *first2++);
    ++out;
}
// Then the final optimized copy
// Only one of these loops will execute.
std::move(first1, last1, out);
std::move(first2, last2, out);

Space optimization

Creating the copy in sort_merge()

    std::vector<vt> left(first, middle);
    std::vector<vt> right(middle, last);

Means that you use more memory because you have a copy in this iteration then make a copy in each recursive call below this one and they are all active at the same time.

If you do the recursive sort_merge() in place. Then in the merge() call perform the copy then you get a much better space usage (as you only have one copy at the current iteration active).

Result

You will notice I have slightly altered the merge. We can make several assumptions about the inputs (because it is only called from sort_merge()):

There is only one type of input iterator.

Because we know we are sorting two halves of the same container.

There is no output iterator

as the merge is done in place.

The comparator does not need a default type.

this is done as part of the sort_merge template.

I don't pass two ranges.

I use begin/mid/end

Code: merge

template<typename I, typename Comparator>
void merge(I first, I mid, I last, Comparator const& cmp)
{
    typedef typename std::iterator_traits<I>::difference_type DiffType;
    typedef typename std::iterator_traits<I>::value_type      ValueType;

    DiffType                size1 = std::distance(first, mid);
    DiffType                size2 = std::distance(mid, last);

    std::vector<ValueType>  hold1;
    std::vector<ValueType>  hold2;
    hold1.reserve(size1);
    hold2.reserve(size2);

    std::move(first, mid,  std::back_inserter(hold1));
    std::move(mid,   last, std::back_inserter(hold2));

    auto first1 = std::begin(hold1);
    auto last1  = std::end(hold1);
    auto first2 = std::begin(hold2);
    auto last2  = std::end(hold2);
    auto out    = first;

    while (first1 != last1 && first2 != last2)
    {
        *out = std::move(cmp(*first1, *first2)
                             ? *first1++
                             : *first2++);
        ++out;
    }
    std::move(first1, last1, out);
    std::move(first2, last2, out);
}

Code: sort_merge

template<typename I,
         typename Comparator = std::less<typename std::iterator_traits<I>::value_type>>
void sort_merge(I first,
                I last,
                Comparator cmp = Comparator())
{
    typedef typename std::iterator_traits<BidirIt>::difference_type DiffType;

    DiffType size = std::distance(first, last);
    if (size > DiffType(1))
    {
        auto middle = first + size / 2;
        auto midIter = begin;
        std::advance(midIter, middle);

        sort_merge(first, midIter, cmp);
        sort_merge(midIter, last,  cmp);

        merge(first, midIter, end, cmp);
    }
}


C++ multithreading logger class I have designed a logger class to log messages to a file. It uses an independent thread to log the messages save to a queue previously using the main thread. I want to receive reviews about it.
#include <iostream>
#include <queue>
#include <mutex>
#include <condition_variable>
#include <fstream>
#include <atomic>
#include <string>
using namespace std::chrono_literals;

class Logger
{
    std::mutex queueMutex;
    std::condition_variable condVar;
    std::queue<std::string> messagesQueue;
    std::thread loggingThread;//background process launcher
    std::atomic_bool exit = false;//safety condition
    void processEntries()//the background process
    {
        // Open log file.
        std::ofstream logFile("log.txt");
        if (logFile.fail()) {
            std::cerr << "Failed to open logfile." << std::endl;
            return;
        }

        // Start processing loop. It process only one message for each iteration
        // to gain more performance
        while (!exit) {
            std::unique_lock lock(queueMutex);

            // Wait for a notification and don't wakeup unless the queue isn't empty.
            condVar.wait(lock, [this]{return !messagesQueue.empty();});

            //log to the file
            logFile << messagesQueue.front() << std::endl;
            messagesQueue.pop();
        }

        //At the end, if the queue has some messages. here you don't need mutexes
        // because you have reached the destructor i.e you won't enqueue any messages anymore
        while(exit && !messagesQueue.empty()){

            //log to the file
            logFile << messagesQueue.front() << std::endl;
            messagesQueue.pop();
        }

    }

public:
    Logger()
    {
        //the default ctor launches the background process task
        loggingThread = std::thread{ &Logger::processEntries, this };
    }
    Logger(const Logger& src) = delete;
    Logger& operator=(const Logger& rhs) = delete;

    //logs the messages to the queue
    void log(std::string_view entry)
    {
        std::unique_lock lock(queueMutex);
        messagesQueue.push(std::string(entry));
        condVar.notify_all();
    }

    ~Logger()
    {
        exit = true;
        loggingThread.join();
    }

};

int main(){

    Logger lg;
    for(int i = 1;i < 10000;i++){
        lg.log("This is the message number " + std::to_string(i));
    }

    std::this_thread::sleep_for(10ms);

    for(int i = 10000;i < 20001;i++){
        lg.log("This is the message number " + std::to_string(i));
    }

}

Pretty good for a beginning, but I would have a lot of feature requests if I were a user.

Allow the user to provide a full file specification so that the file appears where the user expects it.
The logger should get the day and time of day and report that in each message.
The logger should be initialized with the program name so that the all the messages contain the program name, this way if multiple processes are writing to the same file one will know which process made the log entry.

Make it into a library by providing a header file and a C++ source file.


Listing products from a database I've this function to list products from database by category. I'm using prepared statements, but wouldn't this make it pretty pointless to use them?

function getProducts($category,$search_param,$pdo){
      $query = "SELECT * FROM renkaat
        INNER JOIN ajoneuvotyypit ON tyyppiID = ajoneuvotyyppiID
        INNER JOIN vuodenajat ON vuodenaikaID = renkaat.vuodenaika
        INNER JOIN valmistajat ON renkaat.valmistaja = valmistajat.valmistajaID
        WHERE renkaat.$category= ?
        ";
    $smt = $pdo->prepare($query);
    $smt->execute(array($search_param));

      //do stuff
  }

And the function is simply called like this, with no escaping done:

getProducts($_GET['category'],$_GET['search_param'],$pdo);

Should I escape the $category?

Yes, $category in your query string, coming directly from $_GET is a problem.

You should validate $category, and that's not exactly the same as escaping. I assume you have a limited set of valid categories in your database, so validation should be something like:

$valid_categories = array("sector", "industry", "company");
if (!in_array($category, $valid_categories)) {
    // no no
}

As for the other part of your question, paraphrased:

  Is it pointless to use prepared statements if there is a variable part in the query?

It's not pointless, because every time you run the query for the same $category value, it can benefit from the previously prepared statement.


Throw an exception to control the flow of code execution There is a recommendation not to use an Exception to control the flow of execution. This gives me a reason for doubt when I do something like this:

 public void initApplication(String input) {
        try {
            MyClass mClass = new mClass(input);
            saveToDatabase(mClass);
            updateSettingsTheAppIsReady();
        } catch (CancelPreparationsException e) {
            showErrorMessageToUser();
            doRollbacks();
        }
    }

mClass instantiation, save operation to a database, settings update may go wrong, each of them throw their own exceptions, which I catch within the methods/classes and throw my custom exception to stop the initApplication() method execution.

This sounds an easier way than having preconditions checking like:

Create MyClass object.
Check the MyClass object is created then save it to db.
Ensure the object was saved in db then update settings.

Is it really not recommended way of using the Exception to control the flow? Thank you.

I would say that this isn't controlling the flow of your application.  This is handling an exceptional circumstance (i.e., failure of your code to perform what as it's meant to), which is what Exceptions are meant to accommodate.

The recommendation is more for circumstances like this:

public boolean itemExists(List<MyObject> list, MyObject object) {
    try {
        for(MyObject thisOne : list) {
            if(thisOne.getAttribute() == object.getAttribute()) {
                throw new Exception("found it");
            }
        }
    }
    catch(Exception e) {
        if(e.getMessage().equals("found it")) {
            return true;
        }
        throw e;
    }
    return false;
}

This controls the flow of the program based on normal execution.  It should be pretty easy to see why this isn't ideal, simply for code readability, not considering aspects like performance, style, and best practices.

What you have in the above code is fine.  Throwing Exceptions for errors is what they're for.  If you're throwing Exception simply to break out of loops or functions or are using them during the "intended" execution of your code, that's when you're going down the wrong path.


Python OOP - creating library I have been learning programming in Python recently and I got a task to program a library in OOP. You can add books to a library, search for a specific book according to the ISBN, name of the author or search books which cost less than price. Everything seems to be okay, but I am new to OOP so I would like to hear some advice on whether it is programmed a "good way" or not.

class Library(object):
    books = []
    written_by_author = []
    books_under_price = []

    def __init__(self, name, author, isbn, price):
        self.name = name
        self.author = author
        self.isbn = isbn
        self.price = price

    def addBook(self):
        Library.books.append((self.name,self.author,self.isbn,self.price))

    def searchBookISBN(self,isbn):
        for book in Library.books:
            if book[2]==isbn:
                return book

    def searchBookAuthor(self,author):
        for book in Library.books:
            if book[1]==author:
                Library.written_by_author.append(book)
        return Library.written_by_author

    def searchUnderPrice(self,price):
        for book in Library.books:
            if book[3]<price:
                Library.books_under_price.append(book)
        return Library.books_under_price

book = Library('Geometry', 'Jeff Potter', '0596805888', 22)
book.addBook()
book = Library('Math', 'George Harr', '0594805888', 15)
book.addBook()
book = Library('English', 'James Odd', '0596225888', 10)
book.addBook()
book = Library('Physics','Jeff Potter','0597884512',18)
print (book.searchBookISBN('0594805888'))
print (book.searchBookAuthor('George Harr'))
print (book.searchUnderPrice(20))

Your design doesn't follow best practice. Your Library class should create a library when you use __init__, rather than a book.
And so I'd split your code in half, create two classes; one for books, the other for the library.
You shouldn't mutate data on Library. This is as it mutates it on the class, rather than on the instance. And so if you make a new instance, then it will be tied to all the other instances.
When making the book class. This can significantly be simplified with collections.namedtuple. Such as:

Book = namedtuple('Book', 'name author ISBN price')

And so I'd change your code to:

from collections import namedtuple

class Library(object):
    def __init__(self):
        self.books = []

    def addBook(self, book):
        self.books.append(book)

    def searchBookISBN(self, ISBN):
        for book in self.books:
            if book.ISBN == ISBN:
                return book

    def searchBookAuthor(self, author):
        written_by_author = []
        for book in self.books:
            if book.author == author:
                written_by_author.append(book)
        return written_by_author

    def searchUnderPrice(self, price):
        books_under_price = []
        for book in self.books:
            if book.price < price:
                books_under_price.append(book)
        return books_under_price

Book = namedtuple('Book', 'name author ISBN price')

library = Library()
library.addBook(Book('Geometry', 'Jeff Potter', '0596805888', 22))
library.addBook(Book('Math', 'George Harr', '0594805888', 15))
library.addBook(Book('English', 'James Odd', '0596225888', 10))
library.addBook(Book('Physics', 'Jeff Potter', '0597884512', 18))
print(library.searchBookISBN('0594805888'))
print(library.searchBookAuthor('George Harr'))
print(library.searchUnderPrice(20))

After this, I'd recommend:

You read and follow PEP 8, so that your code is easier to read.
You learn list comprehensions

Which would further improve your code to:

class Library(object):
    def __init__(self):
        self.books = []

    def add_book(self, book):
        self.books.append(book)

    def book_with_ISBN(self, ISBN):
        for book in self.books:
            if book.ISBN == ISBN:
                return book

    def books_by_author(self, author):
        return [book for book in self.books if book.author == author]

    def books_under_price(self, price):
        return [book for book in self.books if book.price < price]


Switching functionality on and off in a Scheduled Thread To illustrate how to do leader election with Apache Zookeeper I've created an straightforward application and I would like to have to threading part of the application reviewed.
I've created a simple class (Speaker) which outputs an unique identifier to a file on a regular interval. I've implemented the Runnable interface and start it with a ScheduledExecutorService with a fixed delay like this:

scheduler.scheduleWithFixedDelay(speaker, 0, delay, TimeUnit.MILLISECONDS);

The Speaker class also implements a Listener interface so it can register itself to a Monitor (running in a separate thread, communicating to the Zookeeper server)  which can enable or disable the output to the file.

My review question (besides general code review):  Is this the best way to start and stop functionality in a scheduled thread?

Speaker & Speaker Server:

Complete project: https://github.com/cyberroadie/zookeeper-leader

Code explanation: http://cyberroadie.wordpress.com/2011/11/24/implementing-leader-election-with-zookeeper/

public class Speaker implements Runnable, NodeMonitor.NodeMonitorListener {

    private String message;
    private String processName;
    private long counter = 0;
    private volatile boolean canSpeak = false;

    public Speaker(String message) throws IOException, InterruptedException, KeeperException {
        this.message = message;
         this.processName = getUniqueIdentifier();
    }

    private static String getUniqueIdentifier() {
        String processName = ManagementFactory.getRuntimeMXBean().getName();
        String processId = processName.substring(0, processName.indexOf("@"));
        return "pid-" + processId + ".";
    }

    public void run() {
        try {
            if (canSpeak) {
                handleTask();
            }
        } catch (IOException e) {
            e.printStackTrace();
            System.exit(1);
        }
    }

    public void handleTask() throws IOException {
        FileWriter fstream = new FileWriter("out.txt");
        BufferedWriter out = new BufferedWriter(fstream);
        out.write(message + ": " + counter++ + " " + processName + "\n");
        out.close();
    }

    @Override
    public void startSpeaking() {
        this.canSpeak = true;
    }

    @Override
    public void stopSpeaking() {
        this.canSpeak = false;
    }

    @Override
    public String getProcessName() {
        return processName;
    }
}

Speaker Server:

public class SpeakerServer {

    final static Logger logger = LoggerFactory.getLogger(SpeakerServer.class);

    private static final ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);
    private static NodeMonitor monitor;

    private static void printUsage() {
        System.out.println("program [message] [wait between messages in millisecond]");
    }

    public static void main(String[] args) {
        if (args.length < 2) {
            printUsage();
            System.exit(1);
        }

        long delay = Long.parseLong(args[1]);

        Speaker speaker = null;

        try {
            speaker = new Speaker(args[0]);
            monitor = new NodeMonitor();
            monitor.setListener(speaker);
            monitor.start();
        } catch (Exception e) {
            e.printStackTrace();
            System.exit(1);
        }
        scheduler.scheduleWithFixedDelay(speaker, 0, delay, TimeUnit.MILLISECONDS);
        logger.info("Speaker server started with fixed time delay of " + delay + " milliseconds.");
    }
}

you could also look at using a fixed thread pool. That way, if one thread becomes blocked for some reason, say the disk became full, the threads would wait in a queue until the blocked thread terminated. That way you wouldn't loose any unique id's or run into memory problems if too many threads became blocked.

Hope that helps,

Ross


Bash evaluation using crazy passwords I have some code that is loading passwords from AWS SSM and then using them through the script. I am really concerned that some funky password character is going to be used that is going to get interpreted/escaped/munged by bash. The code in question is below. Are there any obvious problems here? Can I improve this so that no password can trip this up?

updateEnvFile() {
    local user=$1
    local pass=$2
    local host=$3

    cat "${bin}/../environments/development.properties" |
    sed "s;url=jdbc:mysql://localhost/mysql;url=jdbc:mysql://$host/mysql;" |
    sed "s/username=root/username=$user/" | sed "s/password=my-secret-pw/password=$pass/"
}

updateConfig() {
    local dbs=$1
    for db in ${dbs}; do
        local user=${params["/databases/migrate/${env}/${db}.user"]}
        local pass=${params["/databases/migrate/${env}/${db}.password"]}
        local host=${params["/databases/migrate/${env}/${db}.host"]}
        updateEnvFile "${user}" "${pass}" "${host}" > "${bin}/../environments/${db}.properties"
    done
}

That cat is entirely unnecessary. Also, multiple pipes to sed are redundant.

sed by design can take a single long expression and process the data.

As for your delimiters, I'd suggest using non-printable characters when in sed:

SED_DELIM=$(echo -en "\001")

and then:

sed "s${SED_DELIM}url=jdbc:mysql://localhost/mysql${SED_DELIM}url=jdbc:mysql://$host/mysql${SED_DELIM}; s${SED_DELIM}username=root${SED_DELIM}username=$user${SED_DELIM}; s${SED_DELIM}password=my-secret-pw${SED_DELIM}password=$pass${SED_DELIM}" $file_name

Now, that looks quite long. You can evaluate individual expressions:

sed -e "s${SED_DELIM}url=jdbc:mysql://localhost/mysql${SED_DELIM}url=jdbc:mysql://$host/mysql${SED_DELIM};" \
-e "s${SED_DELIM}username=root${SED_DELIM}username=$user${SED_DELIM};" \
-e "s${SED_DELIM}password=my-secret-pw${SED_DELIM}password=$pass${SED_DELIM}" \
$file_name

I'll go ahead and also use back-references:

sed -e "s${SED_DELIM}\(url=jdbc:mysql://\)localhost\(/mysql\)${SED_DELIM}\1$host\2${SED_DELIM};" \
-e "s${SED_DELIM}\(username=\)root${SED_DELIM}\1$user${SED_DELIM};" \
-e "s${SED_DELIM}\(password=\)my-secret-pw${SED_DELIM}\1$pass${SED_DELIM}" \
$file_name


Changing a single jQuery plugin option on window resize I'm looking to change a single jQuery plugin option when the window hits certain breakpoints.

The code below works, however I'm conscious of repeating code and feel there is a more elegant way writing code for examples like this.

My initial thoughts would be to store the first call in a variable.

$(window).smartresize(function () {

        if ( config.wWidth >= 768 && config.wWidth <= 1024 ) {

            $('#va-accordion').vaccordion({
                accordionW       : config.wWidth,
                accordionH       : config.wHeight,
                visibleSlices    : 2,
                expandedHeight   : 600,
                expandedWidth    : 1000,
                contentAnimSpeed : 200
            });

        } else if ( config.wWidth < 768 ) {

            $('#va-accordion').vaccordion({
                accordionW       : config.wWidth,
                accordionH       : config.wHeight,
                visibleSlices    : 1,
                expandedHeight   : 600,
                expandedWidth    : 1000,
                contentAnimSpeed : 200
            });

        } else {

            $('#va-accordion').vaccordion({
                accordionW       : config.wWidth,
                accordionH       : config.wHeight,
                visibleSlices    : 3,
                expandedHeight   : 600,
                expandedWidth    : 1000,
                contentAnimSpeed : 200
            });
        }

    });

    $(window).trigger("smartresize");

$(window).smartresize(function () {

  var chosenSettings;

  // We could map out the defaults first.
  var defaults = {
    accordionW: config.wWidth,
    accordionH: config.wHeight,
    visibleSlices: 0,
    expandedHeight: 600,
    expandedWidth: 1000,
    contentAnimSpeed: 200
  };

  // Then, depending on the conditions, create an object of the modifications
  // This way, it's easier to add in more differences.
  if (config.wWidth >= 768 && config.wWidth <= 1024) {
    chosenSettings = {visibleSlices: 2};
  } else if (config.wWidth < 768) {
    chosenSettings = {visibleSlices: 1};
  } else {
    chosenSettings = {visibleSlices: 3};
  }

  // Then use extend to create an object with the chosen settings
  var settings = $.extend({}, defaults, chosenSettings);
  $('#va-accordion').vaccordion(settings);

// Assuming smartresize returns the same object, we can chain trigger.
}).trigger('smartresize');


Displaying colorful shapes to the user I have a program that displays colorful shapes to the user. It is designed so that it is easy to add new shapes, and add new kinds of views.

Currently, I have only two shapes, and a single text-based view. In the near future, I'm going to implement a Triangle shape and a BezierCurve shape.

I'm also going to implement these views:

GraphicalView - uses a graphics library to render shapes on screen.
OscilloscopeView - draws the shapes on an oscilloscope.
DioramaView - a sophisticated AI directs robotic arms to construct the scene using construction paper, string, and a shoebox.

The MVC pattern is essential here, because otherwise I'd have a big intermixed tangle of oscilloscope and AI and Graphics library code. I want to keep these things as separate as possible.

#model code

class Shape:
    def __init__(self, color, x, y):
        self.color = color
        self.x = x
        self.y = y

class Circle(Shape):
    def __init__(self, color, x, y, radius):
        Shape.__init__(self, color, x, y)
        self.radius = radius

class Rectangle(Shape):
    def __init__(self, color, x, y, width, height):
        Shape.__init__(self, color, x, y)
        self.width = width
        self.height = height

class Model:
    def __init__(self):
        self.shapes = []
    def addShape(self, shape):
        self.shapes.append(shape)

#end of model code

#view code

class TickerTapeView:
    def __init__(self, model):
        self.model = model
    def render(self):
        for shape in self.model.shapes:
            if isinstance(shape, Circle):
                self.showCircle(shape)
            if isinstance(shape, Rectangle):
                self.showRectangle(shape)
    def showCircle(self, circle):
        print "There is a {0} circle with radius {1} at ({2}, {3})".format(circle.color, circle.radius, circle.x, circle.y)
    def showRectangle(self, rectangle):
        print "There is a {0} rectangle with width {1} and height {2} at ({3}, {4})".format(rectangle.color, rectangle.width, rectangle.height, rectangle.x, rectangle.y)

#end of view code

#set up

model = Model()
view = TickerTapeView(model)

model.addShape(Circle   ("red",    4,   8,   15))
model.addShape(Circle   ("orange", 16,  23,  42))
model.addShape(Circle   ("yellow", 1,   1,   2))
model.addShape(Rectangle("blue",   3,   5,   8,   13))
model.addShape(Rectangle("indigo", 21,  34,  55,  89))
model.addShape(Rectangle("violet", 144, 233, 377, 610))

view.render()

I'm very concerned about the render method of TickerTapeView. In my experience, whenever you see code with a bunch of isinstance calls in a big if-elseif block, it signals that the author should have used polymorphism. But in this case, defining a Shape.renderToTickerTape method is forbidden, since I have resolved to keep the implementation details of the view separate from the model.

render is also smelly because it will grow without limit as I add new shapes. If I have 1000 shapes, it will be 2000 lines long.

Is it appropriate to use isinstance in this way? Is there a better solution that doesn't violate model-view separation and doesn't require 2000-line if blocks?

Here's one approach:

SHAPE_RENDERER = {}

def renders(shape):
    def inner(function):
        SHAPE_RENDERER[shape] = function
        return function
    return inner

@renders(Circle)
def draw_circle(circle, view):
    ...

@renders(Triangle)
def draw_triangle(triangle, view):
    ....

def render_shape(shape, view):
    SHAPE_RENDERER[shape.__class__](shape, view)


Parallelly resizing mutiple photos This code snippet is supposed to utilize async await to resize a photo file into multiple sizes in parallel. I just want to know if I am doing it in the right way. I would love to hear some ideas on how to report resize progress on file like how many of them completed.

public class PhotoProcessor : IPhotoProcessor
{
    public void Resize(string filePath, string dropLocation, List<Size> sizes)
    {
        Parallel.ForEach(sizes, async (size) =>
        {
            var fileName = Path.GetFileName(filePath);
            var result = await Resize(filePath, dropLocation, size);
            Console.WriteLine($"Done {result} Resizing {fileName} to {size}");
        });
    }

    public async Task<bool> Resize(string filePath, string dropLocation, Size size)
    {
        if (!File.Exists(filePath))
        {
            throw new FileNotFoundException($"File {filePath} not found");
        }

        using (var image = new MagickImage(filePath))
        {
            var fileName = Path.GetFileNameWithoutExtension(filePath);
            var extension = Path.GetExtension(filePath);
            var newFileName = $"{fileName}_{size}{extension}";
            var newFilePath = $"{dropLocation}{newFileName}";

            var magickSize = new MagickGeometry(size.W, size.H);
            magickSize.IgnoreAspectRatio = false;
            await Task.Run(() =>
            {
                image.Resize(magickSize);
                image.Write(newFilePath);
            });
        }
        return true;
    }
}

public void Resize(string filePath, string dropLocation, List<Size> sizes)

Why List<>? Following the principle of coding to the interface rather than the implementation, this should probably be IList<> or even IReadOnlyList<>.

        Parallel.ForEach(sizes, async (size) =>
        {
            var fileName = Path.GetFileName(filePath);
            var result = await Resize(filePath, dropLocation, size);
            Console.WriteLine($"Done {result} Resizing {fileName} to {size}");
        });

Why async/await inside Parallel.ForEach? If each size is handled on a different thread, there's nothing to be gained by using await.

            var newFilePath = $"{dropLocation}{newFileName}";

Path.Combine exists for a reason: using that rather than manual concatenation will give more robust code.

            await Task.Run(() =>
            {
                image.Resize(magickSize);
                image.Write(newFilePath);
            });

Pretend for a moment that we're not using Parallel.ForEach. The benefit of async/await with something like image processing is that you can keep the CPU busy even when one of the coroutines is blocked on I/O. But half of the I/O has already happened (maybe more if you're always scaling down rather than up). If you want to make an async version, the image load should be async.


Search and sort customers from JSON file I have just completed my first JavaScript/jquery project. It searches customers from .JSON file and sorts them by name or ID. Everything just works fine. The thing is, I am not sure that I have understood all the functions and methods that I have written. Could anyone tell me if this is the best way to read .JSON file and going through the queries that I have written below?

<!DOCTYPE html>

    <title>Toolbox Search</title>

    <link href="autocomplete/easy-autocomplete.min.css" rel="stylesheet" />
    <link rel="stylesheet" href="http://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css">
    <link href="custom.css" rel="stylesheet" />

                <input type="text" id="KUNDE" size="50" placeholder="Search by name." />

                <button class="btn btn-default" type="button" id="buton" value="search" onclick="find();">Search</button>

        <button id="button"class="btn btn-default">
            Clean Results
        </button>

 © Copyright 2016
 .

Your code is really hard to read as noted by @Sumurai8.  That is the first thing that jumps out at you, which is bad.
Consider placing all your javascript code within $(document).ready() handler.  There is no reason any of this code needs to operate in global scope.
Cache your JSON data. There seems to be no reason to continually call getJSON() if you are calling a static file anyway.
Don't keep re-querying the DOM.  If you ever have a case where a selector is going to be used more than once during code operations on a page, you should consider storing the jQuery resource in a variable, so as to minimize queries against the DOM. Of course there is is a trade-off of increased memory consumption for this performance gain, but for most applications, that should not be a concern. Particularly good candidates for this approach are those jQuery selectors that happen within event-handler callbacks that get called repeatedly.
Your filtering condition in the getJSON() success handler does not seem appropriate to me.  I think perhaps the JSON data structure is bad.  Why would you need to iterate over the whole array of searchable objects to figure out which ones meets the keyword criteria?  Why are you then only working with the first returned result that meets this criteria? The data structure should support your desired lookup case. I would consider building a key-based data structure where you combine the keys from the object id and name values and references to applicable objects against that key.  Or alternately have two different data structures - one indexed by id and one indexed by name and make two lookups in response to each find() activity.  Either approach would give operational complexity of O(1) in that the find() operation would take a (relatively) fixed amount of time to execute regardless of the size of the data set you are searching against (once data is loaded).  Your current approach is O(n), such that operation takes linearly longer to complete as the size of your dataset being searched increases. There is no reason at all to make this method perform so poorly.  Between your lack of caching of the data structure and linear complexity, you have a really poorly performing find() function which does all of the following EVERY time the function is called:

You re-load JSON (perhaps via HTTP request, perhaps just from browser cache if lucky) - a costly operation where you read a potentially large string into memory.
You then deserialize that JSON into a javascript array of objects (again a costly operation for potentially large string)
You then iterate every member of that javascript to perform lookup.

You should be striving for a solution where:

Either prior to or with first request to find() you retrieve the JSON data source(s), deserialize them and store them in named javascript variables.
You have a data structure that allows O(1) lookup against the data set.
find() operation uses the stored data structure in javascript to operate against.


Generalizing std::minmax_element In the following answer, @Incomputable basically dared me to implement a generalization of std::minmax_element.

  Have you thought about creating a search function that finds N elements based on N predicates, and return an std::array with the iterators to found elements? I just believe that std::minmax_element can be generalized.

I'm happy to oblige:

#include <array>

template<typename IteT, typename... PredsT>
auto multi_search_elements(IteT begin, IteT end, PredsT const&... preds) {
    const std::size_t count = sizeof...(PredsT);
    std::array<IteT, count> selected;

    selected.fill(begin);
    if(begin != end) {
      for(++begin; begin != end; ++begin) {
        std::size_t i = 0;
        ( (selected[i] = preds(*selected[i], *begin) ? selected[i] : begin,
           ++i), ...);
      }
    }

  return selected;
}

Usage:

#include <iostream>
#include <vector>
#include <functional>

int main() {
  std::vector<int> vals = {1,2,3,4,5};

  auto found = multi_search_elements(vals.begin(), vals.end(), std::less<int>(), std::greater<int>());

  for(auto i : found) {
    std::cout << *i << std::endl;
  }
  return 0;
}

Edit: I understand this fold expression might look like voodoo to people who are not used to that type of code, so I'll elaborate on what's going on here for those of you who are curious:

The fold expression looks like this: (EXPR, ...), which will expand to EXPR, EXPR, EXPR, etc..., where the "binary" operator used is actually the comma operator, which effectively just sequences the expressions while leaving them unrelated.

After that, all I do is make EXPR be SUB_EXPR, ++i, so the whole thing expands as:

(SUB_EXPR, ++i), (SUB_EXPR, ++i), (SUB_EXPR, ++i), (SUB_EXPR, ++i), ...

Which is essentially:

selected[i] = preds_0(*selected[i], *begin) ? selected[i] : begin,
++i,
selected[i] = preds_1(*selected[i], *begin) ? selected[i] : begin,
++i,
selected[i] = preds_2(*selected[i], *begin) ? selected[i] : begin,
++i,
selected[i] = preds_3(*selected[i], *begin) ? selected[i] : begin,
++i

This code is pretty good, and there's very little to improve.  The two-line fold expression did benefit from the explanatory comment in the description, so perhaps it could get a summary in the code?

I found a couple of very minor simplifications:

We can declare count as

    static const auto count = sizeof...(PredsT);

Instead of duplicating ++begin in a for loop, we can transform to a simpler while loop:

    while (++begin != end) {

Instead of indexing selected, we might choose to iterate (and avoid a no-op assignment, and avoid the need to remember precedence of comma operator):

    auto p = selected.begin();
    ((preds(**p, *begin) ? *p++ : *p++ = begin), ...);


Gravity model for a simulator I am designing a planetary system simulator. The coordinates used are standard for JPanel (0,0 in the upper left corner).

My model is based on a class Body:

public class Body {
    private double x, y; // position
    private double w, h; // dimensions for drawing
    private double vx = 0, vy = 0; // velocities along x, y axes
    private double ax = 0, ay = 0; // accelerations along x, y axes
    private double mass = 1; // default = 1
    private boolean stationary = false; // stationary body is centered during simulation

    /* ... not showing all the setters and getters */

Because the force of gravity is the same for any two interacting bodies, I designed the methods to take another body as an argument, so we can call planet1.interact(planet2)

Distance calculation:

public double calculateDistX(Body other) {
    double x1 = this.getX();
    double x2 = other.getX();
    return Math.sqrt(Math.pow(x2 - x1, 2));
}

public double calculateDistY(Body other) {
    double y1 = this.getY();
    double y2 = other.getY();
    return Math.sqrt(Math.pow(y2 - y1, 2));
}

public static double calculateDistance(double dist_x, double dist_y) {
    return Math.sqrt(Math.pow(dist_x, 2) + Math.pow(dist_y, 2));
}

Finally, the method that calculates gravitational force:

public void interact(Body other) {
    double x = calculateDistX(other);
    double y = calculateDistY(other);
    double r = calculateDistance(x, y);

    double force = (this.getMass() * other.getMass()) / Math.pow(r, 2);
    double force_x = force * (x / r); // force * cos
    double force_y = force * (y / r); // force * sin

    /* calculate accelerations for both bodies, set vector orientation */
    if (other.getX() > this.getX()) {
        this.setAx(force_x / this.getMass());
        other.setAx(-force_x / other.getMass());
    } else {
        this.setAx(-force_x / this.getMass());
        other.setAx(force_x / other.getMass());
    }

    if (other.getY() > this.getY()) {
        this.setAy(force_y / this.getMass());
        other.setAy(-force_y / other.getMass());
    } else {
        this.setAy(-force_y / this.getMass());
        other.setAy(force_y / other.getMass());
    }

    /* calculate velocities for both bodies */
    this.setVx(this.getVx() + this.getAx());
    this.setVy(this.getVy() + this.getAy());

    other.setVx(other.getVx() + other.getAx());
    other.setVy(other.getVy() + other.getAy());

    /* calculate positions for both bodies */
    this.setX(this.getX() + this.getVx());
    this.setY(this.getY() + this.getVy());

    other.setX(other.getX() + other.getVx());
    other.setY(other.getY() + other.getVy());
}

I don't really know how good/bad this model is. The section in interact that sets the vector orientation looks like it can be simpler, but I can't figure it out.

Also, I have doubts about this method, because calling:

planet1.interact(planet2);
planet1.interact(planet3);
planet2.interact(planet3);

Makes planet2 and planet3 interact AFTER the original position of planet1 and planet2 has changed, and it's not how it's hapenning in reality. I don't know how important that would be, though.

Math.pow(x, 2) -> x*x

A pet peeve of mine is people calling Math.pow() just to square a number.  To me it seems faster and simpler to just multiply the value by itself.

Shadowing variable names

In interact(), you use the variables x and y.  I would avoid using variable names that shadow instance variables.  I would call these variables dx and dy instead.

X and Y distances

Look closely at your x/y distance calculations functions:

public double calculateDistX(Body other) {
    double x1 = this.getX();
    double x2 = other.getX();
    return Math.sqrt(Math.pow(x2 - x1, 2));
}

It actually just does this:

public double calculateDistX(Body other) {
    return Math.abs(this.getX() - other.getX());
}

This shorter version is also less prone to roundoff errors.  But in fact, I would even remove this function, because it will be more useful to use to preserve the sign of the subtraction than to hide it (read below)

Splitting up interact and update

Your interaction function should do no more than modify the acceleration of the object.  Once the body has interacted with all other bodies, its acceleration will have the final acceleration for that timeslice.  After that, you should have an update function which takes the acceleration and applies it to the velocity, and then applies the velocity to the position.  I'll show an example of that below.

Simplifying the interaction function

Looking at your interaction function, there are a lot of places that can be simplified.  First, if you retained the sign of the x/y distances, you wouldn't need all those if statements.  Second, a lot of the force calculations are related to each other and can be merged into a smaller set of calculations.  In fact, when I rewrote your function, it became this simple:

public void interact(Body other) {
    double dx = other.getX() - x;
    double dy = other.getY() - y;
    double r  = calculateDistance(dx, dy);
    double r3 = r * r * r;

    /* calculate accelerations for both bodies */
    ax += other.getMass() * dx / r3;
    ay += other.getMass() * dy / r3;
    other.addToAx(mass * -dx / r3);
    other.addToAy(mass * -dy / r3);
}

public void update() {
    vx += ax;
    vy += ay;

    ax = 0;
    ay = 0;

    x += vx;
    y += vy;
}

Further simplifications

If you don't actually need the current velocity of the object other than to update the position (e.g for collisions), you can get rid of the acceleration entirely and operate directly on the velocity.

Edit: Also, I think that multiplications are faster than divisions, so you could also invert r3 so that you only need to do one divide.  You can also precompute dx/r^3 and dy/r^3 to reduce the number of multiplies:

public void interact(Body other) {
    double dx = other.getX() - x;
    double dy = other.getY() - y;
    double r  = calculateDistance(dx, dy);
    double inv_r3 = 1.0 / (r * r * r);

    /* Precalculate force component (1/r^2) times direction (dx/r) = dx / r^3 */
    dx *= inv_r3;
    dy *= inv_r3;

    /* calculate accelerations for both bodies */
    vx += other.getMass() * dx;
    vy += other.getMass() * dy;
    other.addToVx(mass * -dx);
    other.addToVy(mass * -dy);
}

public void update() {
    x += vx;
    y += vy;
}


My self-study inheritance and sub-class exercise I'm self-studying Java and have a question about one of the end-of-chapter exercises.  The chapter focus is on inheritance and the book hasn't officially introduced polymorphism so I'm trying to stay within those bounds.

The exercise is:

  Write an inheritance hierarchy for classes Quadrilateral, Trapezoid, Parallelogram, Rectangle and Square.  Use Quadrilateral as the superclass of the hierarchy.  Create and use a Point class to represent the points in each shape.  Make the hierarchy as deep(i.e., as many levels) as possible.  Specify the instance variables and methods for each class.  The private instance variables of Quadrilateral should be the x-y coordinate pairs for the four endpoints of the Quadrilateral.  Write a program that instantiates objects of your classes and outputs each object's area(except Quadrilateral).

My classes are below and they compile and work OK.

Questions:

The exercise statement

  The private instance variables of Quadrilateral should be the x-y coordinate pairs for the four endpoints of the Quadrilateral.

I put the x-y coordinate instance variables in my Point class because it seemed to fit better and if I didn't, what purpose would Point serve other than to hold the four points.  With the way that I did it, it also calculates the distance between the points which seems to fit better to me. Comments?
I thought it would make more sense to use Point class inside Square directly instead of going through Quadrilateral like I did.  I used Quadrilateral because each sub-class will need points.  Taking Point out of Quadrilateral would make it have even less functionality than it does.  Of course it doesn't make sense to add a class just for the sake of adding a class but I'm kind of in that position by trying to meet the exercise objectives.  Comments?
Any other general feedback would be welcome.  Does it look professional?  Even though it's kind of small, would it be accepted in the corporate world?

package exercise9_8;
public class Point {
private int distanceX, distanceY;
private int x0, x1, y0, y1;

public Point( int x0, int x1, int y0, int y1 ){
    this.x0 = x0;
    this.x1 = x1;
    this.y0 = y0;
    this.y1 = y1;
    setDistanceX();
    setDistanceY();
}

private void setDistanceX(){
    distanceX = x1 - x0;
}

public int getDistanceX(){
    return distanceX;
}

private void setDistanceY(){
    distanceY = y1 - y0;
}

public int getDistanceY(){
    return distanceY;
}
}

package exercise9_8;
public class Quadrilateral {
Point point;

public Quadrilateral( int x0, int x1, int y0, int y1 ){
    point = new Point( x0, x1, y0, y1 );
}

protected int getDistanceX(){
    return point.getDistanceX();
}

protected int getDistanceY(){
    return point.getDistanceX();
}
}

package exercise9_8;
public class Square extends Quadrilateral{
private int area;

public Square( int x0, int x1, int y0, int y1 ){
    super( x0, x1, y0, y1 );
    setArea();
}

private void setArea(){
    area = super.getDistanceX() * super.getDistanceY();
}

public int getArea(){
    return area;
}
}

package exercise9_8;
import org.testng.annotations.Test;
import static org.testng.Assert.*;

public class SquareTestNG {

@Test(enabled = true)
public void testGetNumberOfPoints(){
    Square sq = new Square( 5, 3, 5, 3 );
    assertEquals(sq.getArea(), 4);
}
}

First of all, I'd change how the Point class functions because like David Harkness said, your Point class resembles that of a Line.  I would have it representing two values (x, y) and include functions that would make it easier to find the area of a shape.  Here's an example:

    public class Point {

        public float x;
        public float y;

        public Point(float x, float y) {
            this.x = x;
            this.y = y;
        }

        public static float distanceTo(Point from, Point to) {
            // Distance formula
            return Math.sqrt(Math.pow(to.x - to.y, 2) + Math.pow(from.x - from.y, 2));
        }

Using that point class, I could implement it into a Shape class to centralize a getArea() function.  To have different types of shapes, you could extend the Shape class from other classes and specify how many points each class or override the getArea() function.