0
\$\begingroup\$

I hava a collection of schedules - EventSchedules. I want to select from this collection today, tomorrow and after tomorrow schedules. The day start from 06:00 and end 6:00 of next day.

Examples:

  • For today the query return events start in range: 30 July 2012 06:00 - 31 July 2012 06:00
  • For tomorrow the range is: 31 July 2012 06:00 - 01 August 2012 06:00

Any advice for refactoring this code?

private void FillSchedulesByFilter()
{
    var filterTimeStart = DateTime.Today.AddHours(6);
    var filterTimeEnd = filterTimeStart.AddDays(1);

    TodayGroupedSchedules = EventScheduleList.Where(s => s.RecurrenceStart >= filterTimeStart &&
                              s.RecurrenceStart < filterTimeEnd).
                              GroupBy(p => p.BasePlace);

    filterTimeStart = filterTimeStart.AddDays(1);
    filterTimeEnd = filterTimeEnd.AddDays(1);

    TomorrowGroupedSchedules = EventScheduleList.Where(s => s.RecurrenceStart >= filterTimeStart &&
                               s.RecurrenceStart < filterTimeEnd).
                               GroupBy(p => p.BasePlace);

    filterTimeStart = filterTimeStart.AddDays(1);
    filterTimeEnd = filterTimeEnd.AddDays(1);

    AfterTomorrowGroupedSchedules = EventScheduleList.Where(s => s.RecurrenceStart >= filterTimeStart &&
                               s.RecurrenceStart < filterTimeEnd).
                               GroupBy(p => p.BasePlace);

}
\$\endgroup\$
  • \$\begingroup\$ What kind of collection is EventScheduleList? Does performance matter to you or is readability more important? How big is the collection? Is it already sorted? \$\endgroup\$ – svick Jul 30 '12 at 7:59
  • \$\begingroup\$ It's collection of EventSchedule - my class that contain schedule of event: start,end, event name, and other infroamtion. Collection is not big( max 100 records). \$\endgroup\$ – user348173 Jul 30 '12 at 8:21
4
\$\begingroup\$

I can see two issues with your code:

  1. It doesn't work. Because the local variables filterTimeStart and filterTimeEnd are put in the closure, and the Where() lambdas are executed only when you iterate the resulting enumerables, all of your schedules will be the same as AfterTomorrowGroupedSchedules, because the last value assigned to the locals will be used. You can fix this by using ToArray().

  2. It's not DRY. The whole LINQ expression is always exactly the same and the interval is always one day. You should extract that into another method.

The modified code:

private IEnumerable<IGrouping<string, EventSchedule>> GroupedSchedulesForADay(
    DateTime filterTimeStart)
{
    return EventScheduleList.Where(
        s => s.RecurrenceStart >= filterTimeStart &&
            s.RecurrenceStart < filterTimeStart.AddDays(1))
        .GroupBy(p => p.BasePlace)
        .ToArray();
}

private void FillSchedulesByFilter()
{
    var filterTimeStart = DateTime.Today.AddHours(6);

    TodayGroupedSchedules = GroupedSchedulesForADay(filterTimeStart);

    filterTimeStart = filterTimeStart.AddDays(1);

    TomorrowGroupedSchedules = GroupedSchedulesForADay(filterTimeStart);

    filterTimeStart = filterTimeStart.AddDays(1);

    AfterTomorrowGroupedSchedules = GroupedSchedulesForADay(filterTimeStart);
}

Technically, the ToArray() is not necessary anymore, because each invocation of the GroupedSchedulesForADay() method has its own local variable, but I think it's still a good idea.

Also, I was worried that AddHours(6) wouldn't work correctly in the days where daylight saving time changes, but it seems it does work.

If the collection was big, it could make sense to iterate it just once, instead of three times, and build the three collections manually item by item, but it seems that's not necessary here.

\$\endgroup\$
2
\$\begingroup\$

First of all, don't reuse variables to hold values that represent different things. That only makes your code even more confusing to read, especially if it spans many lines. You should declare a variable for each different use you have (a set of variables for today, tomorrow and after tomorrow). That might be a sign to move some code into a separate function in those cases.

The query is the same all around, only the date ranges change so you should put them into a function to minimize repetition.

I would write it like this:

private IEnumerable<IGrouping<string, EventSchedule>>
    GetGroupedSchedulesForDay(DateTime day)
{
    var start = day.Date.AddHours(6);
    var end = start.AddDays(1);

    return // This is in query syntax, doesn't really matter which syntax you use
        from eventSchedule in EventScheduleList
        where eventSchedule.RecurrenceStart >= start
           && eventSchedule.RecurrenceStart < end
        group eventSchedule by eventSchedule.BasePlace;
}

private void FillSchedulesByFilter()
{
    var today = DateTime.Today;
    TodayGroupedSchedules = GetGroupedSchedulesForDay(today);

    var tomorrow = today.AddDays(1);
    TomorrowGroupedSchedules = GetGroupedSchedulesForDay(tomorrow);

    var afterTomorrow = today.AddDays(2);
    AfterTomorrowGroupedSchedules = GetGroupedSchedulesForDay(afterTomorrow);
}
\$\endgroup\$
  • \$\begingroup\$ Not reusing variables would also “accidentally” solve the issue of all lambdas referencing the same variable. \$\endgroup\$ – svick Jul 30 '12 at 8:45
  • \$\begingroup\$ I wouldn't say accidentally solve that issue with the closures, more like deliberately avoiding it all together. ;) \$\endgroup\$ – Jeff Mercado Jul 30 '12 at 8:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.