Skip to main content
Code Review

Return to Answer

Update with static GetCurrentMonday code, etc. Mention NodaTime for .NET Framework.
Source Link
Rick Davin
  • 6.7k
  • 1
  • 20
  • 32

UPDATE Why can it get away with static? Because your date ranges will never vary in length, or have different days of the week involved. It will always begin on start of day Monday. It will always be 5 days in duration. It will always end on end of day Friday. The previous week is always 7 days before the current Monday. The next week is always 7 days after the current Monday. Thus, you don’t need a class or struct that keeps track of a several properties. All you need is a singular DateTime instance, preferably with Kind of Unspecified, which is set to start of day on a Monday.

public static bool SundayIsFirstDayOfWeek { get; set; } = false;
public static DateTime GetCurrentMonday(DateTime date)
{
 int diff = DayOfWeek.Monday - date.DayOfWeek;
 if (date.DayOfWeek == DayOfWeek.Sunday && !SundayIsFirstDayOfWeek)
 {
 diff -= 7;
 }
 return DateTime.SpecifyKind(date.Date.AddDays(diff), DateTimeKind.Unspecified);
}
public static DateTime GetPreviousMonday(DateTime date) => GetCurrentMonday(date).AddDays(-7);
public static DateTime GetNextMonday(DateTime date) => GetCurrentMonday(date).AddDays(7);
public static DateTime GetEndOfCurrentWeek(DateTime date) => GetCurrentMonday(date).AddDays(5);
public static DateTime GetEndOfPreviousWeek(DateTime date) => GetPreviousMonday(date).AddDays(5);
public static DateTime GetEndOfNextWeek(DateTime date) => GetNextMonday(date).AddDays(5);

The above code brings together several of things I speak about through the remainder of my answer. I force the return Kind to be Unspecified, I ensure that the time component is set to midnight, and I have a property to declare if your week is defined as Monday-Sunday (SundayIsFirstDayOfWeek = false) or if it is Sunday-Saturday (SundayIsFirstDayOfWeek = true).

From my code and concerns, there are a lot of potentional pitfalls using DateTime. It bears repeating that with .NET 6 and beyond, that DateOnly would eliminate many of these pitfalls and make the code easier and simpler to follow.

If you are stuck with .NET Framework, then NodaTime's LocalDate object can be used as an equivalent substitution for DateOnly. Pity to use NodaTime just for that little bit when what it is so much far capable when dealing with time zones and calendar systems.

From my code and concerns, there are a lot of potentional pitfalls using DateTime. It bears repeating that with .NET 6 and beyond, that DateOnly would eliminate many of these pitfalls and make the code easier and simpler to follow.

UPDATE Why can it get away with static? Because your date ranges will never vary in length, or have different days of the week involved. It will always begin on start of day Monday. It will always be 5 days in duration. It will always end on end of day Friday. The previous week is always 7 days before the current Monday. The next week is always 7 days after the current Monday. Thus, you don’t need a class or struct that keeps track of a several properties. All you need is a singular DateTime instance, preferably with Kind of Unspecified, which is set to start of day on a Monday.

public static bool SundayIsFirstDayOfWeek { get; set; } = false;
public static DateTime GetCurrentMonday(DateTime date)
{
 int diff = DayOfWeek.Monday - date.DayOfWeek;
 if (date.DayOfWeek == DayOfWeek.Sunday && !SundayIsFirstDayOfWeek)
 {
 diff -= 7;
 }
 return DateTime.SpecifyKind(date.Date.AddDays(diff), DateTimeKind.Unspecified);
}
public static DateTime GetPreviousMonday(DateTime date) => GetCurrentMonday(date).AddDays(-7);
public static DateTime GetNextMonday(DateTime date) => GetCurrentMonday(date).AddDays(7);
public static DateTime GetEndOfCurrentWeek(DateTime date) => GetCurrentMonday(date).AddDays(5);
public static DateTime GetEndOfPreviousWeek(DateTime date) => GetPreviousMonday(date).AddDays(5);
public static DateTime GetEndOfNextWeek(DateTime date) => GetNextMonday(date).AddDays(5);

The above code brings together several of things I speak about through the remainder of my answer. I force the return Kind to be Unspecified, I ensure that the time component is set to midnight, and I have a property to declare if your week is defined as Monday-Sunday (SundayIsFirstDayOfWeek = false) or if it is Sunday-Saturday (SundayIsFirstDayOfWeek = true).

From my code and concerns, there are a lot of potentional pitfalls using DateTime. It bears repeating that with .NET 6 and beyond, that DateOnly would eliminate many of these pitfalls and make the code easier and simpler to follow.

If you are stuck with .NET Framework, then NodaTime's LocalDate object can be used as an equivalent substitution for DateOnly. Pity to use NodaTime just for that little bit when what it is so much far capable when dealing with time zones and calendar systems.

Story of the answer doesn't add value
Source Link
Toby Speight
  • 87.9k
  • 14
  • 104
  • 325

This post went a full year without any answers. I imagine this is far too late to help the OP but for academic purposes, I will post an answer.

This post went a full year without any answers. I imagine this is far too late to help the OP but for academic purposes, I will post an answer.

Source Link
Rick Davin
  • 6.7k
  • 1
  • 20
  • 32

This post went a full year without any answers. I imagine this is far too late to help the OP but for academic purposes, I will post an answer.

.NET 6 DateOnly

Much of what you are doing would be greatly simplified if you were using DateOnly that came out with .NET 6. So many nagging issues would fall by the wayside. For instance, with DateTime, one must be careful of the Kind property as well as the lingering time portion of the value.

Typically when working with calendars and using DateTime at midnight, many choose to make the starting DateTime inclusive and the ending DateTime exclusive, so that you can truly capture the full end date. With DateOnly, the ending date would be inclusive.

Much of my review will focus on the problems regarding using DateTime for such ranges.

Weekdays only, no Holidays

The code addresses a very specific purpose of your defined "work week", which is simply week days Monday-Friday without regard to company holidays. If holidays were to be considered, the code becomes messier and typically would require inputting a list of known holiday DateTime values. Ideally with holidays the recommendation is a list of known holiday dates so that the code the library code is reusable for other uses and/or companies.

I have dabbled with a custom DateTimeRange class in the past, again with an InclusiveStartTime and ExclusiveEndTime, in which the times were not restricted to midnight. Because of this, my class would not be static. If you are restricting yourself to week days only, much of your code is simpler and maybe could possibly get by with being static.

Specific Problems

Confusion over "Day"

Many of your methods use the phrase "Day" with DateTime objects. In .NET, "Day" has other meanings with other enums, including DateTime.Day property, which is a number ranging from 1 – 31, and not another DateTime instance as your code uses.

Start, End, and returned dates should use same Kind

Most likely, for week days only you would want your DateTime values to have a Kind property of Unspecified. Your code really gives no regard to Kind. Sometimes, the return value from a method returns the same kind as the input, and sometimes it does not.

Your original below returns the same kind:

public static DateTime BeginOfDay(DateTime date)
{
 return date.Date; // By returning date component, the time component will be zero values.
}

Though could be shorter as:

public static DateTime BeginOfDay(DateTime date) => date.Date; 

However, the below will always return Kind of Unspecified regardless of the input kind.

public static DateTime EndOfDay(DateTime date)
{
 return new DateTime(date.Year, date.Month, date.Day, 23, 59, 59, 999);
}

To ensure the same Kind,

public static DateTime EndOfDay(DateTime date) => new DateTime(date.Year, date.Month, date.Day, 23, 59, 59, 999, date.Kind);

OR

public static DateTime EndOfDay(DateTime date) => date.Date.AddDays(1).AddTicks(-1);

Where the returned DateTime is not only the same Kind, but also truly the inclusive end of the day the the tick rather than millisecond. I personally prefer working with exclusive end times, so my end of day’s midnight is same thing as start of next day’s midnight, in which case I would omit the AddTicks(-1).

Prefer direct calculation over brute force looping

My biggest complaint is with your looping in methods such as BeginOfMonthContaining or EndOfMonthContaining. Such looping is considered brute force. You can eliminate the loops for a more elegant solution, but what makes it elegant is that is just simple and direct.

public static DateTime BeginOfMonthContaining(DateTime date) => new DateTime(date.Year, date.Month, 1, 0, 0, 0, 0, date.Kind);
public static DateTime EndOfMonthContaining(DateTime date) => BeginOfMonthContaining(date).AddMonths(1).AddTicks(-1);

If using exclusive end times, then the AddTicks would be omitted and perhaps the method renamed, NextMonth.

An alternative to BeginOfMonthContaining could be:

public static DateTime BeginOfMonthContaining(DateTime date) => date.AddDays(date.Day – 1);

For FindFirstNextDay, the direct code could be:

public static DateTime FindFirstNextDay(DateTime date, DayOfWeek wantedDayOfWeek)
{
 int diff = wantedDayOfWeek - date.DayOfWeek;
 return date.Date.AddDays(diff < 1 ? diff + 7 : diff);
}

Note the return calls date.Date rather than date so that midnight start of day is returned. In other words, there is nothing preventing an input date to have a time other than midnight, but you want your method to return midnight.

Possible Confusion over BeginOfWeekContaining and EndOfWeekContaining

When you input a DateTime that falls on a Saturday or Sunday your BeginOfWeekContaining will always return the previous Monday, and that’s perfectly okay as implemented. One could argue that that is intentional behavior.

However, others could argue that since DayOfWeek.Sunday has integer code of 0, that is should return Monday of the current week, whereas Saturday falls in the previous week. I can see reasoning for either argument. But the method name by itself yields no clue, and there is no documentation clarifying the expected behavior. Perhaps a static property could be used on how Sunday is to be treated, i.e. does it fall on the end of the work week or the beginning of the work week.

Easier with DateOnly

From my code and concerns, there are a lot of potentional pitfalls using DateTime. It bears repeating that with .NET 6 and beyond, that DateOnly would eliminate many of these pitfalls and make the code easier and simpler to follow.

lang-cs

AltStyle によって変換されたページ (->オリジナル) /