Are all comments bad?

I’ve been thinking about comments in code. Some people advocate not using comments suggesting that your code should be self documenting. I believe this to be a good practice where possible. If you need comments to explain what your code is doing, then it’s quite likely you’re code isn’t sufficiently broken down into lots of little methods naming what each part does. What happens when you have code that is simply non-intuitive and breaking it out into named methods doesn’t work?

Let’s look at an example to emphasize my point. We’ll take the naive Fibonacci function:

#!/usr/bin/env python

import unittest


def fib(n):
    if (n <= 2):
        return get_base_case(n)
    return fib(n - 1) + fib(n - 2)

def get_base_case(n):
    return n - 1


class FibonacciTests(unittest.TestCase):
    def test_first_fib_number_is_zero(self):
        self.assertEqual(0, fib(1))

    def test_second_fib_number_is_one(self):
        self.assertEqual(1, fib(2))

    def test_third_fib_number_is_one(self):
        self.assertEqual(1, fib(3))

    def test_fourth_fib_number_is_two(self):
        self.assertEqual(2, fib(4))

    def test_sixth_fib_number_is_five(self):
        self.assertEqual(5, fib(6))

if __name__ == '__main__':
    unittest.main()

Now for a tail recursive implementation that is more efficient (but not in Python, as it doesn’t have tail call optimization! I should have done it in C, but I couldn’t face writing the tests using check).

#!/usr/bin/env python

import unittest


def fib(n):
    return fib_accumulator(n, 1, 0)

def fib_accumulator(counter, nth_fib, nth_minus_one_fib):
    if (counter == 1):
        return nth_minus_one_fib
    if (counter == 2):
        return nth_fib

    next_fib = nth_fib + nth_minus_one_fib
    return fib_accumulator(counter - 1, next_fib, nth_fib)


class FibonacciTests(unittest.TestCase):
    def test_first_fib_number_is_zero(self):
        self.assertEqual(0, fib(1))

    def test_second_fib_number_is_one(self):
        self.assertEqual(1, fib(2))

    def test_third_fib_number_is_one(self):
        self.assertEqual(1, fib(3))

    def test_fourth_fib_number_is_two(self):
        self.assertEqual(2, fib(4))

    def test_sixth_fib_number_is_five(self):
        self.assertEqual(5, fib(6))

if __name__ == '__main__':
    unittest.main()

I’ve tried my best to make this as self-explanatory as possible. I still feel that additional explanation would help for whoever has to maintain it. The first time I saw this definition nth_fib was a and nth_minus_one_fib was b, I had absolutely no idea how it worked. Naming things really makes such a difference!

The additional information I’d like to add is:

• We only return nth_minus_one if counter == 1, which implies we’re calling fib(1) so we return the first fibonacci number: 0.
• When n > 1 then counter == 2 terminates the algorithm returning the accumulated value

• A few examples:

    fib(3) -> fib_accumulator(3, 1, 0) -> fib_accumulator(2, (1 + 0), 1) -> return (1 + 0)
  
    fib(5) -> fib_accumulator(5, 1, 0) -> fib_accumulator(4, (1 + 0), 1) -> 
              fib_accumulator(3, (1 + 0 + 1), (1 + 0)) -> fib_accumulator(2, (1 +
              0 + 1 + 1 + 0), (0 + 1 + 1)) -> return (1 + 0 + 1 + 1 + 0) -> return 3
  

I think that information makes it clearer how the function works. I tried to extract the base cases to add more explanation via code, but couldn’t think of descriptive enough methods, maybe that’s because I don’t grok the algorithm. I can see how it works, but find it very hard to articulate how to explain it’s behaviour, when this happens I find it’s best to resort to examples to guide explanation, but this is difficult to do in code! Unit tests you scream!? True, maybe I should add comments there explaining what is happening? How do you handle cases where you feel your code isn’t sufficiently descriptive, but can’t make it more descriptive through refactoring?