3 ms·

Unless things have changed and Ruby has stream fusion now, this is bad advice for scale. You are iterating over a fat object multiple times. Even if its uglier

Unless things have changed and Ruby has stream fusion now, this is bad advice for scale. You are iterating over a fat object multiple times. Even if its uglier its much better in this case to create an empty array/hash, iterate over with #each and #<< to the hash. I worked at the largest Rails shop in the world and this would be rejected in code review. Edited to add more detail: the only method you need to write to implement Enumerable is #each. Every step of your pipeline here is _another_ call to #each. Just do it once.

lloeki 3y ago
There's #lazy to turn things into a lazy enumerator, to be iterated over when you so desire with e.g #force. If you're going to iterate over an accumulator variable, use the for keyword instead of each, it's faster. Alternatively, one can use .reduce({}) { |h, (k, v)| ... h } or .each.with_object({}) { |(k, v), h| ... } which makes the block not close over an external variable, and makes the assignment to that variable "atomic" (wrt the hash construction, the variable will only contain the final result, that is if a final variable is needed at all, which it may not with implicit return of the last value)
mikemcquaid 3y ago
> I worked at the largest Rails shop in the world and this would be rejected in code review. Not sure if this means GitHub or Shopify. Until earlier this year I worked at GitHub for a decade, leaving as a principal engineer, primarily writing Ruby. This would not be rejected at code review there unless the Hash had e.g. millions of values and, even then, it might not be a meaningful performance problem in context. If the Hash is very small and will always be: readability trumps Big-O "performance" when n is very small. Am I being pedantic and appealing to authority? Yup but, well, you started it and I hate to see helpful "Ruby is nice" comments like the grandparent get crapped on for no good reason.

lloeki 3y ago

There's #lazy to turn things into a lazy enumerator, to be iterated over when you so desire with e.g #force. If you're going to iterate over an accumulator variable, use the for keyword instead of each, it's faster. Alternatively, one can use .reduce({}) { |h, (k, v)| ... h } or .each.with_object({}) { |(k, v), h| ... } which makes the block not close over an external variable, and makes the assignment to that variable "atomic" (wrt the hash construction, the variable will only contain the final result, that is if a final variable is needed at all, which it may not with implicit return of the last value)

mikemcquaid 3y ago

> I worked at the largest Rails shop in the world and this would be rejected in code review. Not sure if this means GitHub or Shopify. Until earlier this year I worked at GitHub for a decade, leaving as a principal engineer, primarily writing Ruby. This would not be rejected at code review there unless the Hash had e.g. millions of values and, even then, it might not be a meaningful performance problem in context. If the Hash is very small and will always be: readability trumps Big-O "performance" when n is very small. Am I being pedantic and appealing to authority? Yup but, well, you started it and I hate to see helpful "Ruby is nice" comments like the grandparent get crapped on for no good reason.