Tech Racho エンジニアの「?」を「!」に。
  • Ruby / Rails関連

Rubyのクラスメソッドがリファクタリングに抵抗する理由(翻訳)

概要

原著者の許諾を得て翻訳・公開いたします。

  • 2018/01/30: 初版公開
  • 2023/01/19: 細部を更新

概要

Rubyのクラスメソッドがリファクタリングに抵抗する理由(翻訳)

肥大化したActiveRecordモデルをリファクタリングする7つの方法(翻訳)

私の記事『肥大化したActiveRecordモデルをリファクタリングする7つの方法』に対して、「クラスメソッドでできることをなぜわざわざインスタンスでやるんですか?」という質問をよくいただきました。お答えしましょう。要するに以下が理由です。

私がクラスメソッドよりオブジェクトインスタンスを好む理由は、クラスメソッドはリファクタリングに抵抗するからです

詳しく説明するために、データを外部アナリティクスサービスと同期するバックグラウンドジョブを例に取ることにします。以下ご覧ください。

class SyncToAnalyticsService
  ConnectionFailure = Class.new(StandardError)

  def self.perform(data)
    data              = data.symbolize_keys
    account           = Account.find(data[:account_id])
    analytics_client  = Analytics::Client.new(CC.config[:analytics_api_key])

    account_attributes = {
      account_id:         account.id,
      account_name:       account.name,
      account_user_count: account.users.count
    }

    account.users.each do |user|
      analytics_client.create_or_update({
        id:             user.id,
        email:          user.email,
        account_admin:  account.administered_by?(user)
      }.merge(account_attributes))
    end
  rescue SocketError => ex
    raise ConnectionFailure.new(ex.message)
  end
end

このジョブは、ユーザーごとに属性のハッシュをHTTP POSTとして繰り返し送信します。SocketErrorraiseされるとSyncToAnalyticsService::ConnectionFailureでラップされ、エラートラッキングシステムで正しく分類されるようにします。

このSyncToAnalyticsService.performメソッドはなかなか複雑で、責務をいくつも抱えています。単一責任の原則(SRP)はまるでフラクタルのように、より細かいレベルでアプリ全体・モジュール・クラス・メソッドに渡って適用されると考えられます。

SyncToAnalyticsService.performは、そのメソッドのさまざまな操作が必ずしも同じ抽象化レベルにないため、Composed Methodパターンには該当しません。

訳注

Composed Methodパターンについては以下もどうぞ。

参考: 「文章のように読めるメソッドを作る」Composed Method パターン

解決法のひとつは、Extract Methodパターンを何回か適用してメソッドを切り出すことです。結果は以下のような感じになります。

class SyncToAnalyticsService
  ConnectionFailure = Class.new(StandardError)

  def self.perform(data)
    data                = data.symbolize_keys
    account             = Account.find(data[:account_id])
    analytics_client    = Analytics::Client.new(CC.config[:analytics_api_key])

    sync_users(analytics_client, account)
  end

  def self.sync_users(analytics_client, account)
    account_attributes  = account_attributes(account)

    account.users.each do |user|
      sync_user(analytics_client, account_attributes, user)
    end
  end

  def self.sync_user(analytics_client, account_attributes, user)
    create_or_update_user(analytics_client, account_attributes, user)
  rescue SocketError => ex
    raise ConnectionFailure.new(ex.message)
  end

  def self.create_or_update_user(analytics_client, account_attributes, user)
    attributes = user_attributes(user, account).merge(account_attributes)
    analytics_client.create_or_update(attributes)
  end

  def self.user_attributes(user, account)
    {
      id:             user.id,
      email:          user.email,
      account_admin:  account.administered_by?(user)
    }
  end

  def self.account_attributes(account)
    {
      account_id:         account.id,
      account_name:       account.name,
      account_user_count: account.users.count
    }
  end
end

元のコードと比べれば少しはマシになりましたが、どうも違和感があります。

これではオブジェクト指向になっておらず、手続き型プログラミングと関数型プログラミングの不気味な折衷案がオブジェクトベースの世界で立ち往生しているような感じです。

さらに、切り出したメソッドはどれもクラスレベルなので、private宣言も簡単にはできそうにありません(おそらくclass << selfのような見苦しい方法に切り替えざるを得ないでしょう)。

私がSyncToAnalyticsServiceの元の実装を手がけたとしたら、こんな形のままリファクタリングを完了させる気にはとてもならないでしょう。私なら、代わりに次のようにリファクタリングを始めるでしょう。

class SyncToAnalyticsService
  ConnectionFailure = Class.new(StandardError)

  def self.perform(data)
    new(data).perform
  end

  def initialize(data)
    @data = data.symbolize_keys
  end

  def perform
    account           = Account.find(@data[:account_id])
    analytics_client  = Analytics::Client.new(CC.config[:analytics_api_key])

    account_attributes = {
      account_id:         account.id,
      account_name:       account.name,
      account_user_count: account.users.count
    }

    account.users.each do |user|
      analytics_client.create_or_update({
        id:             user.id,
        email:          user.email,
        account_admin:  account.administered_by?(user)
      }.merge(account_attributes))
    end
  rescue SocketError => ex
    raise ConnectionFailure.new(ex.message)
  end
end

元のコードとほとんど変わらないように見えますが、今度は機能をクラスメソッドではなくインスタンスメソッドにしている点が異なります。ここに再びExtract Methodパターンを適用すると次のような感じになります。

class SyncToAnalyticsService
  ConnectionFailure = Class.new(StandardError)

  def self.perform(data)
    new(data).perform
  end

  def initialize(data)
    @data = data.symbolize_keys
  end

  def perform
    account.users.each do |user|
      create_or_update_user(user)
    end
  rescue SocketError => ex
    raise ConnectionFailure.new(ex.message)
  end

private

  def create_or_update_user(user)
    attributes = user_attributes(user).merge(account_attributes)
    analytics_client.create_or_update(attributes)
  end

  def user_attributes(user)
    {
      id:             user.id,
      email:          user.email,
      account_admin:  account.administered_by?(user)
    }
  end

  def account_attributes
    @account_attributes ||= {
      account_id:         account.id,
      account_name:       account.name,
      account_user_count: account.users.count
    }
  end

  def analytics_client
    @analytics_client ||= Analytics::Client.new(CC.config[:analytics_api_key])
  end

  def account
    @account ||= Account.find(@data[:account_id])
  end
end

操作を完了するために直接変数(immediate variable)をあちこちに引き回さなければならないクラスメソッドを追加するのではなく、結果をメモ化する#account_attributesのようなメソッドを追加しました。これは私のお気に入りの手法です。メソッドを分割するときに、メモ化したアクセサとして直接変数を切り出す方法は、私の大好きなリファクタリングです。当初のクラスはまったくステートを持っていませんでしたが、きれいに分割されたおかげで、クラスに何かを追加するのも簡単になりました

今度の結果は私にとってずっと明確になりました。こういうリファクタリングは完全勝利の気分になれます。

  • ステートとロジックが1個のオブジェクトにきっちりカプセル化されている
  • (与えられた)オブジェクトの作成が操作の呼び出しのタイミングから切り離されているのでテストしやすい
  • AccountAnalytics::Clientといった変数をどこにも引き回していない

さらに、このロジックを用いるどのコード片も(グローバルな)クラス名と癒着していません。これらを新しいクラスと差し替えるのは面倒ですが、新しいインスタンスと差し替えるのは簡単です。このおかげで、追加の動作をコンポジションで楽に構築できます。変更のたびにクラスを再オープンして拡張する必要はありません。

リファクタリングメモ: 私なら、上の最終的なソースの状態でクラスの実装をやめておくでしょう。しかしロジックがさらに複雑になった場合、このジョブは単一ユーザーを同期して分割するための別のクラスを欲しがるでしょう。

さて、このことは本記事で最初に述べた前提とどんな関係があるのでしょうか?クラスメソッドを分割するとコードが見苦しくなるので、クラスメソッドをリファクタリングする機会がなくなってしまうでしょう。最初からインスタンス形式で書いておけばリファクタリングの選択肢も明確になりますし、必要な対策を取るときの抵抗も減らせます。この効果は私自身のコーディングでも何度となく体験していますし、ここ数年のさまざまなRubyチームを外から眺めていてもやはりそうです。

想定反論

YAGNIではないか?

YAGNI: You Aren't Going To Need It(後で必要になるかもしれないという理由でやるべきではない)
参考: YAGNI - Wikipedia

YAGNIが重要な法則であることはもちろんですが、ここで適用するのは筋違いです。これらのクラスをエディタで開いてみれば、そうでないクラスと比べて複雑さはさほど変わりません。「このオブジェクトはYAGNIだ」という指摘は、「インデントをTab文字1個ではなくスペース2文字にするのはYAGNIだ」という指摘と大して変わりません。両者の違いはスタイル上のものでしかありません。オブジェクト指向設計にYAGNIを適用する意味があるのは、わかりやすさに違いが生じる場合だけです(使うクラスが1つなのか2つなのか、など)。

オブジェクトが1つ余分になる

インスタンス形式だとオブジェクトが1つ余分に作成されるという根拠で反対する人もいます。オブジェクトが作成されることについては確かにそのとおりですが、実用上は何の影響もありません。

Railsのリクエストやバックグラウンドジョブでは数千〜数万個ものRubyオブジェクトが作成されます。オブジェクト作成を最適化してRubyのガベージコレクタの負担を軽減するのは正統な手法ですが、それが意味を持つ場合に行うべきです。そして、その必要性を確認できるのは測定だけです。

そのインスタンスの変種がたったひとつの追加オブジェクトを作成しただけでシステムのパフォーマンスに大きく影響するとは考えられません(データ付きの反例をお持ちの方がいたら、ぜひお話をお伺いしたいものです)。

呼び出しが面倒

最後の想定反論は、クラスメソッドの方が入力文字数が少なくて済むというものです。

Job.perform(args)
#  どっちがいいか?
Job.new(args).perform

入力文字数が少ないのはおっしゃるとおりです。私なら、オブジェクトをビルドする手頃なクラスメソッドを1つこしらえ、それに委譲して済ませるでしょう。実際、これは私が認める数少ないクラスメソッドの使い方の1つです。そうやって自分で作って自分でおいしくいただく分には構いません。

まとめ

ステートや複数のメソッドが当分使われないとしても、最初はオブジェクトのインスタンスを使いましょう。いずれ変更のときが来れば、あなたや同僚がリファクタリングしたくなります。

コードが今後も決して変更されなければ、クラスメソッド方式かインスタンスかという違いは無視して構わない性質のものであり、今後そのコードが改悪されることもまずありません。

私がコードでクラスメソッドを使って幸せになれたケースはほとんどありません。あったとすれば、インスタンスの初期化手順をラップして一発で呼び出せる便利メソッドか、連携する他のオブジェクトからオブジェクトをより簡単に構成できる徹底的にシンプルなファクトリーメソッド(多くとも2行まで)ぐらいです。

皆さんはどう思いますか?どちらがお好みですか?そしてその理由は?私が見落としたメリットやデメリットはありますでしょうか?ぜひコメント欄でお知らせください。

追伸: こうした問題に興味をお持ちで、他の記事を読んでみたい方は、元記事末尾のフォームでCodeClimateニュースレターをぜひご購読ください。Rubyに特化したリファクタリングやオブジェクト設計に関する話題を月に一度メール配信いたします。

詳しく知りたい方へ

本記事をレビューしてくれたDoug Cole、Don Morrison、Josh Susserに感謝いたします。

関連記事

肥大化したActiveRecordモデルをリファクタリングする7つの方法(翻訳)

Rails tips: モデルのクエリをカプセル化する2つの方法(翻訳)

Rubyのクラスメソッドをclass << selfで定義している理由(翻訳)


CONTACT

TechRachoでは、パートナーシップをご検討いただける方からの
ご連絡をお待ちしております。ぜひお気軽にご意見・ご相談ください。