検索
コラム

コントローラとモデルをリファクタリングするRailsで目指せ、情熱エンジニア(10)

前回用意したRailsアプリのコントローラのテスト(スペック)をもとに、今回はコントローラとモデルのリファクタリングの実例を紹介します。

PC用表示
Share
Tweet
LINE
Hatena

 前回はコントローラのスペックを作成しました。スペックの全文はGitHubにあります。今回はリファクタリング後のコントローラのコードを解説していきます。リファクタリング前のコードは以下のコマンドで参照可能です。

git co a576102eedc8ca4a931d15c186620a9a43b9ad39

 実際のリファクタリングは一度に全部の変更をしてしまうのではなく、「少し変更」→「テストを走らせて、グリーンであることを確認」→「コミット」というサイクルを何度も繰り返すことで行います。今回のリファクタリングもitems_controllerだけで20回ぐらいに分けてコミットしました。全てのコミットを解説していくときりがないので、主要な変更点をリファクタリングの前後で比較しながら解説していきます。

before_filterを使って重複をなくす

 before_filterは各コントローラのアクションの前段階に実行したいメソッドを指定するためのフィルターです。元のコードでもauthorise_as_ownerというメソッドが、権限のないユーザーをリダイレクトするためのフィルターとして使われています。

class ItemsController < ApplicationController
  before_filter :authorise_as_owner
  def authorise_as_owner
    @user = User.find(params[:user_id])
    unless (user_signed_in? && @user == current_user)
      # You are not the owner of this item!
      flash[:notice] = "Oops, something went wrong!"
      redirect_to users_path
    end
  end
end

 このフィルターメソッドですが、重複項目を1つのメソッドに統一する用途としても使えます。コードを見たところ「@item = Item.find(params[:id])」というコードがcreateアクション以外の全てで重複して使われていたので、これをフィルターでひとくくりにしてみました。

[worklista (d5698ee...)]$ git show e2ee0fc4a8a71103ab249f5270877628115df2a0 app/controllers/items_controller.rb
commit e2ee0fc4a8a71103ab249f5270877628115df2a0
Author: Makoto Inoue <inouemak@googlemail.com>
Date:   Tue Dec 28 00:22:47 2010 +0000
    Refactoring (Added find_item)
diff --git a/app/controllers/items_controller.rb b/app/controllers/items_controller.rb
index bacdec3..e259348 100644
--- a/app/controllers/items_controller.rb
+++ b/app/controllers/items_controller.rb
@@ -1,6 +1,7 @@
 
 class ItemsController < ApplicationController
   before_filter :authorise_as_owner
+  before_filter :find_item, :except => [:create]
  
   def create
     @item = @user.items.new(params[:item])
@@ -17,18 +18,14 @@ class ItemsController < ApplicationController
   end
 
   def destroy
-    @item = Item.find(params[:id])
     @item.destroy
     flash[:notice] = "Successfully destroyed an item."
     redirect_to user_recent_path(current_user.username)
   end
 
-  def edit
-    @item = Item.find(params[:id])
-  end
+  def edit; end
 
   def update
-    @item = Item.find(params[:id])
     if @item.update_attributes(params[:item])
       flash[:notice] = "Successfully updated item."
       redirect_to user_recent_path(current_user.username)
@@ -48,4 +45,5 @@ private
   end
   
   def owner?; user_signed_in? && @user == current_user; end
+  def find_item; @item = Item.find(params[:id]); end
 end

 フィルターには:onlyや:exceptというオプションがあります。それぞれ、「このアクションのみに適用」「このアクション以外の全てに適用」という意味で、フィルターの適用アクションを細かく設定可能です。

役割をコントローラからモデルに移す

 Worklistの主要な機能として、「URLで指定したページのコンテンツの中身(タイトルなど)やソーシャルサイト(bit.ly、hatena)の評価情報を取ってくる」というものがあります。これを担当しているのが、ItemsController#creationアクションです。

  def create
    @user = User.find(params[:user_id])
    @item = @user.items.new(params[:item])
    if @item.url !~ /^(#{URI::regexp(%w(http https))})$/ then
      flash[:notice] = "Invalid URL!!"
      redirect_to user_recent_path(current_user.username)
      return
    end
  
    begin
      Timeout::timeout(8){
        @doc = open(@item.url).read
      }
    rescue Timeout::Error
      flash[:notice] = "Timeout! Could not retrieve data from the URL!!"
      redirect_to user_recent_path(current_user.username)
      return
    end
  
    guess_date @item
    populate @item
  
    if @item.save
      flash[:notice] = "Created an item. Any changes?"
      redirect_to edit_user_item_path(current_user, @item)
    else
      render :action => 'new'
    end
  end

 アクションの中身を列挙してみると

  1. ユーザー情報を取ってくる
  2. アイテムオブジェクトの作成
  3. アイテムのURLがちゃんとしたものかどうか確認
  4. アイテムのURLを元にページの中身を取ってくる
  5. アイテムページの作成時刻を推測
  6. アイテムページの評価情報を取ってくる
  7. アイテムの情報をセーブ

と、コントローラが大忙しです。しかし、2〜6までの情報はItemモデルしか関わらないことなので、それらを全て@item.loadというモデルの1メソッドに押し込んでみました。以下がリファクタリング後です。

  def create
    @item = @user.items.new(params[:item])
    if @item.load
      flash[:notice] = "Created an item. Any changes?"
      redirect_to edit_user_item_path(current_user, @item)
    else
      flash[:error] = "Invalid URL!!"
      redirect_to user_recent_path(current_user.username)
    end
  rescue Timeout::Error
    flash[:error] = "Timeout! Could not retrieve data from the URL!!"
    redirect_to user_recent_path(current_user.username)
  end

 いかがでしょうか? これでcreateアクションが以前説明した以下の3点に近づいてきたのはないでしょうか?

  1. 適切なオブジェクトを取ってくる
  2. オブジェクトに対する何らかの操作を指示する
  3. 操作が成功した際と失敗した際のビューの振る舞いを指定する

 欲を言えば、@item.loadに失敗した時のリダイレクト先とタイムアウトした時のリダイレクト先が全く同じなのでアイテムモデルで処理してしまっても良いかもしれませんね。

 ただそれをやろうとすると、エラーメッセージをflash[:error]に全て押し込むよりも、ビューに@item.errorsの中身を表示するように変えた方が良い気がします。最初は一部分のみをリファクタリングするつもりが、「あそこも変えよう」「ここも変えよう」と欲が出て、数珠つなぎ的にリファクタリングしてしまい、きりがなくなってしまうことを私たちは「Big Bang Refactoring」と呼んでなるべく控えるようにしています。以前オーストラリアから訪れていたエンジニアは、このリファクタリングをしたい誘惑に負けて延々とやってしまうことを「リファクタベーション」(リファクタリング+マスターベーション)と呼んでいました。ヤればヤるほど快感度が増してしまうのですが、やり過ぎには注意が必要です。

モデルのリファクタリング

 「今まであんなに長かったコントローラのコードが一気に短くなって良かった。めでたしめでたし」と言いたいところですが、機能を削らない以上、どこかにコードを移さなければいけません。では、コントローラからモデルに移されたコードがどうなったか見ていきましょう。

 これはリファクタリング前のItemクラスです。

class Item < ActiveRecord::Base
  belongs_to :user
  has_many :taggings, :dependent => :destroy
  has_many :tags, :through => :taggings
  
  # let us do the url validation in the contorller
  
  attr_writer :tag_names
  after_save :assign_tags
  
  def tag_names
    @tag_names || tags.map(&:name).join(' ')
  end
  
private
  def assign_tags
    if @tag_names
      self.tags = @tag_names.split(/\s+/).map do |name|
        Tag.find_or_create_by_name(name)
      end
    end
  end
end

 tagsに関する少量のコードと他のモデルとの関係(has_many、belongs_toなど)が記述されているだけの極めて小さなクラスでした。そしてこれがリファクタリング後のコードです。

class Item < ActiveRecord::Base
  belongs_to :user
  has_many :taggings, :dependent => :destroy
  has_many :tags, :through => :taggings
  validates :url, :format => {:with => URI::regexp(%w(http https))}
  
  attr_writer :tag_names
  attr_accessor :doc
  
  before_create :guess_published_at, :set_title
  after_save :assign_tags
  
  def load
    valid? && fetch && save!
  end
  
  def fetch
    Timeout::timeout(8) do
      set_hatena
      set_retweet_and_bitly_url
    end
  end
  
  def tag_names
    @tag_names || tags.map(&:name).join(' ')
  end
  
  def doc
    @doc ||= open(url).read
  end
  
  private
  def assign_tags
    if @tag_names
      self.tags = @tag_names.split(/\s+/).map do |name|
        Tag.find_or_create_by_name(name)
      end
    end
  end
  
  def guess_published_at
    self.published_at = if doc =~ /(20\d{2}\/[01]?\d\/[012]?\d)/
      Date.strptime($1, "%Y/%m/%d")
    else
      Date.today
    end
  end
  
  def set_title
    self.title = url
    doc.match(/<title>([^<]+)<\/title>/) do |m|
      if m.size == 2
        title = m[1]
        self.title = NKF.nkf("--utf8", title)
      end
    end
  end
  
  def set_hatena
    hatena_api = "http://api.b.st-hatena.com/entry.count?url="
    num = open(hatena_api+url).read
    num = 0 if num == ""
    self.hatena = num
  end
  
  def set_retweet_and_bitly_url
    shortend_url = BITLY.shorten(self.url)
    self.bitly_url = shortend_url.short_url
    self.retweet   = shortend_url.global_clicks
  end
end

 主要な部分だけまたかいつまんで説明します。

  validates :url, :format => {:with => URI::regexp(%w(http https))}

 URLが正しいフォーマットかどうかは、ActiveRecordのvalidationの一部としてみました。

  def load
    valid? && fetch && save!
  end

 そしてloadメソッドの中で、まずURLが適切か確認した後に実際にWebから色々な情報を取ってくるfetchメソッドを呼びます。

  before_create :guess_published_at, :set_title

 最後にitemをデータベースに保存(save!)する前に、URL先のページから必要なデータ(作成日時、タイトルなど)を抜き出しています。

  def set_title
    self.title = url
    doc.match(/<title>([^<]+)<\/title>/) do |m|
      if m.size == 2
        title = m[1]
        self.title = NKF.nkf("--utf8", title)
      end
    end
  end

 「URL先のページから必要なデータなんていつ取りにいったの?」と思われた方はちゃんとコードを丁寧に追っていますね。

  def doc
    @doc ||= open(url).read
  end

 上のコードは「もし@doc変数に値があればそれを返しなさい、もしなければURL先からデータを取って来て、その中身を代入しなさい」という意味です。このメソッドは、メソッドが呼ばれるたびにURL先に接続する手間を省くための、ちょっとしたキャッシュとしての役割も負っています。

 このコードを書くときにfetchメソッドをbefore_createに指定することでアイテムがセーブされる前に自動的に呼び出すようにするかどうか少し迷いました。もしbefore_createに組み入れた場合、わざわざloadというメソッドを作らなくてもsaveだけで全てが完結してしまうからです。あえてそうしなかったのはテストのしやすさを考えた上でのことです。例えば、URL先のページの作成日時を取ってくるためのメソッドをテストするコードがあるとします。

    describe "when document includes date" do
      it "should set published at from document" do
        @item.doc = '2010/09/13'
        @item.save!
        @item.published_at.should == Date.parse('2010/09/13')
      end
    end

 もしこうしてitemを毎回セーブするためにfetchメソッドが発動し、はてなやbit.lyまでデータを取りに行くはめになったらテストが遅くなってかないません。実は「モック」や「スタブ」というメソッドを使ってメソッドが実際に発動するのを防ぐ手段があるのですが、わざわざそういう余計な仕組みを使うよりも、コードの設計の方で回避することにしました。

 「なんでテストのしやすさのためにコードそのものを変えなくちゃいけないんだ?」と思われるかもしれませんが、往々にしてテストしづらいコードというのは設計自体がおかしいことが多いので、テストしやすいコードを書くことは、すっきりしたコードを書くことにつながるのではないかと思っています。

連載のまとめ

 これまで複数回にわたって連載編集担当の西村さんが作ったRailsアプリのWorklistaを通じてコードレビュー、さまざまなテスト技法、リファクタリングの実践などを行ってきました。コードの書き方は各個人によって好みが異なるかもしれませんし、方法論も時代によって、はやり廃りがあると思います。 昔はコントローラやモデル間で共通のメソッドを共有する方法として継承を使うことが多かったですが、過去1年ぐらいの間に関わったプロジェクトでは以下のようなモジュールを作って個別にincludeするパターンを多用しました。

     module M
       def self.included(base)
         base.extend ClassMethods
         base.class_eval do
           scope :disabled, -> { where(disabled: true) }
         end
       end
  
       module ClassMethods
         ...
       end
     end

 ただ「def self.included(base)」みたいなのを毎回書くのもめんどくさいので、最近はActiveSupport::Concernという機能を使うようになりました。ActiveSupport::Concernを使うと、以下のように少し簡潔に書くことができます。

     require 'active_support/concern'
  
     module M
       extend ActiveSupport::Concern
  
       included do
         scope :disabled, -> { where(disabled: true) }
       end
  
       module ClassMethods
         ...
       end
     end

 「もっと良い方法があるよ」と思われる方は、積極的にWorklistaをフォークして、リファクタリング結果を教えていただきたいです。GitHubなどを通じたソーシャルコーディングによってお互いにスキルを高めて行くためのエコシステムがあるのは、Railsの凄いところです。是非、この連載をご覧になった読者の方も、Railsアプリ作りに挑戦してみてください。

Copyright © ITmedia, Inc. All Rights Reserved.

ページトップに戻る