Stop Commenting Your Code

Heavy code commenting is a symptom of bad code.

Most of the time, if you feel like you need to add a comment to explain what a piece of code is doing, your code is probably too complex and should be refactored.

Try to make sense of this sample snippet of code for a PurchaseProcessor module of an imaginary Acme.com department store.

/**
Grabs the shopping cart, searches for the items that have been in the cart of more than 30 days, identified by 'old' flag. Then removes the said items. Sets the 'newProduct' indicator to 'Y' if the customer's shipping state is eligible for Product 2.0. Makes the POST to the service. 
*/
function post() {
   let items = cartManager.getItems();
   //filter out items that are over 30 days old because customer won't be able to purchase them
   items = items.filter(item => item.old === false);
   items.forEach(item => {
   //get the available features resource from the model - needs true to be passed in if the shop is Acme.com

if(modelAccessor.getAvailableFeatures(true).getClearanceProductIndicator(item)) {
   item.saleProduct = 'Y';
}
});

  //generate the XHR params and post to the back end
  xhrManager.post(this.generatePostParams(items)).then(() => {
  //sometimes the items are not deleted from the cart after purchase, if there are any left, purge the cart
  if(cartManager.getItems().length > 0) {
     cartManager.purge();
  } 
});
}

Crystal clear? Not really.

So what’s wrong with the commenting of the above snippet?

Problem# 1

Grabs the shopping cart, searches for the items that have been in the cart of more than 30 days, identified by 'old' flag. Then removes the said items. Sets the 'newProduct' indicator to 'Y' if the customer's shipping state is eligible for Product 2.0. Makes the POST to the service.

Flaw

The function-level comment is too wordy, does not provide a clear description of what the method is intended for. It attempts to explain everything that is happening inside, but even then, it misses some of the key details.

Solution

Keep your function-level comments concise and clear. You should be able to explain the one thing that function is doing. If you can’t — you should probably break down the function.

Makes the POST to the purchase service.

Problem# 2

//filter out items that are over 30 days old because customer won't be able to purchase them

Flaw

The intended purpose of the comment might have been to explain that the old property means “more than 30 days old”. Is that a relevant detail in this context? Will someone reading this method in the future need to know that specific detail?

Solution

It’s best to move the filtering functionality into a new function and limit comments to only what is relevant within the context.

/**
Removes products older than 30 days.
*/
function filterProductsOlderThan30Days(items) {
   items = items.filter(item => item.old === false);
}

Problem# 3

//get the available features resource from the model - needs true to be passed in if the shop is Acme.com
if(modelAccessor.getAvailableFeatures(true).getClearanceProductIndicator(item))

//get the available features resource from the model - needs true to be passed in if the shop is Acme.com
if(modelAccessor.getAvailableFeatures(true).getClearanceProductIndicator(item))

Flaw

There are many problems with this comment and implementation. Why are we passing in a true boolean to the getAvailableFeatures function? What are available features? Why are we getting and setting some clearance product indicator when the customer is trying to purchase?

Does it work? Maybe. But the code is confusing, hence the need for a comment.

Solution

Instead of hardcoding a true parameter to the getAvailableFeatures call — can we build that functionality into the getAvailableFeatures function itself? Rather than performing this indicator assignment on POST of something, can we ensure that the items already have the indicator assigned by the time this function is executed?

/**
Checks to see if the item is eligible to be sold as a clearance product and assigns an indicator.
*/
function mapInNewProductIndicator(item) {
   const availableFeatures = modelAccessor.getAvailableFeatures();
   if(availableFeatures && availableFeatures.getClearanceProductIndicator(item)) {
     item.saleItem = 'Y';
   }
}

Problem# 4

//generate the XHR params and post to the back end
xhrManager.post(this.generatePostParams(items)).then(() => {

Flaw

This comment is explaining a straightforward section of code — it’s simply not needed.

Solution

Remove the comment.

Problem# 5

//sometimes the items are not deleted from the cart after purchase, if there are any left, purge the cart

if(cartManager.getItems().length > 0) {
   cartManager.purge();
}

Flaw

If the comment wasn’t there — would someone reading the actual code understand what it’s doing? Probably. Also, it seems like this kind of cleanup is potentially unnecessary — why do the items get cleared sometimes, but not always? Is there a bug somewhere?

Solution

Remove the comment. Move the logic that checks whether items exist into the cartManager, it makes more sense to have it there rather than in a POST method. Try to understand why the code is necessary to begin with and remove if possible.

Refactored Result

/**
Makes the POST to the purchase service.
*/
function post() {
   let items = cartManager.getItems();
   this.filterProductsOlderThan30Days(items);
   items.forEach(item => {
      this.mapInNewProductIndicator(item);
  });

  xhrManager.post(this.generatePostParams(items)).then(() => {
     cartManager.purge();
  });
}

/**
Removes products older than 30 days.
*/
function filterProductsOlderThan30Days(items) {
   items = items.filter(item => item.old === false);
}

/**
Checks to see if the item is eligible to be sold as a clearance product and assigns an indicator.
*/
function mapInNewProductIndicator(item) {
   const availableFeatures = modelAccessor.getAvailableFeatures();
   if(availableFeatures &&  availableFeatures.getClearanceProductIndicator(item)) {
     item.saleItem = 'Y';
   }
}

By modularizing the code we can eliminate the need for most comments.

The descriptive method names also help explain what different chunks of code are responsible for, eliminating the need for comments.

If you spend 10 more minutes optimizing this code, you could clean it up even more, but you can quickly see how comments are often a symptom of sub-optimal code.

If you’re passionate about JavaScript development, check out the JavaScript Works job-board here!