It’s time to move on from the ReportsController to code that needs to be refactored more.
Using Caliper’s Flay Report, it looks like IssueStatusesController, TrackersController, AuthSourcesController, and RolesController all have the exact same index
action.
1 2 3 4 |
def index list render :action => 'list' unless request.xhr? end |
While I could just use pull up method to remove this duplication, there is a stronger smell here that I’d like to fix…
The Refactoring
In Rails the index
action is generally used to list a collection of records, typically with some pagination. These four controllers are using the list
action for that though and have the index
action just run list
. This is way too much redirection and it can be simplified.
Before
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 |
# app/controllers/issue_statuses_controller.rb class IssueStatusesController :post, :only => [ :destroy, :create, :update, :move, :update_issue_done_ratio ], :redirect_to => { :action => :list } def index list render :action => 'list' unless request.xhr? end def list @issue_status_pages, @issue_statuses = paginate :issue_statuses, :per_page => 25, :order => "position" render :action => "list", :layout => false if request.xhr? end # ... end |
After
1 2 3 4 5 6 7 8 9 10 11 |
# app/controllers/issue_statuses_controller.rb class IssueStatusesController :post, :only => [ :destroy, :create, :update, :move, :update_issue_done_ratio ], :redirect_to => { :action => :index } def index @issue_status_pages, @issue_statuses = paginate :issue_statuses, :per_page => 25, :order => "position" render :action => "index", :layout => false if request.xhr? end # ... end |
Review
Now IssueStatusesController only has the index
action, which follows the Rails standards. This will also make it easier to add on a RESTful web service later on.
Reference commit (includes the changes to the tests and redirects that used list
)