How can I improve readability and length of a method with many if statements?

Viewed 3411

I have a method with 195 ifs. Here is a shorter version:

private BigDecimal calculateTax(String country, BigDecimal amount) throws Exception {
    if(country.equals("POLAND")){
        return new BigDecimal(0.23).multiply(amount);
    }
    else if(country.equals("AUSTRIA")) {
        return new BigDecimal(0.20).multiply(amount);
    }
    else if(country.equals("CYPRUS")) {
        return new BigDecimal(0.19).multiply(amount);
    }
    else {
        throw new Exception("Country not supported");
    }
}

I can change ifs to switches:

private BigDecimal calculateTax(String country, BigDecimal amount) throws Exception {
    switch (country) {
        case "POLAND":
            return new BigDecimal(0.23).multiply(amount);
        case "AUSTRIA":
            return new BigDecimal(0.20).multiply(amount);
        case "CYPRUS":
            return new BigDecimal(0.19).multiply(amount);
        default:
            throw new Exception("Country not supported");
    }
}

but 195 cases is still so long. How could I improve readability and length of that method? What pattern would be the best in this case?

7 Answers

Create a Map<String,Double> that maps country names to their corresponding tax rates:

Map<String,Double> taxRates = new HashMap<> ();
taxRates.put("POLAND",0.23);
...

Use that Map as follows:

private BigDecimal calculateTax(String country, BigDecimal amount) throws Exception {
    if (taxRates.containsKey(country)) {
        return new BigDecimal(taxRates.get(country)).multiply(amount);
    } else {
        throw new Exception("Country not supported");
    }
}

Put the data in an XML file or database, then use it to populate a dictionary. That way you can change the data easily, and separate the data from your application logic. Or, just execute a SQL query in your method instead.

Don't do this!

As it is right now, your calculateTax method is like a container for four actual calculateTax methods, one for each of the 3 countries, and one for the invalid case. Every other method you make along these lines will be like that. Following this pattern, you'll end up with many switches (checking for the same set of cases) within many methods, where each case contains the specifics of a case. But that's exactly polymorphism does, in a much better way!

Patterns like this are a very strong indication that you're not taking advantage of object orientation, and barring any other reasons not to, you definitely should. It's Java after all, and that's kind of the whole schtick.

Create an interface like TaxPolicy:

interface TaxPolicy {
    BigDecimal calculateTaxFor(BigDecimal saleAmount);
}

Create a class that implements it:

class NationalSalesTaxPolicy implements TaxPolicy  {
    String countryName;
    BigDecimal salesTaxRate;

    // Insert constructor, getters, setters, etc. here

    BigDecimal calculateTaxFor(BigDecimal saleAmount) {
        return saleAmount.multiply(salesTaxRate);         
    }
}

Then, create objects of this class, one per country you wish to support. We can wrap this list into a new class, NationalSalesTaxCalculator, which will be our one-stop-shop for calculating sales tax for any country:

class NationalSalesTaxCalculator {
    static Map<String, NationalSalesTaxPolicy> SUPPORTED_COUNTRIES = Stream.of(
        new NationalSalesTaxPolicy("POLAND", "0.23"),
        new NationalSalesTaxPolicy("AUSTRIA", "0.20"),
        new NationalSalesTaxPolicy("CYPRUS", "0.19")
    ).collect(Collectors.toMap(NationalSalesTaxPolicy::getCountryName, c -> c));

    BigDecimal calculateTaxFor(String countryName, BigDecimal saleAmount) {
        NationalSalesTaxPolicy country = SUPPORTED_COUNTRIES.get(countryName);
        if (country == null) throw new UnsupportedOperationException("Country not supported");

        return country.calculateTaxFor(saleAmount);
    }
}

And we can use it like:

NationalSalesTaxCalculator calculator = new NationalSalesTaxCalculator();
BigDecimal salesTax = calculator.calculateTaxFor("AUSTRIA", new BigDecimal("100"));
System.out.println(salesTax);

Some key benefits to notice:

  1. If you add a new country you want to support, you just have to create a new object. All methods that might need that object, automatically "do the right thing", without needing to manually find them all, in order to add in new if statements.
  2. You have room to adapt functionality as necessary. For example, where I live (Ontario, Canada), sales tax isn't charged for groceries. So I could make my own subclass of NationalSalesTaxPolicy that has more nuanced logic.
  3. There's even some more room for improvement. Notice that NationalSalesTaxCalculator.calculateTaxFor() contains some code specific to handling an unsupported country. If we add in new operations to that class, every method would need the same null check and error throw.

    Instead, that could be refactored into using the null object pattern. You implement an UnsuppoertedTaxPolicy, which is a class that implements all interface methods by throwing exceptions. Like so:

    class UnsuppoertedTaxPolicy implements TaxPolicy {
        public BigDecimal calculateTaxFor(BigDecimal saleAmount) {
            throw new UnsupportedOperationException("Country not supported");
        }
    }
    

    You can then do

    TaxPolicy countryTaxPolicy = Optional
        .ofNullable(SUPPORTED_COUNTRIES.get(countryName))
        .orElse(UNSUPPORTED_COUNTRY);
    return countryTaxPolicy.calculateTaxFor(saleAmount);
    

    This "centralizes" all your exceptions into one place, which makes them easier to find (thus easier to set break points on), easier to edit (in case you ever want to migrate the exception types, or change the message), and it declutters the rest of the code, so that it only needs to worry about the happy case.

Here's a working demo: https://repl.it/@alexandermomchilov/Polymorphism-over-ifswitch

As a frame challenge...

195 cases is not too long if it's clear what they're doing and why, and if the code inside each case is minimal. Yes it's long, but it's perfectly readable because you know exactly what it's doing. Length does not necessarily imply unreadability.

As other answers have said of course, this may be a code smell indicating you're not using OO properly. But on its own, it's just long, not unreadable.

If the values are constant and are not meant to be changed regularly (which I doubt). I'd introduce a static metamodel using Enum:

public enum CountryList {

    AUSTRIA(BigDecimal.valueOf(0.20)),
    CYPRUS(BigDecimal.valueOf(0.19)),
    POLAND(BigDecimal.valueOf(0.23));

    private final BigDecimal countryTax;

    CountryList(BigDecimal countryTax) {
        this.countryTax = countryTax;
    }

    public BigDecimal getCountryTax() {
        return countryTax;
    }

    public static BigDecimal countryTaxOf(String countryName) {
        CountryList country = Arrays.stream(CountryList.values())
                .filter(c -> c.name().equalsIgnoreCase(countryName))
                .findAny()
                .orElseThrow(() -> new IllegalArgumentException("Country is not found in the dictionary: " + countryName));

        return country.getCountryTax();
    }
}

Then

private BigDecimal calculateTax(String country, BigDecimal amount) throws Exception {
    return CountryList.countryTaxOf(country).multiply(amount);
}

It's readable, compile time safe, easily extendable with additional metadata per country and less boilerplate.

EDIT: missed @Alexander's answer; it might be a bit overkill, but he's hitting the main point as well: use OOP.
EDIT 2: implemented @Luaan's suggestions

I'm probably missing something obvious, and it might be a bit hard to implement at this late a stage, but this looks to me like a perfect case for Object Oriented Programming:

You create a Country class which contains everything pertaining to a country, such as a name and a calculateTax() method and whatnot, and then your caller (calculateTotalAmount(), or whatever) will call country.calculateTax(amount) instead of calculateTax(country, amount), and the entire if/switch construction is just gone.

Besides, when you add support for a new country (say, there's another civil war somewhere and a country splits up), you just add everything for the new country in one single place instead of hunting down a gazillion methods with gigantic if() chains or switch()es...

If you insist on using switch: since JDK 12, switch expressions potentially improve readability and length of the switch statement by allowing multiple case labels and lambda-style syntax:

private BigDecimal calculateTax(String country, BigDecimal amount) throws Exception {
    double rate = switch(country) {
        case "POLAND", "IRELAND" -> 0.23;
        case "AUSTRIA", "FRANCE" -> 0.20;
        case "CYPRUS", "GERMANY" -> 0.19;
        default -> throw new Exception("Country not supported");
    };
    return new BigDecimal(rate).multiply(amount);
}
Related