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

Question

The UML is down here. There are different products with different preferential strategy.
When adding these products into the shoppingcart, it needs to call the 'checkout()'method to calcute 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'.
I think it breaks the open-closed principle. So can you give me some advice on how to refactor the code?

 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

I would use factory pattern here. First define an interface:

public interface PreferentialStrategy {

  String strategyCode();
  StrategyResult applyStrategy(Product product);
}

StrategyResult is simple pojo holding discount and loyalty points.

Each strategy will be a class implementing PreferentialStrategy:

public class Dis10Strategy implements PreferentialStrategy {

  @Override
  public String strategyCode() {
    return "DIS_10";
  }

  @Override
  public StrategyResult applyStrategy(Product product) {
    double discount = product.getPrice() * 0.1;
    int points = (int) product.getPrice() / 10;
    return new StrategyResult(discount, points);
  }
}

Same for dis_15, dis_20 and so on.

The factory:

public class StrategyFactory {

  private final List<PreferentialStrategy> strategies;
  private final PreferentialStrategy defaultStrategy = new NoPrefStrategy();

  public StrategyFactory(Collection<PreferentialStrategy> strategies) {
    this.strategies = new ArrayList<>(strategies);
  }
  
  public void registerPreferentialStrategy(PreferentialStrategy strategy) {
    this.strategies.add(strategy);
  }

  public PreferentialStrategy getStrategy(Product product) {
    return this.strategies
            .stream()
            .filter(strategy -> product.getProductCode().startsWith(strategy.strategyCode()))
            .findFirst()
            .orElse(this.defaultStrategy);
  }

  private static final class NoPrefStrategy implements PreferentialStrategy {

    @Override
    public String strategyCode() {
      return "";
    }

    @Override
    public StrategyResult applyStrategy(Product product) {
      return new StrategyResult(0, 0);
    }
  }
}

It gets a strategy for the product, if there is none, return a default strategy without any preferences(0 discount and 0 points). Whenever you need to add new strategy, just register it in the factory.

As a side note, since you are dealing with money here, you should not use floating point numbers(they are imprecise), but BigDecimal instead.

Example usage:

public class TempSecond {

  public static void main(String[] args) {
    List<Product> products = Arrays.asList(new Product(1000, "DIS_10", "Prod1"),
            new Product(1000, "DIS_15_asd", "Prod2"),
            new Product(1100, "qwerty", "Prod3"));
    StrategyFactory factory = new StrategyFactory(Arrays.asList(new Dis10Strategy(), new Dis15Strategy()));
    int loyaltyPointsEarned = 0;
    for (Product product : products) {
      PreferentialStrategy preferentialStrategy = factory.getStrategy(product);
      StrategyResult result = preferentialStrategy.applyStrategy(product);
      double discount = result.getDiscount();
      loyaltyPointsEarned += result.getLoyaltyPoints();
    }
  }
}

Answer 2

One approach could be to refactor your Product class into a Product interface with calculateDiscount() and calculateLoyaltyPoints() methods and then create different implementations for different product types. That way, each new type will have to implement its apropriate strategy and your business logic would no longer need to care at all.

E.g.

interface Product {
    double calculateDiscount();
    int calculateLoyaltyPoints();
}

class Dis10Product implements Product {
    double calculateDiscount() {
        return price * 0.1;
    }
    int calculateLoyaltyPoints() {
        return price / 10;
    }
}

class Dis15Product implements Product {
    double calculateDiscount() {
        return price * 0.15;
    }
    int calculateLoyaltyPoints() {
        return price / 15;
    }
}

public Order checkout() {
    double totalPrice = 0;
    Map<String,Integer> buy2Plus1=new HashMap<>();
    int loyaltyPointsEarned = 0;
    for (Product product : products) {
        double discount = product.calculateDiscount();
        loyaltyPointsEarned += product.calculateLoyaltyPoints();
...

As a side note and unrelated to your actual question I suggest reconsidering your approach of using double for prices, cf. Is floating point math broken? There are better alternatives when dealing with monetary amounts.

Answer 3

Marvin’s answer solves the issue in an elegant manner using polymorphism.

But it has a hidden coupling: the product is coupled to the price strategy. As a consequence, depending on how you design the product hierarchy, many products might have to reimplement common strategies or it will be difficult to change the preferential strategy.

Therefore, I’d suggest to isolate the preference strategy using… the strategy pattern. You still have the choice of seing Product as an interface, or a base class of a product hierarchy depending on how much else is common between products.

Answer 4

I am late at the party, but when I started to write a reply, then there was not answers. Nevertheless, I decided to add my reply as it has some explanations that can be useful for future readers. So let's begin.

One of the design priciple says:

Encapsulate what varies

So we can encapsulate varyable behaviours in classes. And it will be what we call Strategy pattern:

Strategy pattern is a behavioral software design pattern that enables selecting an algorithm at runtime. Instead of implementing a single algorithm directly, code receives run-time instructions as to which in a family of algorithms to use.

Let me show an example via C#. I am sorry, I am not Java guy, but let me show via C#. I have not used any special features of C# and I provided comments how to convert this code to Java.

This is our abstraction or moe concretely, it is interface of our exchangeable strategies:

public interface IDiscount
{
    double Calculate(Product product);

    double CalculateLoyaltyPoints(Product product);
}

And its concrete implementations:

public class Percent10Discount : IDiscount // implements in Java
{
    public double Calculate(Product product)
    {
        throw new NotImplementedException();
    }

    public double CalculateLoyaltyPoints(Product product)
    {
        throw new NotImplementedException();
    }
}

public class Percent15Discount : IDiscount // implements in Java
{
    public double Calculate(Product product)
    {
        throw new NotImplementedException();
    }

    public double CalculateLoyaltyPoints(Product product)
    {
        throw new NotImplementedException();
    }
}

And Product class:

public class Product
{
    public string Code { get; set; }
    public double GetPrice() { return 1; }
}

Now, the client still has to decide directly which storage is passed to the public Order checkout(). So we need a place where all strategies can be stored. And we should be able to get necessary strategy from this store. So this is a place where simple factory can be used. Simple factory is not Factory method pattern and not Abstract factory.

public class DiscountFactory 
{
    // Java: Map<String,IDiscount> map = new HashMap<String, IDiscount>(){{
    Dictionary<string, IDiscount> _discountByProductCode = new 
        Dictionary<string, IDiscount>()
    {
        { "DIS_10", new Percent10Discount() },
        { "DIS_15", new Percent15Discount() },
    };

    public IDiscount GetInstanceByProductCode(string productCode) 
    {
        return _discountByProductCode[productCode];
    }
}

And then we can use it like this:

IEnumerable<Product> products = new List<Product>();
DiscountFactory discountFactory = new DiscountFactory();
double discount = 0;
double loyaltyPointsEarned= 0;
foreach (Product product in products)
{
    IDiscount discountStratey = discountFactory
        .GetInstanceByProductCode(product.Code);
    discount = discountStratey.Calculate(product);
    loyaltyPointsEarned += discountStratey.CalculateLoyaltyPoints(product);
}