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][T09-2] Zhang Jingchen #22

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

Conversation

jingchen-z
Copy link

Added a new command: "last" which displays the last command user typed in previously.

Copy link

@RubaP RubaP left a comment

Choose a reason for hiding this comment

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

Interesting feature and very nice attempt. Always make sure that create a PR with a working piece of changes as expected. Not only for the coding practices but also when you make PRs for the project. Please read the review comments and close the PR.

Format: `last`

****
Shows the latest command typed in.
Copy link

Choose a reason for hiding this comment

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

Interesting enhancement! Good that you have updates the user guide. But when you provide examples in the user guide try to stick to valid examples, rather than using the terms 'something' to be more professional with you work and not to confuse the user who is reading the doc.

@@ -456,6 +456,21 @@ public void execute_find_matchesIfAnyKeywordPresent() throws Exception {
expectedList);
}

/**
Copy link

Choose a reason for hiding this comment

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

Avoid unnecessary comment blocks while making a PR

* If yes then append the lastCommand for later execution.
*/
private String checkLastCommand(String userCommandText) throws Exception {
if( userCommandText.substring(0, Math.min(userCommandText.length(), 4)).equals("last")) {
Copy link

Choose a reason for hiding this comment

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

Avoid the magical string usage like "last". You have already defined it as a constant COMMAND_WORD. So please make use of it

*/
private String checkLastCommand(String userCommandText) throws Exception {
if( userCommandText.substring(0, Math.min(userCommandText.length(), 4)).equals("last")) {
userCommandText = userCommandText.concat(" ").concat(lastCommand);
Copy link

Choose a reason for hiding this comment

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

It seems this is logically wrong. if the user type "last" and the previous command is "add something", then you concatenate both here before parsing. But the parser will not understand the concatenated string as it is not a valid command. I hope this process should be carried out once you parse the command (the application realizes it is the command LAST).

@@ -58,6 +58,12 @@ public void listCommand_parsedCorrectly() {
parseAndAssertCommandType(input, ListCommand.class);
}

@Test
public void LastCommand_parsedCorrectly() {
Copy link

Choose a reason for hiding this comment

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

Try to work on adding multiple test cases in the future. e.g. user type last without any last command available

@RubaP RubaP added the Reviewed label Mar 15, 2019
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