I agree with all except the last; I think "someday" comments are very useful.
If behavior is strange, you want to know if it was expected to be that way, and why. You can use the developer's line of reasoning to prioritize. If you understand the conditions that brought the hack, you are better prepared to maintain the code.
The article suggests that most problems should just be fixed right away, instead of leaving "someday" comments. But, a few weeks of maintaining a large code base will cure anyone of this belief. Not everything can be fixed right away, for many reasons:
- Maybe there isn't time, due to pressure to complete X or Y.
- Maybe it's risky to just hack in the first solution that pops into your head, and you need time to come up with the right fix.
- Code bases can be very complicated. You're not usually working on your own brilliant creations, you're working on things that many other people did, and some of them had no clue. You are tinkering with an engine you didn't design, and you have to be careful.
- The bug might actually be important in the short-term. One of the evil realities of large code bases is that "bugward-compatibility" can matter, and "fixing" something may cause more immediate problems than it solves.
I have to agree, I think TODO and FIXME comments are just a fact of life. I haven't seen a large codebase that didn't have them.
But, if you write one of those, at least try to be realistic. Too often I find comments saying "XXX THIS MUST BE FIXED IMMEDIATELY!!" in code that hasn't changed for years.
TODO's and FIXME's are a fact of life, but if you are using a bug or issue tracker, that is probably a better place to report these. Otherwise you'll have tasks in both your code and your issue tracker instead of a central location.
I worked on an inherited project that had /* FIXME: 8372 */ except we no longer owned the bug database. Really wished the comment actually explained WTF was broken, instead of leaving me perform exhaustive tests with a SOAP sandbox halfway across the world, and a security/performance hack that involved keeping track of a per-request nonce AND a 5s sleep :-/
TODOs and FIXMEs are, at least in my opinion, things that aren't manifesting as bugs right now, but might in the future. Having them in the tracking system will just confuse management and QA. Just put them in the code, which has the advantage of putting the FIXME right next to the thing that needs to be fixed.
They've saved me many times, and I've had exactly the situation I describe above occur, so while I sort of am with you in theory, in practice they should go right in the code.
Not necessarily. Yes, if you're fortunate enough to have something like Trac that can easily link between a line of code and a ticket number, then bug reports can handle everything. But otherwise, how would you describe it? Sometimes a problem only "makes sense" if you're looking directly at the affected line of code, and the comment is right there. If you leave a 'grep'-able thing like "FIXME" in your comment, you won't really lose it.
I find articles filled with contrived examples of what not to do not particularly helpful. In my experience, very, very few people write comments that dense (or at least I have been blessed not to work with them). However, plenty of people write comments nearly as dense, but not quite. Showing those people this article won't change their behavior. They'll rightly think, 'well, my comments aren't that worthless'. Real world examples would do a much better job of showing people what they should really avoid, and might even spark some interesting discussion over the grey areas.
The advice from this post is exactly what you'd expect in theory and exactly what not to do in practice. For one simple reason: the source code is (hopefully) the only thing pretty much guaranteed to survive.
I have seen countless shops where valuable history was lost because it was stored on someone's c: drive, a network drive, or some repository that failed to survive some kind of migration. And even if these other files (digital or paper) did survive, chances are that the programmer that needed to see them never did anyway.
Good shops practice keeping audit trails in the source code. This means good commenting. Which means good code review and quality control.
I recently came across a single piece of code that had been changed back and forth 6 times in the previous 2 years. The comments looked something like:
* jeo 02/11/09 Use Ship Date, not Book Date per Sarah in Sales
* jrm 04/15/09 Use Book Date to make military contracts balance
* msl 08/24/09 Use Ship Date per Joe in Ops (military no longer active)
* jrm 12/13/09 Use Book Date per Rick Smith to prepare for new contracts
* jrm 02/14/10 Use Ship Date per Rick Smith after Ops meeting
* jrm 05/25/10 Use Book Date per Rick Smith until Q3 migration
I know that this is an extreme example, but this stuff happens all the time in commercial environments. How easy do you think it would be for the programmer/analyst to provide background if these comments were not in the source code, but somewhere else?
Sure it's a pain it the ass to maintain this, but it immediately provides the needed background to the person who needs it, when he needs, where he's already working. For critical projects with confused users (what isn't), the alternative is usually much more work.
Hard to tell without context but these read very much version control commit messages, an audit trail nicely maintained by a VCS, along with the code it refers to. I think the main reason you see those sort of 'history of the world' comments is force of habit and the more cumbersome operation of older VC systems.
Unfortunately, companies do occasionally change version control systems. History also tends to get lost when the codebase of one project is imported as the beginning of another.
I don't think 'history tends to get lost'. It happens, both intentionally and unintentionally but preservation of history is a big, often deciding factor in such a switch.
These types of comments are common, I think, not so much because it's so hard to migrate history but because the VCS in use doesn't have facilities for cheap local commit and there's no immediate local history. The workaround is to place important version-change information straight in the source. Which is perfectly sensible in those circumstances, I just disagree with the OP's idea that this is somehow a desirable or best practice.
However, the current version control systems usually have compatibility layers and/or import tools.
We have a project that started wih CVS, then moved to SVN and now runs on Hg, and we have the complete commit log (including commits from the CVS and SVN days) in the current repository.
If you treat the source code history as an essential artifact of the development process, and a piece of the product's history (which it is), then it is complete foolishness to switch systems without bringing the history over.
To address the potential complaint that source control logs aren't as immediately accessible as source code comments, most editors and IDEs can hook into cvs/svn/git/Perforce, and most source control systems can insert the log into the source directly.
If you are putting audit trail in source code, merging changes from one release to another becomes a chore. Since there are no trivial merges anymore. You get conflicts in comments which have to be merged by hand.
Also without ability to diff files, you are never sure what actually changed for a given comment. So you are dependent on VCS anyway! I have been burned by this in two large codebases (> 2k files).
I'm currently maintaining a large project, and I find these sorts of comments frustrating, because "jeo", "jrm" and "msl" resigned years ago, and their initials are meaningless, since nobody remembers who they are. The names of the users who requested changes should be in version control commit messages, or better yet, in the bug tracker. Using programmers to deal with change requests from Sales, Ops and "Rick Smith" also seems very inefficient - you need some sort of analyst role to control changes and reduce this sort of thrashing.
I actually like versions of #1 and #5 on some projects. Revision control usually has very fine granularity, whereas I like to be able to tell at a glance who was mainly responsible for certain portions of code--- that Bob is the main person who wrote file.c, while Sue is the main person behind code.c, and John recently added major functionality to file.c. In-file comments also persist longer, so are particularly nice for open-source code. I've run across code from the 70s and 80s that's long since been separated from its version-control system as it floats around the internet, but the comments in the code still let me know who wrote what, and even a brief summary of its evolution.
And "just fix it" for #5 is ideal, sure, but on any significant project there are likely to be "temporary" workarounds that persist for a while and should be noted (along with who to ask for details), e.g. because they're workarounds for a third-party bug that you're waiting to be fixed, or new functionality hooks waiting on some module that isn't done yet.
I actually setup Textmate to do a completion on # with
# `whoami` | `date +"%Y-%m-%d"` |
I find #1 to be very useful, primarily for the source control granularity issue. The other useful thing about these type of comments is that if you look through the code, and you spot something that looks like a hack job, usually a comment like that indicates it wasn't the original author who did it. If you're lucky, you'll be told why the hack job is in there, if you're not, you can at least begin the process of finding out (or removing if need be).
#2 suffers from a similar invisibility problem in source control. I comment out code when it has the possibility to come back. However, if I'm not the primary author of the file, and a feature has been removed, the HEAD has no indications that the code was ever there. I wouldn't know about restoring it, I'd waste time reimplementing.
I think the author's heart is in the right place, but it seems to me that he either has an encyclopedic knowledge of his team's codebase, or he only works on projects by himself.
Tool support could help here (maybe Xcode 4 with it's timeline feature), where want you want is a button that says "show me all the lines this file used to have" in some sort of ethereal, back from the dead, view. GitHub does this somewhat with Compare View[1], but it would be nice from the editor as well. I'm sure someone will come in and tell me that emacs has had this for 20 years ;)
I use #1 and #5 comments all the time. #1 has nothing to do with being a "proud programmer". It has to do with being able to easily undo changes in a granular fashion. I don't want to have to do a check-in to the version control system every five minutes. I want to do check-ins when I'm at a point where the code is KNOWN to be mostly working, or at the very least it compiles without error. #1 comments, provided they have a little more description than just name and date, make it easier to completely replace the boneheaded hack I put in there when I have an epiphany three hours later about how I really should have done it.
#5 comments are very useful when writing code because they allow you to remain focused on the particular functionality that you are trying to implement while ensuring that you don't ignore things that are important in the grand scheme of things but are not particularly important to do at this moment (and certainly not important enough to be permitted to distract you from focusing on what you are accomplishing right now). TODO comments let you write the skeleton of your code and mark the places where you know you need to come back later to do more mundane tasks like carefully considering what kinds of errors may occur and what sort of exceptions should be thrown or caught or thinking about whether or not something should be written to a log. TODO comments are the bedrock of getting things done while still managing to not forget about details.
However, I agree that #1 and #5 comments should not persist in production code. They are little "under construction" signs that should be removed when the code is solid.
I partially disagree with #4: it's actually a good idea to cite your sources - certainly a better idea than a lot of the other boilerplate commenting I see. If your only source for a business rule is "conversation with Bob in Sales" then say that.
I have to agree. One of the main uses for comments is to explain why code is a certain way, and if it is that way because a specific person requested it, then it makes sense to note that.
With that said, it matters what details go into that type of comments. Putting in why a certain analyst asked for a specific change, along with who that analyst was, can be very helpful. Noting what the analyst was wearing the type of coffee he was drinking at the moment are just cluttering things.
A few weeks ago I was perusing a code base that I had never seen before, consisting of some 100 subprojects, mostly written about a decade ago.
The code was mostly entirely uncommented. In a few files, it was heavily commented, but often strangely... such as allegorical comments relating the code to a magic act ("and now we shall make the volunteer vanish into thin air!"), or quotes from movies that loosely described the code.
Let's see, I've worked where source control was not allowed (yay management). So #1 and #2 were necessary for communication. Also been in situations where non-programmers make small changes to certain pieces of code. Yes, that means you have to explain loops. (#3) I have never seen an example like #4 in the wild and other people have already argued against #5.
Being badly burned by VSS in the past and subsequently viewing all source control to be bad. Also, a massive number of "<project name> - revised <date>" folders was good enough in their eyes.
1, 4, and 5 are all called for in certain circumstances.
I've seen a lot of 2 in shell scripts that aren't under version control, and it's necessary. Though I once slimmed a script I didn't write down from about 1000 to 200 where a lot of that was comments and duplicated functions. (As in, the script would repeatedly define the same function with a different name and a different path hard-coded in, and then call each function in turn.)
The only one that I would say is in a class you legitimately should never do is #3, but no one would ever write that comment except to be flippant.
I'm doing sometimes 1, but rarely - for example I've found my nickname about 5 times in a huge codebase. It's easier to search for it, and gives me back clues - memos - that are even easier to see from P4,svn,git etc. log/changelist. And sometimes that depot is not present, especially if you cooperate with different studio and there is no way to exchange depots (they might even use something else).
So sometimes that's okay.
Number 2 is also sometimes needed. But instead of commenting it like C++, we #ifdef it 0. It's probably wrong, but if you are working in "foreign waters" and you are not sure, old code like this might give some clues (this is assuming the code base is not really that good, and not much documentation either).
3 is awfull, but acceptable for standard libraries, or code that is really trying to be specific about something.
4 is also okay, but not like this - it could be: "// TODO: Jimmie's bsp file (quake bsp) wasn't working due to some new lumps introduced. Temporarily fixed, needs proper one.
Also a coder introduced a TODO macro, that in Visual Studio actually prints out TODO when it's building the app.
Seen 5, and it's reasonable - for example: "Due to short deadline, blah blah blah". Again if possible add the TODO macro, so it's fixed later.
For game development for example - It's almost always more important to ship on time, rather than to have some well commented code. I wish that was the case, but I haven't found a studio where this is really done - maybe engine, sdk, framework, library developers do that.
What does bug me are:
1) Out of date comments. I hate when comments say one thing, but clearly the code has changed and the code now does something else.
2) Comments lacking context. I've seen things like, "exception only occurs when persisted"... what's persisted? I don't see how any exception occurs here. It's until an hour later can I find what they're referring to.
I find it easier to remember to add meaningful comment to commit (we have commit hook that prevents you from commiting without comment), than to remember to change every comment in source code that I touch (especially if it is far from the lines of code I changed - eg in the class header, or in the header of the base class when I only change some method in derived class). Commit messages tells me on each line, why it is here (we have policy of adding JIRA task number to the commit message).
I used to tell people not to comment their code. Mostly as a reaction to articles stating that you should document your code down to every line. "The code should be readable enough to make it clear what goes on", I would say.
Now I just seem to write comments where the reasoning isn't obvious. Which is often, but not that often. And they can be of all the kinds described in this article.
If you have to mention requirements in your comments, don’t mention peoples names. Jim from sales probably moved on from the company and most likely the programmers reading this won’t know who he is. Not to mention the fact that it everything else in the comment is irrelevant.
If this is the best knowledge you have of the requirements, then by all means document it. It should be documented in emails to your boss (along with your failed attempts at getting better requirements,) but adding a comment in the code can also be helpful. Maintenance programmers tend to assume that there's some valid, if depressing, reason the code is the way it is, and it's nice to let them know that you didn't have any good reason for doing it the way you did.
I don't know about others but I have a tendency to end up making something close to comment type 3,ie. obvious comments. Or comments explaining what and not why.
Although it may seem that I'm describing why and not what, a couple of weeks later I would be looking at that piece of code and wondering what's the purpose of that piece of code. :|
This happens quite a bit when pseudo-coding in the code and these comments get left behind. However, sometimes these green comments are good to quickly scan through blocks of code.
I agree about type 5 (the TODO comment) but I also think there is a lot of value in type 4. I would not call it "lifestory" but "business reasoning". Far too often one can visit code and think - I know what it does, but why does the business want that to happen? With comments like that the origin of the request is now obvious. And as we are all being Agile-y now and no-one writes down business requirements anymore, type 4 might just save your ass.
If behavior is strange, you want to know if it was expected to be that way, and why. You can use the developer's line of reasoning to prioritize. If you understand the conditions that brought the hack, you are better prepared to maintain the code.
The article suggests that most problems should just be fixed right away, instead of leaving "someday" comments. But, a few weeks of maintaining a large code base will cure anyone of this belief. Not everything can be fixed right away, for many reasons:
- Maybe there isn't time, due to pressure to complete X or Y.
- Maybe it's risky to just hack in the first solution that pops into your head, and you need time to come up with the right fix.
- Code bases can be very complicated. You're not usually working on your own brilliant creations, you're working on things that many other people did, and some of them had no clue. You are tinkering with an engine you didn't design, and you have to be careful.
- The bug might actually be important in the short-term. One of the evil realities of large code bases is that "bugward-compatibility" can matter, and "fixing" something may cause more immediate problems than it solves.