Last week Pavel wrote and published an interesting blog post about Why Checkstyle matters … I remember how he was describing his encounter with JeroMQ to me. Especially the part where he wasn’t able to build the project for a few times because of the Checkstyle issues that pop-up right after he executed the build. I found the whole idea behind running code style checks during build appealing as well and decided to do something similar for Jersey.
My main motivation, besides better readability and not mixing various styles, was to make code reviews easier and a little bit more automated. Reviewer doesn’t need to focus and spend his energy on things that are still important but take time and can be done by machines.
In Jersey we have a document called Coding Conventions that summarizes rules that we (and our contributors) ought to be following. For example,
- The HARD LIMIT for the line length is set to 130 characters. This hard limit is enforced by the checkstyle rules.
Never leave out braces in single-line compound statemets, e.g.:
// Should be: if (i > 0) // if (i > 0) { i--; // i--; // }Don’t use star (.*) imports.
Use spaces instead of tabs.
As mentioned in the first point these rules are checked by Checkstyle but they were never really enforced. And since they were never enforced some of them were not taken seriously. Even we weren’t following them all (e.g. the line length).
After discussion we have decided that a subset of the existing rules (checkstyle.xml from Jersey 2.16) should be really enforced during build. At first I ruled out all rules that check JavaDocs because they simply take too much time. Then I was looking for other common code style definitions which might be good for us too and I found Google’s style page (see the google_checks.xml). The whole effort resulted in our own checkstyle-verify.xml set of rules that are executed in (Maven) validation phase of Jersey build. The rest of the rules (based on Sun’s recommendations) are still present in the workspace (checkstyle.xml), they are run but not enforced.
So I had my set of rules (which was the easy part). All I had to do next was to apply them to the code. Sounds easy, it was not. As Pavel said, “It was pain”. No real automated tool to fix those issues. Perfect! So I updated formatter in IntelliJ, created some regular expression to fix the problems and applied them. Job for a monkey (or an useful idiot like me). However, I was able to do it – you can see it in this commit – 751 changed files (ouch!).
I am not saying that all the rules will be there in a year. Some of them are little controversial and we may end up throwing them away or update their scope. However, in the end I am glad that we took the opportunity to do this and (hopefully) improved our code.
Contributions
I am aware that the above might sound scary. You may think you’re going to spend too much time fixing checkstyle issues when developing/prototyping. Don’t worry, you don’t have to comply with check style rules during the process of developing new features (or fixing a bug). You can turn the check off by either using checkstyleSkip maven profile or by setting maven checkstyle.skip property to true.
mvn clean install -PcheckstyleSkipHowever, before creating a pull request on GitHub you should run the build with checkstyle checks otherwise the verification will fail.
IntelliJ IDEA
I’ve prepared a code style XML file for Intellij IDEA (see Jersey.xml, ~/Library/Preferences/IntelliJIdea14/codestyles (or similar directory on your OS). After the file is in place you can review the styles in Preferences -> Editor -> Code Style -> Java (Scheme: Jersey). Changes I’ve made are entirely on Wrapping and Braces tab. Two notes:
- Make sure to check Keep when reformatting -> Line Breaks on the tab – otherwise even manually aligned code gets formatted (which may be unwanted)
- Consider turning off JavaDoc formatting as it can also produce unwanted changes