Comparison method violates its general contract, how to make it transitive?

I dont see how it is not transitive, please someone suggest me the right way. if both values are null

I return 0, the two other statements are pretty obvious.

Why i got IllegalArgumentException with: Comparison method violates its general contract

My compare method: (I compare doubles)

@Override
public int compare(HashMap<String, String> lhs, HashMap<String, String> rhs) {

    double dist1 = 0;
    double dist2 = 0;

    int compInt1 = 0;
    int compInt2 = 0;

    if (lhs.get("dist") != null && rhs.get("dist") != null && !lhs.get("dist").equals("") && !rhs.get("dist").equals("")) {

        dist1 = Double.parseDouble(lhs.get("dist").substring(0, lhs.get("dist").length() - 3));
        dist2 = Double.parseDouble(rhs.get("dist").substring(0, rhs.get("dist").length() - 3));

        dist1 = dist1 * 100;
        dist2 = dist2 * 100;

        compInt1 = (int) dist1;
        compInt2 = (int) dist2;

    }

    if (compInt1 < compInt2) {
        return -1;
    } else if (compInt1 >= compInt2) {
        return 1;
    } else {
        return 0;
    }

}
Jon Skeet
people
quotationmark

Look at this code:

if (compInt1 < compInt2) {
    return -1;
} else if (compInt1 >= compInt2) {
    return 1;
} else {
    return 0;
}

How do you expect that ever to return 0? What values of compInt1 and compInt2 would make both if conditions fail?

This violates symmetry too - it means that compare(x, y) and compare(y, x) can both return 1...

Assuming you're really just trying to compare compInt1 and compInt2 in the obvious way at this point, just replace the whole block with:

// As of Java 7...
return Integer.compare(compInt1, compInt2);

If you're using Java 6 (or earlier) you could use:

return Integer.valueOf(compInt1).compareTo(compInt2);

That's a little inefficient, but it's at least worth using to start with, just to get everything working.

Additionally, I'd strongly recommend extracting the lhs.get("dist") and rhs.get("dist") expressions from your first part - it's horribly repetitive at the moment. And just declare dist1 and dist2 within the block - they're not used elsewhere.

people

See more on this question at Stackoverflow