Saturday, January 31, 2015
Some commentary about comments and commit messages
Whoever wrote line 4 of the following code snippet decided to access the
clientLeftproperty 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.
