Beginner looking for feedback if possible for coding challenge submission

[Iain Ritchie]

Hello, This is my first post on this website so I'm not sure if it's good practice for me to upload as large a post as I have done. Currently I am following along with an online course to learn programming for Java 17. At the end of each section there are challenges to practice our understanding of the topics covered, the one I have just finished was about Java Collections. What I am finding hard with it is to get feedback on the code that I have wrote.
This challenge was the first where there wasn't as much hand holding and the only thing that was given were the class requirements and their methods and fields to create an Inventory System,  and it was up to us to figure out what type of collection to implement and how each method would function.
I was hoping to get some feedback on best practices, I'm not imagining anyone to go through everything line by line but if someone could maybe have a look at the odd bit and point out how they would have done things differently? One of the things I was most interested in was that I made the Cart class an inner class of the Store class and I'm not sure if this is good practice or not as I am struggling to understand the benefits of inner classes and not sure if they are used much so wanted to experiment to find a use for them and this made sense as the when the cart is instantiated it automatically adds itself to the list of Carts belonging to the Store.

Store
import java.time.LocalDate;
import java.util.*;

public class Store {

    private String storeName;
    Map<String, InventoryItem> inventoryItemMap = new HashMap<>();
    Map<Integer, Cart> carts = new HashMap<>();
    Map<Category, Set<InventoryItem>> aisleInventory = new EnumMap<>(Category.class);

    public class Cart {

        private static int allId = 1;

        private int id;
        private Map<Product, Integer> products;
        private int date;
        private Type type;

        public Cart(int date, Type type) {
            this.id = allId++;
            this.date = date;
            this.type = type;
            this.products = new HashMap<>();
            carts.put(id, this);
        }

        public void addItem(String productName, int quantity){
            InventoryItem item = inventoryItemMap.get(productName.toUpperCase());
            if(item != null){
                if(item.reserveItem(quantity)){
                    if (products.computeIfPresent(item.product, (k, v) -> (v + quantity)) == null) {
                        products.put(item.product, quantity);
                    }
                }
            }
        }

        public void removeItem(String productName, int quantity) {
            InventoryItem item = inventoryItemMap.get(productName.toUpperCase());
            if(item == null) {
                System.out.println("There is no : " + productName);
                return;
            }
            if(products.containsKey(item.product)) {
                int amountInCart = products.get(item.product);
                int amountToRelease = (amountInCart < quantity) ? amountInCart : quantity;
                item.releaseItem(amountToRelease);
                products.computeIfPresent(item.product, (k, v) -> v - amountToRelease);
                products.entrySet().removeIf(e -> e.getValue() <= 0);
            } else {
                System.out.println("You don't have any " + item.product.getName() + " in your cart");
            }
        }

        public void printSalesSlip() {

            System.out.printf("Your cart ID:%s%03d%n", (type == Type.VIRTUAL) ? "V" : "P", id);
            products.forEach((k, v) -> {
                InventoryItem item = inventoryItemMap.get(k.getName().toUpperCase());
                System.out.printf("SKU:%-10s %-20s Quantity:%-10d Unit price:£%-10.2f total:£%-5.2f%n", k.getSku(), k.getName(), v,item.salesPrice , (v * item.salesPrice));
            });
            System.out.println("-".repeat(40));
        }

        public void releaseAll(){
            products.forEach((k,v) -> {
                var item = inventoryItemMap.get(k.getName());
                item.releaseItem(v);
            });
            products.clear();
        }

        public void checkout(){
            List<Double> prices = new ArrayList<>();
            printSalesSlip();
            if(products.size() <= 0) {System.out.println("There is nothing in your basket"); return;}
            products.forEach((k,v) -> {
                InventoryItem item = inventoryItemMap.get(k.getName().toUpperCase());
                prices.add(item.salesPrice * v);
                item.sellItem(v);
            });
            Double total = prices.stream().mapToDouble(Double::doubleValue).sum();
            System.out.printf("Your total comes to: £%.2f%n",total );
//            carts.remove(this.id);
        }

        @Override
        public String toString() {
            return "%s: %s-%03d".formatted(storeName, type.toString(), id);
        }
    }


    public Store(String storeName) {
        this.storeName = storeName;
        for (InventoryItem p : ProductData.getInventoryData()){
            inventoryItemMap.put(p.product.getName().toUpperCase(), p);
        }
    }

    private void updateAisles() {
        inventoryItemMap.forEach((k,v) -> {
            if(aisleInventory.computeIfPresent(v.product.getCategory(), (a,b) -> {
                b.add(v);
                return b;
            }) == null){
                aisleInventory.put(v.product.getCategory(), new HashSet<>(List.of(v)));
            }
        });
    }

    public void listProductsByCategory() {
        updateAisles();
        aisleInventory.forEach((k, v) -> {
            System.out.println(k);
            System.out.printf("%-21s %-19s %s%n", "Item:", "Qty:", "Price:");
            v.forEach(System.out::println);
            System.out.println();
        });
    }

    public void manageStoreCarts() {
        carts.forEach((k,v) -> {
            if(v.date < LocalDate.now().getDayOfYear()){
                v.releaseAll();
            }
        });
        carts.values().removeIf(e -> e.date < LocalDate.now().getDayOfYear());
    }

    public void checkOutCarts() {
        if(carts.size() > 0) {
            carts.forEach((k,v) -> v.checkout());
        }
        carts.clear();
    }
}

Product
public class Product {

    private String sku;
    private String name;
    private String manufacturer;
    private Category category;

    public Product(String sku, String name, String manufacturer, Category category) {
        this.sku = sku;
        this.name = name;
        this.manufacturer = manufacturer;
        this.category = category;
    }

    public String getSku() {
        return sku;
    }

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

        Product product = (Product) o;

        return getSku().equals(product.getSku());
    }

    @Override
    public int hashCode() {
        return getSku().hashCode();
    }

    public String getName() {
        return name;
    }

    public Category getCategory(){
        return category;
    }

    @Override
    public String toString() {
        return "%-10s %-20s %s".formatted(sku,name,manufacturer);
    }
}

InventoryItem
public class InventoryItem {

    public Product product;
    private int initialStock;
    private int qtyTotal;
    private int qtyReserved = 0;
    private int qtyReorder = 0;
    private int qtyLow;
    public double salesPrice;

    public InventoryItem(Product product, int qtyLow, int qtyTotal, double salesPrice) {
        this.initialStock = qtyTotal;
        this.product = product;
        this.qtyTotal = qtyTotal;
        this.qtyLow = qtyLow;
        this.salesPrice = salesPrice;
    }

    public boolean reserveItem(int amount){
        if(qtyTotal - amount >= 0) {
            qtyTotal -= amount;
            qtyReserved += amount;
            if(checkStock()){
                qtyReorder = initialStock - qtyTotal;
                placeInventoryOrder();
            }
            return true;
        }
        return false;
    }

    public void releaseItem(int amount){
        qtyReserved -= amount;
        qtyTotal += amount;

    }

    public void sellItem(int amount){
        qtyReserved -= amount;
    }

    public void placeInventoryOrder(){
        System.out.println(qtyReorder + " to be ordered");
    }

    private boolean checkStock(){
        return (qtyTotal <= qtyLow) ? true : false;
    }

    @Override
    public String toString() {
        return "%-20s  qty:%-15s price:£%.2f".formatted(product.getName(), qtyTotal,salesPrice);
    }
}

Main
import java.time.LocalDate;

public class Main {

    public static void main(String[] args) {
        Store store = new Store("Gubbins Galore");

        Store.Cart cart = store.new Cart(LocalDate.now().getDayOfYear(), Type.VIRTUAL);
        Store.Cart cart2 = store.new Cart(LocalDate.now().getDayOfYear(), Type.PHYSICAL);


        cart.addItem("bagels", 4);
        cart.addItem("Apples", 3);
        cart.addItem("Frozen chips", 2);

        store.checkOutCarts();

    }
}

[Carey Brown]

I would make Cart its own class and not an inner class. I reserve inner classes for those that don't need public exposure or ones that need lots of intimate connections to the outer class. Neither of these are your case. In your Cart constructor I would pass in a reference to the Store it belongs to.

I don't like the name "checkStock()" because it gives you no clue as to what the true and false values represent, whereas a name like "isStockLow()" does.

For InventoryItem, product should be private. Once you construct an InventoryItem the product  shouldn't change.

[Junilu Lacar]

Iain Ritchie wrote:
I was hoping to get some feedback on best practices,..One of the things I was most interested in was that I made the Cart class an inner class of the Store class and I'm not sure if this is good practice or not as I am struggling to understand the benefits of inner classes and not sure if they are used much so wanted to experiment to find a use for them and this made sense as the when the cart is instantiated it automatically adds itself to the list of Carts belonging to the Store.

One guiding principle in design is that of Least Knowledge which states that a module or component should only interact with a limited set of other modules or components, and only in a well-defined way.

Ask yourself if a Cart really needs to "know" about the existence of a Store. What responsibilities does a Cart object have that make it necessary for it to interact with (i.e., invoke methods of) a Store? I don't see any.

I can see a Store needing to be aware of Carts and needing to invoke Cart methods, but not the other way around.

That said, I see no reason at the moment to pull Cart out of Store and make its own entity. As you have declared it, the design is saying that a Cart object cannot exist independently of a Store. In other words, the only reason a Cart exists is because there is a Store that needs it. For me this is fine for what you appear to be trying to do.

Programming in Java requires you to think about relationships between ideas like a Store and Cart and how they interact with each other. Creating a two-way dependency by making each one aware of the other complicates your design, and in this case I think it's unnecessary. A one-way relationship with Store being aware of Cart is simpler.

Another principle to keep in mind is that of simplicity. The simpler you keep your design, the easier it is to understand, change, test, and make it work. Always start simple and add on complexity only when you have a simple working system.

[Carey Brown]

GPT:

In Java, you can create inner classes when you want to encapsulate one class within another class. Inner classes have several use cases, and you should consider using them when the following scenarios apply:

1. Encapsulation and Organization:
  - You want to logically group classes together. Inner classes provide a way to create a more organized and encapsulated code structure.
  - You need to restrict the inner class's visibility to only the outer class or other classes within the same outer class. This can help maintain encapsulation.

2. Improved Code Readability:
  - The inner class is tightly related to the outer class, and having it as an inner class makes the code more readable because it shows the close relationship between the two classes.

3. Event Handling:
  - Inner classes are often used in event handling. For example, when creating graphical user interfaces (GUIs) with Java Swing, you might use inner classes to handle button clicks or other events.

4. Iterator or Iterable Implementations:
  - Inner classes can be useful when implementing custom iterators or iterable objects. The inner class can have access to the private fields of the outer class, making it easier to implement custom iterator logic.

5. Callbacks and Callback Interfaces:
  - When you need to implement callback mechanisms or callback interfaces, inner classes can be convenient. You can create an inner class that implements the callback interface and use it to define the callback logic.

6. Implementing Complex Data Structures:
  - In cases where you are creating complex data structures, inner classes can help in implementing nodes, elements, or components of the data structure.

7. Private Helper Classes:
  - Sometimes, you might have utility or helper classes that are only relevant to a specific outer class. In such cases, making these classes inner classes can restrict their visibility and scope to the outer class.

It's important to note that there are different types of inner classes in Java, including:
- **Member Inner Class**: A non-static inner class that has access to the outer class's instance variables and methods.
- **Static Nested Class**: A static inner class that does not have access to the outer class's instance variables and methods.
- **Local Inner Class**: A class defined within a method or code block.
- **Anonymous Inner Class**: A special type of local inner class defined inline, typically used for creating instances of interfaces or abstract classes.

The choice of which type of inner class to use depends on your specific requirements and design considerations. In many cases, the decision to use an inner class is driven by the need for encapsulation and logical organization of your code.
-----------------
Carey:

In your code I see #2 (readability) and #6 (complexity) at play. When you put two classes together the reader has to be vigilant for interactions between the two because the encapsulation is not as clear cut. It  also suggests some underlying complexity is involved, such as your bi-directional dependency that Junilu pointed out. With inner classes you can get complexity-creep because it's so easy to do.

[Iain Ritchie]

Hi Carey and Junilu, thank you for the feedback it has been very valuable to me. I think I need to focus more on stepping back and taking more consideration with how to plan how things will work together rather than jump straight into the code and try to figure things out as I go which is resulting in me treating each class on its own rather than as part of a whole.  

[Campbell Ritchie]

I hope I am not too late to welcome you to the Ranch

Iain Ritchie wrote:. . . I made the Cart class an inner class of the Store class . . .

If you look at the Java™ Tutorials about nested classes, it says that there should be an intimate relationship between the nested class and the outer class. For example, if you have a “Complex Data Structure”, you can say that the Node is part of the Linked List. Or if you have an “Event Handling” class, that may only make sense inside another class. (Note that at least 50% of event handling classes from before Java8 can now be replaced by λ expressions.)
If we use proper British terms (only saying that to annoy the Transatlantic mods ) for your class, you have a Shop and something to replace a Cart. It might mean a Trolley, but I think it doesn't. I think it means a Purchase or better a Selection, which becomes a Purchase when it has been paid for. Or something like that. Now, is a Purchase intimately related to the Shop? I think it isn't. It isn't “part of the Shop” and it has much more to do with the Shopper/Purchaser/Customer. I think a Purchase/Selection would look the same whether I buy it at Sainsbury's or the Co‑Op or Tesco or Morrison's or Mr XXX's round the corner. So, I find myself agreeing with Junilu and Carey: your class Purchase/Selection should be a “top‑level” class.

[Stephen Morris]

That's a good effort! Just a few minor points:

1) Remember to test the code end-to-end, exercising as many pathways as possible
2) Try to define components loosely so that they can be broken into microservices if required
3) Remember that your code is modelling a real world scenario, so it should make sense when you read through it
4) Reading source code is a very useful skill

Well done.

[Campbell Ritchie]

SM: please explain a little more. I think the OP might not be familiar with all those terms.

[Iain Ritchie]

Hi Campbell, thank you for the welcome and the input. I have started to find myself looking at things differently now and take more consideration into the objects and relationships between them and hoping this will better inform my code. I appreciate the input from everyone and Stephen thank you for the suggestions I will start reading up and studying more source code as I think it would help demystify the workings of larger programmes.

[Campbell Ritchie]

Iain Ritchie wrote:. . . thank you

That's a pleasure

. . . studying more source code . . . would help demystify the workings of larger programmes.

I am afraid I think it will cause more confusion than help. Unless it is very well written.
Remembering that nested classes should only be used where there is an intimate relationship with the outer class, let's see if I can remember what I was told about the simplest and most basic way to design classes. Because your “world” is also simple, you will probably get that to work here.
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
    Write a full description of your scenario. Look at all the nouns in it. You will probably get things like shop, customer, purchase, and item. Decide how many of them you really need. A purchase might be carried in a trolley, but do you really need to model the trolley separately? Remove all duplicates. You will now want a class for each of those nouns.
    Look at the verbs. Did you write, “An XYZ contains a PQR,” or, “An ABC has a DEF,” or similar? Decide how many of those states you really need. Remove all duplicates, again. Those states tell you what fields you want for each class. You will probably want getXXX() methods for those fields, but beware of returning mutable reference types. You will probably do better to return a defensive copy of each mutable field. You will probably also want to override the toString() method. Despite what many people will tell you, I think you should not write setXXX() methods for anything. Beware of writing too much. When you write, “A shop has a postal address, a phone number, a website, a front door, a back door, several windows, a floor, a ceiling, several shelves, and a kettle for the staff tea,” you will have written all sorts of things which are true and which you don't need. Remember every additional field constitutes an additional opportunity for something to go wrong.
    More verbs. You will have actions, and each of those actions will need a method to model it. Beware of duplicates; you don't want two methods doing the same thing. In classical object‑oriented programming using mutable types, those methods will alter the state of the object by modifying or replacing its fields.
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
Many people think you should only use static fields for constants and never as variables. I know you will find many books and websites using static fields like nextId, but this principle would prohibit that. Consider changing such fields. Maybe you have a method in shop that produces purchases, in which case the nextId can be an instance field, and instance fields are allowed to be variables.

[Junilu Lacar]

Iain Ritchie wrote:... looking at things differently now and take more consideration into the objects and relationships between them and hoping this will better inform my code. ... I will start reading up and studying more source code as I think it would help demystify the workings of larger programmes.

Read up on design principles and code smells. This will give you a better grounding on what is good and what isn't when you read other people's code.

Some books I'd recommend:

Understanding the four rules of simple design by Corey Haines

Refactoring: Improving the design of existing code by Martin Fowler

Refactoring to Patterns by Joshua Kerievsky

Working effectively with legacy code by Michael Feathers

These are just a few of the books that helped me improve the way I organize my code to make it readable, maintainable, and testable.

[Campbell Ritchie]

Junilu Lacar wrote:. . . Read up on design principles and code smells. . . .

Then leave it for a year and read the same book again. You will probably understand a lot more the second time. And the third time. And the fourth time. And etc.

[Iain Ritchie]

Hi Campbell, that’s a really useful tip. I have started writing my own programme of my own design to put this information into practice. And Junilu thanks for the recommendations, I have made a start on the book by Corey Haines although I am still following along with the programming course so will delve deeper into it once I have finished that