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

[W7][T08-4]Sonia Sunil #20

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

Conversation

ssunil3232
Copy link

This is a Sort Command that will show the addressbook in alphabetically sorted order.

(Previous PR to master branch to reject)
Copy link

@stephlewyh stephlewyh left a comment

Choose a reason for hiding this comment

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

Hi Sonia,

Good effort here. You have updated the user guide and provided a enhancement. Some areas you could improve on:

(1) Observing header conventions. Each new non-trivial method that you add should be accompanied with header comments telling what is the functionality of this method and the sentence should begin with a verb e.g. Performs sorting of names in addressbook.

(2) Proving test functionality. For every enhancement, you could provided a unit test to test the new functionality, to demonstrate for each given input, the expected output matches the actual output of new method.

You may close this PR after reviewing this comment.

@@ -39,12 +39,12 @@ public static String getMessageForPersonListShownSummary(List<? extends ReadOnly
/**
* Executes the command and returns the result.
*/
public CommandResult execute(){
/*public CommandResult execute(){

Choose a reason for hiding this comment

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

Avoid leaving unnecessary block comments in your code like this one.

@@ -61,6 +62,11 @@ public Address getAddress() {
return new HashSet<>(tags);
}

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.

@@ -44,6 +44,11 @@ public static boolean isValidName(String test) {
return Arrays.asList(fullName.split("\\s+"));
}

public int compareName(Name a){

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.

@@ -58,6 +58,15 @@ public void removePerson(ReadOnlyPerson toRemove) throws PersonNotFoundException
allPersons.remove(toRemove);
}

/**
* to sort the list of people in the address book
*/

Choose a reason for hiding this comment

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

A minor point - Note the header conventions in your next submission, it should start with a verb. E.g. Sorts the list of people in the address book.

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