[T6A1][W09-B3]Shawn Lin Jingjue #390
Conversation
There was a problem hiding this comment.
Good job overall.
As we are now using StorageStub in LogicTest, we need not test the correctness of StorageFile when we test the Logic class. There is a line in the assertCommandBehavior method that checks for the contents on the disk with the contents in memory; this should be removed.
| @@ -45,7 +46,7 @@ void setAddressBook(AddressBook addressBook){ | |||
| * Creates the StorageFile object based on the user specified path (if any) or the default storage path. | |||
| * @throws StorageFile.InvalidStorageFilePathException if the target file path is incorrect. | |||
| */ | |||
| private StorageFile initializeStorage() throws StorageFile.InvalidStorageFilePathException { | |||
| private Storage initializeStorage() throws StorageFile.InvalidStorageFilePathException { | |||
| return new StorageFile(); | |||
| } | |||
There was a problem hiding this comment.
👍 appplying DIP
The creating of StorageFile is a little unfortunate, but nonetheless has to be done. The original design already violates OCP. Fyi, an issue has been raised regarding this se-edu/addressbook-level3#75
| import seedu.addressbook.data.AddressBook; | ||
| import seedu.addressbook.data.exception.IllegalValueException; | ||
|
|
||
| public interface Storage { |
There was a problem hiding this comment.
Header comment?
And try to follow the class diagram by making this an abstract class instead of an interface? By right, interfaces should not have any implementation although Java 8 allows it.
| public InvalidStorageFilePathException(String message) { | ||
| super(message); | ||
| } | ||
| } |
| return null; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
👍 Good job. Minimal and in the correct test directory. ⭐️
|
Ack by closing PR |
First 2 parts