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\$
2
  • \$\begingroup\$ Why do you believe that static methods are bad? \$\endgroup\$
    – t3chb0t
    Aug 2, 2017 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\$ Aug 2, 2017 at 9:04

1 Answer 1

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);
}
\$\endgroup\$
4
  • \$\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\$ Aug 2, 2017 at 14:42
  • \$\begingroup\$ Yes, it will be better. But return IEnumerable<DHXBlockTime> instead of List<DHXBlockTime>. \$\endgroup\$
    – Maxim
    Aug 2, 2017 at 15:08
  • \$\begingroup\$ Why IEnumerable rather than List? \$\endgroup\$ Aug 2, 2017 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, 2017 at 3:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.