do you put your calculations on your sets or your gets . .

Question

which is better ???

public class Order
{
   private double _price;
   private double _quantity;

  public double TotalCash
  {      
   get
   {
    return _price * _quantity;
   }
}

or

public class Order
{

   private double _totalCash;
   private double _price;
   private double _quantity;

  private void CalcCashTotal()
 {
   _totalCash = _price * _quantity
 }

  public double Price
  {      
   set
   {
     _price = value;
     CalcCashTotal();
   }
  }

  public double Quantity
  {      
   set
   {
     _price = value;
     CalcCashTotal();
   }
  }


  public double TotalCash
  {      
   get
   {
    return _totalCash;
   }
}

Answer 1

I'll put a calculation on a read-only get or a set.

I'm of the opinion that a property should behave like it has a backing variable.

I don't like calculations that take long on reads.

Answer 2

Typically I try to put them on set since the value they generate will be stored internally and only need to be calculated once. You should only put calculations on get if the value is likely to change every time its queried.

In your price/quantity example, you could actually have a single separate method that recalculates the quantity when either price or quantity is set.

Answer 3

The first is better because:

• It's shorter.
• It's easier to understand.
• It's slightly presumptuous to recalculate TotalCash every time either price or quantity gets set. It should be as lazy as possible and only calculate when requested.

That being said, putting the calculation in the setter effectively caches it, so if you run into a performance issue, that could be a useful change (at the expense of clarity).

Answer 4

I'd go with the approach of doing the calculation in the getter for TotalCash because less code is almost always better. It also ensures that the value of TotalCash is always correct. As a contrived example, if you had another method NewOrder(Price, Qty) and you forgot to call CalculateTotal at the end of this method you could very easily end up with an incorrect value for TotalCash.

Calculating it in the setter might be better if the calculation takes a while to process and changing the values of only one or two properties would require recalculation but it is almost always better to go for the approach that leaves less room for error, even if it takes slightly longer to execute.

Answer 5

The first, because:
1) Less code is better;
2) Less complexity;
3) Less variables helps to get less colateral problems;
4) The property will be always updated;
5) If you change "CalcCashTotal" procedure name you get more another points to change...

Answer 6

Putting it on the get function is not very ideal. You will be recalculating it for no reason. It doesn't even make any more sense. So here is a case where ::gasp:: optimization makes sense and is preferred. Calculate it once and reap the benefits.

Answer 7

There are tradeoffs. If the calculations simple and does not take a long time, then put it in the get. It makes your life easier, because you don't have to worry about doing the check in every set that total price is dependant on, which could lead to mistakes.

If the calculation takes a lot of time, then you could also take a hybrid approach. You could set a IsDirtyTotalPrice boolean in all the dependant sets, and then do the calculation on the fly in the get and cache it so that the get only calculates the variable when it needs to. You don't do the calculation in the sets because there could be a lot of them, and you wan to do the calculation as few times as possible.

  public class Order
  {
     private double _totalCash;
     private double _price;
     private double _quantity;
     private _IsDirtyTotalCash = true;

  private void CalcCashTotal()
  {
    _totalCash = _price * _quantity
  }

  public double Price
  {      
   set
   {
     _price = value;
     _IsDirtyTotalCash = true;
   }
  }

  public double Quantity
  {      
   set
   {
     _price = value;
     _IsDirtyTotalCash = true;
   }
  }

  public double TotalCash
  {      
   get
   {
        if(_IsDirtyTotalCash)
    {
      _totalCash = CalcTotalCost();
       _isDirtyTotalCash = false;
     }
     return _totalCash;
   }
  }

}

Answer 8

I've always been told that if there is considerable work or calculation being done, that you should do it in a method. There's no major compiler/runtime advantages as far as I know, but it will make more sense to the consumer of the code. A property that takes a while to return the value to me would throw up a red flag that something could be wrong.

That's my 2 cents... but even I would probably just go with your first example if the class is really that simple :-)

Answer 9

The first option is better. The difference in "optimization" is minuscule. Not to mention, what if setting occurs over and over but you only ever need to get TotalCost once? I'd be more concerned about lost developer hours trying to debug the class once it becomes very complex.

But there is an important time when the second option is needed, specifically when the calculated value changes the calculated objects. I'm having a hard time coming up with an example, so I'll use a real-life one where the number of bays in a wall is determined by its width.

class Wall {
    public decimal Width {
       get {
           ...
       }
       set {
           ValidateChanges();
           width = value;
           CreateBays();
       }
    }

    public Bays[] Bays {
       get {
           ...
       }
       set {
           ValidateChanges();
           ...
       }
    }

    private void CreateBays() {
        // Delete all the old bays.
        ...
        // Create a new bay per spacing interval given the wall width.
        ...
    }
}

Here, every time the width changes the bays in the wall are recreated. If this occurred on Bay.getter it would be fairly disastrous for the properties of the Bay object. The getter would have to figure out if the width has changed or not since the last get statement, increasing complexity.

Answer 10

I would go with Charles Graham's hybrid suggestion, but I want to add my two cents as to why.

A lot of the above suggestions talk about complexity and optimization, but forget this all goes out the window when you factor in the consumer of your class. If you are the only consumer, and you used the first implementation, chances are you would remember to:

double subTotal = myOrder.TotalCash;
double tax = subTotal * 0.05;
double shipping = subTotal > 100 ? 0 : 5.95;
double grandTotal = subTotal + tax + shipping;
OutputToUser(subTotal, tax, shipping, grandTotal);

Other people might not. Seeing that myOrder.TotalCash is a property, not a method, at least I would assume it is a cached value. That is, accessing subTotal in the above example is comparable in efficiency as accessing myOrder.TotalCash. Not realizing there's a calculation going on behind the scenes, they write:

double tax = myOrder.TotalCash * 0.05;
double shipping = myOrder.TotalCash > 100 ? 0 : 5.95;
double grandTotal = myOrder.TotalCash + tax + shipping;
OutputToUser(myOrder.TotalCash, tax, shipping, grandTotal);

leaving myOrder.TotalCash to stand for the subtotal. Now, it has been calculated 4 times instead of 1.

In summary, I'm sure I'm not the only one who believes that a property represents a variable or cached value and a method processes something and returns a value. It makes sense to store CalculateTotalCash() and only call it once, because you expect it to be a performance hit. On the other hand, you expect TotalCash to be a cached value and can access it at will. Thus it is important to only recalculate TotalCash when it changes.

The hybrid suggestion wins in the case of multiple sets between reads. That way you're not wasting time calculating a value to be thrown away.

Answer 11

When determining whether a property should be derived/calculated it is important to consider if the value at the time of calculation needs to be persisted.

In this case of TotalCash - if the business logic changes for the calculation, having the TotalCash property change retroactively for existing records may not be desirable.

just putting it out there...

Answer 12

It depends. Is your application read heavy or write heavy? Is the calculation expensive? If the calc is expensive and your application is read heavy, do it on the set, that way you only pay the calc penalty a few times (compared to reads).

Answer 13

My rule, and I recommend this to anyone:

Methods = with side effects Getters = without side effects (EXCEPT memoization - which is allowed in getters, too)