0

I know the fact that global mutable variables are bad as they can be accidentally modified and make testing difficult. However, there are situations when a class needs to cache some information, for e.g. caching the time when the last request was made to a public function of a class, and this information is totally private to the class to make certain decisions in its functions. In this scenario, a class may look like this:

public class MutableStateClass {
  //some dependencies
  private DateTime lastRequestTime;

  public void SomeFunc() {
    ...
    if(some decision based on lastRequestTime) {
      ...
    }
  }

  //other functions may also access lastRequestTime
}

One might say that we can parameterize the functions to have a DateTime parameter which holds the last request time but I think this knowledge is totally private to the class and it doesn't make sense to introduce this parameter in the public interface of the class as the callers don't have to know about the last request time.

Another example could be a bunch of booleans which make decision on what message to log based on some historical point for e.g. a periodic function which might be executed every 10 seconds might need to log some information on each invocation and this logging might depend on some historical knowledge. In this case as well we might end up with some booleans on class level which can be modified by some functions within the class.

So how to avoid these mutable class level variables when parameterizing them is not an option? This generally happens when we need to cache some information. How can we better design the class for this scenario?

I know there are many questions related to global variables but here I want to address on the mutable variables that exist within a class and I am not talking about application wide mutable globals.

9
  • that's not a global variable. its fine
    – Ewan
    Mar 18, 2020 at 10:05
  • @Ewan what about testing the functions? These variables are set by some class functions and don't have a public setter. It will surely make unit tests difficult. Or we need to have a public setter, but then other classes can invoke it. Mar 18, 2020 at 10:19
  • 1
    exactly what do you want to test that you cant? test to the interface not the implementation
    – Ewan
    Mar 18, 2020 at 10:25
  • For e.g. if we have if conditions around lastRequestTime variable, then we might need to set it to different values in order to test different outputs from the function but we can't set its value because it is being modified by class functions only. We don't have a public setter. Mar 18, 2020 at 10:27
  • since you dont expose it then its not part of the interface and shouldnt be tested
    – Ewan
    Mar 18, 2020 at 10:32

4 Answers 4

1

tl;dr If you need to share a mutable state between different methods in the same class, then having that mutable state stored in a field/property seems entirely reasonable. However, you may want to centralize how that state is read/modified rather than having each method duplicating the same access logic. To do this, you may want to write up a simple dispatcher.


It sounds like you've got a property that's being mutated by different methods in the same class:

public class SomeObject
{
    private DateTime LastMethodCallTime { get; set ; }

    //  Methods that read/write to the above property:
    public void SomeMethod_001() { /* ... */ }
    public void SomeMethod_002() { /* ... */ }
    public void SomeMethod_003() { /* ... */ }
}

Having a shared state isn't really a bad thing when it's unavoidable. This is, if you need to have coordination between method calls, then there necessarily must be something providing that coordination. That part's fine.

But why a DateTime? Does each method need to properly implement the same behavior for checking/updating it? Because if so, then that's probably something to fix.

Instead, you might want to code it sorta like:

public class SomeObject
{
    private DateTime LastMethodCallTime { get; set ; }
    private object ___LOCK___lastMetodCallTimeLockObject { get; } = new object();
    protected void PerformTimeSensitiveAction(Action<DateTime> actionToPerform)
    {
        lock (this.___LOCK___lastMetodCallTimeLockObject)
        {
            var lastMethodCallTime = this.LastMethodCallTime;
            actionToPerform(lastMethodCallTime);
            this.LastMethodCallTime = DateTime.Now;
        }
    }

    //  Methods that call this.PerformTimeSensitiveAction():
    public void SomeMethod_001() { /* ... */ }
    public void SomeMethod_002() { /* ... */ }
    public void SomeMethod_003() { /* ... */ }
}

Or, you could segregate the dispatcher logic to avoid having too much in the same class:

public class SomeObject
{
    private SimpleDispatcher Dispatcher { get; } = SimpleDispatcher.New();

    //  Methods that call this.Dispatcher:
    public void SomeMethod_001() { /* ... */ }
    public void SomeMethod_002() { /* ... */ }
    public void SomeMethod_003() { /* ... */ }
}

public class SimpleDispatcher
{
    private DateTime LastMethodCallTime { get; set ; }
    private object ___LOCK___lastMetodCallTimeLockObject { get; } = new object();

    protected SimpleDispatcher() { }
    public static SimpleDispatcher New()
    {
        var toReturn = new SimpleDispatcher();
        return toReturn;
    }

    public void Perform(
                Action<DateTime> actionToPerform
        )
    {
        lock (this.___LOCK___lastMetodCallTimeLockObject)
        {
            var lastMethodCallTime = this.LastMethodCallTime;
            actionToPerform(lastMethodCallTime);
            this.LastMethodCallTime = DateTime.Now;
        }
    }
    public T_Result Perform<T_Result>(
                Func<DateTime, T_Result> funcToPerform
        )
    {
        lock (this.___LOCK___lastMetodCallTimeLockObject)
        {
            var lastMethodCallTime = this.LastMethodCallTime;
            var toReturn = funcToPerform(lastMethodCallTime);
            this.LastMethodCallTime = DateTime.Now;

            return toReturn;
        }
    }
}

Usually an object that controls method accesses like this is called a dispatcher. For example, if you write GUI code (e.g., in WPF), you'll often need to call a dispatcher to perform actions within a GUI thread.

In a case like this, you might not care much about how the dispatcher works, in which case a C#-lock might be sufficient. Or if you want finer behavior, you might implement, e.g., a dispatcher queue using a ConcurrentQueue<Action>.

Whatever the case, the fact that you have a mutable field/property shared across your object doesn't appear to be a problem since, if you need to share state between different methods, that's a pretty logical place to put it. Instead, the concern'd be if you're duplicating the logic that works with that object; so, instead of using a plain DateTime and duplicating how to access it, writing a simple dispatching method or class would seem cleaner.

5
  • You said that keeping the state variables shared among class functions is fine. Let's for a second not create any dispatcher. These state variables are set and reset by class functions. There are no public getters and setters as we don't want other classes to get these variables. I think unit testing in this case would be difficult. Mar 18, 2020 at 10:23
  • @NavjotSingh: A dispatcher would be appropriate for stuff like this; it wouldn't be surprising if a lack of a dispatcher would enable undue complexity in, e.g., unit tests. To be clear: the sharing of a mutable entity isn't problematic, but having scattered logics interacting with it could be.
    – Nat
    Mar 18, 2020 at 10:31
  • Yes, scattered logic makes the code ugly and hard to read. Although I have to still wrap my head around this dispatcher as well. Mar 18, 2020 at 10:32
  • @NavjotSingh: Just to check: are you familiar with multi-threaded programming and issues like race conditions? For example, do you understand the issue asked about in this question?
    – Nat
    Mar 18, 2020 at 10:43
  • to be honest I asked the question in a general sense giving an example which involved DateTime but there can be other examples as well, like the one for logging which I gave in my question. Yes, I'm familiar with multi-threaded programming but my main aim was to look for a better design. Mar 19, 2020 at 8:09
0

So how to avoid these mutable class level variables when parameterizing them is not an option? This generally happens when we need to cache some information. How can we better design the class for this scenario?

You can provide another class to provide and manage the lastRequestTime when constructing the MutableStateClass like

public interface ILastRequestTimeManager {
    DateTime getLastRequestTime();
    setLastRequestTime();
}

public class LastRequestTimeManager : public ILastRequestTimeManager {
    private DateTime lastRequestTime;

    public DateTime getLastRequestTime() {
        return lastRequestTime;
    }
    public setLastRequestTime() {
        lastRequestTime = DateTime.Now();
    }
}

public class MutableStateClass {
    private ILastRequestTimeManager lastRequestTimeMgr;
    public MutableStateClass(ILastRequestTimeManager lastRequestTimeMgr_) : 
        lastRequestTimeMgr(lastRequestTimeMgr_) {}
}

Thus you'll follow the SRP, and the implementation details will be delegated to the implementation of the interface.

3
  • This is a solution which I also had in mind, but it is just an overkill. Here a variable is translated to an entire class and you had an extra interface as well. Imagine having many such classes, it will just fill the namespace with small classes that essentially just hold variables. Mar 18, 2020 at 10:17
  • @NavjotSingh Regarding writing suitable unit tests (as you mention them in your Q) it isnt overkill but just separating responsibilities, and allows you to provide mocks. Mar 18, 2020 at 10:19
  • What exactly are you gaining by this? (Obviously "follow SRP" and "delegate to the interface" are not gaining anything).
    – gnasher729
    Mar 19, 2020 at 9:58
0

If you really need to write Unit Tests around behavior that varies based on things like what happens (as an example) when a call is made and the lastRequestTime is greater than 10 seconds ago, then I would use a List<DateTime> for Requests instead of a single private field, and also provide a test-visible constructor by which you can instantiate the object mocked data for past requests.

public class MutableStateClass {
  //some dependencies
  private List<DateTime> requestHistory;
  private DateTime getLastRequestTime() => requestHistory.Max(); // needs null-check
  private void appendNewRequestTime() => requestHistory.Add(DateTime.Now);

  public void SomeFunc() {
    ...
    if(some decision based on lastRequestTime) {
      ...
    }
  }

  // make sure only the tests use this
  internal MutableStateClass(List<DateTime> _requestHistory) {
    requestHistory = _requestHistory;
  }

  //other functions may also access lastRequestTime
}
0

A private class variable is not global. The problem with a global variable is that it can be read or modified anywhere within your millions of lines of code. A private class variable can only be modified in a very limited scope, so that problem isn’t there.

Global variables would have to be part of a unit test (for example if calling public function f increases a global variable x by 1, then a unit test should test that. For your private class variable it’s not needed. It’s a private implementation detail. If it affects the behaviour of a function then that should be part of the spec of the function and you can write unit tests to verify the function works according to its spec, but that is something you need to do anyway.

Let’s say you have a public method

func calledWithinLast10Seconds () -> Bool

that does what its name says. Not trivial to write unit tests, not trivial to mock either. But that’s due to the spec, not due to the fact that you use some class variable to implement it.

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.