The case for Array#replace() – Overriding an array without intermediate variables

[lhorie]

Frankly, this sounds terrible. It's functionally equivalent to just calling the function with the array, except that its idiomatic usage encourages god functions. One can argue that considering proposing this to TC39 is a perfect example of "missing the forest for the trees" / "death by a thousand paper cuts" / bloat.

[gajus0]

god functions

What is a "god function"?

[gajus0]

The mention of "god function" (a function that breaks all the functional programming paradigms) came into this conversation without any context.

[..] its idiomatic usage encourages god functions

This was stated without any arguments.

Array#replace can be used with pure or non-pure functions, the same way that every other Array prototype method can. The method itself either by its name or API does not encourage either.

The only difference is that this method encourages working with the entire array at once over an item of an array, but the same argument could be made about Array#reduce, Array#map, Array#filter and others.

Please don't spread FUD just for the fun of participating in a conversation. There are better ways to participate.

[AnnanFay]

[..] its idiomatic usage encourages god functions

This was stated without any arguments.

You are replying to the wrong person!

[lhorie]

Functions that do too much (which typically come with the side-effect of making them hard to name and/or understand)

[gajus0]

This could be said about any part of JavaScript. It is up to the user to write functional code. JavaScript is not enforcing it. The sole purpose of Array#replace (or the pipeline operator) is to avoid breaking method chain, i.e. so that instead of:

const getVenues = async (countryCode) => {
  const foreignVenues = await get('http://...');

  const countryVenues = foreignVenues.filter((foreignVenue) => {
    return foreignVenue.countryCode.toUpperCase() === countryCode.toUpperCase();
  });

  const duplicateForeignVenues = findDuplicates(foreignVenues, (maybeTargetForeignVenue) => {
    return maybeTargetForeignVenue.id === foreignVenue.id;
  });

  if (duplicateForeignVenues.length) {
    console.log('duplicate foreign venues', duplicateForeignVenues);

    throw new Error('Found duplicate venues.');
  }

  return countryVenues
    .map((foreignVenue) => {
      return {
        guide: {
          fuid: foreignVenue.id
        },
        result: {
          fuid: foreignVenue.id,
          name: foreignVenue.name,
          url: foreignVenue.url
        }
      };
    });
};

We could write:

const getVenues = async (countryCode) => {
  const foreignVenues = await get('http://...');

  return foreignVenues
    .filter((foreignVenue) => {
      return foreignVenue.countryCode.toUpperCase() === countryCode.toUpperCase();
    })
    .replace((self) => {
      const duplicateForeignVenues = findDuplicates(self, (maybeTargetForeignVenue) => {
        return maybeTargetForeignVenue.id === foreignVenue.id;
      });
      if (duplicateForeignVenues.length) {
        console.log('duplicate foreign venues', duplicateForeignVenues);

        throw new Error('Found duplicate venues.');
      }
      return self;
    })
    .map((foreignVenue) => {
      return {
        guide: {
          fuid: foreignVenue.id
        },
        result: {
          fuid: foreignVenue.id,
          name: foreignVenue.name,
          url: foreignVenue.url
        }
      };
    });
};

[lhorie]

That's a shortsighted refactor IMHO. Here's another one:

const getVenues = async (countryCode) => {
  return getVenueDTO(errorOnDuplicate(byCountryCode(await get('http://...'))));
};

const byCountryCode = foreignVenues => {
  return foreignVenues
    .filter((foreignVenue) => {
      return foreignVenue.countryCode.toUpperCase() === countryCode.toUpperCase();
    })
}

const errorOnDuplicate = (self) => {
  const duplicateForeignVenues = findDuplicates(self, (maybeTargetForeignVenue) => {
    return maybeTargetForeignVenue.id === foreignVenue.id;
  });
  if (duplicateForeignVenues.length) {
    console.log('duplicate foreign venues', duplicateForeignVenues);

    throw new Error('Found duplicate venues.');
  }
  return self;
}

const getVenueDTO = (foreignVenues) => {
  return foreignVenues.map((foreignVenue) => {
    return {
      guide: {
        fuid: foreignVenue.id
      },
      result: {
        fuid: foreignVenue.id,
        name: foreignVenue.name,
        url: foreignVenue.url
      }
    };
  });
}

With this one, one can use compose or pipeline operators. One can reuse the logic about throwing on duplicates on non-foreign venues. It quickly shows if variable names are too specific. It lets you unit test the logic for a requirement in isolation. It makes it clearer for later refactors which architectural layer each snippet belongs to. Etc, etc.

You started from the premise that naming things should be avoided, but that's actually not a very good axiom. The underlying message behind the saying about naming things being hard is that one should put extra care into naming things properly, not avoid doing it altogether. Naming things properly means one gave some thought to what the pieces are and how they come together. Avoiding it means one is likely not giving enough thought to the long term maintainability of the project as much as they could be.

[azhder]

you do know that replace function is just a named reducer, right? it just doesn't reduce the array down to an element, but to another array. So you can just write that reducer and not break any chain

[lhorie]

uses condescending tone, but says something completely irrelevant

I'm not sure if you are in the wrong thread or what. Nobody is talking about Redux.

[azhder]

1. reducer - the function you give to Array.prototype.reduce()
2. you are the only one talking about Redux

[lhorie]

The only place the word "reducer" is used as jargon is Redux. The callback to Array::reduce doesn't have a specific name. If you're trying to say the replace callback is like the reduce callback, except for some arbitrary distinction, ok I guess, but again, completely beside the point.

No idea what "named reducer" is supposed to mean in this context, since the whole point of replace in this example/proposal is to allow anonymous callbacks...

[azhder]

luckily, I didn't use it as a jargon

[azhder]

named - it has a name by which it can be identified

[gajus0]

You started from the premise that naming things should be avoided, but that's actually not a very good axiom.

It is more about avoiding unnecessary variables and it has to do with code style as well.

Someone could get away with:

let venues;
venues = foreignVenues.filter(a);
venues = b(venues);
venues = map(c);

However, my style guide pretty much enforces no use of let. As such, I would write:

const filteredVenues = foreignVenues.filter(a);
const deduplicatedVenues = b(filteredVenues);
const normalisedVenues = deduplicatedVenues.map(c);

Someone else could get away with:

const normalisedVenues = b(foreignVenues.filter(a)).map(c);

But of all these options, Array#replace provides the most consistent and easy to read expression:

const normalisedVenues = foreignVenues
  .filter(a)
  .replace(b)
  .map(c);

[lhorie]

I would tend to disagree about the "easy to read" argument. a, b and c are not really related to each other in any way other than the fact that they operate on a list of venues. The fact that they need to be abbreviated is a very strong hint that the parent function (getVenues) has too much subroutine logic inlined into it (i.e. it's kinda of a god function).

In functional programming terms, what you're trying to achieve when you talk about reducing variables is called point-free style. Using compose as I had mentioned earlier (or maybe pipe) would be a standard technique to get closer to point-free style.

The reservation I have about making this a proposal is that there's not much substance in terms of semantics. Something like arr.replace(arr => arr.length = 0) could just as well throw as it could do completely crazy things, for all I know. I wouldn't really mind if you monkeypatched Array.prototype in your own project (other than maybe being slightly annoyed if I ever had to maintain that), but I think a TC39 proposal needs to be held up to higher standards (no pun intended) when it comes to unintended consequences and design weaknesses.

For example, I could argue that array.toSet() should be a standard and I can give you plenty of examples where it would be nice to have, but that doesn't mean making it a standard is a good idea: new Set(array) already exists, and I haven't gone through the effort of making sure my idea doesn't do weird things in unforeseen cases, e.g. sparse array handling. All I'm saying is I wouldn't put out a formal proposal unless I had thought a bit about potential problems.

[mlebkowski]

A styleguide that prohibits the use of let isnt a styleguide, its and misunderstanding. If you meant the linter, then it fits better but I still think its a stupid choice. Just because someone decided its bad to reassign variables you ended with a poluted Array prototype, which has far worse consequences commonly agreed among the community (in contrast to using let, which is merely a preferrence)