Shopping Cart design interview task

Question

I was interviewed for one position and had the following task.

The goal is to design system according step 1, then adapt it to additional requirements in step 2.

Step 1: Shopping cart

• You are building a checkout system for a shop which only sells apples and oranges.
• Apples cost 60p and oranges cost 25p.
• Build a checkout system which takes a list of items scanned at the till and outputs the total cost
• For example: [ Apple, Apple, Orange, Apple ] => £2.05
• Make reasonable assumptions about the inputs to your solution; for example, many candidates take a list of strings as input

Step 2: Simple offers

The shop decides to introduce two new offers:

• buy one, get one free on Apples
• 3 for the price of 2 on Oranges

Update your checkout functions accordingly

Please find my implementation below and let me know about weak points and what would you do differently or improve.

Checkout.java

import java.util.*;

public class Checkout {

private Set<CheckoutStrategy> strategies = new HashSet<>();

public static Map<String, Double> PRICE_MAP = new HashMap<>();

static {
    PRICE_MAP.put("Orange", 0.25);
    PRICE_MAP.put("Apple", 0.6);
}

public Checkout() {
    this.strategies.add(new SummingStrategy());
}

public Checkout(CheckoutStrategy discountStrategy) {
    this();
    this.strategies.add(discountStrategy);
}

public double calculateTotal(List<String> strings) {
    double[] result = new double[1];

    for (CheckoutStrategy strategy : strategies) {
        strategy.calculateTotal(strings, result);
    }

    return result[0];
}


}

CheckoutStrategy.java

import java.util.List;

public interface CheckoutStrategy {
    void calculateTotal(List<String> strings, double[] result);
}

SummingStrategy.java

import java.util.List;
import static Checkout.PRICE_MAP;

public class SummingStrategy implements CheckoutStrategy {

@Override
public void calculateTotal(List<String> items, double[] result) {
    result[0] = items.stream().mapToDouble(PRICE_MAP::get).sum();
  }

}

DiscountStrategy.java

import Checkout;
import java.util.List;

public class DiscountStrategy implements CheckoutStrategy {

@Override
public void calculateTotal(List<String> items, double[] result) {
    ItemCounter counter = items.stream().collect(ItemCounter::new, ItemCounter::accept, ItemCounter::combine);

    double appleDiscount = (counter.counts[ItemCounter.APPLE_INDEX] / 2) * Checkout.PRICE_MAP.get("Apple");
    double orangeDiscount = (counter.counts[ItemCounter.ORANGE_INDEX] / 3) * Checkout.PRICE_MAP.get("Orange");

    result[0] -= orangeDiscount;
    result[0] -= appleDiscount;
}

class ItemCounter {
    static final int APPLE_INDEX = 0;
    static final int ORANGE_INDEX = 1;
    private int[] counts = new int[2];

    public void accept(String item) {
        switch (item) {
            case "Apple":
                counts[APPLE_INDEX]++;
                break;

            case "Orange":
                counts[ORANGE_INDEX]++;
                break;
        }
    }

    public void combine(ItemCounter other) {
        this.counts[APPLE_INDEX] += other.getCounts()[APPLE_INDEX];
        this.counts[APPLE_INDEX] += other.getCounts()[ORANGE_INDEX];
    }

    public int[] getCounts() {
        return counts;
    }
    }

 }

Answer 1

I won't add to @mtj's comment (I totally agree that an array defined as a means to exchange information is a code smell).

I agree with @RobAu that you should stay far a away from those magic numbers and arrays. That would avoid bugs like:

this.counts[APPLE_INDEX] += other.getCounts()[ORANGE_INDEX];

Now to my comments.

---

Strategy is not the name of the Pattern applied here. It is more like a Composite Pattern, because several Strategies can be applied at the same time (although no method is available to add further discounts, but I suppose it was the intent).

In general I don't advise naming Objects by the Design Pattern they implement. Objects should be named as per what they do, not how you thought about making an architecture around them. It's useful in a tutorial, not in actual code. Carrying XXXStrategy everywhere gets real annoying, and lead to confusions (like a WallObserverListener : does it listen to an observer, or observe a listener?).

The solution to keep people to the page about you using a particular Pattern is Javadoc. This allows for cleaner code with method names that are more business-oriented, like:

public void apply(Discount discount);

Which is more obvious than:

public void addStrategy(CheckoutStrategy strategy);

---

A Better way to combine deals

You could use a Decorator pattern for your deals.

First clean up your interface:

public interface Checkout {
    double calculateTotal(List<String> shoppingCart);
}

Then make a simple price calculator (Note there is no hard-coded pricing, it can be created at runtime:

public class SimpleCheckout implements Checkout {

    private final Map<String, Double> basePrice = new HashMap<>();

    public void addPrice(String item, double price) {
        basePrice.put(item, price);
    }

    @Override
    public double calculateTotal(List<String> shoppingCart) {
        // Your own code, very good use of Streams, here
        return shoppingCart.stream().mapToDouble(basePrice::get).sum();
    }
}

Now we simply need to add discounts as a decoration layer:

public class DiscountValue implements Checkout {

    private final Checkout baseCheckout;
    private final String discountedItem;
    private final int minimumAmount;
    private final double discountValue;

    public DiscountValue(Checkout base, String item, int min, double discount) {
         this.baseCheckout = base;
         this.discountedItem = item;
         this.minimumAmount = min;
         this.discountValue = discount;
    }

    @Override
    public double calculateTotal(List<String> shoppingCart) {
        int count = Collections.frequency(shoppingCart, discountedItem);
        // Integer division gives the number of times the discount is applied
        double deduction = (count / minimumAmount) * discountValue;
        return baseCheckout.calculateTotal(shoppingCart) - deduction;
    }
}

Now this is how you use it:

public static void main(String[] argc){
    Checkout pricing = new SimpleCheckout();
    pricing.addPrice("Apple", 0.25);
    pricing.addPrice("Orange", 0.6);
    pricing = new DiscountValue(pricing, "Apple", 2, 0.25); // I'm using a straight-up refund here
    pricing = new DiscountValue(pricing, "Orange", 3, 0.60); // I'm using a straight-up refund here
    List<String> shoppingCart = new ArrayList<>();
    shoppingCart.add("Apple");
    shoppingCart.add("Orange");
    shoppingCart.add("Apple");
    shoppingCart.add("Apple");
    System.out.prinln(pricing.calculateTotal(shoppingCart));
}

Answer 2

Three points which immediately strike out to me:

1. Mutable double array in the interface. Yuck! This is unclear, hard to read and generally leaves a bad feeling. As you only use it to calculate a single value, try to refactor this to method return values and somehow combine the results.

2. A single discount strategy. While this may solve the problem posted in part 2, you don't estimate the future development here. What if the shop wants to keep the discount on apples but cancel the discount on oranges? They'll have to call the software guy again. This should be split into single strategies to make them individually pluggable.

3. Global price map: while it might be true that this is a singleton in your application, you create an unnecessary dependency from everywhere to the concrete implementation. Better: pass a map to the strategies (maybe as a constructor parameter) to honor the tell-don't-ask principle.

Answer 3

To add to mjt's answer:

4. You never introduce the very sensible object Product (or SKU if you like). Instead, you use String and indices to encode them. Having a class of its own makes it all much more readable.

5. I would skip the magic index values and magic numbers in the ItemCounter. Why not just use a Map<Product, Integer>? This makes the switch() statement (which is very annoying if you add another Product) completely go away as well.
   This also prevents bugs like adding oranges to apples: this.counts[APPLE_INDEX] += other.getCounts()[ORANGE_INDEX];)