Compareto: Comparison method violates its general Contract!

Asked

Viewed 1,484 times

8

I found many questions on this subject, and tried all the suggestions I found, however the problem persisted.

If anyone can help me, there’s my problem:

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

In a specific case, the error quoted in the title happens, I could not handle the error behind the Try/catch and could not identify the reason.

This is the code of my method compareTo:

@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;
}

2 answers

10


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

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

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;
    }
}

Thus, you can simplify your method compareTo, and makes it 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 It’s not cool, it can result in something bizarre when the two dates are very different and some of the most significant bits are cut. In addition, we can compare the dates directly, without needing the getTime(). Then I’ll fix it:

@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 method compareTo can be perfectly used to sort the instances of Notificacao date of end of SLA. The problem is when they don’t have them: return 0 will make them appear to be equivalent.

Let’s assume we have three objects Notificacao. The object To with the date of Monday, B with the date of Tuesday and the C with the date null. So according to the method compareTo:

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

These last four lines are the problem, because if A = C and C = B, then we would conclude that A = B. But no, yeah A < B. That is, your method compareTo is inconsistent, and therefore produces strange and/or incorrect results. This is where the contract of compareTo is being raped.

The solution is to make the elements with date null come or all before or all after those who have dates. I will put as all 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
C.compareTo(A) produces -1, hence C < A
B.compareTo(C) produces +1, so B > C
C.compareTo(B) produces -1, hence C < B

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

9

According to the interface documentation (in free translation):

The implementation must 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 that A is also equal to C. By syllogism, if A > B and B = C, then A > C. So your code is breaking this logic, which is required of whoever implements the interface. And that’s what the error message is saying.


Reference In java, What do the Return values of Comparable.compareTo Mean?

Browser other questions tagged

You are not signed in. Login or sign up in order to post.