Java Inventory System

I'm often applying to jobs just to test out my skills. I have recently applied to one, had to submit a Java problem, but got rejected. Could someone please review my application briefly and tell me what was the worst mistake in my code? I am interested in improving myself but it's hard for me to figure out what I need improvements on.

Here is the problem:


Introduction


Assume you are working on a new warehouse inventory management system
named IMS. IMS will be responsible for the inventory tracking within
physical, single site warehouses. IMS will track the named physical
location of a product within the warehouse and the inventory level of
each product. IMS will be deployed to busy warehouses supporting many
pickers and restockers working with individual terminals and clients.
Updates to inventory levels will be handled in real time to prevent
pickers trying to pick a product that is out of stock.


Assumptions


Each product will be stored at one and only one named location within
the warehouse. Inventory adjustments may be additive (restocks) or
subtractive (picks). No additional product information needs to be
tracked beyond location and level.


Problem


In Java, implement the picking and restocking routines for the IMS
system. The IMS interface will be the first
component to be implemented; all relevant domain objects will have to
has already been distributed to other teams which depend on it.

GitHub

Here are the important parts:

IMS.java (given to us, to create an inventory)

public interface IMS {
  PickingResult pickProduct(String productId, int amountToPick);
  RestockingResult restockProduct(String productId, int amountToRestock);
}

Inventory.java (implements IMS and it's methods)

```
import java.util.Hashtable;
import java.util.Set;

public class Inventory implements IMS {

private Hashtable products = new Hashtable();
private Hashtable locations = new Hashtable();

public vo

Maps

A HashMap or ConcurrentHashMap (if you require the thread-safety) is preferred over the legacy Hashtable, as mentioned in the Javadoc:


As of the Java 2 platform v1.2, this class was retrofitted to implement the Map interface, making it a member of the Java Collections Framework. Unlike the new collection implementations, Hashtable is synchronized. If a thread-safe implementation is not needed, it is recommended to use HashMap in place of Hashtable. If a thread-safe highly-concurrent implementation is desired, then it is recommended to use ConcurrentHashMap in place of Hashtable.

Also, if you need to iterate through the entries of a Map, there is the Map.entrySet() method to do so. Therefore, instead of:

Set keys = map.keySet();
for (K key : keys) {
    V value = map.get(key);
    // do something with key and value
}

It will be more efficient (not to mention compact) to do this:

for (Entry entry : map.entrySet()) {
    // use temporary variables if required
    K key = entry.getKey();
    V value = entry.getValue();
    // do something with key and value
}

edit: If you only need to iterate through the values, there is values() as pointed out by @njzk2's comment:

for (V value : map.values()) {
    // do something with value
}

Throwing Exceptions

For example:

try {
    loc = getLocationByProductID(productId);
    if (loc == null) {
        throw new Exception("Location wasn't found");
    }
    loc.restockProduct(productId, amountToRestock);
    transactionSuccess = true;
    transactionMessage = "We have successfully restocked " + amountToRestock + 
                            " items of " + productId;
} catch (Exception e) {
    transactionMessage = e.getMessage();
}
return new RestockingResult(transactionSuccess, transactionMessage, 
            loc, product, amountToRestock);

You may want to consider using a custom Exception, e.g. LocationNotFoundException, so that you can describe what the specific error is more programmatically. That will also eliminate the unusually wide catch (Exception e) clause, which is sometimes frowned upon because it is practically a catch-all and hints that the codebase is not even sure what can be thrown-and-caught here.

Also, throwing and then catching an Exception immediately in this style also seems like this is being used to control the program flow. It is as if you are using it to skip the last few lines, i.e. for constructing a 'successful' RestockingResult payload. It is arguably better to simply return a 'unsuccessful' RestockingResult payload once an invalid location is encountered.

BTW, were you instructed how PickingResult and RestockingResult should 'look like'?

edit

int vs Integer

Using the primitive int is often recommended over Integer as it prevents the (wrong?) usage of null, unless you really require something to represent the absence of an int. For example, in your method below:

public void restockProduct(String name, Integer amount) {
    Integer previousAmount = getAmountOfProduct(name);
    amountByProductID.put(name, previousAmount + amount);
}

Sure, you actually do a null check in getAmountOfProduct(String), but there's nothing to stop a caller from accidentally passing in amount as null. 0 + null will result in a NullPointerException.

Also, if you happen to be on Java 8, there's the nicer Map.merge(K, V, BiFunction) method:

public void restockProduct(String name, int amount) {
    amountByProductID.merge(name, Integer.valueOf(amount), Integer::sum);
}

This uses the method reference Integer.sum(int, int) to add any existing value with the new value for the given key.

Type inference for generic instance creation

Since Java 7, type inference for generic instance creation, aka diamond operator, can be used for simplification:

// This
Map map = new HashMap<>();
// Instead of
Map map = new HashMap();

StackExchange Code Review Q#105857, answer score: 23