Browse files

Remove return, we are already returning self.

Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
  • Loading branch information...
1 parent 1b6b803 commit ac6e9447e521a6ab2e3b60acd00a1ae2b863329e @miloops miloops committed with spastorino Nov 25, 2010
Showing with 6 additions and 6 deletions.
  1. +6 −6 activerecord/lib/active_record/associations/association_collection.rb
View
12 activerecord/lib/active_record/associations/association_collection.rb
@@ -235,12 +235,12 @@ def destroy(*records)
# Removes all records from this association. Returns +self+ so method calls may be chained.
def clear
- return self if length.zero? # forces load_target if it hasn't happened already
-
- if @reflection.options[:dependent] && @reflection.options[:dependent] == :destroy
- destroy_all
- else
- delete_all
+ unless length.zero? # forces load_target if it hasn't happened already
+ if @reflection.options[:dependent] && @reflection.options[:dependent] == :destroy
@exviva
exviva added a note Nov 25, 2010

Wouldn't if @reflection.options[:dependent] == :destroy be enough? If it's nil or false, for sure it's not == :destroy, right?

@spastorino
Ruby on Rails member

Fixed here 438c018
Thanks.

@exviva
exviva added a note Nov 25, 2010

I've done some benchmarks and it seems that pre-checking for nil makes the whole expression faster if options[:dependent] is indeed nil? in more than 55% percent of the cases, which I suppose is probable since it's the default for @reflection.

Generally it seems that if hash[:key] && hash[:key] == :value is faster than if hash[:key] == :value, if you expect hash[:key] to be nil or false most of the time.

Anyway, it's a total micro-optimization, I was just rubycurious. At least the code is shorter now.

Here are the benchmark details:

require 'benchmark'

Benchmark.bmbm do |x|
  hash = {:dependent => :destroy}
  n = 1_000_000

  x.report('two "and" conditions, first false') { n.times { hash[:foo] && hash[:foo] == :destroy }}
  x.report('two "and" conditions, both true') { n.times { hash[:dependent] && hash[:dependent] == :destroy }}

  x.report('one condition, false') { n.times { hash[:foo] == :destroy }}
  x.report('one condition, true') { n.times { hash[:dependent] == :destroy }}
end

$ ruby benchmark.rb
(...)
                                      user     system      total        real
two "and" conditions, first false   0.350000   0.000000   0.350000 (  0.359705)
two "and" conditions, both true     0.490000   0.000000   0.490000 (  0.492608)
one condition, false                0.440000   0.000000   0.440000 (  0.450253)
one condition, true                 0.330000   0.000000   0.330000 (  0.345920)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ destroy_all
+ else
+ delete_all
+ end
end
self

0 comments on commit ac6e944

Please sign in to comment.