Standardize #equals(Object) implementations

[pyokagan]

Style A or Style B? Vote with 👍 now!

[pyokagan]

Style A

addressbook-level4/src/main/java/seedu/address/commons/core/GuiSettings.java

Lines 43 to 58 in c9b222c

	@Override
	public boolean equals(Object other) {
	if (other == this) {
	return true;
	}
	if (!(other instanceof GuiSettings)) { //this handles null as well.
	return false;
	}
	GuiSettings o = (GuiSettings) other;
	return Objects.equals(windowWidth, o.windowWidth)
	&& Objects.equals(windowHeight, o.windowHeight)
	&& Objects.equals(windowCoordinates.x, o.windowCoordinates.x)
	&& Objects.equals(windowCoordinates.y, o.windowCoordinates.y);
	}

[pyokagan]

Style B

addressbook-level4/src/main/java/seedu/address/model/AddressBook.java

Lines 107 to 112 in c9b222c

	@Override
	public boolean equals(Object other) {
	return other == this // short circuit if same object
	|| (other instanceof AddressBook // instanceof handles nulls
	&& this.persons.equals(((AddressBook) other).persons));
	}

[pyokagan]

@Zhiyuan-Amos @yamgent @yamidark

Okay, it's quite obvious by now that Style A is the winner, but there are a few details to iron out. Here's my proposal:

 public boolean equals(Object other) { 

The name other is overwhelmingly used in our code base, so no need to change it, I think.

     if (other == this) { 
         return true; 
    } 

Some implementations add a // short circuit if same object here. Personally, I don't think it's needed since it's obvious that it is short circuiting.

    // instanceof handles nulls 
    if (!(other instanceof Foo)) {
         return false; 
    } 

Some implementations write the comment as this handles null as well, but I think we overwhelmingly have instanceof handles nulls.

Some implementations don't even put any comment, but I think the comment adds value (I didn't know that when I was starting out with Java :-P )^{Although would instanceof stay in the future??? Stay tuned :-P}

    Foo otherFoo = (Foo) other;

Some implementations call this various names (o, iterator, e etc.), but I think otherFoo is more consistent with our naming convention.

Some implementations add a // state check comment above, but I don't see any value in that.

If the object in question does not have any fields, this can be replaced with just a returns true;.

    return field1.equals(otherFoo.field1)
            && field2.equals(otherFoo.field2);

The instance's own fields must come first, followed by the other object's fields. Use Objects.equals() if the fields are nullable, and Objects.deepEquals() if they are arrays.

Any thoughts?

[yamgent]

Some implementations write the comment as this handles null as well, but I think we overwhelmingly have instanceof handles nulls.

Some implementations don't even put any comment, but I think the comment adds value

I think the comment is certainty useful, since Java's design can be quite haphazard sometimes. One just can't be sure that Java won't do something funny like throwing a NullPointerException for a instanceof check, even though it wouldn't make theoretical sense. :P

The rest looks good. 👍