Rails Best Practices の警告をちゃんと考える

Railsを使っている際にはRails Best Practicesの内容を確認したりしているのですが、今までなんとなく「あー、この方がいいんだー」って眺めていただけなのですが、数が増えてきたので、いい加減ちゃんと読もうと思いました。

結構面白いものもあり、「へー」と感じたものを中心にまとめてみました。

### 更新履歴
#### 2014/12/15 追加

* [Overuse route customizations](#overuse-route-customizations) を追加
* [Always add DB index](#always-add-db-index) を追加


## Use Query Attribute

http://rails-bestpractices.com/posts/56-use-query-attribute

`ActiveRecord` の attribute に値が入っているかチェックする際に `nil?`, `blank?`, `present?` とかは使わなくていいって意味です。
代わりに 'attribute名' + '?' を使いましょうって話です。

悪い例

```erb
<% unless @member.name.present? %>
  名無しの権兵衛
<% end %>
```

良い例

```erb
<% unless @member.name? %>
  名無しの権兵衛
<% end %>
```


## Law of Demeter

http://rails-bestpractices.com/posts/15-the-law-of-demeter

Law of Demeter とは日本語では「[デメテルの法則](http://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)」と呼ばれ、簡単にまとめると、`a.b.c()` という形でオブジェクトの関数などを介して、もう1つ先の関数などを呼び出すのは __良くない__ というものです。

悪い例

```ruby
member.last_name.kana
```

良い例

```ruby
class member
    :
  def last_name_kana
    last_name.kana
  end
    :
end

member.last_name_kana
```

上の悪い例のように`なんとか . なんとか ． 関数` とつなげていくのではなく、関数にしてしまい関数の中でその先を呼び出す形にするべきである、ということです。
こうすることにより、コンポーネント（上の場合はクラス）間の依存関係が少なくなり、いざというときの修正や変更が楽になる、ということです。


## Replace Instance Variables with Local Variables

http://rails-bestpractices.com/posts/27-replace-instance-variable-with-local-variable

Partial View でも、インスタンス変数（`@variable`）を使えますが、partial を含んでいる View との依存関係が強まり、他のところで使いまわしにくくなります。

インスタンス変数ではなくローカル変数として、`render` で呼び出す際に明示的に渡した方がコードの見渡しが良くなります。

##### 悪い例

コントローラ

```ruby
def show
  @members = Member.all    <%# インスタンス変数に値を設定 %>
end
```

アクションのビュー（例：show.html.erb）

```erb
  <%= render partial: 'member_names' %>   <%# ここで暗黙に @members を渡しているのと同じ %>
```

パーシャル（例：_member_names.html.erb）

```erb
<%# @members に依存しているけど、このファイルを開くまでは分からない %>
<%= @members.map { |m| m.name } %>
```

##### 良い例

アクションのビュー（例：show.html.erb）

```erb
  <%= render patial: 'member_names', locals: { members: @members } %>   <%# 明示的にローカル変数として渡している %>
```

注：上のコードは実は冗長です。下の 'Simplify Render in Views' 参照のこと

パーシャル（例：_member_names.html.erb）

```erb
<%# ローカル変数を使う %>
<%= members.map { |m| m.name } %>
```

## Simplify Render in Views

http://rails-bestpractices.com/posts/61-simplify-render-in-views

render って実は結構省略して書けますって話です。
上の例を見てみましょう。

###### Before

```erb
<%= render partial: 'member_names', locals: { members: @members } %>
```

##### After

```erb
<%= render 'member_names', members: @members %>
```

`partial` や `locals` を省いても大丈夫です。だいぶスッキリしましたね。


## Move Model Logic into Model

http://rails-bestpractices.com/posts/7-move-model-logic-into-the-model

MVC (Model View Controller) ではコントローラはなるべく仕事をしない、というのが基本発想です。
なので、

```ruby
class MembersController < ApplicationController
    :
  def update_phone_number
    @member = Member.find(params[:id])

    phone_number = phone_number_params[:phone_number]

    @member.is_temporary = true
    @member.phone_number = remove_non_digit(phone_number)

    @member.save
  end
    :
end
```

と書いてしまうと、

* コントローラにロジックが多くなってしまう。
* `MembersController` と `Member` との依存関係が強くなってしまう
* 流用性が低くなってしまう（ロジックを他で使いにくくなってしまう）

といった弊害がうまれてしまいます。
コントローラではなく、モデルに書くようにしましょう。

```ruby
class Member < ActiveRecord::Base
    :
  # ロジックはモデルの中に書く
  def update_phone_number(phone_number)
    self.is_temporary = true
    self.phone_number = remove_non_digit(phone_number)
    save
  end
    :
end

class MembersController < ApplicationController
    :
  def update_phone_number
    @member = Member.find(params[:id])
    phone_number = phone_number_params[:phone_number]
    @member.update_phone_number(phone_number)
  end
    :
end
```

コントローラがすっきりしましたね１


## Remove Empty Helpers

http://rails-bestpractices.com/posts/72-remove-empty-helpers

Rails 便利なんですが、controller とか scaffold を作ると自動的にいらない helper とかも作られちゃうんですよね。

あまり気持ちいいものでもないので、消してちゃいましょう。


## Remove Trailing Whitespace

http://rails-bestpractices.com/posts/60-remove-trailing-whitespace

コードの行末に空白が残ってしまうと消したときとかに Diff に表示が出てしまって邪魔なので、コミットする前に消すようにしましょう！

と言っても気にしていないと簡単に入ってしまうので、エディタ設定で目立つようにするとか、勝手に消してくれるようにしましょう！

ちなみに [Vim](http://www.vim.org/) ユーザな僕は [vim-trailing-whitespace](https://github.com/bronson/vim-trailing-whitespace) を入れて、赤く表示されるようにしています。

![スクリーンショット 2014-12-03 18.12.17.png](https://qiita-image-store.s3.amazonaws.com/0/29305/eac97fb9-0468-af6b-e81b-ec7185194851.png)

さらに、`:FixWhitespace` と打つと一発で消してくれます。Vim 使いの方はどうぞ〜。

他にも [commit hook を使う方法](http://qiita.com/k0kubun/items/8f6d7eded1d833187449) っていうのもあります。こちらも設定しておくのがいいかもしれませんね。


## Overuse route customizations

http://rails-bestpractices.com/posts/10-overuse-route-customizations

Rails の `routes` って気を付けてないと、美しないコードがどんどん追加されていきます。

最初は

「とりあえず1つしか Action ないし、get 1行でいいや」

```ruby
get 'posts/get_post'
```

が

「あー、前の変えると他の場所も変えないと行けないから面倒だな―。post 1行追加でいいや。」

```ruby
post 'posts/update_post'

get 'posts/get_post'
```

を繰り返し、やたらカスタムな（RESTFUL でない）Routes ができ上がってしまいます。

そうなる前になるべく早めに

```ruby
resources :posts
```

みたいに Rails の標準にのっとったものを使うと、RESTFUL で綺麗なインターフェースが作れます。


## Always add DB index

http://rails-bestpractices.com/posts/21-always-add-db-index

まあ、ちゃんと Index を作ろうねって話です。
特に Rails だと簡単に Migration で外部キーとかを作れてしまい、意外と忘れがちです。
SORT や GROUP BY も Ruby のコードで書けてしまうので、同様に意識しとかないと、やたら遅いクエリーができあがってしまいます。


```ruby
class CreateUsers < ActiveRecord::Migration
  def change
    create_table "computer" do |t|
     t.string :name
     t.integer :os_id

     t.timestamps
    end

    add_index :users, :os_id  # <= これですね
  end
end
```