More than 5 years have passed since last update.

Railsのcontrollerにおいて、メソッド外で初期化したインスタンス変数を参照すべきでない理由

Posted at 2018-08-27

背景

Ruby on Rails を利用した中規模以上のプロジェクトにおいて、 Fat な controller を解消するために、処理を分割することはしばしば行われます。例えば、サブメソッドへの一部処理の切り出し, before_action, concern への処理の委譲が行われます。しかし、処理を分割する際に、メソッド外で初期化したインスタンス変数を参照してしまうような例を時々見かけます。

このような実装は、一見DRYでシンプルに見えてしまうかもしれませんが、実際には開発効率の低下につながったり、バグの原因になる可能性が高くなります。

この記事では、メソッド外で初期化されたインスタンス変数を参照することの危険性と、それを避けた方が良い理由を考察してみたいと思います。

避けた方が良い例

class BooksController
  def index
    set_author
    set_books # これらが before_action で定義されていると、更に悪い
  end

  private

  def set_author
    @author = Author.find(params[:author_id])
  end

  def set_books
    @books = Book.where(author_id: @author.id).to_a
  end
end

結合度の観点から見た危険性

まずは結合度の観点から、上記例の危険性を見てみたいと思います。

Rails では、リクエストを処理するために、controllerのインスタンスが生成されます。オブジェクト指向においては、インスタンスが処理を行う過程において、自らのインスタンス変数にアクセスすることは、そこまで咎められることではありません。
しかし、さらに粒度を細かくして観察してみると、1リクエストを受け取ってから、viewに処理を委譲するまでは一種の手続き的処理だとも考えることが出来ます。

そう考えると、メソッド外、すなわち、別メソッドの内部で初期化されたインスタンス変数を直接参照している実装は、オブジェクト指向の観点からは許されているものの、1つ1つのメソッドをモジュールと考えれば、情報隠蔽を壊している、Content Couplingという、最も結合度の高い状態とみなすことが出来ます。

Content coupling (high)
Content coupling is said to occur when one module uses the code of other module, for instance a branch. This violates information hiding - a basic design concept.

例えば、上記例であれば、 set_books メソッドは set_author の内部で初期化された @author を直接参照しています。この結果、 set_author の内部実装を変更すると、 set_booksの実装も変える必要が出てくるという結合度の非常に高い状態になっています。

上記例では、メソッドの記述場所が隣にあったため、一瞥しただけで影響範囲がわかりましたが、 before_action や concern を利用している場合は、結合度の高いメソッド同士の凝集度が低くなっている可能性が高く、開発効率の低下やバグの原因になる可能性がより一層高くなってしまいます。

契約プログラミングの観点から見た危険性

次に、契約プログラミングの観点から見た理由を考察してみます。

契約プログラミングの観点では、サブルーチンの呼び出し側が事前条件を保証する義務を追うことで、サブルーチンの事後条件が満たされるという利益を得ます。つまり、事前条件を守れば、コードが正常に実行されます。

では、サブルーチンがメソッド外で初期化したインスタンス変数を参照している場合はどうなるのでしょうか。インスタンス変数を参照しているということは、「そのインスタンス変数がきちんと初期化されていること」が事前条件に追加されます。厄介なのは、これがメソッドの見た目（主に引数）からは分からないということです。例えば、上記例では、 index メソッドを見るだけでは、 set_books の事前条件が満たされているのかをすぐに判別することが出来ません。

このような状況下では、一見関係なさそうな２つのメソッドの呼び出し順序を変更するだけで、事前条件が満たされなくなり、エラーが発生します。例えば、 before_action でインスタンス変数をセットしているメソッド群の順序を変更すると、何故かバグるという状態に至ることもあるでしょう。

class BooksController
  before_action :set_author
  before_action :set_books # 順番を変えると、なぜかバグる

  def index
  end
end

何をすべきなのか？

それでは、どのような実装をすれば良いのでしょうか？
上述した例であれば、自らのメソッド外で初期化されたインスタンス変数を直接参照することはせず、引数というインターフェースで受け取るように実装するのが望ましいと思います。こうすることで、データ隠蔽が守られ、結合度が下がります。また、守るべき事前条件も引数に限定され、可視性が高まる為、コードの見通しがよくなります。

class BooksController
  def index
    @author = find_author(params[:author_id])
    @books = find_books(author_id: @author.id) # params[:author_id] でも可能だが例示するため
  end

  private

  # サブメソッドに切り出す程のものではないが説明のため切り出している
  def find_author(author_id)
    Author.find(author_id)
  end

  def find_books(author_id:)
    Books.where(author_id: author_id).to_a
  end
end

また、今回の記事では触れませんでしたが、上記改善バージョンはインスタンス変数へのセットをトップレベルのメソッドに限定することにより、viewに処理を移譲する際の事前条件が満たされているか否かをわかりやすくしています。
もちろん、Convention over Configuration的観点において、set_#{instance_variable_name} というメソッドは必ずその名前のインスタンス変数を設定するというルールが徹底でき、メソッド同士の結合度が低い状態で実現できるのであれば、before_actionでインスタンス変数をセットするのもありかとは思います。

最後に

メソッド外で初期化したインスタンス変数を参照すべきでないのは、見通しの良いcontrollerを設計する上で必要なことだと思います。コードが大規模になっていくと、これ以外にも考慮することが増えますが、Railsのサンプルコードから改造し始めると陥りやすいインスタンス変数の扱い方について考察してみました。

参考

You get articles that match your needs
You can efficiently read back useful information
You can use dark theme

What you can do with signing up