“Clean Code” Book Club: Chapter 4, Comments

Continuing with our book club on Robert Martin’s “Clean Code”, this week we’ve discussed chapter 4 (“Comments”). (As usual, this post collects most of my notes and some points from our group discussion; completeness is explicitly a non-goal.)

After the last chapter, which I had severe issues with, this one got better again. My overall impression is, though, that it’s suffering a lot from a lack of editing: In addition to several redundant passages, there are a few mistakes in sample code, several redundant subsections and plenty of redundancies.

Chapter 4: Comments

The proper use of comments is to compensate for our failure to express ourself in code. (p. 54)

We had rather mixed feelings about the introduction. Martin does a good job explaining potential problems with comments (they can easily become outdated and misleading) and arguing that time spent keeping them up to date is often better spent making code more readable so it doesn’t require comments in the first place. However, he’s really over-doing his spiel: It’s hard to take a statement like “Comments are always failures” seriously when it’s followed by a section listing eight different kinds of “Good Comments”.

Good Comments

We fully agreed with four of the types of good comments (“Explanation of Intent”, “Clarification”, “Warning of Consequences” and “Amplification”) and didn’t have much to discuss or add to those. The other four types led to some discussion:

Legal Comments

For example, copyright and authorship statements are necessary and reasonable things to put into a comment at the start of each source file. (p. 55)

Are they? IANAL, but isn’t it sufficient to have a license in the repository, rather than in each source file? (And why just source files? What about documentation files? Or graphics, like a software’s logo?)

Also, while authorship information in the style of Copyright (C) 2024 by $COMPANY_NAME may work for software written by a single company, how could you meaningfully do this for an open-source project that is developed by, potentially, thousands of individuals? I occasionally see something like Copyright (C) 2024 by $PROJECT_NAME contributors, which is basically tautological. (“This code was written by the people who wrote this code.”)

Informative Comments

This section starts with the (vague enough to be useless) statement that “it is sometimes useful to provide basic information with a comment”. It then gives two examples, each followed by a paragraph explaining that it would be better to change the code and avoid this comment.

Why was this section written in the first place? And how did it make it past an editor?

Clarification

The second example from the “Informative Comments” section—adding a human-readable version of a regex—would have worked well here. (Arguably better than the example actually given here: If the code reads a.compareTo(a) == 0;, adding the comment // a == a really doesn’t offer much benefit.)

TODO Comments

We weren’t big fans of these. Martin argues that IDEs make it easy to locate these comments, so it’s easy for programmers to keep track of them. But that doesn’t square with the chapter introduction: If a major reason to avoid comments is that programmers will not put in the effort to keep them up to date even when editing that exact part of the code, then why would you assume programmers are going to put in the effort to regularly look for TODO comments across the whole code base?

Instead, we discussed keeping track of TODOs in an issue tracker instead; to ensure they’re all in a single place, rather than some being in an issue tracker and some scattered throughout the code base. In the cases where it’s actually necessary to add a TODO comment in the code—e.g. if code may be confusing otherwise—it’s then best to add a link to the respective issue to the TODO comment and include further details and discussion in the issue. This also makes it clear how to find out whether a particular TODO is still relevant.

Bad Comments

Among bad comments, we found ourselves fully agreeing with about half the subsections (“Misleading Comments”, “Journal Comments”, “Don’t Use a Comment When You Can Use a Function or a Variable” (This one in particular had a very nice example!), “Commented-Out Code”, “HTML Comments”, “Nonlocal Information” and “Too Much Information”) and didn’t have much to comment on there.

Mumbling

There’s a mistake in the example for this subsection: The code listing uses a loadedProperties.load function, but the text below refers to loadProperties.load every time. While subtle, this is a potentially important distinction here: loadedProperties strongly implies that some properties (presumably the defaults?) were already loaded; which would make the comment in this example a little clearer.

More generally, while unclear or confusing comments are indeed undesirable, we didn’t find this advice very actionable. Often, the person writing a comment may not even realise that it’s ambiguous and that it may be interpreted differently by another reader.

Redundant Comments

In the second example here, Martin argues that a set of javadocs comments is redundant and thus useless clutter that should be avoided.

/**
 * The container event listeners for this Container.
 */
protected ArrayList listeners = new ArrayList();

/**
 * The Loader implementation with which this Container is
 * associated.
 */
protected Loader loader = null;

This will depend on the context, of course, but we were sceptical about this. For public APIs at least, we’d err on the side of documenting them consistently, even if several of the comments are redundant. A user’s first impression when some functions or variables are missing documentation is not going to be “Oh, clearly they left out documentation because it would be redundant”; it’s gonna be “oh, the documentation is inconsistent; I can’t trust it.”

Mandated Comments

Here, Martin is just repeating his earlier point about redundant information in javadocs, using a different example. By the way: Have you spotted the error in the code listing 4-3?¹

Noise Comments

In the first example of this subsection, Martin is repeating his point about redundant javadocs for the third time. Is this a total lack of editing or a badly written meta-joke about redundancy?

The second example (listing 4-4) is at least different, but I found it quite irritating. In this listing, the programmer added a comment saying //Give me a break! when some exception handling code throws yet another exception. Noise? Sure. A little unprofessional? Perhaps. But also harmless and cathartic and very human.²

Martin’s advice here—the programmer should’ve extracted a try/catch block into a separate function instead—feels emotionally tone-deaf to me. (Not just because I disagree; but also because it’s a complete non sequitur and sounds patronizing: “You seem frustrated. Have you tried doing Yoga extracting a few lines into a separate function?”)

Scary Noise

This is the fourth time in this chapter that Martin is telling us that javadocs can be noisy. Give me a break!

Position Markers

Here, we agreed that these are okay when used sparingly; which even Martin seems to accept. (Though we were a bit puzzled that he writes “the noisy train of slashes at the end” should “especially” be eliminated, when this is exactly what makes the marker so visible and thus useful. That’s like arguing fire alarms are okay sometimes, but they shouldn’t be so loud.)

Closing Brace Comments

Agreed; these are unnecessary clutter. A combination of code indentation and IDEs highlighting matching braces and/or current scope removes the need for this.

Attributions and Bylines

Agreed; git blame is preferable.

(That said, this is just a variant of the “Journal Comments”; so why not merge the two subsections?)

Inobvious Connection

I agree with this point in principle; but what is or isn’t obvious depends heavily on context. Martin’s example here doesn’t strike me as particularly good:

/*
 * start with an array that is big enough to hold all the pixels
 * (plus filter bytes), and an extra 200 bytes for header info
 */
this.pngBytes = new byte[((this.width + 1) * this.height * 3) + 200];

What is a filter byte? Does it relate to the +1? Or to the *3? Both? Is a pixel a byte? Why 200? (p. 70)

I don’t have experience writing image manipulation code; but even to me, it seems fairly obvious that the pixels array is width * height * 3 (where 3 stands for three colour values per pixel—red, green and blue), so the + 1 must correspond to the filter bytes. A developer working on this low level image manipulation code will almost certainly have much more experience in this area than I do; so more detailed explanations are likely unnecessary here.

Also, the exact composition of the PNG header info bytes is arguably irrelevant here? If the comment were to go into detail here—rather than elsewhere in the code, where the header bytes are actually handled—that would be an example of non-local information, which Martin rightly advised against just one page earlier.

Function Headers

Isn’t this just a special case of “Redundant Comments” and “Noise Comments”? Why do these two fairly vague sentences need their own subsection?

Javadocs in Nonpublic Code

I don’t think public vs. nonpublic is the right distinction here. Just because an API is private, doesn’t mean that the people using that API won’t benefit from good docs, just like external users of a public API will.

The better distinction here is between a (public or nonpublic) API on the one hand and internal helper functions on the other hand (which are only relevant to developers who are working on that particular file). (Maybe that is actually what Martin means here and his language contrasting “nonpublic code” with “public APIs” just isn’t very precise.)

1. The docstring and function signature contain int durationInMinutes, but the implementation sets cd.duration = duration;. ↩
2. To be clear: More explicit swearing, violent or insulting language would be completely unacceptable in a professional or open-source code base. But an exasperated “Give me a break!” is something completely different. ↩