実例アプリで学ぶ“Railsらしさ”の基礎Railsで目指せ、情熱エンジニア(6)(2/2 ページ)

» 2011年05月26日 12時00分 公開
[井上真,New Bamboo]
前のページへ 1|2       

 さて、ここまでWorklistaのコードの良い点を指摘しました。次は、改善の余地のある点を探っていきましょう。ここで時間のある方はすぐに私の分析を読むのではなく、まずご自分で改善点リストなどを列挙して、どれくらい私の挙げた点と合致するか、または異なるかを比較してみるのも面白いかもしれません。

Railsらしくコーディングする3つの方針

 Ruby on Railsの掲げる、コーディングの進め方の基本としては以下の3つが有名です。

  • DRY(Don't Repeat Yourself)
  • MVC(Model Controller View)
  • CoC(Convention over Configuration)

 DRY(ドライ)は、自分がやったことを2度繰り返さないということで、同じコードや設定をあちこちに書かないようにすることで、全体の見通しを良くし、メンテナンスも容易にしようということです。MVCについては、@ITの別のRails入門連載に良い解説があるので、そちらを参照してください。Convention over Configuration(設定より規約)は、個々のプロジェクトごとに設定をするのではなく、あらかじめ決められた規約に則ったディレクトリ構成、ネーミングルールなどを採用することで、開発工数や、設定ミスを減らそう、ということです。

 さて、それでは上記の点を照らし合わせながら、Worklistaのソースコードの改善候補点を挙げていきましょう。

DRYでないのはココだ!

 まず私が一番に目を付けたのは、以下の行が、destroy、update、editの3つのメソッドに点在していることです。こういったものはメソッドとして抽出して共有が可能です。

@item = Item.find(params[:id])

 先ほど挙げた点ですが、ここには重複が2種類含まれています。

 1つ目は、populate_という接頭辞です。せっかくこれらがpopulateメソッド内で呼ばれているので、わざわざ同じ名前の接頭辞をつける必要はないでしょう。2つ目は、全てのメソッドがitemを引数として取っている点です。各メソッドの実装を覗いてみると、ほとんどの操作はitemオブジェクトに対して何かを操作している処理が記述されています。

def populate(item)
  populate_title(item)
  populate_hatena(item)
  populate_retweet(item)
end

 「リファクタリング」の著書で知られるマーチン・ファウラー氏はその本の中で「コードの不吉な匂い」のリストを20個ほど挙げています。「重複したコード」はそれ自体が不吉な匂いの1つとして挙げられているのですが、この場合は「属性、操作の横恋慕」という匂いにも合致するのではと思います。

【属性、操作の横恋慕】オブジェクト指向には、処理および処理に必要なデータを1つにまとめてしまうという重要な考え方があります。あるメソッドが自分のクラスよりも他のクラスに興味を持つような場合には古典的な誤りを犯しています。


 こうした場合の対処法として、メソッドを本来所属すべきクラスに移動することが推奨されています。ここでの「本来所属すべきクラス」は、Itemクラスに該当しますよね。

MVCの役割分担通りになっているか?

 「メソッドを本来所属すべきクラスに移動すること」と述べましたが、それではItemsContorllerクラスとItemクラスの役割とはいったい何で、どういった処理がどこに属するべきなのでしょうか?

 私もRailsを始めた頃はこの役割分担でずいぶん悩んだものです。Agile Development With Ruby on Railsでは、以下のようにControllerを定義しています。

 Controllers orchestrate the application. Controllers receive events from the outside world (normally user input), interact with the model, and display an appropriate view to the user.

 Controllersはアプリケーションと統合するものである。Controllersは外界からイベントを受け取り、モデルと対応し、適切なビューをユーザーに表示する。


 正直いって、最初にこれを読んだとき、私には、ちんぷんかんぷんでした。

 こういうのは、実際にコードを見ながら解説した方が分かりやすいと思います。

 理想としては各Controller内のアクションは以下のロジックだけがあることを目指すと良いでしょう。

def create
  @user = User.find(params[:user_id])
  @item = @user.items.new(params[:item])
  if @item.save
    redirect_to edit_user_item_path(@user, @item)
  else
    render :action => 'new'
  end
end

 このメソッドがしていることは、3つあります。

1. 適切なオブジェクトを取ってくる

@user = User.find(params[:user_id])

2. オブジェクトに対する何らかの操作を指示する

@item = @user.items.new(params[:item])

3. 操作が成功した際と失敗した際のビューの振る舞いを指定する

if @item.save
  redirect_to edit_user_item_path(@user, @item)
else
  render :action => 'new'
end

だけです。

 でも実際にはそんなに簡単にいきませんよね。Worklistaに関しても、外界からHTMLを取って来るなど複雑なことをしています。ただ、キーとなることは、上記以外のことをControllerで行おうとした際に、「ひょっとしたらModelでもできるんじゃない?」と考えてみることです。

 もちろん西村さんもそのことについては考えていたようで、以下のようなコメントが残されていたのだと思います。

# let us do the url validation in the contorller

 このコメントの意図について西村さんに問い合わせてみたところ以下のような返事が返ってってきました。

 ユーザーが入力したURLを元にopen-uriを使って実際にWebサーバにアクセスしてHTML文書を取ってきて、その文書からtitleタグを抜き出して、これをDBに入れています。という処理は、DBに書き出す以前に終わらせています。すべてコントローラで起こるので、モデルではダメだろうということです。でも、考えて見れば、いったんitem.saveして、そこからitem.populateとやってもいいのかな? という気もしてきました。

 でしたらこれでも良いですよね。

def create
  @user = User.find(params[:user_id])
  @item = @user.items.new(params[:item])
  if @item.valid? && @item.populate && @item.save
    redirect_to edit_user_item_path(current_user, @item)
  else
    render :action => 'new'
  end
end

 URLのフォーマットなどはバリデーションで行い、HTML文書を抜き取る作業はpopulateメソッドで行う。最後にsaveで保存するといった具合です。こうすることで、Controllerもずいぶんすっきりしてきます。

 面白いのは、私がわざわざそういう模範解答を西村さんにお伝えする前に、彼は自分でその回答までたどり着いている点です。これはペアで作業することの長所であったりもします。何か分からないことがあった際に、他人に系統立てて説明することで、より良い回答を自分で導くことはよくあります。

 ちなみに「@item.valid? && @item.populate && @item.save」のところはもっと改善の余地があるのですが、それは後ほど触れたいと思います。

Railsの規約に則っているか?

 Convention over Configuration(設定より規約)の本来の意味は「デフォルトをあらかじめ設けておくことで、xmlなどの設定ファイルをたくさんも書くことを避ける」というものです。

 これはRailsがフレームワークとしてやっていることなので、自分でデフォルトを設ける必要はあまりないのですが、フレームワークを使う側としても、Rails的なしきたりに従うことで「Railsのよき市民」として振る舞うことが求められます。

 そういう視点で見たときに、以下のクラス変数の設定が少し奇妙に思えました。

conf = APP_CONFIG['bitly']
@@bitly = Bitly.new(conf['username'], conf['apikey'])

 これはURL短縮サービスであるbit.lyのAPIにアクセスするためのオブジェクトを設定しているのですが、これをControllerのクラス変数に入れている例は、あまり見たことがありません。これらはアプリケーションごとに1回設定すれば終わりなので、initializerディレクトリ以下に放り込むほうが一般的に思えます。

 もう1つ、個人的な感想ですが、必要のないコードは極力減らすようにしたほうが良いと思います。

 Ruby on Railsを作ったDHHがパートナーとして勤務する会社の人気ブログのタイトルは「Signal vs Noise」です。これは多くの雑音(Noise)の中から、いかに必要となる情報(Signal)を抜き出すかということですが、その際の秘訣として「不必要なものは削除する」というのがあると思います。

 その際まず削除の第一候補として、「不必要なコメント」があります。

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!

 ここにある「You are not the owner of this item!」というコメントは、かなり自明なのであってもあまり助けになりません。

 もし「user_signed_in? && @user == current_user」というロジックが分かりずらいのであれば、これを「owner?」というメソッドに置き換えてみるのも方法です。コメントには「このコードは何をしているか(what?)」よりも「なぜこのように書いたか(why?)」を書くことをお勧めします。

 そして、Rubyの文法的には正しいのですが、不必要なものとしてthenの使用があります。

if date then
  item.published_at = date
else
  item.published_at = Time.now
end

 thenは1行で書かなければいけない場合には必要ですが、複数行にわたって書かなければいけない場合は特に必要ありません。また、1行で書きたい場合は以下のようにもっと簡潔に書ける方法があるので、thenを使っている例に、最近はあまりお目にかかりません。

ruby-1.9.2-p0 > if 1 == 1  1 else  2 end
SyntaxError: (irb):40: syntax error, unexpected tINTEGER, expecting keyword_then or ';' or '\n'
if 1 == 1  1 else  2 end
 
ruby-1.9.2-p0 > if 1 == 1 then  1 else  2 end
=> 1 
ruby-1.9.2-p0 > if 1 == 1 ;  1 else  2 end
=> 1 
ruby-1.9.2-p0 >  (1 == 1) ?  1 :  2
=> 1 

 これと似た例で、セミコロン(;)があります。セミコロンもワンライナーのときには必要なのですが、以前PHPエンジニアの人が、全ての行にセミコロンを付けたRubyのコードを書いていて、仰天したのを覚えています。

 さて、今回はコードレビューを通して、Railsらしい書き方を解説しました。次回からはリファクタリングを行う上で必要になる様々なテスト手法について、引き続き西村さんの例を使って解説していきたいと思います。

前のページへ 1|2       

Copyright © ITmedia, Inc. All Rights Reserved.

RSSについて

アイティメディアIDについて

メールマガジン登録

@ITのメールマガジンは、 もちろん、すべて無料です。ぜひメールマガジンをご購読ください。