Getting java.lang.illegalArgumentException : comparison method violates its general contract ! while using Comparator to sort a list

Question

I am trying to sort a list in a specific way in Java, and I found that Comparator is a good way to do so.

I will share with you a pseudo code of the problem.

I have a list of DTOs and let's say I want to sort it by a property(String) in a specific order, for example properties starting with "Hi" should be on top and the rest should be below.

Here is my pseudo code :

list.sort(new Comparator<myDto>(){

    @Override
    public int compare(myDto o1, myDto o2){
        if(o1.getProperty1() != null && o2.getProperty1() == null)
            return -1;
        else if(o1.getProperty1() == null && o2.getProperty1() != null)
            return 1;
        else if(o1.getProperty1().startsWith("Hi") && o2.getProperty1().startsWith("Hi"))
            return 0;
        else if(o1.getProperty1().startsWith("Hi") && !o2.getProperty1().startsWith("Hi"))
            return -1;
        return 1;

    }
});

I used like 4, 5 DTO's I created myself to test, but when I inject a file of 14k DTO's I get a java.lang.illegalArgumentException.

Any ideas ?

The question itself *could* be a duplicate of [this one](https://stackoverflow.com/q/8327514/3182664), but there's more wrong here: 1. You could (and should probably) handle the `null`-case with [`Comparator#nullsLast`](https://docs.oracle.com/javase/8/docs/api/java/util/Comparator.html#nullsLast-java.util.Comparator-). 2. You should be **very** careful and **not** use this comparator in a sorted map or so, because it is *not consistent with* `equals`: You're returning 0 even for objects for which `a.equals(b)==true` does not hold. — Marco13, Jul 28 '18 at 22:35

score 4 · Answer 1 · answered Jul 28 '18 at 21:03

4

Change your final return 1 to return o1.getProperty1().compareTo(o2.getProperty1()) the JVM can compare elements a, b or b, a - if you just return 1 at the end then you will always violate the general contract.

answered Jul 28 '18 at 21:03

Elliott Frisch

198,278
20
158
249

Right Elliott, Indeed there was this line missing in my code :) – FrankelStein Jul 30 '18 at 09:44

score 3 · Answer 2 · answered Jul 28 '18 at 22:27

In your text, you say you want those objects starting with "Hi" before (less than) the other ones. In addition, your code implies that you want nulls at the end (higher than anything else). So your Comparator has to consider 9 cases (Hi, non-Hi, null for o1 in combination with Hi, non-Hi, null for o2) and return the following values:

o1=Hi:     0,-1,-1 for o2=Hi,non-Hi,null
o1=non-Hi: 1, 0,-1 for o2=Hi,non-Hi,null
o1=null:   1, 1, 0 for o2=Hi,non-Hi,null

Your code doesn't follow that table, e.g. for a non-Hi/non-Hi you'll always return 1 instead of 0, e.g. when doing compare("Peter","John") as well as compare("John","Peter"). As Elliot already pointed out, it's crucial that compare(a,b) and compare(b,a) either both return 0 or return results with opposite signs.

P.S. The table assumes you don't care for the ordering within the three groups. If you want one, you can replace the zeroes with the result of e.g. a lexical comparator.

Thanx for tour answer Ralf, it dissipates all doubts I had about my Comparator, Also I tested it and it's working perfectly :) — FrankelStein, Jul 30 '18 at 09:41

Tomasz Linkowski · Accepted Answer · 2018-07-29T09:19:01.273

In the other answers, you can find explanation why your Comparator doesn't work - in short, your returning 1 at the end makes the Comparator inconsistent (compare(a,b) != -compare(b,a)).

Direct Comparator implementations are hard to both write and read. That's why in Java 8, you can use the functional approach using various Comparator methods.

Translating your Comparator to the functional approach yields:

Comparator<String> property1Comparator = Comparator.nullsLast(Comparator.comparing(
        property1 -> !property1.startsWith("Hi")
));
Comparator<MyDto> myDtoComparator = Comparator.comparing(MyDto::getProperty1, property1Comparator);

I believe this approach is much more readable than all the direct Comparator implementations.

PS. If you wanted to achieve the same result as in Elliot's solution (which additionally sorts the strings not prefixed with "Hi" in a natural order), you'd need the following property1Comparator:

Comparator<String> property1Comparator = Comparator.nullsLast(Comparator.<String, Boolean>comparing(
        property1 -> !property1.startsWith("Hi")
).thenComparing(Comparator.naturalOrder()));

Thank you Tomasz, I tested all the answers, which are all correct and clear but I chose yours because the functionnal approach is more efficient and easily readable. — FrankelStein, Jul 30 '18 at 09:40

score 1 · Answer 4 · answered Jul 29 '18 at 07:43

You have to consider that a.compareTo(b) == -b.compareTo(a). Your last test just assumed that if either start with "Hi" you can return 1 but this breaks the rule above. What you can do is something like this.

list.sort((o1, o2) -> {
    String o1p1 = o1.getProperty1(), o2p1 = o2.getProperty1();

    boolean isNull1 = o1p1 == null, isNull2 = o2p1 == null;
    if (isNull1)
        return isNull2 ? 0 : -1;
    else if (isNull2)
        return +1;

    boolean o1p1hi = o1p1.startsWith("Hi"), o2p1hi = o1p1.startsWith("Hi");
    if (o1p1hi)
        return o2p1hi ? 0 : -1;
    else if (o2p1hi)
        return +1;

    return o1p1.compareTo(o2p1);
});

Thanx for tour answer Peter, I tested it too and it works great :) — FrankelStein, Jul 30 '18 at 09:44

Getting java.lang.illegalArgumentException : comparison method violates its general contract ! while using Comparator to sort a list

4 Answers4