Addressbook::addTag() is not used #169 #179

PierceAndy · 2017-02-10T18:34:38Z

Fixes #169

Removed unused tag level methods in AddressBook:

addTag()
removeTag()
containsTag()

@yamgent @chao1995 With your unit test experiences, what are your thoughts on how tests that relied on containsTag() were handled?

mention-bot · 2017-02-10T18:34:39Z

@PierceAndy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @YijinL, @thenaesh and @yamgent to be potential reviewers.

CanIHasReview-bot · 2017-02-10T18:36:34Z

v1

@PierceAndy submitted v1 for review.

[v1 1/3] AddressBook: remove unused addTag()
[v1 2/3] AddressBook: remove unused removeTag()
[v1 3/3] AddressBook: remove unused containsTag()

(📚 Archive)

Checkout this PR version locally

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

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

yamgent

[v1 1/3]

commands that support the addition of Tags only; is not part of any
learning objectives; and AddressBook#syncTagsWithMasterList() already
handles the addition of Tags from a Person gracefully without using
exceptions.

Not a grammar guru, but it seems weird to use semi-colons here instead of commas
and a fullstop. Perhaps it is because of the missing subject and the use of and?

Maybe something like this might read a bit better:

commands that support the addition of Tags only. It is not part of any
learning objectives, and AddressBook#syncTagsWithMasterList()...

yamgent · 2017-02-11T01:26:49Z

src/seedu/addressbook/data/tag/UniqueTagList.java

-     *
-     * @throws DuplicateTagException if the Tag to add is a duplicate of an existing Tag in the list.
-     */
-    public void add(Tag toAdd) throws DuplicateTagException {


Nice spot, but this should probably be in a separate commit because [v1 1/3] is about AddressBook#addTag(), not UniqueTagList#add().

Also, not sure whether it is a good reason to remove UniqueTagList#add() just because AddressBook#addTag() no longer exist, since I can understand that you are not supposed to manipulate AddressBook's tags directly, but I don't expect UniqueTagList to no longer allow adding new tags as a result. @chao1995 Your thoughts on this?

AddressBook#addTag() is the only method that uses UniqueTagList#add(), and similarly so with remove in [v1 2/3]. Of course, I can understand the arguments for having a separate issue + PR for unused methods in UniqueTagList.

Actually I meant having a separate commit in this current PR, not a separate PR.

this should probably be in a separate commit

Agree.

whether it is a good reason to remove UniqueTagList#add() just because AddressBook#addTag() no longer exist

Since UniqueTagList#add() is never used as well. I would suggest we remove it.

yamgent

[v1 2/3]

yamgent · 2017-02-11T01:33:08Z

src/seedu/addressbook/data/tag/UniqueTagList.java

-     *
-     * @throws TagNotFoundException if no such Tag could be found in the list.
-     */
-    public void remove(Tag toRemove) throws TagNotFoundException {


Same concern as [v1 1/3].

yamgent

[v1 3/3]

yamgent · 2017-02-11T01:34:03Z

test/java/seedu/addressbook/data/AddressBookTest.java

-        assertFalse(defaultAddressBook.containsTag(tagEconomist));
-        assertFalse(defaultAddressBook.containsTag(tagPrizeWinner));
+        assertFalse(defaultAddressBook.getAllTags().contains(tagEconomist));
+        assertFalse(defaultAddressBook.getAllTags().contains(tagPrizeWinner));


@PierceAndy I assume that you are talking about this one in the OP of the PR.

Since containsTag() is only used in testing, and we are removing it, this alternative looks acceptable. However, it seems that we also have a method AddressBookTest#isTagObjectInAddressBookList() that also allow us to check the tags in the AddressBook (like how it used in line 100). Maybe consider using that instead?

Yes, it does make sense to use that and be consistent.

yamgent

Changes requested (as above)

EDIT: I edited my review regarding the commit message to make it more clear (and also removed an irrelevant part).

PierceAndy · 2017-02-11T02:21:19Z

@yamgent About the use of semi-colons, yes it's possible to use semi-colons in place of commas as a kind of "super-comma" when your list is complex. As to whether it's good practice, I guess I might reconsider it.

chao1995

No additional comment. Can proceed to next iteration.

CanIHasReview-bot · 2017-02-11T13:42:27Z

v2

@PierceAndy submitted v2 for review.

[v2 1/5] AddressBook: remove unused addTag()
[v2 2/5] UniqueTagList: remove unused add()
[v2 3/5] AddressBook: remove unused removeTag()
[v2 4/5] UniqueTagList: remove unused remove()
[v2 5/5] AddressBook: remove unused containsTag()

(📚 Archive) (📈 Interdiff between v1 and v2)

Checkout this PR version locally

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

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

PierceAndy · 2017-02-11T13:45:30Z

v2

Split UniqueTagList changes into their own commits
Replaced uses of AddressBook#getAllTags() + UniqueTagList#contains() in AddressBookTest with AddressBookTest#isTagObjectInAddressBookList()
Reworded commit messages based on feedback.

CanIHasReview-bot · 2017-02-12T07:23:34Z

v3

@PierceAndy submitted v3 for review.

[v3 1/5] AddressBook: remove unused addTag()
[v3 2/5] UniqueTagList: remove unused add()
[v3 3/5] AddressBook: remove unused removeTag()
[v3 4/5] UniqueTagList: remove unused remove()
[v3 5/5] AddressBook: remove unused containsTag()

(📚 Archive) (📈 Interdiff between v2 and v3)

Checkout this PR version locally

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

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

PierceAndy · 2017-02-12T07:28:18Z

v3

Updated commit messages to reflect updated standards for commit messages here: Formats: standardize commit message body format #53 oss-generic/process#60

@yamgent @chao1995

yamgent

Ok, they do look better with the new flow.

chao1995

LGTM.

PierceAndy · 2017-02-13T07:47:05Z

This PR has two approved peer reviews. Proceeding with next round of reviews with senior dev.
Attn: @damithc

damithc

Looks like tests are missing some verifications, but unrelated to this PR. Probably need to fix separately. Create an issue?

damithc · 2017-02-13T14:02:25Z

test/java/seedu/addressbook/data/AddressBookTest.java

        defaultAddressBook.addPerson(davidElliot);
-        assertTrue(defaultAddressBook.containsTag(tagEconomist));
-        assertTrue(defaultAddressBook.containsTag(tagPrizeWinner));
-


Noticed something odd here. The test does not confirm that the person is added correctly.

pyokagan · 2017-02-14T07:46:56Z

Just a side note: I just realised that AddressBookTest#isTagObjectInAddressBookList() isn't actually a 1-to-1 replacement of AddressBook#containsTag(). The former checks for equal references while the latter checks for equal values.

damithc · 2017-02-14T08:05:43Z

Just a side note: I just realised that AddressBookTest#isTagObjectInAddressBookList() isn't actually a 1-to-1 replacement of AddressBook#containsTag(). The former checks for equal references while the latter checks for equal values.

Oh yes. That was why the method was added in the first place (to confirm the Person object points to Tag objects in the common tag list instead of keeping it's own copies of the tags). Probably need better method name and better header comment to emphasize this 'special' nature of the method. That also means the method is specific to testing and it has to exist with or without AddressBook#containsTag().

pyokagan · 2017-02-14T14:00:33Z

@damithc

That also means the method is specific to testing and it has to exist with or without AddressBook#containsTag().

Yes, agreed. In light of this new ~~discovery~~ information (which should be emphasized in the commit message), I agree that AddressBookTest#isTagObjectInAddressBookList() should stay inside the test code, and that we can remove AddressBook#containsTag(). (The alternative would be to make AddressBook#containsTag() compare by reference to fully take over the responsibilities of AddressBookTest#isTagObjectInAddressBookList(), but that would be hugely inconsistent with our API design.)

pyokagan

[v3 5/5]

AddressBook: remove unused containsTag()

AddressBook#containsTag() checks if a given Tag is in the master list of
Tags.

This method is not used anywhere, and can be classified as unused code.
There are no commands that support it, and it is also not part of any
learning objectives. While it is used in two tests for
AddressBook#addPerson(), it should be replaced with the existing
AddressBookTest#isTagObjectInAddressBookList() to improve code reuse.

So this is misleading -- we aren't actually reusing code, since
AddressBookTest#isTagObjectInAddressBookList() isn't an equivalent
replacement for AddressBook#addPerson().

AddressBook#containsTag() should be removed as unused code contributes
to maintenance and comprehension overheads.

Let's remove AddressBook#containsTag().

AddressBook#addTag() adds a Tag to the master list of Tags in AddressBook. This method is not used anywhere, and can be classified as unused code. There are no commands that support the addition of Tags only, and it is also not part of any learning objectives. Furthermore, AddressBook#syncTagsWithMasterList() already handles the addition of Tags from a Person gracefully without using exceptions. It should be removed as unused code contributes to maintenance and comprehension overheads. Let's remove AddressBook#addTag().

UniqueTagList#add() adds a Tag to the list of unique Tags, and throws an exception if it is a duplicate of an existing Tag in the list. With the removal of AddressBook#addTag(), this method is not used anywhere, and can be classified as unused code. It should be removed as unused code contributes to maintenance and comprehension overheads. Let's remove UniqueTagList#add().

AddressBook#removeTag() removes a Tag from the master list of Tags and from any Person in AddressBook. This method is not used anywhere, and can be classified as unused code. There are no commands that support the removal of Tags only, and it is also not part of any learning objectives. It should be removed as unused code contributes to maintenance and comprehension overheads. Let's remove AddressBook#removeTag().

UniqueTagList#remove() removes a Tag from the list of unique Tags, and throws an exception if it is not found in the list. With the removal of AddressBook#removeTag(), this method is not used anywhere, and can be classified as unused code. It should be removed as unused code contributes to maintenance and comprehension overheads. Let's remove UniqueTagList#remove().

CanIHasReview-bot · 2017-02-15T14:30:36Z

v4

@PierceAndy submitted v4 for review.

[v4 1/5] AddressBook: remove unused addTag()
[v4 2/5] UniqueTagList: remove unused add()
[v4 3/5] AddressBook: remove unused removeTag()
[v4 4/5] UniqueTagList: remove unused remove()
[v4 5/5] AddressBook: remove unused containsTag()

(📚 Archive) (📈 Interdiff between v3 and v4)

Checkout this PR version locally

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

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

PierceAndy · 2017-02-15T14:33:00Z

v4

Created issue for missing verification in AddressBookTest Unit tests in AddressBookTest needs improvement #182
Updated [v3 5/5] commit message

AddressBook: remove unused containsTag()

AddressBook#containsTag() checks if the master list of Tags contains a
Tag with the same value as the given Tag.

This method is not used anywhere, and can be classified as unused code.
There are no commands that support it, and it is also not part of any
learning objectives. While it is used in tests, its functionality is
superseded by AddressBookTest#isTagObjectInAddressBookList(), and can be
replaced by the latter in these tests. AddressBook#containsTag() should
be removed as unused code contributes to maintenance and comprehension
overheads.

Let's remove AddressBook#containsTag().

ddressBookTest#isTagObjectInAddressBookList() is chosen over
AddressBook#containsTag() because the former supersedes the latter in
functionalities, and the former has to exist with or without the latter
as the former contains test-specific checks.
AddressBookTest#isTagObjectInAddressBookList() checks if the master list
of Tags from a given AddressBook contains the given Tag object by
reference, thus not only checking if a Tag value is in the master list
like AddressBook#containsTag() does, but also checks that Person objects
points to Tag objects in the master Tag list instead of keeping its own
copies of Tags. The alternative of updating AddressBook#containsTag() to
compare by reference to take over the responsibilities of
AddressBookTest#isTagObjectInAddressBookList() is inconsistent with our
API design, as it would mean adding a method used only in tests to
production code.

damithc · 2017-02-15T15:12:30Z

Tried to shorten the commit message. Does it cover all the points?

AddressBook#containsTag() is not used in the production code.
While it is used in test code, its use can be replaced by 
AddressBookTest#isTagObjectInAddressBookList().

Let's remove AddressBook#containsTag().

Notes: 
* Why not keep AddressBook#containsTag() and remove 
AddressBookTest#isTagObjectInAddressBookList() instead?
The former does a value equality test while the latter goes further
and does a reference equality test. The test code requires a 
reference equality test to verify Person objects refer to Tag
objects in the common tag list instead of keeping its own copies
of Tag objects. 
* Why not change AddressBook#containsTag() to use reference
equality and use that in tests?
Doing so will make AddressBook#containsTag()'s semantics 
inconsistent with the rest of our API.

AddressBook#containsTag() checks if the master list of Tags contains a Tag with the same value as the given Tag. AddressBook#containsTag() is not used in the production code. While it is used in test code, its use can be replaced by AddressBookTest#isTagObjectInAddressBookList(). Let's remove AddressBook#containsTag(). Notes: * Why not keep AddressBook#containsTag() and remove AddressBookTest#isTagObjectInAddressBookList() instead? The former does a value equality test while the latter goes further and does a reference equality test. The test code requires a reference equality test to verify Person objects refer to Tag objects in the common tag list instead of keeping its own copies of Tag objects. * Why not change AddressBook#containsTag() to use reference equality and use that in tests? Doing so will make AddressBook#containsTag()'s semantics inconsistent with the rest of our API.

CanIHasReview-bot · 2017-02-16T07:49:20Z

v5

@PierceAndy submitted v5 for review.

[v5 1/5] AddressBook: remove unused addTag()
[v5 2/5] UniqueTagList: remove unused add()
[v5 3/5] AddressBook: remove unused removeTag()
[v5 4/5] UniqueTagList: remove unused remove()
[v5 5/5] AddressBook: remove unused containsTag()

(📚 Archive) (📈 Interdiff between v4 and v5)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level2.git refs/pr/179/5/head:BRANCHNAME

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

PierceAndy · 2017-02-16T07:49:57Z

v5

Shortened [v4 5/5] commit message

PierceAndy self-assigned this Feb 10, 2017

PierceAndy requested review from yamgent and chao1995 February 10, 2017 18:34

PierceAndy force-pushed the 169-unused-tag-methods branch from b81b6c8 to 033e8be Compare February 10, 2017 18:36

CanIHasReview-bot added the s.ToReview label Feb 10, 2017

yamgent reviewed Feb 11, 2017

View reviewed changes

yamgent requested changes Feb 11, 2017

View reviewed changes

chao1995 reviewed Feb 11, 2017

View reviewed changes

chao1995 added s.Ongoing and removed s.ToReview labels Feb 11, 2017

PierceAndy force-pushed the 169-unused-tag-methods branch from 033e8be to ecb4d74 Compare February 11, 2017 13:41

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Feb 11, 2017

CanIHasReview-bot requested a review from yamgent February 11, 2017 13:42

yamgent approved these changes Feb 12, 2017

View reviewed changes

PierceAndy force-pushed the 169-unused-tag-methods branch from ecb4d74 to a5ed663 Compare February 12, 2017 07:22

yamgent approved these changes Feb 12, 2017

View reviewed changes

PierceAndy requested a review from chao1995 February 12, 2017 09:58

chao1995 approved these changes Feb 13, 2017

View reviewed changes

damithc approved these changes Feb 13, 2017

View reviewed changes

pyokagan requested changes Feb 14, 2017

View reviewed changes

PierceAndy added 4 commits February 15, 2017 21:54

PierceAndy force-pushed the 169-unused-tag-methods branch 2 times, most recently from 022c23d to 4cfe9a5 Compare February 15, 2017 14:18

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Feb 15, 2017

CanIHasReview-bot requested a review from pyokagan February 15, 2017 14:30

PierceAndy force-pushed the 169-unused-tag-methods branch from 4cfe9a5 to a6cc952 Compare February 16, 2017 07:48

pyokagan approved these changes Feb 19, 2017

View reviewed changes

pyokagan added s.ToMerge and removed s.ToReview labels Feb 19, 2017

pyokagan requested review from damithc, yamgent and chao1995 February 19, 2017 07:14

yamgent approved these changes Feb 19, 2017

View reviewed changes

damithc approved these changes Feb 19, 2017

View reviewed changes

chao1995 approved these changes Feb 19, 2017

View reviewed changes

damithc merged commit 4f5b987 into se-edu:master Feb 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addressbook::addTag() is not used #169 #179

Addressbook::addTag() is not used #169 #179

PierceAndy commented Feb 10, 2017 •

edited

Loading

mention-bot commented Feb 10, 2017

CanIHasReview-bot commented Feb 10, 2017

yamgent left a comment •

edited

Loading

yamgent Feb 11, 2017

PierceAndy Feb 11, 2017

yamgent Feb 11, 2017

chao1995 Feb 11, 2017

yamgent left a comment

yamgent Feb 11, 2017

yamgent left a comment

yamgent Feb 11, 2017

PierceAndy Feb 11, 2017

yamgent left a comment •

edited

Loading

PierceAndy commented Feb 11, 2017

chao1995 left a comment

CanIHasReview-bot commented Feb 11, 2017

PierceAndy commented Feb 11, 2017

CanIHasReview-bot commented Feb 12, 2017

PierceAndy commented Feb 12, 2017

yamgent left a comment

chao1995 left a comment

PierceAndy commented Feb 13, 2017

damithc left a comment

damithc Feb 13, 2017

pyokagan commented Feb 14, 2017

damithc commented Feb 14, 2017

pyokagan commented Feb 14, 2017 •

edited

Loading

pyokagan left a comment

CanIHasReview-bot commented Feb 15, 2017

PierceAndy commented Feb 15, 2017

damithc commented Feb 15, 2017

CanIHasReview-bot commented Feb 16, 2017

PierceAndy commented Feb 16, 2017 •

edited

Loading

Addressbook::addTag() is not used #169 #179

Addressbook::addTag() is not used #169 #179

Conversation

PierceAndy commented Feb 10, 2017 • edited Loading

mention-bot commented Feb 10, 2017

CanIHasReview-bot commented Feb 10, 2017

v1

yamgent 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

Choose a reason for hiding this comment

yamgent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamgent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamgent left a comment • edited Loading

Choose a reason for hiding this comment

PierceAndy commented Feb 11, 2017

chao1995 left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Feb 11, 2017

v2

PierceAndy commented Feb 11, 2017

v2

CanIHasReview-bot commented Feb 12, 2017

v3

PierceAndy commented Feb 12, 2017

v3

yamgent left a comment

Choose a reason for hiding this comment

chao1995 left a comment

Choose a reason for hiding this comment

PierceAndy commented Feb 13, 2017

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyokagan commented Feb 14, 2017

damithc commented Feb 14, 2017

pyokagan commented Feb 14, 2017 • edited Loading

pyokagan left a comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Feb 15, 2017

v4

PierceAndy commented Feb 15, 2017

v4

damithc commented Feb 15, 2017

CanIHasReview-bot commented Feb 16, 2017

v5

PierceAndy commented Feb 16, 2017 • edited Loading

v5

PierceAndy commented Feb 10, 2017 •

edited

Loading

yamgent left a comment •

edited

Loading

yamgent left a comment •

edited

Loading

pyokagan commented Feb 14, 2017 •

edited

Loading

PierceAndy commented Feb 16, 2017 •

edited

Loading