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