Possible issues and best practices for this piece of code public class NANDFunction : FunctionInfo
{
    public override string Name { get { return "NAND"; } }
    public override int MinArgs { get { return 2; } }
    public override int MaxArgs { get { return 2; } }

    public override object Evaluate(object[] args)
    {
        bool arg0 = CalcConvert.ToBool(args[0]);
        bool arg1 = CalcConvert.ToBool(args[1]);
        return ((arg0 != arg1) || (!arg0 && !arg1));
    }
}

What do you think of this code?

You may want to add some additional validation around how many arguments you're expecting (since you know via MinArgs and MaxArgs). You can also eliminate having to create an object array explicitly in the caller by using the params keyword (make sure you do it in the declaration in FunctionInfo as well). Lastly, if this class is intended to be non-inheritable (unlike FunctionInfo), mark it as sealed:

public sealed class NANDFunction : FunctionInfo
{
    public override string Name { get { return "NAND"; } }
    public override int MinArgs { get { return 2; } }
    public override int MaxArgs { get { return 2; } }

    public override object Evaluate(params object[] args)
    {
        if ((args.Length < this.MinArgs) || (args.Length > this.MaxArgs))
        {
            throw new ArgumentException("An insufficient number of arguments were passed.");
        }

        bool arg0 = CalcConvert.ToBool(args[0]);
        bool arg1 = CalcConvert.ToBool(args[1]);
        return (arg0 != arg1) || (!arg0 && !arg1);
    }
}


Writing line item information from multiple tables The following is part of a personal budgeting program I'm writing.

This code pulls line item information from multiple tables and writes it into an array and then displays the information by transid => family => category => lineItems. Everything works, and I get the results I want out of it. I want to know if there is a more efficient way to accomplish this task.

Since this is a personal program, I'm only asking so that I can improve my coding abilities.

<?php

include ('../cfg/connect.php');

$s = " : ";
$br = "
";
$ul = "
";
$li = "
";
$_ul = "
";
$_li = "
";

$data = [];
$itemCount = 0;
$arrayItemCount = 0;

$categoryQry = "SELECT a.itemQty, b.transDate, b.transID, b.amount, a.itemPrice, a.itemCategory, c.catFamily, a.itemName, a.itemSource FROM budget.lineItems AS a JOIN budget.quickEntry AS b ON a.transID = b.transID JOIN budget.categories AS c ON a.itemCategory = c.catName WHERE b.processed = 'y' ORDER BY c.catFamily, c.catName, b.transDate";
$categories = $conn->prepare ($categoryQry);
$categories->execute ();
$categories->store_result ();
$categories->bind_result ($itemQty, $transDate, $transID, $totalPrice, $itemPrice, $category, $family, $itemName, $source);

while ($categories->fetch ()) {

    if (!isset($data[$transID]['amount'])) {
        $data[$transID]['amount'] = 0;
    }
    if (!isset($data[$transID]['line'])) {
        $data[$transID]['line'] = '';
    }
    if (!isset($data[$transID]['line'][$family]['amount'])) {
        $data[$transID]['line'][$family]['amount'] = 0;
    }
    if (!isset($data[$transID]['line'][$family]['line'])) {
        $data[$transID]['line'][$family]['line'] = '';
    }
    if (!isset($data[$transID]['line'][$family]['line'][$category]['amount'])) {
        $data[$transID]['line'][$family]['line'][$category]['amount'] = 0;
    }
    if (!isset($data[$transID]['line'][$family]['line'][$category]['line'])) {
        $data[$transID]['line'][$family]['line'][$category]['line'] = '';
    }

    $itemCount++;

    $qtyPrice = $itemPrice * $itemQty;
    $data[$transID]['amount'] += $qtyPrice;
    $data[$transID]['transDate'] = $transDate;
    $data[$transID]['source'] = $source;
    $data[$transID]['line'][$family]['amount'] += $qtyPrice;
    $data[$transID]['line'][$family]['line'][$category]['amount'] += $qtyPrice;
    $data[$transID]['line'][$family]['line'][$category]['line'][$itemName] = ['itemQty' => $itemQty, 'itemPrice' => $itemPrice];
}

foreach ($data as $transID => $transValue) {
    echo $transID .$s.$transValue['transDate'].$s.$transValue['source'].$s.$transValue['amount']. $ul;
    foreach ($transValue['line'] as $category => $categoryValue) {
        echo $li . $category .$s.$categoryValue['amount']. $ul;
        foreach ($categoryValue['line'] as $line => $lineValue) {
            echo $li . $line .$s.$lineValue['amount']. $ul;
            foreach ($lineValue['line'] as $item => $details) {
                echo $li . $item .$s . $details['itemQty'] . $s . $details['itemPrice'] . $_li;
            }
            echo $_ul . $_li;
        }
        echo $_ul . $_li;
    }
    echo $_ul . $br;
}

My thoughts in walking through your code are shown below:

include ('../cfg/connect.php');

This appears to be hard dependency.  You should use require such that this script fails out of you can't load the dependency. Also, your style of adding space between function/method name and opening parenthesis would be considered non-standard by most PHP developers. Typical usage has no space.  This makes reader less likely to confuse this parenthesis with a logical operation.

$s = " : ";
$br = "
";
$ul = "
";
$li = "
";
$_ul = "
";
$_li = "
";

Consider using constants for these, as they don't change during script execution.  You might also find this helps readability of your code when you begin to concatenate with these.

$categoryQry = "SELECT a.itemQty, b.transDate, b.transID, b.amount, a.itemPrice, a.itemCategory, c.catFamily, a.itemName, a.itemSource FROM budget.lineItems AS a JOIN budget.quickEntry AS b ON a.transID = b.transID JOIN budget.categories AS c ON a.itemCategory = c.catName WHERE b.processed = 'y' ORDER BY c.catFamily, c.catName, b.transDate";

This is a horrendously long line of code. You should strive to keep character count on a line under 80.  Break this query up across lines, making it much easier to read.

Example:

$categoryQry = <<<EOT
SELECT
    a.itemQty,
    b.transDate,
    b.transID,
    b.amount,
    a.itemPrice,
    a.itemCategory,
    c.catFamily,
    a.itemName,
    a.itemSource
FROM budget.lineItems AS a
JOIN budget.quickEntry AS b
    ON a.transID = b.transID
JOIN budget.categories AS c
    ON a.itemCategory = c.catName
WHERE b.processed = 'y'
ORDER BY c.catFamily, c.catName, b.transDate
EOT;

$categories = $conn->prepare ($categoryQry);
$categories->execute ();
$categories->store_result ();
$categories->bind_result ($itemQty, $transDate, $transID, $totalPrice, $itemPrice, $category, $family, $itemName, $source);

This all assumes happy-path execution.  What happens if prepare(), or execute() fail?  Last line of code is too long. Break it up

Example:

$categories->bind_result(
    $itemQty, $transDate, $transID, $totalPrice, $itemPrice,
    $category, $family, $itemName, $source
);

if (!isset($data[$transID]['amount'])) {
    $data[$transID]['amount'] = 0;
}
if (!isset($data[$transID]['line'])) {
    $data[$transID]['line'] = '';
}
if (!isset($data[$transID]['line'][$family]['amount'])) {
    $data[$transID]['line'][$family]['amount'] = 0;
}
if (!isset($data[$transID]['line'][$family]['line'])) {
    $data[$transID]['line'][$family]['line'] = '';
}
if (!isset($data[$transID]['line'][$family]['line'][$category]['amount']))
{
    $data[$transID]['line'][$family]['line'][$category]['amount'] = 0;
}
if (!isset($data[$transID]['line'][$family]['line'][$category]['line']))
{
    $data[$transID]['line'][$family]['line'][$category]['line'] = '';
}

$itemCount++;

$qtyPrice = $itemPrice * $itemQty;
$data[$transID]['amount'] += $qtyPrice;
$data[$transID]['transDate'] = $transDate;
$data[$transID]['source'] = $source;
$data[$transID]['line'][$family]['amount'] += $qtyPrice;
$data[$transID]['line'][$family]['line'][$category]['amount'] += $qtyPrice;
$data[$transID]['line'][$family]['line'][$category]['line'][$itemName]
= ['itemQty' => $itemQty, 'itemPrice' => $itemPrice];

I have concern over the overall data structure you are building.  First, I think there are more levels of nesting than is necessary. Second, the repeated use of line as key at different levels of depth seems odd and potentially confusing.

I would encourage you to really rethink your data structure with an object-oriented approach. Think about what are the "real world" properties that should exist for each item in the hierarchy and how you represent the relationship between items.

You might end up with a structure like that shown below (shown using JSON-like notation to indicate object structures - or alternately associative arrays in PHP).  This has 4 levels of nesting instead of 8 as your current model does.  Interestingly enough, you will also note that your logic to output this data structure has 4 levels.  That is because four levels of nesting is what the "real-world" relations between these objects in your hierarchy really dictate.

{
    'transactionid1': {
        'amount': *,
        'date': *,
        'source': *,
        'categories': {
            'categorynameA': {
                'amount': *,
                'lines': {
                    'linenameA': {
                        'amount': *,
                        'items': {
                            'itemnameA': {
                                'qty': *,
                                'price': *
                            },
                            'itemnameB': // more items
                        },
                    'linenameB': // other lines
                },
            'categorynameB': // other categories
        },
    'transactoinid2': // other transactions
}

This isn't exactly optimized for your display loops (which do not require unique lookup capabilities by transaction id, category, etc. as  are required to build this structure from the result set), but I don't think that it makes sense to reconfigure this structure for this simple use case.

There is additionally some confusion on naming of family, categories, lines between the DB and what is being output to user. For example it seems family in DB maps to categories for output which seem strange.  You might consider aliasing your fields in your query to be able to use common terminology throughout both the section of code where you build the data structure and where you output it.

In your output, consider keeping spaces around all . used for concatenation.  This helps make code easy to read and allows for good visual line breaks in editors if a line does happen to go too long while editor is using "word wrapping".

Also, I think using objects (as mentioned in data structure comment above) instead of associative arrays helps in readability.  This along with suggestion to use constants might yield code that looks like the following:

// assume you have constants S, BR, UL, LI, _UL, _LI
foreach ($data as $txn_id => $txn) {
    echo $txn_id . S . $txn->date . S . $txn->source . S . $txn->amount . UL . PHP_EOL;
    foreach ($txn->categories as $cat_name => $cat) {
        echo LI . $cat_name . S .$cat->amount . UL . PHP_EOL;
        foreach ($cat->lines as $line_name => $line) {
            echo LI . $line_name . S . $line->amount . UL . PHP_EOL;
            foreach ($line->items as $item_name => $item) {
                echo LI . $item_name . S . $item->qty . S . $item->price . _LI . PHP_EOL;
            }
            echo _UL . _LI . PHP_EOL;
        }
        echo _UL . _LI . PHP_EOL;
    }
    echo _UL . BR . PHP_EOL;
}

To me, this is much easier to read and a pretty good approach short of going to more of a template approach.


Is there a way to make my Rock, Paper, Scissors game more concise? I am learning JavaScript and I just wrote a Rock, Paper, Scissors game that will first prompt the two players for their choices and validate them. In the event of a tie, each player will choose their answer again and it will once again be validated. Everything works fine, but it feels like I have a lot of repetition going on here. Since I'm new to this, I can't figure out a way to make this more concise if possible.

var compare = function(choice1, choice2){
    while (choice1 != 'rock' && choice1 != 'paper' && choice1 != "scissors"){
        choice1 = prompt('Invalid entry. Player 1, please choose rock, paper, or scissors only');
    }
    while (choice2 != 'rock' && choice2 != 'paper' && choice2 != "scissors"){
        choice2 = prompt('Invalid entry. Player 2, please choose rock, paper, or scissors only');
    }
    while (choice1 == choice2){
        choice1 = prompt('Tiebreaker!. Player 1, please choose rock, paper, or scissors only');
     while (choice1 != 'rock' && choice1 != 'paper' && choice1 != "scissors"){
        choice1 = prompt('Invalid Entry. Player 1, please choose rock, paper, or scissors only');
        }
        choice2 = prompt('Tiebreaker!. Player 2, please choose rock, paper, or scissors only');
     while (choice2 != 'rock' && choice2 != 'paper' && choice2 != "scissors"){
        choice2 = prompt('Invalid Entry. Player 2, please choose rock, paper, or scissors only');
        }

    }
    if (choice1 == 'rock'){
        if (choice2 == 'scissors'){
            return 'rock wins';
        }
        else{
            return 'paper wins';
        }
    }
    if (choice1 == 'paper'){
        if (choice2 == 'rock'){
            return 'paper wins';
        }
        else{
            return 'scissors wins';
        }
    }
    if (choice1 == 'scissors' ){
        if (choice2 == 'rock'){
            return 'rock wins';
        }
        else{
            return 'scissors wins';
        }
    }
}

Just build an object with the associativity.

// what wins to what
var winsTo = {
  'rock': 'scissors',
  'paper': 'rock',
  'scissors': 'paper',
};

// invalid selected
if (!(choice1 in winsTo && choice2 in winsTo)) {
  alert("bad choice !");
  return;
}

// same choice => equality
if (choice1 == choice2) {
  alert("Equality!");
} else if (winsTo[choice1] == choice2) {
  alert("choice1 won!");
} else {
  alert("choice2 won!");
}


Find all triplets in array that add up to a given sum Given an array and a value, find all the triplets in the array whose sum is equal to the given value. For example, if the given array is {12, 3, 4, 1, 6, 9} and the given sum is 24, then this is one triplet (12, 3 and 9) which contributes to the total sum of 24.
Solution for given example:
6, 9, 9
6, 6, 12
3, 9, 12
The ordering of the numbers in the solution does not matter.
Duplicate triplet is not allowed.
A number is not allowed to be used multiple time.

Here is my code:
package kata.array;

import java.util.*;

public class ThreeSum {

  static class Triplet {
        int x, y, z;

        public Triplet(int x, int y, int z) {
            this.x = x;
            this.y = y;
            this.z = z;
        }

        @Override
        public int hashCode() {
            return Objects.hash(x, y, z);
        }

        @Override
        public boolean equals(Object o) {
            if (o instanceof Triplet) {
                Set<Integer> numbers = new HashSet<>();
                numbers.addAll(new ArrayList<>(Arrays.asList(x, y, z)));

                Triplet other = (Triplet) o;
                return numbers.containsAll(new ArrayList<>(Arrays.asList(other.x, other.y, other.z)));
            }

            return false;
        }
    }

    public static Set<Triplet> findTriplets(int numbers[], int targetSum) {
        Set<Triplet> triplets = new HashSet<>();

        // insert all pairs in the look up table
        Map<Integer, int[]> lookup = new HashMap<>();
        for (int i = 0; i < numbers.length - 1; i++) {
            for (int j = i + 1; j < numbers.length; j++) {
                int total = numbers[i] + numbers[j];
                lookup.put(total, new int[]{i, j});
            }
        }

        // look for the complement, if found viola! here you go with the matching triplet
        for (int number : numbers) {
            int complement = targetSum - number;

            if (lookup.containsKey(complement)) {
                int indexes[] = lookup.get(complement);
                int x = numbers[indexes[0]], y = numbers[indexes[1]];
                triplets.add(new Triplet(x, y, number));
            }
        }

        return triplets;
    }
}

To run this code:
public void findTriplets() throws Exception {
    int numbers[] = {12, 3, 4, 1, 6, 9};

    System.out.print(ThreeSum.findTriplets(numbers, 24));

    for (ThreeSum.Triplet triplet : ThreeSum.findTriplets(numbers, 24)) {
        System.out.println(triplet.x + ", " + triplet.y + ", " + triplet.z);
    }

    // can handle duplicate?
    System.out.println("==============================");
    numbers = new int[] {12, 3, 4, 1, 6, 9, 9, 9, 9, 9};

    System.out.print(ThreeSum.findTriplets(numbers, 24));

    for (ThreeSum.Triplet triplet : ThreeSum.findTriplets(numbers, 24)) {
        System.out.println(triplet.x + ", " + triplet.y + ", " + triplet.z);
    }
}

GitHub

Arrays.asList(other.x, other.y, other.z) already returns a new list. There is no need to add the result to a new ArrayList you could just use

numbers.containsAll(Arrays.asList(other.x, other.y, other.z));

You creating some unnecessary objects that still need to be cleaned up by the garbage collector. e.g.: this could save you some memory. Instead of:

numbers.addAll(new ArrayList<>(Arrays.asList(x, y, z)));

use

numbers.add(x);
numbers.add(y);
numbers.add(z);

Here you can save one lookup in the HashMap. Instead of:

if (lookup.containsKey(complement)) {
    int indexes[] = lookup.get(complement);
    ...

you can write:

int indexes[] = lookup.get(complement);
if (indexes != null) {
    ...


RPG character creation implementation This is a little yet unfinished RPG I made with C#. It runs well but I'm wondering if there are things I could improve to make it more clear or optimal.  I'm basically asking for some feedback and suggestions.

The program is obviously not finished but I'm asking now before I move on so I know if I got some serious design or logic issues there. I'm a beginner in programming so keep that in mind when answering.

class Main
{
    static void Main(string[] args)
    {
        Console.ForegroundColor = ConsoleColor.White;
        Console.WriteLine("1. New Game\n" +
                          "2. Load Game\n" +
                          "3. Exit Game\n");
        string choice = Console.ReadLine();
        bool correctChoice = false;
        Hero newHero = new Hero();
        while (correctChoice == false)
        {
            switch (choice)
            {
                case "1":

                    CharacterCreation.createChar(newHero);
                    correctChoice = true;
                    break;
                case "2":
                    Console.WriteLine("This feature is not yet implemented.");
                    correctChoice = false;
                    break;
                case "3":
                    Environment.Exit(0);
                    break;
                default:
                    Console.WriteLine("Please enter a correct value.");
                    break;
            }
            if (correctChoice == true)
                break;
            else
                choice = Console.ReadLine();
        }
        Console.WriteLine("\n Your character is {0}, a {1}.", newHero.heroName, newHero.heroClass);

        Console.ReadLine();
    }
}

class CharacterCreation
{
    public static void createChar(Hero newHero)
    {
        Console.WriteLine("\n We will proceed in character creation.");
        Console.WriteLine("Please enter your character name:");
        newHero.heroName = Console.ReadLine();

        Console.WriteLine("\nNow choose your character's class: \n" +
                          "Warrior......1\n" +
                          "Mage........2\n" +
                          "Rogue........3");
        string classChoice = Console.ReadLine();
        bool correctChoice = false;
        while (correctChoice == false)
        {
            switch (classChoice)
            {
                case "1":
                    newHero.heroClass = "Warrior";
                    correctChoice = true;
                    break;
                case "2":
                    newHero.heroClass = "Mage";
                    correctChoice = true;
                    break;
                case "3":
                    newHero.heroClass = "Rogue";
                    correctChoice = true;
                    break;
                default:
                    Console.WriteLine("Please enter a correct value.");
                    break;
            }
            if (correctChoice == false)
                classChoice = Console.ReadLine();
            else break;
        }
        Hero.ClassSelect(newHero);
    }
}

class Character
{
    public int charHealth, charMaxHealth, charStr, charInt, charAgi;
}

class Hero : Character
{
    public string heroName, heroClass;
    public int level, experience, gold;

    public static void ClassSelect(Hero newHero)
    {
        newHero.charAgi = 10;
        newHero.charInt = 10;
        newHero.charMaxHealth = 100;
        newHero.experience = 0;
        newHero.gold = 0;
        newHero.level = 1;

        switch (newHero.heroClass)
        {
            case "Warrior":
                newHero.charMaxHealth = newHero.charMaxHealth * 115 / 100;
                newHero.charStr = newHero.charStr * 12 / 100;
                break;
            case "Mage":
                newHero.charInt = newHero.charInt * 12 / 100;
                break;
            case "Rogue":
                newHero.charAgi = newHero.charAgi * 12 / 100;
                break;
        }
        newHero.charHealth = newHero.charMaxHealth;
    }
}

I haven't done much work in C#, but from a general coding perspective...

class Main
{
    static void Main(string[] args)
    {
        // lotsa stuff....
    }
}

These (the class and method both) really ought to be public, as I believe that most application runners/executables will be looking for public methods.  Additionally, a Main() method should be mostly empty, containing just enough code to launch the rest of the application (whether this submits an additional process or not).

 Console.WriteLine()  // Many times

Ideally, never call out to the console specifically.  What you should be doing is injecting some sort of output/input stream.  It's fine if these actually come from the console, but explicitly tying it to the console itself is frowned upon.

Console.WriteLine("1. New Game\n" +
                  "2. Load Game\n" +
                  "3. Exit Game\n");

So... you're using WriteLine() to write multiple lines at a time?  Seems a little counter-intuitive.  I'm slightly torn between recommending using individual calls (and potentially having output re-ordered because of some threading issue), or just using Write()...

while (correctChoice == false)
{
    // stuff
}

Using booleans like this is frowned upon; don't compare the boolean with a value, use the boolean itself (that's what it's for) - so, it should be while (!correctChoice) { ... }.  Additionally, the name is somewhat ambiguous: possibly availableOptionChosen?

switch (choice)
{
    // lotsa stuff
}

Look into something called the Strategy Pattern.  While the use of a switch works out for simple things, software patterns are something to always keep in mind.

case "2":
    Console.WriteLine("This feature is not yet implemented.");
    correctChoice = false;
    break;

Strictly speaking, correctChoice = false; shouldn't be necessary.

case "3":
    Environment.Exit(0);
    break;

Using Environment.Exic(0); should end the running thread.  However (if this is anything like Java's System.Exit()) this may have unintended consequences.  It would be much better to simply return to the next level.

default:
    Console.WriteLine("Please enter a correct value.");
    break;

You check the 'remaining' cases.  Good; generally speaking, you should always do so, even if it's impossible (some of those should maybe only be 'asserts', used for verification-time checks).

        Console.ReadLine();
    }
}

You read a line immediately before the program exits.  Why?

class CharacterCreation
{

     // Lotsa stuff
}

This is semantically a Builder-pattern, so the class name is a little odd - perhaps CharacterCreator?

public static void createChar(Hero newHero)
{
    // Stuff
}

Why the abbreviation of 'Character' (and why all the abbreviations, generally)?  Also, if it's named 'create...', you shouldn't be passing it in - it should be the return type (alternatively, call it setupCharacter(), and pass in the Hero). Additionally, newHero is an awkward name here - just use hero (you don't have an `oldHero you're using, do you?).

while (correctChoice == false)
{
    switch (classChoice)
    {

The stuff I mentioned about your main loop applies here as well.  This is especially prevalent with the class, if you're eventually going for something like DnD (which can have hundreds of classes).

class Character
{
    public int charHealth, charMaxHealth, charStr, charInt, charAgi;
}

Always put member definitions on their own lines.  Always make them private, and provide equivalent public properties.  Don't abbreviate unnecessarily, seek to be understandable.  Don't prefix variable names with their enclosing class' name (especially abbreviated) - it's noise.

class Hero : Character
{
     // Acceptable
}

This works, although it may become awkward in the future.

public static void ClassSelect(Hero newHero)
{
    newHero.charAgi = 10;
    newHero.charInt = 10;
    newHero.charMaxHealth = 100;
    newHero.experience = 0;
    newHero.gold = 0;
    newHero.level = 1;

    switch (newHero.heroClass)
    {
        case "Warrior":
            newHero.charMaxHealth = newHero.charMaxHealth * 115 / 100;
            newHero.charStr = newHero.charStr * 12 / 100;
            break;
        case "Mage":
            newHero.charInt = newHero.charInt * 12 / 100;
            break;
        case "Rogue":
            newHero.charAgi = newHero.charAgi * 12 / 100;
            break;
    }
    newHero.charHealth = newHero.charMaxHealth;
}

This, though, belongs in CharacterCreation (that's what it's there for).  Additionally, instead of

newHero.charXxx = newHero.charXxx * 12 / 100;

you should probably just be setting it to the desired value.  Especially because that's probably a bug, since a warrior will start out with a higher Int (10) than a mage

((10 * 12) / 100) = 120 / 100 = 1.2 -> 1

which I doubt is what you want.


Python list dictionary items round robin mixing I have a list of dictionaries, which I want to make round robin mixing.

sample = [
    {'source': 'G', '"serial"': '0'},
    {'source': 'G', '"serial"': '1'},
    {'source': 'G', '"serial"': '2'},
    {'source': 'P', '"serial"': '30'},
    {'source': 'P', '"serial"': '0'},
    {'source': 'P', '"serial"': '1'},
    {'source': 'P', '"serial"': '2'},
    {'source': 'P', '"serial"': '3'},
    {'source': 'T', '"serial"': '2'},
    {'source': 'T', '"serial"': '3'}
]

I want result as below:

sample_solved = [
    {'source': 'G', '"serial"': '0'},
    {'source': 'P', '"serial"': '30'},
    {'source': 'T', '"serial"': '2'},
    {'source': 'G', '"serial"': '1'},
    {'source': 'P', '"serial"': '1'},
    {'source': 'T', '"serial"': '3'},
    {'source': 'G', '"serial"': '2'},
    {'source': 'P', '"serial"': '0'},
    {'source': 'P', '"serial"': '2'},
    {'source': 'P', '"serial"': '3'}
]

The way I solved it as follows:

def roundrobin(*iterables):
    # took from here https://docs.python.org/3/library/itertools.html#itertools-recipes

    "roundrobin('ABC', 'D', 'EF') --> A D E B F C"
    # Recipe credited to George Sakkis

    pending = len(iterables)
    nexts = cycle(iter(it).__next__ for it in iterables)
    while pending:
        try:
            for next in nexts:
                yield next()
        except StopIteration:
            pending -= 1
            nexts = cycle(islice(nexts, pending))

def solve():
    items_by_sources = collections.defaultdict(list)

    for item in sample:
         items_by_sources[item["source"]].append(item)

    t, p, g = items_by_sources.values()

    print(list(roundrobin(t, p, g)))

Using python default dict to separate the items by source and then using roundrobin solution which I got from python docs.

How can make the solution more pythonic or improved?

Using the roundrobin() recipe from itertools is already pythonic. The solve() method could be replaced with more use of itertools.

In particular itertools.groupby() would do the same as your defaultdict and for loop:

>>> import operator as op
>>> g, p, t = [list(v) for k, v in groupby(sample, key=op.itemgetter('source'))]
>>> list(roundrobin(g, p, t))
[{'"serial"': '0', 'source': 'G'},
 {'"serial"': '30', 'source': 'P'},
 {'"serial"': '2', 'source': 'T'},
 {'"serial"': '1', 'source': 'G'},
 {'"serial"': '0', 'source': 'P'},
 {'"serial"': '3', 'source': 'T'},
 {'"serial"': '2', 'source': 'G'},
 {'"serial"': '1', 'source': 'P'},
 {'"serial"': '2', 'source': 'P'},
 {'"serial"': '3', 'source': 'P'}]

You don't really need to unpack as you can make the call to roundrobin() using *, e.g:

>>> x = [list(v) for k, v in it.groupby(sample, key=op.itemgetter('source'))]
>>> list(roundrobin(*x))
[{'"serial"': '0', 'source': 'G'},
 {'"serial"': '30', 'source': 'P'},
...

Note roundrobin() could be rewritten using itertools.zip_longest(), which should be faster for near equal sized iterables e.g.:

def roundrobin(*iterables):
    sentinel = object()
    return (a for x in it.zip_longest(*iterables, fillvalue=sentinel)
            for a in x if a != sentinel)

Did a quick run of a 10000 random items in sample and found the recipe surprisingly slow (need to figure out why):

In [11]: sample = [{'source': random.choice('GPT'), 'serial': random.randrange(100)} for _ in range(10000)]
In [12]: x = [list(v) for k, v in it.groupby(sample, key=op.itemgetter('source'))]
In [13]: %timeit list(roundrobin_recipe(*x))
1 loop, best of 3: 1.48 s per loop
In [14]: %timeit list(roundrobin_ziplongest(*x))
100 loops, best of 3: 4.12 ms per loop
In [15]: %timeit TW_zip_longest(*x)
100 loops, best of 3: 6.36 ms per loop
In [16]: list(roundrobin_recipe(*x)) == list(roundrobin_ziplongest(*x))
True
In [17]: list(roundrobin_recipe(*x)) == TW_zip_longest(*x)
True


Nested try/catch with PDO in PHP I would like to know if this is the correct way to use try/catch when dealing with multiple MySQL queries and PDO:

try {
    $create = $con[$_POST['user-selected-connection']]->query("...");
    KD::notice('success','table successfully created');
    try {
        $insert = $con[$_POST['user-selected-connection']]->query("...");
        KD::notice('success','table was populated successfully');
    } catch(PDOException $e) {
        KD::notice('error','was unabled to insert records'.$e->getMessage());
    }
} catch(PDOException $e) {
    KD::notice('error','The table was not created.'.$e->getMessage());
}

This is part of a get started wizard in my application, and this is the part where the user needs to create tables in order to proceed.  That can be done in two ways; either copy/paste the provided SQL query manually into their MySQL handler (e.g. phpMyAdmin), or by just clicking a button if they have created a user with CREATE privileges in the previous step. (The previous step, which is "set up database, and add users into the application", tells them the benefits of having a super user for future tasks).

Anyway, I need to use try/catch since the user has the ability to chose which user they want to use when creating the tables. And if the selects user doesn't have the required privileges, a fatal error message is returned. That's how I figured I needed to use try/catch to handle the fatal error, and notify that the user didn't have access.

I do this task in two stages to make sure that the required records actually gets inserted after the table is created. (had a few test runs where the table was created, and the records wasn't inserted when in the same query).

Is this an appropriate way to do this?

If the create part fails, will it jump to catch right away, or will it still try to execute the insert query before throwing the error?

Exception handlers that don't either handle exceptions or rethrow them, generally shouldn't exist.

An exception signals that a breakage has occurred -- one serious enough that the thrower thinks that the app should die.  Because you've asked to catch the exception, it's now your job to deal with the problem -- and that means more than just logging it or printing a message or whatever.  That means cleaning up at least your part of the mess, making things right again, deciding what to return, etc.

If the code can't safely recover, then rethrow, or throw your own exception -- or, if there's nothing it can clean up, don't catch at all.

(If you were using a real database, i would have suggested creating the table in a transaction to make cleanup more feasible.  Unfortunately, MySQL is severely gimped transactionwise; CREATE TABLE causes an implicit commit, so rolling back isn't really an option.)

Aside from that, since you don't do any cleanup, there's not much point to the nesting.  You could have one block that has all the actions, and the first error will abort the process.  (If you want different errors for each step, then you could have a "current step" variable you update as you step through the process.  As an added bonus, i find it makes the code a bit more self-documenting.

In other words, i might suggest something like

# because i hate repeating this ugliness
$db = $con[$_POST['user-selected-connection']];

try {
    $state = 'creating table';
    $create = $db->query("CREATE TABLE...");
    KD::notice('success','table successfully created');

    $state = 'populating table';
    $insert = $db->query("INSERT...");
    KD::notice('success','table was populated successfully');
}
catch(PDOException $e) {
    KD::notice('error',"While $state: " . $e->getMessage());
    throw $e;
}


JavaScript dynamic element filtering functionality I am creating a Website with different boxes being displayed to the user, and he can filter them by their Status (the dates are stored on a Django database). The possible statuses are "Active", "Upcoming" and "Ended".

The filtering works with Radio buttons, so he clicks whichever he likes and all other elements that don't match that description will be hidden.

I am not sure to how elegant this solution is overall. It seems as if I should aim to remove the repetitive structure at the end.

$(function(){

  // Radio buttons functionality
  var displayAllOption = $('#display-all-infos');
  var displayActiveOption = $('#display-active-infos');
  var displayUpcomingOption = $('#display-upcoming-infos');
  var displayEndedOption = $('#display-ended-infos');

  function showAllInfos(){
    $('.info-box').parent().show();
  }

  function hideInfosThatDontMatchOption(selectedOption){

    var infos = $('.info-box').parent();

    [].forEach.call(infos, function(info){
      var countdownContent = $(info).find('.info-box').find('.card-block').find('.countdown').html();

      // Hide every info that has not started, yet, based on the prefix
      // that is displayed at the beginning of the countdown timer
      if (!countdownContent.startsWith(selectedOption)){
        $(info).hide();
      }
    });
  }

  /*
     First lines change the Color highlighting by toggling the 'active' class

     Filter Logic: Make sure all info boxes are displayed at the beginning, and then
     hide all the info boxes that the User doesn't want to see
  */

  displayAllOption.on('click', function(){
    showAllInfos();
  });

  displayActiveOption.on('click', function(element){

    displayActiveOption.addClass('active')
    displayUpcomingOption.removeClass('active')
    displayEndedOption.removeClass('active')

    showAllInfos();
    hideInfosThatDontMatchOption('Time Left')
  });

  displayUpcomingOption.on('click', function(element){

    displayUpcomingOption.addClass('active')
    displayActiveOption.removeClass('active')
    displayEndedOption.removeClass('active')

    showAllInfos();
    hideinfosThatDontMatchOption('Starts In')
  });

  displayEndedOption.on('click', function(element){

    displayEndedOption.addClass('active')
    displayActiveOption.removeClass('active')
    displayUpcomingOption.removeClass('active')

    showAllInfos();
    hideInfosThatDontMatchOption('Ended')
  });

});

Here is the Radio Button HTML snippet:

        <input type="radio" name="options" autocomplete="off" checked> All

        <input type="radio" name="options" autocomplete="off" checked> Active

        <input type="radio" name="options" autocomplete="off"> Upcoming

        <input type="radio" name="options" autocomplete="off"> Ended

I'd appreciate all feedback telling me how I can improve it.

Thanks in Advance!

I find that these all radio buttons are of same group , so you can use same name for all the radio button with different values .

<input type="radio" name="displayOption" value="1" checked> All
<input type="radio" name="displayOption" value="2"> Active....

Now in this case you dont have to add and remove active class for the radio buttons. It will handle it by itself.

and write a common function like

$("input[name='displayOption']").change(function(){
 if ($(this).val() === '1') {
      myFunction();
    } else if ($(this).val() === '2') {
      myOtherFunction();
    }
});


Java application for finding permutations efficiently I am an eighth grader with a school project of creating and application in Java that returns the total permutations of two given numbers.  It needs to be light and efficient, for later deployment on a website.  I was wondering if there was any way I could improve the code so it would be more efficient.

class Factorials {
    public static void main(String[] args){
        int n = 8;
        long factorialN = n;
        for (int ForN = n; ForN <= n && ForN > 1; ForN--) {
            factorialN = factorialN * ForN;
        }
        factorialN = factorialN / n;
        System.out.println(factorialN);

        int r = 6;
        int rMinus1 = r - 1;
        long factorialR = rMinus1;
        for (int ForR = rMinus1; ForR <= rMinus1 && ForR > 1; ForR--) {
            factorialR = factorialR * ForR;
        }
        factorialR = factorialR / rMinus1;
        System.out.println(factorialR);

        int nMr = n - r;
        System.out.println(nMr);
        long factorialNmR = nMr;
        if (nMr == 2) {
            factorialNmR = 2;
        }
        else if (nMr <= 1){
            factorialNmR = 1;
        }
        else if (nMr > 2) {
            for (int FornMr = nMr; FornMr <= nMr && FornMr > 1; FornMr--) {
                factorialNmR = factorialNmR * FornMr;
            }
            factorialNmR = factorialNmR / nMr;
            System.out.println(factorialNmR);
        }

        long permutations = factorialN;
        System.out.println(permutations);
        permutations = permutations / factorialNmR;
        System.out.println(permutations);
    }
}

First of all, please write a single function instead of expanding what is essentially the same code three times!

long factorialN = n;
for (int ForN = n; ForN <= n && ForN > 1; ForN--) {
    factorialN = factorialN * ForN;
}
factorialN = factorialN / n;

Also, if your for-loop counter will start at N and only decrement, you do not need to check in every iteration whether it is less than or equal to N; it is guaranteed to be. You only need to check if it is greater than 1. But that incurs no performance penalty, since the Java compiler is clever enough to disregard your superfluous check.

Now, if you want a faster factorial implementation, you can use a recursive one, like the one described in this page. It is in C#, but you should have no problem converting it to Java. The only thing is, you need to gain knowledge of what recursion really is before trying to pass this as a piece of code you understand.

Or, you could use something far more advanced, like the asymptotic prime factorization algorithm described on this page, though you will have a very hard time convincing your teacher that you, an 8th grader, have the slightest clue as to how this algorithm works, and why, so better stay away from it!


Countdown click in JavaScript I did a small countdown clock challenge in JavaScript. Please review it, thanks.

const timeControls = document.querySelector('.timer__controls');
const timeLeft = document.querySelector('.display__time-left');
const endTime = document.querySelector('.display__end-time');
let interval;

function secondsToHourMinSec(seconds) {
  const date = new Date(0);
  date.setSeconds(seconds);
  return date.toISOString().substr(11, 8);
}

function advanceCurrentTime(seconds) {
  const date = new Date(0);
  date.setSeconds(Math.floor(Date.now() / 1000) + seconds);
  return date.toLocaleTimeString().substr(0, 7);
}

function displayRemainingTime(seconds) {
  timeLeft.textContent = secondsToHourMinSec(seconds);
}

function displayEndTime(seconds) {
  endTime.textContent = `Be back at ${advanceCurrentTime(seconds)}`;
}

function displayTime(seconds) {
  displayRemainingTime(seconds);
  displayEndTime(seconds);

  interval = setInterval(function() {
    seconds--;
    if (seconds < 0) {
      clearInterval(interval);
      return;
    }
    displayRemainingTime(seconds);
  }, 1000);
}

function startTimer(seconds) {
  clearInterval(interval);
  displayTime(seconds);
}

timeControls.addEventListener('click', e => {
  const button = e.target.closest('button');
  if (button)
    startTimer(parseInt(button.dataset.time));
});

document.customForm.addEventListener('submit', function(e) {
  e.preventDefault();
  const value = this.minutes.value;
  if (value.length !== 0)
    startTimer(parseInt(value) * 60);
  this.reset();
});
html {
  box-sizing: border-box;
  font-size: 10px;
  background: #8e24aa;
  background: linear-gradient(45deg, #42a5f5 0%, #478ed1 50%, #0d47a1 100%);
}

*,
*:before,
*:after {
  box-sizing: inherit;
}

body {
  margin: 0;
  text-align: center;
  font-family: "Inconsolata", monospace;
}

.display__time-left {
  font-weight: 100;
  font-size: 20rem;
  margin: 0;
  color: white;
  text-shadow: 4px 4px 0 rgba(0, 0, 0, 0.05);
}

.timer {
  display: flex;
  min-height: 100vh;
  flex-direction: column;
}

.timer__controls {
  display: flex;
}

.timer__controls>* {
  flex: 1;
}

.timer__controls form {
  flex: 1;
  display: flex;
}

.timer__controls input {
  flex: 1;
  border: 0;
  padding: 2rem;
}

.timer__button {
  background: none;
  border: 0;
  cursor: pointer;
  color: white;
  font-size: 2rem;
  text-transform: uppercase;
  background: rgba(0, 0, 0, 0.1);
  border-bottom: 3px solid rgba(0, 0, 0, 0.2);
  border-right: 1px solid rgba(0, 0, 0, 0.2);
  padding: 1rem;
  font-family: "Inconsolata", monospace;
}

.timer__button:hover,
.timer__button:focus {
  background: rgba(0, 0, 0, 0.2);
  outline: 0;
}

.display {
  flex: 1;
  display: flex;
  flex-direction: column;
  align-items: center;
  justify-content: center;
}

.display__end-time {
  font-size: 4rem;
  color: white;
}
<!DOCTYPE html>

  <meta charset="UTF-8">
  <title>Countdown Timer</title>
  <link href="https://fonts.googleapis.com/css?family=Inconsolata" rel="stylesheet" type="text/css">
  <link rel="stylesheet" href="style.css">

      <button data-time="20" class="timer__button">20 Secs</button>
      <button data-time="300" class="timer__button">Work 5</button>
      <button data-time="900" class="timer__button">Quick 15</button>
      <button data-time="1200" class="timer__button">Snack 20</button>
      <button data-time="3600" class="timer__button">Lunch Break</button>
      <form name="customForm" id="custom">
        <input type="text" name="minutes" placeholder="Enter Minutes">
      </form>

I have the following ideas for improvement of your code:

Separate different concepts like timers and UI rendering to make them easy to understand and change.
Use milliseconds for consistency with browser API.
Separate formatting and calculation for dates and durations.
Incapsulate mutable state in objects so it won't leak in your main application logic (let interval).
Don't use the substr function to format time because this solution is not robust. Write custom formatter or use a library like momentjs instead.

Here is my solution:
class Timer {
    constructor(duration, delay = 1000) {
        this._remaining = duration;
        this._delay = delay;
        this._callbacks = [];
    }

    start() {
        this._intervalId = setInterval(this._tick.bind(this), this._delay);
    }

    stop() {
        clearInterval(this._intervalId);
    }

    onTick(callback) {
        this._callbacks.push(callback);
    }

    _tick() {
        this._remaining -= this._delay;
        if (this._remaining <= 0) this.stop();
        this._callbacks.forEach((cb) => cb(this._remaining));
    }
}

class Timers {
    makeActive(timer) {
        this._activeTimer = timer;
    }

    stopActive() {
        if (this._activeTimer) this._activeTimer.stop();
    }
}

// I didn't change formatting implementation here because I am lazy
const formatHMS = (date) => date.toISOString().substr(11, 8);
const formatLocaleHMS = (date) => date.toLocaleTimeString().substr(0, 8);

const dateIn = (duration) => {
    const date = new Date(0);
    date.setMilliseconds(Date.now() + duration);
    return date;
}

const durationToDate = (duration) => {
    const date = new Date(0);
    date.setMilliseconds(duration);
    return date;
}

const timeControls = document.querySelector('.timer__controls');
const timeLeft = document.querySelector('.display__time-left');
const endTime = document.querySelector('.display__end-time');

const displayRemaining = (duration) => {
    timeLeft.textContent = formatHMS(durationToDate(duration));
}

const displayEnd = (duration) => {
    endTime.textContent = `Be back at ${formatLocaleHMS(dateIn(duration))}`;
}

const timers = new Timers();

const startAndUpdate = (duration) => {
    displayRemaining(duration);
    displayEnd(duration);

    const timer = new Timer(duration);
    timer.start();
    timer.onTick(displayRemaining);

    timers.stopActive();
    timers.makeActive(timer);
};

const secondsToMs = (seconds) => seconds * 1000;
timeControls.addEventListener('click', (e) => {
    const button = e.target.closest('button');
    if (button) startAndUpdate(secondsToMs(parseInt(button.dataset.time)));
});

const minutesToMs = (minutes) => secondsToMs(minutes * 60);
document.customForm.addEventListener('submit', function (e) {
    e.preventDefault();
    const value = this.minutes.value;
    if (value !== '') startAndUpdate(minutesToMs(parseInt(value)));
});


Iterate array with delay in between using promise I would like to write a nodejs IP driver for my AVR. In order to turn on my AVR and switch it to channel '321' i have to send key presses. A key press can be seen as a 'press key' and 'release key' command.

Basically, I have to write the following code:

press(turn_on)
delay(50ms)
release(turn_on)
delay(50ms)
press(3)
delay(50ms)
release(3)
delay(50ms)
etc.

I am using 'bluebird' and the communication towards the AVR is already written. All I am struggling with is how I can write the above set of commands with promises.

I came to the following code, which works, but I don't know if it is the most elegant and best solution.

var arr = [1,2,3,4,5];
Promise.mapSeries(arr, function (itm) {
      console.log('on: ' + itm);
      return Promise.delay(500).then(function() { console.log('off: ' +itm);
   }).delay(500);
});

The promise is to do the JOB
and report back when done. Promises are not for timeouts to communicate, they do that already via the call back, and a timeout's callback is as good as a promise, with a lot less overhead.
I have not looked at the code for BlueBird so I can not determine its quality and thus your code. But I see two promises where there should only be one, so there is room for improvement.
Why do you need a promise?
You have to consider why you want to have a promise. Creating a promise just so you can start another is adding an extra layer of unneeded complexity.
You want a promise so that you can have some code execute when the job is complete, or if there is a chance the job may not complete, you have a promise to have code executed for that situation.
Whenever you write any sort of code, always ask yourself, is this step really needed, if you can do without then it should not be there.
One promise
The following will do what you want with only one promise needing to be created. The timeouts handle the timing, and all you want to know is that you get a promise to know when the job is done.

const log = arg => console.log(arg);
const keyOn  = key => log("On : "  + key);
const keyOff = key => log("Off : " + key);

const keyClick = (key, ms, end) => {
    keyOn(key);
    setTimeout(() => {
        keyOff(key);
        setTimeout(end, ms);
    }, ms);
}
// using async function means that if the keys array is empty
// you can just let the function fall through to resolve
async function keySequence(keys, ms = 50){
    var complete;  // to hold the resolving promise callback
    const step = () => {
        keyClick(  // do a key click, keyDown 50ms, keyUp, 50ms
            keys.shift(), // get the next key
            ms,           // delay between keys
            (keys.length > 0 ? step : complete) // call next step or resolve
        );
    };
    if (keys.length) {  // only if there are keys
        step();  // start the first key
        return new Promise(done => complete = done ); // get the resolve from promise
    }
    // function fall through automaticly resolves the async promise.

}
keySequence(["1","2","3","4","5"]).then(() => log("done"));

The example can be simplified by having the keyClick body in the step function, I seperated them to make it clearer.
Personally the rampant bad use of promises sometimes makes me wish they were never added to the language. We have gone from callback hell, to the promise chain highway to hell, and by the look of the other answer, awaiting a million promises in hell.


Convert sequence of number into display string My requirement is to convert a sequence of numbers into a display string as below.

Example

  Input

  1,2,3,4,5,7,7,8,8,8,10,15,10,11,12,88,87,86

  Output

  1-5,7-8,10-12,15,86-88

void makeDisplayString(std::vector<int>& vector, std::wstring& string)
{
    if(vector.empty()) return; // If empty do nothing

    std::sort(vector.begin(),vector.end()); // Sort vector first

    int& preValue =vector.at(0);
    string.append(std::to_wstring(preValue));

    bool continueFlag = false; // Flag to check if continuation of value

    for(auto const & num : vector)
    {
        if(preValue ==num)  continue; // if same number as previous , Skip it

        if(preValue+1 == num) // If number is one more than privious value
        {
            preValue = num;
            continueFlag=true; // set continuation flag and continue rest part
            continue;
        }

        if(continueFlag)
        {
            // We reach here only if neither number is save as previous nor it is continous of numbering
            // But befor this number there was a continous numbering
            string.push_back(L'-');
            string.append(std::to_wstring(preValue));
            string.push_back(L',');
            preValue=num;

            string.append(std::to_wstring(num));
            continueFlag = false;
            continue;
        }
        // We reach here only if neither number is save as previous nor it is continous of numbering
        // Also befor this number there was no continous numbering
        preValue = num;
        string.push_back(L',');
        string.append(std::to_wstring(num));
    }
    // This is to handle a sequence ending with continous number
    if(continueFlag)
    {
        string.push_back(L'-');
        string.append(std::to_wstring(*vector.rbegin()));
    }
}

1. Separate data representation from actual output, using standard algorithms where possible.

make_ranges below takes input sequence array and returns limits of all ranges found, in two different sequences from, to:

template<typename F, typename T, typename A>
void make_ranges(F& from, T& to, A array)
{
    auto diff = array;
    std::sort(array.begin(), array.end());
    std::adjacent_difference(array.begin(), array.end(), diff.begin());

    std::vector<size_t> idx(array.size());
    std::iota(idx.begin(), idx.end(), 0);
    auto sep = [&diff] (size_t i) { return diff[i] > 1; };
    for(auto p = idx.begin(); p != idx.end();)
    {
        from.push_back(array[*p]);
        p = std::find_if(++p, idx.end(), sep);
        to.push_back(array[*(p - 1)]);
    }
}

2. For most generic use, use a stream for output:

template<typename S, typename I, typename J>
void print_range(S& stream, I i, J j)
{
    stream << *i;
    if (*j != *i)
        stream << "-" << *j;
}

template<typename S, typename F, typename T>
void print_ranges(S& stream, const F& from, const T& to)
{
    auto i = from.begin();
    auto j = to.begin();
    if (i != from.end())
        print_range(stream, i++, j++);
    while (i != from.end())
    {
        stream << ",";
        print_range(stream, i++, j++);
    }
}

3. For convenient use, provide a streaming operator interface:

template<typename S, typename A>
S& operator<<(S& stream, const array_ranges<A>& ranges)
{
    typename std::decay<A>::type from, to;
    make_ranges(from, to, std::get<0>(ranges));
    print_ranges(stream, from, to);
    return stream;
}

where array_ranges is a helper struct holding a sequence and indicating that its ranges should be streamed out instead of the sequence itself. Given a sequence, an array_ranges instance is made easily via helper function ranges:

template<typename A>
struct array_ranges : std::tuple<A> { using std::tuple<A>::tuple; };

template<typename A>
array_ranges<A> ranges(A&& a) { return array_ranges<A>{std::forward<A>(a)}; }

Example

Now the above can be used as

std::vector<int> arr{1,2,3,4,5,7,7,8,8,8,10,15,10,11,12,88,87,86};
std::cout << arr << std::endl;
std::cout << ranges(arr) << std::endl;

See live example.

If you want to print the ranges into a string, just use an std::ostringstream.

Discussion

With the above separation, you can do more advanced tasks like extracting ranges from arbitrary sequences, processing them (e.g. filtering or merging two sets of ranges), and finally printing them.

If you don't need any extra processing, this separation is more costly than direct printing since it stores intermediate results in arrays from, to. Using std::adjacent_difference introduces another intermediate result.

It is possible to save the extra cost of "intermediate results" while keeping the same design by more advanced techniques like expression templates, but this goes way beyond this simple task.

I would personally prefer a clean design to optimal performance for this task. This makes code more readable and maintainable. Also, with a bit more abstraction, you may find that parts of the code above can be reused elsewhere (especially print_ranges, but I don't want to go too far).

Another choice you could follow is to have an std::pair or (better) a dedicated data structure representing one range. A single sequence would then be enough instead of two sequences from, to, but processing such a sequence would be a bit more involved.

Of course, you are free to flat everything into a single call without intermediates if you prefer.


Determining length of a char* string in C++ I was looking to do this with sizeof() and I just gave up. As minimal as I can make it, I was wondering if there was a less crude way of doing the const char * as a convenience. I thought about doing this as a template, but I thought having another bit of generated code every time it is called seemed kinda dumb for me.

#include "misc.hh"
namespace mlcppl
  {
    int cpl(char * c)
    {
    char ct;
    int i = 0;

    while (ct = *(c+i))
      {
        if (ct == '\0')
          {
            break;
          }
        else
          {
            i++;
          }
      }
    return i;
  }

  int cpl(const char * c)
  {
    char * ct = (char*) c;
    return cpl(ct);
  }
}

What about:

std::cout << ::strlen("Plop Poop") << "\n";

While we are at it:

// use std::size_t to represent a size of something in memory
// It is non negative and is explicitly designed so it can represent
// the biggest in memory object.
//
// Pass C-String as 'char const*' (or const char* (samething))
// The compiler will automatically add a const to objects if required.
// BUT it will never remove a const (apart from 1 special case see below).
// Also the type of a string literal is 'char const*` (for the language lawyers yes it is an array but not relevant here as by the time we will see it is a pointer).
//
// Also, because you don't want to mutate the underlying string it gives
// you some slight protection from accidental assignment into the string.
std::size_t martinStringLen(char const* start)
{
    char const* end = start;

    // Look for the end of the string.
    // Your code increments two objects. I would just increment a single object
    // looking for the end of the string.
    for(;*end;++end){/*Empty*/}

    // The size is then just subtraction.
    return end - start;
}

// Unfortunately the special case is string literals: which is allowed to decay
// to char* for compatibility with old C code.

Other notes on your code:

int cpl(char * c)       // does work
int cpl(const char * c) // converts to char* so it can do work.

You have these two the wrong way around.
Make the int cpl(const char * c) version do the work. As noted above you are not mutating the object so it provides some slight protection from simple mistakes.

Also you will find the int cpl(char * c) version becomes unnecessary. As the compiler will automatically add const (ness) to parameters for you so you don't actually need the this version.

Other comments are same as @Konrad Rudolph

Note on the use of const.

If you put const on the right or left is a style thing. Peronally I always put it on the right. As there are a couple of corner cases (with typedef) were it does make a difference and I want to be consistent.

Note: cost always binds to the type on its left. unless it is the left-most part of a type declaration then it binds right. Read type right to left.

char const*       str1;  // const binds left to char  => (char const)*
                         // Read as: pointer to 'const char'

char const *const str2;  // const binds left => (char const) (* const)
                         // Read as: 'const pointer' to 'const char'

// From the C world (and it has leaked into C++) so a lot of people still us it
const char*       str3;  // const as left-most part binds right => (const char) *
                         // Read as: pointer to 'char const'

Basically str1 and str3 are identical. But the consistency of always thinking that const binds to the left makes it neater in my mind.

One place where it can make a difference is typedefs. This is because the typedef has already formed a type so any external const are applied to the typedef type as a whole thing (not as individual parts).

typedef char* String;
const String   str4;     // const is the left-most item so binds right.
                         // So you may expect this to be the same as str3

                         // unfortunately this is not correct as the typedef
                         // String a whole type so the const applies to `String`
                         // not to the char.

                         // Thus this is like const (char*)
                         // Which is the same as (char*) const
                         // Read: const 'pointer to char' so once set the pointer
                         // can not be moved.
 // If on the other hand you never put const in the left-most position then
 // this problem never even comes up.

So two reasons not to put const as the left-most part of the type.


Updating persons in database with newly created addresses I have the following code, and I'd like to refactor it to a more functional way:

public void processPersons(List<Person> personList) {
    for (Person person : personList) {
        Integer addressId = createAddress(person);
        if (addressId != null) {
            updateDbStatus(addressId, person);
        }
    }
}

How do I convert the above to a more functional style of programming?

One of variants is to have PersonWithAddress subclass with created addressIp:

    persons
            .stream()
            .map(PersonWithAddress::new)
            .forEach(this::updateDbStatus);

You call updateDbStatus where check the address for null:

void updateDbStatus(PersonWithAddress personWithAddress) {
    if (personWithAddress.getAddressIp() != null) {
        // do the update
    }
}

Model classes like these ones:

class Person{
    Person(final Person original) {
        // copy constructor
    }
}

class PersonWithAddress extends Person {
    private final Integer addressIp;

    PersonWithAddress(final Person person) {
        super(person);
        this.addressIp = createAddress();
    }

    Integer getAddressIp() {
        return addressIp;
    }

    private Integer createAddress() {
        // have a logic to create an address
        return 0;
    }
}


Calculating difference between cached period and requested There is a remote resource containing datasets arranged by time, data is static and doesnt change. Whenever we are going to fetch anything from there we will be saving it in our cache.
User might request a data for a particular time period, lets say from 1st Jan 2019 to 1st Jan 2020. If data is not cached, then we request it from remote resource, in case only part of data is cached, then we need to request what is missing. There is a function which calculates missing intervals, and I think that it looks too verbose. Would be nice to get some suggestions for a better solution. Thanks.
    @SneakyThrows
public static ArrayList<TimeSpan> difference(TimeSpan requested, TimeSpan cached){
    ArrayList<TimeSpan> missingIntervals = new ArrayList<>();
    //requested period is within cached
    if (requested.getFrom().compareTo(cached.getFrom()) >= 0 && requested.getTo().compareTo(cached.getTo()) <= 0){
        missingIntervals = null;
    //requested period is before or after cached
    } else if (requested.getTo().compareTo(cached.getFrom()) <= 0 || requested.getFrom().compareTo(cached.getTo()) >= 0){
        missingIntervals.add(requested);
    // requested start and end dates are outside of cached range
    } else if (requested.getFrom().compareTo(cached.getFrom()) <= 0 && requested.getTo().compareTo(cached.getTo()) >= 0){
        missingIntervals.add(new TimeSpan(requested.getFrom(),cached.getFrom()));
        missingIntervals.add(new TimeSpan(cached.getTo(),requested.getTo()));
    // requested start date is before cached period
    } else if (requested.getFrom().compareTo(cached.getFrom()) < 0
            && requested.getTo().compareTo(cached.getTo()) <= 0
            && requested.getTo().compareTo(cached.getFrom()) > 0){
        missingIntervals.add(new TimeSpan(requested.getFrom(),cached.getFrom()));
    // requested end date is after cached period
    } else if (requested.getTo().compareTo(cached.getTo()) >= 0
            && requested.getFrom().compareTo(cached.getFrom()) >= 0
            && requested.getFrom().compareTo(cached.getTo()) < 0){
        missingIntervals.add(new TimeSpan(cached.getTo(),requested.getTo()));
    }
    return missingIntervals;
}

Here is a TimeSpan class:
public class TimeSpan {

public TimeSpan(Date from, Date to) throws Exception{
    if(from.compareTo(to) >= 0){
        throw new Exception("Start date cannot be greater then end date");
    }
    this.from = from;
    this.to = to;
}

private final Date from;
private final Date to;

}

I would suggest moving this method into the TimeSpan class. Let a TimeSpan tell you about its overlaps. Don't interrogate it for internal details and then figure it out for yourself. Private helper methods will make the compareTo calls more readable.
Assorted other thoughts:
A checked exception is probably not right here. I'd suggest preferring IllegalArgumentException, since that's what the problem is. Using a more specific exception than Exception is highly recommended.
Returning early would be preferable to assigning a return value and only returning once. That is a holdover from C, which only allowed a single return.
It's generally preferable to use the List variable type when you don't care about the type of list you're using.
Traditionally, variables are defined at the top of a class, not the bottom.
Traditionally, whitespace is added between if and (, and between ) and {.
The Date class already has an after() method, which would make the TimeSpan constructor cleaner.
Your error message in the constructor is incorrect. The from date also cannot be equal to the to date.
You should strongly consider using java.time instead of the java.util time classes. They are effectively deprecated.
Returning null is probably strictly worse than returning an empty collection, though it's difficult to say that authoritatively without seeing the client code.
If you made all these changes, your code might look something like:
public final class TimeSpan {

    private final Date from;
    private final Date to;

    public TimeSpan(Date from, Date to) throws IllegalArgumentException {
        if (!from.before(to)) {
            throw new IllegalArgumentException("Start date cannot be greater than or equal to end date");
        }
        this.from = from;
        this.to = to;
    }

    /**
     * Subtracts the timeSpan argument from this TimeSpan. This will result in a list of between
     * zero and two TimeSpans, ordered by start time.
     * @param timeSpan the span to subtract from this timespan. May not be null.
     * @return a list of TimeSpans covering the time that is in this timespan and not the argument timespan. Will
     * never return null.
     */
    public List<TimeSpan> minus(TimeSpan timeSpan) {
        // This TimeSpan is completely inside the argument.
        if (timeSpan.contains(this.from) && timeSpan.contains(this.to)) {
            return Collections.emptyList();
        }

        // This TimeSpan is either fully before or fully after argument
        if (this.endsBeforeOrAt(timeSpan) || this.startsAfterOrAt(timeSpan)) {
            return listOf(this); // only OK because this class is immutable
        }

        // This TimeSpan starts before and ends inside the argument
        if (timeSpan.contains(this.to)) {
            return listOf(new TimeSpan(this.from, timeSpan.from));
        }

        // This TimeSpan starts inside and ends after argument
        if (timeSpan.contains(this.from)) {
            return listOf(new TimeSpan(timeSpan.to, this.to));
        }

        // This TimeSpan starts before and ends after argument
        List<TimeSpan> results = new ArrayList<>();
        results.add(new TimeSpan(this.from, timeSpan.from));
        results.add(new TimeSpan(timeSpan.to, this.to));
        return results;
    }

    private boolean endsBeforeOrAt(TimeSpan timeSpan) {
        return this.to.compareTo(timeSpan.from) <= 0;
    }

    private boolean startsAfterOrAt(TimeSpan timeSpan) {
        return this.from.compareTo(timeSpan.to) >= 0;
    }

    private boolean contains(Date date) {
        return (this.from.compareTo(date) >= 0) && (this.to.compareTo(date) <= 0);
    }

    // If you're using Guava, replace this with Lists.of()
    private static List<TimeSpan> listOf(TimeSpan timeSpan) {
        List<TimeSpan> results = new ArrayList<>();
        results.add(timeSpan);
        return results;
    }

}


Fix element on window scroll This code achieves the effect, but is it a resource friendly way of doing it?

Just to clarify: target div.fixMe and fix it to the top of the window when the user scrolls past its natural position. (I do realise there are plugins to achieve this result, but I wanted to build it myself)

Is this the best way to achieve the desired result? Is it a good idea that the whole function triggers whenever the user scrolls, or should I be splitting up code?

(function() {
var fixedElement = $('.fixMe').offset(),
    scrolled = $(window).scroll(function() {
        var winScrolled = $(this).scrollTop();
        if(winScrolled > fixedElement.top - 10) {
            $('.fixMe').css({'position': 'fixed','top' : '10px'})
        }
        else {
            $('.fixMe').css({'position': 'static'})
        }
    });
})()

Any feedback/criticism is welcome. I should probably get/set the width of div.fixMe to avoid display issues.

Two things I noticed/would recommend:

Cache jQuery variables such as $(this) and $('.fixMe') to prevent redundant lookups during each scroll event.
Implement an event debouncer/limiter, this is because the scroll event fires a lot more than you likely think it does. There are many utils/plugins to achieve this such as:

https://github.com/cowboy/jquery-throttle-debounce
http://unscriptable.com/2009/03/20/debouncing-javascript-methods/
http://documentcloud.github.com/underscore/

I even wrote a very simple one: http://limit.gotsomething.com/. My site will show you why debouncing/limiting is useful.

Other than that looks good to me!

cheers,
Marc.


Displaying an arbitrary number of buttons and automatically arranging the buttons It is supposed to take an arbitrary number of arguments, turn those arguments into buttons, and arrange those buttons in a way to give each individual button the largest area possible. Is there a better way to do this? Be as thorough as you like.

import sys
import tkinter as tk
import os

from tkinter import *
###functions
def calcRowsAndColumns(area = None):
    if(area==None):
        area = len(sys.argv) -1
    if(area==1):
        return (1,1)
    rectanglePropertiesList = []
    for rows in range(1, area):
        if(area % rows == 0):
            columns = int(area/rows)
            rectanglePropertiesList.append([rows,columns])
    if (len(rectanglePropertiesList) == 1): #This deals with prime numbers
        return calcRowsAndColumns(area+1)
    return tuple(smallestPerimeter(rectanglePropertiesList))

def smallestPerimeter(propertyList):
    return propertyList[propertyList.index(min(propertyList, key=sum))]

def preserve(gFile):
    print(gFile)
    argList.remove(gFile)
    for i in argList:
        os.remove(i)

def relwidth(root):
    return root.winfo_screenwidth()//(len(argList))-20
    #Subtracting 20 to account for padding

def relheight(root):
    return root.winfo_height()//(len(argList))-20
###Code to be executed
rowsAndColumnTuple = calcRowsAndColumns(None)
root = tk.Tk()
root.attributes("-fullscreen", True)
root.config(bg="white")
root.grid()

listButton = []

argList = [i for i in sys.argv[1:]]

for i in argList:
    listButton.append(Button(root, text=i.rpartition(os.sep)[2],
                             font="-size 20", command=lambda x=i: preserve(x),
                             #lambda used here to allow parameters to be passed to the preserve function
                             padx=10, pady=10,
                             bg ="black", fg = "red"))

iterButtons = iter(listButton)

for i in range(rowsAndColumnTuple[0]):
    root.grid_rowconfigure(i, weight=1)
    for j in range(rowsAndColumnTuple[1]):
        root.grid_columnconfigure(j, weight=1)
        try:
            button=next(iterButtons)
        except StopIteration:
            print("reached end of list : ignoring remaining grid cells")
            break
        button.grid(row=i,column=j)

root.update()

for i in listButton:
    i.config(height=relheight(root), width=relwidth(root))
    #This resizes the buttons to the correct size

root.mainloop()

Don't import tkinter twice
You're doing this:
import tkinter as tk
from tkinter import *

Remove the second import since you are already importing tkinter. That means that everywhere you reference a tkinter class or constant you need to prefix it with tk. (eg: tk.Button). This is much preferable over a wildcard import.
Let tkinter compute the size of the widgets
Since you are creating a grid, there's no reason to explicitly configure the width and the heights of the buttons. Instead, use the sticky attribute to tell tkinter to grow (or shrink) the widget to fit the area given to it.
Because you are properly giving the rows and columns weight, all you need to do is add the sticky attribute when adding the widget to the grid, and then remove the code that explicitly configures the width and height.
For example:
button.grid(row=i,column=j, sticky="nsew")

Don't use variables i and j to represent rows and columns
If you are iterating over rows and columns, use variable names to make that more clear. Variables named i and j carry no meaning. Use row and column (or something similar -- r, c, row_number, column_number, etc) instead. Also, there's no reason to save the results of calcRowsAndColumns to a tuple. Save the individual values as scalar variables.
In other words, instead of this:
rowsAndColumnTuple = calcRowsAndColumns(None)
...
For i in range(rowsAndColumnTuple[0]):
    root.grid_rowconfigure(i, weight=1)
    for j in range(rowsAndColumnTuple[1]):
        root.grid_columnconfigure(j, weight=1)

Do this:
rows, columns = calcRowsAndColumns(None)
for row in range(rows):
    root.grid_rowconfigure(row, weight=1)
    for column in range(columns):
        root.grid_columnconfigure(column, weight=1)

Remove the call to update
With the above changes, you don't need to call root.update() toward the end of the script.
Use classes
Your code is hard to understand at a high level. Part of the reason for this is that you have to define some functions before you create the actual GUI. That means I have to scan through the lines of code looking for where you create the window.
While some people find the use of classes daunting, they make writing GUI code much easier. You should consider moving all of this code into a single class, which will allow you to write the code in a manner that makes it more readable.
A description of my recommendation is in this answer on stackoverflow: https://stackoverflow.com/a/17470842/7432. In a nutshell, it suggests starting with this template:
import tkinter as tk

class MainApplication(tk.Frame):
    def __init__(self, parent, *args, **kwargs):
        tk.Frame.__init__(self, parent, *args, **kwargs)
        self.parent = parent
        # <create the rest of your GUI here>

if __name__ == "__main__":
    root = tk.Tk()
    MainApplication(root).pack(fill="both", expand=True)
    root.mainloop()

You can then move all of your code into MainApplication . This allows the main logic of creating the GUI to be up front rather than buried deep in the code.


Composing QGIS expression based on a QGIS layer tree I am developing a plugin for a GIS (Geographic Information Systems) software which uses Python. I have been using Python for over a year (on-off) and have the following code which works. It basically reads the names of files loaded into the GIS software to compose a QGIS expression for every group of files. Finally it deletes the list, ready for the next group.

The script is quite chunky and uses repetition. I am wondering how would one improve it to make it more compact and run more efficiently?

root = QgsProject.instance().layerTreeRoot()
policy_group = root.findGroup('Names')
formula_score4 = []
formula_score3a = []
formula_score3 = []
formula_score2a = []
formula_score2 = []
formula_score1 = []
for group in policy_group.children():
    for layer in group.children():
        score4 = '"' + layer.layerName() + '_Score' + '"' + ' = 4 OR '
        formula_score4.append(score4)
    for layer in group.children():
        score3a = 'coalesce(' + layer.layerName() + ' = 3, 0.00)*3 OR '
        formula_score3a.append(score3a)
    for layer in group.children():
        score3 = '"' + layer.layerName() + '_Score' + '"' + ' = 3 OR '
        formula_score3.append(score3)
    for layer in group.children():
        score2a = 'coalesce(' + layer.layerName() + ' = 2, 0.00)*2 OR '
        formula_score2a.append(score2a)
    for layer in group.children():
        score2 = '"' + layer.layerName() + '_Score' + '"' + ' = 2 OR '
        formula_score2.append(score2)
    for layer in group.children():
        score1 = '"' + layer.layerName() + '_Score' + '"' + ' = 1 OR '
        formula_score1.append(score1)
    formula1 = "CASE WHEN " + "".join(str(x) for x in formula_score4) + "".join(str(x) for x in formula_score3a) + ">=9 THEN 4 " + "WHEN " + "".join(str(x) for x in formula_score3) + "".join(str(x) for x in formula_score2a) + ">=6 THEN 3 " + "WHEN " + "".join(str(x) for x in formula_score2) + "THEN 2 " + "WHEN " + "".join(str(x) for x in formula_score1) + "THEN 1 ELSE 1 END"
    formula2 = formula1.replace("OR >=9 THEN 4 ", ">=9 THEN 4 ")
    formula3 = formula2.replace("OR >=6 THEN 3 ", ">=6 THEN 3 ")
    formula4 = formula3.replace("OR THEN 2 ", "THEN 2 ")
    final_formula = formula4.replace("OR THEN 1 ELSE 1 END", "THEN 1 ELSE 1 END")
    del formula_score4[:]
    del formula_score3a[:]
    del formula_score3[:]
    del formula_score2a[:]
    del formula_score2[:]
    del formula_score1[:]

I have included a screenshot of running the above code in the Python Console of QGIS. I believe the result is what I was wanting from the code as it matches the same sql expression I wrote manually which is stored in a text file (instead of hardcoding the names of layers and reading the textfile, I wanted a more dynamic approach). Here is the image:

And here is the output for the first group (formatted for readability):

'CASE WHEN "Layer_1a_Score" = 4 OR
"Layer_1b_Score" = 4 OR
"Layer_1c_Score" = 4 OR
"Layer_1d_Score" = 4 OR
"Layer_1e_Score" = 4 OR
"Layer_1f_Score" = 4 OR
coalesce(Layer_1a = 3, 0.00)*3 OR
coalesce(Layer_1b = 3, 0.00)*3 OR
coalesce(Layer_1c = 3, 0.00)*3 OR
coalesce(Layer_1d = 3, 0.00)*3 OR
coalesce(Layer_1e = 3, 0.00)*3 OR
coalesce(Layer_1f = 3, 0.00)*3 >=9
THEN 4 WHEN
"Layer_1a_Score" = 3 OR
"Layer_1b_Score" = 3 OR
"Layer_1c_Score" = 3 OR
"Layer_1d_Score" = 3 OR
"Layer_1e_Score" = 3 OR
"Layer_1f_Score" = 3 OR
coalesce(Layer_1a = 2, 0.00)*2 OR
coalesce(Layer_1b = 2, 0.00)*2 OR
coalesce(Layer_1c = 2, 0.00)*2 OR
coalesce(Layer_1d = 2, 0.00)*2 OR
coalesce(Layer_1e = 2, 0.00)*2 OR
coalesce(Layer_1f = 2, 0.00)*2 >=6
THEN 3 WHEN
"Layer_1a_Score" = 2 OR
"Layer_1b_Score" = 2 OR
"Layer_1c_Score" = 2 OR
"Layer_1d_Score" = 2 OR
"Layer_1e_Score" = 2 OR
"Layer_1f_Score" = 2
THEN 2 WHEN
"Layer_1a_Score" = 1 OR
"Layer_1b_Score" = 1 OR
"Layer_1c_Score" = 1 OR
"Layer_1d_Score" = 1 OR
"Layer_1e_Score" = 1 OR
"Layer_1f_Score" = 1
THEN 1 ELSE 1 END'
...

You do quite a lot or repetition. Using str.format you should be able to make your code smaller by using a comprehensions or map.
Doing these inside a ''.join can allow you to reduce a lot of your code.
And if you make a list to iterate over you can do this all in two comprehensions.
I also think you should make a comprehension to get the layer name from your group.children().
Finally you will want to build formula1 from this which can be done with another string with format.

As all the strings used to format the data are constant, you can use UPPER_SNAKE_CASE variable names to say it's a constant.
And should result in something like:

SCORE_FORMATS = [
    '"{}_Score" = 1 OR ',
    '"{}_Score" = 2 OR ',
    '"{}_Score" = 3 OR ',
    '"{}_Score" = 4 OR ',
    'coalesce({} = 2, 0.00)*2 OR ',
    'coalesce({} = 3, 0.00)*3 OR ']
FORMULA1 = "CASE WHEN {3}{5}>=9 THEN 4 WHEN {2}{4}>=6 THEN 3 WHEN {1}THEN 2 WHEN {0}THEN 1 ELSE 1 END"

Finally to make the main part of your code, formula1 to final_formula, you can use the following as I explained above:

for group in policy_group.children():
    group_children = [layer.layerName() for layer in group.children()]
    formulas = [
        ''.join(map(formula_format.format, group_children))
        for formula_format in SCORE_FORMATS
    ]
    formula1 = FORMULA1.format(*formulas)
    formula2 = formula1.replace("OR >=9 THEN 4 ", ">=9 THEN 4 ")
    formula3 = formula2.replace("OR >=6 THEN 3 ", ">=6 THEN 3 ")
    formula4 = formula3.replace("OR THEN 2 ", "THEN 2 ")
    final_formula = formula4.replace("OR THEN 1 ELSE 1 END", "THEN 1 ELSE 1 END")

The output of this function only uses final_formula where I was originally unsure if you were using all the formulas.
You also used the wrong delimiter for the coalesce.

To improve on the above I'd change SCORE_FORMATS to be tuples of (delimiter, format).
This will allow you to easily change both for all your different functions.
This will also change the ''.join(map(...)) to use your delimiter and will remove the need for all the replace calls.

This unfortunately makes the FORMULA a bit longer,
but give us an excuse to use implicit string concatenation which can make it much easier to read.

SCORE_FORMATS = [
    (' OR ', '"{}_Score" = 1'),
    (' OR ', '"{}_Score" = 2'),
    (' OR ', '"{}_Score" = 3'),
    (' OR ', '"{}_Score" = 4'),
    (' + ', 'coalesce({} = 2, 0.00)*2'),
    (' + ', 'coalesce({} = 3, 0.00)*3')]
FORMULA = ('CASE WHEN {3} OR {5} >= 9 THEN 4 '
            'WHEN {2} OR {4} >= 6 THEN 3 '
            'WHEN {1} THEN 2 '
            'WHEN {0} THEN 1 '
            'ELSE 1 END')

for group in policy_group.children():
    group_children = [layer.layerName() for layer in group.children()]
    formula = FORMULA.format(*[
        delimiter.join(map(formula_format.format, group_children))
        for delimiter, formula_format in SCORE_FORMATS
    ])


Finding the longest repeated substring in a string After optimising my code it still seems pretty brute force and I am looking for ideas to fundamentally improve it.

I have tried improving performance by:

Avoiding creating substrings as much as possible - using while loop to compare string instead.
Intelligently anticipating ends ie. start + shift + max < str.length() - 1
use int instead of Integers

Is there anything obvious I have missed? I have found a few  alternatives solutions on the internet and would love to hear an opinion how they are different/better/worse to my implementation:

Princeton http://introcs.cs.princeton.edu/java/42sort/LRS.java.html
Boyer–Moore string search algorithm
Or any of the other string search implementations (the most notable can be found on wikipedia)

And before I forget this is my code:

 public static String findLongestRepeatedSubString(String str) {
    int max = 0;
    String result = "";
    for (int start = 0; start + max < str.length() - 1; start++) {
        for (int shift = 1; start + shift + max < str.length() - 1; shift++) {
                int length = 0;

                // While characters match count the length
                while(str.charAt(start + length) == str.charAt(start + shift + length) // matching characters
                        && start + shift + length < str.length() - 1){ // hasn't reached the end yet
                    length++;
                }

                // If the length is larger - update the new max size
                if (length > max ) {
                    max = length;
                    result = str.substring(start, start + length + 1);
                }
        }
    }

    return result;
}

Off-by-one error

You have an off-by-one error in the innermost while loop: for input string "aa", this will not reach length 1, and give empty string as the result, which is incorrect.

I looked at this loop with suspicion, because I noticed that it checks the string bound in the second condition. The more natural code organization is to check the bound first, and then the second condition &&-ed to the first will be safe.

I suggest to rearrange that way, and for that you will need to rethink and verify all the indexes and ranges there. Which can be pretty hard to get right. So at this point, before touching anything, I suggest to add a bunch of Junit tests first, try to cover all the corner cases.

More bugs

I tried a bunch of random examples, and the program gives incorrect results:

"abac" -> "ab"
"abcac" -> "ab"
"abcba" -> "bc"

As I suggested in the previous point,
it will be good to add Junit tests to verify the logic.

Longest repeated substring problem

Quoting from Wikipedia:

  This problem can be solved in linear time and space by building a suffix tree for the string, and finding the deepest internal node in the tree. Depth is measured by the number of characters traversed from the root.

I think you should look into that and abandon your current implementation.


Simplifying casting in my C/C++ qsort char** comparator function I have a comparator function I use with qsort to sort an array of char* (i.e., char**), which compiles with current versions of Clang and GCC without giving errors or warnings:

static int
comparator(const void* a, const void* b)
{
#ifdef __cplusplus
    auto a_recast = const_cast<void *>(a);
    auto b_recast = const_cast<void *>(b);
    auto a_rerecast = reinterpret_cast<char **>(a_recast);
    auto b_rerecast = reinterpret_cast<char **>(b_recast);
    auto a_rererecast = const_cast<const char **>(a_rerecast);
    auto b_rererecast = const_cast<const char **>(b_rerecast);
    return std::strcmp(*a_rererecast, *b_rererecast);
#else
    return strcmp(*(const char**) a, *(const char**) b);
#endif
}

It compiles conditionally, that is, depending on whether a C or C++ compiler is used, one or the other block of code is compiled.

I offer both blocks, because this is part of a program that can be compiled alone in C, or in a larger suite of tools compiled with a C++ toolkit.

My question is about the work I had to do to in the C++ block, where I strip const, recast to char **, and reapply const.

Is there a simpler or cleaner way to do this C++-style casting, which generates the correct end results and does not result in warnings about C-style casting?

Clang, in particular, is chatty with warnings about old C-style casts, and I like my build logs to be clean, where fixes are realistic.

I also like the code to be clear and easy to and maintain. I'd also like to inline this code, which would probably be easier for the compiler if the comparator function is smaller.

If I'm doing the correct work, that's fine, but if there are better ways, I'd be interested to learn them. Thanks for your advice.

How about this?

int comparator(const void* a, const void* b)
{
#ifdef __cplusplus
    auto a_recast = static_cast<const char* const *>(a);
    auto b_recast = static_cast<const char* const *>(b);
    return std::strcmp(*a_recast, *b_recast);
#else
    return strcmp(*(const char* const *) a, *(const char* const *) b);
#endif
}

To elaborate, the reason that you had to go through the convoluted series of dangerous casts is that you actually were stripping constness.
const void* means a promise not to change whatever the void turns out to be. That is, the thing pointed at by the rightmost star is const. const char ** on the other hand means a promise not to change the individual characters, but doesn't guarantee that you won't change the direct pointers to those characters. However, it's the pointers to those characters that are pointed at by the rightmost star. Intuitively perhaps, think of char * as being in a unit by itself a c_string, and your a and b are arrays of constant c_strings.

The reason that clang warns about c style casts is precisely that they let you apply such a crazy list of operations and changing constnesses without necessarily realizing what is happening.

As an aside, as far as the compiler is concerned, it is actually only necessary to enforce constness on the pointer. That is, you could also compile with static_cast<char * const *>. I put the additional const in because when your conceptual c_string is constant, you conceptually don't want to be changing any of the characters. (And you probably want the compiler to alert you if anything tries to do that.)


Highest floor of a building from which to drop an egg without breaking it Problem description:

You have two eggs, and need to find the maximum floor of a 100-floor building from which you can drop the eggs without them breaking. Do this with as few attempts as possible.
NB! You are passed the level of where it breaks, n, and an egg is considered broken when your compares and finds test floor is larger than n. You can try to drop the egg as many times as you want (compare test floor and n), but you can only be above twice. Returning n-1 is cheating, consider n to be unknown!

I created a function which takes an argument of the floor, and then calculates the answer (since creating the physics engine to actually simulate the gravity and all that was out of scope).
I'd like to be reviewed on style, accuracy, efficiency, bugs/edge cases, and take into account I did this in a couple minutes while running through interview sample questions that I found on Glassdoor.
def egg_drop(n):
    ''' Find the highest floor in a building that an egg can drop from without breaking '''
    t = [1,11]
    while t[0] < 100:
        if t[1] >= n:
            print 'break_an_egg on floor %d' % t[1]
            for i in range(t[0],t[1]):
                if i >= n:
                    print  'break_an_egg on floor %d' % i
                    return i - 1
        else:
            t[0] += 10
            t[1] += 10
    return 100

Here are some comments on your question:

Bad problem description – I can't help but to make a method which find the input parameter, seems kind of silly. It could simply do return n-1 and you're done. It would have been better if the problem description gave a predefined method which you could call at most two times before it gave some error. But I digress...
Choose good names for your variables – Two store the lower and higher test floors in a list t is bad. What is t[0] or t[1]. Say low, highis way better, and even they can be improved upon
Assign multiple values at once – If you want to use one line to assign multiple values you can do: low, high = 1, 11
Don't use magic numbers – If you have magic numbers, like the number of floors, do either declare them as constants at the top or as default parameters to the method in question
Give proper text output – It took me quite some time to understand what your output actually was meant to describe. It would have been much clearer if you had said "First egg broke on floor x" and "Second egg broke on floor y"
Switch to new style print formatting and function – It is recommend to switch to the new string.format() style for print output. I.e. print ('First egg broke on {}'.format(high))
Error handling is good – As the problem was limited to a 100 floor building, one could/should also let the code reflect that. One way can be to assert legal floor levels, and another could be to make sure that when returning you don't return an illegal level.

Finally you can reduce the number of attempts by a little if you start out attempting with a little higher delta value/range and let it shrink as you get closer to the top1. It can be shown that the optimal delta offset is the result of  \$\frac{\sqrt{1 + 8\cdot {floors}} - 1}{2}\$. With these suggestions, and a simple loop to test for all available floors we get:

import math

MAX_FLOOR = 100

def safest_egg_drop_floor(n):
    """Return the safest floor to drop an egg from."""

    assert 1 <= n <= MAX_FLOOR, 'Building has floors from 1 through {}'.format(MAX_FLOOR)

    # Calculate the optimal delta to start with, to reduce attempt numbers
    delta = int(math.ceil((math.sqrt(1 + 8*MAX_FLOOR) - 1)/ 2))

    low, high = 1, delta

    while low < MAX_FLOOR:
        if high >= n:
            print('  First egg broke on floor {}'.format(high))

            for i in range(low, high+1):
                if i >= n:
                    print('  Second egg broke on floor {}'.format(i))
                    if i == 1:
                        raise ValueError("Eggs will always break")
                    else:
                        return i - 1
        else:
            delta -= 1
            low, high = high, high + delta
            if high > MAX_FLOOR:
               high = MAX_FLOOR

    return MAX_FLOOR

def main():

    for i in range(0, MAX_FLOOR+2):
        print('\nFind egg drop when it breaks at {}'.format(i))
        try:
            max_floor = safest_egg_drop_floor(i)
            print('    Safest drop from floor {}'.format(max_floor))
        except (ValueError, AssertionError) as error:
            print(error)

Which outputs:

Find egg drop when it breaks at 0
Building has floors from 1 through 100

Find egg drop when it breaks at 1
  First egg broke on floor 14
  Second egg broke on floor 1
Eggs will always break

Find egg drop when it breaks at 2
  First egg broke on floor 14
  Second egg broke on floor 2
    Safest drop from floor 1

...

Find egg drop when it breaks at 100
  First egg broke on floor 100
  Second egg broke on floor 100
    Safest drop from floor 99

Find egg drop when it breaks at 101
Building has floors from 1 through 100

1 See http://datagenetics.com/blog/july22012/index.html for a better explanation.


Assigning list index values to String I am just trying to convert each list of values to String by index one by one. My Code is running fine. But I am thinking that I can write better code than this. I Need your good coding possibilities here to convert from list to String.

I am using the code below to convert:

CarComment[] list = form.getCarDataJaxb().getCarCommentList();
JSONObject obj = new JSONObject();
JSONArray comments = new JSONArray();
for (int i=0;i<list.length;i++){
    if (notEmpty(list[i].getText())){
        JSONArray commObj = new JSONArray();

        String str = list[i].getText();
        str = str.replaceAll("<", "&lt;");
        commObj.put(str);

        comments.put(commObj);
    }
}
obj.put("carcomm", comments);
response.setContentType("text/json");
response.getWriter().write(obj.toString());

I am trying to write good coding technique while converting from list to String at line
String str = list[i].getText();

There actually isn't so much to improve here (at least not that I can think of).

Instead of using an index to iterate by, you can use a Java for-each loop. Since you only used the iterator variable to read the index of the list, it is usually more readable to use a for-each loop.

You were not indenting your code correctly (before the edit of your question), which I also fixed below.

And, below I use the opposite way of checking if the text is empty, by using continue if the if-statement is not true, you can avoid having to indent the code an extra step.

CarComment[] list = form.getCarDataJaxb().getCarCommentList();
JSONObject obj = new JSONObject();
JSONArray comments = new JSONArray();
for (CarComment carComment : list) {
    if (!notEmpty(carComment.getText()))
       continue;
    JSONArray commObj = new JSONArray();
    commObj.put(carComment.getText().replaceAll("<", "&lt;"));
    comments.put(commObj);
}
obj.put("carcomm", comments);
response.setContentType("text/json");
response.getWriter().write(obj.toString());

Preferably, to avoid double-negation, you should use if (isEmpty(...)) instead of if (!notEmpty(...))

However, there's no way of having to do a method call to getText (or a similar method call) on your line.

String str = list[i].getText();

What I did was to skip the declaration of a local variable and process the string (by calling replaceAll) directly when adding it to it's destination.


Timing how long garbage collection is taking in C# I'm concerned about how much time my application could be spending during garbage collection so I'm trying to figure out how to add some code to instrument this.

Based on some examples:

https://msdn.microsoft.com/en-us/library/cc713687(v=vs.110).aspx
http://www.codeproject.com/Articles/101136/Garbage-Collection-Notifications-in-NET

I've come up with the code below. I'm see GC taking around 200 ms locally.
Does anyone know if the code below is reasonable? Or is there a better way?

class Program
{
    public static void Main(string[] args)
    {
        var done = false;
        var load = new List<byte[]>();

        var pollGC = new Action(() =>
        {
            // Register for a notification.
            GC.RegisterForFullGCNotification(10, 10);
            Console.WriteLine("Registered for GC notification.");

            Stopwatch gcTimer = new Stopwatch();

            while (!done)
            {
                // Check for a notification of an approaching collection.
                GCNotificationStatus s = GC.WaitForFullGCApproach();
                if (s == GCNotificationStatus.Succeeded)
                {
                    Console.WriteLine("GC is about to start.");
                    load.Clear();
                    gcTimer.Restart();
                }

                // Check for a notification of a completed collection.
                s = GC.WaitForFullGCComplete();
                if (s == GCNotificationStatus.Succeeded)
                {
                    Console.WriteLine("GC has finished in {0} ms", gcTimer.ElapsedMilliseconds);
                }

                Thread.Sleep(500);
            }
            GC.CancelFullGCNotification();

            Console.WriteLine("Finished monitoring GC");

        });

        var doWork = new Action(() =>
        {
            while (!done)
            {
                try
                {
                    load.Add(new byte[10000]);
                }
                catch (OutOfMemoryException)
                {
                    Console.WriteLine("Out of memory. {0}", load.Count);
                }
            }
        });

        Console.WriteLine(GCSettings.IsServerGC);

        Task.Run(pollGC);
        Task.Run(doWork);

        Console.ReadLine();
        done = true;
        GC.CancelFullGCNotification();

        Thread.Sleep(2000);
    }
}

For continuous monitoring I agree with Zer0 to look at the % time spent in GC performance counters.

However for a session based analysis a more informative approach would be to collect the ETW traces from .NET. You can see here for the different GC related events, GCStart and GCEnd would most likely be relevant for you.

view the .NET documentation for Garbage Collection ETW Events

You can specify the GC Collect Only option exists which would be very useful here.

  The GC Collect Only Checkbox - Turns off all providers except those
  that describe garbage collections. Thus even the GC allocation
  sampling is turned off. This mode logs even less data than GC Only,
  and thus can collect data over an every longer period of time (say a
  day) in reasonable size (say 200 meg). The /GCCollectOnly command line
  option achieves the same effect.