Skip to content
This repository has been archived by the owner on Mar 5, 2023. It is now read-only.

Standardize #equals(Object) implementations #973

Closed
wants to merge 4 commits into from
Closed

Standardize #equals(Object) implementations #973

wants to merge 4 commits into from

Conversation

pyokagan
Copy link
Contributor

Fixes #901

@CanIHasReview-bot
Copy link

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

@pyokagan pyokagan force-pushed the chore/equals branch 3 times, most recently from adc7a10 to 0b2baed Compare February 23, 2019 07:16
@pyokagan
PersonCard: remove #equals(object)
6ba2ac7 
This method implements a very weak notion of equivalence, where as long
as the PersonCard's person and id are equal, the object is equal.
However, such a weak notion of equivalence goes against the spirit of
the UI architecture, which states that all UiParts represent a distinct
part of the UI, and more importantly, wraps a subgraph of the JavaFX
scene graph (accessible via UiPart#getRoot()). As such, just because two
PersonCards A and B happen to contain the same ID and person value,
doesn't mean they are "equal", because they may be holding different
JavaFX scene graph objects. Any JavaFX-related operations on PersonCard
A may give a different result to PersonCard B.

Furthermore, this implementation of PersonCard#equals(Object) isn't
actually used, not even by test code. It was introduced in baa5988
(PersonListPanelHandle: Update methods, 2017-07-14) so that one could
do the following to assert that a certain PersonCard was selected:

    private void assertCardSelected(Index index) {
        PersonCard selectedCard = getPersonListPanel().getSelectedCard().get();
        assertEquals(getPersonListPanel().getCard(index.getZeroBased()), selectedCard);
    }

where the assertEquals() would then call PersonCard#equals(Object).

However, since 04a675f (PersonListPanelHandle: Update
getSelectedCard(), 2017-07-20) this pattern of code has been (rightly)
removed, with test code preferring to compare what is actually displayed
on the PersonCard, rather than using PersonCard#equals(Object):

    /**
     * Asserts that {@code actualCard} displays the same values as {@code expectedCard}.
     */
    public static void assertCardEquals(PersonCardHandle expectedCard, PersonCardHandle actualCard) {
        assertEquals(expectedCard.getId(), actualCard.getId());
        assertEquals(expectedCard.getAddress(), actualCard.getAddress());
        assertEquals(expectedCard.getEmail(), actualCard.getEmail());
        assertEquals(expectedCard.getName(), actualCard.getName());
        assertEquals(expectedCard.getPhone(), actualCard.getPhone());
        assertEquals(expectedCard.getTags(), actualCard.getTags());
    }

PersonCard#equals(Object) was, however, not removed then. So let's
remove it now.
@pyokagan pyokagan force-pushed the chore/equals branch 2 times, most recently from 0f7eb40 to 87713b8 Compare February 23, 2019 07:44
@pyokagan
PersonCardHandle: rename #equals(Person) to #contentMatches(Person)
1a6ccf8 
PersonCardHandle#equals(Person) is an overloaded method (with the other
one being Object#equals(Object) inherited from Object). The
implementation of PersonCardHandle#equals(Person) implements a weak
version of equivalence that only compares the displayed contents of the
person card with the field of the person. This gives different results
compared to Object#equals(Object).

This breaks LSP. If all references to the type PersonCardHandle were
replaced with Object (the supertype of PersonCardHandle),
Object#equals(Object) would be called instead of
PersonCardHandle#equals(Person) and the behavior of our tests would
change.

To prevent this, rename #equals(Person) to #contentMatches(Person). This
is a method name that is not present in Object or NodeHandle, and thus
does not violate LSP.

While we are here, update the javadoc to draw a connection to the phrase
"content matches".
@pyokagan pyokagan force-pushed the chore/equals branch 2 times, most recently from 437f968 to 36a025d Compare February 23, 2019 08:00
@damithc damithc closed this Dec 11, 2020
@Eclipse-Dominator Eclipse-Dominator mentioned this pull request Jun 18, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
consider-for-AB3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Standardize #equals(Object) implementations
3 participants
@pyokagan @CanIHasReview-bot @damithc