0
\$\begingroup\$

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;
    }
}
\$\endgroup\$
8
  • \$\begingroup\$ to show how the case will look - this sounds like it wasn't your actual code. \$\endgroup\$
    – t3chb0t
    Dec 12, 2017 at 10:11
  • \$\begingroup\$ @t3chb0t So I am busy rewriting an old vb.net application. the above code was my rewrite. (refactor). I can show the original but your eye might start bleeding. \$\endgroup\$
    – R4nc1d
    Dec 12, 2017 at 10:23
  • 1
    \$\begingroup\$ You can remove the whole switch-case statement if you store all input (employeeId, taxYearId, etc.) in a dictionary (using the same keys as found in TaxCountry.ReportParameters), and pass that on to CreateReport, which can then produce a dictionary with only the required keys. \$\endgroup\$ Dec 12, 2017 at 14:55
  • \$\begingroup\$ @PieterWitvoet OP posted a dictionary in the first version but then removed it :-| \$\endgroup\$
    – t3chb0t
    Dec 12, 2017 at 19:00
  • \$\begingroup\$ @t3chb0t, i am not sure what you mean that i removed a version with a dictionary? Code is still the same? \$\endgroup\$
    – R4nc1d
    Dec 12, 2017 at 19:23

4 Answers 4

1
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ So in my example, I just copy pasted some case statements so that params will vary for each one. So the default case wont really apply here. \$\endgroup\$
    – R4nc1d
    Dec 12, 2017 at 14:18
  • \$\begingroup\$ @R4nc1d thats why we don't allow stub/example code. Someone takes time to answer a question and will be told that it isn't the real code. Just great. \$\endgroup\$
    – Heslacher
    Dec 12, 2017 at 14:26
  • \$\begingroup\$ This results in different behavior when an 'unsupported' country ID is given. I would (for now) keep all cases, but let those with duplicate code fall-through to a single line of code. \$\endgroup\$ Dec 12, 2017 at 14:30
  • \$\begingroup\$ @Heslacher, in all fairness that why i said "I just copied/paste some of the values to show how the case will look" \$\endgroup\$
    – R4nc1d
    Dec 12, 2017 at 14:31
1
\$\begingroup\$

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.

\$\endgroup\$
0
0
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.