『良いコード／悪いコードで学ぶ設計入門』を読んで気になったことのメモ

はじめに

話題となっている『良いコード／悪いコードで学ぶ設計入門 ―保守しやすい 成長し続けるコードの書き方』 (出版社のページ) を読みました。

全体的には「うんうん、そうだよね」と同意できることが多かったです。
もちろん、初めて目にするような考え方, アイディア, テクニックもありました。
一方、気になったことやちょっと引っかかったこともありましたので、メモしておきます。
あくまでもメモなので結論のようなことはありません。

p.55: HitPoint.isZero

HitPoint クラスに isZero メソッドがあります。
「ヒットポイントがゼロであれば true」という仕様で、実装は次のようになっています。

  private static final int MIN = 0;

  boolean isZero() {
    return amount == MIN;
  }

「ゼロ」を表すフィールドの名前がなぜ ZERO ではなく MIN なのだろうかと気になりました。
値は 0 でなくてもかまわないのですが、仕様上「ヒットポイントがゼロ」であることを表すなら MIN より ZERO の方がよいのではないかと思います。

何が気になったのか少し考えてみたところ、「仕様上ヒットポイントがゼロであるというのは、 amount フィールドの値が MIN と等しいことである」という HitPoint クラスの設計が、 isZero メソッド以外のコードからは見えないことです。

p.90: 早期 return

早期 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 のガード節はまったく異論ありません。

p.137: イミュータブルなコレクション

値オブジェクトをイミュータブルにするという考え方は以前から知っていましたし、メリットも理解しているつもりです。
ただ、それらのコレクションまでもイミュータブルにするというのはこれまでなじみがなく、ちょっとした驚きでした。

オリジナル (Java 版)

まずは前提。
Party クラスは Member オブジェクトのコレクションを持っています。

class Party {
  private final List<Member> members;

  Party() {
    members = new ArrayList<Member>();
  }
}

このあと Party クラスのインスタンスに Member オブジェクトを追加する add メソッドを作ります。
そのとき、 Party クラスがイミュータブルであることを維持するために、 add メソッドは新しい Party クラスのインスタンスを生成して返すようにしています。

class Party {
  // 中略
  Party add(final Member newMember) {
    List<Member> adding = new ArrayList<>(members);
    adding.add(newMember);
    return new Party(adding);
  }

なお、次のコンストラクタも追加されています:

  private Party(List<Member> members) {
    this.members = members;
  }

C++ 版

これを読んで私が考えたのは「C++ だったらどうやって実装するだろうか」ということでした。
C++ の場合、オブジェクトは参照のセマンティクスだけでなく値のセマンティクスでも扱えます。
果たして Party オブジェクトが Member オブジェクトを所有する設計が適切なのかはわかりませんが、いったんそういうことにします。

リスト 7.11 と同等の Party クラスを C++ で書いてみるとこうなるでしょうか。
std::vector 型のデータメンバを const にするのは慣れていなくて新鮮です。

class Party {
 public:
  Party() = default;

 private:
  const std::vector<Member> members_;
};

続いて、 std::vector<Member> 型のオブジェクトを受け取れるコンストラクタと add メンバ関数を追加してみます。

class Party {
 public:
  Party() = default;

  Party add(const Member &newMember) {
    std::vector<Member> adding{std::move(members_)};
    adding.push_back(newMember);
    return Party{std::move(adding)};
  }

 private:
  const std::vector<Member> members_;

  explicit Party(std::vector<Member> &&members)
      : members_{std::move(members)} {
  }
};

できました。
オリジナルの Java 版と異なり、 add メンバ関数内で Member 型のオブジェクトのコピーが発生していますがこれはやむを得ないでしょう。
rvalue reference を引数に取るメンバ関数 (add(Member &&member)) をオーバーロードしてやれば、使う側が選択できます。

さて、この Party クラスを使う側のコードはこうなりますね。

  auto newParty = currentParty.add(newMember);

ここで問題となるのが、 add メンバ関数内で自身の members_ データメンバをムーブしてしまっていることです。
メンバを追加したあとも、追加前の currentParty オブジェクトにはアクセスできてしまうので危険です^[1]。
これを防ぐにはムーブせずにコピーすればよいのですが、新しい Member オブジェクトを追加するたびに既存の Member オブジェクトがすべてコピーされるのは実行時間の点でもメモリ使用量の点でも受け入れにくいのではないかと思います。
ただ、 add という名前のメンバ関数の中でムーブされるのは驚き最小の原則に反しているのでよくないですね。

冒頭で棚上げしていたこの課題の解消は、ドメインをきちんと分析してモデリングしてからですね。

果たして Party オブジェクトが Member オブジェクトを所有する設計が適切なのかはわかりませんが、いったんそういうことにします。

このケースでは、自分ならポインタにするんじゃないかなと思います。

著者のミノ駆動 ( ) さんが Rust で書き直しされているそうなので、ちょっと気になっています。