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

Answer 1

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.