Code review request

[David Holmes]

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
-----


> 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
>