Zenn
📗

「良いコード/悪いコードで学ぶ設計入門」レビュー(6章のみ)

2022/08/05に公開
1件
Kotlin
#
設計
#
リファクタリング
tech

「良いコード/悪いコードで学ぶ設計入門」レビュー

「良いコード/悪いコードで学ぶ設計入門」という書籍に色々と気になることが書かれていたので、今更ではありますが、特に気になる6章を中心にレビューしたいと思います。自分は著者を存じ上げませんが、この書籍を執筆中に下読みとして読んだつもりで、気になる点を列挙したいと思います。
なお、書籍のサンプルコードは Java で書かれていましたが、この記事内では Kotlin で書かせていただきます。
著者・関係者の方々の気分を害されたら申し訳ありません。

取り上げる題材の統一感

まず、6章全体で取り上げられている題材(サンプルコード)に統一感がありません。
6章で扱われている題材はざっと分けると次のようになっています。

  • 6.1 魔法発動処理
  • 6.1.2 生命状態判定処理
  • 6.2 魔法種別による分岐処理
  • 6.2.6 面積計算処理
  • 6.2.7 魔法別コスト計算処理
  • 6.3 会員条件判定処理
  • 6.4 宿泊料金判定処理
  • 6.6 ダメージ分岐処理

概ねファンタジー要素の強いゲームを想定した題材が取り上げられていますが、突然 6.2.6 で Shape インターフェースを用いた例が出てきたり、 6.3 では EC サイト、 6.4 ではホテルの宿泊料金に関する例が登場します。読んでいて「なんで突然変わるの？！」と感じました。
せめてこの 6 章の中だけは、ゲーム系の題材(もしくは別の題材)で統一することができなかったのでしょうか。

本書でも別の章で 凝集度 について取り上げられていますが、言ってみれば 6 章の題材については、 低凝集 だと感じました。しかも、 偶発的凝集 の状態だと思います。
ソースコードと同様、書籍も読みやすいに越したことはありません。章の凝集度を高めてほしかったです。

fun chapter6() {
   GameSample.section1()
   GameSample.section2(ShapeSample)
   EcSiteSample.section3()
   HotelRatesSample.section4()
   GameSample.section6()
}

題材を統一することが難しいのであれば、せめて順番に取り上げるような構成になっていたら良かったと思います。「題材が変わったかと思ったら戻った、戻ったと思ったらまた別の題材になった」と読者を混乱させるのではなく、「題材が変わった、一区切りついたんだな」と安心できる構成になっていると良かったと思います。
前述しましたが、まさに「なんで突然変わるの？！」という悪い意味での驚きの連続でした。こちらも書籍の別の章で 驚き最小の原則 として注意すべきこととして取り上げられています。
エンジニアリングにおける設計原則は、書籍の構成においても役に立つのではないでしょうか？

値オブジェクトの説明への不安

6.2 の最後に「丁寧に値オブジェクト化する」というトピックが書かれています。
値の取り違いを懸念して先述されている 3.2.6 への参照がありますが、値オブジェクトについては参照されていません。値オブジェクトの説明がある 3.4.2 にもついて言及が欲しいと思いました。
また、 3.4.2 には以下の記述があります。

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

この説明は疑問です。値をクラスでラッピングしただけでは値オブジェクトとは言えません。値オブジェクトであるならば、少なくとも 等価性 (同等性)には触れるべきではないでしょうか。
値オブジェクトは オブジェクト(クラス)を値のように扱うためのパターン だと自分は認識していました。
3.2.6 には Money クラスが登場していますが、値オブジェクトを名乗るのであれば、以下のようなコードが正しく動作するはずです。

val yen = Currency.getInstance(Locale.JAPAN)
val isSameAmount = Money(100, yen) == Money(100, yen) // true になるはず

しかし、本書で等価性や equals() の実装について取り上げられている様子はありません。

また、 6.2 で値オブジェクト化を説明した後で、 6.6 では damageAmount を int で表現している点も気になります。
damageAmount と AttackPower の間に関係はないかもしれませんが、 int でアクセスさせるコードを見た読者が AttackPower や MagicPoint の value に直接アクセスするコードを書いてしまわないでしょうか。
AttackPower や MagicPoint などのオブジェクトを作成しても、内部の int に直接アクセスできるのであれば、値の取り違いは簡単に起こるでしょう。

val ap = AttackPower(100)
val mp = MagicPower(100)
val damage = ap.value * mp.value

値オブジェクト化について説明した後で、 int を扱うコードを提示するくらいであれば、この章では値オブジェクトのことについては触れずに、別の章で取り上げるなどの工夫があると良かったと思いました。

トピックのタイトルが 丁寧に と添えられている点も気になりました。
もう少し説明を加えないと、すべてのプリミティブ型の値をオブジェクト化する(ラッピングする)ことが、 常に良いこと のように受け取られる可能性が考えられます。
例えば、「スマホを毎回箱にしまって、使うときに取り出す」のと「スマホにケースをつけて使う」のはどちらが 丁寧 な取り扱いでしょうか？人によっては前者を丁寧だと感じるかもしれません。
不便＝丁寧＝正しいと勘違いされないような説明が、別の章でも良いので、もう少しされていたら良かったかと思います。

Map を善、 switch を悪、と捉えている

6.2.3 で switch 文の case の実装漏れを懸念されています。
しかし、これについては言語側のサポートがあれば起こりません。 Map と違い、網羅性を担保できるものでもあります。
逆に Map 側についても、クラスは実装したものの、 Map への代入を忘れていたといったミスは起こりえます。代入漏れは、実行時の NullPointerException になりかねません。
片方だけのデメリットに触れて、もう一方には触れないというのはフェアではないでしょう。入門書ですので読者の視野を広げるものであって欲しいと思いました。

魔法クラスを個別に実装する設計

「6.2 switch 文の重複」では、複数の switch 文を、 interface を用いてリファクタリングしていく様子が書かれています。 switch 文を interface でリファクタリングするということ自体は良いのですが、題材として取り上げられている Magic インターフェースと、その実装クラスについては、サンプルコードとしては少し練度が低いのではないかという印象です。

サンプルコードの実用性はどうか

魔法が出てくることからわかるように、ゲーム開発で利用されるシーンを想定します。
入門書ということで、あくまで説明のためのサンプルコードということも十分考えられますが、設計の入門書ですので、読者は「リファクタリング後のコードは良い設計になっているはず」ということを思うでしょうし、良い設計であれば「実際に利用しよう」と考えてもおかしくありません。

欠点：ソースコードにデータが書かれている

「魔法クラスを個別に実装する設計」の最大の欠点は、やはり、魔法パラメータの調整を行う毎に再ビルドが必要になる点でしょう。
ゲームの規模はわかりませんが、ゲームバランスの調整のフェーズはあると思います。一発で完璧なバランスになることはないので、細かな調整が何度も行われるでしょう。
しかし、本書の設計では、調整を行う度にプログラマに依頼をし、ビルドで実行バイナリが出力されるのを待たなければなりません。
魔法の種類が本書に登場する3つしかない、というような個人の小さなゲームであれば耐えられるかもしれません。それでも拡張性に乏しい設計だということには変わりありません。

欠点：データとクラス名が結びついている

本書では「ファイア」という魔法に対して Fire というクラスが作られています。
もし開発途中でこの魔法を「フレイム」という名前に変えることになったらどうなるでしょうか。
name() で返される文字列を "フレイム" に変更しても、クラス名は Fire のままです。後から「やっぱりファイアも追加して欲しい」という要望が来ることも考えられますので、クラス名を変更しないわけにはいきません。
クラス名だけではなく、 MagicType でも fire が定義されています。こちらも忘れずに変更する必要があります。
仕様変更に弱い設計だと感じました。

欠点: テストは考えられているか

14.2 に「ユニットテストでリファクタリングのミスを防ぐ」としてテストについても書かれています。それぞれの魔法クラスにもユニットテストが用意されることでしょう。では、そのテストの内容はどのようなものでしょうか。
FireTest では「 name() を呼んだとき、 "ファイア" が返されること」のようなテストが用意されるのでしょうか？ ShidenTest でも同様に "紫電" が返されることというテストがあり、 HellFireTest でも同様のテストがあるのでしょうか。このようなテストが本当に必要でしょうか？
パラメータ調整を行った場合、テストコードも書き直しになります。名前の変更についても、テストにまで影響を及ぼします。
果たして運用していける設計でしょうか。

欠点: Member クラスを参照している

各魔法クラスが Member クラスを参照しているのが直感的ではないように思いました。
コードからは「魔法が魔法を使う人を所有している」ように読めます。「魔法を使う人が魔法を所有している」方が自然な構成だと思います。
仮に Fire を使える人が複数いた場合、 Fire のインスタンスが複数必要になります。複数のインスタンスがあっても、差異は Member の参照先だけという状況が生まれます。これは無駄な重複でしょう。
やはり、魔法を使う人が、1つの魔法インスタンスを参照するような構造の方が良い設計なのではないかと思います。

欠点: データと処理の分離ができていない

各魔法クラスが2つの役割を持ってしまっています。
「データの器」としての役割と、「発動時コスト計算」の役割です。
データの器が処理を持っているためテストが必要になります。コスト計算のクラスなのにデータを持っているため、パラメータ調整時に再ビルドが必要になります。
これはどちらかに寄せることで解決できるはずです。
今回は「発動時コスト計算」の 処理 を、「発動時コスト計算タイプ」のような データ に置き換えるのが良いのではないでしょうか。
データをデータシートなどの外部から読み込むように変更することで、再ビルドせずにパラメータ調整が行えるようになります。
発動時のコスト計算については、タイプ別に処理を分ける必要が出てきますが、本章で取り上げられている interface をうまく敷くことで無理なく分岐させることができるでしょう。

代替案

指摘するだけでは説得力もないので、代替案を考えました。
まず、魔法は基本的にデータの器として定義します。 interface ではなく、クラスとして実装してしまって良いでしょう。

Magic

class Magic(
    val name: String,
    val attackPoint: Point,
    val costMp: Point,
    val costTp: Point,
)

Magic クラスが持つ Point は前述の「発動時コスト計算」をデータ化したものです。3つの Point を保持するようになっていますが、本書のように AttackPoint MagicPoint TechnicalPoint と分けるのも良いかと思います。
また、実際にはデータシートとの突き合わせのために識別子(id)が必要になるかと思います。

Point

Point は、以下のような (sealed) interface になっています。

sealed interface Point {
    fun calculate(member: Member): Int

    //... 各種計算定義
}

魔法側に Member は持たせず、コスト計算を行うタイミング(calculate())でのみ参照させるようにしています。将来的に Member 以外も計算の対象になることを考慮して、 ExternalFactor のようなクラスで Member をラップすることも考えましたが、ひとまずそのまま渡しています。

各種コスト計算

各種計算の実装については以下のようになっています。

    data class Fixed(val point: Int) : Point {
        override fun calculate(member: Member): Int = point
    }

    data class ByLevel(val ratio: Float) : Point {
        override fun calculate(member: Member): Int = 
		(member.level * ratio).toInt()
    }

    data class ByAgility(val ratio: Float) : Point { ... }　// 同様に計算
    data class ByVitality(val ratio: Float) : Point { ... }　// 同様に計算
    data class ByMagicAttack(val ratio: Float) : Point { ... }　// 同様に計算

    data class Compound(val points: List<Point>) : Point {
        override fun calculate(member: Member): Int = 
		points.sumOf { it.calculate(member) }
    }

Fire Shiden HellFire のメソッドで用いられていた計算をそのまま用意するのではなく、値となり得る要素をそれぞれクラス化しました。
値の足し算が必要であれば、 Compound に複数の Point を渡すことで値の合成を行えるようにしました。
また、以下のような拡張関数を用意することで合成をしやすくしました。

fun List<Point>.compound(): Point = Point.Compound(this)

各種魔法データ例

これらのクラスを使った「ファイア」「紫電」「地獄の業火」の定義は以下のようになります。

Magic(
    name = "ファイア",
    attackPoint = listOf(
	Point.Fixed(20),
	Point.ByLevel(0.5f),
    ).compound(),
    costMp = Point.Fixed(10),
    costTp = Point.Fixed(0),
),

Magic(
    name = "紫電",
    attackPoint = listOf(
	Point.Fixed(50),
	Point.ByAgility(1.5f),
    ).compound(),
    costMp = listOf(
	Point.Fixed(5),
	Point.ByLevel(0.2f),
    ).compound(),
    costTp = Point.Fixed(5),
),

Magic(
    name = "地獄の業火",
    attackPoint = listOf(
	Point.Fixed(200),
	Point.ByMagicAttack(0.5f),
	Point.ByVitality(2f),
    ).compound(),
    costMp = Point.Fixed(16),
    costTp = listOf(
	Point.Fixed(20),
	Point.ByLevel(0.4f),
    ).compound(),
),

実際にはこのようなデータになるように外部から読み込むのが望ましいでしょう。
コスト計算の組み合わせの指定方法は若干難しくなりそうなので、バランス調整を行う人の能力によっては、調整用のツールを別途作る必要があるかもしれません。

代替案まとめ

名前やパラメータの調整で再ビルドする必要がなくなりました。
コスト計算のパターンが増えても柔軟に拡張することができそうです。
複雑だったコスト計算もバラバラにすることでテストも行いやすくなったのではないでしょうか。

しかし、このようなコードが出来上がっても、このコードではこの著書は完成しなかったのだろうと思います。この章で伝えたいことがあり、今のサンプルコードになっているだと思います。
著者が入門書の犬猫クラスの是非を問うているように、入門書における適切な例示というものはとても難しいことだと感じました。

最後に

今回はこの書籍が出版前の状態だったら、と仮定してレビューを行いました。出版前の状態であれば、いくつかは意見が通って修正してもらえたかもしれません。本書にも著者の謝辞として、レビュワーの名前が挙がっていました。有名な方ばかりかと思いますが、今回の自分のような指摘は挙がらなかったのかと疑問に思います。挙がったもののこのままで良いと判断されたのかもしれません。

少し前に、内容に多くの間違いがあるとして、ある技術書が炎上していました。事前のチェックが甘かったとのとです。その技術書も友人・知人の目は通っていたと思いますが、身内だと贔屓目で見てしまうのでしょうか。甘いレビューになってしまうのかもしれません。

小説の賞では、多数の応募があるため下読みの方がいらっしゃるらしいです。技術書に関しても、出版社の方が懇意にしているエンジニアなどに気軽に下読みを依頼できると良いのかな、と思いました。
そういった依頼があれば是非やってみたいと思っています(得意分野以外は入門書くらいしかレビューできないと思いますが)。

長々と書きましたが、改めて、著者・関係者の気分を害されたのであれば申し訳ありません。
本記事がわずかでもどなたかの参考になれば幸いです。

追記

気になる点が1つ増えたので追記します(2022/08/12)。ツイート埋め込みですが。

Discussion

ひからない人ひからない人

長々と書きましたが、改めて、著者・関係者の気分を害されたのであれば申し訳ありません。
本記事がわずかでもどなたかの参考になれば幸いです。

素敵な言葉だと思います。書籍のレビューは、筆者やその周囲の人の人格攻撃をするものではなく、あくまで内容（またはその内容を公開するに至った経緯など）について言及するものだと、個人的には思っているので。