Skip to content

Commit d8ae276

Browse files
committed
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.
1 parent 522110a commit d8ae276

File tree

3 files changed

+30
-13
lines changed

3 files changed

+30
-13
lines changed

activerecord/lib/active_record/associations/collection_association.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ def delete_all(dependent = nil)
194194
options[:dependent]
195195
end
196196

197-
delete_records(:all, dependent).tap do
197+
delete_all_records(dependent).tap do
198198
reset
199199
loaded!
200200
end

activerecord/lib/active_record/associations/has_many_association.rb

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -105,23 +105,36 @@ def inverse_updates_counter_cache?(reflection = reflection())
105105
}
106106
end
107107

108+
def delete_count(records, method, scope)
109+
case method
110+
when :destroy
111+
records.length
112+
when :delete_all
113+
scope.delete_all
114+
else
115+
scope.update_all(reflection.foreign_key => nil)
116+
end
117+
end
118+
119+
def delete_all_records(method)
120+
scope = self.scope
121+
122+
count = delete_count(:all, method, scope)
123+
124+
update_counter(-count)
125+
end
126+
108127
# Deletes the records according to the <tt>:dependent</tt> option.
109128
def delete_records(records, method)
129+
scope = self.scope.where(reflection.klass.primary_key => records)
130+
131+
count = delete_count(records, method, scope)
132+
110133
if method == :destroy
111134
records.each(&:destroy!)
112-
update_counter(-records.length) unless inverse_updates_counter_cache?
135+
update_counter(-count) unless inverse_updates_counter_cache?
113136
else
114-
if records == :all || !reflection.klass.primary_key
115-
scope = self.scope
116-
else
117-
scope = self.scope.where(reflection.klass.primary_key => records)
118-
end
119-
120-
if method == :delete_all
121-
update_counter(-scope.delete_all)
122-
else
123-
update_counter(-scope.update_all(reflection.foreign_key => nil))
124-
end
137+
update_counter(-count)
125138
end
126139
end
127140

activerecord/lib/active_record/associations/has_many_through_association.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,10 @@ def update_through_counter?(method)
130130
end
131131
end
132132

133+
def delete_all_records(method)
134+
delete_records(:all, method)
135+
end
136+
133137
def delete_records(records, method)
134138
ensure_not_nested
135139

0 commit comments

Comments
 (0)