gitgabrio commented on PR #3788:
URL:
https://github.com/apache/incubator-kie-kogito-runtimes/pull/3788#issuecomment-2511270558
> @Abhitocode hi, as I reviewed #3795 and this PR is related, I share
feedback also on this PR.
> ### 01
>
> As new potential contributor to runtimes and calendars, reading the code:
>
> ```
> businessCal.rollCalendarAfterHolidays(calendar, false);
> ```
>
> It is difficult to understand the `false`. I mean, without checking the
actual method code, it is not clear what that false means. I understand that
author of the code may not have this problem, but new contributors may be in
similar situation as me.
>
> What do you think about introducing two separate API methods:
>
> * `businessCal.rollCalendarAfterHolidays(calendar); //
businessCal.rollCalendarAfterHolidays(calendar, false);`
>
> * `businessCal.rollCalendarAfterHolidaysAndResetTime(calendar);
//businessCal.rollCalendarAfterHolidays(calendar, true);`
>
>
> or maybe change the boolean to some enum?
>
> * `businessCal.rollCalendarAfterHolidays(calendar, RESET_TIMER.NO);`
>
>
> ### 02
>
> ```
> @Test
> void rollCalendarToNextWorkingDay() {
> List<Integer> workingDays = IntStream.range(Calendar.MONDAY,
Calendar.SATURDAY).boxed().toList();
> List<Integer> weekendDays = Arrays.asList(Calendar.SATURDAY,
Calendar.SUNDAY);
> Properties config = new Properties();
> config.setProperty(START_HOUR, "9");
> config.setProperty(END_HOUR, "17");
> config.setProperty(WEEKEND_DAYS,
weekendDays.stream().map(String::valueOf).collect(Collectors.joining(",")));
> BusinessCalendarImpl businessCal =
BusinessCalendarImpl.builder().withCalendarBean(new
CalendarBean(config)).build();
>
> workingDays.forEach(workingDay -> {
> Calendar calendar = getCalendarAtExpectedWeekDay(workingDay);
> businessCal.rollCalendarToNextWorkingDay(calendar, false);
>
assertThat(calendar.get(Calendar.DAY_OF_WEEK)).isEqualTo(workingDay);
> });
> weekendDays.forEach(weekendDay -> {
> Calendar calendar = getCalendarAtExpectedWeekDay(weekendDay);
> businessCal.rollCalendarToNextWorkingDay(calendar, false);
>
assertThat(calendar.get(Calendar.DAY_OF_WEEK)).isEqualTo(Calendar.MONDAY);
> });
> }
> ```
>
> from reading this test ^, is `rollCalendarToNextWorkingDay` actually
rolling? It seems we roll `workingDay` and after rolling we compare for
equality with the original value. I would expect if we roll, that new value is
`+1`. I mean I would expect sth like:
>
> ```
> workingDays.forEach(workingDay -> {
> Calendar calendar = getCalendarAtExpectedWeekDay(workingDay);
> businessCal.rollCalendarToNextWorkingDay(calendar, false);
>
assertThat(calendar.get(Calendar.DAY_OF_WEEK)).isEqualTo(workingDay + 1);
> });
> ```
>
> Sorry if my feedback is not useful, or it proves my lack of knowledge
about the domain. Thank you for your work.
HI @jomarko
The scope of that test is to verify the behavior in tow different situations
- and I agree it could be split in two.
Anyway, in the case of "workingDays", we have to _check that the calendar
does not roll_, and that's why the assertion is that the `Calendar.DAY_OF_WEEK`
does not change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
