The Code that Cried Wolf: Proving that Comments are Unneeded
I spent a lot of years diligently commenting my code. I wrote what the code did next to the statements themselves and I wrote method headers and sometimes I even kept a journal of edits in that header as if there were no such thing as source control. As I mentioned in an earlier post, I’ve since drifted further and further from this approach to the point of almost never commenting my code if left to my own devices. The reason for this gradual but near-complete reversal is that I’ve come to view comments as a crutch that prevents code from standing on its own from a readability perspective.
For instance, consider the following class (and before we go any further, I would just like to mention that the relationship between the “Prepper” and “Employee” is not actually how I would design this — this is just an example for demonstrating a point):
public class EmployeePrepper
{
private readonly IEnumerable _employees;
public EmployeePrepper(IEnumerable employees)
{
_employees = employees;
}
public void SetupEmployees()
{
foreach (var employee in _employees)
{
if (string.IsNullOrEmpty(employee.FirstName))
throw new Exception();
if (string.IsNullOrEmpty(employee.LastName))
throw new Exception();
if (employee.LastName.Length < 3)
employee.LoginName = employee.FirstName + employee.LastName;
else
employee.LoginName = employee.FirstName[0] + employee.LastName;
}
}
}
Imagine that you've asked for a code review and someone says "well, it's a little hard to read and follow -- you've got some magic numbers in there and it's not all that readable." What suggestion is coming next? Or, if none is forthcoming, what do you do to address this? Something like the next code snippet?
///
/// This class takes a bunch of employees that the user entered and sets up derived properties on them
///
public class EmployeePrepper
{
///
/// This is a collection of employees that the constructor will pass in
///
private readonly IEnumerable _employees;
///
/// This constructor takes the class's dependency and initializes it
///
/// The employees this class will work on
public EmployeePrepper(IEnumerable employees)
{
_employees = employees;
}
///
/// Sets up the employees by setting derived properties
///
public void SetupEmployees()
{
//Cycle through the employees
foreach (var employee in _employees)
{
//Make sure a valid first name and last name are entered
if (string.IsNullOrEmpty(employee.FirstName))
throw new Exception();
if (string.IsNullOrEmpty(employee.LastName))
throw new Exception();
//If the employee has a very short last name, then use first concatenated to last
if (employee.LastName.Length < 3)
employee.LoginName = employee.FirstName + employee.LastName;
else //Otherwise, use first initial concatenated to last name
employee.LoginName = employee.FirstName[0] + employee.LastName;
}
}
}
Ah, much better, right? From the outside looking in, with IntelliSense, I learn that (1) the class takes a collection of employees and sets their derived properties, (2) the constructor takes a dependency and initializes itself, and (3) the setup employee method sets up the employees with their derived properties. Of course, I could learn that by (1) inspecting the constructor, (2) common sense, and (3) see item (1).
Okay, so maybe I don't learn all that much of interest from IntelliSense, but there's a lot of clarity in the method itself. I learn that (1) I'm going to cycle through records, (2) I'm going to check properties for validity and (3) a descriptive business rule for setting up login names. Of course, I could learn (1) by knowing what foreach() is and (2) by basic inspection. (3) is interesting in that it's the first comment that isn't just pointless filler. It actually explains something that I need to know.
So 1 out of 6 comments is helpful, but isn't that still a net gain? Well, it is if we assume that there is no downside to having commented code. But there is a downside. Specifically, the addition of comments to the file means extra artifact that must be maintained or else the comments drift from noise to lies. For example, what if we add a constructor parameter and don't pay attention to the doc comments (which pretty much no one does when editing classes)? Suddenly that comment about taking a single employee dependency is nonsensical. So you have 5 liabilities and 1 asset in terms of comments here, which seems like a poor tradeoff (although I'm not going to attempt to place an actual measurable value on the concept of misleading comments versus helpful, explanatory comments -- even if I could, it wouldn't matter much since it's pretty unlikely I'll read your comments anyway :) ).
Okay, I've sold you on my premise, but what do you say to the architects/leads/coworkers/project managers/etc who aren't buying what I'm selling? How do you convince them that this activity is somewhere between pointless and detrimental? Well, prove to them that the comments are pointless. Take away the crutch and make your class names, method names, variable names, and logic flow so clean and so self documenting that the comments look preposterous. Take the original code and turn it into this:
public class EmployeeDerivedPropertySetter
{
private const int LastNameMinimumLength = 3;
public class FirstNameEmptyException : Exception { }
public class LastNameEmptyException : Exception { }
private readonly IEnumerable _employeesPassedInFromConstructor;
public EmployeeDerivedPropertySetter(IEnumerable employees)
{
_employeesPassedInFromConstructor = employees;
}
public void SetupDerivedProperties()
{
foreach (var employee in _employeesPassedInFromConstructor)
SetupDerivedProperties(employee);
}
private static void SetupDerivedProperties(Employee employee)
{
ValidateFirstAndLastName(employee);
SetLoginName(employee);
}
private static void ValidateFirstAndLastName(Employee employee)
{
if (string.IsNullOrEmpty(employee.FirstName))
throw new FirstNameEmptyException();
if (string.IsNullOrEmpty(employee.LastName))
throw new LastNameEmptyException();
}
private static void SetLoginName(Employee employee)
{
if (IsEmployeeLastNameVeryShort(employee))
employee.LoginName = ConcatenateFirstAndLastNames(employee);
else
employee.LoginName = ConcatenateFirstInitialAndLast(employee);
}
private static bool IsEmployeeLastNameVeryShort(Employee employee)
{
return employee.LastName.Length < LastNameMinimumLength;
}
private static string ConcatenateFirstAndLastNames(Employee employee)
{
return employee.FirstName + employee.LastName;
}
private static string ConcatenateFirstInitialAndLast(Employee employee)
{
return employee.FirstName.ElementAt(0) + employee.LastName;
}
}
Now, small factored methods might not be your thing, particularly if you love debugger (or just love big methods and classes for some reason). But I think you'd be hard pressed to argue that this isn't quite readable and self explanatory. There's no method here that you'd look at, then squint at, then rub your head and say, "what the...?" You have things like "If the last name is very short, set login name to first concatenated to last." This class reads like an instruction manual or... kinda like the version with a bunch of comments. Except unlike that version, your code is actually the comments, so it's impossible for it to become obsolete and misleading.
But I'm not trying to tell you how to code -- I'm trying to tell you how to sell this. I'm trying to tell you how to prove that comments are unnecessary. "How," you ask? Well, put the comments back in and see how completely obtuse they now look:
public class EmployeeDerivedPropertySetter
{
///
/// The minimum length of the last name
///
private const int LastNameMinimumLength = 3;
///
/// Thrown when the first name is empty
///
public class FirstNameEmptyException : Exception { }
///
/// Thrown when the last name is empty
///
public class LastNameEmptyException : Exception { }
///
/// This is a collection of employees that the constructor will pass in
///
private readonly IEnumerable _employeesPassedInFromConstructor;
///
/// This constructor takes the class's dependency and initializes it
///
/// The employees this class will work on
public EmployeeDerivedPropertySetter(IEnumerable employees)
{
//Set the employee variable to the passed in value
_employeesPassedInFromConstructor = employees;
}
///
/// Sets up the employees by setting derived properties
///
public void SetupDerivedProperties()
{
//Cycle through the employees
foreach (var employee in _employeesPassedInFromConstructor)
SetupDerivedProperties(employee);
}
///
/// Setup the derived properties
///
/// Employee to set them up for
private static void SetupDerivedProperties(Employee employee)
{
//Make sure a valid first name and last name are entered
ValidateFirstAndLastName(employee);
//Set the employee's login name
SetLoginName(employee);
}
///
/// Validate the first and last name of the employee
///
/// The employee to validate
private static void ValidateFirstAndLastName(Employee employee)
{
//If first name is null or empty throw an exception
if (string.IsNullOrEmpty(employee.FirstName))
throw new FirstNameEmptyException();
//If last name is null or empty, throw an exception
if (string.IsNullOrEmpty(employee.LastName))
throw new LastNameEmptyException();
}
///
/// Set the employee's login name
///
/// Employee whose login name to set
private static void SetLoginName(Employee employee)
{
//If the employee has a very short last name, then user first concatenated to last
if (IsEmployeeLastNameVeryShort(employee))
employee.LoginName = ConcatenateFirstAndLastNames(employee);
else //Otherwise, use first initial concatenated to last name
employee.LoginName = ConcatenateFirstInitialAndLast(employee);
}
///
/// Returns true if the employee last name is very short
///
/// Employee to check for a short last name
/// True if the last name is very short, false if not
private static bool IsEmployeeLastNameVeryShort(Employee employee)
{
//Returns whether or not the last name's length is shorter than the minimum length of the last name
return employee.LastName.Length < LastNameMinimumLength;
}
///
/// Concatenate an employee's first and last name
///
/// Employee whose first and last name to calculate
/// The concatenated first and last name
private static string ConcatenateFirstAndLastNames(Employee employee)
{
//Concatenate the first and last names of the employee
return employee.FirstName + employee.LastName;
}
///
/// Concatenates the first initial and last name of the employee
///
/// Employee whose first initial and last name to concatenate
/// The concatenated first initial and last name
private static string ConcatenateFirstInitialAndLast(Employee employee)
{
//Concatenate the first initial and last name of the employee
return employee.FirstName.ElementAt(0) + employee.LastName;
}
}
I think you'd be hard pressed to find someone who thought that this level of commenting on this self-describing code made any sense. That's the beauty of this demonstration -- more even than just the self documenting code, adding comments next to it demonstrate that there's really nothing left to say. And hey, if this gambit backfires and you find someone who thinks this is a good idea, you've learned a valuable piece of information about someone to bear in mind when considering which projects to work on and where to work in the future. In all seriousness, though, I think this will do a fairly good job of demonstrating that you can express the vast majority of what you would express in comments with the code itself.
There is another very practical benefit to proving this and the resulting Spartan approach to comments beyond the fact that the comments and code tend not to get out of sync; when comments are a rarity, you're more likely to read them. Think of heavily commented code as "The Code that Cried Wolf." If you see a lot of code that looks like the code in this snippet immediately above, don't you start to just kind of tune out the comments? They appear in some other color than the main code, and rather than information, they become comforting white noise, separating your methods and fields from one another the way a little bit of margin/padding separates a bunch of text boxes on the form from one another. You don't study the buffering space -- you just take it for granted. This is how it works with over-commented code bases. If you find yourself in code bases like this, do an exercise and ask yourself when the last time you paid attention to a comment was. Yesterday? Last week? Last month? Now ask yourself how many times you've looked at the code since then? A hundred? A thousand? Ten thousand? That's a pretty low percentage of time that you pay attention to comments.
But imagine a code base with almost no comments. You're flipping between classes and looking at methods, and there's no extras -- just code. Now, you open some class and there's a big, fat, 10 line comment. Betcha notice it immediately and, what's more, betcha read it. And, I'm also willing to wager it's worth reading. If a code base has an absence of noise comments, the developer that wrote this one probably has something important to say: why he chose to do it this way even though it seems obtuse or where she found a template/inspiration for this code via google and why she chose to use it.
Method, parameter, class, and variable names can tell you a lot about what the code elements are. Source control checkin comments can tell you a lot about why various changes were made. Unit tests/BDD Tests/Integration tests can tell you a lot about business rules and requirements. None of these things require adding redundancy to the code itself. Comments in your code should be the exception rather than the rule. And you can prove it.
There are few things in life more frustrating than pouring a good amount of effort into something only to have it shot down without any kind of satisfying explanation of the rationale. For children this tends to be unfortunate for self interested reasons: “I want to do X in the immediate future and I can’t.” But as you get older and are motivated by more complex and nuanced concerns, these rejections get more befuddling and harder to understand. A child making a case for why he should own a Red Rider BB Gun will understand on some level the parental objection “it’s dangerous” and that the parental objection arises out of concern for his welfare. So when the “enough — I’m your parent and I say so” comes up it has the backing of some kind of implied reasoning and concern. An adult making a case that adopting accounting software instead of doing things by hand would be a money saving investment will have a harder time understanding an answer of “no, we aren’t going to do that because I’m your boss and I say so.”
To drive the point home, consider a conversation with a friend or family member instead of a mere acquaintance. Consider that in this scenario the “you’re wrong” would probably be good-natured and said in jest. “Dude, you’re totally wrong–everyone knows red is the best color!” That would roll off your back, I imagine. The first time, anyway. And probably the second time. And the third through 20th times. But, sooner or later, I’m pretty sure that would start to wear on you. You’d think to yourself, “Is there any matter of opinion about which I’m not ‘wrong,’ as he puts it?”
To see what I mean, think of being assigned a hypothetical task to read an XML file full of names (right), remove any entries missing information, alphabetize the list by last name, and print out “First Last” with “President” pre-pended onto the string. So, for the picture here, the first line of the output should be “President Grover Cleveland”. You’ve got your assignment, now, quick, go – start picturing the code in your head!
What did you picture? What did you say to yourself? Was it something like “Well, I’d read the file in using the XDoc API and I’d probably use an IList<> implementer instead of IEnumerable<> to store these things since that makes sorting easier, and I’d probably do a foreach loop for the person in people in the document and while I was doing that write to the list I’ve created, and then it’d probably be better to check for the name attributes in advance than taking exceptions because that’d be more efficient and speaking of efficiency, it’d probably be best to append the president as I was reading them in rather than iterating through the whole loop again afterward, but then again we have to iterate through again to write them to the console since we don’t know where in the list it will fall in the first pass, but that’s fine since it’s still linear time in the file size, and…”
