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

Separate command validation from execution #85

Conversation

ryoarmanda
Copy link

@ryoarmanda ryoarmanda commented Oct 15, 2019

Summary:

  • The Command class now has two abstract methods, validate and execute, and one concrete method, run.
  • run executes the command with validation (just like the previous execute) and execute is the same but with NO validation.
  • validate and execute are supposed to be internal methods, while run is exposed for public use (e.g. LogicManager, tests, etc).

Reason for separation:

I have already modified test names and utilities to adhere to this naming. All tests have passed.

Implications for future developments:

  • All commands have to implement validate even when it does not need validation (look at what I did for exit, etc).
  • You still develop new commands the same way as usual, with execute.

@ryoarmanda ryoarmanda added this to the v1.2 milestone Oct 15, 2019
@coveralls
Copy link

Pull Request Test Coverage Report for Build 89

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 14 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.3%) to 60.409%

Files with Coverage Reduction New Missed Lines %
file:/home/travis/build/AY1920S1-CS2103T-T11-1/main/src/main/java/seedu/address/logic/commands/EditCommand.java 1 98.33%
file:/home/travis/build/AY1920S1-CS2103T-T11-1/main/src/main/java/seedu/address/logic/commands/RemindersCommand.java 3 0.0%
file:/home/travis/build/AY1920S1-CS2103T-T11-1/main/src/main/java/seedu/address/logic/commands/BudgetCommand.java 5 0.0%
file:/home/travis/build/AY1920S1-CS2103T-T11-1/main/src/main/java/seedu/address/logic/commands/EventCommand.java 5 0.0%
Totals Coverage Status
Change from base Build 74: 0.3%
Covered Lines: 1033
Relevant Lines: 1710

💛 - Coveralls

Copy link

@qweiping31415 qweiping31415 left a comment

Choose a reason for hiding this comment

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

Noted on the changes to split old execute command(currently called run), into a method validate that throws Exception if the command is not executable, and execute only when it passes validate.

@qweiping31415 qweiping31415 merged commit 2fb0c5e into AY1920S1-CS2103T-T11-1:master Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants