Daily Refactor #24: Extract method body in IssuesController#edit

I tackled two refactorings today in order to clean up IssuesController#edit.

The Refactoring – #1

This first refactoring moves most of the #edit method’s body into #update, where it belongs. This did cause a some duplication though.

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

# app/controllers/issues_controller.rb
class IssuesController  @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
      @time_entry.attributes = params[:time_entry]
      if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
        attachments = attach_files(@issue, params[:attachments])
        attachments.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
          # Log spend time
          if User.current.allowed_to?(:log_time, @project)
            @time_entry.save
          end
          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})
          respond_to do |format|
            format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
            format.xml  { head :ok }
          end
          return
        end
      end
      # failure
      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.each(&:destroy)
  end
 
  def update
    edit
  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

# app/controllers/issues_controller.rb
class IssuesController  @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
      @time_entry.attributes = params[:time_entry]
      if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
        attachments = attach_files(@issue, params[:attachments])
        attachments.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
          # Log spend time
          if User.current.allowed_to?(:log_time, @project)
            @time_entry.save
          end
          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})
          respond_to do |format|
            format.html { redirect_back_or_default({:action => 'show', :id => @issue}) }
            format.xml  { head :ok }
          end
          return
        end
      end
      # failure
      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.each(&:destroy)
  end
end

The Refactoring – #2

This refactoring used extract method to pull the duplication out of #edit and #update into a utility method.

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

# app/controllers/issues_controller.rb
class IssuesController  @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
      @time_entry.attributes = params[:time_entry]
      if (@time_entry.hours.nil? || @time_entry.valid?) && @issue.valid?
        # ... saving code
      end
      # failure
      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.each(&:destroy)
  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
end

Review

Now that IssuesController is following the REST design for it’s edit and update actions, the next refactorings on them will be to clean up their internal implementation. #edit looks really well factored already, but #update can use some more, especially inside of it’s main if conditional. And #update_issue_from_params is doing quite a bit of work, especially for the read-only #edit action.

Reference commits