In part 2 we wrote three characterization tests for our code sample. Now it’s time to start leveraging them.

As you recall, the code we inherited looks pretty cryptic:

public class SalesUtil {

 final static double BQ = 10000.0;
 final static double BCR = 0.20;
 final static double OQM1 = 1.5;
 final static double OQM2 = OQM1 * 2;

 public static double calculateCommissionDue(double totSales) {
  if (totSales <= BQ) {
   return totSales * BCR;
  } else if (totSales <= BQ * 2){
   return
    (BQ) * BCR +
    (totSales - BQ) * BCR * OQM1;
  } else {
   return (BQ) * BCR +
    (totSales - BQ) * BCR * OQM1 +
    (totSales - BQ * 2) * BCR * OQM2;
  }
 }
}

In order to understand and improve this code, my next step is to find out what those cryptic acronyms (like OQM1) actually mean. By looking what little scattered documentation I can find and asking around, I discover what those acronyms stand for and my first task in improving the code is to replace them with something more legible:

public class SalesUtil {

 final static double BaseQuota = 10000.0;
 final static double BaseCommissionRate = 0.20;
 final static double OverQuotaMultiplier1 = 1.5;
 final static double OverQuotaMultiplier2 = OverQuotaMultiplier1 * 2;

 public static double calculateCommissionDue(double totSales) {
  if (totSales <= BaseQuota) {
    return totSales * BaseCommission;
  } else if (totSales <= BaseQuota * 2) {
    return
     (BaseQuota) * BaseCommission +
     (totSales - BaseQuota) * BaseCommission * OverQuotaMultiplier1;
  } else {
    return (BaseQuota) * BaseCommission +
     (totSales - BaseQuota) * BaseCommission * OverQuotaMultiplier1 +
     (totSales - BaseQuota * 2) * BaseCommission * OverQuotaMultiplier2;
  }
 }
}

Ahhh, that’s better.

I used my IDE’s refactoring capabilities to change the acronyms into more meaningful identifiers, so I doubt I broke anything. But since I already have my characterization tests written and they run quickly, I take advantage of them. As expected, they all run and pass without problem. Excellent.

The code looks better, but it still needs work. I notice, for example, some duplicate code (e.g., BaseCommission * OverQuotaMultiplier1). Duplicate code is considered a code smell, an indication that your code is not as clean as it could be, those smelly areas are a fertile ground for potential bugs. So my next step is to eliminate some duplicate code, and improve readability by creating and using two new variables (OverQuotaCommissionRate1 and OverQuotaCommissionRate2) and calculating the two different commission rate at the time they are declared. Here’s the new code:

public class SalesUtil {

 final static double BaseQuota = 10000.0;
 final static double BaseCommissionRate = 0.20;
 final static double OverQuotaCommissionRate1 = BaseCommissionRate * 1.5;
 final static double OverQuotaCommissionRate2 = BaseCommissionRate * 2;

 public static double calculateCommissionDue(double totSales) {
  if (totSales <= BaseQuota) {
   return totSales * BaseCommissionRate;
  } else if (totSales <= BaseQuota * 2){
   return
    (BaseQuota) * BaseCommissionRate +
    (totSales - BaseQuota) * OverQuotaCommissionRate1;
  } else {
   return (BaseQuota) * BaseCommissionRate +
    (totSales - BaseQuota) * OverQuotaCommissionRate1 +
    (totSales - BaseQuota * 2) * OverQuotaCommissionRate2;
  }
 }
}

There is still some duplicate code, but it’s starting to look better. Let’s see, however, if the code still behaves as it did before. I run my characterization tests and get the following error:

junit.framework.AssertionFailedError: expected:<14000.0> but was:<12000.0>

It was a trivial change and yet I managed to break something. Fortunately I had the characterization tests to save the day. I look at the two versions of the code and I realize my mistake. The line:

final static double OverQuotaCommissionRate2 = BaseCommissionRate * 2;

Should have been:

final static double OverQuotaCommissionRate2 = OverQuotaCommissionRate1 * 2;

This is a typical example of how one’s belief in how the code should work (i.e., OverQuotaCommissionRate2 should be twice the BaseCommissionRate) conflicts with how the code actually works (i.e., OverQuotaCommissionRate2 is twice the already increased commission rate, not twice the basic rate).

Calculated in the original way, OverQuotaCommissionRate2, works out to 0.60 (i.e., 60%) which seems a bit excessive. Something tells me that this is a bug in the original version. I am surprised that it hasn’t already been discovered; on the other hand I have a hard time imagining people complaining for having been paid too much.

My job right now is to preserve the behavior of the original code, bugs and all, so I make a note of the possible problem, and edit the new version of the code with the corrected calculation to make it work just like the original. I re-run the characterization tests, expecting them to pass, but I get the following output:

junit.framework.AssertionFailedError: expected:<14000.0> but was:<14000.000000000002>

Darn. Bitten by some pesky rounding problem. Time for a quick executive decision. By re-arranging some of the calculations, the new code behaves differently, but it’s only off by a fraction of a billionth of a penny. In retrospect, I should have written the characterization tests taking into account that commission checks have to be accurate to the penny, but not beyond that. So I re-write my characterization tests using a version of assertEquals that allows me to specify the desired precision as the third parameter. This is what the latest set of characterization tests looks like:

public void testCalculateCommissionDue1() {
        assertEquals(200.0, SalesUtil.calculateCommissionDue(1000.0), 0.01);
}

public void testCalculateCommissionDue2() {
        assertEquals(5000.0, SalesUtil.calculateCommissionDue(20000.0), 0.01);
}

public void testCalculateCommissionDue3() {
        assertEquals(14000.0, SalesUtil.calculateCommissionDue(30000.0), 0.01);
}

I re-run the tests and they all pass with flying colors. Great. This is a good stopping point for now.

In part 3, we have seen our investment in characterization tests starting to pay dividends. We set out to improve the readability of the code and, by running the tests after each change, we made sure that the original behavior was preserved. We saw that it’s easy to change the behavior of the original code inadvertently – even when making seemingly trivial changes – but that a good set of characterization tests will catch our mistakes.

Hopefully you’ll agree that the latest version of the code – although still not ideal – is considerably easier to read and understand than the original (with all the cryptic acronyms and the duplicate code). Thanks to the characterization tests, you should have some confidence that the original behavior (including some potential bugs) has been preserved.

In part 4, we are going to take fun detour and look at how we can use JUnit Factory, a free web-based characterization test generator (and my pet research project), to help us automate some of the work involved in characterization testing.

If you can't wait for the next part to come out, or simply want to get a head-start, feel free to get some first-hand experience using JUnit Factory with this example. Just go to http://www.junitfactory.com/demo/index.jsp and cut and paste any one of our implementations of the sample code the window that says "Enter Some Java", then press "Generate a Test". After a few seconds, a test for the code should appear in the window labeled "Tests". Click on it to take a look at what characterization test the generator has come up with. Have fun.