35

Let's suppose I have class User:

public class User {
  private Long id;
  private String name;
  private Integer age;
  private BigDecimal account;
  // other fields, getters and setters
}

Is it proper to override the equals method as follows?

@Override
public boolean equals(Object ob) {
   if (ob == null) {
       return false;
   }
   if (this == ob) {
       return true;
   }
   if (ob instanceof User) {
       User other = (User) ob;
       return this.id.equals(other.getId());
   }
   return false;
}

It turns out that the uniqueness of an object is determined only by its ID. But in my application, id is always unique. It's provided at the database. Is my equals implementation competent enough to account for this? Or is this not best practice?

Of course I understand that in this case the hashCode implementation should be the following:

@Override
public int hashCode() {
   return id.intValue();
}
Paul Bellora
  • 54,340
  • 18
  • 130
  • 181
Michael
  • 1,152
  • 6
  • 19
  • 36
  • 2
    What do you need `.equals()` for, if anything? – fge Jun 08 '13 at 20:19
  • It looks like you are good. If the object is equal when the id's are equal, you have the correct code. – greedybuddha Jun 08 '13 at 20:19
  • 3
    Before a `User` is persisted, `id` can be null, and then `equals` will throw. But that's just a wart. I've seen your pattern often. One SO question led to this bit of Hibernate documentation, which may not matter for you: http://docs.jboss.org/hibernate/core/4.0/manual/en-US/html/persistent-classes.html#persistent-classes-equalshashcode – Eric Jablow Jun 08 '13 at 20:23
  • id -> (name , age ,account) .. u are relying in data base constraints..you should make id field FINAL (if u are not using hibernate) – nachokk Jun 08 '13 at 20:38
  • Note that if `id` is null then `equals` and `hashCode` will throw `NullPointerException` – Steve Kuo Jun 08 '13 at 22:01
  • 1
    Since `id` is a `Long` your `hashCode` method can be `return id.hashCode()` – Steve Kuo Jun 08 '13 at 22:02

5 Answers5

9

Whether you should do this depends on the semantics of your class. That is, what does it mean to say that two objects of your class are equivalent?

The most important distinction is between objects with value semantics and objects with entity semantics. Entity objects are not equivalent even if they have equivalent attributes (colour, length, etc.). In many cases, including when the object has been read from a database table that has a primary key, entity objects will have an ID field that is unique. Comparing only the ID field in that case is the right thing to do.

Raedwald
  • 46,613
  • 43
  • 151
  • 237
  • If a `User` object represents the characteristics of some user in a database, should it be possible for more than one such object to exist simultaneously for any given ID? It would seem that for consistency it would be better to have all such objects be constructed by a factory method which uses some sort of weak-referenced collection to ensure that as long as any reference exists to a `User` with some ID, any requests to get a `User` with that ID will return the same object. If that is done, there will be no need for `User` to override `equals`. – supercat Nov 11 '13 at 22:28
2

It's ok to use id for equals method as long as you don't have to compare an entity that wasn't persisted yet to the DB. If you want to compare entities that weren't saved yet, you'll have to compare their attributes.

Amir Kost
  • 2,148
  • 1
  • 16
  • 30
1

It's fine. As long as no two different users can have the same ID your equals function is sufficient. There could be a problem if one user can be represented twice with a different ID (for whatever reason) and you do want to consider them equal.

Kninnug
  • 7,992
  • 1
  • 30
  • 42
1

your equals() method dosen't look like it was generated by IDE as it doesn't check for 'id' null value as stated by @Eric..

this is what my equals()/hashCode() method looked like using same id prop

@Override
public int hashCode() {
    final int prime = 31;
    int result = 1;
    result = prime * result + ((id == null) ? 0 : id.hashCode());
    return result;
}

@Override
public boolean equals(Object obj) {
    if (this == obj)
        return true;
    if (obj == null)
        return false;
    if (getClass() != obj.getClass())
        return false;
    User11 other = (User11) obj;
    if (id == null) {
        if (other.id != null)
            return false;
    } else if (!id.equals(other.id))
        return false;
    return true;
}

we should always use automatic generation for boilerplate code where possible, as it is a lot less error prone.

also, regarding your point regarding uniqueness of 'id' prop, that depends on your preference and how you want to use you equals method (business requirement) i.e. if two User's name are same are they to be considered same when comparing two User Objects later on..

0

I agree it is on the id. But I had trouble when getting data that should update the database. With this example with User where equals only looked on ID I created this.

interface DataEquals<T extends DataEquals> {
   public boolean isDataEquals(T other)
}


User implements DataEquals<User> {
   public boolean isDataEquals(User other) {
      boolean b1 = getName().equals(other.getName());
      boolean b2 = getAge().equals(other.getAge());
      boolean b3 = getAccount().equals(other.getAccount());
      return b1 && b2 && b3;
   }
}

With this we can have this.

public class ListChanges<T extends DataEquals<T>> {

  private List<T> added = new ArrayList<T>();
  private List<T> removed = new ArrayList<T>();
  private List<T> changed = new ArrayList<T>();
  private List<T> unchanged = new ArrayList<T>();

  public ListChanges() {
    super();
  }
  public List<T> getAdded() {
    return added;
  }
  public List<T> getChanged() {
    return changed;
  }
  public List<T> getRemoved() {
    return removed;
  }
  public List<T> getUnchanged() {
    return unchanged;
  }

  public boolean hasAnyChanges() {
    return added.size()>0 || removed.size()>0 || changed.size()>0;
  }

  public void parse(List<T> oldList,List<T> newList) {
    for (T oldObj : oldList) {
        int index =newList.indexOf(oldObj);
        if (index==-1) {
            removed.add(oldObj);
        } else {
            T newObj = newList.get(index);

            if (newObj.isDataEquals(oldObj)) {
                unchanged.add(oldObj);
            } else {
                changed.add(newObj);
            }
        }
    }
    for (T newObj : newList) {
        if (oldList.indexOf(newObj)==-1) {
            added.add(newObj);
        }
    }
 }
}

Then we can do this

List<User> oldList = ....;
List<User> newList = ...;
ListChanges<User> listChanges = new ListChanges<User>();
listChanges.parseChanges(oldList,newList);

Do you agree this is a way to do this. ??????

FrederikH
  • 139
  • 2
  • 7