Checking objects for identity and Sonar issues

Question

We are checking the quality of our code using Sonar, and Sonar found code which compares objects for identity like this:

if (cellOfInterest == currentCell) { … }

Sonar finds this kind of identity check peculiar enough to call it critical and proposes to replace the identity check (using ==) with a check for equality (using .equals() instead). Identity checks, so the rationale behind this, are often not what is meant.

In our case, however, we iterate through a list of Cells and check in each iteration (currentCell) whether we are handling a special cell we already have (cellOfInterest).

I'd like to hear if there are other patterns than ours which are common and which avoid this issue simply by using a different design. Or what solutions do you propose to avoid using an identity check in the mentioned situation?

We considered a replacement of the identity check with an equality check as described above but it does not seem applicable in our situation because other cells might be "equal" as well but not identical.

All ideas are welcome!

Answer 1

For Strings and most other Java objects, it is possible to have 2 instances which are identity unequal but are actually equivalent by .equals. It's conventional to avoid == for comparison (using equals or compareTo instead) but if it works, it works. You can mark the item as a false positive in SonarQube.

Answer 2

At first you will not run into problems if you just replace your identity check with an equals call (except that you will have to check for null values on cellOfInterest), as the default implementation of equals in Object is the identity check.

if (cellOfInterest != null && cellOfInterest.equals(currentCell)) { … }

will not break the code. It behaves exactly the same way as your code, when I may suppose that currentCell will not be null.

To omit the null check (and retain the behaviour on both values being null) you can also use (since Java 7)

if(Objects.equals(cellOfInterest, currentCell)) { ...}

In general using equals is the better architecture.

To view both cases you mentioned:

• If the class is changeable by yourself, you might (or might not) come to the conclusion that there are better ways for equality than identity; so you just change the equals (and do not forget the hashCode!) in your class.

• If you cannot change the class, you have to trust in a meaningful implementation of equals and hashCode by the provider of the class.

Answer 3

If identity is what you need then it is what you need. The warning makes sense as it is often a bug but in this case it's not.

As an example, IdentityHashMap (which works with identity vs. equality) has this in its javadoc:

This class is not a general-purpose Map implementation! [...] This class is designed for use only in the rare cases wherein reference-equality semantics are required.

So it is rarely useful but has its uses. You are in a similar position. And of course, its internal code does exactly what you expect, it uses == to get a key.

Bottom line: I don't see why you would need to make the code more complex just because some static code analysis tool says it may be a problem - such a tool is supposed to help you, not to force you into some weird construct that will essentially do the same thing. I would explain why == is used in a comment for clarity, mark it as a false positive and move on.

If you really want to remove the warning, the only option I can think of is to use equals and change the equals method to either:

• the default Object#equals which is based on identity
• some implementation that uniquely identifies the cells, maybe based on some unique id or coordinates?