3
\$\begingroup\$

A problem I come across very often is what to do with branching classes. The class below is one example of a class that can turn ugly very fast.

The requirements of the class is that if the address is in Alabama use an external service, but all other states should use a database. The switch statements filter down which method to use. Each method has it's own way of processing the data and formatting it properly.

How can I make this class and these types of situations cleaner?

public class TaxRateServiceConnector : ITaxRateServiceConnector
{
    private readonly AppContext _context;

    public TaxRateServiceConnector(AppContext context)
    {
        _context = context;
    }

    public List<Taxes> GetTaxRates(Address address)
    {
        List<Taxes> taxList;

        if (address == null)
        {
            throw new ArgumentNullException("Address may not be null");
        }

        var state = address.Region.Description;

        switch (state)
        {
            case "Alabama":
                taxList = GetAlabamaTaxRates(address);
                break;
            default:
                taxList = GetDefaultTaxRates(address);
                break;
        }

        // If tax list is empty, then return a default 6.5% tax
        return taxList ?? new List<Taxes>()
        {
            new Taxes()
            {
                TaxDescription = "Sales Tax",
                TaxAmount = 0.08M
            }
        };
    }

    // Use Alabama service for taxes
    private List<Taxes> GetAlabamaTaxRates(Address address)
    {
        AlabamaTaxService alabamaService = new AlabamaTaxService();
        List<Taxes> taxRates = null;

        string zip = address.PostCode.Substring(0, 5);
        int zipNum;

        bool convertSuccess = Int32.TryParse(zip, out zipNum);

        if (!convertSuccess)
        {
            return null;
        }

        var alabamaServiceResponse = alabamaService.GetSalesTaxRate(zipNum, DateTime.Now);

        bool zipFound = alabamaServiceResponse.intReturnCode == (int)AlabamaServiceResultCode.ZipFound;
        if (!zipFound)
        {
            return null;
        }

        var taxRateResponse = alabamaServiceResponse.Record.Select(
            r => new TaxRateServiceResponse()
            {
                TaxDescription = r.jurisdiction,
                TaxRate = Convert.ToDecimal(r.GeneralTaxRate) / 100
            }).ToList();

        if (taxRateResponse.Any())
        {
            taxRates = taxRateResponse.Select(
            r => new Taxes()
            {
                TaxDescription = r.TaxDescription,
                TaxRate = r.TaxRate
            }).ToList();
        }

        if (taxRates == null)
        {
            return null;
        }

        // Standardize tax jurisdiction codes
        foreach (var tax in taxRates)
        {
            switch (tax.TaxDescription)
            {
                case "STA":
                    tax.TaxDescription = "State Tax";
                    break;
                case "CTY":
                    tax.TaxDescription = "County Tax";
                    break;
                case "CIT":
                    tax.TaxDescription = "City Tax";
                    break;
                default:
                    tax.TaxDescription = "Local Tax";
                    break;
            }
        }

        return taxRates;
    }

    // Use database for lookup
    private List<Taxes> GetDefaultTaxRates(Address address)
    {
        List<Taxes> taxRates = null;

        var taxRateResponse = _context.GetTaxRates(address.SubRegion, address.RegionId).Select(
            r => new TaxRateServiceResponse()
            {
                TaxDescription = r.FeeType,
                TaxRate = r.Rate
            }).ToList();


        if (taxRateResponse.Any())
        {
            taxRates = taxRateResponse.Select(
            r => new Taxes()
            {
                TaxDescription = r.TaxDescription,
                TaxRate = r.TaxRate
            }).ToList();
        }

        return taxRates;
    }
}

public class TaxRateServiceResponse
{
    public string TaxDescription;
    public decimal TaxRate;
}
\$\endgroup\$
  • \$\begingroup\$ Please also retitle the question according to what the code accomplishes (sales tax calculation) rather than your concern about branching. See How to Ask. \$\endgroup\$ – 200_success Oct 3 '16 at 19:37
  • \$\begingroup\$ @200_success I've fixed the function. It should be GetAlabamaTaxRates(). The class does work as defined. The issue I'm having is with cleaning up the branching, not the sales tax calculation. It gives the correct outputs, but I'm sure there are cleaner methods. \$\endgroup\$ – chiiefs Oct 3 '16 at 19:38
4
\$\begingroup\$

I think you can refactor the sources as ITaxRateSource like this:

public interface ITaxRateSource 
{ 
    List<Taxes> GetTaxRates(Address address);
}

and implement each one in a separate class:

public class DefaultTaxRateSource : ITaxRateSource
{
    public List<Taxes> GetTaxRates(Address address)
    {
        ...
    }
}

public class AlabamaTaxRateSource : ITaxRateSource
{
    public List<Taxes> GetTaxRates(Address address)
    {
        ...
    }
}

Then you modify the constructor to accept custom sources (or by params, whatever you like better):

public TaxRateServiceConnector(
    AppContext context, 
    IEnumerable<KeyValuePair<string, ITaxRateSource>> taxRateSources = null)
{
    _context = context;

    // whether you add a default one or not is your choice
    _taxRateSources = new Dictionary<string, ITaxRateSource>
    {
        [string.Empty] = new DefaultTaxRateSource()
    };

    if (taxRateSources != null)
    {
        foreach (var item in taxRateSources)
        {
            // you may use the indexer _taxRateSources.[item.Key] 
            // if you want to be able to override the default source
            _taxRateSources.Add(item.Key, item.Value);
        }
    }
}

and GetTaxRates to use the dictionary insteas of a switch:

public List<Taxes> GetTaxRates(Address address)
{
    ...

    var state = address.Region.Description;

    var taxRateSource = (ITaxRateSource)null;
    if (!_taxRateSources.TryGetValue(state, out taxRateSource)) 
    {
        taxRateSource = _taxRateSources[string.Empty];
    }

    var taxRates = taxRateSource.GetTaxRates(...);

    ...
}

to create a new instance of the TaxRateServiceConnector you pass it a dictionary with the custom source:

var taxRateSources = new Dictionary<string, ITaxRateSource>
{
    ["Alabama"] = new AlabamaTaxRateSource()
};

Now you are free to create as many custom sources as you like.


Other designs are possible too. You could create custom sources by overriding the GetTaxRates method of same base class. Sometimes it's a matter of taste whether you choose an interface or an inheritance.

Yet another one would be to create an attribute like [State("Alabama")] and decorate the sources with it and build a source with composition and use the attribute for source lookup.

Yet another one would be to create an enum State and decorate its values with a factory attribute that creates the appropriate source for the state.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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