Writing useful comments

In his book Clean Code, Robert C. Martin makes a strong case against comments:

You don’t need comments if you write clean code.

Like any blanket statement, Martin is partially true. Although code clarity increases readability and decreases the need for comments, code has never been able to convey the whole context to the reader.

Programming patterns are often cited as a way to convey this context on a codebase scale. Well named functions and variables also convey some form of context. But most of the time, the context (the “why”) never gets written anywhere.

In this article, I present a few examples meant to showcase how comments may be written to convey the context around code.

Contents:

1. Code example 1
2. Code example 2
3. Code example 3
4. Code example 4
5. Shaping comments
6. Conclusion

Code example 1

I call “in-flight comments” comments that we write to ease the process of writing code. In-flight comments help us articulate what we are trying to achieve.

In the following example taken from the Google’s Technical Writing, the developer wrote a comment as they were writing an algorithm for randomly shuffling a slice of integers:

func Shuffle(slice []int) {
  curIndex := len(slice)

    // If the current index is 0, return directly.
    if (curIndex == 0) {
        return
    }

    ...
}

This comment is a typical in-flight comment: it only focuses on what the code does. This comment does not give context as to why this if statement exists. We can refactor this comment by focusing on the “why”:

func Shuffle(slice []int) {
  curIndex := len(slice)

    // No need to shuffle an array with zero elements.
    if (curIndex == 0) {
      return
    }

    ...
}

The reader now understand why this if exists, and does not have to dig further. Starting with the “why” helps glanceability: the reader only needs to read the first few words to get the idea.

Here is how I would diffenciate the “why” from the “what”:

*Sometimes, the “what” may be valuable for readability purposes. I see three reasons to comment on the “what”:

1. When the code is not self-explanatory, a “what” comment may avoid the reader the struggle of googling;
2. When a block of code is lenghy, adding “what” comments may help create “sections”, helping the reader quicly find the part they are interested in.

Code example 2

Our next example comes from the cert-manager codebase:

// If the certificate request has been denied, set the last failure time to
// now, and set the Issuing status condition to False with reason. We only
// perform this check if the request also doesn't have a Ready condition,
// since some issuers may not honor a Denied condition, and will sign and
// set the Ready condition to True anyway. We would still want to complete
// issuance for requests where the issuer doesn't respect approval.
cond := apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionReady)
if cond == nil {
    if apiutil.CertificateRequestIsDenied(req) {
        return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionDenied))
    }
    return nil
}

After looking at the if statement, the reader wonders: why do we need this if statement?

Their first reaction will probably be to take a look at the comment right above. Unfortunately, the comment starts with even more confusing implementation details. The reader has to keep reading until the 6th line to find out why the code is doing what it is doing.

Let us rate each individual information that the comment conveys:

From the reader’s perspective, the paragraphs (D) and (C) are the most useful. They give a high-level overview of why this if statement exists. We can merge the two paragraphs into one:

(C, D) External issuers may ignore the approval API. An issuer ignores the approval API when it proceeds with the issuance even though the “Denied=True” condition is present on the CertificateRequest. To avoid breaking the issuers that are ignoring the approval API, we want to detect when CertificateRequests has ignored the Denied=True condition; when it is the case, we skip bubbling-up the (possible) Denied condition.

Paragraphs (B) and (A) may also be useful: they give the context about paragraph (C). The sentence lacks precision though, and “this check” may confuse the reader. Let us rephrase it:

(A, B) To know when the Denied=True condition was ignored by the issuer, we look at the CertificateRequest’s Ready condition. If both the Ready and Denied

// Some issuers won't honor the "Denied=True" condition, and we don't want
// to break these issuers. To avoid breaking these issuers, we skip bubbling
// up the "Denied=True" condition from the certificate request object to the
// certificate object when the issuer ignores the "Denied" state.
//
// To know whether or not an issuer ignores the "Denied" state, we pay
// attention to the "Ready" condition on the certificate request. If a
// certificate request is "Denied=True" and that the issuer still proceeds
// to adding the "Ready" condition (to either true or false), then we
// consider that this issuer has ignored the "Denied" state.
cond := apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionReady)
if cond == nil {
    if apiutil.CertificateRequestIsDenied(req) {
        return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionDenied))
    }
    return nil
}

Code example 3

Another example inspired by the cert-manager codebase:

errs := validateIngressTLSBlock(tls)
// if this tls entry is invalid, record an error event on Ingress object and continue to the next tls entry
if len(errs) > 0 {
    rec.Eventf(ingress, "Warning", "BadConfig", fmt.Sprintf("TLS entry %d is invalid: %s", i, errs))
    continue
}

Once again, the in-flight comment can be rewritten to focus on the “why”:

// Let the user know that an TLS entry has been skipped due to being invalid.
errs := validateIngressTLSBlock(tls)
if len(errs) > 0 {
    rec.Eventf(ingress, "Warning", "BadConfig", fmt.Sprintf("TLS entry %d is invalid: %s", i, errs))
    continue
}

Code example 4

The next example is inspired by an other place in cert-manager.

This is another example of in-flight comment focusing on the “what”. The context around this block of code is not obvious, which means this comment should refactored to focus on the “why”.

// check if a Certificate for this secret name exists, and if it
// does then skip this secret name.
expectedCrt := expectedCrt(ingress)
existingCrt, _ := client.Certificates(namespace).Get(ingress.secretName)
if existingCrt != nil {
    if !certMatchesUpdate(existingCrt, crt) {
        continue
    }

    toBeUpdated = append(toBeUpdated, updateToMatchExpected(expectedCrt))
} else {
    toBeCreated = append(toBeCreated, crt)
}

In this case, the code itself would benefit from a bit of refactoring. Regarding the comment, we start with the “why”:

// The secretName has an associated Certificate that has the same name. We want
// to make sure that this Certificate exists and that it matches the expected
// Certificate spec.
existingCrt, _ := client.Certificates(namespace).Get(secretName)
expectedCrt := expectedCrt()

if existingCrt == nil {
    toBeCreated = append(toBeCreated, expectedCrt)
    continue
}

if certMatchesExpected(existingCrt, expectedCrt) {
    continue
}

toBeUpdated = append(toBeUpdated, updateToMatchExpected(expectedCrt))

Note that we removed the else statement for the purpose of readability. The happy path is now clearly “updating the certificate”.

Shaping comments

To help with readability, I suggest hard-wrapping comments at 80 characters. Above 80 characters, the comments will become hard to read in PR suggestion comments. Take this example comment:

// To reduce the memory footprint of the controller-manager,
// we do not cache Secrets.
// We only cache the metadata of Secrets
// which is enough to allow the EnqueueRequestsFromMapFunc to lookup the associated Issuer or ClusterIssuer.
// In NewManager we also use the option ClientDisableCache
// to configure the client to not cache Secrets
// when the token manager Gets and Lists them.

Each sentence is started on a new line. This sort of style is fine in a Markdown document where these soft breaks are rendered as a single paragraph.

The problem with the “random” length of each line is that the reader has to do more work while parsing the text. It is also less aesthetic than a well-wrapped 80 characters comment. Rewrapping the above comment at 80 chars looks like this:

// To reduce the memory footprint of the controller-manager, we do not
// cache Secrets. We only cache the metadata of Secrets which is enough to
// allow the EnqueueRequestsFromMapFunc to lookup the associated Issuer or
// ClusterIssuer. In NewManager we also use the option ClientDisableCache
// to configure the client to not cache Secrets when the token manager Gets
// and Lists them.

You can obtain the re-wrapping by using the qq command on Vim, or the Rewrap extension on VSCode.

If you still want to do separate paragraphs, I’d recommend separating each paragraph with two new line breaks:

// To reduce the memory footprint of the controller-manager, we do not
// cache Secrets. We only cache the metadata of Secrets which is enough to
// allow the EnqueueRequestsFromMapFunc to lookup the associated Issuer or
// ClusterIssuer.
//
// We use this option along with ClientDisableCache in NewManager to
// configure the client to not cache Secrets when the token manager Gets
// and Lists them.

Conclusion

As a developer, I want to write code that is readable and maintainable, and keeping track of the “why” (i.e., the context) in a codebase is essential for readability and maintainability. I found that putting myself in the place of the future reader helps me write comments that focus on the “why”.

HAProxy, Git or the Linux kernel are great examples of projects where there is a great focus on well-documented code:

• ebtree/ebtree.h (HAProxy)
• unpack-trees.c (Git)
• kernel/sched/core.c (Linux kernel).

Updates:

• 24 Feb 2022: add the section “Shaping comments”.