15

FindBugs is giving me a warning about the following line, where invoiceNumber is an Integer object:

text.append(String.format("%010d-", (invoiceNumber == null) ? 0 : invoiceNumber));

The warning is: "Boxed value is unboxed and then immediately reboxed"

Now I think I understand (un)boxing, but I can't see how you would do the same thing without getting the warning?

I have found that I can get rid of the warning using the following code instead, but this seems more long-winded:

int invNo = (invoiceNumber == null) ? 0 : invoiceNumber;
text.append(String.format("%010d-", invNo));

Can someone show me what is the 'correct' way to do the above?

BTW, I've looked at the related questions and I understand what was going on with them, but this doesn't seem to match any of those.

DuncanKinnear
  • 4,563
  • 2
  • 34
  • 65
  • 1
    Perhaps `text.append(String.format("%010d-", (invoiceNumber == null) ? Ineger.valueOf(0) : invoiceNumber));` will do. – Eran Aug 17 '15 at 04:48
  • Yes, that does get rid of the warning, but why is that 'better'. It's constructing an Integer object (which should be expensive), but it shouldn't need to. – DuncanKinnear Aug 17 '15 at 04:53
  • 1
    In the case of 0 (or any integer from -128 to 127), it's not constructing any new Integer instance, since the instance is already available in the IntegerCache. And you are saving the unboxing and boxing operations, so it might give you a slight performance improvement. – Eran Aug 17 '15 at 04:55
  • 1
    Actually you do construct a new `Integer` in your original code as well as in your fix with `int invNo = (invoiceNumber == null) ? 0 : invoiceNumber;` (though FindBugs warning disappears), but using @Eran solution you will not construct the new Integer, but reuse the existing one. Well though Eran answer is correct (upvoted) in general you may just ignore this warning. It's very unlikely that it will actually harm the performance of your application. The created extra `Integer` is very short-lived. – Tagir Valeev Aug 17 '15 at 05:00

2 Answers2

25

The type of the (invoiceNumber == null) ? 0 : invoiceNumber) expression is int. It requires unboxing of invoiceNumber in the case invoiceNumber is not null.

On the other hand, String.format expects a single String argument followed by Object references, which means your int get immediately boxed to Integer again.

You can try to avoid the original unboxing by using (invoiceNumber == null) ? Integer.valueOf(0) : invoiceNumber), which would make this expression return an Integer.

Eran
  • 387,369
  • 54
  • 702
  • 768
  • So does that mean that my 2 line 'solution' in my question is actually less efficient, because the `invoiceNumber` is unboxed to `invNo`, and then reboxed to an `Integer` for the `String.format`? – DuncanKinnear Aug 17 '15 at 04:58
  • 1
    @DuncanKinnear, exactly! – Tagir Valeev Aug 17 '15 at 05:01
  • 8
    @DuncanKinnear I'm assuming that your two line 'solution' only prevents FindBugs from detecting the unboxing and reboxing, so it doesn't actually change anything compared to your original code. – Eran Aug 17 '15 at 05:01
  • @DuncanKinnear No, the second version has the same inefficiency (save the assignment to a one-time-use local variable which the compiler probably optimizes away). – David Harkness Aug 17 '15 at 19:03
1

Try changing (invoiceNumber == null) ? 0 : invoiceNumber to (invoiceNumber == null) ? 0 : invoiceNumber.intValue(). I think the warning comes from you using the Integer again in the "false" case, not an int.

saagarjha
  • 2,221
  • 1
  • 20
  • 37