The War on Comments Isn't Over

Back in January, my very talented colleague Dave Copeland and me spent a bit of time talking about the proper role of comments in Ruby. He wrote a nice piece defending the use of code level comments back then. We disagree. You should go read his piece before getting to my argument. And after you read that you should go buy his new book.

I’ll wait…

Welcome back.

Dave and I wholeheartedly agree on API Documentation. If you are writing a reusable library—even if only you and your coworkers will ever use it—I find it irresponsible to neglect API level documentation.

We semi-agree on what he calls “Why” comments: comments that explain something about the reasons why a piece of code is the way it is. Sometimes there truly is some odd, external constraint that needs to be explained… and it needs to be explained right there in the code. However, I think that explanatory comments are overused and most times indicate a failure of communications that could better be solved by clear tests, improved naming, and good commit messages. I hope to write about this more later.

What I’m writing about today—and where we disagree—is on the used of descriptive comments: comments that explain what a piece of code is doing.

Problem: Your code needs to be commented to be understood. Solution: Add comments - Now you have 2 problems :)

Dave’s argument for explanatory comments is that even the best of us occasionally write code that is convoluted and difficult to understand. I know that, despite my best efforts, I’m responsible for much of the less than clear code that prompted the discussion on comments in the first place.

Here is the example Dave used of convoluted code you could come across in a production app that would benefit from comments:

def isqrt(square)
  square = square.to_i
  return 0 if square == 0
  raise RangeError if square < 0

  n = iter(1, square)
  n1 = iter(n, square)
  n1, n = iter(n1, square), n1 until n1 >= n - 1
  n1 = n1 - 1 until n1*n1 <= square
  return n1
end
def iter(n, square)
  (n + (square / n)) / 2
end

No argument from me. That code is impenetrable. As I write this I still don’t completely understand what it does. Dave suggests adding comments so that the next poor bastard who comes along has an easier time of figuring it out. Here’s his commented version:

def isqrt(square)
  # just in case we don't get an int
  square = square.to_i
  return 0 if square == 0
  raise RangeError if square < 0

  # make our initial guesses, which are initially
  # too high (intentionally; see next)
  n = iter(1, square)
  n1 = iter(n, square)

  # Refine our guesses until we get close enough
  n1, n = iter(n1, square), n1 until n1 >= n - 1
  # We're close enough when we're JUST under square
  n1 = n1 - 1 until n1*n1 <= square
  return n1
end
def iter(n, square)
  (n + (square / n)) / 2
end

And that is an improvement. However, even with the comments I still don’t fully understand what this method is doing. That’s the problem. Dave went to the trouble of figuring out this really tricky piece of code. He even tried to help me— the aforementioned poor bastard reading this code at a later date—and I still don’t get it.

The problem is that despite the helpful comments (and they are helpful) the method is still pretty much incomprehensible.

The hard part of improving a difficult piece of code is understanding it, not making the changes to improve it. After you’ve managed to understand the code you can improve it in two ways: add comments that describe what it is doing or refactor the code to make it easier to understand. Dave did all the hard work of figuring out the isqrt method. But it is still a difficult method to follow.

Let’s see if we can make the code speak for itself.

My first change was the name of the method from isqrt to integer_square_root_of. There is no reason to drop vowels like we’re writing C in the 70’s. Storage is cheap. Editors autocomplete. Don’t be afraid of descriptive variable and method names. I’m not sure if the “i” in isqrt was supposed to mean integer because the method returns an integer approximation of the square root, or iterative because the method uses an iterative algorithm to determine the square root. I went with “integer” so that the method signature now describes exactly what it does: give you the integer square root of a square.

def integer_square_root_of(square)

My next change was to drop the comment just in case we don't get an int. This comment doesn’t really hurt, but it also doesn’t add anything that isn’t already immediately obvious. Furthermore, there should be a test that shows that the method behaves as expected when receiving a non integer parameter.

test "integer_square_root_of should convert non int squares to int" do
  assert_equal 3, integer_square_root_of(9.9)
  assert_equal 3, integer_square_root_of('9')
end

Not hurting isn’t a good enough reason to add a comment to your code. There must be a compelling reason to add a comment. Since this comment doesn’t meet that standard we’re going to go ahead and remove it.

Next up are the special cases: when the square is 0 we short circuit and return 0. When the square is negative we raise a RangeError because there is no such real number square root of a negative number and our method would go into an infinite loop if we didn’t raise an exception. In the original version they were written like this:

  square = square.to_i
  return 0 if square == 0
  raise RangeError if square < 0

The first thing I did was add a blank line to separate them from line 2 (line 1 above as I haven’t figured out how to specify line numbers in Octopress) where we ensure that square is an integer. Blank lines inside of a method definition are greatly underutilized as a tool for grouping related code and improving readability. Think of your application's source code as a book. Classes are chapters. Methods are sections. Each line of code is a sentence. And using blank lines lets us group our “sentences” (lines of code) into paragraphs.

Next I changed the checks for the special cases from two independent lines with postfix if statements to a combined if / elsif structure. Functionally, this is equivalent to the original version. However, by choosing to group them together in one logical block it further serves to emphasize that these two lines are related and draws further attention to the conditionals.

I also add a message to the RangeError that is raised if the square is a negative number. This is an example of what I call executable comments. The body of the message explains why we’re raising the exception—much as a comment would—but because it is part of the executable code it is far less likely to fall out of sync as the code changes.

This yields the following snippet:

  square = square.to_i

  if square == 0
    return 0
  elsif square < 0
    raise RangeError, "square must be non-negative. No negative number has a real square root"
  end

Continuing on, variable names like n1 and n2 don’t tell us anything, so I changed them to upper_guess and lower_guess. These names are far from perfect, but they at least suggest that as we continue to iterate these two variables will converge on the square root.

  upper_guess = next_guess(1, square)
  lower_guess = next_guess(upper_guess, square)

I also renamed the method iter to next_guess as that is what it does, give us our next guess.

def next_guess(n, square)
  (n + (square / n)) / 2
end

Now for the main work of the method. First, we repeatedly refine our guesses. Then, when we’re close enough, we decrement the guess until we get to our answer.

  # Refine our guesses until we get close enough
  n1, n = iter(n1, square), n1 until n1 >= n - 1
  # We're close enough when we're JUST under square
  n1 = n1 - 1 until n1*n1 <= square
  return n1
end

First, I again introduce whitespace to separate logically distinct steps.

The next thing I do is unpack the loop statements from one-liners to blocks. The ability to use postfix loop operators is a good tool for brevity in Ruby. However, a double variable assignment and a loop operator is simply too much for one line. By splitting the loop condition from the variable assignment I allow the reader to focus on each part independently.

Finally, I switch from to while from until for the looping logic. while says “check if this is true then execute the statement and loop” until says “check if this statement is false, and then execute the statement and loop.” This may be a matter of preference, but I find positive logic easier to understand then negative logic.

  while lower_guess < (upper_guess - 1) do
    lower_guess, upper_guess = next_guess(lower_guess, square), lower_guess
  end

  while lower_guess * lower_guess > square do
    lower_guess = lower_guess - 1
  end

  return lower_guess
end

Now, my version is complete:

def integer_square_root_of(square)
  square = square.to_i

  if square == 0
    return 0
  elsif square < 0
    raise RangeError, "square must be non-negative. No negative number has a real square root"
  end

  upper_guess = next_guess(1, square)
  lower_guess = next_guess(upper_guess, square)

  while lower_guess < (upper_guess - 1) do
    lower_guess, upper_guess = next_guess(lower_guess, square), lower_guess
  end

  while lower_guess * lower_guess > square do
    lower_guess = lower_guess - 1
  end

  return lower_guess
end

def next_guess(n, square)
  (n + (square / n)) / 2
end

This post took quite some time to write because explaining code takes a long time. Thankfully, writing code doesn’t. I think it only took me about 15 minutes to read, understand, and rewrite the isqrt method. My bet is that it would have taken about the same amount of time to read, understand, and comment it.

Is this clearer than Dave’s commented version? I obviously think it is, but it still a bit unclear. Perhaps a comment or two would help. My argument isn’t that all comments are unnecessary. It is that the vast majority of comments are unnecessary and that time spent writing comments can be better spent writing clearer code.

What do you think?