ブール値を返す関数、もしくは変数で、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=%E3%82%89%E3%82%8C%E3%81%BE%E3%81%99%E3%80%82-,%E4%BA%8B%E4%BE%8B%3A%20%E8%A4%87%E9%9B%91%E3%81%AA%E6%9D%A1%E4%BB%B6%E5%BC%8F,-function%20isPubliclyValid()
こちらで書かれていたレビュー指摘点の記述も参考にさせていただきました。