Rails アンチパターン - 太ったセッション(Bloated Sessions)

引き続きRails AntiPatternsという本を読んでいます。

https://www.amazon.co.jp/dp/B004C04QE0/ref=dp-kindle-redirect?_encoding=UTF8&btkr=1

前回は10 Building for Failureの2つめ「Inaudible Failures」についてまとめました。 waterlow2013.hatenablog.com

今回は7 Controllersの3つめ「Bloated Sessions」についてまとめます。

Bloated Sessions

セッションに情報を入れておくとユーザやDBとの通信を節約でき、コードがシンプルになる可能性もある。 しかし、RESTの原則から外れるし、スケールアウトに向かない。またActiveRecordのオブジェクトをセッションやキャッシュに入れておくと、Railsのバージョン変更のときに動かなくなることがある。

Solution: Store References Instead of Instances

  • sessionにはidのみを入れる  残りはDBに入れる。  current_userとか。Railsチュートリアルにもあったと思う。
  • hiddenタグに入れてクライアントに持たせる。  formとかでよくやるやつですね。

まとめ

今回も反論の余地なしの回でした。 キャッシュにARのオブジェクト入れるのはよくやってしまうので気をつけたいです!

Rails アンチパターン - 無視されるエラー(Inaudible Failures)

引き続きRails AntiPatternsという本を読んでいます。

https://www.amazon.co.jp/dp/B004C04QE0/ref=dp-kindle-redirect?_encoding=UTF8&btkr=1

前回は10 Building for Failureの1つめ「Continual Catastrophe」についてまとめました。

waterlow2013.hatenablog.com

今回は10 Building for Failureの2つめ「Inaudible Failures」についてまとめます。

Inaudible Failures

エラー対策はUXに影響を与える。

class Ticket < ApplicationRecord
  def self.bulk_change_owner(user)
    all.each do |ticket|
      ticket.owner = user
      ticket.save
    end
  end
end

たとえばこのようなコードでは、Ticketモデルのバリデーションでエラーになってしまってもスルーしてしまう。 ユーザにとって同じ結果でも削除されていたりされていなかったりする。

Solution: Never Fail Quietly

そんなことはやめようという話。

上のサンプルコードでれば、saveは保存できなかったときに例外をあげるsave!に変える。 またエラーが発生した場合にロールバックするためにトランザクションで囲む。そのほうが一貫性がある。

class Ticket < ApplicationRecord
  def self.bulk_change_owner(user)
    transaction do
      all.each do |ticket|
        ticket.owner = user
        ticket.save!
      end
    end
  end
end

その他以下のようなtipsがある。

  • controllerのrescue_fromで500用エラー画面を出す。
  • rescue nilは使わない。
  • 外部サービスを使ってエラーを蓄積・通知する。

まとめ

この章もその通りだなという感じでした。rescue_from使うとか、外部サービス使うとかは確実にやっていきたいです。
rescueについては前も書いたのですが、StandardErrorではなく限定された例外をrescueするのが良さそう。

Rails アンチパターン - 継続的な大災害(Continual Catastrophe)

引き続きRails AntiPatternsという本を読んでいます。

https://www.amazon.co.jp/dp/B004C04QE0/ref=dp-kindle-redirect?_encoding=UTF8&btkr=1

前回は6 Using Third-Party Codeの1つめ「Recutting the Gem」についてまとめました。

waterlow2013.hatenablog.com

今回は10 Building for Failureの1つめ「Continual Catastrophe」についてまとめていきます。

Continual Catastrophe

サンプルとして以下のようなスクリプトを挙げる。

cd /data/tmp/
rm-rf*

/data/tmp/がない場合に、ディレクトリ移動せず2行目が実行されたらどうなるか。ディレクトリが存在しない場合そこで終了するのが望ましい。(bashには-eや&&演算子がある)

Solution: Fail Fast

以下のようなコードがあるとする。

class Portfolio < ApplicationRecord
  def self.close_all!
    all.each do |portfolio|
      unless portfolio.photos.empty?
        raise "Can't close a portfolio with photos."
      end
      portfolio.close!
    end
  end
end

これは途中でエラーになった場合に中途半端にcloseされてしまうので、初めにチェックするようにする。

class Portfolio < ApplicationRecord
  def self.close_all!
    all.each do |portfolio|
      unless portfolio.photos.empty?
        raise "Can't close a portfolio with photos."
      end
    end

    all.each do |portfolio|
      portfolio.close!
    end
  end
end

更に早くチェックする方法として、UIからclose_allできないようにする,コントローラでリクエストを弾くなどがあげられる。

Fail Fastのメリットは

  • 大災害を防げる
  • はじめにチェックが宣言的に書かれているのでわかりやすい
  • よくわからんところでぬるぽがでるのはうんざりだ!

たとえばARのfind_byとかも、レコードあり前提であればfind_by!で適切なエラーで失敗させるのがわかりやすいかなと思います。(ただし、例外を制御フローに組み込むのは絶対NG)createとかも同様ですね。(controllerのcreate, update内は別)

まとめ

その通りだなと思っていました。(それ以上の感想がない…。)

Rails アンチパターン - gemを使わない(Recutting the Gem)

引き続きRails AntiPatternsという本を読んでいます。

https://www.amazon.co.jp/dp/B004C04QE0/ref=dp-kindle-redirect?_encoding=UTF8&btkr=1

前回は5 Servicesの1つめ「Fire and Forget」についてまとめました。

waterlow2013.hatenablog.com

今回は6 Using Third-Party Codeの1つめ「Recutting the Gem」についてまとめていきます。

Recutting the Gem

いくつかのRailsアプリケーションに関わっていると、共通のパターンまたは機能を実装していることがあります。 その機能を必要としている人は世界にたくさんいるはずです。

「だから、gem作りましょう」という話ではないんですよね笑 Solution見ます。

Solution: Look for a Gem First

  • コードを書く前にgemを探しましょうという話
  • 同じような実装を何回もやっていたとしたら、それはgemが使えるかもしれないというサイン

自分も新しいアプリや新しい機能をRailsで実装するときは、めちゃくちゃググりまくってgemや実装、アーキテクチャ等参考にできないか調べているような感じがします。 探し方にもコツが有るのですが、それは次の章で書いてくれているらしいのでそのときに触れようと思います。

まとめ

「ぼくのかんがえたさいきょうのRails開発」みたいなやつどっかでまとめたくなった!

Rails アンチパターン - ファイア・アンド・フォーゲット(Fire and Forget)

引き続きRails AntiPatternsという本を読んでいます。

https://www.amazon.co.jp/dp/B004C04QE0/ref=dp-kindle-redirect?_encoding=UTF8&btkr=1

前回は4 Controllersの2つめ「Fat Controller」についてまとめました。 waterlow2013.hatenablog.com

今回は5 Servicesの1つめ「Fire and Forget」についてまとめていきます。 ここで言うServicesとは外部サービスのことで、サービスクラスやアプリケーションサービスのことではありません。

Fire and Forget

ファイア・アンド・フォーゲット - Wikipedia

外部サービスを利用するとき、レスポンスの扱いとしては以下のようなものがある。

  • レスポンスをチェックし、エラーごとに適切な処理を行う。
  • 成功、エラーのみチェックする。
  • 何もチェックしない。

今回のアンチパターンは3つ目のケース。コントローラで直接呼んでいるとなるとユーザに500エラーを返すことになってしまう。

Solution: Know What Exceptions to Look Out For

発生するエラーを列挙しておき、適切にハンドリングする。たとえばHTTPクライアントなら以下のようなエラーがある。

HTTP_ERRORS = 
  [Timeout::Error,
   Errno::EINVAL,
   Errno::ECONNRESET,
   EOFError,
   Net::HTTPBadResponse,
   Net::HTTPHeaderSyntaxError,
   Net::ProtocolError]

rescue => eしないのは、Timeout::ErrorRuntimeErrorを継承していないためらしいが、今ならRuntimeErrorを継承しているので、雑にやりたければrescue => eでもいいと思う。

Message in a Bottle

  • config.action_mailer.raise_delivery_errorsの設定に気をつける
  • エラーには大きく分けて2種類ある。クライアント側とサーバ側
  • クライアント側のエラー(メールアドレスがおかしい等)はユーザに即通知する
  • サーバ側のエラーはスタッフに通知する等して、何か対応を入れる。

You Don’t Know What You Don’t Know

  • rescueしすぎない
  • 少なめのrescueから初めて必要なものを追加する
  • エラーログサービス(NewRelic, errbit等)も重要
  • エラーハンドリングの詳しくはchapter10を

まとめ

特に違和感を感じたところはなかったです!外部サービスを叩くものは常にエラーが起こりうるという扱いをするのが自然なんだなと思いました。
あとは早めにchapter10を読みたい。

Rails アンチパターン - ファットコントローラー(Fat Controller)

引き続きRails AntiPatternsという本を読んでいます。

https://www.amazon.co.jp/dp/B004C04QE0/ref=dp-kindle-redirect?_encoding=UTF8&btkr=1

前回は2 Modelsの1つめ「Voyeuristic Models」についてまとめました。

waterlow2013.hatenablog.com

今回は4 Controllersの2つめ「Fat Controller」についてまとめていきます。 Solutionが2つあるのですが、今回は1つ目について書きます。

Fat Controller

コントローラーからビジネスロジックを削除し、それをモデルに適切に配置します。
コールバック、セッター、データベースのデフォルトなど、Active Recordによって提供される機能は、このタスクの重要なツールセットです。

Solution: Use Active Record Callbacks and Setters

サンプルコード

class ArticlesController < ApplicationController
  def create
    @article = Article.new(params[:article])
    @article.reporter_id = current_user.id
    begin
      Article.transaction do
        @version = @article.create_version!(params[:version], current_user)
      end
    rescue ActiveRecord::RecordNotSaved, ActiveRecord::RecordInvalid
      render :action => :new and return false end
    end
    redirect_to article_path(@article)
  end
end

コントローラで手続き型っぽく書いてしまっているというざっくりとした理由の他に、以下のような具体的理由があります。

  • transactionをコントローラで呼んでいる
  • saveが使われていない
    • transactionを貼らないといけない?
  • 例外を制御フローに使っている
    • 例外的状況ではない
    • GOTO文と可読性が変わらない

目標

class ArticlesController < ApplicationController
  def create
    @article = Article.new(article_params)
    if @article.save
      redirect_to article_path(@article)
    else
      render :action => :new
    end
  end
end

ActiveRecordのsaveメソッドの戻り値で判断します。scaffoldで作られたものと同じですね。
create_version!メソッドを消す必要があるので、見ていきましょう。

def create_version!(attributes, user)
  if self.versions.empty?
    return create_first_version!(attributes, user)
  end

  # mark old related links as not current
  if self.current_version.relateds.any?
    self.current_version.relateds.each do |rel|
      rel.update_attribute(:current, false)
    end
  end

  version = self.versions.build(attributes)
  version.article_id = self.id
  version.written_at = Time.now
  version.writer_id = user.id
  version.version = self.current_verison.version + 1
  self.save!
  self.update_attribute(:current_version_id, version.id)
  version
end

def create_first_version!(attributes, user)
  version = self.versions.build(attributes)
  version.written_at = Time.now
  version.writer_id = user.id
  version.state ||= "Raw"
  version.version = 1
  self.save!
  self.update_attribute(:current_version_id, version.id)
  version
end

必要に応じて他のメソッドversionモデルも見ていきます。

消せるものを消す。

  • version.written_at = Time.nowはcreated_atとupdated_atで代用できる。
  • version.state ||= "Raw"はDBのデフォルト値を使う(必要ならgemをつかってもいい)
  • version.article_id = self.idself.versions.buildで勝手に入る。
def create_version!(attributes, user)
  if self.versions.empty?
    return create_first_version!(attributes, user)
  end

  # mark old related links as not current
  if self.current_version.relateds.any?
    self.current_version.relateds.each do |rel|
      rel.update_attribute(:current, false)
    end
  end

  version = self.versions.build(attributes)
  version.writer_id = user.id
  version.version = self.current_verison.version + 1
  self.save!
  self.update_attribute(:current_version_id, version.id)
  version
end

def create_first_version!(attributes, user)
  version = self.versions.build(attributes)
  version.writer_id = user.id
  version.version = 1
  self.save!
  self.update_attribute(:current_version_id, version.id)
  version
end

コールバックをつかう

コールバックにするのは以下の3つ。

  • version.versionの設定
  • current_version.relatedsのupdate
  • self.update_attribute(:current_version_id, version.id)

version.versionの設定

class Version < ApplicationRecord
  before_validation :set_version_number, on: :create
  validates :version, presence: true

  private

  def set_version_number
    self.version =
      (article.current_version ?
        article.current_version.version : 0) + 1
  end
end

self.versions.empty?self.current_versionの存在確認でOK。create_first_version!メソッドが必要なくなるので。以下のようになる。

def create_version!(attributes, user)
  # mark old related links as not current
  if self.current_version && self.current_version.relateds.any?
    self.current_version.relateds.each do |rel|
      rel.update_attribute(:current, false)
    end
  end

  version = self.versions.build(attributes)
  version.writer_id = user.id
  self.save!
  self.update_attribute(:current_version_id, version.id)
  version
end

* current_version.relatedsのupdate

class Version < ApplicationRecord
  before_validation :set_version_number, on: :create
  before_create :mark_related_links_not_current, 
                          if: :current_version
  validates :version, presence: true

  private

  def current_version
    article.current_version
  end

  def set_version_number
    self.version = (current_version ? current_version.version : 0) + 1
  end

  def mark_related_links_not_current
    current_version.relateds.each do |rel|
      rel.update_attribute(:current, false)
    end
  end
end

途中細かなリファクタリングをはさんでいますが、最後の結果だけ書いています。

def create_version!(attributes, user)
  version = self.versions.build(attributes)
  version.writer_id = user.id
  self.save!
  self.update_attribute(:current_version_id, version.id)
  version
end

だいぶスッキリしてきました!

self.update_attribute(:current_version_id, version.id)

class Version < ApplicationRecord
  before_validation :set_version_number, on: :create
  before_create :mark_related_links_not_current, 
                          if: :current_version
  after_create :set_current_version_on_article
  validates :version, presence: true

  private

  def set_current_version_on_article
    article.update_attribute :current_version_id, self.id
  end

  def current_version
    article.current_version
  end

  def set_version_number
    self.version = (current_version ? current_version.version : 0) + 1
  end

  def mark_related_links_not_current
    current_version.relateds.each do |rel|
      rel.update_attribute(:current, false)
    end
  end
end

メインのデータ更新以外は全てコールバックに、という流れですね。 create_version!消すまではもう一息という感じです。

def create_version!(attributes, user)
  version = self.versions.build(attributes)
  version.writer_id = user.id
  self.save!
  version
end

create_version!を消す

ここでcreate_version!を消します。 create_version!versionに関するコードはコントローラに移します。

class ArticlesController < ApplicationController
  def create
    @article = Article.new(params[:article])
    @article.reporter_id = current_user.id
    @version = self.versions.build(attributes)
    @version.writer_id = current_user.id

    if @article.save
      redirect_to article_path(@article)
    else
      render action: :new
    end
  end
end

これで目標とする形にかなり近づきました。

その先

この書籍ではnested_attributesを使う方法が書かれていました。最近のRailsバージョンだとaccept_nseted_attributes_forを使うのかと思いますが、実はこの機能、頃合いを見て消される方針のようです。

https://github.com/rails/rails/pull/26976#discussion_r87855694

代替としては、自分が知っているのはFormオブジェクト使うやり方があるのですが、だいぶ本に載っている内容からはそれるので今回はパスします。 Formオブジェクトの作り方はいろいろあると思いますが、パーフェクトRailsあたりに載っていた気がしますし、有料ですがgorailsにも。

gorails.com

まとめ

fat controller解消は何かパターンがあるわけではなく、リファクタリングとコールバックの積み重ね、最終的にどうしようもない所だけFormオブジェクトにするという泥臭い感じになるのだなと思いました。ただこうゆうことをサクッとできないとfat controllerと付き合うしか無いと思うので、この辺できるようにしておきたいです。
scaffoldに近づけるのが理想的という話はめちゃくちゃ同意できました!

[2017-05-07 20:50追記] 2つ目の解決策であるMove to a Presenterはactive_presenterというgemを使うのですが、今はメンテされていない様子です。内容としては上で言うFormオブジェクトっぽいものを宣言的にくためのgemのようでした。こちらに関しては深入りしないことにします。

Rails アンチパターン - 覗き見モデルクラス(Voyeuristic Models)

はじめに

引き続きRails AntiPatternsという本を読んでいます。

https://www.amazon.co.jp/dp/B004C04QE0/ref=dp-kindle-redirect?_encoding=UTF8&btkr=1

前回は2 Domain Modelingの1つめ「Authorization Astronaut」についてまとめました。

waterlow2013.hatenablog.com

今回は1 Modelsの1つめ「Voyeuristic Models」についてまとめていきます。

Voyeuristic Models

Ruby on RailsMVC、およびオブジェクト指向プログラミングが提供する構造を認識していない場合などに、オブジェクト指向の基本的な原則を破ってしまうことがあります。またActiveRecordなどの便利な機能に乗りすぎてしまう場合もあります。
ここでは、MVCオブジェクト指向プログラミングの教えに違反するいくつかのサンプルを見ていきます。

Solution: Follow the Law of Demeter(デルメルの法則に従う)

デメテルの法則 - Wikipedia

サンプルで見ていこうと思います。以下のようなモデルがあるとします。

class Address < ApplicationRecord
  belongs_to :customer
end
class Customer < ApplicationRecord
  has_one :address
  has_many :invoices
end
class Invoice < ApplicationRecord
  belongs_to :customer
end

顧客は住所を1つ、請求を複数持ちます。請求の住所行を表示するビューは、次のようになります。

<%= @invoice.customer.name %>
<%= @invoice.customer.address.street %>
<%= @invoice.customer.address.city %>,
<%= @invoice.customer.address.state %>
<%= @invoice.customer.address.zip_code %>

適切なカプセル化を考えると、@invoiceはcustomerを経由してaddressにアクセスするべきではありません。たとえば、顧客が請求先住所と配送先住所の両方を持つようにアプリケーションを変更すると、これらのオブジェクトを通過して通りを取得するコード内のすべての場所が壊れ、変更する必要があるためです。

幸いにも、ActiveRecordには、delegateというメソッドがあります。このデリゲートメソッドを使用すると、addressオブジェクトのことを知ることなく顧客の住所を表示できます。

class Address < ApplicationRecord
  belongs_to :customer
end

class Customer < ApplicationRecord
  has_one :address
  has_many :invoices
  delegate :street, :city, :state, :zip_code, to: :address
end

class Invoice < ApplicationRecord
  belongs_to :customer
  delegate :name, :street, :city, :state, :zip_code, to: :customer, prefix: true
end

viewのコードは以下のようになります。

<%= @invoice.customer_name %>
<%= @invoice.customer_street %>
<%= @invoice.customer_city %>,
<%= @invoice.customer_state %>
<%= @invoice.customer_zip_code %>

これやったほうがいいとは思うのですが、関連するオブジェクトをそのまま取って出すだけなら最初のままでもいい気がします。ただ悩ましいところではある…。これちょっと修正したいときに.gsubとかviewに書いてしまう事があって、それを助長する気がしています。そしてDRYでない子0ドが生まれていくという。なので1回これに従ってみるのもいいかなと思えてきました。

Solution: Push All find() Calls into Finders on the Model(生のクエリメソッドは全てモデルに書く)

これもサンプルを見ます。

<html>
  <body>
    <ul>
      <% User.order(:last_name).each do |user| -%>
        <li><%= user.last_name %> <%= user.first_name %></li> <% end %>
    </ul>
  </body>
</html>

これはビューに直接sql呼び出しが書いている状態であり、重複するロジックがそれぞれの場所で生まれてしまう可能性があります。
ひとまずコントローラに移します。

class UsersController < ApplicationController
  def index
    @users = User.order(:last_name)
  end
end
<html>
  <body>
    <ul>
      <% @users.each do |user| -%>
        <li><%= user.last_name %> <%= user.first_name %></li> <% end %>
    </ul>
  </body>
</html>

これでモデルのロジックはなくなりましたが、コントローラにロジックの記載が残っています。これをモデルに移します。

class User < ApplicationRecord
  scope :ordered, -> { order(:last_name) }
end
class UsersController < ApplicationController
  def index
    @users = User.ordered
  end
end

Solution: Keep Finders on Their Own Model (生のクエリメソッドは自身のクラスにしか書かない)

検索コールをRailsアプリケーションのコントローラレイヤーからモデル上のカスタムファインダに移動することは、メインテナンス可能なソフトウェアを作成する正しい方向への強力なステップです。しかし、一般的な間違いは、適切な責任の委任を無視して、それらの呼び出しを最も近いモデルに移動することです。

耳が痛い話だ!!!

class UsersController < ApplicationController
  def index
    @user = User.find(params[:id])
    @memberships =
      @user.memberships.where(active: true).
        limit(5).
        order("last_active_on DESC")
  end
end

今まで学んだことに基づいて、そのスコープチェーンをUserモデルのメソッドに徹底的に移動します。UsersControllerを扱っているので、これが最良の場所のようだと思います。

class UsersController < ApplicationController
  def index
    @user = User.find(params[:id])
    @recent_active_memberships = @user.find_recent_active_memberships
  end
end
class User < ApplicationRecord
  has_many :memberships

  def find_recent_active_memberships
    memberships.where(active: true).
      limit(5).
      order('last_active_on DESC')
  end
end

これは間違いなく改善です。 しかし、もう少しすることができます。Userモデルはメンバーシップモデルの実装について、active、last_active_onというカラムのことを知ってしまっています。これは、まだメソッドを十分にプッシュしていないという手がかりです。

(途中経過を省き最終型のみ載せます)

class User < ApplicationRecord
  has_many :memberships

  def find_recent_active_memberships
    memberships.only_active.order_by_activity.limit(5)
  end
end

class Membership < ApplicationRecord
  belongs_to :user

  scope :only_active, where(active: true)
  scope :order_by_activity, order('last_active_on DESC')
end

scopeにしたことによりロジックを組み合わせて使うことができます。

scopeにしたことが良いのと、limit(5)がUserモデルにあるのがすごくいい感じがしています。

このアプローチには、読みやすさとシンプルさの問題だけでなく、デメテルの法律の乱用などの短所もあります。
このアプローチを使用するかどうかは、あなた次第です。多くの高度なリファクタリングと同様に、これは簡単な答えがなく、好みにも大きくよるものです。

まとめ

正直ここまで徹底してやったことがなく、これを都度やっていくコストがどれくらいなのかわかりません。ただ、毎回コントローラに書いていってしまうと共通化、scope化するのがどんどん難しくなっていくので、ある程度の規模ではやるべきなのかなと思っています。モデル50個からとか?
とりあえず、次自分で何か書くときにはこれ徹底してみようと思います。