-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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
proposal: x/exp/slices: Find function to select first element matching a condition #52006
Comments
There could be two functions, Find and FindSorted, where FindSorted expects a presorted slice and the function returns -1, 0, 1 like a typical cmp function so that the lookup is O(log N). |
|
@carlmjohnson That could prove useful, I imagine that in some cases the element at the index returned by @ianlancetaylor Compared to #50340, the |
Given that sort.Find is going to return an index, this function returning the value at an index should not be called Find. I'm still not sure we've established this is common enough. |
This proposal has been added to the active column of the proposals project |
I had intended to open this proposal myself today, nice timing!
Before: vars := p.Variables // []zedhook.Variable
i := slices.IndexFunc(vars, func(v zedhook.Variable) bool {
return v.Key == "HOME"
})
if i == -1 {
t.Fatal("HOME was not found in variables")
}
val := vars[i] After: vars := p.Variables // []zedhook.Variable
val, ok := slices.First(vars, func(v zedhook.Variable) bool {
return v.Key == "HOME"
})
if !ok {
t.Fatal("HOME was not found in variables")
}
Usage: opt, ok := pickFirst[*ndp.PrefixInformation](options) Before: func pickFirst[T ndp.Option](options []ndp.Option) (T, bool) {
for _, o := range options {
if t, ok := o.(T); ok {
return t, true
}
}
return *new(T), false
} After: func pickFirst[T ndp.Option](options []ndp.Option) (T, bool) {
return slices.First(options, func(o ndp.Option) bool {
_, ok := o.(T)
return ok
})
} Overall I am still unsure where to draw the line between writing a manual for loop or trying to experiment with new generic code. But I figured it was worth sharing my experiences if nothing else. |
@mdlayher Thank you for the additional examples and shared sentiment! @rsc Alternatives could be to:
What I like about A question we could ask ourselves is what would be more confusing: having both |
I hadn't read through #50340 before. Seeing it now makes me think, the binary search case is handled by that issue and slices.First is a good name for this. I think finding the first struct that meets some predicate is a fairly common operation. |
I like the proposal! However, I don't like the name |
FindAll is traditionally called “filter”. The name “Last” works about as well as “First”. |
FindAnything is not an option. sort.Find returns an index. This function returns a value. It can't be called FindSomething, because that would make people think it returns an index. It seems like we are looking at the difference between:
and
That is, there is still a required result check, and it still is fundamentally a statement. The win is entirely not repeating x[ ] around the result. It just doesn't seem like it comes up enough to be worth the complexity of adding new functions to the package. (And it's easily done in third-party packages.) |
Not sure if this would make the cut, but I went looking for a func Last[T any](xs T[]) T {
return xs[len(xs) - 1]
} EDIT: Apologies, I put this comment on the wrong thread. |
@rsc Thank you for your input! I find it doubtful naming it "Find" or @brandonbloom This would work as a separate proposal, as the intended functions, possibly named |
To me, First/Last element are only useful if they don't have a length check. There are frequently cases where you are happy to use the zero value if there is no first/last element, so jumping through hoops to get it is unhelpful. I think the point though of slices is to get a collection of functions that are either frequently used and/or somewhat tedious to write your own version of. This is drifting away from that ideal. There are a number of fiddly details, and the easiest way of getting little details right ends up being to just write it yourself as needed. |
@carlmjohnson I've unclearly expressed myself, as I agree with you. What I meant was to have that check inside |
Repeating what I said in #52006 (comment), it seems like we don't have a good name and it doesn't seem to save significant amounts of code. In particular it does not serve as an expression, and we already have code that works just as well as a statement. |
Based on the discussion above, this proposal seems like a likely decline. |
No change in consensus, so declined. |
When we want to retrieve an element that matches a given condition from a slice, with the current design of the
slices
package we useIndexFunc
:The issue with this solution is that many times we do not need the index of the value. In those cases, we either end up with a variable declaration and an otherwise unused index variable that both hamper readability, or we name the returned index suggestively, which results in lengthy names and clumsy access to the element. For example:
Both patterns can get really ugly when having to find elements that match other conditions, too. Either reuse an index variable, or name all of them accordingly. Besides, the
i == -1
(ori >= 0
, if preferred) check adds unnecessary cognitive load.To solve these readability and usability concerns, I propose adding a
Find
function to theslices
package, with the following declaration:The examples above become much more easier to reason about:
The
found
variable can safely be reused, as demonstrated by our usage of the "comma, ok" idiom. It could also be namedok
- this way,Find
would fit together with map lookups and type assertions, which would make the language more uniform.Here are some examples from open-source repositories showing how
Find
could simplify code:Before:
After:
Before:
After:
Before:
After:
In this case, I would have written the function like this:
and create the error message at the call site. This is actually very similar to the use case that made me write this proposal: in our codebase we have a
Store
interface with manyFindBy*
methods, which return the type and a boolean indicated whether the value was found or not.Before:
After:
We see that in most cases the zero value is actually helpful, which results in the found check being completely elided, in the same way we don't always check the
ok
value when using maps.IndexFunc
does not give us this benefit - indexing with -1 is an obvious panic, so we always have to check the index. Having theFind
function could even potentially eliminate all these helper methods shown here - usingFind
directly in code is pleasant and concise.Adding this function would bring us in line with languages like Scala, Rust and Javascript, too.
Note: This is not similar to #50340, as that proposal modifies the existing
BinarySearch
functions.The text was updated successfully, but these errors were encountered: