• Categories
  • Search
Wednesday, February 25, 2015

Why Checkstyle matters...

I’ve read something about ZeroMQ and its java clone recently and decided I should give it a try. JeroMQ is quite small project (same as ZeroMQ I guess), so I decided to checkout the sources directly, since that usually gives me last version of everything, including samples etc. When I opened the project in my IDE, I noticed some minor issues in the code, starting with some unnecessary properties, ending with missing null checks and invalid String comparisons. (You can find these in any project, so that was not so surprising).

Interesting part started when I fixed the issues locally and ran “mvn clean install” to verify my changes, build failed. No surprises there ;) but it failed on check style issues. That was something I did not expect. It turned out, that the project enforces defined set of rules. Not saying that I like the style chosen for that particular project (common guys, its Java, not C), I really liked the idea behind that decision.

Generally, it is far better when single project uses single code style – developer might need to adapt to that and it can take time, but for me, when I got past this period, code is much easier to read and understand, because I don’t need to think about that particular formatting, looking for potentially mistyped code blocks or parenthesis etc. Even when switching among projects with different code style, it is still better than having to adapt to various styles in single module or even class. (Yes, that happens. In any older project, which accepted contributions from lot of people).

After this reflection, I thought about applying this to one project, where I can do almost anything I want – Tyrus. Short version: I did it!

Slightly longer version: It was a pain. Really. And I don’t have by any chance that rich check style rule set as mentioned JeroMQ. To be honest, I thought that we should be fine, since we do often detailed code reviews, but you can always miss something which is not really well formatted.. Nice fact about Checkstyle is, that there are no compromises. You can exclude some rules for some class or even some lines of that class, but .. why? It would add unnecessary housekeeping when modifying the code and it could be accidentally disabled by just refactoring the class name. So I went for “zero tolerance” for defined rules.

The problem is, that there is no simple tool which would fix all check style issues. It kind of makes sense, since lots of them are not easily fixable automatically. Anyway, I found some recipe which involved Eclipse plugin and generating Eclipse formatter from checkstyle.xml. That worked somehow and the formatter can be exported and included to other IDEs (I personally use IntelliJ). There are some issues. One of caveats was max line length. The formatter was able to “clean” these warnings from the build, but sometimes produced code like:

1
2
3
4
5
6
7
8
9
10
if (wseMaxSessionsAnnotation != null) {
TyrusServerEndpointConfig.Builder builder = TyrusServerEndpointConfig.Builder.create(annotatedClass,
wseAnnotation
.value()).
encoders(
encoderClasses)
.decoders(decoderClasses)
.subprotocols(
Arrays.asList(
subProtocols));

Which is far from acceptable :). Long story short, I ended up with defining own formatting rules, setting them in IntelliJ code style, reformatted and then manually checked and fixed issues in all changed source files. It was originally ~500 files, it shrunk a little after my review, since I removed all non-java files from that change. You can see the commit at github (don’t worry if it takes some time to load, it is a bigger one).

Anyway, I’m still glad I did that. Now I know that once the build or pull request passes, it has the code already formatted in a way, that I don’t need to check it (that much). If you are interested, there is checkstyle-build.xml used for build time validation, and the configuration in pom.xml.

What can be better? IntelliJ and I guess even other IDEs could be more accessible for checkstyle configuration. Meaning that formatter/code style/… should have better support for importing checkstyle config file + it should be able to actually use it well. The latter statement is almost true (for some edgecases, which cannot be easily solved I guess), so the first one is more important. Since I know that the settings are already there and its only about correct values of some properties, I cannot see it is not already there.

That’s it for now ;) Hopefully you’ll feel inspired and try to at least run the checkstyle configured to your code style and see whether you can incorporate it easily in your build. If you have any suggestions or comments about this topic, please share them with me, especially if I left out something usable for Tyrus or other projects. Thanks!

Join the discussion

Comments ( 0 )
Please enter your name.Please provide a valid email address.Please enter a comment.CAPTCHA challenge response provided was incorrect. Please try again.Captcha
 

Visit the Oracle Blog

 

Contact Us

Oracle

Integrated Cloud Applications & Platform Services