Skip to main content
Code Review

Return to Question

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

Quite a little while ago, I wrote a VBA macro to generate fiscal calendar date records 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.

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.

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.

Source Link
Mathieu Guindon
  • 75.5k
  • 18
  • 194
  • 467

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

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?

lang-cs

AltStyle によって変換されたページ (->オリジナル) /