Fix code based on feedback (#187)

* Rename ChangeSaveLocationRequest to SaveLocationChange

* Update test

* Put literals in variable for Events

* Rewrite Store Command test one method per test case

* Rewrite test for load command. One method per test case

* Rewrite Storage tests to follow convention

* Rewrite logic manager test for store and load

* Extract variables out of setup

* Rename SaveLocationChange to ChangeSaveLocation

src/main/java/seedu/agendum/commons/events/model/ChangeSaveLocationEvent.java

@@ -0,0 +1,21 @@
+package seedu.agendum.commons.events.model;
+
+import seedu.agendum.commons.events.BaseEvent;
+//@@author A0148095X
+/** Indicates a request from model to change the save location of the data file*/
+public class ChangeSaveLocationEvent extends BaseEvent {
+
+    public final String location;
+
+    private String message;
+
+    public ChangeSaveLocationEvent(String saveLocation){
+        this.location = saveLocation;
+        this.message = "Request to change save location to: " + location;
+    }
+
+    @Override
+    public String toString() {
+        return message;
+    }
+}

src/main/java/seedu/agendum/commons/events/model/ToDoListChangedEvent.java

@@ -7,13 +7,16 @@
 public class ToDoListChangedEvent extends BaseEvent {
 
     public final ReadOnlyToDoList data;
+
+    private String message;
 
     public ToDoListChangedEvent(ReadOnlyToDoList data){
         this.data = data;
+        this.message =  "number of tasks " + data.getTaskList().size();
     }
 
     @Override
     public String toString() {
-        return "number of tasks " + data.getTaskList().size();
+        return message;
     }
 }

src/main/java/seedu/agendum/commons/events/storage/LoadDataCompleteEvent.java

@@ -8,13 +8,16 @@
 public class LoadDataCompleteEvent extends BaseEvent {
 
     public final ReadOnlyToDoList data;
+
+    private String message;
 
     public LoadDataCompleteEvent(ReadOnlyToDoList data){
         this.data = data;
+        this.message = "Todo list data load completed. Task list size: " + data.getTaskList().size();
     }
 
     @Override
     public String toString() {
-        return "Todo list data load completed. Task list size: " + data.getTaskList().size();
+        return message;
     }
 }

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

@@ -11,7 +11,7 @@
 import seedu.agendum.model.task.UniqueTaskList;
 import seedu.agendum.model.task.UniqueTaskList.TaskNotFoundException;
 import seedu.agendum.commons.events.model.LoadDataRequestEvent;
-import seedu.agendum.commons.events.model.ChangeSaveLocationRequestEvent;
+import seedu.agendum.commons.events.model.ChangeSaveLocationEvent;
 import seedu.agendum.commons.events.model.ToDoListChangedEvent;
 import seedu.agendum.commons.events.storage.LoadDataCompleteEvent;
 import seedu.agendum.commons.core.ComponentManager;
@@ -89,8 +89,8 @@ private void indicateToDoListChanged() {
 
     //@@author A0148095X
     /** Raises an event to indicate that save location has changed */
-    private void indicateChangeSaveLocationRequest(String location) {
-        raise(new ChangeSaveLocationRequestEvent(location));
+    private void indicateChangeSaveLocation(String location) {
+        raise(new ChangeSaveLocationEvent(location));
     }
 
     /** Raises an event to indicate that save location has changed */
@@ -179,7 +179,7 @@ private void clearPreviousToDoLists() {
     @Override
     public synchronized void changeSaveLocation(String location){
         assert StringUtil.isValidPathToFile(location);
-        indicateChangeSaveLocationRequest(location);
+        indicateChangeSaveLocation(location);
         indicateToDoListChanged();
     }
 
@@ -188,7 +188,7 @@ public synchronized void loadFromLocation(String location) {
         assert StringUtil.isValidPathToFile(location);
         assert XmlUtil.isFileCorrectFormat(location);
 
-        indicateChangeSaveLocationRequest(location);
+        indicateChangeSaveLocation(location);
         indicateLoadDataRequest(location);
     }
     //@@author

src/main/java/seedu/agendum/storage/Storage.java

@@ -1,7 +1,7 @@
 package seedu.agendum.storage;
 
 import seedu.agendum.commons.events.model.LoadDataRequestEvent;
-import seedu.agendum.commons.events.model.ChangeSaveLocationRequestEvent;
+import seedu.agendum.commons.events.model.ChangeSaveLocationEvent;
 import seedu.agendum.commons.events.model.ToDoListChangedEvent;
 import seedu.agendum.commons.events.storage.DataSavingExceptionEvent;
 import seedu.agendum.commons.exceptions.DataConversionException;
@@ -45,5 +45,5 @@ public interface Storage extends ToDoListStorage, UserPrefsStorage {
     void handleLoadDataRequestEvent(LoadDataRequestEvent event);
 
     /** Sets the save location **/
-    void handleChangeSaveLocationRequestEvent(ChangeSaveLocationRequestEvent event);
+    void handleChangeSaveLocationEvent(ChangeSaveLocationEvent event);
 }

src/main/java/seedu/agendum/storage/StorageManager.java

@@ -11,7 +11,7 @@
 import seedu.agendum.commons.core.Config;
 import seedu.agendum.commons.core.LogsCenter;
 import seedu.agendum.commons.events.model.LoadDataRequestEvent;
-import seedu.agendum.commons.events.model.ChangeSaveLocationRequestEvent;
+import seedu.agendum.commons.events.model.ChangeSaveLocationEvent;
 import seedu.agendum.commons.events.model.ToDoListChangedEvent;
 import seedu.agendum.commons.events.storage.DataLoadingExceptionEvent;
 import seedu.agendum.commons.events.storage.DataSavingExceptionEvent;
@@ -115,7 +115,7 @@ public void handleToDoListChangedEvent(ToDoListChangedEvent event) {
     //@@author A0148095X
     @Override
     @Subscribe
-    public void handleChangeSaveLocationRequestEvent(ChangeSaveLocationRequestEvent event) {
+    public void handleChangeSaveLocationEvent(ChangeSaveLocationEvent event) {
         String location = event.location;
 
         setToDoListFilePath(location);

src/main/java/seedu/agendum/ui/StatusBarFooter.java

@@ -17,7 +17,7 @@
 
 import org.controlsfx.control.StatusBar;
 import seedu.agendum.commons.core.LogsCenter;
-import seedu.agendum.commons.events.model.ChangeSaveLocationRequestEvent;
+import seedu.agendum.commons.events.model.ChangeSaveLocationEvent;
 import seedu.agendum.commons.events.model.ToDoListChangedEvent;
 import seedu.agendum.commons.util.FxViewUtil;
 
@@ -123,7 +123,7 @@ public void handleToDoListChangedEvent(ToDoListChangedEvent event) {
 
     //@@author A0148095X
     @Subscribe
-    public void handleChangeSaveLocationRequestEvent(ChangeSaveLocationRequestEvent event) {
+    public void handleChangeSaveLocationEvent(ChangeSaveLocationEvent event) {
         String saveLocation = event.location;
         logger.info(LogsCenter.getEventHandlingLogMessage(event, "Setting save location to: " + saveLocation));
         setSaveLocation(saveLocation);

src/test/java/guitests/LoadCommandTest.java

@@ -1,13 +1,11 @@
 package guitests;
 
 import java.io.File;
-import java.io.IOException;
 
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
-import seedu.agendum.commons.exceptions.FileDeletionException;
-import seedu.agendum.commons.exceptions.IllegalValueException;
 import seedu.agendum.commons.util.FileUtil;
 import seedu.agendum.logic.commands.LoadCommand;
 import seedu.agendum.model.ToDoList;
@@ -18,54 +16,65 @@
 //@@author A0148095X
 public class LoadCommandTest extends ToDoListGuiTest {
 
-    private String command;
-
+    private final String command = LoadCommand.COMMAND_WORD + " ";
+    private final String fileThatExists = "data/test/FileThatExists.xml";
+    private final String fileThatDoesNotExist = "data/test/DoesNotExist.xml";
+    private final String fileInWrongFormat = "data/test/WrongFormat.xml";
+    private final String missingFileType = "data/test/invalid";
+    private final String missingFileName = "data/test/.bad";    
+
     @Before
-    public void setup() throws Exception{
-        super.setup();
-        command = LoadCommand.COMMAND_WORD + " ";
-    }
-
-    @Test
-    public void loadPathValid() throws IOException, FileDeletionException, IllegalValueException {
-        String fileThatExists = "data/test/FileThatExists.xml";
-        String fileThatDoesNotExist = "data/DoesNotExist.xml";
-        String fileInWrongFormat = "data/WrongFormat.xml";
-
+    public void setup() throws Exception {
+        super.setup();    
+
         // setup storage file
         Task toBeAdded = new Task(new Name("test"));
         ToDoList expectedTDL = new ToDoList();
         expectedTDL.addTask(toBeAdded);
         XmlToDoListStorage xmltdls = new XmlToDoListStorage(fileThatExists);
         xmltdls.saveToDoList(expectedTDL);
 
+        // create empty file
+        FileUtil.createFile(new File(fileInWrongFormat)); 
+    }
+
+    @After
+    public void clean() throws Exception {
+        // cleanup
+        FileUtil.deleteFile(fileThatExists);
+        FileUtil.deleteFile(fileInWrongFormat);
+    }
+
+    @Test
+    public void load_pathValidFileExists_messageSuccess() {
         // load from an existing file
         commandBox.runCommand(command + fileThatExists);
         assertResultMessage(String.format(LoadCommand.MESSAGE_SUCCESS, fileThatExists));
-
+    }
+
+    @Test
+    public void load_pathValidFileDoesNotExist_messageFileDoesNotExist() {
         // load from a non-existing file
         commandBox.runCommand(command + fileThatDoesNotExist);
         assertResultMessage(String.format(LoadCommand.MESSAGE_FILE_DOES_NOT_EXIST, fileThatDoesNotExist));
-
+    }
+
+    @Test
+    public void load_pathValidFileWrongFormat_messageFileWrongFormat() {
         // file in wrong format
-        FileUtil.createFile(new File(fileInWrongFormat)); // create empty file
         commandBox.runCommand(command + fileInWrongFormat);
         assertResultMessage(String.format(LoadCommand.MESSAGE_FILE_WRONG_FORMAT, fileInWrongFormat));
-
-        // cleanup
-        FileUtil.deleteFile(fileInWrongFormat); 
-        FileUtil.deleteFile(fileThatExists);
     }
 
     @Test
-    public void loadPathInvalid(){
-        String missingFileType = "test/invalid";
-        String missingFileName = "test/.bad";
-
+    public void load_fileTypeInvalid_messagePathInvalid() {
         // invalid file type
         commandBox.runCommand(command + missingFileType);
         assertResultMessage(String.format(LoadCommand.MESSAGE_PATH_INVALID, missingFileType));
-
+    }
+
+    @Test
+    public void load_fileNameInvalid_messagePathInvalid() {
         // invalid file name
         commandBox.runCommand(command + missingFileName);
         assertResultMessage(String.format(LoadCommand.MESSAGE_PATH_INVALID, missingFileName));

src/test/java/guitests/StoreCommandTest.java

@@ -13,33 +13,45 @@
 //@@author A0148095X
 public class StoreCommandTest extends ToDoListGuiTest {
 
+    private final String validLocation = "data/test.xml";
+    private final String badLocation = "test/.xml";
+    private final String inaccessibleLocation = "C:/windows/system32/agendum/todolist.xml";
+
     @Test
-    public void store() throws IOException, FileDeletionException {
-        String testLocation = "data/test.xml";
-        String badLocation = "test/.xml";
-        String inaccessibleLocation = "C:/windows/system32/agendum/todolist.xml";
-
+    public void store_validLocation_messageSuccess() {
         //save to a valid directory
-        commandBox.runCommand("store " + testLocation);
-        assertResultMessage(String.format(StoreCommand.MESSAGE_SUCCESS, testLocation));
-
+        commandBox.runCommand("store " + validLocation);
+        assertResultMessage(String.format(StoreCommand.MESSAGE_SUCCESS, validLocation));        
+    }
+
+    @Test
+    public void store_defaultLocation_messageSuccessDefaultLocation() {
         //save to default directory
         commandBox.runCommand("store default");
         assertResultMessage(String.format(StoreCommand.MESSAGE_LOCATION_DEFAULT, Config.DEFAULT_SAVE_LOCATION));
-
-        //invalid directory
+    }
+
+    @Test
+    public void store_invalidLocation_messageInvalidPath() {        
+        //invalid Location
         commandBox.runCommand("store " + badLocation);
         assertResultMessage(StoreCommand.MESSAGE_PATH_WRONG_FORMAT);
-
+    }
+
+    @Test
+    public void store_inaccessibleLocation_messageLocationInaccessible() {
         //inaccessible location
         commandBox.runCommand("store " + inaccessibleLocation);
-        //assertResultMessage(StoreCommand.MESSAGE_LOCATION_INACCESSIBLE);        
-
+        //assertResultMessage(StoreCommand.MESSAGE_LOCATION_INACCESSIBLE);
+    }
+
+    @Test
+    public void store_fileExists_messageFileExists() throws IOException, FileDeletionException {     
         //file exists
-        FileUtil.createIfMissing(new File(testLocation));
-        commandBox.runCommand("store " + testLocation);
+        FileUtil.createIfMissing(new File(validLocation));
+        commandBox.runCommand("store " + validLocation);
         assertResultMessage(StoreCommand.MESSAGE_FILE_EXISTS);
-        FileUtil.deleteFile(testLocation);
+        FileUtil.deleteFile(validLocation);
 
     }
 }