How to handle a TODO in a pull request?

Question

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?

Answer 1

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.

Answer 2

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.)

Answer 3

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.

Answer 4

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.