views:

243

answers:

11

Some of the developers on the project I work on have a habit of commenting their code to show which version of the product it was added for, e.g.

// added for superEnterpriseyWonder v2.5
string superMappingTag = MakeTag(extras);
if (superMappingTag.empty())
{
     autoMapping = false;
}
// end added for superEnterpriseyWonder v2.5

Whenever I see this my blood pressure rises, and I have to spend 5 minutes browsing SO to cool off. It seems to me that they don't understand version control and if I were to use this practice too every other line in the source files would be a comment about when things were added. I'm considering removing all such comments from files that I work on, but am wondering is it just me being picky and is there actually some value to these comments?

A: 

Consider that some may have to grab snapshots from the VCS. In that case, they don't have history to fall back on .. and such comments become useful.

Tim Post
When I first joined my company (over 10 years ago) they were using a home-brewed source control system that had no information about history. They had to use comments to show why things changed and that habit was hard to break. I'm happy to say that we now use subversion and even actively look for these comments and remove them from the code base.
Big GH
+8  A: 

If you're using Source Control then I would advocate adding a build label to Source Control after every build. That way you can search for all source modified for a specific build, with no nasty comments clogging your code.

This from Clean Code, a book by Bob Martin:

"The proper use of comments is to compensate for our failure to express ourself in code. Note that I used the word failure. I meant it. Comments are always failures."

I always think of that quote when I see a comment so I'm not suprised your blood boils.

NickGPS
@NickGPS: "Comments are always failures" is a pretty hard line -- I'd treat that as an aspiration rather than a rule! However I agree 100% that Source Control is the answer to this, not comments.
AAT
+6  A: 

No value whatsoever. If you have a blame tool in your version control this will achieve this, they just add noise.

Whats worse is they will attract further comments to your code to make it completely unreadable

// added for superEnterpriseyWonder v2.5
string superMappingTag = MakeTag(extras);
if (superMappingTag.empty())
{
     // bug fix #12345674 shuld have been true
     autoMapping = true;
     // bug fix #12345674 should have been true
     i++; // v2.6 now need to up the counter DO NOT DELETE
}
// end added for superEnterpriseyWonder v2.5

and then someone will delete the method but leave the code comment in

// added for superEnterpriseyWonder v2.5
     // bug fix #12345674 should have been true
     // v2.6 now need to up the counter DO NOT DELETE
// end added for superEnterpriseyWonder v2.5

Just say no to crappy comments

John Nolan
Your modified code actually looks very close to some of the older code that is in the system (10+ yrs of cruft).
danio
A: 

I saw that a lot in code written by people that didn't use version control until recently. I guess they just took the habit and now it's hard to stop.

Another reason I found was that sometime it is important to know what piece of code is associated with what version. Of course you can always check the version log, but you don't always have time for that, and it's annoying. In some cases saying "code for v3.2" speaks more to other developers than "code to do x, y and z"... it all depends on the conventions established by the team.

Another answer is that in some projects I worked with some code was commented like that, but it was before the project actually started using version control. In that case it also made sense to keep it that way.

marcgg
+5  A: 

I'd say there's no value: This info can also be retrieved with your SCM's annotate/blame functionality. Also, anyone can add text between these comments, which make the comments dated (since you might add something for v2.6 while the comments say v2.5)

Another thing to note is that these comments are essentially hidden: You only see them when you are looking at the source code in question, so you can't use it to generate a changelog or something.

Lennaert
+1  A: 

Could be useful in some cases (e.g. if this helps to understand why some function works differently in V3 than in V2) but in general, it's the job of the SCM to know what has been added when.

ammoQ
+4  A: 

The comment as shown, is probably not to useful. However, there may be times that adding a feature may cause the addition of not so obvious code. In which case, a comment describing the change and/or why it the code is not obvious would be appropriate.

simon
+3  A: 

Not only is there no value here...there's negative value. Maintenance of comments is already sketchy in most places, this just adds another thing for people to screw with. These comments have no value to the reader and are therefore clogging their brain up with useless version information when they could have another line of code in their memory. It's also another line to have a merge conflict on (totally not joking here..I've seen merge conflicts on comments).

Jason Punyon
A: 

I find them useful, it saves chasing through the VCS to find out why a change was made to the code, or to find the BugId for a defect, given I remember what the code change was.

Although in theory the VCS contains the information, in practice it can get buried, particularly by integrations.

In other works which is easier:

// DEF43562 - changed default value
foobar = true

Or

  1. blame(or equiv)
  2. chase through history to find the correct change.
  3. Follow integration to source, and repeat 1&2.
  4. Find bug id attached to original change, if you are lucky.

Basically the comment is a short-cut around the VCS, and flaky VCS/BugTracking integration.

I find that the comments are also useful as a marker for: "This decision has been reviewed by customers/users/review committee, and this is the selected answer, be careful about changing the behaviour".

Douglas Leeder
Yes it's true that it is quicker to find a bug number from a comment but then I still have to load up the bug tracking system to find out why. If it's worth commenting I would say it is worth actually explaining in the comment why the default should be true.
danio
I generally have the bug tracker open anyway - YMMV. And you're right - you need to put the reason for the default in the code as well. My example isn't the greatest.
Douglas Leeder
We use TFS for source control. Every single change is linked to a task or a defect, via the checkin. In Visual Studio this is integrated, so you can view the history (annotate) without much effort.
oɔɯǝɹ
+1  A: 

You are not picky IMHO. There are at least three good reasons not to add this type of comment in source code:

  • their place is actually in a Version Control System, where you can have a global view of everything that has changed to accommodate a new version of a library or a new feature. Provided it is done correctly and the logs are used.
  • if the source code is part of the deliverables to clients, maybe they don't need to know the history of what happened. Imagine you have done a modification for another client, and put that in comments!
  • too many comments are no better than too few.

The line is not clear though, what would be the difference between

// Compliance with specs abc (additional xyz feature)
...
... // some code

and:

// xyz feature:
...
... // some code

In general terms, I would not put anything that is related to the history in the source code, but stick to commenting what is done, how it is done, so that someone else can easily browse through the code and understand it.

My advice: have a methodology document written, or an informal discussion.

RedGlyph
+1  A: 

If seeing a superfluos comment makes your blood pressure rise, you need to take up drinking or something.

That said, I agree that such comments are mostly useless. If used consistently, the program would quickly become a maze of such comments. What if a line is changed once for version 2.5 and then a year later changed again for bug 3294? Do you put two "version" comments on the same line, or just keep the latest? If you only keep the latest, then you've lost the fact that this was originally added for 2.5. If you keep them both, what happens when there's a third change or a fourth? How do we know what the state was at each change? What happens when you add a block of code in version 2.5, and then for version 2.6 you add another block of code embedded within the 2.5 block? Etc etc. The program could easily end up having more version comments than lines of code.

If not done consistently, the comments would quickly become misleading. Someone could put a comment saying this block was added for 2.5, someone else could insert code inside that block for version 2.6 and not comment it, and now we have a comment that seems to say that the second block was added for 2.5.

And do we care that much? It's pretty rare that I care when a change was made. Oh, okay a couple of weeks ago I cared because I wanted to know who to blame for a major screw up.

As others have pointed out, version control systems do this for you on the rare occasions when you need it. I guess if you didn't have any sort of VCS, a case could be made for doing this. But you can get some very nice VCSs for free. If you need one, get one. Otherwise you're like the people who say that you should practice doing arithmetic in your head because otherwise what would you do if your calculator quit working. The assumption apparently being that at any moment, all the calculators in the world might simultaneously break.

You might say that it can help to be able to say, "Ohhhh, this was added to order entry to support the new salesman timecard function" or some such. But then the important this is not "This code was changed by Bob for version 3.2.4", but rather, "This code produces this data which isn't used here but is needed by another module over there".

I am a firm believer in writing comments that introduce sections of code and describe the general idea behind complex or otherwise non-obvious code. But that's an entirely different thing.

Jay