This document summarizes the feedback from the first review round. If you have raised an issue that is not included in this summary, it may have already been discussed (and rejected) prior to the first version of the document, or it may have pointed out an error or oversight in which case it has already been addressed and will be made available in the next version.
Quick links:
“I disagree with the Tooling Support statement. For a project the size of OpenJDK and with the large number of contributors, the only way to successfully transition the code base to the new style and ensure commits adhere to that style is through (automated) tooling.”
“I do not like how the rules say "should general not exceed..." because that is too weak of a statement. I would say "must not exceed" so that OpenJDK can have a single guideline that can be plugged into an automatic formatter.”
“I would recommend providing checkstyle rules along with this document. This will make it easier for teams to adopt.”
“I seem to recall that Java sort of prides itself in allowing Unicode (UTF-16) input. The 7-bit ASCII requirement seems to be at odds with that, and non-English writing developers who use the occasional, e.g., umlaut in string literals might disagree.”
“7-bit ASCII. Nope. UTF-8. There is considerable use of \u escapes including occasionally in silly places like the copyright character. The character confusion issues are a non-compelling reason for disfavouring UTF-8. Selecting 7-bit ASCII in 2015 is quaint by 2025 it will seem archaic.”
“This works in English, but languages with accented letters that can be a problem.”
“UTF-8 is a super set of all character encodings. I don't think limiting the project code to ASCII, a very very old encoding, is a good idea for the future of a project that, I believe, is more likely to get more attention with all of the Oracle licensing issues and bad publicity.&rduo;
Some respondants have asked to put static imports before non-static imports:
“Static imports should go before non-static. This is the standard used by Eclipse and IntelliJ AFAIK. The java.time package follows this convention.”
For reference: Out of 2292 source files containing both static and non-static imports 584 (~25%) put the static imports before non-static imports. (1: All files containing both static and non-static imports were collected, 2: The first 'import' in each file was grepped out and then the number of 'static' were counted: xargs -a both-imports.txt grep -h -m1 import | grep static | wc -l.)
jdk.* and sun.* packages:
“A new package namespace "jdk.*" is coming into common use. The location of those imports should be specified. I'd suggest it be added following the javax packages; it's not clear to me whether jdk.* fits neatly into the "external" or "internal" classification. Also, how about "sun.*" packages?”
“The section does not describe the blocks in enough detail. Should there be a blank line between each group of imports?”
“I agree with these rules for source files, but for test files I think using multiple wildcards should be allowed. Tests often use lots of classes from several different packages, and requiring individual imports for them all adds a lot of clutter.”
“I think just be firm in the 'no wildcard imports should be allowed'. The motivation says "Wildcard imports makes it unclear what actually needs to be imported." which I agree with, but it seems to ease up on that requirement by giving contributors the option. My opinion is that you just don't let people do that as it is easier to enforce and you don't have to deal with people's opinions later down the line”
“Sometimes I like to group fields by functionality rather than just strictly public then protected then package then private. In a large class with many fields this increases readability because you don't have to scan up and down looking for fields related to a specific bit of functionality.”
“I think the class structuring suggestions do not work well and do not fit with huge amounts of existing code. A common structure is to define the public API first, then protected, and finally the private/package-private implementation details. Within each section an ordering of fields, constructors, then methods is reasonable - though again a lot of existing code will leave private fields to the end. I think the structuring rules also need to treat library classes and test programs distinctly - library classes are primarily about the API, while tests are not.”
“I agree with the flexibility about logical ordering taking precedence over a a strict member ordering by kind. My example is a nested class (e.g., an Iterator implementation) that's introduced in support of a particular method (e.g., iterator()). It makes sense for those to be adjacent.”
“Static factory methods should be called out in the ordering list. In my opinion, static factory methods should reside after instance fields and before constructors. This make much more sense when reading a file, particularly when the constructor is non-public. The java.time package follows this convention.”
“There should be a recommendation as to the location of getters and setters. In my opinion, these should be placed after the constructors.
There should be a convention defining the standard order of "common" methods. In my opinion, the order should be
compareTo(),equals(),hashCode(),toString(). These should be the last instance methods in a class, before any inner types.My proposal:
- Static fields (first public, then protected, package level and then private)
- Instance fields (ordered by access modifier as above)
- Static factory methods
- Constructors
- Deserialization readObject()
- Methods
- getters and setters
- Meythods
- other
- Methods -
compareTo(),equals(),hashCode(),toString()- Inner types”
“The one particular clause I do take issue with is ordering of constructors. I believe they should be ordered in the opposite way, from the most general to the least general, for the following reason: I'd rather know what each argument means before I need to know its defaults.”
“Since Java 8, the advice that "interface methods should neither be declared public nor abstract" is outdated. In my opinion, interfaces in Java 8 desparately need to have fully defined modifiers. In my coding I always use "public static", "public default" or "public abstract". This makes the code a lot clearer, and is also justified by the likely future addition of protected, package and private scoped methods to interfaces. See the OpenGamma Strata codebase, eg https://github.com/OpenGamma/Strata/blob/master/modules/collect/src/main/java/com/opengamma/strata/collect/timeseries/LocalDateDoubleTimeSeries.java.”
“The order doesn't match the traditional order as specified by the *Modifier productions in chapters 8 and 9 of JLS8. abstract is too late and volatile/transient are swapped. Furthermore, it's confusing that two field modifiers got injected between the method modifiers synchronized and native.”
“Hmm, the order given here is slightly different from given in the JLS, though in practice it might not matter because the modifiers that are in different orders are mutually exclusive. (I haven't verified this exhaustively.) Note also though that the unified list is somewhat confusing because not all modifiers apply to all kinds of members, e.g. default can occur only in interfaces, volatile only on fields, etc. The JLS has different modifiers for different kinds of members.”
for (int i = long initialization expression;
i < long evaluation expression;
i++)
{
// for loop code
}if (somePredicate() ||
someOtherPredicate())
{
System.out.println("...");
}“Moving the brace to a new line (indented the same as the closing brace and the start of the control statement) provides a very obvious visual clue as to the extent of the enclosed block.”
“In my opinion, there is no good reson to ever allow braces to be omitted. While I know that there is a fair bit of OpenJDK written that way, the new guidelines should not pander to old habits. Using braces always is simply the better and safer approach, for clarity and maintenance.”
“Omitting braces should be discouraged (but probably not forbidden). It is a bug waiting to happen: one adds an additional statement to an un-braced if, meant to be executed only when the if() is true, but instead executes unconditionally. This really happens, for instance with Apple's SSL bug; section 3.3 of http://www.dwheeler.com/essays/apple-goto-fail.html recommends always using braces.”
“In my opinion, the short forms should not be allowed. They do not increase readability or clarity, instead they simply add confusion and additional unecessary rules.”
switch (foo) {
case bar:
bar-handling-code;
case blah:
blah-handling-code;
}“I disagree that "case" statements should be (un)indented to be even with the "switch" statement. It is more readable for the "case" to be indented. The "Don't" example is more readable, and what people are used to seeing in code.”
“In my opinion, the switch/case rule is wrong. The "don'ts" example is exactly how the code should be formatted. The switch/case statement is complex and often long, and fuly justifies being indented twice.”
“"case" lines inside a "switch" should IMHO be indented. The current rule adds an exception to an intuitive approach of "things inside { ... } are always indented", and exceptions are best avoided.”
“I personally disagree with this special case.”
For reference: In the OpenJDK Java code base there's a slight favor for indenting case lines. There are 803 files that do not indent case lines, and there are 968 that do. (1: find . -iname "*.java" -exec grep -A1 -m1 "switch (" {} \;, 2: indentation according to level of switch keyword removed, 3: grepped for " case" vs "^case")
“In my opinion, only "Variant 1" should be encouraged. The others are prone to poor effects when refactoring code. I frequently see code where refactoring has changed a variable name and then left indents of wrapped lines in the wrong place. The right answer to this is to be hardline, and insist on no aligned indented lines - all lines should simply be indented using variant 1. Variants 2, 3 and 4 should be rejected.
In our experience, two other specific wrapping styles should be called out as good practice for method calls. The first is to have the method name and "(", followed by a new line, followed by all the parameters on the next lines, ending with ");". The second is to have every method call paremeter on a new line (including the first) with the last one ending with ");". These two styles tend to produce very readable code that handles refactoring well. (The OpenGamma Strata codebase uses these two almost exclusively for wrapped method calls).
Wrapping method declarations: The proposed standard is flawed IMO as it defines an indentation style that cannot cope with refactoring. If the method is renamed, the convention would require all the argumnts to have their indentation changed. In my opinion, all method parameters, including the first, should be indented at 8 chars when wrapping occurs. This approach also tends to avoid problems with throws clauses. The proposed atyle is also vulnerable to further line length issues, as the wrapped line starts a long way to the right.
Wrapping expressions: Again, indented "." method invokations should be at 8 chars, not aligned which is vulnerable to refactoring and being too far to the right.”
“too many permitted forms and the deep indenting puts the non-whitespace start too near the line limit. The Collector example would be better splitting "implements" through "{" on to a single line. It would generally be preferable to keep the whole declaration for Collector on one line if it can be done sensibly.”
“In my opinion, class declarations must be wrapped when a generic declaration includes either "extends" or "super". The reason for this should be obvious - the class declaration is very confusing when using "extends" generics, as the same keyword is used for extending a superclass. Note that this rule generally avoids wrapping in the middle of the generic declaration (something that should be banned IMO).”
“Drop this rule. It's at odds with most other rules, and it requires too many changes to convert between this form and other forms. And it opens a slippery slope for doing something similar in "if" statements, etc.“
Regarding blank line before package declaration:
“IMO, there should be no blank line between copyright and package statement.”
For reference 17350 (~80%) out of 21424 package declarations are preceeded by a blank line in the OpenJDK code base.
“Blocks logically related statements in a method. Long blocks of code should be avoided but if you must, then adding empty lines allows to structure them”
“I think one variable declaration per line is too restrictive. There are many times when multiple variable declarations are easier to read, while also being less verbose. For example:These four variables are obviously related, they all have the same type, and when used (not as a field but as a variable) they all have the same documentation. Compared with:// The bounds of the rectangle double x, y, w, h;There isn't anything gained by breaking it out in this manner, but IMO you lose readability.”// The bounds of the rectangle double x; double y; double w; double h;
“IMO, allowing multiple annotations on the same line is unecessary. Readability is worse.”
1f seems more common than 1F for instance. Also, F and D can potentially be confused with hexadecimal digits, especially for hexadecimal floating point literals.
“I don't remember ever seeing anybody use capital
Dand capitalFfor the float and long literals, although I have seen capitalLfrequently (to differentiate from littleland1, as the document notes). I'm guessing that is where this rule comes from, because otherwise using lowercase literals is more consistent with the0xand0bprefixes (which are both lower case). I definitely do prefer100fto100F. We used floats heavily in Java2D and Swing and JavaFX and I cannot remember ever seeing100Fin any of that code, meaning that we have reams of OpenJDK sources that are in violation of this proposed convention. Also, sinceFandDare both hexidecimal numbers, seeingfloat f = 1 / 5432Fcould easily be mistaken to think it was a hexidecimal number (it looks like one, except for the missing0x).Since you are being inconsistent one way or the other, I would say let them all be lowercase except for
Lwhich should be upper case.”“IMO, "L" for long should be uppercase, but "d" and "f" for double and short should be lowercase. This has been the most common coding standard that I have seen. Also "D" looks a little like "0", while "d" is very clear.”
“IMO, doubles should never use the literal form "2." (ie. a trailing dot).”
“
double d = 1 / 5432D;That's no good. The D looks like a hex digit! And in 0x5432FP21F, one F is a hex digit, but the other is the float type suffix. 0x5432Fp21f is clearly better.Suggested change, including motivation:
- Long integer literals should use the capital letter
L.- Hexadecimal literals should use capital letters
A-F.- All other numerical prefixes, infixes, and suffixes should use lowercase letters.
Motivation: Lower case L resembles a 1 in many monospace fonts which means that the literal
5432lcan be confused with54321. Using capital L suffix avoids this. The lowercase0x,0b,e,p,f, anddcharacters are easier to spot in a long sequence of digits, and they avoid confusion with the hexadecimal digitsA-F.”“lowercase literals probably outnumber uppercase literals at this point and the java library functions output lower case.”
“
Dmight be confused with0, like 'l' (lowercase L) can be threaten as1.”
“Section should advise against using any octal literals.”
“I think this is overreaching. There are times to use static methods and times to use instance methods. In particular, static factory methods are very common and in many cases are preferable to constructors. I'm not sure a big discussion of these issues is warranted. The other items listed in this section are more clear-cut though and are fine.”