株式会社LITALICO のklrutsaです。
『LITALICO Advent Calendar 2016』13日目の記事です。

はじめに

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

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

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

default_scope

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

要点

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

解説

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

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

default_scopeを無効にしたいときは

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

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

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

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

class User < ActiveRecord::Base
  has_many :articles
end

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

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

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

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

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

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

実際の書き換え作業

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

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

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

解決策

default_scopeは使わない

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

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

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

要点

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

解説

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

おそらく元のコードは

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

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

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

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

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

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

解決策

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

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

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

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

要点

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

解説

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

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

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

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

    # 処理1
    # 処理2
    # 処理3
    # 処理4

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

解決策

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

ApplicationControllerにbefore_actionを書く

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なしで、コーディングしていました。

class ArticleController < AplicationController
  # 各action
end

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

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

実際の書き換え作業

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

同様に

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

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

解決策

大本のAplicationControllerにbefore_actionなどの処理を書かない

rm -rf

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

要点

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

解説

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

もし

system "rm -rf "+ @path.to_s + ".*"

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

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

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

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

解決策

systemやらcommandは極力使わない
FileUtils等のライブラリ使う

終わりに

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

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

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

はじめに

default_scope

要点

解説

実際の書き換え作業

解決策

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

要点

解説

解決策

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

要点

解説

解決策

ApplicationControllerにbefore_actionを書く

解説

実際の書き換え作業

解決策

rm -rf

要点

解説

解決策

終わりに