patterncsharpMinor
ConcurrentThrottledList - Will this implementation be thread safe?
Viewed 0 times
thiswillthreadsafeimplementationconcurrentthrottledlist
Problem
I want a thread safe list with a max item count, will the following implementation correctly provide this?
public class ConcurrentThrottledList
{
private readonly List items = new List();
private readonly int maxItems;
private object _lock = new object();
public ConcurrentThrottledList(int maxItems)
{
this.maxItems = maxItems;
}
public bool TryAdd(T item)
{
lock (_lock)
{
if (items.Count = maxItems; } } }
public T[] Items { get { lock(_lock) { return items.ToArray(); } } }
}Solution
@ChrisW made valuable comments, I'll add mine:
Well done:
Nitpicks:
-
I don't see why
-
-
However,
I agree with
1 I would be led to believe
Well done:
- I like that private fields are all1
readonly, this makes intent explicit, which works towards greater maintainability.
- I like that the class is highly cohesive and focused, this works towards greater readability, maintainability and extensibility.
Nitpicks:
-
I don't see why
_lock gets an underscore and items and maxItems don't; being consistent with the underscore prefix for private fields would mootinate the this qualifier in the constructor.-
Items could be called ToArray, making it more consistent with the framework's lingo, and more representative of its semantics.-
TryAdd and TryRemove both look like they were named after the TryParse pattern; a developper looking at the API would then also be expecting an Add and a Remove method:Addwould add the specified item, throw an exception if it can't, and returnvoid.
TryAddwould add the specified item, returntrueif successful andfalseotherwise.
Removewould remove the specified item, throw an exception if it can't, and returnvoid.
TryRemovewould remove the specified item, returntrueif successful andfalseotherwise.
However,
List.Remove() is already taking care of returning false if the specified item cannot be removed - throwing an exception in that case would be utter nonsense. Therefore, I'll second @ChrisW in saying that TryRemove should be simply called Remove.I agree with
TryAdd being called as such, because I would seriously expect an Add method to return void, so I'd consider also having an Add method that throws some CapacityExceededException (or whatever is more appropriate) when it can't add the specified item.1 I would be led to believe
_lock could also be made readonly, but I don't work with enough multithreaded code to know whether that would/could impact anything.Context
StackExchange Code Review Q#43627, answer score: 5
Revisions (0)
No revisions yet.