Railsのコントローラのアクションがやたらと横に長くて読みにくいときに確認したいこと
こちらの記事*1の続きです。
長くなったので、アクションのコードがやたらと横に長いとき、について書きます。 残りは、パラメータの受け渡され方がよくわからないとき だー。
内容
- アクションのコードがやたらと横に長いとき
アクションのコードがやたらと横に長いとき
コントローラのコードが読みにくいときには、どれだけスクロールさせるんですか、と言いたくなるところまでいかないにしろ、 目の上下動だけではメソッドがよくわからないことになっていることもありますね*2。
そういう場合について、少し考えたことをメモしておきます。
条件分岐がひたすらネストしている
たまに、以下のようなひたすらネストしているようなコードに合う事があります。
if condition_1 if condition_2 some_command # condition_3の判定に必要 if conditoin_3 some_command # conditon_4の判定に必要 if condition_4 if conditon_5 @some_paramter = some_parameter else @some_paramter = some_parameter * other_parameter end end else raise error_a end end else raise error_b end
これも横に長くて困るコードではあるのですが、横に長くなくても読み難いのでやめてください。
if else end は以外に対応する else のブロックの行が何故そこを通過しているのか、ぱっと見わからなくなりがちです。
raise error_a が何の条件のエラーか、折りたたみを使わずに常にすぐ分かる人はそれでいいのでしょうが、
そもそもそういう風に分岐を書く必要も無いです。
処理を進めない場合はすぐに返した方がいいと思います。
raise error_b unless condition_1 # 以下は conditoin_1 == true でなければ例外を発生させて行わない return unless condition_2 # conditon_2 == true でなければ処理を行わないので返していい some_command raise error_a unless conditoin_3 # 以下は conditoin_3 == true でなければ例外を発生させて行わない some_command return unless condition_4 # conditon_4 == true でなければ処理を行わないので返していい # 比較的単純な代入なら三項演算子を使った方がすっきりする @some_paramter = conditon_5 ? some_parameter : some_parameter * other_parameter
ネストしているリクエストパラメータやsessionの値の受け渡しを全部開いて書いてる
また、横に長くなるパターンとしてフォームの入れ子のようなことをしたりして、 パラメータやsessionのハッシュがネストしすぎている、という場合も考えられます。
def some_action @user_name = params[:register_form][:personal_info][:user][:name] @user_age = params[:register_form][:personal_info][:user][:age] @user_var1 = params[:register_form][:personal_info][:user][:var1] end
上の例は極端ですし、この程度ならまだ平気なのですが、これが、条件分岐のネストの中に入ってきて、 sessionのネストとリクエストパラメータのネストが並ぶと辛いものがあります。
def some_action if condition_1 session[:temporary][:user][:name] = params[:register_form][:personal_info][:user][:name] session[:temporary][:user][:age] = params[:register_form][:personal_info][:user][:age] session[:temporary][:user][:var1] = params[:register_form][:personal_info][:user][:var1] end end
例は適当なんでこういう例は実際にないんですが。
あまりにも長いパラメータが複数のアクションで頻出する場合は、そのパラメータのネストしている部分をメソッドで呼び出せるようにしておくと読みやすくなるし、タイポも減るかも。
def some_action if condition_1 temp_user_session[:name] = register_user_params[:name] temp_user_session[:age] = register_user_params[:age] temp_user_session[:var1] = register_user_params[:var1] end end private def temp_user_session session[:temporary][:user] end def register_user_params params[:register_form][:personal_info][:user] end
こういうパラメータに別名を振るような真似、コードがあっちこっち散るような気がしましたが、適切に名前をつけておけば、そのコントローラ見るとき一回読んだら覚えられる感じだったので、ネストの間違いとかタイポを減らして*3、目線を上下移動だけに集中させられるメリットが勝りそう。
１つのアクションの1カ所にしか出て来ないんだったら、その行が連続して出てくる部分の上でやればよくて、メソッドにするのはやりすぎです。
def some_action temp_user_session = session[:temporary][:user] register_user_params = params[:register_form][:personal_info][:user] if condition_1 temp_user_session[:name] = register_user_params[:name] temp_user_session[:age] = register_user_params[:age] temp_user_session[:var1] = register_user_params[:var1] end end
また、こういった長めのパラメータの呼び出しが複数のコントローラで使用されるようであれば、 ヘルパなり、それらのコントローラの基底コントローラなりを用意して、そこにメソッドを置いておくとよいのかもしれないです。
*2:それに見合う横長のデュアルディスプレイがあれば平気とかそういう話は(ry
*3:ネストの間違いやタイポの間違いは、 undefined method [] for Nil Class の形で出てくるので少し面倒くさい
Railsのコントローラのアクションがやたらと縦に長くて読みにくいときに確認したいこと
今週はなんだか見た目はそんなでもなかったのにやたらと読みにくいコントローラのアクションに振り回されていました。
せっかくなので、今回の事だけではないんですが、これまでに思った事をまとめておこうと思います。
長くなったので、アクションのコードがやたらと縦に長い時の場合だけで4000字を超えたので、 先に投下。。
残りは、アクションのコードがやたらと横に長いとき、パラメータの受け渡され方がよくわからないとき、です。
内容
- アクションのコードがやたらと縦に長いとき
アクションのコードがやたらと縦に長いとき
コントローラのアクションを読んでいるとげっそり来るパターンで、まず最もよくあるのが縦に長いパターンではないでしょうか*1。
そういう、ノートパソコンのディスプレイではアクションの全貌を把握できないくらい行数の多いアクションが、どうしてそうなるのかとどうしたら少しはマシになるのか、ということについて考えてみます。
複数のアクションが1つのアクションに詰め込まれていないか
縦に長いアクションの中に request.get? あるいは request.post? が含まれていませんか。
あるいは、複数のアクションからリダイレクトされており、かつそれぞれのリダイレクト元によってrenderされるテンプレートが変わっていたりしませんか。
こういう場合、実質は複数のアクションで書くべき処理を1つのアクションに押し込めているため、そのアクションのコードが長くなっている可能性があります。
考えられる可能性としては、
- エンドポイントのURLが変わるのがいや
- フレームワークにおけるテンプレートの設定の仕方やリダイレクトの仕組みがよく分からない
- RESTがよく分からない
等の理由があると考えられます*2。
実際は、そのフォームがどこのURIにPOSTしているかを気にしているユーザはほとんどいませんので、URIを揃えたいから１つのアクションに押し込める必要は薄いです。
モバイル向けや外部向けの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]
表示のためのビジネスロジックもモデルに置こう
もう１つ考えられるのか、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:もしかしたら新しいモデルを導入してモデルのメソッドとしてうまく処理できるかもしれず、それが分かった時のリファクタもしやすい印象です
urlヘルパを使ってプロトコルがhttpsのurlを吐き出させたい
なんか調べてたら、Railsのurlヘルパーでhttpsの指定ができる、という話を見かけて、よく分からなかったので調べてみました。
前もって把握していた情報は
- *_urlヘルパは、完全URIを返す
- *_pathヘルパは、パス以下を返す
だったので、これじゃなんともいえないなと手に入った情報をいくつか検証してみました。
検索して出てきた関連しそうな情報と参考元
- config/routes.rbで
resources :xxxs, constrains: { protocol: 'https'}+ *_path_helper - _urlヘルパに
protocol: 'https'オプションを与える - environments/xxx.rbで
config.action_controller.default_url_optionsをいじる
*_pathヘルパー + config/routes.rb
httpsはできませんでした。
検証コード
# config/routes.rb resources :products, constrains: { protocol: 'https'} # view <%= link_to 'products_path', products_path %> <%= link_to 'products_path_only_path_false', products_path, :only_path => false %> #=> http://localhost:3000/products
*_urlヘルパにprotocol: 'https'オプションを与える
httpsはできました。しかし、参考先を見てたらこれどっちかというと、Modelとかにurlヘルパをインクルードして使いたいけれど、デフォルトのコントローラでのhostやschemaの設定が効かない場合に使うみたいです。
検証コード
<%= link_to 'products_url_with_option', products_url(protocol: 'https') %> #=> https://localhost:3000/products
environments/xxx.rbで config.action_controller.default_url_optionsをいじる
これは*_pathはいけませんでしたが、*_urlヘルパはいけました。
検証コード
class ProductsController < ApplicationController # Prevent CSRF attacks by raising an exception. # For APIs, you may want to use :null_session instead. protect_from_forgery with: :exception def default_url_options # テスト用に開発環境だけhttps Rails.env.development? ? { protocol: :https }.merge(super) : super end def index end end
<%= link_to 'products_path', products_path %> #=> http://localhost:3000/products <%= link_to 'products_url', products_url %> #=> https://localhost:3000/products
*_urlヘルパはコントローラのリダイレクトでプロトコルを変わるので完全URIが必要、とかいう場合に使うもので、以外調べている感じ、force_sslあたりのメソッドを使う方が健全な気がします。
あと、*_urlヘルパがコントローラのリダイレクト等でプロトコルを変わったり、外部ページへのアクセスになるので完全URIが必要といった場合に使うもので、*_pathヘルパはviewのサイト内リンクでは一文字でも文字を減らしておきたいよね、みたいな場合に使うものだとスタックオーバーフローで紹介されていたのが、面白かったです。
*path are for views because ahrefs are implicitly linked to the current URL. So it’d be a waste of bytes to repeat it over and over. In the controller, though, *url is needed for redirect_to because the HTTP specification mandates that the Location: header in 3xx redirects is a complete URL.
その他メモ
全般的に、読んでいた傾向として、この手の事をしたいとき、コントローラのデフォルト設定をいじるより、コントローラの方でforce_sslを使って全体のプロトコルを設定して、ビューでは*_pathヘルパーを使うのがよさそうな気がしました。
普通は一部だけhttp/httpsにするから、その部分の判定メソッドの条件分岐がややこしくなってきたら、その段で分岐条件は設定ファイルに置いといた方がよさそう、という感じです。
あと、関係ないですが、スキーマとかプロトコルとかの言い方がごっちゃになっていて謎だったのでもう少し調べたら、スキーマはURIの命名法が何かを示す部分で、そこにプロトコル名(http, https)を入れて、このURIはhttpのプロトコルに合わせて書いていますよ、と言う風に示すものらしいことが分かりすっきりしました。
