44

We have been asked to add comments with start tags, end tags, description, solution etc for each change that we make to the code as part of fixing a bug / implementing a CR.

My concern is, does this provide any added value? As it is, we have all the details in the Version control history, which will help us to track each and every change?

But my leads are insisting on having the comments as a "good" programming practice. One of their argument is when a CR has to be de-scoped/changed, it would be cumbersome if comments are not there.

Considering that the changes would be largely in between code, would it really help to add comments for each and every change we make? Shouldn't we leave it to the version control?

Improve this question
0

9 Answers 9

Reset to default
43

You are absolutely right. Tracking changes is the job for your version control system. Every time you do a commit you should write a commit message explaining what was done, and referencing your bug-tracking system if this is a bug fix. Putting a comment in the code saying 

// begin fix for bug XXXXX on 10/9/2012
...
// end fix for bug XXXXX

every time you fix a bug will quickly render your code unreadable and unmaintainable. It will also result in duplicating the same information in two places, which will make the mess even worse.

Comments should not be used for bug tracking and they also should not describe what your code is doing. They should explain why you are doing X, or why you are doing X in this particular way. If you feel the need to write a comment explaining what a block of code is doing, that is a code smell which indicates that you should refactor this block into a function with a descriptive name.

So instead of

// fixed bug XXXXX on 10/9/2012

you might have a comment that says

// doing X, because otherwise Y will break.

or 

// doing X, because doing Y is 10 times slower.
Improve this answer
1
  • 12
    +1 for the code smell of comments that explain "what". It's nice to see a response that code comments are not an automatic benefit in the sense that more comments > fewer comments. I might even pull back a level and think that there are cases where even comments describing "why" might be a smell indicating that the code is not clear. For instance, if I can inject a BubbleSorter or a QuickSorter, the comment "I'm using QuickSorter because it's faster" is superfluous in the same way that "inject a quicksorter" is superfluous. YMMV.
    – Erik Dietrich
    Nov 9, 2012 at 21:02
53

Use the best tool for the job. Your version control system should be the best tool for recording when bugfixes and CRs are made: it automatically records the date and who made the change; it never forgets to add a message (if you've configured it to require commit messages); it never annotates the wrong line of code or accidentally deletes a comment. And if your version control system is already doing a better job than your comments, it's silly to duplicate work by adding comments.

Readability of source code is paramount. A codebase that's cluttered with comments giving the full history of every bugfix and CR made is going to not be very readable at all.

But don't skip comments completely: Good comments (not slavishly documenting every start / stop / description / solution of every bugfix and CR) enhance the readability of the code. For example, for a tricky or unclear bit of code that you add to fix a bug, a comment of the form // fix ISSUE#413 telling people where to find more information in your issue tracker is an excellent idea.

Improve this answer
5
  • 30
    I agree except for one thing: fix ISSUE#413 is not a good comment in code. You should be able to understand the code without having to refer to external documentation. Instead of giving a random number, actually explain why this tricky part of code is required to do what. That's what comments are for: To explain those parts of code that are not obvious.
    – poke
    Nov 9, 2012 at 18:53
  • 12
    @poke - Thank you for pointing that out. I guess I should add that the only place I use comments of the form fix ISSUE#413 is where the issue is so complicated (an extremely OS- and configuration-dependent corner case, or only triggered by particular bad customer data) that adequately describing it would take a couple of paragraphs; that sort of thing is better handled by an issue tracker, IMO. Even then, some sort of brief description is good.
    – Josh Kelley
    Nov 9, 2012 at 18:59
  • 8
    @poke: I'd say that a comment that starts with fix ISSUE#413 is perfectly fine, and even preferable, as long as it also provides a reasonable amount of information about what issue #413 is. Just summarizing the issue report without providing a pointer to it makes life more difficult for a future reader who needs all the details.
    – Keith Thompson
    Nov 9, 2012 at 21:12
  • I agree with poke - you should never have to refer to an external source to understand the code. If I'm reviewing a change, it breaks the flow. I have to go to the issue tracker, pull up the issue, and read all about it. And what happens if you change issue trackers? It may be fine to have fix ISSUE#413 in the comment for completeness, but don't use it as a crutch.
    – Michael Dean
    Nov 10, 2012 at 13:46
  • "it never forgets to add a message (if you've configured it to require commit messages); it never annotates the wrong line of code or accidentally deletes a comment." We've just dealt with SVN corrupting itself, and having to restore from backup. We were able to find code that hadn't made it to backup yet, but when we re-committed the changes, several separate commits become one. My point is never is too strong a word, and lets not forget people DO move to new VCS software, and bringing the revision history might not be feasible or possible.
    – Andy
    Nov 10, 2012 at 17:26
7

Comments in the code are about what the code is at that moment. Taking a snapshot at any given time shouldn't refer to old (or worse, future) versions of the code.

Comments in VCS are about how the code has changed. They should read as a story about development.

Now, every change should include comments? in most cases, yes. The only exception I imagine is when the expected behaviour was already documented but wasn't what you got, because of a bug. Fixing it makes the existing comments more precise, so they don't have to be changed. The bug itself should be documented in the ticket history and the commit comment, but only in the code if the code looks strange. In that case, a // make sure <bad thing> doesn't happen should be enough.

Improve this answer
1
  • 8
    I would upvote this, but I really can't agree with "every change should include comments? in most cases, yes.". A check-in/commit comment, yes, absolutely. Code comments, definitely not necessarily.
    – user
    Nov 9, 2012 at 21:40
6

One type of comment I really appreciate is:

// This implemented for Business Rule 5 of Proposal 2

or whatever the heck you use to gather your requirements.

This has two advantages, one is that it lets you find the reason you implemented a given algorithim without searching and another is that it will help you communicate with non-software engineers that work on/create the requirements docs.

This may not help with smaller teams but if you have analysists that develop your requirements, it can be invaluable.

Improve this answer
2
  • 2
    That's different though because that provides a traceability that is orthogonal to version control: the connection between the code and the requirement specification that it implements.
    – Kaz
    Nov 9, 2012 at 22:54
  • In a system where version control is coupled with the bug/requirements system full tracability is provided without the need for comment. It's sometimes helpful to work the other way. Given a file from SCM, show me what requirements were implemented when. Or, given a requirement show me all files modified to implement it.
    – iivel
    Nov 10, 2012 at 3:13
4

Your leads are right when they say that comments are a good programming practice, however there are exceptions. Adding a comment for every change that you make is one of them. And you are right by saying that this should belong to the version control system. If you have to keep these comments in a single place, then the VCS is the way to go. Comments in source code tend to grow old and unmaintained. No comments are a lot better than bad comments. What you don't want is having comments in both places (in the code and VCS) that are out of sync. The goal is to keep things DRY by having a single source of truth for changes to the code.

Improve this answer
3

In addition to what others have said, consider what happens if a change has ripple effects throughout the system. Say you refactor a part of a core interface in the process of implementing a change request - that kind of change can easily touch a large percentage of the source code files in any non-trivial piece of software with what amounts to trivial changes (class or method name changes). Are you supposed to go through every single file touched by such an operation to manually annotate it with such comments, rather than relying on the VCS doing it all automatically? In one case you're looking at little more than a five minute job with any decent refactoring tool followed by a recompile to make sure nothing broke the build, whereas the other can easily balloon into a day's work. For what specific benefit?

Also consider what happens when you move parts of the code around. One of the database developers I work with is in the camp of largely "each line of SQL should be annotated with the revision in which it was changed, and we're going to do separate revision histories for each file because then it's easier to see who changed what when and why". That works kinda-sorta okay when the change is on the order of changing single lines. It doesn't work quite as well when, like I did recently to fix a serious performance issue, you break out parts of a larger query introducing temporary tables, then change the ordering of some of the queries to better fit the new code flow. Granted, the diff against the previous version was largely meaningless since it said about two thirds of the file had changed, but the check-in comment was also something like "major reorganization to fix performance issues". By the time you looked manually at the two versions, it was pretty clear that large parts really were the same, only moved around. (And it took the stored procedure in question from regularly taking over half a minute to execute, to a few seconds. By that time, it was largely I/O-bound with few places for significant further improvement at least in the short term.)

With very few exceptions, change tracking and issue referencing is the work of the VCS, IMNSHO.

Improve this answer
3

I usually follow this rule: if the change is obvious and the resulting code does not raise questions, does not revert or substantially change any previous behavior in a substantial way - then leave it to the VCS to track bug numbers and other change information.

However, if there is the change that is not obvious, that changes the logic - especially substantially changes the logic done by somebody else in a non-obvious way - it may be very beneficial to add something like "this change is to do this and that because of bug #42742". This way when somebody looks at the code and wonders "why is this here? this looks weird" he has some guidance right in fron of him and does not have to do investigation via VCS. This also prevents situations where people break other's changes because they are familiar with old state of the code but do not notice it has been changed since.

Improve this answer
2

Version-control-related comments do not belong into the source file. They only add clutter. Since they are likely required to be put in the same place (like a comment block at the top of the file) they will cause comment-only nuisance conflicts when parallel branches are merged.

Any tracking information that can be pulled out of version control should not be duplicated in the body of the code. That goes for silly ideas like the RCS checkout keywords like $Log$ and its ilk.

If the code ever travels outside of the scope of the version control system, that trail of comments about its history loses context and therefore most of its value. To properly understand the description of the change, we need access to the revision, so we can view the diff to the previous version.

Some old files in the Linux kernel have big history comment blocks. Those date back to when there was no version control system, just tarballs and patches.

Improve this answer
2

Comments in the code should be minimal and accurate. Adding defect /change information not valuable. You should use version control for it. Some time Version control provides a slightly better way of change- We use ClearCase UCM ; UCM activities are created based on defect numbers, change area etc ( e.g. defect29844_change_sql_to_handle_null).

Detailed comments is preferred in check-in comments.

I prefer to include provide details about back ground information, details of solution NOT implemented due some side effects.

Pramagic Programmer and CleanCode leads to following guideline

Keep the low-level knowledge in the code, where it belongs, and reserve the comments for other, high-level explanations.

Improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.