How can I improve readability and length of a method with many if statements?
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:
- 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.
- 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. 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
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");
}
}