[se-edu#190] Make ReadOnlyAddressBook truly read-only (se-edu#199)

* Remove redundant "get empty address book" methods

It seems strange that we have AddressBook#getEmptyAddressBook() and
TestUtil#generateEmptyAddressBook() methods, when a simple
`new AddressBook()` will give us what we want.

* Fully encapsulate UniquePersonList and UniqueTagList within AddressBook

ReadOnlyAddressBook declares the following methods:

* `UniqueTagList getUniqueTagList();`

* `List<Tag> getTagList();`

* `UniquePersonList getUniquePersonList();`

* `List<ReadOnlyPerson> getPersonList();`

As we can see, there is some redundancy; both `getUniqueTagList()` and
`getTagList()` conceptually do the same thing, and both
`getUniquePersonList()` and `getPersonList()` conceptually do
the same thing as well.

So, to simplify the API we need to pick one set of APIs: either the ones
that return `UniqueXList`, or the ones that return `List<X>`.

Going with the API that returns `List<X>` is more beneficial than
`UniqueXList`: Lists are widely used throughout our code and the
standard library, and thus returning a list would be much more
convenient for developers. `UniquePersonList` and `UniqueTagList`, on
the other hand, only implement the `Iterable` interface, and are thus
slightly more annoying to write code with.

However, there is a deeper problem as well: `UniquePersonList` and
`UniqueTagList` both contain mutating methods, which violates the
contract of `ReadOnlyAddressBook` to provide a read-only view to the
address book. We could solve this by introducing
`ReadOnlyUniquePersonList` and `ReadOnlyUniqueTagList` interfaces, but
as argued above, by virtue of not implementing the `List`
interface these objects are more annoying to deal with and thus it is
likely better to not return them at all.

As such, this commit removes the `getUniqueTagList()` and
`getUniquePersonList()` methods. In addition, we also remove all of the
methods of `AddressBook` that take a `UniquePersonList` or
`UniqueTagList`, thus fully encapsulating these classes within
`AddressBook`. This not only solves the contract violation problem as
discussed above, it also reduces coupling as users will no longer need
to depend directly on `UniquePersonList` and `UniqueTagList` any more.

* ModelManager: merge two similar constructors

Both ModelManager(AddressBook, UserPrefs) and
ModelManager(ReadOnlyAddressBook, UserPrefs) both do exactly the same
thing. Let's DRY up the code by merging them into one single
constructor.

The author notes that the `userPrefs` argument is currently not even
used. However, fixing this is outside the scope of this commit.

* AddressBook: replace getPersons() with getPersonList()

ReadOnlyAddressBook has a getPersonList() method which returns its list
of persons as a List<ReadOnlyPerson>

AddressBook has a getPersons() method which returns its list of persons
as an ObservableList<Person>.

Simplify the API by merging these methods together, by making
getPersonList() return an ObservableList<ReadOnlyPerson> and removing
getPersons().

* ReadOnlyAddressBook: make getTagList() return an ObservableList

Even though there are currently no callers that require the return value
to be an ObservableList, this is still done for consistency with
getPersonList() and to make it easier to implement future UI features
such as a listing of all tags.

* fixup! Fully encapsulate UniquePersonList and UniqueTagList within AddressBook

* fixup! Fully encapsulate UniquePersonList and UniqueTagList within AddressBook

src/main/java/seedu/address/logic/commands/ClearCommand.java

@@ -14,7 +14,7 @@ public class ClearCommand extends Command {
     @Override
     public CommandResult execute() {
         assert model != null;
-        model.resetData(AddressBook.getEmptyAddressBook());
+        model.resetData(new AddressBook());
         return new CommandResult(MESSAGE_SUCCESS);
     }
 }

src/main/java/seedu/address/model/AddressBook.java

@@ -9,6 +9,8 @@
 
 import java.util.*;
 
+import javafx.collections.ObservableList;
+
 /**
  * Wraps all data at the address-book level
  * Duplicates are not allowed (by .equals comparison)
@@ -36,42 +38,34 @@ public AddressBook() {}
      * Creates an AddressBook using the Persons and Tags in the {@code toBeCopied}
      */
     public AddressBook(ReadOnlyAddressBook toBeCopied) {
-        this(toBeCopied.getUniquePersonList(), toBeCopied.getUniqueTagList());
-    }
-
-    /**
-     * Creates an AddressBook by copying the Persons and Tags in the given lists
-     */
-    public AddressBook(UniquePersonList persons, UniqueTagList tags) {
-        resetData(persons, tags);
-    }
-
-    public static ReadOnlyAddressBook getEmptyAddressBook() {
-        return new AddressBook();
+        this();
+        resetData(toBeCopied);
     }
 
 //// list overwrite operations
 
-    public UnmodifiableObservableList<Person> getPersons() {
-        return new UnmodifiableObservableList<>(persons.asObservableList());
-    }
-
-    public void setPersons(UniquePersonList persons) {
+    public void setPersons(List<? extends ReadOnlyPerson> persons)
+            throws UniquePersonList.DuplicatePersonException {
         this.persons.setPersons(persons);
     }
 
-    public void setTags(UniqueTagList tags) {
+    public void setTags(Collection<Tag> tags) throws UniqueTagList.DuplicateTagException {
         this.tags.setTags(tags);
     }
 
-    public void resetData(UniquePersonList newPersons, UniqueTagList newTags) {
-        setPersons(newPersons);
-        setTags(newTags);
-        syncMasterTagListWith(persons);
-    }
-
     public void resetData(ReadOnlyAddressBook newData) {
-        resetData(newData.getUniquePersonList(), newData.getUniqueTagList());
+        assert newData != null;
+        try {
+            setPersons(newData.getPersonList());
+        } catch (UniquePersonList.DuplicatePersonException e) {
+            assert false : "AddressBooks should not have duplicate persons";
+        }
+        try {
+            setTags(newData.getTagList());
+        } catch (UniqueTagList.DuplicateTagException e) {
+            assert false : "AddressBooks should not have duplicate tags";
+        }
+        syncMasterTagListWith(persons);
     }
 
 //// person-level operations
@@ -141,26 +135,15 @@ public String toString() {
     }
 
     @Override
-    public List<ReadOnlyPerson> getPersonList() {
-        return Collections.unmodifiableList(persons.asObservableList());
-    }
-
-    @Override
-    public List<Tag> getTagList() {
-        return Collections.unmodifiableList(tags.asObservableList());
-    }
-
-    @Override
-    public UniquePersonList getUniquePersonList() {
-        return this.persons;
+    public ObservableList<ReadOnlyPerson> getPersonList() {
+        return new UnmodifiableObservableList<>(persons.asObservableList());
     }
 
     @Override
-    public UniqueTagList getUniqueTagList() {
-        return this.tags;
+    public ObservableList<Tag> getTagList() {
+        return new UnmodifiableObservableList<>(tags.asObservableList());
     }
 
-
     @Override
     public boolean equals(Object other) {
         return other == this // short circuit if same object

src/main/java/seedu/address/model/ModelManager.java

@@ -3,6 +3,7 @@
 import javafx.collections.transformation.FilteredList;
 import seedu.address.commons.core.LogsCenter;
 import seedu.address.commons.core.UnmodifiableObservableList;
+import seedu.address.commons.util.CollectionUtil;
 import seedu.address.commons.util.StringUtil;
 import seedu.address.commons.events.model.AddressBookChangedEvent;
 import seedu.address.commons.core.ComponentManager;
@@ -22,32 +23,25 @@ public class ModelManager extends ComponentManager implements Model {
     private static final Logger logger = LogsCenter.getLogger(ModelManager.class);
 
     private final AddressBook addressBook;
-    private final FilteredList<Person> filteredPersons;
+    private final FilteredList<ReadOnlyPerson> filteredPersons;
 
     /**
-     * Initializes a ModelManager with the given AddressBook
-     * AddressBook and its variables should not be null
+     * Initializes a ModelManager with the given addressBook and userPrefs.
      */
-    public ModelManager(AddressBook src, UserPrefs userPrefs) {
+    public ModelManager(ReadOnlyAddressBook addressBook, UserPrefs userPrefs) {
         super();
-        assert src != null;
-        assert userPrefs != null;
+        assert !CollectionUtil.isAnyNull(addressBook, userPrefs);
 
-        logger.fine("Initializing with address book: " + src + " and user prefs " + userPrefs);
+        logger.fine("Initializing with address book: " + addressBook + " and user prefs " + userPrefs);
 
-        addressBook = new AddressBook(src);
-        filteredPersons = new FilteredList<>(addressBook.getPersons());
+        this.addressBook = new AddressBook(addressBook);
+        filteredPersons = new FilteredList<>(this.addressBook.getPersonList());
     }
 
     public ModelManager() {
         this(new AddressBook(), new UserPrefs());
     }
 
-    public ModelManager(ReadOnlyAddressBook initialData, UserPrefs userPrefs) {
-        addressBook = new AddressBook(initialData);
-        filteredPersons = new FilteredList<>(addressBook.getPersons());
-    }
-
     @Override
     public void resetData(ReadOnlyAddressBook newData) {
         addressBook.resetData(newData);

src/main/java/seedu/address/model/ReadOnlyAddressBook.java

@@ -2,29 +2,24 @@
 
 
 import seedu.address.model.person.ReadOnlyPerson;
-import seedu.address.model.person.UniquePersonList;
 import seedu.address.model.tag.Tag;
-import seedu.address.model.tag.UniqueTagList;
-
-import java.util.List;
+import javafx.collections.ObservableList;
 
 /**
  * Unmodifiable view of an address book
  */
 public interface ReadOnlyAddressBook {
 
-    UniqueTagList getUniqueTagList();
-
-    UniquePersonList getUniquePersonList();
-
     /**
-     * Returns an unmodifiable view of persons list
+     * Returns an unmodifiable view of the persons list.
+     * This list will not contain any duplicate persons.
      */
-    List<ReadOnlyPerson> getPersonList();
+    ObservableList<ReadOnlyPerson> getPersonList();
 
     /**
-     * Returns an unmodifiable view of tags list
+     * Returns an unmodifiable view of the tags list.
+     * This list will not contain any duplicate tags.
      */
-    List<Tag> getTagList();
+    ObservableList<Tag> getTagList();
 
 }

src/main/java/seedu/address/model/person/UniquePersonList.java

@@ -59,6 +59,14 @@ public void setPersons(UniquePersonList replacement) {
         this.internalList.setAll(replacement.internalList);
     }
 
+    public void setPersons(List<? extends ReadOnlyPerson> persons) throws DuplicatePersonException {
+        final UniquePersonList replacement = new UniquePersonList();
+        for (final ReadOnlyPerson person : persons) {
+            replacement.add(new Person(person));
+        }
+        setPersons(replacement);
+    }
+
     public UnmodifiableObservableList<Person> asObservableList() {
         return new UnmodifiableObservableList<>(internalList);
     }

src/main/java/seedu/address/model/tag/UniqueTagList.java

@@ -43,11 +43,8 @@ public UniqueTagList(Tag... tags) throws DuplicateTagException {
      * Enforces no null or duplicate elements.
      */
     public UniqueTagList(Collection<Tag> tags) throws DuplicateTagException {
-        CollectionUtil.assertNoNullElements(tags);
-        if (!CollectionUtil.elementsAreUnique(tags)) {
-            throw new DuplicateTagException();
-        }
-        internalList.addAll(tags);
+        this();
+        setTags(tags);
     }
 
     /**
@@ -82,6 +79,14 @@ public void setTags(UniqueTagList replacement) {
         this.internalList.setAll(replacement.internalList);
     }
 
+    public void setTags(Collection<Tag> tags) throws DuplicateTagException {
+        CollectionUtil.assertNoNullElements(tags);
+        if (!CollectionUtil.elementsAreUnique(tags)) {
+            throw new DuplicateTagException();
+        }
+        internalList.setAll(tags);
+    }
+
     /**
      * Ensures every tag in the argument list exists in this object.
      */

src/main/java/seedu/address/storage/XmlSerializableAddressBook.java

@@ -1,14 +1,17 @@
 package seedu.address.storage;
 
+import seedu.address.commons.core.UnmodifiableObservableList;
 import seedu.address.commons.exceptions.IllegalValueException;
 import seedu.address.model.tag.Tag;
-import seedu.address.model.tag.UniqueTagList;
 import seedu.address.model.ReadOnlyAddressBook;
+import seedu.address.model.person.Person;
 import seedu.address.model.person.ReadOnlyPerson;
-import seedu.address.model.person.UniquePersonList;
-
 import javax.xml.bind.annotation.XmlElement;
 import javax.xml.bind.annotation.XmlRootElement;
+
+import javafx.collections.FXCollections;
+import javafx.collections.ObservableList;
+
 import java.util.ArrayList;
 import java.util.List;
 import java.util.stream.Collectors;
@@ -43,55 +46,31 @@ public XmlSerializableAddressBook(ReadOnlyAddressBook src) {
     }
 
     @Override
-    public UniqueTagList getUniqueTagList() {
-        UniqueTagList lists = new UniqueTagList();
-        for (XmlAdaptedTag t : tags) {
-            try {
-                lists.add(t.toModelType());
-            } catch (IllegalValueException e) {
-                //TODO: better error handling
-            }
-        }
-        return lists;
-    }
-
-    @Override
-    public UniquePersonList getUniquePersonList() {
-        UniquePersonList lists = new UniquePersonList();
-        for (XmlAdaptedPerson p : persons) {
-            try {
-                lists.add(p.toModelType());
-            } catch (IllegalValueException e) {
-                //TODO: better error handling
-            }
-        }
-        return lists;
-    }
-
-    @Override
-    public List<ReadOnlyPerson> getPersonList() {
-        return persons.stream().map(p -> {
+    public ObservableList<ReadOnlyPerson> getPersonList() {
+        final ObservableList<Person> persons = this.persons.stream().map(p -> {
             try {
                 return p.toModelType();
             } catch (IllegalValueException e) {
                 e.printStackTrace();
                 //TODO: better error handling
                 return null;
             }
-        }).collect(Collectors.toCollection(ArrayList::new));
+        }).collect(Collectors.toCollection(FXCollections::observableArrayList));
+        return new UnmodifiableObservableList<>(persons);
     }
 
     @Override
-    public List<Tag> getTagList() {
-        return tags.stream().map(t -> {
+    public ObservableList<Tag> getTagList() {
+        final ObservableList<Tag> tags = this.tags.stream().map(t -> {
             try {
                 return t.toModelType();
             } catch (IllegalValueException e) {
                 e.printStackTrace();
                 //TODO: better error handling
                 return null;
             }
-        }).collect(Collectors.toCollection(ArrayList::new));
+        }).collect(Collectors.toCollection(FXCollections::observableArrayList));
+        return new UnmodifiableObservableList<>(tags);
     }
 
 }

src/test/java/guitests/AddressBookGuiTest.java

@@ -80,7 +80,7 @@ public void setup() throws Exception {
      * Return null to use the data in the file specified in {@link #getDataFileLocation()}
      */
     protected AddressBook getInitialData() {
-        AddressBook ab = TestUtil.generateEmptyAddressBook();
+        AddressBook ab = new AddressBook();
         TypicalTestPersons.loadAddressBookWithSampleData(ab);
         return ab;
     }