17

The senior dev in our shop insists that whenever code is modified, the programmer responsible should add an inline comment stating what he did. These comments usually look like // YYYY-MM-DD <User ID> Added this IF block per bug 1234.

We use TFS for revision control, and it seems to me that comments of this sort are much more appropriate as check-in notes rather than inline noise. TFS even allows you to associate a check-in with one or more bugs. Some of our older, often-modified class files look like they have a comment-to-LOC ratio approaching 1:1. To my eyes, these comments make the code harder to read and add zero value.

Is this a standard (or at least common) practice in other shops?

Improve this question
3
  • 12
    I would agree with you, that's what revision logs in version control are for.
    – TZHX
    Commented Feb 16, 2011 at 16:27
  • 1
    The senior dev on your team did this before version control was wide-spread, and hasn't come to the realization that it belongs somewhere else to remove code smell. Show him six open-source projects that use a bugzilla and vc system, ask him if he needed to know in the comments of the jQuery Library that the $.ajax() was recently changed by jaubourg 4 months ago, and all the minor changes made by hundreds of people belong in there too. The whole jQuery library would be a mess of comments, and nothing was gained!
    – Incognito
    Commented Feb 24, 2011 at 14:29
  • 1
    We actually have an unwritten policy of no names in source code as it can create a sense of code-ownership which is considered a BadThing TM.
    – Stephen Paulger
    Commented Mar 3, 2011 at 13:51

8 Answers 8

Reset to default
23

I usually consider such comments a bad practice and I think this kind of information belongs to the SCM commit logs. It just makes the code harder to read in most cases.

However, I still often do something like this for specific types of edits.

Case 1 - Tasks

If you use an IDE like Eclipse, Netbeans, Visual Studio (or have some way of doing text searches on your codebase with anything else), maybe your team uses some specific "comment tags" or "task tags". In which case this can be useful.

I would from time to time, when reviewing code, add something like the following:

// TOREVIEW: [2010-12-09 haylem] marking this for review because blablabla

or:

// FIXME: [2010-12-09 haylem] marking this for review because blablabla

I use different custom task tags that I can see in Eclipse in the task view for this, because having something in the commit logs is a good thing but not enough when you have an executive asking you in a review meeting why bugfix XY was completely forgotten and slipped through. So on urgent matters or really questionable pieces of code, this serves as an additional reminder (but usually I'll keep the comment short and check the commit logs because THAT's what the reminder is here for, so I don't clutter the code too much).

Case 2 - 3rd-Party Libs' Patches

If my product needs to package a 3rd party piece of code as source (or library, but re-built from source) because it needed to be patched for some reason, we document the patch in a separate document where we list those "caveats" for future reference, and the source code will usually contain a comment similar to:

// [PATCH_START:product_name]
//  ... real code here ...
// [PATCH_END:product_name]

Case 3 - Non-Obvious Fixes

This one is a bit more controversial and closer to what your senior dev is asking for.

In the product I work on at the moment, we sometimes (definitely not a common thing) have a comment like:

// BUGFIX: [2010-12-09 haylem] fix for BUG_ID-XYZ

We only do this if the bugfix is non-obvious and the code reads abnormally. This can be the case for browser quirks for instance, or obscure CSS fixes that you need to implement only because there's a document bug in a product. So in general we'd link it to our internal issue repository, which will then contain the detailed reasoning behind the bugfix and pointers to the documentation of the external product's bug (say, a security advisory for a well known Internet Explorer 6 defect, or something like that).

But as mentioned, it's quite rare. And thanks to the task tags, we can regularly run through these and check if these weird fixes still make sense or can be phased out (for instance, if we dropped support for the buggy product causing the bug in the first place).

This just in: A real life example

In some cases, it's better than nothing :)

I just came across a huge statistical computation class in my codebase, where the header comment was in the form of a changelog with the usual yadda yadda: reviewer, date, bug ID.

At first I thought of scrapping but I noticed the bug IDs did not only not match the convention of our current issue tracker but neither did they match the one of the tracker used before I joined the company. So I tried to read through the code and get an understanding of what the class was doing (not being a statistician) and also tried to dig up these defect reports. As it happens they were fairly important and would have maed the life of the next guy to edit the file without knowing about them quite horrible, as it dealt with minor precision issues and special cases based on very specific requirements emitted by the originating customer back then. Bottom line, if these had not been in there, I wouldn't have known. If they hadn't been in there AND I had had a better understanding of the class, I would have noticed that some computations were off and broken them by "fixing" them.

Sometimes it's hard to keep track of very old requirements like these. In the end what I did was still remove the header, but after sneaking in a block comment before each incriminating function describing why these "weird" computations as they are specific requests.

So in that case I still considered these a bad practice, but boy was I happy the original dev did at least put them in! Would have been better to comment the code clearly instead, but I guess that was better than nothing.

Improve this answer
6
  • These situations make sense. Thanks for the input.
    – Joshua Smith
    Commented Feb 16, 2011 at 17:34
  • the second case is an ordinary code comment why there is a patch in the code. the first case is a valid one: just a remark that the code isn't finish or there is some more work to do on that part.
    – Salandur
    Commented Feb 16, 2011 at 18:06
  • Well the senior developer at my place of work (me) says changes go in the commit comments of your software configuration management system . You do have your SCM and defect trackers hooked together , right ? (google SCMBUG) Don't do manually what can be automated.
    – Tim Williscroft
    Commented Feb 16, 2011 at 20:09
  • @Tim: Well, I agree with you, as says the first line of the answer. But in non-obvious cases, programmers (out of laziness, I assume, as they don't want to waste time on it) won't check the commit logs and will miss some key info, while a 10char comment can point them to the right bug ID which will itself contain the logs of the revisions pertaining to that bug if you did set up your SCM and tracker to work together. Best of all the worlds, really.
    – haylem
    Commented Feb 17, 2011 at 0:38
  • In my experience, commit messages are often omitted (despite policies to include them, if those exist at all) or are useless (just an issue number, and often the wrong one). Those same people however will leave comments in code... I'd rather have those and no commit message than nothing at all (and in many environments I've worked getting at the commit messages/source history was so hard it was almost impossible, up to and including having to request sources from other people because access to version control was limited "for security reasons").
    – jwenting
    Commented Feb 24, 2011 at 10:30
3

We use that practice for files not under version control. We have RPG programs that run on a mainframe, and it has proven difficult to do much more than back them up.

For versioned source code, we use the check-in notes. I think that's where they belong, not cluttering up code. It's metadata, after all.

Improve this answer
4
  • I agree, files not under version control are a different story.
    – Joshua Smith
    Commented Feb 16, 2011 at 16:36
  • 1
    "proven difficult to do much more than back them up." Is not really an excuse my CTO would stomach.
    – Tim Williscroft
    Commented Feb 16, 2011 at 20:09
  • @Tim: Always open to suggestions - I can kick them up the chain of command. :) The mainframe is hard to work with to get files off and on of.
    – Michael K
    Commented Feb 16, 2011 at 20:12
  • My suggestion -- presumably you can get them off (backup); why not just dump that somewhere and have a little script push changes to mercurial every night?
    – Wyatt Barnett
    Commented Feb 16, 2011 at 20:41
2

We used to have this practice a long ago in a SW shop where our manager insisted that he wants to be able to read code files on paper and follow their revision history. Needless to say, I don't remember any concrete occasion when he actually looked at a source file printout :-/

Luckily, my managers since then have been more knowledgeable about what version control systems can do (I have to add that we used SourceSafe in that first shop). So I don't add versioning metadata into the code.

Improve this answer
2

It's generally not required if your SCM and IDE allows you to use "show annotation" (called this in Eclipse anyway) feature you can easily see what commit(s) changed a piece of code, and the commit comments should tell who and why.

The only time I'd put a comment like this in the code is if it's a particularly strange piece of code that may cause someone confusion in future, I'll briefly comment with the bug number so they can go to the bug report and read in detail about it.

Improve this answer
1
  • "you are not expected to understand this" is probably a bad comment to put in the code. Again, I'll say link your SCM and bug trackers together.
    – Tim Williscroft
    Commented Feb 16, 2011 at 20:11
2

Obviously, any such comments are almost guaranteed to be incorrect over time; if you edit a line twice, do you add two comments? Where do they go? So a better process is to rely on your tools.

This is a sign that, for whatever reason, the senior dev cannot rely on the new process for tracking changes and connecting changes to the issue tracking system. You must address this discomfort to resolve the conflict.

You need to understand why he cannot rely on it. Is it old habits? A bad experience with the SCM system? A presentation format that doesn't work for him? Maybe he doesn't even know about tools like 'git blame' and Perforce's timeline view (which essentially shows this, though it might not show what issue triggered the change).

Without understanding his reason for the requirement, you will not be able to convince him.

Improve this answer
2

I work on a 20+ year old Windows product. We have a similar practice that has been in place for a long time. I cannot count the number of times this practice has saved my bacon.

I do agree that some stuff is redundant. It used to be developers used this practice to comment out code. When I review this, I tell them to go ahead and delete the code, and the comment isn't necessary.

But, at lot of times, you get to look at code that has been around a decade and modified by 20 developers but never refactored. Everyone has long ago forgotten all the requirement details that were in the original code, never mind all the changes, and there is no reliable documentation.

A comment with a defect number gives me a place to go to look up the origin of code.

So, yeah, the SCM system does a lot, but not everything. Treat these as you would any other comment, and add them wherever a significant change is made. The developer looking at your code 5+ years from how will thank you.

Improve this answer
1

Mentioning every change in comments is pointless if you have a VCS. Mentioning a specific bug or other important note related to specific line is useful. A change may include many altered lines, all under the same commit message.

Improve this answer
1

Not that I can tell.

I do splatter TODOs in my code as I go along, and the goal is to have them removed by review time.

Improve this answer

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.