One problem I ran into, by the way, is that Eclipse does not actually
enforce the max line length. There's tons of bugs which have been open for
years, which prevent the formatter from wrapping in special circumstances.
The most common was simply that it ignored trailing punctuation. In other
words, if the only thing that would have wrapped was punctuation, it
doesn't wrap. This appears to be a competition between the rule about
putting parens/semicolons/braces on the same line and the rule for
wrapping. The same line appears to win.
Checkstyle, on the other hand, gives a hard error on exceeding the line
length (which it should). This means that a file formatted with a formatter
set with a max line length of X may produce lines longer than X, and will
cause checkstyle to fail. The workaround is to manually insert
@formatter:off and @formatter:on lines so you can manually tweak to make
checkstyle happy without worrying about Eclipse reformatting. Currently,
these tags are not available in the google-styleguide. I have an open issue
about that, with a patch:
https://code.google.com/p/google-styleguide/issues/detail?id=27 . There's
also a few random bugs in the checkstyle implementation of the
google-styleguide (https://github.com/checkstyle/checkstyle/issues/533
and others), but these aren't a big deal, and in any case, only affect us
if we decide to adopt google-styleguide's rulesets.
--
Christopher L Tubbs II
http://gravatar.com/ctubbsii
On Tue, Dec 23, 2014 at 1:24 PM, Christopher<[email protected]> wrote:
Devs,
So, I spent some time yesterday investigating the application of a
CheckStyle configuration to apply to our builds.
I started with the CheckStyle implementation[1] of the Google Style
Guide[2], and a fully formatted Accumulo code base (according to our
current Eclipse formatter standards[3]). I then configured the
maven-checkstyle-plugin[4][5] to run during a build and built repeatedly,
whittled down some of the strictness of the Google style and fixed some of
our errors, to get at a minimal checkstyle configuration that might be able
to add some additional checks during our build. I found at least one bug[6]
doing this and evaluating the resulting warnings, and documented some of
the common problems.
Now, I'm not necessarily arguing for switching to adopting the Google
Style Guide for our standards today (we can consider voting on that in a
new thread). However, I think it might be beneficial to adopt the Google
Style Guide, especially because that project has formatters for several
standard IDEs, and because the latest version of CheckStyle has a ruleset
which conforms to it, which enables the Maven tooling. So, if anybody is
interesting in us doing that, I would certainly get behind it (and could
help make it happen: reformatting, merging, and applying build tooling).
One thing I like about the Google Style Guide, in particular, is that it
doesn't just provide tools to enforce it, it also documents the standard
with words and reasoning, so we have something to consult, even if you're
not using an IDE or any of the tools.
As a result of this, I am going to add something to start enforcing
checks for some of the javadoc problems. We can add more checks later, or
we can adopt the Google Style.
In the meantime, here's a list of the most common style problems I
observed (note: these aren't necessarily "bad", just non-conforming):
Line length violations (we're currently using 160, but Google Style Guide
allows 80 or 100, with 100 enforced in the tools by default; many of our
lines exceed the 160).
Escaped unicode in string literals (especially when they represent a
printable character which should just be inserted directly, instead of
encoded).
Use of lower-case L for long literals (prefer 10L over 10l).
Bad import order, use of wildcards.
Missing empty line between license header and package declaration.
Extra whitespace around generic parameters.
Bad package/class/method/parameter/local variable naming conventions
(like single character variables outside of loops, or use of
underscore/non-standard camelCase).
Missing optional braces for blocks (see a representatively bad example at
CompactionInfo:lines 74-82).
Keywords out of order from JLS recommendations (e.g. static public vs.
public static).
Overloaded methods aren't grouped together.
Multiple statements per line, multiple variable declarations per line.
Array brackets should be on type (String[] names;), not variable name
(String names[];).
Switch statements sometimes omit default case or fall through.
Distance between local variable declaration and the first time it is used
is sometimes too long (many lines).
All uppercase abbreviations in names aren't avoided (LockID should be
LockId).
Operators should start the next line, when wrapping (like concatenation
of two long string literals).
Empty catch blocks (catch blocks should throw an AssertionError if it's
supposed to be impossible to reach, or a comment about why it's safe to
ignore).
Some files have more than one top-level class. They should be in their
own file.
Braces not on same line as statement (like "} else" with "{" on next
line).
Missing javadocs on public methods.
Missing paragraph markers(<p>) in javadoc paragraphs.
Missing javadoc sentence descriptions.
Javadoc tags out of order from standards.
Missing html close tags in javadocs (<b> but no</b>,<ul> but no</ul>)
Use of< and> in javadoc instead of< and> or {@code ...}
[1]:
https://github.com/checkstyle/checkstyle/blob/master/google_checks.xml
[2]: https://code.google.com/p/google-styleguide/
[3]:
https://github.com/apache/accumulo/blob/master/contrib/Eclipse-Accumulo-Codestyle.xml
[4]:
http://maven.apache.org/plugins/maven-checkstyle-plugin/check-mojo.html
[5]: http://stackoverflow.com/a/27359107/196405
[6]: https://issues.apache.org/jira/browse/ACCUMULO-3448
--
Christopher L Tubbs II
http://gravatar.com/ctubbsii
