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

Visitor Pattern/Leaky Bucket variant implementation to run an operation at a certain interval

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

Problem

My code is a variant on the Visitor Pattern and a "leaky bucket" variant. The goal is pretty straightforward: the "bucket" will collect a specified number of items (say, for example, 500) and then empty the queue, running some CPU-bound operation on ("visiting") each item. (Yes, I'm aware that there's probably existing code "out there" that does something similar to what I'm doing here - this code really isn't all that novel of an idea).

I've read documentation on how to implement the Visitor pattern before but this is truthfully the first time I've actually implemented one. Is what I have below a "valid" way to implement this? Also, have I properly implemented the concurrency/thread safety? I think it should be free from race conditions but it's also possible that I can implement this more simply.

```
///
/// "Overflowing bucket" implementation
///
/// Type of the data structure we're operating on
/// Type of the items in the data structure
///
/// The basic idea of this data structure is that it'll collect a certain number of items and then empty the queue.
///
/// The "overflowing bucket" metaphor isn't perfect because every time that the "bucket" is "filled to the brim" or starts
/// to "overflow" we just empty the whole thing.
///
public class OverflowingBucket
{
#region Fields
// Not volatile - this will only be accessed by the background thread
private readonly T itemToActOn;

// Not volatile - this will only be accessed by the background thread
private readonly Action visitorOperation;

// This action runs after we empty the queue
private readonly Action afterAction;

// Must be concurrent because we could add to the queue while an operation is in place
private readonly ConcurrentQueue queue;

// Will be accessed by multiple threads
private volatile bool inProgress = false;

// Obviously used for locking
private readonly object lockObj = new object();
#endregion Fields

#region Constructor

Solution

First of all, IMHO the code is well written and has some meaningful comments.

Visitor Pattern

Actually, I am not sure if the OverflowingBucket has something to do with the visitor pattern. In my eyes, the visitor pattern can be used to add logic to a data structure by implementing visitors. A visitor visits each element of the data structure and decide what to do depending of the element's type.

The OverflowingBucket can be used to add logic to an object for processing other items in a special manner. Therfore I wouldn't use the term "visitor pattern" here.

OverflowingBucket

  • Because it is not a visitor in my eyes, I would rename the method Visit


to Process or something like that.

  • Consider to make the method Process private and add another method ProcessPendingItems or something like that.



  • The only case where the method Visit returns false is, if queue.Any() is true and queue.TryDequeue(out item) is false. That should never happen in the current version of the class... therefore I would make the method void.



TestOverflowingBucket

  • The delay does not delay the Add-Task but the main thread. Adding Thread.Sleep() within the operation delegate would be more realistic.



  • There is no need to check if the task is completed. You can just add them all to the tasks list.

Context

StackExchange Code Review Q#142629, answer score: 6

Revisions (0)

No revisions yet.