Today's "refactoring" isn't a refactoring in the classic sense, since it includes a behavior change. But if you squint your eyes enough to only see the Controller as the "public interface", then we can call it a refactor.
The Refactoring
Yesterday I moved the create_bulk_time_entry into the TimeEntry class where it fit better. Today I wanted to refine it more, since the Controller is still doing the actual save on the Model.
Before
# bulk_time_entries_controller.rb
def save
if request.post?
@time_entries = params[:time_entries]
render :update do |page|
@time_entries.each_pair do |html_id, entry|
@time_entry = TimeEntry.create_bulk_time_entry(entry)
unless @time_entry && @time_entry.save
page.replace "entry_#{html_id}", :partial => 'time_entry', :object => @time_entry
else
time_entry_target = if @time_entry.issue
"#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
else
"#{h(@time_entry.project.name)}"
end
page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count => @time_entry.hours, :target => time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
end
end
end
end
end
# time_entry_patch.rb
def create_bulk_time_entry(entry)
return false unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
time_entry = TimeEntry.new(entry)
time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours <= 0
time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
time_entry.user = User.current
time_entry
end
After
# bulk_time_entries_controller.rb
def save
if request.post?
@time_entries = params[:time_entries]
render :update do |page|
@time_entries.each_pair do |html_id, entry|
@time_entry = TimeEntry.create_bulk_time_entry(entry)
if @time_entry.new_record?
page.replace "entry_#{html_id}", :partial => 'time_entry', :object => @time_entry
else
time_entry_target = if @time_entry.issue
"#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
else
"#{h(@time_entry.project.name)}"
end
page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count => @time_entry.hours, :target => time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
end
end
end
end
end
# time_entry_patch.rb
def create_bulk_time_entry(entry)
time_entry = TimeEntry.new(entry)
time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours <= 0
if BulkTimeEntriesController.allowed_project?(entry[:project_id])
time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
end
time_entry.user = User.current
time_entry.save
time_entry
end
Review
The big behavior change is that instead of returning false or an unsaved TimeEntry, the method will now return a TimeEntry (saved if it's valid). This is a better design, since the caller (Controller) will not have to worry about the different types of objects that are returned. It will still need to check that the TimeEntry was saved, but the code is a lot easier to understand (if @time_entry.new_record?).
This refactoring really cleaned up the major flaws in the model method. I think it's now time to fix the controller/view interaction now, especially the inline RJS rendering.
