With #move
and #perform_move
separated in the IssuesController, I can now see a clear path to using extract controller to move them to a new controller. (Extract controller is a Rails specific refactoring that is based on extract class).
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
|
class IssuesController < ApplicationController
def move
# ...
end
def perform_move
# ...
end
private
def prepare_for_issue_move
# ...
end
def find_issues
# ..
end
def set_flash_from_bulk_issue_save
# ...
end
def extract_changed_attributes_for_move
# ...
end
end |
class IssuesController < ApplicationController
def move
# ...
end
def perform_move
# ...
end
private
def prepare_for_issue_move
# ...
end
def find_issues
# ..
end
def set_flash_from_bulk_issue_save
# ...
end
def extract_changed_attributes_for_move
# ...
end
end
1
2
3
4
5
6
7
8
9
10
11
|
# config/routes.rb
map.with_options :controller => 'issues' do |issues_routes|
issues_routes.with_options :conditions => {:method => :get} do |issues_views|
# ...
issues_views.connect 'issues/:id/move', :action => 'move', :id => /\d+/
end
issues_routes.with_options :conditions => {:method => :post} do |issues_actions|
# ...
issues_actions.connect 'issues/:id/:action', :action => /edit|perform_move|destroy/, :id => /\d+/
end
end |
# config/routes.rb
map.with_options :controller => 'issues' do |issues_routes|
issues_routes.with_options :conditions => {:method => :get} do |issues_views|
# ...
issues_views.connect 'issues/:id/move', :action => 'move', :id => /\d+/
end
issues_routes.with_options :conditions => {:method => :post} do |issues_actions|
# ...
issues_actions.connect 'issues/:id/:action', :action => /edit|perform_move|destroy/, :id => /\d+/
end
end
After
1
2
3
|
class IssuesController < ApplicationController
# Methods removed
end |
class IssuesController < ApplicationController
# Methods removed
end
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
|
class IssueMovesController < ApplicationController
default_search_scope :issues
before_filter :find_issues
before_filter :authorize
def new
# ...
end
def create
# ...
end
private
def prepare_for_issue_move
# ...
end
def find_issues
# ..
end
def set_flash_from_bulk_issue_save
# ...
end
def extract_changed_attributes_for_move
# ...
end
end |
class IssueMovesController < ApplicationController
default_search_scope :issues
before_filter :find_issues
before_filter :authorize
def new
# ...
end
def create
# ...
end
private
def prepare_for_issue_move
# ...
end
def find_issues
# ..
end
def set_flash_from_bulk_issue_save
# ...
end
def extract_changed_attributes_for_move
# ...
end
end
1
2
|
# config/routes.rb
map.resources :issue_moves, :only => [:new, :create], :path_prefix => '/issues', :as => 'move' |
# config/routes.rb
map.resources :issue_moves, :only => [:new, :create], :path_prefix => '/issues', :as => 'move'
The results of this refactoring are:
- IssuesController had two actions removed.
- A new RESTful resource was created, IssueMoves.
- Removed some complex and nested route in favor of a
map.resources
route.
- Added some code duplication in the utility methods of IssueMovesController.
Since the new REST resource was extracted, it wouldn’t be difficult to add a new API for moving and copying issues. But first I need to do a few more refactorings to remove the duplication I introduced in IssueMovesController
.
Reference commit