2

I'm implementing a value object for these interfaces:

interface FooConsumer
{
    public void setFoo(FooKey key, Foo foo);
    public Foo getFoo(FooKey key);
}

// intent is for this to be a value object with equivalence based on 
// name and serial number
interface FooKey
{
    public String getName();
    public int getSerialNumber();
}

and from what I've read (e.g. in Enforce "equals" in an interface and toString(), equals(), and hashCode() in an interface) it looks like the recommendation is to provide an abstract base class, e.g.

abstract class AbstractFooKey
{
    final private String name;
    final private int serialNumber
    public AbstractFooKey(String name, int serialNumber)
    {
       if (name == null)
          throw new NullPointerException("name must not be null");
       this.name = name;
       this.serialNumber = serialNumber;
    }
    @Override public boolean equals(Object other)
    {
       if (other == this)
          return true;
       if (!(other instanceof FooKey))
          return false;
       return getName().equals(other.getName() 
          && getSerialNumber() == other.getSerialNumber()
          && hashCode() == other.hashCode();       // ***
    }
    @Override public int hashCode()
    {
       return getName().hashCode() + getSerialNumber()*37;
    }
}

My question is about the last bit I added here, and how to deal with the situation where AbstractFooKey.equals(x) is called with a value for x that is an instance of a class that implements FooKey but does not subclass AbstractFooKey. I'm not sure how to handle this; on the one hand I feel like the semantics of equality should just depend on the name and serialNumber being equal, but it appears like the hashCodes have to be equal as well in order to satisfy the contract for Object.equals().

Should I be:

  1. really lax and just forget about the line marked ***
  2. lax and keep what I have
  3. return false from equals() if the other object is not an AbstractFooKey
  4. be really strict and get rid of the interface FooKey and replace it with a class that is final?
  5. something else?
Community
  • 1
  • 1
Jason S
  • 184,598
  • 164
  • 608
  • 970

3 Answers3

2

Document the required semantics as part of the contract.

Ideally you'd actually have a single implementation which is final, which kind of negates the need of an interface for this particular purpose. You may have other reasons for wanting an interface for the type.

The contract requirements of Object is actually from hashCode: If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result.

You don't need to include hashCode in the equals computation, rather you need to include all properties involved in equals in the hashCode calculation. In this case I'd simply compare serialNumber and name in both equals and hashCode.

Keep it simple unless you have a real reason to complicate it.

Start with a final, immutable class.

If you need an interface, create one to match, and document the semantics and default implementation.

ptomli
  • 11,730
  • 4
  • 40
  • 68
  • Your assertion "rather you need to include all properties involved in equals in the hashCode calculation" is incorrect. `hashCode` needs only to have a subset of the `equals` properties. Taken to an extreme, `public int hashCode() { return 123; }` is perfectly valid. – Steve Kuo Oct 09 '12 at 15:33
1

For the equals and hashmap, there are strict contracts:

  1. Reflexive - It simply means that the object must be equal to itself, which it would be at any given instance; unless you intentionally override the equals method to behave otherwise.

  2. Symmetric - It means that if object of one class is equal to another class object, the other class object must be equal to this class object. In other words, one object can not unilaterally decide whether it is equal to another object; two objects, and consequently the classes to which they belong, must bilaterally decide if they are equal or not. They BOTH must agree.

  3. Transitive - It means that if the first object is equal to the second object and the second object is equal to the third object; then the first object is equal to the third object. In other words, if two objects agree that they are equal, and follow the symmetry principle, one of them can not decide to have a similar contract with another object of different class. All three must agree and follow symmetry principle for various permutations of these three classes.

  4. Consistent - It means that if two objects are equal, they must remain equal as long as they are not modified. Likewise, if they are not equal, they must remain non-equal as long as they are not modified. The modification may take place in any one of them or in both of them.

  5. null comparison - It means that any instantiable class object is not equal to null, hence the equals method must return false if a null is passed to it as an argument. You have to ensure that your implementation of the equals method returns false if a null is passed to it as an argument.

Contract for hashCode():

  1. Consistency during same execution - Firstly, it states that the hash code returned by the hashCode method must be consistently the same for multiple invocations during the same execution of the application as long as the object is not modified to affect the equals method.

  2. Hash Code & Equals relationship - The second requirement of the contract is the hashCode counterpart of the requirement specified by the equals method. It simply emphasizes the same relationship - equal objects must produce the same hash code. However, the third point elaborates that unequal objects need not produce distinct hash codes.

(From: Technofundo: Equals and Hash Code)

However, using instanceof in equals is not the right thing to do. Joshua Bloch detailed this in Effective Java, and your concerns regarding the validity of your equals implementation is valid. Most likely, problems arising from using instanceof are going to violate the transitivity part in the contract when used in connection with descendants of the base class - unless the equals function is made final.

(Detailed a bit better than I could ever do here: Stackoverflow: Any reason to prefer getClass() over instanceof when generating .equals()?)

Also read:

Community
  • 1
  • 1
ppeterka
  • 20,583
  • 6
  • 63
  • 78
  • bit late, but what do you mean, Bloch detailed in Effective Java that it's not the right thing to do using instanceof in an equals method? To quote Effective Java 2nd edition (page 42) under the title: "here’s a recipe for a high-quality equals method:" you'll find: "2. Use the instanceof operator to check if the argument has the correct type." He also recommends using instanceof to see whether the object passed is null or not, instead of adding an additional if ( o == null ) return false; – Stultuske May 11 '15 at 13:57
0

If the equality of a FooKey is such that two FooKeys with the same name and serial numbers are considered to be equal then you can remove the line in the equals() clause that compares the hashcodes.

Or you could leave it in, it does not really matter assuming that all implementors of the FooKey interface have a correct implementation of equals and gethashcode but I would recommend removing it since otherwise a reader of the code could get the impression that it is there because it makes a difference when in reality it does not.

You can also get rid of the '*37' in the gethashcode method, it is unlikely it would contribute to better hashcode distribution.

In terms of your question 3, I would say no, don't do that, unless the equality contract for FooKey is not controlled by you (in which case trying to enforce an equality contract for the interface is questionable anyway)

user469104
  • 1,206
  • 12
  • 15