Is it a bad practice to include non-public fields into equals()

Question

Is it a bad practice to include fields inaccessible by any public methods (and which themselves are not and can not be derived from any other public fields) into equals() ?

What are possible bad consequences of doing so?

Is this described as a bad practice somewhere? (I assume hash/equals contract is ok)

I feel, such equals would be philosophically very wrong. Such equals is not observed in the world. But can it lead to real programming misbehavior, philosophy aside?

Answer 1

I can think of one bad consequence - the users of your class might compare two objects for which all the publicly accessible properties are equal, and they won't understand why your equals method returns false.

I'm assuming (and your edit confirms it) you mean private fields with no public access (i.e. without getters), since in general all properties should always be private, and only accessed by setters and getters.

Answer 2

It's absolutely fine and sometimes necessary. The only fields that you shouldn't compare are ones marked as transient.

I'd assert that two instances are equal if and only if their serialisations are equal. That's why transient matters and the degree of field encapsulation doesn't.

Answer 3

It's fine. If you are writing the class yourself, the users can still get and set the fields using getter and setter methods you might add. Make sure the fields are secure if you add getter and setter methods, though.

If you are not writing getter and setter methods, it can be difficult for Users to identify why objects are not equal based on the values of all the public fields. Having said this, one might be able to use a debugger to step through all the values of the private fields.

Another possibility is to create a special method that will return which specific fields are not equal. Although this does not strictly conform to the requirement that the fields should be "inaccessible by any public methods", returning Strings, primitive types or objects that are created on the fly (such as an array of the field names) will keep the method secure.

Answer 4

What should go into equals()?

There are fields that define the identity of an object and there are fields that define its properties. The serial number of your phone is part of its identity, its colour or battery charge percentage is just a property.

equals() should only depend on fields that define identity, and that is independent of whether the fields are accessible from the outside or not, it comes from the semantics of the fields themselves.

So the question should really be: is there any reason to hide the identity of an object from its clients? And if the answer seems to be "yes", the next, more fundamental question is: is there any reason to identify the objects in the first place? After all, there are lots of objects out there that don't require to have an identity beyond the trivial.

There are several possible problems that arise from it, here's one example.

Example of problem

public class Foo {
    private Bar hidden;
    private String accessible;

    @Override
    public boolean equals( Object ob ) {
       return hidden.equals(((Foo)ob).hidden);
    }

    @Override
    public int hashCode() {
       //Let's not forget that we MUST override hashCode() if we override equals() :)
    }
}

Now image a naive client wants to put these into a TreeSet. The class isn't Comparable but nothing to worry, we can write a comparator for it:

public class FooComparator implements Comparator<Foo> {

    public int compare( Foo first, Foo second ) {
        return first.getAccessible().compareTo( second.getAccessible() );
    }
}

Job done, great!

Except poor client unwittingly violated the contract of TreeSet as the comparator provided isn't consistent with equals().

How to work around this?

Let's suppose that despite knowing all this, you still want your own equals() that depends on hidden fields. Because you want to put them in a HashSet for example.

You can then do this:

public class Foo {
    private Bar hidden;
    private String accessible;
    private final Helper helper = new Helper();


    Helper helper() {
       return helper;
    }      

    class Helper {

        @Override
        public boolean equals( Object ob ) {
          // you can access "hidden" from here
        }

        @Override
        public int hashCode() {
          // you can access "hidden" from here
        }

        public Foo foo() {
            return Foo.this;
        }
    }
}

And you put your Helper objects in the HashSet instead of Foo. Although Helpers visibility is package-private, that's still acceptable in most cases, as this level is not part of the public API.

Answer 5

You should put in fields that makes the object unique. If you can modify the fields are of lesser importence.