6

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);
}

enter image description here

11
  • 5
    The knowledge which product type corresponds to which discount policy should not be coded into the checkout routine. You should represent this either in the product class (a product understands its own discount properties) or in a separate discount managing module (which understands both product and discount types). Sep 12, 2022 at 8:05
  • I took the freedom to improve the wording of this question. Please double check if I got your intentions right.
    – Doc Brown
    Sep 12, 2022 at 8:10
  • 4
    In most reasonable real-world systems, I would expect the business people to be able to change the discount strategy, the loyalty points strategy, and the list of available products with their product codes, without asking a programmer for each new product. That, however, would require a more general approach. So please clarify: what is the expectation of the business here in your case? Or is this just a learning example for you?
    – Doc Brown
    Sep 12, 2022 at 8:21
  • 1
    @Abner we (almost everyone here) wonder if the refactoring is only limited to ShoppingCart (something I deem insufficient) or can be extended to Product.
    – Laiv
    Sep 12, 2022 at 13:36
  • 1
    @Laiv: moving those "if"s from one side to another can be meaningless or meaningful, depending on what those "sides" are and if there are different organizational units responsible for the "one side" or "the other". That is the whole point of my answer. Unfortunately, the OP did not give us any clue so far what the organizational units are in their case, and against whose "changes" the code should be closed, and for whose extensions it should be opened. Because of the missing response, I suspect they are just trying to treat the OCP as some braindead "best practice" without really knowing why.
    – Doc Brown
    Sep 15, 2022 at 18:35

5 Answers 5

8

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.

3

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.

1
  • 5
    I suspect the OP wants adding of new products not even require the modification of code in the Product class, but simply adding a new product record in some database. But without getting more context from the OP, it is hard to tell.
    – Doc Brown
    Sep 12, 2022 at 10:23
0

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

0

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.

0

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);

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