Skip to content

Commit 4568efd

Browse files
author
Ahmad Alhour
committed
Fix critical bugs in SkipList and BinomialMinHeap, modernize tests
Bug fixes: - SkipList: Fix _getNextLevel() starting at 0 instead of 1, causing ~50% of inserted nodes to be lost (fixes #137, #138, #139, #140) - BinomialMinHeap: Fix double size increment in Add() causing incorrect count - BinomialMinHeap: Fix broken size accounting in _removeAtIndex() causing infinite loop due to integer overflow in Merge() Test modernization: - Replace Debug.Assert with Xunit.Assert across graph algorithm tests - Modernize data structure tests with focused test methods - Add comprehensive SkipList edge case tests - Update assertion styles to use Assert.Equal, Assert.Throws, Assert.Contains
1 parent c88b530 commit 4568efd

29 files changed

Lines changed: 3752 additions & 1977 deletions

DataStructures/Heaps/BinomialMinHeap.cs

Lines changed: 67 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -87,14 +87,20 @@ private void _removeAtIndex(int minIndex)
8787
// The min-root lies at _forest[minIndex]
8888
BinomialNode<T> deletedTreeRoot = _forest[minIndex].Child;
8989

90+
// Remove the tree from forest and decrement size by 1 (for the removed root)
91+
_forest[minIndex] = null;
92+
--_size;
93+
9094
// Exit if there was no children under old-min-root
9195
if (deletedTreeRoot == null)
9296
return;
9397

94-
// CONSTRUCT H'' (double-prime)
98+
// CONSTRUCT H'' (double-prime) from the children of the removed root
99+
// The children need to be merged back into the main heap
95100
BinomialMinHeap<T> deletedForest = new BinomialMinHeap<T>();
96-
deletedForest._forest.Resize(minIndex + 1);
97-
deletedForest._size = (1 << minIndex) - 1;
101+
deletedForest._forest.Resize(minIndex);
102+
// Set size to 0 because Merge will add it; we handle sizing separately
103+
deletedForest._size = 0;
98104

99105
for (int i = (minIndex - 1); i >= 0; --i)
100106
{
@@ -103,14 +109,65 @@ private void _removeAtIndex(int minIndex)
103109
deletedForest._forest[i].Sibling = null;
104110
}
105111

106-
// CONSTRUCT H' (single-prime)
107-
_forest[minIndex] = null;
108-
_size = deletedForest._size + 1;
112+
// Merge children back - but don't let Merge mess with our size
113+
// We need to manually handle the merge without size tracking
114+
_mergeForestOnly(deletedForest);
115+
}
109116

110-
Merge(deletedForest);
117+
/// <summary>
118+
/// Merges forest structure only, without modifying _size.
119+
/// Used internally by _removeAtIndex.
120+
/// </summary>
121+
private void _mergeForestOnly(BinomialMinHeap<T> otherHeap)
122+
{
123+
if (otherHeap == null || otherHeap._forest.Count == 0)
124+
return;
111125

112-
// Decrease the size
113-
--_size;
126+
BinomialNode<T> carryNode = null;
127+
128+
// Ensure capacity
129+
int maxCount = Math.Max(this._forest.Count, otherHeap._forest.Count) + 1;
130+
if (_forest.Count < maxCount)
131+
this._forest.Resize(maxCount);
132+
133+
for (int i = 0; i < maxCount; i++)
134+
{
135+
BinomialNode<T> treeRoot1 = (i < _forest.Count ? _forest[i] : null);
136+
BinomialNode<T> treeRoot2 = (i < otherHeap._forest.Count ? otherHeap._forest[i] : null);
137+
138+
int whichCase = (treeRoot1 == null ? 0 : 1);
139+
whichCase += (treeRoot2 == null ? 0 : 2);
140+
whichCase += (carryNode == null ? 0 : 4);
141+
142+
switch (whichCase)
143+
{
144+
case 0: /* No trees */
145+
case 1: /* Only this */
146+
break;
147+
case 2: /* Only otherHeap */
148+
this._forest[i] = treeRoot2;
149+
break;
150+
case 4: /* Only carryNode */
151+
this._forest[i] = carryNode;
152+
carryNode = null;
153+
break;
154+
case 3: /* this and otherHeap */
155+
carryNode = _combineTrees(treeRoot1, treeRoot2);
156+
this._forest[i] = null;
157+
break;
158+
case 5: /* this and carryNode */
159+
carryNode = _combineTrees(treeRoot1, carryNode);
160+
this._forest[i] = null;
161+
break;
162+
case 6: /* otherHeap and carryNode */
163+
carryNode = _combineTrees(treeRoot2, carryNode);
164+
break;
165+
case 7: /* all the nodes */
166+
this._forest[i] = carryNode;
167+
carryNode = _combineTrees(treeRoot1, treeRoot2);
168+
break;
169+
}
170+
}
114171
}
115172

116173
/// <summary>
@@ -209,10 +266,8 @@ public void Add(T heapKey)
209266
tempHeap._size = 1;
210267

211268
// Merge this with tempHeap
269+
// Note: Merge already updates _size, so we don't increment it here
212270
Merge(tempHeap);
213-
214-
// Increase the _size
215-
++_size;
216271
}
217272

218273
/// <summary>

DataStructures/Lists/SkipList.cs

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,17 @@ public class SkipList<T> : ICollection<T>, IEnumerable<T> where T : IComparable<
3131

3232
/// <summary>
3333
/// Private helper. Used in Add method.
34+
/// Returns a random level for a new node. Level is at least 1.
3435
/// </summary>
35-
/// <returns></returns>
36+
/// <returns>A random level between 1 and MaxLevel</returns>
3637
private int _getNextLevel()
3738
{
38-
int lvl = 0;
39+
// BUG FIX: Start at 1, not 0. A node with level 0 would have an empty
40+
// Forwards array and would never be linked into the skip list.
41+
// See: https://github.com/aalhour/C-Sharp-Algorithms/issues/137
42+
int lvl = 1;
3943

40-
while (_randomizer.NextDouble() < Probability && lvl <= _currentMaxLevel && lvl < MaxLevel)
44+
while (_randomizer.NextDouble() < Probability && lvl < _currentMaxLevel && lvl < MaxLevel)
4145
++lvl;
4246

4347
return lvl;

0 commit comments

Comments
 (0)