This is a question about how to implement the equals method when I need to find instance of the object in a List given a value that one of the instances my have in their member.

I have an object where I've 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 find 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 perhaps this might 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);
    }
}

With this instead:

public function userExists(String id) {
    List<Users> users = getAllUsers(); 
    return users.contains(id);
}

• java

Do not override equals for things that are not mathematically equal.

You might think it is a good idea to do

User bob = new User("Bob");
if (bob.equals("Bob")) {
  ...
}

but it rarely is. Do you want all of the equals observing code getting confused when Strings are "equal" to Users?

If you want a lookup method, 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 to maintain the "fast 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 around with the equals implementation, so now you can safely have Sets of Users, Lists of Users, etc. all without worrying if somehow a String will foul the works.

Now if you can't find a good place to maintain a Map of Users indexed by their id, you can always use the slower Iterator solution

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;
}

Doing it the second way is dangerous, because it breaks symmetric property of the equality.

Java expects implementations of equals() to be reflexive, symmetric, and transitive. Second implementation breaks symmetry: if you compare User to a String representing it's ID you'd get true, but if you compare the string to the user, you will get a false.

First, your second implementation is functionally equivalent to the first, because an instance of String is not an instance of User, and the first return statement will short-circuit it before the check for a String happens.

What I mean is,

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 will assume that what was meant is

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);
}

Now we come to the actual problem.

Implementing an equals that returns true for a User-to-String comparison is bad practice because equals is expected to be symmetric ( a.equals(b) if and only if b.equals(a) ). And since a String can never equal a User, a User should never equal a String.

Don't forget to override Object.hashCode() if you override Object.equals().

Equal objects must have equal hash codes and you may run into probs with Collections if you do not obey the contract.

Second choice is very bad. That means you are allowing a user to pass something other than a User into User.equals() (i.e., really Object.equals() when you are not really trying to compare a String with the User. So you are in essence violating the contract for what equals is supposed to do.

Also, neither of your answers handle null-checking.