Refactor Switch to a One-Liner

Intro

In this article, we’ll take a look at why switch is almost never a good idea in OOP. We are going to look from different perspectives at how it affects the code, tests, and maintainability. Afterwards, we’ll do some refactoring on our existing code and analyse the benefits of the newly refactored code. We’ll see how almost any (no matter how long) switch can and should be converted to an elegant one-liner. All the source code used in this article can be found here, which has two branches — the un-refactored code (main) and the result after refactoring (feature/refactor-switch).

switch Is Almost Never a Good Idea

How many times have you come across some code that you need to add some changes to or modify something, and you do this?

The answer for me is way too many! During my career, I’ve encountered a fair amount of code that resembles that example. But what is wrong with that code? Why does it trigger this reaction in so many of us? The answer is simple: switch. I won’t say that switch is always a bad idea, because there are use cases when it might be appropriate, but I will focus on the disadvantages and cons of having a switch in most of the other cases. 

So let’s dive into that:

• switch is almost never a good idea in OOP. It is considered a code smell since, with time, it enforces long methods, long classes, and duplicated code. Martin Fowler says, “Complex conditional logic is one of the hardest things to reason about in programming, so I always look for ways to add structure to conditional logic. ”  
• R.C. Martin has a few good points regarding switch, too: “It’s hard to make a small switch statement. Even a switch statement with only two cases is larger than I’d like a single block or function to be. It’s also hard to make a switch statement that does one thing. By their nature, switch statements always do N things.”
• switch statements are a clear violation of the OCP (Open-Closed Principle) because the class/method hosting it must always change in order to adapt to new types (enum in most cases).
• A switch statement is a direct violation of the SRP (Single Responsibility Principle) because it forces the class to have more than one reason to change, and, clearly, we have N reasons to change, even with a small switch.
• There are rare cases when a switch statement will stay the same as the first time it was written, but it usually tends to grow uncontrollably, either in the number of cases or, if the task requires it, it will grow per case with various new calls or if/else statements. 
• From a testing perspective, it is fairly hard to unit test all the scenarios of every case, particularly if it has other conditionals. More than that, it will bloat the test class of the class hosting it, making it hard to keep track of what is happening. Sometimes, people might come to solutions like using loops or conditionals in tests to avoid creating too many test cases, which again is a smell (http://xunitpatterns.com/Conditional%20Test%20Logic.html) and brings lots of other disadvantages on its own.
• switch statements are a good example of The Broken Window Theory discussed by Andy Hunt and Dave Thomas in their book The Pragmatic Programmer. If the switch statement is not refactored at the right moment, it will encourage other developers to contribute to its growth until it becomes a much bigger problem.
• Having big switch statements with lots of cases and other conditionals inside it will increase the cognitive complexity (cyclomatic complexity), making the code harder to understand and maintain. See what SonarSource has to say about it: https://www.sonarsource.com/resources/cognitive-complexity/ 
• Sometimes,  in order to get rid of Sonar warnings or to have better readability, developers extract lots of private methods in order to have smaller cases. Then, it is hard to keep track of what is happening and what needs to be tested. Kent Beck has a really good point on that in my opinion: “I only test public methods. If a private method is complex enough to need testing, it generally needs its own object.” So not in all cases, but it might come to that point when, for the sake of switch's readability, a lot of logic gets buried in private methods that will need testing.
• We can even think of switch in terms of cohesion and coupling. Our method is focused on way too many things to do and is strongly coupled to the existing cases, and we always drive towards low coupling and high cohesion, right?

OK, What Now?

So enough talk. With the previously presented arguments, it is clear that we need to do something. What should we do? The answer again is simple. Refactor it.

The most obvious thing people usually do is extract the switch to a private method, or they extract the switch cases to a private method, but that doesn’t really fix our problems because we have the switch and all its disadvantages. We can even go and extract a class with a public method responsible for hosting the entire switch logic, but that still leaves us with the switch.

Both Uncle Bob and Martin Fowler mentioned the use of polymorphic behaviour instead of switch. Usually, the behaviour of switch can be easily buried down into a factory responsible for picking the right subclass hosting the case behaviour. This is better, but we still have the switch with most of its disadvantages in place. In my opinion, that is an amazing solution, but to fully get rid of the switch, we can go even further and delegate the switching logic to the subclasses/implementors of an interface. So our factory won’t need to host the switch by case logic, it will simply iterate through all the implementations/subclasses and pick the right task for the job based on the information available in the implementation. With such an approach, we can obliterate the switch from our code.

Coding Time

With the solution in mind, let’s get our hands into refactoring. For this purpose, I created a playground project that will allow us to see the refactoring of a switch into a one-liner. The project is quite simple — it is about a company that has some products for sale, clients with card info, and is able to process “payments” for those products. 

Spoiler alert:  By no means this project is a real example of how payments are done, processed, or things like that, it was created solely for the sake of refactoring demonstration. Lots of things could’ve been implemented better or somewhere else or in another manner. So keep in mind, it is an example that resembles real production code that I’ve seen during my career in various projects.

Prerequisites for refactoring: 

1. You need unit tests. 
2. If the method you are trying to refactor is not unit tested, checkpoint 1.
3. Acceptance of the idea that you’ll have to write more code than it is there.
4. Professional attitude towards the code.

Now that everything is clear. Our entire magic will happen here in PaymentServiceImpl.java

There we have our switch, in the method payforProduct 

          switch (client.getCard().getType()) {
              case REWARDS -> {
                  updatedPrice = price + (price * REWARDS_COMMISSION);
                  log.info("Applied {} commission for client {} with card {} and price now = {} for product id {}",
                          REWARDS_COMMISSION, client.getId(), client.getCard().getType(), updatedPrice, productId);
                  if (client.isRewardEligible()) {
                      updatedPrice = price - (price * REWARDS_DISCOUNT);
                      log.info("Applied {} discount for client {} with card {} and price now = {} for product id {}",
                              REWARDS_DISCOUNT, client.getId(), client.getCard().getType(), updatedPrice, productId);
                  }
              }
              case SECURED -> {
                  updatedPrice = price + (price * SECURED_COMMISSION);
                  log.info("Applied {} commission for client {} with card {} and price now = {} for product id {}",
                          SECURED_COMMISSION, client.getId(), client.getCard().getType(), updatedPrice, productId);
              }
              case LOW_INTEREST -> {
                  updatedPrice = price + (price * LOW_INTEREST_COMMISSION);
                  log.info("Applied {} commission for client {} with card {} and price now = {} for product id {}",
                          LOW_INTEREST_COMMISSION, client.getId(), client.getCard().getType(), updatedPrice, productId);
              }
              case CASHBACK -> {
                  updatedPrice = price + (price * CASHBACK_COMMISSION);
                  log.info("Applied {} commission for client {} with card {} and price now = {} for product id {}",
                          CASHBACK_COMMISSION, client.getId(), client.getCard().getType(), updatedPrice, productId);
                  if (client.getCard().isCashbackEnabled()) {
                      updatedPrice = price - (price * CASHBACK_DISCOUNT);
                      log.info("Applied {} discount for client {} with card {} and price now = {} for product id {}",
                              CASHBACK_DISCOUNT, client.getId(), client.getCard().getType(), updatedPrice, productId);
                  }
              }
              case STUDENT -> {
                  if (client.getAge() >= 18 && client.getAge() <= 21) {
                      updatedPrice = price + (price * STUDENT_SCHOOL_COMMISSION);
                      log.info("Applied {} commission for client {} with card {} and price now = {} for product id {}",
                              STUDENT_SCHOOL_COMMISSION, client.getId(), client.getCard().getType(), updatedPrice, productId);
                  } else if (client.getAge() > 21 && client.getAge() <= 23) {
                      updatedPrice = price + (price * STUDENT_UNIVERSITY_COMMISSION);
                      log.info("Applied {} commission for client {} with card {} and price now = {} for product id {}",
                              STUDENT_UNIVERSITY_COMMISSION, client.getId(), client.getCard().getType(), updatedPrice, productId);
                  } else {
                      updatedPrice = price + (price * DEFAULT_COMMISSION);
                      log.info("Applied {} commission for client {} with card {} and price now = {} for product id {}",
                              DEFAULT_COMMISSION, client.getId(), client.getCard().getType(), updatedPrice, productId);
                  }
              }
              case TRAVEL -> {
                  if (product.getCategory() == Category.TRAVEL) {
                      updatedPrice = price + (price * TRAVEL_COMMISSION);
                      log.info("Applied {} commission for client {} with card {} and price now = {} for product id {}",
                              TRAVEL_COMMISSION, client.getId(), client.getCard().getType(), updatedPrice, productId);
                  } else {
                      updatedPrice = price + (price * DEFAULT_COMMISSION);
                      log.info("Applied {} commission for client {} with card {} and price now = {} for product id {}",
                              DEFAULT_COMMISSION, client.getId(), client.getCard().getType(), updatedPrice, productId);
                  }
              }
              case BUSINESS -> {
                  updatedPrice = price + (price * BUSINESS_COMMISSION) + (price * BUSINESS_TAX);
                  log.info("Applied {} commission and tax {} for client {} with card {} and price now = {} for product id {}",
                          BUSINESS_COMMISSION, BUSINESS_TAX, client.getId(), client.getCard().getType(), updatedPrice, productId);
              }
              default -> {
                  updatedPrice = price + (price * DEFAULT_COMMISSION);
                  log.info("Applied default commission {} for client {} with card {} and price now = {} for product id {}",
                          DEFAULT_COMMISSION, client.getId(), client.getCard().getType(), updatedPrice, productId);
              }
          }

The switch in this case is responsible for updating the price of a product before payment with the applied commission based on the card type. There are certain scenarios when a commission depends on the product category or when based on the card type a certain discount might be applied in order to offer some cash-back and things like that.

Note: This method is unit tested and you can run the tests from the file PaymentServiceImplTest.java to get a better grasp of what is happening. Also, you can get a better view of what I meant when I said that switch statements tend to bloat tests with lots of little tests covering each scenario for every case.

So the first step would be either to introduce a class hierarchy or a contract for the logic that happens inside of each case. We’ll go with the interface-/implementation-based approach in this case, since we are talking only about behaviour. For this reason, we are going to create an interface named CommissionApplierStrategy 

public interface CommissionApplierStrategy {
    double applyCommission(Client client, Product product);
}

So this interface has only a single method, which will be responsible for applying the commission based on the client’s info and product and then return the updated price for further processing. But wait, before we proceed further, what about our factory? How is it going to decide which implementation to choose? A switch again? No, we decided that we aren’t going with a switch. Instead of having our factory decide which implementation to choose with a switch, we are going to let our implementations tell us what they can do and let the factory simply pick the right implementation for the job. For this, we are going to force our implementations to implement another method

public interface CommissionApplierStrategy {
    double applyCommission(Client client, Product product);
    CardType getCardType();
}

Now we have the method CardType getCardType(), which will force our implementations to tell us what card they can apply a commission to.

With this interface in place, everything gets easier from this point. All that is left to do is to move logic from each case in a particular implementation. I am going to show you only one implementation. You can view the rest of them in the inc.evil.refactorswitch.service.commission package. So let’s say we pick our first case, which is responsible for applying commissions for the Rewards card type. This is what our implementation would look like:

@Slf4j
@Service
public class RewardsCommissionApplierStrategyImpl implements CommissionApplierStrategy {
    private static final double REWARDS_COMMISSION = 0.016;
    private static final double REWARDS_DISCOUNT = 0.007;

    @Override
    public double applyCommission(Client client, Product product) {
        double price = product.getPrice();
        Long productId = product.getId();
        double updatedPrice = price + (price * REWARDS_COMMISSION);
        log.info("Applied {} commission for client {} with card {} and price now = {} for product id {}",
                REWARDS_COMMISSION, client.getId(), client.getCard().getType(), updatedPrice, productId);
        if (client.isRewardEligible()) {
            updatedPrice = price - (price * REWARDS_DISCOUNT);
            log.info("Applied {} discount for client {} with card {} and price now = {} for product id {}",
                    REWARDS_DISCOUNT, client.getId(), client.getCard().getType(), updatedPrice, productId);
        }
        return updatedPrice;
    }

    @Override
    public CardType getCardType() {
        return CardType.REWARDS;
    }
}

Now our class does one thing and does it well; it applies the commission for Rewards card type. Our code can now be easily tested in isolation and grow independently as the features in future requests. Also note that our class PaymentServiceImpl.java, aside from the fact that it was bloated with a bunch of cases in the switch, was full of constants particular to each type of commission. Now every constant used in a specific case can be safely moved to its corresponding implementation as we did in RewardsCommissionApplierStrategyImpl.

From this point, we’ll have to refactor each case to its corresponding implementation as we did with the first one. What about the default case? How should we handle this one? We could add a new enum in CardType that will stand for default processing, something like a SIMPLE card type. This would allow us to create another implementation that will cover the default case in the switch statement

Right, at this point, we have all of our logic delegated to specific implementations. What’s left to do is to create the factory/resolver. Having the method  CardType getCardType();, and a little bit of Spring Framework’s magic over DI, we can do something like this.

@Service
@RequiredArgsConstructor
class CommissionApplierStrategyResolverImpl implements CommissionApplierStrategyResolver {

    private final List<CommissionApplierStrategy> commissionApplierStrategies;
    private final DefaultCommissionApplierStrategyImpl defaultCommissionApplierStrategy;

    @Override
    public CommissionApplierStrategy getCommissionApplier(CardType cardType) {
        return commissionApplierStrategies.stream()
                .filter(s -> s.getCardType() == cardType)
                .findFirst()
                .orElse(defaultCommissionApplierStrategy);
    }
}

So in CommissionApplierStrategyResolverImpl there are mainly two things that are of major importance:

1. Dependency injection of strategy implementations via a list. So we all know how useful dependency injection is in Spring, we are all pretty accommodated with @Autowired on particular beans, but did you know that Spring can inject lists of beans or even maps? That is quite useful for our case, having all the implementations injected and ready to be queried.

   1. Not having Spring Framework by your side is not an issue, you can do the injection manually in the factory or use other DI frameworks like Google Guice.

2. Method CommissionApplierStrategy getCommissionApplier(CardType cardType) will return a specific implementation based on the provided CardTypestreaming through all of the implementations and asking who’s the right one to do the job. If no implementation is found, a default one will be provided. This can be even configured to throw an exception if this is not the behaviour that we are expecting. So this method together with Spring’s power acts like a switch, except that we are free of most of the cons switch drags with it.

 Now having the resolver implemented and all the implementations per case, our method payForProduct in PaymentServiceImpl becomes this:

@Override
    public void payForProduct(Long productId, Client client) {
        Product product = productRepository.findById(productId)
                .orElseThrow(() -> new ProductNotFoundException(String.format("Product with id %s not found", productId)));

        double updatedPrice = commissionApplierResolver.getCommissionApplier(client.getCard().getType()).applyCommission(client, product);

        PaymentResponse paymentResponse = paymentClient.debitCard(client.getCard(), updatedPrice);
        if (paymentResponse.isSuccess()) {
            product.setStatus(ProductStatus.PAID);
            //process delivery, etc
            log.info("Product {} paid with success", productId);
        } else {
            //alert the user & other business logic
            log.warn("Product {} payment failed with [{}]", productId, paymentResponse.getErrorMessage());
            throw new ProductPaymentException(paymentResponse.getErrorMessage());
        }

Our entire switch gets reduced to an elegant one-liner. Lovely, isn’t it?

Conclusion

In the end, after refactoring, let’s zoom out and take a look at what we’ve done:

• We reduced a really big block of code to one line, which most probably got us rid of long method and long class
• We got rid of Sonar warnings
• We made justice both for OCP (adding another case is a matter of implementing the interface - no one has to suffer) and SRP (each class/method has its own responsibility)
• We decoupled our code and made it more cohesive
• Our new implementations are Easy To Change (ETC) and follow ORTHOGONALITY principle
• No more Theory of Broken Window
• Each new implementation has its own suite of tests deflating the PaymentServiceImpl’s test suite.
• We have no big or ugly private methods in our code   

Bonus!

If you’ve taken a look at the unit tests you might’ve noticed that I use a peculiar or rather unusual name for each test. That in my opinion is the best way to name a test and I borrowed this approach from the author of “Art of Unit Testing” - Roy Osherove. He has some really strong points why test naming matters here https://osherove.com/blog/2005/4/3/naming-standards-for-unit-tests.html.