Rails アンチパターン - 覗き見モデルクラス（Voyeuristic Models）

はじめに

引き続きRails AntiPatternsという本を読んでいます。

https://www.amazon.co.jp/dp/B004C04QE0/ref=dp-kindle-redirect?_encoding=UTF8&btkr=1

前回は2 Domain Modelingの１つめ「Authorization Astronaut」についてまとめました。

waterlow2013.hatenablog.com

今回は1 Modelsの１つめ「Voyeuristic Models」についてまとめていきます。

Voyeuristic Models

Ruby on Rails、MVC、およびオブジェクト指向プログラミングが提供する構造を認識していない場合などに、オブジェクト指向の基本的な原則を破ってしまうことがあります。またActiveRecordなどの便利な機能に乗りすぎてしまう場合もあります。
ここでは、MVCとオブジェクト指向プログラミングの教えに違反するいくつかのサンプルを見ていきます。

Solution: Follow the Law of Demeter（デルメルの法則に従う）

デメテルの法則 - Wikipedia

サンプルで見ていこうと思います。以下のようなモデルがあるとします。

class Address < ApplicationRecord
  belongs_to :customer
end
class Customer < ApplicationRecord
  has_one :address
  has_many :invoices
end
class Invoice < ApplicationRecord
  belongs_to :customer
end

顧客は住所を１つ、請求を複数持ちます。請求の住所行を表示するビューは、次のようになります。

<%= @invoice.customer.name %>
<%= @invoice.customer.address.street %>
<%= @invoice.customer.address.city %>,
<%= @invoice.customer.address.state %>
<%= @invoice.customer.address.zip_code %>

適切なカプセル化を考えると、@invoiceはcustomerを経由してaddressにアクセスするべきではありません。たとえば、顧客が請求先住所と配送先住所の両方を持つようにアプリケーションを変更すると、これらのオブジェクトを通過して通りを取得するコード内のすべての場所が壊れ、変更する必要があるためです。

幸いにも、ActiveRecordには、delegateというメソッドがあります。このデリゲートメソッドを使用すると、addressオブジェクトのことを知ることなく顧客の住所を表示できます。

class Address < ApplicationRecord
  belongs_to :customer
end

class Customer < ApplicationRecord
  has_one :address
  has_many :invoices
  delegate :street, :city, :state, :zip_code, to: :address
end

class Invoice < ApplicationRecord
  belongs_to :customer
  delegate :name, :street, :city, :state, :zip_code, to: :customer, prefix: true
end

viewのコードは以下のようになります。

<%= @invoice.customer_name %>
<%= @invoice.customer_street %>
<%= @invoice.customer_city %>,
<%= @invoice.customer_state %>
<%= @invoice.customer_zip_code %>

これやったほうがいいとは思うのですが、関連するオブジェクトをそのまま取って出すだけなら最初のままでもいい気がします。ただ悩ましいところではある…。これちょっと修正したいときに.gsubとかviewに書いてしまう事があって、それを助長する気がしています。そしてDRYでない子０ドが生まれていくという。なので１回これに従ってみるのもいいかなと思えてきました。

Solution: Push All find() Calls into Finders on the Model（生のクエリメソッドは全てモデルに書く）

これもサンプルを見ます。

<html>
  <body>
    <ul>
      <% User.order(:last_name).each do |user| -%>
        <li><%= user.last_name %> <%= user.first_name %></li> <% end %>
    </ul>
  </body>
</html>

これはビューに直接sql呼び出しが書いている状態であり、重複するロジックがそれぞれの場所で生まれてしまう可能性があります。
ひとまずコントローラに移します。

class UsersController < ApplicationController
  def index
    @users = User.order(:last_name)
  end
end

<html>
  <body>
    <ul>
      <% @users.each do |user| -%>
        <li><%= user.last_name %> <%= user.first_name %></li> <% end %>
    </ul>
  </body>
</html>

これでモデルのロジックはなくなりましたが、コントローラにロジックの記載が残っています。これをモデルに移します。

class User < ApplicationRecord
  scope :ordered, -> { order(:last_name) }
end

class UsersController < ApplicationController
  def index
    @users = User.ordered
  end
end

Solution: Keep Finders on Their Own Model （生のクエリメソッドは自身のクラスにしか書かない）

検索コールをRailsアプリケーションのコントローラレイヤーからモデル上のカスタムファインダに移動することは、メインテナンス可能なソフトウェアを作成する正しい方向への強力なステップです。しかし、一般的な間違いは、適切な責任の委任を無視して、それらの呼び出しを最も近いモデルに移動することです。

耳が痛い話だ！！！

class UsersController < ApplicationController
  def index
    @user = User.find(params[:id])
    @memberships =
      @user.memberships.where(active: true).
        limit(5).
        order("last_active_on DESC")
  end
end

今まで学んだことに基づいて、そのスコープチェーンをUserモデルのメソッドに徹底的に移動します。UsersControllerを扱っているので、これが最良の場所のようだと思います。

class UsersController < ApplicationController
  def index
    @user = User.find(params[:id])
    @recent_active_memberships = @user.find_recent_active_memberships
  end
end

class User < ApplicationRecord
  has_many :memberships

  def find_recent_active_memberships
    memberships.where(active: true).
      limit(5).
      order('last_active_on DESC')
  end
end

これは間違いなく改善です。しかし、もう少しすることができます。Userモデルはメンバーシップモデルの実装について、active、last_active_onというカラムのことを知ってしまっています。これは、まだメソッドを十分にプッシュしていないという手がかりです。

（途中経過を省き最終型のみ載せます）

class User < ApplicationRecord
  has_many :memberships

  def find_recent_active_memberships
    memberships.only_active.order_by_activity.limit(5)
  end
end

class Membership < ApplicationRecord
  belongs_to :user

  scope :only_active, where(active: true)
  scope :order_by_activity, order('last_active_on DESC')
end

scopeにしたことによりロジックを組み合わせて使うことができます。

scopeにしたことが良いのと、limit(5)がUserモデルにあるのがすごくいい感じがしています。

このアプローチには、読みやすさとシンプルさの問題だけでなく、デメテルの法律の乱用などの短所もあります。
このアプローチを使用するかどうかは、あなた次第です。多くの高度なリファクタリングと同様に、これは簡単な答えがなく、好みにも大きくよるものです。

まとめ

正直ここまで徹底してやったことがなく、これを都度やっていくコストがどれくらいなのかわかりません。ただ、毎回コントローラに書いていってしまうと共通化、scope化するのがどんどん難しくなっていくので、ある程度の規模ではやるべきなのかなと思っています。モデル50個からとか？
とりあえず、次自分で何か書くときにはこれ徹底してみようと思います。

blog.waterlow.work

Ruby, Rails, js, etc...