Refactoring Code to avoid Type Casting

Question

I have following C# code in .Net 4.0. It requires a type casting of IBusiness to IRetailBusiness.

//Type checking
if (bus is IRetailBusiness)
{
       //Type casting
       investmentReturns.Add(new RetailInvestmentReturn((IRetailBusiness)bus));
}

if (bus is IIntellectualRights)
{
       investmentReturns.Add(new IntellectualRightsInvestmentReturn((IIntellectualRights)bus));
}

Business Scenario:

I am designing a software system for and Investment Holding Company. The company has Retail business and IntellectualRights business. BookShop and AudioCDShop are examples of Retail business. EngineDesignPatent and BenzolMedicinePatent are examples of IntellectualRights business. These two business types are totally unrelated.

The investment company has a concept called InvestmentReturn (But each individual business is totally ignorant about this concept). InvestmentReturn is the profit gained from each business and it is calulated using ProfitElement. For each “Business Type” (Retail, IntellectualRights ), the ProfitElement used is different.

QUESTION

How to refactor this class design to avoid this type casting and type checking?

Abstract Investment

public abstract class InvestmentReturn
{
    public double ProfitElement { get; set; }
    public IBusiness Business{ get;  set; }

    public abstract double GetInvestmentProfit();

    public double CalculateBaseProfit()
    {
       double profit = 0;

       if (ProfitElement < 5)
       {
           profit = ProfitElement * 5 / 100;
       }
       else if (ProfitElement < 20)
       {
           profit = ProfitElement * 7 / 100;
       }
       else
       {
           profit = ProfitElement * 10 / 100;
       }

       return profit;
    }
}

Extensions

public class RetailInvestmentReturn : InvestmentReturn
{
    public RetailInvestmentReturn(IRetailBusiness retail)
    {
        Business = retail;
    }

    public override  double GetInvestmentProfit()
    {
        //GrossRevenue is the ProfitElement for RetailBusiness
        ProfitElement = ((IRetailBusiness)Business).GrossRevenue;
        return base.CalculateBaseProfit();
    }  
}

public class IntellectualRightsInvestmentReturn : InvestmentReturn
{

    public IntellectualRightsInvestmentReturn(IIntellectualRights intellectual)
    {
        Business = intellectual;
    }

    public override double GetInvestmentProfit()
    {
        //Royalty is the ProfitElement for IntellectualRights Business
        ProfitElement = ((IIntellectualRights)Business).Royalty;
        return base.CalculateBaseProfit();
    }
}

Client

class Program
{

    static void Main(string[] args)
    {

        #region MyBusines

        List<IBusiness> allMyProfitableBusiness = new List<IBusiness>();

        BookShop bookShop1 = new BookShop(75);
        AudioCDShop cd1Shop = new AudioCDShop(80);
        EngineDesignPatent enginePatent = new EngineDesignPatent(1200);
        BenzolMedicinePatent medicinePatent = new BenzolMedicinePatent(1450);

        allMyProfitableBusiness.Add(bookShop1);
        allMyProfitableBusiness.Add(cd1Shop);
        allMyProfitableBusiness.Add(enginePatent);
        allMyProfitableBusiness.Add(medicinePatent);

        #endregion

        List<InvestmentReturn> investmentReturns = new List<InvestmentReturn>();

        foreach (IBusiness bus in allMyProfitableBusiness)
        {
            //Type checking
            if (bus is IRetailBusiness)
            {
                //Type casting
                investmentReturns.Add(new RetailInvestmentReturn((IRetailBusiness)bus));
            }

            if (bus is IIntellectualRights)
            {
                investmentReturns.Add(new IntellectualRightsInvestmentReturn((IIntellectualRights)bus));
            }
        }

        double totalProfit = 0;
        foreach (var profitelement in investmentReturns)
        {
            totalProfit = totalProfit + profitelement.GetInvestmentProfit();
            Console.WriteLine("Profit: {0:c}", profitelement.GetInvestmentProfit());
        }

        Console.ReadKey();
    }
}

Business Domain Entities

public interface IBusiness
{

}

public abstract class EntityBaseClass
{

}

public interface IRetailBusiness : IBusiness
{
    double GrossRevenue { get; set; }
}

public interface IIntellectualRights : IBusiness
{
    double Royalty { get; set; }
}



#region Intellectuals
public class EngineDesignPatent : EntityBaseClass, IIntellectualRights
{
    public double Royalty { get; set; }
    public EngineDesignPatent(double royalty)
    {
        Royalty = royalty;
    }
}

public class BenzolMedicinePatent : EntityBaseClass, IIntellectualRights
{
    public double Royalty { get; set; }
    public BenzolMedicinePatent(double royalty)
    {
        Royalty = royalty;
    }
}
#endregion

#region Retails
public class BookShop : EntityBaseClass, IRetailBusiness
{
    public double GrossRevenue { get; set; }
    public BookShop(double grossRevenue)
    {
        GrossRevenue = grossRevenue;
    }
}

public class AudioCDShop : EntityBaseClass, IRetailBusiness
{
    public double GrossRevenue { get; set; }
    public AudioCDShop(double grossRevenue)
    {
        GrossRevenue = grossRevenue;
    }
}
#endregion

REFERENCES

1. Refactor my code : Avoiding casting in derived class
2. Cast to generic type in C#
3. How a Visitor implementation can handle unknown nodes
4. Open Closed Principle and Visitor pattern implementation in C#

Prospect

Reference: Refactor my code : Avoiding casting in derived class – Prospect

There's a lot of code here. Can you come up with a simpler (if contrived) example to demonstrate your problem? – Amp

Answer 1

This solution uses the notions that business interfaces know they must create a return and that their concrete implementations know what kind of concrete return to create.

Step 1 Split InvestmentReturn into two interfaces; the original minus the Business property and a new generic subclass:

public abstract class InvestmentReturn
{
    public double ProfitElement { get; set; }
    public abstract double GetInvestmentProfit();

    public double CalculateBaseProfit()
    {
        // ...
    }
}

public abstract class InvestmentReturn<T>: InvestmentReturn where T : IBusiness
{
    public T Business { get; set; }        
}

Step 2 Inherit from the generic one so you can use Business without casting:

public class RetailInvestmentReturn : InvestmentReturn<IRetailBusiness>
{
    // this won't compile; see **Variation** below for resolution to this problem...
    public RetailInvestmentReturn(IRetailBusiness retail)
    {
        Business = retail;
    }

    public override double GetInvestmentProfit()
    {
        ProfitElement = Business.GrossRevenue;
        return CalculateBaseProfit();
    }
}

Step 3 Add a method to IBusiness that returns an InvestmentReturn:

public interface IBusiness
{
    InvestmentReturn GetReturn();
}

Step 4 Introduce a generic sublcass of EntityBaseClass to provide the default implementation of the above method. If you don't do this you'll have to implement it for all the businesses. If you do do this it means all of your classes where you don't want to repeat the GetReturn() implementation must inherit from the class below, which in turn means they must inherit from EntityBaseClass.

public abstract class BusinessBaseClass<T> : EntityBaseClass, IBusiness where T : InvestmentReturn, new()
{
    public virtual InvestmentReturn GetReturn()
    {
        return new T();
    }
}

Step 5 Implement that method for each of your subclasses if necessary. Below is an example for the BookShop:

public class BookShop : BusinessBaseClass<RetailInvestment>, IRetailBusiness
{
    public double GrossRevenue { get; set; }
    public BookShop(double grossRevenue)
    {
        GrossRevenue = grossRevenue;
    }

    // commented because not inheriting from EntityBaseClass directly
    // public InvestmentReturn GetReturn()
    // {
    //     return new RetailInvestmentReturn(this);
    // }
}

Step 6 Modify your Main to add the instances of InvestmentReturn. You don't have to typecast or type-check because that's already been done earlier in a type safe way:

    static void Main(string[] args)
    {
        var allMyProfitableBusiness = new List<IBusiness>();
        // ...
        var investmentReturns = allMyProfitableBusiness.Select(bus => bus.GetReturn()).ToList();
        // ...
    }

If you don't want your concrete businesses to know anything about creating an InvestmentReturn—only knowing that they must create one when asked—then you'll probably want to modify this pattern to incorporate a factory that creates returns given input (e.g. a map between IBusiness implementations and InvestmentReturn subtypes).

Variation

All of the above works fine and will compile if you remove the investment return constructors that set the Business property. Doing this means setting Business elsewhere. That might not be desirable.

An alternative to that would be to set the Business property inside GetReturn. I found a way to do that, but it really starts to make the classes look messy. It's here for your evaluation as to whether its worth it.

Remove the non-default constructor from RetailInvestmentReturn:

public class RetailInvestmentReturn : InvestmentReturn<IRetailBusiness>
{
   public override double GetInvestmentProfit()
   {
       ProfitElement = Business.GrossRevenue;
       return CalculateBaseProfit();
   }
}

Change BusinessBaseClass. This is where it gets messy with a double-cast, but at least it's limited to one place.

public abstract class BusinessBaseClass<T, U> : EntityBaseClass, IBusiness
    where T : InvestmentReturn<U>, new()
    where U : IBusiness
{
    public double GrossRevenue { get; set; }

    public virtual InvestmentReturn GetReturn()
    {
        return new T { Business = (U)(object)this };
    }
}

Finally change your businesses. Here's an example for BookShop:

public class BookShop : BusinessBaseClass<RetailInvestmentReturn, IRetailBusiness>, IRetailBusiness
{
    // ...
}

Topless

Thanks a ton for the idea - Inherit from the generic subclass. This solution overcomes the type casting problem in a smart way. What is the name of this pattern? – Prospect

I updated my answer. Look at step 4 where I introduce a generic business base class to remove that redundancy. – Topless

@Lijo, not sure if there's a name for the pattern. I've used it before when I want to provide both default behavior and a clean way to introduce widely behaving behavior, but without all the casting. – Topless

I made the concrete business to inherit the BusinessBaseClass as below. public class BookShop : BusinessBaseClass<RetailInvestmentReturn>, IRetailBusiness. But I am getting compilation error: RetailInvestmentReturn' must be a non-abstract type with a public parameterless constructor in order to use it as parameter 'T' in the generic type or method 'BusinessBaseClass<T> – Prospect

Without injecting a factory into the mix, you could apply the edit I made. But not sure whether it's worth it to avoid newing in each business. If you have a lot of them, it might be worth it. – Topless

Answer 2

Make the concrete implementations of IBusiness visitable by visitor pattern. then create a visitor in the investment return domain that can visit each business, like this:

public interface IVisitor<T>
{
    T Visit(IIntellectualRights bus);
    T Visit(IRetailBusiness bus);
}

public interface IBusiness
{
    T Accept<T>(IVisitor<T> visitor);
}

public class AudioCDShop : EntityBaseClass, IRetailBusiness
{
     public void Accept(IVisitor visitor)
     {
          return visitor.Visit(this);
     }

//do the same for each IBusiness implementor.

Then in your investment return domain:

 public class InvestmentVisitor : IVisitor<InvestmentReturn>
 {
     public InvestmentReturn GetInvestment(IBusiness bus)
     {
          return bus.Accept(this);
     }

     public InvestmentReturn Visit(IIntellectualRights bus)
     {
          return new IntellectualRightsInvestmentReturn(bus)
     }

     public InvestmentReturn  Visit(IRetailBusiness bus)
     {
          return new RetailInvestmentReturn(bus);
     }
 }

Usage

 var investmentReturn = new InvestmentVisitor().GetInvestment(bus);

totally untested and unverified.. but the concept works. This might also be way overkill if you only have two different kinds of nodes that should be visited.

Fairchild

Answer 3

Basically, if I understood you right, you want to avoid the type casting.

You can accomplish that simply by changing the constructor signature from:

public RetailInvestmentReturn(IRetailBusiness retail) {...}
public IntellectualRightsInvestmentReturn(IIntellectualRights intellectual) {...}

to:

public RetailInvestmentReturn(IBusiness retail) {...}
public IntellectualRightsInvestmentReturn(IBusiness intellectual) {...}

If that is not an option due to design constraints, then you can try to use the strategy pattern. That would still need a type cast, but you would get rid of the terrifying "ifs" and "elses", that, I believe, is your real question, right? In order to do that you need to:

1. Create a dictionary with the "type" and the "method to run"
2. Use it!

In code, that would look something like this:

1. (Create,...)

       Dictionary<Type,Func<IBusiness,InvestmentReturn>> dict = new Dictionary<Type, Func<IBusiness, InvestmentReturn>>
       {
           {typeof (BookShop), business => new RetailInvestmentReturn((IRetailBusiness) business)},
           {typeof (AudioCDShop), business => new IntellectualRightsInvestmentReturn((IIntellectualRights) business)}
       };
   

2. (Use it!)

// try this:

foreach (IBusiness bus in allMyProfitableBusiness)
{
    investmentReturns.Add(dict[bus.GetType()](bus));
}

Maroney

Answer 4

Let the IBusiness interface handle the type of InvestmentReturn type and return the profit amount.

Changes to the foreach loop:

foreach (IBusiness bus in allMyProfitableBusiness)
{
    // No type checking or casting.  It is scalable to new business types.
    investmentReturns.Add(bus.GetReturnInvestment());
}

Updates to the interfaces:

public interface IBusiness
{
    IReturnInvestment GetReturnInvestment();

    double GetProfit();
}

public abstract class EntityBaseClass
{

}

public interface IRetailBusiness : IBusiness
{
    ...
}

public interface IIntellectualRights : IBusiness
{
    ...
}

Business classes with another base class:

#region Intellectuals
public abstract IntellectualRightsBaseClass : EntityBaseClass, IIntellectualRights
{
    ...
    public IReturnInvestment GetReturnInvestment()
    {
        return new IntellectualRightsInvestmentReturn(this);
    }

    public double GetProfit()
    {
        return this.Royalty;
    }
}

public class EngineDesignPatent : IntellectualRightsBaseClass
{
    ...

}

public class BenzolMedicinePatent : IntellectualRightsBaseClass
{
    ...
}
#endregion

#region Retails
public abstract RetailBusinessBaseClass : IRetailBusiness
{
    ...
    public IReturnInvestment GetReturnInvestment()
    {
        return new RetailInvestmentReturn(this);
    }

    public double GetProfit()
    {
        return this.GrossRevenue;
    }
}

public class BookShop : RetailBusinessBaseClass 
{
    ...
}

public class AudioCDShop : RetailBusinessBaseClass 
{
    ...
}
#endregion

Extensions

public class RetailInvestmentReturn : InvestmentReturn
{
    public RetailInvestmentReturn(IRetailBusiness retail)
        : base(retail)
    {
    }

    /* Don't need anymore
    public override  double GetInvestmentProfit()
    {
        //GrossRevenue is the ProfitElement for RetailBusiness
        ProfitElement = Business.GetProfit();
        return base.CalculateBaseProfit();
    } */
}

public class IntellectualRightsInvestmentReturn : InvestmentReturn
{

    public IntellectualRightsInvestmentReturn(IIntellectualRights intellectual)
        : base(intellectual)
    {
    }

    /* Don't need anymore
    public override double GetInvestmentProfit()
    {
        //Royalty is the ProfitElement for IntellectualRights Business
        ProfitElement = Business.GetProfit();
        return base.CalculateBaseProfit();
    } */
}

Abstract InvestmentReturn

public abstract class InvestmentReturn
{
    protected InvestmentReturn(IBusiness business)
    {
        this.Business = business;
    }

    public double ProfitElement { get; set; }
    public IBusiness Business{ get;  set; }

    public override double GetInvestmentProfit()
    {
        ProfitElement = this.Business.GetProfit();
        return this.CalculateBaseProfit();
    }

    public double CalculateBaseProfit()
    {
       double profit = 0;

       if (ProfitElement < 5)
       {
           profit = ProfitElement * 5 / 100;
       }
       else if (ProfitElement < 20)
       {
           profit = ProfitElement * 7 / 100;
       }
       else
       {
           profit = ProfitElement * 10 / 100;
       }

       return profit;
    }
}

Zandrazandt

IBusiness has no information about GetReturnInvestment() in the domain. Hence this won't work. – Prospect

Yeah, that's something that I'm suggesting you add, unless you can't change IBusiness? – Zandrazandt

Even if he can change IBusiness, he shouldn't, you shouldn't force one domain to know about another domain.. domain B depends on domain A, domain A does not know about domain B.. – Fairchild

Answer 5

What about making InvestmentReturn a concrete class and moving ProfitElement logic to IBusiness?

public class InvestmentReturn
{
    public double ProfitElement { get; set; }

    public double InvestmentProfit{ get; set; }

    public RetailInvestmentReturn(IRetailBusiness bus)
    {
        ProfitElement = bus.GetProfitElement();
    }

    public double CalculateBaseProfit()
    {
        ......
    }
}

public class BenzolMedicinePatent : EntityBaseClass, IIntellectualRights
{
    public double Royalty { get; set; }
    public BenzolMedicinePatent(double royalty)
    {
        Royalty = royalty;
    }

    public override double GetProfitElement() 
    {
        return royalty;
    }    
}



public class BookShop : EntityBaseClass, IRetailBusiness
{
    public double GrossRevenue { get; set; }
    public BookShop(double grossRevenue)
    {
        GrossRevenue = grossRevenue;
    }
    public override double GetProfitElement() 
    {
        return royalty;
    } 
}

Now the client becomes:

class Program
{

    static void Main(string[] args)
    {

        #region MyBusines

        List<IBusiness> allMyProfitableBusiness = new List<IBusiness>();

        BookShop bookShop1 = new BookShop(75);
        AudioCDShop cd1Shop = new AudioCDShop(80);
        EngineDesignPatent enginePatent = new EngineDesignPatent(1200);
        BenzolMedicinePatent medicinePatent = new BenzolMedicinePatent(1450);

        allMyProfitableBusiness.Add(bookShop1);
        allMyProfitableBusiness.Add(cd1Shop);
        allMyProfitableBusiness.Add(enginePatent);
        allMyProfitableBusiness.Add(medicinePatent);

        #endregion

        List<InvestmentReturn> investmentReturns = new List<InvestmentReturn>();

        foreach (IBusiness bus in allMyProfitableBusiness)
        {
            investmentReturns.Add(new InvestmentReturn(bus));
        }

        double totalProfit = 0;
        foreach (var profitelement in investmentReturns)
        {
            totalProfit = totalProfit + profitelement.GetInvestmentProfit();
            Console.WriteLine("Profit: {0:c}", profitelement.GetInvestmentProfit());
        }

        Console.ReadKey();
    }
}

Jokester

BenzolMedicinePatent has no information about GetProfitElement() in the domain. Hence this won't work. – Prospect

@Lijo Is Royalty the ProfitElement for IntellectualRights Business? – Jokester

Yes, Indeed. However, the fact that Royalty is the ProfitElement for IntellectualRights Business is an information that is outside of IntellectualRights Business domain. That is something the investment company decides – Prospect

@Lijo I'm sorry, I'm not familiar with the investment holding domain. Could you elaborate more? Like why "royalty is the ProfitElement outside of IntellectualRights Business domain" makes this approach unavailable? – Jokester

Answer 6

All the answers above seem more complicated than necessary, but you don't really say exactly what information you need to keep, so it's hard to know how far one can take the re-design of your classes. If all you wanted to do was to produce the results that the above program produces and still use objects, you could do this:

class Program2
{
    static void Main(string[] args)
    {
        List<Business> allMyProfitableBusiness = new List<Business>();

        Business bookShop1 = new Business(75);
        Business cd1Shop = new Business(80);
        Business enginePatent = new Business(1200);
        Business medicinePatent = new Business(1450);

        allMyProfitableBusiness.Add(bookShop1);
        allMyProfitableBusiness.Add(cd1Shop);
        allMyProfitableBusiness.Add(enginePatent);
        allMyProfitableBusiness.Add(medicinePatent);

        foreach (var business in allMyProfitableBusiness)
        {
            Console.WriteLine("Profit: {0:c}", business.GetInvestmentProfit());
        }

        Console.ReadKey();
    }
}


public class Business
{
    private double ProfitElement;

    public Business(double profitElement)
    {
        ProfitElement = profitElement;
    }

    public double GetInvestmentProfit()
    {
        double profit = 0;

        if (ProfitElement < 5)
        {
            profit = ProfitElement * 5 / 100;
        }
        else if (ProfitElement < 20)
        {
            profit = ProfitElement * 7 / 100;
        }
        else
        {
            profit = ProfitElement * 10 / 100;
        }

        return profit;
    }
}

If, for some reason, you do need to keep the different types of Businesses (Retail and Intellectual), you could still greatly simplify it using base classes. You don't need generics or your InvestmentReturn or EntityBaseClass (and although you calculate totalProfit, it is not used anywhere):

class Program
{
    static void Main(string[] args)
    {
        List<Business> allMyProfitableBusiness = new List<Business>();

        BookShop bookShop1 = new BookShop(75);
        AudioCDShop cd1Shop = new AudioCDShop(80);
        EngineDesignPatent enginePatent = new EngineDesignPatent(1200);
        BenzolMedicinePatent medicinePatent = new BenzolMedicinePatent(1450);

        allMyProfitableBusiness.Add(bookShop1);
        allMyProfitableBusiness.Add(cd1Shop);
        allMyProfitableBusiness.Add(enginePatent);
        allMyProfitableBusiness.Add(medicinePatent);

        double totalProfit = 0;
        foreach (var business in allMyProfitableBusiness)
        {
            totalProfit = totalProfit + business.GetInvestmentProfit();
            Console.WriteLine("Profit: {0:c}", business.GetInvestmentProfit());
        }

        Console.ReadKey();
    }
}


public abstract class Business
{
    public abstract double Profit { get; }

    public double GetInvestmentProfit()
    {
        double profit = 0;

        if (Profit < 5)
        {
            profit = Profit * 5 / 100;
        }
        else if (Profit < 20)
        {
            profit = Profit * 7 / 100;
        }
        else
        {
            profit = Profit * 10 / 100;
        }

        return profit;
    }
}


public abstract class RetailBusiness : Business
{
    public double GrossRevenue { get; set; }
    public override double Profit {get {return GrossRevenue; } }
}

public abstract class IntellectualRights : Business
{
    public double Royalty { get; set; }
    public override double Profit {get {return Royalty; } }
}


#region Intellectuals
public class EngineDesignPatent : IntellectualRights
{
    public EngineDesignPatent(double royalty)
    {
        Royalty = royalty;
    }
}

public class BenzolMedicinePatent : IntellectualRights
{
    public BenzolMedicinePatent(double royalty)
    {
        Royalty = royalty;
    }
}
#endregion


#region Retails
public class BookShop : RetailBusiness
{
    public BookShop(double grossRevenue)
    {
        GrossRevenue = grossRevenue;
    }
}

public class AudioCDShop : RetailBusiness
{
    public AudioCDShop(double grossRevenue)
    {
        GrossRevenue = grossRevenue;
    }
}
#endregion

Of course, the chances that anyone will read this are very, very small :-)

Ostiary