CODEWITHSUNDEEP
javaobjectequals
Snowman
Snowman

Reputation: 32081

Equals method for objects

I'm trying to write an equals method for objects that compares their fields and return true if they're equal.

private int x, y, direction;
private Color color;

public boolean equals(Ghost other){
   if (this.x == other.x && this.y == other.y &&
       this.direction == other.direction && this.color == other.color)
      return true;
   else 
      return false;
}

What could be wrong with this?

Upvotes: 3

Views: 2281

Answers (4)

BoltClock
BoltClock

Reputation: 724552

Since color appears to be a Color, that's a class, and therefore a reference type, which means you need to use equals() to compare the colors.

if (/* ... && */ this.color.equals(other.color)) {

As noted in the comments, using == to compare reference types is really comparing memory addresses in Java. It'll only return true if they both refer to the same object in memory.

akf points out that you need to use the base Object class for your parameter, otherwise you're not overriding Object.equals(), but actually overloading it, i.e. providing a different way of calling the same-named method. If you happen to pass an object of a totally different class by accident, unexpected behavior might occur (although then again if they are of different classes it will return false correctly anyway).

@Override
public boolean equals(Object obj) {
    if (!(obj instanceof Ghost))
        return false;

    // Cast Object to Ghost so the comparison below will work
    Ghost other = (Ghost) obj;

    return this.x == other.x
        && this.y == other.y
        && this.direction == other.direction
        && this.color.equals(other.color);
}

Upvotes: 7

akf
akf

Reputation: 39515

One thing to consider is that you are not overriding the equals method from Object, as you are changing the param type. You might find this method will not be used in all cases as you might expect. Instead of: 

public boolean equals(Ghost other){

you should have: 

public boolean equals(Object other){

and then internally test whether the other param is an instanceof Ghost and cast as necessry.

Upvotes: 3

robert_x44
robert_x44

Reputation: 9324

If you are comparing object variables instead of primitive types, you should be using a this.color.equals(other.color) comparison instead.

In your case, it also depends on how you created the Color objects. if you used the static instances (such as Color.BLUE), then actually, it shouldn't matter. If you created the Color object from rgb values, it definitely matters. Either way, it is best to get used to using .equals() for object variables.

Upvotes: 3

sleske
sleske

Reputation: 83645

In principle, this looks fine.

Note however that you are comparing using ==. For primitives, this is no problem, but for objects it will check for the same instance, not the same value. This may or may not be what you want. If you are comparing e.g. java.lang.Strings, you'd want to use equals instead (and check for null).

Upvotes: 4

Related Questions