Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inject in constantize #5208

Merged
merged 1 commit into from Feb 28, 2012
Merged

Inject in constantize #5208

merged 1 commit into from Feb 28, 2012

Conversation

homakov
Copy link
Contributor

@homakov homakov commented Feb 28, 2012

@homakov
Copy link
Contributor Author

homakov commented Feb 28, 2012

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

tenderlove added a commit that referenced this pull request Feb 28, 2012
@tenderlove tenderlove merged commit f07a957 into rails:master Feb 28, 2012
@fxn
Copy link
Member

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.

@homakov
Copy link
Contributor Author

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

@fxn
Copy link
Member

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.

@homakov
Copy link
Contributor Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants