-
Notifications
You must be signed in to change notification settings - Fork 5
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
Get monthly fee + Get 3 months fee (this month + prev 2 months) function (Only supports non-recurring session for now) #118
Get monthly fee + Get 3 months fee (this month + prev 2 months) function (Only supports non-recurring session for now) #118
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it and works well!
I like how you compared the dateTime with the compareTo() method, clean af haha.
Regarding the 3 month command, I was thinking of using it for the Home fee section. So not too sure if we need a command for it.
Haha I agree ah! For the 3 month command thingy, I was also thinking that we might not need the command. So haven't added it to the UG yet. Probably can keep the command for testing purposes, and remove it after the UI that uses the 3 month function is done. |
Codecov Report
@@ Coverage Diff @@
## master #118 +/- ##
============================================
- Coverage 69.54% 66.40% -3.14%
- Complexity 495 510 +15
============================================
Files 83 88 +5
Lines 1658 1765 +107
Branches 184 197 +13
============================================
+ Hits 1153 1172 +19
- Misses 439 521 +82
- Partials 66 72 +6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it works well! Just maybe could reduce repeat but i can't remember if we're marked on that. If not I think this is good!
} | ||
|
||
@Override | ||
public CommandResult execute(Model model) throws CommandException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im understanding that getPrev3 month is getMonth applied to every student, and for 3 months. I guess it'd be considered less "repeated code" if this method was built on getMonth. possibly using getMonth's methods from within getPrev3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok good suggestion, the only common place for both code base is on model. Hence, I have abstracted out the get fees code over to model. Do have a look, the output should still be the same. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super neat, hadn't thought of putting it at model!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
* Gets current month and previous 2 months expected monthly fee based on the current list of sessions | ||
*/ | ||
public class GetPrev3MonthFeeCommand extends Command { | ||
public static final String COMMAND_WORD = "3Month_Fee"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be consistent with the other commands if not capitalised. eg "past_fees" / "3_fees" / "3_month_fees"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh right! I have changed it to 3_month_fees instead.
public static final String COMMAND_WORD = "3Month_Fee"; | ||
|
||
public static final String MESSAGE_USAGE = COMMAND_WORD + ": Shows the total expected monthly fee" | ||
+ "for this month + previous 2 months"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Shows the totalled fees per month, for the past 3 months." Or something of the sort could be somewhat more accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright! Change from that to "Shows the totalled fees per month, for this month + previous 2 months" instead.
closes #81 |
Update with the following feature:
fee n/John Doe m/1 y/2021
) - Returns monthly fee of a particular student on the specific month and yearConsiderations:
Future Issues / Improvements:
Only works on the filtered student, find_student will change the value (Could make it for all students instead of filtered students)#81 #82