Implement method for finding coarsest ZonedTimeGrain #230
Conversation
QubitPi
force-pushed
the
get-coarsest-ZonedTimeGrain
branch
from
5686dd8
to
fd99497
Compare
QubitPi
force-pushed
the
get-coarsest-ZonedTimeGrain
branch
2 times, most recently
from
1155253
to
816b0f8
Compare
| */ | ||
| public static ZonedTimeGrain getCoarsestTimeGrain(Collection<ZonedTimeGrain> zonedTimeGrains) { | ||
| return zonedTimeGrains.stream() | ||
| .sorted(Comparator.comparing(ZonedTimeGrain::getEstimatedDuration)) |
There was a problem hiding this comment.
Why are we comparing based on estimated duration instead of using the GranularityComparator?
There was a problem hiding this comment.
@cdeszaq Because GranularityComparator compares by taking PhysicalTable not ZonedTimeGrain. I can write a comparator for ZonedTimeGrain if that's you would like to do.
There was a problem hiding this comment.
Good point. That's unfortunate. If the intent of this helper method is to make it easy to select the coarsest grain from a collection of PhysicalTables, perhaps we should convert this method to do that, and then use the GranularityComparator?
| @@ -326,6 +328,32 @@ class IntervalUtilsSpec extends Specification { | |||
| DAY | ["2012-02-03/2017-04-04"] | ["2012-02-03/2012-05-04", "2017-02-03/2017-04-04"] | |||
| } | |||
|
|
|||
| @Unroll | |||
| def "getCoarsestTimeGrain returns duration of #expected among #duration1, #duration2, #duration3"() { | |||
There was a problem hiding this comment.
Why are we testing / asserting behavior about durations when we are generally dealing with ZonedTimeGrain?
There was a problem hiding this comment.
@cdeszaq By digging into the source code, I found that comparing ZonedTimeGrain is the same thing as comparing duration. Would you like me to change the testing behavior anyway?
There was a problem hiding this comment.
Yes. It's better to keep the tests focused on the external interface and the behavior, and to try to not duplicate what the code under test is doing when testing it.
| duration1 | duration2 | duration3 | expected | ||
| 1 | 2 | 3 | 1 | ||
| 1 | 1 | 3 | 1 | ||
| 3 | 3 | 3 | 3 |
There was a problem hiding this comment.
Any reason we're using mocked grains instead of using real grains and giving them a timezone?
| where: | ||
| duration1 | duration2 | duration3 | expected | ||
| 1 | 2 | 3 | 1 | ||
| 1 | 1 | 3 | 1 |
There was a problem hiding this comment.
We should make sure to test what happens when we have the same grain but different time zones.
| /** | ||
| * Returns the coarsest ZonedTimeGrain among a set of ZonedTimeGrains. | ||
| * <p> | ||
| * If the set of ZonedTimeGrains is empty, return null. |
There was a problem hiding this comment.
Why are we returning null here? Do we really expect null to be a valid return value? If we do, we should make that explicit in the return type by using Optional and force callers to deal with the fact that it could come back as null.
QubitPi
force-pushed
the
get-coarsest-ZonedTimeGrain
branch
from
3a33f14
to
c2c5e23
Compare
| MINUTE | HOUR | DAY | 'America/Chicago' | 'America/Los_Angeles' | 'America/Phoenix' | DAY | 'America/Phoenix' | 'different grains and different time zones' | ||
| } | ||
|
|
||
| def "getCoarsestTimeGrain returns null on empty input time grain collections"() { |
There was a problem hiding this comment.
Should be returns empty, not null, right?
|
|
||
| def "getCoarsestTimeGrain returns null on empty input time grain collections"() { | ||
| expect: | ||
| IntervalUtils.getCoarsestTimeGrain(Collections.emptyList()) == Optional.empty() |
There was a problem hiding this comment.
Just use groovy's list literal for an empty list: []
| timeGrain1 | timeGrain2 | timeGrain3 | timeZone1 | timeZone2 | timeZone3 | expectedTimeGrain | expectedTimeZone | description | ||
| DAY | DAY | DAY | 'America/Chicago' | 'America/Los_Angeles' | 'America/Phoenix' | DAY | 'America/Chicago' | 'same grain but different time zones' | ||
| MINUTE | HOUR | DAY | 'America/Phoenix' | 'America/Phoenix' | 'America/Phoenix' | DAY | 'America/Phoenix' | 'same time zone but different grans' | ||
| MINUTE | HOUR | DAY | 'America/Chicago' | 'America/Los_Angeles' | 'America/Phoenix' | DAY | 'America/Phoenix' | 'different grains and different time zones' |
There was a problem hiding this comment.
Could we add some more cases that use week, month, quarter, and year? Some of those (ie. week) are odd in terms of how they line up, so that would be good to double-check.
No description provided.