現場から学ぶ、RailsのControllerアンチパターン

## 0. はじめに
　最近、新規のRailsプロジェクトで一から実装されていて、Controllerの実装の中でこれは分かりにくいなと思ったコードがあり、Controllerのアンチパターンと言えるのではないかと感じたので、改めてここに記述したいと思います。
　RailsのControllerのアンチパターンは検索するといくつか出てきますが、今回はそれとは関係なく、私が出会った実体験を元に紹介したいと思います。
　今回のControllerは、フォームでユーザーを作成したり、申込みを行ったりするControllerに注目しています。APIとしてクライアントサイドから呼びされるControllerではまた状況が異なると思いますので、ご留意ください。

## 1. POSTしたあとにrenderしている
　私は、例えば次のようなコードをみたことがあります。このコードでは、userの作成に成功した後に`show`をrenderしています。

```ruby
def create
  user = User.new(user_params)
  if user.save
    render 'show'
  else
    render 'new'
  end
end
```

### 1-1. アンチパターンである理由
フォームにおいて、POSTしたあとはredirectするのがWeb開発においては定石といえます。Rails Tutorialには、文章上で明記されていないように思いますが、Djangoのチュートリアルには以下の文の記載があります。[^1] 

[^1]: https://docs.djangoproject.com/ja/2.1/intro/tutorial04/

> POST データが成功した後は常に HttpResponseRedirect を返す必要があります。これは Django 固有のものではなく、 Web 開発における良いプラクティスです。

　Djangoにおいて`HttpResponseRedirect`を返すとはRailsでいえば`redirect_to`を返すということに相当します。
　POSTが成功した後にredirectする理由の一つは二重投稿防止です。これは、一般にPRGパターンと呼ばれるもので、二重投稿防止については「[さいきょうの二重サブミット対策](https://qiita.com/syobochim/items/120109315f671918f28d)」が詳しいのでぜひご覧ください。
<img src="https://qiita-image-store.s3.amazonaws.com/0/121304/e76e2f82-b3e4-fc73-3dd6-69bf25e780d8.png" width=350>
図1. リロードすると二重投稿されることがある。Chromeではこのような確認が出ることもある。


### 1-2. アンチパターンを回避する方法
　PRGパターンに従い、userの保存に成功した場合は適切なページへリダイレクトします。

```ruby
def create
  user = User.new(user_params)
  if user.save
    redirect_to user_url(user)
  else
    render 'new' # こちらについては後述
  end
end
```


## 2. 同一アクション内で条件によってrender/redirectの挙動が異なる
例えば次の例[^1]では、saveに成功した時は`root_url`にredirectし、失敗した時はrenderする挙動になっています。この例は、私もRailsを一番最初に勉強する際に学んだRails Tutorialの`UsersController`から拝借していますが、今思えばこれはアンチパターンだと思っています。

[^1]: https://github.com/yasslab/sample_apps/blob/master/5_1_2/ch14/app/controllers/users_controller.rb#L28

```ruby
def create
  @user = User.new(user_params)
  if @user.save # => Validation
    @user.send_activation_email
    flash[:info] = "Please check your email to activate your account."
    redirect_to root_url
  else
    # Failure
    render 'new'      
  end
end
```
### 2-1. アンチパターンである理由
　今回のようなバリデーションに成功/失敗であれば、挙動がわかりやすいですが、実運用されているコードでは、こんなにシンプルなロジックであることは稀です。そのこと自体、FatControllerとして避けるべきですが、実運用では必ずしも安全にリファクタリングができる時間が確保されるとは限りません。したがって、Fat化したControllerにおいて、状況によってrenderとredirectの挙動が変わるのは、（１００歩譲って最初の実装者にとっては分かりやすくても）後から参画した開発者の頭を悩ませることになります。
　こうしたバリデーションに失敗した後の挙動の期待としては、入力フォームに戻ってきてユーザーに入力を再度行わせ、エラーを解消してもらうことです。Rails Tutorialのここの実装の場合、URLが`/signup`から`/users`に変わっており、まずこの時点で違和感がありますが、これだけでは機能上の問題とは言えません。しかし、この実装は機能としても問題であると言えます。
　ユーザーはこのあとどのような操作をするかを考えた時に、もちろん素直に入力し直してユーザー作成を行ってくれることもありますが、ブラウザのリロードを行なうこともあると想定されます。Rails Tutorialでは`/users`でリロードを行なうと`index`アクションが呼ばれ、`before_action`の`logged_in_user`が実行され、`/login`にリダイレクトされます。これは申込みサイトにおいて、大変困った挙動といえます。フォームの数がたくさんある申込み機能において、一生懸命入力した内容がなんとなく`F5`ボタンを押しただけで消去されてしまうのです。
![image.png](https://qiita-image-store.s3.amazonaws.com/0/121304/bf02e152-bc7b-182b-237b-443db52eaa7f.png)
図2. エラー時にはURLが変わってしまう

### 2-2. アンチパターンを回避する方法
　いくつか追加で実装しなければいけません。
　まず一つ目はエラーメッセージの表示です。Railsであれば`flash`を活用するのがよいです。次のように`flash`に`@user.errors.full_messages`を入れておき、view側で`flash`から取り出します。

```ruby
def create
  @user = User.new(user_params)
  if @user.save # => Validation
    # Sucess
    @user.send_activation_email
    flash[:info] = "Please check your email to activate your account."
    redirect_to root_url
  else
    # Failure
    flash[:danger] = @user.errors.full_messages
    redirect_to signup_url
  end
end
```
```erb
<% flash.each do |message_type, messages| %>
  <% messages.each do |message| %>
    <div class="alert alert-<%= message_type %>"><%= message %></div>
  <% end %>
<% end %>
```
　最近では、サーバーサイドのバリデーションとフロントエンドを連携し、サーバーサイドでエラーになった箇所は赤く目立たせるなどの処理を行うことがあります。そういう場合は、`flash`に入れる形式をHashにし、key, value形式で持たせると良いです。view側ではkeyでエラーとなった属性値を取得できるのでフロント側と連動する実装を行います。

```ruby
flash[:danger] = @user.errors.keys.map { |key| [key, @user.errors.full_messages_for(key)] }.to_h
# => {:email=>["Email can't be blank", "Email is invalid"], :error=>[]}
```

　二つ目は、POSTした値の保持です。redirectしてしまうとPOSTの値を保持できなくなります。これではユーザビリティを著しく損なってしまいます。一番簡単なのは`session`に保持しておく方法です。`create`で失敗した際にはsessionに保存し、redirect先の`new`では`session`からUserインスタンスを復元するようにします。

```ruby
  def create
    @user = User.new(user_params)
    if @user.save # => Validation
      # Sucess
      @user.send_activation_email
      session[:user] = nil # *1 成功した際は速やかに削除する
      flash[:info] = "Please check your email to activate your account."
      redirect_to root_url
    else
      # Failure
      session[:user] = @user.attributes.slice(*user_params.keys) # *2 フォームで渡された値のみ保存
      # *3 session[:user] = user_params としないこと！
      flash[:danger] = @user.errors.full_messages
      redirect_to signup_url
    end
  end
```
```ruby
def new
  @user = User.new(session[:user] || {}) # sessionからUserインスタンスを生成
end
```
この実装の際のポイントを上記コードにコメントしましたが、もう少し補足を行わせてください。
*1: `session`に長らく情報を保持しておくと、色々とsessionの値のハンドリングが面倒なことがあるので、使用し終えたら極力速やかに削除しておいたほうが良いです。
*2: `session`に保存する情報ですが、`@user.attributes`としてしまうと、`id`や`created_at`など必要でない値まで保存してしまうので、フォームで渡された値のみを保持するようします。場合によってはパスワードなどはもう一度入力してもらうため、sessionに保存しないようにする必要があるかもしれません。
*3: 保存するときに`user_params`をそのまま保存しないようにします。これは、Railsのバージョンによって`ActionController::Parameters`の仕様が変わる可能性があるからです。現に私はRails4から5にあげる過程でこの問題に直面しました。できる限り、純粋なHash型で保存しておくべきだと思います。

### 2-3. sessionに保存する際の注意点
　サーバーサイドの`session`はブラウザのどのタブで操作が行われたかを判別することができません。というのも、`session`はCookieの`_session_id`などと紐づけられて管理されますが、Cookieはタブごとに別れておらず、ブラウザを通じて一つです。これはブラウザの仕様なのでサーバーサイドでどうにかできる問題ではありません。
　別タブでも同じsession情報として保存されてしまうということは複数タブで同じフォームを開きサブミットすると、後から開いたフォームの値がsessionに保存され、先に開いたフォームでサブミットしても後のフォームの値が送信されてしまうという事象を引き起こします。俗に複数タブ問題と呼ばれる事象です。これは、システムによってはクリティカルな問題となり、できればサーバーサイドの`session`で保存すべきではありません。[^2]
　ここで、何度か「サーバーサイドの」という言葉を使用しました。つまり、サーバーサイドでなければ回避する方法があります。現在のブラウザには、`sessionStorage`という機能[^3]（WebStorageの一つ）が備えられておりJavaScriptなどを用いて`sessionStorage`に保存すれば別タブは別の`sessionStorage`に保存されるので複数タブ問題が回避できます。例えば、Vue.jsでは`vuejs-storage`[^4]などの便利なパッケージが開発されており、簡単に値を`sessionStorage`に格納することができます。

[^2]: 今回の場合、sessionに格納された情報はエラー時のみ使用しているので複数タブ問題は発生しない。createアクション内では、sessionからではなくフォームに入力された値からUserインスタンスを保存していることに注意。
[^3]: https://developer.mozilla.org/ja/docs/Web/API/Window/sessionStorage
[^4]: https://www.npmjs.com/package/vuejs-storage

## 3. アクション、before_action以外でrenderやredirectを行なう
### 3-1. アンチパターンである理由
　いうまでもなく、Controllerでは、アクションメソッド以外にも通常のメソッドも利用できます。例えば、メールアドレスにexampleという文字列が含まれている場合は`root_url`にredirectする処理を行いたいとします。次のコードは正しく動くでしょうか。

```ruby:Wrong
def create
  @user = User.new(user_params)
  redirect_if_example_email
  
  if @user.save # => Validation
    # Success
    @user.send_activation_email
    session[:user] = nil
    flash[:info] = "Please check your email to activate your account."
    redirect_to user_url(@user)
  else
    # Failure
    session[:user] = @user.attributes.slice(*user_params.keys)
    flash[:danger] = @user.errors.full_messages
    redirect_to signup_url
  end
end

def redirect_if_example_email
  redirect_to root_url if @user.email.include?('example')
end
```
　これは正しく動きません。`redirect_if_example_email`の中で`redirect_to`を使った後、さらに`if`の中でも`redirect_to`を使っているので、`AbstractController::DoubleRenderError`が発生してしまいます。それを回避するには、

```ruby
redirect_if_example_email and return
```
のように`and return`を追加してあげる[^5]か、

```ruby
redirect_if_example_email
return if performed?
```
のように`perfomed?`メソッドで判断してあげる必要があります。`perfomed?`メソッドは、すでにそのアクション内で`render`か`redirect`がされたかを確認するメソッドです[^6]が、一つのアクションの中で`render`や`redirect`が色々な箇所で行われると、そのアクションのどこでどう`render`や`redirect`をされているか分からなくなってしまいます。今回の例ではロジックが比較的シンプルなので大きな混乱はないと思います。しかし、これが肥大化するとコードの見通しが非常に悪くなりメンテナンス性を低下させます。私は、アクションの中では`render`や`redirect`は局所化することが望ましいと考えています。

### 3-2. アンチパターンを回避する方法
　今回のケースでは、`email`に`example`という文字列が含まれているかがredirectするかしないかの判断条件になっていました。これは、本来ならば、モデルのバリデーションで行うべき実装です。
　次は、`email`に`example`という文字列が含まれているかをのチェックをモデルに移した例です。

```ruby:user.rb
class User < ApplicationRecord
  VALID_EMAIL_REGEX = /\A[\w+\-.]+@[a-z\d\-.]+\.[a-z]+\z/i
  validates :email, presence: true, length: { maximum: 255 },
                    format: { with: VALID_EMAIL_REGEX },
                    uniqueness: { case_sensitive: false }
  validate :validate_email # <= 追加
  # メソッド化したくない場合は、ラムダで渡しても良い
  # validate -> {
  #   errors.add(:email, "should not contain 'example'") if email.include?('example')
  # }
  
  private

  def validate_email
    errors.add(:email, "should not contain 'example'") if email.include?('example')
  end
end
```
　こうすることで、Controller内に必要以上にロジックを増やすことなくアクションをシンプルにできます。Controller内で異様な量のバリデーションが記載されているプロダクションをいくつか見てきました。これではテストも書きにくいですが、ロジックをモデルに移すことでテストも書きやすくなります。
　ActiveRecordのモデルではない場合は、FormObjectを利用することを検討しても良いでしょう。

[^5]: `redirect`の返り値が`"<html><body>You are being <a href=\"http://localhost:3000/\">redirected</a>.</body></html>"`のような値になっており、これは真偽値では`true`を意味することを利用している。
[^6]: https://apidock.com/rails/ActionController/Metal/performed%3f

## 4. もう少し複雑な例
　これまで紹介した例は比較的シンプルでしたが、私が実際に出会った例を紹介します。少し複雑になるので、興味のある方のみお読みください。

　以下の例では、`update`アクション内で二つのことを行なっています。この呼び出し元のビューには送信ボタンが二つあり、どちらの送信ボタンが押されたかによって挙動が変わります。nameに`commit_api`と指定されている送信ボタンを押した場合は、APIを呼ぶ。APIにはAPI1とAPI2の二つがあり、API2を呼ぶ前にAPI1を呼ばなければいけないようです。

```ruby
def update
  # 送信ボタンが二つあり、送信ボタンのnameによって挙動を変える
  return send_to_api_2 if params.key?(:commit_api)

  update_order
end
```
`send_to_api_2`メソッドと`update_order`はControllerのprivateメソッドとして定義されてあります。（実際には、同様の処理を別のControllerでも用いるので、`concern`にありました。）
 `update_order`メソッドでは、フォームから渡されたパラメータを元に`@order`を更新する処理を行います。`save`に失敗した時に`common_show`というメソッドが呼び出されていますが、のちほど出てきます。このメソッド名を見ると`コードのにおい`というのを感じます。

```ruby
private

def update_order
  @order.assign_attributes(order_params)
  @order.status = :confirmed unless @order.status_changed?
  if @order.save
    redirect_to request.referer
  else
    common_show
    @error_messages = ['エラーがあります。']
    render :show
  end
end
```
　次は、`update`アクションから呼び出される`send_to_api_2`というメソッドです。　
　まず［1］では`@order`のステータスが`confirm`ではない場合は`render_error`メソッドを呼び出しています。呼び出し元の`update`アクションはPOSTであり、送信する前は`/orders/1`などというURLですが`/update`というURLになります。エラーになった後ブラウザをリロードすると、POSTが実行され、何度もAPI連携されてしまいます。POSTしたあとにredirectしないことが無駄なAPI連携を誘発させています。
　このメソッドの中では、API1とAPI2を読んでいます。API1は、`send_to_api_1`というメソッドを呼び出すことでAPI1がコールされています。`send_to_api_1`の中でも`render`が実行されることがあり、［2］で`performed?`を用いることで`AbstractController::DoubleRenderError`を回避しています。同じメソッドなのに、APIコールに失敗した時と成功した時で返り値の型が異なるようになっています。Rubyは返り値の型が条件により異なるような実装も可能ですが、**どんな条件でも極力同じ型で返す**のが良いでしょう。

```ruby
def send_to_api_2
  return render_error('ステータスが確認済みではありません') unless @order.status_confirmed? # ・・・[1]

  send_to_api_1 # API2を呼ぶ前に、API1を呼ぶ 
  return if performed? # ・・・[2]

  # Api2クラスはAPI2をコールするために実装したクラス
  response = Api2.post("/api2_url", params: { order: @order })
  unless response.success? 
    @error_messages = response.errors # 詳細な実装は、GET先のインターフェスによる
    raise # ・・・[3]
  end

  @order.status = :linked
  @order.save!

  @notice = 'API2に送信しました。'
  redirect_to request.referer
rescue StandardError => e # ・・・[4]
  @notice = nil
  @error_messages = ['API2に失敗しました。'].concat(@error_messages || [])
  render_show # ・・・[５]
end
```
　そして、API1を呼び出すメソッドである`send_to_api_1`です。　`send_to_api_1`の中では、呼び出しに失敗した場合、意図的にエラーを起こし（［6］）、`rescue`のなかでrenderしています（［7］）。`send_to_api_1`はAPI1の呼び出しに成功した場合は`render`も`redirect`も行いませんが、エラーとなった場合にのみ`render`するといういびつなメソッドになってしまっています。

```ruby
def send_to_api_1
  # Api1クラスはAPI1をコールするために実装したクラス
  response = Api1.get("/ap1_url", params: { order: @order })
  unless response.success?
    @error_messages = response.errors # 詳細な実装は、GET先のインターフェスによる
    raise  # ・・・[６]
  end

  @notice = 'API1に送信しました。'
rescue StandardError => e
  @notice = nil 
  @error_messages = ['API1に失敗しました。'].concat(@error_messages || [])
  render_show # ・・・[７]
end
```
　残りは、これまでの関数の中で呼び出されたメソッドが実装されています。`render_show`や`common_show`といった関数は[2.](#2-同一アクション内で条件によってrenderredirectの挙動が異なる)で紹介したように、成功した時も失敗した時もPOSTのときはredirectするようにすれば、必要のないメソッドです。うまく共通化されたように感じるかもしれませんが、**共通化の前に考えることは共通化せずに実装できないか**ということです。今回の場合は、`send_to_api_1`や`send_to_api_2`のエラー時にrenderしてしまっているのですが、エラー時もredirectするようにすれば、この処理はすべて`show`に集約することができます。

```ruby
def render_show
  common_show
  render :show
end

def render_error
  @error_messages ||= []
  @error_messages << message
  render_show
end

def common_show
  # 省略
  # インスタンス変数などをセット
end
```
　今回のケースの場合、send_to_api_1やsend_to_api_2の返り値はシンプルにAPIに失敗したか成功したかを`true`, `false`で返し、エラーメッセージは返り値の二つ目で返すようにすると見通しが良くなると思いました。そして、redirect先は、すべて詳細ページに統一します。エラーメッセージはインスタンス変数ではなく、`flash`で渡すようにします。
　以下に、上記のコードよりも見通しを良くしたと感じているコードを次に示します。この例では、アクション以外のメソッドの返り値を配列にし、一つ目に成功したかどうか、二つ目にメッセージを入れるようにしています。より汎用的に用いられるメソッドで分かりやすくするには、Hash型にして`{ success: false, error_code: 'ERR001', messages: ['メッセージ'] }`などとしても分かりやすいでしょう。

```ruby
def update
  success, messages = if params.key?(:commit) 
                        send_to_api_2
                      else
                        update_order
                      end
  flash[success ? :notice : :error] = messages

  redirect_to show_url(@order),
end

def update_order
  @order.assign_attributes(order_params)
  @order.status = :confirmed unless @order.status_changed?
  if @order.save
    [true, ['保存しました']]
  else
    [false, ['エラーがあります。各項目を確認してください。'] + @order.errors.full_messages]
  end
end

def send_to_api_2
  # この条件をOrderモデル内でcontextつきのvalidationにしても良い。
  return [false, ['ステータスが確認済みではありません']] unless @order.status_confirmed?

  success, messages = send_to_api_1 # API2を呼ぶ前に、API1を呼ぶ
  return [false, ['API1に失敗しました'] + messages] unless success

  response = Api2.post("/api2_url", params: { order: @order })
  if response.success?
    @order.status = :linked
    @order.save!
    [true, ['API2に送信しました']]
  else
    [false, ['API2に失敗しました'] + response.errors]
  end
rescue StandardError => e
  [false, ['API連携に失敗しました']]
end

def send_to_api_1
  response = Api1.get("/ap1_url", params: { order: @order })
  if response.success?
    [true, ['API1に送信しました']]
  else
    [false, response.errors]
  end
end
```

　現場からは以上です。
　3年間のRailsの現場での経験を踏まえて、改めてRails　Tutorialを見てみると、必ずしもベストプラクティスな実装にはなっていない箇所があります。それは、Rails TutorialがRails初心者をターゲットにしており、細かな実装にこだわると本筋から離れてしまい、焦点がぶれたチュートリアルになってしまうことを懸念したものだともいえます。Rails TutorialでRailsの概要を理解した上で、開発現場でどういう実装がベストなのかを議論し、各々のエンジニアの経験も踏まえた上で実装を決めて行くのが良いでしょう。
　本記事は以上ですが、あくまで自分の実感としてすので、「いや違うよ」、「こうしたほうがいいのではないか」という意見もあると思います。私自身、模索しながら実装ですので、間違っている点も多々あると思います。ご指摘いただければ幸いです。また、議論のきっかけとなり、より良い実装が浸透していけば嬉しいです。