Code review request

[Paul Sandoz]

Hi Joe,

Thanks for the comments.

I have not resolved the more general comments about code style and tense.


On Feb 23, 2013, at 8:42 PM, Joe Bowbeer <joe.bowbeer at gmail.com> wrote:

> We should send these comments in emails?  I don't see a way to comment at
> the link provided.
> 
> I repeat some of Remi's comments regarding formatting below.
> 
> File:
> 
> http://cr.openjdk.java.net/~briangoetz/jdk-8008670/webrev/src/share/classes/java/util/Map.java.patch
> 
> 1. Please run this through a code formatter to conform with Oracle's
> standard.  Things to fix:
> 
> parameter wrapping should indent only 8 spaces:
> 
> + default V merge(K key, V value,
> +                 BiFunction<? super V, ? super V, ? extends V>
> remappingFunction) {
> 
> if-else brace should be on same line:
> 
> + }
> + else if ((newValue = remappingFunction.apply(oldValue, value)) != null) {
> 
> multi-line 'if' always needs braces?
> 
> + if (replace(key, oldValue, newValue))
> +     return newValue;
> 
> 
> 2. replaceAll javadoc: Function#map => Function#apply
> 
> calling the function's {@code Function#map} method
> 
> =>
> calling the function's {@code Function#apply} method
> 

Fixed in the lambda repo.


> 
> 3. replaceAll question
> 
> What's with all the finals?
> 
> +        final Iterator<Map.Entry<K, V>> entries = entrySet().iterator();
> +        while (entries.hasNext()) {
> +            final Map.Entry<K, V> entry = entries.next();
> +            entry.setValue(function.apply(entry.getKey(),
> entry.getValue()));
> +        }
> 
> Why not code this as follows, just like forEach?
> 
> +        for (Map.Entry<K, V> entry : entrySet()) {
> +            entry.setValue(function.apply(entry.getKey(),
> entry.getValue()));
> +        }
> 

Fixed.


On Feb 24, 2013, at 10:09 PM, Joe Bowbeer <joe.bowbeer at gmail.com> wrote:
> A few more comments.
> 
> 1. General:
> 
> The method descriptions should be written 3rd person declarative, according
> to Oracle's style guide
> 
> http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#styleguide
> 
> This is not followed in many places.  For example:
> 
> Get the {@code StreamShape} describing the input shape of the pipeline
> 
> =>
> Gets the {@code StreamShape} describing the input shape of the pipeline.
> 
> 
> 2. Typo (missing space) in PipelineHelper javadoc:
> 
> 40  * the last intermediate operation described by this {@code
> PipelineHelper}.The
> 

Fixed.


> 
> 3. StreamShape enum is missing its per-element javadoc
> 

Fixed.

Paul.