AddressBook: remove master tag list #383 #385

yamidark · 2018-08-06T19:05:26Z

Fixes #383

We keep track of a master tag list which holds all the tags in
AddressBook.

As the master tag list was deemed unnecessary and may be too
complex for new developers to grasp, as discussed in
se-edu/addressbook-level4#753 and se-edu/addressbook-level4#794,
the master tag list has already been removed in AB4
by se-edu/addressbook-level4#825. 

UniqueTagList was also deemed unnecessary and removed in
addressbook-level4 after the master tag list is removed.

As such, the master tag list and UniqueTagList class should also be
removed in the lower level projects.

Let's remove the master tag list in AddressBook and AdaptedAddressBook,
as well as the UniqueTagList class.

CanIHasReview-bot · 2018-08-06T19:05:27Z

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

See this repository's contribution guide for more information.

CanIHasReview-bot · 2018-08-06T19:11:50Z

v1

@yamidark submitted v1 for review.

[v1 1/4] AddressBook: remove master tag list
[v1 2/4] AdaptedAddressBook: remove master tag list
[v1 3/4] Remove UniqueTagList class
[v1 4/4] docs: update component on UniqueTagList

(📚 Archive)

Checkout this PR version locally

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

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

eugenepeh

[2/4] imports were not removed.

eugenepeh

duplicate, please ignore

CanIHasReview-bot · 2018-08-07T11:16:56Z

v2

@yamidark submitted v2 for review.

[v2 1/4] AddressBook: remove master tag list
[v2 2/4] AdaptedAddressBook: remove master tag list
[v2 3/4] Remove UniqueTagList class
[v2 4/4] docs: update component on UniqueTagList

(📚 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-level2.git refs/pr/385/2/head:BRANCHNAME

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

eugenepeh · 2018-08-07T11:31:30Z

test/java/seedu/addressbook/commands/AddCommandTest.java

@@ -26,7 +26,7 @@

 public class AddCommandTest {
    private static final List<ReadOnlyPerson> EMPTY_PERSON_LIST = Collections.emptyList();
-    private static final Set<String> EMPTY_STRING_LIST = Collections.emptySet();
+    private static final Set<String> EMPTY_STRING_LIST = new HashSet<>();


[3/4] is this change necessary? i think the intention was to be immutable.

Hmm, I think I did a mass replacement for this, didn't notice it changed here as well.
Anyway, since the Person constructor now directly sets the tags, thought it would be safer to ensure that the Set we supply is mutable from the start.
Perhaps would be better if we make Person hold its own mutable copy Set from the start, and make it add the Tags in, like in UniqueTagList?

pyokagan · 2018-08-07T13:55:34Z

@yamidark Can squash [1/4] and [2/4]? [1/4] makes no sense without [2/4], as [2/4] deletes huge chunks of code that were touched in [1/4].

I'm not quite sure about the deletions of some test cases in AddressBookTest, like addPerson_emptyAddressBook, but I also don't quite mind given the time constraints.

btw, [1/4]'s commit message:

AddressBook: remove master tag list

We keep track of a master tag list which holds all the tags in
AddressBook.

As the master tag list was deemed unnecessary and may be too complex
for new developers to grasp, as discussed in
se-edu/addressbook-level4#753 and se-edu/addressbook-level4#794,
the master tag list has already been removed in addressbook-level4
in se-edu/addressbook-level4#824. As such, the master tag list

Should be se-edu/addressbook-level4#825 I think.

should also be removed here for consistency.

Let's remove the master tag list in AddressBook.

pyokagan

[3/4]

pyokagan · 2018-08-07T13:56:23Z

src/seedu/addressbook/data/person/Person.java

        this.name = name;
        this.phone = phone;
        this.email = email;
        this.address = address;
-        this.tags = new UniqueTagList(tags); // protect internal tags from changes in the arg list
+        this.tags = tags;


Should be:

this.tags = new HashSet<>(tags);

so that the Person will have a Set which it has full ownership of, and can't be modified externally (preserves encapsulation).

Can copy from AB-4:

https://github.com/se-edu/addressbook-level4/blob/e186102b646d2307b53928142dd22932a6f4c86d/src/main/java/seedu/address/model/person/Person.java#L25-L37

pyokagan · 2018-08-07T13:57:12Z

test/java/seedu/addressbook/commands/DeleteCommandTest.java

@@ -33,14 +33,14 @@
    @Before
    public void setUp() throws Exception {
        Person johnDoe = new Person(new Name("John Doe"), new Phone("61234567", false),
-                new Email("[email protected]", false), new Address("395C Ben Road", false), new UniqueTagList());
+                new Email("[email protected]", false), new Address("395C Ben Road", false), new HashSet<>());


Does the new constructor you introduced help here?

public Person(Name name, Phone phone, Email email, Address address, Tag... tags)

On second thought, I think you should remove the constructor. AB-4 doesn't have it.

The new constructor was added to replace the UniqueTagList constructor:

public UniqueTagList(Tag... tags)

which AB4 doesn't have.
Although this is only used in tests, so I wouldn't really mind removing it as well to be consistent with AB4.

pyokagan · 2018-08-07T13:57:42Z

test/java/seedu/addressbook/util/TypicalPersons.java

            dan = new Person(new Name("Dan Smith"), new Phone("1234556", true), new Email("[email protected]", true),
-                    new Address("NUS", true), new UniqueTagList(new Tag("Test")));
+                    new Address("NUS", true), new HashSet<>());


Why was the new Tag("Test") removed?

Whoops, didn't notice this

pyokagan

[4/4]

mainClassDiagram.png:

AB-4 puts the Person->Tag arrow as part of the filled diamond (composition), so it should probably be the same to be consistent. (I assume @damithc vetted AB-4's version already)

If that is so, [LO-Composition] should be updated to include Tag in the example of composition (filled diamond) as well.

pyokagan

Changes Requested.

pyokagan · 2018-08-07T14:06:21Z

This wasn't very fun to review due to all the subtle differences between the different ABs. </rant>

We keep track of a master tag list which holds all the tags in AddressBook. As the master tag list was deemed unnecessary and may be too complex for new developers to grasp, as discussed in se-edu/addressbook-level4#753 and se-edu/addressbook-level4#794, the master tag list has already been removed in addressbook-level4 in se-edu/addressbook-level4#825. As such, the master tag list should also be removed here for consistency. Let's remove the master tag list in AddressBook. Similarly, it is now unnecessary for AdaptedAddressBook to store a master tag list as well. Let's remove the master tag list in AdaptedAddressBook.

Many methods in UniqueTagList, such as mergeFrom(UniqueTagList), were previously used by only the AddressBook for the master tag list. Since AddressBook does not use a master tag list anymore, all these methods are no longer necessary. UniqueTagList now only serves to add complexity to the code base. Let's remove the UniqueTagList class, and update Person to use a Set instead for holding its tags instead, since it can only contain unique elements.

CanIHasReview-bot · 2018-08-07T16:29:00Z

v3

@yamidark submitted v3 for review.

[v3 1/4] AddressBook: remove master tag list
[v3 2/4] Remove UniqueTagList class
[v3 3/4] Tag: move Tag from data/tag to data/person package
[v3 4/4] docs: update component on UniqueTagList

(📚 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-level2.git refs/pr/385/3/head:BRANCHNAME

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

eugenepeh

eh... in [4/4] Exercise: Add an Association Class Tagging how come tag used a special purple color?

eugenepeh · 2018-08-07T16:46:33Z

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


        davidElliot    = new Person(new Name("David Elliot"),
                                    new Phone("84512575", false),
                                    new Email("[email protected]", false),
                                    new Address("11 Arts Link", false),
-                                    new UniqueTagList(tagEconomist, tagPrizeWinner));
+                                    new HashSet<>(Arrays.asList(tagEconomist, tagPrizeWinner)));


[2/4] If want to make it immutable, Set.of(...) would probably be a better option.

This is currently still in Java 8, so Set.of(...) is still not available yet 😞.

oh... crap lol

yamidark · 2018-08-07T16:55:24Z

eh... how come tag used a special purple color?

Tag is now strongly part of Person, so should have the same colour as the rest of the fields.
Although not too sure if should keep the original colour for the LO Taggings =x (yep, looks weird with that single purple box inside).

eugenepeh · 2018-08-07T17:10:56Z

Tag is now strongly part of Person, so should have the same colour as the rest of the fields.
Although not too sure if should keep the original colour for the LO Taggings =x (yep, looks weird with that single purple box inside).

I thought it was originally orange?!

yamidark · 2018-08-07T17:14:57Z

I thought it was originally orange?!

Yep, I changed it to purple to be consistent with the rest of the Person fields (in DeveloperGuide).

Oh, just noticed that DeveloperGuide wasn't updated in this PR at all, just changed the diagram, since you probably didn't notice it.

Updated diagram here

eugenepeh · 2018-08-07T17:36:32Z

Oh, just noticed that DeveloperGuide wasn't updated in this PR at all, just changed the diagram, since you probably didn't notice it.

Thanks, didnt notice that.

In this case... I think this line Note how the setTags method of the Person class cannot be converted to a class-level method. in LO is obsolete?

yamidark · 2018-08-07T17:43:43Z

Note how the setTags method of the Person class cannot be converted to a class-level method.

Ehh, as the method is modifying the instance-level variable tags, the setTags method still cannot be converted to class-level right?

eugenepeh · 2018-08-07T18:15:23Z

Ehh, as the method is modifying the instance-level variable tags, the setTags method still cannot be converted to class-level right?

PS, I actually mean that that line doesn't really serve a purpose anymore because prior to this change, it was distinct from the other members because we had a master list and Tags was not part of Person. But now it is no different from name... address... etc. So it brings up question like whats so special about setTags?

Perhaps, we should rephrase it abit to like For example, instead of Note?

yamidark · 2018-08-07T18:28:21Z

Perhaps, we should rephrase it abit to like For example, instead of Note?

Yep, sounds good. Will do that in the next iteration.
Maybe like this:
Note how some instance-level methods, such as the setTags methods of the Person class, cannot be converted to a class-level method.

CanIHasReview-bot · 2018-08-08T06:36:53Z

v4

@yamidark submitted v4 for review.

[v4 1/4] AddressBook: remove master tag list
[v4 2/4] Remove UniqueTagList class
[v4 3/4] Tag: move Tag from data/tag to data/person package
[v4 4/4] docs: update component on UniqueTagList

(📚 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-level2.git refs/pr/385/4/head:BRANCHNAME

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

pyokagan · 2018-08-08T07:43:59Z

[3/4]

Tag: move Tag from data/tag to data/person package

AddressBook used to have a master tag list, which stores
the tag held by each Person.

As the master tag list has been removed, Tag is now strongly
part of Person. As such, Tag should be under data/person
package as well.

Maybe, but how does this weigh against keeping consistent with AB-4 (which does not have such a change?)

Let's refrain from making too many creative changes and drop [3/4]? We are pretty short of time.

AddressBook previously had a UniqueTagList linked to it as the master tag list, which was reflected in the architecture diagrams in the DeveloperGuide and LearningOutcomes. Since AddressBook does not use a master tag list anymore, and UniqueTagList has been removed, these architecture diagrams are no longer consistent with the program. Let's update these architecture diagrams in the DeveloperGuide and LearningOutcomes.

CanIHasReview-bot · 2018-08-08T08:34:10Z

v5

@yamidark submitted v5 for review.

[v5 1/3] AddressBook: remove master tag list
[v5 2/3] Remove UniqueTagList class
[v5 3/3] docs: update component on UniqueTagList

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

Checkout this PR version locally

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

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

yamidark force-pushed the 383-remove-master-tag-list branch from 3bda739 to 891108c Compare August 6, 2018 19:10

CanIHasReview-bot added the s.ToReview label Aug 6, 2018

yamidark requested a review from a team August 6, 2018 19:12

eugenepeh requested changes Aug 7, 2018

View reviewed changes

yamidark force-pushed the 383-remove-master-tag-list branch from 891108c to 48ab4cf Compare August 7, 2018 09:39

pyokagan added s.Ongoing and removed s.ToReview labels Aug 7, 2018

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Aug 7, 2018

CanIHasReview-bot requested a review from eugenepeh August 7, 2018 11:16

eugenepeh reviewed Aug 7, 2018

View reviewed changes

pyokagan reviewed Aug 7, 2018

View reviewed changes

pyokagan requested changes Aug 7, 2018

View reviewed changes

yamidark force-pushed the 383-remove-master-tag-list branch 5 times, most recently from be4a8db to 23672d5 Compare August 7, 2018 16:11

yamidark added 2 commits August 8, 2018 00:14

yamidark force-pushed the 383-remove-master-tag-list branch 3 times, most recently from 06d8367 to 55f8424 Compare August 7, 2018 16:19

CanIHasReview-bot requested review from eugenepeh and pyokagan August 7, 2018 16:29

eugenepeh reviewed Aug 7, 2018

View reviewed changes

yamidark force-pushed the 383-remove-master-tag-list branch from 55f8424 to 7bf1433 Compare August 8, 2018 06:25

CanIHasReview-bot requested a review from eugenepeh August 8, 2018 06:36

yamidark force-pushed the 383-remove-master-tag-list branch from 7bf1433 to 756dccc Compare August 8, 2018 08:13

pyokagan approved these changes Aug 8, 2018

View reviewed changes

yamidark requested a review from damithc August 8, 2018 09:50

damithc approved these changes Aug 8, 2018

View reviewed changes

eugenepeh approved these changes Aug 8, 2018

View reviewed changes

eugenepeh added s.ToMerge and removed s.ToReview labels Aug 8, 2018

pyokagan merged commit ba89e36 into se-edu:master Aug 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AddressBook: remove master tag list #383 #385

AddressBook: remove master tag list #383 #385

yamidark commented Aug 6, 2018 •

edited

Loading

CanIHasReview-bot commented Aug 6, 2018

CanIHasReview-bot commented Aug 6, 2018

eugenepeh left a comment •

edited

Loading

eugenepeh left a comment •

edited

Loading

CanIHasReview-bot commented Aug 7, 2018

eugenepeh Aug 7, 2018 •

edited

Loading

yamidark Aug 7, 2018

pyokagan commented Aug 7, 2018

pyokagan left a comment

pyokagan Aug 7, 2018

pyokagan Aug 7, 2018

pyokagan Aug 7, 2018

yamidark Aug 7, 2018

pyokagan Aug 7, 2018

yamidark Aug 7, 2018

pyokagan left a comment

pyokagan left a comment

pyokagan commented Aug 7, 2018

CanIHasReview-bot commented Aug 7, 2018

eugenepeh left a comment •

edited

Loading

eugenepeh Aug 7, 2018

yamidark Aug 7, 2018

eugenepeh Aug 7, 2018

yamidark commented Aug 7, 2018 •

edited

Loading

eugenepeh commented Aug 7, 2018

yamidark commented Aug 7, 2018 •

edited

Loading

eugenepeh commented Aug 7, 2018

yamidark commented Aug 7, 2018

eugenepeh commented Aug 7, 2018 •

edited

Loading

yamidark commented Aug 7, 2018 •

edited

Loading

CanIHasReview-bot commented Aug 8, 2018

pyokagan commented Aug 8, 2018

CanIHasReview-bot commented Aug 8, 2018

AddressBook: remove master tag list #383 #385

AddressBook: remove master tag list #383 #385

Conversation

yamidark commented Aug 6, 2018 • edited Loading

CanIHasReview-bot commented Aug 6, 2018

CanIHasReview-bot commented Aug 6, 2018

v1

eugenepeh left a comment • edited Loading

Choose a reason for hiding this comment

eugenepeh left a comment • edited Loading

Choose a reason for hiding this comment

CanIHasReview-bot commented Aug 7, 2018

v2

eugenepeh Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyokagan commented Aug 7, 2018

pyokagan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyokagan left a comment

Choose a reason for hiding this comment

pyokagan left a comment

Choose a reason for hiding this comment

pyokagan commented Aug 7, 2018

CanIHasReview-bot commented Aug 7, 2018

v3

eugenepeh 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

yamidark commented Aug 7, 2018 • edited Loading

eugenepeh commented Aug 7, 2018

yamidark commented Aug 7, 2018 • edited Loading

eugenepeh commented Aug 7, 2018

yamidark commented Aug 7, 2018

eugenepeh commented Aug 7, 2018 • edited Loading

yamidark commented Aug 7, 2018 • edited Loading

CanIHasReview-bot commented Aug 8, 2018

v4

pyokagan commented Aug 8, 2018

CanIHasReview-bot commented Aug 8, 2018

v5

yamidark commented Aug 6, 2018 •

edited

Loading

eugenepeh left a comment •

edited

Loading

eugenepeh left a comment •

edited

Loading

eugenepeh Aug 7, 2018 •

edited

Loading

eugenepeh left a comment •

edited

Loading

yamidark commented Aug 7, 2018 •

edited

Loading

yamidark commented Aug 7, 2018 •

edited

Loading

eugenepeh commented Aug 7, 2018 •

edited

Loading

yamidark commented Aug 7, 2018 •

edited

Loading