5
\$\begingroup\$

I want to create a shopping cart project with discounts for products. For instance, if you buy 2 books, you can get 10% off Parker pens. I created a shopping cart example, but without exception handling in this version.

Are my classes Product, Basket, Offers and Discounts properly structured, or what changes do I need to make to make it right? To calculate discounts, should I iterate my basket or iterate the list of offers? The offers are a little tricky as mentioned at the top.

Product

public class Product {

    private String name;
    private int price;

...with get methods + hashcode and equals.

Basket

public class Basket {

    Map<Product, Integer> items;    

    public Basket() {
        items = new HashMap<>();
    }

    public Map<Product, Integer> getItems() {
        return items;
    }

    public void addProduct(Product product){
        if(items.containsKey(product)){
            items.compute(product, (p,q ) -> Integer.valueOf(q+1));
        }else{
            items.put(product, 1);
        }
    }

    public void removeProduct(Product product){
        if(items.get(product).intValue() > 1){
            items.compute(product, (p, q) -> Integer.valueOf(q-1));
        }else{
            items.remove(product);
        }
    }
}

Discount

public class Discount {

    private Product product;    
    private int discountPercent ;

    public Product getProduct() {
        return product;
    }

    public int getDiscountPercent() {
        return discountPercent;
    }

    public Discount(Product product, int discountPercent) {
        super();
        this.product = product;     
        this.discountPercent = discountPercent;
    }

    @Override
    public String toString() {
        return "Discount [product=" + product + ", discountPercent=" + discountPercent + "]";
    }
}

Offer

public class Offer {

    private Product product;
    private int quantity;   
    private Discount discount;

    public Product getProduct() {
        return product;
    }
    public void setProduct(Product product) {
        this.product = product;
    }   

    public int getQuantity() {
        return quantity;
    }
    public void setQuantity(int quantity) {
        this.quantity = quantity;
    }
    public Discount getDiscount() {
        return discount;
    }
    public void setDiscount(Discount discount) {
        this.discount = discount;
    }

    public Offer(Product product, int quantity, Discount discount) {
        super();
        this.product = product;
        this.quantity = quantity;
        this.discount = discount;
    }
    @Override
    public String toString() {
        return "Offer [product=" + product + ", quantity=" + quantity + ", discount=" + discount + "]";
    }

}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mathieu Guindon Oct 19 '16 at 20:23
  • \$\begingroup\$ @Mat'sMug how can I add extra code. I have seen a note that said edit post and not add comment \$\endgroup\$ – Kris Swat Oct 19 '16 at 20:32
  • \$\begingroup\$ If you want an iterative review, you should make a new post with the revised code, not edit your original question to include answers' feedback. \$\endgroup\$ – Mathieu Guindon Oct 19 '16 at 20:34
3
\$\begingroup\$

You should have a class PriceComputer that computes prices and their discounts. This should be the central class of the whole design.

Then, make separate data types for the input and output of that class. That way the compiler prevents you from mixing these during the calculations (which will get quite complicated once you add more rules).

The input just needs to be the basket that you already have. You should change the HashMap to a LinkedHashMap, to keep the products in the same order they were inserted.

Each output element should have the properties product, quantity, resultingPrice, discount, discountPercent (only if applicable). All its fields should be final, since once computed, there is no reason to change them.

The general structure of your classes looks good. You might have a look at Project Lombok (which will save you from writing boring code), and you should make clear what the class Offer stands for, since it wasn't clear to me from the name alone.

Also, given an Offer, there are two ways of reaching a Product (offer.product and offer.discount.product). Are these products always the same? If so, one of them must be removed from the code.

Product.equals should not depend on the name of the product. Instead, only the SKU should be compared. Or, even simpler, make sure that you only have a single Product object per product, and don't override hashCode/equals at all.

\$\endgroup\$
  • \$\begingroup\$ Thanks @roland I think I should give a proper name for Offer. In essence it means all offers available from the store. Offer contains product +.qty to be bought. + discount [this inturn contains product]. The reason is - If user buys "offer.product" which is book and quantity =2,the discount is on offer.discount.product - which is pen. \$\endgroup\$ – Kris Swat Oct 19 '16 at 20:36
  • \$\begingroup\$ When this is the only type of offer you will ever have, the name is ok. And after more thorough reading I figured out what its fields meant. Be careful from the beginning with naming things. If you can, explain your design to non-programmers to see if they understand the structure. If they do, you're probably fine. \$\endgroup\$ – Roland Illig Oct 19 '16 at 20:41
  • \$\begingroup\$ It is only 1, it is a list. Another offer can be like 10% discount on Horlicks, in which case offer product and discount product are the same \$\endgroup\$ – Kris Swat Oct 19 '16 at 20:46
0
\$\begingroup\$

I can't add anything more on previous answer but I can point out to you what you have to be aware when designing simple ecommerce solutions:

  • Basket/Cart is unpaid Order
    • They should have same abstract superclass
  • You should have class Sku that is real selling product (with all info), and keep product as base
    • This is solving variant problem (same product with options), specific product fields etc...
  • Two main workflows/process
    • Checkout - billing/delivery address, delivery, payment
    • Order management - fraud check, payment completion, product reservation (if you have supplier), warehouse
  • Category system

  • Pricing should be its own service/project and it's much more than calculating discounts - there's taxes, credit card bank fees and discounts, user groups, etc. It's called from various places:

    • Product details
    • Re/calculating cart total amount
    • Payment

Basically, you have to have a goal eg. one country, one tax rate, two kinds of products, such as smartphones and digital products, so and two kind of delivery, discounts per product and product categories and none on user groups (e.g. employees).

With that information, you can much more easily design your system, start on strong ground and foresee problems that one day may come up.

\$\endgroup\$
  • \$\begingroup\$ what is SKU? Also i could not understand why abstract super class is required for Basket. My key question was whether to calculate discounts, should I iterate my basket or iterate list of offers? because basket is hashmap, the logic became wierd when using forEach \$\endgroup\$ – Kris Swat Oct 21 '16 at 22:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.