Return to Answer

@ChrisW made valuable comments, I'll add mine:

###Well done:

• I like that private fields are all¹ 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.

_{¹ 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.}

@ChrisW made valuable comments, I'll add mine:

Well done:

• I like that private fields are all¹ 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.

_{¹ 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.}

@ChrisW made valuable comments, I'll add mine:

###Well done:

• I like that private fields are all¹ 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.

_{¹ 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.}

@ChrisW made valuable comments, I'll add mine:

###Well done:

• I like that private fields are all¹ 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.

_{¹ 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.}

@ChrisW made valuable comments, I'll add mine:

###Well done:

• I like that private fields are all¹ 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 added - 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.

_{¹ 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.}

@ChrisW made valuable comments, I'll add mine:

###Well done:

• I like that private fields are all¹ 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.

_{¹ 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.}