#TODOs Never Get To-Done

Over time all software begins to rot. At first a young codebase is small and lean, it’s a pleasure to work on, a single developer may even be able to understand the entire codebase. However, iteration after iteration time constraints and poorly specified features begin to manifest themselves in the codebase. TODOs are everywhere, tests are pending, waiting on unanswered questions related to their pending functionality, half-baked ideas remain commented out so we won’t “lose” them, new developers join the team and contribute to the rot – the broken window theory is in full swing.

Uncle Bob tells us that our goal as developers is “not just working code but clean code” and that we should “always check in code better than you found it”. As professional developers it’s our responsibility to take the initiative in keeping our software maintainable. Let’s take a closer look at some of the above smells.

TODOs

# TODO: Update protected instance variables list

# TODO: Validate that the characters are UTF-8. If they aren't,
# you'll get a weird error down the road, but our form handling
# should really prevent that from happening

# TODO: button_to looks at this ... why?

# TODO: Remove these hacks

Your codebase is not the place for TODOs. The problem with TODOs is that most developers quickly just glance over them, ignoring them because they didn’t write them. Sadly version control often reveals that they’ve been around for quite some time. Is a TODO even still applicable? Was it fixed and someone forgot to remove the TODO? Is there an existing ticket for this TODO?

Move this “necessary” work where it belongs; to your project management/bug tracking tool with the rest of your codebase’s features and bugs. Do the same to FIXMEs, REFACTORs and WTFs.

Pending Tests

describe '.recent' do
  it 'does NOT find admins'
end

describe '#save' do
  it 'normalizes its website URL' do
    pending
  end
end

Every testing tool seems to have the ability to mark a test as pending; RSpec has #pending, Cucumber has @wip, JUnit and NUnit have ignore. These pending tests appear in the output on every test run. Despite this very visible characteristic developers often will let pending tests live on for ages. Test run after test run the same pending tests are output and over time the team learns to live with it.

Pending tests are useful as a kind of TODO list of tests during TDD. And it’s fine to use them while developing a feature in a topic branch but make sure they’re no longer pending when you finish your work and its time to merge it into an integration branch (e.g. master, develop/next, staging, production, etc).

Commented Out Code

def save
  kick = super_save
  # message = { ball => self }.to_json
  # $kicks_machine.publish message
  kick
end

Comments in code are distracting, commented out code is even more distracting. It often sits there for days, weeks, or even months; each passing developer scared to delete it. Even though we’re using version control and we know that every modification made to every file is completely recoverable for some reason we still don’t delete commented out code.

What to Do

Use the following steps to deal with all this cruft, your codebase will thank you for it.

  1. git blame file
  2. Contact the author to discover why
  3. Create a story/ticket for the TODO/pending test/commented out code
  4. Delete the code

Clean Code

Taking the initiative and raising the standard of code cleanliness is contagious. Suddenly your teammates will notice the increase in quality and start to adopt the same techniques. Slowly the codebase improves; there are less distractions, it appears more focused, it starts to look like it was written by developers who take pride in their work. Addressing the above smells are the kind of small changes that individually might not seem like much but over time can make a big difference.

About Jared Carroll

After a short stint in the fashion industry Jared found his true calling at Carbon Five. Yes... he looks like a serial killer in this photo. But really he is as gentle as a flower.
This entry was posted in Process and tagged . Bookmark the permalink.
  • Pingback: #TODOs Never Get To-Done

  • Christian Bradley

    I’d also suggest adding ‘grep todo -ir .’ to the default rake task. Those TODOs being printed out on every run *have* to get annoying after awhile :)

  • Christian Bradley

    By the way, I agree fully with using pending specs as TODOs… the added context helps a ton, and – like grep’ing TODOs in code, these are displayed on every rake as well.

  • Christian Bradley

    I would disagree, however, that comments in code are distracting. If the comments are sparse but placed for readability… I actually find it very helpful for developers to comment blocks of code so that I do not have to run them through my own internal parser.

    Ruby reads much more like the English language than most others, but I still find that when picking up stories it can be very time consuming to scan through lines of code where a single well placed comment would give me the gist.

    Commented-out code has a place within a temporary workflow but never within a master commit. If it’s something you want to know how to do later, create a gist or email it to yourself for God’s sake. I’m with you on that one.

  • Anonymous

    Show me the evidence.

    It’s really simple. There’s already a theory, now show me the evidence. If you can’t do that don’t bother to write a blog post and act like you’re an authority.

    • Michael Wynholds

      What do you mean: “the evidence”? This post is not a theory predicting future behavior, it’s one developer’s opinion of the value of TODOs in the codebase. If you disagree with it, then that’s great – I’d like to hear your opinions as well, and we can have a useful dialog via these comments.

      I am curious, do you think the codebase is the right place for TODOs, as opposed to a project management or bug tracking system? If so, is there a timeframe along which a TODO becomes less useful?

      Give us your opinion.

    • Rob Pak

      # TODO: Gather and show evidence to Anonymous
      # TODO: Act like an authority
      # TODO: Wear sunglasses at night

    • Jared Carroll

      @anonymous – I don’t have any empirical evidence that the above smells cause code rot. However in my experience these smells are exactly the kind of poor code that because they are never addressed, replicate and worsen throughout a codebase.

    • dude

      WHOOSH! Where do you work, Anonymous? We can make sure to avoid it like the plague.

  • Werehamster

    Sorry Anonymous , but I’d have to agree with the original post.

    Surely all you’d have to do is look in your own code and see how many TODOs are present and then use the version control to see how long they have been there. If any of them are more than a month or so old then you know you’ve got a problem because now there’s two places you need to look to get a list of what needs to be done.

    I for one know that there’s a bunch of them in my latest project (a quick check reveals one from 22nd of July), and they should indeed be put into the ticketing system, because I have obviously forgotten about them.

    If you are not using a ticketing system, then I don’t see any problems with just using a todo list inside the code. But the important thing is that the list should be in one place, and one place only.

  • Pafcio00

    //TODO is a great thing to use as a bookmark in code so you wont miss a thing when you must jump to another task leaving something unfinished.
    I use TODO’s sometimes in setters to bookmark those that need additional validation (but I don’t want to write it just yet).
    Thanks to IDE’s those TODO’s are easy to find and it makes work easier sometimes.

    Of course before version release all TODO’s must be removed.

  • http://christianbradley.wordpress.com Christian Bradley
    # TODO: Delete all my TODOs from this codebase
    
  • http://santosh79.tumblr.com Santosh Kumar

    Completely agree, #TODO’s are a sign of the ‘refactor’ step being skipped in red-green-refactor. Contacting devs who wrote the #TODO’s and figuring out if a story needs to be created is a great idea.

    I do like having comments though. I find that there are times, when using local vars to self document steps leads to long’ish variable names and that kind of gets in the way of readability. A couple of comments above a block of code, that has terse local variable names, I feel reads better.

  • http://www.georg-ledermann.de Georg Ledermann

    Nice to read this. I was thinking the same three years ago: https://twitter.com/#!/ledermann/status/877475553

  • http://metaskills.net/ Ken Collins

    Great article. I have never allowed #TODO’s in my large legacy day job app. I even set a rule that they must be cleared out before each release cycle and always be scoped to an issue/ticket. If not, then they must be deleted or transferred to said issue/ticket system.

    On another note, we use #CHANGED a lot. And always scope them to a particular component/gem. So whenever we upgrade a gem dep or library, we checked all the #CHANGED notes to see what we need to re-monkey patch and/or totally remove. A few working examples.

    # CHANGED [Rails3] This monkey patch is no-longer needed.
    # CHANGED [FactoryGirl] Version 2.0.4 should have this fix in issue #140, remove then.