絶対に笑ってはいけないRailsコード集

[株式会社LITALICO](http://litalico.co.jp/) の[klrutsa](http://qiita.com/klriutsa)です。
『[LITALICO Advent Calendar 2016](http://qiita.com/advent-calendar/2016/litalico)』13日目の記事です。

# はじめに

私が遭遇した、Railsアンチパターン集です。
笑えるよりも、笑えないコードのほうが多いですが、よろしくお願いします。

前回の、[負債を抱えすぎたRailsアプリのリファクタリング - Qiita](http://qiita.com/klriutsa/items/09705c87ed221371d0ac)では、複雑な状態遷移への対応方法を書きましたが、その他の負債をどうしたかみたいなことについて書いてみます。

一般的に書いてはいけない、とまではいえないかもしれないですが、
個人的には書かないほうが良いと思っているコード集です。

# default_scope

```ruby
class Article < ActiveRecord::Base
  default_scope { where(status: 'publish') }
end
```

## 要点
* プログラマの認識している動作と実際の動作がズレる
* unscopeしたいために無駄なメソッドが増える
* 一度実装してしまうと、この行を削除するのが困難



## 解説

default_scopeを定義すると、書いたところを勝手に追加してくれます

```ruby
> Article.limit(10)
# SELECT `articles`.* FROM `articles` WHERE `articles`.`status` = 'publish' LIMIT10
```

default_scopeを無効にしたいときは


```ruby
> Article.unscoped.limit(10)
# SELECT `articles`.* FROM `articles` WHERE `articles`.`status` = 'publish' LIMIT10
```

と宣言しなければならなくなり不便になります。
また、default_scope前提で作られてしまうと、後から直すのが非常に難しくなってしまいます


更にassociationが絡んでくると更に不便になります。

以下のようなUserモデルがあった場合

```ruby
class User < ActiveRecord::Base
  has_many :articles
end
```

articlesを参照するとdefault_scopeが有効になってしまいます

```ruby
> User.first.articles
# SELECT `articles`.* FROM `articles` WHERE `articles`.`user_id` = 1 AND `articles`.`status` = 'publish'
```

これをunscopedにするのはなかなか大変で

```ruby
class User < ActiveRecord::Base
  #....
  def unscoped_articles
    Article.unscoped.where(user_id: self.id)
  end
end
```

```ruby
> User.first.unscoped_articles
# SELECT `articles`.* FROM `articles` WHERE `articles`.`user_id` = 1
```

というメソッドを定義するのが一番簡単な方法です。

## 実際の書き換え作業

dafault_scopeを外す作業は大変です。
それは、**少しずつ置き換える**というのが無理だからです。

以下の方法でやるのが良いと思います

* 該当箇所のテストをほぼ全て書く。
* default_scopeを無効にして、該当箇所をすべて書き直す
* テストが通ることを確認する
* 実際に触ってみて確認する

## 解決策
* default_scopeは使わない

# とりあえずインスタンス変数

```ruby
def show
  @item = Item.find_by(id: params[:id])
  @user = User.find_by(id: @item.user_id)
  @user_name = @user.name
  @user_id = @user.id
end
```

```erb
<div>
  <%= @item.title %>
  <%= @user_id %>
  <%= @user_name %>
</div>
```

## 要点
* インスタンス変数が1つでも欠けると動かなくなるView
* 責任がコントローラに存在する
* Viewに必要なインスタンス変数はhtml.erbを見ないと分からない

## 解説

これも、あるある非常によく見る光景だと思います。

おそらく元のコードは

```ruby
def show
  @item = Item.find_by(id: params[:id])
end
```

こうだったんだと思います。
そのあと、Viewに表示したい項目が増えて、インスタンス変数を定義して追加した結果こうなった。
という感じだと思います。

そのうち、「モデルからデータを取ってきて、コントローラにインスタンス変数をたくさん定義して、ビューがそのインスタンス変数を参照する。」
ということがMVCなのだと間違った理解をしてしまいます。

なので、インスタンス変数を一つだけにして

```erb
<div>
  <%= @item.title %>
  <%= @item.user.id %>
  <%= @item.user.name %>
</div>
```

というような感じでビューを書き直すと良いと思います。

## 解決策
* インスタンス変数は基本的に少ない方が良い、究極的には1つ
* 複数モデルにアクセスするときは、facadeパターンを使おう


# 間違ったリファクタリング

```ruby
class ArticleController < ActionController::Base
  def create
    ArticleCreateService.create(params[:id])
  end
end

class ArticleCreateService
  def self.create
    # 処理1
    # 処理2
    # 処理3
    # 処理4
  end
end
```

## 要点
* 機能の重複を消すのではなく、コードの重複を消すことをやっている
* クラスメソッドを使っている点

## 解説

リファクタリングは重複を無くすというような意味ですが、
**コードの重複を無くす**だけではだめです。
**機能の重複を無くして、抽象的に表現するように書き換える**というのがリファクタリングです。

このコードは変更前は以下のように書かれていたんだと思います。

```ruby
class ArticleController < ActionController::Base
  def create
    # 処理1
    # 処理2
    # 処理3
    # 処理4
  end
end
```

実際にコードを読む作業になると、変更前と変更後、どっちも処理1~4まで読まなきゃわからないです。
なので、これはリファクタリングではなく、コードを単に**外へ逃しただけ**です。
よって、コードが読みやすくなったわけではないです。

```ruby
    # 処理1
    # 処理2
    # 処理3
    # 処理4
```

フォーカスするべき点はこの処理一行一行をリファクタリングすることです。
そうすると、ほんとうの意味のリファクタリングができるはずです。

## 解決策
* リファクタリングを正しく理解して書き直す
* 変更前と変更後の読むコストが減るように書く


# ApplicationControllerにbefore_actionを書く

```ruby
class ApplicationController < ActionController::Base
  before_action :find_tags

  def find_tags
    @tags = Tag.all.limit(10)
  end
end

class ArticleController < AplicationController
  skip_before_action :find_tags
end
```

## 解説

before_actionは各actionの前に実行してくれる便利な機能です。
で、便利なのですが、安易に使わないほうが良いメソッドです

実例ですが、最初このようにskip_before_actionなしで、コーディングしていました。

```ruby
class ArticleController < AplicationController
  # 各action
end
```

で、これだと、find_tagsが実行されてしまいます。
そこで、skip_before_actionを書いて対処しています。

このような**プログラマの認識と実際の動作がズレる**コードは要注意です。

## 実際の書き換え作業

この書き換え作業もかなり大変です。
ほぼすべてのコントローラーを書き換えなきゃいけなくなります。

同様に

* 全てのコントローラーのテストを書く
* before_actionを消す
* テストが通ることを確認する
* 人力でも確認してみる

という方法しかないですね。

## 解決策
* 大本のAplicationControllerにbefore_actionなどの処理を書かない


# rm -rf
```ruby
class ItemController < ActionController::Base
  def update
    #...
    system "rm -rf "+ @path + ".*"
  end
end
```

## 要点
* 魔のコードの`rm -rf /`に準ずるコマンドを実行できてしまう
* コントローラのテストを書いて、updateを実行すると予期しないファイルを消してしまう恐れがある

## 解説

`system "コマンド"`でコマンドを実行できます

もし

```ruby
system "rm -rf "+ @path.to_s + ".*"
```
とかだったら、もっと最悪ですね
`@path`がnilのときは`"rm -rf .*"`となってしまうので、いろんなファイルを消してしまいます。

このコードの予想される被害パターンとしては
コードを良く見ていない開発者が、

**「とりあえずよくわからないし、このコントローラのテストを書いてみて実行するか」**

ってなると、危ないですね。開発者の環境で**意図しないrm -rfが実行**される可能性があります。
さらに、本番で間違えてテストを実行しちゃって、rm -rfを実行するかもしれません。
そうなると、大規模障害待ったなしです。
もはや**TDD開発者を死に至らせる罠**です。

## 解決策
* systemやらcommandは極力使わない
* FileUtils等のライブラリ使う


# 終わりに

僕はrm -rfが一番笑えました。

明日は @5kakrさんのプラグアンドプレイで実現する手間なし拠点間ネットワークです
お楽しみに