design pattern to remove multiple if/else clauses with related objects

Question

I have inherited the following (terrible) code and am wondering how best to refactor it.

There are large if/else clauses all over the codebase, one of which is similar to below :

public class BaseResultItem
{
    public int Property1 { get; set; }
}

public class ResultItem1 : BaseResultItem
{
    public int Property2 { get; set; }
}

public class ResultItem2 : BaseResultItem
{
    public int Property3 { get; set; }
}

public class BaseHistoryItem
{
    public int Property1 { get; set; }
}

public class HistoryItem1 : BaseHistoryItem
{
    public int Property2 { get; set; }
}

public class HistoryItem2 : BaseHistoryItem
{
    public int Property3 { get; set; }
}

public class HistoryBuilder
{
    public BaseHistoryItem BuildHistory(BaseResultItem result)
    {
        BaseHistoryItem history = new BaseHistoryItem            
        {
            Property1 = result.Property1
        };

        if (result is ResultItem1)
        {
            ((HistoryItem1)history).Property2 = ((ResultItem1)result).Property2;
        }
        else if (result is ResultItem2)
        {
            ((HistoryItem2)history).Property3 = ((ResultItem2)result).Property3;
        }

        return history;
    }
}

Note that this is a simplified example and there are many more classes involved in the actual code. There are similar if/else clauses all over the place.

I have been looking at the abstract factory pattern but I am having some problems.

Basically I am assuming that to avoid the if/else problems I need to pass the actual dervied types around. So BuildHistory should not use base types and maybe there should be multiple methods, one per derived type?

Answer 1

The general 'design pattern' is simply to use object orientation with polymorphism instead of type checks. Thus: a BuildHistory method inside BaseResultItem, overridden by descendants.

Any code which checks the concrete type of an object smells (in a refactoring sense). Supporting different behaviours for different types is what OO is about.

Answer 2

Use polymorphism to remove the type checks.

if (result is ResultItem1)
{
    ((HistoryItem1)history).Property2 = ((ResultItem1)result).Property2;
}

Becomes then something like

result.addToHistory( history );

If for some reason, you don't want to scatter the logic in the item classes, have a look at the visitor pattern. In this case, you have something like:

public class Visitor {
     History history;
     public visit ( ResultItem1 item )  { ... }
     public visit ( ResultItem2 item )  { ... }
     ...
}

public class ResultItem1 {
     public accept( Visitor v ) { v.visit( this ); }
}

The typecheck is removed by the double-dispatch in the visitor, which is slightly more elegant.

I didn't understood exactly how the various kind of history relates to the various kind of items. So this is just a sketch of possibles direction to follow.

Answer 3

If you can't change the DTO classes perhaps you can try to subclass HistoryBuilder to deal with the different subclasses. Then you use the appropriate HistoryBuilderX to create a HistoryItem from a ResultItem. Then the question is how to get the appropriate HistoryBuilderX for the ResultItem supplied.

Still, if you can't change the BaseResultItem class to include a GetBuilder function you need to use some if..else if.. construct that inspects the classtypes of your ResultItems.

Or you create a Registry where every ResultItem class is registered with its corresponding HistoryBuilderX class. But that might be overkill.