Skip to content

Commit 2f60a0c

Browse files
author
Ahmad Alhour
committed
Fix SkipList sentinel node bugs (#138, #139)
- Fix bug #138: Empty SkipList<int>.Contains(0) incorrectly returned true because sentinel node has default(T) as its value - Fix bug #139: Remove(0) on list with negative ints corrupted the list by accidentally matching the sentinel node - Add regression tests for bugs #137, #138, #139 Fix: Check 'current == _firstNode' before value comparison in Find() and Remove() to prevent matching the sentinel node
1 parent 6c55575 commit 2f60a0c

2 files changed

Lines changed: 89 additions & 0 deletions

File tree

DataStructures/Lists/SkipList.cs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,15 @@ public bool Remove(T item, out T deleted)
179179

180180
current = current.Forwards[0];
181181

182+
// BUG FIX: Don't match the sentinel node (_firstNode).
183+
// The sentinel has default(T) as its value, which could match user queries.
184+
// See: https://github.com/aalhour/C-Sharp-Algorithms/issues/139
185+
if (current == _firstNode)
186+
{
187+
deleted = default(T);
188+
return false;
189+
}
190+
182191
// Return default value of T if the item was not found
183192
if (current.Value.IsEqualTo(item) == false)
184193
{
@@ -228,6 +237,16 @@ public bool Find(T item, out T result)
228237

229238
current = current.Forwards[0];
230239

240+
// BUG FIX: Don't match the sentinel node (_firstNode).
241+
// The sentinel has default(T) as its value, which could match user queries
242+
// (e.g., 0 for int, null for reference types).
243+
// See: https://github.com/aalhour/C-Sharp-Algorithms/issues/138
244+
if (current == _firstNode)
245+
{
246+
result = default(T);
247+
return false;
248+
}
249+
231250
// Return true if we found the element; false otherwise
232251
if (current.Value.IsEqualTo(item))
233252
{

UnitTest/DataStructuresTests/SkipListTest.cs

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,76 @@ public static void CopyTo_NullArray_ThrowsArgumentNullException()
432432

433433
#endregion
434434

435+
#region Bug Regression Tests
436+
437+
/// <summary>
438+
/// Bug #137: _getNextLevel() could return 0, causing items to not be added.
439+
/// Fixed by starting level at 1 instead of 0.
440+
/// </summary>
441+
[Fact]
442+
public static void Bug137_AddedElementIsAlwaysContained()
443+
{
444+
// Run multiple times to catch probabilistic failures
445+
for (int trial = 0; trial < 100; trial++)
446+
{
447+
var skipList = new SkipList<int>();
448+
var random = new Random(trial); // Different seed each trial
449+
450+
for (int i = 0; i < 10; i++)
451+
{
452+
int value = random.Next(-1000, 1000);
453+
skipList.Add(value);
454+
Assert.True(skipList.Contains(value),
455+
$"Trial {trial}: Added {value} but Contains returned false");
456+
}
457+
}
458+
}
459+
460+
/// <summary>
461+
/// Bug #138: Empty SkipList<int>.Contains(0) should return false.
462+
/// The sentinel node has default(T) as its value, which is 0 for int.
463+
/// The Contains method must not match the sentinel.
464+
/// </summary>
465+
[Fact]
466+
public static void Bug138_EmptyIntList_ContainsZero_ReturnsFalse()
467+
{
468+
var skipList = new SkipList<int>();
469+
470+
// Empty list should not contain anything, including 0
471+
Assert.False(skipList.Contains(0),
472+
"Empty SkipList<int> should not contain 0 (sentinel value issue)");
473+
}
474+
475+
/// <summary>
476+
/// Bug #139: Remove(0) on list with negative int should not remove sentinel.
477+
/// </summary>
478+
[Fact]
479+
public static void Bug139_RemoveZeroOnListWithNegatives_DoesNotCorrupt()
480+
{
481+
var skipList = new SkipList<int>();
482+
skipList.Add(-23);
483+
skipList.Add(-5);
484+
skipList.Add(-100);
485+
486+
// Attempt to remove 0 (which was never added)
487+
var result = skipList.Remove(0);
488+
489+
// Should return false - 0 was never added
490+
Assert.False(result, "Remove(0) should return false when 0 was never added");
491+
492+
// List should still be intact
493+
Assert.Equal(3, skipList.Count);
494+
Assert.True(skipList.Contains(-23));
495+
Assert.True(skipList.Contains(-5));
496+
Assert.True(skipList.Contains(-100));
497+
498+
// Enumeration should work
499+
var items = skipList.ToList();
500+
Assert.Equal(3, items.Count);
501+
}
502+
503+
#endregion
504+
435505
#region String Type Tests
436506

437507
[Fact]

0 commit comments

Comments
 (0)