はじめに
ここ数年保守を担当しており、色々な人の書いたコードを目にします。
最近以下のようなコードを目にしました。
public void sample() {
// 何かしら処理
if (checkCommon(dto)) {
// 何かしら処理
if (checkDate(date)) {
// 何かしら処理
}
}
}
(前後の処理をぼかしているのもありますが)ifの中で何をチェックしているのか、check
の意味が広く曖昧であることによって理解できませんでした。
これを例に、なぜ曖昧な単語を使うことがよくないのか、どのように修正すればよいのか、個人的な意見を書きます。
どのようにチェックしているのかわからない
細かい理由はいくつかありますが、個人的にはこれが大きすぎると感じています。
前述のとおりcheck
の意味が広すぎて曖昧であることによって、関数の意味も曖昧になってしまいます。故にこの章のタイトルの通りどのようにチェックしているのかわからない状態となってしまいます。
1つ目の例は少々特殊で、common
も意味が広くて曖昧なので何をチェックしたいのかすらわかりません。
ここでやりたかったことは以下でした。
dtoにはステータスを持っており、どのステータスの場合でも行う必須項目に値が入っていることのチェック
チェック対象はdtoのフィールド変数の内3つだけ
要は全ステータスで共通の必須項目をチェック、ということみたいです。checkCommon
からは読み取れないですね。
2つ目の例は日付に対して何かのチェックすることしかわかりません。
フォーマットのチェックするのかもしれないですし、システム日付と比較をするかもしれません。
仮にスラッシュフォーマットチェックをしていたとしても、この名前からではスラッシュ有りと無しのどちらがtrue
なのかわかりません。
ここでやりたかったことは以下でした。
dateがスラッシュ付フォーマットになっていることをチェック
なんとなーく予測できていましたが処理内容を確認するまでハッキリとはしませんね。
改修
名前にcheck
は必要でしょうか?
今回の例のようなbool値が返る場合は処理自体が何かをチェックしたり、比較したりする性質を持っています。そこに対してわざわざcheck
と名付けなくとも十分に意味が通じるはずです。
これを踏まえて、前章でほぼほぼ記載済みではありますが改めてチェックしたい内容を記載します。
<1つ目>
全ステータス共通で必須項目に値が入っていること
チェック対象はdtoのフィールド変数の内3つだけ
<2つ目>
dateがスラッシュ付フォーマットになっていること
これから、私としては以下のどちらかの実装をします。
public void sample() {
// 何かしら処理
if (presentRequiredField1AndField2AndField3InAllStatus(dto)) {
// 何かしら処理
if (hasSlash(date)) {
// 何かしら処理
}
}
}
public void sample() {
// 何かしら処理
// 対象はField1とField2とField3
if (presentRequiredFieldsInAllStatus(dto)) {
// 何かしら処理
if (hasSlash(date)) {
// 何かしら処理
}
}
}
presentRequiredField1AndField2AndField3InAllStatus
とpresentRequiredFieldsInAllStatus
を使うパターンを記載しました。前者は確認項目が少なく、今後増えることがない時に定義する想定です。後者は確認項目が多い時や、今後項目が増える可能性がある時に定義する想定です。
類似語
check
のみを取り上げましたが、他にも同じ性質を持った単語があります。
これらの使用は避けるようにしましょう。
judge
、compare
、common
※思い付き次第随時更新していきます。
(他にご存知の方がいらっしゃいましたら教えていただきたいです!!)
おわりに
この手の単語はそれらしい名前になるので安易に使われがちですが、他者が見ると何をやっているのかわからないことが多い印象です。私自身過去に使っていた記憶があります(この記事を書くにあたって引っ張りだそうと思ったのですが見つけられませんでした…)。今思えば目の前の作業を終わらせることでいっぱいになっていて、保守の人に流れ着くことやそのコードから自分のレベル感が評価されることまで全く考えられていませんでした。この記事を通して、少しでもそういったところまで考えてもらえたら嬉しいです。
ちょっとした内容ですが、参考になりましたら幸いです。
追記
コメントにて以下ご指摘をいただいたので追記します。
例えば論理的にチェックする、登録する、リソースを解放する、という一連の処理がある場合、物理的にもcheck()、save()、release()で表現されている方が見通しが良くなるのではないでしょうか。
仰る通りで、記事内での考慮が漏れておりました。
本記事、私の考えとしてこのような用途を否定するものではありません。