Getter, Function or just set it on constructor?

Question

I can't find a good performant and legibility way to write some computed inside a class. Imagine the following class and all ways to get the FinalPrice:

public class Order {
    public Order(Product[] products) {
        Items = products;

option 1: Having a variable declaration for each property that want to be computed, horrible legibility

        var orderPrice = products.Sum(p => p.Price * p.Quantity);
        var orderTaxes = products.Sum(p => p.Taxes * p.Quantity);
        var orderDiscount = products.Sum(p => p.Price * p.Quantity * p.Discount);

        OrderPrice = orderPrice;
        OrderTaxes = orderTaxes;
        OrderDiscount = orderDiscount;

        FinalPrice = orderPrice + orderTaxes - orderDiscount;

option 2: having the problem of the order in the class matters! FinalPrice line can't be before the others or it won't work but won't throw error.

        OrderPrice = products.Sum(p => p.Price * p.Quantity);
        OrderTaxes = products.Sum(p => p.Taxes * p.Quantity);
        OrderDiscount = products.Sum(p=> p.Price * p.Quantity * p.Discount);

        FinalPrice = OrderPrice + OrderTaxes - OrderDiscount;

option 3: rewriting all the formulas - Bad for manteinance. Most likely to instroduce differences in prices later on.

        FinalPrice = products.Sum(p => p.Price * p.Quantity) + 
                     products.Sum(p => p.Taxes * p.Quantity) - 
                     products.Sum(p => p.Price * p.Quantity * p.Discount);

    }

option 4: using getters. This will be calculated everytime it's called. This is a simple calculation, but assume something more code heavily.

    public decimal FinalPrice { get {
        return OrderPrice + OrderTaxes - OrderDiscount;
    } }
}

option 5: using a function. Is this a good or bad thing ??

    public decimal CalculateFinalPrice() {
        return OrderPrice + OrderTaxes - OrderDiscount;
    }

score 4 · Answer 1 · answered Aug 29 '13 at 04:47

4

I would create methods for CalculateFinalPrice, CalculateOrderPrice, CalculateOrderTaxes and CalculateOrderDiscount, like this:

public decimal CalculateFinalPrice() {
    return CalculateOrderPrice() + CalculateOrderTaxes() - CalculateOrderDiscount();
}

public decimal CalculateOrderPrice()
{
    // Logic here to calculate order price
    return theOrderPrice;
}

public decimal CalculateOrderTaxes()
{
    // Logic here to calculate order taxes
    return theOrderTaxes;
}

public decimal CalculateOrderDiscount()
{
    // Logic here to calcuate order discount
    return theOrderDiscount;
}

This provides you with more, but smaller pieces that are easier to maintain, read and unit test, because each method has a single responsibility.

answered Aug 29 '13 at 04:47

Karl Anderson

34,606
12
65
80

any word on why Functions instead of Getters ? just wondering why you prefer the functions approach vs Alex who suggest the getter approach – Bart Calixto Aug 29 '13 at 05:04
I prefer the method approach, because methods may or may not be attached to an instance, whereas properties necessarily are part of an instance of a class. For example, you could make the above methods part of a utility class where all of them are static and they could be used by other parts of your application that have no need to know of any particular instance of an object. – Karl Anderson Aug 29 '13 at 05:13
1

@Bart Here are some good explanations for SO questions on Properties vs Methods. [link1](http://stackoverflow.com/q/601621/1531157), [link2](http://stackoverflow.com/q/1294152/1531157) – Imran Aug 29 '13 at 06:03
accepted the other answer as I think getter / setter is more suited for my situation. still your answer is as good as the other. unfortunately I cannot accept two answers. – Bart Calixto Aug 29 '13 at 20:14
@Bart - no worries, as long as you got the help you wanted, that is what matters. Good luck to you. – Karl Anderson Aug 29 '13 at 22:42

Alex Siepman · Accepted Answer · 2013-08-29T07:45:50.207

4

I would do all the logic in the getters:

public decimal CalculateFinalPrice
{
    get { return CalculateOrderPrice + CalculateOrderTaxes - CalculateOrderDiscount; }
}

public decimal CalculateOrderPrice
{
    get { return products.Sum(p => p.Price*p.Quantity); }
}

public decimal CalculateOrderTaxes
{
    get { return products.Sum(p => p.Taxes*p.Quantity); }
}

public decimal CalculateOrderDiscount
{
    get { return products.Sum(p => p.Price*p.Quantity*p.Discount); }
}

If this is slow in your secenario, you can cache the properties.

private decimal? _calculateOrderPrice;
public decimal CalculateOrderPrice
{
    get
    {
        if (_calculateOrderPrice == null)
        {
            _calculateOrderPrice = products.Sum(p => p.Price*p.Quantity;
        }
        return _calculateOrderPrice.Value;
    }
}

If you go to the definition of the property, you can see immediately how it is calculated. Also you don't care about wich calculation needs to be done first.

edited Aug 29 '13 at 07:45

answered Aug 29 '13 at 04:56

Alex Siepman

2,499
23
31

can you add something with a cache scenario ? you mean store the values on private properties ? also any advantage of using getters instead of functions ? – Bart Calixto Aug 29 '13 at 05:01
@Bart. Yes, you could store the cache in a private field use Lazy. When to use properties or when to use functions without parameters. Thats is a long story. For now I should say, if the vanlue never change AND if the first time you use the property it is not very slow but, you can use a (cached) property. – Alex Siepman Aug 29 '13 at 05:16
@AlexSiepman A side note. Caching is fine. But, you need to ensure to invalidate the cache if any one changes the state of the `Order` object (if `Order` class is mutable which it seems not in this example. Once an `Order` object is instantiate with `Product` array, I guess no client will change the array.) – Imran Aug 29 '13 at 05:55
@Imran The way someone use caching depends on the context and the code guidelines. I do not change parameters comming in but maybe others do. Sometimes it is better to use immutable objects or create a clone to make it impossible to change an incomming value. I could add all the scenario's but then the answer would become very long. – Alex Siepman Aug 29 '13 at 07:20
@AlexSiepman Yes, I agree. Caching is not the one OP is asking for. That was the reason I mentioned it is just *a side note*. – Imran Aug 29 '13 at 07:32
thank you all, I think later I will open another question asking more specifically about caching and when to use Lazy<> instead of if (a != null) return a; – Bart Calixto Aug 29 '13 at 20:13

Getter, Function or just set it on constructor?

2 Answers2