Class methods implementation: should it change a class' member variable or take in arguments?

Question

I guess I'll try to explain my question through 2 code snippets:

// snippet 1
class FooBar
{
   private int value;

   public int DetermineSomeResult()
   {
       PerformSomeCalculationOnValue();
       PerformSomeMoreStuffOnValue();

       return value;
   }

   public int GetCachedValue()
    {
       return value;
    }
}

The first implementation basically has methods which operates on the private integer member.

Here's the second implementation

// snippet 2
class FooBar2
{
    private int value;

    public int DetermineSomeResult()
    {
       int tempvalue =GetCachedValue();
       tempvalue =  PerformSomeCalculationOnValue(tempvalue);
       tempvalue = PeformMoreStuffOnValue(tempvalue);

       SetValue(tempvalue);
       return tempvalue;
    }

    public int GetCachedValue()
    {
       return value;
    }
}

For the second implementation, no change is made to the internal state until all the calculations have finished. Design-wise which one is better?

I tend to prefer the second one as it is clearer, shows the dependencies. However, since the class itself already stores a class member intended for the calculated value, it seems silly not to use it.

Any thoughts on this?

Answer 1

I think you faced a design issue described as Command and state separation

in short I would suggest that DetermineSomeResult() should only calculates a new value of the field "value" without returning it and only way to get the "value" is via GetCachedValue()

public void DetermineSomeResult()
{
   PerformSomeCalculationOnValue();
   PerformSomeMoreStuffOnValue();
   // consider "value" has already been set.
}

public int GetCachedValue()
{
   return value;
}

Answer 2

By using internal methods that take parameters rather than treating a class member variable as global for that class you not only make it easier to unit test those functions, but you also reduce the possibility of introducing bugs due to that member variable being altered out of sequence, especially if the class has any event or worker thread driven behaviour.

So I would go for the second example rather than the first.

edit Also if your programming language supports them, you can use pointers to a given variable type for those functions so that they become (in a C style psuedo code):

DoSomeWorkOn(pointerto int);

rather than

int newValue = DoSomeWorkOn(int oldValue);

Answer 3

Just given the code above, both would work.

However, there's a subtle issue here that I would like to talk about. In the 1st case, your internal state may be left in inconsistent state. This can happen if PerformSomeMoreStuffOnValue methods throws an Exception before it can set the final state of value. 2nd solution does not leave in incosistent state w/o taking in account other things that may be at play).