4

I have a class where I have overridden both the hashCode method as well as the equals method. The equals method behaves as I would expect it to, however the hashCode method does not seem to behave as I would expect. I'm assuming therefor my expectation is incorrect, but not sure why. Below are the overridden methods:

public class Car extends SomeBaseClass implements Cloneable, Serializable {
private static final long serialVersionUID = 1L;
private String id;
private String name;
private String carName;
private String carModel;
private String displayTextCar;


public boolean equals(Car car)
{
    return (getCarName().equals(car.getCarName())  && getCarModel().equals(car.getCarModel()));
}

public int hashCode()
{
    return (this.getCarName() + this.getCarModel()).hashCode();

}

Now I have a test class where create two car objects, and call the equals method and then put each instance of car into a HashMap. I set each instance to have the same car Name and Model and calling equals method in fact returns true. However even though each instance returns the same hashCode, when I add them to the HashMap, it keeps two objects int the Map, whereas I would expect the second put to replace the first object in the map ??? Below is the guts of the test class:

HashMap<Car,String> testMap;

Car testCar1 = new Car();
testCar1.setCarName("DaveCar");
testCar1.setCarModel("DaveModelTest");
System.out.println("Car Hash 1: " + testCar1.hashCode());

Car testCar2 = new Car();
testCar2.setCarName("DaveCar");
testCar2.setCarModel("DaveModelTest");
System.out.println("Car Hash 2: " + testCar2.hashCode());

//hashCodes prints identical numbers

System.out.println("Car 1 equal Car 2 ?? " + testCar1.equals(testCar2));
//returns true

testMap.put(testCar1, "3");     
testMap.put(testCar2, "16");

System.out.println("Map size is " + testMap.size());
//I would expect the size to be 1 here, but it's in fact 2.

So this doesn't seem correct to me, I have naturally left some of the code out here, but this is the basic principal. Hoping someone can point out where I have gone wrong here. Note that I did use Eclipse to generate hashCode and equals methods and that worked correctly, however it's bugging me that my implementation of hashCode did not work as I expected, even though both objects seemingly returned the same value for the hashCode. Appreciate anyone's input.

David M
  • 43
  • 2

2 Answers2

15

The problem is that you have provided a wrong equals: it should be equals(Object), not equals(Car).

Essentially, you have provided an overload instead of an override, so HashMap keeps calling the equals from the base class.

Fixing this problem is simple: add an override that does the cast, and calls the equals method that you wrote, like this:

@Override
public boolean equals(Object other) {
    return (other instanceof Car) && equals((Car)other);
}

Note the use of @Override annotation. It helps Java help you spot issues like this automatically.

Note: with this problem out of the way, consider implementing your hashCode method in a more "frugal" way. Rather than creating a throw-away (this.getCarName() + this.getCarModel()) string simply for the purpose of obtaining its hash code, consider rewriting the method as follows:

public int hashCode() {
    return 31*getCarName().hashCode() + getCarModel().hashCode();
}

or in Java 1.7+ you could write

public int hashCode() { // Thanks, fge, for a nice improvement!
    return Objects.hash(carName, carModel);
}
Boann
  • 44,932
  • 13
  • 106
  • 138
Sergey Kalinichenko
  • 675,664
  • 71
  • 998
  • 1,399
  • 1
    No need to test for `other == null` if you use `instanceof` -- `null` is `instanceof` nothing – fge Nov 10 '14 at 18:19
  • @fge You are right, the implementation can be shortened considerably. Thanks! – Sergey Kalinichenko Nov 10 '14 at 18:20
  • Well, if we are to shorten the implementation, the hash code implementation can be rewritten as `Objects.hashCode(carName, carModel)` ;) This will also take care of nulls (OK, only viable since 1.7) – fge Nov 10 '14 at 18:26
  • @fge You are right again, thanks! I've been out of Java development for too long, so I did not notice a very welcome addition of the `Objects` class. Thanks! – Sergey Kalinichenko Nov 10 '14 at 18:37
  • Yes correct I was focused on the hashCode method, and didn't realize the equals was wrong. Definitely using the override annotation would have been helpful. – David M Nov 10 '14 at 18:43
  • 1
    If `carName` and `carModel` were **final**, you could calculate the hashCode just once and store it in a local variable for the ultimate in speed. And they probably **should be final**: in general, the fields you use to generate a hashCode should not be mutable. – user949300 Nov 12 '14 at 00:42
  • @fge The point of changing the hashCode method was to prevent allocating the throwaway string on every call. So it is worth mentioning that the Objects method you refer to, although it is convenient to use, has the unfortunate effect of allocating a throwaway array on every call. – Boann Nov 12 '14 at 10:09
6

The problem is not with .hashCode(); the problem is that you don't override .equals()!

Look at your prototype:

public boolean equals(Car car)

Now have a look at the documentation for Object...

What you should override is:

public boolean equals(Object obj)

Hence the error. You did implement hashCode correctly, however you use Object's .equals() implementation.

fge
  • 110,072
  • 26
  • 223
  • 312