Monthly Archives: December 2012

The Only Good Comment Is a Useful Comment

I’ve talked about comments in The Zen of Comments, but the rising tide of ASDocs (and JDoc, etc.) brings this home: requiring comments is as pointless as requiring lines-of-code or a minimum number of check-ins. The existence of comments adds nothing to source code; they make it harder to see the code itself and are often out of date.

/**
* this is the current record id
*/
public var currentRecordId : String ="";

/**
* handle search button click
*
* @param event the mouse event from the click
*/
private function searchButtonClickHandler(event : MouseEvent) : void
{
   ...
}

/**
* @return Foo
*/
public function get foo() : Foo
{
    return _foo;
}
/** TODO: fix this
*/
public function clearGlobalList() : void
{
    this.globalList.removeAll();
}
/**
* @param control int
* @param clearAll delete extra items
*
* @return the number of deleted items
*/
public function truncateData(maximumCount : Number) : void
{
    ...
}

A bad comment is worse than no comment. The goal is to add information to the source code, nor obsolete noise, and the best way is to name the variables, objects, and methods so they are self-descriptive. As you can see from the examples above, adding ASDoc headers makes the code less readable, and the names of the variables and functions already completely describe their behavior.

Good Nomenclature is the Best Comment

or

“The proper use of comments is to compensate for our failure to express ourselves in code.” — Robert C. Martin

“If the code and the comments disagree, then both are probably wrong” —attributed to Norm Schryer

“Good code is its own best documentation. As you’re about to add a comment, ask yourself, ‘How can I improve the code so that this comment isn’t needed? ‘ Improve the code and then document it to make it even clearer” –Steve McConnell Code Complete

I’m not advocating Hungarian notation. The need to use short variable names has long past, and abbreviating types is more than silly. Perhaps it is a dogged remnant of my sordid past in Pascal: I like complete, explicit names.

  • Class names should be specific nouns and avoid words that have meanings in the context of programming (we are running out of terms for “object”!). For example,
  • Method names should start with a verb (“open”, “delete”, “calculate”) and contain the names of the classes they are going to act on, e.g. importDetail(aDetail : Detail) : void
  • Property and field names should not include their class name (no Client.clientName) unless necessary to differentiate it within the context of the class (Client.clientName and Client.accountName).

Sample Naming Patterns

  • function findFoo(criteria) : Foo returns a Foo or null/-1/empty string if not found. I use an “a” prefix on my parameters to differentiate them from local variables and class fields/properties
  • function getFoo(criteria) : Foo returns a Foo or throws an exception if not found
  • function createFoo(initial data) : Foo returns a new Foo and adds it to the list/collection/model. Throws an exception if the Foo cannot be created (e.g. the key data already exists)
  • function openFoo(initial data) : Foo returns an existing Foo that matches the data or returns a new Foo and adds it to the list/collection/model
  • function removeFoo(aFoo) : Boolean returns true if aFoo was remove and false if aFoo was not present to remove. throws an exception is aFoo was there but the method could, for some reason, not remove it
  • function calculateTotal(fooArray : Array) : Number returns a value based on the parameter. The name indicates that the result is a dynamic value that is not stored persistently and is not based on anything else
  • function composeErrorMessage(error : Error, data : Foo, user : User) : String returns text value based on the parameters. The name indicates that the result is a dynamic value that is not stored persistently and is not based on anything else
  • class DeletionConfirmationView (not DeleteView or event DeleteConfirmationView). “Delete” is a verb and indicates a function (which is going to delete something). The view is going to present a confirmation dialog for the deletion (the noun form of “delete”).
  • function foo(param : type, pleaseDoSomething : Boolean) : void The second parameter is asking the method to do something like delete child objects, suppress notifications. The please makes it clear that it is a parameter, not a field or property of the object. Calling it doSomething or even isDoSomething does not make it clear that it is a parameter

Some bad examples from the Flex framework itself:

  • property includeInLayout, visible, includeInLayout – it should be “isIncludedInLayout”.
  • properties visible and enabled are at least adjectives, not verbs.
Other bad examples
  • property countFavorites – it should be favoritesCount. The function that calculates the value might be called “countFavorites()”

An excellent book referencing the code aesthetics of various luminaries is Clean Code, Robert C. Martin, Prentice Hall 2009, ISBN 0-13-235088-2