Standardize #equals(Object) formatting #197

Eclipse-Dominator · 2023-06-18T21:22:33Z

Partially adapted from AB4#973

To go along with some of the equality checks, some methods and constructors are reevaluated to follow defensive programming by adding suitable null checks.

AB3 has all sorts of #equals(Object) flavors. Some of them are formatted
with nested logic comparison while some contain unnecessary code. It will
enable better readability for others if they were to be standardized to
follow a similar format.

Generally, #equals(Object) should:

Name the parameter 'other' of the method
Check if other == this
Check if other is an instance of this class + comment for null check
Set a temporary variable otherCLASSNAME
Perform other related internal checks.

Should the internal variable be nullable, we should use
Objects.equals(var, var2) to compare between the 2.

Doing the above should improve the clarity and ease of
use of #equals(Object) methods.

canihasreview · 2023-06-18T21:22:36Z

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

See this repository's contribution guide for more information.

codecov · 2023-06-18T21:23:49Z

Codecov Report

Merging #197 (ab8e0e8) into master (a976ec9) will increase coverage by 1.34%.
The diff coverage is 79.13%.

@@             Coverage Diff              @@
##             master     #197      +/-   ##
============================================
+ Coverage     74.03%   75.37%   +1.34%     
+ Complexity      423      419       -4     
============================================
  Files            71       71              
  Lines          1290     1332      +42     
  Branches        127      126       -1     
============================================
+ Hits            955     1004      +49     
+ Misses          301      298       -3     
+ Partials         34       30       -4

Files Changed	Coverage Δ
src/main/java/seedu/address/ui/PersonCard.java	`0.00% <ø> (ø)`
.../main/java/seedu/address/commons/core/Version.java	`82.50% <20.00%> (ø)`
...c/main/java/seedu/address/commons/core/Config.java	`77.27% <25.00%> (ø)`
...n/java/seedu/address/commons/core/GuiSettings.java	`76.92% <25.00%> (ø)`
src/main/java/seedu/address/model/AddressBook.java	`83.33% <33.33%> (-8.98%)`	⬇️
...a/seedu/address/model/person/UniquePersonList.java	`86.95% <33.33%> (-5.91%)`	⬇️
src/main/java/seedu/address/model/tag/Tag.java	`71.42% <33.33%> (-8.58%)`	⬇️
src/main/java/seedu/address/model/UserPrefs.java	`80.00% <50.00%> (ø)`
src/main/java/seedu/address/AppParameters.java	`95.65% <100.00%> (+17.39%)`	⬆️
...n/java/seedu/address/commons/core/index/Index.java	`100.00% <100.00%> (ø)`
... and 12 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

canihasreview · 2023-06-18T21:25:59Z

v1

@Eclipse-Dominator submitted v1 for review.

[v1 1/3] Include additional null checks
[v1 2/3] Add missing javadoc for NameContainsKeywordsPredicate
[v1 3/3] Standardize #equals(Object) implementations

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/197/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

damithc

@Eclipse-Dominator let's include all applicable commits of se-edu/addressbook-level4#973 and not add any other unrelated commits.
When a PR does a bigger task in multiple smaller steps, we should use a merge commit (not a rebase merge, nor a squash commit) so that both steps and overall outcome are clear -- but we can't do that if there are unrelated commits in the PR.

damithc · 2023-06-20T15:29:38Z

src/main/java/seedu/address/commons/util/ToStringBuilder.java

@@ -1,5 +1,7 @@
 package seedu.address.commons.util;


Use chatGPT to refine the commit message? I spotted some grammar errors.

damithc · 2023-06-20T15:33:56Z

src/main/java/seedu/address/commons/util/ToStringBuilder.java

@@ -16,6 +18,7 @@ public class ToStringBuilder {
     * Constructs a {@code ToStringBuilder} whose formatted output will be prefixed with {@code objectName}.
     */
    public ToStringBuilder(String objectName) {
+        requireNonNull(objectName);
        stringBuilder.append(objectName).append(OBJECT_PREFIX);
    }


I don't think we need to check every parameter for null. It need to be used judiciously, to preempt cases where letting a null pass through can cause hard to detect bugs or cause other damage. There is no need to do null checks for cases where a null would cause an obvious and quick failure without affecting system integrity (e.g., without writing incorrect values to the database).
So, we need to think hard if this commit is needed.

damithc · 2023-06-20T15:42:38Z

src/main/java/seedu/address/model/person/NameContainsKeywordsPredicate.java

@@ -14,6 +14,9 @@
 public class NameContainsKeywordsPredicate implements Predicate<Person> {


Not related to this PR. Submit it as a separate PR.

canihasreview · 2023-07-13T00:47:02Z

v2

@Eclipse-Dominator submitted v2 for review.

[v2 1/2] Standardize #equals(Object) implementations
[v2 2/2] Add tests for #equals(Object) implementations

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/197/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

damithc · 2023-07-13T05:43:20Z

@Eclipse-Dominator how does V2 differ from se-edu/addressbook-level4#973 ?

Eclipse-Dominator · 2023-07-13T11:34:58Z

@Eclipse-Dominator how does V2 differ from se-edu/addressbook-level4#973 ?

It includes some additional standardisation of equals() that was not included in ab4#973. The other 2 commit that was left out was from features that was not found in ab3

damithc · 2023-07-13T13:31:16Z

It includes some additional standardisation of equals() that was not included in ab4#973. The other 2 commit that was left out was from features that was not found in ab3

@Eclipse-Dominator I see. Confirmed these additional equals() are actually used/needed? We need not add it to very class, as it would add noise without adding value.

Eclipse-Dominator · 2023-07-13T13:41:14Z

@Eclipse-Dominator I see. Confirmed these additional equals() are actually used/needed? We need not add it to very class, as it would add noise without adding value.

There are no additional equals() methods added. The first commit just standardizes existing equal() implementations. So there is no additional noise added.

The second commit adds some extra test cases for some of equals() method that does not contain a unit test.

damithc

Just a comment on the commit message. The rest seems good to go.

damithc · 2023-07-13T15:33:16Z

docs/tutorials/AddRemark.md

@@ -118,7 +118,6 @@ public class RemarkCommand extends Command {



The commit message can be short, as this change doesn't really require a justification.

The style of the #equals(Object) implementation varies across the code base. Let's standardize it to improve the consistency of the code. Co-authored-by: Zhaoqi <[email protected]>

In general, avoid the following when writing commit messages:

Using should do X as if it's a universally accepted rule. Often, it is just our choice, and there could be ways of doing it. Instead, explain it as what the commit does Let's do X.

Reversing the solution and stating it as a problem. e.g., the code doesn't omit X (problem) ... let's omit X (solution). Often the problem is not an exact opposite of the solution. e.g., Logger is giving a warning when it should not (problem) ... Let's omit X from checkstyle (solution).

damithc · 2023-07-13T15:34:35Z

@se-edu/tech-team-level1 for your review ...

hansstanley

Just a suggestion for your consideration (or amusement), perhaps the #equals standardization could better follow the DRY principle.

A utility function could be added to commons.util.AppUtil (or another shared class):

public class AppUtil {
    ...
    public static <T> boolean isEqual(Class<T> clazz, T target, Object other, BiFunction<T, T, Boolean> resolver) {
        if (target == other) {
            return true;
        }
        if (!clazz.isInstance(other)) { // handles null
            return false;
        }
        return resolver == null || resolver.apply(target, clazz.cast(other));
    }
}

Then other classes could use it like so:

public class Phone {
    ...
    @Override
    public boolean equals(Object other) {
        return AppUtil.isEqual(Phone.class, this, other, (p1, p2) -> {
            return p1.value.equals(p2.value);
        });
    }
}

Not too sure of any downsides so any criticism is welcome :)

cjyothika

LGTM. I have one small doubt regarding one of the methods but generally, the changes look good.

cjyothika · 2023-07-14T03:12:26Z

src/main/java/seedu/address/logic/commands/EditCommand.java

-                    && getAddress().equals(e.getAddress())
-                    && getTags().equals(e.getTags());
+            EditPersonDescriptor otherEditPersonDescriptor = (EditPersonDescriptor) other;
+            return Objects.equals(name, otherEditPersonDescriptor.name)


From my understanding, Objects.equals() considers 2 nulls to be equal. Although, in this case, the name field being null would have been caught already, perhaps it would still be better to use Object.equals() or the default equals method of the respective objects?

eg.
name.equals(otherEditPersonDescriptor.name)

I don't think this will occur due to requireNonNull(editPersonDescriptor);.

it's possible for the local name field to be null. User does not need to edit all the fields of an entry so it's possible to have name being null. In this case name.equals(...) will throw nullpointer exception.

So Objects.equals is used here

damithc · 2023-07-14T04:17:21Z

Just a suggestion for your consideration (or amusement), perhaps the #equals standardization could better follow the DRY principle.

A utility function could be added to commons.util.AppUtil (or another shared class):

public class AppUtil {
    ...
    public static <T> boolean isEqual(Class<T> clazz, T target, Object other, BiFunction<T, T, Boolean> resolver) {
        if (target == other) {
            return true;
        }
        if (!clazz.isInstance(other)) { // handles null
            return false;
        }
        return resolver == null || resolver.apply(target, clazz.cast(other));
    }
}

Then other classes could use it like so:

public class Phone {
    ...
    @Override
    public boolean equals(Object other) {
        return AppUtil.isEqual(Phone.class, this, other, (p1, p2) -> {
            return p1.value.equals(p2.value);
        });
    }
}

Not too sure of any downsides so any criticism is welcome :)

Interesting idea @hansstanley I like how it avoids the repetition of the first two checks. One downside I see is that it goes from a very straightforward code to a more complex code.
Also, the visibly repeating pattern of code might have a learning value in that AB3 developers will remember this implementation as the way to implement the equals method (which is not a bad thing).

SPWwj · 2023-07-14T09:08:03Z

src/main/java/seedu/address/AppParameters.java

@@ -50,12 +50,13 @@ public boolean equals(Object other) {
            return true;
        }

+        // instanceof handles nulls


This comment might not be necessary.

It's to standardize across all equals() so all of the equals() contains this comment.

k instanceof clazz returns false if k is null is not a common way to check for null value of k so the comment is included there.

src/main/java/seedu/address/commons/core/Config.java

SPWwj

LGTM, might need to clean up some unnecessary comments.

Eclipse-Dominator · 2023-07-14T14:34:22Z

I think it's quite an interesting solution but I do agree with @damithc that having the current implementation might be better for students viewing the code.

canihasreview · 2023-07-14T14:39:20Z

v3

@Eclipse-Dominator submitted v3 for review.

[v3 1/2] Standardize #equals(Object) implementations
[v3 2/2] Add tests for #equals(Object) implementations

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/197/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

SPWwj · 2023-07-15T13:41:12Z

src/main/java/seedu/address/ui/PersonCard.java

@@ -69,7 +68,6 @@ public boolean equals(Object other) {
            return false;
        }

-        // state check
        PersonCard card = (PersonCard) other;
        return id.getText().equals(card.id.getText())


Should we add a comment here to inform the reviewer that the 'Label equals()', the id , behavior differs from our 'equals()' method?

@Eclipse-Dominator your thoughts?

@Eclipse-Dominator your thoughts?

@Eclipse-Dominator reminder ...

This method is currently not used in AB3 and does not really fit the idea of equal, thus it's better off removed from the source code.

I have included the commit from AB4 that removes this method in v4

The style of the #equals(Object) implementation varies across the code base. Let's standardize it to improve the consistency of the code. Co-authored-by: Zhaoqi <[email protected]>

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.

canihasreview · 2023-07-27T01:21:15Z

v4

@Eclipse-Dominator submitted v4 for review.

[v4 1/3] Standardize #equals(Object) implementations
[v4 2/3] Add tests for #equals(Object) implementations
[v4 3/3] PersonCard: remove #equals(object)

(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/197/4/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Eclipse-Dominator changed the title ~~Standardize equal formatting~~ Standardize #equals(Object) formatting Jun 18, 2023

canihasreview bot added the s.ToReview label Jun 18, 2023

damithc reviewed Jun 20, 2023

View reviewed changes

Eclipse-Dominator force-pushed the standardize-equal-formatting branch 2 times, most recently from e1c766e to 0c8fd9d Compare July 13, 2023 00:46

damithc reviewed Jul 13, 2023

View reviewed changes

hansstanley reviewed Jul 14, 2023

View reviewed changes

cjyothika reviewed Jul 14, 2023

View reviewed changes

SPWwj reviewed Jul 14, 2023

View reviewed changes

src/main/java/seedu/address/commons/core/Config.java Show resolved Hide resolved

SPWwj reviewed Jul 14, 2023

View reviewed changes

Eclipse-Dominator force-pushed the standardize-equal-formatting branch 2 times, most recently from 20b6dae to 2b257b5 Compare July 14, 2023 14:38

SPWwj reviewed Jul 15, 2023

View reviewed changes

Eclipse-Dominator and others added 3 commits July 27, 2023 09:14

Standardize #equals(Object) implementations

5e2e0b2

The style of the #equals(Object) implementation varies across the code base. Let's standardize it to improve the consistency of the code. Co-authored-by: Zhaoqi <[email protected]>

Add tests for #equals(Object) implementations

12f45fc

Eclipse-Dominator force-pushed the standardize-equal-formatting branch from 2b257b5 to ab8e0e8 Compare July 27, 2023 01:14

damithc approved these changes Jul 27, 2023

View reviewed changes

damithc merged commit e9f1ca7 into se-edu:master Aug 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize #equals(Object) formatting #197

Standardize #equals(Object) formatting #197

Eclipse-Dominator commented Jun 18, 2023

canihasreview bot commented Jun 18, 2023

codecov bot commented Jun 18, 2023 •

edited

Loading

canihasreview bot commented Jun 18, 2023

damithc left a comment •

edited

Loading

damithc Jun 20, 2023

damithc Jun 20, 2023

damithc Jun 20, 2023

canihasreview bot commented Jul 13, 2023

damithc commented Jul 13, 2023

Eclipse-Dominator commented Jul 13, 2023

damithc commented Jul 13, 2023

Eclipse-Dominator commented Jul 13, 2023

damithc left a comment

damithc Jul 13, 2023

damithc commented Jul 13, 2023

hansstanley left a comment •

edited

Loading

cjyothika left a comment

cjyothika Jul 14, 2023

SPWwj Jul 14, 2023

Eclipse-Dominator Jul 14, 2023

damithc commented Jul 14, 2023

SPWwj Jul 14, 2023

Eclipse-Dominator Jul 14, 2023 •

edited

Loading

SPWwj left a comment

Eclipse-Dominator commented Jul 14, 2023

canihasreview bot commented Jul 14, 2023

SPWwj Jul 15, 2023

damithc Jul 19, 2023

damithc Jul 27, 2023

Eclipse-Dominator Jul 27, 2023 •

edited

Loading

canihasreview bot commented Jul 27, 2023

		@@ -14,6 +14,9 @@
		public class NameContainsKeywordsPredicate implements Predicate<Person> {

		@@ -118,7 +118,6 @@ public class RemarkCommand extends Command {

Standardize #equals(Object) formatting #197

Standardize #equals(Object) formatting #197

Conversation

Eclipse-Dominator commented Jun 18, 2023

canihasreview bot commented Jun 18, 2023

codecov bot commented Jun 18, 2023 • edited Loading

Codecov Report

canihasreview bot commented Jun 18, 2023

v1

damithc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canihasreview bot commented Jul 13, 2023

v2

damithc commented Jul 13, 2023

Eclipse-Dominator commented Jul 13, 2023

damithc commented Jul 13, 2023

Eclipse-Dominator commented Jul 13, 2023

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc commented Jul 13, 2023

hansstanley left a comment • edited Loading

Choose a reason for hiding this comment

cjyothika left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc commented Jul 14, 2023

Choose a reason for hiding this comment

Eclipse-Dominator Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

SPWwj left a comment

Choose a reason for hiding this comment

Eclipse-Dominator commented Jul 14, 2023

canihasreview bot commented Jul 14, 2023

v3

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eclipse-Dominator Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

canihasreview bot commented Jul 27, 2023

v4

codecov bot commented Jun 18, 2023 •

edited

Loading

damithc left a comment •

edited

Loading

hansstanley left a comment •

edited

Loading

Eclipse-Dominator Jul 14, 2023 •

edited

Loading

Eclipse-Dominator Jul 27, 2023 •

edited

Loading