Should I be overriding equals and hashCode in child classes even if it's not adding anything?

Question

I have abstract class that override equals() and hashCode(). In those methods I use abstract method getDescription() to check for equality and to generate hashCode. Now when I extend the class and add a field that is only used in getDescription() method I get a sonarLint issue "Extends a class that overrides equals and add fields". Is it just Sonar not being sophisticated enough to understand what's going on or am I doing it the not java way and there is a better/more elegant way?

Parent class:

public abstract String getDescription();   

@Override
public int hashCode()
{
    return new HashCodeBuilder(19, 71).
            append(mViolation).
            append(getDescription()).
            append(mProperties).
            toHashCode();
}

@Override
public boolean equals(
    final Object obj)
{
    boolean equal = false;
    if (this == obj)
    {
        equal = true;
    }
    else if (obj instanceof parent)
    {
        AbstractStructuredDataIssue rhs = (parent) obj;
        equal = new EqualsBuilder().
                append(mViolation, rhs.mViolation).
                append(getDescription(), rhs.getDescription()).
                append(mProperties, rhs.mProperties).
                isEquals();
    }
    return equal;
}

Child class:

public class Child extends Parent {
    private final String mMessage;

    public Child(final String message, final int number) {
        super(number);
        mMessage = message;
    }

    @Override
    public String getDescription()
    {
        return String.format(
                DESCRIPTION_FORMAT,
                mMessage);
    }
}

Answer 1

With your implementation you can have two Parent references which are equal, and yet point to objects of two different classes, so that one can be cast to Child and the other - not.

This is highly unexpected and could lead to problems down the road - and it's Sonar's job to point it out. If you think it's justified for your use case, just document it live with Sonar's warning (this is what I would do).

Answer 2

• Overriding equals() and hashcode() method in child class will be effective considering child class members(variables) and it also helps while using sub-types of Collection framework and Map instances to find right memory space(bucket) during collection framework operations(Eg: save/retrieve).

  Inheriting from super class here may miss child class members to generate hashcode/equals method functionality effectively.

Answer 3

Let's take a look on the rule RSPEC-2160:

Extend a class that overrides equals and add fields without overriding equals in the subclass, and you run the risk of non-equivalent instances of your subclass being seen as equal, because only the superclass fields will be considered in the equality test.

What Sonar points out is the risk of you get unequal objects being seen as equal, since when you call equals in your subclass, without a proper implementation, only the parent class fields will be evaluated.

Noncompliant Code Example (from the docs)

public class Fruit {
  private Season ripe;

  public boolean equals(Object obj) {
    if (obj == this) {
      return true;
    }
    if (this.class != obj.class) {
      return false;
    }
    Fruit fobj = (Fruit) obj;
    if (ripe.equals(fobj.getRipe()) {
      return true;
    }
    return false;
  }
}

public class Raspberry extends Fruit {  // Noncompliant; instances will use Fruit's equals method
  private Color ripeColor;
}

Compliant Solution (also from the docs)

public class Fruit {
  private Season ripe;

  public boolean equals(Object obj) {
    if (obj == this) {
      return true;
    }
    if (this.class != obj.class) {
      return false;
    }
    Fruit fobj = (Fruit) obj;
    if (ripe.equals(fobj.getRipe()) {
      return true;
    }
    return false;
  }
}

public class Raspberry extends Fruit {
  private Color ripeColor;

  public boolean equals(Object obj) {
    if (! super.equals(obj)) {
      return false;
    }
    Raspberry fobj = (Raspberry) obj;
    if (ripeColor.equals(fobj.getRipeColor()) {  // added fields are tested
      return true;
    }
    return false;
  }
}

I agree with your point that Sonar may be not sophisticated enough to see what is happening at runtime, hence it points the Code Smell.

You need to be worried to not break the equals and hashCode contract, your approach is dynamic, which probably is not considered by Sonar.

Answer 4

This is a bit complicated; I have to explain a few things about how equals and hashCode works to explain viable solutions.

There is a 'contract'. The compiler cannot enforce it, but if you do not adhere to this contract, weird things will happen. Specifically: Your objects will just do the wrong thing when used as keys in hashmaps, and possibly other such problems when using third party libraries. To properly adhere to the contract, any given class either needs to opt out of equals/hashCode entirely, OR, the entire chain (so, the class and all its subclasses) need to properly override hashCode and equals, except, you really can't do that unless the parent is properly instrumented to do so.

The contract states that this must always be correct:

• a.equals(b) -> b.equals(a).
• a.equals(b) and b.equals(c) -> a.equals(c).
• a.equals(a).
• a.equals(b) -> a.hashCode() == b.hashCode(). (note, the reverse need not be true; equal hashcodes does not imply the objects are equal).

The contract is REALLY difficult to guarantee in the face of a class hierarchy! Imagine that we take the existing java.util.ArrayList and subclass it with the notion of 'color'. So now we can have a blue ColoredArrayList, or a red ColoredArrayList. It would make perfect sense to say that a blue ColoredArrayList definitely should NOT equal a red ColoredArrayList, except.. the equals impl of ArrayList itself (which you cannot change), effectively defines that you simply cannot extend ArrayList with properties like this at all: if you call a.equals(b) where a is an empty arraylist and b is some empty List (say, an empty red ColoredArrayList), it'll just check equality of each member in it, which, given that they are both empty, is trivially true. So, the empty normal arraylist is equal to both the empty red and the empty blue ColoredArrayList, and therefore the contract stipulated that an empty red ColoredArrayList must equal an empty blue ColoredArrayList. In that sense, sonar is just broken here. There's a problem, and it is unfixable. It is impossible to write the concept of ColoredArrayList in java.

Nevertheless, there is a solution, but only if every class in the hierarchy is on board. This is the canEqual approach. The way out of the colored dilemma as above is to differentiate the notion of 'I am extending, and adding new properties' and 'I am extending, but, these things are still semantically speaking the exact same thing with no new properties'. ColoredArrayList is the former case: It's an extension that adds new properties. The idea of canEqual is that you create a separate method to indicate this, which lets ArrayList figure out: I cannot be equal to ANY ColoredArrayList instance, even if all elements are the same. Then we can adhere to the contract again. ArrayList does NOT have this system in place and therefore, given that you cannot change ArrayList's source code, you're stuck: It is not fixable. But if you write your own class hierarchy, you can add it.

Project Lombok takes care of adding equals and hashCode for you. Even if you don't want to use it, you can look at what it generates and duplicate this in your own code. This will also remove the warnings that sonar emits. See https://projectlombok.org/features/EqualsAndHashCode – this also shows you how the canEqual concept can be used to avoid the ColoredArrayList dilemma.

Here you subclass without adding new properties, so, there's no actual need to replace hashCode and equals. But sonar doesn't know that.