CODEWITHSUNDEEP
javaeclipseequals
Dan
Dan

Reputation: 448

Which part of the equals() general contract does my equals() not satisfy

I'm fairly new to java and am just trying to get my head around understanding @Override of the equals() and hashcode() methods.
I know for the equals method to be correct it needs to be:

  1. Reflexive: a.equals(a)
  2. Symmetric: a.equals(b) then b.equals(a)
  3. Transitive: a.equals(b) && b.equals(c) Then a.equals(c)
  4. Not null: ! a.equals(null)

I am struggling to pinpoint which of the above properties I am and am not satisfying when writing my overide of the equals method.

I am aware that eclipse can generate these for me, however as I haven't yet gotten the concept fully, writing it out helps me to learn.

I have written out the what I think is the correct way to do it, but when I check with the eclipse generated version I seem to be 'missing' some aspects.

Example:

public class People {

    private Name first; //Invariants --> !Null, !=last
    private Name last;  // !Null, !=first
    private int age;    // !Null, ! <=0
    ...
}

What I wrote:

public boolean equals(Object obj){
    if (obj == null){
        return false;
    }
    if (!(obj instanceof People)){
         return false;
    }
    People other = (People) obj;
    if (this.age != other.age){
        return false;
    }
    if (! this.first.equals(other.first)){
        return false;
    }
    if (! this.last.equals(other.last)){
        return false;
    }
    return true;
}

vs eclipse generated

public boolean equals(Object obj) {
    if (this == obj)
        return true;
    if (obj == null)
        return false;
    if (getClass() != obj.getClass())
        return false;

    People other = (People) obj;
    if (first == null) {
        if (other.first != null)
            return false;
    } else if (!first.equals(other.first))
        return false; 
    if (age != other.age)
        return false;
    if (last == null) {
        if (other.last != null)
            return false;
    } else if (!last.equals(other.last))
        return false;

    return true;
}

I am missing:

I'm not sure what getClass() is and is my implmentation incorrect?

Upvotes: 17

Views: 808

Answers (3)

Paul Boddington
Paul Boddington

Reputation: 37645

You don't need to write 

if (obj == null){
        return false;
}
if (!(obj instanceof People)){
     return false;
}

because null always gives false in instanceof checks. So these lines can be simplified to just

if (!(obj instanceof People)){
     return false;
}

As for your main question of whether your method meets the requirements for an equals() method, strictly speaking the answer is no, or at least it's potentially dodgy. This is because it would be possible to extend the class as follows

public class SpecialPeople extends People {

    // code omitted

    @Override
    public boolean equals(Object object) {
        if (object == null || object.getClass() != getClass())
            return false;
        SpecialPeople other = (SpecialPeople) object;
        return other.getAge() == getAge() 
                && other.getFirst().equals(getFirst()) 
                && other.getLast().equals(getLast());
}

Now suppose a is an instance of People and b is an instance of SpecialPeople. Suppose also that a and b have the same name and age. Then

a.equals(b) == true  // instanceof check succeeds
b.equals(a) == false // getClass() check fails

Therefore equals() is not symmetric! For this reason, if you are using instanceof rather than getClass() in equals() you should probably either make the equals() method final or the class final.

Upvotes: 3

Luiggi Mendoza
Luiggi Mendoza

Reputation: 85779

First piece of code:

if (this == obj)
    return true;

This improves performance in case you compare the object reference against itself. Example: a.equals(a);.

Second piece of code:

if (getClass() != obj.getClass())
    return false;

This compares if the class of the reference being compared is the same class of this. The difference between using this approach and instanceof is that it's more restrictive when comparing against a sub class. Example:

public class Foo { }
public class Bar extends Foo { }

//...
Foo foo = new Bar();
System.out.println(foo instanceof Bar); //prints true
System.out.println(foo instanceof Foo); //prints true
Foo foo2 = new Foo();
System.out.println(foo.getClass() == foo2.getClass()); //prints false

Which one should you choose? There's no good or bad approach, it will depend on your desired design.

Third piece of code:

if (first == null) {
        if (other.first != null)
            return false;
    } else if (!first.equals(other.first))
        return false; //For each variable.

This is simply a null check for each object reference field in the class. Note that if this.first is null then doing this.first.equals(...) will throw a NullPointerException.

Upvotes: 8

Elliott Frisch
Elliott Frisch

Reputation: 201429

I don't think your implementation is incorrect, but a few notes:

if (this == obj)
      return true;

Is a performance optimization, it directly tests for reference equality and short-circuits tests where a is a.

if (getClass() != obj.getClass())
        return false;

Is similar to your instanceof call, optimizes away a null check. The other calls seem to be null-checks.

Upvotes: 4

Related Questions