Open/Closed Principle with calculating the total cost of items in a shopping cart

Question

I came across the below example in a tutorial for the open-closed OOP principle. This example demonstrates function of calculating the total cost of items in a shopping cart. I have provided code for both before and after refactoring to comply with the open-closed principle. Both versions are from the tutorial I followed.

The drawback with the old code is that we need to add additional if/else to support new SKUs thus violating "closed for modifications" part in the open-closed principle. But even in the new design, we need to add additional IPriceRule to support new SKUs. So in what way is the new design better than old design? Is this because with the new design it is little more flexible to add complex calculation algorithm?

I have left comments in relevant sections of the code where it violates the principle.

Before:

class OrderItem {
    string sku;
    int quantity;
}

class Cart {
    List<OrderItem> items;

    public Cart() {
        items = new ArrayList<>();
    }

    public void addItem(OrderItem item) {
        items.add(item);
    }

    int totalPrice() {
        int price = 0;

        for(OrderItem item : items) {
            if( item.sku == "foo") {
                price += item.quantity * 10;
            }
            else if (item.sku == "bar") {
                price += item.quantity * 5;
            }
            // This is the problematic part. We need to add more if/else
            // conditions to support different SKUs. Hence this violates 
            // closed for modifications part of open-closed. 
        }

        return price;
    }
}

After refactoring to comply with the Open-Closed principle:

interface IPriceRule {
    boolean isMatch(OrderItem item);
    int calculatePrice(OrderItem item);
}

class FooPriceRule implements IPriceRule {
    @Override
    boolean isMatch(OrderItem item) {
        return item.sku.equals("foo");
    }

    @Override
    int calculatePrice(OrderItem item) {
        return item.quantity * 10;
    }
}

class BarPriceRule implements IPriceRule {
    @Override
    boolean isMatch(OrderItem item) {
        return item.sku.equals("bar");
    }

    @Override
    int calculatePrice(OrderItem item) {
        return item.quantity * 5;
    }
}


interface IPaymentCalculator {
    int calculatePrice(OrderItem item);
}

class PaymentCalculator implements IPaymentCalculator {
    List<IPriceRule> priceRules;

    public PaymentCalculator() {
        priceRules = new ArrayList<>();
        priceRules.add(new FooPriceRule());
        priceRules.add(new BarPriceRule());
        // Add new rules here to support new SKUs. But why is this better
        // than adding if/else in the old design? Because even this class 
        // changes as we add more requirements right?
    }

    @Override
    int calculatePrice(OrderItem item) {
        for(IPriceRule priceRule : priceRules ) {
            if (priceRule.isMatch(item)) {
                return priceRule.calculatePrice(item);
            }
        }

        return 0;
    }
}

class Cart {
    List<OrderItem> items;
    IPaymentCalculator paymentCalculator;

    public Cart() {
        items = new ArrayList<>();
        paymentCalculator = new PaymentCalculator();
    }

    public void addItem(OrderItem item) {
        items.add(item);
    }

    int totalPrice() {
        int price = 0;

        for(OrderItem item : items) {
            price += paymentCalculator.calculatePrice(item);
        }

        return price;
    }
}

Answer 1

Basics

• String comparison shouldn't be done with ==, unless you can carefully ensure you're using String.intern() everywhere so that the reference comparison will always match appropriately (not to forget the documentation too).
• Keep your fields private!
• Should PaymentCalculator.calculatePrice(OrderItem) really return 0 if there are no matching rules? That's like saying I can get oranges for free for a payment calculator that only (accidentally?) handle apples...

Rule-based vs if statements

// Add new rules here to support new SKUs. But why is this better
// than adding if/else in the old design? Because even this class 
// changes as we add more requirements right?

You can think of it that the calculation logic is not baked into the payment calculator, and that the logic can be maintained separately. For example, a more complex IPriceRule implementation might be retrieving prices from a database, and the calculator does not need to deal with any database connections know-how.

Answer 2

I think that as it is, there is in fact not much difference. You could add more complex calculation or matching behavior to a given pricing rule, but in the absence of such there's little benefit. In the example as written, it's just more complexity for little gain.

However, consider that not every item needs a separate rule class, and that the rules could come from another source than being manually created. For example:

class SimplePriceRule implements IPriceRule {
    private string sku;
    private int pricePerItem;

    public SimplePriceRule(string sku, int pricePerItem) {
        this.sku = sku;
        this.pricePerItem = pricePerItem;
    }

    @Override
    boolean isMatch(OrderItem item) {
        return item.sku.equals(sku);
    }

    @Override
    int calculatePrice(OrderItem item) {
        return item.quantity * pricePerItem;
    }
}

Now imagine that your list of rules comes from reading a config file of sku/price info which is parsed into these. Even then, it doesn't need an interface until there's a second type of pricing, but the use of such adds relatively little complexity.