Code review request

Remi Forax forax at univ-mlv.fr
Sat Feb 23 10:51:41 UTC 2013


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.

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