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

こちらの記事*1の続きです。

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

内容

アクションのコードがやたらと横に長いとき
- 条件分岐がひたすらネストしている
- ネストしているリクエストパラメータやsessionの値の受け渡しを全部開いて書いてる
  - ネストしてる部分の呼び出しをメソッドに押し込んでみる
  - 複数箇所で共通して使うなら基底コントローラやそのヘルパに持ってく

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

コントローラのコードが読みにくいときには、どれだけスクロールさせるんですか、と言いたくなるところまでいかないにしろ、目の上下動だけではメソッドがよくわからないことになっていることもありますね*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