It seems like every time I try to apply a big refactoring, there are parts that are missed. I tried to refactor the entire case statement in issue_report_details into a common data structure and a lookup method. But it ended up being more complex than the case statement was so I threw it away and started over. Once again, the simple iterative process wins over the big rewrite.
The Refactoring
Before
# app/controllers/reports_controller.rb
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
After
# app/controllers/reports_controller.rb
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
Review
Based on the raw lines of code changed, there wasn't a lot of duplicated code removed (7 added, 9 removed). But it does really clean up how the action reads. Before the action would:
- collect the data and do the rendering inside each case
Now the case statement
- collects the data
- does the rendering
This also means that the entire case statement can be moved out of the public action into a utility method.
