0

What's wrong with this comparator method?

I have read : Java error: Comparison method violates its general contract

And understand that if c1 > c2, and c2 > c3, then c1 > c3. I believe this should hold true in the above.

getMaxCosine() returns value between 0..1, and 2nd sort is by the length of the text in the card, the longer the higher ranked.

public int compare(Card c1, Card c2) {
   if (getMaxCosine(c1) > getMaxCosine(c2)) {
       return -1;
   } else if (getMaxCosine(c1) == getMaxCosine(c2)) {
       return getMatchingText(c1).length() >= getMatchingText(c2).length() ? -1 : 1;
   } else {
       return 1;
   }
}
Alex Benjamen
  • 3
  • 1
  • 3
    If c1 and c2 have the same max cosine and the same matching text length, what *should* be the result? What is the *actual* result? – JB Nizet May 01 '19 at 14:51
  • 3
    Note that your comparator could be simplified (and fixed) by simply using `Comparator.comparingDouble(this::getMaxCosine).thenComparingInt(c -> c.getMatchingText(c).length)`. – JB Nizet May 01 '19 at 14:54
  • Hi @jb-nizet, Thx - should I change the "else if" block to something like: `else if (getMaxCosine(c1) == getMaxCosine(c2)) { if (getMatchingText(c1).length() > getMatchingText(c2).length()) return -1; else if (getMatchingText(c1).length() < getMatchingText(c2).length()) return 1; else return 0 }` – Alex Benjamen May 01 '19 at 15:27
  • *In that case, the cards should be equal*: OK, so what should the compare() method return, according to the javadoc of Comparator, when two cards are equal? What does it actually return? – JB Nizet May 01 '19 at 15:29
  • @AlexBenjamen Your question is unclear. What exactly is happening, and what do you expect to happen? – ChocolateAndCheese May 01 '19 at 15:30
  • @ChocolateAndCheese: I'm rarely getting an exception which is hard to reproduce: `Comparison method violates its general contract`. – Alex Benjamen May 01 '19 at 15:32
  • 1
    Hint: `compare(card, card)` should return 0 for any value of `card`. It doesn't. – Jon Skeet May 01 '19 at 15:37
  • @AlexBenjamen I'm trying to let you find the solution by asking you a question that can be answered very easily by just reading the javadoc of Comparator.compare(), and the code of your method. But you don't want to answer it. Try answering it. Read the hint (which is basically the answer to my question) of Jon. – JB Nizet May 01 '19 at 15:39

1 Answers1

1

I think your issue is in your if-else block:

else if (getMaxCosine(c1) == getMaxCosine(c2)) {
       return getMatchingText(c1).length() >= getMatchingText(c2).length() ? -1  : 1;
}

If getMatchingText(c1).length() is equal to getMatchingText(c2).length() then you return -1. This yields an "unstable" sort: In other words, the order two objects with equal values will be reversed after sorting. Moreover, you should return 0 for Cards that are equal under this comparator. I suggest changing the >= comparison to just > in this if-else block:

else if (getMaxCosine(c1) == getMaxCosine(c2)) {
       if (getMatchingText(c1).length() == getMatchingText(c2).length()) return 0;
       return getMatchingText(c1).length() > getMatchingText(c2).length() ? -1  : 1;
}
ChocolateAndCheese
  • 889
  • 6
  • 9