🤬

改めて「良いコード/悪いコードで学ぶ設計入門」を読み切った

2023/02/01に公開

設計

リファクタリング

レビュー

idea

IT エンジニア本大賞で「良いコード/悪いコードで学ぶ設計入門」が取り上げられていました。
以前 6 章くらいまでは読んで、レビューを書いたのですが、そこで読むのをやめてしまっていたので、この機会に改めて読んでみました。

総評

ざっくり言うと自分は対象読者ではなかったな、という感じでした。
一応、本書の対象読者は以下のように書かれています。

設計がよくわからない/自信がない方、

これには該当するのですが、「設計入門」というにはミクロな視点のリファクタリングについての内容が多く、もう少しマクロな視点の設計についての記述を期待していた自分にはあまり得るものがありませんでした。
リファクタリングを学びたい初級者や、 DDD を学ぶ前にほんの少しでも内容に触れておきたい向きには良いのかもしれません。

仮に本書が「リファクタリング入門」というタイトルだったら、もう少し適切な対象読者に届いたのではないかなぁ、と思いました。例えるなら「Android アプリ開発入門」という本を読んでみたら、 Kotlin の文法程度しか載っていなかったかのようなそんな気分です。確かに Kotlin も Android 開発の入門だけどさぁ…みたいな…。

個別の気になった内容

評価の高い本書なので良かった箇所のレビューは他の方におまかせし、修正した方が良いのではないかと思ったところのみ挙げていきます。
ページ数は PDF 版のページを記載しています。

p47〜サンプルコードのメソッド名

掲載されていたサンプルコードを一部抜粋します。

class AttackPower {
// 略

  /**
   * 攻撃力を強化する
   * @param increment 攻撃力の増分
   */
  void reinForce(int increment) {
    value += increment;
  }

// 略
}

reinForce って、一般的な英単語なんでしょうか？自分の英語力が足りないだけかもしれませんが、全然知りませんでした…。
F が大文字なので rein と force で分かれるのかと思ったら、 reinforce で「強化する」というような意味みたいですね。 rein と force で分けたときにも同様の意味があるのかはわかりませんでした。

チームの英語力にもよりますが、このような一般的ではない(と思われる)英単語は使うべきではないと思いました。意味のわからない英語を使うことでコードの可読性が低下します。
難しい英語を使うことをカッコイイと思わずに、易しい英語を使って読みやすいコードを心がける方が良いと思います。(これも易しい英語だよ、と言われると私が恥ずかしいだけですが)
チームの大きな設計方針として「難読英語を使わない」ということは大切なことだと思います。

また、 F を大文字にして、 force という単語を意識している点も気になりました。著者は reinforce ではなく、 rein force だと考えているのでしょうかね？
force が「力」という意味だということはわかりやすいですが、クラス名として AttackPower という「力」の概念を名前にしていますので、わざわざ別の英単語を使うべきではないでしょう。
プログラミングでは名前が大切になります。「攻撃力」をモデリングして AttackPower と決めたのなら、紛らわしい force などの別の言葉は使わないようにするのも名前設計の一つだと思います。

p77 current という名前について

current という単語が使われたサンプルコードが掲載されていますが、本質を捉えた命名ではないように感じました。

class MagicPoint {
  // 現在の魔法力残量
  private int currentAmount;
  // オリジナルの魔法力最大値
  private int originalMaxAmount;
  // 魔法力最大値の増分
  private final List<Integer> maxIncrements;

  /** @return 現在の魔法力残量 */
  int current() {
    return currentAmount;
  }

// 略
}

そもそもクラスの説明が少ないので、どういった目的でこのクラスが作られているのかが、きちんと理解できていない部分はあるのですが、このクラスで「現在の」という情報は重要でしょうか。
以下のようなコードが書かれる可能性はないでしょうか？

MagicPoint oldMp;
int oldCurrent = oldMp.current();

意味がわからないコードになってしまいます。重要だったのは current ではなく amount の方でしたね。
サンプルコードには◯印がついていたので、一応「良いコード」になったと考えているのだと思いますが、別のものも含めてサンプルコードに中途半端な状態のものが多いように思います。攻撃力や魔法、装備品、魔法力など次々に異なるサンプルを提示せずに、題材を絞って良いものを提示した方が良かったのではないかな、と思いました。

p79 メソッドチェイン？

以下のようなサンプルコードが掲載されています。

void equipArmor(int memberId, Armor newArmor) {
  if (party.members[memberId].equipments.canChange) {
    party.members[memberId].equipments.armor = newArmor;
  }
}

連なっているのはフィールド(プロパティ)のようですが、これもメソッドチェインと呼ぶのでしょうか？
メソッドチェインと言われたら、実際に メソッド を連ねたものをイメージします。やってることは同じですが、悪いメソッドチェーンとは以下のようなものではないでしょうか？

void equipArmor(int memberId, Armor newArmor) {
  if (party.getMember(memberId).getEquipments().canChange()) {
    party.getMember(memberId).getEquipments().setArmor(newArmor);
  }
}

このような内部の情報を辿るコードは確かに良くないコードです。
しかし、最近は Collection に対してはメソッドチェインはよく使われているように思います。
以下は Kotlin のコードになりますが、以下のようにメソッドチェインを駆使したコードを書く機会はよくあります。

// 年収 500 万以上のユーザーを年齢の高い順に 10 人表示する
userList.filter { it.income > 5000000 }
	.sortedByDiscending { it.age }
	.take(10)
	.forEach { println(it.name) }

メソッドチェインすべてを悪いコードだとしているようで違和感を覚えました。
著者は他の箇所で「 switch を Map に置き換える」というようなリファクタリング方法も紹介していましたが、テクニックの一面だけを捉えてしまっていて、それぞれ良いところがあれば悪いところもあるということを見落としているように感じました。

p108〜魔法クラスの設計

以前レビューを書いた際にも指摘しましたが、魔法ごとにクラスを実装するのは悪手です。
「サンプルコードだから」と割り切って読むこともできるかもしれませんが、本書のタイトルは「設計入門」です。設計入門という文脈で、ゲーム開発には非効率な設計のサンプルコードが掲載されていたら、読む側が白けてしまうのも仕方ありません。

設計とは、課題を効率的に解決するしくみづくりのことです。

上記引用文は本書の「はじめに」の v ページに書かれている言葉です。
ゲーム開発の調整フェーズで、何度もパラメータを変更するという課題が上がることは、誰でも容易に想像がつきます。その課題を放置したサンプルコードは、設計入門を謳う本書には相応しくないでしょう。

なお、著名なゲームクリエイターの桜井氏が YouTube で「パラメータは外に出して調整できるようにしておく」という話をしています。以下 2 本がその動画です。勉強になりますね。

p150〜「 DRY 原則の誤用」の誤用

「 DRY 原則の誤用」という言葉が使われています。

通常割引と夏季限定割引はそれぞれ別の概念です。 DRY にすべきは、それぞれの概念単位なのです。同じようなロジック、似ているロジックであっても、概念が違えば DRY にすべきではないのです。

概念が違えば DRY にすべきではない、という点はその通りなのですが、この「通常割引」と「夏季限定割引」は本当に「別の概念」でしょうか？
モデリングする対象によっても変わってくるので、必ずしも「間違っている」と言えるわけではないですが、「通常割引」と「夏季限定割引」はどう考えても「どちらも割引である」ということはすぐ気が付きます。しかし、サンプルコードでは別物として扱っているため、そのせいで「どちらも割引である」という情報はモデリングできていません。「どちらも割引である」ということを、コードで表現するには、 6 章で出てきた interface を用いるのが良さそうですね。途中心配されている「 5% オフにする」場合も interface が敷いてあれば変更は簡単です。

著者は「 DRY 原則の誤用」というアンチパターンを知っていて、それを恐れるあまりに共通の概念を持つものまで別物として扱ってしまったのではないでしょうか。
アンチパターンを恐れすぎて、過剰な実装・設計になってしまうのはよくありません。

p158 割引クラスの実装例

前述した「どちらも割引である」ことがモデリングされたサンプルコードが、残念なことに「悪いコード」として掲載されています。確かに掲載されているサンプルコードは悪い例ですが、 p150 に掲載されていたサンプルコードからどうしてこうなったのか？という悪化ぶりです。
このような悪いコードを出されては、そりゃあ「 DRY 原則の誤用」と言いたくもなります。けれども、きちんと interface を敷くなりして整えてあげれば、「どちらも割引である」ことを示しつつ、良いコードに持っていけるのではないかと思いました。

後述しますが、個人的には「通常割引」「夏季限定割引」をクラス化するのはどうなんだろうなぁ？と思っています。しかし、ひとまず本書に倣ってクラス化しています。
以下のコードは Kotlin によるものです。

sealed interface Discount {
    val price: Int
    val discountedPrice: Int
    
    // 値引き
    open class PriceDiscount(
        override val price: Int,
        discountAmount: Int,
    ): Discount {
        override val discountedPrice: Int = price - discountAmount
    }
}

// 通常割引
class RegularDiscount(price: Int): Discount.PriceDiscount(price = price, discountAmount = 400)
// 夏季限定割引
class SummerDiscount(price: Int): Discount.PriceDiscount(price = price, discountAmount = 300)

エラー処理などは書いていませんが、これなら十分に「良いコード」ではないでしょうか？
共通の interface を敷いたので List で扱うこともできます。
さらに、心配されていた「 5% オフ」に変えることも簡単です。

sealed interface Discount {
// ...

    // n% 割引
    open class RateDiscount(
        override val price: Int,
        discountRates: Double,
    ): Discount {
        override val discountedPrice: Int = price - (1.0 - discountRates).toInt()
    }
}
    
// 夏季限定割引
class SummerDiscount(price: Int): Discount.RateDiscount(price = price, discountRates = 0.05)

このようなコードもアリだとは思いますが、正直なところ「通常割引」「夏季限定割引」をクラス化する必要性は感じていません。
クラス化せずとも以下のようなコードで十分でしょう。

fun getRegularDiscount(price: Int): Discount {
    return Discount.PriceDiscount(price = price, discountAmount = 400)
}

fun getSummerDiscount(price: Int): Discount {
    return Discount.PriceDiscount(price = price, discountAmount = 300)
}

あるいは、 enum DiscountType 等を用意してタイプ別に Discount を返すメソッドでも良いかもしれません。
どうも著者は概念をクラス化することに妙なこだわりがあるようです。
メソッドでも別の概念であることは十分示すことができていると思います。

p155 に以下の文章があります。

継承はよっぽど注意して扱わないと危険、継承は推奨しませんというのが本書のスタンスです。

これはその通りです。
同様に無駄にクラスを作りすぎることも、開発効率を低下させる要因になりえます。
クラスとして表現すべきなのか、 interface なのか、データなのか、よく考えて設計することが大切だと思います。

p222 値オブジェクトからの値の取り出し

ここ以外でも値オブジェクトから値を取り出すコードが頻繁にサンプルコードに登場します。
これは値オブジェクトの良さを全く活かせていません。
本書では p37 に以下のように書かれています。

値オブジェクト (Value Object) とは、値をクラス(型)として表現する設計パターンです。

せっかく型を用意しているのに、なぜ型の恩恵を捨てるようなコードが書かれているのか疑問です。

また、本書の p257 では「尋ねるな、命じろ」と書かれています。
しかし、多くのサンプルコードでは値オブジェクトに対して命じられていません。
このように、あるところでは「良い」「悪い」と書かれているのに、別のあるところではそれを台無しにするようなサンプルコードをいくつも見かけます。本文で書かれた内容と齟齬が発生していないか、もう少しサンプルコードの品質に注意を払った方が良かったかな、と思いました。

p264 エラーの扱い

エラーは戻り値で返さない、例外をスローすること

Kotlin、 Swift、 Rust くらいしか知らないのですが、最近の言語なら Result 型はよく使われていると思います。
「例外をスローすること」と言い切ってしまっていて、 Result 型について全く触れられていないのが残念に思いました。

p297 ユニットテストが書ける設計だったか

以前のレビューでも触れましたが、第 6 章の魔法クラスはとてもテストを用意できるような設計になっていないと思いました。
データを調整しただけで、テストも修正しなければいけません。

邪悪なコードには、テストコードが書かれていないことが多いです。

魔法クラスは邪悪なクラスに違いないでしょう。

おわりに

私の正直な感想として、本書は読んでいる最中、納得いかないことが多かったです。
入門書を「簡単すぎる」と思いながらも読むということも好きなのですが、本書は簡単すぎるというよりは「間違っているのではないか？」という思いを何度もしました。
おそらく途中で偏見を持ってしまって楽しく読むことができなくなったのだと思います。良くない読み方をしたと反省しています。
やはり、最初に述べたように私が対象読者ではなかったというだけでしょう。

私の感想はさておき、本書は多くの方から高い評価を得ています。多くの方がスキルアップされることを望みます。
また、長々と批判的なレビューを書きましたが、もし改訂版が出ることがあれば、本レビューが一部でも参考になれば幸いです。