Daily Refactor #25: Extract method in IssuesController#update

The Refactoring

This refactoring is what I call the magic trick refactor; it looks like a lot is going on but in reality it’s just smoke and mirrors!

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

# 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
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

# 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.each(&:destroy)
  end
 
  # ...
 
  # TODO: Temporary utility method for #update.  Should be split off
  # and moved to the Issue model (accepts_nested_attributes_for maybe?)
  # TODO: move attach_files to the model so this can be moved to the
  # model also
  def issue_update
    @time_entry = TimeEntry.new(:project => @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})
        return true
      end
    end
    # failure, returns false
 
  end
end

Review

Truthfully, this refactor isn’t just smoke and mirrors. The behavior stayed the same but now #update is easier to read and is working at a higher level of abstraction (e.g. only handling the response). The #issue_update method is still pretty smelly but it’s just about ripe for some refactoring.

Reference commit