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][F12-B4] Lee Jun Han Bryan #524

Closed

Conversation

bryanleejh
Copy link

Copy link

@okkhoy okkhoy left a comment

Choose a reason for hiding this comment

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

appreciate the attention to detail, in terms of adhering to coding standards (spacing before and after braces, symbols etc.,)
Overall good attempt! Please keep up the good work :-)

import seedu.addressbook.data.exception.IllegalValueException;
import seedu.addressbook.storage.jaxb.AdaptedAddressBook;

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 header comments. If not, developers who need 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.

assertEquals(lastShownList, r.getRelevantPersons().get());
}

//Confirm the state of data is as expected
// Confirm the state of data is as expected
Copy link

Choose a reason for hiding this comment

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

in the below, there is a line that checks for the contents on the disk with the contents in memory. using the stub the right way, that should be removed. else the test fails.

@okkhoy okkhoy added the Reviewed label Mar 6, 2017
@okkhoy
Copy link

okkhoy commented Mar 6, 2017

@bryanleejh
Some comments added. Please ack & close the PR after reading comments.

@bryanleejh bryanleejh closed this Mar 7, 2017
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