Update checking for Duplicates in EditCommand

[CodyChew]

fixes #57

Let's

• use a separate Model with only the relevant Persons to check for Duplicates.
• include test cases for EditCommand

The check for duplicate Persons comes from the isSamePerson method in Person class which checks for the same name and having at least one other identity field(Phone or Email) being the same. To reuse the checking mechanism from the Model, a simple way would be to remove the personToEdit from a model and have a separate check.

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

[damithc]

@CodyChew thanks for the fix. Did you also check the info given in https://github.com/se-edu/addressbook-level3/issues/57#issuecomment-739196585 ?

[CodyChew]

@damithc I just checked it out. I thought of it as the checking at the Command being problematic rather than the equality check in Person. I extracted out the checking to only the relevant model and do not have to deal with the transitive property of isSamePerson. Since the checking is only done at one place, it's less prone to bugs and isSamePerson can still be customised.

This check is similar to addCommand where it checks the person to add against the model before adding it, however it requires the creation of an additional model for the check within editCommand.

[codecov-io]

Codecov Report

Merging #68 (bd13fe5) into master (96954ff) will increase coverage by 0.04%. The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master      #68      +/-   ##
============================================
+ Coverage     72.18%   72.22%   +0.04%     
+ Complexity      401      400       -1     
============================================
  Files            70       70              
  Lines          1233     1235       +2     
  Branches        126      126              
============================================
+ Hits            890      892       +2     
  Misses          311      311              
  Partials         32       32              

Impacted Files	Coverage Δ	Complexity Δ
...java/seedu/address/logic/commands/EditCommand.java	97.10% <100.00%> (+0.08%)	11.00 <0.00> (-1.00)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 96954ff...bd13fe5. Read the comment docs.

[canihasreview[bot]]

v1

@CodyChew submitted v1 for review.

• [v1 1/1] Update checking for Duplicates in EditCommand

(:books: Archive)

Checkout this PR version locally

```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/68/1/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.

[damithc]

@fzdy1914 can take a look at this PR?

[damithc]

I am not so sure adding another model is elegant enough. Basically, it requires duplicating the whole data, which is usually unacceptable in a larger project.

There are two ways possible, one is to add another method for checking duplicate person in Addressbook. Second is to delete the person first, do the checking, restore the person if some error happens.

@damithc Prof, you may decide which solution is better.

69 is using the method 1.

Yes, duplicating the entire model seems excessive. We can try one of the other two methods if they are not a lot more complicated than the one used in this PR. But I don't mind an inelegant/inefficient solution if the implementation is a lot simpler and the performance cost is not prohibitive.

@CodyChew what do you think about the other two solutions?

[CodyChew]

There are two ways possible, one is to add another method for checking duplicate person in Addressbook. Second is to delete the person first, do the checking, restore the person if some error happens.

I prefer the second approach since we would still be able to use the same isSamePerson method without introducing different/additional methods for checking. The check will be simpler and it will be less prone to bugs since the check for addCommand and editCommand will be similar.

[damithc]

I prefer the second approach since we would still be able to use the same isSamePerson method without introducing different/additional methods for checking. The check will be simpler and it will be less prone to bugs since the check for addCommand and editCommand will be similar.

Noted. Perhaps you can submit a separate PR using that approach so that we can compare all three?

[CodyChew]

I have made a PR using the second approach, one main issue I came across was that adding and deleting a Person using the current commands would result in different internal lists due to ordering. I have tried to work around it by extending the functionality of the model, please refer to #74 .

[damithc]

@CodyChew thanks for the fix. Did you also check the info given in #57 (comment) ?

I had another look at this comment. Seems the recommendation was to simplify the isSamePerson(). Shouldn't that be simpler than any of the alternatives considered here?

[CodyChew]

Yes, simplifying the method would resolve the issue too. However if we want to keep the current definition of isSamePerson() and allow students to customize their own method behavior, changing the way we check in editCommand would make it independent of the nature of isSamePerson() and reduce such bugs.

Definition of isSamePerson(): * Returns true if both persons of the same name have at least one other identity field that is the same. * This defines a weaker notion of equality between two persons.

[damithc]

Yes, simplifying the method would resolve the issue too. However if we want to keep the current definition of isSamePerson() and allow students to customize their own method behavior, changing the way we check in editCommand would make it independent of the nature of isSamePerson() and reduce such bugs.

Definition of isSamePerson(): * Returns true if both persons of the same name have at least one other identity field that is the same. * This defines a weaker notion of equality between two persons.

I agree that improving the check makes the code more flexible. However, if we use a more sophisticated equality check that students might not even need in the end, it can add an unnecessary bump to the learning curve, especially for weaker students. Therefore, I'm inclined to go with the simpler solution of simplifying the isSamePerson(). Those who need a more complex equality can add one at their own expense. We can add a TODO comment in the code to mention that a more sophisticated equality check can be considered for future versions. It will be a good exercise for students to figure out the implications of such a non-transitive equality relation, uncover this potential bug, and fix it. What do you think?

[CodyChew]

I agree that if we make isSamePerson() to only check the name, it will be easier for them to understand and expand on the method. They would then have to change the guard condition themselves if they encounter this bug should they make isSamePerson() more complex.

Should I make another PR for this solution? Some of the test cases and default Person in PersonBuilder would need to be changed, now that there cannot be Persons of the same name.

[damithc]

Should I make another PR for this solution? Some of the test cases and default Person in PersonBuilder would need to be changed, now that there cannot be Persons of the same name.

Yes, go ahead @CodyChew On a related note, whichever PR we merge, we should give the other 3 as references in the commit message.

[damithc]

Closing in favor of #75