« Change of Blog Technologies - moving from Java to LAMP | Main | For want of a symbolic link... »
September 23, 2004
Checkstyle 3.4
I am readying the 1.0 release of ehcache, a popular cache for Java.
I received a patch which I was not happy with, which led me to wonder about tightening my checkstyle rules. Until recently I thought the use of tools such as checkstyle to be near universal on well-engineered projects. After all, it promotes Shared Code Ownership and Coding Standards, two Agile practices. Some excellent coders from the UK I know don't use it. They say "Oh, thats that thing you use in Australia. How annoying".
On my current project, I have the leading Agilist also arguing for fewer rules. So what should you do when your team gets larger? Remove rules that new members find annoying? What about when you find consistent coding errors?
I think the right answer is to start with tight checkstyle rules and then add to them when you find problems. This has always been hard after the event because of all the code that fails. However Checkstyle 3.4 includes a new suppresions.xml file where you can suppress rules in specific lines of a file, a whole file or a set of files, using a regular expression. In the rest of this article I suggest the process of doing just that on the ehcache project.
My review of my checkstyle rules has yielded the following new additions to be added to checkstyle.xml:
<property name="tabWidth" value="4"/>
<module name="GenericIllegalRegexp">
<property name="format" value="\s$"/>
</module>
<module name="ConstantName"/>
<module name="EmptyForIteratorPad"/>
<module name="IllegalInstantiation"/>
<module name="MethodParamPad"/>
<module name="TypecastParenPad"/>
<module name="FinalClass"/>
<module name="MagicNumber"/>
<module name="LocalFinalVariableName"/>
<module name="ArrayTrailingComma"/>
<!-- Generates quite a few errors -->
<module name="CyclomaticComplexity">
<property name="severity" value="ignore"/>
</module>
<module name="NestedIfDepth">
<property name="max" value="3"/>
</module>
<module name="ExplicitInitialization"/>
<module name="AnonInnerLength">
<property name="max" value="20"/>
</module>
<module name="ExecutableStatementCount">
<property name="max" value="20"/>
<property name="tokens" value="CTOR_DEF, INSTANCE_INIT, STATIC_INIT"/>
</module>
<module name="EmptyForInitializerPad"/>
<module name="CovariantEquals"/>
<module name="DeclarationOrder"/>
<module name="ParameterAssignment"/>
<module name="DefaultComesLast"/>
<module name="FallThrough"/>
<module name="MultipleVariableDeclarations"/>
<module name="HiddenField">
<property name="tokens" value="VARIABLE_DEF"/>
</module>
<module name="IllegalInstantiation">
<property name="classes" value="java.lang.Boolean"/>
</module>
<module name="IllegalTokenText">
<property name="tokens" value="NUM_INT,NUM_LONG"/>
<property name="format" value="^0[^lx]"/>
<property name="ignoreCase" value="true"/>
</module>
<module name="IllegalType">
<property name="ignoredMethodNames" value="getInstance"/>
</module>
<module name="JUnitTestCase"/>
<module name="ReturnCount">
<property name="max" value="3"/>
</module>
<module name="NestedTryDepth">
<property name="max" value="2"/>
</module>
<module name="PackageDeclaration"/>
<module name="RedundantThrows">
<property name="allowUnchecked" value="true"/>
</module>
<module name="StringLiteralEquality"/>
<module name="SuperClone"/>
<module name="SuperFinalize"/>
<module name="MutableException"/>
<module name="ThrowsCount">
<property name="max" value="3"/>
</module>
<module name="StrictDuplicateCode">
<property name="min" value="15"/>
</module>
<module name="BooleanExpressionComplexity">
<property name="max" value="4"/>
</module>
<module name="ClassDataAbstractionCoupling">
<property name="max" value="5"/>
</module>
<module name="ClassFanOutComplexity">
<property name="max" value="20"/>
</module>
<module name="NPathComplexity">
<property name="max" value="20"/>
</module>
<module name="NewlineAtEndOfFile"/>
<module name="TodoComment"/>
<module name="TrailingComment"/>
<module name="usage.OneMethodPrivateField"/>
<module name="usage.UnusedLocalVariable"/>
<module name="usage.UnusedParameter"/>
<module name="usage.UnusedPrivateField"/>
I have removed the following:
<module name="AvoidInlineConditionals"/>
The rationale is that some developer find these hard to understand. This seems similar to the justification not to use the this keyword in method setters. I don't think that is sufficient justification to remove a useful language feature.
These changes generated 880 errors. Adding the following suppressions reduced it to 110. These remaining check failures should be fixed.
<suppressions>
<suppress checks="MagicNumberCheck" files="Test.java" />
<suppress checks="usage.UnusedParameter" files="Test.java" />
<suppress checks="usage.OneMethodPrivateField" files="Test.java" />
<suppress checks="StrictDuplicateCode" files=".java" lines="1-53" />
</suppressions>
I made further changes to my code, suppressions.xml and checkstyle.xml.
Here are my full checkstyle.xml and my suppressions.xml files.
Posted by gluck at September 23, 2004 06:44 PM