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.
1 Answer 1
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;
}
}
-
2\$\begingroup\$ 1. I would go with the
Try-
naming. Similar .Net methods use that naming convention (e.g. those fromSystem.Collections.Concurrent
). 2. I wouldn't useInterlocked
here unless the lock was too slow. Code that useslock
is much easier to understand and also much easier to get right. \$\endgroup\$svick– svick2013年05月21日 09:05:07 +00:00Commented 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\$almaz– almaz2013年05月21日 09:58:14 +00:00Commented 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\$Erik Funkenbusch– Erik Funkenbusch2013年05月21日 15:00:31 +00:00Commented May 21, 2013 at 15:00
-
1\$\begingroup\$ @MystereMan There are no
Interlocked
methods forDateTime
. \$\endgroup\$svick– svick2013年05月21日 22:33:36 +00:00Commented 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\$almaz– almaz2013年05月22日 08:44:35 +00:00Commented May 22, 2013 at 8:44