12
\$\begingroup\$

I have Developed an Extension for DateTime in order to get the start of the week or start of the month (1-xx-xxxx) given a DateTime, and the working days of a month given a datetime and a list with bank days (Type DayOfWeek)

Any suggestions to improve it?

 public static class DateTimeExtensions
{
    public static DateTime StartOfWeek(this DateTime dt, DayOfWeek startOfWeek)
    {

        int diff = ((dt.DayOfWeek - startOfWeek)) % 7;

        if (startOfWeek == DayOfWeek.Sunday)
            return dt.AddDays((7+(startOfWeek - dt.DayOfWeek)) % 7).Date;
        return dt.AddDays(-1 * diff).Date;
    }

    public static DateTime StartOfMonth(this DateTime dt)
    {
        return new DateTime(dt.Year, dt.Month, 1);
    }

    public static int WorkingDays(this DateTime dt, IList<DayOfWeek> freeDays)
    {
        var workingDays = 0;
        var current = new DateTime(dt.Year, dt.Month, 1);
        var nextMonth = current.AddMonths(1);

        while (current < nextMonth)
        {
            if (!freeDays.Contains(current.DayOfWeek))
                workingDays++;
            current =current.AddDays(1);
        }
        return workingDays;
    }
    public static int WorkingDays(this DateTime dt, IList<DayOfWeek> freeDays, IList<DateTime> bankDays)
    {
        var workingDays = 0;
        var current = new DateTime(dt.Year, dt.Month, 1);
        var nextMonth = current.AddMonths(1);

       while (current < nextMonth)
       {
           if (!freeDays.Contains(current.DayOfWeek) && !bankDays.Contains(current))
               workingDays++;
            current =current.AddDays(1);
    }
    return workingDays;
   }


}
\$\endgroup\$
  • 4
    \$\begingroup\$ (Welcome to CR!) (What about bank holidays?) \$\endgroup\$ – greybeard Mar 27 '18 at 7:00
  • 2
    \$\begingroup\$ @greybeard One does not simply roll their own DateTime code... ;-) \$\endgroup\$ – Phrancis Mar 27 '18 at 7:21
  • \$\begingroup\$ @greybeard updated \$\endgroup\$ – David Soler Mar 27 '18 at 7:47
15
\$\begingroup\$

First I'd like to pinpoint few details about methods' interface. You have an overloaded WorkingDays method, what I'd change:

  • Name does not explain what this functions does, I have to check its signature. Does it count the number of working days? Does it return a list of those days? For which period? Let's call it CountWorkingDaysInMonth() and there will be no need to guess.
  • Both freeDays and bankHolidays are not working days. There may be more: days company is closed, paid holidays and so on. Surely do not want to add a list for each one. Just take one changing signature to accept IEnumerable<DateTime> (and caller may easily use Enumerable.Concat() to build the right list.
  • Validate your inputs.

Inside the function I'd try to help the reader using the methods you already defined:

var beginningOfCurrentMonth = dt.StartOfMonth();
var beginningOfNextMonth = beginningOfCurrentMonth.AddMonths(1);

Note the explicative names and the fact that I'm reusing StartOfMonth() instead of duplicating its code (you'll see later why).

Let's introduce a generator (useful if you're working with consecutive dates):

IEnumerable<DateTime> AllDaysInInterval(DateTime start, DateTime end) 
{
    Debug.Assert(end > start);

    for (var current = start; current < end; current = current.AddDays(1))
        yield return current;
}

Declare a local function to determine if a day is a working day or not:

bool IsWorkingDay(DateTime day)
    => !freeDays.Contains(day.DayofWeek) && !notWorkingDays.Contains(day);

Now you can count them:

return AllDaysInInterval(beginningOfCurrentMonth, beginningOfNextMonth)
    .Count(IsWorkingDay);

Short, easy to understand and you now created few reusable functions you may need elsewhere. Putting things all together:

public static int CountWorkingDaysInMonth(this DateTime dt,
    IEnumerable<DayOfWeek> freeDays,
    IEnumerable<DateTime> notWorkingDays)
{
    if (freeDays == null)
        throw new ArgumentNullException(freeDays);

    if (notWorkingDays == null)
        throw new ArgumentNullException(notWorkingDays);

    var beginningOfCurrentMonth = dt.StartOfMonth();
    var beginningOfNextMonth = beginningOfCurrentMonth.AddMonths(1);

    return AllDaysInInterval(beginningOfCurrentMonth, beginningOfNextMonth)
        .Count(IsWorkingDay);

    bool IsWorkingDay(DateTime day)
        => !freeDays.Contains(day.DayofWeek) && !notWorkingDays.Contains(day);
}

Now it's time to go back to StartOfMonth(). Is it always 1st the first day of the month? No. Not every date is a valid date in every calendar. You may, or may not, need to care about this but you should clearly document it (and be aware of).


In StartOfWeek() you can make your code easier to read.

  • int diff = ((dt.DayOfWeek - startOfWeek)) % 7; has unnecessary parenthesis and diff does not say anything about its content. Also it's not used if startOfWeek is Sunday.
  • -1 * diff should simply be -diff.
  • IMO (7+(startOfWeek - dt.DayOfWeek)) % 7 is clearer if moved outside, to a local variable.

What to do? Just (temporary!) introduce a local function to calculate the number of days from the beginning of the week:

int NumberOfDaysFromBeginningOfWeek()
{
    if (startOfWeek == DayOfWeek.Sunday)
        return (7 + (startOfWeek - dt.DayOfWeek)) % 7;

    return (dt.DayOfWeek - startOfWeek) % 7;
}

Is it what we want? Doesn't it look too complicate? It does then it's probably wrong. Algorithm must be much simpler than that. Let's first calculate the number of days elapsed from the beginning of the week without any special case:

int daysFromBeginningOfWeek = dt.DayOfWeek - startOfWeek;

What's wrong with that? Well, we're relying with enumeration values but week may begin any day (it depends on the culture). What may happen if our day is before the beginning of the week? Number goes negative. Let's add the number of days in a week and it will be right:

int NumberOfDaysFromBeginningOfWeek()
{
    int count = dt.DayOfWeek - startOfWeek;
    if (count < 0)
        return count + 7;

    return count;
}

Note that I'm simplifying this code with a huge assumption: there are always seven days in a week. It's not true and you should read the value from the calendar. Final code will be slightly more complicate than this but I guess you've got the idea. Putting things together:

public static DateTime StartOfWeek(this DateTime dt, DayOfWeek startOfWeek)
{
    return dt.AddDays(-NumberOfDaysFromBeginningOfWeek());

    int NumberOfDaysFromBeginningOfWeek()
    {
        int count = dt.DayOfWeek - startOfWeek;
        if (count < 0)
            return count + 7;

        return count;
    }
}

In this last case local function is more educational than useful, reader should absolutely feel free (!) to replace it with a local variable (with a proper descriptive name.)

Please note: I wrote all this code here without any testing. Do not underestimate date/time handling and write your own huge, big, extensive unit testing to include all corner cases, calendars and cultures you may think of and, most importantly, never ever make any assumption about dates. If there is a property somewhere (Calendar, Culture or DateTime) then use it.

\$\endgroup\$
  • 1
    \$\begingroup\$ There are lot of opportunities for the ternary operator ?: that would look better, I think, than the ifs ;-) \$\endgroup\$ – t3chb0t Mar 27 '18 at 11:26
  • 1
    \$\begingroup\$ @t3chb0t definitely, that CalculateTheExactNumberOfDaysFromTheVeryVeryBeginningOfTheWeek() is slightly too much illustrative... \$\endgroup\$ – Adriano Repetti Mar 27 '18 at 16:26
  • \$\begingroup\$ I'm curious - can you give me an example of when the 1st is not the first day of the month? \$\endgroup\$ – Chris Mar 27 '18 at 21:45
  • 1
    \$\begingroup\$ @Chris it's not something we often need to deal (that's why I just mentioned without more details) but when Gregorian calendar has been adopted they introduced "holes". Adoption took more than 400 years and it finished (AFAIK) in the first half of the 1900s (including BIG countries). If the "hole" is in the middle of the month...then day counting fails (because that month had...less days) but if it's near the end of the month then the missing days might be at the beginning (then you may have for example, 31st January and then 4th February). Not something you need to care about when... \$\endgroup\$ – Adriano Repetti Mar 28 '18 at 14:53
  • \$\begingroup\$ ...calculating payslips (but it's Culture specific, it's not even about Calendar). Even more corner-case we should check for the beginning...not every Calendar goes back to DateTime.MinValue (for GregorianCalendar) then we may produce an invalid date. \$\endgroup\$ – Adriano Repetti Mar 28 '18 at 14:56
7
\$\begingroup\$

I largely agree with Adriano in his discussion of your code, but I find his method naming a little too detailed. Below find my review of your extensions.

  public static class DateTimeExtensionsReview
  {
    public static DateTime StartOfWeek(this DateTime dt, DayOfWeek startOfWeek)
    {
      return dt.AddDays(-(((dt.DayOfWeek - startOfWeek) + 7) % 7));
    }

    public static DateTime StartOfMonth(this DateTime dt)
    {
      return new DateTime(dt.Year, dt.Month, 1);
    }

    public static IEnumerable<DateTime> EnumDays(this DateTime start, DateTime end)
    {
      for (DateTime day = start; day < end; day = day.AddDays(1))
      {
        yield return day;
      }
    }

    public static IEnumerable<DateTime> DaysInMonth(this DateTime dt)
    {
      var current = dt.StartOfMonth(); // Use your own extension
      return current.EnumDays(current.AddMonths(1));
    }

    public static IEnumerable<DateTime> MonthWorkDays(this DateTime dt, params DayOfWeek[] offDays)
    {
      return dt.DaysInMonth().Where(d => !offDays.Contains(d.DayOfWeek));
    }

    public static int NumMonthWorkDays(this DateTime dt, params DayOfWeek[] offDays)
    {
      return dt.MonthWorkDays(offDays).Count();
    }
  }

I use , params DayOfWeek[] offDays instead of IList/IEnumerable because there are at most 7 possibilities, so it is IMO a convenient way.


Update

You could maybe consider a more general filter extension like this:

public static IEnumerable<DateTime> FilterPeriod(this DateTime dt, DateTime end, Func<DateTime, bool> filter)
{
  filter = filter ?? ((d) => true);

  return dt.EnumDays(end).Where(filter);
}

It could then be used like this in MonthWorkDays:

public static IEnumerable<DateTime> MonthWorkDays(this DateTime dt, params DayOfWeek[] offDays)
{      
  DateTime monthStart = dt.StartOfMonth();
  return monthStart.FilterPeriod(monthStart.AddMonths(1), d => !offDays.Contains(d.DayOfWeek));
}
\$\endgroup\$
2
\$\begingroup\$

One thing I haven't seen mentioned yet is the DateTimeKind. You may want to preserve it with your methods. Here is but one example:

public static DateTime StartOfMonth(this DateTime dt)
{
        return DateTime.SpecifyKind(new DateTime(dt.Year, dt.Month, 1), dt.Kind);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Absolutely!!! I still remember three days trying to reproduce a bug caused by this! \$\endgroup\$ – Adriano Repetti Mar 28 '18 at 6:35

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.