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
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 |
# 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
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 |
# 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.