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]Huynh Van Tu An #519

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

Conversation

arishuynhvan
Copy link

Implement undo command instead of edit

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.

  • You need to create a StorageStub as a part of DI exercise.
  • you seem to have messed up with branching. there are too many unrelated commits in this PR.
  • I see you have updated user stories etc., here? That should be done on Addressbool-Level 4 code base.

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.

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.

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.

Based on the UML diagram in the exercise, this should be an abstract class, not an interface.

@@ -151,4 +132,10 @@ public String getPath() {
return path.toString();
}

@Override
public Storage newStorage() throws InvalidStorageFilePathException{
// TODO Auto-generated method stub
Copy link

Choose a reason for hiding this comment

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

please remove such unnecessary comments.

@@ -28,13 +28,13 @@
@Rule
public TemporaryFolder saveFolder = new TemporaryFolder();

private StorageFile saveFile;
private Storage saveFile;
Copy link

Choose a reason for hiding this comment

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

you need to use the storage stub here instead. something is not fully correct about this solution.

public Storage newStorage() throws InvalidStorageFilePathException;
public Storage newStorage(String path) throws InvalidStorageFilePathException;
public static Storage newStorage() throws 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.

ouch! this is incorrect. can you think why?

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

okkhoy commented Mar 4, 2017

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

@bryanleejh
Copy link

@arishuynhvan @okkhoy Should our PRs be submitted as a team?

@okkhoy
Copy link

okkhoy commented Mar 4, 2017

@bryanleejh @arishuynhvan not this one. the v0.0 yes.

@bryanleejh
Copy link

@okkhoy @arishuynhvan okay! got it!

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.

3 participants