begin refactoring delete_records method

Refactor by creating two methods delete_all_records and delete_records
to be called by delete_all and delete (or destroy) respectively.
This reduces the number of conditionals required to handle _how_
records get deleted.

The new delete_count method handles how scope is applied to which
delete action.

A delete_all_records method also has to be called in has_many_through
association because of how the methods are chained. This will be
refactored later on.

3 files changed, 30 insertions, 13 deletions

diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index 48628230c7..d83019a920 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -194,7 +194,7 @@ module ActiveRecord
                       options[:dependent]
                     end
 
-        delete_records(:all, dependent).tap do
+        delete_all_records(dependent).tap do
           reset
           loaded!
         end
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index aac85a36c8..eed6b0d952 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -105,23 +105,36 @@ module ActiveRecord
           }
         end
 
+        def delete_count(records, method, scope)
+          case method
+          when :destroy
+            records.length
+          when :delete_all
+            scope.delete_all
+          else
+            scope.update_all(reflection.foreign_key => nil)
+          end
+        end
+
+        def delete_all_records(method)
+          scope = self.scope
+
+          count = delete_count(:all, method, scope)
+
+          update_counter(-count)
+        end
+
         # Deletes the records according to the <tt>:dependent</tt> option.
         def delete_records(records, method)
+          scope = self.scope.where(reflection.klass.primary_key => records)
+
+          count = delete_count(records, method, scope)
+
           if method == :destroy
             records.each(&:destroy!)
-            update_counter(-records.length) unless inverse_updates_counter_cache?
+            update_counter(-count) unless inverse_updates_counter_cache?
           else
-            if records == :all || !reflection.klass.primary_key
-              scope = self.scope
-            else
-              scope = self.scope.where(reflection.klass.primary_key => records)
-            end
-
-            if method == :delete_all
-              update_counter(-scope.delete_all)
-            else
-              update_counter(-scope.update_all(reflection.foreign_key => nil))
-            end
+            update_counter(-count)
           end
         end
 
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 aeb77e2753..5a45016cfa 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -130,6 +130,10 @@ module ActiveRecord
           end
         end
 
+        def delete_all_records(method)
+          delete_records(:all, method)
+        end
+
         def delete_records(records, method)
           ensure_not_nested