Code is going to be written once but read many times. A consistent coding style across the entire code base is important to increase readability and maintainability. Luckily, there are tools that can help to define and enforce such styling rules. From mere cosmetics up to nasty code smells, static code analysis can help increase the quality of your code. I wrote some posts on static code analysis in JavaScript a bit more than a year ago (which in the JavaScript world means the tools are now different, ESLint instead of JSCS/JSHint). In this post we’ll see the Checkstyle tool in the Java world, how to use it with TeamCity and IntelliJ and finally a few words about SonarQube.

Checkstyle is easy to integrate in your existing Maven project. You can add it to your pom:

<project>

    <properties>
        <maven.checkstyle.plugin.version>2.17</maven.checkstyle.plugin.version>
    </properties>

    <reporting>
        <plugins>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-checkstyle-plugin</artifactId>
                <version>${maven.checkstyle.plugin.version}</version>
            </plugin>
        </plugins>
    </reporting>

</project>

This configures it as a reporting plugin. You can then run mvn checkstyle:checkstyle, which will generate a report, listing the violations that are found.

Note that you can additionally configure it as a build plugin and use it to break the build with mvn checkstyle:check. This one will break the build if you have any violations whatsoever. You can also configure the plugin so that the check is done automatically during the default lifecycle. This might not be a good idea for an existing project that has too many problems to solve.

As I mentioned in the previous post about JaCoCo, TeamCity can really help solving the problems one step at a time. It supports consuming the XML report that Checkstyle generates and understands it as Inspection Errors. This is a built-in metric of TeamCity, which means that you can define a custom failure condition for that metric. It’s a two step configuration process. First, you need to tell TeamCity where to find the report. That’s done with the XML report processing Build Feature:

Just like with code coverage, you can tell TeamCity to break the build if you have more Inspection Errors than the last successful build. This ensures we can only improve the code, we can’t make it any worse.

I don’t know if it gets better than this, but this is pretty neat.

Back to Checkstyle itself, perhaps it’s a good idea to see what is it offering us actually. We haven’t specified any rules, so by default it uses the Sun rules. If you’re too young to remember, Java used to belong to a company called Sun Microsystems (if you think this is funny, go talk to a 20-something colleague about Sun). The rules are defined and configured in an XML file and you can see it in the Checkstyle repository. My advice is to use this as a base and start building your own rules. There are a lot of rules that you can use and the default set doesn’t use a lot of them. If you’re like me, you’ll feel like a kid in a candy store. Even better, because linting has no sugar! But let’s see how to customize your rules.

To define your own rules, start by downloading the Sun XML file in order to use it as a base. Put it in your code repository, call it something like checkstyle.xml or my-company-checkstyle.xml, your pick. You now have to configure the pom to use this file instead of the default. As per the documentation, you just need to add this bit in the reporting plugin configuration:

<configuration>
    <configLocation>checkstyle.xml</configLocation>
</configuration>

I made some rules more relaxed and I added some rules that were missing and I find useful. First, the rules I relaxed:

  • disabled the JavadocPackage rule. Since I'm not writing library code, I think that documenting packages are a bit too much for me.
  • changed the JavadocVariable rule to be a bit more lenient. I don't want to document private fields, because usually the documentation will be on their corresponding getter/setter method. I do want to document protected/public fields, because if they exist, they are probably important. Ideally fields should be private, so a protected/public field better have a good documentation comment to justify it.
  • relaxed a bit the AvoidStarImport rule. By default it doesn't allow you to do things like import java.io.*. However, I like to let IntelliJ do these things automatically. If IntelliJ wants to use star imports, I'm fine. I allowed a few packages to ignore this rule such as java.io, java.util
  • LineLength limits lines to 80 characters which is a bit retro (80x24 anyone?). I set it to 100 characters.
  • The HiddenField rule tells you when a method parameter is hiding a class field of the same name. Well, I actually do that on purpose for setters and constructors. Luckily the rule acknowledges these cases and offers to ignore them.
  • Disabled the DesignForExtension rule. It confused me a bit. It said either add a javadoc comment to the method that explains how to override it or make it final. I am already demanding comments, so I find this rule a bit pointless. It is probably more suited for library code, but still, I don't understand the value of it, since adding a comment makes it happy.
  • Disabled the FinalParameters rule. This one demands that the parameters of every method are declared as final. I find that a bit too much typing and there is a different rule that I enabled (ParameterAssignment) that accomplishes the same end goal, which is don't allow re-assignment of parameters.

And now, the rules I added. They’re quite a lot but you’re probably already using them without enforcing them:

  • CovariantEquals: this one tells you to implement equals(Object) if you already implemented equals(MyType).
  • DeclarationOrder. This one tells you how to order things in a class. First the static fields, then the instance fields, then the constructors and then the methods.
  • DefaultComesLast. This one makes sure the default label in a switch statement is the last one. I haven't seen anyone breaking this rule but you never know.
  • EqualsAvoidNull. This is a nice one, I actually practice this. Basically, prefer "hello".equals(variable) instead of variable.equals("hello") because it is guaranteed the "hello" literal is not null.
  • ExplicitInitialization. I like the rationale they provide for this one. If you're explicitly initializing a field with the default value of its type, you're not confident enough in your knowledge of how it works.
  • FallThrough. Prevents falling through multiple cases in a switch statement.
  • IllegalCatch. It will break if you're trying to catch an unchecked exception or the Exception exception itself.
  • IllegalThrows. Similar to the previous one, prevents you from throwing an Error or a RuntimeException.
  • IllegalTokenText. I like this one because I used something similar in JavaScript. It will break if your literals contain some text they shouldn't contain.
  • IllegalType. This one allows you to prevent certain types from being used as variables, return types or parameters. The rationale is to not have coupled code to specific classes when there are interfaces. By default, it will complain for HashMap (use Map), HashSet (use Set), etc. Obviously you can still instantiate these types, but the variable you assign them to must be of the matching interface type.
  • ModifiedControlVariable. Prevents you from modifying the for loop variable inside a for loop.
  • MultipleStringLiterals. If a string literal is repeating in a file, why don't you make a constant out of it?
  • MultipleVariableDeclarations. Makes sure each variable is declared on its own line and with a separate statement.
  • NestedForDepth. Prevents deep nesting of for loops.
  • NestedIfDepth. Prevents deep nesting of if statements.
  • NestedTryDepth. Prevents deep nesting of try-catch-finally blocks.
  • NoClone. A bit opinionated, it tells you to not use the clone method in order to implement cloning. It cites Effective Java: Programming Language Guide First Edition by Joshua Bloch as a reference, which pretty much got me sold instantly.
  • NoFinalizer. Checks you're not using finalizers.
  • OneStatementPerLine. Well, it makes sure each statement is placed on a separate line.
  • OverloadMethodDeclarationOrder. Checks that overload methods are grouped together.
  • PackageDeclaration. Makes sure classes are not defined in the default package.
  • ParameterAssignment. Method parameters should not be modified within a method. Instead, define a new variable that says what you're trying to do.
  • ReturnCount. Multiple return statements can be confusing because they create multiple exit paths in a method. This rule defines a maximum.
  • StringLiteralEquality. Makes sure you are not using the == or != operators to compare strings. This can be a life saver if your background is non-Java.
  • UnnecessaryParentheses. Complains if you put unnecessary parentheses in expressions.
  • CommentsIndentation. Makes sure comments are indented properly.
  • EmptyLinesSeparator. This governs the empty lines between code elements. I configure it so it's not mandatory to add an empty line between fields, it is mandatory to have an empty line between other elements and it's not allowed to have multiple empty lines. Heaven.

One final step remains: use Checkstyle in the IDE. I use IntelliJ these days and it happens that there’s a plugin for Checkstyle. It’s called Checkstyle-IDEA and its configuration looks like this:

Some interesting points:

  • You have to point it to your custom rules if you have these, otherwise it uses Sun's rules as well
  • By default it will list violations as errors (red color), which can be a bit annoying for a legacy project. You can  tone it down to list them as warnings and leave the error highlighting for the actual compilation errors.
  • Scan scope intrigued me because by default it won't check your test code. I don't understand why and I don't know what the default behavior of the checkstyle tool is. I will have to look into that at some point. In my mind, test code is also code, it should adhere to the same standards.

Once you install the plugin and configure it, you’re good to go. You will see the Checkstyle violations as you type, in the usual fashion of IntelliJ.

One final comment about SonarQube and why I don’t like it. SonarQube is a code analysis service. It analyzes your code and gives back a report about its quality. Some of its features overlap with code coverage and static code analysis, but I think it offers some more. It classifies the problems by severity and gives you an estimate of how much time it will take to resolve these issues (e.g. “you have 5 months of technical debt”).

Here’s some reasons why I dislike SonarQube:

  • it does too many things. I like the Unix philosophy of simple tools that are specialized for the job. In this example, Checkstyle.
  • it runs as a service, which means I am dependent on it for my build to pass. If I'm outside the corporate network, I can't validate my code. If the service is down, I get a red build. I find it very counter-intuitive to rely on an online quality service instead of plain offline code tools.
  • its configuration, to the best of my knowledge, is done via the "friendly" web UI, where you can click to activate rules. I think it's easier to keep the configuration in the code. The configuration of individual rules can potentially be more complicated than what a UI can handle.
  • it doesn't play well with existing tools. At least in the JavaScript world, it was impossible to define the rules once in jscs and tell SonarQube to follow these rules.

In my opinion, it’s a fancy tool with gimmicks like “you have 5 months of technical debt” to impress. It’s definitely more impressive than an XML output report. But in the end I don’t need a pretty graph, I need tools that help me pay back the debt, and that’s TeamCity and its integration with established tools from the community.

Update: turns out Checkstyle ignores test code by default. You can set the includeTestSourceDirectory to true in the pom.xml in order to lint also test code. I have already updated the example pom.xml. This however will require commenting every unit test method, which might be a bit too much. Luckily this is easily fixable. In the checkstyle.xml, modify the JavadocMethod rule, so that it excludes methods with certain attributes. I’ve set it to allow missing comments for Override (it does it by default), Test and Before.