Merge pull request #251 from rxchell/fix-code-quality

Improve code quality (model, storage, tests)

src/main/java/seedu/address/MainApp.java

@@ -81,13 +81,23 @@ public void init() throws Exception {
     private Model initModelManager(Storage storage, ReadOnlyUserPrefs userPrefs) {
         logger.info("Using data file : " + storage.getAddressBookFilePath());
 
+        ReadOnlyAddressBook initialPersonsData = loadAddressBookData(storage);
+        ReadOnlyAppointmentBook initialAppointmentData = loadAppointmentBookData(storage, initialPersonsData);
+
+        return new ModelManager(initialPersonsData, initialAppointmentData, userPrefs);
+    }
+
+    /**
+     * Returns a {@code ReadOnlyAddressBook} with the data from {@code storage}'s address book. <br>
+     * The data from the sample address book will be used instead if {@code storage}'s address book is not found,
+     * or an empty address book will be used instead if errors occur when reading {@code storage}'s address book.
+     */
+    private ReadOnlyAddressBook loadAddressBookData(Storage storage) {
         Optional<ReadOnlyAddressBook> addressBookOptional;
         ReadOnlyAddressBook initialPersonsData;
-        Optional<ReadOnlyAppointmentBook> appointmentBookOptional;
-        ReadOnlyAppointmentBook initialAppointmentData;
         try {
             addressBookOptional = storage.readAddressBook();
-            if (!addressBookOptional.isPresent()) {
+            if (addressBookOptional.isEmpty()) {
                 logger.info("Creating a new data file " + storage.getAddressBookFilePath()
                         + " populated with a sample AddressBook.");
             }
@@ -97,9 +107,21 @@ private Model initModelManager(Storage storage, ReadOnlyUserPrefs userPrefs) {
                     + " Will be starting with an empty AddressBook.");
             initialPersonsData = new AddressBook();
         }
+        return initialPersonsData;
+    }
+
+    /**
+     * Returns a {@code ReadOnlyAppointmentBook} with the data from {@code storage}'s appointment book. <br>
+     * Data from the sample appointment book will be used instead if {@code storage}'s appointment book is
+     * not found, or an empty appointment book will be used instead if errors occur when reading {@code
+     * storage}'s appointment book.
+     */
+    private ReadOnlyAppointmentBook loadAppointmentBookData(Storage storage, ReadOnlyAddressBook initialPersonsData) {
+        Optional<ReadOnlyAppointmentBook> appointmentBookOptional;
+        ReadOnlyAppointmentBook initialAppointmentData;
         try {
             appointmentBookOptional = storage.readAppointmentBook(initialPersonsData);
-            if (!appointmentBookOptional.isPresent()) {
+            if (appointmentBookOptional.isEmpty()) {
                 logger.info("Creating a new data file " + storage.getAppointmentBookFilePath()
                         + " populated with a sample AppointmentBook.");
                 initialAppointmentData = SampleDataUtil.getSampleAppointmentBook(initialPersonsData);
@@ -111,7 +133,7 @@ private Model initModelManager(Storage storage, ReadOnlyUserPrefs userPrefs) {
                     + " Will be starting with an empty AppointmentBook.");
             initialAppointmentData = new AppointmentBook();
         }
-        return new ModelManager(initialPersonsData, initialAppointmentData, userPrefs);
+        return initialAppointmentData;
     }
 
     private void initLogging(Config config) {
@@ -138,7 +160,7 @@ protected Config initConfig(Path configFilePath) {
 
         try {
             Optional<Config> configOptional = ConfigUtil.readConfig(configFilePathUsed);
-            if (!configOptional.isPresent()) {
+            if (configOptional.isEmpty()) {
                 logger.info("Creating new config file " + configFilePathUsed);
             }
             initializedConfig = configOptional.orElse(new Config());
@@ -169,7 +191,7 @@ protected UserPrefs initPrefs(UserPrefsStorage storage) {
         UserPrefs initializedPrefs;
         try {
             Optional<UserPrefs> prefsOptional = storage.readUserPrefs();
-            if (!prefsOptional.isPresent()) {
+            if (prefsOptional.isEmpty()) {
                 logger.info("Creating new preference file " + prefsFilePath);
             }
             initializedPrefs = prefsOptional.orElse(new UserPrefs());

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

@@ -80,9 +80,12 @@ public boolean equals(Object other) {
         }
 
         UserPrefs otherUserPrefs = (UserPrefs) other;
-        return guiSettings.equals(otherUserPrefs.guiSettings)
-                && addressBookFilePath.equals(otherUserPrefs.addressBookFilePath)
-                && appointmentBookFilePath.equals(otherUserPrefs.appointmentBookFilePath);
+
+        boolean sameGuiSettings = guiSettings.equals(otherUserPrefs.guiSettings);
+        boolean sameAddressBookFilePath = addressBookFilePath.equals(otherUserPrefs.addressBookFilePath);
+        boolean sameAppointmentBookFilePath = appointmentBookFilePath.equals(otherUserPrefs.appointmentBookFilePath);
+
+        return sameGuiSettings && sameAddressBookFilePath && sameAppointmentBookFilePath;
     }
 
     @Override

src/main/java/seedu/address/model/appointment/Appointment.java

@@ -112,8 +112,10 @@ public Medicine getMedicine() {
      * This defines a weaker notion of equality between two appointments.
      */
     public boolean isSameAppointment(Appointment otherAppointment) {
-        return appointmentDescriptor.isSameAppointment(otherAppointment.appointmentDescriptor)
-                && person.isSamePerson(otherAppointment.person);
+        boolean isSamePerson = person.isSamePerson(otherAppointment.person);
+        boolean isSameAppointmentDescriptor = appointmentDescriptor.isSameAppointment(
+                otherAppointment.appointmentDescriptor);
+        return isSamePerson && isSameAppointmentDescriptor;
     }
 
     public boolean isSameAppointment(AppointmentDescriptor otherAppointment) {

src/main/java/seedu/address/model/appointment/AppointmentDescriptor.java

@@ -60,7 +60,6 @@ public boolean isSameAppointment(AppointmentDescriptor otherAppointment) {
             return true;
         }
 
-        // todo: add other checks for equality
         return otherAppointment != null
                 && otherAppointment.getAppointmentDateTime().equals(getAppointmentDateTime())
                 && otherAppointment.getAppointmentType().equals(getAppointmentType());

src/main/java/seedu/address/model/appointment/UniqueAppointmentList.java

@@ -72,7 +72,7 @@ public void addAppointment(Appointment appointmentToAdd) {
     /**
      * Replaces the appointment {@code target} in the list with {@code editedAppointment}.
      * {@code target} must exist in the list.
-     * The appointmnet identity of {@code editedAppointment} must not be the same as another
+     * The appointment identity of {@code editedAppointment} must not be the same as another
      * existing appointment in the list.
      */
     public void setAppointment(Appointment target, Appointment editedAppointment) {

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

@@ -77,52 +77,103 @@ public static LocalDateTime parseDateTime(String dateTimeStr) throws IllegalValu
     /**
      * Converts this Jackson-friendly adapted Appointment object into the model's {@code Appointment} object.
      *
+     * @param addressBook the address book to find the person from.
+     * @return the model {@code Appointment} object.
      * @throws IllegalValueException if there were any data constraints violated in the adapted person.
      */
     public Appointment toModelType(ReadOnlyAddressBook addressBook) throws IllegalValueException {
+        final AppointmentType modelAppointmentType = validateAndConvertAppointmentType();
+        final LocalDateTime modelAppointmentDateTime = validateAndConvertAppointmentDateTime();
+        final int modelPersonId = validateAndConvertPersonId();
+        final Person modelPerson = validateAndFindPerson(addressBook, modelPersonId);
+        final Sickness modelSickness = validateAndConvertSickness();
+        final Medicine modelMedicine = validateAndConvertMedicine();
+
+        return new Appointment(modelAppointmentType, modelAppointmentDateTime, modelPerson,
+                modelSickness, modelMedicine, appointmentId);
+    }
+
+    /**
+     * Validates and converts the appointment type.
+     *
+     * @return the appointment type.
+     */
+    private AppointmentType validateAndConvertAppointmentType() throws IllegalValueException {
         if (appointmentType == null) {
             throw new IllegalValueException(String.format(MISSING_FIELD_MESSAGE_FORMAT,
                     AppointmentType.class.getSimpleName()));
         }
         if (!AppointmentType.isValidAppointmentType(appointmentType)) {
             throw new IllegalValueException(AppointmentType.MESSAGE_CONSTRAINTS);
         }
-        final AppointmentType modelAppointmentType = new AppointmentType(appointmentType);
+        return new AppointmentType(appointmentType);
+    }
 
+    /**
+     * Validates and converts the appointment date-time.
+     *
+     * @return the appointment date-time.
+     */
+    private LocalDateTime validateAndConvertAppointmentDateTime() throws IllegalValueException {
         if (appointmentDateTime == null) {
             throw new IllegalValueException(String.format(MISSING_FIELD_MESSAGE_FORMAT,
                     LocalDateTime.class.getSimpleName()));
         }
-        final LocalDateTime modelAppointmentDateTime = parseDateTime(appointmentDateTime);
+        return parseDateTime(appointmentDateTime);
+    }
 
+    /**
+     * Validates and converts the personId.
+     *
+     * @return the person ID.
+     */
+    private int validateAndConvertPersonId() throws IllegalValueException {
         if (personId == null) {
             throw new IllegalValueException(String.format(MISSING_FIELD_MESSAGE_FORMAT,
                     Integer.class.getSimpleName()));
         }
         if (personId < 0) {
             throw new IllegalValueException(String.format(INTEGER_CHECK_MESSAGE_FORMAT));
         }
-        final int modelPersonId = personId;
+        return personId;
+    }
 
-        final Optional<Person> modelPersonOptional = addressBook.findPerson(modelPersonId);
-        if (!modelPersonOptional.isPresent()) {
+    /**
+     * Validates and finds the person from the address book.
+     *
+     * @param addressBook the address book to find the person from.
+     * @param personId the person ID to find.
+     * @return the person.
+     */
+    private Person validateAndFindPerson(ReadOnlyAddressBook addressBook, int personId) throws IllegalValueException {
+        final Optional<Person> modelPersonOptional = addressBook.findPerson(personId);
+        if (modelPersonOptional.isEmpty()) {
             throw new IllegalValueException(String.format(PERSON_CHECK_MESSAGE_FORMAT));
         }
-        Person modelPerson = modelPersonOptional.get();
+        return modelPersonOptional.get();
+    }
 
+    /**
+     * Validates and converts the sickness.
+     *
+     * @return the sickness.
+     */
+    private Sickness validateAndConvertSickness() throws IllegalValueException {
         if (!Sickness.isValidSickness(sickness)) {
             throw new IllegalValueException(Sickness.MESSAGE_CONSTRAINTS);
         }
-        final Sickness modelSickness = new Sickness(sickness);
+        return new Sickness(sickness);
+    }
 
+    /**
+     * Validates and converts the medicine.
+     *
+     * @return the medicine.
+     */
+    private Medicine validateAndConvertMedicine() throws IllegalValueException {
         if (!Medicine.isValidMedicine(medicine)) {
             throw new IllegalValueException(Medicine.MESSAGE_CONSTRAINTS);
         }
-        final Medicine modelMedicine = new Medicine(medicine);
-
-        return new Appointment(modelAppointmentType, modelAppointmentDateTime, modelPerson,
-                modelSickness, modelMedicine, appointmentId);
+        return new Medicine(medicine);
     }
-
-
 }

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

@@ -73,52 +73,106 @@ public JsonAdaptedPerson(Person source) {
      * @throws IllegalValueException if there were any data constraints violated in the adapted person.
      */
     public Person toModelType() throws IllegalValueException {
+        final List<Tag> personTags = convertTagsToModelType();
+        final Name modelName = validateAndCreateName();
+        final Phone modelPhone = validateAndCreatePhone();
+        final Email modelEmail = validateAndCreateEmail();
+        final Address modelAddress = validateAndCreateAddress();
+        final Status modelStatus = validateAndCreateStatus();
+        final Set<Tag> modelTags = new HashSet<>(personTags);
+        return new Person(modelName, modelPhone, modelEmail, modelAddress, modelStatus, modelTags, personId);
+    }
+
+    /**
+     * Converts the tags of the JsonAdaptedPerson to the model's Tag object.
+     *
+     * @return List of Tag objects.
+     */
+    private List<Tag> convertTagsToModelType() throws IllegalValueException {
         final List<Tag> personTags = new ArrayList<>();
         for (JsonAdaptedTag tag : tags) {
             personTags.add(tag.toModelType());
         }
+        return personTags;
+    }
+
+    /**
+     * Validates and creates the Name object.
+     *
+     * @return Name object.
+     * @throws IllegalValueException if the name is invalid.
+     */
+    private Name validateAndCreateName() throws IllegalValueException {
         if (name == null) {
             throw new IllegalValueException(String.format(MISSING_FIELD_MESSAGE_FORMAT, Name.class.getSimpleName()));
         }
         if (!Name.isValidName(name)) {
             throw new IllegalValueException(Name.MESSAGE_CONSTRAINTS);
         }
-        final Name modelName = new Name(name);
+        return new Name(name);
+    }
 
+    /**
+     * Validates and creates the Phone object.
+     *
+     * @return Phone object.
+     * @throws IllegalValueException if the phone is invalid
+     */
+    private Phone validateAndCreatePhone() throws IllegalValueException {
         if (phone == null) {
             throw new IllegalValueException(String.format(MISSING_FIELD_MESSAGE_FORMAT, Phone.class.getSimpleName()));
         }
         if (!Phone.isValidPhone(phone)) {
             throw new IllegalValueException(Phone.MESSAGE_CONSTRAINTS);
         }
-        final Phone modelPhone = new Phone(phone);
+        return new Phone(phone);
+    }
 
+    /**
+     * Validates and creates the Email object.
+     *
+     * @return Email object.
+     * @throws IllegalValueException if the email is invalid
+     */
+    private Email validateAndCreateEmail() throws IllegalValueException {
         if (email == null) {
             throw new IllegalValueException(String.format(MISSING_FIELD_MESSAGE_FORMAT, Email.class.getSimpleName()));
         }
         if (!Email.isValidEmail(email)) {
             throw new IllegalValueException(Email.MESSAGE_CONSTRAINTS);
         }
-        final Email modelEmail = new Email(email);
+        return new Email(email);
+    }
 
+    /**
+     * Validates and creates the Address object.
+     *
+     * @return Address object.
+     * @throws IllegalValueException if the address is invalid
+     */
+    private Address validateAndCreateAddress() throws IllegalValueException {
         if (address == null) {
             throw new IllegalValueException(String.format(MISSING_FIELD_MESSAGE_FORMAT, Address.class.getSimpleName()));
         }
         if (!Address.isValidAddress(address)) {
             throw new IllegalValueException(Address.MESSAGE_CONSTRAINTS);
         }
-        final Address modelAddress = new Address(address);
+        return new Address(address);
+    }
 
+    /**
+     * Validates and creates the Status object.
+     *
+     * @return Status object.
+     * @throws IllegalValueException if the status is invalid
+     */
+    private Status validateAndCreateStatus() throws IllegalValueException {
         if (status == null) {
             throw new IllegalValueException(String.format(MISSING_FIELD_MESSAGE_FORMAT, Status.class.getSimpleName()));
         }
         if (!Status.isValidStatus(status)) {
             throw new IllegalValueException(Status.MESSAGE_CONSTRAINTS);
         }
-        final Status modelStatus = new Status(status);
-
-        final Set<Tag> modelTags = new HashSet<>(personTags);
-        return new Person(modelName, modelPhone, modelEmail, modelAddress, modelStatus, modelTags, personId);
+        return new Status(status);
     }
-
 }

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

@@ -1,7 +1,6 @@
 package seedu.address.storage;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
-import com.fasterxml.jackson.annotation.JsonValue;
 
 import seedu.address.commons.exceptions.IllegalValueException;
 import seedu.address.model.tag.Tag;
@@ -13,6 +12,12 @@ class JsonAdaptedTag {
 
     private final String tagName;
 
+
+    // Default constructor needed by Jackson
+    public JsonAdaptedTag() {
+        this.tagName = "";
+    }
+
     /**
      * Constructs a {@code JsonAdaptedTag} with the given {@code tagName}.
      */
@@ -21,18 +26,14 @@ public JsonAdaptedTag(String tagName) {
         this.tagName = tagName;
     }
 
+
     /**
      * Converts a given {@code Tag} into this class for Json use.
      */
     public JsonAdaptedTag(Tag source) {
         tagName = source.tagName;
     }
 
-    @JsonValue
-    public String getTagName() {
-        return tagName;
-    }
-
     /**
      * Converts this Jackson-friendly adapted tag object into the model's {@code Tag} object.
      *