Calculate next working day

Question

I have tried to tackle this question as follows:

The program Calculates the next Working day, taking into account:  
   - weekends (Saturdays and Sundays)  
   - holidays happening in the middle of the week 

param name="OrderDate" the date on which the order is placed
param name="workingDays" the number of workingdays to process the order      
returns the date on which the Order is Requested to Ship

    public static DateTime getNextWorkingDay(DateTime OrderDate, int workingDays)
    {
        DateTime Holiday = new DateTime();
        List<DateTime> SelectedHolidays = new List<DateTime>();
        List<HolidayList> HolidayList = new List<HolidayList>();

        int calculatedWorkingDays=0;
        if (workingDays <= 5)
            calculatedWorkingDays = workingDays;
        else if (workingDays % 6 == 0)
        {
            calculatedWorkingDays = workingDays + workingDays / 6 * 2;
        }
        else
        {
            calculatedWorkingDays = workingDays + workingDays / 7 * 2;
        }



        HolidayList =  "Read the list from Json file";

        foreach (var holiday in HolidayList)
        {
            string year = DateTime.Today.Year.ToString();
            Holiday = DateTime.Parse(holiday.MonthAndDayValue + "-" + year);

            if (Holiday.Day == 1 && Holiday.Month == 1)
            {
                year = (DateTime.Today.Year + 1).ToString();
                Holiday = DateTime.Parse(holiday.MonthAndDayValue + "-" + year);
            }


            SelectedHolidays.Add(Holiday.Date);

            if (Holiday.DayOfWeek == DayOfWeek.Sunday || Holiday.DayOfWeek == DayOfWeek.Saturday)
            {
                //do nothing
            }
            else
            {
                if (Holiday.Date == OrderDate.Date)
                    OrderDate.AddDays(1);
                if (Holiday.Date > OrderDate.Date)
                {
                    int K = (int)(Holiday.Date - OrderDate.Date).TotalDays;
                    if (K <= calculatedWorkingDays)
                    {
                        calculatedWorkingDays++;
                    }
                }
            }
        }
        if (OrderDate.DayOfWeek == DayOfWeek.Sunday)
        {
            calculatedWorkingDays++;
        }
        else if (OrderDate.DayOfWeek == DayOfWeek.Saturday || OrderDate.DayOfWeek == DayOfWeek.Friday)
        {
            calculatedWorkingDays += 2;
        }

        // this is required date 
        DateTime date_OrderRequestedToShip = OrderDate.AddDays(calculatedWorkingDays);

        //check if the last date fall on a non working day(holiday  or weekend)
        calculatedWorkingDays = 0;
        foreach (DateTime d in SelectedHolidays)
            if (d.Date == date_OrderRequestedToShip.Date)
                calculatedWorkingDays++;
        date_OrderRequestedToShip = date_OrderRequestedToShip.AddDays(calculatedWorkingDays);
        if (date_OrderRequestedToShip.DayOfWeek == DayOfWeek.Sunday)
        {
            calculatedWorkingDays = 1;
        }
        else if (date_OrderRequestedToShip.DayOfWeek == DayOfWeek.Saturday)
        {
            calculatedWorkingDays = 2;
        }

        return date_OrderRequestedToShip.AddDays(calculatedWorkingDays);
    }

The holiday list contains the month and day (MM/dd) part only with out the year part:

{
    "holidayName": "Memorial Day",
    "MonthAndDayValue": "05-30"
}

Can anyone help to tune up or suggest changes to make the code more efficient?

Answer 1

First, you should separate a process of parsing your input from your business logic. So, your first step is to convert your holidays input to a structure you'll be working with. So, let's say you have a List<Holiday> where

public struct Holiday
{
    public readonly int Month;
    public readonly int Day;

    public Holiday(int month, int day)
    {
        Month = month;
        Day = day;
    }
}

Now, in your place I'd start with implementation of something that enumerates working days like so:

static IEnumerable<DateTime> GetWorkingDays(DateTime startDate, List<Holiday> holidays)
{
    var date = startDate;

    for (;;date = date.AddDays(1))
    {        
        if (date.DayOfWeek != DayOfWeek.Saturday &&
            date.DayOfWeek != DayOfWeek.Sunday &&
            holidays.All(holiday => holiday.Day != date.Day ||
                                    holiday.Month != date.Month))
        {
            yield return date;
        }
    }
}

Note that this iterator is endless, so you should be cautious enumerating it.

Now, your problem can be solved with just one line

var result = GetWorkingDays(orderDate, holidays).Skip(workingDays).First();

Complete example here: https://dotnetfiddle.net/W3dalr

Answer 2

Your method does too much and that makes it hard to follow/optimize.

This should be extracted into its own method.

    int calculatedWorkingDays=0;
    if (workingDays <= 5)
        calculatedWorkingDays = workingDays;
    else if (workingDays % 6 == 0)
    {
        calculatedWorkingDays = workingDays + workingDays / 6 * 2;
    }
    else
    {
        calculatedWorkingDays = workingDays + workingDays / 7 * 2;
    }

Then this block

    HolidayList =  "Read the list from Json file";

    foreach (var holiday in HolidayList)
    {
        string year = DateTime.Today.Year.ToString();
        Holiday = DateTime.Parse(holiday.MonthAndDayValue + "-" + year);

        if (Holiday.Day == 1 && Holiday.Month == 1)
        {
            year = (DateTime.Today.Year + 1).ToString();
            Holiday = DateTime.Parse(holiday.MonthAndDayValue + "-" + year);
        }


        SelectedHolidays.Add(Holiday.Date);

        if (Holiday.DayOfWeek == DayOfWeek.Sunday || Holiday.DayOfWeek == DayOfWeek.Saturday)
        {
            //do nothing
        }
        else
        {
            if (Holiday.Date == OrderDate.Date)
                OrderDate.AddDays(1);
            if (Holiday.Date > OrderDate.Date)
            {
                int K = (int)(Holiday.Date - OrderDate.Date).TotalDays;
                if (K <= calculatedWorkingDays)
                {
                    calculatedWorkingDays++;
                }
            }
        }
    }

You actually might want to split that into several methods.

Also, watch your white space. There's never a need for more than a single blank line.

Answer 3

Just some points:

    if (workingDays <= 5)
        calculatedWorkingDays = workingDays;
    else if (workingDays % 6 == 0)
    {
        calculatedWorkingDays = workingDays + workingDays / 6 * 2;
    }
    else
    {
        calculatedWorkingDays = workingDays + workingDays / 7 * 2;
    }

Always put braces around your if statements. If not, horrible bugs may occur when you change your code:

For example, lets say you have this if statement:

if(isTrue())
    doSomething();

And you want to add doSomething2() to it. You do:

if(isTrue())
    doSomething();
    doSomething2();

Note that the code above would be equivalent to:

if(isTrue()) {
    doSomething();
}
doSomething2();

Which is probably not what you want. But if you put braces:

if(isTrue()) {
    doSomething();
    doSomething2();
}

Then it is fine.

---

    // ...  
    else
    {
        calculatedWorkingDays = workingDays + workingDays / 7 * 2;
    }



    HolidayList =  "Read the list from Json file";
    // ...

Don't put too much spaces in your code. It makes it look messier and harder to read.

---

        if (Holiday.DayOfWeek == DayOfWeek.Sunday || Holiday.DayOfWeek == DayOfWeek.Saturday)
        {
            //do nothing
        }
        else
        {
            // ...
        }

The empty if can be easily avoided by doing:

        if (!(Holiday.DayOfWeek == DayOfWeek.Sunday || Holiday.DayOfWeek == DayOfWeek.Saturday))
        {
            // ...
        }

---

    foreach (DateTime d in SelectedHolidays)
        if (d.Date == date_OrderRequestedToShip.Date)
            calculatedWorkingDays++;

Again, braces:

    foreach (DateTime d in SelectedHolidays) {
        if (d.Date == date_OrderRequestedToShip.Date) {
            calculatedWorkingDays++;
        }
    }

Answer 4

There are a few things you can improve. First, never have empty if statements:

if (Holiday.DayOfWeek == DayOfWeek.Sunday || Holiday.DayOfWeek == DayOfWeek.Saturday)
{
    //do nothing
}
else
{
    if (Holiday.Date == OrderDate.Date)
        OrderDate.AddDays(1);
    if (Holiday.Date > OrderDate.Date)
    {
        int K = (int)(Holiday.Date - OrderDate.Date).TotalDays;
        if (K <= calculatedWorkingDays)
        {
            calculatedWorkingDays++;
        }
    }
}

Reverse the conditional and get rid of the else:

if (Holiday.DayOfWeek != DayOfWeek.Sunday && Holiday.DayOfWeek != DayOfWeek.Saturday)
{
    if (Holiday.Date == OrderDate.Date)
        OrderDate.AddDays(1);
    if (Holiday.Date > OrderDate.Date)
    {
        int K = (int)(Holiday.Date - OrderDate.Date).TotalDays;
        if (K <= calculatedWorkingDays)
        {
            calculatedWorkingDays++;
        }
    }
}

Also, keep your style similar. In the above, you have a if statement with one line without braces, and another with braces. You should always choose one style, preferably braces, and stick to it; the same applies to loops:

foreach (DateTime d in SelectedHolidays)
        if (d.Date == date_OrderRequestedToShip.Date)
            calculatedWorkingDays++;

Use this instead:

foreach (DateTime d in SelectedHolidays)
{
    if (d.Date == date_OrderRequestedToShip.Date)
    {
        calculatedWorkingDays++;
    }
}

Right here, you only use the variable K once:

int K = (int)(Holiday.Date - OrderDate.Date).TotalDays;
if (K <= calculatedWorkingDays)
{
    calculatedWorkingDays++;
}

Not only is this a bad and undescriptive name for a variable, it is unnecessary. Also, what type does the .TotalDays return, a double? If so, you probably do not need to cast it to an int.

if ((Holiday.Date - OrderDate.Date).TotalDays <= calculatedWorkingDays)
{
    calculatedWorkingDays++;
}

Answer 5

Don't put the braces on the same line with the statement, ie:

if(...) {
}

do this instead:

if (...)
{
}

Why? Putting the brace on the same line as the statement makes it harder to read the code and match the braces. Putting them on a separate improves your ability to read the code faster. And every manager in the world want's you to code faster...