何をしたか

コントローラーのアクション内の処理に関する 6行の記述を、
処理内容がわかりやすいメソッド名にし、 1行にしました。
また、「どのように」から「何を」 を意識するようにリファクタリングしました。

どんなメリットがあるか

createアクションの可読性が上がる

具体的な内容

ApplyController#createでの処理です。
(Applyが単数系なのは、このコントローラがAPIからのリクエストを受け取るコントローラであり、
indexなどの複数のリソースを扱わないためです。)

Applyという応募情報を管理するテーブルがあり、createアクションは応募情報を作成する処理です。
Applyには下記のカラムが存在します。（便宜上、必要最低限のカラムのみとしています。）

メールアドレス...email
生年月日...birthday
A社のアフィリエイトから訪問した際にパラメータに含まれるID...a_id
B社のアフィリエイトから訪問した際にパラメータに含まれるID...b_id

作成処理(createアクション)では、
アフィリエイト経由の場合、
経由もとアフィリエイトへ、成果HTTPリクエストを送りたいという仕様でした。

また、アフィリエイトは2社利用されており、それぞれの送信の条件が異なりました。

a_idはA社へ、
b_idはB社へ送信します。

送信条件としては、
どちらも開発環境では送信させたくないため、環境での条件が科せられています。

A社に関しては、すでにUserという会員情報を管理するテーブルに、
同じメールアドレスでレコードが存在する場合は送信させたくないようです。

また、A社は20~50代の応募のみを対象としています。

class ApplyController < ApplicationController

  def create
    # 受け取ったパラメータでインスタンス化
    apply = Apply.new(apply_params)
    # Userという会員情報を管理するテーブルにすでに存在するメールアドレスの応募か確認している
    is_exists_user_email = User.exists?(email: apply.email)

    # 処理が続く...

    if apply.a_id.present? && !is_exists_user_email && apply.include_age?(range: 20..59)
        && (Rails.env.production? || Rails.env.pre?)
      Net::HTTP.get_print(URI.parse("https://a-company?a_id=#{apply.a_id}"))
    end

    if apply.b_id.present? && (Rails.env.production? || Rails.env.pre?)
      Net::HTTP.get_print(URI.parse("https://b-company?b_id=#{apply.b_id}"))
    end

    # 処理が続く...
  end

end

A社へ送信するリクエストの条件に利用しているApply#include_age?は
Applyのbirthdayカラムから年齢を算出し、Apply#include_age?のrangeキーワード引数に渡された範囲の年齢かを真偽値で返すメソッドです。

class Apply
  # 生年月日から年齢を返すメソッド
  def age_calculation; end

  # 年齢がRangeオブジェクトの範囲内かを真偽値で返すメソッド
  def include_age?(range:)
    range.include?(age_calculation)
  end
end

これらを踏まえてやりたかったこと

createアクションの可読性を上げる
「どのように」から「何を」を意識した設計にする

どのようにしたか

createアクション内では

どんな条件で
どこへ
何をするか

が全て指示されているような設計となっていました。
これは参考文献の4章でも取り上げられていた

犬を散歩へ連れ出すことを考える。
犬の足に直接「歩け」と命じるのはおかしい。
この場合、犬に対して命令し、自分の足の面倒は自分で見させるのが正しい方法。

という考え方を適用してリファクタリングすることにしました。

具体的には 「アフィリエイトへ報酬リクエストを送信する」 という
send_affiliateプライベートメソッドとしました。

またsend_affiliateでは、A社とB社で条件が重複している箇所は一箇所にまとめ、
早期リターンを活用することで、コードを全て読まなくても良いようになりました。

class HogeController < ApplicationController

  def create
    apply = Apply.new(apply_params)
    is_exists_user_email = User.exists?(email: apply.email)

    # 処理が続く...

    send_affiliate(apply, is_exists_user_email)

    # 処理が続く...
  end

  private

  def send_affiliate(apply, is_exists_user_email)
    return unless Rails.env.production? || Rails.env.pre?

    if apply.xuid.present? && !is_exists_user_email && apply.include_age?(range: 20..59)
      Net::HTTP.get_print(URI.parse("https://a-company?a_id=#{apply.a_id}"))
    end

    return if apply.cid.blank?

    Net::HTTP.get_print(URI.parse("https://b-company?b_id=#{apply.b_id}"))
  end
end

これにより、createアクション内のコードリーディング時には、
send_affiliateという単語から分かるように
「アフィリエイトへリクエストを送信しているんだな」ということがわかりやすくなりました。

今後

今回はパブリックインターフェースを単純にプライベートへ移しただけなのですが、
「そもそもHTTPリクエストなどを送るメソッドをコントローラーのプライベートメソッドに記述するのが最適なのか？」
といった疑問があがりました。

「モデルのメソッドとして管理する方法はどうなのか？」
「アフィリエイト専用のクラスを用意するのがいいのか？」
といった、方法も考えつきました。

これらをどのように判断するべきか、判断力がまだ備わっていないことを痛感したため、
参考文献を繰り返し読み込み、判断力を養っていこうと思います。

また、何かベストプラクティスがある場合ご教授いただけると幸いです。

参考文献

オブジェクト指向設計実践ガイド　～Rubyでわかる進化しつづける柔軟なアプリケーションの育て方