Should a C# accessor use a private variable or calculate on the fly?

Which is a better programming practice and why?

I have a class like this:

class data {

    public double time { get; internal set; }
    public double count { get; internal set; }
    public average_co开发者_运维百科unt { ... }

}

Where average_count should be read_only and give a calculation of count / time.

Is it better to write the accessor as:

public average_count { get {

    return (time == 0) ? 0 : (count / time);

}}

Or should I do something like:

private _avg_count;

public average_count {
    get 
    {
        return _avg_count;
    }

    internal set
    {
        return _avg_count;
    }
}

Where _avg_count is updated when in the time and count set accessors?

It seems like the first is easier to read but may be slower if average_count is accessed often. Will the compiler optimizations make the difference insignificant?

---

Doing it on the fly results in more readable code. Precalculating may improve performance, but you should only do this if (a) it's necessary and (b) you've profiled and it makes a difference.

The bottom line is, readability and maintainability should only be sacrificed for performance when it's absolutely necessary.

---

This is such a seemingly simple question, yet it's almost impossible to answer. The reason is that what's "right" depends on a number of factors.

1. Performance

In his answer Skilldrick recommends that you prefer what is readable over what performs best as a general rule:

[R]eadability and maintainability should only be sacrificed for performance when it's absolutely necessary.

I would counter this by saying that it is only true in a typical business application, where performance and functionality are two clearly distinguishable features. In certain high-performance software scenarios, this is not such an easy thing to say as performance and functionality may become inextricably linked -- that is, if how well your program accomplishes its task depends on how solidly it performs (this is the case at my current place of employment, a company that does algorithmic trading).

So it's a judgment call on your part. The best advice is to profile whenever you have an inkling; and if it's appropriate to sacrifice readability for performance in your case, then you should do so.

2. Memory Usage

0xA3 suggests a fairly elegant approach providing a compromise of sorts: only calculate the value as needed, and then cache it.

The downside to this approach, of course, is that it requires more memory to maintain. An int? requires essentially the same amount of memory as an int plus a bool (which, due to alignment issues, could actually mean 64 bits instead of just 40). If you have loads and loads of instances of this data class and memory is a scarce resource on the platform you're targeting, bloating your type with another 32 bits per instance might not be the smartest move.

3. Maintainability

That said, I do agree in general with what others have said that, all else being equal, you're better off erring on the side of readability so that you can understand your code if and when you revisit it in the future. However, none of the various approaches to this problem is particularly complicated, either.

The bottom line is that only you know your exact circumstances and thus you are in the best position to decide what to do here.

---

It depends how often you will call average_count and how often count and time are modified. For such kind of optimization I would suggest you to use profiler.

---

In cases where performance is critical and you need to frequently access the property you have another option as well: Calculating the result when it is needed and then cache the result. The pattern would look like this:

class Foo
{
    int? _cachedResult = null;

    int _someProperty;
    public int SomeProperty
    {
        get { return _someProperty; }
        set { _someProperty = value; _cachedResult = null; }
    }

    int _someOtherProperty;
    public int SomeOtherProperty
    {
        get { return _someOtherProperty; }
        set { _someOtherProperty = value; _cachedResult = null; }
    }

    public int SomeDerivedProperty
    {
        get
        {
            if (_cachedResult == null)
                _cachedResult = someExpensiveCalculation();

            return (int)_cachedResult;
        }
    }
}

---

In this simple case, I would definitely implement the calculated version first; then optimize if neccessary. If you store the value, you will also need extra code to recalculate the value if any of the values it depends on, changes, which leads to possibilities for errors.

Don't optimize prematurely.

---

I'd go with your first option. These are in-memory objects so the computation on what you're doing is going to be extremely fast. Further, if you created a dedicated property for this (e.g., average_count) then you're going to have to add more code to re-calculate this in the setter for both the time and the count.

As a side note (since you're asking about best practice), you should be using Pascal casing in C# which is initial caps and no underscores.

---

Will the compiler optimizations make the difference insignificant?

Depends on what you consider "significant". Reading a varaible is rather quick. Dividing two number is rather quick. In fact, depending on what in the RAM cache, reading the variables may take longer than doing the divide.

Use the first method. If it's seems to slow, then consider the second.

---

If you care about thread safety it may be way easier to do the second option rather than the first.

private double _avg_count;
static readonly object avgLock = new object();

public double time { get; internal set; }
public double count { get; internal set; }


public double average_count {
    get 
    {
        return _avg_count;
    }


}

private void setAverageCount()
{
    _avg_count = time == 0 ? 0 : (count / time);
}


 public void Increment()
 {
     lock (avgLock)
     {
         count += 1;
         setAverageCount();
     }


 }


 public void EndTime(double time)
 {
     lock (avgLock)
     {
         time = time;
         setAverageCount();

     }
 }