Unmaintable Code - It doesn’t have to be this hard…

Two of my all time favorite authors, Kathy Sierra and Bert Bates, coined the phrase, ”Code [your project] as if the next guy to maintain it is a homicidal maniac who knows where you live.” I truly wish more people would take that to heart.  All too often I am working on a maintenance task, and I find that I spend more time trying to figure out what the hell the code is doing than I spend actually fixing the bug.

Take, for instance, the following real code snippet that a colleague of mine recently sent me…

1   for (int i = 0; i < (packagedItems.size() - 1); i++) {
2      PackagedItem item = (PackagedItem) packagedItems.get(i);
3      if (skuToOrderLineIdMap.containsKey(
            item.getOrderedItem().getSku())) {
4         confirmIntermediateLineItem(((Integer)
            skuToOrderLineIdMap.get(
              item.getOrderedItem().getSku())).intValue(),
            item.getOrderedItem().getSku(),
            item.getQuantity().intValue());
5      }
6   }


OK, seriously.  What does that fourth line even mean?  There is no way that any engineer, regardless of skill level, would be able to read that line and have a general idea of what is going on.  Actually, I made it even easier on you by breaking line 4 up across multiple lines so it displayed nicely.  In an IDE it is all one line…

Believe it or not, the confirmIntermediateLineItem method only takes three parameters.  You have to really focus to be able to figure that out.  Hell, I needed to use a white board.

It does not have to be this hard.  When you are writing code, it takes no extra time to make it legible to future developers who may be maintaining it.  Here is my revision of the above snippet:

1  // loop over packaged items and confirm each line item
2  // against the database
3  for (int i = 0; i < (packagedItems.size() - 1); i++) {
4    PackagedItem item = (PackagedItem) packagedItems.get(i);
5    String itemSku = item.getOrderedItem().getSku();
6    Integer itemQuantity = item.getQuantity()
7    Integer orderId =
8      (Integer) skuToOrderLineIdMap.get(itemSku)
9      if (skuToOrderLineIdMap.containsKey(itemSku)) {
10       confirmIntermediateLineItem(orderId, itemSku,
              itemQuantity);
11   }
12 }


Ahhh.  That’s better.  All I did was add a simple comment, and seperate out the method calls.  Much, much easier to read.  And it took me about 2 minutes to do.  There is no performance hit, because all of the methods were called anyway.  There is no memory hit, because the method returns would be stored on the heap anyway.  It is, quite simply, the exact same code, but written in a way that makes it easier to read, and therefore easier to maintain.  Now I can go put away my bloody ax…