Download - Refactoring in Practice - Sunnyconf 2010
![Page 1: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/1.jpg)
REFACTORING IN PRACTICE
![Page 2: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/2.jpg)
WHO IS TEH ME?
Alex Sharp
![Page 3: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/3.jpg)
![Page 4: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/4.jpg)
{ :tweets_as => '@ajsharp' :blogs_at => 'alexjsharp.com' :ships_at => 'github.com/ajsharp' }
![Page 5: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/5.jpg)
_WHY THIS TALK?
![Page 6: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/6.jpg)
APPS GROW UP
![Page 7: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/7.jpg)
APPS GROW UPMATURE
![Page 8: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/8.jpg)
IT’S NOT ALL GREEN FIELDS AND “SOFT LAUNCHES”
![Page 9: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/9.jpg)
so what is this talk about?
![Page 10: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/10.jpg)
large apps are a completely different beast than small apps
![Page 11: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/11.jpg)
domain complexity
![Page 12: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/12.jpg)
tight code coupling across domain concepts
![Page 13: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/13.jpg)
these apps become harder to maintain as size increases
![Page 14: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/14.jpg)
tight domain coupling exposes itself as a symptom of application size
![Page 15: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/15.jpg)
this talk is about working with large apps
![Page 16: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/16.jpg)
it’s about staying on the straight and narrow
![Page 17: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/17.jpg)
it’s about staying on the process
![Page 18: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/18.jpg)
it’s about staying on the habit
![Page 19: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/19.jpg)
it’s about fixing bad code.
![Page 20: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/20.jpg)
it’s about responsibly fixing bad code.
![Page 21: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/21.jpg)
it’s about responsibly fixing bad and improving code.
![Page 22: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/22.jpg)
RULES
OF
REFACTORING
![Page 23: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/23.jpg)
RULE #1TESTS ARE CRUCIAL
![Page 24: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/24.jpg)
we’re out of our element without tests
![Page 25: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/25.jpg)
red, green, refactor doesn’t work without the red
![Page 26: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/26.jpg)
![Page 27: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/27.jpg)
RULE #2AVOID RABBIT HOLES
![Page 28: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/28.jpg)
RULE #3: TAKE SMALL WINS
![Page 29: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/29.jpg)
AVOID THE EPIC REFACTOR(IF POSSIBLE)
![Page 30: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/30.jpg)
what do we mean when we use the term “refactor”
![Page 31: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/31.jpg)
TRADITIONAL DEFINITION
To refactor code is to change it’s implementation while maintaining it’s behavior.
- via Martin Fowler, Kent Beck et. al
![Page 32: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/32.jpg)
in web apps this means that the behavior for the end user remains unchanged
![Page 33: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/33.jpg)
COMMON PROBLEMS(I.E. ANTI-PATTERNS)
![Page 34: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/34.jpg)
LACK OF TECHNICAL LEADERSHIP
• Many common anti-pattens violated consistently
• DRY
• single responsibility
• large procedural code blocks
![Page 35: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/35.jpg)
LACK OF PLATFORM KNOWLEDGE
• Often developers re-engineer features provided by the platform
• frequent offense, but very low-hanging fruit to fix
![Page 36: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/36.jpg)
LACK OF TESTS
• Aggressive deadlines: “there’s no time for tests”
• Lack of testing experience leads to abandonment of tests
• two files with tests in a 150+ model app
• “We’ll add tests later”
A few typical scenarios:
![Page 37: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/37.jpg)
POOR DOMAIN MODELING
• UDD: UI-driven development
• Unfamiliarity with domain modeling
• probably b/c many apps don’t need it
![Page 38: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/38.jpg)
WRANGLING VIEW LOGIC WITH A PRESENTER
![Page 39: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/39.jpg)
COMMON PROBLEMS
• i-var bloat
• results in difficult to test controllers
• high barriers like this typically lead to developers just not testing
• results in brittle views
• instance variables are an implementation, not an interface
![Page 40: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/40.jpg)
THE BILLINGS SCREEN
• Core functionality was simply broken, not working
• This area of the app is somewhat of a “shanty town”
![Page 41: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/41.jpg)
![Page 42: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/42.jpg)
1. READ CODE
![Page 43: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/43.jpg)
class BillingsController < ApplicationController def index @visit_statuses = Visit::Statuses @visit_status = 'Complete' @visit_status = params[:visit_status] unless params[:visit_status].blank?
@billing_status = if @visit_status.upcase != 'COMPLETE' then '' elsif ( params[:billing_status] && params[:billing_status] != '-' ) then params[:billing_status] else Visit::Billing_pending end
@noted = 'true' @noted = 'false' if params[:noted] == 'false' @clinics = current_user.allclinics( @practice.id ) @visits = current_clinic.visits.billable_visits(@billing_status, @visit_status).order_by("visit_date") session[:billing_visits] = @visits.collect{ |v| v.id } endend
![Page 44: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/44.jpg)
class BillingsController < ApplicationController def index @visit_statuses = Visit::Statuses @visit_status = 'Complete' @visit_status = params[:visit_status] unless params[:visit_status].blank?
@billing_status = if @visit_status.upcase != 'COMPLETE' then '' elsif ( params[:billing_status] && params[:billing_status] != '-' ) then params[:billing_status] else Visit::Billing_pending end
@noted = 'true' @noted = 'false' if params[:noted] == 'false' @clinics = current_user.allclinics( @practice.id ) @visits = current_clinic.visits.billable_visits(@billing_status, @visit_status).order_by("visit_date") session[:billing_visits] = @visits.collect{ |v| v.id } endend
Holy @
ivar!Holy @ivar!
Hol
y @
ivar!
Holy @ivar!
Holy @ivar!
Holy @ivar!
Holy @
ivar!
![Page 45: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/45.jpg)
%th=collection_select(:search, :clinic_id, @clinics, :id, :name , {:prompt => "All"}, \ :class => 'filterable')%th=select_tag('search[visit_status_eq]', \ options_for_select( Visit::Statuses.collect(&:capitalize), @visit_status ), {:class => 'filterable'})%th=select_tag('search[billing_status_eq]', \ options_for_select([ "Pending", "Rejected", "Processed" ], @billing_status), {:class => 'filterable'})%th=collection_select(:search, :resource_id, @providers, :id, :full_name, { :prompt => 'All' }, \ :class => 'filterable')%tbody= render :partial => 'visit', :collection => @visits
![Page 46: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/46.jpg)
%th=collection_select(:search, :clinic_id, @clinics, :id, :name , {:prompt => "All"}, \ :class => 'filterable')%th=select_tag('search[visit_status_eq]', \ options_for_select( Visit::Statuses.collect(&:capitalize), @visit_status ), {:class => 'filterable'})%th=select_tag('search[billing_status_eq]', \ options_for_select([ "Pending", "Rejected", "Processed" ], @billing_status), {:class => 'filterable'})%th=collection_select(:search, :resource_id, @providers, :id, :full_name, { :prompt => 'All' }, \ :class => 'filterable')%tbody= render :partial => 'visit', :collection => @visits
![Page 47: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/47.jpg)
SECOND STEP:DOCUMENT SMELLS
![Page 48: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/48.jpg)
SMELLS
• Tricky presentation logic quickly becomes brittle
• Lot’s of hard-to-test, important code
• Needlessly storing constants in ivars
• Craziness going on with @billing_status. Seems important.
![Page 49: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/49.jpg)
class BillingsController < ApplicationController def index @visit_statuses = Visit::Statuses @visit_status = 'Complete' @visit_status = params[:visit_status] unless params[:visit_status].blank?
@billing_status = if @visit_status.upcase != 'COMPLETE' then '' elsif ( params[:billing_status] && params[:billing_status] != '-' ) then params[:billing_status] else Visit::Billing_pending end
@noted = 'true' @noted = 'false' if params[:noted] == 'false' @clinics = current_user.allclinics( @practice.id ) @visits = current_clinic.visits.billable_visits(@billing_status, @visit_status).order_by("visit_date") session[:billing_visits] = @visits.collect{ |v| v.id } endend
seems important...maybe for searching?
![Page 50: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/50.jpg)
class BillingsController < ApplicationController def index @visit_statuses = Visit::Statuses @visit_status = 'Complete' @visit_status = params[:visit_status] unless params[:visit_status].blank?
@billing_status = if @visit_status.upcase != 'COMPLETE' then '' elsif ( params[:billing_status] && params[:billing_status] != '-' ) then params[:billing_status] else Visit::Billing_pending end
@noted = 'true' @noted = 'false' if params[:noted] == 'false' @clinics = current_user.allclinics( @practice.id ) @visits = current_clinic.visits.billable_visits(@billing_status, @visit_status).order_by("visit_date") session[:billing_visits] = @visits.collect{ |v| v.id } endend
WTF!?!?
![Page 51: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/51.jpg)
A QUICK NOTE ABOUT“PERPETUAL WTF SYNDROME”
![Page 52: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/52.jpg)
TOO MANY WTF’S WILL DRIVE YOU NUTS
![Page 53: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/53.jpg)
AWFUL CODE IS DEMORALIZING
![Page 54: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/54.jpg)
YOU FORGET THAT GOOD CODE EXISTS
![Page 55: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/55.jpg)
THAT’S ONE REASON WHY THE NEXT STEP IS SO
IMPORTANT
![Page 56: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/56.jpg)
IT’S KEEPS THE DIALOGUE POSITIVE
![Page 57: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/57.jpg)
3. IDENTIFY ENGINEERING GOALS
![Page 58: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/58.jpg)
ENGINEERING GOALS
• Program to an interface, not an implementation
• Only one instance variable
• Use extract class to wrap up presentation logic
![Page 59: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/59.jpg)
describe BillingsPresenter do before do @practice = mock_model(Practice) @user = mock_model(User, :practice => @practice) @presenter = BillingsPresenter.new({}, @user, @practice) end
describe '#visits' do it 'should receive billable_visits' do @practice.should_receive(:billable_visits) @presenter.visits end end
describe '#visit_status' do it 'should default to the complete status' do @presenter.visit_status.should == "Complete" end end
describe '#billing_status' do it 'should default to default_billing_status' do @presenter.billing_status.should == BillingsPresenter.default_billing_status end
it 'should use the params[:billing_status] if it exists' do presenter = BillingsPresenter.new({:billing_status => 'use me'}, @user, @practice) presenter.billing_status.should == 'use me' end end
describe '#clinics' do it 'should receive user.allclinics' do @user.should_receive(:allclinics).with(@practice.id) @presenter.clinics end end
describe '#providers' do it 'should get the providers from the visit' do
visit = mock_model Visit @presenter.stub!(:visits).and_return([visit]) visit.should_receive(:resource)
@presenter.providers end end
describe '.default_visit_status' do it 'is the capitalized version of COMPLETE' do BillingsPresenter.default_visit_status.should == 'Complete' end end
describe '.default_billing_status' do it 'is the capitalized version of COMPLETE' do BillingsPresenter.default_billing_status.should == 'Pending' end end
describe '#visit_statuses' do it 'is the capitalized version of the visit statuses' do @presenter.visit_statuses.should == [['All', '']] + Visit::Statuses.collect(&:capitalize) end end
describe '#billing_stauses' do it 'is the capitalized version of the billing statuses' do @presenter.billing_statuses.should == [['All', '']] + Visit::Billing_statuses.collect(&:capitalize) end end
end
![Page 60: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/60.jpg)
class BillingsPresenter attr_reader :params, :user, :practice def initialize(params, user, practice) @params = params @user = user @practice = practice end
def self.default_visit_status Visit::COMPLETE.capitalize end
def self.default_billing_status Visit::Billing_pending.capitalize end
def visits @visits ||= practice.billable_visits(params[:search]) end
def visit_status params[:visit_status] || self.class.default_visit_status end
def billing_status params[:billing_status] || self.class.default_billing_status end
def clinics @clinics ||= user.allclinics practice.id end
def providers @providers ||= visits.collect(&:resource).uniq end
def visit_statuses [['All', '']] + Visit::Statuses.collect(&:capitalize) end
def billing_statuses [['All', '']] + Visit::Billing_statuses.collect(&:capitalize) endend
![Page 61: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/61.jpg)
class BillingsController < ApplicationController def index @presenter = BillingsPresenter.new params, current_user, current_practice
session[:billing_visits] = @presenter.visits.collect{ |v| v.id } endend
![Page 62: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/62.jpg)
%th=collection_select(:search, :clinic_id, @presenter.clinics, :id, :name , {:prompt => "All"}, \ :class => 'filterable')%th=select_tag('search[visit_status_eq]', \ options_for_select( Visit::Statuses.collect(&:capitalize), @presenter.visit_status ), { \ :class => 'filterable'})%th=select_tag('search[billing_status_eq]', \ options_for_select([ "Pending", "Rejected", "Processed" ], @presenter.billing_status), {:class => 'filterable'})%th=collection_select(:search, :resource_id, @presenter.providers, :id, :full_name, { :prompt => 'All' }, \ :class => 'filterable')%tbody= render :partial => 'visit', :collection => @presenter.visits
![Page 63: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/63.jpg)
WINS
• Much cleaner, more testable controller
• Much less brittle view template
• The behavior of this action actually works
![Page 64: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/64.jpg)
FAT MODEL / SKINNY CONTROLLER
![Page 65: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/65.jpg)
UPDATE ACTION CRAZINESS
• Update a collection of associated objects in the parent object’s update action
![Page 66: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/66.jpg)
Medical History
Existing Condition
join model*
*
Object Model
![Page 67: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/67.jpg)
STEP 1: READ CODE
![Page 68: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/68.jpg)
def update @medical_history = @patient.medical_histories.find(params[:id])
@medical_history.taken_date = Date.today if @medical_history.taken_date.nil?
success = true conditions = @medical_history.existing_conditions_medical_histories.find(:all)
respond_to do |format| if @medical_history.update_attributes(params[:medical_history]) && success format.html { flash[:notice] = 'Medical History was successfully updated.' redirect_to( patient_medical_histories_url(@patient) ) } format.xml { head :ok } format.js else format.html { render :action => "edit" } format.xml { render :xml => @medical_history.errors, :status => :unprocessable_entity } format.js { render :text => "Error Updating History", :status => :unprocessable_entity} end endend
params[:conditions].each_pair do |key,value| condition_id = key.to_s[10..-1].to_i condition = conditions.detect {|x| x.existing_condition_id == condition_id } if condition.nil? success = @medical_history.existing_conditions_medical_histories.create( :existing_condition_id => condition_id, :has_condition => value) && success elsif condition.has_condition != value success = condition.update_attribute(:has_condition, value) && success end end
![Page 69: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/69.jpg)
def update @medical_history = @patient.medical_histories.find(params[:id])
@medical_history.taken_date = Date.today if @medical_history.taken_date.nil?
success = true conditions = @medical_history.existing_conditions_medical_histories.find(:all)
respond_to do |format| if @medical_history.update_attributes(params[:medical_history]) && success format.html { flash[:notice] = 'Medical History was successfully updated.' redirect_to( patient_medical_histories_url(@patient) ) } format.xml { head :ok } format.js else format.html { render :action => "edit" } format.xml { render :xml => @medical_history.errors, :status => :unprocessable_entity } format.js { render :text => "Error Updating History", :status => :unprocessable_entity} end endend
params[:conditions].each_pair do |key,value| condition_id = key.to_s[10..-1].to_i condition = conditions.detect {|x| x.existing_condition_id == condition_id } if condition.nil? success = @medical_history.existing_conditions_medical_histories.create( :existing_condition_id => condition_id, :has_condition => value) && success elsif condition.has_condition != value success = condition.update_attribute(:has_condition, value) && success end end
smell-fest
![Page 70: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/70.jpg)
STEP 2: DOCUMENT SMELLS
![Page 71: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/71.jpg)
Lot’s of violations:
1. Fat model, skinny controller
2. Single responsibility
3. Not modular
4. Repetition of rails
5. Really hard to fit on a slide
![Page 72: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/72.jpg)
Let’s focus here
conditions = @medical_history.existing_conditions_medical_histories.find(:all)
params[:conditions].each_pair do |key,value| condition_id = key.to_s[10..-1].to_i condition = conditions.detect {|x| x.existing_condition_id == condition_id } if condition.nil? success = @medical_history.existing_conditions_medical_histories.create( :existing_condition_id => condition_id, :has_condition => value) && success elsif condition.has_condition != value success = condition.update_attribute(:has_condition, value) && success end end
![Page 73: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/73.jpg)
STEP 3: IDENTIFY ENGINEERING GOALS
![Page 74: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/74.jpg)
I want to refactor in the following ways:
1. Push this code into the model
2. Extract varying logic into separate methods
![Page 75: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/75.jpg)
We need to write some characterization tests and get this action under test so we can figure out
what it’s doing.
![Page 76: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/76.jpg)
This way we can rely on an automated testing
workflow for instant feedback
![Page 77: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/77.jpg)
describe MedicalHistoriesController, "PUT update" do context "successful" do before :each do @user = Factory(:practice_admin) @patient = Factory(:patient_with_medical_histories, :practice => @user.practice) @medical_history = @patient.medical_histories.first @condition1 = Factory(:existing_condition) @condition2 = Factory(:existing_condition)
stub_request_before_filters @user, :practice => true, :clinic => true
params = { :conditions => { "condition_#{@condition1.id}" => "true", "condition_#{@condition2.id}" => "true" }, :id => @medical_history.id, :patient_id => @patient.id }
put :update, params end it { should redirect_to patient_medical_histories_url } it { should_not render_template :edit }
it "should successfully save a collection of conditions" do @medical_history.existing_conditions.should include @condition1 @medical_history.existing_conditions.should include @condition2 end endend
![Page 78: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/78.jpg)
conditions = @medical_history.existing_conditions_medical_histories.find(:all)
params[:conditions].each_pair do |key,value| condition_id = key.to_s[10..-1].to_i condition = conditions.detect {|x| x.existing_condition_id == condition_id } if condition.nil? success = @medical_history.existing_conditions_medical_histories.create( :existing_condition_id => condition_id, :has_condition => value) && success elsif condition.has_condition != value success = condition.update_attribute(:has_condition, value) && success end end
we can eliminate this, b/c it’s an association of this model
![Page 79: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/79.jpg)
params[:conditions].each_pair do |key,value| condition_id = key.to_s[10..-1].to_i condition = existing_conditions_medical_histories.detect {|x| x.existing_condition_id == condition_id } if condition.nil? success = existing_conditions_medical_histories.create( :existing_condition_id => condition_id, :has_condition => value) && success elsif condition.has_condition != value success = condition.update_attribute(:has_condition, value) && success end end
Next, do extract method on this line
![Page 80: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/80.jpg)
params[:conditions].each_pair do |key,value| condition_id = get_id_from_string(key) condition = existing_conditions_medical_histories.detect {|x| x.existing_condition_id == condition_id } if condition.nil? success = existing_conditions_medical_histories.create( :existing_condition_id => condition_id, :has_condition => value) && success elsif condition.has_condition != value success = condition.update_attribute(:has_condition, value) && success end end
Cover this new method w/ tests
![Page 81: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/81.jpg)
params[:conditions].each_pair do |key,value| condition_id = get_id_from_string(key) condition = existing_conditions_medical_histories.detect {|x| x.existing_condition_id == condition_id } if condition.nil? success = existing_conditions_medical_histories.create( :existing_condition_id => condition_id, :has_condition => value) && success elsif condition.has_condition != value success = condition.update_attribute(:has_condition, value) && success end end
???
![Page 82: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/82.jpg)
params[:conditions].each_pair do |key,value| condition_id = get_id_from_string(key) condition = existing_conditions_medical_histories.detect {|x| x.existing_condition_id == condition_id } if condition.nil? success = existing_conditions_medical_histories.create( :existing_condition_id => condition_id, :has_condition => value) && success elsif condition.has_condition != value success = condition.update_attribute(:has_condition, value) && success end end
i.e. ecmh.find(:first, :conditions => { :existing_condition_id => condition_id })
![Page 83: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/83.jpg)
params[:conditions].each_pair do |key,value| condition_id = get_id_from_string(key) condition = existing_conditions_medical_histories.detect {|x| x.existing_condition_id == condition_id } if condition.nil? success = existing_conditions_medical_histories.create( :existing_condition_id => condition_id, :has_condition => value) && success elsif condition.has_condition != value success = condition.update_attribute(:has_condition, value) && success end end
do extract method here...
![Page 84: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/84.jpg)
params[:conditions].each_pair do |key,value| condition_id = get_id_from_string(key) condition = find_existing_condition(condition_id) if condition.nil? success = existing_conditions_medical_histories.create( :existing_condition_id => condition_id, :has_condition => value) && success elsif condition.has_condition != value success = condition.update_attribute(:has_condition, value) && success end end
???
![Page 85: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/85.jpg)
params[:conditions].each_pair do |key,value| condition_id = get_id_from_string(key) condition = find_existing_condition(condition_id) if condition.nil? success = existing_conditions_medical_histories.create( :existing_condition_id => condition_id, :has_condition => value) && success elsif condition.has_condition != value success = condition.update_attribute(:has_condition, value) && success end end
i.e. create or update
![Page 86: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/86.jpg)
describe MedicalHistory, "#update_conditions" do context "when passed a condition that is not an existing conditions" do before do @medical_history = Factory(:medical_history) @condition = Factory(:existing_condition) @medical_history.update_conditions({ "condition_#{@condition.id}" => "true" }) end it "should add the condition to the existing_condtions association" do @medical_history.existing_conditions.should include @condition end it "should set the :has_condition attribute to the value that was passed in" do @medical_history.existing_conditions_medical_histories.first.has_condition.should == true end end
context "when passed a condition that is an existing condition" do before do ecmh = Factory(:existing_conditions_medical_history, :has_condition => true) @medical_history = ecmh.medical_history @condition = ecmh.existing_condition @medical_history.update_conditions({ "condition_#{@condition.id}" => "false" }) end it "should update the existing_condition_medical_history record" do @medical_history.existing_conditions.should_not include @condition end it "should set the :has_condition attribute to the value that was passed in" do @medical_history.existing_conditions_medical_histories.first.has_condition.should == false end end end
![Page 87: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/87.jpg)
def update_conditions(conditions_param = {}) conditions_param.each_pair do |key, value| create_or_update_condition(get_id_from_string(key), value) end if conditions_param end private def create_or_update_condition(condition_id, value) condition = existing_conditions_medical_histories.find_by_existing_condition_id(condition_id) condition.nil? ? create_condition(condition_id, value) : update_condition(condition, value) end
def create_condition(condition_id, value) existing_conditions_medical_histories.create( :existing_condition_id => condition_id, :has_condition => value ) end
def update_condition(condition, value) condition.update_attribute(:has_condition, value) unless condition.has_condition == value end
![Page 88: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/88.jpg)
def update @medical_history = @patient.medical_histories.find(params[:id])
@medical_history.taken_date = Date.today if @medical_history.taken_date.nil?
success = true conditions = @medical_history.existing_conditions_medical_histories.find(:all)
respond_to do |format| if @medical_history.update_attributes(params[:medical_history]) && success format.html { flash[:notice] = 'Medical History was successfully updated.' redirect_to( patient_medical_histories_url(@patient) ) } format.xml { head :ok } format.js else format.html { render :action => "edit" } format.xml { render :xml => @medical_history.errors, :status => :unprocessable_entity } format.js { render :text => "Error Updating History", :status => :unprocessable_entity} end endend
params[:conditions].each_pair do |key,value| condition_id = key.to_s[10..-1].to_i condition = conditions.detect {|x| x.existing_condition_id == condition_id } if condition.nil? success = @medical_history.existing_conditions_medical_histories.create( :existing_condition_id => condition_id, :has_condition => value) && success elsif condition.has_condition != value success = condition.update_attribute(:has_condition, value) && success end end
We went from this mess in the controller
![Page 89: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/89.jpg)
def update @medical_history = @patient.medical_histories.find(params[:id]) @medical_history.taken_date = Date.today if @medical_history.taken_date.nil?
respond_to do |format| if( @medical_history.update_attributes(params[:medical_history]) && @medical_history.update_conditions(params[:conditions]) ) format.html { flash[:notice] = 'Medical History was successfully updated.' redirect_to( patient_medical_histories_url(@patient) ) } format.js else format.html { render :action => "edit" } format.js { render :text => "Error Updating History", :status => :unprocessable_entity } end end end
To this in the controller...
![Page 90: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/90.jpg)
and this in the model
def update_conditions(conditions_param = {}) conditions_param.each_pair do |key, value| create_or_update_condition(get_id_from_string(key), value) end if conditions_param end private def create_or_update_condition(condition_id, value) condition = existing_conditions_medical_histories.find_by_existing_condition_id(condition_id) condition.nil? ? create_condition(condition_id, value) : update_condition(condition, value) end
def create_condition(condition_id, value) existing_conditions_medical_histories.create( :existing_condition_id => condition_id, :has_condition => value ) end
def update_condition(condition, value) condition.update_attribute(:has_condition, value) unless condition.has_condition == value end
![Page 91: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/91.jpg)
IT’S NOT PERFECT...
![Page 92: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/92.jpg)
def update @medical_history = @patient.medical_histories.find(params[:id]) @medical_history.taken_date = Date.today if @medical_history.taken_date.nil?
respond_to do |format| if( @medical_history.update_attributes(params[:medical_history]) && @medical_history.update_conditions(params[:conditions]) ) format.html { flash[:notice] = 'Medical History was successfully updated.' redirect_to( patient_medical_histories_url(@patient) ) } format.js else format.html { render :action => "edit" } format.js { render :text => "Error Updating History", :status => :unprocessable_entity } end end end
belongs in model
maybe a DB transaction? maybe nested params?
![Page 93: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/93.jpg)
BUT REFACTORING MUST BE DONE IN SMALL STEPS
![Page 94: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/94.jpg)
RECAP: WINS
• improved controller action API
• intention-revealing method names communicate what’s occurring at each stage of the update
• clean, testable public model API
![Page 95: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/95.jpg)
PROCESS RECAP
![Page 96: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/96.jpg)
1. READ CODE
![Page 97: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/97.jpg)
2. DOCUMENT SMELLS
![Page 98: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/98.jpg)
3. IDENTIFY ENGINEERING GOALS
![Page 99: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/99.jpg)
“We’re going to push this nastiness down into the model. That way we can get it tested, so we can more easily change it in
the future.”
![Page 100: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/100.jpg)
Rate my talk: http://bit.ly/ajsharp-sunnyconf-refactoring
Slides: http://slidesha.re/ajsharp-sunnyconf-refactoring-slides
![Page 101: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/101.jpg)
RESOURCES
• Talks/slides
• Rick Bradley’s railsconf slides: http://railsconf2010.rickbradley.com/
• Rick’s (beardless) hoedown 08 talk: http://tinyurl.com/flogtalk
• Books
• Working Effectively with Legacy Code by Michael Feathers
• Refactoring: Ruby Edition by Jay Fields, Martin Fowler, et. al
• Domain Driven Design by Eric Evans
![Page 103: Refactoring in Practice - Sunnyconf 2010](https://reader038.vdocuments.us/reader038/viewer/2022102823/5482bfceb4af9f90098b457f/html5/thumbnails/103.jpg)
THANKS!