Edited at

Railsアンチパターン<モデル編>①覗き見

More than 3 years have passed since last update.

無料で読めるRails antipatternから一つずつエッセンスをメモがてら。割と有用そうな本なのに訳されてないのは残念ですな。あくまでざっくりメモなのでご容赦ください。


ソリューション: デメテルの法則を参考に、delegateの使用を検討する

以下のようなモデル、関連があるとする。

class Address < ActiveRecord::Base

belongs_to :customer
end

class Customer < ActiveRecord::Base
has_one :address
has_many :invoices
end

class Invoice < ActiveRecord::Base
belongs_to :customer
end

それについて以下のようなviewを使ったとする


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

これはカプセル化の概念に反している。もしcustomerが請求先住所と運送先住所を別々にもつようになったら、このようにオブジェクトをまたいで直接属性にアクセスしている箇所を全て修正しないといけない。

デメテルの法則というのがある。一言で言うと、.は一つだけにしておけ、という法則。たとえば@invoice.customer.address.streetなんてのは.が4つもあるのでこの観点から言うとよろしくない。まあもちろんこれは極論ではあるけど、従うべきガイドラインにはなると。

この法則を適用すると、

class Address < ActiveRecord::Base

belongs_to :customer
end

class Customer < ActiveRecord::Base
has_one :address
has_many :invoices

def street
address.street
end

def city
address.city
end

def state
address.state
end

def zip_code
address.zip_code
end
end

class Invoice < ActiveRecord::Base
belongs_to :customer

def customer_name
customer.name
end

def customer_street
customer.street
end

def customer_city
customer.city
end
def customer_state
customer.state
end

def customer_zip_code
customer.zip_code
end
end

するとviewのコードはこう変わります。


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

.が一つになった。が、ラッパーメソッドでクラスが埋め尽くされてちょっとだるくなったり、invoiceクラスのインタフェースが汚くなる(wikipediaにも同様の欠点が書かれている)。が、Railsにはdelgateという便利な機能があるのでそれを使えば、

class Address < ActiveRecord::Base 

belongs_to :customer
end

class Customer < ActiveRecord::Base
has_one :address
has_many :invoices
delegate :street, :city, :state, :zip_code, :to => :address
end

class Invoice < ActiveRecord::Base
belongs_to :customer
delegate :name,
:street,
:city,
:state,
:zip_code,
:to => :customer,
:prefix => true
end

とシンプルに書ける。


結論:delegate使おう


ソリューション:生のfind()呼び出しを避け、モデルにfinderメソッドを実装する

とにかくドメインモデルをcontrller,viewから追い出す

そこらかしこでmodelのfinderをよんでしまうと保守性が下がる。

例えば、すべてのUserをlastnameで並び替えて表示したいとする。viewでそのままfindをよんでしまうと、

<html>

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

確かにこういうのも可能だが、MVCの原則を守るなら、以下のようにすべきだ。

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>

さらに特定の検索条件のところを、scopeを使ってモデルに寄せる。


class User < ActiveRecord::Base
scope :ordered, order("last_name")
end

これで、ロジックがモデルに収まった。


結論:view、controllerにfinderロジックを書くな。モデルのscopeとか使おう。


ソリューション:そのモデルのfinderは「自身」の中に置いておくこと

アプリがもっと大きくなって、以下のようなちょっと複雑なfinderをコントローラーに書くことになった。

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

前述のソリューションを適用して、モデルのfinderメソッドに責任を委譲する。

class UsersController < ApplicationController 

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

class User < ActiveRecord::Base
has_many :memberships

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

コントローラーは薄くなったが、もうちょっと改善できる。問題は、User.find_recent_active_membershipsがMembershipモデルの実装を知りすぎていることだ。責任を適切なモデルに委譲することで、少し改善できる。

class User < ActiveRecord::Base 

has_many :memberships

def find_recent_active_memberships
memberships.find_recently_active
end
end

class Membership < ActiveRecord::Base
belongs_to :user

##ここ
def self.find_recently_active
where(:active => true).limit(5).order("last_active_on DESC")
end
end

さらにscopeを使うことによって


class User < ActiveRecord::Base
has_many :memberships

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

class Membership < ActiveRecord::Base
belongs_to :user
scope :only_active, where(:active => true)
scope :order_by_activity, order('last_active_on DESC')
end

これにより、各々の操作の責任が適切なモデルに分散された。素晴らしい。ただし、見ればわかるように、

memberships.only_active.order_by_activity.limit(5)

少しデメテルの法則に反してしまっている。まあ関連についてさらにメソッドを呼び出す、という流れになるので多少は仕方ないのだけど。すべてのアンチパターン(の対策)は銀の弾丸というわけではないので、それぞれの功罪と自分のアプリケーションの未来を踏まえて適切に判断していくことが重要だ。


結論:finderメソッドは適切なモデル(主に対象モデル)に責任を委譲しよう。