Skip to content

[W5] [T11-1] Windrich - #48

Open
windrichie wants to merge 3 commits into
nusCS2113-AY1819S2:masterfrom
windrichie:W5
Open

[W5] [T11-1] Windrich#48
windrichie wants to merge 3 commits into
nusCS2113-AY1819S2:masterfrom
windrichie:W5

Conversation

@windrichie

Copy link
Copy Markdown

Code in new class command adapted from existing command (AddCommand and ListCommand).

@JKupoluyi

Copy link
Copy Markdown

Good Job!

@stephlewyh stephlewyh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi Windrich,

Excellent job for a first attempt. Do note that you need to provide header comments for your enhancements and with non-trivial methods. Avoid leaving behind commented out code in your submission.

You have provided tests with edge cases. Good job!

return toAdd;
}

@Override

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing header comment. All non-trivial methods should have java doc format header comments.

addressBook.addPerson(toAdd);
List<ReadOnlyPerson> allPersons = addressBook.getAllPersons().immutableListView();
return new CommandResult(getMessageForPersonListShownSummary(allPersons), allPersons);
// return new CommandResult(String.format(MESSAGE_SUCCESS, toAdd));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is an unnecessary inline comment.

}
}

private Command prepareAddList(String args) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing header comment. All non-trivial methods should have java doc format header comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants