HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Thread-safe inventory system

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
inventorysystemthreadsafe

Problem

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 Inv=new ConcurrentHashMap();

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

//Build inventory from i

Solution

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 Inv=new ConcurrentHashMap();


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 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.get

Code Snippets

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

}
/*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 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<>();
String S[]=strLine.split(" ");

Context

StackExchange Code Review Q#107305, answer score: 3

Revisions (0)

No revisions yet.