✨

should_send_email という変数名は適切か？

2023/09/12に公開

ブール値を返す関数、もしくは変数で、is_xxx, should_xxxなどの命名が使われることがあります。
しかし、とあるコードを見た時にモヤっと感じたので、命名に関する自分の考えをまとめたいと思います。

この記事は、Rubyを使ってサンプルコードを書いていますが、固有の機能は極力使わず、言語一般レベルの議論ができればと思います。
(※Rubyにはメソッド名に「?」を使うことができ、慣習的にブール値を返します。この記法だけ今回は使用します。)

変数が何の情報も与えていない場合

具体的には、下記のようなコードです。

should_send_email = user.subscribe_mail_magazine? && mail_magazine
if should_send_email
  send_email(user.email, mail_magazine.content)
end

テキトウに読み下すと、「emailを送るべきとき（ifの条件）、emailを送る(ifのブロック)」と読めると思いますが、この変数は果たして必要なのでしょうか？

emailを送るべき場合にemailを送る、というのは当たり前過ぎて、冗長に感じます。
条件の部分が何も情報を加えていないからです。

これならば最初から、変数を定義せずに下記のように書いたほうが、「ユーザーがメルマガを登録していて」かつmail_magazinが空でない時、メールを送る、と読むことができ、条件＋実行部分の役割分担がしっかりとした構造になりスマートではないでしょうか。

if user.subscribe_mail_magazine? && mail_magazine
  send_email(user.email, mail_magazine.content)
end

では、いくつかの条件（ブール値）の組み合わせを変数にまとめるのは無意味なのでしょうか？

条件を変数でまとめるほうが良い場合

当然ですが、業務コードは次第に書き足されていくものなので、以下のように条件が増えていくかもしれません。

if user.subscribe_mail_magazine? && mail_magazine && user.accept_genre(mail_magazine.genre) && user.last_sign_in_at > Date.today - 30
  send_email(user.email, mail_magazine.content)
end

このように数珠つなぎでどんどん条件が付け足されていくと、意味のまとまりが掴みづらくなります。

ユーザーがメルマガを許容しているかどうか、ということを一つの変数名として、条件式を分割してみます。

user_allow_mail_magazine = user.subscribe_mail_magazine? && mail_magazine && user.accept_genre(mail_magazine.genre)
if user_allow_mail_magazine && user.last_sign_in_at > Date.today - 30
  send_email(user.email, mail_magazine.content)
end

(Rubyユーザーならぼっち演算子を使うべきという気持ちになりますが、言語一般レベルのお話がしたいので、あえて使ってません。)

今後、メールマガジンの受け取りに関して設定項目が増えた場合は
user_allow_mail_magazine変数に条件式を付け足し、その他は if文の末尾に付け足せば良い、ということが明示できるようになります。

これは関数分割にも似ていると思います。

上記のコードではまだ、本当に変数に条件を分割すべきなのか？というのがまだ納得できないかもしれせん。
以下はどうでしょうか？

if !user.deleted_at && user.last_sign_in_at > Date.today - 60 \
  && (user.subscribe_mail_magazine || user.accept_genre(mail_magazine.genre)) \
  && !mail_magazine.content && mail_magazine.status == 'ongoing'
  send_email(user.email, mail_magazine.content)
end

どこに意味のまとまりがあるかわかりません。また、||と&&が複雑に用いられていてメンテナンス性も非常に悪いです。

意味のまとまりから、以下のように改善できます。
今後、例えばユーザーがアクティブかどうか？という条件に何か追加されたとしても、容易に追加できます。

user_active = !user.deleted_at && user.last_sign_in_at > Date.today - 60
user_allow_mail_magazine = user.subscribe_mail_magazine || user.accept_genre(mail_magazine.genre)
mail_magazine_active = !mail_magazine.content && mail_magazine.status == 'ongoing'

if user_active && user_allow_mail_magazine && mail_magazine_active
  send_email(user.email, mail_magazine.content)
end

ここまで読んでいただければわかるかと思いますが、should_send_emailという変数名では変数名が意味する役割が大きすぎて、条件式の分割が難しいということがわかるかと思います。

※ただし、条件式を再利用する場合、またはshouldが新しく何か情報を加える場合など、以下のように有効な場合もあります。(こういう設計が良いかどうかとは別として)

should_send_email = user.subscribe_mail_magazine? && mail_magazine

if should_send_email
  send_email(user.email, mail_magazine.content)
end

...

# emailを送るべき場合は、メール送信者リストにも追加する
if should_send_email
  mailed_list.push(user)
end

これはshould_send_emailの条件式が何か追加された場合でも対応できます。
また、自明でない情報を追加しているということで、shouldが意味を持っています。

まとめ

条件が3つ、4つと増えたり、&&, || で複雑になり始めたら、条件を変数に分割することを一考に入れてみてもよいでしょう。

また、まだまだ浅学の身ですので、これは変数名が適切でない、こういう場合は変数に分割しないほうが良い、などなど何か意見ございましたら、教えていただけると幸いです。

参考文献

こちらで書かれていたレビュー指摘点の記述も参考にさせていただきました。

リーダブルコード

GitHubで編集を提案

変数が何の情報も与えていない場合

条件を変数でまとめるほうが良い場合

まとめ

参考文献

Discussion