Every year is like a loop. A nasty, nested one

Question

Quite a little while ago, I wrote a VBA macro to generate fiscal calendar date records. That macro did the job all this time, but I needed to be able to generate fiscal years from an ETL overnight process, so I rewrote it in a small C# console application.

I went with full-on dependency injection and IoC, wrote dozens of unit tests for each component, and everything works like a charm.

Here's the IGeneratorService interface and its implementation; this object is the heart of the application, that implements the calendar logic.

I really don't like the 5-level nested loops; according to Visual Studio 2013, this class has a maintainability index of 89, and the Generate(int,int) method has one of 78.

namespace FiscalCalendarGenerator
{
    /// <summary>
    /// An object that encapsulates the logic to generate <see cref="FiscalCalendarModel"/> models.
    /// </summary>
    public interface IGeneratorService
    {
        /// <summary>
        /// Generates a <see cref="FiscalCalendarModel"/> for each date in the specified year.
        /// </summary>
        /// <param name="year">Fiscal year to generate records for.</param>
        /// <returns></returns>
        IEnumerable<FiscalCalendar> Generate(int year);

        /// <summary>
        /// Generates a <see cref="FiscalCalendarModel"/> for each date in the specified interval.
        /// </summary>
        /// <param name="fromYear">First fiscal year to generate records for.</param>
        /// <param name="toYear">Last fiscal year to generate records for.</param>
        IEnumerable<FiscalCalendar> Generate(int fromYear, int toYear);
    }

    public class GeneratorService : IGeneratorService
    {
        // reference start date: day 1 of Fiscal 2014 (Sunday).
        public static readonly DateTime ReferenceDate = new DateTime(2013, 12, 1);

        private readonly IFiscalYearStartDateCalculator _calculator;
        public GeneratorService(IFiscalYearStartDateCalculator calculator)
        {
            _calculator = calculator;
        }

        public IEnumerable<FiscalCalendar> Generate(int year)
        {
            return Generate(year, year);
        }

        public IEnumerable<FiscalCalendar> Generate(int fromYear, int toYear)
        {
            var currentDate = _calculator.GetFiscalYearStartDate(fromYear, ReferenceDate);
            for (var currentYear = fromYear; currentYear <= toYear; currentYear++)
            {
                var currentDayOfYear = 1;
                var currentWeekOfYear = 1;
                var currentMonthOfYear = 1;

                for (var currentQuarterOfYear = 1; currentQuarterOfYear <= 4; currentQuarterOfYear++)
                {
                    var currentDayOfQuarter = 1;
                    var currentWeekOfQuarter = 1;

                    for (var currentMonthOfQuarter = 1; currentMonthOfQuarter <= 3; currentMonthOfQuarter++)
                    {
                        var currentDayOfMonth = 1;

                        // weeks in month alternate 4-5-4 in quarter, and leap years add a 5th week in last month of year.
                        var weeksInMonth = (currentMonthOfQuarter % 2 == 0 || (currentMonthOfYear == 12 && DateTime.IsLeapYear(currentYear))) ? 5 : 4;
                        for (var currentWeekOfMonth = 1; currentWeekOfMonth <= weeksInMonth; currentWeekOfMonth++)
                        {
                            for (var currentDayOfWeek = 1; currentDayOfWeek <= 7; currentDayOfWeek++)
                            {
                                yield return new FiscalCalendar(currentDate)
                                {
                                    FiscalDayOfMonth = currentDayOfMonth,
                                    FiscalDayOfQuarter = currentDayOfQuarter,
                                    FiscalDayOfWeek = currentDayOfWeek,
                                    FiscalDayOfYear = currentDayOfYear,
                                    FiscalMonthOfQuarter = currentMonthOfQuarter,
                                    FiscalMonthOfYear = currentMonthOfYear,
                                    FiscalWeekOfMonth = currentWeekOfMonth,
                                    FiscalWeekOfQuarter = currentWeekOfQuarter,
                                    FiscalWeekOfYear = currentWeekOfYear,
                                    FiscalQuarterOfYear = currentQuarterOfYear,
                                    FiscalYear = currentYear
                                };

                                currentDate = currentDate.AddDays(1);
                                currentDayOfMonth++;
                                currentDayOfQuarter++;
                                currentDayOfYear++;
                            }

                            currentWeekOfYear++;
                        }

                        currentMonthOfYear++;
                    }
                }
            }
        }
    }
}

---

Another part I would like to improve is the FiscalYearStartDateCalculator class, featuring two methods that Visual Studio evaluates as having a maintainability index of 62 - I don't know how that metric is computed, but it does correlate pretty much exactly with how I feel about my code:

namespace FiscalCalendarGenerator
{
    /// <summary>
    /// An object responsible for calculating a fiscal year's start date.
    /// </summary>
    public interface IFiscalYearStartDateCalculator
    {
        /// <summary>
        /// Calculates the start date of specified fiscal year, given a reference start date for a reference fiscal year.
        /// </summary>
        /// <param name="fiscalYear">The fiscal year to get the start date for.</param>
        /// <param name="reference">The start date of the reference fiscal year.</param>
        /// <returns>Returns date of the first day of the specified fiscal year.</returns>
        DateTime GetFiscalYearStartDate(int fiscalYear, DateTime reference);
    }

    public class FiscalYearStartDateCalculator : IFiscalYearStartDateCalculator
    {
        public DateTime GetFiscalYearStartDate(int fiscalYear, DateTime reference)
        {
            var result = reference;
            var referenceFiscalYear = reference.Year + 1; // fiscal years start in the previous calendar year

            if (fiscalYear < referenceFiscalYear)
            {
                var years = referenceFiscalYear - fiscalYear;
                // result is 52*7 days for each year after the reference date, plus 7 days for each leap year in-between
                result = reference.AddDays(-1 * 52 * 7 * years)
                                  .AddDays(-1 * CountLeapYearsInRange(fiscalYear, reference.Year) * 7);
            }
            else if (fiscalYear > referenceFiscalYear)
            {
                var years = fiscalYear - referenceFiscalYear;
                // result is 52*7 days for each year before reference date, minus 7 days for each leap year in-between
                result = reference.AddDays(52 * 7 * years)
                                  .AddDays(CountLeapYearsInRange(fiscalYear, reference.Year) * 7);
            }

            return result;
        }

        private int CountLeapYearsInRange(int year1, int year2)
        {
            var firstYear = year1;
            var endYear = year2;

            if (year1 > year2)
            {
                firstYear = year2;
                endYear = year1;
            }
            else if (year1 == year2)
            {
                return DateTime.IsLeapYear(year1) ? 1 : 0;
            }

            return Enumerable.Range(firstYear, endYear - firstYear)
                             .Count(year => DateTime.IsLeapYear(year));
        }
    }
}

How can I improve this code?

Answer 1

I think I would make a private nested class like this:

class Counter
{
    public DateTime Date            { get; set; }
    public int DayOfMonth           { get; set; }
    public int DayOfQuarter         { get; set; }
    public int DayOfWeek            { get; set; }
    public int MonthOfQuarter       { get; set; }
    [...]
}

and than split your loops into different sub methods, and pass the counter object.

public IEnumerable<FiscalCalendar> Generate(int fromYear, int toYear)
{
    var counter = new Counter();

    counter.Date = _calculator.GetFiscalYearStartDate(fromYear, ReferenceDate);

    for (var currentYear = fromYear; currentYear <= toYear; currentYear++)
    {
        foreach ( var fiscalCalender in GenerateForYear(counter, currentYear) )
            yield return fiscalCalender;
    }
}


public IEnumerable<FiscalCalendar> GenerateForYear(Counter counter, currentYear)
{
    counter.currentYear = currentYear
    counter.DayOfYear = 1;
    counter.WeekOfYear = 1;
    counter.MonthOfYear = 1;

    for (var currentQuarterOfYear = 1; currentQuarterOfYear <= 4; currentQuarterOfYear++)
    {   
        [...]
        foreach ( var fiscalCalender in GenerateQuarterOfYear(counter, currentQuarterOfYear) )
            yield return fiscalCalender;            
    }

Anyway, something like this :)

Answer 2

Bug!

I've just installed ReSharper, and it made an interesting observation:

var currentWeekOfQuarter = 1;

This variable can be converted to a constant - it's never incremented... and that's a bug!

It should be incremented along with currentWeekOfYear:

currentWeekOfYear++;
currentWeekOfQuarter++;

This also means that you are missing at least one unit test, otherwise this would have easily been picked up.

Answer 3

Well, the calculator one at least could be simplified since you're always adding/subtracting the same:

 // 52*7 days for each year from the reference date, plus 7 days for each leap year in-between
 days = 52 * 7 * years + CountLeapYearsInRange(fiscalYear, reference.Year) * 7;
 if (fiscalYear < referenceFiscalYear) days = days * -1;
 return reference.AddDays(days);

I've got no idea on the actual generator though....