スプラウトクラスで挑むRubyのリファクタリング：テストできないレガシーコードを実践的に改善する

レガシーなアプリケーションの保守で最も厄介な課題の一つは、コードがそもそもテストしやすい形で書かれていないことです。その結果、意味のあるテストを書くことが難しい、あるいはほぼ不可能になってしまいます。

これはまさに「鶏と卵」の問題です。レガシーアプリケーションにテストを書くためにはコードを変更する必要がありますが、テストなしに安心してコードを変更することはできません。

このパラドックスにどう向き合えばよいのでしょうか？

このテーマは、Michael Feathersの名著『Working Effectively with Legacy Code（レガシーコード改善ガイド）』で詳しく扱われています。本記事では、その中からスプラウトクラス（Sprout Class）と呼ばれるテクニックに焦点を絞って解説します。

いざ、レガシーコードの世界へ！

まずは、Appointmentという古いActiveRecordクラスを見てみましょう。かなりの長さですが、実際の現場ではさらに数百行に及ぶことも珍しくありません。

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
  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

新機能を追加する

ここで、取引レポート周りに新しい機能を追加してほしいと依頼されたとします。しかしAppointmentクラスは依存関係が多すぎて、大規模なリファクタリングなしにはテストできません。どのように進めればよいのでしょうか？

選択肢1：その場でコードを書き足す

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 # 新しく追加したコード
  end
end

def send_thank_you_email_to_client
  ThankYouMailer.thank_you_email(self).deliver
end

このアプローチには問題がある

上記のコードには2つの問題があります。

1. Appointmentクラスはすでに多くの責務を抱えており（これは単一責任原則（SRP）の違反です）、その一つが取引の記録です。取引関連のコードをさらにAppointmentクラスに追加すると、コードは少しずつ悪化していきます。

2. 新しい統合テストを書いてメール送信を確認することはできるかもしれませんが、Appointmentクラス自体がテスト可能な状態ではないため、ユニットテストを追加することはできません。テストされていないコードを増やすことになり、これは明らかに好ましくありません。（実際、Michael Feathersはレガシーコードを「テストのないコード」と定義しています。つまり、レガシーコードの上にさらにレガシーコードを積み重ねていることになるのです。）

分割して抽出するほうがずっと良い

新しいコードをそのままインラインで追加するよりも良い解決策は、取引記録の振る舞いを独立したクラスとして抽出することです。ここでは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)
  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

リファクタリングの成果

これにより、Appointmentクラスは次のように大幅にスリム化できます。

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
end

もちろん、Appointment側のコードも変更しているため、その部分は依然としてテストできません。しかし、TransactionRecorderの中身はすべてテスト可能になりました。さらに、各メソッドがインスタンス変数に直接アクセスする代わりに引数を受け取るように設計したことで、各メソッドを個別に単体テストすることさえ可能です。着手前と比べて、はるかに良い状態に到達したと言えるでしょう。