Method equal to () priority
This is a question about how to implement the equals method when I need to find an instance of an object in a list, given the value that one of the instances of my has in its member.
I have an object where I have implemented equals:
class User {
private String id;
public User(id) {
this.id = id;
}
public boolean equals(Object obj) {
if (!(obj instanceof User)) {
return false;
}
return ((User)obj).id.equals(this.id);
}
}
Now, if I want to search for something in the List, I would do something like this:
public function userExists(String id) {
List<Users> users = getAllUsers();
return users.contains(new User(id));
}
But maybe this could be a better implementation?
class User {
private String id;
public boolean equals(Object obj) {
if (!(obj instanceof User)) {
return false;
}
if (obj instanceof String) {
return ((String)obj).equals(this.id);
}
return ((User)obj).id.equals(this.id);
}
}
Instead of this:
public function userExists(String id) {
List<Users> users = getAllUsers();
return users.contains(id);
}
source to share
Don't override equals for things that aren't mathematically equal.
You might think this is a good idea.
User bob = new User("Bob");
if (bob.equals("Bob")) {
...
}
but that rarely happens. Do you want all equivalent watchdog codes to get confused when they Strings
are equal " Users
?
If you need a search method please write it
class User {
private String id;
public boolean equals(Object obj) {
if (obj instanceof User) {
User other = (User)obj;
if (id.equals(other.id)) {
return true;
}
}
return false;
}
public String getId() {
return id;
}
}
Then the code elsewhere maintains the quick lookup table.
Map<String, User> idTable = new HashMap<String, User>();
User bob = new User("Bob");
idTable.put(bob.getId(), bob);
public User findUser(String id) {
return idTable.get(id);
}
Note that this doesn't mess with the equals implementation, so you can now safely have Sets
from Users
, Lists
of Users
, etc. all without worrying that the String will somehow pollute the works.
Now if you can't find a good place to maintain Map
of Users
indexed them id
you can always use the slower solutionIterator
List<User> users = new List<User>();
users.add(new User("Bob"));
users.add(new User("Steve"));
users.ass(new User("Ann"));
public User findUser(String id) {
Iterator<User> index = users.iterator();
while (index.hasNext()) {
User user = index.next();
if (id.equals(user.getId())) {
return user;
}
}
return null;
}
source to share
Doing this second way is dangerous because it violates the symmetric property of equality.
Java expects implementations to equals()
be reflexive, symmetric, and transitive. The second implementation breaks symmetry: if you compare User
with String
representing its id, you get true
, but if you compare a string with the user, you get false
.
source to share
First, your second implementation is functionally equivalent to the first, because an instance is String
not an instance User
, and the first statement return
will close it before checking for String
happens.
What I mean,
public boolean equals(Object obj) {
if (!(obj instanceof User)) { // This will execute if obj is a String
return false;
}
if (obj instanceof String) {
// Never executes, because if obj is a String, we already
// returned false above
return ((String)obj).equals(this.id);
}
return ((User)obj).id.equals(this.id);
}
So, for the remainder of the answer, I'll assume what was meant by
public boolean equals(Object obj) {
if ( obj == null ) return false; // Add a null check for good measure.
if (!(obj instanceof User)) {
if (obj instanceof String) {
// Now we're checking for a String only if it isn't a User.
return ((String)obj).equals(this.id);
}
return false;
}
return ((User)obj).id.equals(this.id);
}
We now get to the real problem.
An implementation equals
that returns -to- true
for comparison is bad practice because it is expected to be symmetric ( if and only if ). And since it can never equal a , a should never equal .User
String
equals
a.equals(b)
b.equals(a)
String
User
User
String
source to share
The second choice is very bad. This means that you are allowing the user to pass something other than User
in User.equals()
(ie, indeed Object.equals()
, when you are not actually trying to compare the String to the user. Thus, you are essentially breaking the contract of what you equals
should do.
Also, none of your answers handle checking for nulls.
source to share