Skip to content

Instantly share code, notes, and snippets.

@kbaribeau
Created August 24, 2012 16:48
Show Gist options
  • Save kbaribeau/3452778 to your computer and use it in GitHub Desktop.
Save kbaribeau/3452778 to your computer and use it in GitHub Desktop.
Ruby style poll
#Do you prefer this?
merchant["thing"] = important_stuff.map { |important_thing| important_thing["units"] }.inject(&:+)
#Or this?
important_stuff.each { |important_thing| merchant["thing"] += important_thing["units"] }
#Why? Is it completely objective? Or are ruby's idioms/social mores/conventions the deciding factor?
@xentek
Copy link

xentek commented Aug 24, 2012

First line is trying to be too clever and isn't immediately obvious what it's trying to do (the inject at the end isn't helping it's clarity). It's also a tad longer and possibly involves more ops than the the 2nd line which immediately jumped out to me as "I'm building merchant['thing'] up from some important things!"

@bbasata
Copy link

bbasata commented Aug 24, 2012

I like that the first lends itself to immutability of merchant["thing"] and it reads as a value assignment.

Possible adjustment:

merchant["thing"] = important_stuff.inject(identity) { |things, important_thing| things += important_thing["units"] }

@schoblaska
Copy link

I prefer the second simply because it is more readable. If you're using Rails / ActiveSupport, you may also consider:

merchant["thing"] = important_stuff.sum{|important_thing| important_thing["units"]}

@mikelikesbikes
Copy link

👍 to @bbasata's comment. The first one is trying to be too clever... no need for the map, just a reduce (ie. always reduce rather than inject).

@gfredericks
Copy link

I think the first one is easier to think about than @bbasata's because the reduce is much simpler (mostly for being commutative). When a non-commutative reduce can be expressed as a map and a commutative reduce, I like it.

@richievos
Copy link

I vote for:
#1 sum
#2 a pure inject
#3 the top one
#4 the bottom one

I think the top one is easier to follow. You're setting something to the result of something else. In the bottom one you have to look into the bits of the loop to realize there's a mutation going on.

# easy to see we're doing x = y
hash = {}
hash[:a] = [1, 2, 3, 4].map { |x| x + 1 }.inject(&:+)
hash[:b] = "asdf123"
hash[:c] = [2, 9, 8].map { |x| x * 2 }.inject(&:+)

# have to scan around to see what's going on
hash = { :a => 0 }
[1, 2, 3, 4].each { |x| hash[:a] += (x + 1) }
hash[:b] = "asdf123"
[2, 9, 8].each { |x| hash[:c] += x * 2 }.inject(&:+)

@richievos
Copy link

Forgot a :c => 0 in there

# easy to see we're doing x = y
hash = {}
hash[:a] = [1, 2, 3, 4].map { |x| x + 1 }.inject(&:+)
hash[:b] = "asdf123"
hash[:c] = [2, 9, 8].map { |x| x * 2 }.inject(&:+)

# have to scan around to see what's going on
hash = { :a => 0, :c => 0 }
[1, 2, 3, 4].each { |x| hash[:a] += (x + 1) }
hash[:b] = "asdf123"
[2, 9, 8].each { |x| hash[:c] += x * 2 }.inject(&:+)

@kbaribeau
Copy link
Author

I still like the .sum solution, but @fredericksgary's point is what makes me like #1 a little more than just an inject.

Is there something that makes a pure inject clearer than solution #1? Objectively, it's one loop vs two. You could argue that one loop is executing fewer instructions, but I think two simple loops is clearer than one (slightly) more complex one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment