Is this a real warning or an overly 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 int
not Integer
s, so can I be safe @SuppressWarnings
on this line?
source to share
You are coding scary because it uses wrapper classes and is comparable when it can use primitives. Also, your code is too smart. You should try writing 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;
}
source to share
Since it compareTo
returns a primitive int
, you are correct and this code is fine. I recommend passing this as a bug to FindBugs.
As a side note, you are also causing unnecessary autoboxing on your variables. You can just save them in int
and use Integer.compare(propertyValue, operandValue)
.
source to share
I think the code is fine as I am comparing ints not Integer, so can I safely @SuppressWarnings on that line?
I assumed you could. (I don't see myself as dangerous in what you are doing.)
However, you are wrong in your belief that you are comparing int
s.
You have declared propertyValue
and operandValue
how Integer
, and therefore the call is compareTo
comparing objects Integer
. (Although it's safe ...) I'm pretty sure that's what FindBugs is complaining about.
See Elliot Frisch Answer for a better way to write code ... it's most likely faster and won't annoy FindBugs.
source to share