Migrated DateAndTime to TimeAndDate to support Java8 Time, and depreca… #1210
Conversation
…te Timestamps and java.util.Date code.
|
Ugh, just one minor comment. Apparently I was half asleep and set approve anyway 🤷♂️ |
Appreciate the approval, I fixed the naming, and a few other minor improvements. If you want to have another look, that would be great! |
| * @return a random birthday between {@code minAge} and {@code maxAge} years ago from now. | ||
| * Negative {@code minAge} and {@code maxAge} are supported. | ||
| */ | ||
| public LocalDate birthday(int minAge, int maxAge) { |
There was a problem hiding this comment.
Make sure they're not negative or zero?
There was a problem hiding this comment.
Okay, wasn't sure that was intentional.
| Instant now = Instant.now(); | ||
| Instant future = timeAndDate.future(5, 4, TimeUnit.SECONDS); | ||
| assertThat(future) | ||
| .isBetween(now.plusMillis(3500), now.plusMillis(5500)); |
There was a problem hiding this comment.
Make constants for these magic numbers or add a comment that clarifies. I'm sure it makes sense if I read the code but less digging is better 😉
There was a problem hiding this comment.
I agree, but I mostly ported the old code to the new types. I'll have a look later.
There was a problem hiding this comment.
This is actually great feedback. I'm reading this code now, and to me, it doesn't really make sense. Why are 5 and 4 flipped around? It would make more sense to me to have a future date between 4 and 5 seconds into the future, not between 5 and 4. What do you think? (the challenge is that the behaviour of this method would be slightly different from the old one, so maybe this should throw an exception if argument 2 is lower than argument 1? Thoughts?
There was a problem hiding this comment.
Hahaha I was actually on about the 3500 & 5500.
But ya I agree changing the item you're talking about seems better too and enforcing the condition as well.
| final LocalDate from = now.minusYears(18).toLocalDate(); | ||
| final LocalDate to = now.minusYears(65).toLocalDate(); |
There was a problem hiding this comment.
Use the constants from the new class?
|
@kingthorin I've merged this PR, but I will take your feedback into account. Only reason I merged is so I can ask @asolntsev to use this faker instead in the other PR. |
bodiam
changed the title
…te Timestamps and java.util.Date code.
Should address #1183