prateek3255 / blog

Prateek's digital home and blog.
https://prateeksurana.me
MIT License
11 stars 6 forks source link

blog/why-using-object-spread-with-reduce-bad-idea/ #20

Open utterances-bot opened 3 years ago

utterances-bot commented 3 years ago

Why using object spread with reduce probably a bad idea

Explore why using object spread with .reduce() can sometimes significantly affect the performance of your JavaScript apps and libraries.

https://prateeksurana.me/blog/why-using-object-spread-with-reduce-bad-idea/

nchambrier-iziwork commented 3 years ago

May I suggest to use Map for those usages?

users
  .filter((user) => user.active)
  .reduce((acc, user) => acc.set(user.id, user.name), new Map());

More readable, and more performant :)

eggplantiny commented 3 years ago

Very good article thx 😊 Is it okay if I translate this article and post it on my blog? I'll make sure to write down the source!

prateek3255 commented 3 years ago

Sure @eggplantiny go ahead!

pepkin88 commented 3 years ago

One liner in LiveScript:

result = {[..id, ..name] for users when ..active}
exdeniz commented 3 years ago

Best solution with Map() from @nchambrier-iziwork

Testing with 10000 items
Reduce with spread: 843.050ms
Reduce with mutation: 0.932ms
forEach: 0.662ms
Corrected Object.assign method from tweet: 28.715ms
Set Map: 0.337ms
joezimjs commented 3 years ago

Why the heck is anyone using spread there in the first place? Just assign the new property to the accumulator and return it. Also, I still prefer to use reduce vs your forEach alternative.

const result = arr.reduce((acc, user) => {
    if (user.active) acc[user.id] = user.name;
    return acc;
}, {});
prateek3255 commented 3 years ago

Why the heck is anyone using spread there in the first place?

@joezimjs I have seen many people use it and I think its probably to "avoid mutations" (which no benefit here since you own the object from inception to completion), and I guess it probably becomes a habit that people just apply mindlessly.

Also, I still prefer to use reduce vs your forEach alternative.

Yes, it is also equally performant, I also included it in the repl in the last section.

joezimjs commented 3 years ago

I also tried running through a filter instead of having the if block inside the reduce and even when I bumped it to 10k items it didn't seem to make a difference in speed to just the reduce (in Brave and Firefox):

arr.filter((user) => user.active)
  .reduce( (acc, user) => { acc[user.id] = user.name; return acc; }, {} );

Object.assign seems to be a little slow once you start getting 10k+ items in the array. And it's not because of the intermediate map call because I still had the same performance when converting it to this:

arr.filter((user) => user.active)
  .reduce((acc, user) => Object.assign(acc, { [user.id]: user.name }), {});

THIS is definitely "premature optimization", though, if you're not using large datasets.

l-cornelius-dol commented 3 years ago

Very well written and useful. Thanks.