“TODO: Refactor this…” Twice the Code Smell
Thursday, July 30, 2009 – 5:00 AMToday while reviewing some code* I came across the following:
// TODO: Refactor this.
Followed by a lot of code which clearly needed some love and attention. Why is this code smell? Well it’s undone work just for starters. Someone checked this in and called it good, knowing that it was incomplete.
Then you’ve got to ask yourself… Why this was left as a “TODO”? This is the real issue.
What the code really needed was a series of simple refactorings based around extract method, followed by some general moving of things around. Not exactly a gargantuan task. But in order to refactor code quickly and safely you need some tests around it. In this case there were no tests at all!
This is of course a “WTF!” moment. Not only is the code itself incomplete but the work is incomplete because it’s untested.
The lesson. If you’re reviewing code and you see a “TODO: Refactor this…” ask yourself, do we have a deeper issue here?
* I’m happy to say this was not source code delivered as guidance by p&p.
Welcome to my blog on software development and probably a whole lot more.
4 Responses to ““TODO: Refactor this…” Twice the Code Smell”
I couldn’t agree less. For one thing, this comment applies to all code at all times – code is never completed, merely abandoned because of release dates and economic need. There’s always something to improve in any non-trivial piece of code.
Second of all, the unique combination of knowledge, time and manpower necessary to create a more perfect design are not available at all times. Sometimes you just need something that works, and you know should refactor to more general pattern, or to an abstraction, but you don’t know what the precise pattern is and you still have higher priorities on the list.
Making every design element satisfy your inner architecture astronaut all the time is irresponsible. Marking something with “need to refactor” is simply a reminder for the downtime.
By Chris B. Behrens on Jul 30, 2009
I’ve done this – say I’m in the middle of a refactor of A, and run across something else that REALLY need refactoring, but is legacy, has no tests etc. I’ll pop a todo: Refactor This, finish up refactor A, and CHECK IN. Why? It’s a rollback point – something that WORKS, and I can revert to. Then I’ll start on refactor B or C or whatever. This stuff is usually in DEEP legacy code that I’m cleaning/trying to make work (port from one language to another etc). It’s a mark of “Gad, there is a stink here, I know it, but I’m busy on other stinks”
By KG2V on Jul 31, 2009
Chris is correct, software is usually never truely finished so you could say that everything could still be considered a TODO. In reality TODO’s like this get attached to the worst offending bits of code, legacy or not. You’ll note that I’m advocating removing every single TODO or completing every single refactoring.
Like most software development what to do here is “it depends”. My lesson from the code I found is that you should ask is something worse lerking here?
By Ade Miller on Aug 3, 2009
Ade,
What I was trying to say is a lot of what Chris said – who says the code is “done” when it’s checked in?
You’re doing a code change/refactor, and see something that works, but should be refactored, so you flag it with a todo, finish what you’re doing, check in, and hopefully don’t get pulled to another project (some of us don’t have the luxury of spending time fixing old dirty legacy code “just because” – he who signs the paycheck makes the rules)
As I said, I’ve done that exact todo, but always while fixing/refactoring some legacy code, where the code is in maintainence mode, and in fact, in near end of life mode (we have 10-15 old code that gets a change/fix every 3-4 years. You fix what you need to, refactor what you can/have the time to, and make notes on code smells so that they are easier to find the next time you open the code base, in 2-3 years
By KG2V on Aug 4, 2009