コントローラとモデルをリファクタリングする: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.