場合分けのあるアクションをどのように綺麗に書くか

1

例えばUsersController#showで、表示するユーザに

  • 自分の場合
  • 友達の場合
  • 自分でも友達でもない場合

の3パターンが有り、それぞれでビューテンプレートを切り替えて、かつ表示する項目が増減したりhookのような処理があるとします。

これをそれなりに素直に実装すると下記のようなコード(これは実際の savanna.io のコードです)になりますが、もっときれいに書けるのでは？という気もします。なにか良いアイデアはありますでしょうか？

  def show
    @user = User.find_by_screen_name!(params[:screen_name])
    @boards = @user.created_boards.readable(current_user).opened.page(1).per(5)
    if current_user == @user
      render 'me', layout: 'user'
    elsif current_user.friends_with?(@user)
      Notification::ConditionReader.call(
        reader: current_user, conditions: @user.latest_condition
      )
      render 'friend', layout: 'user'
    else
      @mutual_friends = MutualFriendsFinder.find(@user, current_user, limit: 9)
      render 'not_friend', layout: 'user'
    end
  end
「いいね！」 1
2

layoutについてですが。
もしこのコントローラで共通のレイアウトファイルを使っているなら、現在の user.html.erb を users.html.erb に名称変更して、/app/views/layout に置いておけば render メソッドでレイアウトオプションの定義が不要になりますね。
あるいは今トーラのクラス名の直下あたりに layout: 'user' と書いても良いですが。

このアクションだけで使用する場合は、

def show
  render layout: 'user`
  @user = User.find_by_screen_name!(params[:screen_name])
    :

というふうに書けば後続の render メソッドでも省略できるのかな？

3

layoutは質問の本筋ではないので無視していただければ… :pray:

def show
  render layout: 'user`
  @user = User.find_by_screen_name!(params[:screen_name])

↑こう書いてしまうと、最初のrenderでテンプレートを探しに行ってActionView::MissingTemplateになりますね

「いいね！」 1
4

これを ViewModel と呼ぶのにはちょっと抵抗があるんですが、View を知っているモデルを使って

class UserViewModel
  def initialize(user:)
    @user = user
  end

  def boards
    @user.created_boards.readable(current_user).opened.page(1).per(5)
  end

  def view_name
    # ActiveSupport::CurrentAttributes で登録済みの current_user を参照する
    if CurrentAttributes.user == @user
      'me'
    elsif
      # ...
    end
  end

  def layout
    'user'
  end
end

def show
  @user_vm = UserViewModel.new(
    user: User.find_by_screen_name!(params[:screen_name])
  )
  render @user_vm.view_name, layout: @user_vm.layout
end

のように書くのはどうですか？

ActiveSupport::CurrentAttributes を使えばモデル内でも current_user へアクセス可能ですし(Model 内で参照する ActiveSupport::CurrentAttributes で設定した値を参照するのは異論はあると思うんですが) もしそれが気持ち悪いなら context として渡してあげても良さそうです。

5

いったんViewModel(とService)を使う形で対応しました。もう少しなんとかならないかなという気持ちはあるので(初めて見るひとがギョッとしそう＞＜)、他の案引き続き募集しています！

  def show
    ShowUserService.call(
      controller: self, params: params, current_user: current_user
    )
  end

class ShowUserService < Yuba::Service
  property :current_user
  property :params
  property :controller

  delegate :render, to: :controller
  delegate :myself?, :friends?, to: :view_model

  def call
    if myself?
      render 'me', layout: 'user', view_model: view_model
    elsif friends?
      read_latest_condition
      render 'friend', layout: 'user', view_model: view_model
    else
      render 'not_friend', layout: 'user', view_model: view_model
    end
  end

  private

  def view_model
    @view_model ||= UserShowViewModel.new(
      current_user: current_user, params: params
    )
  end

  def read_latest_condition
    Notification::ConditionReader.call(
      reader: current_user, conditions: view_model.user.latest_condition
    )
  end
end

class UserShowViewModel < Yuba::ViewModel
  property :current_user
  property :params

  def user
    @user ||= User.find_by_screen_name!(params[:screen_name])
  end

  def boards
    @boards ||= user.created_boards.readable(current_user).opened.page(1).per(5)
  end

  def myself?
    current_user == user
  end

  def friends?
    current_user.friends_with?(user)
  end

  # 友達ではないときにだけ使用される
  def mutual_friends
    @mutual_friends ||= MutualFriendsFinder.find(user, current_user, limit: 9)
  end
end
6

まず、最初の例に示されている

それなりに素直に実装すると下記のようなコード

このコードはそれほど筋悪くないと思うんですよね。Controller の責務は守っているので、Controller の責務を守ったまま “行数” が多くなること自体はそれほど実害ないと思うんですよ。例えば、Model の責務が Controller に漏れ出してきた結果 Fat Controller になるのは良くないと思いますが、この例は Controller の責務は守ってますよね。

むしろ Service にするほうが筋悪いと思っていて、render するのは Service の責務じゃないと思うので、 Service にするよりは、show 内の if 文で

  • show_自分の場合
  • show_友達の場合
  • show_自分でも友達でもない場合

みたいな private メソッドに振り分けるだけに留めておくくらいのほうが筋が良い気がします（あくまで Controller の責務なので Controller で処理する）。

んで、そもそも show 内に分岐があるとなぜキモく感じるのかというと、これって Controller がルーティングテーブルの再発明になってしまっている からじゃないかと思うんですよね。Rails のルーターって URI（というか path）と HTTP Method が決まると action が決まりますよね。だけど、これがもし「URI + HTTP Method」に加えて current_user（実体は Session ですね）で action を決定できるようなものであればルーティングテーブル側で解決できて、action 内で分岐しなくて済みますね。

これは僕も詳しくないので REST 厨の人の意見が聞きたいところなんですけど、「URI + HTTP Method」が同一でも Session によって action というか、振る舞いが異なるのって、この例のような GET リクエストの場合において、REST 的に許容できるものなのか、できないものなのか、どちらなんですかね。もし REST 的にそれは許容できるものなのであれば、Rails のルーターがそれに対応していないということですかね。そうなのであれば、ジャストアイデアですけど、例えば Rack middleware を作って action をリライトするとかのほうが正しさ的には正しいのかなあ。けど、この程度のことのためだけにそこまでしたくないですね。

「いいね！」 4