[T6A1][W10-B4]Li Zihan #407
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.
| void setStorage(StorageFile storage){ | ||
| this.storage = storage; | ||
| void setStorage(Storage storage2){ | ||
| this.storage = storage2; |
| */ | ||
| private StorageFile initializeStorage() throws StorageFile.InvalidStorageFilePathException { | ||
| private Storage initializeStorage() throws StorageFile.InvalidStorageFilePathException { | ||
| return new StorageFile(); |
There was a problem hiding this comment.
👍 Good job applying 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.storage.StorageFile.StorageOperationException; | ||
|
|
||
| public abstract class Storage { |
There was a problem hiding this comment.
Header comment?
You should also include the two exception handling implementations here.
| import seedu.addressbook.data.AddressBook; | ||
| import seedu.addressbook.storage.StorageFile.StorageOperationException; | ||
|
|
||
| public class StorageStub extends Storage { |
There was a problem hiding this comment.
Better to have this in the test directory rather than src since it's for testing purposes only.
| @Override | ||
| public AddressBook load() throws StorageOperationException { | ||
| return null; | ||
| } |
|
Ack by closing PR |
No description provided.