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][W09-3] Sivakumar Bavadharini #156

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

Conversation

bava98
Copy link

@bava98 bava98 commented Feb 12, 2019

Add a sort command which sorts the list alphabetically and shows the sorted list, or no names if the list is empty.

Copy link

@Qing-Yuan Qing-Yuan left a comment

Choose a reason for hiding this comment

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

Good OOP when creating new SortCommand class.
Tests and User Guide updated comprehensively.
Coding standard complied with although the use of import * could result in importing unnecessary classes.
Very useful enhancement, overall well done!

Copy link

@marlenekoh marlenekoh left a comment

Choose a reason for hiding this comment

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

@bava98 Nice job on implementing the sort command (: Remember to update the help command as well. Also, try to create a new branch for your feature (:

Please close this PR after reading review comments.

/**
* Sorts all persons in the address book by their name and lists them to the user.
*/
public class SortCommand extends Command{

Choose a reason for hiding this comment

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

Remember to add a whitespace before the curly brace (:

public UniquePersonList sortListByName() {
try {
UniquePersonList sortedList = new UniquePersonList(this.internalList);
sortedList.internalList.sort(new SortByName());

Choose a reason for hiding this comment

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

Good job on modifying a copy of the data instead of the original data (:

case HelpCommand.COMMAND_WORD: // Fallthrough
default:
return new HelpCommand();
case AddCommand.COMMAND_WORD:

Choose a reason for hiding this comment

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

Try to follow the indentation in the coding standard for switch statements (:

@@ -15,6 +13,8 @@
import seedu.addressbook.data.person.ReadOnlyPerson;
import seedu.addressbook.util.TypicalPersons;

import static org.testng.AssertJUnit.assertEquals;

Choose a reason for hiding this comment

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

Try not to commit unrelated changes in this PR

import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.*;

Choose a reason for hiding this comment

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

Wildcard imports violate the coding standard. Please use single imports and change your settings in Intellij: Settings > Editor > Code Style > Java > Imports and set both class count and names count to 99 (or any large number)

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