-
Notifications
You must be signed in to change notification settings - Fork 166
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.*; |
There was a problem hiding this comment.
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>() { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
add sort command