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?
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); } 
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).
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.
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.
if. ;-)this.x == other.x) before the if statement. See which one(s) fails.