Open/Closed Principle with calculating the total cost of items in a shopping cart 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&lt;OrderItem&gt; items;

    public Cart() {
        items = new ArrayList&lt;&gt;();
    }

    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&lt;IPriceRule&gt; priceRules;

    public PaymentCalculator() {
        priceRules = new ArrayList&lt;&gt;();
        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&lt;OrderItem&gt; items;
    IPaymentCalculator paymentCalculator;

    public Cart() {
        items = new ArrayList&lt;&gt;();
        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;
    }
}
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.