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

func Find(ElementType) int #115

Open
elliotchance opened this issue May 9, 2019 · 10 comments
Open

func Find(ElementType) int #115

elliotchance opened this issue May 9, 2019 · 10 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@elliotchance
Copy link
Owner

elliotchance commented May 9, 2019

Find will return the index of the first element that matches, or -1 if none match.

@elliotchance elliotchance changed the title func Find(ElementType) []int func Find(ElementType) int May 9, 2019
@elliotchance elliotchance added the good first issue Good for newcomers label May 9, 2019
@danielpsf
Copy link
Contributor

danielpsf commented May 9, 2019

Hey there,

You mean something like First, but in this case, it will return the index of the first match instead of the element, right? Something like findIndex for JS (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/findIndex)

The way I read this it would be something like:

package main

import (
    "fmt"
    "strings"

    "github.com/elliotchance/pie/pie"
)

func main() {
    nameIdx := pie.Strings{"Bob", "Sally", "John", "Jane"}.
        Find(func (name string) bool {
            return name == "John"
        })

    fmt.Println("John found on position: " + nameIdx) // "2"
}

Or will it be something like find from JS? (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find)

I'm excited to help this pie to get bigger 😅, but I'll wait to hear back from you to submit a PR.

@elliotchance
Copy link
Owner Author

You are right. First and Last are better names.

They will take a value as the argument and return the respective index. When a callback is used instead of a value we append "Using", like:

nameIdx := pie.Strings{"Bob", "Sally", "John", "Jane"}.
    First("John")
nameIdx := pie.Strings{"Bob", "Sally", "John", "Jane"}.
    FirstUsing(func (name string) bool {
        return name == "John"
    })

I'm glad you are so excited to contribute! I'm looking forward to the PR.

@elliotchance
Copy link
Owner Author

Oops, I should have read my own documentation. First and Last are already taken. So FindFirst and FindLast are more appropriate.

@elliotchance elliotchance added the enhancement New feature or request label May 9, 2019
@danielpsf
Copy link
Contributor

Ops, do you want to make both FindFirst and FindLast part of the same PR or do you want me to update the description to say that I'm just adding FindFirst as part of #140 and not actually closing this feature request?

@zhiburt
Copy link
Contributor

zhiburt commented May 13, 2019

Hi everyone.

I looked into implementation and I came up one question to myself.
I consider that Find should find the first index but the first is not always the nearest to begin of slice, isn't it?
My question is about using binary search if slice is sorted.

I might be wrong, but even if I'm wrong you also could find index by binary search and move to left for the first.

@elliotchance
Copy link
Owner Author

@zhiburt: First, Index, IndexOf, etc in my experience means the lowest index for a match. The easiest way to do this is to check from 0th index and up.

Binary search would only be meaningful for sorted slices as you say, which is rarely the case, and so it doesn’t make sense to add the IsSorted overhead.

One other thing worth mentioning is that a binary search will also have to be aware of duplicate values to ensure that it always traverses to the lowest index on a match. It’s easiest and the most general case to just brute-force it.

@danielpsf: I’m totally fine with either.

@DylanMeeus
Copy link
Contributor

FindFirst and FindLas really do sound like they would return the item if it is found. This sounds much more like IndexOf.

@danielpsf
Copy link
Contributor

danielpsf commented May 17, 2019

@DylanMeeus, it makes sense, although Find already does what you said so it would be kinda weird to move forward this way.

Now that FindFirstUsing was already released with #140 (v1.27.0) I don't know if we could take it back and rename it to FindIndexUsing or FindFirstIndexUsing, like on loadash, right @elliotchance?

I'm still working on FindFirst, FindLast and FindLastUsing(expect PRs to arrive next week).

@elliotchance
Copy link
Owner Author

I know using "Index" in the name is common in other languages but that's really not necessary here because I would rather be explicit about the direction. The prototypes are:

func FindFirst(ElementType) int
func FindFirstUsing(fn(ElementType) bool) int
func FindLast(ElementType) int
func FindLastUsing(fn(ElementType) bool) int

Furthermore, there could also be:

func FindAll(ElementType) []int
func FindAllUsing(fn(ElementType) bool) []int

In my opinion, it's easier and more understandable to be consistent with the first word. "Find" means we are talking about returning indexes. If we wanted to return another type, such as all elements that might match we use a different prefix, like Filter, Match, etc.

@danielpsf
Copy link
Contributor

danielpsf commented May 18, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants