September 15, 2009
One of the best phrases I’ve encountered in my professional programming career is “smelly code”. It’s great: it sounds childish enough to make me smile, yet also highlights an important indicator in code quality.
Like many, I first encountered the concept of code smells in Martin Fowler’s essential “Refactoring” book, although the term was coined by Kent Beck and predates the book. We’ve all seen examples, and even contributed our own aroma to code. Code duplication; excessively large classes, methods, functions, modules; unreadable code; tightly-coupled code; fragile code; overly complex code; code that circumvents encapsulation.
Over the years, I’ve picked up a nose for code smells. I get a feel for code that is starting to smell, and usually get that prick of guilt when my own code begins to emit an unpleasant fragrance, even if I don’t always know immediately how to deal with it. Deodorant comments like “# REFACTOR: this feels wrong” or “// REFACTOR: this code is ugly!” start to crop up as a warning sign that things need to be looked at, if immediate action can’t be taken.
The nose kicked in this afternoon while working on a Django view. Many smells start off with the best of intentions, and this was no exception. In order to make my tests pass, I started off with a fairly harmless bit of code that interacted with a form object. Over a period of a few tests, the code began to expand and some duplication kicked in. List comprehensions entered into the equation. The nose began to kick in. I can’t show the code, unfortunately, and I’m too lazy to concoct some example code that illustrates my point, so bear with me…
Once all the functionality I needed had been implemented and the code checked-in, I cast an eye over the code and realised it looked ugly. The warning signs were there:
- two almost-identical list comprehensions
- temporary variables used to make the list comprehensions more readable
- several lines to do something which should be simple
- the need to re-read the code more than once to “see” what it does
*facepalm* What was I thinking when I wrote that??
First step was to extract the code into a utility function, in the hope of making the view code more readable and make it easier to eliminate the duplication. I find extracting code can be an excellent first step in dealing with ugly code, but it has to be used responsibly: otherwise you end up moving the smells into another layer of abstraction.
In the process of creating the function, another smell became apparent:
- borderline intimate knowledge of the form class
This smell instantly pointed out where and how to refactor the code. The extraction was still necessary, but instead of creating a utility function in the view it was apparent that the code could be moved into two methods of the form. A quick change and a re-run of the tests confirmed that the extraction worked and the view instantly looked simpler and more readable. However, the new methods still didn’t quite feel right.
Next step was to take the list comprehensions and break them out into multi-line for loops. It might sound counter-intuitive to take code that occupies one line (albeit with preparatory code) and turn it into four or five lines of code, but the result was for the best. The intent of the code became clearer and the need for those temporary variables was removed.
All that remains is a minor whiff, a single bit of hardcoding used to identify particular fields in the form. My nose is telling me that introspection will lead me down the path of perfumed code…
Also of note:
(I’ve not tried this yet – just found it!)