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

[W5][T12-1]Zhang Chenxi #168

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

Conversation

ZhangCX10032
Copy link

No description provided.

@sreycodes
Copy link

sreycodes commented Feb 13, 2019

The grouping feature can be really useful :)You just need to add some tests!

Copy link

@nerrons nerrons left a comment

Choose a reason for hiding this comment

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

You also need to update test cases (the expected.txt and another test method like GroupCommandTest.java) and the help command (so that group shows up when you type help)

@@ -37,6 +37,8 @@ protected DuplicatePersonException() {

private final List<Person> internalList = new ArrayList<>();

private final List<Person> groupedPersonList = new ArrayList<>();
Copy link

Choose a reason for hiding this comment

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

Perhaps add comments to groupedPersonList and explain what it does and why do we need this?

@Tejas2805
Copy link

Really nice enhancement! We can maybe use this enhancement for group related activities that we have in our mind for the final project.

Copy link

@bqnguyen94 bqnguyen94 left a comment

Choose a reason for hiding this comment

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

Is your PR ready for review yet? Because I don't think the changes are doing what they supposed are to do. Also, please add test cases for your new function as well :)

@@ -111,6 +111,26 @@ Deletes the 2nd person in the address book.
`delete 1` +
Deletes the 1st person in the results of the `find` command.

== Highlighting a person : `group`

Deletes the specified person from the address book. Irreversible. +

Choose a reason for hiding this comment

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

This is wrong?

public CommandResult execute() {
try {
final ReadOnlyPerson target = getTargetPerson();
addressBook.removePerson(target);

Choose a reason for hiding this comment

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

Uhm this will remove the target person from address book?

@@ -15,6 +15,7 @@
import seedu.addressbook.commands.ClearCommand;
import seedu.addressbook.commands.Command;
import seedu.addressbook.commands.DeleteCommand;
import seedu.addressbook.commands.GroupCommand;

Choose a reason for hiding this comment

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

This import is in wrong order since import statements are sorted alphabetically :)

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.

6 participants