HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Function that tests and mutates

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
functionteststhatandmutates

Problem

I am a huge proponent of self-documenting code (when at all possible). I prefer to have code be self evident by the very nature of the names of the variables, methods, etc.

For instance:

if (TheSky.IsBlue && TheGrass.IsGreen)
    BeHappy();


I have run into a situation that has defied all attempts to name the method in a way that illustrates what it does. The problem is that the method is required to do two things.

I have a class that must maintain its own state, however it must have a method that both verifies a condition, and changes the state via a thread safe lock to prevent race conditions.

It basically looks like the code below:

public class TimeWindow 
{
    private static object lockObject = new object();
    private DateTime Timestamp;
    private int WindowCounter;

    // What do I call this?  CanWeDoSomething() implies that it is merely
    // calculating a true/false state, not mutating state. 
    //
    // I don't want to call it 
    // CanWeDoSomethingAndDecrementItIfWeActuallyDoIt() as that is
    // just too annoying, and it messes up the self-documenting nature
    // since if (CanWeDoSomethingAndDecrementItIfWeAcutallyDoIt()) describes
    // two actions, but only one of them is a Boolean condition, so it doesn't
    // really fit in with the if statement.
    public bool CanWeDoSomething(int WindowSeconds) {
        lock(lockObject) {
            if ((DateTime.Now - TimeStamp).TotalSeconds > WindowSeconds) {
               Timestamp = DateTime.Now;
               WindowCounter = 0;
            }
            if (WindowsCounter < 10) {
               ++WindowCounter;
               return true;
            }
            return false;
        }
    }
}


As you can see, I have to perform the test (to retrieve the Boolean) and modify state at the same time. If I didn't do this, then a race condition is possible where the value can be changed by another thread in between checking it's value and mutating its state.

The naming

Solution

I would call this method AcquireTimeSlot or similar, since the main action here is actually capturing a timely-limited resource. Result of the method specifies whether resource has been acquired or not. You may also call it TryAcquireTimeSlot to highlight the fact that acquisition may fail.

As a side note - you can rewrite the code without locks in case if that's the only place you use the lockObject. It will be a bit more verbose though...

public class TimeWindow
{
    private static readonly object _lockObject = new object();
    private long _timestampTicks;
    private int _windowCounter;

    private void CheckWindowExpiration(int windowSeconds)
    {
        long timestampTicks = _timestampTicks;
        while (TimeSpan.FromTicks(DateTime.Now.Ticks - timestampTicks).TotalSeconds > windowSeconds)
        {
            if (Interlocked.CompareExchange(ref _timestampTicks, DateTime.Now.Ticks, timestampTicks) == timestampTicks)
            {
                _windowCounter = 0;
                break;
            }

            timestampTicks = _timestampTicks;
        }
    }

    public bool TryAcquireTimeSlot(int windowSeconds)
    {
        CheckWindowExpiration(windowSeconds);

        int windowCounter = _windowCounter;
        while (windowCounter < 10)
        {
            if (Interlocked.CompareExchange(ref _windowCounter, windowCounter + 1, windowCounter) == windowCounter)
                return true;

            windowCounter = _windowCounter;
        }

        return false;
    }
}

Code Snippets

public class TimeWindow
{
    private static readonly object _lockObject = new object();
    private long _timestampTicks;
    private int _windowCounter;

    private void CheckWindowExpiration(int windowSeconds)
    {
        long timestampTicks = _timestampTicks;
        while (TimeSpan.FromTicks(DateTime.Now.Ticks - timestampTicks).TotalSeconds > windowSeconds)
        {
            if (Interlocked.CompareExchange(ref _timestampTicks, DateTime.Now.Ticks, timestampTicks) == timestampTicks)
            {
                _windowCounter = 0;
                break;
            }

            timestampTicks = _timestampTicks;
        }
    }

    public bool TryAcquireTimeSlot(int windowSeconds)
    {
        CheckWindowExpiration(windowSeconds);

        int windowCounter = _windowCounter;
        while (windowCounter < 10)
        {
            if (Interlocked.CompareExchange(ref _windowCounter, windowCounter + 1, windowCounter) == windowCounter)
                return true;

            windowCounter = _windowCounter;
        }

        return false;
    }
}

Context

StackExchange Code Review Q#26407, answer score: 4

Revisions (0)

No revisions yet.