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.

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

Conversation

michalPyciaTeamwork
Copy link

No description provided.

Comment on lines 63 to 65
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

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

Copy link
Member

@ripexz ripexz left a 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 😄

Comment on lines +215 to +227
// 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
}
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

Comment on lines +340 to +342
{[]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},
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 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants