Code review request (original) (raw)

Remi Forax forax at univ-mlv.fr
Mon Feb 25 09:03:24 UTC 2013

On 02/25/2013 04:07 AM, David Holmes wrote:

On 23/02/2013 8:51 PM, Remi Forax wrote:

On 02/21/2013 08:17 PM, Brian Goetz wrote:

At http://cr.openjdk.java.net/~briangoetz/jdk-8008670/webrev/

I've posted a webrev for about half the classes in java.util.stream. None of these are public classes, so there are no public API issues here, but plenty of internal API issues, naming issues (ooh, a bikeshed), and code quality issues.

Hi Brian, All protected fields should not be protected but package visible. Classes are package private so there is no need to use a modifier which offer a wider visibility. The same is true for constructors. I believe some of these may end up being public (TBD), in which case better to define member accessibility as if they were already public as it greatly simplifies the changes needed later. David -----

Given that the release of jdk9 is at least two years from now, this API will change, one will come with a GPU pipeline (Sumatra?) or with a flattened bytecode pipeline (my pet project), so trying to figure out now what should be public or not is like predicting the future in a crystal ball. I think it's better to let all members package private and see later. BTW, I have no problem with protected methods, my main concern is protected fields or protected inner classes.

Rémi

For default method, some of them are marked public, some of them are not, what the coding convention said ? Code convention again, there is a lot of if/else with no curly braces, or only curly braces on the if part but not on the else part. Also, when a if block ends with a return, there is no need to use 'else', if (result != null) { foundResult(result); return result; } else return null; can be simply written: if (result != null) { foundResult(result); return result; } return null;

All inner class should not have private constructors, like by example FindOp.FindTask, because the compiler will have to generate a special accessor for them when they are called from the outer class. In AbstractShortCircuitTask: It's not clear that cancel and sharedResult can be accessed directly given that they both have methods that acts as getter and setter. If they can be accessed directly, I think it's better to declare them private and to use getters. Depending on the ops, some of them do nullcheks of arguments at creating time (ForEachOp) , some of them don't (FindOp). In ForEachUntilOp, the 'consumer' is checked but 'until' is not. in ForEachOp, most of the keyword protected are not needed, ForEachUntilOp which inherits from ForEachOp is in the same package. In ForEachUntilOp, the constructor should be private (like for all the other ops). In MatchOp, line 110, I think the compiler bug is fixed now ? The enum MatchKind should not be public and all constructor of all inner classes should not be private. In OpsUtils, some static methods are public and some are not, in Tripwire: enabled should be in uppercase (ENABLED). method trip() should be: public static void trip(Class<?> trippingClass, String msg) cheers, Rémi

More information about the core-libs-dev mailing list