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

Refactoring chart generator in Linq

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

Problem

I'm kinda curious how I'd refactor this in the best way. I've only used Linq for simple queries.

What it does:

Depending on the type that is submitted, we're grouping events or payments by duration (eg. every year, every month, every day (for the moment).

I'd like to extend this to Quarterly and Weekly, but the class is getting to bulky.

Any tips on refactoring this?

```
public ActionResult Generate(MembershipManagement.ViewModels.ReportViewModel vm)
{
var chart = new ViewModels.ChartData();

Club club = Helpers.SiteHelper.GetCurrentClub();

List x_values = new List();
DateTime i = vm.StartDate.Date;

//go back to the first occurence of this duration
switch (vm.Duration)
{
case Enums.ReportDuration.Month:
i = i.AddDays(-(i.Day - 1));
break;
case Enums.ReportDuration.Quarterly:
//add check - 4 maand
i = i.AddDays(-(i.Day - 1));
break;
case Enums.ReportDuration.Year:
i = i.AddDays(-(i.Day - 1));
i = i.AddMonths(-(i.Month - 1));
break;
}
//case Enums.ReportDuration.Week:
// i = i.AddDays(7);
// if (i.Day attendance_data = null;
var attendance_raw_data = db.Attendances.Where(el => el.ClubId.Equals(club.Id) && ( el.On >= vm.StartDate && el.On dl.On);
var attendance_filtered_data = attendance_raw_data.Select(dl => new { On = dl.On, Count = dl.Count }).ToList();
switch (vm.Duration)
{
case Enums.ReportDuration.Day:
chart.Period = "day";
attendance_data = (from o in attendance_filtered_data
group o by o.On.ToString("yyyy-MM-dd") into g
select new ViewModels.ChartValue { x =

Solution

The problem is that you have a method that does many, many, many things.

The first thing I'd address is the switch blocks; a common way of refactoring those, is to use a Dictionary.

DateTime i = vm.StartDate.Date;

   //go back to the first occurence of this duration
   switch (vm.Duration)
   { 
       case Enums.ReportDuration.Month:
           i = i.AddDays(-(i.Day - 1));
           break;
       case Enums.ReportDuration.Quarterly:
           //add check - 4 maand
           i = i.AddDays(-(i.Day - 1));
           break;
       case Enums.ReportDuration.Year:
           i = i.AddDays(-(i.Day - 1));
           i = i.AddMonths(-(i.Month - 1));
           break;
   }


Here you'd have a method for each case, whose role is to determine the value of i (which is a very, very naughty name for a DateTime). Something along these lines:

private DateTime GetMonthlyReportStartDate(DateTime date)
{
    return date.AddDays(-(date.Day - 1));
}

private DateTime GetQuarterlyReportStartDate(DateTime date)
{
    //add check - 4 maand // <~ whatever that means
    return date.AddDays(-(date.Day - 1));
}

private DateTime GetYearlyReportStartDate(DateTime date)
{
    return date.AddDays(-(date.Day - 1))
               .AddMonths(-(date.Month - 1));
}


Then you map each method to a Dictionary value:

var reportStartDateSelector = new Dictionary>
    {
        { Enums.ReportDuration.Month, GetMonthlyReportStartDate },
        { Enums.ReportDuration.Quarterly, GetQuarterlyReportStartDate },
        { Enums.ReportDuration.Year, GetYearlyReportStartDate }
    };


Armed with such a dictionary, you can now replace the whole switch block like this:

var reportStartDate = reportStartDateSelector[vm.Duration](vm.StartDate.Date);


Rinse & repeat for all switch blocks; before you know it your Generate method will be much easier to follow (and debug!).

As you're refactoring your switch blocks into methods, you'll notice the redundant query code can be passed as some filteredData parameter (thus eliminating the redundancy) - notice I didn't put an underscore in the variable's name.

That said, this line:

Club club = Helpers.SiteHelper.GetCurrentClub();


Smells like ambient context to me. That would be the next thing I'd address; static helper classes are a nuisance if you want your code to be testable.

Code Snippets

DateTime i = vm.StartDate.Date;

   //go back to the first occurence of this duration
   switch (vm.Duration)
   { 
       case Enums.ReportDuration.Month:
           i = i.AddDays(-(i.Day - 1));
           break;
       case Enums.ReportDuration.Quarterly:
           //add check - 4 maand
           i = i.AddDays(-(i.Day - 1));
           break;
       case Enums.ReportDuration.Year:
           i = i.AddDays(-(i.Day - 1));
           i = i.AddMonths(-(i.Month - 1));
           break;
   }
private DateTime GetMonthlyReportStartDate(DateTime date)
{
    return date.AddDays(-(date.Day - 1));
}

private DateTime GetQuarterlyReportStartDate(DateTime date)
{
    //add check - 4 maand // <~ whatever that means
    return date.AddDays(-(date.Day - 1));
}

private DateTime GetYearlyReportStartDate(DateTime date)
{
    return date.AddDays(-(date.Day - 1))
               .AddMonths(-(date.Month - 1));
}
var reportStartDateSelector = new Dictionary<Enums.ReportDuration, Func<DateTime, DateTime>>
    {
        { Enums.ReportDuration.Month, GetMonthlyReportStartDate },
        { Enums.ReportDuration.Quarterly, GetQuarterlyReportStartDate },
        { Enums.ReportDuration.Year, GetYearlyReportStartDate }
    };
var reportStartDate = reportStartDateSelector[vm.Duration](vm.StartDate.Date);
Club club = Helpers.SiteHelper.GetCurrentClub();

Context

StackExchange Code Review Q#38192, answer score: 7

Revisions (0)

No revisions yet.