4
\$\begingroup\$

I have implemented a thread safe inventory system. The Product is bound to a location in a warehouse. I have a Product class and a Location class. In the Product class constructor, you will pass the Location. Further, I have an Inventory class which will read from input file to build the inventory in a ConcurrentHashMap data structure for binding the product and the quantity.

I have taken the ConcurrentHashMap so as to have the pick method and restock method inside the Inventory class to be thread safe. I have also taken HashMap for binding product id and product so as to retrieve product from id in \$O(1)\$ time. The ConcurrentHashMap is taken so as to implement pick and restock method quickly as quantity can be retrieved in \$O(1)\$ time and can be updated quickly. Also to mention here, Inventory will build a ConcurrentHashMap from Inventory.txt which is written in: ProductId, ProductName, InitialQuantity, LocationId, LocationName. The records of the file are: 1 Pens 20 100 Walmart, 2 Pencils 50 101 Walmart.

Product.Java

public class Product {

    int id;
    String name;
    Location loc;

    Product(int id, String name, Location loc){
        this.id=id;
        this.name=name;
        this.loc=loc;
    }

}

Location.java

public class Location {

    int id;
    String name;

    Location(int id, String name){

        this.id=id;
        this.name=name;

    }

}

Inventory.java

import java.io.BufferedReader;
import java.io.FileInputStream;
import java.io.InputStreamReader;
import java.util.HashMap;
import java.util.concurrent.ConcurrentHashMap;


public class Inventory {

    /*Map for binding Product with Quantity. This map will speed up the things in picking and re-stocking.
    otherwise, we have to iterate every time.*/

    ConcurrentHashMap<Product,Integer> Inv=new ConcurrentHashMap<Product, Integer>();

    //Map for binding product id with Product object
    HashMap<Integer, Product> actualProduct=new HashMap<Integer, Product>();


    //Build inventory from input file. The file record is written in: ProductId, Product Name, Initial Quantity, Location Id, Location Name
    //The records of the file are: 1 Pens 20 100 Walmart, 2 Pencils 50 101 Walmart

    void buildInventory(String filename){

        try{
        FileInputStream fstream = new FileInputStream("C:\\dev\\eclipse_workspace\\Warehouse\\src\\"+filename);
        BufferedReader br = new BufferedReader(new InputStreamReader(fstream));

        String strLine; 

        //Read File Line By Line
        while ((strLine = br.readLine()) != null){

          //split string
          String S[]=strLine.split(" ");
          int id=Integer.parseInt(S[0]);
          int quantity=Integer.parseInt(S[2]);

          //create location
          int lid=Integer.parseInt(S[3]);
          Location L=new Location(lid, S[4]);

          //create product
          Product P=new Product(id, S[1], L);

          //Put product and Quantity in ConcurrentHashMap
          Inv.put(P, quantity);
          //Put id and Product in HashMap
          actualProduct.put(id, P);

        }

        //Close the input stream
        br.close();

       }catch(Exception e){
        e.printStackTrace();
    }
}

    //Print current inventory
    void printInventory(){

        System.out.println("Printing Inventory");
        for(Product P:Inv.keySet()){
            System.out.print(P.id+" ");
            System.out.print(P.name+" ");
            System.out.print(P.loc.id+" "+P.loc.name+" ");
            System.out.print(Inv.get(P));
            System.out.println();
        }

    }

    //Subtract quantity if the product is purchased.
    void pickProduct(int id, int quantity){
        if(!actualProduct.containsKey(id))
        System.out.println("Sorry, the product is not available in inventory");

        else if(quantity<=0)
        System.out.println("Invalid Quantity");

        else if(Inv.get(actualProduct.get(id)).intValue()<quantity)
        System.out.println("Not enough stock");

        else
        Inv.put(actualProduct.get(id), Inv.get(actualProduct.get(id)).intValue()-quantity); 
    }

    //Add quantity so as to restock the product
    void restockProduct(int id, int quantity){
        if(!actualProduct.containsKey(id))
        System.out.println("Sorry, the product is not available in inventory");

        else if(quantity<=0)
        System.out.println("Invalid Quantity");

        else
        Inv.put(actualProduct.get(id), Inv.get(actualProduct.get(id)).intValue()+quantity); 
    }

    public static void main(String args[]){
        System.out.println(args[0]);
        final Inventory Inv=new Inventory();
        Inv.buildInventory(args[0]);
        Inv.printInventory();
        //Inv.pickProduct(2, 40);
        //Inv.printInventory();

        Thread t1 = new Thread(new Runnable() {
               public void run() {
                  Inv.pickProduct(2, 40);
               }
            });
            t1.start();

        Inv.printInventory();

        Thread t2 = new Thread(new Runnable() {
                   public void run() {
                      Inv.pickProduct(2, 40);
                   }
                });
            t2.start();

        Inv.printInventory();

        Thread t3 = new Thread(new Runnable() {
                   public void run() {
                      Inv.restockProduct(2, 50);
                   }
                });
            t3.start();

        Inv.printInventory();

    }

}
\$\endgroup\$
0

2 Answers 2

3
\$\begingroup\$

For all of your small value classes, you should have access modifiers and, optionally, accessors. For example,

Product.java

public final class Product {

    public final int id;
    public final String name;
    public final Location location;

    public Product(final int id, final String name, final Location location) {
        this.id = id;
        this.name = name;
        this.location= location;
    }
}

Notice the following changes:

  • The class is declared as final so it cannot be extended.

  • The instance variables are declared as public and final. Without the access modifier, the variables are defaulted to package access. And since you don't modifier them either, they can be nicely declared as immutable.

  • The constructor parameters are declared as final. This is entirely optional, but ensures that you're not modifying them, which is often a no-no.

  • Some lovely spacing in the assignments.

  • Renaming loc to location.

In the above refactor, I didn't provide any accessors because the fields are now final. However, if you wanted to, you could do so easily:

Location.java

public final class Location {

    private final int id;
    private final String name;

    Location(final int id, final String name) {
        this.id = id;
        this.name = name;
    }

    public int getId() {
        return id;
    }

    public String getName() {
        return name;
    }

}

Now, for your main class,

Inventory.java

/*Map for binding Product with Quantity. This map will speed up the things in picking and re-stocking.
otherwise, we have to iterate every time.*/

ConcurrentHashMap<Product,Integer> Inv=new ConcurrentHashMap<Product, Integer>();

should be

/**
 * Map for binding Product with Quantity. This map will speed up the things in picking and re-stocking;
 * otherwise, we have to iterate every time
 */
Map<Product,Integer> inventory = new ConcurrentHashMap<>();

Notice:

  • Actual JavaDoc formatting of a comment is used.

  • We define the variable using the interface (Map) rather than an implementation.

  • We make use Java 7's Diamond Operator.

The same can be done for actualProduct.

For the buildInventory method, take a look into another Java 7 feature: Try-With-Resources. This will take care of closing your streams even in the event of an exception (which you're not currently doing).

Also, no variable should start with a capital letter, or be a single character long (except in some instances). So this:

String S[]=strLine.split(" ");

could be

final String[] values = strLine.split(" ");

Additionally, there's a small performance issue using String#split, as a new Pattern has to be compiled each time. You can futher improve this with the following:

private static final SPLIT_PATTERN = Pattern.compile(" ");

...

final String values = SPLIT_PATTERN.split(strLine);

You're now playing with an array now, and accessing it blindly. This is scary stuff to me, as you're just inviting an ArrayIndexOutOfBoundsException to occur. Always assume faulty input, and be sure to handle those exceptional cases. Catching Exception e at the bottom of your method is not a suitable solution.

Here is a possible refactor of the method:

  private static final Pattern SPLIT_PATTERN = Pattern.compile(" ");

  private static final int ID_INDEX = 0;

  private static final int PRODUCT_INDEX = 1;

  private static final int QUANTITY_INDEX = 2;

  private static final int LOCATION_ID_INDEX = 3;

  private static final int LOCATION_INDEX = 4;

  private static final int EXPECTED_INDEX_SIZE = 4;

  public void buildInventory(final String filename) {

    try (final InputStream inputStream =
             new FileInputStream("C:\\dev\\eclipse_workspace\\Warehouse\\src\\" + filename);
         final BufferedReader reader =
             new BufferedReader(new InputStreamReader(inputStream))) {
      String strLine;

      while ((strLine = reader.readLine()) != null) {
        try {
          final String line[] = SPLIT_PATTERN.split(strLine);

          if (EXPECTED_INDEX_SIZE != line.length) {
            System.out.println("Malformated line: " + strLine);
            continue;
          }

          final int id = Integer.parseInt(line[ID_INDEX]);
          final int quantity = Integer.parseInt(line[QUANTITY_INDEX]);
          final int locationId = Integer.parseInt(line[LOCATION_ID_INDEX]);

          final Location location = new Location(locationId, line[LOCATION_INDEX]);
          final Product product = new Product(id, line[PRODUCT_INDEX], location);

          inventory.put(product, quantity);
          actualProduct.put(id, product);
        } catch (final NumberFormatException e) {
          System.out.println("Error parsing value: " + e.getMessage());
        }
      }
    } catch (final FileNotFoundException e) {
      System.out.println("Error reading file: " + e.getMessage());
    } catch (final IOException e) {
      System.out.println("Error reading file: " + e.getMessage());
    }
  }

}

Finally, if you're going to store objects in a hashmap, it's recommended practice to override equals and hashcode. IDEs can typically do this for you. For example, IntelliJ gives me the following for your Product class (which you use as a key in Inv object`):

  public final class Product {

    public final int id;
    public final String name;
    public final Location location;

    public Product(final int id, final String name, final Location location) {
      this.id = id;
      this.name = name;
      this.location = location;
    }

    @Override
    public boolean equals(Object o) {
      if (this == o) return true;
      if (o == null || getClass() != o.getClass()) return false;

      Inventory.Product product = (Inventory.Product) o;

      if (id != product.id) return false;
      if (name != null ? !name.equals(product.name) : product.name != null) return false;
      return !(location != null ? !location.equals(product.location) : product.location != null);

    }

    @Override
    public int hashCode() {
      int result = id;
      result = 31 * result + (name != null ? name.hashCode() : 0);
      result = 31 * result + (location != null ? location.hashCode() : 0);
      return result;
    }

  }
\$\endgroup\$
6
  • 1
    \$\begingroup\$ For member variable declarations, using final != immutable. It only means the reference cannot be changed, not that values of the class being referenced cannot be changed. \$\endgroup\$
    – Eric Stein
    Commented Oct 12, 2015 at 21:04
  • 1
    \$\begingroup\$ @EricStein Yes that's true, but if the referenced fields are also immutable, as I've done in the Location class which is a member of the Product class, then you effectively have immutability. \$\endgroup\$
    – lealand
    Commented Oct 12, 2015 at 21:08
  • \$\begingroup\$ @EricStein : If I want to change the quantity of a product, then final should not be used in 'Product' class. Only access modifier public should remain there in Product class. But the thing is if members fields are not final, how it can be made thread safe? \$\endgroup\$
    – Akansha
    Commented Oct 13, 2015 at 0:05
  • \$\begingroup\$ @lealand I agree. I just wanted to call out that final by itself does not mean immutable. That's a common mistake for junior devs. \$\endgroup\$
    – Eric Stein
    Commented Oct 13, 2015 at 0:09
  • \$\begingroup\$ Thanks @EricStein and Iealand for your inquisitive remarks.. \$\endgroup\$
    – Akansha
    Commented Oct 13, 2015 at 0:25
3
\$\begingroup\$

Member Variables Access:

You are not specifying an access modifier, Java defaults to package private. This means that any code outside of the package containing these classes can not access any of the member variables. This greatly reduces the usability of Product and Location.

In addition to this, there is nothing preventing code in the same package from replacing the existing values in Inventory.Inv or Inventory.actualProduct. If this happens, your code will start having many unexpected results (including runtime exceptions). Even if I don't replace the current instance, all of the public methods on both maps are accessible allowing for adding and removing elements outside of the control of Inventory.

Instead, you should make both Inventory member variables private final. These means 1) only Inventory can access the variables and 2) once they are assigned a value, it may not be changed. The second part will prevent Inventory from accidentally creating a new map instance.

In the case of Product and Location, they are both simple data objects. In these simple cases, it might be acceptable to directly expose the member variables. However, you have to take care in doing this. For example, say you want to represent a circle and in doing so, expose a radius variable. Everything is happy and other people start using your code. Then someone comes by and says because of reason X, the implementation of all circles needs to be done with a diameter. But since your public API already exposes a radius variable, removing it would everyone using your code now needs to change. However, if you had previously made the variable private and had a getRadius() method, you can easily change the class's underlying implementation without effecting anyone using your code.


Names:

The convention for member variables in Java is camelCase. This means the concurrent map should not be capitalized.

My personal preference is to prefix member variables with an underscore. This is not a standard convention, so you can choose not to use it, but it has advantages.

  1. You never have to worry about variable shadowing.
  2. You never have to prefix them with this. because the function's arguments or local variables will never have the same name.
  3. You can easily see what variables being used in a function are local versus member variables.

The variable names in Inventory are bad. Inv doesn't tell me any thing. If the class instance is the inventory, why does this member variable have a name that implies it is an inventory? Or maybe Inv actually stands for something else? I don't know because the name was abbreviated just to save a few characters. On the other side, actualProduct is a collection of things, but the name makes me thing it is only a single item. Your variable name should help the reader understand what value is being stored there. When you have multiple maps that are related, it is important to distinguish the relationship of the key to the value for each map.


Mutability:

I've touched on it in a few places, but your code assumes people will use it the way it was indented and not break things. When you are trying to deal with thread safety, this is a very dangerous assumption. I've already explained why the Inventory member variables should be private final. When dealing with data objects like Product and Location, thread safety is much easier when the objects are not mutable. This means making their member variables final as well. Now you don't have to worry about the id for the pen object changing from 1 to 100.

Map Lookups and Hashcodes:

Inv.put(actualProduct.get(id), Inv.get(actualProduct.get(id)).intValue()+quantity);

This is super ugly. In order to do a simple increment, you are doing 3 look ups. One can be immediately removed by storing the actual product in a local variable.

Product acutal = actualProduct.get(id);
Inv.put(acutal, Inv.get(acutal).intValue()+quantity);

The API you expose to the user expects an id, but mapping of products to quantities doesn't use the id as the key. Maybe that isn't the best way to do things.

Lets try changing the API to take a product instance instead of an id.

void restockProduct(Product product, int quantity){
    //... 
    Inv.put(product, Inv.get(product).intValue()-quantity); 
}

That seems nicer from the implementation stand point. However it won't work because two Product instances with exactly the same values are not seen as equivalent and will not be seen as the same key. In oder to solve this, you need to override equals() and haschode() for you data objects. And now that they are immutable, you don't need to worry about the hashcode of an instance changing when a member variable changes.

Taking a Product instance in the API might not be the correct answer, but having a map that uses a mutable value where the hashcode does not match what people would expect of objects with equivalent values is the wrong way to do things.

Curly Brackets and Indentation:

void pickProduct(int id, int quantity){
    if(!actualProduct.containsKey(id))
    System.out.println("Sorry, the product is not available in inventory");

    else if(quantity<=0)
    System.out.println("Invalid Quantity");

    else if(Inv.get(actualProduct.get(id)).intValue()<quantity)
    System.out.println("Not enough stock");

    else
    Inv.put(actualProduct.get(id), Inv.get(actualProduct.get(id)).intValue()-quantity); 
}

This is hard to people to read and easy to introduce unexpected bugs. If you aren't paying attention, you might not notice the chained else statements. Or you might want to add another statement to one of the blocks and completely break everything else. Instead, do the following:

void pickProduct(int id, int quantity){
    if(!actualProduct.containsKey(id)) {
        System.out.println("Sorry, the product is not available in inventory");
    } else if(quantity<=0) {
        System.out.println("Invalid Quantity");
    } else if(Inv.get(actualProduct.get(id)).intValue()<quantity) {
        System.out.println("Not enough stock");
    } else {
        Inv.put(actualProduct.get(id), Inv.get(actualProduct.get(id)).intValue()-quantity); 
    }
}

The indentations make it clear what the sub-block is. The curly brackets ensure that a second line can easily be added without worrying about if curly brackets are now required.

Separation of Concerns:

Inventory knows about anything this code will ever do and this prevents it from being used in other locations.

  • buildInventory() is the only way to add items to the inventory. In tern, this means this code can only be used on a computer that:
    1. Runs Windows
    2. Has the directory path \dev\eclipse_workspace\Warehouse\src\.
    3. That directory contains a files with a specific custom format.
  • All of the other methods write directly to stdout. This means the code can only be used as a console application or it will write a bunch of output that might not make sense for a GUI application.
  • There are very few operations that can be done with the information in the inventory and there is no way to do anything else.

I'm fairly confident that you are just starting out, so all of these things are not that surprising. It is more important to see how things can be done differently to allow you to reuse the code you have already written in different ways. While the following suggestions will make the project more complex, I hope you can see how it provides more flexibility.

  • Keep all the business logic in Inventory.
  • Extract all the output to the user to a different class that uses an Inventory instance.
  • Extract the code to parse the file and produce the initial mapping of products to quantities. Then pass that mapping in as an argument to the Inventory constructor.

In general, each class should do one thing and do it well. The Inventory class should be in charge of tracking what products it contains and how many items exist for each product. It might have the following interface:

public interface Inventory {
  void addProduct(Product product);
  void removeProduct(Product product);
  void stockProduct(Product product, int count);
  void pickProduct(Product product, int count);
  List<Product> allProducts();
  int countOf(Product product);
}

There might be a few other methods to make the interface easier to work with, such as one that can add a product with an initial count or add a collection of products, but the basics are there.

There would then be an independent class that would be in charge of parsing your file format and producing the a Map<Product, Integer> that could be then added to the Inventory. This way, if you wanted to change your application so that the user can add a new product, they can do that directly instead of editing a text file and then restarting the application.

Finally, there would a third class that handles interacting with the user. As it interacts with the Inventory instance, it decides how these changes should be displayed to the user. By separating the UI from the business logic you can change from a console front end to a GUI or web front end without needing to make any changes to the code Inventory class. Your main() could be as simple as simple as creating am Inventory instance and passing it to the UI class and telling the UI class to start executing.

Thread Safety:

I touched on a few things in some of the other points, but I haven't addressed the main implementation as a whole. The current implementation is not thread safe. While you are using a ConcurrentHashMap to store the quantities, pickProduct() and restockProduct() both make multiple calls to that instance. ConcurrentHashMap ensures that one thread can't call put() while another thread calls get(). However, it does not prevent the following:

  1. T1: map.contains(x);
  2. T2: map.remove(x);
  3. T1: map.get(x);

In this case, the class that has the reference to the Map must ensure that multiple sequential calls happen in a logical block.

public Inventory {
  private final Object _lock = new Object();
  private final Map<Product, Integer> _quantities = new HashMap<>();
  private final Map<Integer, Product> _idToProduct = new HashMap<>();

  public pickProduct(int id, int quantity) {
    synchronize(_lock) {
      if (!idToProduct.containsKey(id)) {
        throw new NoProductException(id); // the UI class would catch this and decide how to tell the user something bad happened
      }  else if (quantity <= 0)
        throw new IllegalArgumentException(quantity);
      }
      Product product = _idToProduct.get(id);
      int currentCount = _quantities.get(product); // Java will unbox the value for you
      if (currentCount < quantity) {
        throw InsufficentQuantityException(id, quantity);
      }
      _quantities.put(product, currentCount - quantity); 
    }
  }
}

Once you follow the similar pattern for wrapping your public methods in synchronize blocks, this will ensure that one thread can not partially execute pickProduct() while another thread is trying to restockProduct() the same product.

\$\endgroup\$
13
  • \$\begingroup\$ Great advice. +1 for the separation of concerns section. \$\endgroup\$
    – lealand
    Commented Oct 12, 2015 at 20:31
  • \$\begingroup\$ Anyone who can provide more insight to the design of Thread-safe inventory system?? \$\endgroup\$
    – Akansha
    Commented Oct 13, 2015 at 1:13
  • \$\begingroup\$ @unholysampler: Can you please elaborate on second point? Extract all the output to the user to a different class that uses an Inventory instance. And you mentioned to Extract the code to parse the file..I want to know that I can construct Map in my main method and pass that Map to constructor?? That will be a good design.. \$\endgroup\$
    – Akansha
    Commented Oct 13, 2015 at 3:06
  • \$\begingroup\$ @unholysampler: Also, I like your insight to the data structure. Can you help me choosing more appropriate data structure for Thread-safe inventory system? \$\endgroup\$
    – Akansha
    Commented Oct 13, 2015 at 3:11
  • \$\begingroup\$ @h.j.k. : Can you please comment as I saw your comments in an another discussion on inventory system? \$\endgroup\$
    – Akansha
    Commented Oct 13, 2015 at 4:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.