Tax Calculator using Java 8 and BigDecimal

Question

I am learning Java8 and BigDecimal. The following exercise involved writing a Tax Calculator that took a salary and calculated the tax payable based on the tax band into which the salary falls. The test passes as the code stands but if I remove the call to

setScale(0, BigDecimal.ROUND_UP)

then the rounding does not occur and the tests fail like this>

TaxCalculator.calculateTax > 712.50
java.lang.AssertionError: Expected: is <713>
 but: was <712.50>

I don't understand that because the 2nd parameter to the divide call specifies that rounding should be performed.

Also, Can anyone suggest improvements to the way this class is written please? I'm just trying to nail down best practices with respect to the Streams API. Am I using an appropriate way to calculate a percentage in Java?

import org.junit.Before;
import org.junit.Test;
import java.math.BigDecimal;
import java.util.HashMap;
import java.util.TreeMap;
import static org.hamcrest.CoreMatchers.is;
import static org.junit.Assert.assertThat;
public class TaxCalculatorTest {
 private TaxCalculator taxCalculator;
 private HashMap<BigDecimal, BigDecimal> testData = new HashMap<>(); // <salary, taxPayable>
 private TreeMap<BigDecimal, BigDecimal> taxBands = new TreeMap<>(); // uses natural ordering of keys
 // Could have used LinkedHashMap to maintain order but that relies on entries being inserted in the correct order
 @Before
 public void setup() {
 testData.put(new BigDecimal("5000"), new BigDecimal("225"));
 testData.put(new BigDecimal("11000"), new BigDecimal("413"));
 testData.put(new BigDecimal("19000"), new BigDecimal("713"));
 testData.put(new BigDecimal("27000"), new BigDecimal("905"));
 taxBands.put(new BigDecimal("10000"), new BigDecimal("4.5"));
 taxBands.put(new BigDecimal("35000"), new BigDecimal("3.35")); // TreeMap fixes the ordering
 taxBands.put(new BigDecimal("20000"), new BigDecimal("3.75")); // This entry will be inserted before the previous one
 taxCalculator = new TaxCalculator(taxBands);
 }
 @Test
 public void calculateTax(){
 testData.forEach((salary, taxPayable) -> {
 assertThat(taxCalculator.calculateTax(salary), is(taxPayable));
 }) ;
 }
}
import java.math.BigDecimal;
import java.util.HashMap;
import java.util.TreeMap;
class TaxCalculator {
 private static final BigDecimal NO_TAX = new BigDecimal("0");
 /*
 * scale > the number of digits to the right of the decimal point
 *
 * precision > the number of significant digits
 *
 * BigDecimal instances are immutable
 *
 */
 private BigDecimal ONE_HUNDRED = new BigDecimal("100");
 private final TreeMap<BigDecimal, BigDecimal> taxBands;
 TaxCalculator(TreeMap<BigDecimal, BigDecimal> taxBands) {
 this.taxBands = taxBands;
 }
 BigDecimal calculateTax(BigDecimal salary) {
 BigDecimal taxPayable = salary.multiply(getTaxRate(salary)).divide(ONE_HUNDRED, BigDecimal.ROUND_UP).setScale(0, BigDecimal.ROUND_UP);
 System.out.println("TaxCalculator.calculateTax > " + taxPayable);
 return taxPayable;
 }
 private BigDecimal getTaxRate(BigDecimal salary) {
 return taxBands.entrySet().stream()
 .filter((entry) -> salary.compareTo(entry.getKey()) < 0)
 .map(HashMap.Entry::getValue)
 .findFirst().orElse(NO_TAX);
 }
}

Answer 1

1. The rounding doesn't work the way you expect it to work for division. From documentation:

   Returns a BigDecimal whose value is (this / divisor), and whose scale is this.scale().

   The product of the salary and the tax rate has two digits after the decimal point. So does the quotient.

2. The getTaxRate method is unnecessarily long and complicated. You're trying to find the largest entry in a TreeMap such that is less than the given value. That's exactly what the lower method does.

3. Printing something inside the calculateTax method is a bad idea. It makes the code less reusable (what if I want to just calculate the tax without any logging?). And violates the single responsibility principle (it calculates the tax and prints something to standard output).

4. This kind of comments:

   /*
    * scale > the number of digits to the right of the decimal point
    *
    * precision > the number of significant digits
    *
    * BigDecimal instances are immutable
    *
    */
   

   is even worse than useless. It clutters the code with irrelevant information. The documentation for the BigDecimal class belongs to the BigDecimal class, not to a TaxCalculator.

   There several other comments in your code that repeat parts of the TreeMap class documentation. They're also gibberish. Removing all these comments will make the code cleaner.

5. Adding doc comments for your TaxCalculator class and its public is a good idea. They should describe what it represents, what are the pre- and post-conditions of each method (for instance, what happens if I pass a negative salary? In your case, it's garbage in- garbage out. Document that. It's obvious from the method signature itself).

6. You might want to add proper input validation (for instance, you can throw the exception if the salary is negative).

7. You can also use the valueOf static factory method to convert integers to BigDecimal's: to me, BigDecimal.valueOf(100) looks better than new BigDecimal("100"). It can also reuse frequent values.

• Stingy: It doesn't change the principle, but I think the purpose of getTaxRate(BigDecimal) is to find the smallest key in the map that is greater than salary.

Answer 2

• You don't need to create a new BigDecimal instance to represent zero, because an instance of zero already exists: BigDecimal.ZERO
• taxBands is declared as a TreeMap, which is a class, and your TaxCalculator constructor likewise accepts a TreeMap, but actually, you only need taxBands to exhibit a specific behavior defined in an interface, namely SortedMap (or, if you follow kraskevich's suggestion in point 2 of his answer, a NavigableMap), but you don't depend on the specific implementation of that interface, so I'd suggest declaring the taxBands as a SortedMap or NavigableMap, depending on which functionality you need.

• Also, about the constructor:

  TaxCalculator(TreeMap<BigDecimal, BigDecimal> taxBands) {
   this.taxBands = taxBands; //this is dangerous
  }
  

  The constructor is passed a mutable object which the newly created TaxCalculator now references, so your TaxCalculator does not have exclusive control over taxBands, because if the TreeMap referenced by taxBands is modified from outside your TaxCalculator object, this modification will be reflected in the TaxCalculator. It would be safer to do this:

  this.taxBands = new TreeMap<>(taxBands);
  

  This also has the effect that the constructor could now accept any Map<BigDecimal, BigDecimal>, and not only a TreeMap, SortedMap, NavigableMap or whatever.

• This line is confusing:

  .map(HashMap.Entry::getValue)
  

  The Entry interface you want is defined in the interface Map, not in the class HashMap. Apparently, it still compiles and references the correct interface, but it would be less ambiguous to write:

  .map(Map.Entry::getValue)
  

  Interestingly, the following does not compile, even if taxBands is declared as a TreeMap:

  .map(TreeMap.Entry::getValue)
  

  The reason for this is that, now, the code doesn't refer to the interface Map.Entry, but to a package-private class named Entry in TreeMap, and since you are not inside the package java.util, you are denied access to java.util.TreeMap.Entry. Actually, even if TreeMap.Entry were public, I don't think the code above would compile, because even TreeMap.entrySet() only returns a Set<Map.Entry>, and a Map.Entry cannot be assigned to a TreeMap.Entry (only the other way round were possible, since TreeMap.Entry implements Map.Entry).