27

When I review the changes in a pull request, I sometimes stumble upon a comment with a "TODO" note which may be there for different reasons, in our case mostly because of:

  • the solution used to solve a problem can be improved but would require significantly more time investment. An author chose a quicker solution but put a comment that a better option is potentially available
  • there is a temporary code to workaround an existing bug that should be fixed soon

Knowing that TODOs generally stay in the codebase for the lifetime of the codebase, how should I react to them in a pull request? How can I politely request to avoid it, or if it is really justified, how can I make sure the author of the PR would follow it up later in the future?

3
  • If undone TODOs are becoming a problem for your team, couldn't you assign someone (or, if you lack that authority yourself, ask your boss to assign someone) to spend some time going through all of your code and doing all the TODOs?
    – nick012000
    Commented Dec 17, 2017 at 11:04
  • One important factor is, whether the particular TODO in question is of a nature that can be resolved by developers other than the author. Another factor is to pragmatically assess the risk or relevance of that TODO - is it just crying wolf?
    – rwong
    Commented Dec 17, 2017 at 14:10
  • Having a few TODOs in your code base is no problem at all.
    – Oliver Watkins
    Commented Jan 25, 2019 at 13:34

4 Answers 4

Reset to default
27
+50

When you say that they "generally stay in the codebase for the lifetime of the codebase" in your team/department/organization, consider the following:

  • Write it down in your DoD that TODO, FIXME, or similar tags should be avoided.
  • Use a static code analysis tool such as SonarQube to automatically mark the build unstable.
  • Temporarily allow them if, and only if, there is a corresponding ticket in your issue tracker. Then, the code may look like TODO [ID-123] Description ...

As mentioned in my comment, the last statement probably only makes sense in an environment that doesn't let tickets rot (e.g. if you follow a zero-bug policy).

Personally, I think TODOs are sometimes reasonable, but one should not use them excessively. Taken from Robert C. Martin's "Clean Code: A Handbook of Agile Software Craftsmanship" (p. 59):

TODOs are jobs that the programmer thinks should be done, but for some reason can't do at the moment. It might be a reminder to delete a deprecated feature or a plea for someone else to look at a problem. It might be a request for someone else to think of a better name or a reminder to make a change that is dependent on a planned event. Whatever else a TODO might be, it is not an excuse to leave bad code in the system.

6
  • 2
    Won't the correspnding ticket remain in the backlog forever? I've seen both TODOs and tickets remaining unsolved forever.
    – dzieciou
    Commented Dec 13, 2017 at 8:03
  • 5
    @dzieciou not necessarily, but, of course, there is a risk that it will stay there forever, too. However, if you have a ticket, that risk is probably lower compared to a code comment only. I think it depends on the team/department/organization whether this makes sense.
    – beatngu13
    Commented Dec 13, 2017 at 8:17
  • 13
    If you have a no-todo policy, developers will simply leave the todo comment out of the code and leave the questionable quick and dirty code in. Bad idea.
    – RibaldEddie
    Commented Dec 15, 2017 at 16:39
  • 8
    Todos are a form of communication. Between developers. Sometimes over long stretches of time. The use of the word TODO in a code comment is nothing but syntactic sugar for a particular concern.
    – RibaldEddie
    Commented Dec 15, 2017 at 16:46
  • 2
    I think the mention of adding it to the issue tracker is key here. If you do that, it might even happen that people start fixing it without you knowing about it. I have seen it happen in an organization; issues were picked up just because people liked to fix bugs, refactor code etc. Can be pretty neat sometimes.
    – Per Lundberg
    Commented Dec 16, 2017 at 19:18
5

TODO's them selves are not evil. I am a big fan of never going more then one layer deep, and always fixing 1 issue per tracker ticket.

I often use TODO or FIXME as a way to remind my self that I needed or wanted to come back to fix an issue.

For example, I may call add(a, b) and add a TODO to refactor the add method. I don't want to work on the add method now, but I do want to come back to it.

So in a pull request you will see TODO or FIXME. I use FIXME for example to alert other devs of areas of code that they have responsibility over, that I shouldn't mess with. For example FIXME add can't accept negative numbers.

To get around the problem of them not being seen or being ignored, I use a script that lists out all the TODO FIXME and DEGUG lines. And that gets appended to the commit message.

It's hard to ignore a 400 line commit message that is all TODOs. So people fix them, either while already touching the code in question or by creating new tickets and fixing the problem code standalone.

To be fair, I also make sure that every project has build in code clean up time. Yes be can be ready to launch on the 15th, but were doing code clean up from the 15th to the 30th. (seem odd but if you have ever managed a product, you know that if you try to have low visibility tasks pre-launch, you will never be allowed to get to them. Something else will gain priority.)

4
  • 1
    I agree that TODOs are not evil per se, but using them that often is what I would consider noise. I also think that a 400 line commit message easily gets ignored because people tend to skip that much text. Moreover, many Git/VCS frontends (e.g. GitHub) truncate any subject line longer than a certain number of characters.
    – beatngu13
    Commented Dec 16, 2017 at 19:09
  • Yes that's my point, at around 4-5 lines people tend to want to clean it up. So they start doing TODO. 400 lines was a exaggeration.
    – coteyr
    Commented Dec 16, 2017 at 20:41
  • I would be interested how the script to add the TODOs to the commit message works.
    – Simon
    Commented Dec 21, 2017 at 10:43
  • There is a "commit-msg" hook you can tie in with.
    – coteyr
    Commented Dec 21, 2017 at 12:10
1

It will happen that there are pull requests that are not perfect. Right now I'm doing some work that can be done "good enough" in the available time, but not perfect. So I submit a pull request, I put TODO with comments into the right places in the code, and at the same time I add another change request to change things from "good enough" to "perfect".

This new change request can then be prioritised, and it will be handled when it is the highest priority item. If it is decided that it needs to be perfect right now, then it will be the next thing handed.

Now your question: "How can I politely request to avoid it, or if it is really justified, how can I make sure the author of the PR would follow it up later in the future?" With what I describe, that seems to be rather daft. If I have the choice between shipping late, and shipping what is good enough, then it is not your decision to make. You can ask your product manager about it if you like. And "make sure the author of the PR would follow it up"? If you have a team, then you should ensure that this is logged in your systems, and hopefully your team is well enough organised that logged things don't get lost, and someone will fix it eventually, when there are no higher priority items. Remember, it's a team effort.

1

If your project tracks pending items in source code with TODO comments, then you have to allow it.

The fact that the presence of a TODO comment in the pull request is bugging you should tell you that tracking pending items in source code is a bad idea. Things tend to get lost or ignored that way. Now, if you're talking about a pull request to a "working fork", then the situation is different. A "working fork" is just that -- a work in-progress. But a fork like that doesn't usually require a pull request. The "House Rules" suggested here are for pull requests for the master branch.

House Rule #1 - All commits to master should be ready for testing, since master is built daily after any check-ins. Those commits should also include any additional tests required.

If the TODO comment is there because the code isn't finished, or the tests aren't finished, or the code is in any way not ready for testing, then that code belongs in a local commit, not a pull request. Call me when it's ready.

House Rule #2 - All information regarding open issues is stored in the issue tracker. All of it. Notes, scribbles, hunches, whatever.

If the TODO comment pertains to an open issue, and isn't an actual fix for that issue, then that information belongs in the issue tracker. That way, before an issue is closed, all of the information can be reviewed and verified, if needed, to make sure the issue is actually resolved.

House Rule #3 - All information regarding pending project tasks belongs in the priority queue (or whatever your system's name for that is).

For clarification, a pending project task is something that needs to be done in the project that has a set priority, whether it be a defect that was discovered before it generated an issue ticket, or the implementation of a specific design requirement, or one of that requirement's necessary components.

If the TODO comment is there to say that the new code will impact a pending task, or to point out something else in the codebase that needs looked at which was discovered when implementing the new code, then that information belongs in the priority queue, either on the existing task or as a new one.

House Rule #4 - Suggestions belong in the Idea Box (or whatever).

If the TODO comment is suggesting a change in the design or implementation of the software, then that information belongs in the project's idea box, or "vNext", or "Design Notes", or whatever you have for that sort of thing.

House Rule #5 - TODO comments are not allowed in source code. PERIOD.

If you stick to this rule, then you won't have to worry about anyone following up on anything.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.