【2023/1/29 ~ 2023/6/30】コードレビューで知ったことをざっくりまとめてみた part5

目次

Rails

退会機能のエンドポイントは、put /users/me/cancel_membershipsではなくて、destory /users/meのがシンプル

ここら辺はAPIのルールとかによるかも

serviceの（executeのような）実行トリガーには引数を渡すことは想定としてない

serviceの（executeのような）実行トリガーには引数を渡すことは想定としてない。serviceは時として複雑な前提条件が必要な場合が多く、それをexecuteの引数のみで受け取ることは非現実的である。引数がハッシュ化した場合、型を担保できない。型を使えない代わりにキーワード引数のようなものを使えばまだマシではあるが、serviceの実行に必要な前提条件があるのであれば、それは適切なsetterを用意して、そちらである程度の値の担保をした方が良い。

サイト名が重複してる場合、手前の文言は 退会希望を受け付けました とだけにしておく

退会理由だけを手入力するのはめんどくさくて、まともなデータが収集されない

退会理由だけを手入力するのはめんどくさくて、まともなデータが収集されない。enumで簡単に選択できるものと、詳しくて入力できるものを2つ置くのが常套手段。

カラム名がcancel_membership_atだと、名前が長すぎるので、deactivated_atにしたい

退会する場合、emailにはdeactivated- のようなprefixがついていると分かりやすい

email: "deactivated-#{SecureRandom.uuid}@soctoc.jp",

退会する場合、マスク/削除処理はsidekiq側で処理する。

退会する場合、マスク/削除処理は - 一度書き換えると取り返しがつかない - データ量次第ではAPIがタイムアウトする可能性がある などの理由から、ある程度間をおいて非同期で処理した方が良い。 退会処理から1日間はマスク/削除実行の猶予を持って、sidekiq側で処理するように調整する（それまでは復旧可能時間になる）

「退会」という状態を「論理削除」で表現したくない、statusで取り扱う。

ユーザーと退会操作をした時に選択した値のログは一対多の関係性にする

退会操作が一度しかできないのは、「退会」という操作の特殊性からくる「結果」みたいのもので、それはデータ構造とは切り離して考えた方が良い。データ的には「退会操作をした時に選択した値のログ」なので、これは has_many が適切である。 （たまたま「退会」という操作はユーザー自身は1度しか行わないというだけで、開発時に何度も退会操作をやり直したりする際にこのlogも普通に積み上がる）

つまり、ここを一対一にした場合、開発時に退会操作を何回もしたときに毎回ログを消さないとデータの整合性が保証できないので、辛い。データ的には操作をした時のログなので、has_manyが適切。

enumの各値の命名はもっと雑で良くて短い命名にする

before

                        - can_not_find_item_i_wanted
                        - delivery_area_did_not_include_my_home

after

- not_found_wanted
- not_covered_area

registrationsコントローラ は「登録」に関する処理で、createで登録が作成されdestroyで登録が削除されると考えるのは自然

逆にcancel_membershipコントローラーであれば、destroyメソッドではなくcreateであった方が良い。

ユーザーの作成をUsersコントローラのcreateアクションにしないで、Users::Registrationsコントローラのcreateアクションにした理由は、deviseを使う場合の通例に従ったため。

退会時、電話番号はnull更新をする

これはプロジェクトにもよるかな。

Jobではperformメソッドの引数のユーザーが存在しない場合、そもそものdesrializeに失敗するので present? の判定は不要。

ただ、退会後気持ちが変わり期限までに復旧操作が行われた場合、退会状態ではないアカウントがこの処理に届く可能性があるので、退会ユーザーかどうかのチェックが必要。

beforen

class User::CancelMembershipJob < ApplicationJob
  # @params [Stock] stock
  def perform(user)
    return unless user.present?

after

class User::CancelMembershipJob < ApplicationJob
  # @params [Stock] stock
  def perform(user)
    return unless user.deactivated?

リクエストに cancel_membership_log として投げるの、「あまりにも内部情報を露出しすぎてる」という感じがする

cancel_membership_logをcreateするAPIならこれでも良いが、これは退会を受け付けるAPIでその副作用でログがつまれるという事でしかないので。あと、APIの内部構造を知っているから、cancel_membership_logで囲むということを知っている。内部構造を知らないと扱えないAPIは直感的に使えないので、普通に使いづらい。

before

                cancel_membership_log:
                  type: object
                  properties:
                    description:
                      type: string
                    reason:
                      type: string
                      enum:
                        - high_commissions
                        - can_not_find_item_i_wanted
                        - delivery_area_did_not_include_my_home
                        - no
                        - other
                  required:
                    - reason

after

                description:
                  type: string
                reason:
                  type: string
                  enum:
                    - high_commissions
                    - not_found_wanted
                    - not_covered_area
                    - no
                    - other
              required:
                - reason

destroyアクションの場合、create_paramsと命名するのではなくて、destroy_paramsにする

updateは戻り値は真偽値だけど、レシーバの値も自動で書き換えてくれる。なので、updateした後に、わざわざ更新後の値を取得して返す必要はない。レシーバが更新されているんだから、それを参照すれば良いだけ。

三人称単数現在は、その名前の通り「三人称」に対する英文のルールなので、自分自身の関心ごとに対する命名としては不適切。

before

class User
  def cancels_membership!
  end
end

after

def cancel_membership!
  def cancel_membership!
  end
end

退会日時やステータスなど、ユーザーに返す必要のない値を返さない。シリアライザーに書かない

自分がバンされてんだとか、わかってしまう為。

サービスを作る際に、同じリソースの要素を個別にsetter用意するのは煩雑過ぎる。リソースの要素が増えた場合、無限にsetterを増やさないといけなくなる。

before

        service.reason = destroy_params[:reason]
        service.description = destroy_params[:description]

after

APIとサービスの関係

APIとサービスは全然違うという感想を持つかもしれないが、

「一連の処理を単一のエンドポイント（サービスの場合はメソッド）を起点にしてひとまとめに処理してくれるもの」

という点では同一である。そして、「サービス」という名前が示す通り、命名的にはむしろサービスの方がその傾向が強い。 この場合の適切な見出しというのは「APIのリクエストキー」であり、「サービスの各種セッター」である。

他のエンジニアにとって関係ないどうでも良い情報を外部に露出しない

事前登録のデータは、退会受付と同時に即削除する

メール配信されてしまう可能性があるため+取り返しがつかない状態になっても影響が小さいデータであるため

Job内に実処理をメソッドで持つことはあんましない

perform内でやっている実際のマスク処理も、それ用のserviceを別途作った方が良い

before

// 後でMaskJobに変わる。マスクをするジョブなので。
// jobは可能な限り冪等性を担保した方が良い
class User::CancelMembershipJob < ApplicationJob
  # @params [Stock] stock
  def perform(user)
    return unless user.deactivated?

    destroy_pre_registration!(user)
    user.skip_reconfirmation!
    user.update!(
      first_name: "user",
      last_name: "deactivated",
      phone: nil,
      email: "deactivated-#{SecureRandom.uuid}@soctoc.jp",
    )
    user.user_default_address.destroy!
    user.user_default_payment_method.destroy!
    user.user_addresses.each(&:destroy!)
    user.user_payment_methods.each(&:destroy!)
    user.user_providers.each(&:destroy!)
  end

  def destroy_pre_registration!(user)
    pre_registration = PreRegistration.find_by_lower(user.email)
    pre_registration&.destroy!
  end
end

after

destroy_pre_registration!は退会処理に関するサービスにprivateメソッドとして定義すれば良い。

class User::CancelMembershipService
  attr_reader :user
  attr_accessor :reason, :description
  def initialize(user:)
    @user = user
  end
  # @param [String] cancellation_reason
  # @return [User]
  def execute!
    deactivated_user = ActiveRecord::Base.transaction do
      user.cancel_membership!
      destroy_pre_registration!(user)
      User::CancelMembershipJob.set(wait_until: Time.zone.now.tomorrow).perform_later(user)
      user.cancel_membership_logs.create!(reason:, description:)
      user
    end
    RegistrationMailer.cancel_membership(deactivated_user).deliver_later
    deactivated_user
  end

  private

  def destroy_pre_registration!(user)
    pre_registration = PreRegistration.find_by_lower_email(user.email)
    pre_registration&.destroy!
  end
end

マスクはマスク用のサービスを作る 退会の文脈とは関係なく「マスクを提供しているだけのサービス」なので、CancelMembership のprefixは必要ない。

after

class User::MaskService
  attr_reader :user,

  def initialize(user:)
    @user = user
  end

  def mask!
    ActiveRecord::Base.transaction do
      user.skip_reconfirmation!
      user.update!(
        first_name: "user",
        last_name: "deactivated",
        phone: nil,
        email: "deactivated-#{SecureRandom.uuid}@soctoc.jp",
      )
      user.user_default_address.destroy!
      user.user_default_payment_method.destroy!
      user.user_addresses.each(&:destroy!)
      user.user_payment_methods.each(&:destroy!)
      user.user_providers.each(&:destroy!)
      user.cart.destroy!
      user.user_tokens.each(&:destroy!)
    end
  end
end

「内部の実装を外部に露出しない」命名にするのが、ほとんど全ての実装に共通する理念

退会処理でいえば、 - 具体的な退会手続きを実行するための CancelMembershipServiceがあり - 必要な情報は対象のユーザーと、退会理由 という状況なので、このサービスはシンプルにこの二つを受け取るためのインターフェースを持てば良い。

「退会理由」をセットするにあたって、なぜか set_cancel_membership_log としてセットしている。 これは set_cancel_reason(reason:, description:) のようなインターフェースになっている方が良い。

なぜなら、退会情報がどこでどんな扱いを受けるかなんて、サービス利用者にとってどうでもいい事だからである。 加えて、退会にあたってその理由を明示する必要性があると言うのは利用者にとってもとても自然な事である。

プログラムにあたって「抽象化」は、最重要項目である。

こういうふうに、「内部でlogに保存する情報だからset_logをはやそう」みたいなインターフェース実装を「抽象度の低い実装」と呼び、利用者が利用しやすいように、「内部実装に依存しない自然なインターフェースを生やす」実装を「抽象度の高い実装」と呼ぶ。

抽象度の低い実装は、何か仕様変更が入った際に根本的な修正を広範囲に対して行う必要性が出る可能性が高くなることが多く、逆にうまく抽象化されていると変更に対する強さが自然と内包されることになります。 （実装依存のインターファースは、実装が変われば変わる弱いインターフェース。目的重視の抽象度の高い実装は、実装が変わっても変化しないことが多い強いインターフェース）

小文字のemailで事前登録した人を取得する場合、find_by_lower だけだと、一体何が lower なのかわからない。

find_by_lower_emailの方が適切。使う側の人も何によってfind処理がされるのかわかりやすい。

1サービス、1目的

「publicなメソッド」という点だけに関して言えば、サービス毎にそれぞれ必要なものが生やしていますが、生やされているpublicなメソッドには明確に複数の種類が存在している。

一つは、「サービスを提供するにあたって、必要な情報を受け取ったり整理したりするもの」で、いわゆる set_* や build 系のメソッドである。

もう一つは、「具体的な処理をトリガーするもの」で、主に execute! や calculate! みたいなメソッドで、副作用が実際に伴うことがほとんどである。 そして、ほとんどすべてのサービスで、トリガー形のメソッドは一つしか生やしていない。 （もちろん例外もある） これは「1サービス、1目的」という原則を課しているため。 なので、Mask用のサービスを作る。

Rspec

APIの副作用の検証をリクエストスペックで頑張ろうとしすぎない

このspecはあくまでもAPIの呼び出しに関する検証のみを行った方がよくて、次点でその直接的な副作用までの検証に止める。

今回のは、「APIの副作用としてマスク用のジョブが発行され、そのジョブの副作用でユーザーデータにマスクがかかる」という「副作用の副作用」の検証をここでやってしまっています。これは明らかにこのAPIの責務の範疇外ですよねー。

API呼び出しの検証と、その直接的な副作用の検証は良いが(User.findとか取得したデータを検証)、副作用の副作用を検証するのはやりすぎ。

ジョブに関するテストをしたい場合は、ジョブ用のテストを作る

フロントエンド