If you’re commenting, you’re doing it wrong.

…well, maybe not always. There are two legitimate roles for comments that I’ll get to at the end of this post, but continuing the theme of the DRY Principle from the last few posts, I wish to point out that comments are inherently WET (Write Everything Twice), not DRY (Don’t Repeat Yourself). The problem is that comments inevitably become out of sync with the code, at which point they are the opposite of helpful.

Consider this function to validate a United States phone number.

// Returns true if phoneNumber is of the form
// (999)999-999, 999-999-9999, or 999999999.
// Returns false otherwise.
function isValidPhoneNumber(phoneNumber) {
  if (typeof(phoneNumber) !== 'string') {
    throw new Error('Phone number must be a string.');
  }
  return phoneNumber === '' ||
         phoneNumber.match(/^\(\d{3}\)\d{3}-\d{4}$/) !== null ||
         phoneNumber.match(/^\d{3}-\d{3}-\d{4}$/) !== null || 
         phoneNumber.match(/^\d{10}$/) !== null;
}

Note how helpfully commented it is! So far, so straightforward.

Now we decide to allow an optional extension of the form ” x” followed by any number of digits. We could add a regular expression that means “optional extension” onto the end of each of the three existing regular expressions, but we decide it would be more fun as well as more DRY to refactor to a single regular expression that consists of the three previous ones OR’ed together, followed by a new one for the extension

// Returns true if phoneNumber is of the form
// (999)999-999, 999-999-9999, or 999999999.
// Returns false otherwise.
function isValidPhoneNumber(phoneNumber) {
  if (typeof(phoneNumber) !== 'string') {
    throw new Error('Phone number must be a string.');
  }
  if (phoneNumber === '')
    return true;
 
  var phoneRegex = /^((\(\d{3}\)\d{3}-\d{4})|(\d{3}-\d{3}-\d{4})|(\d{10}))( x\d+)?$/;
  return phoneNumber.match(phoneRegex) !== null;
}

Proud but exhausted, we move on to other things.

The problem, of course, is that the comments at the top of the function are now out of sync with the code. How many times has this happened to you?

If you’re practicing test-driven development, your unit tests should generally be all the comments you need. Although the unit tests do repeat the intention of the code, they do so in a useful manner. Furthermore, if they become out of sync with the code, you’ll know about it as soon as you run the tests. And which would you rather read: the regular expression or this output of my Jasmine unit tests?

isValidPhoneNumberTests

At the start of this post, I suggested that there may be two good roles for comments.

The first is to comment code that is so dense that nobody will be able to read it. If you do this, I suggest tying your comments to the code so tightly that any semi-conscious developer will keep the comments up-to-date. A regular expression is actually a great example. Here’s how I’d comment the one for the phone number.

  var phoneRegex = /^((\(\d{3}\)\d{3}-\d{4})|(\d{3}-\d{3}-\d{4})|(\d{10}))( x\d+)?$/;
  //                ^ ^         ^    ^^     ^ ^    ^^    ^^     ^ ^       ^       ^
  //     Start string (999)     999  -9999  | |    |     ||     | |       |       End of string
  //                                       OR 999  -999  -9999  | |       +---+
  //                                                           OR 9999999999  |
  //                                                                          Optional 1 occurrence of " x followed by digits"

A second way comments can be useful is to explain the overall strategy of the code — at a high enough level that it will not change.

Appendix: The Unit Tests

If you’re not familiar with Jasmine, here’s how easy it is:

describe('isValidPhoneNumber(phoneNumber)', function() {
  it('throws if phoneNumber is not a string', function() {
    [
      1234567890,
      { abc: 123 },
      NaN,
      null,
      true,
      undefined
    ].forEach(function(phoneNumber) {
      expect(function() { 
        isValidPhoneNumber(phoneNumber);
      }).toThrow();
    });
  });
  it ('returns true for an empty string', function() {
    expect(isValidPhoneNumber('')).toBe(true);
  });
  it('returns true for phoneNumber like (999)999-9999', function() {
    expect(isValidPhoneNumber('(123)456-7890')).toBe(true);
  });
  it('returns true for phoneNumber like 999-999-9999', function() {
    expect(isValidPhoneNumber('123-456-7890')).toBe(true);
  });  
  it('returns true for phoneNumber like 9999999999', function() {
    expect(isValidPhoneNumber('1234567890')).toBe(true);
  });
  it('returns false if phoneNumber has too many digits', function() {
    expect(isValidPhoneNumber('12345678901')).toBe(false);
  });
  it('allows an optional " x" after any valid phone number', function() {
    [
      '(123)456-7890 x9',
      '123-456-7890 x9999',
      '1234567890 x99999999'
    ].forEach(function(phoneNumber) {
      expect(isValidPhoneNumber(phoneNumber)).toBe(true);
    });
  });
});

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s