Follow your nose

Today's tale concerns what happens when we see code smells, and the twisty path we sometimes follow between getting a whiff of something, and reaching the real problem, and/or solution.

It started innocently enough. I was going through the compiler warnings from Eclipse, cleaning them up. Most of these fixes are improvements but not especially big or difficult. For example, getting rid of an unused variable, running the "organize imports" tool, adding missing @Override annotations, etc. But then I saw one which was clearly pointing to something bigger. A simplified version of the code with the warning is:

int FLAT_INTEREST = 1;
int DECLINING_INTEREST = 2;
createLoan(FLAT_INTEREST);

and the warning was because DECLINING_INTEREST was never used. Now, even without going through code archeology I can pretty much guess the sequence of events:

* Developer one creates a method

createLoan(int interestType)
// 1=flat, 2=declining

and various calls to it of the form

createLoan(1);
createLoan(2);

* Developer two sees some of these calls, and in the process of trying to read this code, or write something similar, wants to make the code better say what the parameter means:

int FLAT_INTEREST = 1;
createLoan(FLAT_INTEREST);

She (or the next developer) also figures out that "flat" is as opposed to "declining" and that two means declining. Hence the:

int DECLINING_INTEREST = 2;

All of this is all well and good. Sure, it is easy to find fault with the idea that local variables are a good place for these constants, but the local variable is better than what we had before - createLoan(1) - and we should be willing to improve code clarity one step at a time, rather than trying to solve everything at once.

A next small step could be to turn these local variables into constants in some central place, but I instead looked at the implementation of createLoan. It was something along the lines of:

createLoan(int interestType) {
  return new Loan(
  InterestType.fromInt(
    interestType));
}

That is, this int was getting turned into an
enum anyway. Well, once we see that there is already an enum, we realize most of the job is already done, and we end up with something like the following:

createLoan(InterestType type) { . . . }

createLoan(InterestType.FLAT);

Well, actually there were too many callers to convert everything in one step, but I converted some, and kept the createLoan(int) method as a transitional aid, as I've described previously.

(For those people wanting to see the actual code in MIFOS, the code which had had the warnings was getLoanAccount in CollectionSheetHelperTest, the method which I called createLoan above really is createLoanOffering in TestObjectFactory and the enum is InterestType).