Controllerのリファクタリング ~または私は如何にして肥大化していくControllerをやめてDHHによるControllerの書き方を愛するようになったか~

こんにちは！
Misoca開発チームのめろたん(@renyamizuno_)です！

最近LEGOランドが開園したので行きたいのですが、地味に遠いため行けてないマンです。

最近あった面白いことは、スマホでスクショとか撮れないなぁと思って色々確認したら、

最近スクショとかカメラ撮れんなーと思ってストレージがいっぱいなのかなぁ
と思って見たら
最大が25GBに対してアプリ1TBとかでクソ笑う pic.twitter.com/inymgYrToH
— わかり亭めろたん。 (@renyamizuno_) 2017年4月7日

アプリとかキャッシュとかがスマホのストレージ容量をありえないくらい超越してたことです。

はい。

今回は最近ありえないほど大きくなってしまったControllerをいかにリファクタリングしたか*1というのをざっと書きたいと思います。

おおきくなるController

どうしても大きくなっちゃいますよね。

なにも考えないと大きくなっちゃうのは知ってたので*2、 Controllerのアクションをmoduleとして切り出して、Controllerにincludeするようにしていました。

# app/controllers/hoge_contoller.rb
class HogeController < ApplicationController
  include HogeController::FugaActions

  def show
  end
    
  # ...
end

# app/controllers/hoge_contoller/fuga_actions.rb
class HogeController
  module FugaActions
    def fuga
      # ...
    end
  end
end

最初の頃は、「これめっちゃええやん。最高かよ。」と喜んでいました。

ですが、これで思考を停止してしまい機能を増やすたびにHogeActionsを増やすという愚行をやっていました。思考を停止しているのでなーんにも気にせず追加していたのですが、ある時メンバーから「これは…ひどいよ…。」という声が上がってハッとして見直したら

class HogeController < ApplicationController
  include HogeController::FugaActions
  include HogeController::FugaFugaActions
  include HogeController::FugaFugaFugaActions
  include HogeController::FugaFugaFugaFugaActions
  # ...

  before_action :hoge
  before_action :hogehoge, only: :add_hoge
  # ...

  def show
  end
    
  # ...
end

実際のコードを乗っけるのは流石にアレなのでざっくりとしかかけないのですが、実際はもっとひどくてなんか大変でした…。

さあリファクタリングだ

現状がやっべぇのは認識できたのでそこからどう直すか考え方針をまとめて、
メンバーにレビューしてもらい方針を固めました。

大きく3ステップで進めようとなりました。

1. たりとらんテストを足せ

そもそもなんでテスト足りてないんだよ…という話なのですが、申し訳。

Misocaではビジネスロジックをクラスに切り出して「ユースケースオブジェクト」と呼んでいます。 Controllerはユースケースオブジェクトを呼び出して、ビジネスロジック処理の結果を受け取るという風にしています。でそのユースケースオブジェクトのユニットテストが明らかに足りていないものが多くありました…。

リファクタリングをして動作が壊れてもテストがないとわからないので、まずそのへんのテストをちゃんと一通り書こうとなりました。

2.責務を切り離せ

ユースケースオブジェクトに「『これ』をやって『これ』をする。」のような感じの物がありました。具体的には「権限を確認」して、問題なければ「なにかを作る」のような感じでした。

これは複数の事を一つのクラスでおこなっているため良くないね。というような感じになりました。なので責務を切り分けて、別のクラスに切り出しました。もちろんテストもちゃんとかきました。

3.HogeActionsをHogeControllerに

最後にHogeActionsとなっているのをHogeControllerに変更します。

方針としては、下の記事を参考にしてやりました。

postd.cc

これはとても一般的なパターンですよね。私も以前はもっとこのパターンを使っていました。しかし今は「いやいや違う」と思うようになりました。indexという単一のメソッドのみを持つInboxes::PendingsControllerという、新しいコントローラを持つのです。

基本的に彼が言っているのは、コントローラはデフォルトのCRUDアクションindex、show、new、edit、create、update、destroyのみを使うべきだということです。その他のアクションはどれも専用の（それ自体はデフォルトのCRUDアクションしか持たない）コントローラの作成につながるのです。

現状のコードを見ると、

class HogeController
  module FugaActions
    def fuga
    end
  end
end

となっている。

のでこれを上の記事に従ってControllerに書き換えます。

module Hoge
  class FugaController
    def create
    end
  end
end

このfugaアクションはFugaを作るものだったので、DHHの教えに従ってcreateにリネームしました。

あとはroutes.rbを修正するのみです。

resources :hoge, only: [:show] do    
  member do
    post :fuga
  end
end

というのを

resources :hoge, only: [:show] do    
  member do
    resources :fuga, only: :create, module: 'hoge'
  end
end

のように修正して完成です！！！

まとめ

まだリファクタリング途中なのでアレですが…

before

class HogeController < ApplicationController
  include HogeController::FugaActions
  include HogeController::FugaFugaActions
  include HogeController::FugaFugaFugaActions
  include HogeController::FugaFugaFugaFugaActions
  # ...

  before_action :hoge
  before_action :hogehoge, only: :add_hoge
  # ...

  def show
  end
    
  # ...
end

after

# app/controllers/hoge_contoller.rb
class HogeController < ApplicationController
  def show
    # ...
  end
end


# app/controllers/hoge/fuga_contoller.rb
module Hoge
  class FugaController < ApplicationController
    # アクセスできるできるユーザか確認
    before :authenticate

    def create
      # ...
    end

    private

    def authenticate
      # ...
    end
  end
end

別のクラスになるのでどこまで影響があるのかわかりやすくなりましたね。たとえばbefore_actionが、別ファイルにあるアクションに影響が〜とかがなくなったので良いですね。またCRUDアクションだけになるので、なにをするのかがControllerの名前だけみればわかるようになったのも良かったです。

あとDHHがこれやってるっていうのがめっちゃ安心感があってよかったです。