Mandatory documentation is a pest

5/2/2014

Rant:

I recently needed to check the source of a class I was working with. The codebase is checked with Findbugs and Checkstyle on every build. Not merely as a metric, mind you, but failing the build when there are any warnings at all.

I needed to find out what kind of values came out of a user status field. This is what I found:

public class User {

...

/**
 * Returns the user status.
 *
 * @return the user status
 */
public int getStatus() {
    return status;
}

/**
 * Sets the user status.
 *
 * @param status the user status to set
 */
public void setStatus(final int status) {
    this.status = status;
}

/**
 * Sets the status.
 *
 * @param status the status to set
 * @return this
 */
// CHECKSTYLE.OFF: HiddenField
public User withStatus(final int status) {
    this.status = status;
    return this;
}
// CHECKSTYLE.ON: HiddenField

...
}

Now, there are a number of problems with this code:

The original developer probably knew, but needed to conform to the coding standard, which requires a load of crap. As he was auto-generating the comments, he forgot to add anything that might actually explain or clarify, such as adding a comment or, better yet, replacing the dreaded integer with an enumeration.

This class-level documentation was obviously copied from somewhere else, where it had been auto-generated:

/**
 * Description of RenderImageTeaser
 */
class RenderLivestream { ... }

The naming of the following class is unfortunate, but the class documentation makes it so much better:

/**
 * Impl of {@link MyService.} //O RLY?
 */
public class MyServiceImpl { ... }

If you don't trust your developers to express themselves clearly in code, why do you think they could do so in comments?

Thesis 1: Comments are a code smell.

If you find the need to explain what you are doing in comments, try improving the code instead. Have a complex boolean expression? Break it up into clearly named booleans or functions. Have a complex switch statement? Try some polymorphism and let the type system do the work for you. You'll get code that's both readable and mostly void of comments, because there is nothing that needs explaining. If there are comments in the code, they should explain always why, never what.

Thesis 2: Mandatory documentation is a pest.

The getStatus() method from earlier? All the information in the JavaDoc comment is clearly visible from the method signature. Of course, if you write a public API and feel the need to add clarifying documentation to a class or method, by any means, go ahead! I'm a particular fan of proper class-level documentation, especially with working examples. But requiring the developer to JavaDoc everything leads to a lot of repetition and auto-generated comments to appease the build process. This stuff is at best redundant, but more commonly outdated or plain wrong. (By the way, the shortest possible comment that appeases Checkstyle's default pattern is /** . */.)

Maybe, if the developer of the getStatus() method did not have to provide mandatory complete JavaDoc with every method, he would have found the time to explain what the integer returned from the method actually means. (But then again, maybe he would not have, or he did not in fact know.)

A Checkstyle config that is strict, but not retarded

Here's a checkstyle config adapted from the default, with some of the more retarded bits commented out, that could help you get started on a sensible code convention yourself.

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE module PUBLIC "-//Puppy Crawl//DTD Check Configuration 1.3//EN" "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
  <property name="severity" value="warning"/>
  <module name="TreeWalker">
    <module name="FileContentsHolder"/>

    <!-- Requiring comments on all public members is a clear fail. What you get are auto-generated 
         comments that are never maintained.

         Good comments - as per the Clean Code handbook - inform, explain intent, clarify, warn of 
         consequences.

         99% of our Javadoc comments are the exact opposite: They repeat the method signature and 
         add nothing else. They are trash and should be removed over time.

         Also, if you don't trust other coders to express themselves clearly in code, why do you think 
         they will do so in comments?

         -snip-
          /**
           * Returns the user status.
           *
           * @return the user status
           */
          public int getStatus() {
              return status;
          }
          -snap-
          -->
    <!--<module name="JavadocMethod"/>
    <module name="JavadocType">/>
    <module name="JavadocVariable"/>-->
    <module name="JavadocStyle"/>

    <module name="ConstantName">
      <!-- This workaround allows "static final logger" as an exception. Checkstyle's check is 
           dead wrong, the convention is that only identifiers that are never followed by a dot 
           or array accessor are to be in caps ("MY_VALUE", but "logger.info()").

          The Java Coding Style Guide, § 3.3:

          Names of fields being used as constants should be all upper-case, with underscores 
          separating words. The following are considered to be constants:

          All static final primitive types (Remember that all interface fields are inherently 
          static final).
          All static final object reference types that are never followed by "." (dot).
          All static final arrays that are never followed by "[" (dot).
          Examples:

          MIN_VALUE, MAX_BUFFER_SIZE, OPTIONS_FILE_NAME
           -->
      <property name="format" value="^([A-Z][A-Z0-9]*(_[A-Z0-9]+)*|logger)$"/>
    </module>
    <module name="LocalFinalVariableName"/>
    <module name="LocalVariableName"/>
    <module name="MemberName"/>
    <module name="MethodName"/>
    <module name="PackageName"/>
    <module name="ParameterName"/>
    <module name="StaticVariableName"/>
    <module name="TypeName"/>

    <!-- I'd tend to disallow the use of long import lists rather than the .* import. -->
    <!--<module name="AvoidStarImport"/>-->

    <module name="IllegalImport"/>
    <module name="RedundantImport"/>
    <module name="UnusedImports"/>

    <module name="MethodLength">
      <property name="max" value="60"/>
    </module>
    <module name="ParameterNumber"/>
    <module name="LineLength">
      <property name="max" value="120"/>
    </module>
    <module name="EmptyForIteratorPad"/>
    <module name="MethodParamPad"/>
    <module name="NoWhitespaceAfter"/>
    <module name="NoWhitespaceBefore"/>
    <module name="OperatorWrap">
      <property name="tokens" value="BAND,BOR,BSR,BXOR,COLON,DIV,EQUAL,GE,GT,LAND,LE,LITERAL_INSTANCEOF,LOR,LT,MINUS,MOD,NOT_EQUAL,QUESTION,SL,SR,STAR"/>
    </module>
    <module name="ParenPad"/>
    <module name="TypecastParenPad"/>

    <module name="WhitespaceAfter"/>

    <!-- Also barks at things that are not methods at all, e.g. annotation values that are arrays. -->
    <!--<module name="WhitespaceAround"/>-->

    <module name="ModifierOrder"/>
    <module name="RedundantModifier"/>
    <module name="AvoidNestedBlocks"/>
    <module name="EmptyBlock"/>
    <module name="LeftCurly"/>
    <module name="NeedBraces"/>
    <module name="RightCurly"/>
    <!-- By all means avoid them if you like, but if they help readability, go ahead. Flat-out 
         disallowing themis wrong for my taste. -->
    <!--<module name="AvoidInlineConditionals"/>-->
    <module name="EmptyStatement"/>
    <module name="EqualsHashCode"/>
    <module name="HiddenField">
      <property name="ignoreConstructorParameter" value="true"/>
      <property name="ignoreSetter" value="true"/>
      <property name="ignoreAbstractMethods" value="true"/>
      <property name="tokens" value="VARIABLE_DEF"/>
    </module>
    <module name="IllegalInstantiation"/>
    <module name="InnerAssignment"/>
    <module name="MagicNumber">
      <!-- Unless ignoreHashCodeMethod is enabled, this check disallows most standard 
           hashCode() implementations. -->
      <property name="ignoreHashCodeMethod" value="true"/>
      <!-- Unless disabled, this check disallows standard use of e.g. the length property 
           in JPA annotations.
           Sometimes, the meaning of the number 31 is 31: @Column(length = 31). -->
      <property name="ignoreAnnotation" value="true"/>
      <property name="ignoreNumbers" value="-1, 0, 1, 2, 1000"/>
    </module>
    <module name="MissingSwitchDefault"/>
    <module name="RedundantThrows">
      <property name="suppressLoadErrors" value="true"/>
    </module>
    <module name="SimplifyBooleanExpression"/>
    <module name="SimplifyBooleanReturn"/>
    <module name="FinalClass"/>
    <module name="HideUtilityClassConstructor"/>
    <module name="InterfaceIsType"/>
    <module name="VisibilityModifier">
      <property name="protectedAllowed" value="true" />
    </module>
    <module name="ArrayTypeStyle"/>
    <module name="FinalParameters"/>

    <!-- Erm, no. -->
    <!--<module name="TodoComment"/>-->

    <module name="UpperEll"/> <!-- Long literals with upper case L, i.e. 43L. -->
    <module name="CyclomaticComplexity"/>
    <module name="BooleanExpressionComplexity"/>
    <module name="ClassDataAbstractionCoupling"/>
    <module name="ClassFanOutComplexity"/>
    <module name="NPathComplexity"/>
    <module name="AnnotationUseStyle">
        <property name="elementStyle" value="compact"/>
    </module>
    <module name="MissingDeprecated"/>
    <module name="DefaultComesLast"/>
    <module name="DeclarationOrder"/>

    <!-- Under rare circumstances, this enforces Yoda conditions with nullable strings,
         e.g. "mystring".equals(possiblyNullString). -->
    <module name="EqualsAvoidNull"/>

    <!-- This should remain disabled. Explicit initialization to default values can and is 
         used to explain intent, this check disallows good code.
         E.g. class Query { private Integer limit = null; ... } makes it clear that the member 
         is supposed to be null initially, and that the developer hasn't just forgotten to 
         initialize it properly. -->
    <!--<module name="ExplicitInitialization"/>-->

    <module name="FallThrough"/>
    <module name="IllegalCatch"/>
    <module name="IllegalThrows"/>
    <module name="IllegalToken">
      <property name="severity" value="ignore"/>
      <property name="tokens" value="POST_INC,POST_DEC,LITERAL_SWITCH"/>
      <metadata name="net.sf.eclipsecs.core.lastEnabledSeverity" value="inherit"/>
    </module>
    <module name="IllegalType">
      <property name="tokens" value="METHOD_DEF,PARAMETER_DEF,VARIABLE_DEF"/>
    </module>
    <module name="JUnitTestCase"/>
    <module name="MultipleVariableDeclarations"/>
    <module name="NoFinalizer"/>
    <module name="SuperClone"/>

    <!-- Only catches very basic mistakes, does not know operator precedence, and is 
         therefore not as bad an idea as it first sounds. -->
    <module name="UnnecessaryParentheses"/>

    <module name="OneStatementPerLine"/>
  </module>
  <module name="NewlineAtEndOfFile">
    <property name="lineSeparator" value="lf"/>
  </module>
  <module name="Translation"/>
  <module name="FileLength">
    <property name="max" value="800"/>
  </module>
  <module name="FileTabCharacter"/>
  <module name="StrictDuplicateCode">
    <property name="min" value="25"/>
    <property name="fileExtensions" value="java"/>
  </module>
  <module name="SuppressionCommentFilter">
    <property name="offCommentFormat" value="CHECKSTYLE.OFF\: ([\w\|]+)"/>
    <property name="onCommentFormat" value="CHECKSTYLE.ON\: ([\w\|]+)"/>
    <property name="checkFormat" value="$1"/>
  </module>
</module>

Comments