java.util.Optional fields

[Remi Forax]

On 09/21/2012 04:51 PM, Brian Goetz wrote:
>>> An alternative of throwing NoSuchElementException was replaced by
>>> returning an Optional.
>> Having Optional enables me to do fluent API thingies like:
>>     stream.getFirst().orElseThrow(() -> new MyFancyException())
>>
>> I would speculate *this* was the intended goal for Optional.
> Absolutely.
>
> Take the example from SotL/L from the reflection library.
>
> Old way:
>
> for (Method m : enclosingInfo.getEnclosingClass().getDeclaredMethods()) {
>        if (m.getName().equals(enclosingInfo.getName()) ) {
>            Class<?>[] candidateParamClasses = m.getParameterTypes();
>            if (candidateParamClasses.length == parameterClasses.length) {
>                boolean matches = true;
>                for(int i = 0; i < candidateParamClasses.length; i++) {
>                    if
> (!candidateParamClasses[i].equals(parameterClasses[i])) {
>                        matches = false;
>                        break;
>                    }
>                }
>
>                if (matches) { // finally, check return type
>                    if (m.getReturnType().equals(returnType) )
>                        return m;
>                }
>            }
>        }
>    }
>
>    throw new InternalError("Enclosing method not found");
>
> Without Optional:
>
> Method matching =
>        Arrays.asList(enclosingInfo.getEnclosingClass().getDeclaredMethods())
>           .filter(m -> Objects.equals(m.getName(), enclosingInfo.getName())
>           .filter(m ->  Arrays.equals(m.getParameterTypes(),
> parameterClasses))
>           .filter(m -> Objects.equals(m.getReturnType(), returnType))
>           .getFirst();
> if (matching == null)
>       throw new InternalError("Enclosing method not found");
> return matching;
>
> This is much better, but we still have a "garbage varaiable", matching,
> which we have to test and throw before returning.
>
> With Optional:
>
> return
>     Arrays.asList(enclosingInfo.getEnclosingClass().getDeclaredMethods())
>           .filter(m -> Objects.equals(m.getName(), enclosingInfo.getName())
>           .filter(m ->  Arrays.equals(m.getParameterTypes(),
> parameterClasses))
>           .filter(m -> Objects.equals(m.getReturnType(), returnType))
>           .findFirst()
>           .getOrThrow(() -> new InternalError(...));
>

Focusing on findFirst() is an error in my opinion, the corresponding 
imperative code
is more readable (if written correctly) and also faster.

Here is how it should be written:

for (Method m : enclosingInfo.getEnclosingClass().getDeclaredMethods()) {
   if (m.getReturnType() == returnType &&
       m.getName().equals(enclosingInfo.getName()) &&
       Arrays.equals(m.getParameterTypes(), parameterClasses)) {
     return m;
   }
}
throw new InternalError(...)

The question is where to draw the line, where to use the stream API 
versus the imperative one.
The stream API is interesting when map/reduce/groupBy or flatMap (etc.) 
are involved,
something that require local variables in their imperative counterparts 
because the stream version
in that case more straightforward to read.

cheers,
Rémi