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.