Remove slice.RemoveAll. Add additional test cases for Remove after mo…

…re chaos testing
mailgun · Dec 11, 2023 · 8d8e898 · 8d8e898
1 parent 438df1f
commit 8d8e898
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 110 deletions.
diff --git a/slice/remove.go b/slice/remove.go
@@ -6,33 +6,18 @@ package slice
 // Remove modifies the contents of the given slice and performs no allocations
 // or copies.
 func Remove[T comparable](slice []T, i, j int) []T {
-	// Nothing to do
-	if len(slice) == 0 {
+	// Nothing to do for emtpy slices or starting indecies outside range
+	if len(slice) == 0 || i > len(slice) {
 		return slice
 	}
 	// Prevent invalid slice indexing
-	if i < 0 || j > len(slice)-1 {
+	if i < 0 || j > len(slice) {
 		return slice
 	}
+	// Removing the last element is a simple re-slice
+	if i == len(slice)-1 && j >= len(slice) {
+		return slice[:i]
+	}
 	// Note: this modifies the slice
 	return append(slice[:i], slice[j:]...)
 }
-
-// RemoveAll removes all instances of the given value in the given slice, in place, preserving element order.
-// RemoveAll modifies the contents of the given slice.
-// If value is not present in the slice, or if the slice is nil or empty, the slice is returned unaltered.
-// RemoveAll performs no allocations or copies.
-// Note: DO NOT use this for long-lived slices as the elements removed are not garbage collected
-func RemoveAll[T comparable](slice []T, value T) []T {
-	// Nothing to do
-	if len(slice) == 0 {
-		return slice
-	}
-	b := slice[:0]
-	for _, v := range slice {
-		if v != value {
-			b = append(b, v)
-		}
-	}
-	return b
-}
diff --git a/slice/remove_test.go b/slice/remove_test.go
@@ -19,12 +19,6 @@ func ExampleRemove() {
 	// [1 3]
 }
 
-func ExampleRemoveAll() {
-	s := []int{1, 2, 3, 2, 4, 2}
-	fmt.Println(slice.RemoveAll(s, 2))
-	// Output: [1 3 4]
-}
-
 func TestRemove(t *testing.T) {
 	for _, test := range []struct {
 		name     string
@@ -37,102 +31,64 @@ func TestRemove(t *testing.T) {
 		inSlice:  []int{},
 		outSlice: []int{},
 	}, {
-		name:     "out of range should not panic or modify the slice",
-		i:        4,
-		j:        5,
+		name:     "nil check",
+		i:        1,
+		j:        3,
+		inSlice:  nil,
+		outSlice: nil,
+	}, {
+		name:     "start index out of range should not panic or modify the slice",
+		i:        -2,
+		j:        2,
 		inSlice:  []int{2},
 		outSlice: []int{2},
 	}, {
-		name:     "correct start index but invalid end index",
+		name:     "end index out of range should not panic or modify the slice",
 		i:        0,
-		j:        2,
+		j:        5,
+		inSlice:  []int{2},
+		outSlice: []int{2},
+	}, {
+		name:     "invalid start and end index should not panic or modify the slice",
+		i:        4,
+		j:        7,
 		inSlice:  []int{2},
 		outSlice: []int{2},
 	}, {
 		name:     "single value remove",
 		i:        0,
 		j:        1,
-		inSlice:  []int{2},
-		outSlice: []int{},
+		inSlice:  []int{1, 2, 3},
+		outSlice: []int{2, 3},
 	}, {
 		name:     "middle value removed",
 		i:        1,
 		j:        2,
 		inSlice:  []int{1, 2, 3},
 		outSlice: []int{1, 3},
 	}, {
-		name:     "multi-value remove",
-		i:        1,
+		name:     "last value removed",
+		i:        2,
 		j:        3,
 		inSlice:  []int{1, 2, 3},
-		outSlice: []int{1},
+		outSlice: []int{1, 2},
 	}, {
-		name:     "nil check",
+		name:     "multi-value remove",
 		i:        1,
 		j:        3,
-		inSlice:  nil,
-		outSlice: nil,
-	}, {}} {
-		t.Run(test.name, func(t *testing.T) {
-			got := slice.Remove(test.inSlice, test.i, test.j)
-			for i, expected := range test.outSlice {
-				assert.Equal(t, expected, got[i])
-			}
-		})
-	}
-}
-
-func TestRemoveAll(t *testing.T) {
-	for _, test := range []struct {
-		name     string
-		remove   string
-		inSlice  []string
-		outSlice []string
-	}{{
-		name:     "empty slice",
-		remove:   "localhost",
-		inSlice:  []string{},
-		outSlice: []string{},
-	}, {
-		name:     "nil check",
-		remove:   "localhost",
-		inSlice:  nil,
-		outSlice: nil,
-	}, {
-		name:     "only value should be removed",
-		remove:   "localhost",
-		inSlice:  []string{"localhost"},
-		outSlice: []string{},
-	}, {
-		name:     "beginning of list",
-		remove:   "localhost",
-		inSlice:  []string{"localhost", "foo"},
-		outSlice: []string{"foo"},
-	}, {
-		name:     "end of list",
-		remove:   "localhost",
-		inSlice:  []string{"foo", "localhost"},
-		outSlice: []string{"foo"},
-	}, {
-		name:     "middle of the list",
-		remove:   "localhost",
-		inSlice:  []string{"foo", "localhost", "bar"},
-		outSlice: []string{"foo", "bar"},
-	}, {
-		name:     "all instances should be removed",
-		remove:   "localhost",
-		inSlice:  []string{"localhost", "foo", "localhost"},
-		outSlice: []string{"foo"},
+		inSlice:  []int{1, 2, 3},
+		outSlice: []int{1},
 	}, {
-		name:     "order and repeats shouldn't matter",
-		remove:   "localhost",
-		inSlice:  []string{"foo", "localhost", "bar", "localhost", "localhost", "baz.com"},
-		outSlice: []string{"foo", "bar", "baz.com"},
+		name:     "end of slice gut check for invalid ending index",
+		i:        2,
+		j:        5, // This index being out of bounds shouldn't matter
+		inSlice:  []int{1, 2, 3},
+		outSlice: []int{1, 2},
 	}} {
 		t.Run(test.name, func(t *testing.T) {
-			out := slice.RemoveAll(test.inSlice, test.remove)
-			for i, expected := range test.outSlice {
-				assert.Equal(t, expected, out[i])
+			got := slice.Remove(test.inSlice, test.i, test.j)
+			for i := range test.outSlice {
+				assert.Equal(t, test.outSlice[i], got[i])
 			}
 		})
 	}
@@ -150,14 +106,3 @@ func BenchmarkRemove(b *testing.B) {
 	}
 	out = o
 }
-
-func BenchmarkRemoveAll(b *testing.B) {
-	o := []string{}
-	in := []string{"localhost", "mg.example.com", "testlabs.io", "localhost", "m.example.com.br", "localhost", "localhost"}
-
-	b.ResetTimer()
-	for i := 0; i < b.N; i++ {
-		o = slice.RemoveAll(in, "localhost")
-	}
-	out = o
-}