7
\$\begingroup\$

My goal is to obtain the nth weekday of a given month. Parameters are a date from a given month and the nth weekday I'm trying to obtain. It returns the Nth Weekday of the month if it exist and returns DateTime.MinValue othewise. For my purposes weekdays are M-F.

Is there a more elegant or efficient way to implement this logic? My implementation feels crude, basically a while loop that counts weekdays until it finds the desired date or the end of the month.

    public static DateTime getNthWeekdayOfMonth(DateTime date, int nthWeekday)
    {
        //Valid inputs are greater than 0 but less than 24 
        if (nthWeekday > 23 || nthWeekday < 1)
            return DateTime.MinValue;

        //start with 1st day of month from date param
        DateTime currentDay = new DateTime(date.Year, date.Month, 1);

        int i = 0;
        while(i < nthWeekday && currentDay.Month == date.Month)
        {
            if (currentDay.DayOfWeek != DayOfWeek.Saturday && currentDay.DayOfWeek != DayOfWeek.Sunday)
            {
                i++;
                if(i == nthWeekday)
                    return currentDay;
            }
            currentDay = currentDay.AddDays(1);
        }
        return DateTime.MinValue;
    }

Usage

var date1 = getNthWeekdayOfMonth(new DateTime(2015, 8, 11), 22);
//returns DateTime.MinValue
var date2 = getNthWeekdayOfMonth(new DateTime(2015, 8, 11), 21);
//returns 8/31/2015
var date3 = getNthWeekdayOfMonth(new DateTime(2015, 8, 11), 1);
//returns 8/3/2015
\$\endgroup\$
  • \$\begingroup\$ My only suggestion - not worthy of an answer, would be to change the return type to DateTime? and return null instead of DateTime.MinValue in the case next weekday does not exist. It seem more pragmatic to me than using the "magic" of MinValue. \$\endgroup\$ – Jesse C. Slicer Aug 11 '15 at 17:25
  • 1
    \$\begingroup\$ @JesseC.Slicer Excellent suggestion, thank you. \$\endgroup\$ – temarsden Aug 11 '15 at 17:36
  • \$\begingroup\$ @JesseC.Slicer how about throwing instead? \$\endgroup\$ – Mathieu Guindon Aug 11 '15 at 17:37
  • 1
    \$\begingroup\$ The OP also uses the input date's Year when declaring currentDay. So not just the month is used. I would go as far as to include date's Kind just because. \$\endgroup\$ – Rick Davin Aug 11 '15 at 17:46
  • 1
    \$\begingroup\$ @Mat'sMug just a clarification - I believe the return at the bottom of the method is for expected values and should be null. The first return being used in the guard clause is the perfect place for an exception. \$\endgroup\$ – Jesse C. Slicer Aug 11 '15 at 19:30
7
\$\begingroup\$
    int i = 0;
    while(i < nthWeekday && currentDay.Month == date.Month)
    {
        if (currentDay.DayOfWeek != DayOfWeek.Saturday && currentDay.DayOfWeek != DayOfWeek.Sunday)
        {
            i++;
            if(i == nthWeekday)
                return currentDay;
        }
        currentDay = currentDay.AddDays(1);
    }

Your while loop is a little messy let's clean that up first, and you should also have an early continue statement in there.

Let me show you what I mean:

int i = 1;
while(i <= nthWeekday && currentDay.Month == date.Month)
{
    bool isWeekendDay = currentDay.DayOfWeek == DayOfWeek.Saturday || currentDay.DayOfWeek == DayOfWeek.Sunday;

    if (isWeekendDay)
    {
        currentDay = currentDay.AddDays(1);
        continue;
    }
    if (i == nthWeekday) 
    {
        return currentDay;
    }
    currentDay = currentDay.AddDays(1);
    i++;
}

This is a lot cleaner, and shows exactly what our intent is. It is much easier to read.

You shouldn't nest if statements inside of other if statements if you don't have to, it can get very confusing.

\$\endgroup\$
  • 1
    \$\begingroup\$ I doubt that the performance/readability trade off is worth it, but you could just check if it's a Saturday and then add two. This would result in a much smaller edge case where the first day of the month is a Sunday (yes, I know, premature optimization and all that) \$\endgroup\$ – MikeTheLiar Aug 11 '15 at 21:50
  • \$\begingroup\$ we have been talking about ways to make this code smaller or cleaner almost all day @mikeTheLiar, in Chat here \$\endgroup\$ – Malachi Aug 11 '15 at 21:54
7
\$\begingroup\$

It wasn't immediately obvious to me what the "Nth weekday" of a month might be. What's especially confusing from an API standpoint, is that you're passing in a DateTime, but you're only using the Year and Month parts of it and discarding everything else.

Taking in an int, and validating its value being between 1 and 12 would make a friendlier API.

/// <summary>
/// Computes the date of the nth workday in specified month of specified year, 
/// assuming Monday-Friday work weeks.
/// </summary>
public static DateTime WorkdayOfMonth(int year, int month, int n)

Notice the PascalCase name and the XML comments clarifying usage and assumptions.

    //Valid inputs are greater than 0 but less than 24 
    if (nthWeekday > 23 || nthWeekday < 1)
        return DateTime.MinValue;

I don't think this is a good idea. Here's roughly how I would validate the input:

if (month < 1 || month > 12)
{
    throw new ArgumentException("Invalid 'month' parameter.");
}
if (n < 1 || n > 23)
{
    throw new ArgumentException("Specified 'n' parameter would be for a date outside the specified 'month'.");
}

Be merciless: if your method is given parameters that don't allow it to function normally, then you should not return a valid DateTime return value. An alternative could be to return a null DateTime? instead, but in my opinion if this function is given month=13 and n=42 there's clearly a serious bug in the application, and by returning a "magic" DateTime.MinValue you do nothing to stop the bleeding. Throwing an exception is the right thing to do in these exceptional circumstances.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the input. I chose to use DateTime as a parameter because I need to use both the Month and the Year to from the date value to determine a distinct month. \$\endgroup\$ – temarsden Aug 11 '15 at 17:54
  • 4
    \$\begingroup\$ You may want to use ArgumentOutOfRangeException instead of ArgumentException, but otherwise, it looks good :) \$\endgroup\$ – Dan Lyons Aug 11 '15 at 17:57
6
\$\begingroup\$

I would approach it more declaratively to avoid looping manually which I found rather hard to read and modify. The approach is similar to what Day Lyons did in his answer, but my code starts by generating the dates in the separate generator method rather than generating numbers.

The need to use C# generators (yield) makes the code more verbose and requires additional function, but I think it's worth it.

public DateTime? GetNthWeekdayOfMonth(int year, int month, int n)
{
    if (month < 1 || month > 12)
    {
        throw new ArgumentException("Invalid 'month' parameter", "month");
    }
    if (n < 1)
    {
        throw new ArgumentException("Argument specifying the nth workday must be positive", "n");
    }


    return GenerateDays(year, month)
        .TakeWhile(d => d.Month == month)
        .Where(d => d.DayOfWeek != DayOfWeek.Saturday && d.DayOfWeek != DayOfWeek.Sunday)
        .Skip(n - 1)
        .Cast<DateTime?>()
        .FirstOrDefault();
}

/// <summary>
/// Generates the infinite sequence of subsequent days beginning with the first day of specified month in the specified year.
/// </summary>
private IEnumerable<DateTime> GenerateDays(int fromYear, int fromMonth)
{
    var date = new DateTime(fromYear, fromMonth, 1);
    while (true) 
    {
        yield return date;
        date = date.AddDays(1);
    }
}

Notes:

  • Return type of the method is changed to DateTime?, because as others have pointed out, null makes more sense as as "missing value" than DateTime.MinValue
  • Input types changed from DateTime to year and month, because only those are really required
  • NodaTime's LocalDate type would represent better the return type here. DateTime contains also the time component and unnecessary Kind mess which makes code less clear
\$\endgroup\$
4
\$\begingroup\$

Another way is to use LINQ, which has the benefit of being a bit more declarative (at the expense of speed).

Since LINQ requires an IEnumerable<T>, you can start with a numbers table via Enumerable.Range. Then, you can build the days by taking your range and selecting the current number as a number of days and adding to your start day. From there, you just filter down to weekdays in the current month and skip to the proper day.

private static DateTime GetNthWeekdayOfMonth(DateTime date, int nthWeekday)
{
    var firstDay = new DateTime(date.Year, date.Month, 1);
    var numbers = Enumerable.Range(0, 31);
    var days = numbers.Select(n => firstDay.AddDays(n)).Where(d => d.Month == date.Month);
    var weekdays = days.Where(d => d.DayOfWeek != DayOfWeek.Saturday && d.DayOfWeek != DayOfWeek.Sunday);
    return weekdays.Skip(nthWeekday - 1).FirstOrDefault();
}

There's a happy coincidence that you wanted to return DateTime.MinValue when there is no nth weekday, which happens to be the default for DateTime.

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

you can avoid the loop..

 public static DateTime getNthWeekday(DateTime date, int nthWeekDay) {
            DateTime firstDayOfMonth = new DateTime(date.Year, date.Month, 1);
            int additionalWeekend = 0;
            if (firstDayOfMonth.DayOfWeek == DayOfWeek.Saturday)
            {
                additionalWeekend = 2;
            }
            if (firstDayOfMonth.DayOfWeek == DayOfWeek.Sunday)
            {
                additionalWeekend = 1;
            }

            int res = ((nthWeekDay / 5) * 2) + nthWeekDay + additionalWeekend;
            DateTime actualDay = DateTime.MinValue;
            if (res <= DateTime.DaysInMonth(firstDayOfMonth.Year, firstDayOfMonth.Month))
            {
                actualDay = new DateTime(date.Year, date.Month, res);
                if (actualDay.DayOfWeek == DayOfWeek.Saturday)
                {
                    actualDay = actualDay.AddDays(-1);
                }
                if (actualDay.DayOfWeek == DayOfWeek.Sunday)
                {
                    actualDay = actualDay.AddDays(-2);
                }
            }
            return actualDay;
        }
\$\endgroup\$
  • 4
    \$\begingroup\$ Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and how it improves upon the original) so that the author can learn from your thought process. \$\endgroup\$ – Malachi Aug 11 '15 at 18:12
  • \$\begingroup\$ I would think avoiding the loop to be self-explanatory. Why execute your code n times when you only need to do it once. This is basically the approach I would try to take. \$\endgroup\$ – Jonathan Wood Aug 11 '15 at 23:35
  • \$\begingroup\$ some people are still learning and would like some explanation over techniques they haven't learned yet, @JonathanWood. \$\endgroup\$ – Malachi Aug 12 '15 at 14:08

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.