I'm back up in the ReportsController now, getting ready to tackle some more of the duplication.
The Refactoring
Before
# 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
# 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
# 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
# app/controllers/reports_controller.rb
class ReportsController < ApplicationController
menu_item :issues
before_filter :find_project, :authorize
def issue_report
@statuses = IssueStatus.find(:all, :order => '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_reportwas 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
casestatement 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.
