Final Thoughts

After struggling to get the last few posts out I have adopted a more agile approach to my blog. Now, whatever state the post is in it, will be published on Monday evening. I didn’t quite get everything finished off last week so here goes with my final thoughts about implementing CheckStyle into a new project.

The last changes in my CheckStyle implementation

Having set the Javadoc rules to ignore in my ruleset, I of course remembered that this is the role of the suppression file. The first fix then was to set them back to ‘error’ severity. I’ll explain the changes in the suppression file below.

I have now fixed all the other issues raised in my code base apart from one. I had selected the ‘‘design for extension‘ check. I like the sound of this, but I have to put my hand up and say that this is going a little beyond my comfort zone: Even the notes suggest that it would involve judicious use of ‘@SuppressWarnings’ to use effectively and I was not happy with my efforts to write suitable Javadoc comments. I’ve cut that, for now at least, and added Effective Java (3rd Edition) by Joshua Bloch to my reading list.

Using a suppression file

Having set up a suppression file, so far I had done nothing with it. So considering the comment above, how do I go about suppressing rules using it?

<?xml version="1.0"?>

<!DOCTYPE suppressions PUBLIC
        "-//Puppy Crawl//DTD Suppressions 1.1//EN"
        "http://checkstyle.sourceforge.net/dtds/suppressions_1_1.dtd">

<suppressions>
    <!--  Temporarily ignore Javadoc errors until I have time to fix them  -->
    <suppress files=".*[\\/]src[\\/].*.*" checks="Javadoc"/>
    
    <!--  Permanently suppress these checks in test classes  -->
    <suppress files=".*[\\/]src[\\/].*[Tt]est[\\/].*" checks="JavadocMethod"/>
</suppressions>

As you can see, the files on which to apply the suppression are described using Regex. You can apply them to a single file, or multiple as here using

".*[\\/]src[\\/].*.*"

to apply to all files in the project or

".*[\\/]src[\\/].*[Tt]est[\\/].*"

for test files.

It is quite common to suppress some rules in test files. In my case, I don’t want to write Javadoc for my test methods as my intention is always to have a method name that explains what is being tested. I’m also using test descriptions in case that isn’t clear enough. The fact that I am not requiring documentation doesn’t prevent me from adding Javadoc if I have a parameterised test with many parameters to describe.

Other common rules to suppress in tests are for ‘multiple string literals’ and ‘magic number.’ This makes a lot of sense for unit and integration tests where all your data for assertions will be in the test sources. In the case of a test framework like this, I am more interested in putting my test data into steps or even the page and block classes that I will end up using. I also like to avoid repeating myself with test data as much as anything else. Constants, fields and even enums are useful places to store reusable data depending on the scope. These checks are a good way to remind me to do that. I’m pretty sure I would get lazy and forget about it without the prompts.

Where to apply suppressions

If you apply all suppressions in the suppression file, you have a single place to view your technical debt. The downside of this is that it is separated from your code, and if you incidentally fix an issue, you are unlikely to remember to delete the suppression. For this reason I prefer using the ‘@SuppressWarnings’ annotation on the method. e.g.

@SuppressWarnings("ParameterNumber")

If you have longer methods it is also possible to suppress a few lines only inline using:

// CHECKSTYLE_OFF

and

// CHECKSTYLE_ON

around the code to disable all checks. Alternatively

   // CSOFF: ParameterNumber

and

   // CSON: ParameterNumber

disable just the one check.

We can verify that this works by creating a little class with some warnings to see what happens:

package tech.alexontest.poftutor;

public class InlineSuppressionDemo {
    // This method signature has issues --> (line 5) warnings for LineLength and ParameterNumber expected
    public void hello(final int a, final int b, final int c, final int d, final int e, final int f, final int g, final int h) {
        System.out.println("Hello World!");
    }

    // All checks disabled between the comments --> (line 11) no warnings expected
    // CHECKSTYLE_OFF
    public void hello2(final int a, final int b, final int c, final int d, final int e, final int f, final int g, final int h) {
        System.out.println("Hello World!");
    }
    // CHECKSTYLE_ON

    // Only a single check disabled between the comments --> (line 18) warning for LineLength only expected
    // CSOFF: ParameterNumber
    public void hello3(final int a, final int b, final int c, final int d, final int e, final int f, final int g, final int h) {
        // CSON: ParameterNumber
        System.out.println("Hello World!");
    }

    // All checks back in operation --> (line 24) warnings for LineLength and ParameterNumber expected
    public void hello4(final int a, final int b, final int c, final int d, final int e, final int f, final int g, final int h) {
        System.out.println("Hello World!");
    }

and here we go:

That’s all good, the right warnings displaying on the right lines.

That’s it then, all done?

Well not quite. I haven’t discussed how to apply CheckStyle as part of my build yet. Fortunately there is a CheckStyle plugin for Gradle. Simply add

checkstyle {
    final config_loc = 'config/checkstyle'
    // Adding the line below will cause builds to fail with only warnings, not just with errors
    // maxWarnings = 0
    // Removing the line below will not fail the build, only report results. (use whilst fixing for a newly applied ruleset)
    ignoreFailures = false
    configFile = rootProject.file("${config_loc}/checkstyle.xml")
    toolVersion = '8.5'
}

beneath

apply plugin: 'java'

in your build.gradle and off you go.

I like Gradle’s implementation as it separates out tasks for main and test code unlike the Maven plugin that will only run them all together. The downside of this is that you get your results on two html pages.

build/reports/checkstyle/main.html

build/reports/checkstyle/test.html

 

Other considerations:

Fail build due to warnings

It can be tempting to set it up so that any errors or warnings will fail your build, and in fact this is how I have it set at work.

Fortunately, the plugin has a configuration option to do that. Uncommenting

maxWarnings = 0

in the build.gradle and prepare to fix everything:

I am not so convinced about this however. Styling issues should always be fixed at once, but problems with excessive file length and especially large parameter counts however will often require considerable work to refactor. In practice I want the warning to remind me to fix it, and if I suppress the warning I have to look at the suppression fields for that reminder.

On the other hand, I know that if my build spews more than a handful of warnings, they just turn into spam that you ignore. This makes it less likely that I will bother about remedying the situation when it first arrives. I guess project size plays a lot into this too. Whilst its small, I can keep those in as warnings without failing anything. As my project gets larger, if I don’t pay attention and fix the warnings I may need to rethink that.

 

When it all goes wrong (parsing errors)

A problem I have come across in a previous project, is what happens when CheckStyle cannot parse one of the files. Tempting as it is to think that you can add it to the suppression file, it doesn’t work. Suppression happens by filtering the results after the whole project has been parsed. Sadly, by default a single exception in parsing fails the check. Fortunately there is a new option (CheckStyle 8.6) for the checkstyle.xml to exclude files before parsing: BeforeExecutionExclusionFileFilter

<module name="BeforeExecutionExclusionFileFilter">
  <property name="fileNamePattern" value="module\-info\.java$"/>
</module>

and this will skip the problem file hopefully allowing the scan to complete.

That’s all folks!

And that brings my exploration of CheckStyle to a close. I’m looking forward to getting back to WebDriver in the very near future.

Progress made:

• Tweaked my rules and suppression file.
• Checked that my inline suppression options all work.
• CheckStyle running when starting Tests using the ‘Check’ task.
• Builds failing on the presence of errors.
• Fixed or suppressed all violations so my build completes.

Lessons learnt:

• How a simple task (implement a code style) can get massive.
• How to fail on warnings.
• How to prevent parser errors crashing your scan.

Series Navigation

<< CheckStyle – Enforcing a coding style: part 3