Code Quality – How to Handle a TODO in a Pull Request

code-qualitycode-reviewsmanagementpull-requeststeamwork

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?

Best Answer

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.

Related Topic