何をしたか
コントローラーのアクション内の処理に関する 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リクエストなどを送るメソッドをコントローラーのプライベートメソッドに記述するのが最適なのか?」
といった疑問があがりました。
「モデルのメソッドとして管理する方法はどうなのか?」
「アフィリエイト専用のクラスを用意するのがいいのか?」
といった、方法も考えつきました。
これらをどのように判断するべきか、判断力がまだ備わっていないことを痛感したため、
参考文献を繰り返し読み込み、判断力を養っていこうと思います。
また、何かベストプラクティスがある場合ご教授いただけると幸いです。