Daily Refactor #11: Extract action in ReportsController

I’m back up in the ReportsController now, getting ready to tackle some more of the duplication.

The Refactoring

Before

1
2
3
4
5

# config/routes.rb
  map.with_options :controller => 'reports', :action => 'issue_report', :conditions => {:method => :get} do |reports|
    reports.connect 'projects/:id/issues/report'
    reports.connect 'projects/:id/issues/report/:detail'
  end
 
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

# app/controllers/reports_controller.rb
  def issue_report
    @statuses = IssueStatus.find(:all, :order => 'position')
 
    case params[:detail]
    when "tracker"
      @field = "tracker_id"
      @rows = @project.trackers
      @data = issues_by_tracker
      @report_title = l(:field_tracker)
      render :template => "reports/issue_report_details"
    when "version"
      @field = "fixed_version_id"
      @rows = @project.shared_versions.sort
      @data = issues_by_version
      @report_title = l(:field_version)
      render :template => "reports/issue_report_details"
    when "priority"
      @field = "priority_id"
      @rows = IssuePriority.all
      @data = issues_by_priority
      @report_title = l(:field_priority)
      render :template => "reports/issue_report_details"   
    when "category"
      @field = "category_id"
      @rows = @project.issue_categories
      @data = issues_by_category
      @report_title = l(:field_category)
      render :template => "reports/issue_report_details"   
    when "assigned_to"
      @field = "assigned_to_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_assigned_to
      @report_title = l(:field_assigned_to)
      render :template => "reports/issue_report_details"
    when "author"
      @field = "author_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_author
      @report_title = l(:field_author)
      render :template => "reports/issue_report_details"  
    when "subproject"
      @field = "project_id"
      @rows = @project.descendants.active
      @data = issues_by_subproject
      @report_title = l(:field_subproject)
      render :template => "reports/issue_report_details"  
    else
      @trackers = @project.trackers
      @versions = @project.shared_versions.sort
      @priorities = IssuePriority.all
      @categories = @project.issue_categories
      @assignees = @project.members.collect { |m| m.user }.sort
      @authors = @project.members.collect { |m| m.user }.sort
      @subprojects = @project.descendants.active
      issues_by_tracker
      issues_by_version
      issues_by_priority
      issues_by_category
      issues_by_assigned_to
      issues_by_author
      issues_by_subproject
 
      render :template => "reports/issue_report"
    end
  end  
 
private
  # ...
end

After

1
2
3
4
5

# config/routes.rb
  map.with_options :controller => 'reports', :conditions => {:method => :get} do |reports|
    reports.connect 'projects/:id/issues/report', :action => 'issue_report'
    reports.connect 'projects/:id/issues/report/:detail', :action => 'issue_report_details'
  end
 
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

# app/controllers/reports_controller.rb
class ReportsController  'position')
 
    @trackers = @project.trackers
    @versions = @project.shared_versions.sort
    @priorities = IssuePriority.all
    @categories = @project.issue_categories
    @assignees = @project.members.collect { |m| m.user }.sort
    @authors = @project.members.collect { |m| m.user }.sort
    @subprojects = @project.descendants.active
    issues_by_tracker
    issues_by_version
    issues_by_priority
    issues_by_category
    issues_by_assigned_to
    issues_by_author
    issues_by_subproject
 
    render :template => "reports/issue_report"
  end  
 
  def issue_report_details
    @statuses = IssueStatus.find(:all, :order => 'position')
 
    case params[:detail]
    when "tracker"
      @field = "tracker_id"
      @rows = @project.trackers
      @data = issues_by_tracker
      @report_title = l(:field_tracker)
      render :template => "reports/issue_report_details"
    when "version"
      @field = "fixed_version_id"
      @rows = @project.shared_versions.sort
      @data = issues_by_version
      @report_title = l(:field_version)
      render :template => "reports/issue_report_details"
    when "priority"
      @field = "priority_id"
      @rows = IssuePriority.all
      @data = issues_by_priority
      @report_title = l(:field_priority)
      render :template => "reports/issue_report_details"   
    when "category"
      @field = "category_id"
      @rows = @project.issue_categories
      @data = issues_by_category
      @report_title = l(:field_category)
      render :template => "reports/issue_report_details"   
    when "assigned_to"
      @field = "assigned_to_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_assigned_to
      @report_title = l(:field_assigned_to)
      render :template => "reports/issue_report_details"
    when "author"
      @field = "author_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_author
      @report_title = l(:field_author)
      render :template => "reports/issue_report_details"  
    when "subproject"
      @field = "project_id"
      @rows = @project.descendants.active
      @data = issues_by_subproject
      @report_title = l(:field_subproject)
      render :template => "reports/issue_report_details"  
    else
      redirect_to :action => 'issue_report', :id => @project
    end
 
  end
private
  # ...
end

Review

This refactoring split the issue_report action into two separate actions: issue_report and issue_report_details. I did this for a few reasons:

  • I felt like issue_report was trying to do too much. It would generate a summary report and also one of 7 details reports.
  • Having one large action in a controller gets away from the standard RESTful design principles in Rails.
  • Having the implementation hidden inside of a large case statement made it more difficult to understand what was happening.

Now I can start to really remove the duplication from the internals of the actions. It’s a good thing I’ve been building up the unit test suite as I’ve gone; the safety net of the tests will be valuable over the next few refactorings.

Reference commit