0
public class Car
{
    private String name;  
    public int id;     


    public Car(String name, int id) 
    {
        this.name = name;
        this.id = id;
    }

@Override
public boolean equals(Object ob) 
{
    if (!(ob instanceof Car))
    {    
      return false;
    }
 Car that = (Car)ob;
 return this.id == that.id;
}

@Override
public int hashCode() 
{
    return id;
}
// this class also got getters and setters 

Then I got another class

public class CarList
{
        private Collection<Car> cars;

    public CarList()
    {
        cars = new HashSet<>();
    }


   public boolean insertCar(Car car)
    {
        return cars.add(car); 
    }

My question is: How to properly override equals() and hashCode() method, where I consider 'id' and 'name' attribute for object comparsion and hashCode calculation ( so there is no possibility to have 2 objects with the same name and ID - because in this code as it is - it only takes 'id' attribute for object comparsion)?

Amiqo Amiqo
  • 31
  • 1
  • 5
  • If you are using a powerful IDE like IntelliJ, you can likely automatically generate very good versions of hashCode() and equals() with a keypress (e.g. Alt-insert...) – vikingsteve Oct 28 '13 at 19:05
  • First of all, you rightfully declared `equals` as `boolean` but fail to implement it this way. You may want to review answers to [Overriding equals and hashCode in Java](http://stackoverflow.com/questions/27581/overriding-equals-and-hashcode-in-java/40669#40669) post. – PM 77-1 Oct 28 '13 at 19:06

2 Answers2

3

As of Java 7, there are static methods on Objects that makes implementing hashCode and equals easier. This should work well, assuming you don't want to use getClass() instead of instanceof to determine type compatibility. That depends on how subclasses of Car should compare to Cars.

@Override
public boolean equals(Object ob) 
{
    if (!(ob instanceof Car))
    {    
      return false;
    }
 Car that = (Car)ob;
 return Objects.equals(this.id, that.id) && Objects.equals(this.name, that.name);
}

@Override
public int hashCode() 
{
    return Objects.hash(id, name);
}
Tim S.
  • 55,448
  • 7
  • 96
  • 122
  • FYI, Java best practices would be to not invent the wheel & just use something that provides this already, like `java.util.Objects.hashCode(Object ...)` (available in SE 7) or [Guava's](https://code.google.com/p/guava-libraries/) `Objects.hash(Object ...)`. – TC1 Oct 28 '13 at 19:06
  • why `instanceof`? this call is just awfully slow. – Zhedar Oct 28 '13 at 19:07
  • 1
    @Zhedar To avoid `ClassCastException`. The way shown is somewhat idiomatic if you implement it yourself. – TC1 Oct 28 '13 at 19:15
  • 1
    Since you edited the `hash`, could've checked the docs. The way to implement `equals()` has also changed. There's an [Objects.equals](http://docs.oracle.com/javase/7/docs/api/java/util/Objects.html) in both SE7 and Guava, and the way to implement `equals()` now is `Objects.equals(field1, other.field1) && Objects.equals(...)`. This goes after the `instanceof` of course, but you don't have to check for `null` etc this way. – TC1 Oct 28 '13 at 19:19
  • @TC1 I've added that now. – Tim S. Oct 28 '13 at 19:26
3

Instead of using

if (!(ob instanceof Car))
{    
  return false;
}

You should think about using

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

Lets assume you have ForWdCar extends Car and TwoWdCar extends Car with equal name and id.

Do you want them to be equal? 1st solution,

Do you want them to be unequal? 2nd solution

You don't care, such cases don't happen? Second solution, it's faster.

Daniel
  • 36,610
  • 3
  • 36
  • 69
  • I'm going to, very grudgingly, downvote. (actally, I just undid the downvote, but it was close) It is not true that you "should use" the getClass() == obj.getClass(). It is true that, _in some cases_, you should use that, and in others instanceOf is appropriate. I depends on how you want your subclasses to behave with equals(). – user949300 Oct 28 '13 at 19:17
  • @user949300: You are right. I did not really thought about because I never saw a case when `(!(ob instanceof Car))` really matches the intended behaviour. I'll update my answer. – Daniel Oct 28 '13 at 19:25
  • Good update. I'll give you an upvote! This is an issue where people get into religious wars... – user949300 Oct 28 '13 at 19:33