What is the correct way of overriding hashCode () and equals () methods of persistent entity?

Question

I have a simple class Role:

@Entity
@Table (name = "ROLE")
public class Role implements Serializable {

    @Id
    @GeneratedValue
    private Integer id;
    @Column
    private String roleName;

    public Role () { }

    public Role (String roleName) {
        this.roleName = roleName;
    }

    public void setId (Integer id) {
        this.id = id;
    }

    public Integer getId () {
        return id;
    }

    public void setRoleName (String roleName) {
        this.roleName = roleName;
    }

    public String getRoleName () {
        return roleName;
    }
}

Now I want to override its methods equals and hashCode. My first suggestion is:

public boolean equals (Object obj) {
    if (obj instanceof Role) {
        return ((Role)obj).getRoleName ().equals (roleName);
    }
    return false;
}

public int hashCode () {
    return id; 
}

But when I create new Role object, its id is null. That's why I have some problem with hashCode method implementation. Now I can simply return roleName.hashCode () but what if roleName is not necessary field? I'm almost sure that it's not so difficult to make up more complicated example which can't be solved by returning hashCode of one of its fields.

So I'd like to see some links to related discussions or to hear your experience of solving this problem. Thanks!

Answer 1

Bauer and King's book Java Persistence with Hibernate advises against using the key field for equals and hashCode. They advise you should pick out what would be the object's business key fields (if there was no artificial key) and use those to test equality. So in this case if role name was not a necessary field you would find the fields that were necessary and use them in combination. In the case of the code you post where rolename is all you have besides the id, rolename would be what I'd go with.

Here's a quote from page 398:

We argue that essentially every entity class should have some business key, even if it includes all properties of the class (this would be appropriate for some immutable classes). The business key is what the user things of as uniquely identifying a particular record, whereas the surrogate key is what the application and database use.

Business key equality means that the equals() method compares only the properties that form the business key. This is a perfect solution that avoids all the problems presented earlier. The only downside is that it requires extra thought to identify the correct business key in the first place. This effort is required anyway; it's important to identify any unique keys if your database must ensure data integrity via constraint checking.

An easy way I use to construct an equals and hashcode method is to create a toString method that returns the values of the 'business key' fields, then use that in the equals() and hashCode() methods. CLARIFICATION: This is a lazy approach for when I am not concerned about performance (for instance, in rinky-dink internal webapps), if performance is expected to be an issue then write the methods yourself or use your IDE's code generation facilities.

Answer 2

I'm sorry to jump in late with criticism, but nobody else has mentioned it and there is a serious flaw here. Possibly two, actually.

First, others have mentioned how to handle the possibility of null, but one critical element of a good hashcode() and equals() method pair is that they must obey the contract, and your code above does not do this.

The contract is that objects for which equals() returns true must return equal hashcode values, but in your class above, the fields id and roleName are independent.

This is fatally flawed practice: you could easily have two objects with the same roleName value, but different id values.

The practice is to use the same fields to generate the hashcode value as are used by the equals() method, and in the same order. Below is my replacement for your hashcode method:

public int hashCode () {
    return ((roleName==null) ? 0 : roleName.hashcode()); 
}

Note: I don't know what you intended by the use of the id field as hashcode, or what you meant to do with the id field. I see from the annotation that it's generated, but it's externally generated, so the class as written fails to fulfill the contract.

If for some reason you find yourself in a situation where this class is exclusively managed by another which faithfully generates "id" values for roleNames which do fulfill the contract, you wouldn't have a functionality problem, but it would still be bad practice, or at least have what people refer to as a "code smell". Besides the fact that there's nothing in the class definition to guarantee that the class is only usable in that way, hashcodes aren't ids, so ids aren't hashcodes.

That doesn't mean you couldn't use a guaranteed-equal-for-equal-rolename-values identifier as the hashcode, but they're not conceptually the same, so at the very least, you should have a block of comment to explain your departure from expected practice.

And as a good general rule, if you find yourself having to do that, you've probably made a design error. Not always, but probably. One reason for that? People don't always read comments, so even if you create a perfectly functioning system, over time, someone will "misuse" your class and cause problems.

Having the class itself manage the generation of hashcode values avoids that. And you could still save and make available the externally generated id, for whatever purpose you use it.