😺

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

に公開4件
プログラミング
設計
メモ
品質
ミノ駆動本
tech

はじめに

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

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

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 にはならないです。 ↩︎

Discussion

nabeyangnabeyang

この本は持ってないのですが、この記事はがっつり読んだので感想を残したいと思います。

isZeroについて

HitPoint.isZeroについてですが、HitPoint.isMinが適切かなと思いました。ヒットポイントが0以上の値であることを認めたうえでないとisZeroの意味が良くわからないです。その知識を要求してしまうのなら

HitPoint == 0

と大差なさそうです。

イミュータブルなコレクションについて

イミュータブルなコレクションはArrays.asListのようなものを指すと思うのですが、addを実装するならミュータブルで良いと思いました。正直なところ、addを加えた場合、両者の違いは計算効率くらいなのかなと思います。

mafafamafafa

コメントいただきありがとうございます。 がっつりお読みいただき恐縮です。

isZero について

とても興味深いご指摘です。 響いたのでもう少し考えてみました。
書籍の p.54 (4.3.3) には次の仕様が示されています:

  • ヒットポイントは 0 以上。
  • ヒットポイントが 0 になった場合、死亡状態にする。

つまり、 0 というのは次の 2 つの意味が与えられていると解釈できます:

  • ヒットポイントが取り得る値の下限
  • メンバ (ゲームの登場人物) の状態を死亡にする特定の値

著者がフィールド名を MIN にされたのはひとつ目の性質を意識されてのことではないかと想像します。

これはヒットポイントという概念が生まれながらに備えている普遍的な性質なのか、それともたまたまこのゲームではそうであるというだけなのか[1]。
もし後者であれば、次のような設計もあり得ると思います。

HitPoint.java
class HitPoint {
  private static final int MIN = 0;   // 取り得る値の下限
  private static final int DEATH = 0; // メンバの状態を死亡にする特定の値
  int amount;

  HitPoint(final int amount) {
    if (amount < MIN) {
      throw new IllegalArgumentException();
    }

    this.amount = amount;
  }

  void damage(final int damageAmount) {
    final int nextAmount = amount - damageAmount;
    amount = Math.max(MIN, nextAmount);
  }

  /*
   * isZero の代わりに isDead を公開する。
   * amount と比較するのは MIN ではなく DEATH。
   */
  boolean isDead() {
    return amount == DEATH;
  }
}
Member.java
class Member {
  HitPoint hitPoint;
  States states;

  void damage(final int damageAmount) {
    hitPoint.damage(damageAmount);
    if (hitPoint.isDead()) {
      states.add(StateType.dead);
    }
  }
}

ところで、ヒットポイントが 0 になったらメンバを死亡状態にするという性質はヒットポイントのものなのかメンバのものなのか、という議論もあると思います。
前述のコードはヒットポイントの性質であるという前提です。
そうではなくメンバの性質であるならばこうですね。

HitPoint.java
class HitPoint {
  private static final int MIN = 0;
  int amount;

  HitPoint(final int amount) { ... }
  void damage(final int damageAmount) { ... }

  int value() {
    return amount;
  }
}
Member.java
class Member {
  private static final int HITPOINT_DEATH = 0;
  HitPoint hitPoint;
  States states;

  void damage(final int damageAmount) {
    hitPoint.damage(damageAmount);
    if (hitPoint.value() == HITPOINT_DEATH) {
      states.add(StateType.dead);
    }
  }

Member クラスの HITPOINT_DEATH フィールドの型が int ですが、これを HitPoint 型にして、死亡の判定の条件式を hitPoint.equals(HITPOINT_DEATH) としてもよいと思います。

余談ですが、対象の概念を掘り下げて行ってその性質を明らかにしていく過程が私は好きです。
この作業をチームメンバと一緒に行うことで対象に対する理解が深まり、認識が共有されていきます。
また、これによってシステム内での設計の矛盾の発生を防いだり、将来の仕様変更への耐性が高まったりすると考えています。

脚注

  1. 仕様分析時に、「ヒットポイントが 0 になった場合」は「ヒットポイントが下限値になった場合」と解釈してよいのか、と確認するでしょう。 ↩︎

kaxnaduki_soukaxnaduki_sou

mafafaさんのレビュー、とても勉強になりました。
ありがとうございます。

僕も全体的には、この本の内容に納得できたのですが、いくつか気になるところがありました。

mafafaさんもおっしゃっていたように、イミュータブルなコレクションにすべき、という主張がこの本にありましたが、
P202では、ExcellentCustumerRuleがイミュータブルのコレクションになっておりませんでした。
addメソッドの戻り値の型がExcellentCustomerPolicyではなく、無しでした。

「ミュータブルで良いんだろうか、良いのだとしたら理由は何なのだろうか」
という疑問が浮かびました。

mafafamafafa

コメントいただきありがとうございます。
ご指摘のコードは 6.3.1 のリスト 6.47 でよいでしょうか？
(kaxnaduki_sou さんは電子版をお読みではないかと思います。 私は書籍を購入したのですが、両者間でページ番号は一致しないみたいです。 うかつでした...)

私の見解を述べます。

著者が 7.3.1 で解消したかったのは、同一のコレクションに対する操作がコード中のさまざまな場所に登場して凝集度が低下することだと読み取りました。
それを回避するためにファーストクラスコレクション (Party クラス) を作り、生のコレクション (List<Member> クラス) をこれ以外からさわらせないようにしようという流れですね。
Party クラスの add メソッドで新しい Party オブジェクトを生成している (Party クラスをイミュータブルにしている) のは、 add メソッドの副作用 (members フィールドを変更すること) を防ぎたかったからです。

ご指摘の ExcellentCustomerPolicy クラスも Party クラスと同様にイミュータブルにすることはできると思います。
もし、 ExcellentCustomerPolicy クラスがプログラムのさまざまな場所から使われ、さまざまなところで add されるようだとすると、これをイミュータブルにすることは検討の価値があると思います。
でも、実際にはそうではなくて、 GoldCustomerPolicy や SilverCustomerPolicy クラスのコンストラクタの中で add したらそれ以降は変わらないと思われます。

ExcellentCustomerPolicy クラスは実装の手段としてコレクション (Set クラス) を使っているだけで、クラスの意味としては ExcellentCustomerRule のコレクションというわけではない、という違いもあるかもしれません。