5
\$\begingroup\$

This is the domain class called WordType which has Id as its unique DB generated value and Name and Description as other values.

I was going through the Effective Java resource by Joshua Bloch and having a tough time assimilating the entire contents the first time. So I am looking at the implementation in my code by some other programmers so I learn by example and also validate the existing implementation. And the author states that for the usual classes the existing equals and hashcode works fine.

The easiest way to avoid problems is not to override the equals method, in which case each instance is equal only to itself.

I am really skeptical if the below equals and hashcode implementation will work or if we should live with the default equals and hashcode without overriding them.

Each word would be unique as in dictionary for instance, and hence the ID would also be unique. If so, should I combine ID and Word to generate an equals and hashcode method?

public class WordType implements Serializable {
    private static final long serialVersionUID = 1L;

//... other properties, getters and setters


   @Override
    public int hashCode() {
        int hash = 0;
        hash += (id != null ? id.hashCode() : 0);
        return hash;
    }

    @Override
    public boolean equals(Object object) {

        if (!(object instanceof WordType)) {
            return false;
        }
        WordType other = (WordType) object;

        if ((this.id == null && other.id != null) || (this.id != null && !this.id.equals(other.id))) {
            return false;
        }
        return true;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ There is a good checklist to check against. Looks like your implementation is fine (assuming id can't mutate over time). It is weird that id is nullable though. \$\endgroup\$ – Dan Jan 2 '14 at 23:29
5
\$\begingroup\$

The hashCode method can be simplified down to (just simply restructuring your logic and removing redundancy):

@Override
public int hashCode() {
    return id == null ? 0 : id.hashCode();
}

The equals method is a bit more complicated. Your code does appear to keep the hashCode/equals contract, so that is good, but, sometimes there are better ways than others to test for things. For example, often when you override hashCode and equals it is because these instances are used in a Set or Map. A shortcut is to often check for identity before checking for equality.... i.e. if (other == this) return true; This can (often/sometimes) save a lot of internal processing to resolve equality.

I have a template I tend to use for equals(...) and it goes like this:

@Override
public boolean equals(final Object other) {
    if (this == other) {
        // nothing declared for simplest case.
        return true;
    }
    if (!(other instanceof MyClass)) {
        // other is null or not one of us.....
        return false;
    }
    // Code appropriate for **your** class goes here
    final Myclass their = (MyClass)other;
    if (this.id == their.id) {
        // identity goes first.
        // this works for both values null too.
        return true;
    }
    if (this.id == null) {
        // we are null, they are not.
        return false;
    }
    return id.equals(their.id);
}

This method is longer than yours, but it makes the logic clear.

When you have to conform to 'bigger' contracts like the hashCode/equals contract (or the Comparable contract), it is often better to make the logic progression very obvious....

\$\endgroup\$
3
\$\begingroup\$

What exactly is the id variable? int? long? Integer? Your hashCode and equals implementations really just "pass the buck" to the id.

That being said, the code you posted will work fine; two WordType objects are equal if they both have the same id.

\$\endgroup\$
  • \$\begingroup\$ Thank you, ID is just an auto-generated field in the database. \$\endgroup\$ – oneworld Jan 2 '14 at 22:49
  • 2
    \$\begingroup\$ @oneworld If it is an autogenerated field, make sure you put the instance in a dictionary after it has been assigned an ID, or contains would break. \$\endgroup\$ – Dan Jan 2 '14 at 23:41
3
\$\begingroup\$

I'm nitpicking but instead of

if ((this.id == null && other.id != null) || (this.id != null && !this.id.equals(other.id))) {
    return false;
}
return true;

you could write

return (this.id == other.id)
    || (this.id != null && this.id.equals(other.id));

This reads easier because the intent is obvious (“same or equal”).

I prefer rolfl's verbose option though.

\$\endgroup\$
0
\$\begingroup\$

NEVER implement a hashcode on the base of attributes that may change. So following implementation is invalid by definition as the "id" is either expected to change or you have nonsensical objects:

@Override
public int hashCode() {
    return id == null ? 0 : id.hashCode(); // do not do this!!!
}

A valid implementation would be:

@Override
public int hashCode() {
    return id.hashCode();
}

The minimal implementation would be:

@Override
public int hashCode() {
    return 0;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.