Comparison method violates its general contract when sorting

Question

I keep getting: Comparison method violates its general contract! Exception for the below compare function when I call Arrays.sort(ScreenItems)
One assumption I have is that the ParseInt below is throwing an exception for the left object but not to the right object
Could that be the case?

public int compare(Object o1, Object o2) {
    if (o2 == null || o1 == null)
        return 0;

    if (!(o1 instanceof ScreenItem) || !(o2 instanceof ScreenItem))
        return 0;

    ScreenItem item1 = (ScreenItem) o1;
    ScreenItem item2 = (ScreenItem) o2;

    String subSystem1 = item1.getSubSystem();
    String subSystem2 = item2.getSubSystem();

    if(subSystem1.equals(subSystem2)) {
        return 0;
    } else if(subSystem1.startsWith(subSystem2)) {
        return 1;
    } else if (subSystem2.startsWith(subSystem1)) {
        return -1;
    }

    String order1 = item1.getOrder();
    String order2 = item2.getOrder();

    if (order1 == null || order2 == null){
        String name1 = item1.getName();
        String name2 = item2.getName();

        if(name1 == null || name2 == null)
            return 0;

        return name1.compareToIgnoreCase(name2);
    }

    try {
        return Integer.parseInt(order1) - Integer.parseInt(order2);
    } catch (Exception ex) {
        return 0;
    }
}

Rather than making assumptions about exceptions, I suggest putting logging code in the catch block. In any case, you should be much more specific in what you catch if you plan to continue computing after an exception. — Patricia Shanahan, Jun 17 '15 at 21:50
To elaborate on @PatriciaShanahan's comment, you should probably check to see which of your two operands throws an exception when parsed, and have separate rules in place for: (1) invalid *op* valid, (2) valid *op* invalid, and (3) invalid *op* invalid; all of these should be along the lines of assigning non-numeric order strings a fixed sort position ("non-numeric orders precede `-INT_MAX`", for example). — Shotgun Ninja, Jun 17 '15 at 21:53
@CommuSoft: Huh? That's the very first thing in the method. (I'm not at all sure I think it's right, though. I would expect an instance not to be equal to `null`.) — T.J. Crowder, Jun 17 '15 at 21:55
Establishing a robust comparison method that adheres to a strict contract for use in sorting is more tricky than it appears. — Shotgun Ninja, Jun 17 '15 at 21:56
@user648026: can you give an example of input on which your method fails. You perhaps better explain the *logic behind* you comparison. — Willem Van Onsem, Jun 17 '15 at 21:57
@T.J.Crowder: well a problem is that transitivity is no longer guaranteed. Because `null == a < b == null` means `null < null`, which violates the transitivity of an order relation. As is said [here](http://stackoverflow.com/questions/2401606/comparator-with-null-values) you should handle `null` as infinity far away. — Willem Van Onsem, Jun 17 '15 at 21:58
The fact that it invalidates the general contract means that your relation is either not *reflexive*, *antisymmetric*, or *transitive*. — Willem Van Onsem, Jun 17 '15 at 22:01
@CommuSoft: That's what I meant, which is why I found [your original comment](http://stackoverflow.com/questions/30902970/comparison-method-violates-its-general-contract-when-sorting?noredirect=1#comment49844992_30902970) confusing. Did you mean "shouldn't?" — T.J. Crowder, Jun 18 '15 at 07:06
@T.J.Crowder: sorry, yes indeed, for some reason the `'nt` got stuck in my keyboard ;). — Willem Van Onsem, Jun 18 '15 at 11:40

score 4 · Accepted Answer · answered Jun 17 '15 at 22:30

This is one example of the sort of change that I think is needed. As @CommuSoft pointed out in a comment, the current treatment of null for o1 and o2 breaks transitivity.

I would replace:

if (o2 == null || o1 == null)
    return 0;

with:

if (o2 == null && o1 == null)
    return 0;
if (o1 == null)
    return -1;
if (o2 == null)
    return 1;

This treats null as equal to itself, but less than all non-null references. Of course, you could also choose to treat null as greater than all non-null references, as long as you are consistent. Treating it as equal to everything, as is done in the current code, is not consistent if there are any two non-null references for which you return a non-zero value.

More generally, I suggest writing a set of rules for the ordering, ensuring they meet the Comparator contract, then writing both code and tests to match those rules.

Comparison method violates its general contract when sorting

1 Answers1