1877: Add checkstyle to project

[andygibson]

Add checkstyle plugin to common.gradle
Add a checkstyle.xml based on google checkstyle xml with ammendments.
Checkstyle errors do not fail the build since everything is marked as a warning.
modules can be refined over time as per comments in #1877.

Checkstyle can be run with ./gradlew checkstyleMain - it runs for all projects.

[stephengold]

Andy, thank you for your contribution to the JMonkeyEngine project.

[stephengold]

Now that continuous tests have run, I see some changes will be required. Andy, would you be willing to do some rework on this PR?

1. To avoid breaking JDK8 builds, the CheckStyle toolVersion should be rolled back to 9.3
2. As it stands, the output is too overwhelming to be of use. I downloaded the archived log for the "windows jdk17" run, and it came to 12.3 MBytes. We should start smaller and expand coverage later, either with just a handful of Checkstyle modules (as proposed in checking Java code style #1877) or else just checking a subset of the project that's relatively clean (such as jme3-core/src/main/java/com/jme3/renderer, which was cleaned up by apply preferred style to com.jme3.renderer #1653). Both approaches have trade-offs.

[andygibson]

Ah hey, just saw your comments - already did the fix for 9.3 and pushed a new checkstyle.xml to go with it.
I'll have a look at curtailing some of the checks, I'll just mark them with ignore if thats ok so we can re-enable them later and don't miss any.

[andygibson]

So I've got it down to about 2.2M, ignore some modules and skipping the jme3-vr and jme3-networking projects.

At this point, taking out modules is about the only way to get it down. jme3-examples and jm3-core are the next two biggest culprits at 632K and 912K respectively.

[stephengold]

Progress!
I imagine you could use the "BeforeExecutionExclusionFileFilter" module to limit coverage just to com/jme3/renderer ...

[andygibson]

BeforeExecutionExclusionFileFilter wasn't playing nicely, checkstyle favours exclusion over inclusion so makes it difficult to exclude everything but a specific set.
Adding an include to the config means we only process that package, now we only get a very small number of errors in one 19Kb file.

[andygibson]

Its always just after you push you spot the obvious mistakes ;-)

I put the include in, but it was only looking at java files directly under com/jme3/renderer I've changed it to look in packages below that.
It now produces 200Kb of content over 4 projects. I can roll this back if you want the smaller-version.

[stephengold]

The portion of the codebase that was cleaned up in #1653 was specifically just the files directly under com/jme3/renderer in the jme3-core sub-project, so expanding it to packages below that in 4 sub-projects seems to me like a step backward. But since the scope of checking is supposed to evolve over time, it's not crucial to perfect it right now. The key things are:

1. you've demonstrated how the scope can be tweaked and
2. you've shrunk the GitHub Actions logs down to a size that can be browsed using a web browser

That's good progress!

Looking at the GitHub Actions logs, I see some CheckStyle diagnostics for code I believe conforms to our preferred coding style. For instance:

[ant:checkstyle] [WARN] /home/runner/work/jmonkeyengine/jmonkeyengine/jme3-core/src/main/java/com/jme3/renderer/Camera.java:744:5: All overloaded methods should be placed next to each other. Placing non-overloaded methods in between overloaded methods with the same type is a violation. Previous overloaded method located at line '712'. [OverloadMethodsDeclarationOrder]

Regarding Section 3.4.2 of the Google style guide, our guidelines say "Logical ordering of class contents is encouraged but not required." So I suggest removing the OverloadMethodsDeclarationOrder module entirely.

Also:

[ant:checkstyle] [WARN] /home/runner/work/jmonkeyengine/jmonkeyengine/jme3-core/src/main/java/com/jme3/renderer/Camera.java:1135:26: Local variable name 'rVal' must match pattern '^[a-z]([a-z0-9][a-zA-Z0-9]*)?$'. [LocalVariableName]

Section 5.2.7 of the Google style guide says only that "local variable names are written in lowerCamelCase". I don't see any reason the second character can't be uppercase. I suggest modifying the pattern to ^[a-z]([a-zA-Z0-9]*)?$

Also

[ant:checkstyle] [WARN] /home/runner/work/jmonkeyengine/jmonkeyengine/jme3-core/src/main/java/com/jme3/renderer/Camera.java:1142:1: Comment has incorrect indentation level 0, expected is 12, indentation should be the same level as line 1,144. [CommentsIndentation]

Regarding Section 4.8.6.1 of the Google style guide, our guidelines say "Commented-out code need not be indented at the same level as surrounding code." I suggest removing the CommentsIndentation module entirely.

Also

[ant:checkstyle] [WARN] /home/runner/work/jmonkeyengine/jmonkeyengine/jme3-core/src/main/java/com/jme3/renderer/Camera.java:1457:45: Abbreviation in name 'viewZPos' must contain no more than '1' consecutive capital letters. [AbbreviationAsWordInName]

The letter "Z" is not an abbreviation or acronym, so I think viewZPos is acceptable camelcase. I suggest increasing the value of the allowedAbbreviationLength property to 1.

[andygibson]

I hadn't really looked too much at the actual content, just took the google config at face value with a few changes as per the project style guide. The details and scope of projects its checking can be changed over time.

Could consider in future changing the max allowedAbbreviationLength to 3, there are a lot of RGB, VBO, EXT, GLXxxx, ARB references in there. You can specify allowedAbbreviations, but it doesn't improve things much.

[stephengold]

Could consider in future changing the max allowedAbbreviationLength to 3, there are a lot of RGB, VBO, EXT, GLXxxx, ARB references in there. You can specify allowedAbbreviations, but it doesn't improve things much.

Sure. But unlike the z in viewZPos, most of these references are actual abbreviations, acronyms, or initialisms that have been (incorrectly IMO) converted to camel case. (And many of them are defined in APIs we don't control, which is a whole 'nother issue!)

I foresee a need for further tweaking of the Checkstyle configuration. For instance, the VariableDeclarationUsageDistance module is generating false positives that might actually encourage poor coding style. But such tweaking can be an ongoing project. No need to creep the scope of issue #1877 any further.

Thank you @andygibson for getting the ball rolling, so to speak.

Unless there's further substantive discussion of this pull request, I plan to integrate it in about 96 hours.