Used any? instead of length call #3779

Merged
merged 1 commit into from Nov 28, 2011

Conversation

Projects
None yet
3 participants
@rahul100885
Contributor

rahul100885 commented Nov 28, 2011

No description provided.

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Nov 28, 2011

Contributor

Thanks but in general we don't do only style changes commits in Rails. This is to avoid a sequence of unnecessary pull requests that ends up replacing all entries of length > 0 by any? in Rails. That said, I am merging this one but following ones will be closed. Thanks!

Contributor

josevalim commented Nov 28, 2011

Thanks but in general we don't do only style changes commits in Rails. This is to avoid a sequence of unnecessary pull requests that ends up replacing all entries of length > 0 by any? in Rails. That said, I am merging this one but following ones will be closed. Thanks!

josevalim added a commit that referenced this pull request Nov 28, 2011

@josevalim josevalim merged commit 5cf56a4 into rails:master Nov 28, 2011

@rahul100885

This comment has been minimized.

Show comment Hide comment
@rahul100885

rahul100885 Nov 28, 2011

Contributor

Thanks for guidance will take care of it. Intension was to avoid two different call one for length and then for checking for > 0.

I am going through the commits and rails code. I have replace in one place but found that it is used in other places also so I have modified all files where it was.

Anyways really thanks for guidance.

Contributor

rahul100885 commented Nov 28, 2011

Thanks for guidance will take care of it. Intension was to avoid two different call one for length and then for checking for > 0.

I am going through the commits and rails code. I have replace in one place but found that it is used in other places also so I have modified all files where it was.

Anyways really thanks for guidance.

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 28, 2011

Member

@rahul100885 this benchmark does not seem to back up the micro-optimization

require 'benchmark'

TIMES = 1000000
Benchmark.bm(7) do |x|
  x.report(:length) {TIMES.times { %w(foo bar baz).length > 0}}
  x.report(:any) {TIMES.times { %w(foo bar baz).any? }}
end

__END__
              user     system      total        real
length    0.550000   0.000000   0.550000 (  0.546523)
any       0.860000   0.000000   0.860000 (  0.878242)

In addition to that, we are using a method call that it is not equivalent. I don't want to be thinking that if errors has items then they are true and so any? is checking for emptyness. length > 0 involves no extra brain cycles.

In summary I believe this is neither an optimization nor more readable.

Member

fxn commented Nov 28, 2011

@rahul100885 this benchmark does not seem to back up the micro-optimization

require 'benchmark'

TIMES = 1000000
Benchmark.bm(7) do |x|
  x.report(:length) {TIMES.times { %w(foo bar baz).length > 0}}
  x.report(:any) {TIMES.times { %w(foo bar baz).any? }}
end

__END__
              user     system      total        real
length    0.550000   0.000000   0.550000 (  0.546523)
any       0.860000   0.000000   0.860000 (  0.878242)

In addition to that, we are using a method call that it is not equivalent. I don't want to be thinking that if errors has items then they are true and so any? is checking for emptyness. length > 0 involves no extra brain cycles.

In summary I believe this is neither an optimization nor more readable.

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Nov 28, 2011

Contributor

The best alternatives here is to use empty? or present?. Given this
discussion, could someone please provide a commit reverting those changes
(or using empty? or present? instead).

Contributor

josevalim commented Nov 28, 2011

The best alternatives here is to use empty? or present?. Given this
discussion, could someone please provide a commit reverting those changes
(or using empty? or present? instead).

@rahul100885

This comment has been minimized.

Show comment Hide comment
@rahul100885

rahul100885 Nov 28, 2011

Contributor

@fxn thanks for your benchmark report. I never tried benchmark tool but from now I will try to use it.

Contributor

rahul100885 commented Nov 28, 2011

@fxn thanks for your benchmark report. I never tried benchmark tool but from now I will try to use it.

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 28, 2011

Member

@rahul100885 yes it is fundamental for these kind of decisions. Compilers and interpreters often defy our intuitions. Always better measure than guess.

Member

fxn commented Nov 28, 2011

@rahul100885 yes it is fundamental for these kind of decisions. Compilers and interpreters often defy our intuitions. Always better measure than guess.

@rahul100885

This comment has been minimized.

Show comment Hide comment
@rahul100885

rahul100885 Nov 28, 2011

Contributor

Sure @fxn

Contributor

rahul100885 commented Nov 28, 2011

Sure @fxn

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 28, 2011

Member

@rahul100885 oh by the way, I see your readability point in having a predicate instead of length > 0. But the problem here is that any? reads like English, but it does not mean the same as in English, there's the nuance of being true in Ruby. The empty? predicate on the other hand allows you to have a really equivalent alternative.

Member

fxn commented Nov 28, 2011

@rahul100885 oh by the way, I see your readability point in having a predicate instead of length > 0. But the problem here is that any? reads like English, but it does not mean the same as in English, there's the nuance of being true in Ruby. The empty? predicate on the other hand allows you to have a really equivalent alternative.

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Nov 28, 2011

Member

@josevalim done

Member

fxn commented Nov 28, 2011

@josevalim done

@rahul100885

This comment has been minimized.

Show comment Hide comment
@rahul100885

rahul100885 Nov 28, 2011

Contributor

@josevalim Revert as you have asked

#3781

Contributor

rahul100885 commented Nov 28, 2011

@josevalim Revert as you have asked

#3781

@josevalim

This comment has been minimized.

Show comment Hide comment
@josevalim

josevalim Nov 28, 2011

Contributor

@josevalim done

tks, i reverted the other!

Contributor

josevalim commented Nov 28, 2011

@josevalim done

tks, i reverted the other!

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