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

Question

I'm trying to sort a collection of loans where the statuses of loans are Active, Rejected, Pending and Approved.Active loans should get the highest priority and the others in any order.I implemented the Comparator interface's compare().

here 's the implementation of compare()

@Override
public int compare(Object o1, Object o2) {
    LoanAccountData loanAccountData1 = (LoanAccountData) o1;
    LoanAccountData loanAccountData2 = (LoanAccountData) o2;

    if (loanAccountData1.getStatusStringValue().equals("Active")) {
        return -1;
    } else {
        return 1;
    }
}

You're comparing two objects by completely ignoring one of them. So that doesn't make sense. For example, if both objects are active, comparing a to b makes a < b, and comparing b to a makes b < a. Also, it seems your comparator is not generic (generics exist since Java 5). It sgould be a Comparator. — JB Nizet, Nov 18 '17 at 07:44

davidxxx · Answer 1 · 2017-11-18T07:54:10.860

Here :

if (loanAccountData1.getStatusStringValue().equals("Active")) {
    return -1;

You never compare with the second object.
If both loanAccountData1.getStatusStringValue().equals("Active") and loanAccountData2.getStatusStringValue().equals("Active") are true, you violate the symmetric principle as you should return 0.

With a Generic Comparator you could write :

public class LoanAccountDataComparatorByStatusActive implements Comparator<LoanAccountData> {

    @Override
    public int compare(LoanAccountData  o1, LoanAccountData o2) {    

        boolean isData1StatusActive = o1.getStatusStringValue().equals("Active");
        boolean isData2StatusActive = o2.getStatusStringValue().equals("Active");

        // if both status active, these have the same rank
        if (isData1StatusActive && isData2StatusActive){
           return 0;
        }

        if (isData1StatusActive){
           return -1;
        }

        if (isData2StatusActive){
           return 1;
        }
        // if no status active, these have the same rank
        return 0;
    }

}

Note that it assumes that getStatusStringValue() never returns null.
Otherwise add required checks before referencing it.

score 2 · Accepted Answer · answered Nov 18 '17 at 07:53

According to the contract of compare: here

Your code violates this rule:

The implementor must ensure that sgn(compare(x, y)) == -sgn(compare(y, x)) for all x and y. (This implies that compare(x, y) must throw an exception if and only if compare(y, x) throws an exception.)

If the status of x is active, and y's status is also active, then compare(x, y) will say that x is smaller, while compare(y, x) will say that y is smaller.

I think what you want to do here is to sort all the active accounts to the start of the list. This means that if both accounts are active or both are something else, they are equal. If one account is active, that is considered smaller.

LoanAccountData loanAccountData1 = (LoanAccountData) o1;
LoanAccountData loanAccountData2 = (LoanAccountData) o2;
boolean o1Active = loanAccountData1.getStatusStringValue().equals("Active");
boolean o2Active = loanAccountData2.getStatusStringValue().equals("Active");
if (o1Active == o2Active) {
    return 0;
} else if (o1Active) {
    return -1;
} else {
    return 1;
}

Could be even shorter with https://docs.oracle.com/javase/7/docs/api/java/lang/Boolean.html#compare(boolean,%20boolean) — JB Nizet, Nov 18 '17 at 09:28
@JBNizet I think at that point you start to lose readability... — Sweeper, Nov 18 '17 at 09:30
To me, `Comparator.comparing(loanAccountData -> loadAccountData.getStatusStringValue().equals("Active"))` is much more concise, has less repetition, and is more readable. — JB Nizet, Nov 18 '17 at 09:33

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

2 Answers2