Railsにはrails_best_practiceというGemがあるのですが、
これは今時の書き方を提案してくれる便利なGemです。
URL: https://github.com/railsbp/rails_best_practices
このgithubページにいくつか実装されている内容が書かれているのですが、個人的に良さそうだなと思ったところだけピックアップして書きたいと思います。
the Law of Demeter(デメテルの法則）
これは、デメテルの法則に反するという警告を指し示しています。デメテルの法則とは、オブジェクトは直接関わっているオブジェクトの状態だけを把握しているというソフトウェア設計の概念です。Railsではオブジェクト指向の設計なので、特に設計の概念に適用されるでしょう。
悪い例が、
@vote.user.name
良い例が、
@vote.user_name
メソッドを二段階で呼び出すということをやっているというのは、デメテルの法則に反するということです。デメテルの法則は、オブジェクトは直接関わっているオブジェクトの状態だけを把握している概念です。なので、間接的にメソッドを呼び出すような処理は好ましくないということです。
Railsのモデルに書かれている処理にこれの警告が出ます。
class Vote < ActiveRecord::Base
belongs_to :user
end
<%= @vote.user.name %>
ActiveSupportには、delegateというインスタンスメソッドが存在するので、そちらを使ってあげましょうとのこと。
class Vote < ActiveRecord::Base
belongs_to :user
delegate :name, to: :user, prefix: true, allow_nil: true
end
<%= @vote.user_name %>
delegateメソッドは、引数でtoが必須になっております。toで関連するモデルを指定してあげましょう。
prefixの指定がtrueの場合、@vote.user_nameの呼び出しが可能になります。もしも、falseの場合は、@vote.nameという呼び出し方になってしまい、これでは、「voteオブジェクトのname」を呼び出しているようになってしまい、本末転倒です。加えて、prefixにはboolean値以外にもstringが指定できて、例えば、prefix: :authorとすると、@vote.author_nameといった形で呼び出せるようになります。
また、allow_nilというオプションもあるのですが、これは単純にnilが入ってきても良いかという指定です。
Move code into helper
これはviewで書いてしまっているoption_tagが少し複雑になってしまっているので、それだったらhelperに移動しようという警告です。
<%= select_tag('favorite_color',
options_for_select([
['赤', red],
['青', blue],
['黄', yellow],
['緑', green]]) %>
<%= select_tag('favorite_color', options_for_select_color) %>
# app/helpers/color_helper.rb
def select_color_tag
options_for_select([["赤", red],
["青", blue],
["黄", yellow],
["緑", green]])
end
みたいな感じで、移行するのをおすすめされます。デフォルトだと、optionが3つ以上だと警告を示します。ただこれは、複雑になる場合を想定しているので、あくまで「optionの中身をhelperに見に行かなければいけない」というところに注意する必要があるでしょう。そのoptionの指定が複数ある場合は、統一する必要があるかもしれませんが、単一であるならば、viewのみで書いておくのが良いのではないかと思います。
Remove empty helpers
何も使ってないhelperは削除しなさい！ということですね。削除しましょう。
また、rails generate controller hogehogeとかでhelperファイルが生成するのを嫌がるのであれば、config.generators.helper = falseをconfig/application.rbに追加してあげると、生成されずに済みます。
Replace Complex Creation with Factory Method
本来Modelがやるべき処理をControllerでやってしまっているとこういう警告が出るでしょう。
悪い例がこちらです。
class VotesController < ApplicationController
def create
@vote = Vote.new
@vote.user_id = @user.id
@vote.reason = params[:reason]
@vote.type = 0
@vote.public_flag = params[:public_flag]
@vote.save
end
end
これはとてもよくありません。なぜかController内で、モデルオブジェクトの生成から値のセットを行っております。データに関する処理はModelがやるべきですね。
class VotesController < ApplicationController
def create
@vote = Vote.new_by_user(params, @user.id)
@vote.save
end
end
class Vote < ActiveRecord::Base
def self.new_by_user(data, user_id)
vote = self.new
vote.user_id = user_id
vote.reason = data[:reason]
vote.type = 0
vote.public_flag = data[:public_flag]
vote
end
end
こういった形の方がよいかもしれません。またnewを呼び出してその後にメソッドを何度も呼び出しているようなコードになってしまっているので、newの引数として値を入れてしまうかattributesメソッドを使用してやるかが良いと思います。
class Vote < ActiveRecord::Base
def self.new_by_user(data, user_id)
self.new(user_id: user_id,
reason: data[:reason],
type: 0,
public_flag: data[:public_flag])
end
end
# もしくは
class Vote < ActiveRecord::Base
def self.new_by_user(data, user_id)
vote = self.new
vote.attributes = {user_id: user_id,
reason: data[:reason],
type: 0,
public_flag: data[:public_flag]}
end
end
Use query attribute
これはnilか""か[]のときにtrueを返すblank?やfalseを返すpresent?メソッドを使用していると出る警告です。ただし、ActiveRecordを通して取得してきたメソッドに対してのみです。例えば、このようなかたちにします。
# before
<% if @user.email.present? %>
<p><%= @user.email %></p>
<% end %>
# after
<% if @user.email? %>
<p><%= @user.email %></p>
<% end %>
すっきりしますが、「値が入ってるかどうか」を知りたければ、明示的にpresent?などを書いてあげる方が親切かもしれないと個人的には思います。
ただquery attributeを使用すると、0もfalseになるらしく、0か1で判定しているテーブルには使うと良いかもしれません。
Simplify render in views/controllers
レンダリングメソッドがあるのですが、シンプルに書きましょうということです。
# before
<%= render :partial => "header" %>
<%= render :partial => 'posts/post', :object => @post %>
<%= render :partial => 'post', :collection => @posts %>
<%= render :partial => 'comments/comment', :locals => { :parent => post } %>
# after
<%= render "header" %>
<%= render @post %>
<%= render @posts %>
<%= render 'comments/comment', :parent => post %>
また、例はViewで使用するときのものですが、controllerにも当てはまるような内容なので、controllerでrenderメソッドを使うときも省略しましょうということですね。
Restrict auto-generated routes
使ってないroutingの設定を省きましょうということです。
resoucesを使用して、7種類のrouting(index, new, create, show, edit, update, destroy)が生成されますが、もしもどれか使用していない場合は、config/route.rbの中で除外してしまいましょう。except: ["edit", "show"]のようにして指定してあげましょう。
また7種類なので4個以上除外してしまうような場合は、only: ["create", "destroy"]といったかたちで、使用するものだけを指定してあげるのがよいでしょう。
class VotesController < ApplicationController
def index; end
def new; end
def create; end
# def show; end
# def edit; end
# def update; end
def destroy; end
end
Rails.application.routes.draw do
resources 'users', :except => [:show, :edit, :destroy]
end
まとめ
あくまでRailsでの良い書き方を指摘してくれるGemですので、プロダクトを作る上で便利かどうかはわかりませんが、チームでの開発を行う中でこのようなGemを導入しておくと、コードに統一感が出る他、余計なものを残さずに済むのでよいのではないでしょうか。
