コントローラとモデルをリファクタリングする：Railsで目指せ、情熱エンジニア（10）
前回用意したRailsアプリのコントローラのテスト（スペック）をもとに、今回はコントローラとモデルのリファクタリングの実例を紹介します。
前回はコントローラのスペックを作成しました。スペックの全文は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
アクションの中身を列挙してみると
- ユーザー情報を取ってくる
- アイテムオブジェクトの作成
- アイテムのURLがちゃんとしたものかどうか確認
- アイテムのURLを元にページの中身を取ってくる
- アイテムページの作成時刻を推測
- アイテムページの評価情報を取ってくる
- アイテムの情報をセーブ
と、コントローラが大忙しです。しかし、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点に近づいてきたのはないでしょうか？
- 適切なオブジェクトを取ってくる
- オブジェクトに対する何らかの操作を指示する
- 操作が成功した際と失敗した際のビューの振る舞いを指定する
欲を言えば、@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.
ITmediaはアイティメディア株式会社の登録商標です。