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

無料で読める[Rails antipattern](http://it-ebooks.info/book/1504/)から一つずつエッセンスをメモがてら。割と有用そうな本なのに訳されてないのは残念ですな。あくまでざっくりメモなのでご容赦ください。

ソリューション: [デメテルの法則](https://ja.wikipedia.org/wiki/%E3%83%87%E3%83%A1%E3%83%86%E3%83%AB%E3%81%AE%E6%B3%95%E5%89%87)を参考に、delegateの使用を検討する
=
以下のようなモデル、関連があるとする。

```.rb
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を使ったとする

```.erb

<%= @invoice.customer_name %> 
<%= @invoice.customer.address.street %> 
<%= @invoice.custome.address.city %>,
<%= @invoice.customer.address.state %>
<%= @invoice.customer.address.zip_code %>
```
これはカプセル化の概念に反している。もしcustomerが請求先住所と運送先住所を別々にもつようになったら、このようにオブジェクトをまたいで直接属性にアクセスしている箇所を全て修正しないといけない。

[デメテルの法則](https://ja.wikipedia.org/wiki/%E3%83%87%E3%83%A1%E3%83%86%E3%83%AB%E3%81%AE%E6%B3%95%E5%89%87)というのがある。一言で言うと、`.`は一つだけにしておけ、という法則。たとえば`@invoice.customer.address.street`なんてのは`.`が4つもあるのでこの観点から言うとよろしくない。まあもちろんこれは極論ではあるけど、従うべきガイドラインにはなると。
この法則を適用すると、

```.rb
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のコードはこう変わります。

```.erb

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

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

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

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

```html
<html> 
  <body>
    <ul>
      <% @users.each do |user| -%>
        <li><%= user.last_name %> <%= user.first_name %></li> 
      <% end %>
    </ul> 
  </body>
</html>
```
さらに特定の検索条件のところを、scopeを使ってモデルに寄せる。

```.rb

class User < ActiveRecord::Base
 scope :ordered, order("last_name")
end
```
これで、ロジックがモデルに収まった。
##結論:view、controllerにfinderロジックを書くな。モデルのscopeとか使おう。

ソリューション:そのモデルのfinderは「自身」の中に置いておくこと
=
アプリがもっと大きくなって、以下のようなちょっと複雑なfinderをコントローラーに書くことになった。

```.rb
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メソッドに責任を委譲する。

```.rb
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モデルの実装を知りすぎていることだ。責任を**適切な**モデルに委譲することで、少し改善できる。

```.rb
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を使うことによって

```.rb

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

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

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