CODEWITHSUNDEEP
javafindbugs
Ken Y-N
Ken Y-N

Reputation: 15008

Is this a real warning or an over-sensitive lint?

I have this method:

private Boolean compare(String property, String relationOperator,
        String operand) {
    Integer propertyValue = NumberUtils.toInt(property);
    Integer operandValue = NumberUtils.toInt(operand);

    switch (relationOperator)
    {
        case "<":  return propertyValue.compareTo(operandValue) < 0;
        case "<=": return propertyValue.compareTo(operandValue) <= 0;
/*WARN*/case "=":  return propertyValue.compareTo(operandValue) == 0;
        case ">=": return propertyValue.compareTo(operandValue) >= 0;
        case ">":  return propertyValue.compareTo(operandValue) > 0;
        case "!=": return propertyValue.compareTo(operandValue) != 0;
    }
    return Boolean.FALSE;
}

For the line marked /*WARN*/, FindBugs 3.0.0 tells me:

Suspicious comparison of Integer references in com.foo.MyClass.compare(String, String, String) [Scariest(1), High confidence]

I think the code is OK as I am comparing ints not Integers, so can I safely @SuppressWarnings on this line?

Upvotes: 3

Views: 216

Answers (3)

Stephen C
Stephen C

Reputation: 719238

I think the code is OK as I am comparing ints not Integers, so can I safely @SuppressWarnings on this line?

I supposed you could. (I don't see anything intrinsicly unsafe in what you are doing.)

However, you are incorrect in your belief that you are comparing ints.

You have declared propertyValue and operandValue as Integer, and therefore the compareTo call is comparing Integer objects. (Albeit in a way that is safe ...) I'm pretty sure that that is what FindBugs is complaining about.

See Elliot Frisch's Answer for a better way to write the code ... that is most likely faster, and that won't irritate FindBugs.

Upvotes: 0

Elliott Frisch
Elliott Frisch

Reputation: 201477

You code is scary because it uses wrapper classes and comparable when it could use primitives. Also, your code is overly clever. You should try to write dumb code. Something like,

private boolean compare(String property, String operator, String operand) {
    int pv = Integer.parseInt(property);
    int ov = Integer.parseInt(operand);
    if (operator.equals("<")) {
        return pv < ov;
    } else if (operator.equals("<=")) {
        return pv <= ov;
    } else if (operator.equals(">")) {
        return pv > ov;
    } else if (operator.equals(">=")) {
        return pv >= ov;
    } else if (operator.equals("!=")) {
        return pv != ov;
    } else if (operator.equals("=") || operator.equals("==")) {
        return pv == ov;
    }
    return false;
}

Upvotes: 1

chrylis -cautiouslyoptimistic-
chrylis -cautiouslyoptimistic-

Reputation: 77196

Since compareTo returns a primitive int, you're correct, and this code is fine. I recommend submitting this as a bug against FindBugs.

As a note, you're also causing unnecessary autoboxing for your variables. You can just store them in ints and use Integer.compare(propertyValue, operandValue).

Upvotes: 5

Related Questions