Tuesday, January 06, 2009

Java Equals Issues

I tracked down a bug in jSuneido to a string comparison that was failing because I was using == instead of .equals

From The Java Programming Language:
The equality operators (==, !=) test for reference identity, not object equivalence. Two references are identical if they refer to the same object; two objects are equivalent if they logically have the same value.
and:
If you use == to compare string objects, you are actually comparing to see if they refer to the same object, not testing if the strings have the same contents.
I know this, but it's obviously not as ingrained into my thinking as it needs to be. When I see str1 == str2 I don't immediately see a problem. (It worked fine in C++.)

To complicate matters, it's ok to use == for characters, numbers, enums, and for comparing to null. It's even ok for strings if they're interned or if they came from literals (since they're interned).

After finding this bug, I reviewed every == in my code. I found a half dozen more that should have been .equals (and got sore eyes).

If I had been writing the code from scratch I probably would not have had as much problem with this. But porting the code from C++ was worse because in C++ you handle this by redefining operator== (And I didn't know enough to review every ==)

To add insult to injury, switching to .equals opens up the potential for another error - null values. If you do x.equals(y) and x happens to be null, then you'll get a run-time exception. (Which you wouldn't have gotten with x == y.) This is why you should write "string".equals(x) instead of x.equals("string") because "string" can't be null.

(Interesting that it's called a NullPointerException. I thought Java didn't have pointers :-) Another leaky abstraction.)

So potentially I've introduced new bugs in the process of fixing the ones caused by == instead of .equals

It looks like FindBugs can catch this problem, I'll have to give it a try.

Maybe I'll learn to like it, but right now this part of the Java language seems pretty ugly.

3 comments:

Unknown said...

I don't understand. This works.. Two separate objects.

@Test
public void testStringCMP() {
String str1 = "test";
String str2 = "test";
org.junit.Assert.assertTrue(str1==str2);
}

Andrew McKinlay said...

That's one of the reasons this issue is confusing.

String literals (e.g. "string") are "interned" which means that Java ensures there is only one instance of each string. So your two "test" string literals actually get combined into one value. So it's as if you wrote:

str1 = str2 = "test"

But if the string comes from somewhere else, e.g. user input, then this won't be the case.

I think this would fail:

str1.substring(0, 1) == str2.substring(3, 4)

i.e. comparing first "t" to last "t"

Unknown said...

Fun with autoboxing.
Andrew have a look at this man trap.

Integer i1 = 100;
Integer i2 = 100;
Integer i3 = 10;
Integer i4 = 10;

if(i1!=i2) {
System.out.println("different objects");
}

if(i3==i4) {
System.out.println("Same object");
}

The == is unwrapped and != is not unwrapped. I found this while studying for my SCJP exam. I think when using objects I'm only going to use equals unless I interested in the object reference.

Keep up the good work with jsuneido.
I downloaded to the code the other day could not get it to compile I guess you were still working on views and triggers. I can't wait to have a play.

Have you ever heard of the Java Posse podcast? It would be brilliant to hear an interview with you and the Posse talking about the upcoming jsuneido?