First, I really enjoyed this, so please take that. It's lovely to see someone learn so much. This is the sort of code I try help my teammates write. Neat, organised and with clear optimisations.
I want to note a couple further optimisations though.
First, you could create the full dataset in a single reduce using the .add() method on Set. I don't know how Array.from on a Set is or a new Set on an array are implemented but if it isn't some recast of memory then you actually have a few implicit nested loops in this section.
Then, the .includes() test on currentRegions could be faster (and this might matter for large data sets) if the values in currentRegions were either a Set or stored as object keys. This would allow testing via .has() or .hasOwnProperty(), which are O(1) rather than O(n) lookups.
And this is less a matter of optimisation (indeed less optimal on memory but also not coupled to line location) than my taste for non-mutative code, but you can return directly from a reduce using something like:
I’m a web developer & data visualizer working at a think tank in D.C. I'm a self-taught dev trying to better my skills. I spend most of my time on the front end of the stack.
Location
Washington, D.C.
Work
Lead Developer at Center for Strategic and International Studies
Hey, thanks for the comments! I'm always happy to hear constructive criticism on how to make my code better. :)
If I'm understanding you correctly, you're suggesting creating the arrays of types, years, and regions within the .reduce function? That's not a bad idea, and you're right, would provide some optimization. I didn't explain this in the post, but part of my rationale for keeping it separate is to maintain separation of concerns. Those 3 lines are specifically about isolating the values of those variables within the dataset, while the .reduce accomplishes a separate task. But you make a good point and it's something to consider for future implementations!
I hadn't considered storing those values as a Set instead of an array, that's a great idea
Yep! However, I intentionally wrote the longer version for clarity. :) Certainly is worth considering how to make it more concise in production, though!
Re 1, it's not so much where it is but rather that I suspect Array.from loops and new Set loops and the map loops, which means 3 loops for each construction of those items.
That said, you could do it all in one reduce and keep the concerns separated with object keys, and then destructure them into variables in the outer scope.
I think re 3 you're talking about my non-mutation comment?
If you destructure the variables it can be terser, but my case is more for non-destructive operations. I don't like modifying things because it makes the original unavailable for later use without larger refactor. But again I'm an ideologue about this and it isn't common opinion.
For further actions, you may consider blocking this person and/or reporting abuse
We're a place where coders share, stay up-to-date and grow their careers.
First, I really enjoyed this, so please take that. It's lovely to see someone learn so much. This is the sort of code I try help my teammates write. Neat, organised and with clear optimisations.
I want to note a couple further optimisations though.
First, you could create the full dataset in a single reduce using the .add() method on Set. I don't know how Array.from on a Set is or a new Set on an array are implemented but if it isn't some recast of memory then you actually have a few implicit nested loops in this section.
Then, the .includes() test on currentRegions could be faster (and this might matter for large data sets) if the values in currentRegions were either a Set or stored as object keys. This would allow testing via .has() or .hasOwnProperty(), which are O(1) rather than O(n) lookups.
And this is less a matter of optimisation (indeed less optimal on memory but also not coupled to line location) than my taste for non-mutative code, but you can return directly from a reduce using something like:
return {...acc, [curr.type]: { [curr.year]: [...acc[curr.type][curr.year], ...curr] } }
But still bravo on a lot of this and thanks for writing your journey.
(Sorry to write without code highlighting.)
Hey, thanks for the comments! I'm always happy to hear constructive criticism on how to make my code better. :)
If I'm understanding you correctly, you're suggesting creating the arrays of types, years, and regions within the
.reducefunction? That's not a bad idea, and you're right, would provide some optimization. I didn't explain this in the post, but part of my rationale for keeping it separate is to maintain separation of concerns. Those 3 lines are specifically about isolating the values of those variables within the dataset, while the.reduceaccomplishes a separate task. But you make a good point and it's something to consider for future implementations!I hadn't considered storing those values as a Set instead of an array, that's a great idea
Yep! However, I intentionally wrote the longer version for clarity. :) Certainly is worth considering how to make it more concise in production, though!
Re 1, it's not so much where it is but rather that I suspect Array.from loops and new Set loops and the map loops, which means 3 loops for each construction of those items.
That said, you could do it all in one reduce and keep the concerns separated with object keys, and then destructure them into variables in the outer scope.
I think re 3 you're talking about my non-mutation comment?
If you destructure the variables it can be terser, but my case is more for non-destructive operations. I don't like modifying things because it makes the original unavailable for later use without larger refactor. But again I'm an ideologue about this and it isn't common opinion.