Equals() should be type safe, null-aware and hashcode() should be overridden

[vsigler]

When playing around with ActiveAndroid I found weird behavior of the Model.equals() method.

• For two new blank models, equals would not consider them equal.
• Equals would throw ClassCastException when compared to anything else than another model.
• Equals against null throws NullPointerException

After looking at the equals method, it does not check type or null/notnull of the other object. Also, once a model has null id (not yet saved), it is automatically not equal to anything, which I also see as a bug. There is also a missing hashcode override, so models cannot be used in e.g. hash sets.

The proposed changes fix these problems - overridden hashcode, type safe and null-aware equals and null id does not automatically mean not equal to anything. I've also added a test for this behavior.

[markuspfeiffer]

Is it really a good idea to consider two instances without an ID as equal? This would mean both instances are considered equal until at least one has been saved at which point they stop being equal (due to different IDs).

I would usually do something like this:

@Override
public boolean equals(Object obj) {

    if (obj == null) {
        return false;
    }

    if (obj == this) {
        return true;
    }

    if (obj instanceof Model) {
        final Model other = (Model) obj;
        return ((this.mId != null && this.mId.equals(other.mId))
            && (this.mTableInfo.getTableName().equals(other.mTableInfo.getTableName()));
    }

    return false;
}

This way an instance is always considered equal to itself (regardless of the ID being set or not) while two different instances are only considered equal if they share the same ID and table.

[vsigler]

I'm looking at it the way that a model is essentially a DTO and DAO fused together. What I should note is that I am assuming that whoever extends Model also extends the equals() and hashcode() to include the added fields. So the other fields also play role in the equality evaluation and therefore two unsaved instances with different fields will not be equal.

The way I look at the ID is that it is just another (although special) field. When I create two unsaved instances and fill in the same values, I would expect consistent behavior of the equals() to consider them equal and also for them to generate the same hashcode.

Two data-wise identical instances being equal until one is saved is the point. Upon saving the id field changes, making the two instances non-equal. One already has a table row "attached", the other does not, so they're not equal.

Splitting the equals relation like this can hinder (or at least have weird effects on) the use of models in collections.

On the other hand, if we assume that the user does not extend the equals/hashcode, then you are right that this would cause ANY two unsaved models for the same table to be equal, which might be undesired. Although in my eyes it is just a nudge to the user to properly override those methods.

[markuspfeiffer]

I completely agree that implementers should (usually) override equals() and hashcode(), since they've got a better idea of what is and isn't equal. However, the default implementation should still be technically valid.

My definition of "valid" would be this: Two objects should be considered equal if, and only if, their primary key matches. In other words, two instances are equal if they represent the same row in the database (i.e. the same entity). Lacking an implementer-defined (compound) primary key a model's ID is its primary key.

Because of this, two instances that have not been saved to the database cannot be equal since they will end up in two different rows (each with its own id). On the other hand, two models with the same primary key (ID) are equal, since they represent the same row/entity.

This would also be more or less compatible with Java's default implementation of equals. Two objects are considered equal only if they're actually the same instance. Once we have an ID we can include it in the comparison, but until then we must assume that two instances do not represent the same entity.

[vsigler]

Allright, you have a valid point here. I've updated the equals and hashcode accordigly.

[leonardoxh]

Yep I like it

[vsigler]

Bump :)

Is there something preventing the merge? The change was reasonably small...

[kramer65]

@pardom
I wonder about the same thing as vsigler; is there currently anything that prevents this merge?

[lhoracek]

Bump
not implemented hashcode is real pain in the hashset :-)

[vsigler]

Bumping this dinosaur once more...