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.

  1. Commenting getters and setters is important. As it makes the api documentation look good :P

    Commenting on fields is very useful if it is not obvious what it is for. And trust me it may be obvious now but in a few years it will not be obvious at all.

    This is the what why and how of commenting.
    1) What: what is it supposed to do (api’s are a good place to document this.

    2) Why: why does this code exist? What reason is there for it to be around in the first place. Rarely do I see this.

    3) How: how does this do what it is supposed to do. Mostly self documenting for a reasonable coder. However comments should be made to explain how it is being done and why that was a good choice of implementation.

    Code itself only says what it does. You have no idea about the context.

  2. questionable 3rd party code that you integrate with needs a liberal sprinkling of “in code” comments to explain the pitfalls you’re side stepping.

  3. For third-party external systems (via web services or whatever), I think that’s a very good point. For questionable third-party local code, refuse to use it :)

  4. Nicolas Forney

    I do totally agree with your article. However, a lot of comments can be removed just by giving explicit functions and variables names. Robert Martin already spoke a lot about it in its book : “Clean Code”.

    A lot of programmers try to name variables and functions as short as possible with a lot of incomprehensible abbreviations. For what reason? Sincerely, I never understood. This only tends to make the code unreadable and fail to pursue THE objective each programmer should be following. Namely, being able to produce self documenting code.

Leave a Comment


NOTE - You can use these HTML tags and attributes:
<a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>