What is the correct way to implement compareObjects()

Question

I have compareObjects method implemented as below

public static int compareObjects(Comparable a, Comparable b){

        if (a == null && b == null){
            return 0;
        } else if (a == null && b != null){
            return -1;
        } else if (a != null && b == null){
            return 1;
        } else {
            return a.compareTo(b);
        }
    }

When I run this through findBugs, I get this suggestion on the line return a.compareTo(b):

There is a branch of statement that, if executed, guarantees that a null value will be dereferenced, which would generate a NullPointerException when the code is executed. Of course, the problem might be that the branch or statement is infeasible and that the null pointer exception can't ever be executed; deciding that is beyond the ability of FindBugs. Due to the fact that this value had been previously tested for nullness, this is a definite possibility.

At this point a can never be null. Why does FindBugs show me this suggestion? How can I correct this; what is the correct way to implement compareObjects()?

From the description, it sound like it doesn't like the fact that you're going to say that the objects are different if one of them is null(IE return -1 if a == null, which is masking the fact that a == null). — Nicholas, Aug 30 '11 at 15:17
Comparable is a generic type. Are the type parameters of A and B the same? Otherwise you might get some interesting errors at runtime, since compareTo is only meant to be called on objects with the same type. Also, why do you need this helper method in the first place? To provide an ordering when one element is null? — Thorn G, Aug 30 '11 at 15:18
Static analysis tools are not infallible - they can and do make false positives sometimes — matt b, Aug 30 '11 at 15:24

score 1 · Answer 1 · edited May 23 '17 at 12:26

It may be a limitation in FindBugs; I agree that you've covered all the bases, but your null-check is split across two different conditions. Now these conditions happen to be complementary, so at least one of them will fire if a is null, but depending how sophisticated FindBugs is, it may not recognise this.

Two options here, then:

Just ignore the FindBugs warning. Due to its nature it will raise some false positives from time to time, so don't feel like you have to rewrite your code to make it 100% happy if you don't think the rewrite is worthwhile on its own merits.

You can use the @SuppressWarnings annotation to actually communicate this to FindBugs, if you want the report to show a nice big zero at the end. See this question for an example.
Restructure the condition so that the nullity check on a is more explicit, by nesting if blocks:
```
if (a == null) {
   return b == null ? 0 : -1;
}
return b == null ? 1 : a.compareTo(b);
```
Depending on your tastes and style that might be a better rewrite anyway, in that is more clearly says "if a is null, do this calculation and return it, otherwise do this calculation". You can of course change the ternary condition into another if-else block if you prefer that.

Thanks Andrzej, yes there is no point to rewrite my code to make findBugs 100% happy :) I like your statement. — javanerd, Aug 30 '11 at 15:27

score 1 · Accepted Answer · answered Aug 30 '11 at 15:26

I think it might be because you don't need the extra && statements. After the first if statement you already know that one of them is null.

public static int compareObjects(Comparable a, Comparable b){

    if (a == null && b == null){
        return 0;
    } else if (a == null){
        return -1;
    } else if (b == null){
        return 1;
    } else {
        return a.compareTo(b);
    }
}

score 1 · Answer 3 · answered Aug 30 '11 at 15:28

1

Looking at it again , try this code:

if (a == null && b == null){
    return 0;
} 

if (a == null){
    return -1;
} 

if (b == null){
    return 1;
} 

return a.compareTo(b);

answered Aug 30 '11 at 15:28

TofuBeer

60,850
18
118
163

What is the correct way to implement compareObjects()

3 Answers3