Comments can be a sign of poorly designed code

Consider the following snippets of commented code:

// Check if the subscription is active.
if (subscription.endDate > Date.now) {  }

interface FormState {
    user: User | null; // the logged in user
    formErrors: FormErrors | null; // if this is not null, then the user can't submit the form and a popup should be displayed alerting the user to fix their errors
}

In both of these cases, there is knowledge about the code that the author felt a need to indicate using a comment that described what the code is doing. These types of comments are often referred to as "what" comments because they describe "what" the code is doing. In his book "Clean Code: A Handbook of Agile Software Craftsmanship", Robert C. Martin writes that a programmer should try to avoid mental mapping:

Readers shouldn’t have to mentally translate your names into other names they already know. This problem generally arises from a choice to use neither problem domain terms nor solution domain terms. (p.25)

Many, including Robert C. Martin, Kent Beck, and Martin Fowler have written on the idea that comments can easily fall out of sync with code and thereby express lies. In Clean Code, Martin suggests:

So when you find yourself in a position where you need to write a comment, think it through and see whether there isn’t some way to turn the tables and express yourself in code. (p.54)

Similarly, and looking at this another way, Dave Thomas and Andy Hunt, in their book "The PRagmattic Programmer" write that

EVERY PIECE OF KNOWLEDGE MUST HAVE A SINGLE, UNAMBIGUOUS, AUTHORITATIVE REPRESENTATION WITHIN A SYSTEM. (p. 27)

One form of Duplication, they write, is "Inadvertent duplication":

Developers don’t realize that they are duplicating information (p. 27)

In discussing code comments, they discuss the trade-off between wanting to have comments and ensuring that we don't have too many or the wrong type of comments:

Code should have comments, but too many comments can be just as bad as too few.

In general, comments should discuss why something is done, its purpose and its goal. The code already shows how it is done, so commenting on this is redundant—and is a violation of the DRY principle. (p. 249)

Okay, so let's go back to the examples and see how we can apply this:

The first example comes from clean-code-typescript on Github:

Bad

// Check if the subscription is active.
if (subscription.endDate > Date.now) {  }

Good

const isSubscriptionActive = subscription.endDate > Date.now;
if (isSubscriptionActive) { /* ... */ }

Here, the suggestion is to use a meaningful variable name and extract the logic into the variable declaration. Notice the name isSubscriptionActive does not fully take the place of the comment Check if the subscription is active., but rather expresses the concept of "subscription is active" explicitly so that the code if(isSubscriptionActive) no longer requires a comment.

Let's look at the second example:

interface FormState {
    user: User | null; // the logged in user
    formErrors: FormErrors | null; // if this is not null, then the user can't submit the form and a popup should be displayed alerting the user to fix their errors
}

In documenting an interface, using Javadoc-style comments will generally allow a code editor or IDE to display documentation inline when data is used. Let's start by making that change:

interface FormState {
    /**
     * The logged-in user
     */
    user: User | null;
    /**
     * if this is not null, then the user can't submit the form
     * and a popup should be displayed alerting the user to fix
     * their errors
     */
    formErrors: FormErrors | null;
}

The second comment now looks clearly out of place. It describes how the data is intended to be used, but that knowledge is now separated from where the logic actually occurs. If we change the logic in the code to, say allow a user to submit a form with errors and rely on the backend to decide if an invalid form submission is allowed, we are likely to forget to update this comment and then it becomes a lie.

At the same time, we'd like to represent that the formErrors field in this interface will be used, not only to observe the errors but also to observe whether or not there are errors. We can represent this as follows:

interface FormState {
    /**
     * The logged-in user
     */
    user: User | null;
    /**
     * The errors present while editing the form.
     * 
     * If this is null, the form is considered valid; otherwise, invalid.
     */
    formErrors: FormErrors | null;
}

We can further refine this to change the name of the field to indicate that these errors are derived while editing the form, and not after a separate validation phase on the server:

interface FormState {
    /**
     * The logged-in user
     */
    user: User | null;
    /**
     * The transient errors in the form (i.e. present while editing).
     * 
     * If this is null, the form is considered valid; otherwise, invalid.
     */
    transientFormErrors: FormErrors | null;
}

We can make a similar modification for the other field:

interface FormState {
    /**
     * The logged-in user.
     * 
     * If this is null, the user is unauthenticated
     */
    loggedInUser: User | null;
    /**
     * The transient errors in the form (i.e. present while editing).
     * 
     * If this is null, the form is considered valid; otherwise, invalid.
     */
    transientFormErrors: FormErrors | null;
}

Lastly, if we have logic in our code such as the following:

if (formErrors !== null) {
    showErrorAlert(formErrors);
}

We may wish the express the concepts that we had previously documented in our interface more explicitly using helper/utility functions:

interface FormState {
    /**
     * The logged-in user.
     * 
     * If this is null, the user is unauthenticated
     */
    loggedInUser: User | null;
    /**
     * The transient errors in the form (i.e. present while editing).
     * 
     * If this is null, the form is considered valid; otherwise, invalid.
     */
    transientFormErrors: FormErrors | null;
}

function isValid(formState: FormState) {
    return formState.transientFormErrors === null;
}

function canSubmitForm(formState: FormState) {
    return isValid(formstate);
}

function displayFormErrorPopup(formState: FormState) {
    showErrorAlert(formErrors);
}

The form logic then becomes:

if (!canSubmitForm(formState)) {
    displayFormErrorPopup(formState.transientFormErrors);
}

Later, if we want to change the logic to always send the form to the server, we can change canSubmitForm to return true. Alternatively, we may decide that we want to provide the user with a message about their errors, but still allow them to submit the form:

function isValid(formState: FormState) {
    return formState.transientFormErrors === null;
}

function canSubmitForm(formState: FormState) {
    return isValid(formstate) || formState.userRequestedInvalidFormSubmission;
}

function displayFormErrorPopup(formState: FormState) {
    formState.userRequestedInvalidFormSubmission = showErrorAlert(formErrors);
}

Hopefully, this brief exposition of the software engineering literature on code comments and how they affect the "DRYness" of code helps you write better comments in your own code. If you have any ideas for how I could improve this article or anything else you wish I'd discuss, shoot me an email at tzvi@tzvipm.dev. Happy coding :)