5
\$\begingroup\$

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 is important because if it implies no state change, then people call the method multiple times without realizing that it also changes state.

Can anyone suggest a name that adequately documents what the method is doing, without making it overly complex? The name should indicate that it's mutating the object as well as performing an action that is a Boolean state.

Alternatively, if you can suggest a better way to accomplish this in a thread safe manner that would work better, that would be good too.

\$\endgroup\$

1 Answer 1

Reset to default
4
\$\begingroup\$

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;
    }
}
\$\endgroup\$
5
  • 2
    \$\begingroup\$ 1. I would go with the Try- naming. Similar .Net methods use that naming convention (e.g. those from System.Collections.Concurrent). 2. I wouldn't use Interlocked here unless the lock was too slow. Code that uses lock is much easier to understand and also much easier to get right. \$\endgroup\$ Commented May 21, 2013 at 9:05
  • \$\begingroup\$ @svick agree, I added Interlocked version as an example of lock-free implementation. It's always good to know your options :) \$\endgroup\$ Commented May 21, 2013 at 9:58
  • \$\begingroup\$ Out of curiosity, why did you choose to do the math via ticks? It seems like there are more conversions involved there. \$\endgroup\$ Commented May 21, 2013 at 15:00
  • 1
    \$\begingroup\$ @MystereMan There are no Interlocked methods for DateTime. \$\endgroup\$ Commented May 21, 2013 at 22:33
  • \$\begingroup\$ yep, @svick is right, I had to switch to ticks in order to use atomic operations on it. \$\endgroup\$ Commented May 22, 2013 at 8:44

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.