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

2023/09/12に公開

https://qiita.com/medora/items/86a6bc1771a64f2bc1de
こちらのページの移転です。

ブール値を返す関数、もしくは変数で、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つと増えたり、&&, || で複雑になり始めたら、条件を変数に分割することを一考に入れてみてもよいでしょう。

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

参考文献

https://buildersbox.corp-sansan.com/entry/2020/05/29/110000#:~:text=られます。-,事例%3A 複雑な条件式,-function isPubliclyValid()
こちらで書かれていたレビュー指摘点の記述も参考にさせていただきました。

https://www.oreilly.co.jp/books/9784873115658/
リーダブルコード

GitHubで編集を提案

Discussion