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

Add indexed fold functions #10

Closed elliotdickison closed 8 years ago

elliotdickison commented 8 years ago

I was missing the index parameter available in JavaScript's reduce and reduceRight. These changes add indexedFoldl and indexedFoldr to handle use cases where that parameter is helpful/necessary. These functions are identical to List.foldl and List.foldr except that the index of the current element being processed is passed to the reducer function along with the element itself.

jvoigtlaender commented 8 years ago

You forgot to export the functions from the module.

I would swap the order of the first and second arguments to the step function.

For both functions, simpler implementations seem possible by just combining "indexedMap (,)" and foldl/foldr.

elliotdickison commented 8 years ago

Woah thanks for the lightning fast response. Added the exports and switched the args (good call).

I did consider indexedMap + the fold functions, but went with the current implementation to avoid taking an extra pass over the list. The particular use-case that I first wrote these for involved fairly large lists, and I figured that the (albeit very slight) performance gain was worth the slightly more complex implementation under the hood.

jvoigtlaender commented 8 years ago

For indexedFoldr you do have two traversals in the currrent version as well, due to the List.length call. But I do get that it may still be more efficient than going via indexedMap.

elliotdickison commented 8 years ago

Interesting, I didn't know that (makes sense). I can't think of a way around it w/o dropping down to JS though (and even then it would be 2 traversals at best instead of the current 3, looking at the implementation of List.foldr). Thoughts?

jvoigtlaender commented 8 years ago

Let's hear what the maintainer of this package thinks. @abadi199

abadi199 commented 8 years ago

I think the current implementation is fine. If anyone can think of a better implementation, just make a new pull request. I'll merge this and publish the package.

abadi199 commented 8 years ago

Published! http://package.elm-lang.org/packages/elm-community/list-extra/3.1.0/ Thanks @elliotdickison for your contribution.