Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a unique tag set #149

Merged
merged 2 commits into from
Oct 13, 2020

Conversation

qwoprocks
Copy link
Member

Description

Implemented a unique tag set. Now, whenever clients are added, edited, or loaded, their set of tags will contain unique, global objects that are contained inside the UniqueTagSet, instead of every client having their own local Tag objects.

Testing

The junit tests I wrote covers all methods I added.
The application runs fine, and I used the following commands to ensure no regressions happened:
client edit 1 t/friends t/colleagues
client edit 1 t/friends
client add n/Tom p/999 e/a@a.com a/Somewhere c/US tz/GMT+6 t/rivals t/bestfriends
client delete 9

Remarks

Right now, it is not possible to delete tags from the unique tag set when clients are deleted without major refactoring and design changes. I would suggest leaving this part for Ritesh to consider when he transfers this to Notes, since that will most likely require major changes as well. However, this is not a huge problem, as the unique tag set will reload from the current tags in storage when the application restarts.

@qwoprocks qwoprocks added type.Task Something that needs to be done, but not a story, bug, or an epic. e.g. Move testing code into a new priority.High Must do labels Oct 13, 2020
@qwoprocks qwoprocks added this to the v1.2 milestone Oct 13, 2020
@qwoprocks qwoprocks self-assigned this Oct 13, 2020
@qwoprocks qwoprocks added this to In progress in v1.2 via automation Oct 13, 2020
v1.2 automation moved this from In progress to Reviewer approved Oct 13, 2020
Copy link
Member

@rtshkmr rtshkmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, would be nice to have a synchronizeTagSets() function but good to merge so I can try putting this in for notes

Comment on lines +87 to +92
private void replaceClientTagSet(Client client) {
Set<Tag> clientLocalTags = client.getTags();
tags.addAll(clientLocalTags);
client.replaceTags(tags.getTags(clientLocalTags));
}

Copy link
Member

@rtshkmr rtshkmr Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qwoprocks mentioned that having a one-way transfer of information from Clients to AddressBook is more complicated to implement and so to Synchronise the two set of tags, this method was chosen.

also may I suggest having a helper function called synchronizeTagSets()

@qwoprocks qwoprocks merged commit 726e58a into AY2021S1-CS2103T-F11-4:master Oct 13, 2020
v1.2 automation moved this from Reviewer approved to Done Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.High Must do type.Task Something that needs to be done, but not a story, bug, or an epic. e.g. Move testing code into a new
Projects
No open projects
v1.2
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants