Collections.sort() Comparison method violates its general contract in Java

Question

I know that this kind of question has been asked millions of times if not billions, however I couldn't find my answer yet :)

This compare() method doesn't have Long, Double, Float, ..., it only has Date, boolean, and Null checker, however it shows me that contract violation error, can any one help plz?

Collections.sort(users, new Comparator<MiniUser>() {
        @Override
        public int compare(MiniUser u1, MiniUser u2) {
            boolean resComing = checkMatchConditions(u1,user);
            boolean resExists = checkMatchConditions(u2,user);

            if(Boolean.valueOf(resComing) && Boolean.valueOf(resExists)) {
                if(u1.getLastMatchDate() == null){
                    return -1;
                }else if(u2.getLastMatchDate() ==null ){
                    return 1;
                }else if (u1.getLastMatchDate().toInstant().isBefore(u2.getLastMatchDate().toInstant())){
                    return -1;
                }else {
                    return 1;
                }
            }

            else if (Boolean.valueOf(resComing)) {
                return -1;
            }
            return 1;
        }

    });

MiniUser.class

public class MiniUser implements Serializable {

    String id;
    String name;
    Date lastMatchDate;
    boolean showCompleteName;

//getters, setters
}

checkMatchConditions return boolean based on some calculations

Why do you think you need `Boolean.valueOf(resComing)` etc? They are immediately unboxed by to `resComing`. — Andy Turner, Feb 07 '18 at 08:36
This obviously violates the contract, because it doesn't return 0 when comparing something to itself (if `resComing` and `resExists` are both false). — Andy Turner, Feb 07 '18 at 08:37
I'm pretty sure you want to `return 0` in the end, since all comparison didn't establish an order. — daniu, Feb 07 '18 at 08:38
I was just using resComing but after I read about how this method treat Long, Float, ... which is non-sense, I told my self may be I need to apply this to Boolean — Tamer Saleh, Feb 07 '18 at 08:44
Note: `if(Boolean.valueOf(resComing) && Boolean.valueOf(resExists)) ` is the same as `if(resComing && resExists)` — Peter Lawrey, Feb 07 '18 at 09:00

Malte Hartwig · Accepted Answer · 2018-03-02T14:20:38.860

You should start by reading the JavaDoc of Comparator.compare() to understand what that "contract" is:

The implementor must ensure that sgn(compare(x, y)) == -sgn(compare(y, x)) for all x and y.

In normal terms it says that if "x is greater than y, y must be smaller than x". Sounds obvious, but is not the case in your comparator:

In your case you violate it when two Users have checkMatchConditions false, in which case compare(u1, u2) and compare(u2, u1) both return 1. Hence, there are cases where u1 is greater than u2, and u2 is greater than u1, which is a violation.
Similarely, if both Users have checkMatchConditions true, and their lastMatchDates are both null, they will also violate the contract.
In addition, because you manually try to compare the dates with isBefore, you also return -1 in both cases when two Users have checkMatchConditions true and their lastMatchDates are both equal.

In order to fix this, you should first add a natural language description of how you want the Users to be ordered. Then you can work out the comparator logic.

The error has nothing to do with the Boolean.valueOf() by the way.

Now that you explained how you want to order, have a look at this comparator:

public int compare(MiniUser u1, MiniUser u2)
{
    // order by match
    boolean u1Matches = checkMatchConditions(u1, user);
    boolean u2Matches = checkMatchConditions(u2, user);

    if (u1Matches != u2Matches)
    {
        // put matching ones first
        return u1Matches ? -1 : 1;
    }
    else if (u1Matches)
    {
        // order by dates
        boolean u1HasDate = u1.getLastMatchDate() != null;
        boolean u2HasDate = u2.getLastMatchDate() != null;

        if (u1HasDate != u2HasDate)
        {
            // put the ones without date first
            return u1HasDate ? 1 : -1;
        }
        else if (u1HasDate)
        {
            // order chronologically
            return u1.getLastMatchDate().compareTo(u2.getLastMatchDate());
        }
        else
        {
            // no dates, no order possible
            return 0;
        }
    }
    else
    {
        // both don't match, no order possible
        return 0;
    }
}

If I understood your requirements correctly, this should impose a consistent order to your elements. Note how I use Date's compareTo for the date ordering instead of doing it myself, and how I return 0 in case they are "equal" in regards to the order instead of "randomly" returning 1 or -1.

Thanks a lot Malte, the way I want to sort users here is, if resComing && resExists are true then sort them according to the date, or if resComing is true and resExists is false then put the user of resComing first, other than that I dont care, so its all about adding more conditions? — Tamer Saleh, Feb 07 '18 at 09:01
@TamerSaleh even if you don't care whether a > b or a < b you have to make a consistent choice. — Peter Lawrey, Feb 07 '18 at 09:06
@TamerSaleh Look at the comparator I added. It should order consistently if I understood your explanation correctly. Try to understand how it improves the handling of special cases, especially when the two Users should be considered equal (i.e. "I don't care how these are ordered"). — Malte Hartwig, Feb 07 '18 at 09:33
@MalteHartwig is a date at which this user got a match,so I need to sort users by this date ASC (ppl got a match since a long ago or never got a match comes first) so I can give those ppl who have been waiting the longest a match, — Tamer Saleh, Feb 08 '18 at 09:01
@TamerSaleh is it possible that this date is changed by `checkMatchConditions`? In that case, it would mess with the order during the sorting, which might explain the inconsistencies. Otherwise, could try Peter Lawrey's advice to see if you can identify which MiniUsers cause the problem? It would help if you could add a **small** collection of MiniUser data that results in that error. — Malte Hartwig, Feb 08 '18 at 10:42

score 3 · Answer 2 · answered Feb 07 '18 at 09:04

You need to find where sgn(compare(x, y)) == -sgn(compare(y, x)) doesn't hold. I suggest you use brute force to find examples.

Comparator<MiniUser> comp = ...
for (MiniUser a : users) {
    for (MiniUser b: users) {
         if (a == b) continue;
         if (comp.compare(a, b) != -comp.compare(b, a)) {
            // print an error message with these two.
         }
    }
}

Collections.sort() Comparison method violates its general contract in Java

2 Answers2

Linked

Related