The Refactoring
Today I used the inline method refactoring to make the ReportsController even more concise by removing all of the utility methods.
Before
# 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)
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
# 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 = 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.
