Class equals method is confusing

Question

I'm studying object oriented programming in Java at my school and I had to do an exercise to compare Circles.

I had the Circle Class with these

private int id;

private String bgColor;

private String fgColor;

And inside it I had to use the equals method to compare two circles (by using these three attributes): a circle is equal to other circle if its radius and the bg and fgColor are the same.

public boolean equals(Object obj) {
    boolean found;
    if (obj == null) {
        found = false;
    }
    if (getClass() != obj.getClass()) {
        found = false;
    }
    final Circle other = (Circle) obj;
    if (Double.doubleToLongBits(this.radius) == Double.doubleToLongBits(other.radius)) {
        //found = false;
        
        if (Objects.equals(this.bgColor, other.bgColor)) {
            //found = false;
            
            if (Objects.equals(this.fgColor, other.fgColor)) {
                return true;
            }//end if fgColor
            else{
                found = false;
            }   
        }//end if bgcolor
        else{
            found = false;
        }   
    }//end if radius
    else{
        found = false;
    }   

    return found;
}

But my teacher told me that the code above is "confusing", but I don't understand why.

Do you know a better solution?

My teacher wants that we folow this structure (this case is only comparing one property):

public boolean equals (Object obj)
{
      boolean b;

      if(obj == null)
      {
              b = false;
      }
      else
      {
              if(this == obj)//same object
              {
                      b = true;
              }
              else
              {
                      if(obj instanceof Book)                   
                      {
                              Book other = (Book) obj; 
                              b = (this.id == other.id); 
                      } 
                      else
                      {
                              b = false;
                      }
              }
      }

      return b;
}

Answer 1

This is about the most concise version (assuming that radius and colors can't be null). The null check for obj is taken care of by the instanceof test:

public boolean equals(Object obj) {

   if( ! (obj instanceof Circle ) )
       return false;

    Circle rhs = (Circle)obj;
    return Double.compare( radius, rhs.radius ) == 0 &&
        bgColor.equals( rhs.bgColor ) &&
        fgColor.equals( rhs.fgColor );
}

Answer 2

If you are using a IDE (I hope you do) probably it has an option to generate code for equals method. Eclipse generates something like:

@Override
public boolean equals(Object obj) {
    if (this == obj)
        return true;
    if (obj == null)
        return false;
    if (getClass() != obj.getClass())
        return false;
    Circle other = (Circle) obj;
    if (bgColor == null) {
        if (other.bgColor != null)
            return false;
    } else if (!bgColor.equals(other.bgColor))
        return false;
    if (fgColor == null) {
        if (other.fgColor != null)
            return false;
    } else if (!fgColor.equals(other.fgColor))
        return false;
    if (Double.doubleToLongBits(radius) != Double.doubleToLongBits(other.radius))
        return false;
    return true;
}

And don't forget implements hashcode method when you implements equals method and vicecersa.

Answer 3

Rather than having a single return statement consider using multiple return points to simplify the code. This way you do not need extra boolean variables to hold on to the results of prior conditions.

public class Circle {
    public double radius;
    public String bgColor;
    public String fgColor;

    public boolean equals(Object obj) {
        if (obj == null) {
            return false;
        } else if (obj instanceof Circle) {
            Circle other = (Circle) obj;
            if (Double.compare(this.radius, other.redius) == 0 
                    && compareStrings(this.fgColor, other.fgColor)
                    && compareStrings(this.bgColor, other.bgColor)) {
                return true;
            } else {
                return false;
            }
        } else {
            return false;
        }
    }

    private boolean compareStrings(String a, String b) {
        if (a == null && b == null) {
            return true;
        } else if (a != null) {
            return a.equals(b); 
        } else if (b != null) {
            return b.equals(a);
        }
        return false;
    }   
}

This solution allows for the possibility that either of the String fgColor or bgColor might be null without throwing a NPE. The String comparison has been extracted into its own function to aid readability and reduce confusion.

Answer 4

public boolean equals(Object obj) {
        if (obj == null) {
            return false;
        }
        if (getClass() != obj.getClass()) {
            return false;
        }
        // its a Circle so its safe to case
        Circle other = (Circle)obj;

        // equals ONLY if 3 conditions are met
        if (radius == other.getRadius() &&
            bgColor.equals(other.getBgColor()) &&
            fgColor.equals(other.getFgColor())){
            return true;
        }
        return false;
    }

Answer 5

As a follow-up to my previous answer:

Writing an equals method that works correctly in the presence of subclassing is extremely non-trivial (see Joshua Bloch's comments in Item 8 of `Effective Java').

Indeed, until relatively recently the was no widely known single method for doing this.

In 2009, the article "How to Write an Equality Method in Java" by Martin Odersky, Lex Spoon, and Bill Venners shows that this can be achieved in terms of a `canEqual' method.