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

はじめに

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

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

p.55: HitPoint.isZero

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

リスト 4.25

  private static final int MIN = 0;

リスト 4.25

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

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

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

p.90: 早期 return

早期 return の狙いやメリットは私も理解しているつもりです。
でも、リスト 6.9 のコードはちょっと引っかかりました。

リスト 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 オブジェクトのコレクションを持っています。

リスト 7.11

class Party {
  private final List<Member> members;

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

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

リスト 7.13

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

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

リスト 7.14

  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 オブジェクトを所有する設計が適切なのかはわかりませんが、いったんそういうことにします。

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

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

https://twitter.com/MinoDriven/status/1520650026644799493

p.139: Member.id

id フィールドがプリミティブ型 (int) なのが気になりました。
第 3 章 (クラス設計) の指針にしたがい、これも専用のクラスにすべきではないかと思います。

私はこの本を読む前からプリミティブ型 (C++ では fundamental types) を多用するスタイルに違和感を持っていて、周囲にも「インタフェースには bool 型を除いて fundamental types を使わないようにしよう」と主張していたくちです。

なお、「bool 型を除いて」のところは妥協の結果だったのですが、次の記事を読んで考えが変わりました。

https://zenn.dev/reputeless/articles/cpp-article-yesno

p.295: isDisabled

Customer や Comic といったクラスに isEnabled メソッドがあります。
「enabled でない」引数をガード節で取り除く際の条件式は !xxx.isEnabled() となり、そこに若干の読みにくさがあるということです。
そのため、論理否定 (!) を使わずに判断できるように isDisabled メソッドを追加するというアイディアです。

なるほど、と思いましたが、否定的な意味のメソッドを持たせることがそもそもわかりにくくならないかという懸念があります。
isDisabled は直感的に理解できますが、何がよくて何がだめなのかの客観的な基準を作るのは難しく、その線引きに悩むくらいなら一律禁止にしてしまえ、と考えてしまいそうです。

あと、 !xxx.isDisabled() と書くプログラマーが出てきたら嫌だなぁと...

おわりに

「はじめに」にも書きましたが、この本で著者が主張されていることの大部分は理由を含めて同意できるものでした。
ひとつふたつあげるだけならもしかしたらほかの人にもできるかもしれませんが、これが一冊の本にまとまっていることに価値を感じます。
「～という場合にこうなる傾向がある」など、「悪魔」の発生につながる条件や環境に言及されているところもあわせて、著者の長年の設計改善による経験の蓄積を感じさせます。

よく見られる「悪いコード」を初めに示し、その問題点を読者に理解させた上で、最終的に「良いコード」に改善していく、という流れはわかりやすいと感じました。
動くことが優先で、設計の品質や保守性 (変更容易性) といったことまで配慮が行き届いていない現場もありそうなので、 GW の連休が明けたら職場で広めようと思います。

また、第 16 章 (設計を妨げる開発プロセスとの戦い) は今の私のポジション・役割にはうってつけの内容でした。
参考にさせてもらって実践していこうと思います。
この 記事 メモ自体が 16.4.3 (敬意と礼儀) の内容を踏まえられているかが少々気がかりです...

脚注

1. std::vector クラステンプレートの場合は UB にはならないです。