9 ms·

In my opinion, any comment prefixed by TODO, XXX, or the like should not survive past the review stage. Either fix the problem immediately or accept that it's g

In my opinion, any comment prefixed by TODO, XXX, or the like should not survive past the review stage. Either fix the problem immediately or accept that it's going to be wonky forever. Edit: Amending here to avoid replying to ten different threads individually. 1. Long-term improvements should be managed by a ticket tracking system. The code is not the correct place to manage that. 2. Most of the disagreements below seem to come down to nomenclature differences. In my own work, the code is liberally sprinkled with "Note:"s. These are, as others have pointed out, useful for providing context of what is going on, why, how it is non-optimal, etc. TODOs that I come across tend to be of the "You ain't gonna need it" variety. If you can get a pull through review as-is, then the thing wasn't actually a "TODO", and if it was, then it should be added as a ticket to the backlog. XXXs tend to show up in the reviews I do as code that is explicitly meant to be fixed before the pull is merged. So to summarize: 1. "Note:"s in code are great. 2. "XXX" shouldn't survive the review 3. "TODO" should be handled by the ticketing backlog to better separate out "actually needed" from "theoretically nice, but not necessary in practice". Further, things that are "TODO" today often make zero sense as "TODO" in a year when the code has grown and evolved more, but since someone put it in as a TODO, I find that they rarely ever get removed since "surely someone knows what that means" (but that person is usually either gone or no longer remembers).

connor4312 7y ago
I've been in many scenarios where fixing a certain behavior or implementing something in an optimal way requires a change in an external or upstream product. Or it may require a larger refactoring that's in discussion or otherwise outside the scope of the quick fix. "Either fix the problem immediately or accept that it's going to be wonky forever," sounds like a nice ideal but it's not practical to treat it as an absolute. What I usually do is link the TODO with the associated Github issue/bug tracker (in the commit message too, so it shows up as a reference) to avoid the TODO being forgotten when its blocker is resolved.
- Too 7y ago
  Yep, the todo is there to indicate to future code readers that yes we know this sucks but can't fix it properly yet because of blocker X, so don't try to make a half-assed fix yourself without consulting issue tracker nr ### first. See it as an index into the issue tracker so you don't have to search the issue tracker any time you come across some really strange code. This could of course be done with normal comments also, not necessarily todo.
imposterr 7y ago
Doesn't that just fall under the "don't let perfect be the enemy of good" ethos?
- xscott 7y ago
  I suspect it falls under the "discourage truthful comments" umbrella... Beware of misguided incentives.
- war1025 7y ago
  I amended my comment to hopefully be more clear about the point I intended to convey. And as with all advice, it shouldn't be followed blindly.
juniorplenty 7y ago
Disagree. There are lots of reasons TODOs live on, mostly having to do with stashing ideas that aren’t functionally critical, or pointers for future devs who might inherit the codebase without immediately grokking a performance optimization or corner case that occurred to the previous owner, but wasn’t important enough to deal with at that time. Sometimes, they just save face for the original dev who would love to make something better, but had to move. Black-and-white rules like “No TODOs after review!!” are not only too trivial to enforce for a real production team working on deadline, they remove the soft fuzzy subjective edges that make what we do art, not math.
fogetti 7y ago
I am hesitant to write this, because in general I agree, but let's say a product manager stops a feature short of completed in some sense, even though there are some aspects which would make the feature more robust, secure or optimized, the code gets released and one would need a tombstone as a visible nuisance and indicator for these aspects, wouldn't it? I mean the best place to communicate important ideas and warnings about code, is the code itself isn't it?
- Jach 7y ago
  If you've let a PM stop a feature short of something you as a dev think is rather important, you've already failed to some extent as a professional. There are ways out of such a mess, but it's better to avoid it to begin with, and if it really couldn't be helped, to have a paper trail if not only for yourself then for the benefit of others. Coming to the project as a new hire, I'd find a "TODO[3 years ago]: maybe add a cache for X to speed things up on this flow" comment less illuminating than "We think this will probably start to fall over at some scale point because we're not using DB connections wisely (and maybe want a cache layer) but our PM didn't give us time [or 'we weren't able to stop the PM shipping before we had time'] to perf test and rework the flow that was patterned after all the other DB-conn-happy logic". (Not that either comments are that useful -- guess why I'm reading this part of the code?) But I'm mature enough to think, coming across such code without any of those two comments, that the latter scenario is more likely than "Argh stupid devs, stupid this-dev-in-particular who I can see with git-blame, probably never even thought of a cache! I don't even need to see if there was discussion in the review!" That sort of thought only comes with direct experience dealing with people who repeatedly show themselves as professional failures who should at least retire into management but even better should find a different career field... Comments like "NOTICE: Don't optimize this loop or you'll introduce a timing attack!" are always welcome in my book. Though if whatever you're warning about can be enforced with a unit test, that's even better insurance against someone naively or even idiotically changing it.
  - ilammy 7y ago
    > TODO[3 years ago] The thing is, you might also be not reading that piece of code for the sake of performance optimization. In this case such TODO is a testimony of premature optimization being evil: like, the code has obviously been fine for at least three years without any smartypants optimization that the developer at that time envisioned to become necessary. Though yeah, it could have equally been just a 3-year-old ticket in the issue tracker. However, you won't learn about it when casually exploring the code.
kjs3 7y ago
Serious question as someone who drops XXX occasionally: There are all sort of places where you want to note "there's probably a better way to do this" or "maybe look at this edge case" or whatever. Not all of those are important enough to address in the next review/sprint/whatever...why isn't it better to leave them in and keep that tribal knowledge around rather that delete them and loose that insight? Maybe a better question: is there a better way to collect those sorts of low priority tasks?
- nfoz 7y ago
  I think these sorts of notes belong somewhere external of the code itself; an issue ticketing system or whatnot. Because more often than not, I find those "TODO"s lose meaning over time. They often suggest a particular way to solve something, but by the time we might come back to fixing it, that solution is no longer the best way to do it. A ticket can better express the problem, so that we can evaluate and triage how serious that problem is, and when the time comes to fix it often we'll come up with a better solution than the TODO would have expressed. A FIXME is even more obvious: if something needs to be fixed, then let's track an issue/ticket for it, otherwise noone will ever know to fix it.
  - kjs3 7y ago
    Interesting perspective. I don't per se disagree, but my gut feeling (that is, I have no idea if I'm right) is that the farther away from the source code those notes get the less likely they'll ever get looked at much less addressed. I think your view requires more discipline, which, of course, is what we should be striving for.
  - wutbrodo 7y ago
    > Because more often than not, I find those "TODO"s lose meaning over time. They often suggest a particular way to solve something, but by the time we might come back to fixing it, that solution is no longer the best way to do it. This seems dramatically more likely when tracked anywhere _but_ the code. As usual, implicit couplings are dangerous in engineering, and are far more likely to fall out of sync when they live far away from the code they're describing. When the TODO proposal lives with the code, it's a lot easier for discrepancies between the proposal and the current logic to be caught and fixed, whether during implementation, review, or during later reading of the code. None of these possibilities for keeping the comment in sync with the logic arise organically when the proposed improvement is in a bugtracker somewhere, with no pointer from the logic to said tracker.
namirez 7y ago
> TODO, XXX, or the like should not survive past the review stage I'm afraid if you stick to this principle, you will never have any code ready for review. Using TODO is not an excuse for writing buggy code, but an indication of future improvements and optimizations.
- shantly 7y ago
  It's a great rule if you want to get people to never write down whatever information would have been contained in their TODOs. Anywhere, probably. Best case it remains but just without the TODO label, so... congratulations? Or goes in an issue tracker with all the other junk-issues that no one ever looks at.
jniedrauer 7y ago
Having some way to designate that you're not happy with the code, but it works, is valuable for future refactoring. Another one I see a lot is some variation of "this is a hack." Grepping for "hack" in a codebase can be very entertaining. TODO is more to the point though.
newnewpdro 7y ago
And your kernel would likely never ship, at least not with many drivers anyways. Don't forget Linux is largely a grassroots effort with heaps of reverse-engineered or otherwise improvised/ad-hoc hardware enablement going on. It wasn't until relatively recent history that we could even suspend/resume reliably!
- war1025 7y ago
  Some form of tracking tool is the correct place to document the roadmap, in my opinion. The roadmap can be continuously monitored and updated. TODOs in code just end up as comments that exist for a decade because no one along the way wants to delete them, even if they have no relevance anymore.
  - alacombe 7y ago
    Even though "mainline" has been structuring a tad over the past 5 years or so, Linux has no roadmap, no product management team scheduling features, no nothing. It is the epitome of bazaar development model done over mailing lists... and it's been working pretty well.
    - war1025 7y ago
      > Linux has no roadmap, no product management team scheduling features, no nothing. I would bet large money that Linux has a great many of all three of those. They are just handled in a distributed manner by the various teams working on the different features. Bazaar doesn't mean disorganized. It means there is no central planning authority. There are many individual planning authorities with their own agendas.
  - thu2111 7y ago
    Whereas every issue tracker ever has no abandoned or ignored pile of ancient tickets? Must be a nice place to work! Because I've never seen anything like that before.
geofft 7y ago
I agree with you - specifically because you said "accept that it's going to be wonky forever". The advantage of TODO/XXX/etc. is that you can grep for it. I look for it before committing, vim will syntax-highlight it as an error, etc. There's no point in grepping for every possible improvement someone might have wanted to do at some point in any part of the codebase. It's much more useful immediately. (I use XXX as a marker to myself for "finish doing this before sending out code for review," specifically because it does get highlighted as unusual in vim.) If you've got something you want to do in the future, that's totally fine, just don't put it as a code comment. File it in a bugtracker, or if you don't have a bugtracker, put it in a todo file or something and check it in. That lets you at least slice up the work by area of the code (since there is likely no contributor who is equally well-equipped to fix a TODO in any arbitrary spot in the code and equally interested) and by priority and continued relevance. If you have the luxury/curse of doing this for a paying job with scheduled, paid developers, then proper project management will eventually cut the things you never plan to do (or you can use the backlog to decide to hire more developers). Whether or not you do, it's valuable to look at a proper issue tracker, or even a text file, and say "Hm, this thing is so full of unreached improvements that maybe we should flip out and improve it" vs. "You know, this is working fine, it's worth documenting for posterity if someone comes back to this code, but it's not worth specifically calling attention to." If someone wants to document things about how and why the code was written a certain way - including possible other ways the code could be written, but isn't - a perfectly normal code comment will do the job.
- alacombe 7y ago
  Issues bugtrackers get lost, code stay. I prefer to see a TODO and have a glimpse of what was going on in the dev mind (or even my own mind, 6 months ago) rather than "hide this information for the sake of cleanliness." Just the same way, to a large extend, documentation should be in the code, not in a wiki.
  - geofft 7y ago
    Not for the sake of cleanliness - for the sake of storing the information in a useful format. If you want the bugtracker or documentation to be checked into the same source repository, that seems entirely reasonable to me. I don't know good tools beyond text files for doing this for bugtracking (although I think Fossil does this, kind of, and I'm sad that Simple Defects never took off), though if your work is uncomplicated enough to go into code comments, it's definitely uncomplicated enough to go into a separate todo file, as I mentioned. It's very straightforward to do this for documentation. Probably the right thing is to use your existing patch-contribution workflow for this, but make it lighter-weight for docs (reduced or nonexistent code review), but you can also hook up your docs to something like Ikiwiki if you prefer: being in a wiki and being in the source control repo aren't at odds with each other. And for seeing what was in the dev's mind six months ago - use git log and git blame for that. Even TODOs get refactored away, or moved around enough that they no longer make sense. Good commit messages will show you the whole change that was being made, in context, and what the programmer was thinking when making that change.
NonEUCitizen 7y ago
Let us know when you ship your perfect alternative to Linux.
- war1025 7y ago
  That is an uncharitable interpretation of what I wrote, and I suspect you know that. "Many over a decade old" fits pretty directly with what I wrote. TODOs are where good intentions go to die. They get added and rarely addressed or removed.
wutbrodo 7y ago
What's the advantage of throwing away context that could be useful in the future? I've taken ownership of codebases with technical debt before, and when modifying the code for something time-sensitive, I often annotate issues with the code whose solutions (usually refactoring) can't be fit into the timeline of the current project. I (or others) can then take advantage of these annotations when I have time to dedicate to refactoring, or in many cases, fit the refactoring into feature work. The fact that a portion of the design groundwork has already been done (as captured by the TODO) makes it easier to fit in the cleanup work. IME, this approach leads to much more rapid improvement of legacy codebases than your description of do-it-now-or-forget-about-it.
- sedatk 7y ago
  You don't throw it away, you put it in the issue tracker where it can be prioritized, tracked and categorized. If it's not worth of the issue tracker, it shouldn't be worth of a TODO either.
  - danShumway 7y ago
    Eh. This is a company culture thing that depends on how you use your tracker. At many companies, issue trackers are not used for things like "refactor this method". They're used for user-facing stories or architectural projects, and they're transparent to managers who don't want them cluttered with non-user facing stories. I've run into managers who didn't want any TODOs in code, they wanted everything to go through the tracker. I've also run into managers that told me to stop putting everything into the tracker because they didn't know how to prioritize a random method refactor, and they felt like that information wasn't relevant to their scheduling. I don't have a strong preference, but I lightly lean towards preferring the latter strategy. Issue trackers are slow. They are so slow, and so cumbersome, and so hard to organize, and you waste so much time linking to code files that get refactored or moved around so the context is lost. The nice thing about a TODO in code is if the method gets deleted, the TODO also gets deleted. When you're refactoring code, you don't have to go search an issue tracker and think, "wait, is there anything related to this refactor I need to update?" If documentation is code that never gets compiled, issue trackers are like dynamically linked libraries that never actually get compiled or linked. It's very hard to keep them up-to-date; it's very hard to preserve the references. For smaller, single-person independent projects, I don't use issue trackers. I use high-level todo lists, and all of my notes are in code next to the context they're being used for. This is because an issue tracker is just bloat for those kinds of projects.
yongjik 7y ago
That's a terrible advice. When I worked at Google, many code reviews added more TODOs because the reviewer identified a potential source of problem but also correctly decided that fixing it right there was not the best use of developers' time. When code has potential issues, I want it to be marked with TODO which basically says "The original developer was not an idiot, but was working with limited resource and the best way to fix the issue wasn't clear then."
- rumanator 7y ago
  > That's a terrible advice. When I worked at Google, many code reviews added more TODOs because the reviewer identified a potential source of problem but also correctly decided that fixing it right there was not the best use of developers' time. I'm sure Google is able to adopt a working ticketing system. If you already have an issue tracker then it makes absolutely no sense to keep a separate out-of-band ticketing repository such as source code sprinkled with TODO entries. > When code has potential issues, I want it to be marked with TODO which basically says "The original developer was not an idiot, but was working with limited resource and the best way to fix the issue wasn't clear then." That makes absolutely no sense at all, unless your goal is to simultaneously avoid accountability, poopoo other people's work, and actually do nothing to fix the problem.
  - TeMPOraL 7y ago
    From the point of view of being in the middle of working with some piece of code, it's the issue tracker that's out-of-band! Also, a lot of those TODO entries tend to not be good entries into issue tracker (do you want to track "add extra error checks foobar at line 213 in quux.c"), and they tend to be very localized to the area of code they're left in - which means that, unless your issue tracker supports some way of encoding coordinates in files that survives changes to those files, future people modifying a piece of code are very unlikely to see those notes if they're not left in the code itself.
    - regularfry 7y ago
      The counter-argument would be that if it's work, and it needs to be scheduled (which is what we're talking about here) then it needs to be tracked wherever all the rest of the work is, so that it can be made visible and prioritised alongside everything else. The "how do I encode a link to code that doesn't go stale" solution is to use a link to the source code repo browser which points to a specific time and place - don't point at `master`, point at the specific revision hash you're talking about.
sedatk 7y ago
I agree and don't understand people who downvote you as a punishment for expressing your opinion. A TODO item is missing significant business information: date, priority, severity, risk, impact, applicability etc and leaves all those assessments on developer's shoulders who shouldn't have to deal with it at all. As a matter of fact, every time a developer encounters a TODO around the change he did, it suddenly becomes a burden. He needs to answer "should I be doing this?", "what's the business impact of adopting this?" etc. Even a simple code change can mean business impact in large projects and has regression risks. What if the aforementioned developer isn't at the right caliber for the task, but they don't know it and take it on? Due to the missing information, it's very hard to analyze TODO data and come up with reasonable engineering decisions too. TL;DR: It makes everyone's job harder. I only find TODO's suitable for personal projects where you don't want to go back and forth between IDE and the issue tracker.
- l0b0 7y ago
  Exactly, TODO comments are basically noise. Nobody goes through code explicitly looking for them, and by the next time someone needs to modify that code the TODO is very likely obsolete or just confusing because of how much the rest of the code and circumstances have changed. In code reviews I always encourage replacing TODOs with a ticket or just removing them. I just can't think of any time when a TODO was actually useful.
- IggleSniggle 7y ago
  You have almost converted me from using TODO. I typically place a "Note:" when there's code that is hacky but not obviously so, to provide context. I also make sure to provide a link if some kind to a relevant issue tracker. But a TODO is something different. It's for prototype code, where I don't even know if the feature is going to stick around long enough for the hack to warrant fixing. It says "don't worry about just deleting this whole thing outright and starting fresh." It's graffiti to intentionally make the code uglier, so it's not confused for being part of some larger design. It says, "hey, so, I didn't actually expect this code to survive, but since you're here reading this, it clearly did, you're probably working on something related to it, and you might want to know a few things upfront because it's not going to go the way you think it should go."
im3w1l 7y ago
This may sound weird but I use TODO's to document things I expect to be wonky forever or at least for a very long time. Things I actually intend to fix go in the tracker.
- kaetemi 7y ago
  Yep. Tracker is for stuff that definitely will get planned in. TODO's are just little reminders for when returning to a piece of code some time in the future. For actual immediate tasks, a TODO_123 with the active ticket number is more practical when stubbing some partially written code.
Forge36 7y ago
TODO see issue X is also a great way to link the two together. Is the issue still worth fixing? Well the file was completely rewritten 1 year ago, the TODO removed, and no one requesting this now/pushing for it? Close it. Have an issue but not sure if it's relevant so it stays open forever? I've seen issues outlive the code their for by decades because investigating the lots priority issue wasn't a priority, quick "see: function" references are similar, easily searchable terms which makes old issue review very fast to close. This also ensures the backlog is reviewed periodically
JMTQp8lwXL 7y ago
There are times where there is no review phase. It's a single employee, trying to get something done for their employer in a reasonable amount time, usually limited by a lack of colleagues (not enough engineers) or a rapid delivery timeline. The TODO gets committed, the code is shipped to production, and the TODO comment gets no further attention.
pacifika 7y ago
IMHO TODOS help code quality by informing the reader of the weaknesses in the code and considerations with which the code was developed. As a result if it’s out of scope of the ticket then it’s better to have them left in the code then lower code quality by removing them.
rjsw 7y ago
I can see a TODO from 1999 that could be fixed now. There couldn't have been a plan to fix it back then. I don't run Linux myself, I'm not going to bother looking through some bug tracking system. If someone had deleted the TODO then I think I would be less likely to contact the original author.
iforgotpassword 7y ago
Ticket systems are great and I use them for pretty much everything I do, and places like github make it very easy to use them even for your small hobby projects. However I still think it's valuable to add TODO and the likes to your code, especially when working on open source, since the issue tracker might not be around forever, you switch platforms, project gets forked, some part reused in another project. It also makes it much easier to get into a new code base, just like comments in general. Even at work we do this even though using tickets for everything is mandatory. I like it. And in case we open source some stuff folks won't have access to our internal ticket system obviously, so having something more than "see #4642" in your code is good.
grumple 7y ago
> 1. Long-term improvements should be managed by a ticket tracking system. The code is not the correct place to manage that. Your code will probably outlive your ticketing system. It's also easier to grep for "TODO" than navigate any ticketing system that I've seen. TODO also tells you where the problem exists in code, which is not something that I often see on tickets. Additionally, the non-technical people that handle ticketing within most organizations are unlikely to recognize or prioritize code issues as actual issues. Some organizations might prevent devs from ticketing things themselves as well.

connor4312 7y ago

I've been in many scenarios where fixing a certain behavior or implementing something in an optimal way requires a change in an external or upstream product. Or it may require a larger refactoring that's in discussion or otherwise outside the scope of the quick fix. "Either fix the problem immediately or accept that it's going to be wonky forever," sounds like a nice ideal but it's not practical to treat it as an absolute. What I usually do is link the TODO with the associated Github issue/bug tracker (in the commit message too, so it shows up as a reference) to avoid the TODO being forgotten when its blocker is resolved.

Too 7y ago
Yep, the todo is there to indicate to future code readers that yes we know this sucks but can't fix it properly yet because of blocker X, so don't try to make a half-assed fix yourself without consulting issue tracker nr ### first. See it as an index into the issue tracker so you don't have to search the issue tracker any time you come across some really strange code. This could of course be done with normal comments also, not necessarily todo.

imposterr 7y ago

Doesn't that just fall under the "don't let perfect be the enemy of good" ethos?

xscott 7y ago
I suspect it falls under the "discourage truthful comments" umbrella... Beware of misguided incentives.
war1025 7y ago
I amended my comment to hopefully be more clear about the point I intended to convey. And as with all advice, it shouldn't be followed blindly.

juniorplenty 7y ago

Disagree. There are lots of reasons TODOs live on, mostly having to do with stashing ideas that aren’t functionally critical, or pointers for future devs who might inherit the codebase without immediately grokking a performance optimization or corner case that occurred to the previous owner, but wasn’t important enough to deal with at that time. Sometimes, they just save face for the original dev who would love to make something better, but had to move. Black-and-white rules like “No TODOs after review!!” are not only too trivial to enforce for a real production team working on deadline, they remove the soft fuzzy subjective edges that make what we do art, not math.

fogetti 7y ago

I am hesitant to write this, because in general I agree, but let's say a product manager stops a feature short of completed in some sense, even though there are some aspects which would make the feature more robust, secure or optimized, the code gets released and one would need a tombstone as a visible nuisance and indicator for these aspects, wouldn't it? I mean the best place to communicate important ideas and warnings about code, is the code itself isn't it?

Jach 7y ago
If you've let a PM stop a feature short of something you as a dev think is rather important, you've already failed to some extent as a professional. There are ways out of such a mess, but it's better to avoid it to begin with, and if it really couldn't be helped, to have a paper trail if not only for yourself then for the benefit of others. Coming to the project as a new hire, I'd find a "TODO[3 years ago]: maybe add a cache for X to speed things up on this flow" comment less illuminating than "We think this will probably start to fall over at some scale point because we're not using DB connections wisely (and maybe want a cache layer) but our PM didn't give us time [or 'we weren't able to stop the PM shipping before we had time'] to perf test and rework the flow that was patterned after all the other DB-conn-happy logic". (Not that either comments are that useful -- guess why I'm reading this part of the code?) But I'm mature enough to think, coming across such code without any of those two comments, that the latter scenario is more likely than "Argh stupid devs, stupid this-dev-in-particular who I can see with git-blame, probably never even thought of a cache! I don't even need to see if there was discussion in the review!" That sort of thought only comes with direct experience dealing with people who repeatedly show themselves as professional failures who should at least retire into management but even better should find a different career field... Comments like "NOTICE: Don't optimize this loop or you'll introduce a timing attack!" are always welcome in my book. Though if whatever you're warning about can be enforced with a unit test, that's even better insurance against someone naively or even idiotically changing it.
- ilammy 7y ago
  > TODO[3 years ago] The thing is, you might also be not reading that piece of code for the sake of performance optimization. In this case such TODO is a testimony of premature optimization being evil: like, the code has obviously been fine for at least three years without any smartypants optimization that the developer at that time envisioned to become necessary. Though yeah, it could have equally been just a 3-year-old ticket in the issue tracker. However, you won't learn about it when casually exploring the code.

kjs3 7y ago

Serious question as someone who drops XXX occasionally: There are all sort of places where you want to note "there's probably a better way to do this" or "maybe look at this edge case" or whatever. Not all of those are important enough to address in the next review/sprint/whatever...why isn't it better to leave them in and keep that tribal knowledge around rather that delete them and loose that insight? Maybe a better question: is there a better way to collect those sorts of low priority tasks?

nfoz 7y ago
I think these sorts of notes belong somewhere external of the code itself; an issue ticketing system or whatnot. Because more often than not, I find those "TODO"s lose meaning over time. They often suggest a particular way to solve something, but by the time we might come back to fixing it, that solution is no longer the best way to do it. A ticket can better express the problem, so that we can evaluate and triage how serious that problem is, and when the time comes to fix it often we'll come up with a better solution than the TODO would have expressed. A FIXME is even more obvious: if something needs to be fixed, then let's track an issue/ticket for it, otherwise noone will ever know to fix it.
- kjs3 7y ago
  Interesting perspective. I don't per se disagree, but my gut feeling (that is, I have no idea if I'm right) is that the farther away from the source code those notes get the less likely they'll ever get looked at much less addressed. I think your view requires more discipline, which, of course, is what we should be striving for.
- wutbrodo 7y ago
  > Because more often than not, I find those "TODO"s lose meaning over time. They often suggest a particular way to solve something, but by the time we might come back to fixing it, that solution is no longer the best way to do it. This seems dramatically more likely when tracked anywhere _but_ the code. As usual, implicit couplings are dangerous in engineering, and are far more likely to fall out of sync when they live far away from the code they're describing. When the TODO proposal lives with the code, it's a lot easier for discrepancies between the proposal and the current logic to be caught and fixed, whether during implementation, review, or during later reading of the code. None of these possibilities for keeping the comment in sync with the logic arise organically when the proposed improvement is in a bugtracker somewhere, with no pointer from the logic to said tracker.

namirez 7y ago

> TODO, XXX, or the like should not survive past the review stage I'm afraid if you stick to this principle, you will never have any code ready for review. Using TODO is not an excuse for writing buggy code, but an indication of future improvements and optimizations.

shantly 7y ago
It's a great rule if you want to get people to never write down whatever information would have been contained in their TODOs. Anywhere, probably. Best case it remains but just without the TODO label, so... congratulations? Or goes in an issue tracker with all the other junk-issues that no one ever looks at.

jniedrauer 7y ago

Having some way to designate that you're not happy with the code, but it works, is valuable for future refactoring. Another one I see a lot is some variation of "this is a hack." Grepping for "hack" in a codebase can be very entertaining. TODO is more to the point though.

newnewpdro 7y ago

And your kernel would likely never ship, at least not with many drivers anyways. Don't forget Linux is largely a grassroots effort with heaps of reverse-engineered or otherwise improvised/ad-hoc hardware enablement going on. It wasn't until relatively recent history that we could even suspend/resume reliably!

war1025 7y ago
Some form of tracking tool is the correct place to document the roadmap, in my opinion. The roadmap can be continuously monitored and updated. TODOs in code just end up as comments that exist for a decade because no one along the way wants to delete them, even if they have no relevance anymore.
- alacombe 7y ago
  Even though "mainline" has been structuring a tad over the past 5 years or so, Linux has no roadmap, no product management team scheduling features, no nothing. It is the epitome of bazaar development model done over mailing lists... and it's been working pretty well.
  - war1025 7y ago
    > Linux has no roadmap, no product management team scheduling features, no nothing. I would bet large money that Linux has a great many of all three of those. They are just handled in a distributed manner by the various teams working on the different features. Bazaar doesn't mean disorganized. It means there is no central planning authority. There are many individual planning authorities with their own agendas.
- thu2111 7y ago
  Whereas every issue tracker ever has no abandoned or ignored pile of ancient tickets? Must be a nice place to work! Because I've never seen anything like that before.

geofft 7y ago

I agree with you - specifically because you said "accept that it's going to be wonky forever". The advantage of TODO/XXX/etc. is that you can grep for it. I look for it before committing, vim will syntax-highlight it as an error, etc. There's no point in grepping for every possible improvement someone might have wanted to do at some point in any part of the codebase. It's much more useful immediately. (I use XXX as a marker to myself for "finish doing this before sending out code for review," specifically because it does get highlighted as unusual in vim.) If you've got something you want to do in the future, that's totally fine, just don't put it as a code comment. File it in a bugtracker, or if you don't have a bugtracker, put it in a todo file or something and check it in. That lets you at least slice up the work by area of the code (since there is likely no contributor who is equally well-equipped to fix a TODO in any arbitrary spot in the code and equally interested) and by priority and continued relevance. If you have the luxury/curse of doing this for a paying job with scheduled, paid developers, then proper project management will eventually cut the things you never plan to do (or you can use the backlog to decide to hire more developers). Whether or not you do, it's valuable to look at a proper issue tracker, or even a text file, and say "Hm, this thing is so full of unreached improvements that maybe we should flip out and improve it" vs. "You know, this is working fine, it's worth documenting for posterity if someone comes back to this code, but it's not worth specifically calling attention to." If someone wants to document things about how and why the code was written a certain way - including possible other ways the code could be written, but isn't - a perfectly normal code comment will do the job.

alacombe 7y ago
Issues bugtrackers get lost, code stay. I prefer to see a TODO and have a glimpse of what was going on in the dev mind (or even my own mind, 6 months ago) rather than "hide this information for the sake of cleanliness." Just the same way, to a large extend, documentation should be in the code, not in a wiki.
- geofft 7y ago
  Not for the sake of cleanliness - for the sake of storing the information in a useful format. If you want the bugtracker or documentation to be checked into the same source repository, that seems entirely reasonable to me. I don't know good tools beyond text files for doing this for bugtracking (although I think Fossil does this, kind of, and I'm sad that Simple Defects never took off), though if your work is uncomplicated enough to go into code comments, it's definitely uncomplicated enough to go into a separate todo file, as I mentioned. It's very straightforward to do this for documentation. Probably the right thing is to use your existing patch-contribution workflow for this, but make it lighter-weight for docs (reduced or nonexistent code review), but you can also hook up your docs to something like Ikiwiki if you prefer: being in a wiki and being in the source control repo aren't at odds with each other. And for seeing what was in the dev's mind six months ago - use git log and git blame for that. Even TODOs get refactored away, or moved around enough that they no longer make sense. Good commit messages will show you the whole change that was being made, in context, and what the programmer was thinking when making that change.

NonEUCitizen 7y ago

Let us know when you ship your perfect alternative to Linux.

war1025 7y ago
That is an uncharitable interpretation of what I wrote, and I suspect you know that. "Many over a decade old" fits pretty directly with what I wrote. TODOs are where good intentions go to die. They get added and rarely addressed or removed.

wutbrodo 7y ago

What's the advantage of throwing away context that could be useful in the future? I've taken ownership of codebases with technical debt before, and when modifying the code for something time-sensitive, I often annotate issues with the code whose solutions (usually refactoring) can't be fit into the timeline of the current project. I (or others) can then take advantage of these annotations when I have time to dedicate to refactoring, or in many cases, fit the refactoring into feature work. The fact that a portion of the design groundwork has already been done (as captured by the TODO) makes it easier to fit in the cleanup work. IME, this approach leads to much more rapid improvement of legacy codebases than your description of do-it-now-or-forget-about-it.

sedatk 7y ago
You don't throw it away, you put it in the issue tracker where it can be prioritized, tracked and categorized. If it's not worth of the issue tracker, it shouldn't be worth of a TODO either.
- danShumway 7y ago
  Eh. This is a company culture thing that depends on how you use your tracker. At many companies, issue trackers are not used for things like "refactor this method". They're used for user-facing stories or architectural projects, and they're transparent to managers who don't want them cluttered with non-user facing stories. I've run into managers who didn't want any TODOs in code, they wanted everything to go through the tracker. I've also run into managers that told me to stop putting everything into the tracker because they didn't know how to prioritize a random method refactor, and they felt like that information wasn't relevant to their scheduling. I don't have a strong preference, but I lightly lean towards preferring the latter strategy. Issue trackers are slow. They are so slow, and so cumbersome, and so hard to organize, and you waste so much time linking to code files that get refactored or moved around so the context is lost. The nice thing about a TODO in code is if the method gets deleted, the TODO also gets deleted. When you're refactoring code, you don't have to go search an issue tracker and think, "wait, is there anything related to this refactor I need to update?" If documentation is code that never gets compiled, issue trackers are like dynamically linked libraries that never actually get compiled or linked. It's very hard to keep them up-to-date; it's very hard to preserve the references. For smaller, single-person independent projects, I don't use issue trackers. I use high-level todo lists, and all of my notes are in code next to the context they're being used for. This is because an issue tracker is just bloat for those kinds of projects.

yongjik 7y ago

That's a terrible advice. When I worked at Google, many code reviews added more TODOs because the reviewer identified a potential source of problem but also correctly decided that fixing it right there was not the best use of developers' time. When code has potential issues, I want it to be marked with TODO which basically says "The original developer was not an idiot, but was working with limited resource and the best way to fix the issue wasn't clear then."

rumanator 7y ago
> That's a terrible advice. When I worked at Google, many code reviews added more TODOs because the reviewer identified a potential source of problem but also correctly decided that fixing it right there was not the best use of developers' time. I'm sure Google is able to adopt a working ticketing system. If you already have an issue tracker then it makes absolutely no sense to keep a separate out-of-band ticketing repository such as source code sprinkled with TODO entries. > When code has potential issues, I want it to be marked with TODO which basically says "The original developer was not an idiot, but was working with limited resource and the best way to fix the issue wasn't clear then." That makes absolutely no sense at all, unless your goal is to simultaneously avoid accountability, poopoo other people's work, and actually do nothing to fix the problem.
- TeMPOraL 7y ago
  From the point of view of being in the middle of working with some piece of code, it's the issue tracker that's out-of-band! Also, a lot of those TODO entries tend to not be good entries into issue tracker (do you want to track "add extra error checks foobar at line 213 in quux.c"), and they tend to be very localized to the area of code they're left in - which means that, unless your issue tracker supports some way of encoding coordinates in files that survives changes to those files, future people modifying a piece of code are very unlikely to see those notes if they're not left in the code itself.
  - regularfry 7y ago
    The counter-argument would be that if it's work, and it needs to be scheduled (which is what we're talking about here) then it needs to be tracked wherever all the rest of the work is, so that it can be made visible and prioritised alongside everything else. The "how do I encode a link to code that doesn't go stale" solution is to use a link to the source code repo browser which points to a specific time and place - don't point at `master`, point at the specific revision hash you're talking about.

sedatk 7y ago

I agree and don't understand people who downvote you as a punishment for expressing your opinion. A TODO item is missing significant business information: date, priority, severity, risk, impact, applicability etc and leaves all those assessments on developer's shoulders who shouldn't have to deal with it at all. As a matter of fact, every time a developer encounters a TODO around the change he did, it suddenly becomes a burden. He needs to answer "should I be doing this?", "what's the business impact of adopting this?" etc. Even a simple code change can mean business impact in large projects and has regression risks. What if the aforementioned developer isn't at the right caliber for the task, but they don't know it and take it on? Due to the missing information, it's very hard to analyze TODO data and come up with reasonable engineering decisions too. TL;DR: It makes everyone's job harder. I only find TODO's suitable for personal projects where you don't want to go back and forth between IDE and the issue tracker.

l0b0 7y ago
Exactly, TODO comments are basically noise. Nobody goes through code explicitly looking for them, and by the next time someone needs to modify that code the TODO is very likely obsolete or just confusing because of how much the rest of the code and circumstances have changed. In code reviews I always encourage replacing TODOs with a ticket or just removing them. I just can't think of any time when a TODO was actually useful.
IggleSniggle 7y ago
You have almost converted me from using TODO. I typically place a "Note:" when there's code that is hacky but not obviously so, to provide context. I also make sure to provide a link if some kind to a relevant issue tracker. But a TODO is something different. It's for prototype code, where I don't even know if the feature is going to stick around long enough for the hack to warrant fixing. It says "don't worry about just deleting this whole thing outright and starting fresh." It's graffiti to intentionally make the code uglier, so it's not confused for being part of some larger design. It says, "hey, so, I didn't actually expect this code to survive, but since you're here reading this, it clearly did, you're probably working on something related to it, and you might want to know a few things upfront because it's not going to go the way you think it should go."

im3w1l 7y ago

This may sound weird but I use TODO's to document things I expect to be wonky forever or at least for a very long time. Things I actually intend to fix go in the tracker.

kaetemi 7y ago
Yep. Tracker is for stuff that definitely will get planned in. TODO's are just little reminders for when returning to a piece of code some time in the future. For actual immediate tasks, a TODO_123 with the active ticket number is more practical when stubbing some partially written code.

Forge36 7y ago

TODO see issue X is also a great way to link the two together. Is the issue still worth fixing? Well the file was completely rewritten 1 year ago, the TODO removed, and no one requesting this now/pushing for it? Close it. Have an issue but not sure if it's relevant so it stays open forever? I've seen issues outlive the code their for by decades because investigating the lots priority issue wasn't a priority, quick "see: function" references are similar, easily searchable terms which makes old issue review very fast to close. This also ensures the backlog is reviewed periodically

JMTQp8lwXL 7y ago

There are times where there is no review phase. It's a single employee, trying to get something done for their employer in a reasonable amount time, usually limited by a lack of colleagues (not enough engineers) or a rapid delivery timeline. The TODO gets committed, the code is shipped to production, and the TODO comment gets no further attention.

pacifika 7y ago

IMHO TODOS help code quality by informing the reader of the weaknesses in the code and considerations with which the code was developed. As a result if it’s out of scope of the ticket then it’s better to have them left in the code then lower code quality by removing them.

rjsw 7y ago

I can see a TODO from 1999 that could be fixed now. There couldn't have been a plan to fix it back then. I don't run Linux myself, I'm not going to bother looking through some bug tracking system. If someone had deleted the TODO then I think I would be less likely to contact the original author.

iforgotpassword 7y ago

Ticket systems are great and I use them for pretty much everything I do, and places like github make it very easy to use them even for your small hobby projects. However I still think it's valuable to add TODO and the likes to your code, especially when working on open source, since the issue tracker might not be around forever, you switch platforms, project gets forked, some part reused in another project. It also makes it much easier to get into a new code base, just like comments in general. Even at work we do this even though using tickets for everything is mandatory. I like it. And in case we open source some stuff folks won't have access to our internal ticket system obviously, so having something more than "see #4642" in your code is good.

grumple 7y ago

> 1. Long-term improvements should be managed by a ticket tracking system. The code is not the correct place to manage that. Your code will probably outlive your ticketing system. It's also easier to grep for "TODO" than navigate any ticketing system that I've seen. TODO also tells you where the problem exists in code, which is not something that I often see on tickets. Additionally, the non-technical people that handle ticketing within most organizations are unlikely to recognize or prioritize code issues as actual issues. Some organizations might prevent devs from ticketing things themselves as well.