written by Eric Davis on August 12, 2010
0 Comments
I've already written about the problem of code duplication when you don't refactor Rails. A similar problem I see all the time is test duplication.
Test Duplication
As Rubyists, we are lucky to have unit testing deeply ingrained in our community. Many developers write tests, some even writing the tests first. But a big problem that comes up is that these tests aren't maintained as much as the application code.
When I learned the Red-Green-Refactor cycle, it only talked about refactoring the code implementation. Nothing about refactoring the tests. So I would end up with some great refactored code but a dozen test cases thrown together. Just like with code duplication, these unfactored tests make modifications more costly and created bugs from the multiple branches of code.
Costly Modifications
Lets say you wrote a test to submit a request to a Rails application. When it's given a success parameter that's greater than 0, the request is successful. When it's less than or equal to 0, the request is redirected. Writing tests for it, you want to make sure you test the all of the different ways it can be called:
class ExampleControllerTest < ActionController::TestCase
should 'be successful with a params success of 1' do
post :create, :success => '1'
assert_response :success
end
should 'be successful with a params success of 2' do
post :create, :success => '2'
assert_response :success
end
should 'be successful with a params success of 3' do
post :create, :success => '3'
assert_response :success
end
should 'be redirected with a params success of -1' do
post :create, :success => '-1'
assert_response :redirect
end
should 'be redirected with a params success of 0' do
post :create, :success => '0'
assert_response :redirect
end
end
As you go through Red-Green-Refactor, you'd probably end up with a simple comparison instead of enumerating every integer in your controller action.
class ExampleController < ApplicationController
def create
if params[:success].to_i > 0
render :text => 'Refactored'
else
redirect_to('/failed')
end
end
end
Now the problem is, what if your requirements change and you need change the parameter name to :content? Or the action should be called "new"? You'll have very little to change in your controller because it's well factored. But you have 5 things to change in your tests. I hope you don't miss one and have one method branching.
Bugs from Branching Code
Just like with code duplication, when there are N copies of test code in an application they all have to be modified at the same time. Say you add a new parameter to the example that forces a redirect. Now you'd need to change each test so you have one version with the parameter and one version without the parameter (10 tests total). Forgetting a test means there is one combination you aren't testing, which means your user will "get" to test for you. Hopefully it will work exactly like you planned.
Remember, your tests are code too. If you don't invest time to refactor them, they can cause problems just like unfactored code.
Example: Showing one way to refactor the test case
In case you're interested, here is how those example tests could be refactored using shoulda:
class ExampleControllerTest < ActionController::TestCase
def self.should_be_successful_with_a_param_of(param)
should "be successful with a success param of #{param}" do
post :create, :success => param
assert_response :success
end
end
def self.should_be_redirected_with_a_param_of(param)
should "be redirected with a success param of #{param}" do
post :create, :success => param
assert_response :redirect
end
end
context "POST to :create" do
should_be_successful_with_a_param_of('1')
should_be_successful_with_a_param_of('2')
should_be_successful_with_a_param_of('3')
should_be_redirected_with_a_param_of('-1')
should_be_redirected_with_a_param_of('0')
end
end
Now if your requirements change so you need to use a new parameter or action, there is very little you'd need to change in your tests
Like this post? Read the rest of the Problems When You Don't Refactor Rails series
- Problems when you don't Refactor Rails: Code Duplication
- Problems when you don't Refactor Rails: Test Duplication
- Problems when you don't Refactor Rails: Wild Estimates
- Problems when you don't Refactor Rails: Lowered morale
- Problems when you don't Refactor Rails: Lower Participation
Tagged: ruby refactoring
written by Eric Davis on August 11, 2010
0 Comments
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
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
# 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
class IssuesController < ApplicationController
# Methods removed
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
# 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
Tagged: ruby redmine refactoring extract-controller extract-class
written by Eric Davis on August 10, 2010
0 Comments
Now that #move and #perform_move are separated, I can start to refactor the duplication they share.
Before
class IssuesController < ApplicationController
def move
@issues.sort!
@copy = params[:copy_options] && params[:copy_options][:copy]
@allowed_projects = Issue.allowed_target_projects_on_move
@target_project = @allowed_projects.detect {|p| p.id.to_s == params[:new_project_id]} if params[:new_project_id]
@target_project ||= @project
@trackers = @target_project.trackers
@available_statuses = Workflow.available_statuses(@project)
render :layout => false if request.xhr?
end
# TODO: more descriptive name? move to separate controller like IssueMovesController?
def perform_move
@issues.sort!
@copy = params[:copy_options] && params[:copy_options][:copy]
@allowed_projects = Issue.allowed_target_projects_on_move
@target_project = @allowed_projects.detect {|p| p.id.to_s == params[:new_project_id]} if params[:new_project_id]
@target_project ||= @project
@trackers = @target_project.trackers
@available_statuses = Workflow.available_statuses(@project)
if request.post?
new_tracker = params[:new_tracker_id].blank? ? nil : @target_project.trackers.find_by_id(params[:new_tracker_id])
unsaved_issue_ids = []
moved_issues = []
@issues.each do |issue|
issue.reload
issue.init_journal(User.current)
call_hook(:controller_issues_move_before_save, { :params => params, :issue => issue, :target_project => @target_project, :copy => !!@copy })
if r = issue.move_to_project(@target_project, new_tracker, {:copy => @copy, :attributes => extract_changed_attributes_for_move(params)})
moved_issues << r
else
unsaved_issue_ids << issue.id
end
end
set_flash_from_bulk_issue_save(@issues, unsaved_issue_ids)
if params[:follow]
if @issues.size == 1 && moved_issues.size == 1
redirect_to :controller => 'issues', :action => 'show', :id => moved_issues.first
else
redirect_to :controller => 'issues', :action => 'index', :project_id => (@target_project || @project)
end
else
redirect_to :controller => 'issues', :action => 'index', :project_id => @project
end
return
end
end
end
After
class IssuesController < ApplicationController
def move
prepare_for_issue_move
render :layout => false if request.xhr?
end
# TODO: more descriptive name? move to separate controller like IssueMovesController?
def perform_move
prepare_for_issue_move
if request.post?
new_tracker = params[:new_tracker_id].blank? ? nil : @target_project.trackers.find_by_id(params[:new_tracker_id])
unsaved_issue_ids = []
moved_issues = []
@issues.each do |issue|
issue.reload
issue.init_journal(User.current)
call_hook(:controller_issues_move_before_save, { :params => params, :issue => issue, :target_project => @target_project, :copy => !!@copy })
if r = issue.move_to_project(@target_project, new_tracker, {:copy => @copy, :attributes => extract_changed_attributes_for_move(params)})
moved_issues << r
else
unsaved_issue_ids << issue.id
end
end
set_flash_from_bulk_issue_save(@issues, unsaved_issue_ids)
if params[:follow]
if @issues.size == 1 && moved_issues.size == 1
redirect_to :controller => 'issues', :action => 'show', :id => moved_issues.first
else
redirect_to :controller => 'issues', :action => 'index', :project_id => (@target_project || @project)
end
else
redirect_to :controller => 'issues', :action => 'index', :project_id => @project
end
return
end
end
def prepare_for_issue_move
@issues.sort!
@copy = params[:copy_options] && params[:copy_options][:copy]
@allowed_projects = Issue.allowed_target_projects_on_move
@target_project = @allowed_projects.detect {|p| p.id.to_s == params[:new_project_id]} if params[:new_project_id]
@target_project ||= @project
@trackers = @target_project.trackers
@available_statuses = Workflow.available_statuses(@project)
end
end
Using extract method, I was able to pull out all of the duplication between #move and #perform_move. #perform_move still has some messy code in it's method body but I think that can wait until after my next set of refactoring, extracting these methods to a new controller.
Reference commit
Tagged: ruby redmine refactoring extract-method
written by Eric Davis on August 10, 2010
0 Comments
I've written about refactoring Redmine here but I feel I haven't explained why refactoring is important. The most common reason is to remove code duplication.
Code Duplication
Code duplication occurs when an application has two sections of code that are identical or almost identical. Sometimes this happens because the application requires the duplication but most of the time the duplication if from a developer being lazy or not having "enough time" (I'll address the "not enough time" excuse later). Two big problems come from code duplication, an increased modification cost and bugs from branching code.
Increased Modification Cost
This problem is a tax that every future developer has to pay. Instead of preventing the code duplication upfront, the original developer tried to save time but just copying and pasting the code. When the next developer comes along, they now have to spend more time to modify that code, since there are now N copies of the code. When they forget to modify one copy, that causes the second problem.
Bugs from Branching Code
When there are N copies of code in an application, they all have to be modified at the same time. Otherwise, you'll end up with a bug fixed in one copy that still occurs in another copy. Repeat this a few times and now you have bugs 2 and 4 fixed in Healthly::Recipe and bugs 1, 3, and 7 fixed in Vegan::Recipe. So are those bugs really fixed?
No
At the worst time possible, someone will run into bug 1 in Healthy::Recipe crash your application, melt your hard drives, alert Skynet, and end the world. Or probably just upset the user, your customer, and make you rush to patch the code.
As the copies diverge more and more, refactoring them back to a common code base gets harder over time. This is one of the biggest problems I have when I'm refactoring Redmine. If I prevented the duplication right from the start or even refactored it early on, the code today would be in a lot better state. Instead the duplicated code has grown, diverged, and takes extra work to clean up.
The sooner you refactor your code duplication, the easier it will be to keep it under control.
Like this post? Read the rest of the Problems When You Don't Refactor Rails series
- Problems when you don't Refactor Rails: Code Duplication
- Problems when you don't Refactor Rails: Test Duplication
- Problems when you don't Refactor Rails: Wild Estimates
- Problems when you don't Refactor Rails: Lowered morale
- Problems when you don't Refactor Rails: Lower Participation
Tagged: ruby refactoring
written by Eric Davis on August 09, 2010
0 Comments
With this refactoring I start making some major changes to IssuesController#move and #peform_move.
Before
class IssuesController < ApplicationController
def move
@issues.sort!
@copy = params[:copy_options] && params[:copy_options][:copy]
@allowed_projects = Issue.allowed_target_projects_on_move
@target_project = @allowed_projects.detect {|p| p.id.to_s == params[:new_project_id]} if params[:new_project_id]
@target_project ||= @project
@trackers = @target_project.trackers
@available_statuses = Workflow.available_statuses(@project)
if request.post?
new_tracker = params[:new_tracker_id].blank? ? nil : @target_project.trackers.find_by_id(params[:new_tracker_id])
unsaved_issue_ids = []
moved_issues = []
@issues.each do |issue|
issue.reload
issue.init_journal(User.current)
call_hook(:controller_issues_move_before_save, { :params => params, :issue => issue, :target_project => @target_project, :copy => !!@copy })
if r = issue.move_to_project(@target_project, new_tracker, {:copy => @copy, :attributes => extract_changed_attributes_for_move(params)})
moved_issues << r
else
unsaved_issue_ids << issue.id
end
end
set_flash_from_bulk_issue_save(@issues, unsaved_issue_ids)
if params[:follow]
if @issues.size == 1 && moved_issues.size == 1
redirect_to :controller => 'issues', :action => 'show', :id => moved_issues.first
else
redirect_to :controller => 'issues', :action => 'index', :project_id => (@target_project || @project)
end
else
redirect_to :controller => 'issues', :action => 'index', :project_id => @project
end
return
end
end
def perform_move
move
end
end
After
class IssuesController < ApplicationController
def move
@issues.sort!
@copy = params[:copy_options] && params[:copy_options][:copy]
@allowed_projects = Issue.allowed_target_projects_on_move
@target_project = @allowed_projects.detect {|p| p.id.to_s == params[:new_project_id]} if params[:new_project_id]
@target_project ||= @project
@trackers = @target_project.trackers
@available_statuses = Workflow.available_statuses(@project)
render :layout => false if request.xhr?
end
# TODO: more descriptive name? move to separate controller like IssueMovesController?
def perform_move
@issues.sort!
@copy = params[:copy_options] && params[:copy_options][:copy]
@allowed_projects = Issue.allowed_target_projects_on_move
@target_project = @allowed_projects.detect {|p| p.id.to_s == params[:new_project_id]} if params[:new_project_id]
@target_project ||= @project
@trackers = @target_project.trackers
@available_statuses = Workflow.available_statuses(@project)
if request.post?
new_tracker = params[:new_tracker_id].blank? ? nil : @target_project.trackers.find_by_id(params[:new_tracker_id])
unsaved_issue_ids = []
moved_issues = []
@issues.each do |issue|
issue.reload
issue.init_journal(User.current)
call_hook(:controller_issues_move_before_save, { :params => params, :issue => issue, :target_project => @target_project, :copy => !!@copy })
if r = issue.move_to_project(@target_project, new_tracker, {:copy => @copy, :attributes => extract_changed_attributes_for_move(params)})
moved_issues << r
else
unsaved_issue_ids << issue.id
end
end
set_flash_from_bulk_issue_save(@issues, unsaved_issue_ids)
if params[:follow]
if @issues.size == 1 && moved_issues.size == 1
redirect_to :controller => 'issues', :action => 'show', :id => moved_issues.first
else
redirect_to :controller => 'issues', :action => 'index', :project_id => (@target_project || @project)
end
else
redirect_to :controller => 'issues', :action => 'index', :project_id => @project
end
return
end
end
end
This caused some intentional duplication of some of #move but now there is a clean separation between #move and #perform_move. Now all I need to do is to remove the duplication for this refactoring.
Reference commit
Tagged: ruby redmine refactoring extract-method
written by Eric Davis on August 05, 2010
0 Comments
Now I want to start refactoring IssuesController#move so it's two separate actions; one to display the form and one to do the actual move. This will make adding more RESTful urls and APIs to Redmine easier.
To start, I create a second method that wraps the existing #move method so I can change the routing and views.
Before
class IssuesController < ApplicationController
def move
# ...
end
end
# app/views/issues/move.rhtml
<% form_tag({}, :id => 'move_form') do %>
class RoutingTest < ActionController::IntegrationTest
context "issues" do
should_route :get, "/issues/1/move", :controller => 'issues', :action => 'move', :id => '1'
should_route :post, "/issues/1/move", :controller => 'issues', :action => 'move', :id => '1'
end
end
After
class IssuesController < ApplicationController
def move
# ...
end
def perform_move
move
end
end
# app/views/issues/move.rhtml
<% form_tag({:action => 'perform_move'}, :id => 'move_form') do %>
class RoutingTest < ActionController::IntegrationTest
context "issues" do
should_route :get, "/issues/1/move", :controller => 'issues', :action => 'move', :id => '1'
should_route :post, "/issues/1/perform_move", :controller => 'issues', :action => 'perform_move', :id => '1'
end
end
There aren't a lot of code changes because I'm taking these refactorings very slow. Since Rails' actions are the actual code that runs when a user requests a page, there is a lot of risk changing them. By building up to the main refactoring slowly, I can make sure each one works before I start on the next.
The reference commit shows some more changes to Redmine that were needed but aren't relevant to the refactoring:
- functional test changes
- permission changes
before_filter changes- route changes
Reference commit
Tagged: ruby redmine refactoring extract-method
written by Eric Davis on August 05, 2010
0 Comments
Whenever I work with legacy code I always turn to extract method and move method for most of my refactoring. Redmine's IssuesController is no exception.
Using extract method on the #move action I was able to extract several lines of code deep in the method and also remove a temporary variable.
Before
class IssuesController < ApplicationController
def move
# ...
@issues.each do |issue|
issue.reload
changed_attributes = {}
[:assigned_to_id, :status_id, :start_date, :due_date].each do |valid_attribute|
unless params[valid_attribute].blank?
changed_attributes[valid_attribute] = (params[valid_attribute] == 'none' ? nil : params[valid_attribute])
end
end
issue.init_journal(User.current)
call_hook(:controller_issues_move_before_save, { :params => params, :issue => issue, :target_project => @target_project, :copy => !!@copy })
if r = issue.move_to_project(@target_project, new_tracker, {:copy => @copy, :attributes => changed_attributes})
moved_issues << r
else
unsaved_issue_ids << issue.id
end
end
# ...
end
end
After
class IssuesController < ApplicationController
def move
# ...
@issues.each do |issue|
issue.reload
issue.init_journal(User.current)
call_hook(:controller_issues_move_before_save, { :params => params, :issue => issue, :target_project => @target_project, :copy => !!@copy })
if r = issue.move_to_project(@target_project, new_tracker, {:copy => @copy, :attributes => extract_changed_attributes_for_move(params)})
moved_issues << r
else
unsaved_issue_ids << issue.id
end
end
# ...
end
def extract_changed_attributes_for_move(params)
changed_attributes = {}
[:assigned_to_id, :status_id, :start_date, :due_date].each do |valid_attribute|
unless params[valid_attribute].blank?
changed_attributes[valid_attribute] = (params[valid_attribute] == 'none' ? nil : params[valid_attribute])
end
end
changed_attributes
end
end
This refactoring helps to remove some of the smells that metric_fu found with #move. It also let me remove the changed_attributes local variable, since it's only used once outside of extract_changed_attributes_for_move.
Reference commit
Tagged: ruby redmine refactoring extract-method
written by Eric Davis on August 04, 2010
0 Comments
I'm starting to refactor Redmine's IssuesController again. It's a very cluttered controller that hasn't been refactored as it's grown, so I'll need to build up my confidence with it over the next few refactorings. To start I used extract method to pull out a new useful utility method to the ApplicationController.
Before
class IssuesController < ApplicationController
def context_menu
# ...
@statuses = IssueStatus.find(:all, :order => 'position')
@back = params[:back_url] || request.env['HTTP_REFERER']
end
end
class ApplicationController < ActionController::Base
# ...
end
After
class IssuesController < ApplicationController
def context_menu
# ...
@statuses = IssueStatus.find(:all, :order => 'position')
@back = back_url
end
end
class ApplicationController < ActionController::Base
def back_url
params[:back_url] || request.env['HTTP_REFERER']
end
end
I'm still trying to figure out a good way to approach the rest of IssuesController. I'm debating between splitting the controller and trying to move all of the queries to the models.
Reference commit
Tagged: ruby redmine refactoring extract-method
written by Eric Davis on August 03, 2010
0 Comments
I wanted to do a refactoring that I noticed while working on my redmine_lock_users plugin. Redmine tracks the User status with a single integer field and provides some constants to access them. The problem with this is that every other class has to know and use those constants to change a status. By using self encapsulate field I was able to make the User class responsible for changing the status and all the caller has to do is to call the correct method.
Before
class AccountController < ApplicationController
def activate
# ...
redirect_to(home_url) && return unless user.status == User::STATUS_REGISTERED
user.status = User::STATUS_ACTIVE
# ...
end
def register_automatically(user, &block)
# ...
user.status = User::STATUS_ACTIVE
user.last_login_on = Time.now
# ...
end
end
After
class AccountController < ApplicationController
def activate
# ...
redirect_to(home_url) && return unless user.registered?
user.activate
# ...
end
def register_automatically(user, &block)
# ...
user.activate
user.last_login_on = Time.now
# ...
end
end
class User < Principal
def activate
self.status = STATUS_ACTIVE
end
def register
self.status = STATUS_REGISTERED
end
def lock
self.status = STATUS_LOCKED
end
def activate!
update_attribute(:status, STATUS_ACTIVE)
end
def register!
update_attribute(:status, STATUS_REGISTERED)
end
def lock!
update_attribute(:status, STATUS_LOCKED)
end
end
Now the caller's code (Controller) is working at a higher level when it wants to change a user status. This helps decouple both classes so I can now modify either one to add new behavior. For example, it would be trivial to add a notification when a user is activated or locked.
Reference commit
Tagged: ruby redmine refactoring self-encapsulate-field
written by Eric Davis on August 02, 2010
0 Comments
I'm starting on my daily refactors again but this time I'm going to focus on the Redmine core codebase. With Redmine's recent 1.0 release, I now have the flexibility to make some major changes to it's internal code to get it ready for some new features.
To start, I wanted to use extract method to refactor a test method into a shoulda macro.
Before
class AccountControllerTest < ActionController::TestCase
context "POST #register" do
context "with self registration on automatic" do
should "create a new user" do
user = User.last(:conditions => {:login => 'register'})
assert user
assert_kind_of User, user
end
end
end
end
After
# test/functional/account_controller_test.rb
class AccountControllerTest < ActionController::TestCase
context "POST #register" do
context "with self registration on automatic" do
should_create_a_new_user { User.last(:conditions => {:login => 'register'}) }
end
end
end
# test/test_helper.rb
class ActiveSupport::TestCase
def self.should_create_a_new_user(&block)
should "create a new user" do
user = instance_eval &block
assert user
assert_kind_of User, user
assert !user.new_record?
end
end
end
I've used this refactoring several times before to extract common test logic. Using shoulda macros in tests make them easier to read and with the help of blocks and instance_eval it's easy to make them work with dynamic data.
Reference commit
Tagged: ruby redmine refactoring extract-method
