RubyonRails:最近頂いたコードレビューを整理してみました。

みなさん、こんばんは！まどぎわです(・∀・)
最近、ソースコードレビューを受ける機会があったので、指摘頂いた内容を復習も兼ねて備忘目的で下記に整理してみました。

また私が作ったRailsアプリケーション環境関連は下記の通りです。

category	version
ruby	2.4.3
rails	5.1.4
mysql	14.14

また、viewのテンプレートエンジンにはhamlを使用しています。

レビュー内容まとめ

Model

migrationとmodelのvalidationは合わせる

modelで必須とした項目は、migrationファイルでもnull: falseとして、DBでもNOTNULL制約を設定するとDBでも必須チェックを行うため安心です(・∀・)

# model
validates :name, presence: true

# migration
# before
t.string :name
# after
t.string :name, null: false

validationの検証の実行条件はifオプションを活用する

validationメソッドには、エラーとなる条件のみの記載し、ifオプション側にvalidationメソッドの実行条件を記載することで、validationメソッドが汎用的になり、可読性も上がりますφ(..)

# before
validate :birthday_cannot_be_in_the_future
def birthday_cannot_be_in_the_future
    if birthday.present? && birthday.future?
    errors.add(:birthday, "can not specify your future date as your birth date.")
  end
end

# after
validate :birthday_cannot_be_in_the_future, if: -> { birthday.present? }
def birthday_cannot_be_in_the_future
  if birthday.future?
    errors.add(:birthday, "can not specify your future date as your birth date.")
  end
end

Model内でしか活用しないメソッドはprivateにする

インスタンス生成後に呼ばれないメソッド等はprivateに定義した方が、不用意に呼ばれたりすることがなくなり良いです(・∀・)

# before
validate :birthday_cannot_be_in_the_future, if: -> { birthday.present? }
def birthday_cannot_be_in_the_future
  if birthday.future?
    errors.add(:birthday, "can not specify your future date as your birth date.")
  end
end

# after
validate :birthday_cannot_be_in_the_future, if: -> { birthday.present? }

private

def birthday_cannot_be_in_the_future
  if birthday.future?
    errors.add(:birthday, "can not specify your future date as your birth date.")
  end
end

Controller

インスタンスの生成処理がシンプルな場合はbeforeアクションに記載せず各アクションに記述する

scaffoldで生成されるコードでは、before_actionでインスタンス変数を設定するようなコードになっていますが、findしてインスタンス変数を設定するぐらいであれば、各アクションに記述した方が可読性が良くなることが多いようですφ(..)

# before
before_action :set_user, only: [:show, :edit, :update, :destroy]

def show
end

private

def set_user
  @user = User.find(params[:id])
end

# after

def show
  @user = User.find(params[:id])
end

Controllerのbefore_actionにおける インスタンス変数セットについて

あればfind無ければnewする場合は、find_or_initializeメソッドを使用する

特定の値で検索し、無ければその値でインスタンス変数を生成する場合は、find_or_initializeメソッドを使うとスッキリかけますφ(..)

  # before
  def set_profile
    @profile = Profile.find_by(user_id: params[:id])
    @profile ||= Profile.new
  end

  # after
  def set_profile
    @profile = Profile.find_or_initialize_by(user_id: params[:id])
  end

※findする条件とインスタンスを生成する際の値が違う場合は、検索条件を引数にwhereを呼び出し、インスタンス生成時のプロパティを引数にfirst_or_initializeを呼び出すと良いです。

  # before
  def set_profile_user
    @profile = Profile.find_by(id: params[:id])
    @profile ||= Profile.new(profile_params)
    @user = User.find(@profile.user_id)
  end
  
  #after
  def set_profile_user
    @profile = Profile.where(id: params[:id])
                      .first_or_initialize(profile_params)
    @user = User.find(@profile.user_id)
  end

first_or_initializeとfind_or_initialize_by - Qiita

View

省略出来る{}は削除する

シンプルなことですが、意外とやりがちだと思うので、注意したいですねφ(..)

- # before
= render 'form',{ user: @user, next_action: 'create' }

- # after
= render 'form', user: @user, next_action: 'create'

機能単位にpartialを切りだす

viewファイルは機能単位にpartialを切り出してあげた方が、application.html.erbは特にモリモリになりがちなので良いです。
また、ファイルが分割されることにより、可読性があがるだけでなくファイル競合の防止等のメリットもあるかと思いました(・∀・)

- # before
  %body
    %header
      - unless current_user
        = link_to "Login", login_path
        = link_to "Signup", signup_path
      - else
        = link_to "Profile", current_user
        = link_to "Logout", logout_path, method: :delete
        
- # after
  %body
    %header
      = render 'layouts/navbar'

TestCode

登録成否は、change matcherを使用する

change matcherだけでなく、色々なmatcherを使って綺麗なテストコードを書けるように意識したいです。

# before
let(:user_before_cnt){ User.count }
context "User#create時にエラーが発生した場合" do
  before do
    post signup_path, params: invalid_user_param
  end
  it "ユーザーが登録されないこと" do
    expect(User.count).to eq user_before_cnt
  end
end

[使えるRSpec入門・その2「使用頻度の高いマッチャを使いこなす」 - Qiita](https://qiita.com/jnchito/items/2e79a1abe7cd8214caa5)

# after
context "User#create時にエラーが発生した場合" do
  it "ユーザーが登録されないこと" do
    expect {
      post signup_path, params: invalid_user_param
    }.to_not change(User, :count)
  end
end

メソッド実行結果のtrue/false判定はbe_methodnameを使用する

メソッドの返り値のtrue/falseを検証する場合は、eqではなくbe_methodnameを使用した方が綺麗にかけますφ(..)

# before
it "ログイン状態となること" do
  post signup_path, params: valid_user_param
  expect(!!current_user).to eq true
end

# after
it "ログイン状態となること" do
  post signup_path, params: valid_user_param
  expect(current_user).not_to be_blank
end

インスタンス変数ではなく、letを使う

letは、遅延評価(使用される時に変数が生成される)ため、実行負荷の軽減が見込めるようですφ(..)
他にもメリットがいくつかあるようです、下に参考のリンクを記載しておきました！

# before
@user = FactoryBot.create(:valid_user)

# after
let!(:user){ FactoryBot.create(:valid_user) }

RSpecのletを使うのはどんなときか？（翻訳） - Qiita

factoryの共通部分はtraitでまとめる

traitを使うとfactory(テストデータ)の共通部分をまとめることが出来ます。
でもなんとなくfactoryを入れ子にするのは、まだ少し違和感がある・・・(._.)

# before
FactoryBot.define do
  factory :user do
    name 'Example User'
    email "User@example.com"
    password 'foobar'
    password_confirmation 'foobar'
  end
  factory :valid_user_with_career, class: User do
    name 'Example User'
    email 'User_with_career@example.com'
    password 'foobar'
    password_confirmation 'foobar'
    after :create do |user|
      create(:vallid_career, user: user)
    end
  end
end

# after
FactoryBot.define do
  factory :user_difine do
    trait :valid do
      name 'Example User'
      email "User@example.com"
      password 'foobar'
      password_confirmation 'foobar'
    end
    trait :with_profile do
      after :create do |user|
        create(:profile, user: user)
      end
    end
    factory :valid_user, class: User, traits: [:valid]
    factory :valid_user_with_profile, class: User, traits: [:valid,:with_profile]
  end
end

FactoryGirlのtransientとtraitを活用する - Qiita

1度しか使わないようなデータは、factoryに定義せずテストコードで生成する

1度しか使わないのもは、factoryに書くよりもテストコードに直接書いた方が、factoryがスッキリし、テストコード上に意図が現れるのでわかりやすいですね(・∀・)

# before
FactoryBot.define do
  factory :no_name_user, class: User do
    name ''
    email 'user@example.com'
    password 'foobar'
    password_confirmation 'foobar'
  end
end
RSpec.describe User, type: :model do
  describe 'Userのモデリングに関するテスト' do
    it 'nameが必須項目となっていること' do
      user = FactoryBot.build(:no_name_user)
      user.name = ''
      expect(user.valid?).to eq false
    end
  end
end

# after
RSpec.describe User, type: :model do
  describe 'Userのモデリングに関するテスト' do
    let(:user){ FactoryBot.build(:user) }
    it 'nameが必須項目となっていること' do
      user = FactoryBot.build(:no_name_user)
      user.name = ''
      expect(user.valid?).to eq false
    end
  end
end

おわりに

個人開発だけだと自分が書いているコードが良い感じなのかが良くわからないので、ソースコードレビューは、本当に学びが多いですね・・・！
これからもレビュー内容を定期的に整理して、きちんと身につけられるようにしたいですφ(..)