Code Comments

What kind of code warrants a code comment? What makes a good code comment? How should I know whether to leave a code comment or no comment at all?

Code commenting isn't a refined science but can greatly improve the readability and longevity of code if done well. There are some general rules of thumb we can follow to be more effective in writing comments. Let's answer these questions about comments together.

Code Comments ¶

A code comment should answer "why", not "what". We don't need to explain "what" is happening in the code because code is expected to be verbose enough to understand. Instead, we should be answering "why" something is happening, such as a specific implementation detail of the product. If a comment doesn't answer "why" the code exists, it probably shouldn't be a comment.

This is the programmer version of "show, don't tell".

Let's go over a few real life examples from code:

// Due to floating point arithmetic precision issues in JS, sometimes
// the quantity divided by the "unit weight" results in strangly long
// floats. To avoid this in the UI, we must format our float using
// fixed-point notation (toFixed()) and coerce it back into a Number.
const quantity = Number((quantity / unitWeight).toFixed(0));

This is a great comment because it explains the reason why we take a numeric value, convert it to a string, then cast it back into a number. Without this comment, I might try to remove the toFixed() call because it seems redundant. This comment clarifies any questions on why the code exists.

const runtimeCaching = [
  {
    // only cache images
    urlPattern: /\.(?:png|jpg|jpeg|svg|webp|gif)$/,
    handler: 'StaleWhileRevalidate',
    options: {
      cacheName: 'images',
      expiration: {
        maxEntries: 200,
      },
    },
  },
]

This is a bad comment. It tells us something we might already infer from the urlPattern but it doesn't tell us why only images can be cached. Am I allowed to add other static media files (videos, pdfs)? Was this added for performance? What if we want to modify the served images? Bad comments not only create more questions than answers but also deincentivize future developers from touching the code. I don't want to introduce changes to this code I don't understand and potentially break something in production.

try {
  const ip = getForwardedForIp(ssrContext) ?? ''
  const [geolocation] = await Promise.all([
    fetchGeoLocationByIp(ip),
    fetchUserQueryAndOptOutRestrictions({
      ssrContext,
      queryClient,
      isAuthenticated,
    }),
  ])

  zipFromIp = geolocation.address?.zip_code ?? ''
} catch {
  // fail silently
}

This is a bad comment. It doesn't tell me why we want to silently catch errors. Do we not need to log these errors? Are we trying to prevent these promises from failing the parent function?

if resp.StatusCode != http.StatusNoContent { // For deletion, a 204 No Content is a standard response from CMS.
  return APIResponse{
    Status:     "error",
    Message:    "Failed to delete CMS page, status code: " + resp.Status,
    StatusCode: resp.StatusCode,
  }, nil
}

This is a good comment because it tells us why this condition specifically checks against http.StatusNoContent. It might be better worded for someone without context, but it answers the reason why the condition is written this way.

type SearchResult_Raw<T = Product_Raw> = {
  facets: FilterGroup_Raw[];
  hits: T[];
  total_hits: number;
  hits_per_page: number;
  page: number;
  shelves: any[]; // coming soon
  endcaps: Endcap_Raw[];
  beacons?: {
    onLoadBeacon?: string;
    featuredTransactionID?: string;
  };
  coupons: CouponClipped_Raw[]; // Unsure which is the better one to use, clipped or unclipped
  metadata: { status: string; redirect_url: string };
};

These are two bad comments. First, what is coming soon? And how soon is it coming (this code was committed four year ago)? Based on the surrounding code we can infer that the first comment refers to adding better typing to shelves, but it doesn't give a deadline and there's no actionability behind this comment. It would be better if a formal TODO was added with a link to a support ticket such as // TODO: use a more specific type https://example.com/my-internal-ticket-link.

The second comment is equally confusing. Is this a rhetorical question? I don't know what the answer is either! Again, this comment could have a ticket attached with a TODO if this is something we would like to investigate further down the road.

// Based on logs, this API frequently fails on the first attempt.
// Adding a single retry to reduce failure rates.
let response: TypedResponse<AllCartsResponse>;
try {
  response = await fetchWithAuth<AllCartsResponse>(Endpoints.ALL_CARTS, requestParams);
} catch {
  response = await fetchWithAuth<AllCartsResponse>(Endpoints.ALL_CARTS, requestParams);
}

This is a good comment because it explains the technical reason we fetch the same API again in the case of a failure. If I didn't have a comment here, I might think this was an accidental typo rather than an intentional choice.

When done well, code comments can make code review faster and reduce unnecessary back and forth communication between engineers and teams. When done poorly, code comments deincentivize future changes to code. Let's try to keep good comments throughout our codebases.

Bonus: PR Comments ¶

If you leave comments on your own pull request, they should be clarifying answers to questions before they pop up. If anything, it's better to leave great commit messages and a great pull request description before leaving comments. The goal of any pull request description or comment is to help get your pull request approved and merged faster.