株式会社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さんのプラグアンドプレイで実現する手間なし拠点間ネットワークです
お楽しみに