Daily Refactor #26: Convert Procedural Design to Objects in #issue_update

The Refactoring

Now that I have a single method to start refactoring, it’s time to start digging into #issue_update. The first smell I see is that the method is working on an Issue, TimeEntry, and Attachments all at once, jumping between the three procedurally. So first I needed to separate these three objects.

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

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

# app/controllers/issues_controller.rb
class IssuesController  @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 = 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
        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

Now it’s easier to see the section that deals with a TimeEntry. Using ActiveRecord I was able to store the unsaved TimeEntry onto the @issue so that when @issue.save is called they are both saved. The TimeEntry section can now be refactored to a separate method to further simplify the logic (maybe even refactored to a method on the Model).

Reference commit