[T6A1][F11-B3]Hsieh Hsin Han #513
Conversation
…e depend on the abstract class Storage.
| import seedu.addressbook.data.AddressBook; | ||
| import seedu.addressbook.data.exception.IllegalValueException; | ||
|
|
||
| public abstract class Storage { |
There was a problem hiding this comment.
This class and its methods should have a header comments. If not, developers who needs to inherit from this class will not know how exactly they should override the methods and other developers who write client code for this class will not know how to use it either.
| * Signals that some error has occured while trying to convert and read/write data between the application | ||
| * and the storage file. | ||
| */ | ||
| public static class StorageOperationException extends Exception { |
There was a problem hiding this comment.
Indentation problem here?. Our coding standard requires you to use 4 spaces instead of tabs. You can configure Eclipse to convert tabs to spaces automatically.
| public abstract String getPath(); | ||
|
|
||
| /** | ||
| * Signals that some error has occured while trying to convert and read/write data between the application |
There was a problem hiding this comment.
Proper Javadoc should like
/**
* Signals that...
* ....
*/| /** | ||
| * @throws InvalidStorageFilePathException if the given file path is invalid | ||
| */ | ||
| public StorageStub(String filePath) throws InvalidStorageFilePathException { |
There was a problem hiding this comment.
Why do you need this code here? A return null is enough? Stubs don't usually do any work.
| * @throws StorageOperationException if there were errors reading and/or converting data from file. | ||
| */ | ||
| @Override | ||
| public AddressBook load() throws StorageOperationException { |
There was a problem hiding this comment.
Same here.
Why do you need this code here? A return null is enough? Stubs don't usually do any work.
| @@ -31,7 +31,7 @@ public Gui(Logic logic, String version) { | |||
|
|
|||
| public void start(Stage stage, Stoppable mainApp) throws IOException { | |||
| mainWindow = createMainWindow(stage, mainApp); | |||
| mainWindow.displayWelcomeMessage(version, logic.getStorageFilePath()); | |||
| mainWindow.displayWelcomeMessage(version, logic.getStoragePath()); | |||
There was a problem hiding this comment.
Great! You also remember to change this one.
| addressBook = new AddressBook(); | ||
| saveFile.save(addressBook); | ||
| logic = new Logic(saveFile, addressBook); | ||
| saveStub.save(addressBook); |
There was a problem hiding this comment.
Why do you need this code here since stub actually should do nothing in this case?
| @@ -90,7 +91,7 @@ private void assertCommandBehavior(String inputCommand, | |||
| //Confirm the state of data is as expected | |||
| assertEquals(expectedAddressBook, addressBook); | |||
| assertEquals(lastShownList, logic.getLastShownList()); | |||
| assertEquals(addressBook, saveFile.load()); | |||
| assertEquals(addressBook, saveStub.load()); | |||
There was a problem hiding this comment.
Since you are doing DI. You no longer need to test the storage when you test the logic.
|
@Tony-Hsieh Some comments added. Please close the PR after reading comments. |
No description provided.