【2022/11/21 ~ 2022/11/25】コードレビューで知ったことをざっくりまとめてみた

この記事を書く背景

前職を退職したときに、コードレビューされた内容を思い出せなくて、もったいないなと感じました。今回はそのようなことをしないように、コードレビューを受けて印象に残ったことを書いていこうと思います。復習の意味も込めてます。指摘事項に関しては、ジャンルごとに分けて書いていこうと思います。

Open API

Open APIでエンドポイントを定義する場合、タグを付与する

タグは、各エンドポイントをタグで分類する際に必要です。タグの付与は必須だそうです。あまりよくわかっていませんが、タグを付与しないと、yamlファイルで定義したAPIにリクエストを出せないそうです。あまりよくわかっていないので、次使うときに調べます。

(※) タグは単数系かつキャメルケースで書きます。しかし、職場によってルールが違うかもしれません。

一対多のエンドポイントにはidを加える

一対多のエンドポイントにはidを加えます

↓ 例

/events/{id}/logs

対象のスキーマが一つしかない場合、allofは不要

そもそもallofは、モデル定義を組み合わせたり拡張(プロパティの上書きや追加)して1つのスキーマを作成したい場合に使用します。対象のスキーマが一つしかない場合、allofは使わなくて大丈夫です。

(※) スキーマとは、ざっくり言うとひとまとまりのデータ定義のことです。OpenAPIの文脈では、レスポンスの返却内容の定義を指すこともあります。

operationIdがそのまま自動生成されるAPIコード上でのメソッド名になるので、使いにくいメソッド名になっていたら変更する

特にoperationIdに「id」文字列が入っていたら、削除します。

OpenAPIでdate型を使う場合、format: dateを指定する

Stoplight上でformat: dateとexampleを定義します。

  date:
    type: string
    format: date
    example: '2022-01-01'

nullを許容する場合、nullable: trueを追加する

OpenAPI3.1より前の場合、nullが型としてサポートされていないです。そのため、nullを型として使いたい場合、nullable: trueを追加します。 Stoplight上ではnullable: trueを追加できないので、yamlファイルを直に編集して追加します。jsonファイルが必要な場合は、編集後のyamlファイルからエクスポートします。

  reserve_date:
    type: string
    format: date
    example: '2022-01-01'
    nullable: true

Rails

フロントエンドとバックエンドが別のアプリの場合、どちらにもバリデーションをかける

フロントエンドとバックエンドが別のアプリの場合、どちらにもバリデーションをかけます。

シリアライザー内で外部キーのハッシュIDを生成する際、encode_idメソッドを使用する

hashid-rails gemはDBから取得した整数のidをハッシュ化するために使用します。外部キーなどのidをハッシュ化したい場合、その外部キーに対応するモデルに対して、encode_idメソッドを実行します。

class EventSerializer < ApplicationSerializer
  # JSONで出力したい属性をここに書く
  # reserve_dateはrender json: で返却するオブジェクトの属性を参照する
  # id、user_idの場合、メソッドを定義しているので、そちらが優勢して呼ばれる
  attribute :id
  attribute :reserve_date
  attribute :user_id

  # 省略

  def id
    object.hashid
  end

  def user_id
    User.encode_id(object.user_id)
  end
end

(※) hashid-rails gemを導入しても、DB側ではidは整数として保持されます。その点がuuidとの違いです。ハッシュidはuuidより短いので、扱いやすいです。

(注) コントローラでrender json:を使用すると、返却するオブジェクトのシリアライザーをRailsが検索して、利用可能であればそれを利用します。そのため、render json:の部分にわざわざシリアライザーを指定しなくても大丈夫です。

(※) シリアライザー内にattributeで属性を定義することで、シリアライザーはrender jsonで指定したオブジェクトの属性を検索します。しかし、シリアライザーはオブジェクトの属性を検索する前に、その属性の名前を持つメソッドが存在しないかを確認します。これにより、値をメソッドで修正したり、オリジナルのプロパティを追加したりできます。上の例の場合、attribute :idでidメソッド、attribute :user_idでuser_idメソッドが先に呼ばれています。

hashid-rails/rails_spec.rb at e0e2982583fc32a635d1fd5da937588fb04a8e03 · jcypret/hashid-rails · GitHub

エラーハンドリングは大元のApplicationControllerに書く

rescue_fromで例外時に呼び出すメソッドを指定します。このrescue_fromと例外時に呼び出すメソッドは全てのコントローラが継承しているコントローラ(ApplicationController)に書きます。そうすることで、同じ例外処理を何度も書く必要がなくなります。

module API
  class ApplicationController < ActionController::API
    rescue_from ActiveRecord::RecordInvalid, with: :error_record_invalid

    # @param [ActiveRecord::RecordInvalid] e
    def error_record_invalid(e)
      # 省略
      render json: #省略, status: :bad_request
    end
  end
end

Rubyは型のある言語ではないので、可能な限りYARDによるドキュメントの記載をする

Rubyの場合、引数や戻り値がどんな型なのか不明なので、書いた方が親切です。以下の場合、YARDによるドキュメントを書くことで、引数の型が明確になります。また、エディタの補完機能でeと入力すると、関連するメソッドが表示されるようになります。基本的にはメソッドに対してドキュメントを書きます。

  # @param [ActiveRecord::RecordInvalid] e
  def error_record_invalid(e)
    # 省略
    render json: #省略, status: :bad_request
  end

(注) コードを見て明らかに分かる部分は、コメントとして書かなくても大丈夫です。

eachメソッド内でクエリメソッドを実行して同じものを取得しない

例えばユーザーが複数のイベントを登録する場合を考えます。イベントを登録するユーザーは常に同じなので、以下のような書き方をすると、ループごとにユーザーの取得をしてしまい、処理的にあまり良くないです。そのため、取得結果が常に同じ部分に関しては、ループ外で実行します。

↓ before

permitted.each do |event|
  event[:user_id] = User.find(params[:user_id]).id
  event[:place_id] = Place.find(event[:place_id]).id
end

↓ after

user_id = User.find(params[:user_id]).id
permitted.each do |event|
  event[:user_id] = user_id
  event[:place_id] = Place.find(event[:place_id]).id
end

適切な場所に書かないことが、ファットコントローラやファットモデルの原因になる。再利用しないメソッドを定義するとこれらを引き起こす可能性があるので、それなら愚直に書く

save_allなどの独自メソッドをモデルに定義すると、コントローラはスッキリしますが、モデルを見に行かないとどんな処理をしているのか分かりません。また、それを繰り返すとモデルがファットになります。そもそもメソッドを定義する目的は、処理を一つのメソッドとしてまとめて、処理を複数の場所で呼び出せるようにすることです。そのため、複数の場所でメソッドを呼び出す予定がない場合、独自メソッドを定義せず、愚直に書いた方が良いです。

複数のコントローラで独自メソッドを使用する場合、モデルに定義します。あるコントローラ内の複数のアクションで使用する場合、そのコントローラ内にプライベートメソッドとして定義します。

一連の処理のどこかで例外が発生する可能性がある場合、それらの処理をトランザクションとして定義する

トランザクションとは、 分割不可な一連の処理のことです。 一連の処理をトランザクションとして定義したい場合、それらの処理を ApplicationRecord.transaction メソッドで囲みます。transactionブロック内で例外が発生した場合、transactionブロック内で行われたデータベース更新処理を全てロールバックすることができます。そのため、Railsでトランザクションを機能させるためには、transactionメソッド内で例外を発生させる必要があります。

      def create
        permitted = create_params
        events = []
        ApplicationRecord.transaction do
          permitted.each do |event_params|
            event = Event.new(event_params)
            event.save!
            events.push(event)
          end
        end

        render json: # 省略, status: :created
      end

(注) 上の例でtransactionメソッドを使用しない場合、例外が発生する以前にeachメソッド内で登録されたデータをロールバックすることができません。つまり、例外発生前にeachメソッド内で登録されたデータを登録しなかったことにすることができないので、中途半端にデータがDBに登録された状態になってしまいます。また、フロント側には例外処理した結果が返されるので、ユーザーがもう一度データを登録しようと再リクエストした場合、DBの整合性が保たれていなくて、データが保存できなくなる可能性があります。もしくは、同じデータが重複して登録される可能性があります。そのため、 複数のデータをまとめて登録する等の「分割不可な一連の処理」は、トランザクションとして定義した方が良いです。

error400というメソッドの命名がメソッド内の処理を表していない場合、別の命名を考える

error400 と言いつつ、RecordInvalid 専用の処理になっている場合、error_record_invalid のような処理内容をちゃんと表す命名に修正します。

module API
  class ApplicationController < ActionController::API
    rescue_from ActiveRecord::RecordInvalid, with: :error_record_invalid

    # @param [ActiveRecord::RecordInvalid] e
    def error_record_invalid(e)
      # 省略
      render json: #省略, status: :bad_request
    end
  end
end

201 createdのレスポンスボディには、基本的に新しく生成したリソースを入れる

201 createdのレスポンスボディには、基本的に新しく生成したリソースを入れます。

可能な限り括弧を省略するのは避ける

括弧を省略する/しないは、個人の感覚に依存するので、可能な限り括弧を省略するのは避けましょう。特に引数の入ったメソッドを呼び出す場合、括弧を省略しないようにします。

↓ before

end_date.after? start_date

↓ after

end_date.after?(start_date)

括弧を省略した方が明らかに良い場合、括弧を省略します。

引数のない関数呼び出し
empty? など
DSL拡張系の関数
belongs_to や validates など

メソッドチェーンでむやみにインデントを増やさない

メソッドチェーンの「.」を改行して揃えるのは良いですが、むやみにインデントを増やすと可読性が落ちるので、その場合は、増やさない方が良いです。

↓ before

        permitted = params.permit(
          events: [
            # 省略
          ],
        )
                          .to_h[:product_logs]

↓ after

        permitted = params.permit(
          events: [
            # 省略
          ],
        ).to_h[:product_logs]

日付の比較をするだけのバリデーションなら、ラムダを使うか、Rails7ならComparisonValidatorを使う

日付の比較をするバリデーションを定義するために独自メソッドを定義することがありますが、日付の比較などの簡単な比較の場合、validationsメソッドが用意しているヘルパーを使用した方が良いです。簡単な要件のバリデーションを実装するのに、独自メソッドを定義するのはやりすぎです。Rails7の場合、ComparisonValidatorを使うとスッキリ書くことができます。以下の例では、「予約日が今日以降の日付ならOK。それ以外なら例外を発生させる」というバリデーションを定義しています。

↓ before

  validate :reserve_date_cannot_be_in_the_past

  private

  def reserve_date_cannot_be_in_the_past
    if !reserve_date.present?
      errors.add(:reserve_date, message: "cannot be in the past") if reserve_date.before?(Date.today)
    end
  end

↓ after(予約日が今日以降の日付ならOK。それ以外なら例外を発生させる)

 validates :reserve_date, comparison: { greater_than_or_equal_to: proc { Date.today } }, allow_nil: true

バリデーションのエラーメッセージはe.messageで取得できる

module API
  class ApplicationController < ActionController::API
    rescue_from ActiveRecord::RecordInvalid, with: :error_record_invalid

    # @param [ActiveRecord::RecordInvalid] e
    def error_record_invalid(e)
      # 省略
      e.message # => Validation failed: Date must be less than or equal to 2022-11-25
      render json: #省略, status: :bad_request
    end
  end
end

テスト(Rspec)

リクエストスペックのリクエストボディにidを入れる場合、ハッシュIDを渡す

(注) hashid-railsを使用する前提で話しています。

なるべく実際にユーザーが使う状況と同じ状況を再現した方が良いので、リクエストスペックのリクエストボディにidを入れる場合、ハッシュidに変換して渡します。

    let(:id) { create(:event).hashid }

過去、未来、現在の日付ごとにテストを作成する

時制の日付の違いで異なるテスト結果が得られる場合、それぞれに対応するテストケースを作成した方が良いです。

ちなみにFaker gemで過去、未来、現在の日付は以下のように作ります。

  # 現在
  let(:date) { Date.today.strftime("%Y-%m-%d") }

  # 過去
  let(:date) { Faker::Date.between(from: 5.days.ago, to: Date.today - 1).strftime("%Y-%m-%d") }

  # 未来
  let(:date) { Faker::Date.between(from: Date.today.next_day, to: 5.days.from_now).strftime("%Y-%m-%d") }

２つのデータを登録するテストケースで2つとも同じデータを使用しない。その際、Factoryを使わずに愚直にテストを書く

1つのデータを登録するテストケースは基本のテストケースです。1つのデータは一番外側のスコープに定義します。２つのデータを登録するテストケースでは、実際の状況を再現するために、2つとも別のデータを使用します。以下のように、2個目のデータはベタ書きします。 テストではDRYが推奨されておらず、愚直に書くぐらいの方がわかりやすいので、テストを書くときは過度にDRYにしないよう気をつけましょう

    let(:params) do
      {
        events: [
          {
            reserve_date:,
            price:,
            # 省略
          },
        ],
      }
    end
    
    let(:price) { price: Faker::Number.number(digits: 5) }
    # 省略

    context "when reserve_date is present date" do
      let(:reserve_date) { Date.today.strftime("%Y-%m-%d") }

      context "one test data" do
        # 省略
      end

      context "two test data" do
        let(:params) do
          {
            events: [
              {
                reserve_date:,
                price:,
                # 省略
              },
              {
                date:,
                price: Faker::Number.number(digits: 5),
                # 省略
              },
            ],
          }
        end

        it "returns 201" do
          # 省略
        end
      end
    end

(注) FactoryBotを使ってリクエストデータを一気に生成することもできますが、FactoryBotの本来の使い方は、モデルのインスタンスを用意するためであり、リクエストデータを作る目的で開発されたわけではありません。また、定義したFactoryを見ないとどんなデータがリクエストで使われているか不明です。そのため、リクエストでデータを渡す場合、愚直にletで書いた方が良いです。その方がテストの可読性が上がります。

テストケースをそこまで書いていない場合、shared_examplesはspec全体の見通しが悪くなるので、使わない

テストケースをそこまで書いていない場合、shared_examplesは、spec全体の見通しが悪くなるので、使わないようにします。それぞれのcontext内のテストケースがどういうレスポンスであるべきなのかの見通しが悪くなってしまうので、愚直に書いたほうがいいです。先ほどのテストでは過度なDRYを避けるに通じます。

contextの説明を具体的にする

contextは、テストの内容を「状況・状態」のバリエーションごとに分類するために利用します。contextには、具体的に状況や状態を書きましょう。以下の場合、 何がpresentなのか不明なので、具体的に書いた方が可読性が上がるので良いです。

↓ before

context "present" do

↓ after

context "when date is present date" do

むやみにテストケースを増やすようなデータの持ち方をしない

例として2つのデータを登録するテストケースを考えます。データにはexpiry_date属性があり、この属性はnilを許容するとします。expiry_date属性がnilではない同じような2つのデータを登録するテストケースを作成した場合、expiry_date属性がnilの時のテストケースを作成する必要があります。むやみにテストケースを増やしたくないのと、expiry_date属性がnilでもレスポンス結果が変わらない(データが登録できる)ので、2つ目のデータのexpiry_date属性をnilにします。そうすることで、expiry_date属性がnilの時のテストケースを作成せずにnilの時の検証ができます。

      context "two test data" do
        let(:params) do
          {
            events: [
              {
                date:,
                price:,
                expiry_date:,
                # 省略
              },
              {
                date:,
                price: Faker::Number.number(digits: 5),
                expiry_date: nil,
                # 省略
              },
            ],
          }
        end

レスポンスが変化しない場合、テストケースを増やさなくて良い

日付が現在の場合で、2つのデータが登録できるなら、日付が過去の場合で2つのデータが登録できることを検証しなくても大丈夫です。テストケースが増えるのを避けたいのと、既に検証済みのテストケースなので、あえて増やさなくても大丈夫です。

また、データを渡すと例外を返すテストケースの場合、データが1個でも2個でも同じレスポンスを返すので、データが2個以上の時のテストケースをわざわざ作成する必要はないです。

同じようなテストを再度する必要はない(重要)。

テストケースが網羅されることは重要ですが、数が増えすぎると見通しが悪くなり、テストのメンテナンスコストが上がってしまい、生産性を下げてしまいます。そのため、テストケースは「必要最低限」を心がけます。

感想

転職してバックエエンドのコードレビューを初めて受けたのですが、自分の分かっていない部分が明確になって良かったです。Rails7、OpenAPI、テストがもっとできるようになりたい、、とりあえずテストに関しては、「テストケースを無駄に増やしすぎずかつ網羅する、DRYに書かずに愚直に書く」というのが知れて良かったです。また、Railsに関しても、「どのタイミングでメソッドを定義するか、トランザクションはどうやって定義するか」を知ることができて良かったです。ちなみにComparisonValidatorはRails7の公式ドキュメントから見つけたので、「やっぱり公式ドキュメントって偉大だな、まず最初に見るべき情報だな」と感じました。

どういう基準でテストケースを作成するか、OpenAPIが具体的に分かっていないので、実務を通して理解を深めていこうと思います。

あとTDDでAPIを実装したのですが、TDDだとすごく実装しやすかったので、引き続きAPIを実装するときはTDDで実装しようと思います。