emmanueltouzery / prelude-ts

Functional programming, immutable collections and FP constructs for typescript and javascript
ISC License
377 stars 21 forks source link

Seq.indexOf and lastIndexOf proposal #34

Closed aleksei-berezkin closed 4 years ago

aleksei-berezkin commented 4 years ago

Here's my proposal which I'm ready to implement:

interface Seq<T> {
    indexOf(predicate: (v: T) => boolean): Option<number>;
    lastIndexOf(predicate: (v: T) => boolean): Option<number>;
}

This can be useful for example in model-view updates:

highlight(id: string) {
    items
        .indexOf(item => item.id === id)
        .map(index => views.get(index))
        .ifSome(view => view.highlight());
}

This can be made with zipWithIndex and find but that'll be quite verbose, especially searching for last which would require Seq reversing. Yet this may produce some overhead copying lists.

emmanueltouzery commented 4 years ago

Interesting! In principle I'm for it, I really rarely need indexOf, but sometimes it's needed, I can see that. With JS libraries for instance. So if you submit a PR, besides style, documentation, tests, I'll merge it.

But you mention it for Seq.. It should be only for Vector I guess, because performance on LinkedList would be bad. And index makes no sense on hashset.

In your specific example though, if views and items don't change much, you could zip them.

views.zip(items).find(i => i[1].id == id).ifSome(r => r[0].highlight())

But I don't know your problem well enough. If you make a PR it should probably call the underlying function from the 'list' library. Did you notice the apidoc examples get tested? Make sure to add a couple & make sure the tests pass.

Looking forward to your PR!

aleksei-berezkin commented 4 years ago

About performance: checked the code, yep, lastIndexOf() will require either a) deep recursion or b) tails unfold — so agreed, it's better to omit. indexOf() would be okay, though it's generally discouraged to use indices in linked list, so it's better to omit too. Surprised a bit with your mentioning HashSet, as I see it's not Seq (do I miss anything?). So, the only target will be Vector.

Thanks for mentioning apidoc tests, didn't notice them.

Yet, as we are speaking of searching, probably it's worth adding findLast() as a pair to find() into a Vector. There is such function in underlying list, so it's going to be straightforward. What do you think?

emmanueltouzery commented 4 years ago

Good point about hashset, you're right. I'm ok with findLast too. With linked list I just don't want to encourage working with indexes.

aleksei-berezkin commented 4 years ago

Well, it appears that list lacks the method for lastIndexOf(predicate). Created issue, hope they'll accept it too.

emmanueltouzery commented 4 years ago

Maybe you can make a PR here for what list already supports, then a followup one later for lastIndexOf?

aleksei-berezkin commented 4 years ago

Done. Closing this issue