Daily Refactor #13: Inline utility methods in ReportsController

The Refactoring

Today I used the inline method refactoring to make the ReportsController even more concise by removing all of the utility methods.

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
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100

# 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)
    when "version"
      @field = "fixed_version_id"
      @rows = @project.shared_versions.sort
      @data = issues_by_version
      @report_title = l(:field_version)
    when "priority"
      @field = "priority_id"
      @rows = IssuePriority.all
      @data = issues_by_priority
      @report_title = l(:field_priority)
    when "category"
      @field = "category_id"
      @rows = @project.issue_categories
      @data = issues_by_category
      @report_title = l(:field_category)
    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)
    when "author"
      @field = "author_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = issues_by_author
      @report_title = l(:field_author)
    when "subproject"
      @field = "project_id"
      @rows = @project.descendants.active
      @data = issues_by_subproject
      @report_title = l(:field_subproject)
    end
 
    respond_to do |format|
      if @field
        format.html {}
      else
        format.html { redirect_to :action => 'issue_report', :id => @project }
      end
    end
  end
private
  def issues_by_tracker
    @issues_by_tracker ||= Issue.by_tracker(@project)
  end
 
  def issues_by_version
    @issues_by_version ||= Issue.by_version(@project)
  end
 
  def issues_by_priority    
    @issues_by_priority ||= Issue.by_priority(@project)
  end
 
  def issues_by_category   
    @issues_by_category ||= Issue.by_category(@project)
  end
 
  def issues_by_assigned_to
    @issues_by_assigned_to ||= Issue.by_assigned_to(@project)
  end
 
  def issues_by_author
    @issues_by_author ||= Issue.by_author(@project)
  end
 
  def issues_by_subproject
    @issues_by_subproject ||= Issue.by_subproject(@project)
    @issues_by_subproject ||= []
  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

# 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 = Issue.by_tracker(@project)
    @issues_by_version = Issue.by_version(@project)
    @issues_by_priority = Issue.by_priority(@project)
    @issues_by_category = Issue.by_category(@project)
    @issues_by_assigned_to = Issue.by_assigned_to(@project)
    @issues_by_author = Issue.by_author(@project)
    @issues_by_subproject = Issue.by_subproject(@project) || []
 
    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 = Issue.by_tracker(@project)
      @report_title = l(:field_tracker)
    when "version"
      @field = "fixed_version_id"
      @rows = @project.shared_versions.sort
      @data = Issue.by_version(@project)
      @report_title = l(:field_version)
    when "priority"
      @field = "priority_id"
      @rows = IssuePriority.all
      @data = Issue.by_priority(@project)
      @report_title = l(:field_priority)
    when "category"
      @field = "category_id"
      @rows = @project.issue_categories
      @data = Issue.by_category(@project)
      @report_title = l(:field_category)
    when "assigned_to"
      @field = "assigned_to_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = Issue.by_assigned_to(@project)
      @report_title = l(:field_assigned_to)
    when "author"
      @field = "author_id"
      @rows = @project.members.collect { |m| m.user }.sort
      @data = Issue.by_author(@project)
      @report_title = l(:field_author)
    when "subproject"
      @field = "project_id"
      @rows = @project.descendants.active
      @data = Issue.by_subproject(@project) || []
      @report_title = l(:field_subproject)
    end
 
    respond_to do |format|
      if @field
        format.html {}
      else
        format.html { redirect_to :action => 'issue_report', :id => @project }
      end
    end
  end
 
end

Review

This refactoring shows that extracting methods isn’t always the best approach. Since the utility methods’ implementation did exactly what the name said, it’s clearer to remove the utility method and inline the implementation directly in the caller. This was made possible because I moved most of implementation to the Model in a previous refactoring.

This also cleans up a subtle side effect in the issue_report_details action. Extra instance variables were being created when calling the utility methods even though the results was stored to @data. Though it’s small, this would use resources and would expose those instance variables to the view.

Reference commit