patternjavaMinor
Thread-safe inventory system
Viewed 0 times
inventorysystemthreadsafe
Problem
I have implemented a thread safe inventory system. The
I have taken the
Product.Java
Location.java
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
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
Notice the following changes:
-
The class is declared as
-
The instance variables are declared as
-
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
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
Now, for your main class,
Inventory.java
should be
Notice:
-
Actual JavaDoc formatting of a comment is used.
-
We define the variable using the interface (
-
We make use Java 7's Diamond Operator.
The same can be done for
For the
Also, no variable should start with a capital letter, or be a single character long (except in some instances). So this:
could be
Additionally, there's a small performance issue using
You're now playing with an array now, and accessing it blindly. This is scary stuff to me, as you're just inviting an
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
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.