みなさん、こんばんは！まどぎわです(・∀・)
最近、ソースコードレビューを受ける機会があったので、指摘頂いた内容を復習も兼ねて備忘目的で下記に整理してみました。
また私が作ったRailsアプリケーション環境関連は下記の通りです。
また、viewのテンプレートエンジンにはhamlを使用しています。
レビュー内容まとめ
Model
migrationとmodelのvalidationは合わせる
modelで必須とした項目は、migrationファイルでもnull: falseとして、DBでもNOTNULL制約を設定するとDBでも必須チェックを行うため安心です(・∀・)
validates :name, presence: true
t.string :name
t.string :name, null: false
validationの検証の実行条件はifオプションを活用する
validationメソッドには、エラーとなる条件のみの記載し、ifオプション側にvalidationメソッドの実行条件を記載することで、validationメソッドが汎用的になり、可読性も上がりますφ(..)
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
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に定義した方が、不用意に呼ばれたりすることがなくなり良いです(・∀・)
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
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_action :set_user, only: [:show, :edit, :update, :destroy]
def show
end
private
def set_user
@user = User.find(params[:id])
end
def show
@user = User.find(params[:id])
end
Controllerのbefore_actionにおける インスタンス変数セットについて
あればfind無ければnewする場合は、find_or_initializeメソッドを使用する
特定の値で検索し、無ければその値でインスタンス変数を生成する場合は、find_or_initializeメソッドを使うとスッキリかけますφ(..)
def set_profile
@profile = Profile.find_by(user_id: params[:id])
@profile ||= Profile.new
end
def set_profile
@profile = Profile.find_or_initialize_by(user_id: params[:id])
end
※findする条件とインスタンスを生成する際の値が違う場合は、検索条件を引数にwhereを呼び出し、インスタンス生成時のプロパティを引数にfirst_or_initializeを呼び出すと良いです。
def set_profile_user
@profile = Profile.find_by(id: params[:id])
@profile ||= Profile.new(profile_params)
@user = User.find(@profile.user_id)
end
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
省略出来る{}は削除する
シンプルなことですが、意外とやりがちだと思うので、注意したいですねφ(..)
-
= render 'form',{ user: @user, next_action: 'create' }
-
= render 'form', user: @user, next_action: 'create'
機能単位にpartialを切りだす
viewファイルは機能単位にpartialを切り出してあげた方が、application.html.erbは特にモリモリになりがちなので良いです。
また、ファイルが分割されることにより、可読性があがるだけでなくファイル競合の防止等のメリットもあるかと思いました(・∀・)
-
%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
-
%body
%header
= render 'layouts/navbar'
TestCode
登録成否は、change matcherを使用する
change matcherだけでなく、色々なmatcherを使って綺麗なテストコードを書けるように意識したいです。
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)
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を使用した方が綺麗にかけますφ(..)
it "ログイン状態となること" do
post signup_path, params: valid_user_param
expect(!!current_user).to eq true
end
it "ログイン状態となること" do
post signup_path, params: valid_user_param
expect(current_user).not_to be_blank
end
インスタンス変数ではなく、letを使う
letは、遅延評価(使用される時に変数が生成される)ため、実行負荷の軽減が見込めるようですφ(..)
他にもメリットがいくつかあるようです、下に参考のリンクを記載しておきました！
@user = FactoryBot.create(:valid_user)
let!(:user){ FactoryBot.create(:valid_user) }
RSpecのletを使うのはどんなときか？（翻訳） - Qiita
factoryの共通部分はtraitでまとめる
traitを使うとfactory(テストデータ)の共通部分をまとめることが出来ます。
でもなんとなくfactoryを入れ子にするのは、まだ少し違和感がある・・・(._.)
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
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がスッキリし、テストコード上に意図が現れるのでわかりやすいですね(・∀・)
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
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
おわりに
個人開発だけだと自分が書いているコードが良い感じなのかが良くわからないので、ソースコードレビューは、本当に学びが多いですね・・・！
これからもレビュー内容を定期的に整理して、きちんと身につけられるようにしたいですφ(..)
