【Rails】『コードレビューで学ぶ Ruby on Rails』を読んで自分のWebアプリをコードレビューしてみた

はじめに

お疲れ様です！
おおくまです！

今回は、「『コードレビューで学ぶ Ruby on Rails』を読んで自分のWebアプリをコードレビューしてみた」ということで、自分なりにコードレビューについてまとめてみました！

少しでも皆様の参考になりますと幸いです！

対象読者

• Ruby on Railsを学習中の方
• コードレビューについて学びたい方

注意点

• 内容に誤りがある場合があります！
• コメント等で教えていただけると幸甚です！

『コードレビューで学ぶ Ruby on Rails』とは？

『コードレビューで学ぶ Ruby on Rails』とは、技術書典で販売されている、株式会社ソニックガーデンの中谷 一郎さん、田中 義人さん、祖浦 雅人さんが執筆された、Ruby on Railsを使ったWebアプリケーション開発においてより良い書き方を提案する技術書です！
とても分かりやすくコードレビューについてまとめられており、初学者の私にもとても読みやすかったです！

https://techbookfest.org/product/wsrsXmV6u0q7hB5F0yt49C?productVariantID=e1UcMVjnwJUx2hy8nxMZTt

コードレビューするWebアプリ

私は、2023年11月に未経験からWebエンジニアとして転職しました！
転職活動をするにあたって、Ruby on RailsでWebアプリを作成したので、こちらをコードレビューしていきたいと思います！

https://github.com/kumaryoya/zoo_mania

コードレビュー

一般ユーザーと管理者ユーザーはモデルを分ける

app/models/user.rb

class User < ApplicationRecord
  authenticates_with_sorcery!

  has_many :favorite_zoos, dependent: :destroy
  has_many :zoos, through: :favorite_zoos
  has_many :posts, dependent: :destroy
  has_many :likes, dependent: :destroy

  validates :name, presence: true
  validates :email,presence: true, uniqueness: true
  validates :password, length: { minimum: 3 }, if: -> { new_record? || changes[:crypted_password] }
  validates :password, confirmation: true, if: -> { new_record? || changes[:crypted_password] }
  validates :password_confirmation, presence: true, if: -> { new_record? || changes[:crypted_password] }
  validates :reset_password_token, uniqueness: true, allow_nil: true

  enum role: { general: 0, admin: 1}

  mount_uploader :avatar, AvatarUploader
end

app/controllers/admin/base_controller.rb

class Admin::BaseController < ApplicationController
  before_action :require_login
  before_action :check_admin
  layout 'admin/layouts/application'

  private

  def not_authenticated
    redirect_to admin_login_path, alert: t('message.require_login')
  end

  def check_admin
    return if current_user.admin?

    redirect_to root_path, alert: t('message.no_authority')
  end
end

これは、ユーザーのモデルファイルと、ユーザーが管理者ユーザーかどうか判定するロジックが書いてあるコントローラファイルです！

私は、enumで、ユーザーモデルのroleカラムを使って、一般ユーザーと管理者ユーザーを識別していました！
しかし、この技術書によると2つの理由から、一般ユーザーと管理者ユーザーは、モデルを分けた方が良いとのことです！

1. 判定ロジックの誤りによるセキュリティ問題を引き起こしやすい
   同じモデルにする場合、特定のユーザーを管理者ユーザーとして扱う判定ロジックが必要です！
   しかし、もし判定ロジックに誤りがあると、一般ユーザーが管理者ユーザとして判定され、セキュリティ上の事故に繋がる可能性があります！
   なので、enumなど管理者フラグで管理している場合、判定ロジック周りは、注意深くコードを記述する必要があります！

2. 一般ユーザーと管理者では要件が異なることが多い
   一般ユーザーと管理者ユーザーでは、必要な情報が違うことが多いです！
   一般ユーザーと管理者ユーザーの要件が異なってくると、他方では使われないカラムやコードが多くなり、同じモデルで管理する意味がなくなっていきます！

以上の理由から、一般ユーザーと管理者ユーザーはモデルを分ける方が安全性が高く、コードも必要最小限に保てるため、推奨されているみたいです！

必ず値が入るべきカラムには、データベースに NOT NULL 制約をつける

db/migrate/20230709092658_create_favorite_zoos.rb

class CreateFavoriteZoos < ActiveRecord::Migration[7.0]
  def change
    create_table :favorite_zoos do |t|
      t.belongs_to :user, index: true
      t.belongs_to :zoo, index: true
      t.integer :rank
      t.timestamps
    end
  end
end

app/models/favorite_zoo.rb

class FavoriteZoo < ApplicationRecord
  belongs_to :user
  belongs_to :zoo

  validates :rank, presence: true, numericality: { only_integer: true, greater_than_or_equal_to: 1, less_than_or_equal_to: 3 }
  validate :validate_rank_uniqueness
  validate :validate_zoo_uniqueness

  private

  def validate_rank_uniqueness
    return unless rank.present? && user_id.present?
    existing_records = FavoriteZoo.where(user_id: user_id, rank: rank)
    errors.add(:rank, "はすでに登録されています") if existing_records.exists?
  end

  def validate_zoo_uniqueness
    return unless zoo_id.present? && user_id.present?
    existing_records = FavoriteZoo.where(user_id: user_id, zoo_id: zoo_id)
    errors.add(:zoo_id, "すでに登録されています") if existing_records.exists?
  end
end

これは、ユーザーがお気に入りの動物園トップ3を登録するための中間テーブルのマイグレーションファイルとモデルファイルです！

rankカラムは、1~3のどれかの値が必ず入るはずなので、モデルファイルには、presence: trueという記述があるのですが、マイグレーションファイルの方には、null: falseという記述がないため、データベース側では、NULLを許容してしまっています！
Railsのバリデーションは、アプリケーションレベルでの制約ですが、NOT NULL 制約は更に低レイヤーなデータベースレベルの制約な為、より信頼性があります！
バリデーションは、スキップできてしまうため、値を必ず入れる必要があるカラムには、データベース側でも制限をかけましょう！

テストのdescribe/context/itを上手く使い分ける

spec/models/user_spec.rb

require 'rails_helper'

RSpec.describe User, type: :model do
  it "名前とメールアドレスとパスワードがあれば登録できる" do
    expect(FactoryBot.build(:user)).to be_valid
  end

  it "名前がなければ登録できない" do
    expect(FactoryBot.build(:user, name: "")).to be_invalid
  end

  it "メールアドレスがなければ登録できない" do
    expect(FactoryBot.build(:user, email: "")).to be_invalid
  end

  it "メールアドレスが重複していたら登録できない" do
    user = FactoryBot.create(:user)
    expect(FactoryBot.build(:user, email: user.email)).to be_invalid
  end

  it "パスワードがなければ登録できない" do
    expect(FactoryBot.build(:user, password: "")).to be_invalid
  end

  it "パスワード確認がなければ登録できない" do
    expect(FactoryBot.build(:user, password_confirmation: "")).to be_invalid
  end

  it "パスワードとパスワード確認が一致しなければ登録できない" do
    expect(FactoryBot.build(:user, password_confirmation: "password_confirmation")).to be_invalid
  end
end

これは、実際に私が書いたユーザーのモデルスペックですが、この技術書を読んでから、改めてこのテストコードを見てみるとかなりヒドいことが分かります。笑
この技術書では、describe/context/itの使い分けを以下のように解説していました！

• describe

  • テスト対象を説明する
  • クラス名、メソッド名、機能名をつけることが多い

• context

  • 状況や条件を説明する
  • 「～～の場合」などの表現をすることが多い

• it

  • 期待する動作・結果を説明する
  • 「～～であること」や「～～できること」などで終わるような表現をすることが多い

これを踏まえて、私の書いたモデルスペックを一部書き直してみました！

spec/models/user_spec.rb

require 'rails_helper'

RSpec.describe User, type: :model do
  context "名前、メールアドレス、パスワードがある場合" do
    it "ユーザー登録できること" do
      expect(FactoryBot.build(:user)).to be_valid
    end
  end

  context "名前がない場合"
    it "ユーザー登録できないこと" do
      expect(FactoryBot.build(:user, name: "")).to be_invalid
    end
  end
end

こちらの方が、テストの条件や期待する挙動が分かりやすいですね！
これからは、このように書いていきたいと思います！

さいごに

『コードレビューで学ぶ Ruby on Rails』を読んで、自分のWebアプリを3点、指摘してみました！
今回、私のWebアプリから指摘したもの以外に、たくさん学びになることがこの技術書にはありました！
今後、コード書く際は、意識していきたいと思います！
最後まで読んでいただき、ありがとうございました！