Why is pairs defined at the top, when it's only used for the final summation? This increases cognitive overhead, since we need to dig deeper to realize it's not being used in the reduce loop.
Why the reduce? We are mutating a single object by reference, and using reduce just forces us to thread the reference through.
Why the explicit boolean coercion? It relies on the same truthiness rules the conditional expression does.
Why a conditional expression? We aren't using the expression return value, we are performing side effects upon the acc object.
Why Object.keys if we only care about values, regardless of the color?
Why parseInt? It's slower and less idiomatic than Math.floor (I used |0 to make it "integer division")
Why the if condition? Even if we did get 0 sometimes, x+=0 is fine to do, and more functional.
My impression of this code is that it's an improved algorithm, but looks to be in the state of an unfinished refactor.
@theodesp
this was just me commenting on the code without changing the algorithm at all.
My own solution (which does use a counter) was posted separately:
This doesn't iterate again over the colors encountered, and uses the minimal memory required for the task: The running total of pairs and the unpaired colors, encountering which again would be the completion of a pair.
Hi Theofanis, thanks for adding to the discussion. I totally agree that the last reducer is not needed and when it comes to performance, the for loop is always going to outperform the functional methods for large datasets. What is more interesting is the possibility to make it in one pass and that's where I believe your solution shines the most.
I even took a stab at that approach in one of my previous solutions, see code below.
functionsockMerchant(n,ar){let_colors={};letpairs=0;for(leti=0;i<n;i++){_colors[ar[i]]=(!!_colors[ar[i]])?_colors[ar[i]]+1:1;// do it in one passif((_colors[ar[i]]%2)==0)pairs+=1;}returnpairs;}constn=9;constsocks=[10,20,20,10,10,30,50,10,20];console.group('Sorted and counted');console.log(`There is a total of ${sockMerchant(n,socks)} pairs`);console.groupEnd();
The difference with yours ultimately is that your colors value can only be 1, 2 or 0 which I believe for the context of this exercise - just counting the pairs - is perfect.
The one above since it's keep adding to the total of stocked socks, needs to do a division by 2 anytime a new sock is added.
I will of course run all against the benchmark and since JS Perf is back up and running, I'll put them all there and make a public share of the results for documentation purpose.
MM has come up with some JS wizardry here ... flipping those 1's and 0's ... I would'nt have come up with it myself, but hats off to him; it's a cracker!
I'd write the provided code as follows:
Comments:
pairs
defined at the top, when it's only used for the final summation? This increases cognitive overhead, since we need to dig deeper to realize it's not being used in the reduce loop.acc
object.Object.keys
if we only care about values, regardless of the color?parseInt
? It's slower and less idiomatic thanMath.floor
(I used |0 to make it "integer division")if
condition? Even if we did get0
sometimes,x+=0
is fine to do, and more functional.My impression of this code is that it's an improved algorithm, but looks to be in the state of an unfinished refactor.
I don't think you even need the last reducer. Just keep a running counter.
Most of the times it reports submillisecond cost with 10000 elements
Call to sockMerchant took 0.22000004537403584 milliseconds.
@Ady can you check this?
Nice solution!
@theodesp this was just me commenting on the code without changing the algorithm at all.
My own solution (which does use a counter) was posted separately:
My own solution would be:
This doesn't iterate again over the colors encountered, and uses the minimal memory required for the task: The running total of pairs and the unpaired colors, encountering which again would be the completion of a pair.
Hi Theofanis, thanks for adding to the discussion. I totally agree that the last reducer is not needed and when it comes to performance, the for loop is always going to outperform the functional methods for large datasets. What is more interesting is the possibility to make it in one pass and that's where I believe your solution shines the most.
I even took a stab at that approach in one of my previous solutions, see code below.
The difference with yours ultimately is that your colors value can only be 1, 2 or 0 which I believe for the context of this exercise - just counting the pairs - is perfect.
The one above since it's keep adding to the total of stocked socks, needs to do a division by 2 anytime a new sock is added.
I will of course run all against the benchmark and since JS Perf is back up and running, I'll put them all there and make a public share of the results for documentation purpose.
Cheers
Another hilarious one:
... and just when I thought it could not get better...
Sweet!
Nice indeed 😀
MM has come up with some JS wizardry here ... flipping those 1's and 0's ... I would'nt have come up with it myself, but hats off to him; it's a cracker!
I wonder if he plays code golf?
MM is afraid he doesn't.
I did golf a dweet once.
Oh! how cool is that!
I could watch that all day in fullscreen mode, but I wouldn't get anything else done.
Cheers for sharing.
It's surprisingly calming and on my big screen monitor truly awesome indeed
Oh! now your just boasting Ady ... how big?
Would be an excellent screen saver; don't you think?
boasting lol :) I've converted a 40" flat screen tv into a monitor. the image quality is exquisite and yes a great screen saver
Oh my!
That's what I would call ... total immersion!