woshidan's blog

そんなことよりコードにダイブ。

Railsのコントローラのアクションがやたらと縦に長くて読みにくいときに確認したいこと

今週はなんだか見た目はそんなでもなかったのにやたらと読みにくいコントローラのアクションに振り回されていました。

せっかくなので、今回の事だけではないんですが、これまでに思った事をまとめておこうと思います。

長くなったので、アクションのコードがやたらと縦に長い時の場合だけで4000字を超えたので、 先に投下。。

残りは、アクションのコードがやたらと横に長いとき、パラメータの受け渡され方がよくわからないとき、です。

内容

  • アクションのコードがやたらと縦に長いとき
    • 複数のアクションが1つのアクションに詰め込まれていないか
    • 共通して繰り返す処理や一定の法則の分岐をそのままアクションに書いていないか
    • 表示のためのビジネスロジックもモデルに置こう

アクションのコードがやたらと縦に長いとき

コントローラのアクションを読んでいるとげっそり来るパターンで、まず最もよくあるのが縦に長いパターンではないでしょうか*1

そういう、ノートパソコンのディスプレイではアクションの全貌を把握できないくらい行数の多いアクションが、どうしてそうなるのかとどうしたら少しはマシになるのか、ということについて考えてみます。

複数のアクションが1つのアクションに詰め込まれていないか

縦に長いアクションの中に request.get? あるいは request.post? が含まれていませんか。

あるいは、複数のアクションからリダイレクトされており、かつそれぞれのリダイレクト元によってrenderされるテンプレートが変わっていたりしませんか。

こういう場合、実質は複数のアクションで書くべき処理を1つのアクションに押し込めているため、そのアクションのコードが長くなっている可能性があります。

考えられる可能性としては、

  • エンドポイントのURLが変わるのがいや
  • フレームワークにおけるテンプレートの設定の仕方やリダイレクトの仕組みがよく分からない
  • RESTがよく分からない

等の理由があると考えられます*2

実際は、そのフォームがどこのURIにPOSTしているかを気にしているユーザはほとんどいませんので、URIを揃えたいから1つのアクションに押し込める必要は薄いです。

モバイル向けや外部向けのAPIにおいてはむしろ、そういったURIの設定の仕方はあまりされないのではないでしょうか。 フォームからの投稿リクエストも同じように考えていただけると幸いです。

フレームワークにおけるテンプレートの設定の仕方やリダイレクトの仕組みがよく分からないので、同じテンプレートをrenderするアクションは全部1つのアクションに押し込めてしまう、というような感じのコードを住所入力、決済入力...といった入力画面が続く画面遷移の部分で見た事があります。

ただでさえ、複雑な遷移なので、テンプレートの設定の仕方やリダイレクトの仕組みまで手が回らないという感じがしますが、POSTとGETのリクエストに対するアクションがごちゃまぜに書いてある方が読んでいる方にはややこしいです。

特にPOSTのリクエストをGETのアクションでも受けてしまう、というケースを何度か見た事がありますが、 Railsの場合は下記のようにアクション内でrenderするテンプレートが変更できますので、POSTのリクエストを受けるアクションをGETのアクションと混ぜる前にご確認ください。

# http://railstutorial.jp/book/ruby-on-rails-tutorial?version=4.0#sec-signup_failure より
class UsersController < ApplicationController
  .
  .
  .
  def create
    @user = User.new(params[:user])    # 実装は終わっていないことに注意!
    if @user.save
      # 保存の成功をここで扱う。
    else
      render 'new'
    end
  end
end

個人的には、webアプリケーション用のサーバサイドMVCフレームワークは比較的基本的な機能についてはよく似ているなぁという感触を持っているので*3フレームワークにおけるテンプレートの設定の仕方やリダイレクトの仕組みの方を覚えた方が潰しがきくと思います。

ので、対処法としては可能な限り、違うアクションは違うアクションとして分けたほうがよいと思います。

無理な場合は、リクエストメソッド毎に縦にメソッドが追えるように、物理的にアクションを分けておくのでもいいと思います。

分岐部分が極端に少なく、メソッド自体も単純で10行程度ならそのままでもいいと思いますが、30行を超えてきたら少し身構えた方が良さそうです。

共通して繰り返す処理や一定の法則の分岐をそのままアクションに書いていないか

一連のアクションで、アクションの最後にテンプレートをデバイスによって細かく分けたいので、テンプレートの指定の処理が長くなってしまう、という場合があります。

その結果、アクションの中で全体としてどのようなメソッドが呼ばれているか、よりテンプレートの指定の処理が目立って読み難くなってしまう事があります。

respond_to do |format|
  format.html { .. }
  format.js   { .. }
  format.xml  { .. }
end

こういった箇所が複数続いて、それが一定回数登場するなら、コントローラ全体で使い回せるようにメソッドで括り出せるなら括り出してしまいましょう。

また、外部からのAPIであるとかいう場合でなければ、こういったことは少しお考えになった方がいいです。

コントローラはViewとは独立しているべきですが、JavaScriptからリクエストを叩かれる時の処理、スマートフォンアプリからリクエストを投げられた時の処理、ブラウザからリクエストを投げられた時の処理が最近では異なる場合もあるので...*4

次に、一定の法則で分岐している部分について。

たとえばそうですね、決済方法によって、表示に必要なインスタンス変数が異なる場合や、次のアクションに渡すパラメータが異なる場合などでしょうか。

この部分の分岐処理も、気がついたらかなり縦に長く伸びてしまい、全体として何をしているのかよく分からなくなってしまいます。

# 各種決済サービスに登録するために必要なパラメータをフォームから受け取る
case params[:payment_gataway_id].to_i
when 1 # A社の決済サービスの場合
  @var = params[:var_1]
  ...
  ...
  ...
when 2 # B社の決済サービスの場合
  @var = params[:var_2]
  ...
  ...
  ...
when 3 # C社の決済サービスの場合
  @var = params[:var_3]
  ...
  ...
  ...
end

こういった分岐のさらに厄介な点はおそらく関連するアクションすべてに繰り返し登場することです。

パラメータの受け渡しが複雑でないならば*5、これを扱うためのモデルを用意して、そのモデルに担当させるか、この分岐の部分にそれらしい名前を付けて、コントローラのprivateメソッドに押し込んでしまいましょう。

def some_action
  # 各種決済サービスに登録するために必要なパラメータをフォームから受け取る
  set_params_for_registeration_to_payment_gateway
  ...
  ...
end

private
  # どちらかといえば、このメソッドでインスタンス変数を代入するより、
  # 値を代入したオブジェクトをアクションに返してアクションの中でインスタンス変数の代入を行う方が
  # 読む側に分かりやすいかも
  def set_params_for_registeration_to_payment_gateway
    case params[:payment_gataway_id].to_i
    when 1 # A社の決済サービスの場合
      payment_params[:var_1] = params[:var_1]
      ...
      ...
      ...
    when 2 # B社の決済サービスの場合
      payment_params[:var_2] = params[:var_2]
      ...
      ...
      ...
    when 3 # C社の決済サービスの場合
      payment_params[:var_3] = params[:var_3]
      ...
      ...
      ...
    end
  end

メリットとしては分岐処理を分岐用のメソッドに押し込めてしまう事で、 分岐にさらに選択肢を足したい場合、メソッドの部分だけ見に行けば良い事がはっきりしやすい事と、 詳細は隠れてしまいますが、適切に命名orコメントが出来れば全体として流れが分かりやすくなることです*6

おそらく、アクションが普通に書いたらものすごく長くなってしまった場合は、ざっと流れを把握して、修正したい部分のコードを細かく読んで行くと思うのですが、でしたらソースコードも元々そのように整理しておくと楽なのではないでしょうか。

また、フォームから投稿されているなどして、パラメータが綺麗にネストされている場合には、メソッドで分岐させてパラメータ1つずつに対して頑張らずとも1行で受け取ることができる場合があるのでご確認ください。

  def set_params_for_registeration_to_payment_gateway
    case params[:payment_gataway_id].to_i
    when 1 # A社の決済サービスの場合
      payment_params[:var_1] = params[:payment_form][:var_1]
      ...
      ...
      ...
    when 2 # B社の決済サービスの場合
      payment_params[:var_2] = params[:payment_form][:var_2]
      ...
      ...
      ...
    when 3 # C社の決済サービスの場合
      payment_params[:var_3] = params[:payment_form][:var_3]
      ...
      ...
      ...
    end
  end

# 上記は下記1行で代替出来る場合があって、その場合はメソッドを設ける必要は無さそう
payment_params = params[:payment_form]

表示のためのビジネスロジックもモデルに置こう

もう1つ考えられるのか、Aという値がパラメータに飛んできていたら、1000を返して、Bという値がパラメータに飛んできていたら、2000を返して...というパラメータの値に応じて、返す値を決める処理をコントローラに書いているパターンです。

一番単純な例としては下記になります。

@card_name = case params[:payment_type_id]
when 1 then 'カードA'
when 2 then 'カードB'
when 3 then 'カードC'
when 4 then 'カードD'
when 5 then 'カードE'
..
end

こういう処理は、表示用の値を出しているだけなのですが、表示用の値を代入する部分と受け取った値を判定して表示用の値を返す部分に分けられます。

受け取った値を判定して表示用の値を返す部分はビジネスロジックなので、関連するモデルに置いて、コントローラからはそのメソッドにパラメータを渡す、という風にしたほうが良いと思います。

というよりは、複数の箇所に書いてばらついてしまったら困るので、モデルかそれ用のymlに置いてください、お願いします...。

計算したり、データベースを叩いたりといった処理は割とモデルにおいてあっても、こういった表記の部分はしばしばコントローラに長々とベタで書いてあったりすることがあって、しかも微妙に表記がずれてて...という経験が何度かあってそのたびにあたふたしています。

*1:それに見合う縦長のデュアルディスプレイがあれば平気とかそういう話はいいです

*2:他に見つかったら追記します

*3:http://woshidan.hatenablog.com/entry/2015/01/03/090800

*4:理論的には、コントローラはViewをは関係なく存在するべきです。が、jsとhtmlで返したい内容の意味は、つまり、異なる拡張子をつけてリクエストしてくるクライアントが求めているアクションは本当に一緒でしょうか。アクションが一緒になるように設計できている場合に一緒であると判断して進めるならそっちの方がいいのですが。明らかに違うのに一部だけかすってるから、と適当なアクションを叩くようにして行くと修正するときに影響箇所の特定が困難にあってしまう気がするので

*5:パラメータの受け渡しが複雑なら、結局コントローラで受け渡しの行が発生する事になって処理の流れは単純化できたとしてもコントローラは縦に短くなりません...。

*6:もしかしたら新しいモデルを導入してモデルのメソッドとしてうまく処理できるかもしれず、それが分かった時のリファクタもしやすい印象です