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 59 forks source link

added removeSubsequentDuplicates to Extra.elm #97

Open jackhp95 opened 6 years ago

jackhp95 commented 6 years ago

This is my first submission to OSS!

I ran into a situation where I needed a function like this, and I couldn't find a function that would remove subsequent duplicates in this library or in the elm-core. So I made removeSubsequentDuplicates and I figured someone may find it useful.

I hope I handled things correctly, please let me know of things I could improve. Thank you!

pzp1997 commented 6 years ago

This seems like a good function to have, but I'm not crazy about the name. You should also write some tests for your function in list-extra/tests/Tests.elm.

I haven't done any benchmarking, but I have a feeling that the following foldr implementation is faster.

removeSubsequentDuplicatesHelper : a -> List a -> List a
removeSubsequentDuplicatesHelper x acc =
    case head acc of
        Just y ->
            if x == y then
                acc
            else
                x :: acc

        Nothing ->
            x :: acc

removeSubsequentDuplicates : List a -> List a
removeSubsequentDuplicates list =
    foldr removeSubsequentDuplicatesHelper [] list

Here's another possible implementation that is potentially even faster than the foldr implementation but not stack safe.

removeSubsequentDuplicates : List a -> List a
removeSubsequentDuplicates list =
    case list of
        x :: y :: ys ->
            if x == y then
                removeSubsequentDuplicates (y :: ys)
            else
                x :: removeSubsequentDuplicates (y :: ys)

        _ ->
            list

I think we should do some benchmarking to quantify how fast each option is before making a decision on which to use.

jackhp95 commented 6 years ago

Ah! Thanks for the quick feedback! I'll work on benchmarking and tests and I'll get back to you. :)

Chadtech commented 6 years ago

Thanks @jackhp95 and @pzp1997 !

Yeah I also am not crazy about the name. Reading it left me with the impression that all subsequent duplicates in the list were filtered, not just the immediate next elements. Maybe uniqueNeighbors? Just an idea.

And then, echoing what @pzp1997 said, if you could get the code working, add a test to our test file, and maybe benchmark a few alternative implementations to see which one is fastest. For bench marking you can use Ellie app. I always just copy and paste the code of an old bench mark we did for List.Extra and start from there, such as this one from another PR: https://ellie-app.com/5K7f2nGwsa1/1

Thanks for the submission! Congratulations on contributing to OSS. Let me know if you need any help.

pzp1997 commented 6 years ago

@jackhp95 Sorry, if I came off as rude earlier. I hope I didn't discourage you from contributing! Submitting PRs is a very rewarding process, and we are here to help you get your first one accepted. Please reach out with any questions.

Anyway, here's one more implementation for benchmarking. I expect it to be comparable to the other foldr implementation. It might even be slightly faster since the helper does not call head for each element in the list.

help : a -> ( a, List a ) -> ( a, List a )
help y ( x, xs ) =
    if x == y then
        ( x, xs )
    else
        ( y, x :: xs )

removeSubsequentDuplicates : List a -> List a
removeSubsequentDuplicates list =
    case list of
        [] ->
            []

        x :: xs ->
            uncurry (::) (foldr help ( x, [] ) xs)