-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/10 conditional holidays #22
Conversation
TODO: alternative date holidays
instead of int month and day
incl. CHANGELOG.md, README.md, tests, holidays with alterntive
src/main/java/org/itsallcode/holidays/calculator/logic/conditions/Condition.java
Show resolved
Hide resolved
@Override | ||
public boolean applies(Year year) { | ||
if (pivot == null) { | ||
throw new UnspecifiedPivotDateException("Cannot apply DayOfWeekCondition with unspecified pivot date."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If pivot
is mandatory, you should make it a final
field and check that it is not null in the constructor. If you want to keep withPivotDate()
for constructing an object, consider using a builder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, if I understood correctly.
My idea was to support two flavors of Conditions:
- a) with individual pivot date
- b) with pivot date inherited from Holiday, example:
Condition isSunday = new DayOfWeekCondition(DayOfWeek.SUNDAY);
Holiday KONINGSDAG = new FixedDateHoliday("holiday", "Koningsdag", MonthDay.of(4, 27))
.withAlternative(isSunday, MonthDay.of(4, 26));
In case b) the isolated condition is incomplete and unusable as long as it is not added to a holiday.
Do I understand your proposal correctly then, to
- create a class ConditionBuilder and
- pass an instance of that to Holiday
- in order to instantiate the Condition
- only after that and
- from within the holiday instance and
- including the pivot date copied from the date of the holiday?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next step: try approach as described above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kaklakariada, I started to use a builder for conditions and it feels quite good, see branch 28-refactor-conditional-holidays.
I plan to use a builder for Holidays, too.
Stay tuned for updates 😄
fixes #10