Skip to content
This repository has been archived by the owner on Mar 5, 2023. It is now read-only.

AddressBook needs to track tags properly #753

Closed
pyokagan opened this issue Dec 4, 2017 · 6 comments · Fixed by #825
Closed

AddressBook needs to track tags properly #753

pyokagan opened this issue Dec 4, 2017 · 6 comments · Fixed by #825
Labels
a-Model d.Committers p.Medium

Comments

@pyokagan
Copy link
Contributor

pyokagan commented Dec 4, 2017

addressbook-level4/src/main/java/seedu/address/model/AddressBook.java

Lines 89 to 91 in edc42f0

// TODO: the tags master list will be updated even though the below line fails.
// This can cause the tags master list to have additional tags that are not tagged to any person
// in the person list.

addressbook-level4/src/main/java/seedu/address/model/AddressBook.java

Lines 111 to 113 in edc42f0

// TODO: the tags master list will be updated even though the below line fails.
// This can cause the tags master list to have additional tags that are not tagged to any person
// in the person list.

There's also the issue that tags are not removed once the last person using that tag is removed from the address book (i.e. the set of tags will just keep growing :-P)

The tag list is also actually not used at all. We should either find some use for it, or remove it.
@damithc
Copy link
Contributor

damithc commented Dec 5, 2017

IIRC, the original intention was to have only one instance of each tag as a list of Tag objects and all Person objects should point to the relevant tags in the list, instead of keeping its own copy. That way, we just need to modify one Tag object when a Tag is edited.

@yamgent yamgent mentioned this issue Jan 12, 2018
Closed
@pyokagan pyokagan mentioned this issue Jan 22, 2018
Closed
@Zhiyuan-Amos
Copy link
Contributor

That way, we just need to modify one Tag object when a Tag is edited.

Tag is immutable though :P Also, we don't have a tag renaming feature.

@damithc
Copy link
Contributor

damithc commented Jan 23, 2018 •
edited
Loading

Tag is immutable though :P Also, we don't have a tag renaming feature

Tag renaming is one of the common extensions students do. Also, another possible extension is the ability to set/change color of a tag. As an OOP solution, we should treat tags as an object that is referenced by multiple other objects rather than a value that is being copied to multiple other objects?

@pyokagan
Copy link
Contributor Author

@damithc

As an OOP solution, we should treat tags as an object that is referenced by multiple other objects rather than a value that is being copied to multiple other objects?

I dunno, I wonder if students in the CS2103 level still have trouble with pointers. Even some SE-EDU developers in the past had trouble deciphering the intent of syncMasterTagListWith

@damithc
Copy link
Contributor

damithc commented Jan 24, 2018

How about we do it the simple way (i.e., no independent tag list) and give the other approach as a 'more OOP' alternative in the dev guide?

@Zhiyuan-Amos
Copy link
Contributor

As discussed here, we will be removing UniqueTagList.

@Rinder5 Rinder5 mentioned this issue Feb 9, 2018
Merged
Rinder5 added a commit to Rinder5/addressbook-level4 that referenced this issue Feb 15, 2018
@Rinder5
AddressBook: remove master tag list
27358f0 
We keep track of a master tag list which holds all the tags in our
address book.

The tag list is not used at all. The original intention was for
persons; tags to point to the tags in the master tag list. However,
implementing it in this way may be too complex for CS2103 students to
grasp, as discussed in se-edu#753. As discussed in se-edu#794, the tag list in its
current state is not needed.

Let's remove the master tag list in AddressBook.
Rinder5 added a commit to Rinder5/addressbook-level4 that referenced this issue Feb 22, 2018
@Rinder5
AddressBook: remove master tag list
9dcdc94 
We keep track of a master tag list which holds all the tags in our
address book.

The tag list is not used at all. The original intention was for
persons; tags to point to the tags in the master tag list. However,
implementing it in this way may be too complex for CS2103 students to
grasp, as discussed in se-edu#753. As discussed in se-edu#794, the tag list in its
current state is not needed.

Let's remove the master tag list in AddressBook.
Rinder5 added a commit to Rinder5/addressbook-level4 that referenced this issue Feb 22, 2018
@Rinder5
AddressBook: remove master tag list
88d9065 
We keep track of a master tag list which holds all the tags in our
address book.

The tag list is not used at all. The original intention was for
persons; tags to point to the tags in the master tag list. However,
implementing it in this way may be too complex for CS2103 students to
grasp, as discussed in se-edu#753. As discussed in se-edu#794, the tag list in its
current state is not needed.

Let's remove the master tag list in AddressBook.
Rinder5 added a commit to Rinder5/addressbook-level4 that referenced this issue Feb 22, 2018
@Rinder5
AddressBook: remove master tag list
a18fc7a 
We keep track of a master tag list which holds all the tags in our
address book.

The tag list is not used at all. The original intention was for
persons; tags to point to the tags in the master tag list. However,
implementing it in this way may be too complex for CS2103 students to
grasp, as discussed in se-edu#753. As discussed in se-edu#794, the tag list in its
current state is not needed.

Let's remove the master tag list in AddressBook.
Rinder5 added a commit to Rinder5/addressbook-level4 that referenced this issue Mar 7, 2018
@Rinder5
AddressBook: remove master tag list
ba19ac1 
We keep track of a master tag list which holds all the tags in our
address book.

The tag list is not used at all. The original intention was for
persons; tags to point to the tags in the master tag list. However,
implementing it in this way may be too complex for new developers to
grasp, as discussed in se-edu#753. Furthermore, from the discussion in se-edu#794,
the tag list in its current state is not needed.

Let's remove the master tag list in AddressBook.
Rinder5 added a commit to Rinder5/addressbook-level4 that referenced this issue Mar 11, 2018
@Rinder5
AddressBook: remove master tag list
b78bf9a 
We keep track of a master tag list which holds all the tags in our
address book.

The tag list is not used at all. The original intention was for
persons; tags to point to the tags in the master tag list. However,
implementing it in this way may be too complex for new developers to
grasp, as discussed in se-edu#753. Furthermore, from the discussion in se-edu#794,
the tag list in its current state is not needed.

Let's remove the master tag list in AddressBook.
Rinder5 added a commit to Rinder5/addressbook-level4 that referenced this issue Mar 15, 2018
@Rinder5
AddressBook & XmlSerializableAddressBook: remove tags list
a7e3b40 
We keep track of a master tag list which holds all the tags in our
address book.

The tag list is not used at all. The original intention was for
persons; tags to point to the tags in the master tag list. However,
implementing it in this way may be too complex for new developers to
grasp, as discussed in se-edu#753. Furthermore, from the discussion in se-edu#794,
the tag list in its current state is not needed.

Let's remove the master tag list in AddressBook.

XmlSerializableAddressBook keeps a list of tags, which is used for
storing the master tag list in the AddressBook object.

Since AddressBook#resetData(ReadOnlyAddressBook) will restore the
master tag list in a new AddressBook object through
AddressBook#syncWithMasterList, it is unnecessary to store the master
tag list in XmlSerializableAddressBook.

Let's also remove tags from XmlSerializableAddressBook.
Rinder5 added a commit to Rinder5/addressbook-level4 that referenced this issue Mar 15, 2018
@Rinder5
AddressBook: remove master tag list
4720946 
We keep track of a master tag list which holds all the tags in our
address book.

The tag list is not used at all. The original intention was for
persons; tags to point to the tags in the master tag list. However,
implementing it in this way may be too complex for new developers to
grasp, as discussed in se-edu#753. Furthermore, from the discussion in se-edu#794,
the tag list in its current state is not needed.

Let's remove the master tag list in AddressBook.
Rinder5 added a commit to Rinder5/addressbook-level4 that referenced this issue Mar 26, 2018
@Rinder5
AddressBook: remove master tag list
1a0c40e 
We keep track of a master tag list which holds all the tags in AddressBook.

The tag list is not used at all. The original intention was for
tags in Person to point to the tags in the master tag list. However,
implementing it in this way may be too complex for new developers to
grasp, as discussed in se-edu#753. Furthermore, from the discussion in se-edu#794,
the tag list in its current state is not needed.

Let's remove the master tag list in AddressBook.
Rinder5 added a commit to Rinder5/addressbook-level4 that referenced this issue Mar 27, 2018
@Rinder5
AddressBook: remove master tag list
97a30e7 
We keep track of a master tag list which holds all the tags in AddressBook.

The tag list is not used at all. The original intention was for
tags in Person to point to the tags in the master tag list. However,
implementing it in this way may be too complex for new developers to
grasp, as discussed in se-edu#753. Furthermore, from the discussion in se-edu#794,
the tag list in its current state is not needed.

Let's remove the master tag list in AddressBook.
Rinder5 added a commit to Rinder5/addressbook-level4 that referenced this issue Mar 28, 2018
@Rinder5
AddressBook: remove master tag list
c29a1be 
We keep track of a master tag list which holds all the tags in AddressBook.

The tag list is not used at all. The original intention was for
tags in Person to point to the tags in the master tag list. However,
implementing it in this way may be too complex for new developers to
grasp, as discussed in se-edu#753. Furthermore, from the discussion in se-edu#794,
the tag list in its current state is not needed.

Let's remove the master tag list in AddressBook.
Rinder5 added a commit to Rinder5/addressbook-level4 that referenced this issue Mar 28, 2018
@Rinder5
AddressBook: remove master tag list
5ccb34f 
We keep track of a master tag list which holds all the tags in
AddressBook.

The tag list is not used at all. The original intention was for
tags in Person to point to the tags in the master tag list. However,
implementing it in this way may be too complex for new developers to
grasp, as discussed in se-edu#753. Furthermore, from the discussion in se-edu#794,
the tag list in its current state is not needed.

Let's remove the master tag list in AddressBook.
Rinder5 added a commit to Rinder5/addressbook-level4 that referenced this issue Mar 28, 2018
@Rinder5
AddressBook: remove master tag list
6383115 
We keep track of a master tag list which holds all the tags in
AddressBook.

The tag list is not used at all. The original intention was for
tags in Person to point to the tags in the master tag list. However,
implementing it in this way may be too complex for new developers to
grasp, as discussed in se-edu#753. Furthermore, from the discussion in se-edu#794,
the tag list in its current state is not needed.

Let's remove the master tag list in AddressBook.
Rinder5 added a commit to Rinder5/addressbook-level4 that referenced this issue Apr 3, 2018
@Rinder5
AddressBook: remove master tag list
d51894a 
We keep track of a master tag list which holds all the tags in
AddressBook.

The tag list is not implemented correctly. The original intention was
for tags in Person to point to the tags in the master tag list.
However, implementing it in this way may be too complex for new
developers to grasp, as discussed in se-edu#753. Furthermore, from the
discussion in se-edu#794, the tag list in its current state is not needed.

Let's remove the master tag list in AddressBook.
Zhiyuan-Amos added a commit that referenced this issue Apr 4, 2018
@Zhiyuan-Amos
[#753] AddressBook: remove master tag list (#825)
285f0a1 
We keep track of a master tag list which holds all the tags in
AddressBook.

The tag list is not implemented correctly. The original intention was 
for tags in Person to point to the tags in the master tag list. 
However, implementing it in this way may be too complex for new 
developers to grasp, as discussed in #753. Furthermore, from the 
discussion in #794, the tag list in its current state is not needed.

Let's remove the master tag list in AddressBook.

Furthermore, without the master tag list in AddressBook,
UniqueTagList is no longer necessary.

Let's remove UniqueTagList.
@yamidark yamidark mentioned this issue Aug 5, 2018
Closed
yamidark added a commit to yamidark/addressbook-level2 that referenced this issue Aug 6, 2018
@yamidark
AddressBook: remove master tag list
cb5a2ea 
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 also be removed here for consistency.

Let's remove the master tag list in AddressBook.
yamidark added a commit to yamidark/addressbook-level2 that referenced this issue Aug 6, 2018
@yamidark
AddressBook: remove master tag list
9924cb9 
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 also be removed here for consistency.

Let's remove the master tag list in AddressBook.
yamidark added a commit to yamidark/addressbook-level2 that referenced this issue Aug 6, 2018
@yamidark
AddressBook: remove master tag list
ee3077e 
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 also be removed here for consistency.

Let's remove the master tag list in AddressBook.
yamidark added a commit to yamidark/addressbook-level2 that referenced this issue Aug 7, 2018
@yamidark
AddressBook: remove master tag list
e6cac41 
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.
yamidark added a commit to yamidark/addressbook-level2 that referenced this issue Aug 7, 2018
@yamidark
AddressBook: remove master tag list
9e692fa 
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.
yamidark added a commit to yamidark/addressbook-level2 that referenced this issue Aug 7, 2018
@yamidark
AddressBook: remove master tag list
6cd0518 
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.
yamidark added a commit to yamidark/addressbook-level2 that referenced this issue Aug 7, 2018
@yamidark
AddressBook: remove master tag list
b67d92d 
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.
pyokagan added a commit to se-edu/addressbook-level2 that referenced this issue Aug 8, 2018
@pyokagan
[#383] AddressBook: remove master tag list (#385)
ba89e36 
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.

  [1/3] AddressBook: remove master tag list
  [2/3] Remove UniqueTagList class
  [3/3] docs: update component on UniqueTagList
yamidark added a commit to yamidark/addressbook-level3 that referenced this issue Aug 9, 2018
@yamidark
AddressBook: remove master tag list
ac15304 
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.
yamidark added a commit to yamidark/addressbook-level3 that referenced this issue Aug 11, 2018
@yamidark
AddressBook: remove master tag list
5cc38cf 
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.
yamidark added a commit to yamidark/addressbook-level3 that referenced this issue Aug 11, 2018
@yamidark
AddressBook: remove master tag list
c877f7c 
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.
@yamidark yamidark mentioned this issue Aug 11, 2018
Merged
yamidark added a commit to yamidark/addressbook-level3 that referenced this issue Aug 12, 2018
@yamidark
AddressBook: remove master tag list
a7b85a3 
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.
yamidark added a commit to yamidark/addressbook-level3 that referenced this issue Aug 12, 2018
@yamidark
AddressBook: remove master tag list
0d22fc5 
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.
yamidark added a commit to se-edu/addressbook-level25 that referenced this issue Aug 13, 2018
@yamidark
AddressBook: remove master tag list (#149)
ab72b1f 
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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
a-Model d.Committers p.Medium
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

AddressBook: remove master tag list #753 Rinder5/addressbook-level4
3 participants
@pyokagan @damithc @Zhiyuan-Amos