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?

• Jeroen Vannevel: Does it work though? If I enter 2015年03月13日 as orderdate with 3 working days and I use the 15th, 16th and 17th as holidays it tells me that the 18th is the next working day. But then I've only got the 14th as a tru
• msdundar: Well, this is a difficult problem to solve sometimes as holidays differ greatly every year and therefore, I recommend using a Holiday API that provides up to date information.

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++;
 }
 }

• BCdotWEB: "if statement's braces should start on the same line (as a convention)" This is absolutely not true. Perhaps this is the convention at your place, but I've always written curly braces on a new line. Consistency is important, however: don't mix styles.
• TheCoffeeCup: @BCdotWEB thanks for the point, I will remove that from my answer.

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...

• Pieter Witvoet: I've fixed the formatting for you. You may want to read about the formatting syntax before jumping to conclusions.
• Pieter Witvoet: Note that the OP already uses the style you recommend, so I don't see how this review is relevant. It looks like it's commenting on TheCoffeeCup's answer instead?