Standardize #equals(Object) formatting

[Eclipse-Dominator]

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[bot]]

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

See this repository's contribution guide for more information.

[codecov[bot]]

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%)	:arrow_down:
...a/seedu/address/model/person/UniquePersonList.java	86.95% <33.33%> (-5.91%)	:arrow_down:
src/main/java/seedu/address/model/tag/Tag.java	71.42% <33.33%> (-8.58%)	:arrow_down:
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%)	:arrow_up:
...n/java/seedu/address/commons/core/index/Index.java	100.00% <100.00%> (ø)
... and 12 more

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

[canihasreview[bot]]

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

(:books: Archive)

Checkout this PR version locally

```sh 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.

[canihasreview[bot]]

v2

@Eclipse-Dominator submitted v2 for review.

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

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v1 and v2) (:chart_with_upwards_trend: Range-Diff between v1 and v2)

Checkout this PR version locally

```sh 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]

@Eclipse-Dominator how does V2 differ from https://github.com/se-edu/addressbook-level4/pull/973/ ?

[Eclipse-Dominator]

@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]

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]

@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]

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

[damithc]

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).

[Eclipse-Dominator]

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[bot]]

v3

@Eclipse-Dominator submitted v3 for review.

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

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v2 and v3) (:chart_with_upwards_trend: Range-Diff between v2 and v3)

Checkout this PR version locally

```sh 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.

[canihasreview[bot]]

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)

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v3 and v4) (:chart_with_upwards_trend: Range-Diff between v3 and v4)

Checkout this PR version locally

```sh 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.