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
# 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
# app/controllers/reports_controller.rb
class ReportsController < ApplicationController
menu_item :issues
before_filter :find_project, :authorize
private
# ... other private methods
end
# 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.
