Thursday, December 6, 2007

Check in Comments

My first approach to check in comments was not to use them. "You're just going to have to diff the code to see what really changed anyways!" I would have said.

My second approach has changed a bit. I still think you're going to have to diff the code to see what really changed. Like I talked about in Code Documentation, meta data is a bad substitute for reading code. However, check in comments can help answer the following questions:
  1. What did that dude change?
  2. Which files do I need to diff?
  3. Could this change be of interest to me?
The key is you have to write your check in comments to be helpful in answering those questions and leave the details to the diff.
  1. If you didn't really change anything, don't leave a comment. If you're checking in a file and all you've done is change the spelling of a variable, or the capitalization of a method, or the wording of a comment, don't bother with the check in comment. No one is ever going to need to diff or read your changes for that version, so save me the time in reading your comment just to figure out that you didn't change anything, and don't leave a comment. If you're required to leave a comment, just put "none" or something equally simple.
  2. Title what you changed. Pretend you're trying to give the diff between your version and the previous version a Chapter Title. It should summarize the changes you've made at a high level.
  3. Be as brief as possible. Don't write a book. Don't even use full sentences. How few words can you use to describe the changes you made?
  4. Group check ins by change. If you're checking in 10 files which have had 5 mutually exclusive changes made to them, don't check them in all at once. Do 5 check ins of two files each. Each check in gets its own comment describing only the changes in those files.
  5. Check in one change at a time. If you're frequently writing check in comments that say, "Did this and that," could you have done "this" and checked in, then done "that" and checked in?
One example of a bad check in comment I frequently see is a list of everything that was changed, written in one run on sentence. For example, "Updated the <class> to have 3 new public methods, and broke <method> into two different methods, one for <a> and the other for <b>. Also changed the parameter types of the methods from <x> to <y>." There are a number of things wrong here. First, why should the check in comment document the purpose of the methods? I'm going to assume there are comments in the code you're checking in that does that. Second, why tell me about the actual parameter types you changed from and to? It's probably enough just to say the parameter types changed. Third, do I really need to know the class, and method names? And finally, this is too long to help me figure out if I want to look at this version of the file. You could convey all the information needed by saying "Refactored public interface" or if the refactoring has added new capabilities, "Supports a, b, and c."

A check in comment should convey what changed at a high, abstract, level. To figure out how those changes were accomplished: diff. To figure out if those changes might be causing a bug you just found: diff. To figure out how to use the changes: diff. You're better off looking at code to answer those kinds of questions anyway. The check in comment is just a guide to help you figure out where to look.

3 comments:

  1. I like check in comments. I hate useless ones (namely the automatically created ones Enterprise Architect has that no one seems to change and only tell you that the item was checked in). I don't mind if the comments just says something like "Updated code to meet coding standards." I prefer that over no comment because I can't be sure that no comment really means minor readability changes or whether the person made some major change that they didn't comment on. The check in comments come in real handy when you need to develop a version description document a few versions in and have to figure out what all changed. I don't want to diff the code to find out. Most of the time when I'm updating my code with code from CVS, I usually check the comments before I check the diff. I don't really care what all changed in the code unless the check in comment gives me a reason to be interested.

    ReplyDelete
  2. I agree with Chaz.

    Its like the warning light problem ... you can't have the absence of the light mean things are OK and the light on meaning there is trouble -> what happens when the light bulb is burnt out and something goes wrong?

    (Thank you PSY 445)

    if someone is checking in a major change and/or one that is associated with a defect/task/change request, then I would say the person should cite the issue at a minimum. If the changes are for no documented reason and are too simplistic in nature to warrant typing a standardized "reformatting" or "minor non-functional changes" message, then I would ask "Are they needed?"

    nothing is fool proof.

    If I am updating and/or merging some code and it is directly effecting files I am working with I will probably diff the files. If its code I am not currently working with then I either don't care what the change was (you have to trust the other people you work with if you want to get your own work done) or if I am curious I want to read a single line to tell me if I need to look further, a very simple message helps with that.

    plus, most CVS utilities I have used remember your recent comments, and I usually have some standardized messages in there, so its no time at all to add them.

    ReplyDelete
  3. Check in comments would be useful for a version description document. Especially if they were written following the guidelines I outlined because you want to know what that version is, not the details of the code.

    Also, it isn't like the warning light problem. If you can't trust your devs to write a check in comment when they make a change, you certainly can't trust your devs to write good check in comments. Requiring check in comments doesn't solve the problem, though it will keep people from forgetting.

    Your point about referencing the defect is a good one though. If you have some kind of defect tracking system it could be useful to know what code changes are supposed to have fixed it. Of course, lots of defect tracking systems have built in support for this (TFS and Fogbugz do).

    ReplyDelete