Ben Lavender
Adventures
in
Refactoring Ben Lavender
About me and the talk •
@bhuga on github, twitter, TLDs
!
About me and the talk •
@bhuga on github, twitter, TLDs, but not snapchat
!
About me and the talk •
@bhuga on github, twitter, TLDs, but not snapchat
•
Work at GitHub on internal tools
About me and the talk •
@bhuga on github, twitter, TLDs, but not snapchat
•
Work at GitHub on internal tools
•
Has refactored a lot of “quick one offs” that turned important
if foo if bar if fum puts "yeah!" end end end !
-> !
return unless foo && bar && fum puts "yeah!"
What is Refactoring?
What is Refactoring? Changing code without changing behavior.
What is Refactoring? trying to
Changing code without ^ change behavior.
Bad reasons to Refactor
Bad reasons to Refactor
•
Increase consistency
x = "foo" y = 'bar' !
-> !
x = "foo" y = "bar"
Bad reasons to Refactor
•
Increase consistency
•
DRY things up
user = load_user widgets = user.widgets !
user = load_user widgets = user.widgets !
-> load_user_widgets
Bad reasons to Refactor •
Increase consistency
•
DRY things up
•
Improve abstractions
user.get_first_address_city !
-> user.addresses.first.city
Why were those
Bad
Reasons
!
?
Good software projects
Measure
Success
Measuring
Refactoring
Success
Measuring Success:
Maintain
Correctness
user.get_first_address_city !
-> user.addresses.first.city
user.get_first_address_city # "Toledo" !
-> user.addresses.first.city # "Akron"
Measuring Success:
The
Almighty
Diffstat
🔪 :hocho:
Measuring Success:
Improved
Test
Coverage
# 100% test coverage! if foo puts "foo!" end !
if bar explode if foo puts "bar!" end
Measuring Success:
Improved
Test
Expressability
describe "deployment" do context "when master needs merging" do context "when CI is green" do # test context "when CI is not green" do # test context "when branch is up-to-date" do context "when CI is green" do # test context "when CI is not green" do # test
class ChecklistItem ... end class MasterMerged < ChecklistItem ... end class CIGreen < ChecklistItem ... end
class Deployment def run_checklist! checklist.each do |checklist_item| # checklist_item is # a class like MasterMerged checklist_item.new(self).check
describe MasterMerged do it "merges master if required" it "does nothing if master merge not required"
!
describe CIGreen do it "adds a blocker if CI is not green" it "lets the deployment continue if CI is green"
Measuring Success:
Improved
Performance
Measuring Success:
Developer
Happiness
Measuring Success:
Developer
Happiness
😁
Focus on Metrics
Reasons
to
Refactor
Reason 1:
Developer
Happiness
Reason 2:
Increase
Performance
Reason 3:
Gain Confidence
for
Future Work
Reason 3:
Gain Confidence
for
Future Work
AKA paying off technical debt
Reason 4:
Developer Education
Part 2:
Techniques
Technique 1: Little
bits
of
Happy
if foo if bar if fum puts "yeah!" end end end !
-> !
return unless foo && bar && fum puts "yeah!"
user = load_user widgets = user.widgets !
user = load_user widgets = user.widgets !
-> load_user_widgets
user.get_first_address_city !
-> user.addresses.first.city
x = "foo" y = 'bar' !
-> !
x = "foo" y = "bar"
if some_pretty_long_condition ? "foo string" : "bar string" -> return "foo string" if some_pretty_long_condition "bar string"
object.some_method "foo", :fi_fo_fum => widget, :baz => fum, :free => fallin !
-> object.some_method "foo", :fi_fo_fum => widget, :baz => fum, :free => fallin
Get
a
Styleguide
!
$ go fmt
Technique 2:
Types
for
Verbs
class Account def transfer(amount, other_account) other_account.add(amount) self.subtract(amount) end end
class Account def transfer(amount, other_account, at) if at BackgroundJob.schedule(at, self, :transfer, amount, other_account) return end other_account.add(amount) self.subtract(amount) end end
def Transaction def initialize(@from, @to, @amount, @at) BackgroundJob.schedule(self, @at) end def run(from, to, amount) from.subtract(amount) to.add(amount) end end
class Account def transfer(amount, other_account, at) Transaction.new(self, other_account, amount, at) end end
transaction.successful? transaction.finished_at transaction.failure_reason
Technique 3:
Add
useful
Abstractions
pull.branch_valid? pull.branch_exists?
pull.branch.valid? pull.branch.exists?
pull.git_mergeable? pull.status_mergeable?
if pull.git_mergeable? show_merge_button else show_disabled_merge_button end
if pull.stable? show_merge_button else show_disabled_merge_button end
pull.merge_status.okay? pull.merge_status.git_okay? pull.merge_status.ci_okay?
class PullRequest def git_mergeable? end def status_mergeable? end def stable? git_mergeable? && status_mergeable? end end
class MergeState def initialize(pull) @pull = pull end def git_okay? end def ci_okay? end def stable? git_okay? && ci_okay? end end
Technique 4:
Remove
unused
Abstractions
class CampfireAdapter < ChatAdapter #... end !
class ChatAdapter def post(room, message) room = lookup_room(room) room.post(message) end end
chat = ChatAdapter.new(:campfire) chat.post(room, message)
class Campfire def self.post(room_id, message) RestClient.post "rooms/room_id", {:message => message } end end
Campfire.post room, message
Problems
Problem 1:
Existing
Bugs
Problem 2:
Performance
Changes
Problem 3:
Context
gitrpc.client do |c| c.fetch(file) end
gitrpc.client do |c| c.fetch(file, 4096) end
class TreeEntry def data ... end end
fetch(file) find_file_by_name(file) setup_gitrpc_client(…) rpc_client_connect(…) entry = rpc_get_current_tree(…) entry.data
module GitRPC def head_bytes(file) fetch(file).head_bytes end def tail_bytes(file) fetch(file).tail_bytes end end
module GitRPC def head_bytes(file) fetch(file, 4096).head_bytes end def tail_bytes(file) fetch(file).tail_bytes end end
Big
Refactorings
are a
Risk
Break
them
Down
Breaking it down 1:
Deprecate
module GitRPC # deprecated def fetch(file) end def fetch_with_limit(file, size) end end
module GitRPC def fetch(file) end deprecate :fetch, "Migrate to fetch_with_limit plz" def fetch_with_limit(file, size) end end
module GitRPC def head_bytes(file) fetch(file).head_bytes end def tail_bytes(file) fetch(file).tail_bytes end end
module GitRPC def fetch(file) end deprecate :fetch, "Migrate to fetch_with_limit plz" def tail_bytes(file) # Not deprecated here, really fetch(file).tail_bytes end end
Breaking it down 2:
Backwards
Compatible
Design
-------------- id | current | -------------- 1 | false | 2 | true | --------------
-------------- id | current | -------------- 1 | true | 2 | true | --------------
class Widget def current_widget find_by(:current => true) end -> def current_widget find_all(:current => true).last end end
!
->
Big
Refactorings
are a
Risk
Write
Tools
Backscatter
def data backscatter_trace 72 load_data end
Science
def get_widgets widgetify end
def get_widgets widgetify end !
def get_widgets_new load_widgets_from_database end
def get_widgets science "widgets.loading" do |e| e.use { get_widgets_old } e.try { get_widgets_new } end end
GitHub.subscribe "science.mismatch" do |payload| puts "old value returned #{payload.control}" puts "new value returned #{payload.candidate}" end
https://rubygems.org/gems/scientist
Part 3: Complaining
Editor-level facilities
Editor-level facilities
•
“Really good search/replace”
Editor-level facilities
•
“Really good search/replace”
•
“Extract resource”
Language Facilities
Language Facilities
•
Java @deprecate annotation
/** * @deprecated As of release 1.3, replaced by {@link #getPreferredSize()} */ @Deprecated public Dimension preferredSize() { return getPreferredSize(); }
Language Facilities
•
Java @deprecate annotation
•
C deprecation pragma
[[deprecated("Replaced by bar")]] void foo(int);
Compile-time deprecation sucks
$x = 1 | 2 x == 1 # true x == 2 # true $x = $x * 3 x == 3 && x == 6 # true
Example: nothing!
Go refactor I’m all done
in
Refactoring Ben Lavender
About me and the talk •
@bhuga on github, twitter, TLDs
!
About me and the talk •
@bhuga on github, twitter, TLDs, but not snapchat
!
About me and the talk •
@bhuga on github, twitter, TLDs, but not snapchat
•
Work at GitHub on internal tools
About me and the talk •
@bhuga on github, twitter, TLDs, but not snapchat
•
Work at GitHub on internal tools
•
Has refactored a lot of “quick one offs” that turned important
if foo if bar if fum puts "yeah!" end end end !
-> !
return unless foo && bar && fum puts "yeah!"
What is Refactoring?
What is Refactoring? Changing code without changing behavior.
What is Refactoring? trying to
Changing code without ^ change behavior.
Bad reasons to Refactor
Bad reasons to Refactor
•
Increase consistency
x = "foo" y = 'bar' !
-> !
x = "foo" y = "bar"
Bad reasons to Refactor
•
Increase consistency
•
DRY things up
user = load_user widgets = user.widgets !
user = load_user widgets = user.widgets !
-> load_user_widgets
Bad reasons to Refactor •
Increase consistency
•
DRY things up
•
Improve abstractions
user.get_first_address_city !
-> user.addresses.first.city
Why were those
Bad
Reasons
!
?
Good software projects
Measure
Success
Measuring
Refactoring
Success
Measuring Success:
Maintain
Correctness
user.get_first_address_city !
-> user.addresses.first.city
user.get_first_address_city # "Toledo" !
-> user.addresses.first.city # "Akron"
Measuring Success:
The
Almighty
Diffstat
🔪 :hocho:
Measuring Success:
Improved
Test
Coverage
# 100% test coverage! if foo puts "foo!" end !
if bar explode if foo puts "bar!" end
Measuring Success:
Improved
Test
Expressability
describe "deployment" do context "when master needs merging" do context "when CI is green" do # test context "when CI is not green" do # test context "when branch is up-to-date" do context "when CI is green" do # test context "when CI is not green" do # test
class ChecklistItem ... end class MasterMerged < ChecklistItem ... end class CIGreen < ChecklistItem ... end
class Deployment def run_checklist! checklist.each do |checklist_item| # checklist_item is # a class like MasterMerged checklist_item.new(self).check
describe MasterMerged do it "merges master if required" it "does nothing if master merge not required"
!
describe CIGreen do it "adds a blocker if CI is not green" it "lets the deployment continue if CI is green"
Measuring Success:
Improved
Performance
Measuring Success:
Developer
Happiness
Measuring Success:
Developer
Happiness
😁
Focus on Metrics
Reasons
to
Refactor
Reason 1:
Developer
Happiness
Reason 2:
Increase
Performance
Reason 3:
Gain Confidence
for
Future Work
Reason 3:
Gain Confidence
for
Future Work
AKA paying off technical debt
Reason 4:
Developer Education
Part 2:
Techniques
Technique 1: Little
bits
of
Happy
if foo if bar if fum puts "yeah!" end end end !
-> !
return unless foo && bar && fum puts "yeah!"
user = load_user widgets = user.widgets !
user = load_user widgets = user.widgets !
-> load_user_widgets
user.get_first_address_city !
-> user.addresses.first.city
x = "foo" y = 'bar' !
-> !
x = "foo" y = "bar"
if some_pretty_long_condition ? "foo string" : "bar string" -> return "foo string" if some_pretty_long_condition "bar string"
object.some_method "foo", :fi_fo_fum => widget, :baz => fum, :free => fallin !
-> object.some_method "foo", :fi_fo_fum => widget, :baz => fum, :free => fallin
Get
a
Styleguide
!
$ go fmt
Technique 2:
Types
for
Verbs
class Account def transfer(amount, other_account) other_account.add(amount) self.subtract(amount) end end
class Account def transfer(amount, other_account, at) if at BackgroundJob.schedule(at, self, :transfer, amount, other_account) return end other_account.add(amount) self.subtract(amount) end end
def Transaction def initialize(@from, @to, @amount, @at) BackgroundJob.schedule(self, @at) end def run(from, to, amount) from.subtract(amount) to.add(amount) end end
class Account def transfer(amount, other_account, at) Transaction.new(self, other_account, amount, at) end end
transaction.successful? transaction.finished_at transaction.failure_reason
Technique 3:
Add
useful
Abstractions
pull.branch_valid? pull.branch_exists?
pull.branch.valid? pull.branch.exists?
pull.git_mergeable? pull.status_mergeable?
if pull.git_mergeable? show_merge_button else show_disabled_merge_button end
if pull.stable? show_merge_button else show_disabled_merge_button end
pull.merge_status.okay? pull.merge_status.git_okay? pull.merge_status.ci_okay?
class PullRequest def git_mergeable? end def status_mergeable? end def stable? git_mergeable? && status_mergeable? end end
class MergeState def initialize(pull) @pull = pull end def git_okay? end def ci_okay? end def stable? git_okay? && ci_okay? end end
Technique 4:
Remove
unused
Abstractions
class CampfireAdapter < ChatAdapter #... end !
class ChatAdapter def post(room, message) room = lookup_room(room) room.post(message) end end
chat = ChatAdapter.new(:campfire) chat.post(room, message)
class Campfire def self.post(room_id, message) RestClient.post "rooms/room_id", {:message => message } end end
Campfire.post room, message
Problems
Problem 1:
Existing
Bugs
Problem 2:
Performance
Changes
Problem 3:
Context
gitrpc.client do |c| c.fetch(file) end
gitrpc.client do |c| c.fetch(file, 4096) end
class TreeEntry def data ... end end
fetch(file) find_file_by_name(file) setup_gitrpc_client(…) rpc_client_connect(…) entry = rpc_get_current_tree(…) entry.data
module GitRPC def head_bytes(file) fetch(file).head_bytes end def tail_bytes(file) fetch(file).tail_bytes end end
module GitRPC def head_bytes(file) fetch(file, 4096).head_bytes end def tail_bytes(file) fetch(file).tail_bytes end end
Big
Refactorings
are a
Risk
Break
them
Down
Breaking it down 1:
Deprecate
module GitRPC # deprecated def fetch(file) end def fetch_with_limit(file, size) end end
module GitRPC def fetch(file) end deprecate :fetch, "Migrate to fetch_with_limit plz" def fetch_with_limit(file, size) end end
module GitRPC def head_bytes(file) fetch(file).head_bytes end def tail_bytes(file) fetch(file).tail_bytes end end
module GitRPC def fetch(file) end deprecate :fetch, "Migrate to fetch_with_limit plz" def tail_bytes(file) # Not deprecated here, really fetch(file).tail_bytes end end
Breaking it down 2:
Backwards
Compatible
Design
-------------- id | current | -------------- 1 | false | 2 | true | --------------
-------------- id | current | -------------- 1 | true | 2 | true | --------------
class Widget def current_widget find_by(:current => true) end -> def current_widget find_all(:current => true).last end end
!
->
Big
Refactorings
are a
Risk
Write
Tools
Backscatter
def data backscatter_trace 72 load_data end
Science
def get_widgets widgetify end
def get_widgets widgetify end !
def get_widgets_new load_widgets_from_database end
def get_widgets science "widgets.loading" do |e| e.use { get_widgets_old } e.try { get_widgets_new } end end
GitHub.subscribe "science.mismatch" do |payload| puts "old value returned #{payload.control}" puts "new value returned #{payload.candidate}" end
https://rubygems.org/gems/scientist
Part 3: Complaining
Editor-level facilities
Editor-level facilities
•
“Really good search/replace”
Editor-level facilities
•
“Really good search/replace”
•
“Extract resource”
Language Facilities
Language Facilities
•
Java @deprecate annotation
/** * @deprecated As of release 1.3, replaced by {@link #getPreferredSize()} */ @Deprecated public Dimension preferredSize() { return getPreferredSize(); }
Language Facilities
•
Java @deprecate annotation
•
C deprecation pragma
[[deprecated("Replaced by bar")]] void foo(int);
Compile-time deprecation sucks
$x = 1 | 2 x == 1 # true x == 2 # true $x = $x * 3 x == 3 && x == 6 # true
Example: nothing!
Go refactor I’m all done
