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

#reduce, #map and #join violate their Array intuitive meaning #10

Open
c69 opened this issue Mar 15, 2018 · 8 comments
Open

#reduce, #map and #join violate their Array intuitive meaning #10

c69 opened this issue Mar 15, 2018 · 8 comments

Comments

@c69
Copy link

c69 commented Mar 15, 2018

While filter, find, some or every play nicely with Set, and provide useful convenience.
Map, reduce and join have problems, which (in my point of view) warrant their exclusion from proposal.

  1. Array.map always produces array of same length.
    Set.map could potentially reduce the size of the set.
(new Set([1, 2, 3]).map(v => 1); // set of (1)
  1. Array.reduce is going from left to right, applying functor in order.
    Set.reduce will go by the "unique insertion order", which is not obvious and you cannot control (or update) it, as there is no #sort method on Set.
    Also, #reduce is not required to return same type as original collection, even more - aggregate is often desired to be of different type.

  2. Same as reduce, Array.join is joining elements in order. And "h,e,l,l,o" is very different from "o,e,l,h,l".
    Join returns string.

Suggestion: remove set#reduce and set#join methods - as the use cases involving them are trivially solved with [ ..mySet].reduce(fn, a)

For set#map - maybe consider different name..

@Ginden
Copy link
Collaborator

Ginden commented Mar 15, 2018

Set.map could potentially reduce the size of the set.

That's expected behaviour.

Set.reduce will go by the "unique insertion order", which is not obvious and you cannot control (or update) it, as there is no #sort method on Set.

That's why I argued that Set iteration should be entirely implementation-dependant long time ago, but it's expected behaviour to follow established convention for iterators and

Though, you shouldn't care about iteration order when you use Set.

Same as reduce, Array.join is joining elements in order. And "h,e,l,l,o" is very different from "o,e,l,h,l".

You shouldn't use Set if you care about ordering. And "I don't care about order, but I don't want duplicates" is common use case.

BTW, find depends on insertion order too.

new Set([1, 2, 3, 4]).find(e => e %2)

@c69
Copy link
Author

c69 commented Mar 16, 2018

"I don't care about order, but I don't want duplicates" is common use case.

indeed. Still what to do with the Set once we got rid of duplicates is up to developer.

find depends on insertion order too

true.

But back to join and reduce.

Main point is that all of them are redundant in Ecmascript.

In contrast to very needed union, intersect and difference, or more general filter, which have type of (this: Set) => Set. The methods I oppose to (join and reduce) all have the signature of (this: Set) => any, with emphasis on any. And my question is

  • why do we need to have methods on Set that duplicate array methods just to allow conversion from Set to any, if we already have a reasonably cheap (in both performance and syntax sense) way to convert set to array (via spread or Array.from) ? e.g.:
let mySet = new Set([1,2,3]);

[...mySet].find(v => v % 2);
mySet.find(v => v % 2);

[...mySet].reduce((a,v) => a*v, 1);
mySet.reduce((a,v) => a*v, 1);

[...mySet].join('|');
mySet.join('|');

For some, every and find, the justification could be that because those three do not need to always traverse the full collections, their performance could be better on Set, compared to use of intermediate array (as spread has to put all he elements in).

And extra points against join below.

Violated assumptions:

  • Array.join is a dual of String.split. But there is no String.splitToSet and there should not be.

Inconsistency:

  • Array.join is needed to "turn array elements into a string with custom separator maintaining their order", being something like configurable brother of Array.toString. But mySet.toString() and JSON.stringify(mySet) behave quite differently (and useless) from their Array counterparts and Array.join:
let a = new Set([1, '2', {x: 3}, [4]]); 
[...a].toString(); // "1,2,[object Object],4"
[...a].join(); // "1,2,[object Object],4"
JSON.stringify([...a]); // "[1,"2",{"x":3},[4]]"
a.toString(); // "[object Set]"
JSON.stringify(a); // "{}"

Limited usefulness:

  • If its members are not primitives, mySet.join() will return quite a useless string, unless your objects have overridden their toString method.
let b = new Set([{}, {a:42}, {}]);
[...b].join(); // "[object Object],[object Object],[object Object]"

TL;DR
if those methods are added, we gain almost nothing for the effort spent

@Ginden
Copy link
Collaborator

Ginden commented Mar 16, 2018

we already have a reasonably cheap (in both performance and syntax sense)

Well, compare this:

return set
    .map(fn1)
    .filter(fn2)
    .map(fn3);

To this:

return new Set([...new Set([...set].map(fn1)).filter(fn2)]).map(fn3);

Conversion Set => Array => Set isn't reasonably cheap in syntax sense.

The methods I oppose to (join and reduce) all have the signature of (this: Set) => any, with emphasis on any.

.join is always (this: Set) => string.

If its members are not primitives, mySet.join() will return quite a useless string

Array.prototype.join behaves the same way.

And such behaviour of Array.prototype.toString is a mistake that can't be fixed due to backwards compatibility.

@c69
Copy link
Author

c69 commented Mar 16, 2018

Your example of filter and map illustrates well why we need new methods with (this: Set) => Set. Maybe i was not clear enough, but i wholehartly support filter and map (with maybe different name).

But once discussion focuses on join and reduce, then syntactic cost becomes much lighter. Because join terminates the chain (by returning string), and reduce does not need extra parenthesis to return new Set, e.g.:

[...mySet].reduce ((a,v) => a.add(magic(v)), new Set()).add(1).delete(2).size;

Now, if you need chained reduce calls for some reason.... things become uglier:

[...[...mySet].reduce ((a,v) => a.add(magic(v)), new Set()).add(1)].reduce (f2).delete(2).size;

I can argue, though, that chained reduce is a rare case, and its hard to imagine real example which cannot be converted to [...mySet.filter(f1).filter(f2)].reduce(g)

But yes, - nested spread blocks are not pretty, just wandering how frequent such cases are.

@c69
Copy link
Author

c69 commented Mar 16, 2018

After reading more threads, with valid points about being able to conver their Sets (and Maps) to anything...

How about Set.fold (and i guess Map.fold) with nondeterministic order and type signature? :

 
function fold(this: Set, accumulator: any, value: any): any { 
}
  • no access to full collection (unless you pull it into closure)
  • no access to current element index
  • no assumptions about order
  • no confusion with reduce

@ljharb
Copy link
Member

ljharb commented Mar 16, 2018

I'm still not understanding what confusion there would be with "reduce" - fold and reduce do the same thing.

As for the "index", sets don't have an index, so likely just like forEach, the callback would either get (value, value) or just (value).

Assumptions about order are correct - Set and Map in JS are inherently ordered, by insertion order.

@c69
Copy link
Author

c69 commented Mar 16, 2018

@ljharb maybe you are right, and i am overdramatizing the "imaginary" developer's confusion.

So let's leave reduce out of the discussion in this thread.
And just focus on the join.

@dead-claudia
Copy link

@c69 .join(sep) is basically .reduce((acc, value) => acc == null ? value : acc + sep + value, null). Any deviation here would be unexpected, and if you provide .reduce, .join just logically follows from it.

IMHO, the only thing here that really raises questions is .map. I'm not convinced it has enough of a use case to warrant standalone inclusion on Set.prototype.

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

No branches or pull requests

4 participants