API convention for `reverse` versions of Belt functions

1

I’ve created a PR to add some functions to Belt.Array, mostly reverse functions of forEach, map, etc. In order not to bloat the API surface of Belt with mapReverse, forEachReverse etc, Hongbo suggests instead to add a flag to the existing functions.

Something like

let map: (array<'a>, ~reverse: bool=?, 'a => 'b) => array<'b>

Normal usage would be like before since the flag would be false by default, and the reverse version would be used like this:

let foo = bar->Belt.Array.map(x => x + 1, ~reverse=true)

What would you prefer? If you have other ideas, don’t hesitate to comment!

Preferred API:
  • add a reverse flag to existing functions
  • add extra functions (mapReverse, forEachReverse, etc)
  • other (please comment)

0 voters

2

Well, another possibility is Belt.Array.Reverse.map. Not sure it’s a good idea, just an idea :slight_smile:

1 Like
3

Something to think about with a reverse flag is that it would add a slight runtime to each function (regardless of whether or not the user enables reverse). It may be so small that it doesn’t matter in practice, but functions like Belt.Array.map are also used quite a lot. A slight performance impact may add up.

5 Likes
4

I agree it will add a slight performance impact, but probably nothing noticeable.

5

I like the extra module Array.Reverse with function that make sense. What is the reason for those functions by the way, why not reverse first and then map? (is this for performance reasons?)

6

Yes for performance reason, initially I had to write this for one of my projects where this was in a hot path, so I thought I could just add it to Belt. I also added joinWith. My idea is to make Belt as feature complete as possible so Js module is not needed anymore.

7

A Reverse module could indeed be a nice way to prevent bloating the Array namespace, the same could be used for WithIndex functions actually.

8

The cost of checking the reverse flag will always be O(1), nothing serious compared to traversing the array.

3 Likes
9

If the time complexity is a point of view, why do we even need it?
reverse first and then forEach to map in-place is also O(n+n)=O(n).

For me, The more important question is whether this is a good design decision.
map and reverse are better when they are decoupled.
reverse is nothing special than shuffle.

If this is the right decision, there’s no reason to not insist that we should include ~shuffle: bool?.

10

@namenu The same could be said about e.g.Belt.Array.keepMap, but I still prefer to use that over 2 separate function calls.
Why should I iterate over an array multiple times, when I can achieve the same result with only one iteration?

I haven’t had the usecase for reverse map yet myself. But I think supporting this usecase is a good thing: either by an additional argument or a new function.

11

Yeah, I thought that weird too.
(can someone point me the rationale for this?)
Besides, keepMap is poorly named since it’s not clear that the function will keep first or map first.

The only reason for their existence, I can guess, is performance.
Since Belt is not based on laziness, you have no choice but to iterate multiple times without such functions.

But I don’t think you would agree that we should supply all combinations in this sense.
My concern is weather reverse is general enough.

12

well it actually does both at the same time.

The only rationale I have for the reverse versions of map and forEach is indeed performance, it can be a nice utility function when you transform an array to a React array but in the reverse order.