Hello @core-libs-dev <[email protected]>,

Any bandwidth for putting together an Optional.compare method?

Here is a JBS entry from 2017 -- https://bugs.openjdk.org/browse/JDK-8184703
.

I am happy to provide the implementation and the PR, if given permission to.

I ran into an annoying bug today that had to do with me implementing a
Comparator over Optional incorrectly. Long story short, I needed a
histogram, and to avoid using null as a key, I used Optional. But I also
wanted the results sorted, Optional.empty() being the first result, then
normal order for the rest.

Here was the implementation.

(o1, o2) -> o1.isEmpty() ? -1 : o2.isEmpty() ? 1:
o1.get().compareTo(o2.get())

I plugged that into a TreeMap, and put the TreeMap into a
Collectors.groupingBy. Like this.

Collectors.groupingBy
(
    Function.identity(),
    () -> new TreeMap(/* above implementation */),
    Collectors.counting()
)

Upon adding my Comparator, all instances of Optional.empty() stopped being
summed up, and instead, generated a separate entry in the TreeMap
(meanwhile the present Optionals were being summed up just fine, as before)

The bug is that my equals was not aligning with the definition of
comparable, and thus, extra unnecessary entries were being generated in my
TreeMap. The correct implementation should have been this instead.

(o1, o2) -> o1.isEmpty() && o2.isEmpty() ? 0 : o1.isEmpty() ? -1 :
o2.isEmpty() ? 1: o1.get().compareTo(o2.get())

It's an elementary bug, but when introduced into a complex pipeline, can be
easy to overlook. Plus, with the syntax sugar of
Comparator.comparing(...).thenComparing(...), my high school CS instincts
were starting to fade lol.

Anyways, this email is just me poking to see if times have changed enough
that this is worth implementing.

Thank you for your time and attention.
David Alayachew

Reply via email to