Thursday, September 20, 2007

Helpless Helpers and Useless Utilities

With any code base of a reasonable size there are lots of issues you would normally take care of immediately when you come across them, however often there is just no time for it. In the end you will have to live with the knowledge that you had to leave some ugly hacks in it just to meet the deadline.

Because we have recently finished development of the next major release of our software product, there is some time now to do code cleanup and get some more automated tests on the way. Because one of the bugs that almost prevented us from holding our schedule was a particularly nasty - but well hidden one - there has (again) been some discussion about coding guidelines and quality.

People always seem to agree that you need to talk to each other, think in larger terms than just your specific problem at the time and strive for code readability and re-usability. For starters I personally would sometimes even do away with just a little more of the former one...

The bug I mentioned above was in a method to determine whether a given date is a holiday or a business day. Because this is not a trivial problem to solve generically for several countries, we rely on a database table that is pre-populated with the holidays for the next few years in a given country. To tell whether or not a date is a holiday is just a mere database lookup. Nothing can really go wrong here, can it? The method in question was implemented like this:

public static boolean isHoliday(Date aDate) {
   if (getRecordFromHolidayTableFor(aDate) == null) {
      return true;
   }
   return false;
}

Of course there was a corresponding method for business days, too:

public static boolean isWorkday(Date aDate) {
 Calendar cal = Calendar.getInstance();
 cal.setTime(aDate);
 int day = cal.get(Calendar.DAY_OF_WEEK);
 if (day == Calendar.SATURDAY || day == Calendar.SUNDAY) {
     return false;
 }
 return isHoliday(aDate);
}

Just in case you do not know: Saturday is usually considered a business day in Germany, for our customer it definitely is. So this part of the latter method alone is at the very least somewhat dubious.

By now you will probably have stopped looking at the code, trying to find out what it is you miss to make this work correctly. You are right to do so, because it is of course just plain wrong. The isHoliday method returns just the opposite to what you would expect. And the isWorkday code - apart from the wrong condition for the DAY_OF_WEEK even leverages this and works in fact (almost) correctly.

One might ask how this could go unnoticed till the last moment of development and I can tell you, the answer will not be pleasant: It didn't!

There were about 20 calls to the holiday method. 19 of these always inverted the result! The author of the 20th call had relied on this seemingly simple method to just work and only found out about the bug when he accidentally connected to a database with an empty holidays table and still got true out of it.

Both of the above methods were part of a class called "DateUtil". Apparently someone had had the idea of providing common date functions with this class, to spare everyone from writing their own methods to manipulate and use dates.

While this, of course, is generally a good idea it would have been even better to provide some thorough testing code for it, because even in the seemingly simplest of methods you have a sufficiently high chance of making a mistake while implementing it. One could point a finger to the author of that class, because he introduced the bug in the first place. But I would rather have 19 more fingers to point at each and every one who noticed the problem and just worked around it in his or her private piece of code, telling no one about the problem and letting everyone find out for him- or herself again. I will not even try and estimate how many hours were spent uselessly because of this...

But it did not end there. Alerted to this I decided to have a look at the project source tree. In the course of development I had already seen classes like StringUtil, CalendarUtil and others. So I just ran a find workspace/ -regex ".*(Util|Utility|Utilities|Helper)\.java" to look for classes matching that pattern. I ended up with over 160 helpers and utilities. Many of them were obviously duplicating functionality just by looking at their names. Several PersistenceUtilitys, StringHelpers and the like. Looking through them they often contained very similar methods, re-implementing the same task over and over again.

Because practically none of these were covered with (J)Unit tests but had only been empirically tested on the precise context of their calls, you could not even just go ahead and replace calls to a similar sounding method in a more central place - and vice versa, making almost all of them virtually useless from a more general perspective.

So the whole idea of creating utility and helper classes had in fact been reduced to absurdity. Right now we are in the process of looking through all of them and consolidating to a handful of really helpful helpers and useful utilities - each of them accompanied by a set of unit tests to prevent another "a holiday-is-a-business-day" problem in the future. We will see what other gems bubble up and see the light of day in the process...

No comments: