FindBugs controversial description

Question

Am I understanding it wrong, or is the description wrong?

Equals checks for noncompatible operand (EQ_CHECK_FOR_OPERAND_NOT_COMPATIBLE_WITH_THIS)

This equals method is checking to see if the argument is some incompatible type (i.e., a class that is neither a supertype nor subtype of the class that defines the equals method). For example, the Foo class might have an equals method that looks like:

public boolean equals(Object o) {
  if (o instanceof Foo)
    return name.equals(((Foo)o).name);
  else if (o instanceof String)
    return name.equals(o);
  else return false;

This is considered bad practice, as it makes it very hard to implement an equals method that is symmetric and transitive. Without those properties, very unexpected behavoirs are possible.

From: http://findbugs.sourceforge.net/bugDescriptions.html#EQ_CHECK_FOR_OPERAND_NOT_COMPATIBLE_WITH_THIS

The description says that the Foo class might have an equals method like that, and after it says that "This is considered bad practice". I'm not getting the "right way"..

How should the following method be to be right?

@Override
public boolean equals(Object obj) {
if (obj instanceof DefaultTableModel)
    return model.equals((DefaultTableModel)obj);
else
    return false;
}

Answer 1

When the FindBugs description says that such-and-such "might be the case", they are saying not that it's an acceptable practice, but that it's a hypothetical circumstance, which would lead to the warning in question.

You should not say that your object is equal to some DefaultTableModel, since there is no way to enforce the reflexivity of that relationship. This means that given

DefaultTableModel dtm = new DefaultTableModel(...);
YourObject foo = new YourObject(dtm);
foo.equals(dtm); // true
dtm.equals(foo); // false!

Answer 2

The problem with the first version of the method (the FindBugs complains about) is that it is NOT symmetric.

If you have an Foo object f for which f.equals("someName") is true, then symmetry says that "someName".equals(f) should also be true. But there is no way you can implement that: "someName".equals(...) is going to return false for any parameter that is not a String.

The second version of the method is wrong too because you are saying that a Foo instance can be equal to a DefaultTableModel instance... but not another Foo instance. That means that a Foo instance cannot be equal to itself, and that equals is therefore NOT reflexive. Besides, it is not clear what the model identifier is ...

Answer 3

The unusual part of your implementation is that you compare the field "model" with the complete object "obj". Typically you should compare each field of self to each field of the other object after you have checked that they are both of the same class or sub class.