0
\$\begingroup\$

I want to write an extension method for getting Sunday Date based on weekOffset and current date. For example if weekOffset value is 1 that means next week sunday date and if weekOffset is 2 that means next to next week's Sunday date.

I have come up with below code

private static List<string> GetWeekRange(this DateTimeOffset date, int weekOffset = 0)
        {
            var sundayDate = new DateTimeOffset(date.Year, date.Month, date.Day, 0, 0, 0, date.Offset).AddDays(weekOffset * 7 - (int) new DateTimeOffset(date.Year, date.Month, date.Day, 0, 0, 0, date.Offset).DayOfWeek);
            var saturdayDate = new DateTimeOffset(weekStartDate.Year, weekStartDate.Month, weekStartDate.Day, 0, 0, 0, weekStartDate.Offset).AddDays(7).AddMilliseconds(-1);
            return new List<string> { weekStartDate.ToString("o"), weekEndDate.ToString("o") };
        }

I was wondering if there can be a better way to do this?

\$\endgroup\$
6
  • 1
    \$\begingroup\$ You should perhaps be clear about your definition of "better" \$\endgroup\$
    – Caius Jard
    Commented Mar 16, 2021 at 7:26
  • 1
    \$\begingroup\$ This code does not compile. Apparently you have mixed up naming with sundayDate & weekStartDate and saturdayDate & weekEndDate. FIX YOUR CODE. \$\endgroup\$
    – Rick Davin
    Commented Mar 16, 2021 at 12:21
  • \$\begingroup\$ @RickDavin Sorry, Fixed now \$\endgroup\$
    – Coder
    Commented Mar 16, 2021 at 12:33
  • \$\begingroup\$ You also have a logic bug that does not account for DST. Bottom line is this method only works as long as a week does not have a DST transition. That is the input date has an Offset property that is fixed for the week being observed. Last week, my local Offset was -06:00. This week it is -05:00. \$\endgroup\$
    – Rick Davin
    Commented Mar 16, 2021 at 12:43
  • 1
    \$\begingroup\$ Welcome to Code Review! Changing the code in the question violates the question-and-answer nature of this site. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so the answers make sense again. \$\endgroup\$ Commented Mar 16, 2021 at 13:37

2 Answers 2

1
\$\begingroup\$

I think your code could be more succinctly and usefully (ie don't return string) written as:

    private static DateTimeOffset[] GetWeekRange(this DateTimeOffset d, int weekOffset = 0)
    {
        var weekStartDate = new DateTimeOffset(d.Date, d.Offset).AddDays(weekOffset * 7 - (int)d.DayOfWeek);
        var weekEndDate = weekStartDate.AddDays(7);
        return new [] { weekStartDate, weekEndDate };
    }

But then given that end is so easily calc'd from start, maybe just return start, forget the array

\$\endgroup\$
1
\$\begingroup\$

My comments to the OP mention there is a flaw in the method if the week in question has a DST transition. The UTC offset for the DateTimeOffset is constant for the week, that is to say that both the Start and End dates must have the same UTC offset, which may not be desired. Unless you were to account for a time zone, you will have to accept this flaw.

I would suggest you could have phrased the question better. And the same thinking could also go into your variable naming. You really aren't trying to find Sunday's start. Rather you are trying to find the start of a "week". If you pass in 1 for your weekOffset, then the start of the week is Monday, not Sunday. Thus the variable naming of sundayDate really should be startDate or something like that since it does not have to be Sunday.

There is no constraint on the int weekOffset parameter. Since a DateTimeOffset also has an Offset property (for UTC offset), I paused and had to slowly read to see what purpose was served by weekOffset. This is cleaned up better in my version below where I use a DayOfWeek enum instead. As a enum, it serves as a constrait but also is more direct that it serves as the beginning of a "week".

There is no need to substract a millisecond to get the week End date. Why a millisecond? Why not 1 tick? Better yet, why bother at all? Generally one accepts that one week's End is another week's Start. The convention is therefore to have the Start be inclusive (i.e. >=) and the End be exclusive (i.e. <) when filtering is applied to see if a given DateTime falls within that "week".

Your variable naming could go a long way to clear up any confusion. It is a relative rookie mistake to return strings. As the other answer says, you could either return an array of DateTimeOffset or really just return the start of the week since you know the end of that week is just 7 days away. Let's go further. Let's return a tuple with named values, and let's have those names clear up the confusion.

    public static (DateTimeOffset InclusiveStartDate, DateTimeOffset ExclusiveEndDate) GetWeekRange(this DateTimeOffset date, DayOfWeek weekStartingDay = DayOfWeek.Sunday)
    {
        date = new DateTimeOffset(date.Year, date.Month, date.Day, 0, 0, 0, date.Offset);
        var offsetDays = ((int)weekStartingDay * 7) - (int)date.DayOfWeek;
        var startDate = date.AddDays(offsetDays);
        return (startDate, startDate.AddDays(7));
    }

Any developer using this clearly sees that there is an InclusiveStartDate and an ExclusiveEndDate. Furthermore, instead of the weekOffset being an int, where one could pass in bad values like -999, I pass in a DayOfWeek enumerated value.

To see it in action:

static void Main(string[] args)
{
    var date = DateTimeOffset.Now;
    DisplayWeekRange(date, DayOfWeek.Sunday);
    DisplayWeekRange(date.AddDays(-7), DayOfWeek.Sunday);

    Console.WriteLine("\nPress ENTER key to close.");
    Console.ReadLine();
}

private static void DisplayWeekRange(DateTimeOffset date, DayOfWeek weekStartingDay)
{
    var weekRange = date.GetWeekRange(weekStartingDay);
    Console.WriteLine($"Input Date: {date}");
    Console.WriteLine($"Week Starting Day: {weekStartingDay}");
    Console.WriteLine($"  Inclusive Start: {weekRange.InclusiveStartDate}");
    Console.WriteLine($"  Exclusive End  : {weekRange.ExclusiveEndDate}");
}

And here is some sample output:

Input Date: 3/16/2021 8:47:24 AM -05:00
Week Starting Day: Sunday
  Inclusive Start: 3/14/2021 12:00:00 AM -05:00
  Exclusive End  : 3/21/2021 12:00:00 AM -05:00
Input Date: 3/9/2021 8:47:24 AM -05:00
Week Starting Day: Sunday
  Inclusive Start: 3/7/2021 12:00:00 AM -05:00
  Exclusive End  : 3/14/2021 12:00:00 AM -05:00

Press ENTER key to close.

The second example shows the flaw with DST Transition. I am in US Central and just 3 days ago we had the Spring Forward to DST. This week's range is correct. However, last week's range should really show a UTC offset of -06:00 since last week was Standard time. Again, this flaw is not a logic bug as much as a design bug. Without knowing what time zone I am using, I cannot account for DST transitions. So a future version could maybe include that.

Also, anytime there is a question of time zones, times, etc., I always refer you to:

https://nodatime.org/

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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