"Suspicious comparison of integer references" while checking for a monotonically increasing sequence

Question

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.

Answer 1

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.

Answer 2

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.

Answer 3

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