Refactoring Legacy Ruby với Sprout Classes

Một thách thức "rối rắm" nhất làm việc với legacy app: code không viết để testable. Vì vậy viết meaningful test khó hoặc không thể.

Đây là vấn đề chicken-and-egg: để viết test cho legacy app, phải thay đổi code, nhưng không thể tự tin thay đổi code nếu chưa viết test!

Làm sao giải quyết nghịch lý này?

Đây là một trong nhiều chủ đề trong cuốn Working Effectively with Legacy Code xuất sắc của Michael Feathers. Hôm nay zoom vào một kỹ thuật cụ thể từ sách: Sprout Class.

Sẵn sàng cho LEGACY CODE!

Xem class ActiveRecord cũ `Appointment`. Nó dài, thực tế dài gấp hàng trăm dòng.

class Appointment < ActiveRecord::Base
  has_many :appointment_services, :dependent => :destroy
  has_many :services, :through => :appointment_services
  has_many :appointment_products, :dependent => :destroy
  has_many :products, :through => :appointment_products
  has_many :payments, :dependent => :destroy
  has_many :transaction_items
  belongs_to :client
  belongs_to :stylist
  belongs_to :time_block_type

  def record_transactions
    transaction_items.destroy_all if paid_for?
    save_service_transaction_items
    save_product_transaction_items
    save_tip_transaction_item
  end

  def save_service_transaction_items
    appointment_services.reload.each { |s| s.save_transaction_item(self.id) }
  end

  def save_product_transaction_items
    appointment_products.reload.each { |p| p.save_transaction_item(self.id) }
  end

  def save_tip_transaction_item
    TransactionItem.create!(
      :appointment_id => self.id,
      :stylist_id => self.stylist_id,
      :label => "Tip",
      :price => self.tip,
      :transaction_item_type_id => TransactionItemType.find_or_create_by_code("TIP").id
    )
  end
end

Thêm tính năng

Nếu được yêu cầu thêm chức năng mới cho transaction-reporting, nhưng class `Appointment` có quá nhiều dependency không testable nếu không refactor nhiều, làm sao?

Option 1: Chỉ làm thay đổi

def record_transactions
  transaction_items.destroy_all if paid_for?
  save_service_transaction_items
  save_product_transaction_items
  save_tip_transaction_item
  send_thank_you_email_to_client # Code mới
end

def send_thank_you_email_to_client
  ThankYouMailer.thank_you_email(self).deliver
end

Cái đó "kind of sucks"

Hai vấn đề:

1. Appointment có nhiều trách nhiệm (vi phạm Single Responsibility Principle), một trong đó là recording transactions. Thêm code transaction-related vào `Appointment` làm code hơi tệ hơn.
2. Có thể viết integration test check email gửi đi, nhưng vì `Appointment` không testable, không thể thêm unit test. Thêm untested code — xấu. (Feathers định nghĩa legacy code là "code without tests", nên chúng ta thêm nhiều legacy code vào legacy code.)

Chia nhỏ tốt hơn

Giải pháp tốt hơn thêm code inline: extract transaction-recording behavior vào class riêng. Gọi `TransactionRecorder`:

class TransactionRecorder
  def initialize(options)
    @appointment_id = options[:appointment_id]
    @appointment_services = options[:appointment_services]
    @appointment_products = options[:appointment_products]
    @stylist_id = options[:stylist_id]
    @tip = options[:tip]
  end

  def run
    save_service_transaction_items(@appointment_services)
    save_product_transaction_items(@appointment_products)
    save_tip_transaction_item(@appointment_id, @stylist_id, @tip_amount)
  end

  def save_service_transaction_items(appointment_services)
    appointment_services.each { |s| s.save_transaction_item(appointment_id) }
  end

  def save_product_transaction_items(appointment_products)
    appointment_products.each { |p| p.save_transaction_item(appointment_id) }
  end

  def save_tip_transaction_item(appointment_id, stylist_id, tip)
    TransactionItem.create!(
      appointment_id: appointment_id,
      stylist_id: stylist_id,
      label: "Tip",
      price: tip,
      transaction_item_type_id: TransactionItemType.find_or_create_by_code("TIP").id
    )
  end
end

Kết quả

Sau đó `Appointment` gọn nhẹ chỉ còn:

class Appointment < ActiveRecord::Base
  has_many :appointment_services, :dependent => :destroy
  has_many :services, :through => :appointment_services
  has_many :appointment_products, :dependent => :destroy
  has_many :products, :through => :appointment_products
  has_many :payments, :dependent => :destroy
  has_many :transaction_items
  belongs_to :client
  belongs_to :stylist
  belongs_to :time_block_type

  def record_transactions
    transaction_items.destroy_all if paid_for?
    TransactionRecorder.new(
      appointment_id: id,
      appointment_services: appointment_services,
      appointment_products: appointment_products,
      stylist_id: stylist_id,
      tip: tip
    ).run
  end
end

Vẫn modify code trong `Appointment` (không testable), nhưng giờ có thể test mọi thứ trong `TransactionRecorder`, và vì đổi function nhận argument thay vì instance variable, thậm chí test từng function độc lập. Vị trí tốt hơn nhiều so với ban đầu.