compare week-year not calendar year in compareWeeks#408
Conversation
|
It has just occurred to me - the same issue may occur with WEEK_OF_MONTH. Maybe an alternative approach would be to initially compare days since the epoch. If the difference between the dates is less than a week, then compare WEEK_OF values. If they are the same, then return 0, otherwise return the result of the epoch comparison. |
AbstractCalendarValidator.compare ordered WEEK_OF_YEAR on Calendar.YEAR then the week number, but a week belongs to its own week-year, which at the year boundary differs from the calendar year (31 December can be week 1 of the following year). So compareWeeks reported 31 December and 1 January of the same calendar year as the same week, and treated two dates that share one week across the boundary as different weeks. WEEK_OF_MONTH resets in the same way at the start of a month. Order both week fields by the distance in days first: dates a week or more apart are always in different weeks, and nearer dates share a week only when the week number also matches. This drops the week-year lookup and its unsupported-calendar fallback. WEEK_OF_MONTH keeps its previous results; only the WEEK_OF_YEAR boundary case changes. Tests cover both fields across the year boundary.
sahvx655-wq
force-pushed
the
calendar-week-year-compare
branch
from
684f5ea to
55526f4
Compare
|
Took the epoch route you suggested. compare() now sends both WEEK_OF_YEAR and WEEK_OF_MONTH through one helper that looks at the day gap first: a week or more apart is always a different week, and within a week they only count as the same week when the week number also matches. That removes the year-boundary false match on WEEK_OF_YEAR and drops the getWeekYear()/isWeekDateSupported() machinery the earlier version leaned on. On WEEK_OF_MONTH: the old path compared MONTH before WEEK_OF_MONTH, so a December day and a January day never collided the way the year-boundary case did, which is why it wasn't visibly broken. Routing it through the same guard keeps every existing WEEK_OF_MONTH result and makes that invariant explicit rather than incidental. I added boundary tests for both fields and the full build (mvn clean verify with checkstyle/spotbugs/pmd, 1092 tests) is green. |
|
There’s another issue here - the WEEK_OF_* values depend on the Calendar settings, so only make sense if the calendars are the same according to .equals(). Does it make ever make sense to compare calendars in different time zones? However, that can be handled separately. |
|
On the WEEK_OF_* values depending on calendar settings, and comparing across time zones: I agree those only really hold up when the calendars match, but since you noted it can be handled separately I've kept it out of this PR so the change stays scoped to the week-boundary fix. Happy to follow up on it on its own. |
|
Thanks! |
…that span a year gap (#408)
2 participants
compareWeeks reports 31 December and 1 January of the same calendar year as the same week. AbstractCalendarValidator.compare ordered the WEEK_OF_YEAR comparison on Calendar.YEAR and then the week number, but a week belongs to its own week-year, and at the turn of the year that week-year differs from the calendar year. Under the US locale 31 Dec 2018 has WEEK_OF_YEAR 1 and a week-year of 2019, so against 1 Jan 2018 (also WEEK_OF_YEAR 1) the year and the week number both match and two dates roughly 52 weeks apart compare equal. The mirror case fails the other way: 31 Dec 2018 and 1 Jan 2019 share one week yet compare as different weeks because their calendar years differ. WEEK_OF_MONTH resets the same way at the start of a month.
Following the review, this now orders both WEEK_OF_YEAR and WEEK_OF_MONTH by the distance in days first: dates a week or more apart are always in different weeks, and nearer dates share a week only when the week number also matches. That removes the year-boundary false match for WEEK_OF_YEAR and is correct by construction for WEEK_OF_MONTH, which keeps its previous results. It also drops the earlier getWeekYear() lookup and its unsupported-calendar fallback. Tests cover both fields across the year boundary and the full suite passes.
mvn; that'smvnon the command line by itself.