Refactoring chart generator in Linq

Question

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<DateTime> x_values = new List<DateTime>();
 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 <= 6)
 // {
 // i = i.AddDays(-(i.Day - 1));
 // }
 // break;
 //add x-as values (not required currently, perhaps for refactoring this as it contains the limits of the x-as.
 while (i <= vm.EndDate)
 {
 x_values.Add(i);
 switch (vm.Duration)
 {
 case Enums.ReportDuration.Quarterly:
 x_values.Add(i);
 i = i.AddMonths(4);
 break;
 case Enums.ReportDuration.Month:
 x_values.Add(i);
 i = i.AddMonths(1);
 break;
 case Enums.ReportDuration.Year:
 x_values.Add(i);
 i = i.AddYears(1);
 break;
 default:
 x_values.Add(i);
 i = i.AddDays(1);
 break;
 }
 }
 switch (vm.Source)
 { 
 case Enums.ReportSource.Attendance:
 //1
 List<ViewModels.ChartValue> attendance_data = null;
 var attendance_raw_data = db.Attendances.Where(el => el.ClubId.Equals(club.Id) && ( el.On >= vm.StartDate && el.On <= vm.EndDate)).OrderBy(dl => 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 = g.Key, value = g.Sum(dl => dl.Count) }).ToList();
 break;
 case Enums.ReportDuration.Month:
 chart.Period = "month";
 attendance_data = (from o in attendance_filtered_data
 group o by o.On.ToString("yyyy-MM") into g
 select new ViewModels.ChartValue { x = g.Key, value = g.Sum(dl => dl.Count) }).ToList();
 break;
 case Enums.ReportDuration.Quarterly:
 break;
 case Enums.ReportDuration.Year:
 chart.Period = "year";
 attendance_data = (from o in attendance_filtered_data
 group o by o.On.ToString("yyyy") into g
 select new ViewModels.ChartValue { x = g.Key, value = g.Sum(dl => dl.Count) }).ToList();
 break;
 }
 chart.Labels.Add("Aanwezigheden");//= attendance_data.Select(el => System.Globalization.CultureInfo.CurrentCulture.DateTimeFormat.GetMonthName(int.Parse(el.x.Substring(5, 2)))).ToList();
 chart.data = attendance_data;
 chart.xkey = "x";
 chart.yKeys.Add("value");
 //chart.Period = "month";
 //2
 //var raw_data = db.Attendances.Where(el => el.ClubId.Equals(club.Id) && (el.On >= vm.StartDate && el.On <= vm.EndDate)).OrderBy(dl => dl.On)
 // .Select(dl => new { On = dl.On, Count = dl.Count}).ToList();
 //var att_data = from year in Enumerable.Range(vm.StartDate.Year,vm.EndDate.Year - vm.StartDate.Year)
 // from month in Enum
 break;
 case Enums.ReportSource.Financial:
 var financial_raw_data = db.Payments.Where(el => el.ClubId.Equals(club.Id) &&( el.On >= vm.StartDate && el.On <= vm.EndDate)).OrderBy(dl => dl.On);
 var financial_filtered_data = financial_raw_data.Select(dl => new { On = dl.On, Value = dl.Amount * (int)dl.InOrOut }).ToList();
 List<ViewModels.ChartValue> financial_data = null;
 switch (vm.Duration)
 {
 case Enums.ReportDuration.Day:
 chart.Period = "day";
 financial_data = (from o in financial_filtered_data
 group o by o.On.ToString("yyyy-MM-dd") into g
 select new ViewModels.ChartValue { x = g.Key, value = (int)g.Sum(el => el.Value) }).ToList();
 break;
 case Enums.ReportDuration.Month:
 chart.Period = "month";
 financial_data = (from o in financial_filtered_data
 group o by o.On.ToString("yyyy-MM") into g
 select new ViewModels.ChartValue { x = g.Key, value = (int)g.Sum(el => el.Value) }).ToList();
 break;
 case Enums.ReportDuration.Quarterly:
 break;
 case Enums.ReportDuration.Year:
 chart.Period = "year";
 financial_data = (from o in financial_filtered_data
 group o by o.On.ToString("yyyy") into g
 select new ViewModels.ChartValue { x = g.Key, value = (int)g.Sum(el => el.Value) }).ToList();
 break;
 }
 chart.Labels.Add("Financiëel");// financial_data.Select(el => System.Globalization.CultureInfo.CurrentCulture.DateTimeFormat.GetMonthName(int.Parse(el.x.Substring(5, 2))) + " €").ToList();
 chart.data = financial_data;
 chart.xkey = "x";
 chart.yKeys.Add("value");
 break;
 default:
 break;
 }
 /*
 * 1
 * 
 * var query = repository.Flights .GroupBy(flight => flight.Date.Day) .OrderBy(group => group.Key) .ToList();
 * 
 * var xValues = query.Select(group => group.Key).ToList(); 
 * totalDelaySeries.Points.DataBindXY( xValues, query.Select( group => group.Sum( flight => flight.ArrivalDelay)).ToList()); 
 * weatherDelaySeries.Points.DataBindXY( xValues, query.Select( group => group.Sum( flight => flight.WeatherDelay)).ToList());
 * 
 * var overThresholdPoints = taxiOutSeries.Points .Where(p => p.YValues.First() > _taxiThreshold); 
 * foreach (var point in overThresholdPoints) { point.Color = Color.Red; }
 * 
 * 2
 * 
 * var sales = from o in context.Orders
 * where o.DatePlaced.Value.Year == year
 * group o by o.DatePlaced.Value.Month into g
 * select new {Month = g, Orders = g.Count()};
 * 
 * foreach (var sale in sales)
 * SalesChart.Series["Series1"].Points
 * .AddXY(Enum.Parse(typeof(Month), 
 * sale.Month.FirstOrDefault().DatePlaced.Value.Month.ToString()).ToString(),
 * (double)sale.Orders);
 */
 vm.chartData = chart;
 return View(vm);
 }

Post answer - Refactored outcome:

public ActionResult Generate(MembershipManagement.ViewModels.ReportViewModel vm)
 {
 Club club = Helpers.SiteHelper.GetCurrentClub();
 //calculate the first date
 DateTime reportStartDate = ChartExtensions.StartDateSelector[vm.Duration](vm.StartDate.Date);
 //generate chart
 vm.chartData = GenerateChartByType[vm.Source](club.Id, vm.StartDate, vm.EndDate, vm.Duration);
 return View(vm);
 }

• Mathieu Guindon: You turned that monster into a 4-liner! Well done! Remember that comments say why, the code says what. Also I'm not sure about calling a dictionary GenerateChartByType - that's a name for a method, but it's a dictionary of methods you have in there; also, if it's more readable (probably is), you could separate the dictionary value extraction from the method call, like var someDelegate = GenerateChartByType[vm.Source];. Don't hesitate to post your refactored code as another CR question! Feed the dragon!

Answer 1

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<TKey,TValue>.

 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, Func<DateTime, DateTime>>
 {
 { 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.

• NicoJuicy: Yeah, i'm using a static SiteHelper (most of the time) to get an object i use a lot. Any docs on refactoring that? (currently it's helped me... But always open to suggestions..)
• NicoJuicy: PS. You've shown some great refactoring (and things i'll use in the future).
• svick: So you're saying that for each switch, there should be a separate dictionary of delegates? I think that a single dictionary of objects that implement an interface (something like IPeriod) would be much cleaner.