Fix type comparison in OrderedItemList #85
Conversation
|
in the issue i added a comment to look at how the whole class can be cleaned-up. This fix is so "minimal" it seems that request has not been taken into account. Does that mean the class needs no further improvement? |
|
I beg your pardon. My fault, I concentrated on the description of the issue and did not notice a comment. I will work through it a little bit later. |
… reduce repeatable code.
CDP4Common/Types/OrderedItemList.cs
Outdated
| if (index < 0 || index >= this.sortedItems.Count) | ||
| { | ||
| throw new ArgumentOutOfRangeException( | ||
| "index", string.Format("The index {0} does not exist in the ordered item list, valid range is 0 to {1}", index, this.sortedItems.Count - 1)); |
There was a problem hiding this comment.
please make use of string interpolation $ syntax instead of string.format
There was a problem hiding this comment.
If SortedItems doesn't contain any items, then the error states "valid range is 0 to -1".
CDP4Common/Types/OrderedItemList.cs
Outdated
| if (this.sortedItems.Values.Contains(item)) | ||
| { | ||
| var thing = item as Thing; | ||
| throw new InvalidOperationException(string.Format("The sorted list already contains the item {0}", thing != null ? thing.Iid.ToString() : "An item is not a Thing. Incorrect use of validation.")); |
There was a problem hiding this comment.
please make use of string interpolation $ syntax instead of string.format
Since the class is called OrderedItemList I propose that in the exception message we replace sorted with ordered
CDP4Common/Types/OrderedItemList.cs
Outdated
| if (index < 0 || index >= this.sortedItems.Count) | ||
| { | ||
| throw new ArgumentOutOfRangeException( | ||
| "index", string.Format("The index {0} does not exist in the ordered item list, valid range is 0 to {1}", index, this.sortedItems.Count - 1)); |
There was a problem hiding this comment.
If SortedItems doesn't contain any items, then the error states "valid range is 0 to -1".
CDP4Common/Types/OrderedItemList.cs
Outdated
| /// <exception cref="ArgumentNullException"> | ||
| /// Thrown when <see paramref="value"/> is null. | ||
| /// </exception> | ||
| private void ValidateValueForNull(object value, string message = "") |
There was a problem hiding this comment.
If you make the default value for message "An item cannot be null", you can remove it from 3 other places and it will be easier to reuse it for future changes.
Prerequisites
Description
Closes #82