ArrayList not using the overridden equals

Question

I'm having a problem with getting an ArrayList to correctly use an overriden equals. the problem is that I'm trying to use the equals to only test for a single key field, and using ArrayList.contains() to test for the existence of an object with the correct field. Here is an example

public class TestClass  {
    private static class InnerClass{    
    private final String testKey;
    //data and such

    InnerClass(String testKey, int dataStuff) {
        this.testKey =testKey;
        //etc
    }
    @Override
    public boolean equals (Object in) {
        System.out.println("reached here");
        if(in == null) {
        return false;
        }else if( in instanceof String) {
        String inString = (String) in;
        return testKey == null ? false : testKey.equals(inString);
        }else {
        return false;
        }       
    }       
    }

    public static void main(String[] args) {    
    ArrayList<InnerClass> objectList = new ArrayList<InnerClass>();
    //add some entries
    objectList.add(new InnerClass("UNIQUE ID1", 42));
    System.out.println( objectList.contains("UNIQUE ID1")); 
    }    
}

What worries me is that not only am I getting false on the output, but I'm also not getting the "reached here" output.

Does anyone have any ideas why this override is being completely ignored? Is there some subtlety with overrides and inner classes I don't know of?

Edit: Having problems with the site so I cant seem to mark the answered. Thanks for the quick response: yes an oversight on my part that it is the String .equals thta is called, not my custom one. I guess it's old fashioned checks for now

Solution 1

If you check sources of ArrayList, you will see that it calls equals of other object. In your case it will call equals of String "UNIQUE ID1" which will check that other object is not of type String and just returns false:

public boolean contains(Object o) {
    return indexOf(o) >= 0;
}

public int indexOf(Object o) {
    ...     
    for (int i = 0; i < size; i++)
    if (o.equals(elementData[i]))
        return i;
    ...
    return -1;
}

For your case call contains with InnerClass that only contains id:

objectList.contains(new InnerClass("UNIQUE ID1"))

Don't forget to implement equals for InnerClass which compares id only.

Solution 2

According to the JavaDoc of List.contains(o), it is defined to return true

if and only if this list contains at least one element e such that (o==null ? e==null : o.equals(e)).

Note that this definition calls equals on o, which is the parameter and not the element that is in the List.

Therefore String.equals() will be called and not InnerClass.equals().

Also note that the contract for Object.equals() states that

It is symmetric: for any non-null reference values x and y, x.equals(y) should return true if and only if y.equals(x) returns true.

But you violate this constraint, since new TestClass("foo", 1).equals("foo") returns true but "foo".equals(new TestClass("foo", 1)) will always return false.

Unfortunately this means that your use case (a custom class that can be equal to another standard class) can not be implemented in a completely conforming way.

If you still want to do something like this, you'll have to read the specification (and sometimes the implementation) of all your collection classes very carefully and check for pitfalls such as this.

Solution 3

You're invoking contains with an argument that's a String and not an InnerClass:

System.out.println( objectList.contains("UNIQUE ID1"))

In my JDK:

public class ArrayList {

    public boolean contains(Object o) {
    return indexOf(o) >= 0;
    }

    public int indexOf(Object o) {
    if (o == null) {
        // omitted for brevity - aix
    } else {
        for (int i = 0; i < size; i++)
        if (o.equals(elementData[i])) // <<<<<<<<<<<<<<<<<<<<<<
            return i;
    }
    return -1;
    }
}

Note how indexOf calls o.equals(). In your case, o is a String, so your objectList.contains will be using String.equals and not InnerClass.equals.

Solution 4

Generally, you need to also override hashCode() but this is not the main problem here. You are having an asymmetric equals(..) method. The docs make it clear that it should be symmetric:

It is symmetric: for any non-null reference values x and y, x.equals(y) should return true if and only if y.equals(x) returns true.

And what you observe is an unexpected behaviour due to broken contract.

Create an utility method that iterates all items and verifies with equals(..) on the string:

public static boolean containsString(List<InnerClass> items, String str) {
    for (InnerClass item : items) {
        if (item.getTestKey().equals(str)) {
           return true;
        }
    }
    return false;
} 

You can do a similar thing with guava's Iterables.any(..) method:

final String str = "Foo";
boolean contains = Iterables.any(items, new Predicate<InnerClass>() {
   @Override
   public boolean apply(InnerClass input){ 
       return input.getTestKey().equals(str);
   }
}

Solution 5

Your equals implementation is wrong. Your in parameter should not be a String. It should be an InnerClass.

public boolean equals(Object o) {
  if (this == o) return true;
  if (!(o instanceof InnerClass) return false;
  InnerClass that = (InnerClass)o;
  // check for null keys if you need to
  return this.testKey.equals(that.testKey);
}

(Note that instanceof null returns false, so you don't need to check for null first).

You would then test for existence of an equivalent object in your list using:

objectList.contains(new InnerClass("UNIQUE ID1"));

But if you really want to check for InnerClass by String key, why not use Map<String,InnerClass> instead?

View more solutions

Comments

[Joachim Sauer]

While it's true that you should implement both, it does not help in this specific case, since ArrayList is not in any way hash-based: it doesn't care about the hashCode() implementation.

[Sergey Aslanov]

ArrayList doesn't use hashCode

[K.Barad]

Been working too long hours. Never a good sign when you start forgetting the basics, or misreading api specifications. I've found the workaround of having a constructor of new InnerClass(String testKey) to make a test object, but with the actually data as null/0s.

[K.Barad]

I do understand the call flow, but missed that it was using String.equals rather than InnerClass.equals. As ArrayList is not hash based I saw no need to include the hashCode in the snippet. Naturally I include a hashCode override, even if all it needs here is to return testKey.hashCode()

[K.Barad]

To be blunt it's been some 6 years since I last used Java and while I've got a coder's mindset, I lack experience. I'm not yet fully familiar with Maps, or Java hashing just yet since I learnt C originally. Using java now because for small application / tool development it's a lot faster and I need decent international and platform independence. I have added a constructor (and for some of my other classes which implement an abstract parent, a sister class) to make a testobject with just the test key.

[Sufian]

Thanks. That suggestion about new InnerClass() was really helpful as well. :)

[marcolopes]

In my opinion the implementation of indexOf should be the other way around: elementData[i].equals(o)...