0
\$\begingroup\$

I have written the following class:

NOTE: There is a bug in the third party component that means that if a DHXBlockTime is there for a day of the week and a specific date then only the ones with a specific date are shown so that is why there is the seemingly redundant AddNonWorkingHoursAsDateInsteadOfDay method.

internal  class CalendarHelper
{
    internal static IEnumerable<DHXBlockTime> GetUnavailableBlocks(IEnumerable<Driver> availableDrivers, string viewName)
    {
        var unavailable = new List<DHXBlockTime>();

        foreach (var driver in availableDrivers)
        {
            AddNonWorkingHours(viewName, unavailable, driver);

            foreach (var away in driver.Aways)
            {
                unavailable.Add(new DHXBlockTime()
                {
                    StartDate = away.StartDate,
                    EndDate = away.EndDate,
                    Sections = new List<Section>()
                    {
                        new Section(viewName, new string[] {away.EntityPersonStaffId.ToString()})
                    }
                });

                var startDate = new DateTime(away.StartDate.Year, away.StartDate.Month, away.StartDate.Day);
                var endDate = new DateTime(away.EndDate.Year, away.EndDate.Month, away.EndDate.Day);

                var workingHours = driver.WorkingHours.FirstOrDefault(w => w.Active && w.DayOfWeekIndex == (int)startDate.DayOfWeek);
                AddNonWorkingHoursAsDateInsteadOfDay(viewName, workingHours, unavailable, startDate, away.EntityPersonStaffId);

                if (startDate < endDate)
                {
                    workingHours = driver.WorkingHours.FirstOrDefault(w => w.Active && w.DayOfWeekIndex == (int)endDate.DayOfWeek);
                    AddNonWorkingHoursAsDateInsteadOfDay(viewName, workingHours, unavailable, endDate, away.EntityPersonStaffId);
                }

            }
        }

        return unavailable;
    }

    private static void AddNonWorkingHoursAsDateInsteadOfDay(string viewName, EntityPersonStaffHours workingHours, ICollection<DHXBlockTime> unavailable, DateTime awayDate, int staffId)
    {
        if (workingHours == null)
        {
            AddDefaultUnavailableTimes(viewName, unavailable, awayDate, staffId);
        }
        else
        {
            if (!workingHours.IsWorking)
            {
                unavailable.Add(new DHXBlockTime()
                {
                    StartDate = awayDate,
                    EndDate = awayDate.AddHours(24),
                    Sections = new List<Section>()
                    {
                        new Section(viewName,
                            new string[] {staffId.ToString()})
                    }
                });
            }
            else
            {
                int startMinutes = 7 * 60;
                int endMinutes = 22 * 60;

                if (workingHours.StartTime != null) startMinutes = (int) workingHours.StartTime.Value.TotalMinutes;
                if (workingHours.EndTime != null) endMinutes = (int) workingHours.EndTime.Value.TotalMinutes;

                unavailable.Add(new DHXBlockTime()
                {
                    StartDate = awayDate,
                    EndDate = awayDate.AddMinutes(startMinutes),
                    Sections = new List<Section>()
                    {
                        new Section(viewName, new string[] {staffId.ToString()})
                    }
                });

                unavailable.Add(new DHXBlockTime()
                {
                    StartDate = awayDate.AddMinutes(endMinutes),
                    EndDate = awayDate.AddHours(24),
                    Sections = new List<Section>()
                    {
                        new Section(viewName, new string[] {staffId.ToString()})
                    }
                });

            }
        }
    }

    private static void AddDefaultUnavailableTimes(string viewName, ICollection<DHXBlockTime> unavailable, DateTime awayDate, int staffId)
    {
        unavailable.Add(new DHXBlockTime()
        {
            StartDate = awayDate,
            EndDate = awayDate.AddHours(7),
            Sections = new List<Section>()
            {
                new Section(viewName, new string[] {staffId.ToString()})
            }
        });
        unavailable.Add(new DHXBlockTime()
        {
            StartDate = awayDate.AddHours(22),
            EndDate = awayDate.AddHours(24),
            Sections = new List<Section>()
            {
                new Section(viewName, new string[] {staffId.ToString()})
            }
        });
    }

    private static void AddNonWorkingHours(string viewName, ICollection<DHXBlockTime> unavailable, Driver driver)
    {
        for (int day = 0; day < 7; day++)
        {
            int startOne = 0;
            int endOne = 7 * 60;
            int startTwo = 22 * 60;
            int endTwo = 24 * 60;

            if (driver.WorkingHours.Count > 0)
            {
                var workingHours = driver.WorkingHours.FirstOrDefault(w => w.Active && w.DayOfWeekIndex == day);
                if (workingHours != null)
                {
                    if (workingHours.IsWorking)
                    {
                        if (workingHours.StartTime != null)
                            endOne = (int)workingHours.StartTime.Value.TotalMinutes;
                        if (workingHours.EndTime != null)
                            startTwo = (int)workingHours.EndTime.Value.TotalMinutes;
                    }
                    else
                    {
                        endOne = 12 * 60;
                        startTwo = 12 * 60;
                    }
                }
            }

            var zones = new List<Zone>();
            if (endOne == startTwo)
                zones.Add(new Zone { Start = startOne, End = endTwo });
            else
            {
                zones.Add(new Zone { Start = startOne, End = endOne });
                zones.Add(new Zone { Start = startTwo, End = endTwo });
            }

            unavailable.Add(new DHXBlockTime()
            {
                Day = (DayOfWeek)day,
                Zones = zones,
                Sections = new List<Section>()
                {
                    new Section(viewName, new string[] {driver.ID.ToString()})
                }
            });
        }
    }

}

I am pretty sure that it needs refactored as it is all static methods, but I am not sure how exactly. I think the class should be given a constructor as follows:

private IEnumerable<Driver> _availableDrivers;
private readonly string _viewName;

internal CalendarHelper(IEnumerable<Driver> availableDrivers, string viewName)
{
    _availableDrivers = availableDrivers;
    _viewName = viewName;
}

And then the availableDrivers and viewName parameters are not needed on each method and they can become instance methods rather than static methods, but then I get a class which only has one public method. Another developer has commented that doing this is a needless complication and so I am looking for guidance.

\$\endgroup\$
  • \$\begingroup\$ Why do you believe that static methods are bad? \$\endgroup\$ – t3chb0t Aug 2 '17 at 4:27
  • \$\begingroup\$ @t3chb0t I don't believe static methods are bad, it's just that in this instance I have a chain of calls passing at least a couple of the same parameters - that is what seems bad. \$\endgroup\$ – James Culshaw Aug 2 '17 at 9:04
1
\$\begingroup\$

You have a lot of repeating code like this:

unavailable.Add(new DHXBlockTime()
{
    StartDate = away.StartDate,
    EndDate = away.EndDate,
    Sections = new List<Section>()
    {
        new Section(viewName, new string[] {away.EntityPersonStaffId.ToString()})
    }
});

Define a method that will do all this stuff:

private static void AddUnavailableBlockTime(ICollection<DHXBlockTime> unavailable,
                                            DateTime startDate,
                                            DateTime endDate,
                                            string sectionViewName,
                                            int sectionDriverId,
                                            IEnumerable<Zone> zones = null)
{
    unavailable.Add(new DHXBlockTime
    {
        StartDate = startDate,
        EndDate = endDate,
        Zones = zones,
        Sections = new List<Section>
        {
            new Section(sectionViewName, new[] { sectionDriverId.ToString() })
        }
    });
}

Then you can use it like this:

private static void AddDefaultUnavailableTimes(string viewName, ICollection<DHXBlockTime> unavailable, DateTime awayDate, int staffId)
{
    AddUnavailableBlockTime(unavailable,
                            awayDate,
                            awayDate.AddHours(7),
                            viewName,
                            staffId);
    AddUnavailableBlockTime(unavailable,
                            awayDate.AddHours(22),
                            awayDate.AddHours(24),
                            viewName,
                            staffId);
}

As for this code:

int startOne = 0;
int endOne = 7 * 60;
int startTwo = 22 * 60;
int endTwo = 24 * 60;

you should definitely extract all this values to named constants since it is unclear what these magic numbers mean.

And finally I recommend to change List<DHXBlockTime> everywhere in your code on IEnumerable<DHXBlockTime> and use yield return to produce result collection. It is not good to pass changeable collection between multiple methods. For example AddUnavailableBlockTime will be turned into

private static DHXBlockTime GetUnavailableBlockTime(DateTime startDate,
                                                    DateTime endDate,
                                                    string sectionViewName,
                                                    int sectionDriverId,
                                                    IEnumerable<Zone> zones = null)
{
    return new DHXBlockTime
    {
        StartDate = startDate,
        EndDate = endDate,
        Zones = zones,
        Sections = new List<Section>
        {
            new Section(sectionViewName, new[] { sectionDriverId.ToString() })
        }
    };
}

and AddDefaultUnavailableTimes will become

private static IEnumerable<DHXBlockTime> GetDefaultUnavailableTimes(string viewName, DateTime awayDate, int staffId)
{
    yield return GetUnavailableBlockTime(awayDate,
                                         awayDate.AddHours(7),
                                         viewName,
                                         staffId);
    yield return AddUnavailableBlockTime(awayDate.AddHours(22),
                                         awayDate.AddHours(24),
                                         viewName,
                                         staffId);
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Would it be better to change private static void AddNonWorkingHours(string viewName, ICollection<DHXBlockTime> unavailable, Driver driver) to private static List<DHXBlockTime> GetNonWorkingHours(string viewName, Driver driver) and then use unavailable.AddRange(GetNonWorkingHours(viewName, driver))? \$\endgroup\$ – James Culshaw Aug 2 '17 at 14:42
  • \$\begingroup\$ Yes, it will be better. But return IEnumerable<DHXBlockTime> instead of List<DHXBlockTime>. \$\endgroup\$ – Maxim Aug 2 '17 at 15:08
  • \$\begingroup\$ Why IEnumerable rather than List? \$\endgroup\$ – James Culshaw Aug 2 '17 at 22:05
  • \$\begingroup\$ If you don't suppose to use members specific for particular type you should use more general type. AddRange takes IEnumerable<T> as an argument, so what is the purpose of returning a List<T>? \$\endgroup\$ – Maxim Aug 3 '17 at 3: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.