We can update the webrev next week.
On Feb 23, 2013, at 11:51 AM, Remi Forax <[email protected]
<mailto:[email protected]>> 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 agree with this, if there are no further objections i will fix in the
lambda repo towards the end of the week.
For default method, some of them are marked public, some of them are not,
what the coding convention said ?
AFAICT "public" was only on two such default methods, so i have removed
that modifier.
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;
Regarding code conventions i would prefer to auto-format all code to
ensure consistency, as to what that consistency is, well we could argue
until heat death of the universe :-) I am fine as long as it is
consistent and easy to hit Alt-Cmd-L or what ever it is in ones
favourite IDE.
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.
I have made changes to all inner classes to conform to this. I have also
marked all classes as final where appropriate.
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.
They should be private, they are not accessed outside of that class. I
will fix.
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.
OK, there are probably lots of missing null checks in the code...
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).
Done.
In MatchOp, line 110, I think the compiler bug is fixed now ?
Not yet, i can still reproduce it.
The enum MatchKind should not be public and all constructor of all
inner classes should not be private.
Done.
In OpsUtils, some static methods are public and some are not,
OpUtils is now gone in the lambda repo. The forEach and reduce
functionality is moved into the corresponding op classes. The static
method has been moved to a default method on PipelineHelper.
in Tripwire:
enabled should be in uppercase (ENABLED).
method trip() should be:
public static void trip(Class<?> trippingClass, String msg)
Done. I also made the field and method package private.
Thanks,
Paul.
