-
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 215 #219
Fix 215 #219
Conversation
I was thinking more in the line of "the two methods should point to the same string" rather than "the two different strings should have the same content", but that might take a bit of work. Also related PR #208 not sure if theres overlaps? |
Codecov Report
@@ Coverage Diff @@
## master #219 +/- ##
============================================
+ Coverage 71.07% 71.10% +0.03%
- Complexity 552 554 +2
============================================
Files 87 87
Lines 1884 1886 +2
Branches 151 151
============================================
+ Hits 1339 1341 +2
Misses 486 486
Partials 59 59
Continue to review full report at Codecov.
|
Yeap i think it overlaps x.x |
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 the old implementation of getMessageConstraint is better than this one. I have given more details about why I think so in one of the comments
private String getMessageConstraint() { | ||
return this.getMacronutrientType() + " amount can only take in value larger than 0 and less than 1000"; | ||
} |
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 we should move "amount can only ..."
outside to be a string called MESSAGE_CONSTRAINT_SUFFIX
. The old implementation works just as fine as the new one. The old one even has fewer repeated code. So why do we need to change?
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.
It is to make sure all of them point to the same message so that the messages shown will be consistent everywhere
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.
The old one also makes sure all of them point to the same message, isn't it
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
Closes #215