-
-
Notifications
You must be signed in to change notification settings - Fork 837
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
Feat: Adding the equivalent of checking if two slices are the same #498
base: master
Are you sure you want to change the base?
Conversation
Your tests are focusing on slices Did you take a look at this? |
intersect.go
Outdated
var m = make(map[T]int, l) | ||
for i := range collection { | ||
m[collection[i]] += 1 | ||
} | ||
for i := range subset { | ||
m[subset[i]] -= 1 | ||
} | ||
for _, v := range m { | ||
if v != 0 { | ||
return 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.
Your logic requires extra space in the map and also 3 times iterating over the map.
Why not just sort the 2 arrays and compare if both are the same ??
What are your thoughts ??
Also after adding the function, you can add the function to Readme also.
Look this PR as reference : #494
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.
Because just !=
or ==
is allowed on generics comparable, we can't sort this slice.
Additionally, sort.Slice() will change the parameter's original order, which I think is unsafe.
I have added doc and removed an unessential iterating based on your suggestion.
However, slice.Equal() return is effected by elements order. In my case, order is dosen't matter. |
Indeed, you may add this in the comment of method, especially how it differs from slice.Equal |
I have added the comments based on your suggestion. Additionally, I have modified the generic declaration to mirror slice.Equal. :) |
intersect.go
Outdated
@@ -44,6 +44,28 @@ func EveryBy[T any](collection []T, predicate func(item T) bool) bool { | |||
return true | |||
} | |||
|
|||
// Equivalent Returns true if the subset has the same elements and the same number of each element as the collection. | |||
func Equivalent[T comparable](collection, subset []T) bool { |
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.
Following #498 (comment)
func Equivalent[T comparable](collection, subset []T) bool { | |
func SameItems[T comparable](collection, subset []T) bool { |
Maybe this name would be more appropriate
Thanks @Dokiys for your first contribution I agree with @ccoVeille: this helper's name is not self-explaining. Some ideas: EqualUnorderedSlice, EqualUnordered ("slice" is implicit because maps cannot be ordered)... Also, can the last argument be converted into a vaarg? It might slow it down at some point. Feel free to argue against it if your benchmarks show it is a bad idea. |
1cc78ef
to
4450609
Compare
README.md
Outdated
@@ -196,6 +196,7 @@ Supported intersection helpers: | |||
- [ContainsBy](#containsby) | |||
- [Every](#every) | |||
- [EveryBy](#everyby) | |||
- [Equivalent](#equivalent) |
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.
You have to fix this
@samber @ccoVeille Thanks for your suggestions. I agree that using the name EqualUnordered would indeed be more appropriate. However, regarding the use of vaarg, I have reservations. This isn't based on performance issues. |
Often, I need to compare if two slices are same in elements and count of each element. However, I have not found an existing method that accomplishes this functionality. Therefore, I have added this method.