patterncsharpMinor
Priorizatable command queue
Viewed 0 times
commandpriorizatablequeue
Problem
This code consistently executes commands with a given priority. How can I to improve it?
```
public class PriorizatableCommandQueue : IDisposable
{
private readonly object _lock;
private readonly Action _action;
private ConcurrentQueue _forLowestPriority;
private ConcurrentQueue _forBelowNormalPriority;
private ConcurrentQueue _forNormalPriority;
private ConcurrentQueue _forAboveNormalPriority;
private ConcurrentQueue _forHighestPriority;
private ThreadPriority _currentPriority;
private CancellationTokenSource _cts;
private Task _task;
private bool _isExecuting;
private bool _isDisposed;
public PriorizatableCommandQueue(Action action)
{
_forLowestPriority = new ConcurrentQueue();
_forBelowNormalPriority = new ConcurrentQueue();
_forNormalPriority = new ConcurrentQueue();
_forAboveNormalPriority = new ConcurrentQueue();
_forHighestPriority = new ConcurrentQueue();
_lock = new object();
_action = action;
_currentPriority = ThreadPriority.Normal;
_cts = new CancellationTokenSource();
_task = new Task(() => { }, _cts.Token);
_task.Start();
_cts.Cancel();
_isExecuting = false;
_isDisposed = false;
}
private void ExecuteCommands(ConcurrentQueue queue, CancellationToken token)
{
T command;
while (true)
{
if (token.IsCancellationRequested)
{
lock (_lock)
_isExecuting = false;
throw new OperationCanceledException(token);
}
if (queue.TryDequeue(out command))
_action(command);
else
break;
}
lock (_lock)
{
queue = null;
if (_forHighestPriority.Count > 0)
{
queue = _forHighestPriority;
_currentPriority = ThreadPriority.Highest;
```
public class PriorizatableCommandQueue : IDisposable
{
private readonly object _lock;
private readonly Action _action;
private ConcurrentQueue _forLowestPriority;
private ConcurrentQueue _forBelowNormalPriority;
private ConcurrentQueue _forNormalPriority;
private ConcurrentQueue _forAboveNormalPriority;
private ConcurrentQueue _forHighestPriority;
private ThreadPriority _currentPriority;
private CancellationTokenSource _cts;
private Task _task;
private bool _isExecuting;
private bool _isDisposed;
public PriorizatableCommandQueue(Action action)
{
_forLowestPriority = new ConcurrentQueue();
_forBelowNormalPriority = new ConcurrentQueue();
_forNormalPriority = new ConcurrentQueue();
_forAboveNormalPriority = new ConcurrentQueue();
_forHighestPriority = new ConcurrentQueue();
_lock = new object();
_action = action;
_currentPriority = ThreadPriority.Normal;
_cts = new CancellationTokenSource();
_task = new Task(() => { }, _cts.Token);
_task.Start();
_cts.Cancel();
_isExecuting = false;
_isDisposed = false;
}
private void ExecuteCommands(ConcurrentQueue queue, CancellationToken token)
{
T command;
while (true)
{
if (token.IsCancellationRequested)
{
lock (_lock)
_isExecuting = false;
throw new OperationCanceledException(token);
}
if (queue.TryDequeue(out command))
_action(command);
else
break;
}
lock (_lock)
{
queue = null;
if (_forHighestPriority.Count > 0)
{
queue = _forHighestPriority;
_currentPriority = ThreadPriority.Highest;
Solution
First of all, have you considered implementing a Priority Queue? This will allow you to have just one queue, which automatically arranges items in it based on priority - this will allow you to just have one simple listener to get the Next Item from the queue, and rely on the priority to ensure the Next Item is always the highest priority available. There are many implementations of it, including one here on CodeReview.SE I found in the "Related" sidebar. Another advantage over your queue-per-priority is that you can have more complex prioritization logic, rather than just base it on one field.
However, if you wish to stick your your (conceptually simpler to grok) multiple queues, you can still make the code cleaner and more generic by replacing the variable-for-each-queue with a
However, if you wish to stick your your (conceptually simpler to grok) multiple queues, you can still make the code cleaner and more generic by replacing the variable-for-each-queue with a
SortedList - have the queues by keyed by priority, and instead of having a big switch/case block to determine the current queue, you can iterate on the Keys, which are sorted.Context
StackExchange Code Review Q#112412, answer score: 3
Revisions (0)
No revisions yet.