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

Refactor code to extend from Main #14

Closed

Conversation

bjhoohaha
Copy link

No description provided.

Copy link

@ngzhaoming ngzhaoming left a comment

Choose a reason for hiding this comment

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

Thanks for managing and identifying the classes that are specifically for the address book. This will help free up the section for the team to implement their own features for Travezy. Really appreciate the help!

@@ -3,34 +3,34 @@

Actor User as user USER_COLOR
Participant ":UI" as ui UI_COLOR
Participant ":Logic" as logic LOGIC_COLOR
Participant ":Logic" as addressBookLogic LOGIC_COLOR

Choose a reason for hiding this comment

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

Good use of addressBookLogic to differentiate that this variable was previously from adress book

@Override
Optional<AddressBookUserPrefs> readUserPrefs() throws DataConversionException, IOException;

@Override

Choose a reason for hiding this comment

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

Possible to add java docs explaining what the new overridden method does


/**
* Represents an error which occurs during execution of a {@link Command}.
* Represents an error which occurs during execution of a {@link seedu.main.logic.commands.Command}.

Choose a reason for hiding this comment

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

Good rewriting of the java docs comments to suit the current changes

import seedu.address.model.AddressBook;
import seedu.address.testutil.TypicalPersons;
import seedu.main.commons.exceptions.IllegalValueException;

Choose a reason for hiding this comment

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

I don't think there is a need to shift down the imports. Since the imports are meant to be sorted by alphabetical order. Hence, the imports with common should be at the first 2 lines

@honhaochen
Copy link

Great Job refactoring!
The codes now are almost complete to be served as a skeletal for the team.
However, due to some implementation changes during our discussion, I'll suggest not to merge this PR first before we all decided and agreed on a more concrete and unified idea.

@bjhoohaha
Copy link
Author

Implementation to refactor code was largely confusing.

@bjhoohaha bjhoohaha closed this Oct 22, 2019
bjhoohaha added a commit that referenced this pull request Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants