Back to Basics – good comments are targeted comments

I can’t think of a single person who enjoys writing comments in code. I don’t, my friends and colleagues don’t, and I’m pretty sure there isn’t a meetup group for fans of it. Outside of code that I write for blog posts, I can pretty much guarantee there are only place where I write comments is in interfaces.

The simple reason for this is that a) interfaces should be explicit, and b) implementations should be self-documenting.

The reason for this blog post is that I’m seeing a lot of code at the moment that falls into one of two traps – either everything is documented, or nothing is documented.

Everything is documented

Spot what’s wrong with this code:

/**
 * The foo class represents blah blah blah...and so on, describing the 
 * class in such detail it's a pity the code couldn't be generated from it
 */
public class Foo {

    /** The name */
    private String name;

   /**
    * Do something with this instance.
    */
   public void doSomething() {
      // Get the name
      String localName = this.getName();

      // munge the local name
      localName.munge();
   }

   /**
    * Get the name.
    * @return the name
    */
   public String getName() {
      // return the name 
      return this.name;
   } 

   /**
    * Set the name.
    * @param name the name
    */
   public void setName(String name) {
      // set the name 
      this.name = name;
   } 
}

Or, to put it another way, spot what’s right with it. That’s a much shorter answer. The code is full of unnecessary comments – e.g. getName() gets the name – and code that seems to have been written just so it could be commented – e.g; String localName – this.getName(); The names have been changed to protect the guilty, but this is real code I’ve seen in a live codebase.

As as I’m concerned, implementations don’t need code-level comments because that’s what the code does anyway.

Nothing is documented

At the other end of the scale is this little gem:

public interface Parser {
    void parse(InputStream is) throws IOException, SQLFeatureNotSupportedException 
}

Interfaces, on the other hand, should have clear documentation that defines what goes in, a generic description of what should happen, and a clear description of what comes out and what exceptions can be thrown. Information at this level should state if, for example, null arguments are allowed, if a null value can be returned, the circumstances in which certain exceptions can be thrown, and so on.

Interfaces should be explicit

Interfaces, to my way of thinking, are contracts, and contracts – as any blood-sucking lawyer can tell you – exist to be honoured. They can’t be honoured if the terms are not explicitly set out.

There are no Burger Kings in Belgium, so if I’m in the UK or the Netherlands I am generally tempted to have one. On my most recent visit, I noticed this at the bottom of the receipt:

“Free drink with any adult burger with this receipt Excluding Hamburger, Cheeseburger or King deal or any promotional offers”

Or, to put it another way…

/**
 * Get the free drink promised by the receipt.  This is valid for any burger.
 * @param burger the burger
 * @param receipt the receipt
 * @returns the free drink
 * @throws InvalidBurgerException if the burger doesn't qualify for a free drink
 */
public Drink getFreeDrink(Burger burger,
                          Receipt receipt) throws InvalidBurgerException {
    if (MealType.HAMBURGER == meal.type()
        || MealType.CHEESEBURGER == meal.type()
        || MealType.KING_DEAL == meal.type()
        || meal.isPromo()) {
        throw new InvalidBurgerException();
    }
    return new Drink();
}

To my simple brain, this is confusing and contradictory as hell. Your API should be clear and (as my university teachers beat into me) unambiguous – for example, the words “any” and “except” should not appear in the same sentence. Strive for clarity – if you find your API is too hard to clearly document, there’s a good chance it will be annoying to use. In the case above, an improvement would be something along the lines of

/**
 * Get the free drink promised by the receipt.  Not every burger qualifies for a free drink.
 * @param burger the requested burger.  This may or may not qualify for a free drink
 * @param receipt the receipt containing the offer
 * @returns the free drink. May be null, depending on the burger
 */
public Drink getFreeDrink(Burger burger,
                          Receipt receipt) {
    // implementation
}

Note that I’ve also got rid of the exception as a result of being more explicit.

Conclusion

Annoying as it is, documentation is extremely important in the correct place and completely useless everywhere else. Done correctly, they will make your API easier to use and easier to maintain, which is generally a good thing.

Here endeth the lesson.