Refactor the implementations of AssociatioCollection#delete and #destroy to be more consistent with each other, and to stop passing blocks around, thus making the execution easier to follow.

5 files changed, 15 insertions, 19 deletions

diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index acb9fe7ff8..3e3237c348 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -178,10 +178,7 @@ module ActiveRecord
       # are actually removed from the database, that depends precisely on
       # +delete_records+. They are in any case removed from the collection.
       def delete(*records)
-        remove_records(records) do |_records, old_records|
-          delete_records(old_records) if old_records.any?
-          _records.each { |record| @target.delete(record) }
-        end
+        delete_or_destroy(records, @reflection.options[:dependent])
       end
 
       # Destroy +records+ and remove them from this association calling
@@ -190,12 +187,8 @@ module ActiveRecord
       # Note that this method will _always_ remove records from the database
       # ignoring the +:dependent+ option.
       def destroy(*records)
-        records = find(records) if records.any? {|record| record.kind_of?(Fixnum) || record.kind_of?(String)}
-        remove_records(records) do |_records, old_records|
-          delete_records(old_records, :destroy) if old_records.any?
-        end
-
-        load_target
+        records = find(records) if records.any? { |record| record.kind_of?(Fixnum) || record.kind_of?(String) }
+        delete_or_destroy(records, :destroy)
       end
 
       def create(attrs = {})
@@ -450,21 +443,24 @@ module ActiveRecord
           add_record_to_target_with_callbacks(record, &block)
         end
 
-        def remove_records(*records)
+        def delete_or_destroy(records, method)
           records = records.flatten
           records.each { |record| raise_on_type_mismatch(record) }
+          existing_records = records.reject { |r| r.new_record? }
 
           transaction do
             records.each { |record| callback(:before_remove, record) }
-            old_records = records.reject { |r| r.new_record? }
-            yield(records, old_records)
+
+            delete_records(existing_records, method) if existing_records.any?
+            records.each { |record| @target.delete(record) }
+
             records.each { |record| callback(:after_remove, record) }
           end
         end
 
         # Delete the given records from the association, using one of the methods :destroy,
-        # :delete_all or :nullify. The default method used is given by the :dependent option.
-        def delete_records(records, method = @reflection.options[:dependent])
+        # :delete_all or :nullify (or nil, in which case a default is used).
+        def delete_records(records, method)
           raise NotImplementedError
         end
 
diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
index d70f326e55..b0ccab2de6 100644
--- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
@@ -40,7 +40,7 @@ module ActiveRecord
           load_target.size
         end
 
-        def delete_records(records, method = nil)
+        def delete_records(records, method)
           if sql = @reflection.options[:delete_sql]
             records.each { |record| @owner.connection.delete(interpolate_sql(sql, record)) }
           else
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 732f63713e..0b246587c0 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -80,7 +80,7 @@ module ActiveRecord
         end
 
         # Deletes the records according to the <tt>:dependent</tt> option.
-        def delete_records(records, method = @reflection.options[:dependent])
+        def delete_records(records, method)
           if method == :destroy
             records.each { |r| r.destroy }
             update_counter(-records.length) unless inverse_updates_counter_cache?
diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
index 040de7e98f..81dd373f7b 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -57,7 +57,7 @@ module ActiveRecord
           end
         end
 
-        def delete_records(records, method = @reflection.options[:dependent])
+        def delete_records(records, method)
           through = @owner.send(:association_proxy, @reflection.through_reflection.name)
           scope   = through.scoped.where(construct_join_attributes(*records))
 
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index c2f2609c9e..dd2ce786aa 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -837,7 +837,7 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase
     @pirate.parrots.each { |parrot| parrot.mark_for_destruction }
     assert @pirate.save
 
-    assert_queries(1) do
+    assert_no_queries do
       assert @pirate.save
     end
   end