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

ConcurrentThrottledList - Will this implementation be thread safe?

Submitted by: @import:stackexchange-codereview··
0
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:

  • 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:

  • Add would add the specified item, throw an exception if it can't, and return void.



  • TryAdd would add the specified item, return true if successful and false otherwise.



  • Remove would remove the specified item, throw an exception if it can't, and return void.



  • TryRemove would remove the specified item, return true if successful and false otherwise.



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.