Code review request

[David Holmes]

On 26/02/2013 3:31 AM, Paul Sandoz wrote:
> 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

I see a lot of private things that are now package-access. Is that 
because they are now being used within the package?

The access modifiers document intended usage even if there is limited 
accessibility to the class defining the member. The idea that a class 
restricted to package-access should have member access modifiers 
restricted to package-only or else private, is just plain wrong in my 
view. Each type should have a public, protected and private API. The 
exposure of the type within a package is a separate matter. 
Package-access then becomes a limited-sharing mechanism.

David

> We can update the webrev next week.
>
>
> On Feb 23, 2013, at 11:51 AM, Remi Forax <forax at univ-mlv.fr
> <mailto:forax at 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.
>