HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
yearloopeverylikenestednastyone

Problem

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
{
///
/// An object that encapsulates the logic to generate models.
///
public interface IGeneratorService
{
///
/// Generates a for each date in the specified year.
///
/// Fiscal year to generate records for.
///
IEnumerable Generate(int year);

///
/// Generates a for each date in the specified interval.
///
/// First fiscal year to generate records for.
/// Last fiscal year to generate records for.
IEnumerable 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 Generate(int year)
{
return Generate(year, year);
}

public IEnumerable Generate(int fromYear, int toYear)
{
var currentDate = _calculator.GetFiscalYearStartDate(fromY

Solution

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 Generate(int fromYear, int toYear)
{
    var counter = new Counter();

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

    for (var currentYear = fromYear; currentYear  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 :)

Code Snippets

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; }
    [...]
}
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;            
    }

Context

StackExchange Code Review Q#97154, answer score: 6

Revisions (0)

No revisions yet.