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

Refactor add command for frequent expense and frequent income #183

Conversation

siddarth2824
Copy link

Resolves #182.

Changes:

  • Created and returned a new CommandResultupon creating a new AddFrequentExpenseCommand. The new CommandResult contains the index of the Expense Tab to switch to.
  • Created and returned a new CommandResultupon creating a new AddFrequentIncomeCommand. The new CommandResult contains the index of the Income Tab to switch to.

@siddarth2824 siddarth2824 added type.enhancement 👍 New feature or request priority.medium 🥈 Todo for current iteration labels Oct 25, 2020
@siddarth2824 siddarth2824 added this to the v1.3 milestone Oct 25, 2020
@siddarth2824 siddarth2824 requested a review from a team October 25, 2020 17:05
zhaojj2209
zhaojj2209 previously approved these changes Oct 25, 2020
Copy link

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -47,7 +52,8 @@ public CommandResult execute(Model model) throws CommandException {
} catch (DuplicateFrequentTransactionException e) {
throw new CommandException(e.getMessage());
}
return new CommandResult(String.format(MESSAGE_SUCCESS, toAdd));
Tab tabToSwitchTo = Tab.values()[EXPENSE_TAB_INDEX.getZeroBased()];
return new CommandResult(String.format(MESSAGE_SUCCESS, toAdd), tabToSwitchTo);

Choose a reason for hiding this comment

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

Suggested change
return new CommandResult(String.format(MESSAGE_SUCCESS, toAdd), tabToSwitchTo);
return new CommandResult(String.format(MESSAGE_SUCCESS, toAdd), Tab.EXPENSES);

Same for AddFrequentIncomeCommand as well.
This will remove the need to have an Index field to specify which Tab to switch to.

@codecov-io
Copy link

Codecov Report

Merging #183 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #183   +/-   ##
=========================================
  Coverage     73.66%   73.66%           
  Complexity      787      787           
=========================================
  Files           129      129           
  Lines          2442     2442           
  Branches        254      254           
=========================================
  Hits           1799     1799           
  Misses          547      547           
  Partials         96       96           
Impacted Files Coverage Δ Complexity Δ
...c/commands/frequent/AddFrequentExpenseCommand.java 84.61% <100.00%> (ø) 7.00 <0.00> (ø)
...ic/commands/frequent/AddFrequentIncomeCommand.java 84.61% <100.00%> (ø) 7.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 190a895...ef63324. Read the comment docs.

Copy link

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@yongping827 yongping827 left a comment

Choose a reason for hiding this comment

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

LGTM

@yongping827 yongping827 merged commit 78cb1fd into AY2021S1-CS2103T-W16-3:master Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.medium 🥈 Todo for current iteration type.enhancement 👍 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Direct user to appropriate tabs upon adding frequent transactions
4 participants