In the article "In Praise of the Lowly Comment," Mike Gunderloy presents a good case for the sorts of comments that are appropriate and necessary. I agree with many of Mr Gunderloy's points, including one particularly important one, that "software [development] is hard." We also both are skeptical of the notion of self-documenting code.
Gunderloy summarizes four types of comments that are worthwhile for particularly difficult sections of code:
- "Todo," or placeholder, comments
- Summary comments
- "Why," or intent, comments
- Explanatory comments
I think these are worthwhile categorizations, and I'll add my, uh, comments to Gunderloy's thoughts.
Placeholder Comments
Placeholder comments are great reminders of things that need to be done. I use them occasionally. However, as Gunderloy mentions, you want to review these prior to shipping, to ensure that no embarrassing comments remain. My stance on placeholder comments is a bit more stringent: You don't want them to exist past an iteration or release boundary.
Placeholder comments are much like warnings and rabbits. As soon as you have a couple or more that stick around, they rapidly proliferate. This is the "broken window syndrome." Unfortunately, most teams aren't good at doing anything about such comments, so they often stick around forever. I've been embarrassed by "TODO" comments of my own that lay dormant in code for over a year.
The important thing is that developers get in the habit of looking for and actually fixing these hopefully small problems. First, as Gunderloy suggests, developers should use standard tag names (such as Eclipse's TODO) for such ephemeral comments. That will at least allow their regular and immediate perusal. Second, developers should have the attitude that placeholders represent incomplete work, and must be resolved by release or iteration end. At that time, any remaining placeholder comments are removed. Bigger "todo's" that can't be resolved in time get represented on some sort of external task list (which elevates their visibility).
Summary Comments
I've found summary comments to be occasionally valuable in explaining code. However, I tend to write summaries for classes, not methods, because there's usually a better way to represent code chunks within a method. Gunderloy mentions, "the more procedural the code gets," the more likely it needs a comment. My reaction is to refactor code to the point where it isn't so procedural (I'm talking about object-oriented code here).
The code in Listing 1, from a library system example codebase, is not atypical for most of the hundreds of systems I've encountered. Most developers would consider this code and its associated comments as perfectly acceptable. When teaching test-driven development, however, I have students work at improving the checkin method by using basic refactoring techniques.
My term for summary comments appearing within a method is "guiding comments:" They supposedly help you step through the body of a longer method. To me, they are beacons that often suggest application of the Extract Method refactoring.
Most students end up with a checkIn method similar to that shown in Listing 2. Each of the extracted methods, not shown, is now similarly short (most of them can be further cleaned up, and some could be moved to more appropriate classes). The code in checkIn now provides a readable, concise summary of a check-in policy. The willingness to extract methods and to freely rename variable and method names quickly turned a difficult method into a comprehensible one.
Listing 1: Difficult code
public void checkIn(String barCode, Date date, Branch branch) {
Holding hld = findBarCode(barCode);
// set the holding to returned status
List holdings = null;
hld.checkIn(date, branch);
Holding patHld = null;
// locate the patron with the checked out book
Patron f = null;
Patron p = null;
for (Iterator it = getPatrons().iterator(); it.hasNext();) {
p = (Patron)it.next();
holdings = p.holdings();
for (Iterator it1 = holdings.iterator(); it1.hasNext();) {
patHld = (Holding)it1.next();
if (hld.getBook().equals(patHld.getBook()))
f = p;
}
}
// remove the book from the patron
f.remove(hld);
// check for late returns
boolean isLate = false;
Calendar c = Calendar.getInstance();
c.setTime(hld.dateDue());
int d = Calendar.DAY_OF_YEAR;
// check for last day in year
if (c.get(d) > c
.getActualMaximum(d)) {
c.set(d, 1);
c.set(Calendar.YEAR, c.get(Calendar.YEAR) + 1);
}
if (hld.dateLastCheckedIn().after(c.getTime())) // is it late?
isLate = true;
if (isLate) {
int daysLate = 1; // calculate # of days past due
switch (hld.getBook().getType()) {
case Book.TYPE_BOOK:
f.addFine(Book.BOOK_DAILY_FINE * daysLate);
break;
case Book.TYPE_MOVIE:
int fine = Math.min(1000, 100 + Book.MOVIE_DAILY_FINE *
daysLate);
f.addFine(fine);
break;
case Book.TYPE_NEW_RELEASE:
f.addFine(Book.NEW_RELEASE_DAILY_FINE * daysLate);
break;
}
}
}
Listing 2: Clearly stated policy
public void checkIn(String barCode, Date date, Branch branch) {
Holding holding = findHolding(barCode);
holding.checkIn(date, branch);
Patron patron = findPatron(holding);
patron.remove(holding);
if (isLate(holding))
addFines(patron, holding);
}
IE 7
Firefox 2.0
