views:

125

answers:

4

Hi,

I'm trying to figure out how to design a small application more elegantly, and make it more resistant to change.

Basically it is a sort of project price calculator, and the problem is that there are many parameters that can affect the pricing. I'm trying to avoid cluttering the code with a lot of if-clauses for each parameter, but still I have e.g. if-clauses in two places checking for the value of the size parameter.

I have the Head First Design Patterns book, and have tried to find ideas there, but the closest I got was the decorator pattern, which has an example where starbuzz coffee sets prices depending first on condiments added, and then later in an exercise by adding a size parameter (Tall, Grande, Venti). But that didn't seem to help, because adding that parameter still seemed to add if-clause complexity in a lot of places (and this being an exercise they didn't explain that further).

What I am trying to avoid is having to change several classes if a parameter were to change or a new parameter added, or at least change in as few places as possible (there's some fancy design principle word for this that I don't rememeber :-)).

Here below is the code. Basically it calculates the price for a project that has the tasks "Writing" and "Analysis" with a size parameter and different pricing models. There will be other parameters coming in later too, like "How new is the product?" (New, 1-5 years old, 6-10 years old), etc. Any advice on the best design would be greatly appreciated, whether a "design pattern" or just good object oriented principles that would make it resistant to change (e.g. adding another size, or changing one of the size values, and only have to change in one place rather than in several if-clauses):

public class Project
{
    private readonly int _numberOfProducts;
    protected Size _size;
    public Task Analysis { get; set; }
    public Task Writing { get; set; }

    public Project(int numberOfProducts)
    {
        _numberOfProducts = numberOfProducts;
        _size = GetSize();
        Analysis = new AnalysisTask(numberOfProducts, _size);
        Writing = new WritingTask(numberOfProducts, _size);

    }

    private Size GetSize()
    {
        if (_numberOfProducts <= 2)
            return Size.small;
        if (_numberOfProducts <= 8)
            return Size.medium;
        return Size.large;
    }
    public double GetPrice()
    {
        return Analysis.GetPrice() + Writing.GetPrice();
    }
}

public abstract class Task
{
    protected readonly int _numberOfProducts;
    protected Size _size;
    protected double _pricePerHour;
    protected Dictionary<Size, int> _hours;
    public abstract int TotalHours { get; }

    public double Price { get; set; }

    protected Task(int numberOfProducts, Size size)
    {
        _numberOfProducts = numberOfProducts;
        _size = size;
    }

    public double GetPrice()
    {
        return _pricePerHour * TotalHours;
    }
}

public class AnalysisTask : Task
{
    public AnalysisTask(int numberOfProducts, Size size)
        : base(numberOfProducts, size)
    {
        _pricePerHour = 850;
        _hours = new Dictionary<Size, int>() { { Size.small, 56 }, { Size.medium, 104 }, { Size.large, 200 } };
    }

    public override int TotalHours
    {
        get { return _hours[_size]; }
    }
}

public class WritingTask : Task
{
    public WritingTask(int numberOfProducts, Size size)
        : base(numberOfProducts, size)
    {
        _pricePerHour = 650;
        _hours = new Dictionary<Size, int>() { { Size.small, 125 }, { Size.medium, 100 }, { Size.large, 60 } };
    }

    public override int TotalHours
    {
        get
        {
            if (_size == Size.small)
                return _hours[_size] * _numberOfProducts;
            if (_size == Size.medium)
                return (_hours[Size.small] * 2) + (_hours[Size.medium] * (_numberOfProducts - 2));
            return (_hours[Size.small] * 2) + (_hours[Size.medium] * (8 - 2)) + (_hours[Size.large] * (_numberOfProducts - 8));
        }
    }
}

public enum Size
{
    small, medium, large
}

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 comboBoxNumberOfProducts_SelectedIndexChanged(object sender, EventArgs e)
    {
        Project project = new Project((int)comboBoxNumberOfProducts.SelectedItem);
        labelPrice.Text = project.GetPrice().ToString();
        labelWriterHours.Text = project.Writing.TotalHours.ToString();
        labelAnalysisHours.Text = project.Analysis.TotalHours.ToString();
    }
}

At the end is a simple current calling code in the change event for a combobox that set size... (BTW, I don't like the fact that I have to use several dots to get to the TotalHours at the end here either, as far as I can recall, that violates the "principle of least knowledge" or "the law of demeter", so input on that would be appreciated too, but it's not the main point of the question)

Regards,

Anders

+3  A: 

First of all, you should in my opinion rethink you design. Projects don't look like that and as far as I looked in your code, you have no way to add more tasks to a project. Also consider separating the Project and the way in whch you will calculate the prize. What if you have different calculation methods? It is also about responsibility, soon you project might grow and it will be hard to separate the way you calculate the price and the project structure. Generally avoiding "if" is done using polymorphism - maybe you sould like to have different project types depending on their parameters. This can be achieved using the Factory Method, which will take the arguments, do the "if"s once and than create some Project subtype, which will know how to calculate its prize correctly. If you separate the project and the calculation, than consider instead using strategy pattern for calculating the prize. The concern about law of demeter is here adequate, because you expose the tasks. Try instead using a method which will return the total price and will delegate. The reason is, that class where will that method be (project or the calculating strategy) could decide how to calculate it, it could take also information from other tasks. You would have to adjust the method if you plan to add more tasks, maybe use one method with string or enum parameter to select a concrete task to calculate prize. BTW. why do you use underscores so much?

Gabriel Ščerbák
Ok, thanks, I'll try some of this. Could you specify what you mean by "Projects don't look like that"? Of course this is for a special type of project where tasks usually consist of writing and analysis jobs (although it should be extendable, but at the moment they consist of those jobs).I've considered the factory method, but wouldn't I have to have a slew of classes? Or what classes do you propose? SmallProject, MediumProject, LargeProject? What about Analysis and Writing, should they change? Good suggestions, but I would love some concrete examples of your ideas if possible. Thanks!
Anders Svensson
I ment, as is mentioned in other answer, that projects usually have different structure, but as you mention, if it is sufficient as is no worry...:) Many classes are not a problem, actually it is often a good design to have many small classes, although many languages look like as if it was wrong, because they create obstacles for creating a new class. As for examples, I simply do not have enough time, sorry.
Gabriel Ščerbák
A: 

So the application that you have designed has what I would say is one major gap in the design principal:

It assumes a single usage dataset.

By this I mean that it assumes, there are only two possible tasks, each has a hard coded price (something that in the business world simply doesn't exist), and each task calculates is "hours" deterministically against against a constant set of sizes. My advise is to make almost all of this configurable either by use of a database to store new possible tasks/properties/prices/hourly ratios/sizes or by some other storage means and write a configuration form for managing it.

This will almost immediately remove the design problem you are implying by the fact that you remove all hard coded domain contexts and instead set a configuration precedent that can then be exposed via API if someone doesn't like your configuration method or wants to use this in some other way.

Edit: I wanted to comment this below but ran out of room:

extend the depth of your xml to meaningfully represent the larger datastructures (WritingTask and AnalysisTask) as well as their component parts (properties and methods). The methods can often be defined by a set of rules. You can tokenize properties and rules so they can be interacted with independently. Example:

<task name="WritingTask">
<property name="numberofproducts" type="int"/>
<property name="Size" type="size">
    <property name="Price" type="decimal">
    <param name="priceperhour" value="650">
    </property>
<property name="hours" type="Dictionary">
    <param name="Size.small" value="125"/> 
    <param name="Size.medium" value="100"/>     
        <param name="Size.large" value="60"/>   
    </property>     
    <method name="TotalHours">
        <rule condition="_size == Size.Small">
    <return value="_hours[_size] * _numberofproducts"/>
        </rule>
        <rule condition="_size == Size.medium">
        <return value="(_hours[Size.small] * 2) + (_hours[Size.medium] * _numberOfProducts - 2))"/>
    </rule>
    <return value="(_hours[Size.small] * 2) + (_hours[Size.medium] * (8 - 2)) + (_hours[Size.large] * (_numberOfProducts - 8))"/> 
    </method> 
</task>

Anyway it is entirely too late in the morning for me to try this any further but I will follow up tomorrow with you. By setting resultant properties and associating methods in configuration with rules, you leave the reconciliation of the if's to your dataset (it should know best) and tune your code to interpret this data accurately. It stays flexible enough to handle growth (through a sudo language for dataset creation) but remains unchanged internally without great need to improve a more universal operation.

Gabriel
Right, yes I forgot to mention that I hardcoded these values only to make it simple to post the code here. The values will in fact be in an xml file. But how does that help with e.g. the if-clauses in two places?
Anders Svensson
I'll be honest and say that I really don't understand this strategy at all... It doesn't look like any kind of technique I've ever used, and even if it were really good it would be too unfamiliar to me to get a grip on. Thanks anyway.
Anders Svensson
It basically just the idea of taking your object base type, and making a factory out of it. So you can easily extend it (inheritance), check against dynamic properties etc. I only mentioned it because it was something I had to do when working on a project. If you can come up with a really complete and generic way to describe the potential objects in your domain then you can just add them to the structure at will. I am probably not explaining it very well. Sorry.
Gabriel
A: 

He is using underscores for member varibles. you could use "me." or "this." instead, but its just as clear. I believe this comes from some old java style standard? personally I quite like it.

Mr Shoubs
I'd say that the color coding found in all modern IDEs makes _ and similar techniques obsolete :)
willcodejavaforfood
...I saw it back in C++, but even there I prefered "its" prefix, because underscores were often used for some internal stuff... and they look super ugly at the start of a word or after the dot.
Gabriel Ščerbák
That's right, it's to indicate member variables as opposed to local variables. It's just the standard I learned when I first started programming in c#.
Anders Svensson
When you have a property and want to call it Name, you would then have a varible called name, which you would access though Me.name so your property and varible are now the same, which you can't do (in Vb.Net), so if you used _name you'd get around this. Me._name looks a bit weird, but the _ removes the need for Me. so its clearer.
Mr Shoubs
+1  A: 

If you have such kind of if...else statement based on the properties of class, try to refacor it using strategy pattern. You can try the book called "Refactor to Patterns", which is a good book on refatoring.

sza