refactor: extract and simplify holiday parsing (PH/SH) #543
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, the SH selector assumed holiday types were defined in chronological order in the data files. If winter holidays (Dec-Jan) were listed before spring holidays (Apr-May), dates in spring would incorrectly return false. Now all holiday ranges are collected first and sorted chronologically before checking, making the code more robust against different data orderings (e.g., if/when we switch to other sources like OpenHolidays API).
Extracted three helper functions to simplify the SH selector logic: - getDateNumber(month, day): Convert month/day to comparable number - collectSHRangesForYear(holidays, year): Collect all ranges from all holiday types - sortRangesByStartDate(ranges): Sort ranges chronologically This makes the code more readable and easier to maintain. The SH selector now uses these helpers instead of inline calculations.
- Created createPublicHolidaySelector() to encapsulate PH selector logic - Reduces parseHoliday complexity by 57 lines - PH parsing now uses single function call instead of inline closure
- Created createSchoolHolidaySelector() to encapsulate SH selector logic - Reduces parseHoliday complexity by ~80 lines - SH parsing now uses single function call instead of inline closure - Removed obsolete FIXME comment (from 2015, Issue opening-hours#75) - Added clarifying comment: SH does not support move days by design (school holidays are date ranges, not single-day events like PH)
…loop - Extract single holiday token parsing into dedicated helper function - Replace for-loop with clearer while-loop in parseHoliday - Explicit comma skipping makes control flow more obvious - Reduce code duplication by using target_array variable The helper function parseSingleHolidayToken handles PH/SH token parsing and returns the new position, making the main loop simpler and easier to understand. The while-loop eliminates the confusion of having 'at' modified both in the loop header (at++) and in the loop body.
Collaborator
Author
|
Since the tests cover the area quite well and they are green, I'm merging this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The
parseHoliday()function was ~180 lines long with complex nested logic for handling both public holidays (PH) and school holidays (SH). This refactoring improves readability by extracting reusable helper functions and simplifying the main parsing loop.Changes
getDateNumber(month, day)- converts to comparable numbercollectSHRangesForYear()- collects all SH ranges for a yearsortRangesByStartDate()- sorts ranges chronologicallycreatePublicHolidaySelector()- PH selector as standalone functioncreateSchoolHolidaySelector()- SH selector as standalone functionparseSingleHolidayToken()- handles single PH/SH tokenparseHoliday(): Reduced from ~180 lines to ~30 linesSide effect
SH parsing is now robust against non-chronological data order (ranges are sorted before processing). This simplifies future migration to alternative data sources (e.g. OpenHolidays API), which I am currently exploring for #300.
Breaking Changes
None. Internal refactoring only.
Happy New Year! 🥳