Comment Why, Not What

I've been involved with interviews and the hiring process at DealerOn for a number of years. Part of this process is to administer a code test, where we give the candidate their choice of a few non-strenuous coding challenges, and they take a crack at solving the prompt (at their leisure) and sending us their code that they wrote. There are some patterns that we commonly notice on junior candidates fresh out of college, most notably in the comments in their code.

Students often have it hammered into them by their computer science professors to leave lots of comments in their code, sometimes to the point of a student being penalized if the grader feels there aren't enough of them in his or her submitted work. The problem is, a high quantity of low-quality comments can often make the code more confusing than if there were none at all.

Take a look at this example:

/// <summary>
/// This method gets the sales targets for 
/// each day of the week from the database.
/// </summary>
/// <param name="employeeName">The employee's name</param>
/// <returns>A Dictionary</returns>
public Dictionary<string, decimal> GetSalesTargets(string employeeName) 
{
  // create a new array
  var employees = new Dictionary<string, decimal>(); 
  if (employeeName != "Charlie") // check if employee is Charlie
  {
    // employee is not Charlie
    // add Monday to the array
    employees.Add("Monday", GetSalesTarget("Monday", employeeName)); 
    // add Tuesday to the array
    employees.Add("Tuesday", GetSalesTarget("Tuesday", employeeName)); 
    // add Wednesday to the array
    employees.Add("Wednesday", GetSalesTarget("Wednesday", employeeName)); 
    // add Thursday to the array
    employees.Add("Thursday", GetSalesTarget("Thursday", employeeName)); 
    // add Friday to the array
    employees.Add("Friday", GetSalesTarget("Friday", employeeName)); 
  }
  else
  {
    // employee is Charlie
    // add Tuesday to the array
    employees.Add("Tuesday", GetSalesTarget("Tuesday", employeeName)); 
    // add Wednesday to the array
    employees.Add("Wednesday", GetSalesTarget("Wednesday", employeeName)); 
    // add Thursday to the array
    employees.Add("Thursday", GetSalesTarget("Thursday", employeeName)); 
    // add Friday to the array
    employees.Add("Friday", GetSalesTarget("Friday", employeeName)); 
    // add Friday to the array
    employees.Add("Saturday", GetSalesTarget("Saturday", employeeName)); 
  }
  // return the array
  return employees;
} 

Did you spot all the problems with those comments? For starters, the variable we're adding the days of the week to is a Dictionary, not an array, and why is it called employees if it's working with the days of the week? Also, comments like create a new array, add to the array, and return the array are unnecessary because any programmer can look at the syntax of those lines and clearly understand what is happening, even if the language itself is unfamiliar. The main problem with this code is, these comments are answering "What?" when they should be answering "Why?".

But why?

You probably found yourself saying "but why?" at several points reading the above code. The code has comments, but all they are doing is explaining what the code is doing at that moment; they do a poor job of explaining the things that actually need clarification. Imagine a new developer tasked with making a slight modification to this code. They may be struck with questions such as:

• For what purpose are we retrieving this data?
• Shouldn't Saturday and Sunday always be included in the returned value?
• Why is Charlie a special case?
• What format must the employee's name be in for the parameter? Will it work if the employee's last name is given as well?
• What service will be using this data?

None of these questions are answerable with the code as it currently is, leaving our poor developer to scratch his or her head for a while and then ask for help. Let's rewrite this method to be cleaner and to answer the above questions.

/// <summary>
/// This method gets the sales targets for a given employee for 
/// each day of this employee's work week, formatted for displaying 
/// on the company's sales dashboard.
/// </summary>
/// <param name="employeeFirstName">The employee's first name, as recorded 
/// in this company's LDAP server.</param>
/// <returns>A Dictionary mapping the employee's work days to their 
/// sales goal for that individual day of the week.</returns>
public Dictionary<string, decimal> GetDailySalesTargetsForEmployee(string employeeFirstName) 
{
  var salesTargets = new Dictionary<string, decimal>();
  var normalSchedule = new[] { "Monday", "Tuesday", "Wednesday", "Thursday", "Friday" };
  var modifiedSchedule = new[] { "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday" };

  if (!EmployeeWorksModifiedSchedule(employeeFirstName))
  {
    foreach (var day in normalSchedule) 
    {
      salesTargets.Add(day, GetSalesTarget(day, employeeFirstName));
    }
  }
  else
  {
    foreach (var day in modifiedSchedule) 
    {
      salesTargets.Add(day, GetSalesTarget(day, employeeFirstName));
    }
  }
  return salesTargets;
}

This can, of course, be condensed further by using a ternary expression or some LINQ. Regardless, this version of the method is much more readable, and a new developer would have a better idea of what's going on, saving everyone some time. It's amazing how much more clear the code can be with a modest refactor and by using more specific naming. This code answers most questions an otherwise unfamiliar developer would have, all without a single comment in the method body.

Comments can become out of date due to inattentive developers. Did you notice in the first example, in addition to the array/Dictionary mistakes, there's a copy/paste error, probably by someone that copied the original method in order to support Saturdays, and forgot the change the final comment. This is a tame example, but there is a phenomenon that, when doing any rote task such as copying and pasting a line of code over and over, more errors are made on the final iteration than any other. This is the so-called "Last-Line Effect". Comments become invisible when you're in the code for too long, but it happens to the best of us. At DealerOn we have a pretty infamous method still in our codebase which has comments stating that the method checks a string for the word "checked" and returns a Boolean, when it actually checks a string for a different word entirely and returns 0 or 1. This code isn't referenced anymore, but it still exists as a cautionary tale.

Comment Where Necessary

To be clear, I'm not saying there is no place for comments in modern development, nor do we at DealerOn judge code test submissions harshly for using too many comments. We suggest candidates do the code prompt like they would a school assignment, and if they are accustomed to verbose comments, that's perfectly fine. Speaking for myself however, if I'm unsure whether or not to leave a comment, I ask myself if what the code is doing is unclear, and if it can't be made more clear with a refactor. That last part is important: if the code itself can be made easier to understand, do it!

If a refactor isn't in scope for the unit of work, I use comments when something looks counter-intuitive, like when a method needs to account for an edge-case but it's unclear why. They're mostly "Trust me, I know what I'm doing" comments. There's a lot to be said about self-documenting code, code which is clear and easy to read, without resorting to method body comments.