Skip to content

‘//TODO’ considered harmful

Yesterday I said that developers should start being a little more militant about the craftsmanship of their code, i.e. pushing back on broken methodology that demands poorly-built code  be released into the wild. This sort of code is always inherently fragile and will break your software if it has not already.

Today I just want to meditate on a code artifact that often manifests itself in such environments. I speak of the comment //TODO seen frequently in such code. The idea that future “refactoring” goes on the “todo” list in such an environment is completely broken. An environment that allows that to happen means it will never get done. Do it now, or don’t do it. TODO is considered harmful.

I have come around to the view that ‘//TODO’ comments should make a build fail. Either:

  1. It is a future story yet to be implemented and placed into the backlog,
  2. It is work in progress (and by definition doesn’t need a TODO because everything still needs to be ‘done’), or,
  3. It is done, as in DONE-done, and therefore the story is complete and there’s nothing left “TO DO”.

In other words, “//TODO” is a indicator of a process that’s not letting developers get to “done”. Identify the problem, propose a counter-measure and fix the process that stopping you from getting to completion.

21 Comments

  1. John Smith wrote:

    I’m glad you don’t work on my team. We use TODO’s to mark parts in the code where we need to spend a little extra time. Before we release, we have a meeting where we assign all TODO’s to developers so they are taken care of or assigned to a future release (for example, someone might put a TODO to mark where code would go for a future known enhancement).

    If you were on the team, people would have to keep these in their heads? Not very efficient.

    Saturday, June 6, 2009 at 06:20 | Permalink
  2. Scot Mcphee wrote:

    John

    Why not just spend the “little extra time” when the issue is first discovered?

    Why isn’t a story card written – or a bug raised – to cover the TODO at time of discovery?

    As for “future known enhancement” – either it’s already a story in the backlog (so why does it need the TODO) or You Ain’t Gonna Need It.

    Saturday, June 6, 2009 at 08:00 | Permalink
  3. Robert wrote:

    What is needed is categorisation, management, and follow-through.

    I like a few levels of these markers, but have never convinced a team to apply them appropriately, so I only do them on my own code.

    I have:
    TODO – stuff that is part of the story I’m working on (or part of the larger story arc – e.g. if error handling is broken out to a different story), but not going into this commit, but WILL go in soon. I put this marker in just to make it easy to jump back. The anticipated story number usually goes with the comment.

    CLEANUP: An area of the code that is “good enough”, but could be improved if time allows. I don’t like using this one, but sometimes you find code that you would like to spend a day or two tidying up and you can’t do that right now.

    WARNING: An indication of code that has known breaking points, but which are acceptable for now. Oh, for example, if you’ve taken a strategy that works well for 100 X, but will break for 100,000 X. These don’t need to get fixed at all unless the requirements change, but they help to encode the assumptions that go into the code.

    FIXME: Hey, we’ve just found a bug! We can’t fix it right now (maybe it takes time, maybe we don’t want to be distracted from the story we’re in, maybe we need to get clarification on the correct behaviour). We’ve put it in the bug tracker (and the issue number is next to the FIXME). When it’s time to fix the bug, the marker makes it easy to find.

    Note that these markers are either keeping track of potential problems (but not things that stop me being DONE done), or are linked to stories or issues – making them a form of tagging. You’re right that not keeping track of this backlog isn’t professional, but there’s nothing wrong with putting a marker in – like a bookmark – to go with the backlog. Managing the backlog then falls back to normal product management processes.

    (This would also imply that your markers are visible – e.g. published in your build reports)

    IDEs can work well with this. For example, with Eclipse and the Mylyn extension they bundle in, you can write a TODO (or other marker of your choice) with a comment, then easily create a Task in your TaskRepository (e.g. Jira or Bugzilla) from the Task Marker. Lodging a new issue can be done in seconds without leaving your IDE, and it even flags which markers have no tasks – so you can put the marker in while coding, then as part of the commit process create the new tasks.

    Still, there’s something worse than people putting TODO comments in and not adding to the backlog: the people who don’t even notice that the code isn’t fixed yet.

    Saturday, June 6, 2009 at 12:48 | Permalink
  4. Scot Mcphee wrote:

    I still think that WARNING and FIXME are signs that the development process is broken or at least needs severe improvement. What’s stopping the fixes from being applied immediately? Everything external to the code itself.

    Saturday, June 6, 2009 at 15:11 | Permalink
  5. If //TODO “is a future story yet to be implemented and placed into the backlog”, then is removing the //TODO going to waste someone’s time? I mean, if you know there is a future story in the backlog, then isn’t the proper way to handle the //TODO is to associate it with a story in the backlog, rather than removing it?

    If you remove the //TODO, then the programmer who catches the story in the future is going to have to search for where in the code the change should be implemented. By maintaining the //TODO, you save some time.

    Sunday, June 7, 2009 at 03:54 | Permalink
  6. Scot Mcphee wrote:

    William

    two points; first i’m saying the //TODO shouldn’t be in there – a story should be written.

    second, if the programmer has to search the code for the //TODO in order to find where to make the change, you’ve got a bigger problem with your codebase/design than you think.

    Sunday, June 7, 2009 at 07:48 | Permalink
  7. two points; first i’m saying the //TODO shouldn’t be in there – a story should be written.

    I know. What I’m saying is that the story should be associated with the TODO marker.

    second, if the programmer has to search the code for the //TODO in order to find where to make the change, you’ve got a bigger problem with your codebase/design than you think.

    Never worked on a project larger than around 10k lines, huh? Or on a project with more than two developers?

    Because a human being can only track 7 items give or take 2 in short term memory. (http://www.psych.utoronto.ca/users/peterson/psy430s2001/Miller%20GA%20Magical%20Seven%20Psych%20Review%201955.pdf) Which means no matter how well designed your software platform is, if it grows beyond 7 modules or 7 source files, cognitively even the best programmer is going to have to search for where a change needs to be made.

    Sunday, June 7, 2009 at 09:02 | Permalink
  8. Robert wrote:

    William, having worked with Scot, I can assure he’s worked on projects of >100,000 lines with more than 20 devs.

    Also – if you’re keeping your software design in _short term_ memory, which by nature only lives for about 15-30 minutes, you’ve got bigger problems. I keep my software designs, for large complex systems, in long term memory; the limits are a lot different.

    Scot – the problem you’re describing is more about “how do we track our backlog” and general code hygiene. There are legitimate situations that occur where we don’t want to fix a bug or other issue we’ve just found _right now_. Markers are legitimate tools for tracking these. You do need to keep track of these properly.

    It’s one thing to be working on a story dealing, say, with happy-case user input and putting a marker “STORY-xyz: validation goes here” for the validation story coming up soon.

    It’s a very different thing to come across code that’s been in production for six months and see “TODO: Add error handling to deal with XYZ” – esp. if you’ve just had an XYZ event in production that crashed your system.

    As usual with a lot of Agile concepts, it seems that promoting transparency and accountability will lead to healthier systems.

    Sunday, June 7, 2009 at 09:11 | Permalink
  9. Scot Mcphee wrote:

    William, I’ve worked on some very large projects with multiple teams of 20 developers or more. A well-designed codebase – even a large one – should allow me to find the place I want fairly easily without the use of a TODO. For a start, I can start with the basic architectural diagrams that show me the overall structure of the modules so I can identify which modules are affected.

    Sunday, June 7, 2009 at 09:15 | Permalink
  10. Scot Mcphee wrote:

    Robert, I agree absolutely with this statement: “As usual with a lot of Agile concepts, it seems that promoting transparency and accountability will lead to healthier systems.”

    I agree with your case that in fairly trivial cases they can be somewhat helpful (Why the input validation needs a separate story is another topic for another day!). On the other hand, I think that it can be very difficult to designate where the line gets crossed to the second case.

    I’d contend that perhaps a tool like Mylyn hooked up to an issue tracker like JIRA or Confluence can probably track not only the ‘happy case’ (and wire it straight to the story concerned) but also the nasty bug cases – i.e. instead of just writing a ‘TODO’ you’ve added an actual bug report. Which hopefully will make the problems in the code base visible more widely to the whole team (and to management) and prevent the bug from escaping into the wild.

    Sunday, June 7, 2009 at 09:23 | Permalink
  11. “For a start, I can start with the basic architectural diagrams that show me the overall structure of the modules so I can identify which modules are affected.”

    In other words, you start your search with the architectural diagrams.

    Which was my point. Any code base above a few thousand lines of code will require a search. Note that I didn’t say “grep”, I said “search” and in that traditional meaning of the word search–to find something by looking for it.

    If you remove the ‘//TODO’ marker, then you have to start the search in the architectural diagrams, hoping all the developers actually built their code according to design, and maintained the documentation correctly. Or you can grep for the ‘//TODO’ marker which was documented with the back story.

    Sunday, June 7, 2009 at 09:36 | Permalink
  12. Kevin wrote:

    Ideally, this is a really good idea and developers should take note. Why add a TODO comment, when you can just do it right away and get it over with?

    Back to reality… Working in small shops (less than 5 developers) with extremely short time-lines, limited resources, and juggling multiple projects on a daily basis, it’s a fantasy to expect 100% of the code that will be produced is up to standards. There are certain times when code quality does take a backseat to how fast a deliverable can be produced. Usually this is a sign of poor management.

    Monday, June 8, 2009 at 08:59 | Permalink
  13. Scot Mcphee wrote:

    There are certain times when code quality does take a backseat to how fast a deliverable can be produced. Usually this is a sign of poor management.

    Kevin, yes, the last sentence is the key about poor management. It possible to enlighten them (my commiserations if you’ve tried and failed). In my mind it’s a symptom that the engineering practices of the engineers you’ve employed aren’t valued by their managers. It’s lucky we’re not building bridges or nuclear power stations or anything that could kill someone as a result of its poor engineering I suppose.

    Monday, June 8, 2009 at 09:20 | Permalink
  14. Scot Mcphee wrote:

    There are certain times when code quality does take a backseat to how fast a deliverable can be produced. Usually this is a sign of poor management.

    The last sentence is I think the key one. Management that doesn’t value engineering practices is very frustrating to work with. It’s lucky I guess that we don’t generally make anything like bridges or nuclear power stations where poor engineering can easily kill someone. However I believe this complacency on the part of management needs to be challenged by developers if it’s ever to be overcome.

    Monday, June 8, 2009 at 09:33 | Permalink
  15. Bruce Goldstein wrote:

    I totally agree. When I see lots of //To Dos in a code base I know it is not well maintained. The number of these artifacts always goes up and never down and no one ever fixes them at a future \meeting\ or such as they just get lost. If you need to mark code then simply say in your bug report or story which project, file and line number. This is just as good and keeps the info with the bug which is where it should be. Note that I use java and have read lots of source code and I do not see a lot of //To Do s there.

    Tuesday, June 9, 2009 at 02:22 | Permalink
  16. John Doe wrote:

    Usually TODO means will be done when I’m gone, I was aware of it but I didn’t have time to do it.

    Tuesday, June 9, 2009 at 02:40 | Permalink
  17. Dave wrote:

    There’s nothing broken about leaving a reminder to yourself or another team member. Are there other ways to leave reminders? Sure; create something in an issue tracker, “write a story”, and so on. Those are all external to the code.

    Undisciplined use of “TODO” is bad, just like anything else. But if I’m in flow a “TODO” (or “FIXME” or whatever) captures all I need to know, quickly and efficiently, doesn’t add significant cognitive overhead/disruption, and can be trivially returned to.

    Tuesday, June 9, 2009 at 04:14 | Permalink
  18. Scot Mcphee wrote:

    “I didn’t have time to do it” just mean your environment is deciding that code quality is not important enough.

    “Those are all external to the code” that’s true enough as it is, but they are not external to visibility, and to other members of the team not currently reading the file containing the ‘//TODO’. My point still stands;

    1. real future work due to owner requirement (backlog)

    2. definite refactoring needed (fix it then and there)

    3. bug (fix it then and there, or file a bug).

    4. “enhancement” required later (YAGNI).

    Tuesday, June 9, 2009 at 06:26 | Permalink
  19. Tungano wrote:

    When programming in a flow it can be very interrupting having to stop work and go to an issue tracker or backlog to insert and document the issue.
    The ‘quick notes’ you do there typically never get put in sprint by the customer anyhow since they don’t understand what’s been written.
    Leaving something behind at least lets you stay focused on what the customer -does- find important.
    Typically the bug was already there and should have already -been- handled before running into it (again and again). Often the NOTE or TODO is additional to an bug item already existant somewhere deep down on the backlog, the section that never sees the light of day.
    At least now you leave a sign behind which can be complimentary or help when you -do- try a real effort for the customer to take notice of the ‘under the hood’ issue.

    Wednesday, June 24, 2009 at 23:46 | Permalink
  20. Scot Mcphee wrote:

    “Typically the bug was already there and should have already -been- handled before running into it (again and again)”

    Well, again, I have to say: what’s stopping your from fixing it now or any one of those other times. In other words, at one extreme you’re just accepting a situation that’s second-best, and at the other extreme, you’re collaborating in making the problem worse.

    Wednesday, June 24, 2009 at 23:59 | Permalink
  21. Tungano wrote:

    Not something I am happy with, but where I work it’s a customer decision whether or not to fix a bug. They tend to leave bugs in knowingly, or refuse to accept their existence because it’s not directly visible to them.
    “We roughly loose 0.0001 percent in sales due to this bug, which costs such and such to fix”, conclusion -> it’s not worth it to fix.
    With the stinking code remaining to sit there and considering ‘the code is the documentation’ leaving at least some markers around for the next unwary maintenance developer to run into it, is somewhat like “be aware, unpaved road ahead” or “don’t take a left the next turn or you’ll drop down a big whooping crater”.

    Thursday, June 25, 2009 at 00:18 | Permalink

2 Trackbacks/Pingbacks

  1. Process NAZIs « Development Chaos Theory on Sunday, June 7, 2009 at 06:54

    […] example, today I encountered a complaint that ‘//TODO’ considered harmful–an assertion that if you are putting ‘//TODO’ markers in your code, you’re […]

  2. let x=x › To do redux on Sunday, June 7, 2009 at 08:50

    […] just want to answer the anonymous “process nazis” trackback on yesterday’s ‘//TODO’ Considered Harmful post, because that blog desn’t allow comments without a login. Quite apart from issues with […]