patterncsharpMinor
Refactoring chart generator in Linq
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 =
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
Here you'd have a method for each case, whose role is to determine the value of
Then you map each method to a
Armed with such a dictionary, you can now replace the whole
Rinse & repeat for all
As you're refactoring your
That said, this line:
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.
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.