CODEWITHSUNDEEP
javafindbugs
lv.
lv.

Reputation: 466

Why is findbugs throwing a null pointer dereference in this code?

I'm running findbugs on our code through Sonarqube, and I'm getting an error for a null pointer dereference:

There is a branch of statement that, if executed, guarantees that a null value will be dereferenced.

The faulty code is simply this:

public static boolean isBigDecimalDifferent(BigDecimal x, BigDecimal y) {
        return (x != null || y != null)
                && ((x != null && y == null) || (x == null && y != null) || x.compareTo(y) != 0);   
}

I'm wondering how this is possible. The only place where an NPE is possible is when calling x.compareTo(y), but if x=null then Java will never analyse that branch, right?

Is this a bug, or am I missing something about the way Java would analyse this statement?

UPDATE

Thanks for the input. I ended up suggesting them to change it to something like:

if (x!=null && y != null)
    return x.compare(y);
else
    return x!=y;

which I find a bit clearer. If no one agrees to the change, I'll do as suggested and just ignore the issue, even though I'd rather avoid that.

Upvotes: 2

Views: 840

Answers (2)

duffymo
duffymo

Reputation: 309008

I'd write that method this way:

public static boolean isBigDecimalDifferent(BigDecimal x, BigDecimal y) {
   if (x == null) throw new IllegalArgumentException("x cannot be null");
   if (y == null) throw new IllegalArgumentException("y cannot be null");
   return (x.compareTo(y) != 0);
}

I think it's far easier to read.

If you don't want the pre-conditions expressed in exceptions I'd say this is clearer, too:

public static boolean isBigDecimalDifferent(BigDecimal x, BigDecimal y) {
   if (x == null) return (y != null);
   if (y == null) return (x != null);
   return (x.compareTo(y) != 0);
}

Upvotes: 0

T.J. Crowder
T.J. Crowder

Reputation: 1075567

The logic is just too complex for FindBugs, it's getting it wrong here. You're right that you've defended against dereferencing null in that code.

I'd simplify it, so FindBugs understands it, and so that any subsequent human reader can also easily make out what it's doing:

public static boolean isBigDecimalDifferent(BigDecimal x, BigDecimal y) {
    if (x == null) {
        return y != null;
    }
    if (y == null) {
        return x != null;
    }
    return x.compareTo(y) != 0;
}

Side note: Usually, you'd have a method to check for equality, and use ! if you want to check for inequality.

In a comment you've said:

Unfortunately, it is legacy code that I haven't authority to change (I do wish I could!)

Then you'll have to note that FindBugs can't figure it out, and make it an exception in your FindBugs setup (described in this question's answers).

Upvotes: 6

Related Questions