Railsで処理を別クラスに切り出す方法について

こんにちは。メドピアのRuby(Rails)化をお手伝いしている@willnetです。最近はエンジニアが増えた影響か、Railsの質問に答えていることが多いです。

以前、Railsの太ったモデルをダイエットさせる方法についてというタイトルでPOROを使っていこうという話を書きました。その際にコード例などもなるべく多く載せるようにしたのですが、このエントリだけを読んだ状態では、いざ「POROを使ってみよう！」としたときにまだ悩む余地がありそうです。

POROはその名の通り普通のRubyオブジェクトなので、いろんな書き方ができてしまいます。それなりに経験がある人でないと、どのように書いたらいいんだろう…と悩んで時間を使ってしまいそうですね。さらに、複数人で開発しているチームだと書き方のバラツキも気になるところです。きっと、POROを書くときのお作法が決まっている方が開発しやすいはず。

そこで、お作法を決める手助けをするために例を出してみます。

コード例

slackのようなサービスを作っていると想像してみてください。Messageモデルをsaveしたときに、それがhereメンションのときはチャンネル内のアクティブなメンバーのみ、channelメンションのときはチャンネル内のすべてのメンバーに対してMentionを作るという処理をMessageモデルに定義しています。

class Message < ApplicationRecord
  has_many :mentions
  belongs_to :creator, class_name: 'User'
  belongs_to :channel

  after_create :create_here_mention, if: :here?
  after_create :create_channel_mention, if: :channel?

  def here?; end # 省略
  def channel?; end #省略

  def create_here_mention
    members = channel.members.active - [creator]
    create_mentions(members)
  end

  def create_channel_mention
    members = channel.members - [creator]
    create_mentions(members)
  end

  private

  def create_mentions(members)
    members.each do |member|
      mentions.create!(to: member, chennel: channel)
    end
  end
end

そもそもコールバック使うのどうなの？など議論の余地があるコードですが、そこまで考え出すとこのエントリで取り上げる範囲が広がりすぎてしまうためそのあたりは無視してください*1。このコードからcreate_here_mentionとcreate_channel_mention、create_mentionsを別クラスに切り出してみるとします。さてどう切り出すのが良いでしょうか。

切り出し方にいろいろな選択肢が存在します。

クラスやメソッドの名前はどのような観点で決めると良いでしょうか？
メソッドはクラスメソッドにすべきでしょうか。インスタンスメソッドにしたほうが良いでしょうか？
クラスは一つでいいでしょうか？切り出すメソッドごとにクラスを作ったほうがよいでしょうか？

これらの点について、僕は自分なりの意見を持っています。それが正しいかはさておき、他の人がどういう観点で判断をしているのかを知ることで、みなさんがPOROを書くときに迷うことが減るのではないかと思います。

POROに切り出した後のコード

説明の前に、切り出した後のコードを載せます。このコードを参考にしつつ、どういう観点で切り出しているのか書いていきます。

class Message < ApplicationRecord
  has_many :mentions
  belongs_to :creator, class_name: 'User'
  belongs_to :channel

  after_create :create_here_mention, if: :here?
  after_create :create_channel_mention, if: :channel?

  def here?; end # 省略
  def channel?; end #省略

  def create_here_mention
    HereMentionCreator.call(message: self)
  end

  def create_channel_mention
    ChannelMentionCreator.call(message: self)
  end
end

class HereMentionCreator
  delegate :channel, :creator, to: :message

  def self.call(message:)
    new(message: message).call
  end

  def initialize(message:)
    @message = message
  end

  def call
    members.each do |member|
      message.mentions.create!(to: member, chennel: channel)
    end
  end

  private

  attr_reader :message

  def members
    @members ||= channel.members.active - [creator]
  end
end

class ChannelMentionCreator
  delegate :channel, :creator, to: :message

  def self.call(message:)
    new(message: message).call
  end

  def initialize(message:)
    @message = message
  end

  def call
    members.each do |member|
      message.mentions.create!(to: member, chennel: channel)
    end
  end

  private

  attr_reader :message

  def members
    @members ||= channel.members - [creator]
  end
end

メソッド名は統一する

POROに切り出したとき、publicなインターフェースはcallもしくはnew(つまりinitialize)で統一するようにしています。基本的にはcallで、インスタンス化したオブジェクトを返すだけでよいときのみnewという使い分けをしています。

まずメソッド名を考え、それから属するクラスを決めるものだ、という言説があるのは知っていて(要出典)以前はそのように実装していました。しかしHereMentionCreatorのようなクラス名をつけることで、callメソッドがhereメンションを作るのだな、と十分推測可能です。またメソッド名が統一されていると「このクラスのメソッド名ってなんだっけ？」とならずに便利なので最近は統一するようにしています。

処理の実態はインスタンスメソッドに書く

今回やろうとしていることは、「hereメンションを作る」と「channelメンションを作る」という手続きを切り出すことです。なので次のようにクラスメソッドで実装する人も時々見かけます。

class HereMentionCreator
  def self.call(message:)  
    channel = message.channel
    members = channel.members.active - [message.creator]
    
    members.each do |member|
      message.mentions.create!(to: member, chennel: channel)
    end
  end
end

サンプルコードが簡単なので、なんだかこれでも問題なさそうに見えますね。しかし手続きがもっと多くなるとどうでしょうか。

チャンネルのミュートの概念を追加し、さらにプッシュ通知もするように機能追加したコードを書いてみます。

class HereMentionCreator
  PUSH_NOTIFICATION_LIMIT = 100

  def self.call(message:)
    channel = message.channel
    members = channel.members.includes(:mute_channels).active - [message.creator]

    members.each do |member|
      message.mentions.create!(to: member, chennel: channel, mute: member.mute?(channel))
    end

    not_mute_members = members.reject { |member| member.mute?(channel) }
    not_mute_members.map(&:id).each_slice(PUSH_NOTIFICATION_LIMIT).with_index do |ids, index|
      PushNotificationWorker.perform_in(index.minutes, message.id, uids)
    end
  end
end

これでも読める人は問題なく読めると思いますが、さっきよりも概要を掴みづらくなったのは間違いないはず。

インスタンスメソッドで実装すると次のように書くことができます。

class HereMentionCreator
  PUSH_NOTIFICATION_LIMIT = 100

  delegate :channel, :creator, to: :message

  def self.call(message:)
    new(message: message).call
  end

  def initialize(message:)
    @message = message
  end

  def call
    create_notifications
    create_push_notifications
  end

  private

  attr_reader :message

  def create_notifications
    members.each do |member|
      message.mentions.create!(to: member, chennel: channel, mute: member.mute?(channel))
    end
  end

  def create_push_notifications
    not_mute_members.map(&:id).each_slice(PUSH_NOTIFICATION_LIMIT).with_index do |ids, index|
      PushNotificationWorker.perform_in(index.minutes, message.id, uids)
    end
  end

  def members
    @members ||= channel.members.includes(:mute_channels).active - [message.creator]
  end

  def not_mute_members
    @not_mute_members ||= members.reject { |member| member.mute?(channel) }
  end
end

callメソッドがcreate_notificationsメソッドとcreate_push_notificationsメソッドを呼ぶだけになり、処理の概要がつかみやすくなりました。また、membersやnot_mute_membersもローカル変数からインスタンスメソッドに切り出されたことで、それぞれのメソッドの行数が減り、処理の内容を把握しやすくなっています。

このように、メソッド分割することで抽象化がしやすくなるのがインスタンスメソッドを利用する主な理由です。

こう書くとクラスメソッドでもメソッド分割できるのでは？という意見がでてきそうですが、クラスメソッドで同様のことをやろうとするとクラスインスタンス変数を更新するコードになり、結果としてスレッドセーフではないコードになってしまいます。

一つのクラスには一つの公開インターフェース

今回はHereMentionCreatorとChannelMentionCreatorのように２つのクラスに切り出しましたが、次のように単一のクラスにhereメンションをするメソッドとchannelメンションをするメソッドを定義する人もいるのではないでしょうか。

class MentionCreator
  delegate :channel, :creator, to: :message

  def self.here(message:)
    new(message: message, type: :here).call
  end

  def self.channel(message:)
    new(message: message, type: :channel).call
  end

  def initialize(message:, type:)
    @message = message
    @type = type
  end

  def call
    members.each do |member|
      message.mentions.create!(to: member, chennel: channel)
    end
  end

  private

  attr_reader :message, :type

  def members
    @members ||= if type == :here
      channel.members.active - [creator]
    else
      channel.members - [creator]
    end
  end
end

一見これでも問題なさそうに見えます。しかし仕様が変更されるについてメンテナンスが難しくなってきます。例えば新しくeveryoneメンションもMentionCreatorで扱うようにするとどうなるでしょうか。everyoneメンションは基本的にchannelメンションと同じですが、すべての人が参加しているチャンネル(generalチャンネル)以外ではメンションとして扱われないという仕様です。

素直にMentionCreatorクラスを拡張してみます。

class MentionCreator
  delegate :channel, :creator, to: :message

  def self.here(message:)
    new(message: message, type: :here).call
  end

  def self.channel(message:)
    new(message: message, type: :channel).call
  end

  # 追加
  def self.everyone(message:)
    new(message: message, type: :everyone).call
  end

  def initialize(message:, type:)
    @message = message
    @type = type
  end

  def call
    return if type == :everyone && !channel.general? # 追加

    members.each do |member|
      message.mentions.create!(to: member, chennel: channel)
    end
  end

  private

  attr_reader :message, :type

  def members
    @members ||= if type == :here
      channel.members.active - [creator]
    else
      channel.members - [creator]
    end
  end
end

結果として、callメソッドに分岐が一つ増えることになりました。このように分岐が増えていくと、1つのユースケース(例えばhereメンションのとき)だけについて考えたい状況でも別のユースケース(channelやeveryoneメンションのとき)のコードについて理解しなければいけなくなり、そのぶん可読性が落ちます。また、コードを修正したときに想定していない箇所でバグを仕込んでしまう、というケースも次第に増えていくことでしょう。

最初の例のように1つの処理ごとにクラスを作り、できるかぎり分岐を避けることでメンテナンスしやすくなります。