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 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.
  • Loading branch information
Rinder5 committed Mar 7, 2018
1 parent 0432672 commit ba19ac1
Show file tree
Hide file tree
Showing 5 changed files with 12 additions and 115 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();
}
}
79 changes: 7 additions & 72 deletions src/main/java/seedu/address/model/AddressBook.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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();
Expand All @@ -55,22 +45,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 +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<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 @@ -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
}

Expand All @@ -162,22 +104,15 @@ public ObservableList<Person> getPersonList() {
return persons.asObservableList();
}

@Override
public ObservableList<Tag> 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();
}
}
7 changes: 0 additions & 7 deletions src/main/java/seedu/address/model/ReadOnlyAddressBook.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -15,10 +14,4 @@ public interface ReadOnlyAddressBook {
*/
ObservableList<Person> getPersonList();

/**
* Returns an unmodifiable view of the tags list.
* This list will not contain any duplicate tags.
*/
ObservableList<Tag> getTagList();

}
23 changes: 3 additions & 20 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 @@ -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 {

Expand All @@ -29,7 +27,6 @@ public class AddressBookTest {
@Test
public void constructor() {
assertEquals(Collections.emptyList(), addressBook.getPersonList());
assertEquals(Collections.emptyList(), addressBook.getTagList());
}

@Test
Expand All @@ -49,8 +46,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,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<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
public ObservableList<Person> getPersonList() {
return persons;
}

@Override
public ObservableList<Tag> getTagList() {
return tags;
}
}

}
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 ba19ac1

Please sign in to comment.