Better way of refactoring this ?

Question

I have the following classes:

public abstract class BaseClass
{
    private readonly double cachedValue;

    public BaseClass()
    {
         cachedValue = ComputeValue();
    }

    protected abstract double ComputeValue()          
}


public class ClassA : BaseClass
{
    protected override double ComputeValue()  { ... }            
}

public class ClassB : BaseClass
{
    protected override double ComputeValue()  { ... }                
}

where ComputeValue() is a time consuming computation.

Problem is, there are other methods in the ClassA and ClassB which require the value returned from ComputeValue(). I was thinking of adding a protected property named 'CachedValue' in BaseClass but I find this approach to be redundant and confusing to other programmers who might not be aware of its existence, and might call ComputeValue() directly.

The second option is to use nullable type at the derived class level as I don't necessarily require the computation to be done in the constructor in BaseClass, lazy computation might be a better option:

protected override double ComputeValue()  
{
    if(cachedValue.HasValue)
    {
        return (double) cachedValue;
    }

    // Do computation
    cachedValue = ...

    return cachedValue;
}        

but I feel I could do better.

What are your thoughts on this ?

Thank you.

Edit: Just to clarify, I'm trying to prevent ComputeValue() from getting called more than once by enforcing the use of 'cachedValue'.

Answer 1

Why not take a delagate in the constructor which has the same purpose of ComputeValue and then expose the value as a protected readonly field?

public abstract class BaseClass {
  protected readonly double cachedValue;

  protected BaseClass(Func<double> computeValue) {
    cachedValue = computeValue();
  }
}

Answer 2

Is there a relevant link between the logic that needs the calculated value, and the logic to calculate the value?

If it isn't, or at least there is no 100% match, you could put the calculation logic in separate classes: CalculatorA and CalculatorB, which both inherit from the interface ICalculator. The Base class could then be the only class that accesses this interface and cache the results.

Answer 3

At some point you will have to initialize this value, there is no way around that. So I think the method where you use the nullable value makes sense - I agree that it's much clearer than having a cached property in there too.

You might though want to add a parameter to ComputeValue which forces the value to be computed again:

protected override double ComputeValue(bool force)   
{ 
    if(!force && cachedValue.HasValue) 
    { 
        return cachedValue; 
    } 

    // Do computation 
    cachedValue = ... 

    return cachedValue; 
}         

Answer 4

Another approach would be to add a public interface method GetValue on the Baseclass and only allow to overwrite the ComputeValue method for inherited classes - this way you can still extend the functionality, but you control the behavior/context of the result of ComputeValue in your base class, i.e. you can add memoization like in the example or decorate/extend as needed.

This follows the Non-Virtual interface (NVI) pattern .

    public abstract class BaseClass
    {
        private double? cachedValue;

        public BaseClass()
        {
        }

        protected abstract double ComputeValue();

        public virtual double GetValue()
        {
            if (cachedValue.HasValue)
            {
                return (double)cachedValue;
            }
            else
            {
                cachedValue = ComputeValue();
                return (double)cachedValue;
            }

        }
    }

Answer 5

I think ComputeValue should be lazily called in a property getter:

public abstract class BaseClass
{
    private Func<double> _computeValue;

    private double? _cachedValue;
    protected double cachedValue
    {
       get
       {
          if(_cachedValue == null)
             _cachedValue = _computeValue();
          return (double)_cachedValue;
       }
       private set
       {
          _cachedValue = value;
       }
    }

    private BaseClass(){};
    public BaseClass(Func<double> computeValue)
    {
         _computeValue = computeValue;
    }    

}

Answer 6

If you're on NET 4just use Lazy<T>.

public class ClassA
{
    Lazy<double> value = new Lazy<double>(()=>something.SomethingComplicated());

    public void AnyMethod()
    {
        double d = value.Value;
        //...
    }

}

See http://msdn.microsoft.com/en-us/library/dd642331.aspx

Update: Since you're on .NET 3.5, here's a great article about implementing lazy yourself (it's only about 20 LOC): http://msdn.microsoft.com/en-us/vcsharp/bb870976.aspx

A generally good piece of advice is to always use composition over inheritance :)

Answer 7

For storing/caching the computed value, you can use the Singleton Pattern, basically you declare a static field then you check for null value before attempt to calculate. So the computed value will be calculated only the first time the method or property is called/acceded. If some derived class require a different computed value the you override the base class method/property (the virtual modifier is required to assure polymorphism). It is recommended to use properties instead of methods for elements that represent a kind of data for the class.

public abstract class BaseClass {
    private static double _cachedValue = null;
    public BaseClass() { }
    // base class implements a Singleton Pattern to store a computed value
    protected virtual double ComputeValue
    {
        get
        {
            if( _cachedValue == null) { _cachedValue = CalculateComputedValue; }
            return _cachedValue;
        }
    }   
    private double CalculateComputedValue() { return someComplexComputedValue; }
}

public class ClassA : BaseClass
{
    private static double _cachedValue = null;
    //this class does require calculate a specific computed value.
    protected override double ComputeValue
    { 
        get
        {
            if( _cachedValue == null) { _cachedValue = CalculateComputedValue; }
            return _cachedValue;
        }
    }
    private double CalculateComputedValue() { return someComplexComputedValue; }
}

public class ClassB : BaseClass
{
    //this class does not require to calculate compute value, inherited value is used instead.
}