-
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
Fix error messages for payment class and refactored code #163
Fix error messages for payment class and refactored code #163
Conversation
Codecov Report
@@ Coverage Diff @@
## master #163 +/- ##
============================================
+ Coverage 78.52% 79.42% +0.90%
- Complexity 685 696 +11
============================================
Files 95 95
Lines 2095 2095
Branches 255 258 +3
============================================
+ Hits 1645 1664 +19
+ Misses 374 361 -13
+ Partials 76 70 -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.
Good job on the thorough test cases! Left a few comments and suggestions. 😄
|
||
if (zeroPaymentVal.equals(existingPaymentValue) && newPayByDate == null && existingPayByDate == null) { | ||
boolean hasZeroPaymentValue = ZERO_PAYMENT_VAL.equals(existingPaymentValue); |
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.
As "0" is not equal to "0.00", perhaps a simpler implementation would be
boolean hasZeroPaymentValue = ZERO_PAYMENT_VAL.equals(existingPaymentValue); | |
boolean hasZeroPaymentValue = Double.parseDouble(existingPaymentValue) == 0; |
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.
Thanks! Have committed the above suggestions
@@ -126,7 +126,7 @@ public String getOverdueStatus() { | |||
} else if (payByDateAsString.equals("-")) { | |||
return "No (Pay-by date not set)"; | |||
} else { | |||
return "No (by " + payByDateAsString + ")"; | |||
return "No (Next payment date by: " + payByDateAsString + ")"; |
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 this will clear the confusion that our testers had 😅
Resolved #147, #126
For #147, changed payment message from
Overdue: No (By: XXX)
toOverdue: No (Next payment date by: XXX)
to make it more explicit that it refers to the next upcoming payment date to be expected.For #126, adjusted according to what was suggested and changed error message to
tutee index is invalid
.Added test cases for payment commands as well