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

    }

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.