CompareTo: Comparison method violates its general contract!

8

I found a lot of questions about this, and tried all the suggestions I found, however, the problem persisted.

If someone can help me, here's my problem:

I have a list of requests, this list is updated from pull to refresh, and reordered by compareTo(Object o) of interface Comparable<> .

In a specific case, the error mentioned in the title occurs, I could not handle the error behind try / catch and can not identify the reason.

This is the code for my compareTo method:

@Override
public int compareTo(Notificacao another) {

    if ((another.getEndSLA() != null && another.getEndSLA().length() > 1) && 
            (getEndSLA() != null && getEndSLA().length() > 1)) {

        SimpleDateFormat format = new SimpleDateFormat("dd/MM/yyyy HH:mm",
                Locale.getDefault());

        try {
            Date date = format.parse(getEndSLA());
            Date dateAnother = format.parse(another.getEndSLA());

            return (int) (date.getTime() - dateAnother.getTime());

        } catch (ParseException e) {
            e.printStackTrace();
            return 0;
        } catch (Exception e) {
            Log.d("EXCECAO", "EXCECAO LANCADA : " + e.getMessage() + " THIS : " + getEndSLA() + 
                    " ANOTHER : " + another.getEndSLA());
        } 
    }
    return 0;
}
    
asked by anonymous 15.12.2014 / 19:44

2 answers

9

First, the getEndSLA method should return a Date , or at least have some other method that does, to improve object orientation. It should not be the responsibility of the comparison method to have to know how to convert strings to dates representing SLAs.

So, assuming you can not change the return type of getEndSLA , I recommend you add this from here in the Notificacao class:

public Date getEndSLADate() {
    String s = getEndSLA();
    if (s == null || s.isEmpty()) return null;
    SimpleDateFormat format = new SimpleDateFormat("dd/MM/yyyy HH:mm",
            Locale.getDefault());

    try {
        return format.parse(s);
    } catch (ParseException e) {
        e.printStackTrace();
        return null;
    }
}

So you can simplify your compareTo method, and it's easier to understand, analyze, and improve it:

@Override
public int compareTo(Notificacao another) {
    Date a = getEndSLADate();
    Date b = another.getEndSLADate();

    if (a != null && b != null) return (int) (a.getTime() - b.getTime());
    return 0;
}

Much simpler, right?

That cast is not cool, it can result in something bizarre when the two dates are very different and some of the more significant bits are cut off. Also, we can compare dates directly, without needing getTime() . So I'll sort this out:

@Override
public int compareTo(Notificacao another) {
    Date a = getEndSLADate();
    Date b = another.getEndSLADate();

    if (a != null && b != null) return a.compareTo(b);
    return 0;
}

Clearly, the compareTo method can be perfectly used to sort instances of Notificacao that have the SLA end date . The problem is when they do not have them: return 0 will make them appear to be equivalent.

Let's suppose we have three objects Notificacao . The A with the date of Monday, the B with the date of Tuesday and the C with the date null . Then, according to the compareTo method:

  

A.compareTo(A) produces 0, so A = A B.compareTo(B) produces 0, so B = B C.compareTo(C) produces 0, so C = C A.compareTo(B) produces -1, logo A < B
B.compareTo(A) produces +1, so B > A A.compareTo(C) produces 0, so A = C C.compareTo(A) produces 0, so C = A B.compareTo(C) produces 0, so B = C C.compareTo(B) produces 0, so C = B

These last four lines are the problem, because if A = C and C = B , then we conclude that A = B . But no, because A < B . That is, its compareTo method is inconsistent, so it produces strange and / or incorrect results. This is where the compareTo contract is being violated.

The solution is to make elements with date null come or all before or all after dates. I'll put like everyone before:

@Override
public int compareTo(Notificacao another) {
    Date a = getEndSLADate();
    Date b = another.getEndSLADate();

    if (a == null && b == null) return 0;
    if (a == null) return -1;
    if (b == null) return 1;
    return a.compareTo(b);
}

And now we have this:

  

A.compareTo(C) produces +1, so A > C with C.compareTo(A) produces -1, so C < The% w / w of% produces +1, so B > C with B.compareTo(C) produces -1, so C < B

And finally we have C.compareTo(B) , and there is no inconsistency in your C < A < B .

    
15.12.2014 / 21:19
8

According to interface documentation :

  

The implementation should also ensure that the relationship is transitive: (x.compareTo(y)>0 && y.compareTo(z)>0) implies that x.compareTo(z)>0 .

Now consider the following hypothetical objects:

Objeto  | getEndSLA()
--------|------------
  A     |     2
  B     |     1
  C     |     null

Your comparator would return the following values:

A.compareTo(B) === 1
B.compareTo(C) === 0
A.compareTo(C) === 0

A is greater than B , and B is equal to C . Only A is also equal to C . By syllogism, if A > B and B = C , then A > C . Therefore your code is breaking this logic, which is required of who implements the interface. And that's what the error message is saying.

Reference In Java, What do the return values of Comparable.compareTo mean?

    
15.12.2014 / 20:43