The Refactoring
This is the first part to a larger refactoring of the ReportsController in Redmine. This controller is responsible for calculating the total number of issues based on different criteria (e.g. issue status, issue priority). There are two big problems with the current code:
- It has a lot of duplication, both in the main methods and the utility methods
- It has raw query methods in the controller (
ActiveRecord::Base.connection.select_all
)
So the first step will be to move all of the raw query methods out of the controller and into the Issue model where they belong.
(I’ve included the entire ReportsController
to make it easier to see how much of it will be changed. Don’t let your eyes glaze over in awe over this code.)
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 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173 174 175 176 177 178 179 |
# app/controllers/reports_controller.rb class ReportsController '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 # Find project of id params[:id] def find_project @project = Project.find(params[:id]) rescue ActiveRecord::RecordNotFound render_404 end def issues_by_tracker @issues_by_tracker ||= ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, t.id as tracker_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{Tracker.table_name} t where i.status_id=s.id and i.tracker_id=t.id and i.project_id=#{@project.id} group by s.id, s.is_closed, t.id") end def issues_by_version @issues_by_version ||= ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, v.id as fixed_version_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{Version.table_name} v where i.status_id=s.id and i.fixed_version_id=v.id and i.project_id=#{@project.id} group by s.id, s.is_closed, v.id") end def issues_by_priority @issues_by_priority ||= ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, p.id as priority_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssuePriority.table_name} p where i.status_id=s.id and i.priority_id=p.id and i.project_id=#{@project.id} group by s.id, s.is_closed, p.id") end def issues_by_category @issues_by_category ||= ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, c.id as category_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssueCategory.table_name} c where i.status_id=s.id and i.category_id=c.id and i.project_id=#{@project.id} group by s.id, s.is_closed, c.id") end def issues_by_assigned_to @issues_by_assigned_to ||= ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, a.id as assigned_to_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a where i.status_id=s.id and i.assigned_to_id=a.id and i.project_id=#{@project.id} group by s.id, s.is_closed, a.id") end def issues_by_author @issues_by_author ||= ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, a.id as author_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a where i.status_id=s.id and i.author_id=a.id and i.project_id=#{@project.id} group by s.id, s.is_closed, a.id") end def issues_by_subproject @issues_by_subproject ||= ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, i.project_id as project_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s where i.status_id=s.id and i.project_id IN (#{@project.descendants.active.collect{|p| p.id}.join(',')}) group by s.id, s.is_closed, i.project_id") if @project.descendants.active.any? @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 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 101 102 103 |
# app/controllers/reports_controller.rb class ReportsController '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 # Find project of id params[:id] def find_project @project = Project.find(params[:id]) rescue ActiveRecord::RecordNotFound render_404 end 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 |
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 101 102 103 |
# app/models/issue.rb class Issue < ActiveRecord::Base # Extracted from the ReportsController. # TODO: refactor into a common factory or named scopes def self.by_tracker(project) ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, t.id as tracker_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{Tracker.table_name} t where i.status_id=s.id and i.tracker_id=t.id and i.project_id=#{project.id} group by s.id, s.is_closed, t.id") end def self.by_version(project) ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, v.id as fixed_version_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{Version.table_name} v where i.status_id=s.id and i.fixed_version_id=v.id and i.project_id=#{project.id} group by s.id, s.is_closed, v.id") end def self.by_priority(project) ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, p.id as priority_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssuePriority.table_name} p where i.status_id=s.id and i.priority_id=p.id and i.project_id=#{project.id} group by s.id, s.is_closed, p.id") end def self.by_category(project) ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, c.id as category_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{IssueCategory.table_name} c where i.status_id=s.id and i.category_id=c.id and i.project_id=#{project.id} group by s.id, s.is_closed, c.id") end def self.by_assigned_to(project) ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, a.id as assigned_to_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a where i.status_id=s.id and i.assigned_to_id=a.id and i.project_id=#{project.id} group by s.id, s.is_closed, a.id") end def self.by_author(project) ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, a.id as author_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s, #{User.table_name} a where i.status_id=s.id and i.author_id=a.id and i.project_id=#{project.id} group by s.id, s.is_closed, a.id") end def self.by_subproject(project) ActiveRecord::Base.connection.select_all("select s.id as status_id, s.is_closed as closed, i.project_id as project_id, count(i.id) as total from #{Issue.table_name} i, #{IssueStatus.table_name} s where i.status_id=s.id and i.project_id IN (#{project.descendants.active.collect{|p| p.id}.join(',')}) group by s.id, s.is_closed, i.project_id") if project.descendants.active.any? end # End ReportsController extraction end |
Review
This refactor alone moved over 30% of the code to the Model and makes it easier to see that is only one public method in this entire controller. There is still a bit that needs to be done in the Issue
, since all I really did was to move some code duplication down to the model. But now that the model has all this code, it’s easier to see some similarities in the methods in order to extract it to a utility method.