elm-community / list-extra

Convenience functions for working with List.
http://package.elm-lang.org/packages/elm-community/list-extra/latest
MIT License
135 stars 58 forks source link

findMap #82

Closed gergely-marko closed 3 years ago

gergely-marko commented 6 years ago

A short helper to avoid applying the mapper function on every element in the list, in cases where only the first match is needed. Basically a shortcut to list |> List.filterMap |> List.head

Chadtech commented 6 years ago

Hey @gergely-marko

Im having a hard time understanding the purpose of this function. Did you find yourself using this function? If so, what were you trying to do?

Furthermore I dont think you need your findMap function to avoid List.filterMap >> List.head, consider..

case List.Extra.find .isModerator users of
    Just moderator ->
        toAdmin moderator

    Nothing ->
        newModerator
gergely-marko commented 6 years ago

Hi,

yes, I do use this function. This is the same as List.filter vs List.filterMap You apply a mapping in the predicate to get the final result in one step.

I use it mostly in cases where the list is of some union type. This way I don't have to pattern match again on the result of the find.

type Page
    = HomePage HomePageData
    | LoginPage LoginPageData
    | ProjectPage ProjectPageData

findMapHomePage : List Page -> Maybe HomePageData
findMapHomePage pages =
    pages
        |> findMap
            (\page ->
                case page of
                    HomePage homePageData ->
                        Just homePageData

                    _ ->
                        Nothing
            )

findHomePage : List Page -> Maybe HomePageData
findHomePage pages =
    pages
        |> find
            (\page ->
                case page of
                    HomePage _ ->
                        True

                    _ ->
                        False
            )
        |> Maybe.andThen
            (\page ->
                case page of
                    HomePage homePageData ->
                        Just homePageData

                    _ ->
                        Nothing
            )
Chadtech commented 6 years ago

You could also do

findHomePage : List Page -> Maybe HomePageData
findHomePage pages =
    case pages of
        HomePage data :: _ ->
            Just data

        _ :: rest ->
            findHomePage rest

        [] ->
            Nothing

What do you think?

Also is that your actual use case? Its not clear to me why a Page would be stored in a list.

Sorry, Im just trying to exhaust all practical considerations.

gergely-marko commented 6 years ago

findMap is the generic solution of your findHomePage example.

I use findMap sometimes for the exact same reason as I use filterMap sometimes. To apply a mapping in the predicate. I just don't want to write a fold-like recursive function every time I need find or findMap

Pages in a List was just an example. I can imagine Pages with their actual state stored in a List to be useful. E.g. the content of this list depends on the role of the logged in user. Admin has all the pages, a simple user has no UsersPage for example.

gergely-marko commented 6 years ago

This is your package, I don't mind if you don't merge my PR.

I just wrote this function because I needed it as a generic solution. I was searching for the type annotation in elm-search and found the very similar find function. I thought it might be useful for others that's why I created the PR. I think find and findMap in List.Extra would be just nicely in parallel with filter and filterMap in List.

Chadtech commented 6 years ago

Alright lets include it. findMap is really no weirder than any of the other special maps we have in List-Extra or even List core.

pzp1997 commented 6 years ago

I don't see how findMap is any clearer or easier than using find followed by andThen. I think that in the vast majority of cases, findMap will just lead to less readable code.

Chadtech commented 6 years ago

Yeah Im sorry for going back and forth on this. @pzp1997 expresses my thoughts.

MattCheely commented 6 years ago

FWIW, I have a use case for this, and find the findMap version of the implementation clearer. Briefly, it's an ordered list of entries in a menu, where some items are a category that lead to a deeper branching selection, and some items are an entity that can be selected directly.

findCategoryById : List PresenceOrCategory -> String -> Maybe Presence.Category
findCategoryById presenceOptions id =
    presenceOptions
        |> find
            (\item ->
                case item of
                    Category category ->
                        if category.id == id then
                            True
                        else
                            False

                    Instance _ ->
                        False
            )
        |> Maybe.andThen
            (\theCategory ->
                case theCategory of
                    Category category ->
                        Just category

                    Instance _ ->
                        Nothing
            )

For someone coming to the code above for the first time, the redundant pattern match in Maybe.andThen makes it look like there are more decisions being made than actually happen in practice. Aditionally, the case check in andThen introduces an extra conditional step where a clumsy refactor or mistake could result in a function that type checks, but only ever returns Nothing. I'd be reluctant to ship that implementation and would probably fall back to the recursive implementation or using List.filter and List.head if there are reasonable bounds on the data set.

The find map implemenation below has only the logical conditionals required for the application logic. There are no redundant steps that introduce an opportunity to negate the decision made when finding the target element. I don't follow the argument that the first example is clearer, given that there are strictly fewer branching paths in the second example, and the branching paths that are common to the two examples are identical.

findCategoryById : List PresenceOrCategory -> String -> Maybe Presence.Category
findCategoryById presenceOptions id =
    presenceOptions
        |> findMap
            (\item ->
                case item of
                    Category category ->
                        if category.id == id then
                            Just Category
                        else
                            Nothing

                    Instance _ ->
                        Nothing
            )

This isn't a deal-breaker by any means, but I do think findMap makes for a clearer implementation. The next best approach is probably direct use of recursion, IMO, but I like to keep recursion out of application code where I can. Junior developers are often uncomfortable with it, and it adds some mental overhead during implementation and refactoring to ensure that the recursive calls can benefit from TCO.

zwilias commented 6 years ago

Having a function category : PresenceOrCategory -> Maybe Presence.Category would make this a lot easier to read, I think:

findCategoryById : List PresenceOrCategory -> String -> Maybe Presence.Category
findCategoryById presenceOptions id =
    presenceOptions
        |> find (\item -> category item |> Maybe.map (\cat -> cat.id == id) |> Maybe.withDefault False)
        |> Maybe.andThen category

category : PresenceOrCategory -> Maybe Presence.Category
category item =
    case item of
        Category category ->
            Just category

            Instance _ ->
                Nothing

In other words, find without findMap pushes you to write helper functions, which doesn't seem like a bad thing.

Chadtech commented 6 years ago

Heres just another idea, more vanilla.

findCategoryById : List PresenceOrCategory -> String -> Maybe Presence.Category
findCategoryById presenceOptions id =
    presenceOptions
        |> find (byId id)
        |> Maybe.andThen category

byId : String -> PresenceOrCategory -> Bool
byId id presceneOrCategory =
    case category presceneOrCategory of
        Just category ->
            category.id == id

        Nothing ->
            False

category : PresenceOrCategory -> Maybe Presence.Category
category item =
    case item of
        Category category ->
            Just category

        Instance _ ->
            Nothing

It really seems like you cant go wrong recommending people (including junior devs) to write more helper functions.

zwilias commented 6 years ago

(sidenote - if <bool> then <bool> else <bool> is a longwinded way for saying <bool> or not <bool>)

Chadtech commented 6 years ago

Oh jeez, thank you!

MattCheely commented 6 years ago

(sidenote - if bool then bool else bool is a longwinded way for saying bool or not bool)

Yup. I noticed that thinko after I posted the example. Chalk it up to a hasty writeup. That said, I think the point about it the two alternatives having the same logical flow still stands, despite the uncessary if expression.

Creating a separate category function does resolve my primary concern, which is the potential to create conflicting pattern matches. I'm not sure I'm convinced that find without findMap actually pushes folks to write more helper functions as opposed to just making it trickier to implement the equivalent of findMap in a robust way, given how long this thread went before that suggestion came up. That said, the implementation you proposed is pretty nice, @Chadtech. It reads clearly and resolves the issues from my original implementation.

Thanks for the suggestions!

gergely-marko commented 6 years ago

A note to the example of @MattCheely, solved without findMap: I still find it really strange to pattern match on presenceOptions twice. If you put that pattern match to a helper function, it still stays an unneeded pattern match, because you already know the consturctor of that type.

zwilias commented 6 years ago

So then, an alternative that doesn't require pattern matching twice: presenceOptions |> List.filterMap category |> find (\cat -> cat.id == id)

The tradeoff is time complexity of O (n + m) for n presenceOptions of which m are categories.

If performance is a potential issue, my recommendation is to use a Dict String Category rather than a single list conflating two types being used for lookups.


On a side-note, pattern-matching more than once, even on the same thing, isn't that special - it happens basically anytime you chain map, andThen or andMap on a Maybe, a Result, a List or any other union type. In some cases, you can use function composition so prevent that, but for anything other than long lists, that's usually a premature performance optimization.

gergely-marko commented 6 years ago

I started the thread with stating, that this function is to remove the unnecessary iterating of the whole list, when only the first occurance is needed. Your example just illustrates, how similiar pairs find - findMap and filter - filterMap are.

Following your logic, even find is not needed, because it could be solved via List.map |> List.head

I wrote this function not because of performance as the issue, but clarity and simplicity. If you use map or andThen on Maybe or List, then most of the cases you don't need to pattern match on the same thing again, I think this is the power of the different map functions.

Keyamoon commented 6 years ago

I'm in favor of adding findMap. I encountered many scenarios where I needed to use something like this function; mainly for performance reasons and to avoid using an expensive function twice, once for finding the element and once again for mapping it. Without this function, you'd have to resort to writing your own recursive function each time, which is fine but when it happens a dozen times, it makes sense to abstract it away.

Chadtech commented 6 years ago

I think you are over stating findMap. For instance, you say you dont have to rewrite a recursive function 20 times, but you still have to write a function that returns a Maybe a 20 times, which accounts for almost all of the space. The actual difference, as I see it, is do you want to write List.Extra.findMap 20 times, or case .. of 20 times. Theres no obvious winner in that comparison

I made a benchmark comparison, and findMap and recursion are almost identical in their performance: https://ellie-app.com/dgy6KZVcMa1/1

There seems to be no performance difference, and theres no significant difference in how much code needs to get written. The difference comes entirely down whether its nicer to do things with one recursive function, or in two explicitly different functions (the find map function, and the find map helper function). The code I have seen with this findMap approach doesnt look prettier, and it seems to encourage bad practices like big messy anonymous functions.

I guess, trying to be really open minded about this, if you needed to dynamically generate different a -> Maybe a functions, and performance was a big constraint, it would make sense to do findMap, but Im not really sure thats anyones use case. To me, it seems like some of us just naturally find this code easier to think about from a find perspective, and findMap is a natural extension of find.

That said, maybe we should include findMap, just because its so basic. I will think about this more and come back to it. On the other hand, it really doesnt seem obviously better, and arguably worse, and I am not inclined to merge additions that wont improve the code bases that use List.Extra.

Like always, I am curious for more thoughts and opinions on this. And I appreciate your interest in this @Keyamoon .

showell commented 4 years ago

I really don't feel strongly about this, because I needed something akin to findMap only one time, and I was able to write it pretty quickly. But I do think there's a use case for doing this:

My use case was basically similar to parser.oneOf, except I was working with parsers that weren't directly from Parser.

ludat commented 3 years ago

I just wanted this function and I had to implement it myself.

TLDR: My usecase was similar to what's been already discussed and I think using findMap makes sense when the function passed is very context dependent and if it were extracted as top level it would have a too specific name that never actually used again.

In my case I'm writing a type checker which is build on a list of relations between different types in a program, for example one of these could be Equal (NumberType) (TypeVariable "a") which means that the type variable "a" should be Number type.

The typechecker also has records so in the case I find a Equal (Record content) (TypeVariable n) I need to find the first record that matches that type variable and also has a record and keep the record content, to do things with it (the real case is even more complex since Subtype is implemented as well as Equal)

So here I'd end up with a function getRecordThatSubtypesATypeVariable which takes a variable but it's never used again since it's so very specific.

I think these kinds of trade-offs are very common in FP, named vs anonymous functions and being able to decide if it's worth naming.

lydell commented 3 years ago

In F# this function is in the standard library and is called List.tryPick. I searched through our code base at work and got 27 usages over 4 different services. I find it useful and have missed it in Elm since I got used to it in F#. Using List.filterMap f >> List.head for now.

Chadtech commented 3 years ago

@lydell Would you mind sharing some of your F# and Elm use cases?

gergely-marko commented 3 years ago

This thread is getting really funny. This is from the rust std: find_map<B, F>(&mut self, f: F) -> Option<B> Applies function to the elements of iterator and returns the first non-none result.

lydell commented 3 years ago

@chadtech Sure, here are some examples!

Elm

Getting the first assignee:

timeline
    |> List.filterMap
        (\commit ->
            case commit.event of
                Timeline.Assigned assignee ->
                    Just assignee

                _ ->
                    Nothing
        )
    |> List.head

Finding whether a case has been confirmed and if so with which caseId:

actions
    |> List.filterMap
        (\action ->
            case action of
                Actions.Confirm (Actions.Done caseId) ->
                    Just caseId

                _ ->
                    Nothing
        )
    |> List.head

Finding the first file upload error (if any) for displaying:

files
    |> List.filterMap
        (\(FileToUpload fileToUpload) ->
            case fileToUpload.uploadStatus of
                UploadFailed err ->
                    Just err

                _ ->
                    Nothing
        )
    |> List.head

F

Turns out basically all usages of tryPick are related to finding the first (and usually only) event of a certain kind in an event sourced system and extracting something from it:

let private getContact : List<Event.CaseEvent> -> Option<Event.Contact> =
    List.tryPick (function
        | Event.Claim payload -> Some payload.contact
        | _ -> None)
Chadtech commented 3 years ago

Okay. Lets add this function. Thanks for you patience. @gergely-marko would you like to update this PR?

In the last three years (three years!) I do feel like I have been in more situations where I "pluck" a data type out of a big data type into a smaller isolated variant of it via -> Maybe a. In some use-cases, that a -> Maybe b function is really salient to what you are doing, so it makes a lot of sense that you encapsulate that step into one a -> Maybe b function and then re-use it everywhere.

Its not really any more concise code, but having distinct findMap and a -> Maybe b seems to follow along the natural conceptual parts of the process.

I found this Rust snippet on github with find_map in it and it makes a lot of sense to me.

    pub fn port(&self) -> Option<&OutputLabel> {
        self.modifiers.iter().find_map(|md| match md {
            PressModifier::Port(lbl) => Some(lbl),
            _ => None,
        })
    }
    pub fn channel(&self) -> Option<MidiChannel> {
        self.modifiers.iter().find_map(|md| match md {
            PressModifier::Channel(c) => Some(*c),
            _ => None,
        })
    }
    pub fn velocity(&self) -> Option<PressVelocity> {
        self.modifiers.iter().find_map(|md| match md {
            PressModifier::Velocity(v) => Some(*v),
            _ => None,
        })
    }
    pub fn duration(&self) -> Option<WaitTime> {
        self.modifiers.iter().find_map(|md| match md {
            PressModifier::Duration(d) => Some(*d),
            _ => None,
        })
    }
Chadtech commented 3 years ago

I just merged findMap into master on my own. I couldnt work off this branch, I think because gergely-marko/list-extra no longer exists?

Thanks everyone for your input! This PR was open for a long time, but I'm really pleased we could do this in a slow and thoughtful way.