Total cost calculator for shopping cart

Problem Statement

Design a system which helps calculate the TotalCost of the items in the Cart. You will be given a list of items in the Cart with Discounts like in the example below. The list would contain either items or discounts and the sequence matters:

Sample cart: $10 Book xyz, 10% Discount on all items,$20 stationary, 20% discount on next item, $100 Shirt,$15 off on 5th Item of Type Book.

Types of Discounts: 10% Discount on all items 20% discount on next item $15 off on 5th Item of Type Book (More type of Discounts can be added later to the system) Here is my solution for this, I will appreciate pointers to improve further: CartItem package cart; public interface CartItem { double value(); String description(); }  Product package cart; public class Product implements CartItem { private double unitPrice; private String name; public Product(double unitPrice, String name) { this.unitPrice = unitPrice; this.name = name; } @Override public double value() { return unitPrice; } @Override public String description() { return name; } }  Coupon package cart; public class Coupon implements CartItem { private double value; private String description; private String type; public Coupon(double value, String description, String type) { this.value = value; this.description = description; this.type = type; } public String getType() { return type; } @Override public double value() { return value; } @Override public String description() { return description; } }  Cart package cart; import discount.Discount; import discount.DollarDiscount; import discount.PercentDiscount; import java.util.*; public class Cart { List<CartItem> cartItemList; public Cart() { cartItemList = new ArrayList<>(); } public void add(CartItem cartItem) { cartItemList.add(cartItem); } private Discount getDiscountStrategy(Coupon coupon, Product product) { if(coupon.getType() == "PercentDiscountForAll" || coupon.getType() == "PercentDiscountForNext") { return new PercentDiscount(product, coupon); } else { return new DollarDiscount(product, coupon); } } private double checkout() { Map<Product, Integer> productMap = new LinkedHashMap<>(); double total = 0; Iterator<CartItem> itemIterator = cartItemList.listIterator(); while(itemIterator.hasNext()) { CartItem cartItem = itemIterator.next(); if(cartItem instanceof Product) { productMap.put((Product) cartItem, productMap.getOrDefault(cartItem, 0)+1); total += cartItem.value(); } else if (cartItem instanceof Coupon) { if(((Coupon) cartItem).getType() == "PercentDiscountForAll") { for(Product product : productMap.keySet()) { total = total - product.value(); double discountedPrice = getDiscountStrategy((Coupon) cartItem, product).applyDiscount(); total = total+discountedPrice; } } else if (((Coupon) cartItem).getType() == "PercentDiscountForNext") { //Code and percent discount for next will come here. } else if (((Coupon) cartItem).getType() == "DollarDiscountForNth") { //Code and dollar discount for Nth will come here. } } } return total; } public static void main(String[] args) { Cart cart = new Cart(); CartItem apple1 = new Product(1, "Apple"); CartItem apple2 = new Product(1, "Apple"); CartItem apple3 = new Product(1, "Apple"); CartItem coupon = new Coupon(10, "10% Percen Discount For All", "PercentDiscountForAll"); cart.add(apple1); cart.add(apple2); cart.add(apple3); cart.add(coupon); System.out.println(cart.checkout()); } }  Discount package discount; public interface Discount { double applyDiscount(); }  PercentDiscount package discount; import cart.Coupon; import cart.Product; public class PercentDiscount implements Discount { private Product product; private Coupon coupon; public PercentDiscount(Product product, Coupon coupon) { this.product = product; this.coupon = coupon; } @Override public double applyDiscount() { double discountedPrice = product.value() - ((product.value() * coupon.value())/100); return discountedPrice; } }  DollarDiscount package discount; import cart.Coupon; import cart.Product; public class DollarDiscount implements Discount { private Product product; private Coupon coupon; public DollarDiscount(Product product, Coupon coupon) { this.product = product; this.coupon = coupon; } @Override public double applyDiscount() { return product.value() - coupon.value(); } }  I have tried to do this using both composite and strategy pattern, for discount I am trying to use Strategy Pattern and for items in cart I am trying to use Composite Pattern . I will highly appreciate any pointers to improve or simplify this. • Welcome to Code Review! I hope you get some great answers. – Phrancis May 31 '18 at 17:17 1 Answer Here are my comments in order of severity 1) Bugs 1.1) String comparison You use == operator to compare String values. You probably would discover that yourself had you done more extensive testing than what is specified in main() 1.2) Product Type From the assignment I understand that coupons may apply to products of a certain type. So Product must have a type property. This is also evident from the example that specifies a product as a three properties' entity: "$10 Book xyz"

1.3) No documentation

There are zero comments in the code (apart from TODO placeholders). That made my work (and all other persons who are not you) all the more difficult. Altough this will not cause run time errors, In my book, that is a bug. For example, it took me a while to figure out your total calculation. I am still puzzled by productMap, especially since you never retrieve the values of the map.

2) Design

First of all, the code is incomplete. You did not show how you intend to "match" a coupon with a single product. Perhaps you intended to use productMap, but you build the map and apply the coupons in the same iteration so a coupon that specifies a discount on the next product will not find its match in the map.

2.1) Single responsibility principle

The Cart class is responsible for determining the discount strategy. This means that adding a new type of discount (as is stated in the assignment) will require a change in the Cart class. The clsss should be limited to adding items and then figuring out total amount.

2.2) Coupon type

The coupon type is specifying two separate things:

1. percent or fixed amount
2. target product/s

This is problematic. First of all these are not mutually exclusive. You can have a percent discount for next item and also a fixed amount for the next item. Second, in getDiscountStrategy() you only need to know the first point, and in checkout() you only need the second. You should separate the coupon type into two distinct variables. Perhaps coupon can recevie a reference to an instance of Discount in the constructor and then Cart won't need getDiscountStrategy()

Note: fixed amount discount is a better name than dollar discount since you may wish to support other currencies.

2.3) Extensibility: further breaking down coupon type

(this is an advanced topic that may fall into "nice to have" category.) If we try to accomodate for the requirement to support future discount types, we can say that target product can also break into two things:

1. filter based on product properties
2. quantifier

So the client can specify one or more filter criteria. product type is just one example. product country of origin may be another (imagine a "Canada independence day" sale); product age rating ("going bak to school" sale); and so on.

going even further, a coupon may alter its behavior if other coupons exist, and so on.

To support this, you need to give a coupon the capability to traverse the cart and search for a match by itself.

2.3) Make use of Enums

discount strategy, whether it is the original set of values or the broken down version, is a predefined closed set. an Enum here would be fitting. This has the advantage that the compiler will help if you misspell a certain value. Furthermore, you can attach an instance of DiscountStrategy to every enum value:

enum DiscountType {
PERCENT (new PercentDiscount()),
FIXED (new FixedAmountDiscount()),
...

public Discount getDiscount() {
return discount;
}
private Discount discount;

private DiscountType(Discount discount) {
this.discount = discount;
}
}


So the coupon class will have an instance var of type DiscountType and you directly get the proper Discount implementation.

2.4) Make use of Lambda expr

Looking at Discount we see that this fits the Functional Interface definition. This means that instead of having to write a separate implementation class, we can write a Lambda expr as the enum argument:

enum DiscountType {
PERCENT ((product, coupon) -> product.value() - ((product.value() * coupon.value())/100),
FIXED ((product, coupon) -> product.value() - coupon.value()),
...


and there you have it: the enum and its implementation in one line.