Java FindBugs: Suspicious comparison of Long references

4
votes

FindBugs recognizes the following bug in my code:

Suspicious comparison of Long references

I have tried Googling this particular error but with no luck. As far as I know comparing fields of data type Long should be done with == operator. 

if (materialDefinition.getId() == definitionToRemoveFromClass.getId()) {
    //...
}

I am certain both methods .getId() return a Long.

Is this really an issue and if so, how do I fix it?

4
javafindbugs
Are these longs the primitive type or are they the object? (java.lang.Long) They are 2 different things - DirkyJerky
In which version of FindBugs is this rule defined? I could not find it on the list of descriptions for version 3.0.1 (findbugs.sourceforge.net/bugDescriptions.html), and would like to activate it, for enhanced code safety. - Jose Tepedino

4 Answers

14
votes

Is this really an issue and if so, how do I fix it?

Yes, you're comparing references not values, so two different Long instances with identical values will result in that comparison being false. You should use Long#equals() instead:

final Long id1 = materialDefinition.getId();
final Long id2 = definitionToRemoveFromClass.getId();

if (id1.equals(id2)) {
    //...
}

If getId() can return null, then also be sure to include the appropriate null check (although getId() doesn't sound like a method that should be returning null).

1
votes

You may want to do something like this.

public static boolean equals(Long a, Long b){
    if (a==null && b==null) return true;
    else if ((a==null) != (b==null)) return false;
    else return a.equals(b);
}
0
votes

You'd better use if (materialDefinition.getId().equals(definitionToRemoveFromClass.getId()))

Be sure that materialDefinition.getId() is not null.

To compare these values (< >) you can use .compareTo() method :

A == B: A.compareTo(B) == 0

A < B: A.compareTo(B) < 0

A > B: A.compareTo(B) > 0

0
votes

Yes it's a bad smell, and you should use the equals() method instead, as explained @arshajii above.

What I've found most intriguing is the fact that such comparison might even work sometimes, like starting with small values (-128 to 127), and will break for larger numbers. For example:

@Test
 public void compareSmallIds() {
     Long id1 = 127L;
     Long id2 = 127L;

     Assert.assertTrue(id1 == id2);
     Assert.assertTrue(id1.equals(id2));
     Assert.assertTrue((long) id1 == (long) id2);
     Assert.assertTrue(id1.longValue() == id2.longValue());
 }

@Test
 public void compareIds() {
     Long id1 = 500L;
     Long id2 = 500L;

     Assert.assertFalse(id1 == id2);
     Assert.assertTrue(id1.equals(id2));
     Assert.assertTrue((long) id1 == (long) id2);
     Assert.assertTrue(id1.longValue() == id2.longValue());
}

It seems this behavior is based on number cache policy definition, with byte values range as default. But definitely, code with non-primitive values should not rely on the == operator for value comparison!