@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<T>.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.
Count
similarly if isIsFull
is useful. For example in a client/server application to show the clients the (most up-to-date available) number elements in some lists and not just if they are full or not. Currently.Items.Length
would cause memory waste. \$\endgroup\$