SQL-Server Merge Statement with .NET DataTable Background:
I'm trying to synchronize order information between Oracle and SQL-Server databases and allow users to be able to track any changes between what has been replicated via an ASP.NET web page.  Users will be able to select which line items they want to be able to work on via this page and the remainder will be excluded going forward.  Users will also have the option to change which items have been excluded at any time until the process has been finalized.

The initial load of this order information replicates (from Oracle to SQL-Server) all of the line items and presents them for assignment to the user.  Any subsequent load of the order will compare replicated data to the original data.  I'm using the following statement to handle the replication and reconciliation based on user selections:

CREATE TABLE #tempSpecOrderReplication
(
    Ord_L_ID INT,
    Order_Number INT,
    Ord_ID INT,
    Item_Number VARCHAR(50),
    Quantity DECIMAL(18, 2),
    UOM VARCHAR(50),
    Price DECIMAL(18, 2),
    Is_Spec_Order BIT
);

INSERT INTO #tempSpecOrderReplication
([Ord_L_ID], [Ord_ID], [Quantity], [UOM], [Price],
 [Is_Spec_Order], [Item_Number], [Order_Number])
VALUES (...); --These values come from the .NET DataTable

MERGE Spec_Order_Replication WITH (HOLDLOCK) AS target
USING (SELECT * FROM #tempSpecOrderReplication) AS source
ON (target.Ord_L_ID = source.Ord_L_ID)
WHEN MATCHED THEN
    UPDATE SET Is_Spec_Order = source.Is_Spec_Order
WHEN NOT MATCHED THEN
    INSERT
    (Ord_L_ID, Order_Number, Ord_ID, Item_Number,
     Quantity, UOM, Price, Is_Spec_Order)
    VALUES
    (source.Ord_L_ID, source.Order_Number, source.Ord_ID, source.Item_Number,
     source.Quantity, source.UOM, source.Price, source.Is_Spec_Order);

DROP TABLE #tempSpecOrderReplication

Questions:

Is there any way I can improve the existing SQL statement?
Is there a better way that I can go about this?

I'm using SQL-Server version 2008 R2.

Please ask if you need any further information.

Well done.
I honestly cannot find anything bad to say about your SQL code. Your capitalization of keywords and indentation are consistent. Your query is properly explicit, there is no guessing work that the SQL engine would have to make. You clean up after your operation by dropping your #tempSpecOrderReplication.
The only thing that looked unusual, though not bad, is that you used square brackets in this clause but nowhere else. I would personally prefer if it were consistent thoughout, if I had to maintain it. But it's just a nitpick.
INSERT INTO #tempSpecOrderReplication
([Ord_L_ID], [Ord_ID], [Quantity], [UOM], [Price],
 [Is_Spec_Order], [Item_Number], [Order_Number])
VALUES (...); --These values come from the .NET DataTable


UWP scan for music files I am implementing a music player for Windows 10, and I am in a bit of a pickle at the library phase.

I am scanning for all the files in a music library (plus optional folders the user selects) and I need to get their tags (only need the title and artist; don't need complex details).

The problem is performance. Getting the music properties cost a LOT if you have a large media library. At the moment I am doing the following:

private void UserControl_Loaded(object sender, RoutedEventArgs e)
{
    Task.Factory.StartNew(async () =>
    {
        try
        {
            var files = await StorageHelper.GetFiles(KnownFolders.MusicLibrary);
            List<LocalAudio> tempList = new List<LocalAudio>();
            foreach (var file in files)
            {
                MusicProperties properties = await file.Properties.GetMusicPropertiesAsync();

                var title = string.IsNullOrWhiteSpace(properties.Title) ? file.Name : properties.Title;
                var artist = string.IsNullOrWhiteSpace(properties.Artist) ? file.Path : properties.Artist;
                tempList.Add(new LocalAudio() { Title = title, FilePath = file.Path, Artist = artist, Duration = properties.Duration });
                if (tempList.Count > 50)
                {
                    await AddToLibrary(tempList);
                    tempList.Clear();
                }
            }
            await AddToLibrary(tempList);
            tempList.Clear();
        }
        catch (Exception ex)
        {
            //log exception here
        }
    });
}

private async Task AddToLibrary(List<LocalAudio> tempList)
{
    await Dispatcher.RunAsync(Windows.UI.Core.CoreDispatcherPriority.Normal, () =>
    {
        foreach (var item in tempList)
        {
            this.LocalFiles.Add(item);
        }
    });
}

Where LocalAudio is my audio model and this.LocalFiles is an ObservableCollection. Also, the StorageHelper.GetFiles(StorageFolder) method returns an IEnumerable of StorageFiles (music files) (it's a recursive method to go in depth).

I am adding batches of 50 files on the dispatcher thread so I won't block the UI (I know, even if it is an async method running in a task that I started by fire-and-forget it still blocks the UI if I add them continuously...).

How can I optimize this better? Any idea is good. I was thinking to scan them once and save the music properties (title, artist and duration) in a file and read that file on application start and just sync the filename with the data I have in that file (a dictionary or something and read the file and keep it in memory). Not sure how good this idea is.

Task.Factory.StartNew(async () =>

I don't see any expensive non-async operations here, so I would consider directly running this code on the UI thread. It would also mean that you don't have to use Dispatcher.RunAsync in AddToLibrary.

Getting the properties will stay asynchronous, so it shouldn't block the UI.

List<LocalAudio> tempList = new List<LocalAudio>();

You can use var here, since the type of the variable is clear.

All async methods should be named ending with Async, in your case, that means AddToLibraryAsync.

I don't know if this is going to help, but you might consider getting the music properties in parallel. For example (using Batch from MoreLINQ or your own implementation):

var files = await StorageHelper.GetFiles(KnownFolders.MusicLibrary);

foreach (var batch in files.Batch(50))
{
    IEnumerable<Task<LocalAudio>> propertiesTasks = batch.Select(async file =>
    {
        MusicProperties properties = await file.Properties.GetMusicPropertiesAsync();

        var title = string.IsNullOrWhiteSpace(properties.Title) ? file.Name : properties.Title;
        var artist = string.IsNullOrWhiteSpace(properties.Artist) ? file.Path : properties.Artist;
        return new LocalAudio { Title = title, FilePath = file.Path, Artist = artist, Duration = properties.Duration };
    });

    LocalAudio[] properties = await Task.WhenAll(propertiesTasks);

    AddToLibrary(properties); // I'm assuming AddToLibrary is no longer async
}

You seem to be worried that adding the data to your UI will take too long. The solution to that is to not show the user so much data. Some solutions to that are only showing one "page" of data at a time or using virtualization to not create more UI elements than necessary (I have no idea if or how well does virtualization work on UPW).

Another possibility is to use paging, virtualization or just showing only a limited number of items, and combine it with some form of filtering. That way, when a user is looking for a song, they for example can type its name, instead of scrolling though a long list.


BST insertion and in order traversal package com.goman;

public class BST {

    private BST left;
    private BST right;
    private int data;

    public BST(int data, BST left, BST right){
        this.left = left;
        this.right = right;
        this.data = data;
    }

    public BST insert(BST node, BST root){
        if(root == null){
            return node;
        }

        if(root.data > node.data){
            root.left = insert(node, root.left);
        } else if(root.data < node.data){
            root.right = insert(node, root.right);
        } else{
            System.out.println("Duplicate valuesa are not allowed");
            return root;
        }

        return root;
    }

    @Override
    public String toString() {
        return "BST [left=" + left + ",right=" + right + ",data=" + data
                + "]";
    }

    public void inOrder(BST root){
        if(root==null){
            return;
        }
        inOrder(root.left);
        System.out.println(root.data);
        inOrder(root.right);
    }
}

class Test{
    public static void main(String[] a){
        BST root = new BST(5, null, null);
        root.insert(new BST(3, null, null), root);
        root.insert(new BST(8, null, null), root);
        root.insert(new BST(2, null, null), root);
        root.insert(new BST(4, null, null), root);
        root.inOrder(root);
        System.out.println(root);
    }
}

Broken API

The class name BST suggests that it's a binary search tree,
but it's not.
It's a node (in a binary tree), with private references to other nodes.

If it's intended to be a node in a binary search tree,
its API is broken.
The insert method takes two parameters,
both of them BST instances.
It works fine when the first parameter is a BST instance without children (left and right nodes both null),
but if you pass to it two nodes with children,
the result will be no longer a binary search tree,
for example:

BST root = new BST(5, null, null);
root.insert(new BST(3, null, null), root);

BST root2 = new BST(7, null, null);
root2.insert(new BST(3, null, null), root2);

root2.insert(root2, root);
root.inOrder(root);

System.out.println(root);

This will output:

3
5
3
7
BST [left=BST [left=null,right=null,data=3],right=BST [left=BST [left=null,right=null,data=3],right=null,data=7],data=5]

The result corresponds to this tree, which is not a binary search tree:

  5
 / \
3   7
   /
  3

You could avoid this by allowing to pass as parameter only data, not another BST instance.

Note that the constructor suffers from the same problem,
because the left and right parameters to insert arbitrary BST instances with children,
breaking the binary search tree property.

Strange API

The insert method doesn't use any fields or methods of the instance.
It could have been a static method.
It would have made more sense to use this method signature:

public void insert(int data) {

This method could use this.data, this.left, and this.right to perform inserting a new value,
and it could successfully protect the binary search tree property.

The inOrder method has the same issue.
I suggest to drop the BST argument.

After you rewrite the insert method this way,
and also drop the dangerous left and right constructor parameters,
the example code in your question will become simpler and more natural too:

BST root = new BST(5);
root.insert(3);
root.insert(8);
root.insert(2);
root.insert(4);
root.inOrder();
System.out.println(root);

Extension ideas

Instead of int as the data,
you could make this class generic,
by declaring as public class BST<T extends Comparable<T>>,
and using type T everywhere instead of int,
and rewriting x < y comparisons with x.compareTo(y) < 0.

Instead of inOrder printing data,
you could allow a Consumer<T> argument,
to let users decide what they want to do with the visited nodes.
For example, if you rewrite inOrder like this:

public void inOrder(Consumer<T> consumer) {
    left.inOrder(consumer);
    consumer.accept(data);
    right.inOrder(consumer);
}

Then users could print the node values in in-order with:

root.inOrder(System.out::println);

toString value hard to read

The toString method returns this for the tree in your example:

BST [left=BST [left=BST [left=null,right=null,data=2],right=BST [left=null,right=null,data=4],data=3],right=BST [left=null,right=null,data=8],data=5]

This is hard to read, because the data is printed after the children.
I suggest to print data before the children,
and also the implementation maybe slightly easier to read using String.format:

return String.format("BST [data=%s,left=%s,right=%s]", data, left, right);


'Genetic algorithm' implementation for constructing a battle mech This is a simple but complete console application where I experiment with genetic algorithm to try to figure out the best way to construct a battle mech that would win in a simple turn-based combat.

I welcome your criticism and pointers about what I could improve in the code above: architecture-wise, technique-wise, presentation-wise, and whatever else catches your interest. To provide at least a sample code as per the local rules, this is the method to make two mechs meet in combat:

// Match::match()
// ==============
// A static method that can be used to execute a match between the two provided mechs.
// Return value: 0 = draw, 1 = mech #1 wins, 2 = mech #2 wins.

int Match::match (Mech& m1, Mech& m2) {

    m1.resetCombatValues();
    m2.resetCombatValues();

    int turn = 0;
    static const int turnLimit = 25;

    // Each turn:
    while (turn < turnLimit) {

        m1.actCombatTurn (m2);
        m2.actCombatTurn (m1);

        bool mech1Alive = m1.isAlive();
        bool mech2Alive = m2.isAlive();

        if (!mech1Alive && !mech2Alive) return 0;

        if (!mech1Alive) return 2;
        if (!mech2Alive) return 1;

        turn++;

    }

    // Turn limit reached.
    return 0;

}

Here's a summary of my comments in the answer format:

Instead of coded return values, I'd consider using an enumeration for clarity: http://en.cppreference.com/w/cpp/language/enum
// A good rule of thumb is that if you have to document a special meaning of (implicit) enumerators, like "0 = draw, 1 = mech #1 wins, 2 = mech #2 wins", then it's probably better to use an explicit enumeration.
I would also suggest using a namespace non-member function instead of a static member function whenever possible: https://stackoverflow.com/a/1435105/859774
Further, unless you're supporting negative turns count turnLimit or want turn to go backward (would that even make sense in your context? BTW, turn++ looks awkward, change it to ++turn), I would use size_t instead of int for turn and turnLimit: http://en.cppreference.com/w/cpp/types/size_t
// NOTE: in fact, the cases where int is the right type to choose are significantly rarer than most beginning developers seem to assume -- as a good rule of thumb, whenever you think you should use an int, you should always stop and think whether it's actually a good idea -- e.g., are you sure you really want negative values?
I also wouldn't use mech1Alive or mech2Alive but rather their opposites (e.g., mech1Dead or mech2Dead) -- note that you're always negating them in your if statements, so it would improve code clarity to do so straight away, at the outset.
I've just looked at the online version, BTW. Your next step should be to remove all uses of new (and, consequently, all manual invocations of delete) and all of the resource-owning raw pointers: preferably change them to (stack-allocated) automatic variables (using references, preferably to const, when avoiding copies is necessary), then only for the ones for which you can't possibly do that (if there are any, don't just assume this, but carefully verify that free-store/heap dynamic allocation is in fact absolutely required!) use std::unique_ptr, and finally, as a last resort, use std::shared_ptr (and, as a last resort after this last resort ;], in the extremely rare case you have need to break a cycle, consider std::weak_ptr).
See: https://codereview.stackexchange.com/a/25721/24670


Multiple dice roll game with multiple players I feel as if I did not write it efficiently. Any suggestions on slimming? I had trouble when player 2 would guess the number the first try, it would say player 2 won but keep continuing to the re-roll loop, hence the sys.exit().

import random
import time
import sys

diceNumber = random.randint(1,6)

print("lets play dice of destiny")

play1 = input("Player 1 name?")
play2 = input("Player 2 name?")
prize = input("What does the loser have to do?")

print("Hi " + play1 + " & " + play2 + ", let" + "'" + "s roll the dice
of destiny.")

play1Num = input(play1 + " choose a number 1-6.")
play2Num = input(play2 + " choose a number 1-6.")

play1Num = int(play1Num)
play2Num = int(play2Num)

if play1Num == diceNumber:
    print("The Dice rolled...")
    print(diceNumber)
    print(play1.upper() + " WINS!")
    print(play2 + " must: " + prize)
    sys.exit()

if play2Num == diceNumber:
    print("The Dice rolled...")
    print(diceNumber)
    print(play2.upper() + " WINS!")
    print(play1 + " must: " + prize)
    sys.exit()

while diceNumber != play1Num and play2Num:
    print("The Dice rolled...")
    print(diceNumber)
    print("both wrong, rolling again...")
    diceNumber = random.randint(1,6)
    time.sleep(1)

if play1Num == diceNumber:
    print("The Dice rolled...")
    print(diceNumber)
    print(play1.upper() + " WINS!")
    print(play2 + " must: " + prize)
    break

if play2Num == diceNumber:
    print("The Dice rolled...")
    print(diceNumber)
    print(play2.upper() + " WINS!")
    print(play1 + " must: " + prize)
    break

I can't yet comment, so a couple little things.
1. Your print statements are way too long. In your if statement those 4 prints can be one. And why the breaks in your intro statement (let + ' + s)?
2. Your logic gets a little messy in the case of a tie, though I suppose it will print both statements.
3. You may already know this, but you cannot concatenate strings with objects or integers in Python, and due to dynamic typing, it can be hard to keep track of thing.

This whole program could be produced in 2 functions. A function called win, which takes a variable winningPlayer, and a function called Roll for the dice. That would take care of your infinite loop as well.
NOTE: I'm bad with dynamic types. There are probably unnecessary casts to strings here, but I'm just showing the concept.

import random
import time
import sys
#Rolls the dice when players are wrong
def Roll(rollNum):
    print("The Dice rolled..." + str(rollNum))
    print("both wrong, rolling again...")
    reRoll = random.randint(1,6)
    time.sleep(1)
    return reRoll
#Generic function for what the winner and loser do; not player specific
def Win(winner, loser, prz):
    print("The Dice rolled..." + str(diceNumber) + "\n" + str(winner).upper() + "Wins\n" + str(loser) + " must " + str(prz))
#Copied from your code
print("lets play dice of destiny")
play1 = input("Player 1 name?")
play2 = input("Player 2 name?")
prize = input("What does the loser have to do?")
#slightly condensed your code
print("Hi " + play1 + " & " + play2 + ", let's roll the dice of destiny.")
#moved this to a more appropriate place
diceNumber = random.randint(1,6)
play1Num = int(input(play1 + " choose a number 1-6."))
play2Num = int(input(play2 + " choose a number 1-6."))
#changed while loop to roll the dice every time both are wrong
while play1Num != diceNumber and play2Num != diceNumber:
    diceNumber = Roll(diceNumber)
    play1Num = int(input(play1 + " choose a number 1-6."))
    play2Num = int(input(play2 + " choose a number 1-6."))
#Applies the win method to both players; an array would be a better approach
if play1Num == diceNumber:
    Win(play1, play2, prize)
elif play2Num == diceNumber:
    Win(play2, play1, prize)
#Array implementation of win (with more than 2 players, would be best done using *args in win)
players = [play1Num, play2Num]
for item in players
    if diceNumber == item:
        players.remove(item)
        Win(item, players[1], prize)


I'm practicing turning for loops into recursive functions. what do you think? I am trying to turn this function:

collection = ['hey', 5, 'd']
for x in collection:
    print(x)

Into this one:

def printElement(inputlist):
        newlist=inputlist
        if len(newlist)==0:
                Element=newlist[:]
                return Element
        else:
                removedElement=newlist[len(inputlist)-1]
                newlist=newlist[:len(inputlist)-1]
                Element=printElement(newlist)
                print(removedElement)

collection = ['hey', 5, 'd']

printElement(collection)

It works, but I wonder if it's okay there's no "return" line under "else:"
Is this as "clean" as I can make it?
Is it better code with or without the newlist?

but I wonder if it's okay there's no "return" line under "else:"

Yes, that's OK. You don't need to return anything from your function if you don't want to. In fact, in the interest of consistency, you may as well remove the thing returned in the if block too:

def printElement(inputlist):
        newlist=inputlist
        if len(newlist)==0:
                return
        else:
                removedElement=newlist[len(inputlist)-1]
                newlist=newlist[:len(inputlist)-1]
                Element=printElement(newlist)
                print(removedElement)

collection = ['hey', 5, 'd']

printElement(collection)

  Is it better code with or without the newlist?

Assigning new things to inputlist won't modify it outside of the function, so there's no harm in doing so. May as well get rid of newlist.

def printElement(inputlist):
        if len(inputlist)==0:
                return
        else:
                removedElement=inputlist[len(inputlist)-1]
                inputlist=inputlist[:len(inputlist)-1]
                Element=printElement(inputlist)
                print(removedElement)

collection = ['hey', 5, 'd']

printElement(collection)

you don't use Element after assigning it, so you may as well not assign it at all.

def printElement(inputlist):
        if len(inputlist)==0:
                return
        else:
                removedElement=inputlist[len(inputlist)-1]
                inputlist=inputlist[:len(inputlist)-1]
                printElement(inputlist)
                print(removedElement)

collection = ['hey', 5, 'd']

printElement(collection)

You don't really need to modify inputlist, since you only use it once after modifying it. Just stick that expression straight into the printElement call. And now that inputlist is never modified, you can get rid of removedElement too, and just inline its expression in the print function.

def printElement(inputlist):
        if len(inputlist)==0:
                return
        else:
                printElement(inputlist[:len(inputlist)-1])
                print(inputlist[len(inputlist)-1])

collection = ['hey', 5, 'd']

printElement(collection)

Fun fact: for any list x, x[len(x)-1] can be shortened to x[-1]. Same with x[:len(x)-1] to x[:-1].

def printElement(inputlist):
        if len(inputlist)==0:
                return
        else:
                printElement(inputlist[:-1])
                print(inputlist[-1])

collection = ['hey', 5, 'd']

printElement(collection)

Since the first block unconditionally returns, you could remove the else and just put that code at the function level, without changing the code's behavior. Some people find this less easy to read. Personally, I like my code to have the least amount of indentation possible.

def printElement(inputlist):
        if len(inputlist)==0:
                return
        printElement(inputlist[:-1])
        print(inputlist[-1])

collection = ['hey', 5, 'd']

printElement(collection)

That's about as compact as you can get, with a recursive solution. You should probably just stick with the iterative version, for a few reasons:

Fewer lines
More easily understood
Doesn't raise a "maximum recursion depth exceeded" exception on lists with 200+ elements


Simple customer SQL query using a class and function I have been writing PHP for a number of years but in a procedural style. I have always shied away from object oriented PHP because I just didn't understand it. However now I'm back for another go.

I have a large web application which is written in procedural style PHP and as a learning exercise I would like to re-write it entirely in object-oriented PHP.

So before I start coding away thinking I'm doing the right thing, I would like to help, tips, and advice on what I have written so far. I don't think I need to explain what I am doing here because it is pretty simple stuff and self-explanatory.

pdo.inc.php

<?php

$host = "localhost";
$db = "dev";
$user = "dev";
$pass = "dev";
$dsn = "mysql:host=$host;dbname=$db;";

try
{
    $pdo = new PDO($dsn, $user, $pass);
} catch (PDOException $ex) {
    echo 'Connection failed: ' . $ex->getMessage();
    die();
}

?>

customer.class.php

<?php

include_once('pdo.inc.php');

class Customers
{
    function GetCustomerName($pdo, $customer_id)
    {
        $query = "SELECT first_name FROM Customers WHERE customer_id = :customer_id LIMIT 1";
        $stmt = $pdo->prepare($query);
        $stmt->bindParam(':customer_id', $customer_id);
        $stmt->execute();
        return $stmt->fetchColumn();
    }
}

?>

test.php

<?php

include_once('customers.class.php');

$a = new Customers();

echo $a->GetCustomerName($pdo, 1);

?>

Having looked at a number of PHP OOP tutorials it seems people use try/catch differently. I have seen some people use the try/catch within the class's method, and some people call the method from within the try/catch. What's the recommended usage of try/catch?

One thing you are doing well:

Dependency injection - I like how you pass the PDO object to the method that needs to use it. Since that is not an abstract function however, I wonder if you should be passing the PDO object to the class upon instantiation in the constructor (that is of course if the class needs a valid PDO object for other methods as well).

Some things I would suggest

TheGetCustomerName() method only considers the happy path with regards to the query.  What happens if prepare() fails?  What happens if execute() fails?  What happens if query operates correctly but no query match is found (i.e. id doesn't exist in table)? You should be handling these edge cases and either logging errors and/or handling exceptions if using PDO in exception mode (which it looks like you are).  You might consider throwing or rethrowing an Exception from this method itself if something fails.  You should consider returning null or false from this method if something fails and/or the SQL lookup is not successful.
You should consider enforcing types in you method signature.

Perhaps something like:

function GetCustomerName(PDO $pdo, $customer_id) {

or in PHP7 (which supports type hinting for scalars) and assuming $customer_id need to be an integer value:

function GetCustomerName(PDO $pdo, int $customer_id) {

This will allow you to enforce that a valid PDO instance is passed to the method, otherwise an InvalidArgumentException will be thrown.  This prevents you from having to do something like if ($pdo instanceof PDO) in your method.

Related to the above item. Right now your method assumes all passed parameters are valid.  You should always assume you are getting passed bad data and make sure you have appropriate parameters passed to the method before doing any other work in the method.  I would enforce PDO in signature as noted above, and if not using PHP 7 at least add a line like the following.

Example:

if (empty($customer_id) || !is_int($customer_id) || $customer_id < 1) {
    throw new InvalidArgumentException(
        'Send me a valid positive integer for $customer_id' .
        ' Value sent: ' . var_export($customer_id, true)
    );
}

You do not need an include for PDO code in your customer class.  You are taking on the PDO object as a dependency (which is a good example of dependency injection).  The customer class code therefore doesn't need to reference the code where the PDO object is created. It just needs to know it has a valid PDO object passed to it, not reference the code where such objects are created.  You should reference your include_once('pdo.inc.php'); line in test.php where the dependent PDO object is actually instantiated.
You should consider following typical PHP syntax schemes.  Typically in PHP you would NEVER see a function or method name that starts with a capital letter. This syntax is typically reserved for classes and interfaces.  I would consider renaming to getCustomerName().
I would consider separating your configuration from the place where you instantiate the PDO objects.  If you ever have the need to instantiate multiple database connections, you would want to separate these.  Oftentimes it is best to use constants to define these sorts of configuration values such that you don't tend to overwrite them and can see directly in code that these should be constant values.
You should consider changing the class name to Customer instead of Customers if a single class instance is expected to only deal with a single customer vs. a collection of customers. This may seem nitpicky, but it is typically best practice for developers to use meaningful names for items in code.  If Customer more correctly describes what the object represents than Customers, the singular version should be used.

You also ask about try-catch usage.  Generally you should use a try-catch block around any lines of code that you think might throw an exception and you want to be able to handle that exception in the code such that code execution will not terminate due to the uncaught exception.  For example in you method.  You should, at a minimum, have a try-catch block around the prepare(), bindParam()and execute() calls if you are using PDO in exception mode (which I would recommend). You could have one block around all this or multiple blocks depending on how granularity you want to handle the failure.

In test.php for example you would want to place a try-catch around the call to $a->GetCustomerName($pdo, 1); As this call could throw an exception (particularly if you added some of the parameter handling logic I mentioned earlier).

So for me, a rewrite of your code might look like:

pdoConfig.inc.php

<?php
define('DB_CONFIG_HOST', 'localhost');
define('DB_CONFIG_DB', 'dev');
define('DB_CONFIG_USER', 'dev');
define('DB_CONFIG_PW', 'dev');
$dsn = 'mysql:host=' . DB_CONFIG_HOST . ';dbname=' . DB_CONFIG_DB . ';';
define('DB_CONFIG_DSN', $dsn);l
?>

customer.class.php

<?php
class Customer
{
    function getCustomerName(PDO $pdo, $customer_id)
    {
        // validate parameters
        // in PHP 7 you would just add int type hinting in method signature
        // and could change the conditional below to only look
        // positive integer values assuming the id is > 0
        if (empty($customer_id) || !is_int($customer_id) || $customer_id < 1) {
            throw new InvalidArgumentException(
                'Send me a valid positive integer for $customer_id' .
                ' Value sent: ' . var_export($customer_id, true)
            );
        }

        // query DB for customer name
        $query = "SELECT first_name
                  FROM Customers
                  WHERE customer_id = :customer_id
                  LIMIT 1";
        try {
            $stmt = $pdo->prepare($query);
            $stmt->bindParam(':customer_id', $customer_id);
            $stmt->execute();
            if($stmt->rowCount === 0) {
                // no match found
                return false;
            } else {
                return $stmt->fetchColumn();
            }
        } catch (PDOException $e) {
            error_log(
                'Something went wrong with query. Message: ' .
                $e->getMessage();
            );
            // you could also rethrow exception here instead of returning null
            // if you want to abort method completion and have caller handle exception.
            // or of course not use try-catch block here at all
            // if you don't want any extra exception handling in this method.
            return null;
        }
    }
}

?>

test.php

<?php

include_once('pdoConfig.inc.php');
include_once('customers.class.php');

// instantiate PDO object
try
{
    $pdo = new PDO(DB_CONFIG_DSN, DB_CONFIG_USER, DB_CONFIG_PW);
} catch (PDOException $e) {
    error_log('Connection failed with: ' . $e->getMessage());
    die();
}

// instantiate customer object
// you might want to include in try-catch if instantiation is expected
// to potentially throw exceptions
$a = new Customer();

try {
    echo $a->getCustomerName($pdo, 1);
} catch (Exception $e) {
    error_log('Customer name lookup failed with: ' . $e->getMessage());
}
?>


Code that rotates images in a set of divs I am rotating images inside some divs.

An image is associated with a link.Eg: image0 is associated with link0, image1 with link1 and so on.

image0 and link0 should display in pos-1

image1/link1 and image2/link2 should rotate on page refresh in pos-2

image3/link3 and image4/link4 should rotate  on page refresh in pos-3

Although currently I want to display only one image in pos-1, at a later stage I may rotate two images as well in pos-1. Similarly I may display only one image in pos-2.

$(function(){
var image = new Array ();
  image[0] = "";
  image[1] = "";
  image[2] = "";
  image[3] = "";
  image[4] = "";

var link = new Array ();
    link[0] = "";
    link[1] = "";
    link[2] = "";
    link[3] = "";
    link[4] = "";

var min = 0;
var max = 1;
var x = Math.floor(Math.random() * (max - min) + min);
$('#pos-1').append(link[x]+image[x]);
min=1;
max=3;
x = Math.floor(Math.random() * (max - min) + min);
$('#pos-2').append(link[x]+image[x]);
min=3;
max=5;
x = Math.floor(Math.random() * (max - min) + min);
$('#pos-3').append(link[x]+image[x]);
  });

Please let me know if this code can be improved to perform better?
Demo: https://jsfiddle.net/y0hga2of/5/

The first thing I see in your code are the strings who represents an HTML element (in this case an img and an anchor element).

This is a bad practice. Instead of appending a whole element to your HTML just update the src or href attribute.

The second thing I noticed is your implementation of a new array. Instead of using:

var image = new Array ();
 image[0] = "";

Use this approach:

var image = [
       "https://placehold.it/200/09f/fff.png?text=image1",
       "https://placehold.it/200/09f/fff.png?text=image2"
];

Also the naming of your variables is badly. The name:

 var image = new Array ();

and

var link = new Array ();

Suggests that they are holding exactly one image and one link. Actually they are  holding multiple links and images. Try to use names that describes what you can expect inside the variable. So use images and links instead.

Goodluck!

(Sorry for bad english, not a native speaker)


Return value when the requested variable returns null I have a method that returns a list from a HTML page:

private List<HtmlNodes> GetRelevantNodes(HtmlDocument aHtmlDoc, string aID)
{
    List<HtmlNode> nodes = new List<HtmlNode>();

    HtmlNode currentDiv = aHtmlDoc.GetElementbyId(aID);
    if (currentDiv == null)
            return null;
    else
            //Do stuff - fill the nodes list...

    return nodes;
}

I'm not sure what to do with the return value in case the currentDiv variable is null. Obviously I can't continue the method regularly and I need to return from this method with some indication that there's no such div.

If I return null and handle it up the stack it'll work fine but it's less readable code (at list in my opinion... don't you think?). Another thing I can add is an out string with the error message and maybe use it later for the end user output but that still doesn't solve my readable problem.

I'd be happy to hear your ideas and advice.

You're right, this is not the best thing to do. The correct way would be to return an empty list.

private List<HtmlNodes> GetNodes(HtmlDocument aHtmlDoc, string aID)
{
    List<HtmlNode> nodes = new List<HtmlNode>();

    HtmlNode currentDiv = aHtmlDoc.GetElementbyId(aID);
    if (currentDiv != null)
    {
            //Do stuff - fill the nodes list...
    }

    return nodes;
}

Now why is that better than returning null? Simple, it allows to remove checking from the client code. Imagine your function would return null if there are no nodes, clients always need to write the following code when interacting with it:

List<HtmlNode> nodes = GetNodes(htmlDoc, id);
if (nodes != null)
{
    foreach (HtmlNode node in nodes)
    {
        // Super secret business code
    }
}

Always. Now if you'd return an empty list, the checking becomes optional on the client side:

foreach (HtmlNode node in GetNodes(htmlDoc, id))
{
    // Still secret...
}

Or with checking:

List<HtmlNode> nodes = GetNodes(htmlDoc, id);
if (nodes.Length > 0)
{
    foreach (HtmlNode node in nodes)
    {
        // Super secret business code
    }
}

You should always write XML Documentation.

private List<HtmlNodes> GetNodes(HtmlDocument aHtmlDoc, string aID)

There's a typo, it should be HtmlNode.

Also your variables are named not optimal. Drop that a prefix, nobody cares if it is an argument or not, it's a variable you have to work with.

The name of the function is also not good, it's called GetNodes yet from what I see (and interpret into it) it does not fetch the nodes with the given ID, it fetches the children of the first node with the given ID. So it should actually be called GetChildren.


Check whether a permutation with one fixed point can be rotated into one with two fixed points A friend gave me the following riddle:

  Given n people with n distinct names, you place n names tags on a round table with n seats. If the n people now sit at those seats randomly such that exactly one person sits correctly (in the seat with the correct name tag with their name one it), can we rotate the table such that (at least) two people sit correctly?

For odd n this is false, which you can see by considering by placing the name tags 1, ..., n in order and seating the people as 1, n, n - 1, ... 2. I haven't been able that this true for even n so I wanted to try out small even n with a python script.

I wrote the following code. Unfortunately, it is very slow (n = 10 takes about 25 seconds, but n = 12 takes nearly 11 minutes!) so I am searching for libraries which can speed up the things I implemented from scratch or maybe a method which speeds things up.

I have 4 methods:

perms(n), which gives me all permutations of 0, ..., n- 1 which have one fixed point,
fixtest(L,m), which tests if the permutation L has m fixed points,
rot(L), which rotates the permutation L, i.e. [1,2,3] becomes [2,3,1] and
test(n) which, for a given n generates all permutations of 0, ..., n- 1 which have one fixed point with perms(n) and then for each one, performs all rotations and for every rotation checks the number of fixed points and writes them in a list C. If C only contains ones, we have found a counterexample to the riddle above.

My code looks like this

from itertools import permutations
from collections import deque

# find all permutations of [n] with 1 fixed point
def perms(n):
    P = (perm for perm in permutations(range(n)))
    D = []                        # D will contain the 'right' permutations (those with one fixed point)
    for perm in P:                # count fixed points (counter c)
        c = 0
        for idx, k in enumerate(perm):
            if idx == k:
                c +=1
        if c == 1:
            D.append(perm)
    return D

 # tests if a sequence L has m fixed points
def fixtest(L,m):
    L = list(L)
    c = 0
    for idx, k in enumerate(L):
        if idx == k:
            c +=1
    if c == m:
        return True
    else:
        return False

# rotates the list L
def rot(L):
    L = deque(L)
    a = L.pop()
    L.appendleft(a)
    return list(L)

def test(n):
    for f in perms(n):
        k = 0
        C = []
        f_real = list(f)
        while k < n - 1:
            f = rot(f)
            C.append(fixtest(f,1))
            k +=1
        if all(x == True for x in C):
            return n, 'Counterexample found:', f_real
    return n, 'no counterexamples found'

Doc-string

You can add a docstring to the method using the convention of PEP-257 so IDE's etc can parse it and show it when looking at the method.

typing

By adding type annotations you can make it more clear to the user of the function what input the methods require and what to expect as answers, serving as extra documentation. It also allows static analysers like mypy to discover bugs.

generator

Instead of returning a list, you can have the permutation generate yield the correct permutations one by one

reduce the number of permutations

If any permutation where 1 person sits on the correct chair is ok, why not pick one where person 0 sits on the correct chair. This reduces the number of permutations to check n times

import itertools
import typing
def perms_gen(n: int) -> typing.Iterator[typing.Sequence[int]]:
    """Yields the permutations where only the first person sits on his own seat."""
    for perm in itertools.permutations(range(1, n)):
        if any(i == j  for i,j in enumerate(perm, 1)):
            continue
        yield (0,) + perm

This is a generator function which yields a new permutation each time another function (next, for-loop, ..) asks for it,instead of all generating them in one go. This way, only those permutations that are needed are generated, and they occupy less memory. I you want a list of all permutations, you have to explicitly ask for it by list(perms_gen(10))

rotate

Rotating a list can be done by splice indexing. No need to call a deque. If you want to use a deque, then use its rotate method, instead of popping and appending yourself

Saving the 3 letters to type is also not useful.

def rotate(
    seq: typing.Sequence[typing.Any], n: int
) -> typing.Sequence[typing.Any]:
    """Rotates `seq` `n` places."""
    n %= len(seq)
    return seq[n:] + seq[:n]

The in-place modulo is to make sure n is in the range [0, len(seq))

Testing further rotations

To see whether there is a rotation which allows more than 1 person in his or her seat, you can also count for each person how much spaces they need to go to their correct seat. If more than 1 person needs the same number of rotations, you have a match. To check whether there is no rotation of a permutation so 2 people sit in their correct seat, you can use this set comprehension {(i - j) % n for i, j in enumerate(permutation)} and test whether its length is n

return values

When there is no response, I would either raise an exception or a sentinel value. You do approximately the same, by your return n, 'Counterexample found:', f_real in case of success and in case of failure return n, 'no counterexamples found'. But using a string as sentinel value means the users of this function needs a complicated test, further complicated by the face that the lengths of the return tuples are different, so you can not use tuple unpacking.

Returning n has no use, since the user called the method with n as an argument. If he wants to produce a message with n included, he doesn't need to get it returned here.

As sentinel for failure I would return None or raise an exception.

putting it together:

def main(n: int) -> typing.Optional[typing.Tuple[int, ...]]:
    """Tests whether there is a permutation so not more than 1 person
    return to their original seat when rotated.

    returns a counter example when found, or None.
    """
    for permutation in perms_gen(n):
        if len({(i - j) % n for i, j in enumerate(permutation)}) == n:
            return permutation

This either returns a tuple with an example, or None is implicitly returned when all permutations are tested.

timings

On my machine, for n == 10 this takes about 700ms compared to 30s for your original

Keeping it as a generator

If you rewrite it a bit, you can keep the rotations_to_place as a generator, only calculating as needed, and yielding the correct permutations

def main_generator(n: int) -> typing.Iterator[typing.Tuple[int, ...]]:
    """Yields all complying permutations."""
    permutations = perms_gen(n)
    for permutation in perms_gen(n):
        if len({(i - j) % n for i, j in enumerate(permutation)}) == n:
            yield permutation

If you want to test if there is a permutation, you can do any(main(n)). If you want all permutations, you can do list(main(n))

For 9:

results = list(main_generator(9))

[(0, 2, 1, 6, 8, 7, 3, 5, 4),
 (0, 2, 1, 6, 8, 7, 4, 3, 5),
 (0, 2, 1, 7, 6, 8, 3, 5, 4),
 (0, 2, 1, 7, 6, 8, 4, 3, 5),
 ...
 (0, 8, 6, 5, 3, 2, 7, 1, 4),
 (0, 8, 7, 4, 6, 2, 5, 1, 3),
 (0, 8, 7, 5, 3, 6, 1, 4, 2),
 (0, 8, 7, 6, 5, 4, 3, 2, 1)]


A very basic web framework inspired by Flask I was curious how hard it would be to implement a routing system like flask uses. As it turn's out it's actually not that hard if you understand how decorator's work.

The flask implementation has a lot of features like routes based on a regular expression. Mine just maps a path to a function.

#!/usr/bin/env python

from http.server import BaseHTTPRequestHandler, HTTPServer
import sys

DEFAULT_PORT = 8080
LOCALHOST = '127.0.0.1'
DEFAULT_ENCODING = 'utf-16'
ROUTES = None

class Routes(object):
    def __init__(self):
        self.routes = self.__dict__
        global ROUTES
        ROUTES = self

    def add(self, route, cb):
        self.routes[route] = cb()

    def get(self, route):
        return self.routes.get(route)

    def exists(self, route):
        return route in self.routes

    def __call__(self, route):
        return self.get(route)

class Route(object):
    def __init__(self, route):
        self.route = route

    def __call__(self, func):
        ROUTES.add(self.route, func)

class RequestHandler(BaseHTTPRequestHandler):
  Routes()
  def send_msg(self, msg, encoding=DEFAULT_ENCODING):
      self.wfile.write(bytes(msg, encoding))

  def do_GET(self):
      if not ROUTES.exists(self.path):
          self.send_header('content-type', 'text/html')
          self.send_response(404)
          self.end_headers()
          self.send_msg('
404
')
          return

      self.send_response(200)
      self.send_header('Content-type','text/html')
      self.end_headers()
      self.send_msg(ROUTES(self.path))
      return

def run(port=8080, server_addr=LOCALHOST):
  print('starting server...')
  server_address = (server_addr, port)
  httpd = HTTPServer(server_address, RequestHandler)
  print('running server...')
  httpd.serve_forever()

if __name__ == '__main__':
    @Route('/')
    def root():
        return '
It Works
'
    run()

That's a nice mini-flask exercise.

ROUTES = None

Deferring the assignment to later, and initializing to Routes(), would seem more natural, to avoid having to test for is None.

    global ROUTES
    ROUTES = self
...
if __name__ == '__main__':
    @Route('/')

Oh, wait, you're not testing against None. You're just jamming it in there. Consider making the assignment the caller's responsibility.

    self.routes = self.__dict__

It's not obvious to me why you were reluctant to create a new dict(). Doesn't the current implementation preclude callers from creating routes to e.g. '/add' and '/get'?.

def add(self, route, cb):

I'm sure that's a very nice signature, and perhaps cb is the appropriate name to use. But it's obscure, and demands at least a docstring explaining it.

class Routes(object):
...
class Route(object):

Honestly, I'm not in love with the class names, there. It's not immediately obvious to the reader how a Routes is different from a Route, so distinguishing names with plural / singular probably isn't a win. Consider RouteCollection instead. But, better, consider deleting the Route class entirely, and using a method hanging off ROUTES to do the work instead. If you do retain two classes, you really really need to add a docstring to each one explaining what its Single Responsibility is.

class RequestHandler(BaseHTTPRequestHandler):
  Routes()

I suggest turning the ctor call into an assignment: ROUTES = Routes(). Better, make the assignment the responsibility of the "if __name__ is main" clause.

  self.send_msg(ROUTES(self.path))

Clearly this works. But it is less obvious than it might be. Consider ignoring __call__, in favor of boring syntax like ROUTES.get_document(self.path).

      self.send_msg('
404
')

This is correct.

def root():
    return '
It Works
'

I like the root identifier. But the HTML is incorrect, as  belongs outside of
. Consider using CSS style= to replace that center.

run()

Please run flake8 and follow its advice to precede this line with two blank lines, setting it off from the decorated root function.

Overall, it's an interesting piece of code, I'm glad you shared it.


Minimax Alpha-beta code for Java Not so long ago I learned how to implement the minimax algorithm with alpha beta pruning, and even created a perfect Tic Tac Toe player. How can I improve this?

public Best chooseMove(boolean side,int[]board,int alpha,int beta,int depth,int maxDepth){
    Best myBest = new Best();
    Best reply;
    int num;

    if(Board.checkGameOver(board)||depth==maxDepth){
        Best fakeBest = new Best();
        fakeBest.setScore(returnPositionScore(board));
        return fakeBest;
    }

    if(side){
        myBest.setScore(alpha);
        num = numberOfEngine;
    }
    else{
        myBest.setScore(beta);
        num = numberOfOpponent;
    }
    ArrayList<Integer> availableMoves = new ArrayList<Integer>(searchAvailableMoves(board));

    for(Integer move:availableMoves){
        board[move.intValue()] = num;
        reply = chooseMove(!side,board,alpha,beta,depth+1,maxDepth);
        board[move.intValue()] = 0;
        if(side&&reply.getScore()>myBest.getScore()){
            myBest.setMove(move.intValue());
            myBest.setScore(reply.getScore());
            alpha = reply.getScore();

        }
        else if(!side&&reply.getScore()<myBest.getScore()){
            myBest.setMove(move.intValue());
            myBest.setScore(reply.getScore());
            beta = reply.getScore();

        }
        if(alpha>=beta){
            return myBest;
        }
    }

    return myBest;
}

As a starting point, address the issue you have: instead of returning an int (which is not sufficient for your needs), return an object that contains everything you WANT to get as a returnvalue.

In your case, that is:

an int (the return value of the alfabeta code)
a bestmove-object

So, define a return value class like this:

class MySpecialReturnvalueClass {
    int alfabetavalue;
    Best bestmove;
}

Of course you make the fields private, with getter-methods and 1 constructor with the 2 values, or setters, but that's a lower-level detail. The point is, if you NEED something to be returned, then just go and DO return it.

From that starting point, you change your code, then step back, look at the resulting code - and then you may see in which direction you could re-arrange. Maybe it turns out that class Best can contain the alfabeta value, thus the need for the new class vanishes, and you can return Best instead.


Referring to nested arrays and array_merge (php) In such an array:

$arr = Array (
    [key0] => Array (
        [subkey0] => Array (
            [0] => "value0"
            [1] => "value1"
        )
        [subkey1] => Array (
            [0] => "value0"
            [1] => "value1"
        )
    )
    [key1] => Array (
        [subkey0] => Array (
            [0] => "value0"
            [1] => "value1"
        )
        [subkey1] => Array (
            [0] => "value0"
            [1] => "value1"
        )
    )
)

In order to add $varX to the end of $arr[keyX][subkeyX] as [X] => $varX, I use, inside a loop, the following:

$arr[$key] = array(
    "subkey0" => array_merge((array)$arr[$key]["subkey0"], (array)$var1),
    "subkey1" => array_merge((array)$arr[$key]["subkey1"], (array)$var2)
    ...
)

And I might be working with even more nested arrays, so I was wondering if there was a way for subkey0 to refer to itself, or if I could do something like:

"subkeyX" =>+ (array)$varX

Or use array_merge() in a different way to add $varX to the end of subkeyX as [X] => $varX.

I don't know if I understand the question right. But this should work.

foreach ($keys as $key) {
    $arr2[$key]['subkey0'][] = $var1;
    $arr2[$key]['subkey1'][] = $var2;
}


Infix to Postfix Converter Program This is my homework.  Kindly help me check it?
The instructions are:

Implement an infix expression to postfix expression converter. You are to implement the infix to postfix algorithm presented in the lecture. You are to use only the stack that was provided in the lab lecture. The use of the stack of JDK or any other stack is not allowed. The supported operators are +, -, *, / and ^. Grouping operator are ( and ).
Input: Infix expresseion where each token (operand and operator) are space-separated (console-based). Explore the StringTokenizer class for tokenizing (separating the infix expression into tokesn) the input. The book of Deitel and Deitel has examples on how to use StringTokenizer.
Output: Postfix expression (console-based). The tokens (operand and operator) are also space-separated in the output. "

The code I wrote is:
import java.util.*;

public class PostfixConverter {
private String infixExpression;

public PostfixConverter(String infixExpression) {
this.infixExpression = infixExpression;
}

private String convertInfixExpression() {
StringTokenizer tokens = new StringTokenizer(infixExpression);
Stack operatorStack = new ArrayStack();
String converted = "";

while(tokens.hasMoreTokens()) {
    String token = tokens.nextToken();
    if(token.equals("(")) {
        operatorStack.push(new String(token));
    }
    else if(token.equals(")")) {
        while(operatorStack.top().equals("(") != true) {
                converted = converted + " " + operatorStack.pop();
            }
        if(operatorStack.top().equals("(")) {
            operatorStack.pop();
        }
    }
    else if (isOperator(token)){
        if(operatorStack.isEmpty() == true) {
            operatorStack.push(new String(token));
        }
        else {
            if(ICP(token) < ISP((String) operatorStack.top())) {
                converted = converted + " " + operatorStack.pop();
                operatorStack.push(token);
            }
            else {
                operatorStack.push(new String(token));
            }
        }
    }
    else {
        converted = converted + " " + new String(token);
    }
}
while(operatorStack.isEmpty() != true) {
    converted = converted + " " + operatorStack.pop();
    }

return converted;
}

public int ISP(String token) {
int precedence = 0;
if(token.equals("+")|| token.equals("-")) {
    precedence = 2;
}
else if(token.equals("*") || token.equals("/")) {
    precedence = 4;
}
else if(token.equals("^")) {
    precedence = 5;
}
else if(token.equals("(")) {
    precedence = 0;
}
return precedence;
}

public int ICP(String token) {
int precedence = 0;
if(token.equals("+")|| token.equals("-")) {
    precedence = 1;
}
else if(token.equals("*") || token.equals("/")) {
    precedence = 3;
}
else if(token.equals("^")) {
    precedence = 6;
}
return precedence;
}

private boolean isOperator(String token) {
return (token.equals("+") ||
        token.equals("-") ||
        token.equals("*") ||
        token.equals("/") ||
        token.equals("^") );
}

public static void main(String[] args) {
System.out.println("Input the infix expression "
        + "(operands, operators are separated by spaces):");
Scanner input = new Scanner(System.in);
String infixExpression = input.nextLine();
PostfixConverter converter = new PostfixConverter(infixExpression);
System.out.println("The converted expression is " +
        converter.convertInfixExpression());
}

}

I like this opportunity for a code review because I hopefully can share some good programming techniques that I didn't have a chance to learn until I actually got out of school and into the industry. I can edit to explain anything further.

Your variable infixExpression can be defined as final:

private final String infixExpression;

This isn't necessary but is good practice as it's a good idea to maximize the immutability of your classes. Anything variable which is only modified when declared, or in a constructor, may be final.
The convertInfixExpression method should also be public. I know this is for an assignment, but that should not stop you from thinking about using it outside the main method. Alternatively, you could do away with the constructor altogether, and just make this method static. That way, you would use it by calling

PostfixConverter.convertInfixExpression("my infix expression");

This is possible by changing your current convertInfixExpression method signature to

public static String convertInfixExpression(String infixExpression)

I don't know what library ArrayStack came from, but I can assume how it works. That being said, you don't parametize it which is a big no no, and causes yourself some extra work later. This should be

Stack<String> operatorStack = new ArrayStack<String>();

In Java, appending to a String is typically a bad idea. If you have an object you know you'll be appending too, use a StringBuilder:

StringBuilder converted = new StringBuilder();

Concerning the first while loop in the method, I don't like the looks of it from a glance. There's way too many nested while, if, and else statements. We'll see if we can simplify some of that. To start, let's look at the highest level of what's going on:

If token is a '(' push to stack
If token is a ')' do complicated stuff
If token is an Operator do complicated stuff
Otherwise append the token to the result

So the hint here is that you want to move all that complicated stuff to separate methods. Your code benefits from this by being much easier to read and reason about.
There's almost never a reason to use the new keyword next to String in Java.

operatorStack.push(token);

not

operatorStack.push(new String(token));

The first complicated block (fixed formatting):

while(operatorStack.top().equals("(") != true) {
    converted = converted + " " + operatorStack.pop();
}
if(operatorStack.top().equals("(")) {
    operatorStack.pop();
}

I guess this isn't too complicated, but I don't like seeing nested while statements... This is how I would write it:

while(!operatorStack.top().equals("(")) {
    converted.append(" ").append(operatorStack.pop());
}
operatorStack.pop();

I'll highlight the differences:

There's no reason to ever compare anything to true or false in an if statement.
Used append from the StringBuilder. Check out the Oracle tutorial on this class to find out why.
There's no reason for the if statement. We already know the a ( is on the top of the stack, otherwise the previous loop wouldn't have ended.

Regarding the second complicated bit, I would write it like so:

else if (isOperator(token)){
    if(operatorStack.isEmpty()) {
        operatorStack.push(token);
    }
    else {
        if(ICP(token) < ISP(operatorStack.top())) {
            converted.append(" ").append(operatorStack.pop());
            operatorStack.push(token);
        }
        else {
            operatorStack.push(token);
        }
    }
}

Pretty much the same comments as above, but notice you don't need to cast to a String because you've now parametized your stack. Too be honest, though I'm still not entirely sure of the purpose of the logic here, but mainly because I have no idea what ICP and ISP mean!
Use descriptive and conventional method names. If ICP and ISP return a value, they should start with get. I know they're return the precedence of operators, but the S and C is still unclear to me. That being said, you can simplify all of this by using some Map constants. For example,

private static final Map<String, Integer> S_PRECEDENCE;
static {
    Map<String, Integer> map = new HashMap<String, Integer>();
    map.put("+", 2);
    map.put("-", 2);
    map.put("*", 4);
    ....
    S_PRECEDENCE = Collections.unmodifiableMap(map);
}

And then you can change the ISP method to something like

public int ISP(String token) {
    return S_PRECEDENCE.contains(token) ? S_PRECEDENCE.get(token) : 0;
}

I'll note that S_PRECEDENCE is a bad name and should be expanded with whatever S means.
Along the same lines, you can make a Set of your operators, and simply check if that collection contains the token.


Counter increment for search volume I have written a jQuery function which accepts an elementId and an integer. The function then generates a counter which increments at the average speed of searches made (per month) since you initialised the function.

The elementId is where the counter is placed on the page.
The integer is the number of monthly searches manually taken from Google adWords.

There are a number of factors I'd like to improve; firstly, the error handling as I am trying to get a reasonably accurate counter Math.round shouldn't be used. But other functions to handle divide by zero issues etc are fine.

I'd also like to improve how the calculations are performed to establish the interval speed, it is currently written to show relatively clear calculation steps (I'd like to think).

(function ($) {

/* Initialise Counters*/
keywordSearchCounter(9292208, 'footballCounter');
keywordSearchCounter(292410, 'tennisCounter');
keywordSearchCounter(779180, 'chessCounter');

function keywordSearchCounter(searchesPerMonth, elementId) {

    if($.type(searchesPerMonth) !== "number") return;

    if(searchesPerMonth === 0) return;

    console.log("function initialised for '" + elementId.toString() + "'");

    /* Do some math on searches */
    var daysInMonth = 28,
        hoursInDay = 24,
        minutesInHour = 60,
        secondsInMinute = 60;

    var searchesPerDay = searchesPerMonth / daysInMonth;
    var searches = searchesPerDay;
    console.log('per day: ' + searches);

    var searchesPerHour = searchesPerDay / hoursInDay;
    searches = searchesPerHour;
    console.log('per hour: ' + searches);

    var searchesPerMinute = searchesPerHour / minutesInHour;
    searches = searchesPerMinute;
    console.log('per minute: ' + searches);

    var searchesPerSecond = searchesPerMinute / secondsInMinute;
    searches = searchesPerSecond;
    console.log('per second: ' + searches);

    var interval = (1 / searches) * 1000;
    var counter = 0;
    console.log('interval set to: ' + interval);

    /* Set the counter speed */
    setInterval(function () {
        counter += 1;
        document.getElementById(elementId).innerHTML = counter;
    }, interval);
}
}(jQuery));

A have provided a jsFiddle to show a working example: Demo

Any advice, even on the instantiation of my IIFE would be most welcome!

I'm going to suggest something completely different to your current implementation... I'll dump the code first (see js fiddle) notice that the fiddle has changed from running the script 'onload' to 'no wrap - in '

// Wrap it all in an IIFE if you want.
var SECONDS_IN_A_MONTH = 60*60*24*30; //seconds * minutes * hours * days

// Search for elements to be turned into counters
$('.search-counter').each(function() {
    var $this, searches, interval, counter;
    $this = $(this);
    searches = parseInt($this.data('searches'), 10);

    if (searches < 1  || isNaN(searches)) return;

    interval = (SECONDS_IN_A_MONTH/searches)
    counter = 0;

    // Start the counter at zero.
    $this.html(counter);

    setInterval(function () {
        $this.html(++counter);
    }, interval);
});

I've modified your html to include the number of searches per month in a data attribute. I've also added a class to the elements that will be turned into counters. This allows the javascript to deal with any number of counters on a page. E.g. a counter would look like:

 "Chess" has had...

searches

I don't think you gain anything by splitting up the calculation of the interval. If you think it about another way, you're trying to calculate the time between searches which is clearly just the number of seconds in a month divided by the number of times the search was executed.

If you can't change the html, or would rather keep your code then I have a few points.

It's nicer to start the counter at zero.
It's better not to rely on hoisting - put simply, only use a function after you've written it.
document.getElementById(elementId).innerHTML = counter; will have to search the DOM on every 'tick'. Cache the element
I used 30 for days in a month rather than 28 - use whichever period the data is from.

All pretty minor stuff really.


Creating charts for my UPS metrics - Part 2 Following up from my previous question

  Streaming my UPS metrics for graphical viewing in bash

#!/usr/bin/env bash

[ -v METRICS_CHECK ] && set -x

API_URL="https://api.thingspeak.com/update"
UPS_PROPS=("LINEV" "LOADPCT" "TIMELEFT")
API_PARAMS=("api_key==XXXXXXXXXXXXXX")

for index in "${!UPS_PROPS[@]}"
do
    value=$(/sbin/apcaccess -up "${UPS_PROPS[$index]}")
    field=$((index + 1))
    if [ "$field" -eq 2 ]
        then
        # bc required for floating point arithmetic
        value=$(echo "$value * 6" | bc)  # 600 Watt * load percent
    fi
    PARAM=$(printf "field%d==%s" "$field" "$value")
    API_PARAMS+=("${PARAM}")
done

/usr/local/bin/http "$API_URL" ${API_PARAMS[@]}

Updates:

Collects data for different metrics supported by apcaccess
Use a variable check for enabling debug mode
Split parameters sent to API URL into a different array
Move away from curl to use httpie
Custom computation for certain attributes

The shellcheck passes everything except the unquoted ${API_PARAMS[@]} in the last line. I am not sure that httpie would work if I quote that and pass it as a single argument.

Charts for viewing pleasure (last 600 minutes data from the time post was created):

Input line voltage

Load (in wattage) on the UPS

Battery time left for the UPS

Click the images to get live charts.

Quoting - you need double-quotes on the last line: it should be "${API_PARAMS[@]}" (remember that the [@] makes the shell expand it into multiple words, so it won't be seen by your http program as just one).

Instead of if [ "$field" -eq 2 ], it's clearer to compare the field name.  That will help if you change the properties list in future.  It may help to create a variable:

propname="${UPS_PROPS[$index]}"
value=$(/sbin/apcaccess -up "$propname")
if [ "$propname" = LOADPCT ]

We can supply standard input from a string without needing an echo process, by using <<< redirection:

    # bc required for floating point arithmetic
    value=$(<<<"$value * 6" bc)  # 600 Watt * load percent

We can make the array 1-based, and avoid the need to have $field different from $index:

UPS_PROPS=([1]="LINEV" "LOADPCT" "TIMELEFT")


Function composition using std::bind Given a number of functions f1, f2, f3, ..., fn I want to create a composite function g so that g(x) calls f1(f2(f3(...(fn(x)))).

I came across a blog post which was mentioned in a related Stack Overflow question.

These solutions seem to be very involved. In my own code, I am using a very simple recursion of std::bind instead.

#pragma once

#include <functional>

// TODO: C++14: remove traits and use auto return type deduction

// traits to infer the return type of recursive binds
template<typename... Fn>
struct composite_function_traits;

// bind a single function with a placeholder
template<typename F1>
struct composite_function_traits<F1> { typedef decltype(std::bind(std::declval<F1>(), std::placeholders::_1)) type; };

template<typename F1>
typename composite_function_traits<F1>::type make_composite_function(F1&& f1)
{
    return std::bind(std::forward<F1>(f1), std::placeholders::_1);
}

// recursively bind multiple functions
template<typename F1, typename... Fn>
struct composite_function_traits<F1, Fn...> { typedef decltype(std::bind(std::declval<F1>(), std::declval<typename composite_function_traits<Fn...>::type>())) type; };

template<typename F1, typename... Fn>
typename composite_function_traits<F1, Fn...>::type make_composite_function(F1&& f1, Fn&&... fn)
{
    return std::bind(std::forward<F1>(f1), make_composite_function(std::forward<Fn>(fn)...));
}

Are there advantages to the more complex methods for function composition?

A very interesting question. I can identify two main differences between your implementation (using variadic templates) and the linked article's implementation (using Boost.Fusion):

The definition of function composition. A call to your make_composite_function(f1, f2, f3)(x) is semantically equivalent to f1(f2(f3(x))). Calling the linked article's compose(f1, f2, f3) is semantically equivalent to f3(f2(f1(x))).

Which definition is more natural is completely up to the reader. Both are perfectly valid. Your make_composite_function has the nice property that it is very easy to implement with variadic templates (since the recursion uses "pop front" logic). This is actually mentioned as an alternative in the linked article.

Furthermore, they argue for their definition of function composition because it follows the reading direction. E.g., compose(receive, decode, store)(data) is semantically equivalent to store(decode(receive(data))) which follows the expected behaviour of first receiving data, then decoding it, and finally storing it. To get the same semantics with your definition, we would have to write make_composite_function(store, decode, receive). Again, this does the same thing but is arguably harder to decipher during a cursory read-through.

Alas, the linked article goes through a world of trouble to get that "pop back" recursion logic to work (using Boost.Fusion). So they do as they do not because they are oblivious of variadic templates but because variadic recursion simply doesn't cut it. Here's a quote from the article:

  Variadic templates unfortunately work in a "pop front" logic, making the code actually much less straightforward (because these unintelligible recursive preprocessor macros are obviously mundane). You could nevertheless write a pop front variadic template version with an adapter that reverses the list of parameters.

Note the proposed alternative implementation using a reversion adaptor. That would be a nice C++ exercise. I wonder how that would compare complexity-wise.
The linked article's implementation supports std::tuple arguments such as compose(std::make_tuple(f1, f2, f3)). Yours don't. Of course, this adds extra complexity to their implementation compared to yours. There's not much more to say on this point.

To sum up, the difference in complexity is because you seek different interfaces. Both interfaces are valid and work. Which one is more natural depends on the application.

Edit: Here is a code sample of your version. Note that I just use auto return type deduction (as you suggested yourself in the code comment).


A z-shell function to quickly cd into projects I think many know the problem of working with multiple projects: private projects, company projects, probably even projects for multiple companies.

From day-one, I've always searched for better ways to handle all those projects on the file system level.  I think I figured out a nice basic directory structure that works for me and my workflow:

~/code/$company_name/$project_name/[$application_name]

But, even with auto-completion and a $CODE var that pointed to ~/code, I had to type a lot more than I wanted when I had to switch between some projects.

So, for the sake of laziness (I also heard some smart guys call it "efficiency," but nah, I'm just lazy) and to learn a bit more about my shell, I decided to write a function that will do some of the work I'm not willing to do.

These were the functional requirements I defined for that function:

quickly cd into a project by given company and project name
possibility to add an optional application name for projects with multiple applications
possibility to pass a flag to create new project directories

Since I'm still a junior developer (1-year work experience, no degree, autodidact) and don't have much experience with shell-scripting, I'm pretty sure there is a lot to improve in my solution.

And since I'm always on the search for improvements, I decided to show my code to those of whom might be more experienced in this than me.

function pcd () {
  local create=false
  local target_dir="$HOME/code"
  local argument_count=$#

  local root
  local app_name
  local project_name

  # When first param is -c, set create flag to true and remove the param
  if [[ $1 == "-c" ]]; then
    create=true
    (( argument_count=argument_count-1 ))
    shift
  fi

  # the root (private, company1, company2, etc...)
  # I really would like to make that optional too, but
  # that would require
  root=$1

  # $PROJECT_ALIASES is a associative array to map some
  # project aliases to their actual names, it's set globally
  # since it's used in other scripts to and maintained through
  # it's own script
  project_name=$PROJECT_ALIASES[$2]

  # if the passed project string wasn't an alias,
  # assume it was the raw project name
  if [[ ! -n $project_name ]]; then
    project_name=$2
  fi

  # the app folder (for multi application projects, optional)
  app_name=$3

  # validate required params
  if [[ ! (( -n $root && -n $project_name )) ]]; then
    echo "[ERROR] - Please pass at least a root dir and a project name" >&2
    return 1
  fi

  target_dir="$target_dir/$root/$project_name"
  if [[ -n $app_name ]]; then
    target_dir="$target_dir/$app_name"
  fi

  # When the create flag is true (-c), create target directory
  if [[ $create == true ]]; then
    mkdir -p $target_dir
  fi

  # Finally, cd to the targeted directory
  cd $target_dir
}

Remove unused variables

The variable argument_count is not used for anything, so you should remove it.

Simplify shell arithmetics

Instead of this:

(( argument_count=argument_count-1 ))

You could simplify using the postfix operator:

(( argument_count-- ))

Simplify condition

This condition can be written simpler:

if [[ ! (( -n $root && -n $project_name )) ]]; then

Like this:

if ! [[ -n $root && -n $project_name ]]; then

Note that you could also drop both of the -n flags there,
the expression will still mean the same:

if ! [[ $root && $project_name ]]; then

For the same reason, I suggest to drop all the -n in all conditions.

Always double-quote path variables

At some places you did a good job double-quoting path variables:

  target_dir="$target_dir/$root/$project_name"
  if [[ -n $app_name ]]; then
    target_dir="$target_dir/$app_name"
  fi

At others you didn't:

  # When the create flag is true (-c), create target directory
  if [[ $create == true ]]; then
    mkdir -p $target_dir
  fi

  # Finally, cd to the targeted directory
  cd $target_dir

Make it a habit to double-quote path variables always.


Basic jQuery hide/show code My code works perfectly but it is redundant. How does one condense this? How do you use the =! operator? Any help would be greatly appreciated.

$("#about").click(function(){
  $( ".home" ).hide();
  $( ".headr" ).show();
  $( ".about" ).show();
  $( ".skills" ).hide();
  $( ".experience" ).hide();
  $( ".education" ).hide();
  $( ".examples" ).hide();
  console.log("about clicked");
});

$("#skills").click(function(){
  $( ".home" ).hide();
  $( ".headr" ).show();
  $( ".about" ).hide();
  $( ".skills" ).show();
  $( ".experience" ).hide();
  $( ".education" ).hide();
  $( ".examples" ).hide();
  console.log("skills clicked");
});

$("#experience").click(function(){
  $( ".home" ).hide();
  $( ".headr" ).show();
  $( ".about" ).hide();
  $( ".skills" ).hide();
  $( ".experience" ).show();
  $( ".education" ).hide();
  $( ".examples" ).hide();
  console.log("experience clicked");
});

$("#education").click(function(){
  $( ".home" ).hide();
  $( ".headr" ).show();
  $( ".about" ).hide();
  $( ".skills" ).hide();
  $( ".experience" ).hide();
  $( ".education" ).show();
  $( ".examples" ).hide();
  console.log("education clicked");
});

$("#examples").click(function(){
  $( ".home" ).hide();
  $( ".headr" ).show();
  $( ".about" ).hide();
  $( ".skills" ).hide();
  $( ".experience" ).hide();
  $( ".education" ).hide();
  $( ".examples" ).show();
  console.log("examples clicked");
});

1) Add another common class (using toggle in the example) to all of your tags

data

data

data

data

data

data

data

2) on your click handler add data tags (using data-show in this example) for what you want to show

<button data-show="home">home</button>
<button data-show="headr">header</button>
<button data-show="about">about</button>
<button data-show="skills">skills</button>
<button data-show="experience">experience</button>
<button data-show="education">education</button>
<button data-show="examples">examples</button>

3) Set up one function to handle all clicks

$('button').click(function(event) {
  $('.toggle').hide();
  $('.' + event.currentTarget.data.show).show();
  console.log(event.currentTarget.data.show + ' clicked');
});


Getting hours during the day I have this method that returns an array of all the hours for the past 24 hours, ex: if the current time is 3:24pm it looks like @[3pm,4pm...12am,1am...2pm,3pm];

Everything is working as it should, I was wondering if there was a more efficient (or pretty) way to do it than how I have it:

NSCalendar *calendar = [NSCalendar currentCalendar];
NSDate *today = [NSDate date];
NSDateComponents *comps = [calendar components:NSCalendarUnitHour|NSCalendarUnitDay fromDate:today];

NSMutableArray *units = [[NSMutableArray alloc] init];

int currentHour = (int)[comps hour];

for (int i = 0; i < 25; i++) {

    int hourToInsert = currentHour + i;

    NSString *insertString = @"";

    if (hourToInsert == 12 || hourToInsert == 36) {

        //special case for noon
        insertString = @"12PM";

        [units addObject:insertString];

    } else if (hourToInsert == 24) {

        //special case for midnight
        insertString = @"12AM";

        [units addObject:insertString];

    } else if (hourToInsert > 36) {

        //next day PM
        hourToInsert = hourToInsert - 36;

        insertString = [NSString stringWithFormat:@"%iPM", hourToInsert];

        [units addObject:insertString];

    } else if (hourToInsert > 24) {

        //next day
        hourToInsert = hourToInsert - 24;

        insertString = [NSString stringWithFormat:@"%iAM", hourToInsert];

        [units addObject:insertString];

    } else if (hourToInsert > 12) {

        hourToInsert = hourToInsert - 12;

        insertString = [NSString stringWithFormat:@"%iPM", hourToInsert];

        [units addObject:insertString];

    } else {

        insertString = [NSString stringWithFormat:@"%iAM", hourToInsert];

        [units addObject:insertString];

    }

}

Instead of handling all the different cases (am, pm, special rules for
midnight and noon) you can use a date formatter which does all these
things for you:

NSDateFormatter *fmt = [[NSDateFormatter alloc] init];
fmt.dateFormat = @"ha"; // h = 0..12, a = am/pm

NSString *string = [fmt stringFromDate:someDate];

Then note that not all people use the 12hour AM/PM system. You might
consider to display the hours according to the locale/region that a user has
configured on his computer. That is achieved simply by using the date format

fmt.dateFormat = [NSDateFormatter dateFormatFromTemplate:@"h" options:0 locale:[NSLocale currentLocale]];

Another problem occurs when the clock is adjusted for the daylight
saving time. For example, when DST starts in Germany, the hour is
put forward at 2am by one hour, so that the time "2am" does not exist
at all. Similarly, when DST ends and the hour is put back at 3am by
one hour, then time "2am" occurs twice.

Therefore, instead of simply incrementing an "hour" variable, I would
recommend to use calendrical calculations which handle all these
situations correctly:

// A date formatter that prints hours according to the user's locale/region settings:
NSDateFormatter *fmt = [[NSDateFormatter alloc] init];
fmt.dateFormat = [NSDateFormatter dateFormatFromTemplate:@"h" options:0 locale:[NSLocale currentLocale]];

NSCalendar *calendar = [NSCalendar currentCalendar];

// The last hour to print is the full hour of the current point in time:
NSDate *lastHour;
[calendar rangeOfUnit:NSCalendarUnitHour startDate:&lastHour interval:nil forDate:[NSDate date]];

// The first hour to print is one day earlier:
NSDate *firstHour = [calendar dateByAddingUnit:NSCalendarUnitDay value:-1 toDate:lastHour options:0];

NSMutableArray *units = [[NSMutableArray alloc] init];

// hour = firstHour ... lastHour:
NSDate *hour = firstHour;
do {
    [units addObject:[fmt stringFromDate:hour]];

    // Add one hour:
    hour = [calendar dateByAddingUnit:NSCalendarUnitHour value:1 toDate:hour options:0];
} while ([hour compare:lastHour] != NSOrderedDescending); // while hour <= lastHour


Basic Java bank account I'm having a bit of trouble setting up a basic bank account in Java. I'm trying to create a transfer method that I can call to transfer funds from one account to another. The problem is that the method I have created has an unnecessary step where I have to declare the name of the object twice. To execute the transfer method, my current code requires me to write michael.acc.transfer(michael, max); if I want to transfer funds from michael to max. Any ideas on how I can simplify this a bit?

For example: michael.acc.transfer(max);

Here is the class I created with the methods that I can execute to an account:

public class Account {

static double balance;
String accountId;
static int nextId = 0;
static final int ROUTING_NUMBER = 12345;
String bankName;

{
    if (ROUTING_NUMBER == 12345) {
        bankName = "USA Bank";
    }
    else {
        bankName = "Other bank";
    }
}

public void deposit(double amount) {
    balance = balance + amount;
}

public void withdraw(double amount) {
    balance = balance - amount;
}

public void transfer (Customer c1, Customer c2) {
    double transferAmount;
    int routingNumber;
    {
        Scanner input = new Scanner(System.in);
        System.out.println("Please enter transfer amount: ");
        transferAmount = input.nextDouble();

        System.out.println("Please enter recipient's routing number: ");
        routingNumber = input.nextInt();

        if (routingNumber == ROUTING_NUMBER) {
            System.out.println("Your funds will transfer instantly, you and your recipient share the same bank!");
            System.out.println("Bank name: " + bankName);
        } else {
            System.out.println("Your funds will transfer in 2-3 business days.");
        }
    }
    c1.customerBalance -= transferAmount;
    c2.customerBalance += transferAmount;

}
public static String getNextId() {
    return "ACCT #" + nextId++;
}

The next class is the one I created with methods to create the customer:

public class Customer {

public String firstName;
public String lastName;
public Account acc;
public double customerBalance = acc.balance;
int defaultBalance = 100;

public Customer(String firstName, String lastName) {
    this.firstName = firstName;
    this.lastName = lastName;
 }

public Customer() {
    firstName = "John";
    lastName = "Doe";
 }

public void addAccount(double initialBalance) {
    acc = new Account();
    acc.accountId = "ACCT ID: " + Account.getNextId();
    customerBalance = initialBalance;
 }

public void addAccount() {
    addAccount(100);
 }

}

The following are the two accounts that I created to transfer funds from one to the other:

public class Bank {

public static void main(String[] args) {

    Customer max = new Customer("Max", "Doe");
    max.addAccount(1500);

    Customer michael = new Customer("Michael", "Smith");
    michael.addAccount(3000);

 }
}

I don't see any reason why you have to pass the two customers to you transfer method. But first, there are some main remarks :

Do you want to do a transfer from Customer or Account ? Usually I transfer from different accounts not customers. So let's assume that what you want is a method to transfer a given amount of money from one account to another.
You cannot assume that you will have a console when transferring. How can someone reuse this code for a web app ? Please, use a parameter to receive the amount to transfer. And in the meantime, don't ask for the recipient's routing number, because it will be into the recipient account.

So you will have a method like Account.transfer(double amount, Account recipient)

@Test
public void transfer_must_move_amount() {
    Account michael = new Account();
    Account max = new Account();

    michael.transfer(15, max);

    assertThat(max.getBalance()).isEqualTo(15);
}

Your code just have to be refactored a little :

public void transfer(double amount, Account recipient) {
        double transferAmount = amount;
        int routingNumber = recipient.ROUTING_NUMBER;
            if (routingNumber == ROUTING_NUMBER) {
                System.out.println("Your funds will transfer instantly, you and your recipient share the same bank!");
                System.out.println("Bank name: " + bankName);
            } else {
                System.out.println("Your funds will transfer in 2-3 business days.");
            }
        this.balance -= transferAmount;
        recipient.balance += transferAmount;
    }

This test will fail with Expected: 15.0, Actual: 0.0. This is because you balance is static and static members belong to the class instead of one Account. Remove it and everything will be okay.

But there is much more than can be improved on your code.

Protect the Account#balance property form the outside; make it private and initialize it to 0 to be sure.

By doing that your Customer will be red because acc.balance is not avlid anymore. This a good thing because by copying the value, if you change the balance in your Account the customer one will not change anymore. Remove it and replace it by a getBalance() method that will return the balance from your Account.

Since you have one addAccount method, I suppose that a customer may have many accounts, so replace the Account acc field with a collection of Account. The getBalance will returns the sum of all accounts.

Back to the Account where your two deposit and withdraw methods are unused. You may use them via your transfer method. This may safe you later if some Account have some fees on those operations. You just have to override those methods and the transfer method will not be touched.

The only things that we have not touched is the ROUTING_NUMBER. Usually this information is part of your account number. So it may be the good time to add a AccountNumber class. This one is a value object that enforce some rules and can be used to verify if two accounts are in the same bank.

--
I have ignored the nextId and bankName from this review because they are out of your "transfer" scope.

https://github.com/gervaisb/stackexchange-codereview/tree/q183675


Checking a user's emails using an ID I'm trying to do this:

Getting user's ID based on his email name.
Using that user_id to check all his emails that have been sent for him so far.

// db connection here
$sth = $dbh_conn
->prepare("SELECT id FROM users WHERE email = ?")
->execute(array($email));
$num_rows = $sth->fetch(PDO::FETCH_ASSOC);

if($num_rows) {

    $user_id = $num_rows['id'];

    $stmt = $dbh_conn
    ->prepare("SELECT 1
               FROM ( SELECT count(*) AS num_week,
                             sum(date_time > unix_timestamp(DATE_SUB(now(), INTERVAL 1  day))) as num_day,
                             sum(date_time > unix_timestamp(DATE_SUB(now(), INTERVAL 1  hour))) as num_hour,
                             sum(date_time > unix_timestamp(DATE_SUB(now(), INTERVAL 1 minute))) as num_1min
                       FROM resend_pass
                      WHERE user_id   = ?
                        AND date_time > unix_timestamp(DATE_SUB(now(), INTERVAL 1 WEEK))
                    ) a
                WHERE num_week < 12 AND num_day < 6 AND num_hour < 4 AND num_1min < 1;")
    ->execute(array($num_rows['id']));
    $result = $stmt->fetchColumn();

    if ( $result ) {
        echo 'send email';
    } else {
        echo 'you have sent lot of requests to reset your password. try it later';
    }

} else {
    echo 'invalid email address';
}

My code works as well. I guess it can be better.

Here is my table's structure:

// users
+----+--------+----------------------+------------+
| id |  name  |         email        | date_time  |
+----+--------+----------------------+------------+
| 1  | Jack   | jack012@gmail.com    | 1467911301 |
| 2  | John   | J_mx@ymail.com       | 1467920198 |
| 3  | Peter  | Matrix_P@yahoo.com   | 1467933196 |
+----+--------+----------------------+------------+

// resend_pass
+----+---------+-----------------+------------+
| id | user_id |      token      |  date_time |
+----+---------+-----------------+------------+
| 1  | 3       | 8u2934hfr43...  | 1467922396 |
+----+---------+-----------------+------------+

And I'm trying to follow this rule:

  Basic rule of thumb: anytime you have nested queries in a loop, and inner loop is using data from the outer loop, you should re-write as a single joined query.

Do you have any advice?

Some thoughts:

You should strongly consider using appropriate datetime, timestamp, etc. column types when storing date and/or time information in your DB. Unix timestamps may seem simple for working with in PHP, but at some point you will have to query on those fields and having to convert to date/time entities in a query will disallow indexing on that field.
I am concerned that you don't store a flag to indicate if password reset has actually been used (i.e. user clicks on link in email).  Why would you care if a user reset their password 10 times in an hour if they actually completed the whole process each time (something a probing attacker would presumably not be able to do without access to email account)? I would be worried about the number of unused reset attempts.  I would actually also be more concerned with frequent activity coming from a single IP trying to submit different email addresses, but that may require more network-layer security controls.
I think you could simplify your approach by only looking for X number of unused requests within a given time frame. Perhaps 12 per week.  An automated attacker would violate this constraint potentially within seconds anyway, so I don't see why having different thresholds for different periods of time makes much sense, it only adds to complexity. You could possibly remove time threshold altogether and just limit it to X unused attempts total.  At that point you probably either have a legitimate attacker (not likely) or you have a user experience/confusion problem in your process where you might need to give the user more specific guidance, or put them in touch with support, or whatever.
You also may have need to mark a record as expired, that is if there are 10 unsuccessful requests, only the most recent one would actually still work.
I don't like fluent style when performing prepared statements used here, as I think you need to be able to try the prepare first and validate that it worked before trying to execute it.
Consider better name than $num_rows.  You are storing a database record (in this case an array with one element) there, not a row count.


Example of a Multithreaded C program In answering password cracker in c with multithreading, I ended up writing a sample in C, which is not my forte.

Is there anything that I missed which should have been included in a responsible sample (read: likely to be copied and pasted into production code)?

// compile with `gcc counter.c -o counter -lpthread -Wall -std=gnu99`
#include <pthread.h>
#include <stdio.h>
#include <stdbool.h>
#include <limits.h>

#define SEARCH_THREAD_COUNT (10)
#define SEARCH_VALUE (21325)
#define SEARCH_MAX (INT_MAX)

typedef struct {
    int start;
    int end;
    int search;

    bool* shouldStop;
    int* answer;
    pthread_mutex_t* answerMutex;
    pthread_cond_t* answerFound;

    pthread_t thisThread;
} find_worker_init;

void* find_value(void* vpInitInfo) {
    find_worker_init* pUnitOfWork = (find_worker_init*)vpInitInfo;

    int cValue = pUnitOfWork->start;
    while (!(*(pUnitOfWork->shouldStop))) {
        if (cValue == pUnitOfWork->search) {
            printf("found value\n");

            // found the search value
            pthread_mutex_lock(pUnitOfWork->answerMutex);
            if (!(*(pUnitOfWork->shouldStop))) {
                *(pUnitOfWork->shouldStop) = true;
                *(pUnitOfWork->answer) = cValue;

                pthread_cond_broadcast(pUnitOfWork->answerFound);
            }
            pthread_mutex_unlock(pUnitOfWork->answerMutex);
            return NULL;
        }

        cValue++;
        if (cValue == pUnitOfWork->end) {
            // we exhausted our search space, end.
            printf("exhausted\n");
            return NULL;
        }
    }

    // we were usurped by another thread
    printf("usurped\n");
    return NULL;
}

int main( int argc, const char* argv[] ) {
    pthread_mutex_t answerMutex = PTHREAD_MUTEX_INITIALIZER;
    pthread_cond_t answerFound = PTHREAD_COND_INITIALIZER;
    bool shouldStop = false;
    int answer = -1;

    // initialize thread jobs
    find_worker_init startInfo[SEARCH_THREAD_COUNT];
    int current_search_start = 0;
    int search_unit = SEARCH_MAX / SEARCH_THREAD_COUNT;
    for (int i = 0; i < SEARCH_THREAD_COUNT; i++) {
        startInfo[i].start = current_search_start;

        // set the search space for this thread to be either the standard
        // search space, or for the last thread, whatever is remaining
        // (this is to prevent integer-division from skipping a portion
        // at the end)
        int current_size = (i == SEARCH_THREAD_COUNT - 1)
            ? (SEARCH_MAX - current_search_start)
            : search_unit;

        startInfo[i].end = current_search_start + current_size;
        startInfo[i].search = SEARCH_VALUE;

        startInfo[i].shouldStop = &shouldStop;
        startInfo[i].answer = &answer;
        startInfo[i].answerMutex = &answerMutex;
        startInfo[i].answerFound = &answerFound;
    }

    // start threads
    for (int i = 0; i < SEARCH_THREAD_COUNT; i++) {
        if (pthread_create(&(startInfo[i].thisThread), NULL, find_value,
            &(startInfo[i])))
        {
            fprintf(stderr, "Error creating thread\n");
            return 1;
        }
    }

    // wait for answer to be found
    pthread_mutex_lock(&answerMutex);
    while (!shouldStop) {
        pthread_cond_wait(&answerFound, &answerMutex);
    }
    printf("signaled\n");

    // join
    for (int i = 0; i < SEARCH_THREAD_COUNT; i++) {
        if (pthread_join(startInfo[i].thisThread, NULL)) {
            fprintf(stderr, "Error joining thread\n");
            return 1;
        }
    }

    // answer
    printf("answer: %d\n", answer);

    pthread_mutex_unlock(&answerMutex);

    return 0;
}

Mitch, you have a nice program there.  As I pointed out above, current_search_start is not updated which means 9 of 10 threads are searching the same space and the last searches the whole space.  Minor error though.

Your use of condition variables and mutexes is nicely done.  Overall, nothing significant to comment on.

Some minor comments:

The precedence of -> is higher than that of * so this:

if (!(*(pUnitOfWork->shouldStop))) {

can be written:

if (!(*pUnitOfWork->shouldStop)) {

or some might prefer:

if (*pUnitOfWork->shouldStop == false) {

And

*(pUnitOfWork->shouldStop) = true;

can omit the parenthesis and be neater.
I would change some variable names.  cValue seems meaningless. pUnitOfWork would be just work.  The p (and vp) prefixes you used look like Hungarian.  I would never use such things unless forced to by an outdated coding convention.
find_value should strictly be static
The loop in find_value would be more normal as a for:

for (int c = work->start; c < work->end; ++c) {
    if (*work->shouldStop) { ... }
    if (c == work->search) { ... }
}

Also I would add a thread number and some debug to see which thread number is successful (especially with the current_search_start bug, as the thread is not always the same :-)


String vs List to check if a country is part of a continent I simply need to check if a country is part of a particular continent (only one). The continent has around 50 countries. The list is constant and must be initialized at the time of creation. Which one of the solutions below is a better approach from performance point of view?

Having all the counties in a single string and doing simple String.contains()

String countryList = "country1 country2";
boolean isPartOfContinent = countryList.contains("country1");

Creating a List and using contains method

List<String> countryList  = Arrays.asList("country1 ", "country2");
boolean isPartOfContinent = countryList.contains("country1");

I realise the use case is pretty trivial to hamper performance. I feel that List is an overkill for simple stuff like this, whereas my colleague thinks Strings are "meh!" and Java programmers should stick to Lists. What would you use if you had to do this?

The number of countries won't change and is constant so maintainability and performance over a range of countries shouldn't be considered.

To summarise my question: is the use of a comparatively complex data structure than String justifiable when the requirement is so trivial?

Option 1 is sloppy.  By that logic, Guinea is a country in Oceania.  The end-of-string delimiters are an important part of the data and cannot be discarded like that.

Moreover, option 2 has a chance of being faster: if the first few characters fail to match, it can skip to the next item.

However, I would say that neither implementation is recommended.  Better solutions include

A simple data structure that allows searching in O(log n) time:

String[] countries = new String[] { "Countries", "in", "lexicographical", "order" };
boolean isPartOfContinent = (0 <= Arrays.binarySearch(countries, "Country1"));

A more complicated data structure that can perform lookups in constant time.

Set<String> countries = new HashSet<String>(Arrays.asList(…));
boolean isPartOfContinent = countries.contains("Country1");

Considering options 3 and 4 are not any harder to write than options 1 and 2, you might as well choose the better-performing options.