-
Notifications
You must be signed in to change notification settings - Fork 119
Small fix for recurring time window testcase #522
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
Conversation
| TimeSpan minGap = TimeSpan.FromDays(DaysPerWeek); | ||
|
|
||
| foreach (DayOfWeek dayOfWeek in sortedDaysOfWeek) | ||
| foreach (DayOfWeek dayOfWeek in sortedDaysOfWeek.Skip(1)) |
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 don't like this Skip syntax- it seems less readable, could you add a comment why we skip the first?
I'd prefer we don't do a skip and explicitly check if prev == firstDay or if prev != null instead.
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.
How about this? Using index to skip the first day, instead of .Skip(1)
List<DayOfWeek> sortedDaysOfWeek = SortDaysOfWeek(daysOfWeek, firstDayOfWeek);
DayOfWeek firstDay = sortedDaysOfWeek.First(); // the closest occurrence day to the first day of week
DayOfWeek prev = firstDay;
TimeSpan minGap = TimeSpan.FromDays(DaysPerWeek);
for (int i = 1; i < sortedDaysOfWeek.Count(); i++)
{
DayOfWeek dayOfWeek = sortedDaysOfWeek[i];
TimeSpan gap = TimeSpan.FromDays(CalculateWeeklyDayOffset(dayOfWeek, prev));
if (gap < minGap)
{
minGap = gap;
}
prev = dayOfWeek;
}| private static List<DayOfWeek> SortDaysOfWeek(IEnumerable<DayOfWeek> daysOfWeek, DayOfWeek firstDayOfWeek) | ||
| { | ||
| List<DayOfWeek> result = daysOfWeek.ToList(); | ||
| List<DayOfWeek> result = daysOfWeek.Distinct().ToList(); // dedup |
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.
Could we take a Set instead so we don't need to Distinct it?
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 personally like the Distinct one, it is more readable. If we use a set here, we will either need to write two lines of code (1 line for new set and 1 line for ToList) or we will write List<DayOfWeek> result = new HashSet<DayOfWeek>(daysOfWeek).ToList(); I feel this single line ugly. I believe the background implementation of Distinct is based on hash set. Do you have any other concern about using Distinct
…-Dotnet into zhiyuanliang/fix-recurring-timewindow
|
@rossgrambo I am going to merge this pr. If you have any further concern about what we've discussed in the comments, please let me know. |
Why this PR?
The validation of recurrence configuration has been decoupled from recurrence evaluation. I forgot to update the related testcase. Also, update the implementation to make it more readable, not a bug fix.
related PR: #266
Visible Changes
-changes that are visible to developers using this library-