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

added removeSubsequentDuplicates to Extra.elm #97

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jackhp95
Copy link

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
Copy link
Contributor

pzp1997 commented Apr 15, 2018

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
Copy link
Author

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

@Chadtech
Copy link
Collaborator

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
Copy link
Contributor

pzp1997 commented Apr 25, 2018

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants