CODEWITHSUNDEEP
javafindbugs
Anonymoose
Anonymoose

Reputation: 2471

Java FindBugs: Suspicious comparison of Long references

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?

Upvotes: 5

Views: 10859

Answers (4)

Jose Tepedino
Jose Tepedino

Reputation: 1584

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!

Upvotes: 1

arshajii
arshajii

Reputation: 129537

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

Upvotes: 15

lpratlong
lpratlong

Reputation: 1461

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

Upvotes: 0

peter.petrov
peter.petrov

Reputation: 39477

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);
}

Upvotes: 1

Related Questions