Lack of clear alternative for reduceRight in no-array-reduce

[harrysarson]

I do not wish to re-litigate #623 but I am here as the rule triggered for Array#reduceRight after updating xo.

For Array#reduce there is a clear alternative which is to use a for (const ... of array) { ... } loop.

However, there is no clean way to iterate over an Array backwards, the best I can come up with is for (let i = array.length - 1; i >= 0; i--) { const ... = array[i]; ... } but in my (admittedly subjective) opinion having to use indicies is worse than the complexity introduced by reverseRight.

Rule: unicorn/no-array-reduce

The code in question looks a bit like:

const listEmpty = null;

const listCons = (element, list) => ({
  head: element,
  tail: list,
});

const listFromArray = (array) =>
  array.reduceRight((list, element) => listCons(element, list), listEmpty);

And I think that (the best alternative that I can think of)

const listFromArray = (array) => {
    let list = listEmpty;
    for (let i = array.length - 1; i >= 0; i--) {
        const element = array[i];
        list = listCons(element, list);
    }
    return list;
}

is worse because of how easy it is to make an off-by-one error with the indexing logic.

---

I am happy to (and have) used a splash of eslint-disable-next-line so happy for this to be closed but I thought it is worth sharing.

Of course it is possible that there is a much nicer way to iterated backwards over an array that I have missed. If there is it would be great to add it to the docs for unicorn/no-array-reduce which currently do not show an alternative for reduceRight.

[papb]

What about:

const listFromArray = (array) => {
  let list = listEmpty;
  for (const element of array.slice().reverse()) {
    list = listCons(element, list);
  }
  return list;
}

What do you think?

If performance is more important than readability here (which rarely is the case) you could profile this code, I don't know how much v8 can optimize here. If it happens to not optimize the extra allocations for .slice().reverse(), you can do a helper function to keep performance and readability:

function * makeBackwardsIterator(array) {
  for (let i = array.length - 1; i >= 0; i--) {
    yield array[i];
  }
}

const listFromArray = (array) => {
  let list = listEmpty;
  for (const element of makeBackwardsIterator(array)) {
    list = listCons(element, list);
  }
  return list;
}

[harrysarson]

The reverse method is nice but I don't have the motivation to benchmark it (even v8 isn't that smart is it?).

I think its easier to add the eslint-disable than the helper function in my case (my case is unusually function programming style for js so is probably uniquely suited to a reduce) but I agree it is probably the best of both worlds solution. Would a PR adding it to the docs be welcome?

[fisker]

👎 for .reverse(), it reverse the array, you don't even know the current order if you plan to reuse the array.

Even a new created array, I avoid to use .reverse() 63f52ef

[papb]

Ooops, great catch @fisker! That should have been .slice().reverse() instead. I will edit to avoid confusion.

[papb]

I think its easier to add the eslint-disable than the helper function in my case (my case is unusually function programming style for js so is probably uniquely suited to a reduce) but I agree it is probably the best of both worlds solution. Would a PR adding it to the docs be welcome?

I am not a maintainer of this repo but for what I know of @sindresorhus I'm pretty sure a PR would be very welcome, and I guess @fisker would agree :)

[fisker]

FYI: this rule is based on a twitter message, you can check to if there is better alternative https://twitter.com/jaffathecake/status/1213077702300852224, and we use loop ourself

eslint-plugin-unicorn/rules/utils/cartesian-product-samples.js

Line 18 in d707e83

for (let combinationIndex = combinations.length - 1; combinationIndex >= 0; combinationIndex--) {

[harrysarson]

I see no mention of reduceRight in the twitter thread though, only reduce. It is not 100% clear to me that the critism applies to both functions equally.

[sindresorhus]

If only https://github.com/tc39/proposal-iterator-helpers was a thing...

I think .slice().reverse() is fine for most cases with small amounts of elements. .reduce and .reduceRight are already slower than a for loop anyway.

[sindresorhus]

I see no mention of reduceRight in the twitter thread though, only reduce. It is not 100% clear to me that the critism applies to both functions equally.

It's not mentioned there because it's not a common need, but the same argument about readability apply.

[fregante]

The way exists now thanks to .toReversed()

const words = ['what?', 'is', 'name', 'my'];

❌

const sentence = words.reduceRight((accumulator, currentValue) => accumulator + ' ' + currentValue, '');

✅

let sentence = '';
for (const word of words.toReversed()) {
    sentence += word + ' ';
}

I'll open a PR to update the docs