Back up in the ReportsController, there is still some duplication that needs to be resolved. There are nine methods:

• 1 controller action (issue_report)
• 1 private before_filter method (find_project)
• 7 private utility methods

The controller action is ripe for a few refactorings but I wanted to start with the find_project for personal reasons. I used the pull up method to move find_project to the superclass (ApplicationController).

The Refactoring

Before

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

# app/controllers/reports_controller.rb
class ReportsController < ApplicationController
  menu_item :issues
  before_filter :find_project, :authorize
 
  private
  # Find project of id params[:id]
  def find_project
    @project = Project.find(params[:id])        
  rescue ActiveRecord::RecordNotFound
    render_404
  end
 
  # ... other private methods
end

After

1
2
3
4
5
6
7
8

# app/controllers/reports_controller.rb
class ReportsController < ApplicationController
  menu_item :issues
  before_filter :find_project, :authorize
 
  private
  # ... other private methods
end

1
2
3
4
5
6
7
8
9

# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
  # Find project of id params[:id]
  def find_project
    @project = Project.find(params[:id])
  rescue ActiveRecord::RecordNotFound
    render_404
  end
end

Review

Just looking at the ReportsController, this refactoring doesn’t look that effective. But I didn’t include the four other controllers in Redmine that had the exact same find_project method. Or the dozen plugins where I’ve had to implement an identical find_project.

So now ReportsController is starting to clean up quite nicely. Next week I should be able start refactoring the large controller action and clean up a lot of the internal duplication. I’m even thinking of splitting up the action a bit to remove the case statement.

Reference commit