Java Inventory System

Question

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<String, Product> products = new Hashtable<String, Product>();
    private Hashtable<String, Location> locations = new Hashtable<String, Location>();

    public void addLocation(Location location) {
        locations.put(location.getName(), location);
    }

    public void addProduct(Product product, Location location, int amount) {
        products.put(product.getName(), product);
        location.restockProduct(product.getName(), amount);
    }

    /*
     * returns location that has the product when you pass a product ID to it
     */
    public Location getLocationByProductID(String productId) throws Exception {
        Set<String> keys = locations.keySet();
        Location result = null;
        for (String key : keys) {
            Location current = locations.get(key);

            if (current.hasProduct(productId)) {
                if (result == null) {
                    result = current;
                } else {
                    // oh uh, we found the product in two locations, warning the
                    // user
                    throw new Exception("product found in two locations");
                }
            }
        }

        return result;
    }

    public void displayInventory() {
        Set<String> keys = locations.keySet();
        for (String key : keys) {
            Location current = locations.get(key);

            System.out.println(current.getName());
            current.displayInventory();
        }
        System.out.println("");
    }

    @Override
    public PickingResult pickProduct(String productId, int amountToPick) {
        Location loc = null;
        Product product = products.get(productId);

        // transaction data
        boolean transactionSuccess = false;
        String transactionMessage = "";

        try {
            loc = getLocationByProductID(productId);

            if (loc == null) {
                throw new Exception("Product " + productId + " wasn't found in any location");
            }

            int amount = loc.getAmountOfProduct(productId);
            if (amount < amountToPick) {
                throw new Exception("We do not have enough products for this transaction (quantity available: " + amount
                        + "), please restock the product " + productId + "!");
            }

            loc.pickProduct(productId, amountToPick);

            transactionSuccess = true;
            transactionMessage = "We have successfully picked " + amountToPick + " items of " + productId;
        } catch (Exception e) {
            transactionMessage = e.getMessage();
        }

        return new PickingResult(transactionSuccess, transactionMessage, loc, product, amountToPick);
    }

    @Override
    public RestockingResult restockProduct(String productId, int amountToRestock) {
        Location loc = null;
        Product product = products.get(productId);

        // transaction data
        boolean transactionSuccess = false;
        String transactionMessage = "";

        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);
    }

}

Location.java

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

public class Location {

    private String name;
    private Hashtable<String, Integer> amountByProductID = new Hashtable<String, Integer>();

    public Location(String name) {
        this.name = name;
    }

    public void restockProduct(String name, Integer amount) {
        Integer previousAmount = getAmountOfProduct(name);

        amountByProductID.put(name, previousAmount + amount);
    }

    /*
     * returns false if we don't have enough product
     */
    public boolean pickProduct(String name, Integer amount) {
        Integer previousAmount = getAmountOfProduct(name);

        if (previousAmount < amount) {
            // not enough items
            return false;
        }

        amountByProductID.put(name, previousAmount - amount);

        return true;
    }

    public boolean hasProduct(String productId) {
        return amountByProductID.get(productId) != null;
    }

    public Integer getAmountOfProduct(String productId) {
        Integer amount = amountByProductID.get(productId);

        return amount != null ? amount : 0;
    }

    public String getName() {
        return this.name;
    }

    public void displayInventory() {
        Set<String> keys = amountByProductID.keySet();
        for (String productId : keys) {
            Integer amount = amountByProductID.get(productId);

            System.out.println("  " + productId + ": " + amount.toString());
        }
    }
}

(I renamed a few variables just so that it's not searchable by other candidates).

Answer 1

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<K> 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<K, V> 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<KeyType, ValueType> map = new HashMap<>();
// Instead of
Map<KeyType, ValueType> map = new HashMap<KeyType, ValueType>();

Answer 2

h.j.k was very polite concerning Hashtable. Java 2 which deprecated Hashtable came out in 1998. That was 17 years ago. Your first line of actual code starts with a Hashtable. If I were a hiring manager, I would have stopped reading right away. IT moves very fast and using classes that were deprecated 17 years ago is really really bad.

I did not read your code fully, but I think you should avoid exceptions. If a request has no answer, I would just write that in the return value instead of throwing an exception. It will likely happen that impossible requests will be made in an inventory management system and I would not like the whole system to stop because of one impossible request. Exceptions should be reserved for truly exceptional events, and I don't think erroneous requests are exceptional events in an inventory system. There is also a performance cost to using exceptions over standard return values. Others might argue that using exceptions is the right way to proceed here. It's even possible they requested that you use exceptions to conform to their API.

Answer 3

While location is mentioned, it's not present in the interface you were given, so I'm not sure I'd bother with it in the absence of a clearly defined requirement. That is, even if you perfectly implemented location, there's no way in the interface you were given, for a client program to set a location. (A location can be returned in PickingResult and RestockingResult, but any such location would be arbitrary -- you might as well return "Someplace" or "in the warehouse" for every location.)

There are a few more hints in the IMS interface; that fact that it returns PickingResult and RestockingResult is a pretty clear indication that throwing exceptions is NOT wanted.

What should the PickingResult and RestockingResult values be when picking/restocking succeeds? When they fail? When can they fail?

Should PickingResult and RestockingResult be concrete leaf classes, or are they better as interfaces/abstract classes? (I.e., like Scala's abstract Option, with concrete subclasses Some() and None.)

One of the requirements is that "Implementations of this interface and access to shared data must be thread-safe." Are there places where your code is not thread-safe? (Hint: yes, several.)

Hint: look at https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/atomic/AtomicInteger.html.

You'll need to understand exactly how to use it (in particular, retries) and it won't solve all your concurrency issues, but it will solve most. (Which problem won't it solve, and what can you do to solve that?)