STORES 予約のリファクタリング(前編)

STORES 予約で Webアプリケーションエンジニアをしています。ykpythemindです。

STORES 予約では機能開発する際に積極的にリファクタリングを行っています。今回は実際の例からその手法を前後編に分けて紹介します。想定読者はWebアプリケーション開発をしているすべての方を対象にしています。

リファクタリング、いつやる？

リファクタリングとは、「外部から見た挙動は変えずに、プログラムの内部構造を整理すること」です。システムを開発、運用していく上でコードの変更容易性は重要ですので、リファクタリングはアプリケーションエンジニアの仕事の1つだと考えています。

とはいえ、事業のための機能開発との兼ね合いがあるので、STORES 予約では「プロダクト開発の質とスピードが最大化するのであればリファクタリングを選択する」ことにして、通常のSprint backlogにタスクとして積むことが多いです。*1

例えば単に設定のトグルをオンオフするような画面のリファクタリングをしてもプロダクトの質がほとんど向上せず、有効なリファクタリングとは言えないでしょう。後述のような予約の支払い方法選択部分はほぼすべての予約者が使う機能でプロダクトの改修が入りやすい、かつ少し複雑度があるものなので、質と開発スピードがとても重要になります。

支払い方法関係のリファクタリング

本記事では、支払い方法検証のAPI部分 ( Ruby on Rails製 )を例にして、実際に行ったリファクタリングの手法を紹介します。これは先日リリースされたクーポン機能の実装に先立って実施されたものです。なお、リファクタリングの前にはテストを書いておきましょう。今回は支払い部分の実装ということで厚めにテストが書かれている部分でした。

STORES 予約では予約ページという単位でエンドユーザーから予約を受け付けることができます。予約ページごとにオーナーさま(事業者)が支払い方法を設定でき、予約者がその中から自由に選択して予約できます。

予約者が特定の回数券を支払い方法として選択して次に進んだときや、予約の確定時にその情報をPOSTし、実際に使用可能な支払い方法かどうかの検証が行われます。

以下がリファクタリング前のコードです。Grape gem で作成しているエンドポイントで、読者理解のためにコメントをつけ、簡略化しています。

# ユーザーが選択した支払い方法を検証するエンドポイントです。
post '/payment/validate' do
  # 以下のような構造がpostされてきます
  # { method: '支払い方法名', その他のフィールドは支払い方法ごとに異なる... } 
  payment_method_params = params[:input][:payment_method]

  # 月謝/回数券で支払うときはログインが必須
  user =
    if payment_method_params[:method] == 'subscription' ||
         payment_method_params[:method] == 'ticket'
      current_user!
    end

  # すべての処理をValidation classに委譲
  payment_method_validator =
    ::Reservations::PaymentMethodValidator.new(
      @reservation,
      params: payment_method_params,
      user: user,
    )

  # 選択した支払い方法が正しくなければ構造化したエラーレスポンスを返す。
  unless payment_method_validator.valid?
    error!(payment_method_validator.payment_method_error, 400)
  end

  status :ok
end

# 支払い方法を選択した状態で予約を確定するエンドポイントです。
post '/finalize' do
  payment_method_params = params[:input][:payment_method]

  # 支払い方法が設定されている予約ページで、支払い方法パラメータが送られてこなかったらだめなのでエラーにする
  if @booking_page.require_payment? && payment_method_params.blank?
    error!({ success: false, errors: { payment_method: 'required' } }, 400)
  end

  # クレジットカード支払いの場合のハンドリング. ctokはStripeから発行された一時的な決済トークン
  if payment_method_params && payment_method_params[:method] == 'credit_card'
    if current_user.present? && current_user!.credit_card_of_user.blank? &&
         payment_method_params[:ctok].blank?
      render_400
    end
    if !payment_method_params[:restore_credit_card] && payment_method_params[:ctok].blank?
      render_400
    end
  end

  # 月謝/回数券で支払うときはログインが必須
  user =
    if payment_method_params[:method] == 'subscription' ||
         payment_method_params[:method] == 'ticket'
      current_user!
    end

  # このエンドポイントでも同様に支払い方法パラメータの検証を行う
  payment_method_validator =
    ::Reservations::PaymentMethodValidator.new(
      @reservation,
      params: payment_method_params,
      user: user,
    )

  unless payment_method_validator.valid?
    error!(payment_method_validator.payment_method_error, 400)
  end

  # -- 中略 (予約処理等) --

  # 支払い方法が設定されているページでは支払いを実施し、レコードを作成する
  if @booking_page.require_payment?
    update_payment! @reservation, params[:input][:payment_method]
  end
end

# 予約に対して支払いを実施し、結果を記録します
def update_payment!(reservation, params = {})
  payment_service = Reservation::PaymentService.new reservation.merchant

  # 支払い方法によって分岐
  case params[:method].to_sym
  when :subscription
    subscription =
      current_user!.end_user_customer_subscriptions.find_by(
        public_id: params[:customer_subscription_id],
      )
    payment_service.pay_with_subscription!(reservation, subscription)

  when :ticket
    product = Product.find_by(public_id: params[:product_id])
    ticket_book_ids =
      current_user!.end_user_ticket_books.where(product: product).available.pluck(:id)

    payment_service.pay_with_tickets!(reservation, current_user!.end_user_tickets.valid.where(ticket_book_id: ticket_book_ids)

  when :credit_card
    payment_service.pay_with_credit_card!(reservation, remote_ip, params[:ctok], credit_card)
  end
end

処理を半分以上簡略化していますが、いくつかの問題のあるコードでした。

パラメータのハッシュオブジェクトを引き回していたり、パラメータの文字列比較で分岐していて、可読性が悪く支払い方法の種類ごとに必要なパラメータがわかりにくい。
支払い方法ごとに分岐する箇所が多い。 PaymentService#pay_with_tickets! (回数券支払い) / PaymentService#pay_with_credit_card! (クレカ支払い) のような具体的な名前のメソッドを呼んでおり、PaymentServiceがなんでもできてしまう神クラスになりかけている

処理をシーケンス図にすると以下のようになっています。

それでは段階的にリファクタリングを実施していきます。

まずは現状把握のために、支払い方法の種類ごとにリクエストのパラメータをクラス化しましょう。 *3

class PaymentRequest
  # params is request parameter hash
  def initialize(params)
    # 各支払いリクエストクラスにマッピングする
    @request =
      case params
      in method: 'subscription'
        Subscription.new(customer_subscription_id: params[:customer_subscription_id])
      in method: 'ticket'
        Ticket.new(product_id: params[:product_id])
      in method: 'credit_card'
        CreditCard.new(ctok: params[:ctok], restore_credit_card: params[:restore_credit_card])
      in method: 'onsite'
        Onsite.new
      in nil
        Noop.new
      else
        raise ArgumentError.new("invalid PaymentRequest parameter #{params.inspect}")
      end
  end

  # メソッド呼び出しをすべて 支払いリクエストのインスタンスに委譲します
  delegate_missing_to :@request
end

以後はストラテジパターン *4 で実装していくことになります。各支払い方法が選択されたときのパラメータを支払いリクエストのクラスに入れていきます。

クレジットカード払いの場合は、決済トークンと、ユーザーが登録したクレカを再度使用するかどうかのフラグを受け付けて内部に保持しておきます。実装しているメソッドについては後述します。

class PaymentRequest
  # クレジットカード払いの場合の支払いリクエスト。
  class CreditCard < Base
    def initialize(ctok:, restore_credit_card:)
      @ctok = ctok # ctokはStripeから発行された一時的な決済トークン
      @restore_credit_card = restore_credit_card
      super()
    end

    def need_user_login?
      false
    end

    def noop?
      false
    end
    
    # 一旦外部からタイプでハンドリングできるように定義するが、後々具体的なtypeではハンドリングできないようにします
    def type
      :credit_card
    end
    
    attr_reader :ctok, :restore_credit_card
    # ...

月謝払いの場合は以下のような形です。予約者が持っている月謝のIDをパラメータとして受け付けます。*5

ユーザーのログインが必須かどうかを支払い方法ごとに判断しているので、支払いリクエストのインターフェースとしてneed_user_login?メソッドを定義させることにしましょう。月謝と回数券の支払い方法クラスの場合はこのメソッドはtrueを返すようにし、それ以外のクラスではログイン必須ではないためfalseを返すよう実装します。

class PaymentRequest
  class Subscription < Base
    def initialize(customer_subscription_id:)
      @customer_subscription_id = customer_subscription_id
      super()
    end

    # 月謝を使うときはログイン必須です。
    def need_user_login?
      true
    end

    def noop?
      false
    end

    def type
      :subscription
    end
    # ...

事業者は予約者に、支払い方法を選ばずとも予約させる設定も可能です。現状ではパラメータのハッシュオブジェクトが存在するかnilかどうかで判断していたのですが、nilの扱いが面倒なため、支払い方法を選ばなかった という支払いリクエストのクラスを作成します。*6

class PaymentRequest
  class Noop < Base
    def need_user_login?
      false
    end

    def noop?
      true
    end
    
    def type
      :noop
    end
    # ...

それでは、実際のリクエストのハンドリングするコントローラで作成したクラスを使うように置き換えていきましょう。

diff --git a/r.rb b/r2.rb
index d724985..fb70541 100644
--- a/r.rb
+++ b/r2.rb
@@ -1,19 +1,15 @@
 # ユーザーが選択した支払い方法を検証するエンドポイントです。
 post '/payment/validate' do
-  payment_method_params = params[:input][:payment_method]
+  payment_request = PaymentRequest.new(params[:input][:payment_method])
 
-  # 月謝/回数券で支払うときはログインが必須
-  user =
-    if payment_method_params[:method] == 'subscription' ||
-         payment_method_params[:method] == 'ticket'
-      current_user!
-    end
+  # 特定の支払い方法の場合はログインが必須
+  user = current_user! if payment_request.need_user_login?
 
   # すべての処理をValidation classに委譲
   payment_method_validator =
     ::Reservations::PaymentMethodValidator.new(
       @reservation,
-      params: payment_method_params,
+      payment_request: payment_request,
       user: user,
     )
 
@@ -27,36 +23,32 @@ end
 
 # 支払い方法を選択した状態で予約を確定するエンドポイントです。
 post '/finalize' do
-  payment_method_params = params[:input][:payment_method]
+  payment_request = PaymentRequest.new(params[:input][:payment_method])
 
-  # 支払い方法が設定されている予約ページで、支払い方法パラメータが送られてこなかったらだめなのでエラーにする
-  if @booking_page.require_payment? && payment_method_params.blank?
+  # 支払い方法が設定されている予約ページにもかかわらず支払いを実行しない場合はエラーにする
+  if @booking_page.require_payment? && payment_request.noop?
     error!({ success: false, errors: { payment_method: 'required' } }, 400)
   end
 
   # クレジットカード支払いの場合のハンドリング. ctokはStripeから発行された一時的な決済トークン
-  if payment_method_params && payment_method_params[:method] == 'credit_card'
-    if current_user.present? && current_user!.credit_card_of_user.blank? &&
-         payment_method_params[:ctok].blank?
+  if payment_request.type == :credit_card
+    if current_user.present? && current_user!.credit_card_of_user.blank? && payment_request.ctok.blank?
       render_400
     end
-    if !payment_method_params[:restore_credit_card] && payment_method_params[:ctok].blank?
+    # 一旦paramsを直接参照しないようにする
+    if !payment_request.restore_credit_card && payment_request.ctok.blank?
       render_400
     end
   end
 
-  # 月謝/回数券で支払うときはログインが必須
-  user =
-    if payment_method_params[:method] == 'subscription' ||
-         payment_method_params[:method] == 'ticket'
-      current_user!
-    end
+  # 特定の支払い方法の場合はログインが必須
+  user = current_user! if payment_request.need_user_login?
 
   # このエンドポイントでも同様に支払い方法パラメータの検証を行う
   payment_method_validator =
     ::Reservations::PaymentMethodValidator.new(
       @reservation,
-      params: payment_method_params,
+      payment_request: payment_request,
       user: user,
     )
 
@@ -68,32 +60,31 @@ post '/finalize' do
 
   # 支払い方法が設定されているページでは支払いを実施し、レコードを作成する
   if @booking_page.require_payment?
-    update_payment! @reservation, params[:input][:payment_method]
+    update_payment! @reservation, payment_request
   end
 end
 
 # 予約に対して支払いを実施し、結果を記録します
-def update_payment!(reservation, params = {})
+def update_payment!(reservation, payment_request)
   payment_service = Reservation::PaymentService.new reservation.merchant
 
   # 支払い方法によって分岐
-  case params[:method].to_sym
+  case payment_request.type
   when :subscription
     subscription =
       current_user!.end_user_customer_subscriptions.find_by(
-        public_id: params[:customer_subscription_id],
+        public_id: payment_request.customer_subscription_id,
       )
     payment_service.pay_with_subscription!(reservation, subscription)
 
   when :ticket
-    product = Product.find_by(public_id: params[:product_id])
+    product = Product.find_by(public_id: payment_request.product_id)
     ticket_book_ids =
       current_user!.end_user_ticket_books.where(product: product).available.pluck(:id)
 
     payment_service.pay_with_tickets!(reservation, current_user!.end_user_tickets.valid.where(ticket_book_id: ticket_book_ids)
 
   when :credit_card
-    payment_service.pay_with_credit_card!(reservation, remote_ip, params[:ctok], credit_card)
+    payment_service.pay_with_credit_card!(reservation, remote_ip, payment_request.ctok, credit_card)
   end
 end

ここまでで、パラメータのハッシュオブジェクトを直接触ることがなくなったのと、一部支払い方法のタイプによって分岐していた部分(ユーザーのログインを要求するかどうか等)をクラス側に閉じ込めることができました。 *7

変更ごとにこまめにCIを回して既存の振る舞いを壊していないか確認しましょう。変更量が多くなるので、一旦 PaymentRequest#type を実装して、支払い方法の具体的な内容によって分岐するコードはそのまま残しています。このあたりでPull Requestにして一度デプロイを実施しました。

前編まとめ

いかがでしたでしょうか。段階的にリファクタリングを実施するため、まずはクラスをつくりオブジェクトに名前を付けるところから初めました。段階的に組み替えていくことで方針が見えることが多いので、まず手を動かしてみています。

次回はポリモーフィズムを用いて分岐部分を取り除きます。

(お知らせ) ヘイ株式会社では絶賛採用活動中です。ご興味を持たれた方はぜひご応募ください。

*1: つまり、直近開発しない機能を積極的にリファクタリングするわけではないのが現状ではあります。また、時間をかけたとしても2,3日程度で終わるものしか行いません。

*2: Grape gemは予約を確定するエンドポイント群で採用されています。ちなみに本コードではGrapeのDSLの機能でパラメータの型や項目のチェックは行われており、マスアサインメント脆弱性への対応がされている状態です。

*3: Martin Fowler著『リファクタリング』第２版にある『レコードのカプセル化』『オブジェクトによるプリミティブの置き換え』の手法

*4: https://ja.wikipedia.org/wiki/Strategy_%E3%83%91%E3%82%BF%E3%83%BC%E3%83%B3

*5: ユーザーが実際に所持している月謝IDであるか別途検証が必要です。

*6: 『リファクタリング第２版』では特殊ケースの導入/Null Objectパターンとして紹介されています

*7: update_payment!メソッドの引数がパラメータのハッシュオブジェクトではなく PaymentRequest に変わっている部分は『リファクタリング』第２版の『パラメータオブジェクトの導入』を行うことができました。

STORES Product Blog

こだわりを持ったお商売を支える「STORES」のテクノロジー部門のメンバーによるブログです。

STORES 予約のリファクタリング(前編)

リファクタリング、いつやる？

支払い方法関係のリファクタリング

前編まとめ