Collectors.minBy/maxBy behavior for null value

[Tagir Valeev]

Hello!

I have not checked first, but actually Stream.reduce(BinaryOperator),
Stream.max(Comparator) and Stream.min(Comparator) behave like findFirst():
all of them throw NPE if the result is null. So I would agree with Peter
and Remi: Collectors.reducing(BinaryOperator) should be changed to throw
NPE if the result of reduction is null to be consistent with Stream.reduce.

Seems that the fix is very simple, one line change in Collectors.reducing
finisher (like a -> a.present ? Optional.of(a.value) : Optional.empty()
instead of a -> Optional.ofNullable(a.value)). However this changes the
published (though unspecified) behavior. Should I log an issue and make a
patch?

With best regards,
Tagir Valeev.

On Wed, Feb 24, 2016 at 2:14 AM, Remi Forax <forax at univ-mlv.fr> wrote:

> I agree with Peter,
> nulls in stream are tolerated for backward compatibility with existing
> code,
> null is not something that should trigger a specific behavior apart a NPE.
>
> regards,
> Rémi
>
> ----- Mail original -----
> > De: "Peter Levart" <peter.levart at gmail.com>
> > À: "Tagir Valeev" <amaembo at gmail.com>, "core-libs-dev" <
> core-libs-dev at openjdk.java.net>
> > Envoyé: Lundi 22 Février 2016 16:09:45
> > Objet: Re: Collectors.minBy/maxBy behavior for null value
> >
> >
> >
> > On 02/22/2016 11:06 AM, Tagir Valeev wrote:
> > > Hello!
> > >
> > > Consider the following code:
> > >
> > > Comparator<String> cmp =
> Comparator.nullsFirst(Comparator.naturalOrder());
> > > System.out.println(Stream.of("a", "b",
> > > null).collect(Collectors.minBy(cmp)));
> > >
> > > It prints Optional.empty, so the result is indistinguishable from empty
> > > Stream (siimlar for maxBy). This behavior is not consistent with
> > > Stream.findFirst() (which throws NPE if the resulting element is null).
> > > Currently minBy spec says:
> > >
> > > This produces a result equivalent to:
> > >       reducing(BinaryOperator.minBy(comparator))
> > >
> > > However reducing spec says nothing what will occur if the result of
> > > reduction is null. To my opinion at very least the spec of reducing
> should
> > > be updated to specify the behavior when reducing result is null. It
> > > probably would also be nice to add a note into minBy/maxBy spec about
> this.
> > >
> > > What do you think?
> >
> > My personal opinion is that it would be better to throw NPE in this case
> > (like with findFirst()). Silently converting null element to
> > Optional.empty() is probably not the desired behavior and I doubt anyone
> > is exploiting this implementation detail in such way.
> >
> > Regards, Peter
> >
> > >
> > > With best regards,
> > > Tagir Valeev.
> >
> >
>