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][T11-2]Teo Xuan Wei #56

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

Conversation

shawn-t
Copy link

@shawn-t shawn-t commented Feb 11, 2019

No description provided.

- prints "addressbook is sorted in alphabetical order" when command is called.
- DummySort is reflected in the User Guide with the full list of the remaining commands (when command "help" is entered).
@liguanlong
Copy link

It is nice that you update the version of the address book from 1.0 to 1.1, and put blank lines among code to enhance readability in DummySortCommand.java.

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 Xuan Wei,

Good effort in updating the documentation, however you could be more accurate in describing what action does your pseudosort command perform - it only prints a sort success statement, and does not pseudo sort, nor provide any operations stubs that suggests so.

Also, you could have written an I/O or unit test for your new enhancement.

@@ -21,9 +21,9 @@
* Initializes the application and starts the interaction with the user.
*/
public class Main {

// test commit

Choose a reason for hiding this comment

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

this comment is not relevant to the enhancement. you may wish to remove this.


@Override
public CommandResult execute() {
return new CommandResult(MESSAGE_SUCCESS);

Choose a reason for hiding this comment

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

It would be good if you have also attempted the actual sort logic, instead of returning a print statement.
Note that you are not performing a pseudo-sort here, unlike what is written in your documentation. Only a success statement is printed.


public static final String MESSAGE_SUCCESS = "Address book is sorted in alphabetical order! ^^";

@Override

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.

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