The Boston Diaries

The ongoing saga of a programmer who doesn't live in Boston, nor does he even like Boston, but yet named his weblog/journal “The Boston Diaries.”

Go figure.

Saturday, January 31, 2015

Some commentary about comments and commit messages

Whoever wrote line 4 of the following code snippet decided to access the clientLeft property of a DOM node for some reason, but do nothing with the result. It’s pretty mysterious. Can you tell why they did it, or is it safe to change or remove that call in the future?

If someone pasted you this code, like I did here, you probably won’t be able to tell who wrote this line, what was their reasoning, and is it necessary to keep it. However, most of the time when working on a project you’ll have access to its history via version control systems.

Via Hacker News, Every line of code is always documented

At work, a former developer would always include a huge comment at the top of each file, describing the history of bug fixes to the code. It looked something like this:

//---------------------------------------------------------------------------
//
// HISTORY OF CHANGE  
//
// -----+--------+------------------------------------------+------+---------
// VERS | DATE   | DESCRIPTION OF CHANGE                    | INIT | PR#
// =====+========+==========================================+======+=========
//  001 |06/09/05| File creation.                           | ABC  | B1017 
// -----+--------+------------------------------------------+------+---------
//  002 |06/24/07| Optimize the fob.                        | XYZ  | B1130
// -----+--------+------------------------------------------+------+---------
//  003 |01/08/08| Make the fob more flexible.              | XYZ  | B0019
// -----+--------+------------------------------------------+------+---------
//  004 |04/15/09| The fob wasn't frobbing the widget.      | XYZ  | B0021
// -----+--------+------------------------------------------+------+---------
//  005 |02/02/10| Reduce the size of the gadget handler.   | XYZ  | B0005
// -----+--------+------------------------------------------+------+---------
//  006 |04/05/10| Work around Whatzat Protocol bug.        | XYZ  | B0024
//      |04/14/10| Generalize the widget protocol.          |      | B0007
// -----+--------+------------------------------------------+------+---------
//  007 |01/20/10| Performance enhancement.                 | XYZ  | B1019
// -----+--------+------------------------------------------+------+---------
//  008 |03/09/11| Race condition between hare and turtle.  | XYZ  | B1019
//      |03/15/11| Beef up authentication.                  |      | B1021
// -----+--------+------------------------------------------+------+---------

Seeing how we already use a version control system, which already contains this information and thus, there is no real need to put this in the source code itself, I asked him about it.

“Because not everyone has access to the version control server,” he said.

“What? Who can see the source code but does not have access to the version control server?” I asked.

“Customers we license the code to,” he said.

“But we don't license the code to anyone,” I said.

He then stormed off, saying I didn't understand how software was developed, but I think I see where he was coming from. He has a side-company where he licenses the source code to his products and didn't want his customers to have access to his version control server. Adding this information to the files themselves let his customers know what has been fixed. But he kept the practice up at The Corporation because that's the way he's always done it. It made sense for his company, but not for The Corporation. We aren't going to license this code to anyone, and anyone that is working on it has access to the version control server and can see the entire history of any of the files in the project.

But that's not to say that all commentary about the code should go exclusively in version control. The history of bug fixes, yes, that can go into version control as there's no need to clutter up the source code files with that information. But comments that describe why the code is doing something odd should go in the file itself, above the odd bit of code.

For example, in my own blogging engine, I have this commit:

commit eab6ecb7149f8b70614657cd20fe09337cf5e977
Author: Sean Conner <spc@conman.org>
Date:   Tue Jan 15 23:43:19 2013 -0500

    Bug fix---tumbler problems, but not necessarily with the parsing
    
    The intent was if the start date was later than the end date, swap the
    two dates and set the reverse flag.  Unfortunately, the logic to
    determine the swap was wrong.  Wong wrong wrong wrong wrong.  I moved
    the check down, and rewrote the check logic.  It should be fine now, but
    get back to me in another decade.

But the code that handles this is a bit odd (it was even worse before I fixed it) so I added a comment to the modified code:

    /*------------------------------------------------------------------------
    ; swap tumblers if required.  If the parts are the default values, swap
    ; them, as they probably weren't specified and need swapping as well.
    ;
    ; Example:
    ;
    ;     2000/02/04    - 01/26
    ;  -> 2000/02/04.1  - 2000/01/26.23
    ;  -> 2000/01/26.23 - 2000/02/04.1
    ;  -> 2000/01/26.1  - 2000/02/04.23
    ;
    ;-----------------------------------------------------------------------*/

    if (btm_cmp(&start,&end) > 0)
    {
      struct btm tmp;

      gd.f.reverse = true;
      tmp   = start;
      start = end;
      end   = tmp;

      if ((start.part == 23) && (end.part == 1))
      {
        start.part = 1;
        end.part = 23;
      }
    }
  }

The comment describes what the code is doing (and shows an example; I'll admit, the comment could be a bit better). But notice how the commit describes why I made the change—it's a comment on the comment as it were.

Obligatory Picture

[It's the most wonderful time of the year!]

Obligatory Contact Info

Obligatory Feeds

Obligatory Links

Obligatory Miscellaneous

You have my permission to link freely to any entry here. Go ahead, I won't bite. I promise.

The dates are the permanent links to that day's entries (or entry, if there is only one entry). The titles are the permanent links to that entry only. The format for the links are simple: Start with the base link for this site: http://boston.conman.org/, then add the date you are interested in, say 2000/08/01, so that would make the final URL:

http://boston.conman.org/2000/08/01

You can also specify the entire month by leaving off the day portion. You can even select an arbitrary portion of time.

You may also note subtle shading of the links and that's intentional: the “closer” the link is (relative to the page) the “brighter” it appears. It's an experiment in using color shading to denote the distance a link is from here. If you don't notice it, don't worry; it's not all that important.

It is assumed that every brand name, slogan, corporate name, symbol, design element, et cetera mentioned in these pages is a protected and/or trademarked entity, the sole property of its owner(s), and acknowledgement of this status is implied.

Copyright © 1999-2021 by Sean Conner. All Rights Reserved.