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?
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");
}
}
This doesn't seem like a reasonable place to throw a checked exception. I know that OP's code does this, but I'd still rather not propagate it.
@BenP. I see no issue with throwing a checked exception if you receive an unexpected/not supported input. I'd probably use some custom exception and not the base Exception class though.
An
IllegalArgumentException
should be fine, but a plainException
is a no-go...You should definitely not use a map, it'll work for this toy problem, but it's really limited in future expandability (when you want more cases, more payloads in addition to the tax rate, etc.). This is what object orientation is for. See my answer.
Small nitpick:
Map.ofEntries()
is probably nicer than repeating all theput
calls. Larger nitpick: That's still too much code duplication, and some of the other commenters are right, IMHO, to factor the data out into a file or database and populate the map from it or query the data source directly. Medium nit: yeah, what others said about exceptions. :)