https://zenn.dev/mafafa/articles/d781c2dfdfc3a8720270
話題となっている『良いコード／悪いコードで学ぶ設計入門 ―保守しやすい 成長し続けるコードの書き方』 (出版社のページ) を読みました。
全体的には「うんうん、そうだよね」と同意できることが多かったです。 もちろん、初めて目にするような考え方, アイディア, テクニックもありました。 一方、気になったことやちょっと引っかかったこともありましたので、メモしておきます。 あくまでもメモなので結論のようなことはありません。
HitPoint クラスに isZero メソッドがあります。
「ヒットポイントがゼロであれば true」という仕様で、実装は次のようになっています。
「ゼロ」を表すフィールドの名前がなぜ ZERO ではなく MIN なのだろうかと気になりました。
値は 0 でなくてもかまわないのですが、仕様上「ヒットポイントがゼロ」であることを表すなら MIN より ZERO の方がよいのではないかと思います。
何が気になったのか少し考えてみたところ、「仕様上ヒットポイントがゼロであるというのは、 amount フィールドの値が MIN と等しいことである」という HitPoint クラスの設計が、 isZero メソッド以外のコードからは見えないことです。
早期 return の狙いやメリットは私も理解しているつもりです。 でも、リスト 6.9 のコードはちょっと引っかかりました。
if (hitPointRate == 0) return HealthCondition.dead;
if (hitPointRate < 0.3) return HealthCondition.danger;
if (hitPointRate < 0.5) return HealthCondition.caution;
return HealthCondition.fine;
変更前のコード (リスト 6.7 やリスト 6.8) にあった else がなくなった結果、条件が見えにくくなっているように感じます。
たとえば 3 行目 (if (hitPointRate < 0.5) ...) です。
生命状態が「注意 (caution)」であるのは hitPointRate が [0.3, 0.5) の範囲内である場合ですが、「0.3 以上」は前の行を見ないとわからないです。
また、将来「危険 (danger)」と「注意 (caution)」との間にもうひとつ生命状態が追加された場合、それを判定する if 文は 2 行目と 3 行目の間に間違えずに追加する必要があります。else がなくなって見やすさは改善されていますが、コードを読む場合は else がある場合と同様に先行する if 文の条件も見ないとだめで、それなら else がある方が前とつながっていることが明確でわかりやすいのではないか、というのが引っかかった理由です。
まぁ、単に私が書き慣れていない, 読み慣れていないという理由もあるかもしれません。 リスト 6.4 の早期 return やリスト 3.4 のガード節はまったく異論ありません。
値オブジェクトをイミュータブルにするという考え方は以前から知っていましたし、メリットも理解しているつもりです。 ただ、それらのコレクションまでもイミュータブルにするというのはこれまでなじみがなく、ちょっとした驚きでした。
まずは前提。Party クラスは Member オブジェクトのコレクションを持っています。
class Party {
private final List<Member> members;
Party() {
members = new ArrayList<Member>();
}
}