Symphonious

Andrae Muys provides some excellent advice on implementing Object.equals() however I do have to correct one thing. Andrae presents the code:

class A {
  int n1;
  public boolean equals(Object o) {
    if (!(o instanceof A)) {
      return false;
    } else {
      A rhs = (A)o;
      return this.n1 == rhs.n1;
    }
  }
}

and suggests that it is incorrect. It is not. This is absolutely, 100% the correct way to implement equals for that class. The alternative he presents on the other hand is incorrect:

class A {
  int n1;
  public boolean equals(Object o) {
    if (o == this) {
      return true;
    } else if (o == null || !getClass().equals(o.getClass())) {
      return false;
    } else {
      A rhs = (A)o;
      return this.n1 == rhs.n1;
    }
  }
}

The problem with this second form only appears when a subclass is used:

class C extends A {
  public getValue() {
    return n1;
  }
}

With the first implementation, equals returns the correct value regardless of whether the object is an instance of C or A, but the second will not compare A and C correctly even though C has not changed the semantics of equality in any way. Andrae presents the following class to justify his assertion of the first way being wrong:

class B extends A {
  int n2;
  public boolean equals(Object o) {
    if (!(o instanceof B)) {
      return false;
    } else {
      B rhs = (B)o;
      return this.n1 == rhs.n1 && this.n2 == rhs.n2;
    }
  }
}

There is a bug here but it is in class B and not in class A. Class B is incorrectly assuming that it knows how to compare class A’s data, and more importantly class B is changing the behavior of A in a way that is not compatible with the original model. Essentially, class A defines equality as having the same n1 values and class B is in the wrong for changing that.

The golden rule here is that when you extend a class you are responsible for maintaining compatibility with the parent class. You should never extend a class and change the behavior of a method such that it no longer meets the contract of the original. This does not mean you can’t change the behavior, merely that it must be match the documented behavior - ie: it should do what the JavaDocs say the method does but not necessarily what the code actually does.

To give an example, consider the common example of a payroll system’s employee class:

public class Employee {

  /** The salary (in cents per year) for this employee. */
  private int salary;

  // Getter/setter for salary as appropriate and an appropriate constructor.

  /** Calculates the amount due to the employee for the current pay cycle.
    *
    * @return the amount (in cents) due to the employee for the current pay cycle.
    */
  public int calculatePayDue() {
    return salary / 52;
  }
}

It would be valid to extend the employee class as follows:

public class SalesPerson extends Employee {

    /** The amount of commission (in cents) due to this employee for the current
      * pay period.
      */
    private int commission;

    // Getters and setters as appropriate and an appropriate constructor.

    /** @see A.calculatePayDue() */
    public int calculatePayDue() {
        return super.calculatePayDue() + commission;
    }
}

The subclass changes the behavior of calculatePayDue but it still meets the original contract for the method, so it is correct. Similarly, Andrae’s class A meets the original contract for Object.equals and is thus correct, it is the implementation of equals in Andrae’s class B that breaks that contract and is thus incorrect.

There are two important lessons to learn from this (on top of the numerous important lessons to learn from Andrae’s post):

1. The JavaDoc is more important than the code. It is the JavaDoc that defines the contract for the method in Java and if the code doesn’t match the JavaDoc, the code is incorrect - though it may be more desirable to fix that by changing the JavaDoc if it’s a private method.
2. You should never extend a class in a way that breaks the contract of the super class. You should be able to run the unit tests for class A over class B and have most of them pass.

To clarify that second point, some of the tests for class A will fail because they actually test the behavior of A instead of the contract for A. The way to avoid this is to separate out the contract into an interface, you would then expect all the tests for class A’s implementation of the interface to also pass when an instance of class B was used (regardless of whether or not B extended from A or just implemented the same interface).

This entry was posted on Monday, August 30th, 2004 at 5:33 pm and is filed under Code and Geek Stuff. You can follow any responses to this entry through the Atom feed. You can leave a response, or trackback from your own site.