0

I am currently refactoring a piece of code to keep things testable and maintainable in our c# application.

I've stumbled upon a scenario where an existing method returns data with lists and enums that is then processed with lots of if else conditions in a big long method that is 800+ lines long. Lucky me!

I was trying to pick a suitable design pattern from the Gang Of Four, but cannot see something that fits. I will outline what I have in place at the minute. Any ideas on how to best tackle/refactor the following?

The following code has been simplified from what I have in production. Its purpose is to show the cut down logic in the scenario. This is what I have refactored to so far. Don't worry about statics and that there are no interfaces - interfaces have been omitted for simplicity.

public class MySearchClass
{
    public static SearchResult Find(QueryParameters queryParams)
    {
        ...
    }
}

public class SearchEntityResult
{
    public IEnumerable<SearchEntityData> Matches { get; set; }
}

public class SearchEntityData
{
    public SearchMatchType MatchType { get; set; }
    public Guid SearchEntityId { get; set; }
    public string EntityName { get; set; }
}

public enum SearchMatchType
{
    Partial,
    Exact   
}


public class BigLongMethodClassProcessor
{
    public static void DoFindAndProcess()
    {
        ... spaghetti code ...

        SearchEntityResult result = MySearchClass.Find(...);

        if (result.BestMatch == SearchMatchType.Exact)
        {
            ...
        }
        else
        {
            foreach (var m in  result.Matches)
            {
                if (m.MatchType == SearchMatchType.Partial)
                {
                    ... do this ...
                }
                else if (m.MatchType == SearchMatchType.Exact)
                {
                    ... do that ...
                }
            }   
        }

        ...
    }
}

My thoughts was to have a factory that would look at the SearchEntityResult and create an appropriate SearchEntityResultProcessor.

public class ExactSearchEntityResultProcessor : ISearchEntityResultProcessor
{
    public void Process(SearchEntityResult result)
    {
        ...
    }
}

public class SearchEntityResultProcessorFactory
{
    public static ISearchEntityResultProcessor Create(SearchEntityResult result)
    {
        if (result.BestMatch == SearchMatchType.Exact)
        {
            return new ExactSearchEntityResultProcessor();
        }
        else if (result.BestMatch == SearchMatchType.Partial)
        {
            return new PartialSearchEntityResultProcessor();
        }
        else
        {
            // throw
        }
    }
}

So BigLongMethodClassProcessor will look like:

public class BigLongMethodClassProcessor
{
    public static void DoFindAndProcess()
    {           
        SearchEntityResult result = MySearchClass.Find(...);
        ISearchEntityResultProcessor processor = SearchEntityResultProcessorFactory.Create(result);
        processor.Process(result);
    }
}

Then all statics will be removed and interfaces introduced.

  • 6
    Keep in mind that Patterns aren't a buffet of solutions - they are documentation tools. They can't fix anything! I recommend trying to understand the possibilities of your language first and drawing your own conclusions about how to solve this instead of looking for a magic formula for that. They are not finished designs. Instead, look at them as how other people solved their problems and think about yours in a more critical way. – T. Sar Mar 20 '17 at 14:22
  • Re-factor your code using the Hollywood Principle and use the Command Pattern with each of your Processor class being Commands. – Martin Spamer Mar 20 '17 at 20:46
  • 1
    "interface introduced" is not an inherent advantage, but a disadvantage, unless the interface is needed. The same is true for all superfluous abstractions. – Frank Hileman Mar 21 '17 at 15:16
1

Your solution looks good to me. You are constructing the processor that can handle a result then processing that result using it.

Another option is the chain of responsibility pattern. http://www.dofactory.com/net/chain-of-responsibility-design-pattern

This is like your solution except the check for whether a processor can handle the result is contained within the processor itself. Then you'd just have a list of all processors and pass the result to each in turn until one of them was able to process it. That way if a new processor is developed it just needs to be added to the list.

0

This approach doesn't look too bad.

You have one remaining section where you have a bunch of if statements. If your processors are stateless, you could eliminate that by using a lookup table, like this:

public class SearchEntityResultProcessorFactory
{
    static private Dictionary<SearchMatchType,ISearchEntityResultProcessor > _processorLookup = new  Dictionary<SearchMatchType,ISearchEntityResultProcessor>();

    static SearchEntityResultProcessorFactory()
    {
        _processorLookup.Add(SearchMatchType.Exact, new ExactSearchEntityResultProcessor());
        _processorLookup.Add(SearchMatchType.Partial, new PartialSearchEntityResultProcessor());
    }

    public static ISearchEntityResultProcessor Create(SearchEntityResult result)
    {
        return _processorLookup[result.BestMatch];
    }
}

If they are not stateless, you can still do it, but have to use delegates:

public class SearchEntityResultProcessorFactory
{
    static private Dictionary<SearchMatchType,Func<ISearchEntityResultProcessor>> _processorLookup = new Dictionary<SearchMatchType,Func<ISearchEntityResultProcessor>>();

    static SearchEntityResultProcessorFactory()
    {
        _processorLookup.Add(SearchMatchType.Exact, () => new ExactSearchEntityResultProcessor());
        _processorLookup.Add(SearchMatchType.Partial, () => new PartialSearchEntityResultProcessor());
    }

    public static ISearchEntityResultProcessor Create(SearchEntityResult result)
    {
        return _processorLookup[result.BestMatch]();
    }
}

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.