2

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.

G. Ann - SonarSource Team
  • 22,346
  • 4
  • 40
  • 76
lv.
  • 446
  • 6
  • 18
  • 1
    Simply? Nothing simple about this code. It's very hard to read and understand. I'd refactor it. – duffymo Jan 05 '16 at 10:40
  • unless x is null and y is not null – Stultuske Jan 05 '16 at 10:40
  • What do you want to test? I think FindBugs is getting confused because of all the boolean checks, I can't find a path that would lead to a NPE. Try to refactor the code. – Tunaki Jan 05 '16 at 10:41
  • 1
    The code is horrible but is logically null-safe. Horrible though. – khelwood Jan 05 '16 at 10:46
  • Yeah, I agree completely, I had to make a boolean table just to make sure of what it was trying to do. Unfortunately, it is legacy code that I haven't authority to change (I do wish I could!) – lv. Jan 05 '16 at 10:50
  • @Stultuske In that case, I believe it would never reach the last branch, since (x == null && y != null) would return true before that happened. – lv. Jan 05 '16 at 10:53
  • I too agree the logic is too complex for FindBugs. I would go for something like https://commons.apache.org/proper/commons-lang/javadocs/api-3.1/org/apache/commons/lang3/ObjectUtils.html – Michal Jan 05 '16 at 10:54
  • @lv my bad. the problem is when both are null, it seems. the code is indeed a mess to read. – Stultuske Jan 05 '16 at 10:55
  • if you write ugly code you might as well want to keep it as short as possible :P : `return x == null || y == null ? x != y : x.compareTo(y) != 0;` will do the same ;) – ParkerHalo Jan 05 '16 at 10:59

2 Answers2

6

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).

Community
  • 1
  • 1
T.J. Crowder
  • 1,031,962
  • 187
  • 1,923
  • 1,875
0

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);
}
duffymo
  • 305,152
  • 44
  • 369
  • 561