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][W09-B3]Shawn Lin Jingjue #390

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/seedu/addressbook/logic/Logic.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import seedu.addressbook.data.AddressBook;
import seedu.addressbook.data.person.ReadOnlyPerson;
import seedu.addressbook.parser.Parser;
import seedu.addressbook.storage.Storage;
import seedu.addressbook.storage.StorageFile;

import java.util.Collections;
Expand All @@ -17,7 +18,7 @@
public class Logic {


private StorageFile storage;
private Storage storage;
private AddressBook addressBook;

/** The list of person shown to the user most recently. */
Expand All @@ -28,12 +29,12 @@ public Logic() throws Exception{
setAddressBook(storage.load());
}

Logic(StorageFile storageFile, AddressBook addressBook){
Logic(Storage storageFile, AddressBook addressBook){
setStorage(storageFile);
setAddressBook(addressBook);
}

void setStorage(StorageFile storage){
void setStorage(Storage storage){
this.storage = storage;
}

Expand All @@ -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();
}
Copy link

Choose a reason for hiding this comment

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

👍 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


Expand Down
45 changes: 45 additions & 0 deletions src/seedu/addressbook/storage/Storage.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package seedu.addressbook.storage;

import seedu.addressbook.data.AddressBook;
import seedu.addressbook.data.exception.IllegalValueException;

public interface 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?

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.


/** Default file path used if the user doesn't provide the file name. */
String DEFAULT_STORAGE_FILEPATH = "addressbook.txt";

/**
* Saves all data to this storage file.
*
* @throws StorageOperationException if there were errors converting and/or storing data to file.
*/
void save(AddressBook addressBook) throws StorageOperationException;

/**
* Loads data from this storage file.
*
* @throws StorageOperationException if there were errors reading and/or converting data from file.
*/
AddressBook load() throws StorageOperationException;

String getPath();

/**
* Signals that the given file path does not fulfill the storage filepath constraints.
*/
public static class InvalidStorageFilePathException extends IllegalValueException {
public InvalidStorageFilePathException(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.

👍


/**
* Signals that some error has occurred while trying to convert and read/write data between the application
* and the storage file.
*/
public static class StorageOperationException extends Exception {
public StorageOperationException(String message) {
super(message);
}
}
}
53 changes: 16 additions & 37 deletions src/seedu/addressbook/storage/StorageFile.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,34 +15,11 @@
/**
* Represents the file used to store address book data.
*/
public class StorageFile {

/** Default file path used if the user doesn't provide the file name. */
public static final String DEFAULT_STORAGE_FILEPATH = "addressbook.txt";
public class StorageFile implements Storage {

/* Note: Note the use of nested classes below.
* More info https://docs.oracle.com/javase/tutorial/java/javaOO/nested.html
*/

/**
* Signals that the given file path does not fulfill the storage filepath constraints.
*/
public static class InvalidStorageFilePathException extends IllegalValueException {
public InvalidStorageFilePathException(String message) {
super(message);
}
}

/**
* 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 {
public StorageOperationException(String message) {
super(message);
}
}

private final JAXBContext jaxbContext;

public final Path path;
Expand Down Expand Up @@ -78,12 +55,11 @@ private static boolean isValidPath(Path filePath) {
return filePath.toString().endsWith(".txt");
}

/**
* Saves all data to this storage file.
*
* @throws StorageOperationException if there were errors converting and/or storing data to file.
*/
public void save(AddressBook addressBook) throws StorageOperationException {
/* (non-Javadoc)
* @see seedu.addressbook.storage.Storage#save(seedu.addressbook.data.AddressBook)
*/
@Override
public void save(AddressBook addressBook) throws StorageOperationException {

/* Note: Note the 'try with resource' statement below.
* More info: https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
Expand All @@ -103,12 +79,11 @@ public void save(AddressBook addressBook) throws StorageOperationException {
}
}

/**
* Loads data from this storage file.
*
* @throws StorageOperationException if there were errors reading and/or converting data from file.
*/
public AddressBook load() throws StorageOperationException {
/* (non-Javadoc)
* @see seedu.addressbook.storage.Storage#load()
*/
@Override
public AddressBook load() throws StorageOperationException {
try (final Reader fileReader =
new BufferedReader(new FileReader(path.toFile()))) {

Expand Down Expand Up @@ -141,7 +116,11 @@ public AddressBook load() throws StorageOperationException {
}
}

public String getPath() {
/* (non-Javadoc)
* @see seedu.addressbook.storage.Storage#getPath()
*/
@Override
public String getPath() {
return path.toString();
}

Expand Down
7 changes: 3 additions & 4 deletions test/java/seedu/addressbook/logic/LogicTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@
import seedu.addressbook.data.person.*;
import seedu.addressbook.data.tag.Tag;
import seedu.addressbook.data.tag.UniqueTagList;
import seedu.addressbook.storage.StorageFile;

import seedu.addressbook.storage.Storage;
import java.util.*;

import static junit.framework.TestCase.assertEquals;
Expand All @@ -28,13 +27,13 @@ public class LogicTest {
@Rule
public TemporaryFolder saveFolder = new TemporaryFolder();

private StorageFile saveFile;
private Storage saveFile;
private AddressBook addressBook;
private Logic logic;

@Before
public void setup() throws Exception {
saveFile = new StorageFile(saveFolder.newFile("testSaveFile.txt").getPath());
saveFile = new StorageStub(saveFolder.newFile("testSaveFile.txt").getPath());
addressBook = new AddressBook();
saveFile.save(addressBook);
logic = new Logic(saveFile, addressBook);
Expand Down
30 changes: 30 additions & 0 deletions test/java/seedu/addressbook/logic/StorageStub.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package seedu.addressbook.logic;

import seedu.addressbook.data.AddressBook;
import seedu.addressbook.storage.Storage;

public class StorageStub implements Storage {

public StorageStub(String path) {
// TODO Auto-generated constructor stub
}

@Override
public void save(AddressBook addressBook) throws StorageOperationException {
// TODO Auto-generated method stub

}

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

@Override
public String getPath() {
// 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.

👍 Good job. Minimal and in the correct test directory. ⭐️

12 changes: 6 additions & 6 deletions test/java/seedu/addressbook/storage/StorageFileTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public void constructor_noTxtExtension_exceptionThrown() throws Exception {
@Test
public void load_invalidFormat_exceptionThrown() throws Exception {
// The file contains valid xml data, but does not match the AddressBook class
StorageFile storage = getStorage("InvalidData.txt");
Storage storage = getStorage("InvalidData.txt");
thrown.expect(StorageOperationException.class);
storage.load();
}
Expand All @@ -61,15 +61,15 @@ public void load_validFormat() throws Exception {

@Test
public void save_nullAddressBook_exceptionThrown() throws Exception {
StorageFile storage = getTempStorage();
Storage storage = getTempStorage();
thrown.expect(NullPointerException.class);
storage.save(null);
}

@Test
public void save_validAddressBook() throws Exception {
AddressBook ab = getTestAddressBook();
StorageFile storage = getTempStorage();
Storage storage = getTempStorage();
storage.save(ab);

assertStorageFilesEqual(storage, getStorage("ValidData.txt"));
Expand All @@ -80,15 +80,15 @@ public void save_validAddressBook() throws Exception {
/**
* Asserts that the contents of two storage files are the same.
*/
private void assertStorageFilesEqual(StorageFile sf1, StorageFile sf2) throws Exception {
private void assertStorageFilesEqual(Storage sf1, Storage sf2) throws Exception {
assertTextFilesEqual(Paths.get(sf1.getPath()), Paths.get(sf2.getPath()));
}

private StorageFile getStorage(String fileName) throws Exception {
private Storage getStorage(String fileName) throws Exception {
return new StorageFile(TEST_DATA_FOLDER + "/" + fileName);
}

private StorageFile getTempStorage() throws Exception {
private Storage getTempStorage() throws Exception {
return new StorageFile(testFolder.getRoot().getPath() + "/" + "temp.txt");
}

Expand Down