There are a number of schools of thought when it comes to comments. Some of them favor liberal application, and like an over-seasoned dish too many comments or the wrong comments can make is much more difficult to quickly process the meat of the codebase.

Let’s go over some of the common anti-patterns of comments and how to avoid them.

The Created by

/**
 * Created by randomdeveloper
 **/
public class SomeClass {}

This is a common pattern which is more often than not introduced by your IDE. It’s an anti-pattern because projects of any significant size will have multiple developers, who will probably touch every piece of code multiple times. Does it really matter who created the file at that point? If it does matter then let version control do the work for you.

The fix: Change your IDE settings to remove this template when creating new files. Rely on your version control to record who created the file, and when, based on your commit.

The Cruft

public String reverseString(String aString) {
  //if (aString == null || aString.length() == 0) {
  //  throw new IllegalArgumentException("Must be a valid string");
  //}

  char[] charArray = aString.toCharArray();
  StringBuilder builder = new StringBuilder();
  for (int i=charArray.length-1; i>-1; i--) {
    builder.append(charArray[i]);
    //System.out.println(builder.toString());
  }

  return builder.toString();
}

Comments such as the ones above are common in many legacy codebases for any number of reasons. Maybe there are a lot of junior developers on the project, maybe there was a rush to get the work done and no one could take the time to clean things up before they pushed the code. For whatever reason, these comments made it in to version control somehow. The result is a shift in the signal/noise ratio towards the noise end of the spectrum.

The fix: Simply remove the dead code and let the code speak for itself.

The Demarcation

  public void godMethod() {
    //Call the OS

    ... More code ...

    // Read the file

    ... More code ...

    // Save to the database

    ... More Code ...

    // Call the web service ...

  }

This is one of my favorite (most-hated) commenting anti-patterns. You’ll notice that in the code above there is a kind of stream of consciousness development occurring. The method is commenting a step-by-step recipe for how the work is getting done. It’s an indicator that there are multiple responsibilities and concerns being addressed in one place. This is a bad thing, because code that does multiple things at once is hard to change, hard to debug, and hard to test.

One of my favorite refactoring techniques is to pull apart huge methods like this and extract smaller methods (and eventually classes) at each of the demarcation lines. Eventually you’ll be able to inject in dependencies that handle the behavior that used to be part of this method, making it easy to change, debug, and test.

The fix: Extract methods and classes to act as collaborators from the code between each demarcation and name them according to what the comment previously stated. Remove the comments.

The Outdated Method Signature

  /**
   * @param The message to send.
   **/
  public void sendMessage(Context context, String message) {...}

Notice here that the param list in the Javadoc doesn’t match the param list in the method signature. You’ll see this type of comment a lot in Javadocs, especially in codebases which are under active development. Tearing the comments out may not be an option in this case if the comments are for a public-facing API. Instead you’ll need to be vigilant about updating the comment and naming the parameters to be as plainly obvious about their usage as possible.

The fix: Maintain the comment, name classes, methods, parameters appropriately.

The Internal Documentation

public class GodClass {

  /**
   * The context for doing things.
   **/   
  private Context context;

  /**
   * This method does some of the things.
   **/
  private void doSomeOfTheThings() {

  }
}

Commenting on the internals of a class is another indication that the code is not well understood. Private members and methods shouldn’t need documentation; instead the code should be organized and named as to make it blindingly obvious what it’s purpose is. Javadocs especially should be avoided on private implementation.

The fix: Remove internal comments on private members and methods. Refactor and rename until the code is more clear.

The Log Block

public class GodClass {

  public void doSomeOfTheThings(List<String> params) {
    //for (String param : params) {
    //  System.out.println("Param: " + param);
    //}

    ... more code ...
  }
}

Commented blocks of code such as the one above are usually artifacts of previous debugging sessions when someone was trying to figure out the state of the code at runtime. Rather than using an actual debugger they wrote variables out to the output, and then decided that that was a useful thing to do and left code in place, albeit commented out. It is still noise however, and shouldn’t exist in the production code.

The fix: First, insure there is an adequate level of tests covering the code which can serve as documentation and a regression suite all in one. Second, convert commented out blocks of code such as these into actual logging using a log framework such as log4j. You can control the log level of these statement, allowing fine-grained logging to take place.

The Defect Fix


public class GodClass {

  public void doSomeOftheThings() {
    //JIRA-1218 - This fixes the NPE
    if (context == null) {
      context = new Context();
    }
  }
}

This is one of those situations where it sounds like a good idea at first, until you realize how brittle and ephemeral this solution is. On the one hand you’re documenting where a fix has been applied. On the other, the code will continue to evolve until eventually (read: sooner than you think) this comment will be meaningless as the code changes out from under it. A cousin to this anti-pattern is to create a test which contains the issue reference in its name (public void fixForJIRA1218), which is just as bad for similar reasons.

The fix: Write a test that forces and records the expected behavior, rather than its implementation. Remove the comment. Insure that the commit message of the fix includes any issue tracking information (JIRA ticket #, etc).

As you can see the theme here is generally “remove the comment, add tests, fall back on your version control tools”. Another aspect of this theme is to slow down and take your time. If you feel the need to add a comment because you don’t understand what’s happening then take the time to understand it, and to make the code understandable. This will pay off well in the long run.