10
\$\begingroup\$

I have the following code which iterates on an Integer[] and makes sure that the the values in the array are in ascending order and that there are no neighbouring duplicates:

Integer previous = null;
for (Integer tb : trancheBoundaries) {
    if (previous == tb) {
        throw new IllegalThresholdConfigurationException("Two boundaries are the same");
    } else if (previous != null && previous > tb) {
        throw new IllegalThresholdConfigurationException("Previous is larger than current - tranche boundaries must be in ascending order");
    } else {
        previous = tb;
    }
}

I am getting a warning from Sonar warning of "suspicious comparison of integer references" on the first if clause.

I know why I am getting this, but in a situation like this, is it bad practice? I think it's ok because I know that I will always be comparing the same instances.

\$\endgroup\$
  • 2
    \$\begingroup\$ Even if it does works in your case, I think it looks too suspicions to future readers. I think you should compare with Objects.equals to avoid causing confusion, following "the principle of least surprise". \$\endgroup\$ – Lii Dec 18 '15 at 13:46
11
\$\begingroup\$

Integer[] vs int[]

This may be down to a 'personal preference' thing, but using primitive wrappers in an array instead of a Collection seems a little odd. This is because it lets null values sneak into the array compared to a 'plain' int[], and speaking of which...

Boundary cases

  • When this loop encounters a null (previous == null), is it really because two boundaries are the same?
  • When equal numbers are encountered consecutively (previous == tb), is this really a case of valid ascending order?

Miscellaneous

You can opt to remove the final else clause as previous = tb is the final 'step' in your comparison. Again, I think this is a personal preferences thing, as some may prefer to see the 'boundaries' of the if-blocks.

\$\endgroup\$
  • 1
    \$\begingroup\$ I hadn't considered the possibility of a null being in the Integer[] - looking at the code I don't see how it could happen but it pays to be sure. This is enough to make me use an int[] instead. \$\endgroup\$ – Rich Dec 18 '15 at 11:56
  • 1
    \$\begingroup\$ When equal numbers are encountered consecutively, in the context of this check, that is not a case of valid ascending order. As for your comment in the miscellaneous section, I prefer to have that in as it seems more explicit. But on the other hand the reference equals is probably less explicit! \$\endgroup\$ – Rich Dec 18 '15 at 11:58
  • 1
    \$\begingroup\$ Wouldn't previous > tb have to be run with tb == null for a null from the array to get into previous? And trying to do > null appears to throw a NullPointerException. \$\endgroup\$ – jpmc26 Dec 19 '15 at 6:01
  • \$\begingroup\$ @jpmc26 it can be the first element being a null. \$\endgroup\$ – h.j.k. Dec 19 '15 at 16:00
16
\$\begingroup\$

You should only use == if you work with elementary data types (boolean, byte, char, short, int, long, float, long).

If you use == on instances of classes like Integer you check if it is the same instance of the class not if the value is the same. You hardly ever want to check if two references refer to the same instance. That is especially true for the primitive wrappers.

So as a rule of thump:

  • use == for boolean, byte, char, short, int, long, float, long
  • use equals(..) otherwise

For little values of wrapper types == might work, but this totally depends on the VM implementation. JLS 5.1.7 Boxing Conversion:

Ideally, boxing a given primitive value p, would always yield an identical reference. In practice, this may not be feasible using existing implementation techniques. The rules above are a pragmatic compromise. The final clause above requires that certain common values always be boxed into indistinguishable objects. The implementation may cache these, lazily or eagerly. For other values, this formulation disallows any assumptions about the identity of the boxed values on the programmer's part. This would allow (but not require) sharing of some or all of these references.

This ensures that in most common cases, the behavior will be the desired one, without imposing an undue performance penalty, especially on small devices. Less memory-limited implementations might, for example, cache all char and short values, as well as int and long values in the range of -32K to +32K.

\$\endgroup\$
  • \$\begingroup\$ I know about - but in this case the value of previous is only ever null or a value which has come from the Integer[], so I feel I can safely use the reference comparison. \$\endgroup\$ – Rich Dec 18 '15 at 11:32
  • \$\begingroup\$ new Integer(x) == new Integer(x) might not be true, because must not be true because it depends on the VM implementation if instances are reused when representing the same value. It is likely to be true for little values because of performance reasons. But do not rely on it. I added a citation and the link to the java specification. By the way you will gain next to nothing to use '==' instead of 'equals' \$\endgroup\$ – MrSmith42 Dec 18 '15 at 11:42
  • \$\begingroup\$ I know that new Integer(x) == new Integer(x) might not be true, but I'm only ever comparing the values that come from the array. I suppose the risk is that the code that built the array could put in two different instances of Integer with the same value, thus causing the test to fail. \$\endgroup\$ – Rich Dec 18 '15 at 12:00
  • 1
    \$\begingroup\$ new Integer(x) == new Integer(x) will never be true, for any JLS conforming VM implementation. \$\endgroup\$ – Lii Dec 18 '15 at 13:41
  • \$\begingroup\$ yes, but I never was doing new Integer(x) == new Integer(x). I was doing myIntegerArray[x] == myIntegerArray[x]. \$\endgroup\$ – Rich Dec 18 '15 at 14:53
5
\$\begingroup\$

You should definitely heed this Sonar warning. It's not just a matter of bad style or bad practice.

Your code will appear to work for numbers between -128 and 127, but will break for numbers outside that range. To check for the equality of two Integer values, use .equals(), not ==.

\$\endgroup\$
  • \$\begingroup\$ In this case the value of previous is only ever null or a value which has come from the Integer[] - which I think means that I can't have the -128/127 autoboxing problem. \$\endgroup\$ – Rich Dec 18 '15 at 11:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.