From afb5ecc67138af573ac65932aeed428c2bbc1664 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 26 Sep 2026 04:24:01 +0000 Subject: [PATCH] fix: make OrderedCollection.IndexOf and Remove find the first occurrence [patch] IndexOf and Remove were built on BinarySearch, which returns whichever equal element the midpoint lands on. With duplicates, IndexOf(1) on [1,1,1] returned 1 rather than the documented first occurrence, and Remove could drop a different element with the same key under a key-based comparer. Both now use a lower-bound search. Remove prefers the element in the equal-key run that also Equals the argument, falling back to the first. BinarySearch keeps its any-match contract for insertion, and its doc now says so. Fixes ktsu-dev/Containers#62 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01UHW69XTeuuLbfVdQdJ8uQh --- Containers.Test/OrderedCollectionTests.cs | 59 +++++++++++++++++++ Containers/OrderedCollection.cs | 72 ++++++++++++++++++++--- 2 files changed, 122 insertions(+), 9 deletions(-) diff --git a/Containers.Test/OrderedCollectionTests.cs b/Containers.Test/OrderedCollectionTests.cs index d4c5065..70b01f6 100644 --- a/Containers.Test/OrderedCollectionTests.cs +++ b/Containers.Test/OrderedCollectionTests.cs @@ -593,4 +593,63 @@ public void Constructor_WithNonComparableType_WithoutComparer_ThrowsArgumentExce Assert.ThrowsExactly(() => new OrderedCollection(10)); Assert.ThrowsExactly(() => new OrderedCollection([])); } + + [TestMethod] + public void IndexOf_OddLengthDuplicateRun_ReturnsFirstOccurrence() + { + // Arrange + OrderedCollection collection = [1, 1, 1]; + + // Act & Assert + Assert.AreEqual(0, collection.IndexOf(1)); + } + + [TestMethod] + public void IndexOf_EvenLengthDuplicateRun_ReturnsFirstOccurrence() + { + // Arrange + OrderedCollection collection = [0, 2, 2, 2, 2, 3]; + + // Act & Assert + Assert.AreEqual(1, collection.IndexOf(2)); + } + + [TestMethod] + public void Remove_KeyComparerWithDuplicateKeys_RemovesTheElementPassedIn() + { + // Arrange + IComparer<(int Key, string Name)> byKey = Comparer<(int Key, string Name)>.Create((x, y) => x.Key.CompareTo(y.Key)); + OrderedCollection<(int Key, string Name)> collection = new(byKey) + { + (1, "a"), (1, "b"), (1, "c"), (1, "d"), (0, "zero"), (2, "two"), + }; + + // Act + bool removed = collection.Remove((1, "c")); + + // Assert + Assert.IsTrue(removed); + Assert.HasCount(5, collection); + Assert.DoesNotContain((1, "c"), collection.ToList()); + } + + [TestMethod] + public void Remove_KeyComparerWithNoExactMatch_RemovesFirstOccurrence() + { + // Arrange + IComparer<(int Key, string Name)> byKey = Comparer<(int Key, string Name)>.Create((x, y) => x.Key.CompareTo(y.Key)); + OrderedCollection<(int Key, string Name)> collection = new(byKey) + { + (0, "zero"), (1, "a"), (1, "b"), (1, "c"), (1, "d"), (2, "two"), + }; + List<(int Key, string Name)> expected = [.. collection]; + expected.RemoveAt(1); + + // Act + bool removed = collection.Remove((1, "missing")); + + // Assert + Assert.IsTrue(removed); + Assert.AreSequenceEqual(expected, collection); + } } diff --git a/Containers/OrderedCollection.cs b/Containers/OrderedCollection.cs index 55d735d..ee4d199 100644 --- a/Containers/OrderedCollection.cs +++ b/Containers/OrderedCollection.cs @@ -256,17 +256,32 @@ public void CopyTo(T[] array, int arrayIndex) /// true if the element was found and removed; otherwise, false. /// /// This operation uses binary search to locate the element and has O(n) time complexity - /// due to the need to shift elements after removal. + /// due to the need to shift elements after removal. When several elements compare equal to + /// , the first of them that also equals it is removed, so a key-based + /// comparer does not cause a different element with the same key to be removed. If none of + /// them equals it, the first element that compares equal is removed. /// public bool Remove(T item) { - int index = BinarySearch(item); - if (index >= 0) + int first = FindFirst(item); + if (first < 0) { - items.RemoveAt(index); - return true; + return false; + } + + int index = first; + EqualityComparer equality = EqualityComparer.Default; + for (int i = first; i < items.Count && comparer.Compare(items[i], item) == 0; i++) + { + if (equality.Equals(items[i], item)) + { + index = i; + break; + } } - return false; + + items.RemoveAt(index); + return true; } /// @@ -286,9 +301,14 @@ public void RemoveAt(int index) /// /// The element to search for. /// - /// The zero-based index of the element if found; otherwise, a negative number that is the - /// bitwise complement of the index where the element should be inserted. + /// The zero-based index of an element that compares equal to if found; + /// otherwise, a negative number that is the bitwise complement of the index where the element + /// should be inserted. /// + /// + /// When the collection holds several elements that compare equal to , + /// the index of any one of them may be returned. Use for the first. + /// public int BinarySearch(T item) { int left = 0; @@ -323,10 +343,44 @@ public int BinarySearch(T item) /// The zero-based index of the first occurrence if found; otherwise, -1. public int IndexOf(T item) { - int index = BinarySearch(item); + int index = FindFirst(item); return index >= 0 ? index : -1; } + /// + /// Binary searches for the leftmost element that compares equal to the specified element. + /// + /// The element to search for. + /// The index of the first matching element, or -1 if there is none. + private int FindFirst(T item) + { + int left = 0; + int right = items.Count - 1; + int found = -1; + + while (left <= right) + { + int mid = left + ((right - left) / 2); + int comparison = comparer.Compare(items[mid], item); + + if (comparison == 0) + { + found = mid; + right = mid - 1; + } + else if (comparison < 0) + { + left = mid + 1; + } + else + { + right = mid - 1; + } + } + + return found; + } + /// /// Returns an enumerator that iterates through the collection in sorted order. ///