Code Review: Please Remove Comments From Your Code

When I was a student in University, I learned from many people that I should make a fair amount of comments in my code to make sure other people can understand it. In an onboarding course of an outsourcing company that I participated in, the guide told me that comments should take an average of one-third of lines. I followed this guideline for the early years of my career, but I found that it’s not the right choice later on. These days, I usually ask other people to remove mostly all their comments in code review. This post will talk about the reason why I do so.

Comments Lie

That fact may surprise people, but there are many cases the comments lie. To explain that, let’s start with a small sample below. The function tends to download multiple files into the browser’s memory, compress them, and save the output to a local drive. Everything looked perfect when the developer finished his work. He explained everything that others may need on the header and at every step inside the function. The style is also what I used to prefer in the early days of my career.

/* 
 * Download and zip multiple files using browser, then save the zip file to user's local drive
 * 
 * Steps:
 * 1. Download all files to memory. Devide the files by patches, each patch contains maximum 5 files
 * 2. Zip the files in memory
 * 3. Download the zip file with default name follows this format:
 * `combined-<Current Date Time>.zip`
 */
const download = async urls => {
  // 1. Download all files to memory
  const blobs = await Promise.map(
    urls, 
    async url => {
      // Download the file
      const resp = await fetch(url);
      return await resp.blob();
    },
    {concurrency: 5} // Maximum 5 files per batch
  );

  // 2. Zip them
  const zip = JsZip();
  blobs.forEach((blob, i) => {
    zip.file(`file-${i}.csv`, blob);
  });
  const zipFile = zip.generateAsync({type: 'blob'})

  // 3. Download the zip file from browser
  return FileSaver.saveAs(zipFile, `combined-${new Date().getTime()}.zip`);
}

A few months later, the team decides to change. Instead of download five files concurrently, they want ten. Another developer took this task. He changed 5 to 10 and updated the inline comment. But he didn’t notice that he need to update the head comment as well. Later on, another developer changed the default filename from combined-<DateTime>.zip to compressed-<DateTime>.zip. And this guy also overlooked the head comment. During code review, because the comparison tool showed only changes, other developers also skipped the head comment.

/* 
 * Download and zip multiple files using browser, then save the zip file to user's local drive
 * 
 * Steps:
 * 1. Download all files to memory. Devide the files by patches, each patch contains maximum 5 files
 * 2. Zip the files in memory
 * 3. Download the zip file with default name follows this format:
 * `combined-<Current Date Time>.zip`
 */
const download = async urls => {
  // 1. Download all files to memory
  const blobs = await Promise.map(
    urls, 
    async url => {
      // Download the file
      const resp = await fetch(url);
      return await resp.blob();
    },
    {concurrency: 10} // Maximum 10 files per batch
  );

  // 2. Zip them
  const zip = JsZip();
  blobs.forEach((blob, i) => {
    zip.file(`file-${i}.csv`, blob);
  });
  const zipFile = zip.generateAsync({type: 'blob'})

  // 3. Download the zip file from browser
  return FileSaver.saveAs(zipFile, `compressed-${new Date().getTime()}.zip`);
}

The comments came with a good purpose and were very helpful at the beginning. But due to corporative history, it became a liar now. Some people may argue that it’s a problem of other uncareful people, not the comments themselves. But we are all human. No individual can ensure not to run into the same mistake. And while tests can help to prevent us not breaking existing features, no equivalent system can do the same thing for comments.

Comments demotivate us from writing descriptive code

Come back with the example above. While the comments can describe the code pretty well, they also discourage developers from thinking and writing code more descriptively. If the developer forced himself to remove all comments but still ensure readability, he might come with a better version. The example below demonstrate how he enhanced the original code to the downloadAndZipFiles function.

const download = async url => {
  const reps = await fetch(url);
  return await resp.blob();
};

const downloadByPatches = (urls, files_per_patch) => {
  return Promise.map(
    urls, 
    async url => {
      return await download(url);
    },
    {concurrency: files_per_patch}
  );
}

const exportZip = async blobs => {
  const zip = JsZip();
  blobs.forEach((blob, i) => {
    zip.file(`file-${i}.csv`, blob);
  });
  return await zip.generateAsync({type: 'blob'})
}

const generateDefaultFileName = () => {
  const currentDateTime = new Date().getTime();
  return `combined-${currentDateTime}.zip`;
}

const saveFile = (content, fileName) => {
  return FileSaver.saveAs(zipFile, fileName);
}

const NUMBER_FILE_PER_PATCH = 5

export const downloadAndZipFiles = urls => {
  const blobFiles = await downloadByPatches(urls, NUMBER_FILE_PER_PATCH);
  const zipFile = await exportZip(blobFiles);
  saveFile(zipFile, generateDefaultFileName());
}

Instead of commenting on each step, he applied guidelines from clean codes:

1. Use small functions. Each function came with a single responsibility.
2. Use a descriptive name for all functions and variables.

They are small changes but can make his code much more clean and clear. And I can be pretty sure that other people can understand this code without any comment in the 1st sample.

Test Is A Safer Way To Explain Feature

Most of the case, we can describe the functionality of a function using its unit tests. If you still see some hidden logic, it’s more likely that you should add more tests. Using tests strictly also helps for code review. If a developer makes changes on implementation but didn’t introduce corresponding updates on tests, it’s more likely that he needs to review and add more work. Below is a simple example comparing comments and unit tests.

/*
 * Validate a password string
 * The string should contains:
 * - At least 8 characters
 * - At least 1 symbols
 * - At least 1 lowercase character
 * - At least 1 upercase character
 */
const validatePassword = pw => {
  ...
}

// Or we can remove all comments and describe this function by unit tests as below
describe("validatePassword", () => {
  ...

  it("should return error if password length is less than 8 characters", () => {
    ...
  });

  it("should return error if password doesn't contain at least 1 symbol", () => {
    ...
  });

  it("should return error if password doesn't contain at least 1 lowercase character", () => {
    ...
  });

  it("should return error if password doesn't contain at least 1 upercase character", () => {
    ...
  });

  ...
})

Commented Code Can Be Safely Removed

Many people have a habit of commenting out code instead of removing them completely. This habit has a historical cause. When the versioning tool was not powerful enough in the old days, tracing changes on a module was a bit hard. I remember when I was a student, some senior guys advised me to do so. “Don’t delete any code. Comment them out with your name and an apparent reason on top of that”. That guideline is obsolete these days when we can trace changes within a few clicks. I see those commented codes like garbages. We should never keep them in any commit.

Some Exceptions

There are still exceptional comments that we should keep. I highly recommend maintaining comments or even adding more details on those rare cases.

“Why” Comments

The comments that explain why the code exists or designs that way are usually valuable. Lacking context often leads developers to confusion, questioning, or even blaming the past works. Some candidates for this type of comments are:

• Decisions from the team that developer think it could be strange for others later on
• Limitation of the codebase or framework
• Agreed workaround for a well-known bug
• …

Exceptional “Hard To Read”

We should add comments when the code contains regular expressions or complex algorithms. Those parts are usually hard to read and often greatly benefit from the comments.

=====

This article is part of my series about Code Review, which talks about my notes while reviewing code. Each of them could be tiny or nothing special for some people. But because a slight mistake can cost developers a lot to figure out, it could be helpful for others to mind all the possibilities.

For someone may need, the sample snippets above came from one of my posts: Download Files And Zip Them In Your Browsers Using Javascript.