2
\$\begingroup\$

The code below attempts to extract incomplete weeks from a List<DateTime>.

For example, a list containing all the days in Jan 2015 would result in the 5th to the 25th inclusive.

I know the list going in will contain unique dates, and are in date order. It seems to be working but I can't help but think this could be done better.

private IEnumerable<DateTime> extractIncompleteWeeks(IEnumerable<DateTime> dates)
{
    var mondays = dates
        .Where((d, i) => d.DayOfWeek == DayOfWeek.Monday)
        .Select(d => d.Date);

    var results = new List<DateTime>();

    foreach (var monday in mondays)
    {
        var fullWeek = new HashSet<DateTime>(dates.SkipWhile(d => d < monday).Take(7).Select(d => d.Date));

        if (fullWeek.Last().Date == monday.AddDays(6).Date)
            results.AddRange(fullWeek);
    }

    return results;
}
\$\endgroup\$
  • \$\begingroup\$ Is your description accurate? Because this reminds me of a StackOverflow question from a while back. \$\endgroup\$ – BCdotWEB Mar 31 '15 at 12:19
  • \$\begingroup\$ @BCdotWEB: No, I don't think that's quite the same. I need to get all dates where the week is complete. So if there is a wednesday missing in the list passed in, I won't get any dates from that week. Only dates where all "siblings" (mon to sun) are present will be returned. \$\endgroup\$ – ETFairfax Mar 31 '15 at 13:39
1
\$\begingroup\$

Naming:

Names of methods in C# use PascalCasing. Your method name would become ExtractIncompleteWeeks. Although the naming convention is now correct, the name is ambiguous. Extract incomplete weeks means you want to fetch only the dates of the weeks which are incomplete. Instead rename it to ChopIncompleteWeeks or ExtractCompleteWeeks. This makes clear that you want to eliminate the incomplete weeks or want to extract the complete weeks.

Return type:

The return type of the method is an IEnumerable<DateTime> yet you return a List<DateTime>. Either make the return type also List<DateTime> but preferably return one element at a time by using yield.

private IEnumerable<DateTime> ExtractCompleteWeeks(IEnumerable<DateTime> dates)
{
    ...

    foreach (var monday in mondays)
    {
        ...

        if (fullWeek.Last().Date == monday.AddDays(6).Date)
        {
            foreach(var day in fullWeek)
            {
                yield return day;
            }
        }
    }
}

Here are some questions from StackOverflow and Programmers.SE regarding yield:

I really like this answer from the first question to get a very basic understanding of using yield instead of a temporary list:

Populating a temporary list is like downloading the whole video, whereas using yield is like streaming that video.

HashSet:

Is there a specific reason why you create a HashSet<DateTime> only to take the last element and compare it against monday.AddDays(6).Date? You can replace that line by following:

var fullWeek = dates.SkipWhile(d => d < monday).Take(7).Select(d => d.Date);

Complete code:

private IEnumerable<DateTime> ExtractCompleteWeeks(IEnumerable<DateTime> dates)
{
    var mondays = dates.Where(d => d.DayOfWeek == DayOfWeek.Monday)
                       .Select(d => d.Date);

    foreach (var monday in mondays)
    {
        var fullWeek = dates.SkipWhile(d => d < monday).Take(7).Select(d => d.Date);

        if (fullWeek.Last().Date == monday.AddDays(6).Date)
        {
            foreach(var day in fullWeek)
            {
                yield return day;
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I've always named my private methods with a starting lower case. Is that "wrong"? I've not really used the yield before - I shall have a read up on it! Many thanks for all the suggestions. \$\endgroup\$ – ETFairfax Mar 31 '15 at 13:32
  • \$\begingroup\$ @ETFairfax PascalCase is standard for methods. \$\endgroup\$ – BCdotWEB Mar 31 '15 at 14:23
  • \$\begingroup\$ It is not wrong in the sense of runtime/compile errors but aesthetically it is wrong if you want to follow Microsoft's naming guidelines. I updated my answer a bit where I talk about yield to give you a simple understanding on it. \$\endgroup\$ – Abbas Mar 31 '15 at 14:27
  • \$\begingroup\$ @Abbas Thanks for the update - that streaming analogy helped a lot. \$\endgroup\$ – ETFairfax Mar 31 '15 at 15:16

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.