-
Notifications
You must be signed in to change notification settings - Fork 6
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.
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
base: master
Are you sure you want to change the base?
Conversation
sliceutil/sets.go
Outdated
if len(slices) == 1 { | ||
return slices[0] | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, while it's not part of the PR, might as well fix the unused-parameter
lint issues, it's only 4 lines and if you don't see them locally make sure to update your golangci-lint
😄
// 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 | ||
} |
There was a problem hiding this comment.
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:
// 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
{[]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}, |
There was a problem hiding this comment.
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
😊
No description provided.