Daily Refactor #61: Extract Method in IssuesController for REST

By on April 21, 2010 in General

Now I’m starting to refactor Redmine’s IssuesController again in order to have it match the standard Rails REST controllers. This time I’m working on the new/create action pair.

The Refactoring

This refactoring splits the single #new action into the two separate 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

# app/controllers/issues_controller.rb
class IssuesController  [:new, :update_form, :preview, :auto_complete]
 
  def new
    @issue = Issue.new
    @issue.copy_from(params[:copy_from]) if params[:copy_from]
    @issue.project = @project
    # Tracker must be set before custom field values
    @issue.tracker ||= @project.trackers.find((params[:issue] && params[:issue][:tracker_id]) || params[:tracker_id] || :first)
    if @issue.tracker.nil?
      render_error l(:error_no_tracker_in_project)
      return
    end
    if @issue.status.nil?
      render_error l(:error_no_default_issue_status)
      return
    end
    if params[:issue].is_a?(Hash)
      @issue.safe_attributes = params[:issue]
      @issue.watcher_user_ids = params[:issue]['watcher_user_ids'] if User.current.allowed_to?(:add_issue_watchers, @project)
    end
    @issue.author = User.current
 
    if request.get? || request.xhr?
      @issue.start_date ||= Date.today
    else
      call_hook(:controller_issues_new_before_save, { :params => params, :issue => @issue })
      if @issue.save
        attachments = Attachment.attach_files(@issue, params[:attachments])
        render_attachment_warning_if_needed(@issue)
        flash[:notice] = l(:notice_successful_create)
        call_hook(:controller_issues_new_after_save, { :params => params, :issue => @issue})
        respond_to do |format|
          format.html {
            redirect_to(params[:continue] ? { :action => 'new', :issue => {:tracker_id => @issue.tracker, 
                                                                           :parent_issue_id => @issue.parent_issue_id}.reject {|k,v| v.nil?} } :
                                            { :action => 'show', :id => @issue })
          }
          format.xml  { render :action => 'show', :status => :created, :location => url_for(:controller => 'issues', :action => 'show', :id => @issue) }
        end
        return
      else
        respond_to do |format|
          format.html { }
          format.xml  { render(:xml => @issue.errors, :status => :unprocessable_entity); return }
        end
      end
    end 
    @priorities = IssuePriority.all
    @allowed_statuses = @issue.new_statuses_allowed_to(User.current, true)
    render :layout => !request.xhr?
  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
64
65
66
67
68
69
70
71
72
73
74
75
76

# app/controllers/issues_controller.rb
class IssuesController  [:new, :create, :update_form, :preview, :auto_complete]
 
  verify :method => :post, :only => :create, :render => {:nothing => true, :status => :method_not_allowed }
 
  def new
    @issue = Issue.new
    @issue.copy_from(params[:copy_from]) if params[:copy_from]
    @issue.project = @project
    # Tracker must be set before custom field values
    @issue.tracker ||= @project.trackers.find((params[:issue] && params[:issue][:tracker_id]) || params[:tracker_id] || :first)
    if @issue.tracker.nil?
      render_error l(:error_no_tracker_in_project)
      return
    end
    if @issue.status.nil?
      render_error l(:error_no_default_issue_status)
      return
    end
    if params[:issue].is_a?(Hash)
      @issue.safe_attributes = params[:issue]
      @issue.watcher_user_ids = params[:issue]['watcher_user_ids'] if User.current.allowed_to?(:add_issue_watchers, @project)
    end
    @issue.author = User.current
    @issue.start_date ||= Date.today
    @priorities = IssuePriority.all
    @allowed_statuses = @issue.new_statuses_allowed_to(User.current, true)
    render :action => 'new', :layout => !request.xhr?
  end
 
  def create
    @issue = Issue.new
    @issue.copy_from(params[:copy_from]) if params[:copy_from]
    @issue.project = @project
    # Tracker must be set before custom field values
    @issue.tracker ||= @project.trackers.find((params[:issue] && params[:issue][:tracker_id]) || params[:tracker_id] || :first)
    if @issue.tracker.nil?
      render_error l(:error_no_tracker_in_project)
      return
    end
    if @issue.status.nil?
      render_error l(:error_no_default_issue_status)
      return
    end
    if params[:issue].is_a?(Hash)
      @issue.safe_attributes = params[:issue]
      @issue.watcher_user_ids = params[:issue]['watcher_user_ids'] if User.current.allowed_to?(:add_issue_watchers, @project)
    end
    @issue.author = User.current
 
    @priorities = IssuePriority.all
    @allowed_statuses = @issue.new_statuses_allowed_to(User.current, true)
 
    call_hook(:controller_issues_new_before_save, { :params => params, :issue => @issue })
    if @issue.save
      attachments = Attachment.attach_files(@issue, params[:attachments])
      render_attachment_warning_if_needed(@issue)
      flash[:notice] = l(:notice_successful_create)
      call_hook(:controller_issues_new_after_save, { :params => params, :issue => @issue})
      respond_to do |format|
        format.html {
          redirect_to(params[:continue] ? { :action => 'new', :issue => {:tracker_id => @issue.tracker, :parent_issue_id => @issue.parent_issue_id}.reject {|k,v| v.nil?} } :
                      { :action => 'show', :id => @issue })
        }
        format.xml  { render :action => 'show', :status => :created, :location => url_for(:controller => 'issues', :action => 'show', :id => @issue) }
      end
      return
    else
      respond_to do |format|
        format.html { render :action => 'new' }
        format.xml  { render(:xml => @issue.errors, :status => :unprocessable_entity); return }
      end
    end
  end
 
end

Review

This helped #new because it’s now not concerned with creating a new Issue, it only builds an Issue object for the view. Now #create will only need to be concerned with saving the Issue.

This refactoring did introduce a lot of duplication, which I’ll be resolving over the next few days. Something taking on more code debt, in the form of duplication, is required over the short term in order to reach your goal.

Reference commit

Share

  • Facebook
  • Twitter
  • HackerNews
  • Reddit
  • Tumblr
  • Delicious
  • Email
  • RSS

Related posts:

  1. Redmine Refactor #140: Extract Method WikiController#edit to #update
  2. Redmine Refactor #118: Split Edit Method in NewsController
  3. Redmine Refactor #88: Extract Method from move and perform_move
  4. Daily Refactor #63: Extract Method to before_filter in IssuesController
  5. Daily Refactor #38: Extract Method in StuffToDo

About Eric Davis

I founded Little Stream Software where I provide Redmine and ChiliProject services to help projects teams. I also created an ebook, Redmine Tips, were I show you how to become more productive using Redmine. I am also the author of Refactoring Redmine, where I go about refactoring Rails using Redmine as an example.
View all posts by Eric Davis →

, , ,

Chirk HR     Reuse your existing job applicants »
WP Socializer Aakash Web