RFR : JDK-8001642 : Add Optional<T>, OptionalDouble, OptionalInt, OptionalLong

[Joshua Bloch]

A few minor comments based on a quick perusal of Optional.java:

39  * of code if the value is present.)

Period should be outside paren.

47     private final static Optional<?> EMPTY = new Optional<>();


Should be "private static final" according to JLS
8.3.1<http://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.3.1>

Global: The "summary description" (first sentence) of the doc  comment for
most methods and constructors don't use the conventiontal (third-person
singular) verb tense. In other words, the standard would be:

  74      * Returns an empty {@code Optional}.

but this code says:

  74      * Return an empty {@code Optional}.

Also the use of a class name as a noun is suspect. Either:

  74      * Return an empty {@code Optional} instance.

or:

  74      * Return an empty optional.

are generally preferable.

Line 76 (only tangentially related to this change):

  76      * @apiNote

Historically, we used "Note that" to indicate that what followed was a
consequence of some previous normative text, and didn't mandate any
additional restrictions.

  79      * Instead, use {@code isPresent()}.

Shouldn't this be @link instead of @code?

  84     @SuppressWarnings("unchecked")

It's really confusing that the @suppressWarnings tag is on the method
declaration when the the unchecked cast is in the body of the method.
I'd create a local variable initialized to the cast expression, and
place the warning on the local variable declaration, like so:

    @SuppressWarnings("unchecked") Optional<T> result =  (Optional<T>) EMPTY;

    return result;


 125      * Execute the specified consumer with the value if a value is present,


The phrase "execute the specified consumer" is ungainly.  It suggests
martial law at Walmart.  How about "have the specified consumer accept
the value if it is present" or some such?


 156     public T orElseGet(Supplier<T> other) {

Should Supplier<T> be Supplier<? extends T>? That's certainly what a
cursory analysis suggests, but I know how easy it is to get these things
wrong:(

 181     public boolean equals(Object o) {

The equals behavior *must *be documented: that two Optional instances
are equal if an only if (1) their present values are equal or (2)
neither instance contain a value. If you don't document this, users
can't depend on it and independent reimplementers aren't required to
duplicate it. Oh, and speaking of reimplementation, Oracle
<http://www.groklaw.net/pdf4/OraGoogleAppeal-43.pdf> and its amici
(Microsoft <http://www.groklaw.net/pdf4/13-1021-55.pdf>, Scott McNealy
<http://www.groklaw.net/pdf4/13-1021-85.pdf>, BSA
<http://www.groklaw.net/pdf4/13-1021-72.pdf>, Picture Archive Council
<http://www.groklaw.net/pdf4/13-1021-54.pdf>, Ralph Oman
<http://www.groklaw.net/pdf4/13-1021-68.pdf>, Gene Spafford
<http://www.groklaw.net/pdf4/13-1021-62.pdf>) are still claiming that
independently reimplementing an API that is described in a copyrighted
document violates that copyright. Are you guys really comfortable with
this?

Finally:


 172     public <V extends Throwable> T orElseThrow(Supplier<V>
exceptionSupplier) throws V {


I believe that X is a much better name than V for a type variable that
represents an exception (Throwable). The name V is generally reserved
for value types in key-value pairs.

    Josh


On Mon, Mar 4, 2013 at 12:47 PM, Tim Peierls <tim at peierls.net> wrote:

> I like this.
>
> Typo in all four classes: "who's result" -> "whose result" (or find better
> wording)
>
> --tim
>
>
> On Mon, Mar 4, 2013 at 3:29 PM, Mike Duigou <mike.duigou at oracle.com>wrote:
>
>> Optional<T>, OptionalDouble, OptionalInt and OptionalLong are now posted
>> for review on core-libs and lambda-dev.
>>
>> Any comments can be sent to core-libs-dev or this list.
>>
>> http://cr.openjdk.java.net/~mduigou/JDK-8001642/0/webrev/
>>
>> Mike
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/lambda-libs-spec-experts/attachments/20130304/38b1dd67/attachment.html