Hi Remi, Thanks for the feedback i have addressed some of this, mostly related to inner classes, in following change set to the lambda repo:
http://hg.openjdk.java.net/lambda/lambda/jdk/rev/3e50294c68ea We can update the webrev next week. On Feb 23, 2013, at 11:51 AM, Remi Forax <fo...@univ-mlv.fr> 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.
