Wednesday, May 30, 2007

"How to Keep Your Code From Destroying You"...or not

Slashdot's carrying a link to an article called How to Keep Your Code From Destroying You by Jeff Vogel, the summary of which is:

  1. Comment code
  2. Use constants
  3. Use descriptive variable names
  4. Include error handling
  5. Don't optimise until you've found you need to
  6. Favour clearness over cleverness
Tips which are valuable to developers just starting out more than anything. Any developer who's been even slightly mentored will know these instinctively anyway.

But I'm afraid I really have to disagree with the remark about comments. Some circumstances will of course require the odd comment around it, but if a chunk of code requires a comment at such an in depth level, then the code needs to be refactored.

Take this example of Jeff's:
// This procedure moves the bullet upwards.
// It's called NUM_BULLET_MOVES_PER_SECOND
// times per second. It returns TRUE if the
// bullet is to be erased (because it hit a
// target or the top of the screen) and FALSE
// otherwise.
Boolean player_bullet::move_it()
{
Boolean is_destroyed = FALSE;

// Calculate the bullet's new position.
[Small chunk of code.]

// See if an enemy is in the new position.
// If so, call enemy destruction call and
// set is_destroyed to TRUE
[small chunk of code]

// See if bullet hits top of screen.
// If so, set is_destroyed to TRUE
[Small chunk of code.]

// Change bullet's position.
[Small chunk of code.]

Return is_destroyed;
}
Some points on this:
  • Those "small chunks of code" preceded by the comment are clear candidates for being moved into their own methods
  • A description of a method is fine, but why comment on where it is used? I doubt this comment will be updated when the method is used elsewhere
  • If a method requires a lengthy comment like this it's named incorrectly or is doing too much. Call the method MoveUp() if it's moving the bullet up the screen.
At Esendex we rarely find the need to comment at such a detailed degree. Comments just aren't updated when new functionality is added for one, and if code can be self-documenting, then so be it. It may only take a minute to add a comment, but if you've already written the code that says what it does, then it's a minute saved.

If you've got a method called "CreateAndPersistCustomer" that returns a Customer object, then it's pretty obvious that the method will instantiate a Customer object, persist it, and return the object it's just persisted. The only worthwhile comment I can see that could be added to the top of this method is possibly a note on where it is being persisted, but this itself becomes redundant if your system only persists to one place.

I take exception to comments such as "This method will create a customer and then insert it into the Customer table. It returns the customer that has been inserted". It's pointless if your naming conventions are any decent.

There's feedback in the Slashdot article that points to this code file as an example of when code really needs a comment. But I have to disagree here for the most part as well.

It's fine putting comments around bitwise operations such as shifts and the like, but when you've got a comment on a function that says "Updates that pad's states according to event inputs", then surely you're just masking unnecessary code complexity with comments. Why not call the function UpdatePadState?

It's sort of a tradition in C to have obscure type names, but don't compound that fact by needlessly adding your own. Code with a variable called dInt won't compile or run any faster than if you call the same thing directionIndicator.

Have meaningful names for everything. If you want to create a class (or unit, or whatever your programming language calls an encapsulation of something) that models a game pad, for god's sake call it GamePad. If that object needs a property to get the last event then call it LastEvent. Anything else is just being lazy.

Self-documenting code is clean and easy to maintain. My personal view is that if someone has to rely on reading comments, and can't read the code itself then they have no business updating it. If you don't know the difference between i++ and ++i, then get back to school and don't come back until you're qualified.

Comments will always have their place, but don't use them when refactoring your code will make more sense. Comments won't stop you duplicating code, refactoring will. Commenting a chunk of code doesn't make it usable from another method--moving that code into its own method will.

So the next time you're about to write a comment just think to yourself (or say aloud to your partner if you're pair programming) "Why do I need a comment here?".

Think of ways in which you can name your methods, variable names and classes so you negate the need for lengthy comments. Save your comments for the complicated bitwise operations, complex predicate logic and the like--places where you really do need to comment.

Don't comment the top of methods when you could just name the method differently and make more sense to everyone.

1 comment:

Kevin Pearman said...

Working with Ian at Esendex i whole heartedly agree that we don't need to comment our code too much, at least for the most part. The only real point i'd like to add is if you feel that it really is neccessary to comment a method decleration then use XML comments /// so that people can read the comment using intellisense. The comment will pop up whenever the method is being used in code which is the only reason i can think of to comment a method. If the user has opened the code to take a look at what the method does then, as previously said, the method code should be self explanatory but an XML comment can save you going to have a look at what the method is doing.