0

I'm currently revising for an exam. On a past paper there was the question,

Override the equals method in the following class. The method shall check for content equality of the whole state.

 class Employee
    {
        String firstName;
        int age;
    } 

[2 marks]

I did some fiddling for the right answer and have come up so far with this. Is there a simpler way to answer the question and is this right? Many thanks for help.

 public class Employee
    {

     int age;

    public boolean equals(Object obj)
    {
        if(this == obj)
            {
                return true; //Reference equality.
            }    
        if(!(obj instanceof Employee)) 
            {
                return false; // not the same type.
            }
        Employee other = (Employee) obj;
        return firstName == other.firstName;
        return age == other.age;
        }
    }
6
  • 2
    You still don't check for null. And Strings should be compared with equals, not ==. Commented May 13, 2011 at 13:01
  • 2
    @Etienne de Martel: null is NOT an instanceof Employee. Commented May 13, 2011 at 13:04
  • @bob - See Overriding equals and hashCode in Java
    – McDowell
    Commented May 13, 2011 at 13:05
  • 1
    @Tom Tresansky Interesting, I didn't know that. Commented May 13, 2011 at 13:06
  • 4
    If you are using Eclipse look in the menu Source->Generate hashCode() and equals(). Eclipse will then auto generate some very robust code you can study. Commented May 13, 2011 at 13:06

7 Answers 7

1

Use

return (((this.firstName == null || other.firstName == null)
            && this.firstName == other.firstName)
       ||
       this.firstName.equals(other.firstName))
       && age == other.age;

This handles null cases too.

8
  • He can get it fixed, no need to give the complete code here in my humble opinion. Commented May 13, 2011 at 13:13
  • I am saying that the guides you've given still leaves other bugs in his equals implementation, so it should not be considered a complete answer to his question.
    – Håvard S
    Commented May 13, 2011 at 13:14
  • Ye. I'm still quite confused guys
    – bob
    Commented May 13, 2011 at 13:17
  • Try to read the code. If one of names are null, we check both are null then return true if ages are equal, if none null, we compare with String's equals and then return if they are equal also with ages. Replace your return stmt with this. Commented May 13, 2011 at 13:22
  • This will throw a NullPointerException if this.firstName is null and other.firstName is not null. Commented May 13, 2011 at 13:37
1

One thing to note, and I wouldn't imagine you would get docked for this in an exam...

It's usually poor practice to do an instanceof when the class isn't final. The reason for that is that equals() must be symmetric. Accepting subclasses (who might also implement equals with their own new aspects) could cause it to not be symmetric.

Example (I think the example is the same used in Effective Java 2ed):

class Point {
    protected int x, y;
    //equals method uses instanceof Point and checks x and y values are the same
}

class ColorPoint extends Point {
    protected Color color;
    //equals method checks that it's a ColorPoint, that super.equals is true, 
    //then checks the Color
}

new Point(1, 2).equals(new ColorPoint(1, 2, Color.red)); //true
new ColorPoint(1, 2, Color.red).equals(new Point(1, 2)); //false

It's a very subtle point that even most of the answerers here didn't take into account. But it's the reason that most generators of equals (such as the one in your favourite IDE) tend to do exact class comparison:

  if ( this.getClass() != other.getClass() ) {
     return false;
  }

When the equals method uses instanceof it's usually a good move to document that subclasses must follow the exact same specification.

6
  • 2
    @Mark I think there is no way to "guarantee" symmetry when inheritance is in place. By using getClass() you ensure only objects of the same class could be equal, but somebody might like to extend the root class without adding funtionality subject to equals or hashcode. With this code, comparisons will yield false unless you override the method again. So, this does not solves the symmetry problem and brings another issue to the table. It is an alternative, but not free of nuances, just like the other. Whatever the strategy, unit testing should guarantee proper behavior. Commented May 13, 2011 at 13:43
  • @edalorzo: The shortcomings you mention do not affect symmetry, though they may make the class harder to extend. Simply using instanceof and hoping creates a pseudo-incompliant implementation in itself. An alternative way to guarantee symmetry is to use instanceof but to make the equals method final. That will accept subclasses but will ensure they can't override equals. Commented May 13, 2011 at 13:48
  • The real problem with the suggestion in my answer is that it can be construed to break substitutability. IMO making equals final is a better alternative. Commented May 13, 2011 at 13:57
  • @Mark Imagine a Coordinate abstract root class, it has two points, and it implements equals and hashCode based on them. It has an abstract method calculateDistance(). There are two implementations, one calculates the straight distance, the other takes into account the curvature of the earth. None of the implementations changes the state. I want to compare for equality instances of any of the implementations through the inherited equals method. From business logic standpoint, these should be symmetrically comparable, using getClass() you would be forced to override the equals method. Commented May 13, 2011 at 15:20
  • All I want to point out is that once we introduce inheritance, the implementation of equals is subject to certain difficulties that can lead to improper implementations of the method contract. And whatever the strategy, those nuances should be taken into account. There is simply no perfect solution for this. Commented May 13, 2011 at 15:23
1

A couple of points:

  1. You need to check if obj is null.
  2. To compare String contents in Java, use equals(), i.e. firstName.equals(other.firstName). Check to see if firstName is null first.

Here's an improved implementation:

public boolean equals(Object obj)
{
  if(obj == null)
  {
    return false;
  }
  if(this == obj)
  {
    return true; //Reference equality.
  }    
  if(this.getClass() != obj.getClass())
  {
    return false;
  }
  Employee other = (Employee) obj;
  if(firstName == null)
  {
    if(other.firstName != null)
    {
      return false;
    }
  }
  else if(!firstName.equals(other.firstName))
  {
    return false;
  }
  return age == other.age;
}

EDIT: Updated type comparison to make equals() symmetric in accordance with @Mark Peters' answer.

5
  • this method will fail if both object has null firstName. Commented May 13, 2011 at 13:27
  • No, it checks if firstName is null before doing equals(). The || operator in Java short circuits and evaluates left to right, so the code is fine.
    – Håvard S
    Commented May 13, 2011 at 13:29
  • if firstName is null, than it will not evaluate the second condition and straitforward returns false. It will not check whether other.firstName is null or not. You can try this test case.. :) Commented May 13, 2011 at 13:34
  • Didn't see that one, no. Good catch.
    – Håvard S
    Commented May 13, 2011 at 13:39
  • I fixed it too, but forgot mentioning it, by the way. :)
    – Håvard S
    Commented May 13, 2011 at 13:51
1

The String firstName should be compared with .equals(), NOT ==. The == compare is ok for the primitive int age field.

What if both firstNames are identical, yet age is unequal? Shouldn't this fail?

Something like return (firstName.equals(obj.firstName)) && (age == obj.age);

Of course, that doesn't consider the case when this.firstName is null, which would result in a NullPointerException being thrown.

Are the Employees considered equal if both have null firstNames? What if one is null and the other not? Assuming both must be null, or both must be String.equals(), you could use:

return ((null == firstName && null == obj.firstName) 
  || (null != firstName && firstName.equals(obj.firstName)))
  && (age == obj.age);

instead of your 2 return statements. The rest looks ok.

0
public boolean equals(Object o){
   if(this==o){ //same instance, no need to check more
       return true;
   }
   if(o instanceof Employee){ //when null this will yield false
       Employee other = (Employee) o;
       return (this.name == other.name || (this.name != null && this.name.equals(other.name)) && this.age == other.age;
   }
   return false;
}
0

To put together in one answer all the parts already mentioned:

public boolean equals(Object obj) {
    if(this == obj) {
        return true; //Reference equality.
    }
    if(obj == null || !(obj instanceof Employee))
    {
        return false; // not the same type.
    }
Employee other = (Employee) obj;
return (firstName.equals(other.firstName) && age == other.age);
}
0

Your last line, the age comparison, is unreachable; you shouldn't use == to compare Strings; and you need to account for null values.

Since the general movement seems to be towards laying it all out for you, here's Eclipse's implementation:

public class Employee {
    private final String firstName;
    private final int age;

    public Employee(final String firstName, final int age) {
        super();
        this.firstName = firstName;
        this.age = age;
    }

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

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

And a battery of tests:

import org.junit.Test;

public class EmployeeTest {
    @Test
    public void testEmployeeEquals() {
        final Employee nullNameEmp = new Employee(null, -1);
        final Employee empA1 = new Employee("a", 1);
        final Employee empA1Clone = new Employee("a", 1);
        final Employee empA2 = new Employee("a", 2);
        final Employee empB1 = new Employee("b", 1);
        final Employee empB2 = new Employee("b", 2);
        final Employee subEmp = new Employee("a", 1) {

        };

        assert !nullNameEmp.equals(empA1);
        assert !nullNameEmp.equals(empA1Clone);
        assert !nullNameEmp.equals(empA2);
        assert !nullNameEmp.equals(empB1);
        assert !nullNameEmp.equals(empB2);
        assert !nullNameEmp.equals(subEmp);
        assert !nullNameEmp.equals(null);

        assert !empA1.equals(nullNameEmp);
        assert empA1.equals(empA1Clone);
        assert !empA1.equals(empA2);
        assert !empA1.equals(empB1);
        assert !empA1.equals(empB2);
        assert !empA1.equals(subEmp);
        assert !empA1.equals(null);

        assert !empA2.equals(nullNameEmp);
        assert !empA2.equals(empA1);
        assert !nullNameEmp.equals(empA1Clone);
        assert !empA2.equals(empB1);
        assert !empA2.equals(empB2);
        assert !empA2.equals(subEmp);
        assert !empA2.equals(null);

        assert !empB1.equals(nullNameEmp);
        assert !empB1.equals(empA1);
        assert !empB1.equals(empA1Clone);
        assert !empB1.equals(empA2);
        assert !empB1.equals(empB2);
        assert !empB1.equals(subEmp);
        assert !empB1.equals(null);

        assert !empB2.equals(nullNameEmp);
        assert !empB2.equals(empA1);
        assert !empB2.equals(empA1Clone);
        assert !empB2.equals(empA2);
        assert !empB2.equals(empB1);
        assert !empB2.equals(subEmp);
        assert !empB2.equals(null);

        assert !subEmp.equals(nullNameEmp);
        assert !subEmp.equals(empA1);
        assert !subEmp.equals(empA1Clone);
        assert !subEmp.equals(empA2);
        assert !subEmp.equals(empB1);
        assert !subEmp.equals(empB2);
        assert !subEmp.equals(null);

        assert nullNameEmp.equals(nullNameEmp);
        assert empA1.equals(empA1);
        assert empA1Clone.equals(empA1Clone);
        assert empA2.equals(empA2);
        assert empB1.equals(empB1);
        assert empB2.equals(empB2);
        assert subEmp.equals(subEmp);
    }
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.