0

I have a Vehicle class that needs to be sorted whenever I have a list of Vehicle objects. I've implemented the Comparable interface and overrode the compareTo() method.

It works, but occasionally the application crashes (this is an Android application). The error I get is java.lang.IllegalArgumentException: Comparison method violates its general contract!.

Here's the code of the compareTo() method:

//this is used as a comparison criteria when using the Collections.sort() method on a Vehicle list
@Override
public int compareTo(@NonNull Object o) {
    Vehicle v = (Vehicle) o;
    try {
        if (this.getVehicleParameters().getMainSwitch().equals("on") && v.getVehicleParameters().getMainSwitch().equals("on")) {
            if (this.getVehicleId().compareTo(v.getVehicleId()) > 0) value = 1;
            else if (this.getVehicleId().compareTo(v.getVehicleId()) == 0) value = 0;
            else value = -1;
        } else if (this.getVehicleParameters().getMainSwitch().equals("on") && v.getVehicleParameters().getMainSwitch().equals("off")) {
            value = -1;
        } else if (this.getVehicleParameters().getMainSwitch().equals("off") && v.getVehicleParameters().getMainSwitch().equals("on")) {
            value = 1;
        } else if (this.getVehicleParameters().getMainSwitch().equals("off") && v.getVehicleParameters().getMainSwitch().equals("off")) {
            if (this.getVehicleId().compareTo(v.getVehicleId()) > 0) value = 1;
            else if (this.getVehicleId().compareTo(v.getVehicleId()) == 0) value = 0;
            else value = -1;
        }
    } catch (Exception e) {
        if (this.getVehicleParameters().getMainSwitch() == null) value = -1;
        else value = 1;
    }

    return value;
}

What it's doing is this: it first sorts by the main switch of the vehicle (if the engine is on or off) and then it sorts it alphabetically. So you get a list with the vehicles that have the engine turned on, sorted alphabetically, and then at the end of this list are the vehicles with the engine turned off, sorted alphabetically.

In the catch block I put a simple rule that if the current object's main switch is null, then return the value -1. Else, return 1.

However, every now and then the application crashes, giving me the error java.lang.IllegalArgumentException: Comparison method violates its general contract!

Can you see what's wrong here?

PS. This is NOT a duplicate of another question simply because we're dealing with possible NULL values here, and that's when the issue arises - when it gets in the catch block. At least, that's the only reasonable explanation. The error appears randomly, maybe after 2 minutes, maybe after 30 minutes (the application is real-time).

Robert Ruxandrescu
  • 627
  • 2
  • 10
  • 22
  • You're saying that it's not a duplicate, yet it is: all comparisons have a notion of null handling, expressed in forms of "nulls first" and "nulls last", but they are not part of the contract for that method, and your method does violate the general contract. – M. Prokhorov Aug 21 '17 at 11:44
  • I have a suggestion though: reimplement your comparison method using a `Comparator.comparing` builder methods. It will save you many o`lines and possibly relieve of that exception as well. – M. Prokhorov Aug 21 '17 at 11:52
  • Can you be a little more specific? I'm not familiar with either the Comparator nor the nulls first/nulls last behavior. – Robert Ruxandrescu Aug 21 '17 at 11:55
  • 1
    A comparator is a materialized expression responsible for comparing two objects of a given type, similarly to what is expected of `compareTo` method, but without knowledge of internal object structure, with allowing to have more than one way of comparing a thing to another thing, and without requirement for a thing to implement `Comparable` itself. Used when "thing" doesn't define any natural ordering, or for cases where ordering is business- or user-defined to a certain extent. – M. Prokhorov Aug 21 '17 at 12:34
  • Well, honestly, I'd rather keep it the way it is and just find out what's wrong about it than to re-write it altogether. – Robert Ruxandrescu Aug 21 '17 at 12:46
  • Well, what is wrong with it is it uses exceptions for controlling the flow, and it uses nested `if` chains. This added up should've caused code duplication, if it followed the contract, but there is no code duplication, thus it doesn't follow requirement of "for every a and b if `a < b`, then always `b > a`". – M. Prokhorov Aug 21 '17 at 13:08
  • That might be, but the thing I don't get is - where is the issue in my current implementation. The vehicle ID isn't checked for, but it's pretty much never null. And also, the scenario where the value returned is 0 is never taken into account. These are the only issues I can think about. – Robert Ruxandrescu Aug 21 '17 at 13:46

0 Answers0