Friday, 12 November 2010

When later you realize your own code sucks!

Recently, I had my first experience where I looked at some code I had written in the past (roughly 2 years ago) and realized it was quite bad. Bad it the sense you think "OMG, did *I* write this?!". Bad also in the sense that it could have been written in a better way in the same (or less) amount of time that it took to write the bad code. This last point is important because it is common (and somewhat expected) that developers, even good ones, will sometimes write bad code when writing the good code instead would take longer. This situation presents a trade-off of short-term vs. long-term advantages, and deciding this trade-off is a classical software engineering decision.
But that was the case here. Here, the good code was actually quicker to write, I just didn't see it then. The issue was that I had two collections, and the goal was to compare them for equality, but instead of putting the elements in Java's HashSets and then just using the built-in Set.equals(), I stored the elements in two arrays, and checked the equality myself using 2 nested linear searches... So, not only was the code roughly twice as complex, the asymptotic performance was much worse, (quadratic instead of linear). And on top of that, this code was duplicated (with a few minor differences) in another class, something I rarely allow myself doing.
In my defense though, there are some mitigating points:
  • The elements from the two collections were not of the same type. So with the array approach I could directly write the comparison code for the pair-wise element equality while iterating, whereas in the HashSet approach you can't, I had to convert the elements of one of the collections to the same type as the other elements, to be able to use Set.equals() on both sets.
  • This code was part of testing code, not actual application code. Thus the performance issue was not very relevant: it was not present in the application, and the inputs were small, known in advance (because they were specified in the code, not by the user).
I might have not forgiven me if code like this was in application code. :P Or, for that matter, if the code was bad enough that I would not easily understand it anymore, like this. (Exception made for cryptic language features though, like many in C++ ...)