In Pitfall #1, #2, and #3:
result = (this.getX() == that.getX() && this.getY() == that.getY());
I would prefer:
result = that.xEquals( getX() ) && that.yEquals( getY() );
Or even:
result = that.equals( getX(), getY() );
This:
* avoids duplicating code (writing this.getX() == that.getX() will likely occur more than once);
* allows changing of the underlying data type (as Java cannot override a method signature based on return type alone);
* allows subclasses to inject behavioural changes, such as logging (ideally without breaking LSP);
* promotes encapsulation: the object containing the data performs tasks upon it; and
* is shorter.
The equals method then reduces to:
@Override public boolean equals(Object other) {
return other instanceof Point ? ((Point)other).equals( getX(), getY() ) : false;
}
For Pitfall #3, you could also implement an Observer - Observable pattern so that the collection can rehash its objects if any of them are changed. Using a separate method, such as equalsContents, however, is simpler.
For the section on the canEqual method, I would evoke the same encapsulation metaphor:
result = that.canEqual( this ) && that.colorEquals( getColor() ) && super.equals( that );
Note that the color instance variable, even though it was declared final, is best not touched directly (at least until a new revision of Java is released that is like Smalltalk in how it handles instance variables). It seems to me that direct manipulation of class-scope instance variables and null data have needlessly introduced more bugs in the various Java-based systems I have worked on than all other problems combined! (I am, of course, exaggerating.)
I still feel like there is some redundancy in the canEquals and equals methods. Specifically:
@Override public boolean equals(Object other) {
boolean result = false;
if (other instanceof ColoredPoint) {
And:
@Override public boolean canEqual(Object other) {
return (other instanceof ColoredPoint);
I have not worked out the logic (reversing this and that might be incorrect), but perhaps the equals method can be reduced to:
@Override public boolean equals(Object other) {
boolean result = false;
if( this.canEqual( other ) ) {
ColoredPoint that = (ColoredPoint) other;
result = that.colorEquals( getColor() ) && super.equals( that );
}
return result;
}
Which would further reduce to:
return this.canEqual( other ) ?
((ColoredPoint)other).colorEquals( getColor() ) && super.equals( that ) :
false;
An exceptional article; thank you for writing it!