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][F11-B3]Hsieh Hsin Han #513

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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
15 changes: 8 additions & 7 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){
setStorage(storageFile);
Logic(Storage storage, AddressBook addressBook){
setStorage(storage);
setAddressBook(addressBook);
}

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

Expand All @@ -43,13 +44,13 @@ 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.
* @throws Storage.InvalidStorageFilePathException if the target file path is incorrect.
*/
private StorageFile initializeStorage() throws StorageFile.InvalidStorageFilePathException {
private Storage initializeStorage() throws Storage.InvalidStorageFilePathException {
return new StorageFile();
}

public String getStorageFilePath() {
public String getStoragePath() {
return storage.getPath();
}

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

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

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.

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.


public abstract void save(AddressBook addressBook) throws StorageOperationException ;

public abstract AddressBook load() throws StorageOperationException;

public abstract String getPath();

/**
* Signals that some error has occured while trying to convert and read/write data between the application
Copy link

Choose a reason for hiding this comment

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

Proper Javadoc should like

/**
* Signals that...
* ....
*/

* and the storage file.
*/
public static class StorageOperationException extends Exception {
Copy link

Choose a reason for hiding this comment

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

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

public static class InvalidStorageFilePathException extends IllegalValueException {
public InvalidStorageFilePathException(String message) {
super(message);
}
}
}
26 changes: 5 additions & 21 deletions src/seedu/addressbook/storage/StorageFile.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
/**
* Represents the file used to store address book data.
*/
public class StorageFile {
public class StorageFile extends Storage{

/** Default file path used if the user doesn't provide the file name. */
public static final String DEFAULT_STORAGE_FILEPATH = "addressbook.txt";
Expand All @@ -24,25 +24,6 @@ public class StorageFile {
* 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 @@ -83,6 +64,7 @@ private static boolean isValidPath(Path filePath) {
*
* @throws StorageOperationException if there were errors converting and/or storing data to file.
*/
@Override
public void save(AddressBook addressBook) throws StorageOperationException {

/* Note: Note the 'try with resource' statement below.
Expand All @@ -108,6 +90,7 @@ public void save(AddressBook addressBook) throws StorageOperationException {
*
* @throws StorageOperationException if there were errors reading and/or converting data from file.
*/
@Override
public AddressBook load() throws StorageOperationException {
try (final Reader fileReader =
new BufferedReader(new FileReader(path.toFile()))) {
Expand Down Expand Up @@ -140,7 +123,8 @@ public AddressBook load() throws StorageOperationException {
throw new StorageOperationException("File contains illegal data values; data type constraints not met");
}
}


@Override
public String getPath() {
return path.toString();
}
Expand Down
120 changes: 120 additions & 0 deletions src/seedu/addressbook/storage/StorageStub.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
package seedu.addressbook.storage;

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.io.Reader;
import java.io.Writer;
import java.nio.file.Path;
import java.nio.file.Paths;

import javax.xml.bind.JAXBContext;
import javax.xml.bind.JAXBException;
import javax.xml.bind.Marshaller;
import javax.xml.bind.Unmarshaller;

import seedu.addressbook.data.AddressBook;
import seedu.addressbook.data.exception.IllegalValueException;
import seedu.addressbook.storage.Storage.InvalidStorageFilePathException;
import seedu.addressbook.storage.jaxb.AdaptedAddressBook;

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

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

private final JAXBContext jaxbContext;

public final Path path;

/**
* @throws InvalidStorageFilePathException if the default path is invalid
*/
public StorageStub() throws InvalidStorageFilePathException {
this(DEFAULT_STORAGE_FILEPATH);
}

/**
* @throws InvalidStorageFilePathException if the given file path is invalid
*/
public StorageStub(String filePath) throws InvalidStorageFilePathException {
Copy link

Choose a reason for hiding this comment

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

Why do you need this code here? A return null is enough? Stubs don't usually do any work.

try {
jaxbContext = JAXBContext.newInstance(AdaptedAddressBook.class);
} catch (JAXBException jaxbe) {
throw new RuntimeException("jaxb initialisation error");
}

path = Paths.get(filePath);
if (!isValidPath(path)) {
throw new InvalidStorageFilePathException("Storage file should end with '.txt'");
}
}

/**
* Returns true if the given path is acceptable as a storage file.
* The file path is considered acceptable if it ends with '.txt'
*/
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.
*/
@Override
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.
*/
@Override
public 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.

Same here.
Why do you need this code here? A return null is enough? Stubs don't usually do any work.

try (final Reader fileReader =
new BufferedReader(new FileReader(path.toFile()))) {

final Unmarshaller unmarshaller = jaxbContext.createUnmarshaller();
final AdaptedAddressBook loaded = (AdaptedAddressBook) unmarshaller.unmarshal(fileReader);
// manual check for missing elements
if (loaded.isAnyRequiredFieldMissing()) {
throw new StorageOperationException("File data missing some elements");
}
return loaded.toModelType();

/* Note: Here, we are using an exception to create the file if it is missing. However, we should minimize
* using exceptions to facilitate normal paths of execution. If we consider the missing file as a 'normal'
* situation (i.e. not truly exceptional) we should not use an exception to handle it.
*/

// create empty file if not found
} catch (FileNotFoundException fnfe) {
final AddressBook empty = new AddressBook();
save(empty);
return empty;

// other errors
} catch (IOException ioe) {
throw new StorageOperationException("Error writing to file: " + path);
} catch (JAXBException jaxbe) {
throw new StorageOperationException("Error parsing file data format");
} catch (IllegalValueException ive) {
throw new StorageOperationException("File contains illegal data values; data type constraints not met");
}
}

@Override
public String getPath() {
return path.toString();
}
}
2 changes: 1 addition & 1 deletion src/seedu/addressbook/ui/Gui.java
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link

Choose a reason for hiding this comment

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

Great! You also remember to change this one.

}

private MainWindow createMainWindow(Stage stage, Stoppable mainApp) throws IOException{
Expand Down
11 changes: 6 additions & 5 deletions test/java/seedu/addressbook/logic/LogicTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import seedu.addressbook.data.tag.Tag;
import seedu.addressbook.data.tag.UniqueTagList;
import seedu.addressbook.storage.StorageFile;
import seedu.addressbook.storage.StorageStub;

import java.util.*;

Expand All @@ -28,16 +29,16 @@ public class LogicTest {
@Rule
public TemporaryFolder saveFolder = new TemporaryFolder();

private StorageFile saveFile;
private StorageStub saveStub;
private AddressBook addressBook;
private Logic logic;

@Before
public void setup() throws Exception {
saveFile = new StorageFile(saveFolder.newFile("testSaveFile.txt").getPath());
saveStub = new StorageStub(saveFolder.newFile("testSaveFile.txt").getPath());
addressBook = new AddressBook();
saveFile.save(addressBook);
logic = new Logic(saveFile, addressBook);
saveStub.save(addressBook);
Copy link

Choose a reason for hiding this comment

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

Why do you need this code here since stub actually should do nothing in this case?

logic = new Logic(saveStub, addressBook);
}

@Test
Expand Down Expand Up @@ -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());
Copy link

Choose a reason for hiding this comment

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

Since you are doing DI. You no longer need to test the storage when you test the logic.

}


Expand Down
2 changes: 1 addition & 1 deletion test/java/seedu/addressbook/storage/StorageFileTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import seedu.addressbook.data.person.Phone;
import seedu.addressbook.data.tag.Tag;
import seedu.addressbook.data.tag.UniqueTagList;
import seedu.addressbook.storage.StorageFile.StorageOperationException;
import seedu.addressbook.storage.Storage.StorageOperationException;
import static seedu.addressbook.util.TestUtil.assertTextFilesEqual;

public class StorageFileTest {
Expand Down