Inject in constantize #5208

Merged
merged 1 commit into from Feb 28, 2012

Conversation

Projects
None yet
3 participants
Contributor

homakov commented Feb 28, 2012

Contributor

homakov commented Feb 28, 2012

Just fixing that verbose pull request - now it is 1 proper commit, right?

tenderlove merged commit f07a957 into rails:master Feb 28, 2012

Owner

fxn commented Feb 29, 2012

Well, the commit message says there's one less variable, but the number of variables stays the same right?

The number of operations superficially seems smaller, but you can't really compare because it depends on the implementation of inject.

Indeed, for nestings of size 2 or 3 the each version is a little bit faster

require 'benchmark'

module A
  module B
    module C
    end
  end
end

def with_each(names)
  constant = Object
  names.each do |name|
    constant = constant.const_get(name, false)
  end
  constant
end

def with_inject(names)
  names.inject(Object) do |constant, name|
    constant.const_get(name, false)
  end
end

NAMES = %w(A B C)
Benchmark.bm do |bm|
  bm.report(:each)   { 10000.times { with_each(NAMES) } }
  bm.report(:inject) { 10000.times { with_inject(NAMES) } }
end
__END__
fxn@cauchy:~/tmp $ ruby1.9 foo.rb
       user     system      total        real
each  0.020000   0.000000   0.020000 (  0.014777)
inject  0.020000   0.000000   0.020000 (  0.019295)

I don't mind the commit, not a fan of inject but it is fine. But I'd like to point out this is a stylistic/abstraction refactor rather than what the commit message says.

Contributor

homakov commented Feb 29, 2012

Yes, surely, by commit message i didn't mean any boost. It is only matter of taste and ruby style.
Inject implementation surely makes variable - it is obvious, but you know, 10.times looks better than for(i=0; i<10;i+=1) and now the similar case.
BTW i'm sorry for using 'inject' work, which is used mostly by rubyists. In my opinion synonym 'reduce' suits better due to prevalence in other languages and in MapReduce term

Owner

fxn commented Feb 29, 2012

Yeah, you will find rubyists who dislike inject and prefer the more obvious each loop, and you won't find rubyists that prefer a for loop over 10.times. So on this basis that is more debatable.

But it is fine, I don't mind the change in style, no big deal, wanted only to point out that the refactor moves constant to the block as a parameter. It is a stylistic refactor and the commit message says a different thing.

Contributor

homakov commented Feb 29, 2012

Deal. You live in barce - you know the meaning of word 'beauty' way better:)
(i enjoy park guall and sagrada soooo much)

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