Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[T6A1][W10-B3] Chen Penghao #358

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

chenpenghao
Copy link

No description provided.

Copy link

@hwkchia hwkchia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Storage.InvalidStoragePathException {
return new StorageFile();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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.exception.IllegalValueException;
import seedu.addressbook.storage.Storage.StorageOperationException;

public abstract class Storage {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Header comment?

public InvalidStoragePathException(String message) {
super(message);
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
}

public abstract AddressBook load() throws StorageOperationException;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Override
public AddressBook load() throws StorageOperationException {
// TODO Auto-generated method stub
return null;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

import seedu.addressbook.storage.Storage.InvalidStoragePathException;
import seedu.addressbook.storage.jaxb.AdaptedAddressBook;

public class StorageStub extends Storage {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StorageStub should be as minimal as possible since it's just there to support testing other functionalities.

You should also place StorageStub in the test directory rather than src

@hwkchia
Copy link

hwkchia commented Mar 7, 2017

Ack by closing PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants