HTML-797 Add Velocity date addition and utility functions #252
Conversation
| /** | ||
| * Date utility functions useful for HTML forms | ||
| */ | ||
| public Date yesterday(String date) { |
There was a problem hiding this comment.
@ssmusoke I'm not sure what field "6" refers to, but I don't think it's the correct one (at least from a quick look here): https://docs.oracle.com/javase/7/docs/api/constant-values.html#java.util.Calendar.DATE. I would imagine if you added a unit test that spanned a calendar year (eg. "2021-01-03" minus 7 days) it would not pass.
I don't love using the value like this here. I'd rather we just support a couple of general purpose methods. ie. instead of a generic "addInterval", just have an "addDays" and "addMonths" function. Those 2 functions probably cover 99% of all use cases you'll run into. You could also add "addSeconds" if you want to retain more flexibility as a catch-all, as this can be used for addMinutes, addHours, addDays, etc just with simple math. The addMonths function can be used to do addYears just with multiplying by 12. But the String interval is a bit too abstract.
There was a problem hiding this comment.
Makes sense to me, thanks @mseaton !
There was a problem hiding this comment.
@mseaton Thanks for the feedback let me update the PR - makes alot of sense
There was a problem hiding this comment.
@mseaton Requested changes added to the PR, I actually feel much better about it now
| public Date addDays(Date date, String value) { | ||
| Calendar cal = Calendar.getInstance(); | ||
| cal.setTime(date); | ||
|
|
There was a problem hiding this comment.
How about this? org.joda.time.LocalDate.fromDateFields(date).plusDays(Integer.parseInt(value))
There was a problem hiding this comment.
@dkayiwa Works, I had not looked into the Joda time option now I see its an included dependency & much cleaner
| Calendar cal = Calendar.getInstance(); | ||
| cal.setTime(date); | ||
|
|
||
| cal.add(Calendar.MONTH, Integer.parseInt(value)); |
There was a problem hiding this comment.
How about this? org.joda.time.LocalDate.fromDateFields(date).plusMonths(Integer.parseInt(value))
There was a problem hiding this comment.
@dkayiwa Thanks update made
Description of what I changed
Add date Velocity addition and utility functions
Issue I worked on
see https://issues.openmrs.org/browse/HTML-797
Checklist: I completed these to help reviewers :)
My pull request only contains ONE single commit
(the number above, next to the 'Commits' tab is 1).
No? -> read here on how to squash multiple commits into one
My IDE is configured to follow the code style of this project.
Yes
existing code that was well tested you do not have to add tests)
Yes
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
Yes
Yes
Yes