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.

Sign up for GitHub

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

Jump to bottom

Updated gradebook delete command #149

Merged
merged 6 commits into from Oct 23, 2018
Merged

Updated gradebook delete command #149

merged 6 commits into from Oct 23, 2018

Conversation

tristyxxnana
Copy link

  • added tests for gradebook add parser

@tristyxxnana tristyxxnana added the Changes in Code This PR contains changes to code label Oct 23, 2018
@tristyxxnana tristyxxnana added this to the v1.3 milestone Oct 23, 2018
@tristyxxnana tristyxxnana requested a review from a team October 23, 2018 15:18
@coveralls
Copy link

Coverage Status

Coverage increased (+3.4%) to 69.096% when pulling b72ebfd on tristyxxnana:UpdatedGradebookDeleteCommand into 5d0a065 on CS2113-AY1819S1-T16-1:master.

harriuscai
harriuscai approved these changes Oct 23, 2018
View reviewed changes
Copy link
Collaborator

@harriuscai harriuscai left a comment

Choose a reason for hiding this comment

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

A few comments to address, but overall a good job 👍 . Please try to use ModuleCode instead of plain strings in a future PR :)

src/main/java/seedu/address/logic/commands/GradebookDeleteCommand.java Show resolved Hide resolved
src/main/java/seedu/address/logic/parser/GradebookEditCommandParser.java Show resolved Hide resolved
src/test/java/seedu/address/logic/commands/GradebookAddCommandTest.java
gradebookComponentName,
gradebookMaxMarks,
gradebookWeightage)),
new CommandHistory(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I advise you to break this up by at least creating the Gradebook object beforehand i.e. above the call to assertCommandSuccess(). That would improve the readability of your code.

Likewise, you can also create the CommandHistory object beforehand if you feel it improves readability :)

src/test/java/seedu/address/logic/parser/GradebookDeleteCommandParserTest.java Show resolved Hide resolved
@harriuscai harriuscai added Changes in Testing Code This PR contains changes to testing code Approved with Minor Comments This PR contains comments by reviewer for potential improvements. labels Oct 23, 2018
@harriuscai harriuscai merged commit f8c656e into CS2113-AY1819S1-T16-1:master Oct 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harriuscai harriuscai harriuscai approved these changes

Assignees
No one assigned
Labels
Approved with Minor Comments This PR contains comments by reviewer for potential improvements. Changes in Code This PR contains changes to code Changes in Testing Code This PR contains changes to testing code
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tristyxxnana @coveralls @harriuscai