Refactoring class design to convey the design intention

Question

I have following class design. The complete code is available in " How to achieve this functionality using Generics? ". The code works fine and resolves the casting issue mentioned in " Refactoring Code to avoid Type Casting "

In the RetailInvestmentReturnCalculator class, the GetInvestmentProfit() method utilizes CalculateBaseProfit() method present in InvestmentReturnCalculator abstract base class. This fact is not evident from the class design.

QUESTION

How to refactor this class design to convey the above mentioned fact?
What is the design guideline that will prevent this sort of design mistakes?

Note: Martin Fowler: Is Design Dead? says

What do we mean by a software architecture? To me the term architecture conveys a notion of the core elements of the system, the pieces that are difficult to change. A foundation on which the rest must be built

Class Diagram

enter image description here

Abstract

public abstract class InvestmentReturnCalculator
{
    #region Public

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

    #endregion

    #region Protected

    protected  double CalculateBaseProfit()
    {
        double profit = 0;
        if (ProfitElement < 5)
        {
            profit = ProfitElement * 5 / 100;
        }
        else
        {
            profit = ProfitElement * 10 / 100;
        }
        return profit;
    }

    #endregion
}

public abstract class InvestmentReturnCalculator<T> : InvestmentReturnCalculator where T : IBusiness
{
    public T BusinessType { get; set; }
}

Concrete

public class RetailInvestmentReturnCalculator : InvestmentReturnCalculator<IRetailBusiness>
{
    public RetailInvestmentReturnCalculator(IRetailBusiness retail)
    {
        BusinessType = retail;
        //Business = new BookShop(100);
    }

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

How is it not evident from the class design? Making the access modifier `protected`, instead of `private`, makes that evident. More importantly, perhaps, this method `GetInvestmentProfit` is not well-named, in that it does not describe the TWO separate things that it does... Perhaps it should be renamed? how about `SetProfitElementAndCalculateProfit()` ? — Charles Bretana, Feb 05 '14 at 14:13
@CharlesBretana Thanks for the `private` idea. Isn't `the TWO separate things` in a method something to be refactored? — LCJ, Feb 05 '14 at 14:15
Generally methods, classes, etc. should do ONE thing, and do it well. This is called the Single Responsibility Principle (SRP). And it is a very good general principle to follow. However, as you move up and down in the stack, what "ONE THING" actually means can change as the level of abstraction changes. AT a high Level this might be "Order a book". Several layers down, it might mean adjust inventory, Move book to shipping, Send invoice, etc. But at each level, a single method should be responsible for completing a single task at that level of abstraction... — Charles Bretana, Feb 05 '14 at 14:24

Grzenio · Accepted Answer · 2014-02-05T14:31:13.567

The ProfitElement field is rather ugly. I would make it an abstract property on InvestmentReturnCalculator and implement it in base classes (rather than setting the value) - this is called the template method pattern. Then you don't need the GetInvestmentProfit() method.

public abstract class InvestmentReturnCalculator
{
    #region Public

    public abstract double ProfitElement { get; }

    #endregion

    #region Protected

    public double GetInvestmentProfit()
    {
        double profit = 0;
        if (ProfitElement < 5)
        {
            profit = ProfitElement * 5 / 100;
        }
        else
        {
            profit = ProfitElement * 10 / 100;
        }
        return profit;
    }

    #endregion
}

public abstract class InvestmentReturnCalculator<T> : InvestmentReturnCalculator where T : IBusiness
{
    public T BusinessType { get; set; }
}

public class RetailInvestmentReturnCalculator : InvestmentReturnCalculator<IRetailBusiness>
{
    public RetailInvestmentReturnCalculator(IRetailBusiness retail)
    {
        BusinessType = retail;
        //Business = new BookShop(100);
    }

    public override double ProfitElement {get { return BusinessType.GrossRevenue;}}

}

Nice solution. However is it Template `Method` pattern? We are overriding `property` in the derived class (which eventually be called by base class); not `method` isn't it? — LCJ, Feb 05 '14 at 14:21
Also, `override` keyword is missing for ProfitElement in the derived class. And `GetInvestmentProfit()` should be made `public` — LCJ, Feb 05 '14 at 14:26
@Lijo, thanks for the comments, fixed the bugs. Regarding your first comment, we are overriding a getter, which is a method. A property is just a syntactic sugar around the pair of methods: getter + setter. — Grzenio, Feb 05 '14 at 14:33

Refactoring class design to convey the design intention

1 Answers1

Linked