Chapter 13: Comments Should Describe Things that Aren't Obvious from the Code

Core Idea

“Comments should describe things that aren’t obvious from the code.” Good comments work at a different level of detail from the code: lower-level comments add precision, and higher-level comments add intuition. Interface comments define abstractions and must stay free of implementation details.

This is the longest chapter in the book, with many before-and-after comment examples, so this file runs over the usual length.

Frameworks Introduced

• Pick conventions: decide what you will comment and in what format. Follow the documentation tool’s conventions if one exists (Javadoc, Doxygen, godoc); otherwise borrow from a similar language or project. Conventions make comments consistent, and they make sure comments actually get written.
• Four categories of comments:
  1. Interface: precedes a class, data structure, function, or method. Every class and every method should have one.
  2. Data structure member: next to a field. Every class variable should have one.
  3. Implementation: inside a method. Often unnecessary.
  4. Cross-module: describes dependencies across module boundaries. Rare and hard to write, but important when needed.
  • Rule: comment everything in categories 1 and 2. It is easier than deciding case by case, even though a few getters and setters need nothing.
• The repeat test: “could someone who has never seen the code write the comment just by looking at the code next to the comment?” If yes, the comment adds nothing.
• Different level of detail: comments at the same level as the code repeat it.
  • Lower-level comments add precision (mainly for variables, arguments, and return values). Answer: What are the units? Are boundaries inclusive or exclusive? What does null mean? Who frees or closes the resource? What invariants hold (such as “this list always contains at least one entry”)?
  • Higher-level comments enhance intuition (for blocks inside methods and for interface comments). Ask: “What is this code trying to do? What is the simplest thing you can say that explains everything in the code? What is the most important thing about this code?”
• Interface comment checklist for a method:
  1. A sentence or two describing behavior as callers see it.
  2. Every argument and the return value, precisely, including constraints and dependencies between arguments.
  3. Side effects: any consequence that affects future behavior but isn’t part of the result, such as adding to an internal structure or writing to the file system.
  4. Every exception that can come out of the method.
  5. Preconditions, such as another method that must be called first, or a sorted list for binary search. Minimize them, but document the ones that remain.
• Interface comment for a class: the overall abstraction, what each instance represents, and limitations (for example, “single-threaded”), with no implementation details.
• Implementation comments: what and why, not how: put a short comment before each major block of a long method and before complex loops (what each iteration does). Explain tricky code and the reasons for bug fixes, referring to the bug tracker rather than repeating it (“Fixes RAM-436, related to device driver crashes in Linux 2.4.x”).
• Placing cross-module comments:
  • If there is an obvious central place developers must visit, put the documentation there (the Status enum below).
  • If not, write a central designNotes file with labeled sections, and leave short pointers in the code: // See "Zombies" in designNotes. The downside is that the notes are far from the code and may go stale.

Key Concepts

• Red Flag: Comment Repeats Code: the information is already obvious from the code next to it, often because the comment reuses the words of the name.
• Red Flag: Implementation Documentation Contaminates Interface: an interface comment describes implementation details that users don’t need.
• “Think nouns, not verbs”: a variable comment should say what the variable represents, not how code manipulates it.
• “How we get here” comments: explain why code runs, such as the conditions under which a method is usually called.
• “Obvious” means obvious to a first-time reader, not to you. “If a reader thinks it’s not obvious, then it’s not obvious.”
• “The code” means the code next to the comment: the declaration, not all the code in the application.

Mental Models

• If an interface comment must describe the implementation, the module is shallow. Writing comments is a design check (ch15).
• A good higher-level comment gives “one or a few simple ideas that provide a conceptual framework,” such as “append to an existing RPC.” Readers can then map each statement to the goal and judge whether the code is correct.
• Use different words from the name. Explain what a term means rather than repeating it.
• In code review, don’t argue with a reader who found something unclear. Fix it with better comments or better code.

Anti-patterns

• One comment per line restating the line: # Get pointer copy above ptr_copy = get_copy(obj).
• Name echoes: “Downcast PARAMETER to TYPE.” The only new word is “to.”
• Vague variable comments: “Current offset in resp Buffer” (current in what sense?).
• Comments that mirror code structure: “Toggled to TRUE when … Toggled to FALSE when …”
• Low-level block comments that restate conditions without saying what the block is for.
• Mentioning private config constants and RPC names in a class’s interface comment.

Code Examples

Lower-level precision, before and after:

// Current offset in resp Buffer
uint32_t offset;

// Contains all line-widths inside the document and
// number of appearances.
private TreeMap<Integer, Integer> lineWidths;

//  Position in this buffer of the first object that hasn't
//  been returned to the client.
uint32_t offset;

//  Holds statistics about line lengths of the form <length, count>
//  where length is the number of characters in a line (including
//  the newline), and count is the number of lines with
//  exactly that many characters. If there are no lines with
//  a particular length, then there is no entry for that length.
private TreeMap<Integer, Integer> numLinesWithLength;

• What it demonstrates: precise meaning, units (characters, so “length” replaces “width,” which suggests pixels), and what a missing entry means. The better name carries information too.

Nouns, not verbs:

/* True means that a heartbeat has been received since the last time
 * the election timer was reset. Used for communication between the
 * Receiver and PeriodicTasks threads.  */
private boolean receivedValidHeartbeat;

A higher-level block comment. The original said “If there is a LOADING readRpc using the same session as PKHash pointed to by assignPos, and the last PKHash in that readRPC is smaller than current assigning PKHash, then we put assigning PKHash into that readRPC.” The better one:

// Try to append the current key hash onto an existing
// RPC to the desired server that hasn't been sent yet.

From this a reader can infer what the loop over RPCs, the session test, the LOADING test, and the MAX_PKHASHES_PERRPC test are for; only the maxPos test is left unexplained.

A method interface comment (Doxygen syntax) that defines range errors out of existence:

/**
 * Copy a range of bytes from a buffer to an external location.
 *
 * \param offset
 *        Index within the buffer of the first byte to copy.
 * \param length
 *        Number of bytes to copy.
 * \param dest
 *        Where to copy the bytes: must have room for at least
 *        length bytes.
 *
 * \return
 *        The return value is the actual number of bytes copied,
 *        which may be less than length if the requested range of
 *        bytes extends past the end of the buffer. 0 is returned
 *        if there is no overlap between the requested range and
 *        the actual buffer.
 */
uint32_t
Buffer::copy(uint32_t offset, uint32_t length, void* dest)

A cross-module comment at the end of RAMCloud’s Status enum lists every other update needed when you add a status: (1) update STATUS_MAX_VALUE and keep it last; (2) add entries to the “messages” and “symbols” tables in Status.cc; (3) add an exception class to ClientException.h; (4) add a case to ClientException::throwException; (5) and (6) add the exception class and its case in ClientException.java; (7) add it to Status.java in the right position. The comment sits at the end because new values are added there.

Worked Example

Documenting the IndexLookup class (RAMCloud). Applications use it for range queries over a distributed index:

query = new IndexLookup(table, index, key1, key2);
while (true) {
    object = query.getNext();
    if (object == NULL) {
        break;
    }
    ... process object ...
}

1. Which facts belong in the interface comment? The message format sent to servers: no, it is an implementation detail. The comparison function (integers, floats, or strings): yes, users need it. The data structure for indexes on servers: no, not even IndexLookup should know it. Whether requests go to servers concurrently: possibly, at a high level, because users may care about performance. How server crashes are handled: no, because recovery is automatic and invisible; if crashes did reach applications, the comment would say how they show up, but not how recovery works.
2. The original class comment named the LookupIndexKeys and IndexedRead RPCs, listed private configuration parameters, told users to include IndexLookup.h, and said “by providing all necessary information,” which says nothing.
3. The better comment: “This class is used by client applications to make range queries using indexes. Each instance represents a single range query.” It then explains that a client creates an instance, calls getNext() to retrieve objects, and calls getKey(), getKeyLength(), getValue(), and getValueLength() for each object. It leaves out NULL returns (that belongs in getNext’s own comment) and server crashes.
4. The isReady method. The original mentioned a “DCFT module,” a “rule-based approach,” and the internals of getNext, and its first sentence (“Check if the next object is RESULT_READY”) was cryptic. The better comment says the method “Indicates whether an indexed read has made enough progress for getNext to return immediately without blocking”, that it does most of the real work and so must be invoked (directly or through getNext) for the read to progress, and that true means the next getNext won’t block (an object is available or the end has been reached). Useful implementation notes move inside the method body.

Key Takeaways

1. Write an interface comment for every class and method, and a comment for every class variable.
2. Apply the repeat test; rewrite any comment a stranger could write from the adjacent code.
3. For variables, add precision: units, boundaries, null, ownership, invariants.
4. For code blocks, add intuition: what the block is trying to do and why it runs.
5. Keep implementation details out of interface comments, and put them inside the method.
6. Put cross-module documentation where developers will naturally look, or in a designNotes file with pointers.
7. Judge “obvious” from the first-time reader’s point of view.

Connects To

• Ch 4: formal and informal interfaces; comments describe the informal part.
• Ch 10: the Buffer::copy comment shows a method that defines range errors away.
• Ch 12: why comments matter.
• Ch 14: names carry part of the precision.
• Ch 15: writing comments first, and comments as a design canary.
• Ch 16: keeping comments near code and avoiding duplication.
• Ch 18: obvious code reduces how much needs commenting.