Railsでの危ないコーディング

既存のRailsアプリを改修するにあたって目の当たりにした光景を幾つか紹介する。
よくあることなのかどうかは定かではない。

ActiveRecordのモデルでafter_initializeを使用して値を更新する


after_initialize は初期化後に呼ばれるメソッドであるが、そこで値を更新(変換)をしているケースがあった。主に編集画面で表示される値と格納される値が違う場合に使用されていた。格納されている値を生で参照できなくなってしまって不都合が起きたり、一覧で取得したい際も呼ばれるので無意味なロジックが走る場合がある。

私はこの場合、 after_initialize は極力使用しないで、明示的に変換ロジックを呼んでいる。また、永続化されていない属性に対しては使用してもいいと思う。

同じアクションで get と post で場合分け


hoge_controller#setting で取得(get)と更新(post)を両方受け入れていた。

def setting
  request.get? ? show_setting : update_setting
end

素直にアクションを別にすべき。URI設計をしていなかったのだと思われる。

valiadtion で値を書き換える


validate メソッドを定義し、値が正常であればDBに格納するために変換を行う。最終的にvalid?がfalseになる状況であれば値を再度戻すなどかなりしんどいことをしていた。
このケースだと、複数のモデルに対して一度バリデーションだけ実行し、全て問題なければsaveを実行するということができない。1回目のバリデーションで値が変更されているのでsave時のバリデーションでエラーになるはずだからだ。

そもそも、バリデーションは検証のみなので値の変更はすべきではない。

リクエストパラメータ(params)を書き換える


view側で使用するためにparams[:hoge]の情報がそのまま使えないのならインスタンス変数で格納すべき。

helperにインスタンス変数

def convert_hoge(hoge_id)
  foo = @fuga.fuga
  foo + hoge_id
end

確かに、helper内でもコントローラで設定したインスタンス変数は使用できるが、外部状態に依存するのは気持ち悪い。ヘルパーメソッドはインプットが同じならアウトプットは同じであるべき。テストを書いていれば、こういう実装にはならないはず。


こういう状態になっている場合は大抵テストもあまり書かれていないのがほとんどなのでにっちもさっちもいかない状況になっている。末期症状に近い状況だった。
こうならないためにも、テストをしっかり書き、コードレビューを綿密に行うように心がけている。