How to refactor this code to obey the ‘open-closed’ principle?

Question

The UML is listed below. There are different products with different preferential strategies. After adding these products into the shopping cart, the caller needs to call the checkout() method to calculate the totalPrice and loyaltyPoints according to its specific preferential strategy. But when a new product comes with a new preferential strategy, I need to add another else if. This currently breaks the open-closed principle.

class ShopppingCart {
// ...
    public Order checkout() {
        double totalPrice = 0;
        Map<String,Integer> buy2Plus1=new HashMap<>();
        int loyaltyPointsEarned = 0;

        for (Product product : products) {
            double discount = 0;
            if (product.getProductCode().startsWith("DIS_10")) {
                discount = (product.getPrice() * 0.1);
                loyaltyPointsEarned += (product.getPrice() / 10);
            } else if (product.getProductCode().startsWith("DIS_15")) {
                discount = (product.getPrice() * 0.15);
                loyaltyPointsEarned += (product.getPrice() / 15);
            } else if(product.getProductCode().startsWith("DIS_20")){
                discount=(product.getPrice()*0.2);
                loyaltyPointsEarned+=(product.getPrice()/20);
           }else if(product.getProductCode().startsWith("Buy2Plus1")){
            if(buy2Plus1.containsKey(product.getProductCode())){
                buy2Plus1.put(product.getProductCode(), buy2Plus1.get(product.getProductCode())+1);
            }
            else{
                buy2Plus1.put(product.getProductCode(),1);
            }
            if(buy2Plus1.get(product.getProductCode())%3==0){
                discount+=product.getPrice();
                continue;
            }
            loyaltyPointsEarned+=(product.getPrice()/5);
        }else {
            loyaltyPointsEarned += (product.getPrice() / 5);
        }
        totalPrice += product.getPrice() - discount;
    }

    return new Order(totalPrice, loyaltyPointsEarned);
}

Answer 1

The OCP is about allowing "team 1" to provide a black-box framework containing classes like Product, Order, and ShoppingCart, and "team 2" to change the list of products and the discount strategy without asking team 1 for a change in their code.

There are usually three major situations to consider:

1. "Team 2" is a second development team. "Team 1" has to provide "injection points" for team 2 where they can provide new discount strategies along with new products.

2. "Team 2" are the business people. They don't want to ask the programmers, but simply add new products and discount calculation rules at run time through some nice GUI or configuration files.

3. Team 1 and 2 are identical (there is no "team 2"). So whenever the business people add a new product or product code which does not fit to the current discount logic, they ask the devs of team 1 to implement this, for each and every minor change.

For case 1, there are several options. The goal is to allow team 2 to provide new discount rules without changing the original ShoppingCart or Product code. Team 1 may provide an abstract interface PriceCalculator, let the the checkout method take an object of that type and let team 2 pass a concrete PriceCalculator object as a parameter. Team 1 may let team 2 add new product codes into some list or database table. Alternatively, one could also decide to let team 2 provide a list of DiscountStrategy objects as a parameter to checkout. Each of those DiscountStrategy objects could have a method Discount calcDiscount(Product), which returns null in case the strategy does not apply, or a discount object (with the discount and loyalty points) in case it applies. Then, the checkout method can simply iterate over the DiscountStrategy objects and stop when the first call to calcDiscount(Product) returns something different from null.

For case 2, one needs to parametrize all kinds of discount calculation rules, store that parameters in some database or file and and implement a generic PriceCalculator module which can evaluate that parameters (along with some UI for the business people to change them). In your contrived example, this looks pretty simple: introduce a parameter object with three attributes ProductCodePattern, DiscountFactor and LoyaltyPointsFactor. However, reality, discount strategies will often be more complex, requiring different formulas for different product codes.

In case there is more flexibility required, the PriceCalculator could be some small "DSL" (domain specific language)" interpreter, and the business people specify their discount rules using that DSL.

For case 3, applying the OCP is probably overdesign and not necessary. Nethertheless it may be a good idea to extract the price and discount calculation from the shopping card into some PriceCalculator class, for increased testability. Since the discount calculation depends mainly on the product, it probably fits better into the Product class itself. But beware, inheriting from the Product class and providing a different discount calculation rule for each subclass is most probably the wrong approach here.

So in short, for applying the OCP most sensibly, one needs to know the organizational context. The OCP is not an end in itself, you need to know which parts of your code should be open for changes to whom, and which should be closed to whom, and for what purpose.

Answer 2

Let's forget the "Open-Closed Principle" for one second and concentrate on what you actually want. As far as I can tell, you have an implicit "non-functional" requirement, that adding a Product should be fast and not require modification of other code.

Ok, that "obviously" means that code that you would add to the checkout should be in the Product. This would mean the Product should take a little bit of "responsibility" instead of being just data.

To do this, you'll need to come up with proper behavior of the Product, maybe a checkout method in the Product itself? This would work unless you have policies which apply to the cart itself and not to individual products.

Answer 3

Most onlineshops have a seperate PriceCalculationService that calculates the prices depending on orderQuantity (quantity-discounts), current stock-level, voucher-codes, .... .

This obeys the open-closed-principle: If you change the price calculation strategy there is no more need to change the ShopppingCart implementation

Answer 4

The only variable here is the discount. So if you factor that out by having some discount provider/selector class, that hopefully gets values from some outside maintainable data store, then this class should not change again because of it.

Answer 5

In addition to not being consistent with the "Open Closed Principle", Order has feature envy, knowing too much about a product.

To address both from an Order's point of view, move create Product with discount and loyaltyPointsEarned methods. As a result, Order is now closed to Product changes.

class ShopppingCart {
// ...
public Order checkout() {
    Map<String,Integer> buy2Plus1=new HashMap<>();
    int loyaltyPointsEarned = 0;

    for (Product product : products) {
        double discount = 0;

        totalPrice += product.getPrice() - product.getDiscount(buy2Plus1);
        loyaltyPointsEarned += product.loyaltyPointsEarned(buy2Plus1);
     
     }

     return new Order(totalPrice, loyaltyPointsEarned);
}

And then Product would be instantiated as

Product product = new Product(“DIS_10”, .1, 10);