Guava library violates Google Java Style guide

[romani]

Affected Files:796
Violations: 4071

Google Java Style guide (version of March 21, 2014):
https://google-styleguide.googlecode.com/svn-history/r130/trunk/javaguide.html

Checkstyle Maven plugin report for Guava 17.0:
http://checkstyle.sourceforge.net/reports/google-style/guava/
All violations are thoroughly rechecked, but some very occasional false-positives could be present.

Detailed report (match of each Google Java Style rule to Checkstyle):
http://checkstyle.sourceforge.net/google_style.html

How to run (command line):

~ $ cd /var/tmp
/var/tmp $ git clone https://code.google.com/p/guava-libraries/

/var/tmp $ wget -O checkstyle-6.1-all.jar http://downloads.sourceforge.net/project/checkstyle/checkstyle/6.1/checkstyle-6.1-all.jar?r=&ts=1416082535&use_mirror=tcpdiag

/var/tmp $ wget https://raw.githubusercontent.com/checkstyle/checkstyle/master/google_checks.xml

/var/tmp $ time java -jar checkstyle-6.1-all.jar -c google_checks.xml -o checkstyle-report-guava.txt -r guava-libraries

real    2m55.599s
user    3m12.596s
sys 0m1.056s

PS: all work was done during Google Summer of Code 2014

[kevinb9n]

This is awesome!

Our hacked-up version of checkstyle that we use inside Google misses a ton of things -- well, as I guess you can see -- because we're hyper-averse to false positives, and because we've never put quite enough effort into it.

That said, the state of Guava is not quite as bad as this report initially makes it look. :-)

These seem to be the top findings:

926 ParameterName
This is one of the "should be avoided" guidelines in the style guide that is not treated as a bona fide rule. In fact, I don't think anyone ever really intended it to apply to non-public methods in the first place. While I imagine there are a few parameter names in here that really should be improved, I think that checkstyle should stop warning on single-character names. (And I'll try to get the guide fixed.)

740 JavadocMethod
As you can see from browsing through Guava's generated docs, there aren't really anywhere near that many missing method docs. These violations are mostly things like _CustomFieldSerializer classes and other invisible details. (I will grant you that the style guide ought to somehow make it clear that these are exempt.) But actually, according to Google style it is really impossible to enforce this check anyway, since the developer is allowed to use judgment about when the documentation would "have nothing useful to say" and is allowed to skip it in that case.

512 EmptyLineSeparator
Yes, in this case we were just really bad about following this rule and at some point decided we would address it by auto-reformatting all the code one day (which we will probably actually do soon).

307 Indentation
A couple real mistakes here, but mostly we need to exclude LongAdder and Striped64 from checkstyle runs because we intentionally left them in Doug Lea's style.

263 LocalVariableName
Same as with parameter names, the guide never meant to make these actually illegal, and I think we need to fix this.

185 SingleLineJavadoc
Single-line javadoc is explicitly allowed by 7.1.1.

185 EmptyBlock (empty catch block)
When the caught exception is named 'expected' the warning should not be given (6.2). The fix for almost all these cases is just to rename things like 'ignored' or 'tolerated' to expected.

157 JavadocParagraph
Ok, most of these seem to be real problems.

92 OverloadMethodsDeclarationOrder
Ok now this is awesome. I definitely think we should fix all of these.

80 WhitespaceAround
Once again these all look like they should be fixed. However, most of them are "catch... {}" and I think the error will have to be reported in different language or users will be very confused. ("Empty blocks may only be represented as {} when not part of a multi-block statement (4.1.3)")

[kevinb9n]

Colin: as all this github migration stuff is settling down, maybe looking at checkstyle convergence might be possible? :-) :-) (What do you want for Christmas?)

[romani]

Our hacked-up version of checkstyle that we use inside Google misses a ton of things

Why not to contribute Google's fixes over Checkstyle to Checkstyle ? :)

926 ParameterName
263 LocalVariableName

Pelase update a Google Style Guide.

185 SingleLineJavadoc

from style guide "The single-line form may be substituted when there are no at-clauses present," . So all one line javadocs that have atclauses are not allowed.
So please fix Google Style Guide. and lets switch off that rule or update it.

185 EmptyBlock (empty catch block)

Googe Style Guide need to updated to clearly state under what names and comments inside it will be allowed to have empty block. After that we could update Rules to cover them.

[kevinb9n]

Re: SingleLineJavadoc, I will try to fix the bit about "no at-clauses present", as it seems totally unnecessary. We never intended for this to require

/**
 * @see blah
 */

over

/** @see blah */

Also, we used this term "at-clauses" without defining it, which was bad of us. It refers to the separate sections that start with @param, @return, @throws, etc. It does not refer to inline usages of @code, @link, etc., so even regardless of the above, this line shouldn't be warning anyway:

/** Blah blah including a {@code bit of code}. */

Again, I will try to fix this.

Re: EmptyBlock,

(a) the current guide says this: "Exception: In tests, a caught exception may be ignored without comment if it is named expected."
(b) otherwise, as for what comments are allowed, checkstyle has to assume that any comment is good enough.

[cgdecker]

For EmptyBlock, we're also just allowing any exception named expected to be ignored. People should only be doing that in tests, but it's not worth it to try to enforce that.

[romani]

926 ParameterName

Google’s Java Style Guide should be updated

740 JavadocMethod
These violations are mostly things like _CustomFieldSerializer classes

Some exceptions for general rules will always be. You can use suppressions specific to Guava project:

<module name="Checker">
  <module name="SuppressionFilter">
    <property name="file" value="{path}/checkstyle_suppression.xml"/>
  </module>
  ...
</module>

checkstyle_suppression.xml :

<suppressions>
    <suppress checks="JavadocMethod"
              files=".*_CustomFieldSerializer.java"
              />
</suppressions>

since the developer is allowed to use judgment about when the documentation would "have nothing useful to say" and is allowed to skip it in that case

JavadocMethod Check have option “minLineCount”="2", we could extend it to value you think in general mean “self explanatory method”. If that is not an option please confirm switching off that Check.
But my advice is NOT switch it off, as developers can miss meaningful javadocs easily and developer will spend extra 1 min of this time to describe a method in javadoc. 1 minute is not critical time during development or before merge to target branch, but that time save you from other more sad mistakes.

307 Indentation
A couple real mistakes here, but mostly we need to exclude LongAdder and Striped64 from checkstyle runs because we intentionally left them in Doug Lea's style.

checkstyle_suppression.xml :

<suppressions>
    <suppress checks="Indentation"
              files="LongAdder.java|Striped64.java"
              />
</suppressions>

263 LocalVariableName

ok, it is matter of your code standard, just change style guide.

185 SingleLineJavadoc

completely reasonable, but guide is not exact - so guide have to extended to be more precise.

185 EmptyBlock (empty catch block)

We could create special Check to do validation base on comment inside and base on name of variable. Checkstyle could not distinguish test code and production code so that allowance by name will be applied to all code.
Are you agree ?

80 WhitespaceAround
However, most of them are "catch... {}" and I think the error will have to be reported in different language or users will be very confused. ("Empty blocks may only be represented as {} when not part of a multi-block statement (4.1.3)")

Checkstyle have ability to change message in configuration, example - https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/google_checks.xml#L40, so we could make it like:
“WhitespaceAround: '{' is not followed by whitespace. Empty blocks may only be represented as {} when not part of a multi-block statement (4.1.3)”
Are you agree ?

[kevinb9n]

EmptyBlock: yes, that's what we do too. The guide may not say you can name it 'expected' and skip the comment in production code, but checkstyle will let you.

WhitespaceAround: agreed.

Most of the rest of your comment looks like just echoing what we already know needs to be fixed. I can't promise a fixed version of the document will be coming soon though.

[romani]

@kevinb9n , it will also bee good to clarify what code is preferable (discussion at checkstyle/checkstyle#533):

boolean same = Util.<Integer, String>compare(p1, p2);  // Generic preceded method name

vs

boolean same = Util.<Integer, String> compare(p1, p2);  // Generic preceded method name, extra space between

will be good to put that in style document to resolve any further discussions.

[kevinb9n]

That one is addressed here:

https://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s4.6.2-horizontal-whitespace

"Beyond where required by the language or other style rules, and apart from literals, comments and Javadoc, a single ASCII space also appears in the following places only ."

[romani]

Empty Catch Block is implemented and released in Checkstyle 6.4, report was updated - "Last Published: 2015-02-28 | Version: 19.0-SNAPSHOT"
http://checkstyle.sourceforge.net/reports/google-style/guava/

in case you need to regenerate report please use that simple steps: https://github.com/checkstyle/checkstyle/wiki/How-to-generate-Checkstyle-report-for-Google-Guava-project

FYI:
so the only item that is left to be done by Checkstyle team is "4.8.6.1 Block comment style", but will be addressed in next release.

[Vladlis]

New options "ignoreInlineTags" and "ignoredTags" for Single Line Javadoc were added and released in Checkstyle 6.8.
ignoreInlineTags allows inline tags (such as {@code}, {@link} etc.) to be put in a single line javadoc. Report generated with Checkstyle 6.9 over Guava 19.0-SNAPSHOT with this option switched on shows a lot less violations: 53 (http://vladlis.github.io/reports/google_style/) against 227 (http://checkstyle.sourceforge.net/reports/google-style/guava/). So it makes sense to extend the Style Guide (http://google.github.io/styleguide/javaguide.html#s7.1.3-javadoc-at-clauses) and make it more precise in order to allow inline tags. Some googlers already reported that issue at checkstyle/checkstyle#1545, but to change config with new option we need to have update in Google Java Style first.
ignoredTags could also be used if some tags, for instance @see, were allowed by the Guide in a single line javadoc.

[MEZk]

CommentsIndentationCheck was introduced and reliased in Checkstyle 6.12. The Check covers the section 4.8.6.1 Block comment style from Google Java Style Guide. Report generated with Checkstyle 6.12 over Guava 19.0-SNAPSHOT with CommentsIndentationCheck shows 11 violations.
So, Google Java Style Guide was covered by Checkstyle as much as it was possible. See [Google's Java Style Checkstyle Coverage](Google's Java Style Checkstyle Coverage).

[romani]

926 ParameterName

guide is updated - https://google.github.io/styleguide/javaguide.html#s5.2.6-parameter-names

263 LocalVariableName

Clarification issue request was created to style project - google/styleguide#214

185 SingleLineJavadoc
Single-line javadoc is explicitly allowed by 7.1.1.

confirmation request - google/styleguide#215

[kevinb9n]

I think this issue doesn't constitute a Guava bug (anymore), right?