Replace conditional with polymorphism refactoring or similar?

Question

I have tried to ask a variant of this question before. I got some helpful answers, but still nothing that felt quite right to me. It seems to me this shouldn't really be that hard a nut to crack, but I'm not able to find an elegant simple solution. (Here's my previous post, but please try to look at the problem stated here as procedural code first so as not to be influenced by the earlier explanation which seemed to lead to very complicated solutions: http://stackoverflow.com/questions/2772858/design-pattern-for-cost-calculator-app )

Basically, the problem is to create a calculator for hours needed for projects that can contain a number of services. In this case "writing" and "analysis". The hours are calculated differently for the different services: writing is calculated by multiplying a "per product" hour rate with the number of products, and the more products are included in the project, the lower the hour rate is, but the total number of hours is accumulated progressively (i.e. for a medium-sized project you take both the small range pricing and then add the medium range pricing up to the number of actual products). Whereas for analysis it's much simpler, it is just a bulk rate for each size range.

How would you be able to refactor this into an elegant and preferably simple object-oriented version (please note that I would never write it like this in a purely procedural manner, this is just to show the problem in another way succinctly).

I have been thinking in terms of factory, strategy and decorator patterns, but can't get any to work well. (I read Head First Design Patterns a while back, and both the decorator and factory patterns described have some similarities to this problem, but I have trouble seeing them as good solutions as stated there. The decorator example seemed very complicated there for just adding condiments, but maybe it could work better here, I don't know. At least the fact that the calculation of hours accumulates progressively made me think of the decorator pattern... And the factory pattern example from the book with the pizza factory...well it just seems to create such a ridiculous explosion of classes, at least in their example. I have found good use for factory patterns before, but I can't see how I could use it here without getting a really complicated set of classes)

The main goal would be to only have to change in one place (loose coupling etc) if I were to add a new parameter (say another size, like XSMALL, and/or another service, like "Administration"). Here's the procedural code example:

public class Conditional
{
    private int _numberOfManuals;
    private string _serviceType;
    private const int SMALL = 2;
    private const int MEDIUM = 8;

    public int GetHours()
    {
        if (_numberOfManuals <= SMALL)
        {
            if (_serviceType == "writing")
                return 30 * _numberOfManuals;
            if (_serviceType == "analysis")
                return 10;
        }
        else if (_numberOfManuals <= MEDIUM)
        {
            if (_serviceType == "writing")
                return (SMALL * 30) + (20 * _numberOfManuals - SMALL);
            if (_serviceType == "analysis")
                return 20;
        }
        else //i.e. LARGE
        {
            if (_serviceType == "writing")
                return (SMALL * 30) + (20 * (MEDIUM - SMALL)) + (10 * _numberOfManuals - MEDIUM);
            if (_serviceType == "analysis")
                return 30;
        }
        return 0; //Just a default fallback for this contrived example
    }
}

All replies are appreciated! (But as I stated in my previous posts I would appreciate actual code examples rather than just "Try this pattern", because as I mentioned, that is what I'm having trouble with...) I hope someone has a really elegant solution to this problem that I actually thought from the beginning would be really simple...

========================================================

NEW ADDITION:

I appreciate all the answers so far, but I'm still not seeing a really simple and flexible solution to the problem (one I thought wouldn't be very complex at first, but apparently is). It may also be that I haven't quite understood each answer correctly yet. But I thought I'd post my current attempt at working it out (with some help from reading all the different angles in answers here). Please tell me if I'm on the right track or not. But at least now it feels like it's starting to get more flexible... I can quite easily add new parameters without having to change in lots of places (I think!), and the conditional logic is all in one place. I have some of it in xml to get the basic data, which simplifies part of the problem, and part of it is an attempt at a strategy type solution.

Here's the code:

 public class Service
{
    protected HourCalculatingStrategy _calculatingStrategy;
    public int NumberOfProducts { get; set; }
    public const int SMALL = 3;
    public const int MEDIUM = 9;
    public const int LARGE = 20;
    protected string _serviceType;
    protected Dictionary<string, decimal> _reuseLevels;

    protected Service(int numberOfProducts)
    {
        NumberOfProducts = numberOfProducts;
    }

    public virtual decimal GetHours()
    {
        decimal hours = _calculatingStrategy.GetHours(NumberOfProducts, _serviceType);
        return hours;
    }
}

public class WritingService : Service
{
    public WritingService(int numberOfProducts)
        : base(numberOfProducts)
    {
        _calculatingStrategy = new VariableCalculatingStrategy();
        _serviceType = "writing";
    }
}

class AnalysisService : Service
{
    public AnalysisService(int numberOfProducts)
        : base(numberOfProducts)
    {
        _calculatingStrategy = new FixedCalculatingStrategy();
        _serviceType = "analysis";
    }
}

public abstract class HourCalculatingStrategy
{
    public abstract int GetHours(int numberOfProducts, string serviceType);

    protected int GetHourRate(string serviceType, Size size)
    {
        XmlDocument doc = new XmlDocument();
        doc.Load("calculatorData.xml");
        string result = doc.SelectSingleNode(string.Format("//*[@type='{0}']/{1}", serviceType, size)).InnerText;
        return int.Parse(result);
    }
    protected Size GetSize(int index)
    {
        if (index < Service.SMALL)
            return Size.small;
        if (index < Service.MEDIUM)
            return Size.medium;
        if (index < Service.LARGE)
            return Size.large;
        return Size.xlarge;
    }
}

public class VariableCalculatingStrategy : HourCalculatingStrategy
{
    public override int GetHours(int numberOfProducts, string serviceType)
    {
        int hours = 0;
        for (int i = 0; i < numberOfProducts; i++)
        {
            hours += GetHourRate(serviceType, GetSize(i + 1));
        }
        return hours;
    }
}

public class FixedCalculatingStrategy : HourCalculatingStrategy
{
    public override int GetHours(int numberOfProducts, string serviceType)
    {
        return GetHourRate(serviceType, GetSize(numberOfProducts));
    }
}

And a simple example form that calls it (I guess I could also have a wrapper Project class with a Dictionary containing the Service objects, but I haven't gotten to that):

    public partial class Form1 : Form
{
    public Form1()
    {
        InitializeComponent();
        List<int> quantities = new List<int>();

        for (int i = 0; i < 100; i++)
        {
            quantities.Add(i);
        }
        comboBoxNumberOfProducts.DataSource = quantities;
    }


    private void CreateProject()
    {
        int numberOfProducts = (int)comboBoxNumberOfProducts.SelectedItem;
        Service writing = new WritingService(numberOfProducts);
        Service analysis = new AnalysisService(numberOfProducts);

        labelWriterHours.Text = writing.GetHours().ToString();
        labelAnalysisHours.Text = analysis.GetHours().ToString();
    }
    private void comboBoxNumberOfProducts_SelectedIndexChanged(object sender, EventArgs e)
    {
        CreateProject();
    }

}

(I wasn't able to include the xml because it got automatically formatted on this page, but it's basically just a bunch of elements with each service type, and each service type containing the sizes with the hour rates as values.)

I'm not sure if I'm just pushing the problem over to the xml file (I'd still have to add new elements for each new servicetype, and add elements for any new size in each servicetype if that changed.) But maybe it's impossible to achieve what I am trying to do and not having to do at least that type of change. Using a database rather than xml the change would be as simple as adding a field and a row:

ServiceType Small Medium Large

Writing 125 100 60

Analysis 56 104 200

(Simply formatted as a "table" here, although the columns aren't quite aligned... I'm not the best at database design though, and maybe it should be done differently, but you get the idea...)

Please tell me what you think!

Answer 1

I would tend to start with an enumeration ProjectSize {Small, Medium, Large} and a simple function to return the appropriate enum given a numberOfManuals. From there, I would write different ServiceHourCalculators, the WritingServiceHourCalculator and the AnalysisServiceHourCalculator (because their logic is sufficiently different). Each would take a numberOfManuals, a ProjectSize, and return the number of hours. I'd probably create a map from string to ServiceHourCalculator, so I could say:

ProjectSize projectSize = getProjectSize(_numberOfManuals);
int hours = serviceMap.getService(_serviceType).getHours(projectSize, _numberOfManuals);

This way, when I added a new project size, the compiler would balk at some unhandled cases for each service. It's not all handled in one place, but it is all handled before it will compile again, and that's all I need.

Update I know Java, not C# (very well), so this may not be 100% right, but creating the map would be something like this:

Map<String, ServiceHourCalculator> serviceMap = new HashMap<String, ServiceHourCalculator>();
serviceMap.put("writing", new WritingServiceHourCalculator());
serviceMap.put("analysis", new AnalysisServiceHourCalculator());

Answer 2

A good start would be to extract the conditional statement into a method(although only a small method) and give it a really explicit name. Then extract the logic within the if statement into their own methods - again with really explicit names. (Don't worry if the method names are long - as long as they do what they're called)

I would write this out in code but it would be better for you to pick names.

I would then move onto more complicated refactoring methods and patterns. Its only when your looking at a series of method calls will it seem appropriate to start applying patterns etc..

Make your first goal to write clean, easy to read and comprehend code. It is easy to get excited about patterns (speaking from experience) but they are very hard to apply if you can't describe your existing code in abstractions.

EDIT: So to clarify - you should aim to get your if statement looking like this

if( isBox() )
{
    doBoxAction();
}
else if( isSquirrel() )
{
    doSquirrelAction();
}

Once you do this, in my opinion, then it is easier to apply some of the patterns mentioned here. But once you still have calculatios etc... in your if statement, then it is harder to see the wood from the trees as you are at too low of an abstraction.

Answer 3

Hi there, this is a common problem, there are a few options that i can think of. There are two design pattern that come to mind, firstly the Strategy Pattern and secondly the Factory Pattern. With the strategy pattern it is possible to encapsulate the calculation into an object, for example you could encapsulate your GetHours method into individual classes, each one would represent a calculation based on size. Once we have defined the different calculation strategies we wrap then in a factory. The factory would be responsible for selecting the strategy to perform the calculation just like your if statement in the GetHours method. Any way have a look at the code below and see what you think

At any point you could create a new strategy to perform a different calculation. The strategy can be shared between different objects allowing the same calculation to be used in multiple places. Also the factory could dynamically work out which strategy to use based on configuration, for example

class Program
{
    static void Main(string[] args)
    {
        var factory = new HourCalculationStrategyFactory();
        var strategy = factory.CreateStrategy(1, "writing");

        Console.WriteLine(strategy.Calculate());
    }
}

public class HourCalculationStrategy
{
    public const int Small = 2;
    public const int Medium = 8;

    private readonly string _serviceType;
    private readonly int _numberOfManuals;

    public HourCalculationStrategy(int numberOfManuals, string serviceType)
    {
        _serviceType = serviceType;
        _numberOfManuals = numberOfManuals;
    }

    public int Calculate()
    {
        return this.CalculateImplementation(_numberOfManuals, _serviceType);
    }

    protected virtual int CalculateImplementation(int numberOfManuals, string serviceType)
    {
        if (serviceType == "writing")
            return (Small * 30) + (20 * (Medium - Small)) + (10 * numberOfManuals - Medium);
        if (serviceType == "analysis")
            return 30;

        return 0;
    }
}

public class SmallHourCalculationStrategy : HourCalculationStrategy
{
    public SmallHourCalculationStrategy(int numberOfManuals, string serviceType) : base(numberOfManuals, serviceType)
    {
    }

    protected override int CalculateImplementation(int numberOfManuals, string serviceType)
    {
        if (serviceType == "writing")
            return 30 * numberOfManuals;
        if (serviceType == "analysis")
            return 10;

        return 0;
    }
}

public class MediumHourCalculationStrategy : HourCalculationStrategy
{
    public MediumHourCalculationStrategy(int numberOfManuals, string serviceType) : base(numberOfManuals, serviceType)
    {
    }

    protected override int CalculateImplementation(int numberOfManuals, string serviceType)
    {
        if (serviceType == "writing")
            return (Small * 30) + (20 * numberOfManuals - Small);
        if (serviceType == "analysis")
            return 20;

        return 0;
    }
}

public class HourCalculationStrategyFactory
{
    public HourCalculationStrategy CreateStrategy(int numberOfManuals, string serviceType)
    {
        if (numberOfManuals <= HourCalculationStrategy.Small)
        {
            return new SmallHourCalculationStrategy(numberOfManuals, serviceType);
        }

        if (numberOfManuals <= HourCalculationStrategy.Medium)
        {
            return new MediumHourCalculationStrategy(numberOfManuals, serviceType);
        }

        return new HourCalculationStrategy(numberOfManuals, serviceType);
    }
}

Answer 4

I would go with a strategy pattern derivative. This adds additional classes, but is more maintainable over the long haul. Also, keep in mind that there are still opporunities for refactoring here:

public class Conditional
{
    private int _numberOfManuals;
    private string _serviceType;
    public const int SMALL = 2;
    public const int MEDIUM = 8;
    public int NumberOfManuals { get { return _numberOfManuals; } }
    public string ServiceType { get { return _serviceType; } }
    private Dictionary<int, IResult> resultStrategy;

    public Conditional(int numberOfManuals, string serviceType)
    {
        _numberOfManuals = numberOfManuals;
        _serviceType = serviceType;
        resultStrategy = new Dictionary<int, IResult>
        {
              { SMALL, new SmallResult() },
              { MEDIUM, new MediumResult() },
              { MEDIUM + 1, new LargeResult() }
        };
    }

    public int GetHours()
    {
        return resultStrategy.Where(k => _numberOfManuals <= k.Key).First().Value.GetResult(this);
    }
}

public interface IResult
{
    int GetResult(Conditional conditional);
}

public class SmallResult : IResult
{
    public int GetResult(Conditional conditional)
    {
        return conditional.ServiceType.IsWriting() ? WritingResult(conditional) : AnalysisResult(conditional); ;
    }

    private int WritingResult(Conditional conditional)
    {
        return 30 * conditional.NumberOfManuals;
    }

    private int AnalysisResult(Conditional conditional)
    {
        return 10;
    }
}

public class MediumResult : IResult
{
    public int GetResult(Conditional conditional)
    {
        return conditional.ServiceType.IsWriting() ? WritingResult(conditional) : AnalysisResult(conditional); ;
    }

    private int WritingResult(Conditional conditional)
    {
        return (Conditional.SMALL * 30) + (20 * conditional.NumberOfManuals - Conditional.SMALL);

    }

    private int AnalysisResult(Conditional conditional)
    {
        return 20;
    }
}

public class LargeResult : IResult
{
    public int GetResult(Conditional conditional)
    {
        return conditional.ServiceType.IsWriting() ? WritingResult(conditional) : AnalysisResult(conditional); ;
    }

    private int WritingResult(Conditional conditional)
    {
        return (Conditional.SMALL * 30) + (20 * (Conditional.MEDIUM - Conditional.SMALL)) + (10 * conditional.NumberOfManuals - Conditional.MEDIUM);

    }

    private int AnalysisResult(Conditional conditional)
    {
        return 30;
    }
}

public static class ExtensionMethods
{
    public static bool IsWriting(this string value)
    {
        return value == "writing";
    }
}

Answer 5

You don't need the Factory if your subclasses filter themselves on what they want to charge for. That requires a Project class to hold the data, if nothing else:

class Project {
    TaskType Type { get; set; }
    int? NumberOfHours { get; set; }
}

Since you want to add new calculations easily, you need an interface:

IProjectHours {
    public void SetHours(IEnumerable<Project> projects);
}

And, some classes to implement the interface:

class AnalysisProjectHours : IProjectHours {
    public void SetHours(IEnumerable<Project> projects) {
       projects.Where(p => p.Type == TaskType.Analysis)
               .Each(p => p.NumberOfHours += 30);
    }
}

// Non-LINQ equivalent
class AnalysisProjectHours : IProjectHours {
    public void SetHours(IEnumerable<Project> projects) {
       foreach (Project p in projects) {
          if (p.Type == TaskType.Analysis) {
             p.NumberOfHours += 30;
          }
       }
    }
}

class WritingProjectHours : IProjectHours {
    public void SetHours(IEnumerable<Project> projects) {
       projects.Where(p => p.Type == TaskType.Writing)
               .Skip(0).Take(2).Each(p => p.NumberOfHours += 30);
       projects.Where(p => p.Type == TaskType.Writing)
               .Skip(2).Take(6).Each(p => p.NumberOfHours += 20);
       projects.Where(p => p.Type == TaskType.Writing)
               .Skip(8).Each(p => p.NumberOfHours += 10);
    }
}

// Non-LINQ equivalent
class WritingProjectHours : IProjectHours {
    public void SetHours(IEnumerable<Project> projects) {
       int writingProjectsCount = 0;
       foreach (Project p in projects) {
          if (p.Type != TaskType.Writing) {
             continue;
          }
          writingProjectsCount++;
          switch (writingProjectsCount) {
              case 1: case 2:
                p.NumberOfHours += 30;
                break;
              case 3: case 4: case 5: case 6: case 7: case 8:
                p.NumberOfHours += 20;
                break;
              default:
                p.NumberOfHours += 10;
                break;
          }
       }
    }
}

class NewProjectHours : IProjectHours {
    public void SetHours(IEnumerable<Project> projects) {
       projects.Where(p => p.Id == null).Each(p => p.NumberOfHours += 5);
    }
}

// Non-LINQ equivalent
class NewProjectHours : IProjectHours {
    public void SetHours(IEnumerable<Project> projects) {
       foreach (Project p in projects) {
          if (p.Id == null) {
            // Add 5 additional hours to each new project
            p.NumberOfHours += 5; 
          }
       }
    }
}    

The calling code can either dynamically load IProjectHours implementors (or static them) and then just walk the list of Projects through them:

foreach (var h in AssemblyHelper.GetImplementors<IProjectHours>()) {
   h.SetHours(projects);
}
Console.WriteLine(projects.Sum(p => p.NumberOfHours));
// Non-LINQ equivalent
int totalNumberHours = 0;
foreach (Project p in projects) {
   totalNumberOfHours += p.NumberOfHours;
}
Console.WriteLine(totalNumberOfHours);