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.11r][W15-B3] Pham Vu Hung #775

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

Conversation

pvuhung
Copy link

@pvuhung pvuhung commented Mar 10, 2018

Added 2 enhancements: SortCommand and EditCommand

I sincerely apologize for submitting late

Copy link

@eldriclim eldriclim left a comment

Choose a reason for hiding this comment

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

Good job on unit test coverage, take note of the comments on edit command.

And remember to submit on time in the future.


import seedu.addressbook.common.Messages;
import seedu.addressbook.data.exception.IllegalValueException;
import seedu.addressbook.data.person.*;

Choose a reason for hiding this comment

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

You should avoid importing by *

public CommandResult execute() {
try {
final ReadOnlyPerson target = getTargetPerson();
toEdit.changeName(target.getName());

Choose a reason for hiding this comment

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

There shouldn't be a random setter() just to cater for editting.

);
}

public ReadOnlyPerson getPerson() {

Choose a reason for hiding this comment

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

Is this really necessary?

tagSet.add(new Tag(tagName));
}
this.toEdit = new Person(
new Name("dummy"),

Choose a reason for hiding this comment

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

Could use the index to retrieve the name of toEdit.

@@ -82,6 +82,19 @@ public void addPerson(Person toAdd) throws DuplicatePersonException {
allPersons.add(toAdd);
}

public void editPerson(ReadOnlyPerson target, Person editedPerson) throws PersonNotFoundException {

Choose a reason for hiding this comment

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

Every public facing method should have a method header according to JavaDoc standards.

*
* @param newName
*/
public void changeName(Name newName) {

Choose a reason for hiding this comment

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

This should not be allowed.

}

@Test
public void execute_edit_invalidArgsFormat() throws Exception {
Copy link

Choose a reason for hiding this comment

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

your test names do not follow the convention specified in our coding guidelines.
please take note and name them appropriately in your project

@okkhoy
Copy link

okkhoy commented Mar 12, 2018

@LeonidAgarth
Some comments added. Please ack & close the PR after reading 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.

5 participants