The Refactoring
I’m still hammering on #issue_update today. It’s almost to the point where I can move the entire method down into the Issue model but it’s creating too many instance variables that view needs. This refactoring moves one of those variables up into the higher level #edit and #update actions.
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 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 |
# app/controllers/issues_controller.rb class IssuesController 'show', :id => @issue}) } format.xml { head :ok } end else respond_to do |format| format.html { render :action => 'edit' } format.xml { render :xml => @issue.errors, :status => :unprocessable_entity } end end rescue ActiveRecord::StaleObjectError # Optimistic locking exception flash.now[:error] = l(:notice_locking_conflict) # Remove the previously added attachments if issue was not updated attachments[:files].each(&:destroy) if attachments[:files] end def update_issue_from_params @allowed_statuses = @issue.new_statuses_allowed_to(User.current) @priorities = IssuePriority.all @edit_allowed = User.current.allowed_to?(:edit_issues, @project) @time_entry = TimeEntry.new @notes = params[:notes] @journal = @issue.init_journal(User.current, @notes) # User can change issue attributes only if he has :edit permission or if a workflow transition is allowed if (@edit_allowed || !@allowed_statuses.empty?) && params[:issue] attrs = params[:issue].dup attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s} @issue.safe_attributes = attrs end end def issue_update if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, @project) @time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today) @time_entry.attributes = params[:time_entry] @issue.time_entries << @time_entry end if @issue.valid? attachments = Attachment.attach_files(@issue, params[:attachments]) render_attachment_warning_if_needed(@issue) attachments[:files].each {|a| @journal.details < 'attachment', :prop_key => a.id, :value => a.filename)} call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal}) if @issue.save if !@journal.new_record? # Only send notification if something was actually changed flash[:notice] = l(:notice_successful_update) end call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @journal}) return true end end # failure, returns false end 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 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 |
# app/controllers/issues_controller.rb class IssuesController 'show', :id => @issue}) } format.xml { head :ok } end else @journal = @issue.current_journal respond_to do |format| format.html { render :action => 'edit' } format.xml { render :xml => @issue.errors, :status => :unprocessable_entity } end end rescue ActiveRecord::StaleObjectError # Optimistic locking exception flash.now[:error] = l(:notice_locking_conflict) # Remove the previously added attachments if issue was not updated attachments[:files].each(&:destroy) if attachments[:files] end def update_issue_from_params @allowed_statuses = @issue.new_statuses_allowed_to(User.current) @priorities = IssuePriority.all @edit_allowed = User.current.allowed_to?(:edit_issues, @project) @time_entry = TimeEntry.new @notes = params[:notes] @issue.init_journal(User.current, @notes) # User can change issue attributes only if he has :edit permission or if a workflow transition is allowed if (@edit_allowed || !@allowed_statuses.empty?) && params[:issue] attrs = params[:issue].dup attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s} @issue.safe_attributes = attrs end end def issue_update if params[:time_entry] && params[:time_entry][:hours].present? && User.current.allowed_to?(:log_time, @project) @time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today) @time_entry.attributes = params[:time_entry] @issue.time_entries << @time_entry end if @issue.valid? attachments = Attachment.attach_files(@issue, params[:attachments]) render_attachment_warning_if_needed(@issue) attachments[:files].each {|a| @issue.current_journal.details < 'attachment', :prop_key => a.id, :value => a.filename)} call_hook(:controller_issues_edit_before_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @issue.current_journal}) if @issue.save if !@issue.current_journal.new_record? # Only send notification if something was actually changed flash[:notice] = l(:notice_successful_update) end call_hook(:controller_issues_edit_after_save, { :params => params, :issue => @issue, :time_entry => @time_entry, :journal => @issue.current_journal}) return true end end # failure, returns false end end |
Review
Using replace temp with query I was able to replace the temp (@journal) with a query for the object (@issue.current_journal). I still have to set the temp in the actions but now it’s used as a way to access the object instead of doing actual work on it. I think I’ll be able to finish this refactoring set soon, I only see a couple more refactorings left on #issue_update.