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][W13-3]Ruhani Suri #159

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

Conversation

suriruhani
Copy link

Add a 'history' command that lists all past user entered commands in reverse chronological order.
Format: history

@case141
Copy link

case141 commented Feb 12, 2019

Nice feature. You may want to add some I/O tests as well as update the user guide too.

@ToonDragon
Copy link

Excellent usage of the stack to store all the commands since they follow a first in last out concept. A possible future improvement could be to allow users to select and repeat past commands like that found in the console commands when you press the up arrow key.

Other than that, the code is well written, functions are well defined and variables are properly named.

@case141
Copy link

case141 commented Feb 12, 2019

A possible future improvement could be to allow users to select and repeat past commands like that found in the console commands when you press the up arrow key.

@DarrenDragonLee Nice suggestion, that would fall under the undo command.

Copy link

@ewaldhew ewaldhew left a comment

Choose a reason for hiding this comment

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

Good enhancement. Nice use of Java library functions. :)

Close the PR after reading @suriruhani

@@ -24,7 +25,7 @@

/** Version info of the program. */
public static final String VERSION = "AddressBook Level 2 - Version 1.0";

public static Stack<String> CommandHistory = new Stack<>();

Choose a reason for hiding this comment

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

Does this need to be public?

Copy link
Author

Choose a reason for hiding this comment

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

I have changed it to private in Main.java now, however it is public in Parser.java and HistoryCommand.java. I think that is needed because I will be passing the CommandHistory Stack from main to parser to the the command handling class. Please let me know if this can be done in a better way and why this approach is not preferred.

Copy link

@ewaldhew ewaldhew Feb 16, 2019

Choose a reason for hiding this comment

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

It isn't that its not preferred, as long as you have a reason to make things publicly accessible then that's fine :)

Remember to close this PR!

src/seedu/addressbook/commands/HistoryCommand.java Outdated Show resolved Hide resolved
@suriruhani
Copy link
Author

A possible future improvement could be to allow users to select and repeat past commands like that found in the console commands when you press the up arrow key.

@DarrenDragonLee Nice suggestion, that would fall under the undo command.

I think @DarrenDragonLee meant being able to choose a particular past command and repeat it. This is different from redo-ing the last executed command. This seems like a tricky yet interesting feature to implement. We can think about including it in V2.0. Thanks Darren!

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