Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New generics for slices #80

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

New generics for slices #80

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ab23d2
added new functions
michalPyciaTeamwork Jun 21, 2024
5cd10ab
review fix
michalPyciaTeamwork Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions sliceutil/sets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,21 @@ func Intersection[T comparable](a, b []T) []T {
}
return inter
}

// IntersectionOfMany returns the elements common to all arguments
func IntersectionOfMany[T comparable](slices ...[]T) []T {
if len(slices) == 1 {
return slices[0]
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably not necessary as you have common = slices[0] below and with continue on first item, it should return immediately after that, albeit it's not bad to have this here 🤔

On the other hand it might be worth checking if len(slices) == 0 and then return nil as otherwise the line below would panic 😅 Edge case, but if you have some sliceOfSlices = foo() followed by IntersectionOfMany(sliceOfSlices...) and the result was empty, you'd get an out of bounds panic

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ripexz, great caught!!!
i've rearranged a little bit, now if there are is no input we will return empty slice, this might be easier to handle when using IntersectionOfMany() as it will always return same type. We got this covered in tests with TestIntersectionOfMany():EmptyLists example

common := slices[0]
for i, s := range slices {
if i == 0 {
continue
}
common = Intersection(common, s)
if len(common) == 0 {
break
}
}
return common
}
31 changes: 31 additions & 0 deletions sliceutil/sets_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,37 @@ func TestIntersection(t *testing.T) {
}
}

func TestIntersectionOfMany(t *testing.T) {
type ciTest struct {
name string
a []int64
b []int64
c []int64
result []int64
}
tests := []ciTest{
{
name: "EmptyLists",
result: []int64{},
}, {
name: "three",
a: []int64{1, 2, 3},
b: []int64{3},
c: []int64{3, 4, 5},
result: []int64{3},
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
result := IntersectionOfMany(test.a, test.b)

if !reflect.DeepEqual(result, test.result) {
t.Errorf("result wrong\ngot: %#v\nwant: %#v\n", result, test.result)
}
})
}
}

func BenchmarkComplement_equal(b *testing.B) {
listA := []int64{1, 2, 3}
listB := []int64{1, 2, 3}
Expand Down
24 changes: 24 additions & 0 deletions sliceutil/sliceutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,16 @@ func Contains[T comparable](tt []T, item T) bool {
return false
}

// ContainsAny checks if slice contain element
func ContainsAny[T comparable](slice []T, elements ...T) bool {
for _, element := range elements {
if Contains(slice, element) {
return true
}
}
return false
}

// InFoldedStringSlice reports whether str is within list(case-insensitive)
func InFoldedStringSlice(list []string, str string) bool {
for _, item := range list {
Expand Down Expand Up @@ -201,3 +211,17 @@ func Values[T comparable, N any](tt []T, fn func(T) N) []N {

return ret
}

// IsSubset returns if all elements of 'slice' are in 'set'
func IsSubset[T comparable](slice, subset []T) bool {
subsetMap := make(map[T]bool, len(subset))
for _, v := range subset {
subsetMap[v] = true
}
for _, v := range slice {
if !subsetMap[v] {
return false
}
}
return true
}
Comment on lines +215 to +227
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This func right now is flipped (checking if slice is in subset rather than the other way round), something like this might work better:

Suggested change
// IsSubset returns if all elements of 'slice' are in 'set'
func IsSubset[T comparable](slice, subset []T) bool {
subsetMap := make(map[T]bool, len(subset))
for _, v := range subset {
subsetMap[v] = true
}
for _, v := range slice {
if !subsetMap[v] {
return false
}
}
return true
}
// IsSubset returns if all elements of 'slice' are in 'set'
func IsSubset[T comparable](set, subset []T) bool {
setMap := make(map[T]bool, len(set))
for _, v := range set {
setMap[v] = true
}
for _, v := range subset {
if !setMap[v] {
return false
}
}
return true
}

This is also why your tests are failing as the logic is basically flipped. Also even with the above change the last test would fail, however the expected value of false is incorrect as any set is considered a subset of itself.

Also considering this is working with sets, I'd move it to sets.go (and tests to sets_test.go) rather than the base sliceutil.go file.

Lastly, if you'd prefer a bit more readability/reuse over performance, you could simplify it to something like:

for _, v := range subset {
	if !Contains(set, v) {
		return false
	}
}
return true

45 changes: 45 additions & 0 deletions sliceutil/sliceutil_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,51 @@ func TestItemInSlice_String(t *testing.T) {
}
}

func TestContainsAny(t *testing.T) {
tests := []struct {
list []string
find string
find2 string
expected bool
}{
{[]string{"hello"}, "hello", "world", true},
{[]string{"hello"}, "hell", "world", false},
{[]string{"hello", "world", "test"}, "world", "potato", true},
{[]string{"hello", "world", "test"}, "", "potato", false},
{[]string{}, "", "", false},
}

for i, tc := range tests {
t.Run(fmt.Sprintf("test-%v", i), func(t *testing.T) {
got := ContainsAny(tc.list, tc.find)
if got != tc.expected {
t.Errorf(diff.Cmp(tc.expected, got))
}
})
}
}

func TestIsSubset(t *testing.T) {
tests := []struct {
set []int
subset []int
expected bool
}{
{[]int{1, 2, 3, 4}, []int{2, 3}, true},
{[]int{1, 2, 3, 4}, []int{2, 3, 8}, false},
{[]int{1, 2, 3, 4}, []int{1, 2, 3, 4}, false},
Comment on lines +340 to +342
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, last case should be true, I'd also add a couple more tests cases that would be common edge cases (empty slice, nil slice), and potentially move this func to sets_test.go 😊

}

for i, tc := range tests {
t.Run(fmt.Sprintf("test-%v", i), func(t *testing.T) {
got := IsSubset(tc.set, tc.subset)
if got != tc.expected {
t.Errorf(diff.Cmp(tc.expected, got))
}
})
}
}

func TestInFoldedStringSlice(t *testing.T) {
tests := []struct {
list []string
Expand Down
Loading