I had a bit of a quandary today as I reviewed some code today and I decided to write up this little post on the experience. In essence it came down to choosing between code readability and code reuse. I am a huge fan of both and they almost never conflict in fact they almost always exist in a symbiotic state. But not this time.
This title of this post has been buzzing around in my head recently as I have been doing a lot of performance tuning on a very large and HIGHLY trafficked web site of a major print publication. Anyway I was recently reading (reference coming soon! until then dig through my delicious links) a blog post on the topic of performance tuning and why it is silly how often people micro-tuning. Think of that in the same negative respect that you think of micro-managing. It’s a waste of time and resources. The moral: even though performance tuning is normally a good thing, after a while it looses it’s value. You spend your money on the low hanging fruit (AHEM more hardware!)
Back to today. The code that I was reviewing had a few very cryptic stanzas that included a call to a function that consumed a single integer based parameter which after thorough inspection simply ended up determining if the query it finally triggers against the database is ascending or descending. Now I honestly doubt this developer (no matter how Jr) actually thought they would be increasing the performance of the application by including this integer parameter. In fact I am sure that they assumed that they were doing a good thing by reusing code (which I am a HUGE proponent of normally!) but when it came time for another developer to get into the code to augment it slightly it took exponentially more time then it would have if the original developer had simply a) created the database queries inline or b) created two separate nearly identical functions. Instead (fyi we are using an ORM that follows the ActiveRecord Pattern) the object has one single method getNextArticle(int foo) that returns both the previous AND the next article! How confusing! Anyway the best solution that required the least new untested code at this point was to wrap that function with two new functions that map to the integer values. Now we have:
/*Note that the following function has a different method signature than the original and thus can coexist due to the method overloading feature of java*/
getNextArticle(){
return this.getNextArticle(1);
}
getPreviousArticle(){
return this.getNextArticle(2);
}
I can already hear someone out there saying: “That’s silly why not re-factor the whole object so that it has:
getNextArticle()
getPreviousArticle()
wrapping some new function that is called with a String paramater like ASC or DESC.” Well to put it simply, time. Regression testing takes time and we would have to do a heck of a lot more of it on this project that unfortunately does not yet have 100% unit test coverage (to my DISDAIN) and does not have an automated testing setup. So the moral of the story is this. Even though code reuse is almost always a GOOD sign of proper software development practices sometimes it can lead to poor readability which then becomes more of a maintenance problem than the code reuse actually solves. In retrospect it would have been great to have found this code before it went through thorough user acceptance testing. However due to the accelerated nature of meeting client demands and working on unrealistic project schedules we were not able to do enough peer code reviews of code nor were we able to create a team large enough to do pair programming.
AHH how the real world always ruins principles, techniques, patterns and paradigms that work so well on paper!
