Sunday, February 28, 2010

Descriptive Code

I'm not a big fan of the term "self-documenting code" mainly because it seems to light the fires under both extremes of whether it's nobler to comment everything, or wiser to insist all code expresses what it does perfectly well. As with everything, moderation is the key.

I've participated in project teams that believe that every method in every class should be commented, and in others where I was told in a code review to remove all comments. At the time I was a bit confused why I would have been told to remove comments, but over time it did begin to make sense.

The objective of self-documenting code is that the code should describe it's intent without the need for additional external documentation.

My original attitude about commenting code was that while anyone reading code should be able to understand "what" the code does, there should be comments to reflect "why" the code does what it does. However, the trouble with commenting is that as you re-factor code, the intent of the code can change from what was originally commented. "Comments Lie."

On the other end of the spectrum, commenting everything is a complete waste of time and just leads to copy & paste errors, and more lies. Why do you need a comment like this on something like a Customer Presenter?

/// <summary>
/// Saves the provided Customer.
/// </summary>
/// <param name="customer">Customer to save.</param>
public void Save(Customer customer)
{
//...
}

Ok, if your intent is to be able to produce documentation about an API, by all means comment public interface methods and comment them well; but for general code? This is just a waste of time. Most of the time when I see this in practice there are plenty of examples where the comment headers aren't accurate anyway; referencing parameters that were removed, or missing parameters that were added.

I still stand beside my original belief. Code by itself is misleading, it only tells half of the story. I can see what it does, but not why it does it. The best advice I can give regarding self-documenting code:

1. Code should be organized into distinct units of work. It should do one thing, and one thing only, even if that is just to coordinate other distinct actions. The name of that method should perfectly describe what that code is supposed to do.
For example: private void updateOrderStatusToDispatched(Order order)
This method name perfectly describes what you intend its code to perform. When this method is called it is crystal clear what you are intending to do.

2. Reduce, or better eliminate the use of conditional logic in your code. Nesting ifs/else ifs or a switch statement is a good indication that you can break code down into smaller units of meaningful work. 90% of the "if" statements I use in code are for fail-fast assert behaviour. If the method should do nothing based on the parameters it's given, return now. If a parameter isn't valid, throw an exception.

Where you do have conditional bits for a reason, be sure to use a comment to describe "why" that condition exists. Too many times I've seen code break when some conditional term was added for one particular case which was not documented, it caused a bug in another case, so the bug fix of removing that condition fixed the one case, but broke the original. You might remember why you added an "if" condition around a particular block after 1 month, or maybe 3 months, how about 6 months? two years? And what about the other poor souls that are working on the code base with you?

3. Write unit tests around the behaviour of the functionality, not specific code implementation. Something adopted from BDD (Behaviour-Driven-Design/Development) is that a unit test can represent the original functional requirement for the application. In this way, the code being tested is documented with the "why" it is implemented the way that it is.
For example:
[Test]
public void EnsureThatWhenACustomerConfirmsAnOrderTheOrderIsDispatched()

This test method would not test a method like updateOrderStatusToDispatched() directly. The update method is a private member, but it does test the behavior of when a customer confirms an order and asserts that the expected behaviour of that action is that the order should be dispatched.

Methods such as updateOrderStatusToDispatched() serve self-documentation by describing precisely what their intended behaviour. By incorporating unit tests that represent the various requirements of our application, our code is now documenting itself in ways that are virtually impossible with separate requirements and design documents. If I change the behaviour of the code unexpectedly, my unit tests break telling me I changed behaviour. If I need to change or add behaviour, the changes to my unit tests assert those changes as I re-design the code implementation. Keeping a separate "Word" document up to date for a software design is frustrating by comparison.

No comments:

Post a Comment