Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Clean Code

Clean Code

A fuller version of my previous clean code presentation

Luís Ferreira

September 16, 2014
Tweet

More Decks by Luís Ferreira

Other Decks in Programming

Transcript

  1. 1. Looks like you cared 2. Pretty much what you

    expected 3. Language looks as perfect fit for problem
  2. 1. Looks like you cared 2. Pretty much what you

    expected 3. Language looks as perfect fit for problem My definition
  3. class Report def initialize @title = 'Monthly Report' @text =

    [ 'Things are going', 'really, really well.' ] end ! def output_report ...HTML specific stuff... end end template method: reports
  4. bad code class Report def initialize @title = 'Monthly Report'

    @text = [ 'Things are going', 'really, really well.' ] end ! def output_report ...HTML specific stuff... end end template method: reports
  5. template method: reports class Report def initialize @title = 'Monthly

    Report' @text = ['Things are going', 'really, really well.'] end ! def output_report output_start output_head @text.each do |line| output_line(line) end output_end end ! def output_start; end ! def output_head output_line(@title) end ! def output_line(line) raise 'Called abstract method: output_line' end ! def output_end; end end
  6. template method: reports better code class Report def initialize @title

    = 'Monthly Report' @text = ['Things are going', 'really, really well.'] end ! def output_report output_start output_head @text.each do |line| output_line(line) end output_end end ! def output_start; end ! def output_head output_line(@title) end ! def output_line(line) raise 'Called abstract method: output_line' end ! def output_end; end end
  7. class Invitation < ActiveRecord::Base! EMAIL_REGEX = /\A([^@\s]+)@((:?[a-z0-9]+\.)+[a-z]{2,})\z/i! STATUSES = %w(pending

    accepted)! ! belongs_to :sender, class_name: 'User'! belongs_to :survey! ! before_create :set_token! ! validates :recipient_email, presence: true, format: EMAIL_REGEX! validates :status, inclusion: { in: STATUSES }! ! def to_param! token! end! ! def deliver! Mailer.invitation_notification(self).deliver! end! ! private! ! def set_token! self.token = SecureRandom.urlsafe_base64! end! end
  8. class Invitation < ActiveRecord::Base! EMAIL_REGEX = /\A([^@\s]+)@((:?[a-z0-9]+\.)+[a-z]{2,})\z/i! STATUSES = %w(pending

    accepted)! ! belongs_to :sender, class_name: 'User'! belongs_to :survey! ! before_create :set_token! ! validates :recipient_email, presence: true, format: EMAIL_REGEX! validates :status, inclusion: { in: STATUSES }! ! def to_param! token! end! ! def deliver! Mailer.invitation_notification(self).deliver! end! ! private! ! def set_token! self.token = SecureRandom.urlsafe_base64! end! end
  9. class TokenizedModel < SimpleDelegator! def save! __getobj__.token ||= SecureRandom.urlsafe_base64! __getobj__.save!

    end! ! def to_param! __getobj__.token! end! end TokenizedModel.new(invitation) Decorator Pattern
  10. class Invitation < ActiveRecord::Base! EMAIL_REGEX = /\A([^@\s]+)@((:?[a-z0-9]+\.)+[a-z]{2,})\z/i! STATUSES = %w(pending

    accepted)! ! belongs_to :sender, class_name: 'User'! belongs_to :survey! ! validates :recipient_email, presence: true, format: EMAIL_REGEX! validates :status, inclusion: { in: STATUSES }! ! def deliver! Mailer.invitation_notification(self).deliver! end! end
  11. class Invitation < ActiveRecord::Base! EMAIL_REGEX = /\A([^@\s]+)@((:?[a-z0-9]+\.)+[a-z]{2,})\z/i! STATUSES = %w(pending

    accepted)! ! belongs_to :sender, class_name: 'User'! belongs_to :survey! ! before_create :set_token! ! validates :recipient_email, presence: true, format: EMAIL_REGEX! validates :status, inclusion: { in: STATUSES }! ! def to_param! token! end! ! def deliver! Mailer.invitation_notification(self).deliver! end! ! private! ! def set_token! self.token = SecureRandom.urlsafe_base64! end! end
  12. class Invitation < ActiveRecord::Base! EMAIL_REGEX = /\A([^@\s]+)@((:?[a-z0-9]+\.)+[a-z]{2,})\z/i! STATUSES = %w(pending

    accepted)! ! belongs_to :sender, class_name: 'User'! belongs_to :survey! ! before_create :set_token! ! validates :recipient_email, presence: true, format: EMAIL_REGEX! validates :status, inclusion: { in: STATUSES }! ! def to_param! token! end! ! def deliver! Mailer.invitation_notification(self).deliver! end! ! private! ! def set_token! self.token = SecureRandom.urlsafe_base64! end! end
  13. class Invitation < ActiveRecord::Base! EMAIL_REGEX = /\A([^@\s]+)@((:?[a-z0-9]+\.)+[a-z]{2,})\z/i! STATUSES = %w(pending

    accepted)! ! belongs_to :sender, class_name: 'User'! belongs_to :survey! ! before_create :set_token! ! validates :recipient_email, presence: true, format: EMAIL_REGEX! validates :status, inclusion: { in: STATUSES }! ! def to_param! token! end! ! def deliver! Mailer.invitation_notification(self).deliver! end! ! private! ! def set_token! self.token = SecureRandom.urlsafe_base64! end! end
  14. class Invitation < ActiveRecord::Base! EMAIL_REGEX = /\A([^@\s]+)@((:?[a-z0-9]+\.)+[a-z]{2,})\z/i! STATUSES = %w(pending

    accepted)! ! belongs_to :sender, class_name: 'User'! belongs_to :survey! ! before_create :set_token! ! validates :recipient_email, presence: true, format: EMAIL_REGEX! validates :status, inclusion: { in: STATUSES }! ! def to_param! token! end! ! def deliver! Mailer.invitation_notification(self).deliver! end! ! private! ! def set_token! self.token = SecureRandom.urlsafe_base64! end! end
  15. class Invitation < ActiveRecord::Base! EMAIL_REGEX = /\A([^@\s]+)@((:?[a-z0-9]+\.)+[a-z]{2,})\z/i! STATUSES = %w(pending

    accepted)! ! belongs_to :sender, class_name: 'User'! belongs_to :survey! ! before_create :set_token! ! validates :recipient_email, presence: true, format: EMAIL_REGEX! validates :status, inclusion: { in: STATUSES }! ! def to_param! token! end! ! def deliver! Mailer.invitation_notification(self).deliver! end! ! private! ! def set_token! self.token = SecureRandom.urlsafe_base64! end! end
  16. class Invitation < ActiveRecord::Base! EMAIL_REGEX = /\A([^@\s]+)@((:?[a-z0-9]+\.)+[a-z]{2,})\z/i! STATUSES = %w(pending

    accepted)! ! belongs_to :sender, class_name: 'User'! belongs_to :survey! ! before_create :set_token! ! validates :recipient_email, presence: true, format: EMAIL_REGEX! validates :status, inclusion: { in: STATUSES }! ! def to_param! token! end! ! def deliver! Mailer.invitation_notification(self).deliver! end! ! private! ! def set_token! self.token = SecureRandom.urlsafe_base64! end! end
  17. class Invitation < ActiveRecord::Base! EMAIL_REGEX = /\A([^@\s]+)@((:?[a-z0-9]+\.)+[a-z]{2,})\z/i! STATUSES = %w(pending

    accepted)! ! belongs_to :sender, class_name: 'User'! belongs_to :survey! ! before_create :set_token! ! validates :recipient_email, presence: true, format: EMAIL_REGEX! validates :status, inclusion: { in: STATUSES }! ! def to_param! token! end! ! def deliver! Mailer.invitation_notification(self).deliver! end! ! private! ! def set_token! self.token = SecureRandom.urlsafe_base64! end! end
  18. class Invitation < ActiveRecord::Base! EMAIL_REGEX = /\A([^@\s]+)@((:?[a-z0-9]+\.)+[a-z]{2,})\z/i! STATUSES = %w(pending

    accepted)! ! belongs_to :sender, class_name: 'User'! belongs_to :survey! ! before_create :set_token! ! validates :recipient_email, presence: true, format: EMAIL_REGEX! validates :status, inclusion: { in: STATUSES }! ! def to_param! token! end! ! def deliver! Mailer.invitation_notification(self).deliver! end! ! private! ! def set_token! self.token = SecureRandom.urlsafe_base64! end! end
  19. # does not follow OCP! class Purchase! def charge_user!! Stripe.charge(user:

    user, amount: amount)! end! end # follows OCP by depending on an abstraction! class Purchase! def charge_user!(payment_processor)! payment_processor.charge(user: user, amount: amount)! end! end
  20. class Printer! def initialize(item)! @item = item! end! ! def

    print! thing_to_print = case @item! when Text! @item.to_s! when Image! @item.filename! when Document! @item.formatted! end! ! send_to_printer(thing_to_print)! end! end
  21. class Printer! def initialize(item)! @item = item! end! ! def

    print! send_to_printer(@item.printable_representation)! end! end Dependency Injection
  22. # does not follow OCP! class Unsubscriber! def unsubscribe!! SubscriptionCanceller.new(user).process!

    CancellationNotifier.new(user).notify! CampfireNotifier.announce_sad_news(user)! end! end
  23. class UnsubscriptionCompositeObserver! def initialize(observers)! @observers = observers! end! ! def

    notify(user)! @observers.each do |observer|! observer.notify(user)! end! end! end! ! class Unsubscriber! def initialize(observer)! @observer = observer! end! ! def unsubscribe!(user)! observer.notify(user)! end! end Composite Pattern
  24. class UnsubscriptionCompositeObserver! def initialize(observers)! @observers = observers! end! ! def

    notify(user)! @observers.each do |observer|! observer.notify(user)! end! end! end! ! class Unsubscriber! def initialize(observer)! @observer = observer! end! ! def unsubscribe!(user)! observer.notify(user)! end! end Observer Pattern
  25. class UnsubscriptionCompositeObserver! def initialize(observers)! @observers = observers! end! ! def

    notify(user)! @observers.each do |observer|! observer.notify(user)! end! end! end! ! class Unsubscriber! def initialize(observer)! @observer = observer! end! ! def unsubscribe!(user)! observer.notify(user)! end! end
  26. Let q(x) be a property provable about objects x of

    type T. Then q(y) should be provable for objects y of type S where S is a subtype of T. “
  27. blah q(x) blah blah blah blah blah blah blah blah

    x blah T. Blah q(y) blah blah blah blah blah blah y blah blah S blah S blah blah blah blah T. “
  28. class Signup! def initialize(attributes)! @attributes = attributes! end! ! def

    save! account = Account.create!(@attributes[:account])! account.users.create!(@attributes)! end! end! ! class SignupsController < ApplicationController! def create! build_signup.save! end! ! def build_signup! Signup.new(params[:signup])! end! end
  29. class SignupsController < ApplicationController! def create! signup = build_signup! if

    params[:invitation_id]! invitation = Invitation.find(params[:invitation_id])! signup.save(invitation)! else! signup.save! end! end! ! def build_signup! if params[:invitation_id]! InvitationSignup.new(params[:signup])! else! Signup.new(params[:signup])! end! end! end
  30. class SignupsController < ApplicationController! def create! signup = build_signup! if

    params[:invitation_id]! invitation = Invitation.find(params[:invitation_id])! signup.save(invitation)! else! signup.save! end! end! ! def build_signup! if params[:invitation_id]! InvitationSignup.new(params[:signup])! else! Signup.new(params[:signup])! end! end! end
  31. def InvitationSignup < Signup! def initialize(attributes, invitation)! super(attributes)! @invitation =

    invitation! end! ! def save! user = super! @invitation.accept(user)! user! end! end The public API remains the same
  32. class SignupsController < ApplicationController! def create! build_signup.save! end! ! def

    build_signup! if params[:invitation_id]! InvitationSignup.new(params[:signup])! else! Signup.new(params[:signup])! end! end! end
  33. public class WebPage implements IWebPage {! public String currentUrl() {};!

    public void setUrl(String url) {};! public String[] consoleMessages() {};! public String[] alertMessages() {};! public String[] confirmMessages() {};! public String[] promptMessages() {};! }
  34. public class WebPage implements IWebPage {! public String currentUrl() {};!

    public void setUrl(String url) {};! public String[] consoleMessages() {};! public String[] alertMessages() {};! public String[] confirmMessages() {};! public String[] promptMessages() {};! } Not all methods are related
  35. public class VisitCommand {! public VisitCommand(WebPage page, String url) {!

    this.page = page;! this.url = url;! }! ! public void start() {! page.setUrl(url)! }! ! private WebPage page;! private String url;! } Command Pattern
  36. public interface HasUrl {! public String currentUrl() {};! public void

    setUrl(String url) {};! }! ! public interface HasMessages {! public String[] consoleMessages() {};! public String[] alertMessages() {};! public String[] confirmMessages() {};! public String[] promptMessages() {};! } public class WebPage implements HasUrl, HasMessages {
  37. public class VisitCommand {! public VisitCommand(HasUrl page, String url) {!

    this.page = page;! this.url = url;! }! ! public void start() {! page.setUrl(url)! }! ! private HasUrl page;! private String url;! }
  38. public class VisitCommand {! public VisitCommand(HasUrl page, String url) {!

    this.page = page;! this.url = url;! }! ! public void start() {! page.setUrl(url)! }! ! private HasUrl page;! private String url;! }
  39. http://tenderlovemaking.com/2014/06/02/yagni-methods-are-killing-me.html loop do params[:foo] params.delete :foo params[:foo] = [Object.new] end

    def convert_value_to_parameters(value) if value.is_a?(Array) && !converted_arrays.member?(value) converted = value.map { |_| convert_value_to_parameters(_) } converted_arrays << converted converted elsif value.is_a?(Parameters) || !value.is_a?(Hash) value else self.class.new(value) end end
  40. “High level modules should not depend on low level modules,

    they should both depend on abstractions
  41. #Violates DIP! class Copier! def self.copy! reader = KeyboardReader.new! writer

    = Printer.new! ! keystrokes = reader.read_until_eof! writer.write(keystrokes)! end! end
  42. #Obeys DIP. Is decoupled and reusable.! class Copier! def initialize(reader,

    writer)! @reader = reader! @writer = writer! end! ! def copy! keystrokes = @reader.read_until_eof! @writer.write(keystrokes)! end! end! ! Copier.new(KeyboardReader.new, Printer.new)! Copier.new(KeyboardReader.new, NetworkPrinter.new)
  43. class NetworkPrinterAdapter! def initialize(network_printer)! @network_printer = network_printer! end! ! def

    write(keystrokes)! @network_printer.print(keystrokes)! end! end! ! network_printer = ! NetworkPrinterAdapter.new(NetworkPrinter.new)! Copier.new(KeyboardReader.new, network_printer) Adapter Pattern
  44. class Product < ActiveRecord::Base! has_many :purchases! end! ! class Purchase

    < ActiveRecord::Base! belongs_to :Product! ! def has_receipt?! receipt.present?! end! ! def receipt_id! receipt.id! end! ! def receipt_address! receipt.details.address! end! ! private! ! def receipt! Stripe::Receipt.find(stripe_order_id)! end! end
  45. class PurchaseWithReceipt < SimpleDelegator! def initialize(purchase, receipt_finder)! super(purchase)! @receipt_finder =

    receipt_finder! end! ! def has_receipt?! receipt.present?! end! ! def receipt_id! receipt.id! end! ! def receipt_address! receipt.details.address! end! ! private! ! def receipt! @receipt_finder.find(stripe_order_id)! end! end class Product < ActiveRecord::Base! has_many :purchases! end! ! class Purchase < ActiveRecord::Base! belongs_to :product! end
  46. S O L I D ingle responsibility pen closed iskov

    substitution nterface segragation ependency inversion Uncle Bob Martin
  47. Can’t tell what it does at a glance, it’s TOO

    LONG More than one level of nesting, probably
  48. Can’t tell what it does at a glance, it’s TOO

    LONG More than one level of nesting, probably TOO LONG
  49. Can’t tell what it does at a glance, it’s TOO

    LONG More than one level of nesting, probably TOO LONG More than one level of abstraction, may be
  50. Can’t tell what it does at a glance, it’s TOO

    LONG More than one level of nesting, probably TOO LONG TOO LONG More than one level of abstraction, may be
  51. Can’t tell what it does at a glance, it’s TOO

    LONG More than one level of nesting, probably TOO LONG TOO LONG More than one level of abstraction, may be More than 5 lines
  52. Can’t tell what it does at a glance, it’s TOO

    LONG More than one level of nesting, probably TOO LONG TOO LONG More than one level of abstraction, may be More than 5 lines TOO LONG
  53. Can’t tell what it does at a glance, it’s TOO

    LONG More than one level of nesting, probably TOO LONG TOO LONG More than one level of abstraction, may be More than 5 lines TOO LONG REFACTOR
  54. Need to scroll to know what it does, it’s TOO

    BIG More private methods than public, it’s
  55. Need to scroll to know what it does, it’s TOO

    BIG More private methods than public, it’s TOO BIG
  56. Need to scroll to know what it does, it’s TOO

    BIG More private methods than public, it’s TOO BIG More than seven methods, probably
  57. Need to scroll to know what it does, it’s TOO

    BIG More private methods than public, it’s TOO BIG TOO BIG More than seven methods, probably
  58. Need to scroll to know what it does, it’s TOO

    BIG More private methods than public, it’s TOO BIG TOO BIG More than seven methods, probably More than 100 lines
  59. Need to scroll to know what it does, it’s TOO

    BIG More private methods than public, it’s TOO BIG TOO BIG More than seven methods, probably More than 100 lines TOO BIG
  60. Need to scroll to know what it does, it’s TOO

    BIG More private methods than public, it’s TOO BIG TOO BIG More than seven methods, probably More than 100 lines TOO BIG REFACTOR
  61. class Report def initialize @title = 'Monthly Report' @text =

    [ 'Things are going', 'really, really well.' ] end ! def output_report ...HTML specific stuff... end end strategy pattern: reports
  62. bad code class Report def initialize @title = 'Monthly Report'

    @text = [ 'Things are going', 'really, really well.' ] end ! def output_report ...HTML specific stuff... end end strategy pattern: reports
  63. class Report def initialize(formatter) @title = 'Monthly Report' @text =

    [ 'Things are going', 'really, really well.' ] @formatter = formatter.new(@title, @text) end ! def formatter=(formatter) @formatter = formatter.new(@title, @text) end ! def output_report @formatter.output_report end end strategy pattern: reports
  64. class Report def initialize(formatter) @title = 'Monthly Report' @text =

    [ 'Things are going', 'really, really well.' ] @formatter = formatter.new(@title, @text) end ! def formatter=(formatter) @formatter = formatter.new(@title, @text) end ! def output_report @formatter.output_report end end better code strategy pattern: reports
  65. Smelling feature envy: Local variables or params that are used

    more than instance variables and methods
  66. Smelling feature envy: class Completion! def score! answers.inject(0) do |result,

    answer|! question = answer.question! result + question.score(answer.text)! end! end! end
  67. Extract method: If possible move part of a method to

    the appropriate class Solution #1
  68. Solving feature envy: class Answer! def score! question.score(text)! end! end!

    ! class Completion! answers.inject(0) do |result, answer|! result + answer.score! end! end
  69. Hexagonal Architecture Domain Driven Design (DDD) Presenters Service Objects Value

    Objects and Immutability Behavior Driven Development (BDD) Object Composition vs Inheritance