Skip to content

Commit

Permalink
AddressBook: remove master tag list
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Rinder5 committed Mar 26, 2018
1 parent a4ef522 commit 1a0c40e
Show file tree
Hide file tree
Showing 5 changed files with 13 additions and 88 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
65 changes: 7 additions & 58 deletions src/main/java/seedu/address/model/AddressBook.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,7 @@

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;
Expand Down Expand Up @@ -42,7 +36,7 @@ public class AddressBook implements ReadOnlyAddressBook {
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();
Expand All @@ -55,22 +49,14 @@ public void setPersons(List<Person> persons) throws DuplicatePersonException {
this.persons.setPersons(persons);
}

public void setTags(Set<Tag> 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<Person> 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");
}
Expand All @@ -80,59 +66,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<Tag, Tag> 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<Tag> 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);
}

/**
Expand All @@ -146,14 +98,13 @@ public void removePerson(Person key) throws PersonNotFoundException {
//// 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
}

Expand All @@ -171,13 +122,11 @@ public ObservableList<Tag> getTagList() {
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();
}
}
2 changes: 1 addition & 1 deletion src/test/java/seedu/address/commons/util/XmlUtilTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public void saveDataToFile_validFile_dataSaved() throws Exception {

AddressBookBuilder builder = new AddressBookBuilder(new AddressBook());
dataToWrite = new XmlSerializableAddressBook(
builder.withPerson(new PersonBuilder().build()).withTag("Friends").build());
builder.withPerson(new PersonBuilder().build()).build());

XmlUtil.saveDataToFile(TEMP_FILE, dataToWrite);
dataFromFile = XmlUtil.getDataFromFile(TEMP_FILE, XmlSerializableAddressBook.class);
Expand Down
16 changes: 3 additions & 13 deletions src/test/java/seedu/address/model/AddressBookTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -29,7 +28,6 @@ public class AddressBookTest {
@Test
public void constructor() {
assertEquals(Collections.emptyList(), addressBook.getPersonList());
assertEquals(Collections.emptyList(), addressBook.getTagList());
}

@Test
Expand All @@ -49,8 +47,7 @@ public void resetData_withValidReadOnlyAddressBook_replacesData() {
public void resetData_withDuplicatePersons_throwsAssertionError() {
// Repeat ALICE twice
List<Person> newPersons = Arrays.asList(ALICE, ALICE);
List<Tag> newTags = new ArrayList<>(ALICE.getTags());
AddressBookStub newData = new AddressBookStub(newPersons, newTags);
AddressBookStub newData = new AddressBookStub(newPersons);

thrown.expect(AssertionError.class);
addressBook.resetData(newData);
Expand All @@ -62,22 +59,15 @@ 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<Person> persons = FXCollections.observableArrayList();
private final ObservableList<Tag> tags = FXCollections.observableArrayList();

AddressBookStub(Collection<Person> persons, Collection<? extends Tag> tags) {
AddressBookStub(Collection<Person> persons) {
this.persons.setAll(persons);
this.tags.setAll(tags);
}

@Override
Expand Down
16 changes: 1 addition & 15 deletions src/test/java/seedu/address/testutil/AddressBookBuilder.java
Original file line number Diff line number Diff line change
@@ -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: <br>
* {@code AddressBook ab = new AddressBookBuilder().withPerson("John", "Doe").withTag("Friend").build();}
* {@code AddressBook ab = new AddressBookBuilder().withPerson("John", "Doe").build();}
*/
public class AddressBookBuilder {

Expand All @@ -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;
}
Expand Down

0 comments on commit 1a0c40e

Please sign in to comment.