3
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ 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 \$\endgroup\$ – Jeroen Vannevel Mar 9 '15 at 18:10
7
\$\begingroup\$

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

|improve this answer|||||
\$\endgroup\$
4
\$\begingroup\$

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.

|improve this answer|||||
\$\endgroup\$
3
\$\begingroup\$

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++;
        }
    }
|improve this answer|||||
\$\endgroup\$
  • 6
    \$\begingroup\$ "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. \$\endgroup\$ – BCdotWEB Mar 9 '15 at 18:19
  • \$\begingroup\$ @BCdotWEB thanks for the point, I will remove that from my answer. \$\endgroup\$ – TheCoffeeCup Mar 9 '15 at 18:20
3
\$\begingroup\$

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++;
}
|improve this answer|||||
\$\endgroup\$
-1
\$\begingroup\$

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

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ I've fixed the formatting for you. You may want to read about the formatting syntax before jumping to conclusions. \$\endgroup\$ – Pieter Witvoet Oct 2 '17 at 13:32
  • 5
    \$\begingroup\$ 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? \$\endgroup\$ – Pieter Witvoet Oct 2 '17 at 13:52

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.