8
\$\begingroup\$

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?

\$\endgroup\$
  • 3
    \$\begingroup\$ 1. Split the if and else branches in GetFiscalYearStartDate() into their own methods. 2. CountLeapYearsInRange() can be static. \$\endgroup\$ – Jesse C. Slicer Jul 16 '15 at 19:59
  • \$\begingroup\$ 78 isn't that bad I must say! \$\endgroup\$ – IEatBagels Oct 8 '15 at 17:44
6
\$\begingroup\$

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 :)

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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

\$\endgroup\$

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.