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.7][T12-2]Zhou Zegang #431

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ZZG229
Copy link

@ZZG229 ZZG229 commented Sep 12, 2018

add sort command

Copy link

@eugenepeh eugenepeh 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 completing the activity.
Some comments added. Please close the PR after reading the comments.

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.

remember to disable intellij auto import wildcard
Why is wild card import bad?


@Override
public CommandResult execute() {
Comparator<? super ReadOnlyPerson> comparator = new Comparator<ReadOnlyPerson>() {

Choose a reason for hiding this comment

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

Good! another way would be to simplify it to use lambda expression.

@Test
public void parse_SortCommand_parsedCorrectly() {
final String input = "sort";
parseAndAssertCommandType(input, SortCommand.class);

Choose a reason for hiding this comment

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

good, but perhaps you can add a test to verify that your sort command is working properly?

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.

3 participants