Tuesday, November 07, 2006

Dangerous equals(Object) template in Eclipse

Problem statement


I found it very nice where I discovered that new Eclipse 3.x allows developers to automatically generate equals(Object) method. "This is great!" I thought that time. Unfortunately when I developed simple GUI application utilizing SWT and JFace as a front-end and Hibernate as a database access framework I found that something does not work. It took me some time to find this stupid bug. It was equals method of course. Look at this example (I added { to the originally generated code cause I hate when Java developers do not add curly braces for blocks with only one operation - it's just ugly and in my view disallowed - how could Eclipse.org release such shitty code!?):

public boolean equals(Object obj) {
if (this == obj) {
return true;
}
if (obj == null) {
return false;
}

if (getClass() != obj.getClass()) {
return false;
}

// omitted - checking selected attributes
}
The code looks fine but it consist one subtle bug (BTW. I found that authors of let's say Java bible namely Core Java use the same notation and the same equals template). It is in line:
if (getClass() != obj.getClass())

I agree that checking class objects is faster but it simply does not work when you operate not on classes you created but on their proxies. Hibernate wraps your domain (or persistent) objects into proxies generated supposedly by ASM library. In case of Hibernate you almost (is almost necessary in this sentence?) never operate on your classes but on runtime generated proxies. This way this buggy line stressed above will always return false unless you use instanceof operator.

Solution


I will repeat what Josh Bloch wrote in his great book "Effective Java" that you should not optimize your code if you are not 100% sure it will work in every situation. My solution (at least for Hibernate) is to replace this piece of code:

if (getClass() != obj.getClass()) {
return false;
}
with this one:

if (!(obj instanceof NameOfYourClass)) {
return false;
}
Of course instanceof operation is less effective that != but it is not so fragile. If you suppose that your classes can be used (and created) by dynamic proxies you should use instanceof operator instead of !=.

Conclusions


Generally equals method template presented by me as buggy is correct in many cases. Unfortunately we as developers never know how our classes will be used and that's why we should protect them as well as possible. Nowadays many frameworks grill our classes and use dynamic proxies (with added logging, security and whatever else) and we should be aware of this. My proposal is to use instanceof operator when you are almost sure that your classes will be instantiated by dynamic proxies or use ==, != operators otherwise. In such case you should clearly state in JavaDoc that you use such and such operator and, if your class will be wrapped by dynamic proxy, equals method will not work properly even if two comparing objects will be equal (not same) in terms of business logic.

2 comments:

fseidl said...

Your Solution may be buggy as well.

Have a look at Bloch's Effecive Java Chapter on Equals to see why - instanceof may not be sufficient in equals, because any equals that allows an instance of a class and its superclass to be equal may violate transitivity.

In addition to that, you should better not override equals if you use Hibernate. Any equals on fields that may change within a session, namely the id, is strictly prohibited in hibernate (see hibernate documetation on this).

Przemysław Bielicki said...

In fact both have advantages and disadvantages and current versions of Eclipse support both templates - it's up to you which OO principle you want to sacrifice (yes, Bloch suggests fine solution to avoid sacrifice but it's not always acceptable too).

About Hibernate - yes you should be careful but "you should better not override equals if you use Hibernate" is a huge exaggeration. You may and should (if you need) override equals while using Hibernate - just do not take into account DB id.