Loose ends: Optional
brian.goetz at oracle.com
Fri Jun 7 12:47:35 PDT 2013
> I think we have clearly two options, and I'm fine with both of them:
> Option A: We want to add map/flatMap.
> In that case, we also need to add a way to do the conversion back and
> fore to null (orNull() and fromNullable()).
I see your point, but I don't think this necessarily follows (though it
is certainly something to discuss.)
Neither Guava, Fugue, nor j.u.Optional allow Optional to hold a null.
The fromNullable proposal is merely a convenience method; anyone can
write it as an expression (o == null ? O.empty() : O.of(o)) or as a
static method (MyFoo.ofNullable). So it is by no means *necessary*.
Now, the same can be superficially said of map/flatMap -- people can
easily write these themselves too -- but there is a key difference;
fluency. People want flatMap not because it offers functionality people
can't write for themselves, but that they can't otherwise do it fluently
at the end of a chain. But at the other end of the chain, whether the
factory method is "Optional.ofNullable" or "MyFoo.ofNullable", there is
effectively no difference, so these are different cases.
So, I would amend your statement to: if we add map/flatMap, we then have
to ask the question of whether we should also add ofNullable.
I find this method also in the category of "mostly harmless", but left
it out of the initial proposal in spirit of minimality; limiting the set
of proposed methods to those that where having them in the JDK type adds
the most value.
If the EG will only take filter/map/flatMap if we add ofNullable, I'm OK
with that. (On the other hand, if where you are going is "if we add X
we have to add Y, and then we have to add Z, and X+Y+Z is too much, so
let's do none of them", then I'm not so OK.)
> Note that the original code proposed by Brian below already does a
> null check in map().
There are two null checks. First is a check if the mapper function
itself is null -- since a null function is clearly a bug. I assume you
are talking of the other, which is the "treat null return as nothing
there" interpretation of the mapper result. There's a valid choice here
-- NPE vs interpret as empty. The latter is consistent with the other
methods we've added to Map, such as computeIfAbsent. But we can also
discuss throwing NPE here.
> Regardless the option chosen above,
> - Optional should not implement Iterable or have a forEach method. It
> makes the code weird to read.
Sam made a very compelling argument for forEach based on experience at
Twitter over lunch last week. I'll summarize, but Sam please correct me
if I mangle it. Basically: in their library development efforts, there
were a lot of "unrelated" containerish classes (including a variant of
Optional), which each had their own class-specific method names. Over
time, the method names converged to a common vocabulary (filter, map,
forEach) even though they had no common supertype, and as a result, the
perceived usability went way up.
The current ifPresent(Consumer) is something I pulled out of a hat and
the name does not connect to anything. On the other hand,
forEach(Consumer) is familiar from a lot of other classes. I prefer
forEach, on the theory of not introducing new names for new stuff. (I
already hate the forEach / forEachRemaining distinction between
Collection and Iterator, and don't want to make this problem any worse.)
The "don't implement Iterable" and "don't call it forEach" all feel like
they are motivated by "make it less useful so people won't be tempted to
use it as much." There's a (possible) value in that Optional; giving
users multiple reasonable ways to get it at means they have to distort
their code less to use Optional effectively.
> In that case, flatMap is in my opinion useless because the code that returns null will not magically
> be re-written to send an Optional as parameter.
I think you may have a misunderstanding of what flatMap does? The point
here is that you *already* have a method that returns an Optional, and
you want to avoid double-wrapping with Optional<Optional<T>>, which is
what you'd get if you did optional.map(). For example, for "find the
first element in stream B that is larger than any element in stream A":
Optional<T> aMax = a.max();
Optional<T> nextInB = aMax.flatMap(m -> b.filter(e -> e > m)
(written non-fluently for explicitness, but in reality you'd likely
chain off a.max()). Without flatMap, it would be messier.
> - filter doesn't seem to pull it's own weight, for what it worth, I
> never use it on an Option in Scala.
Agreed this is probably the least useful of the bunch, but again, can't
be easily added with fluency by users, the semantics and implementation
are obvious, and its absence might look weird given few things support
map but not filter.
So, here are the decisions I see from this list:
- Given map/flatMap, must we / do we also need ofNullable and orNull?
- If the mapper returns null, should we continue to silently convert to
empty optional or throw NPE?
- Does renaming ifPresent to forEach help or hurt? What about
More information about the lambda-libs-spec-observers