From ba19ac1fe7fcb1f36a5a591958822e74cd9d674e Mon Sep 17 00:00:00 2001 From: A0162011A Date: Thu, 8 Mar 2018 04:21:59 +0800 Subject: [PATCH] AddressBook: remove master tag list 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 #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. --- .../events/model/AddressBookChangedEvent.java | 2 +- .../java/seedu/address/model/AddressBook.java | 79 ++----------------- .../address/model/ReadOnlyAddressBook.java | 7 -- .../seedu/address/model/AddressBookTest.java | 23 +----- .../address/testutil/AddressBookBuilder.java | 16 +--- 5 files changed, 12 insertions(+), 115 deletions(-) diff --git a/src/main/java/seedu/address/commons/events/model/AddressBookChangedEvent.java b/src/main/java/seedu/address/commons/events/model/AddressBookChangedEvent.java index 7db9b5c48ed6..b72ad4740e5a 100644 --- a/src/main/java/seedu/address/commons/events/model/AddressBookChangedEvent.java +++ b/src/main/java/seedu/address/commons/events/model/AddressBookChangedEvent.java @@ -14,6 +14,6 @@ public AddressBookChangedEvent(ReadOnlyAddressBook data) { @Override public String toString() { - return "number of persons " + data.getPersonList().size() + ", number of tags " + data.getTagList().size(); + return "number of persons " + data.getPersonList().size(); } } diff --git a/src/main/java/seedu/address/model/AddressBook.java b/src/main/java/seedu/address/model/AddressBook.java index 7251d520bbaf..b424e74aff0f 100644 --- a/src/main/java/seedu/address/model/AddressBook.java +++ b/src/main/java/seedu/address/model/AddressBook.java @@ -2,21 +2,13 @@ import static java.util.Objects.requireNonNull; -import java.util.HashMap; -import java.util.HashSet; import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.Set; -import java.util.stream.Collectors; import javafx.collections.ObservableList; import seedu.address.model.person.Person; import seedu.address.model.person.UniquePersonList; import seedu.address.model.person.exceptions.DuplicatePersonException; import seedu.address.model.person.exceptions.PersonNotFoundException; -import seedu.address.model.tag.Tag; -import seedu.address.model.tag.UniqueTagList; /** * Wraps all data at the address-book level @@ -25,7 +17,6 @@ public class AddressBook implements ReadOnlyAddressBook { private final UniquePersonList persons; - private final UniqueTagList tags; /* * The 'unusual' code block below is an non-static initialization block, sometimes used to avoid duplication @@ -36,13 +27,12 @@ public class AddressBook implements ReadOnlyAddressBook { */ { persons = new UniquePersonList(); - tags = new UniqueTagList(); } public AddressBook() {} /** - * Creates an AddressBook using the Persons and Tags in the {@code toBeCopied} + * Creates an AddressBook using the Persons in the {@code toBeCopied} */ public AddressBook(ReadOnlyAddressBook toBeCopied) { this(); @@ -55,22 +45,14 @@ public void setPersons(List persons) throws DuplicatePersonException { this.persons.setPersons(persons); } - public void setTags(Set tags) { - this.tags.setTags(tags); - } - /** * Resets the existing data of this {@code AddressBook} with {@code newData}. */ public void resetData(ReadOnlyAddressBook newData) { requireNonNull(newData); - setTags(new HashSet<>(newData.getTagList())); - List syncedPersonList = newData.getPersonList().stream() - .map(this::syncWithMasterTagList) - .collect(Collectors.toList()); try { - setPersons(syncedPersonList); + setPersons(newData.getPersonList()); } catch (DuplicatePersonException e) { throw new AssertionError("AddressBooks should not have duplicate persons"); } @@ -80,59 +62,25 @@ public void resetData(ReadOnlyAddressBook newData) { /** * Adds a person to the address book. - * Also checks the new person's tags and updates {@link #tags} with any new tags found, - * and updates the Tag objects in the person to point to those in {@link #tags}. * * @throws DuplicatePersonException if an equivalent person already exists. */ public void addPerson(Person p) throws DuplicatePersonException { - Person person = syncWithMasterTagList(p); - // 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. - persons.add(person); + persons.add(p); } /** * Replaces the given person {@code target} in the list with {@code editedPerson}. - * {@code AddressBook}'s tag list will be updated with the tags of {@code editedPerson}. * * @throws DuplicatePersonException if updating the person's details causes the person to be equivalent to * another existing person in the list. * @throws PersonNotFoundException if {@code target} could not be found in the list. - * - * @see #syncWithMasterTagList(Person) */ public void updatePerson(Person target, Person editedPerson) throws DuplicatePersonException, PersonNotFoundException { requireNonNull(editedPerson); - Person syncedEditedPerson = syncWithMasterTagList(editedPerson); - // 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. - persons.setPerson(target, syncedEditedPerson); - } - - /** - * Updates the master tag list to include tags in {@code person} that are not in the list. - * @return a copy of this {@code person} such that every tag in this person points to a Tag object in the master - * list. - */ - private Person syncWithMasterTagList(Person person) { - final UniqueTagList personTags = new UniqueTagList(person.getTags()); - tags.mergeFrom(personTags); - - // Create map with values = tag object references in the master list - // used for checking person tag references - final Map masterTagObjects = new HashMap<>(); - tags.forEach(tag -> masterTagObjects.put(tag, tag)); - - // Rebuild the list of person tags to point to the relevant tags in the master tag list. - final Set correctTagReferences = new HashSet<>(); - personTags.forEach(tag -> correctTagReferences.add(masterTagObjects.get(tag))); - return new Person( - person.getName(), person.getPhone(), person.getEmail(), person.getAddress(), correctTagReferences); + persons.setPerson(target, editedPerson); } /** @@ -143,17 +91,11 @@ public void removePerson(Person key) throws PersonNotFoundException { persons.remove(key); } - //// tag-level operations - - public void addTag(Tag t) throws UniqueTagList.DuplicateTagException { - tags.add(t); - } - //// util methods @Override public String toString() { - return persons.asObservableList().size() + " persons, " + tags.asObservableList().size() + " tags"; + return persons.asObservableList().size() + " persons"; // TODO: refine later } @@ -162,22 +104,15 @@ public ObservableList getPersonList() { return persons.asObservableList(); } - @Override - public ObservableList getTagList() { - return tags.asObservableList(); - } - @Override public boolean equals(Object other) { return other == this // short circuit if same object || (other instanceof AddressBook // instanceof handles nulls - && this.persons.equals(((AddressBook) other).persons) - && this.tags.equalsOrderInsensitive(((AddressBook) other).tags)); + && this.persons.equals(((AddressBook) other).persons)); } @Override public int hashCode() { - // use this method for custom fields hashing instead of implementing your own - return Objects.hash(persons, tags); + return persons.hashCode(); } } diff --git a/src/main/java/seedu/address/model/ReadOnlyAddressBook.java b/src/main/java/seedu/address/model/ReadOnlyAddressBook.java index 1f4e49a37d67..6ddc2cd9a290 100644 --- a/src/main/java/seedu/address/model/ReadOnlyAddressBook.java +++ b/src/main/java/seedu/address/model/ReadOnlyAddressBook.java @@ -2,7 +2,6 @@ import javafx.collections.ObservableList; import seedu.address.model.person.Person; -import seedu.address.model.tag.Tag; /** * Unmodifiable view of an address book @@ -15,10 +14,4 @@ public interface ReadOnlyAddressBook { */ ObservableList getPersonList(); - /** - * Returns an unmodifiable view of the tags list. - * This list will not contain any duplicate tags. - */ - ObservableList getTagList(); - } diff --git a/src/test/java/seedu/address/model/AddressBookTest.java b/src/test/java/seedu/address/model/AddressBookTest.java index bf26f68896b8..73a9572c0a15 100644 --- a/src/test/java/seedu/address/model/AddressBookTest.java +++ b/src/test/java/seedu/address/model/AddressBookTest.java @@ -4,7 +4,6 @@ import static seedu.address.testutil.TypicalPersons.ALICE; import static seedu.address.testutil.TypicalPersons.getTypicalAddressBook; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -17,7 +16,6 @@ import javafx.collections.FXCollections; import javafx.collections.ObservableList; import seedu.address.model.person.Person; -import seedu.address.model.tag.Tag; public class AddressBookTest { @@ -29,7 +27,6 @@ public class AddressBookTest { @Test public void constructor() { assertEquals(Collections.emptyList(), addressBook.getPersonList()); - assertEquals(Collections.emptyList(), addressBook.getTagList()); } @Test @@ -49,8 +46,7 @@ public void resetData_withValidReadOnlyAddressBook_replacesData() { public void resetData_withDuplicatePersons_throwsAssertionError() { // Repeat ALICE twice List newPersons = Arrays.asList(ALICE, ALICE); - List newTags = new ArrayList<>(ALICE.getTags()); - AddressBookStub newData = new AddressBookStub(newPersons, newTags); + AddressBookStub newData = new AddressBookStub(newPersons); thrown.expect(AssertionError.class); addressBook.resetData(newData); @@ -62,33 +58,20 @@ public void getPersonList_modifyList_throwsUnsupportedOperationException() { addressBook.getPersonList().remove(0); } - @Test - public void getTagList_modifyList_throwsUnsupportedOperationException() { - thrown.expect(UnsupportedOperationException.class); - addressBook.getTagList().remove(0); - } - /** - * A stub ReadOnlyAddressBook whose persons and tags lists can violate interface constraints. + * A stub ReadOnlyAddressBook whose persons list can violate interface constraints. */ private static class AddressBookStub implements ReadOnlyAddressBook { private final ObservableList persons = FXCollections.observableArrayList(); - private final ObservableList tags = FXCollections.observableArrayList(); - AddressBookStub(Collection persons, Collection tags) { + AddressBookStub(Collection persons) { this.persons.setAll(persons); - this.tags.setAll(tags); } @Override public ObservableList getPersonList() { return persons; } - - @Override - public ObservableList getTagList() { - return tags; - } } } diff --git a/src/test/java/seedu/address/testutil/AddressBookBuilder.java b/src/test/java/seedu/address/testutil/AddressBookBuilder.java index 6e73a762b0c1..d34f9c47f0da 100644 --- a/src/test/java/seedu/address/testutil/AddressBookBuilder.java +++ b/src/test/java/seedu/address/testutil/AddressBookBuilder.java @@ -1,15 +1,13 @@ package seedu.address.testutil; -import seedu.address.commons.exceptions.IllegalValueException; import seedu.address.model.AddressBook; import seedu.address.model.person.Person; import seedu.address.model.person.exceptions.DuplicatePersonException; -import seedu.address.model.tag.Tag; /** * A utility class to help with building Addressbook objects. * Example usage:
- * {@code AddressBook ab = new AddressBookBuilder().withPerson("John", "Doe").withTag("Friend").build();} + * {@code AddressBook ab = new AddressBookBuilder().withPerson("John", "Doe").build();} */ public class AddressBookBuilder { @@ -35,18 +33,6 @@ public AddressBookBuilder withPerson(Person person) { return this; } - /** - * Parses {@code tagName} into a {@code Tag} and adds it to the {@code AddressBook} that we are building. - */ - public AddressBookBuilder withTag(String tagName) { - try { - addressBook.addTag(new Tag(tagName)); - } catch (IllegalValueException ive) { - throw new IllegalArgumentException("tagName is expected to be valid."); - } - return this; - } - public AddressBook build() { return addressBook; }