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.

1 files changed, 11 insertions, 15 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