Problem
I am trying to build a report based on certain condition. The problem is that I have a massive switch statement which is a “code smell”.
How can refactor the following so that it is more maintainable?
I just copied/paste some of the values to show how the case will look.
switch (countryId)
{
case 1:
reportParameterValues = TaxReportParameterFactory.CreateReport(taxCountry, employeeId, taxYearId, frequencyId, null, "N", employeeRun.RunId, 0, 0, 0, true);
break;
case 4:
reportParameterValues = TaxReportParameterFactory.CreateReport(taxCountry, employeeId, taxYearId, frequencyId, null, "N");
break;
case 23:
reportParameterValues = TaxReportParameterFactory.CreateReport(taxCountry, "N", employeeRun.RunId, 0, 0, 0, true);
break;
case 20:
reportParameterValues = TaxReportParameterFactory.CreateReport(taxCountry, employeeId, taxYearId, frequencyId, null, "N", employeeRun.RunId, 0, 0, 0, true);
break;
case 25:
reportParameterValues = TaxReportParameterFactory.CreateReport(taxCountry, employeeId, taxYearId, frequencyId, null, "N", employeeRun.RunId, 0, 0, 0, true);
break;
case 27:
reportParameterValues = TaxReportParameterFactory.CreateReport(taxCountry, employeeId, taxYearId, frequencyId, null, "N", employeeRun.RunId, 0, 0, 0, true);
break;
case 14:
reportParameterValues = TaxReportParameterFactory.CreateReport(taxCountry, employeeId, taxYearId, frequencyId, null, "N", employeeRun.RunId, 0, 0, 0, true);
break;
case 31:
reportParameterValues = TaxReportParameterFactory.CreateReport(taxCountry, employeeId, taxYearId, frequencyId, null, "N", employeeRun.RunId, 0, 0, 0, true);
break;
case 30:
reportParameterValues = TaxReportParameterFactory.CreateReport(taxCountry, employeeId, taxYearId, frequencyId, null, "N", employeeRun.RunId, 0, 0, 0, true);
break;
case 28:
reportParameterValues = TaxReportParameterFactory.CreateReport(taxCountry, employeeId, taxYearId, frequencyId, null, "N", employeeRun.RunId, 0, 0, 0, true);
break;
}
public class TaxCounty
{
public string CountryCode { get; set; }
public string ReportPath { get; set; }
public string[] ReportParameters { get; set; }
}
public static class TaxCountries
{
public static Dictionary<int, TaxCounty> Type =>
new Dictionary<int, TaxCounty>
{
{ 1, new TaxCounty { CountryCode = "SA", ReportPath = "/PathToReport", ReportParameters = new[] { "EmployeeID", "BureauTaxID", "FrequencyID", "DirectiveNumber", "Directive", "RunID", "UserID", "GroupID", "RegionID", "IncludeESS" } } },
{ 4, new TaxCounty { CountryCode = "SWZ", ReportPath = "/PathToReport", ReportParameters = new[] { "EmploymentStatusID", "BureauTaxID", "FrequencyID", "RunID" } } },
{ 23, new TaxCounty { CountryCode = "BOTS", ReportPath = "/PathToReport", ReportParameters = new[] { "EmploymentStatusID", "BureauTaxID", "FrequencyID", "RunID" } } },
{ 20, new TaxCounty { CountryCode = "MOZ", ReportPath = "/PathToReport", ReportParameters = new[] { "EmploymentStatusID", "BureauTaxID", "FrequencyID", "RunID" } } },
{ 25, new TaxCounty { CountryCode = "Tanzania", ReportPath = "/PathToReport", ReportParameters = new[] { "EmployeeID", "BureauTaxID" } } },
{ 27, new TaxCounty { CountryCode = "NAM", ReportPath = "/PathToReport", ReportParameters = new[] { "BureauTaxID", "EmploymentStatusID", "FrequencyID" } } },
{ 14, new TaxCounty { CountryCode = "GHANA", ReportPath = "/PathToReport", ReportParameters = new[] { "BureauTaxID", "FrequencyID", "fkEmpStatusID", "CompanyID" } } },
{ 31, new TaxCounty { CountryCode = "MALA", ReportPath = "/PathToReport", ReportParameters = new[] { "BureauTaxID", "EmploymentStatusID", "FrequencyID" } } },
{ 30, new TaxCounty { CountryCode = "ZIM", ReportPath = "/PathToReport", ReportParameters = new[] { "BureauTaxID", "FrequencyID", "EmpStatus" } } },
{ 28, new TaxCounty { CountryCode = "MAU", ReportPath = "/PathToReport", ReportParameters = new[] { "BureauTaxID", "EmploymentStatusID", "FrequencyID" } } },
{ 12, new TaxCounty { CountryCode = "GAB", ReportPath = "/PathToReport", ReportParameters = new[] { "BureauTaxID", "EmploymentStatusID", "CompanyID" } } },
{ 37, new TaxCounty { CountryCode = "CONGO", ReportPath = "/PathToReport", ReportParameters = new[] { "BureauTaxID", "EmploymentStatusID", "CompanyID" } } },
{ 49, new TaxCounty { CountryCode = "LESO", ReportPath = "/PathToReport", ReportParameters = new[] { "BureauTaxID", "EmploymentStatusID", "FrequencyID" } } },
{ 22, new TaxCounty { CountryCode = "KENYA", ReportPath = "/PathToReport", ReportParameters = new[] { "CompanyID", "EmpID", "TaxYearID", "CompanyFrequencyID" } } }
};
}
I have a Dictionary that holds all the information so that I can do an easy lookup. Then I use the factory map the parameters. But what a maintance nightmare.
public static class TaxReportParameterFactory
{
public static Dictionary<string, object> CreateReport(TaxCounty taxCountry, params object[] values)
{
var reportParameterValues = taxCountry.ReportParameters.Zip(values, (k, v) => new { k, v }).ToDictionary(x => x.k, x => x.v);
reportParameterValues.Add("ReportPath", taxCountry.ReportPath);
return reportParameterValues;
}
}
Solution
Refactor incrementally, it will give you better insight into the code:
I’d start with a quick refactor of the switch statement as follows:
switch (countryId)
{
case 4:
reportParameterValues = TaxReportParameterFactory.CreateReport(taxCountry, employeeId, taxYearId, frequencyId, null, "N");
break;
case 23:
reportParameterValues = TaxReportParameterFactory.CreateReport(taxCountry, "N", employeeRun.RunId, 0, 0, 0, true);
break;
default:
reportParameterValues = TaxReportParameterFactory.CreateReport(taxCountry, employeeId, taxYearId, frequencyId, null, "N", employeeRun.RunId, 0, 0, 0, true);
break;
}
That’s a little better and it allows you to focus on the primary issue; the fact that the CreateReport method is overloaded and thus it parameters can vary.
I’d opt for the version of the CreateReport method with the most parameters and refactor the other calls to use that instead and then look at the parameters themselves. If possible I’d set some defaults for those arguments passed to that function and eliminate the switch statement altogether.
Use the “Replace conditional with polymorphism” refactoring. You didn’t provide much context to the switch statement, so I’m afraid that that is all I can say.
I can’t see the declaration of the method that contains the switch, but does it have a long, giant list of arguments, like employeeId, taxYearId, frequencyId, and employeeRun? One step might be to package those up into a single parameters class.
When creating the report parameters, it looks like many reports expect the same parameters, but using different names. One expects EmploymentStatusID, another fkEmpStatusID, but it’s the same value. Can you modify the reports to accept the same parameters – the class containing your parameters – and have them map to whatever specific parameters they need?
These solutions don’t immediately get you to where you’re going. They just clear up some of the confusion so that you can see the path more clearly. Each of those reports has its own nuances and differently named parameters, and what’s happened is that they’ve pushed their confusion out into the code that calls them.
One of the first steps toward dealing with all of these reports polymorphically is that from the calling code, they all need to look the same. They need to present the same interface. Then, within their own implementations they can deal with their own issues – I don’t like this parameter name, I want it named that instead, etc.
If the calling code has to deal with the quirks and differences of all of those different reports, then the methods will be overwhelming and confusing to look at. That confusion makes it hard to even see what you want to do with this.
I think it was the book Code Complete that said that the job of the programmer is to manage complexity. Everything is going to be as complex as it’s going to be. The idea is to keep the complexity of each component isolated. At a higher level of abstraction you just see a bunch of reports, perhaps declaring an interface like ITaxReport. At a lower level, in the varying implementations, that’s where you deal with the individual behaviors.
It looks like you have a limited number of parameter templates that are being duplicated based on the country in which they are used. My thought would by to create an intermediate structure or additional piece of data that tracks which parameter template to use because you appear to have fewer parameter templates than countries.