15

My team is using clear-case as the version control. The project which I am working is not started 7-8 years back. During the entire life time of the project we had several releases bug-fixes service packs etc. The issues are tracked using the bug tracking system and most of the people who work on the bug-fixes follows a routine of enclosing the comment in START/END block with the date, author, bug-id etc.

I feel this is quite irrelevant and making the code cluttered and uneasy to maintain and these are the things which must be part of check-in comments/labels etc, where we can keep additional life cycle information of the work product.

What's the best practice to be followed?

Some of the reviewers of the code insist to out the comments about the bug and fixes to ease out their life. In my understanding they must review the files by mapping it to a view and get the change log of the branch and review it. It would be helpful if I can get some best practices on submitting the updated code for review.

sarat
  • 1,121
  • 2
  • 11
  • 19
  • 3
    The real question is WHY they do it like that. This might be a very old procedure from before source control. –  Jul 26 '11 at 06:57
  • @anderson - I feel they dint have proper understanding about using clearcase when it was introduced. So that practice might have followed further.... – sarat Jul 26 '11 at 07:10
  • considered finding out? –  Jul 26 '11 at 07:16
  • possible duplicate of [Is there a point to including a "change log" in every code file when you are using version control?](http://programmers.stackexchange.com/questions/83797/is-there-a-point-to-including-a-change-log-in-every-code-file-when-you-are-usin) – Péter Török Jul 26 '11 at 07:48
  • I won't say it's a bad practice. Anyway one of the software quality measurement is comments percentage through the source code. – Rudy Jul 26 '11 at 10:44
  • notes about bug fixes or code changes go in the SCM system. The one exception being if there is a known bug that due to some outstanding reason needs to be left in. then I will make a comment in the code where the bug is to document that it is a known bug and was purposely left alone for some reason. – Alan Barber Jul 26 '11 at 12:32

7 Answers7

27

The problem with adding the bugfix as a comment to the code is, you don't get the full story. If I see a perfectly fine piece of code tagged "this is a fix to the bug blah", my first reaction would be to say "so what?". The code is there, it works. The only thing I need to know to maintain the code is a comment that tells me what it does.

A better practice would be to add bugfix references in SCM commit logs. That way, you see what the bug is, where it was introduced, and how it was fixed. In addition, when the time for a release comes, you can simply extract the SCM logs, and add a bullet point stating that there was a bug, and it was fixed. If another branch or version introduces the same bug, it is easy to locate the fix, and reapply if it is indeed the same thing.

Having said all this, I also agree with Charles' answer. If the reason for a piece of code is not obvious, by all means, tell the maintainer that the code is there for a reason, and should be treated with care.

Dysaster
  • 925
  • 6
  • 6
  • Excellent answer. I've added few more points to my question. Please check and update your answer. thank you. BTW, Would you able to help me out with clearcase commands to get commit logs from a specific branch? – sarat Jul 26 '11 at 06:51
  • @Sarath I am afraid I have never used ClearCase before. Feel free to use superuser to ask away. I am sure there are lots of people in the know willing to help. – Dysaster Jul 26 '11 at 06:54
  • 4
    Good bug trackers like JIRA can look in the commit logs of your SCM and pull out information for bugs. Just think that you commit something for a bug, and the bug tracker automatically adds a note to the bug report that commit X referred to the bug. –  Jul 26 '11 at 06:59
  • @Andersen - Thanks we're using JIRA. But I need to make sure if it's using properly or not. – sarat Jul 26 '11 at 07:00
  • @Thorbjørn Ah, you've got it easy. I had to do it manually back in the day with CVS/Bugzilla combo (and later SVN/Bugzilla). Add bugfix reference to my commit, and add the commit reference to bugzilla. Error prone process, and developers tended to forget one or the other. But the information came in very handy on a number of occasions. – Dysaster Jul 26 '11 at 07:02
  • @dysaster, I am just mentioning it as this is really nice. That you had to bang rocks together back with the dinosaurs is hopefully just a happy memory these days? –  Jul 26 '11 at 07:13
  • @Sarath, depends if JIRA is allowed to snoop in your repository. I can highly recommend it. –  Jul 26 '11 at 07:14
  • @Thorbjørn Indeed it is. :) I just mentioned it to point out that even when it is not comfortable and easy, there are benefits to doing it. I am all for using tools that automate away the drudgery. – Dysaster Jul 26 '11 at 07:22
  • In our team we have a post-commit hook that sends an email to the issue tracker. The issue tracker looks for the commit log message for the issue number and adds and entry to that issue. – yasouser Jul 27 '11 at 03:47
23

Mostly bad practice. I won't say it should never be done. Occasionally you run into something like a bug in an external API which you have to work around. The work-around can look completely brain dead if you don't know about the underlying bug. In that case it may be a good idea to document the bug in the code so co-workers or your later self don't try to "fix" the obviously brain dead code.

Charles E. Grant
  • 16,612
  • 1
  • 46
  • 73
  • 3
    Agreed. Add these kind of comments when they add significant value. Don't do them just for the sake of turning a handle, though. – quickly_now Jul 26 '11 at 05:50
  • Nice, this supports the idea of comments telling "Why" some code is there. See http://programmers.stackexchange.com/questions/1/comments-are-a-code-smell – Gabriel Oct 27 '11 at 10:26
  • I have done this a few times: code that, at first glance, completely defies logic ("what the f* is this code here for?") but is actually there for a very good reason, so you want people to tread carefully around it. Then, adding a cautionary comment explaining the *why* of that code can make everyone's life easier. Then, if someone can think of a better way to solve the original problem, all credit to them, I say - they know why the braindead code was put in place, and what it's supposed to accomplish, so it's much easier to implement an alternative solution. – user Nov 07 '11 at 15:14
9

Bad practice. Comments will get out of date and clutter up code. The information is still available in the version history of your SCC system if needed.

Craig
  • 4,302
  • 3
  • 21
  • 21
3

Sounds like a bad practice. What if your organisation decides to migrate to another bug tracking system? Don't tie your product too tightly to the tools you are currently using. Instead of referring to specific bug IDs, and the reasons for why the code looks like it does are unclear, motivate your design decision with comments in the code.

fejd
  • 272
  • 1
  • 6
2

My first reaction would be don't repeat yourself so get it out of the code and into the SCM logs. We have had a similar discussion over here about revision comment for functions, author names and creation dates for files and functions. In the past (before SCM was used) all this information was kept in the files to be able to reconstruct the evolution of a file.

About half of the developers wants this information in to be able to have all the information in one place (this triggers them to search for changes in the SCM). The other half of the developers do not start their search for clues of what changed in the coe, but from the SCM so they don't need the information in the code. We have yet to make a decision what to do with those comments. It very much depends of the way people are working and some people are very stubborn about leaving their known methods. Same thing about commenting out block of code and leave them in the code forever.

refro
  • 1,386
  • 6
  • 17
  • I guess that commented code should be checked by the SCM and refused to even be committed... It's adding clutter to the code just for the sake of gaining 5 minutes of search in the SCM history if some hypothetical day in the future you need it back. – Julien Roncaglia Jul 26 '11 at 07:26
  • Agreed but try to implement that in sourcesafe :D In our project team we have been working with reviews, so for now those blocks are refused by the reviewer. – refro Jul 26 '11 at 07:43
  • Oh SourceSafe... sorry for you and your team. – Julien Roncaglia Jul 26 '11 at 07:48
  • Don't be, its better than nothing. And at the moment all new development is done with Subversion so each month I have less to do with sourcesafe. – refro Jul 26 '11 at 09:20
1

Just to add to what Dyaster et al. have said, although JIRA has really nice abilities to display changes associated with bugfixes, the absolute best place to document a bugfix is in a test case. If the code isn't clear without a comment indicating what bug was fixed, that's "code smell". That said, if you don't have time to clean up the smell, the comment should refer to the test case, where it should be much more obvious why the code is doing what it's doing. If you don't have time to write a test case explaining the bug fix, then the bug hasn't really been fixed yet, it's just been postponed.

Ben Hocking
  • 111
  • 4
1

I would agree with adding bugfix ids in commit messages, and not within the code itself. Bug trackers that automatically scrape commit messages for bug ids are very useful.

In addition, you can use the blame/annotate/praise command of your version control system to help replace these comments. Then, when you run something like:

vcs blame file.ext

you can see useful information, usually including who changed each line, when they changed it, and the commit id. From the commit id, you can get the full message, which should include the bug id.

Good VCS systems will let you ignore whitespace when calculating who modified a line.

I don't know what Clear Case has for this feature.