8

Having issues with comparing two products. I wish to compare the vintage (which is optional) attribute of each of them. But whenever this attribute is null, a NPE is thrown. I thought with Comparator.nullsLast(..) I can deal with null values... But it seems I either have a misunderstanding of how this works or there's something wrong with the code. What do I need to change to get this work null-friendly?

@Override
public int compare(IProduct product1, IProduct product2) throws ProductComparisonException {

    Comparator<IShopProduct> comparator =
        Comparator.nullsLast(Comparator.comparing(IShopProduct::getVintage));

    return comparator.compare((IShopProduct)product1.getProvidedProductData(),
                              (IShopProduct)product2.getProvidedProductData());
}

Thanks in advance

Novaterata
  • 4,356
  • 3
  • 29
  • 51
Remo
  • 1,112
  • 2
  • 12
  • 25
  • is there a chance that `product1.getProvidedProductData()` returns null ? – gvmani May 07 '18 at 13:27
  • Please look over the standard java style guide. People will be more likely to help you if they aren't distracted by idiosyncratic styling issues. In particular, the unnecessary spaces and the fact that the line is so long I have to scroll horizontally – Novaterata May 07 '18 at 13:49

2 Answers2

8

It should be

Comparator<IShopProduct> comparator = 
            Comparator.comparing( IShopProduct::getVintage, 
                             Comparator.nullsLast(Comparator.naturalOrder()));

Comparator.nullsFirst()/nullsLast() consider null value being greater/smaller than nonNull object

Edit

This is implementation of Comparator.comparing():

public static <T, U extends Comparable<? super U>> Comparator<T> comparing(
        Function<? super T, ? extends U> keyExtractor)
{
    Objects.requireNonNull(keyExtractor);
    return (Comparator<T> & Serializable)
        (c1, c2) -> keyExtractor.apply(c1).compareTo(keyExtractor.apply(c2));
}

As you can see it calls keyExtractor.apply(c1).compareTo() so it will throw NPE if keyExtractor.apply(c1) is null

My suggested code using the following function:

public static <T, U> Comparator<T> comparing(
        Function<? super T, ? extends U> keyExtractor,
        Comparator<? super U> keyComparator)
{
    Objects.requireNonNull(keyExtractor);
    Objects.requireNonNull(keyComparator);
    return (Comparator<T> & Serializable)
        (c1, c2) -> keyComparator.compare(keyExtractor.apply(c1),
                                          keyExtractor.apply(c2));
}

Basically it will extracts the value, then pass the comparing values to Comparator.

The values will be passed to the naturalOrder() comparator which resolved to value1.compareTo(value2). Normally it will throw NPE but we have wrapped it with Comparator.nullsLast which have special null handler.

Community
  • 1
  • 1
Mạnh Quyết Nguyễn
  • 17,677
  • 1
  • 23
  • 51
4

This overload of the comparing method will throw an exception if the passed in key extractor function is null or the property extracted is null. Since, you mentioned the vintage property can be null at times then this is the cause of the NullPointerException.

An alternative, to overcome the problem is to use this comparator:

 Comparator<IShopProduct> comparator = 
      Comparator.comparing(IShopProduct::getVintage,
                Comparator.nullsLast(naturalOrder()));

The key extractor i.e IShopProduct::getVintage is the function used to extract the sort key.

The key comparator i.e. the Comparator.nullsLast(Comparator.naturalOrder()) is used to compare the sort key.

Comparator.naturalOrder() here simply returns a comparator that compares Comparable objects in natural order.

Ousmane D.
  • 54,915
  • 8
  • 91
  • 126