1

After trying for 3 hours all the solutiond that I found with no success, I'm posting my issue here. I am getting the exception:

java.lang.IllegalArgumentException: Comparison method violates its general contract

This is my code:

public int compare(InstrumentModel o1, InstrumentModel o2) {
    int c = 0;
    if(c == 0 && o1.getUnderlyingAsset()!=null) {
        c = o1.getUnderlyingAsset().compareTo(o2.getUnderlyingAsset());
    }
    if(c == 0 && o1.getSymbol()!=null) {
        c = o1.getSymbol().compareTo(o2.getSymbol());
    }
    if(c == 0 && o1.getExpiryDateInDate()!=null && o2.getExpiryDateInDate()!=null) {
        DateFormat df = new SimpleDateFormat("yyyy-mm-dd");
        Date date1 = null;
        Date date2 = null;
        try {
            date1 = df.parse(df.format(o1.getExpiryDateInDate()));
            date2 = df.parse(df.format(o2.getExpiryDateInDate()));
        } catch (ParseException e) {
            e.printStackTrace();
        }
        c = date1.before(date2) ? 1 : date1.after(date2) ? -1 : 0 ;
    }
    return c;
}
gic186
  • 786
  • 6
  • 18
mayukh g
  • 11
  • 1
  • 6
  • 1
    which line is throwing the exception ? – vincrichaud Jun 19 '18 at 09:20
  • @vincrichaud line number is not printed in exception. this method is inside a custom comparator class. stack trace telling the line number from Collections.sort(list, new ExpiryComparator()) only. sorting based on underlyingasset, then symbol and lastly on expirydateindate properties. If I remove expirydateindate here, then records are sorted as expected manner. – mayukh g Jun 19 '18 at 09:22
  • @Berger Could you please help me to understand the line from the post: One way to satisfy the transitivity requirement in compareParents() is to traverse the getParent() chain instead of only looking at the immediate ancestor. – mayukh g Jun 19 '18 at 09:27
  • @mayukhg it means to test also `getParent().getParent()` and `getPArent().getParent().getParent()` and so on, until there is no more parent. Then you'll have covered all cases and can't violate the contract – vincrichaud Jun 19 '18 at 09:29
  • Below suggestion worked for me by checking null values. – mayukh g Jun 19 '18 at 09:59
  • Other issues with your code (ignore if not interested): Does formatting the date and parsing it back buy you anything? Your format pattern string is wrong, use uppercase `MM` for month (or sorting dates from different months won’t work). Consider dropping the long outdated `Date` and the notoriously troublesome `SimpleDateFormat`. [`java.time`, the modern Java date and time API,](https://docs.oracle.com/javase/tutorial/datetime/) is so much nicer to work with. – Ole V.V. Jun 19 '18 at 10:57

1 Answers1

3

You have bad handling of null values.

If one of the compared InstrumentModels has a null getExpiryDateInDate and the other has a non null value, you should not return 0.

You should be consistent. For example:

  • if both are null, return 0
  • if o1.getExpiryDateInDate()!=null && o2.getExpiryDateInDate()==null, return 1
  • if o1.getExpiryDateInDate()==null && o2.getExpiryDateInDate()!=null, return -1.
  • finally, only if both are not null run your date comparison code to determine what to return.

Note: Your null handling of the asset and symbol properties also doesn't look good, but perhaps they are never null (or at least if one has a value, the other also has a value). Otherwise, comparisons such as o1.getUnderlyingAsset().compareTo(o2.getUnderlyingAsset()) could lead to trouble if o2.getUnderlyingAsset() is null (depending on the type of getUnderlyingAsset() and how it implements compareTo).

Eran
  • 387,369
  • 54
  • 702
  • 768
  • thank you!! this suggestion works. I checked null values for expirydateindate field as you suggested. – mayukh g Jun 19 '18 at 09:56
  • If this was the problem, how come the asker didn’t see any `NullPointerException`? – Ole V.V. Jun 19 '18 at 10:46
  • 1
    @OleV.V. `if(c == 0 && o1.getExpiryDateInDate()!=null && o2.getExpiryDateInDate()!=null)`. Based on the OP's comments there was only an issue with the comparison of the expiry dates. No risk of NPE there. – Eran Jun 19 '18 at 10:48
  • Thanks, that explains. I obviously didn’t read the code closely enough the first time. – Ole V.V. Jun 19 '18 at 10:50
  • You may take advantage of [`Comparator.nullsFirst()`](https://docs.oracle.com/javase/9/docs/api/java/util/Comparator.html#nullsFirst-java.util.Comparator-) (or `nullsLast`). – Ole V.V. Jun 20 '18 at 05:13