Zenn
📖

書籍「良いコード／悪いコードで学ぶ設計入門 保守しやすい成長し続けるコードの書き方」レビュー

2022/09/07に公開
設計
リファクタリング
レビュー
リーダブル
idea

書籍「良いコード／悪いコードで学ぶ設計入門 保守しやすい成長し続けるコードの書き方」を読んだので、そのレビュー（メモ）。
著者：仙場大也氏
Twitter：ミノ駆動

購入理由

タイトルが良いのと、最近人気のため購入。
Twitterでもちらほら見たり、QiitaやZennの記事で出てきたり、前回参加したConnpassのリーダブルコード LT会 - vol.4で登壇者７名の内３名ほどが本書に言及する（社内勉強会を開いている会社も複数あった）等、トレンドとなっている。

総評

内容

1 悪しき構造の弊害を知覚する
1.1 意味不明な命名
1.2 理解を困難にする条件分岐のネスト
1.3 さまざまな悪魔を招きやすいデータクラス
1.4 悪魔退治の基本

大きくifで囲われて、ネストが多重になったコードがダメだということを書いている。「なんの冗談かと思うかもしれませんが、この手のコードは実在します。」と書かれているが、実際、つい最近委託先のコードレビューにてこの問題を指摘して修正させた。意識していなかったり、とりあえず動くコードだけを書ければ良いという人は、とにかく何も考えずにifの中にifを書いて、その中に更にifを書くという書き方をしがちと思う。early returnを使う等して、結局ネストは１つまでにできたが、これくらいは皆コードレビューを受ける前に自己レビューでできるようになってほしいと思った。

データは、クラスでカプセル化して、その振る舞いをメソッドとしてちゃんと持たせないと、そのデータを使っているコードで、似たロジックが大量に重複してしまうため、収集がつかなくなるといったことが書かれている。オブジェクト指向の基本とは思うが、意外と意識していない人も確かに多いと思う。

2 設計の初歩
2.1 省略せずに意図が伝わる名前を設計する
2.2 変数を使い回さない，目的ごとの変数を用意する
2.3 ベタ書きせず，意味のあるまとまりでメソッド化
2.4 関係し合うデータとロジックをクラスにまとめる

プログラムは、適切な名前をつけることに大きな意味があると思う。特に、C言語で書かれたコードに多い気がするが、省略語を変数や関数につけており、読んだときに何の略かよくわからず、コメントも書いてないから周りに書いている内容から推測して読む必要があるという経験をしたことがある人も多いと思う。この本に書かれているわけではないが、Ruby製作者のMatz（松本）さんが、プログラマが知るべき９７のことにて、

私の設計上の座右の銘は「名前重要」です。あらゆる機能をデザインする時に、私はその名前にもっともこだわります。プログラマとしてのキャリアの中で、適切な名前をつけることができた機能は成功し、そうでない機能については後で後悔することが多かったように思うからです。

と書いている通り、名前は重要だという認識を皆持てれば良いと思う。

変数へ再代入すると、可読性が下がるので目的ごとに変数を変更して再代入を回避した方が良い。ただし、再代入が絶対禁止というわけでも無いとは思う。場合によっては、再代入しないと無駄に長くて読みにくくなることもあると思う。

ちゃんと関数化しましょう、というのも当たり前だが、こうして本に書かれることに意味があると思う。ベタ書きで処理をずらずら書いているコードも何度もレビューしたことがある。。組込み業界は、可読性やメンテナンス性をあまり重視しない人が多い印象はある。マイコン等でそもそもC99の規格に対応していない、無駄なメモリ確保ができない等、リソース制約が大きい場合は仕方ない場合もあるが、それでもネスト等はむしろメモリ確保も減らせるはずだが、そうした可読性を意識していない人が多い印象はある（業界で一括りにするのも良く無いかもしれないが、connpass等の勉強会でも組込み系の人はあまりいないのかなと感じている。Web系とはまた違う難しさがあるので一概には言えないが）。組込みの人でもこうした本を読んで、設計の基本を学ぶ人が増えれば良いなと思う。特に組込みLinux等で比較的リッチな環境での開発であれば、基本的にはWeb系と変わらないと思う。

3 クラス設計 ―すべてにつながる設計の基盤―
3.1 クラス単体で正常に動作するよう設計する
3.2 成熟したクラスへ成長させる設計術
3.3 悪魔退治の効果を検証する
3.4 プログラム構造の問題解決に役立つ設計パターン
Column 種類の異なる言語と本書のノウハウ

メソッドはインスタンス変数を必ず使用するようにし、インスタンス変数は完全コンストラクタの形でfinalで初期化するといったことが書かれている。そうできる場合は基本的にそうすべきと思う。インスタンス変数を不変にして、変更する場合はメソッドの戻り値でインスタンスを生成するというDDDの値オブジェクトで行う方法を推奨している。確かに、メソッドによって副作用があるのか、変更後のインスタンスを返してくれるのかが名前だけからは判断できないことも多いので、基本的には副作用のないように、書かれている通りインスタンス変数を生成して返すのは良いとは思う。ただ、これを全てのクラスで行うと、結構面倒なことになる場合もあるので、比較的小規模であればどこまで徹底するかはそれぞれの現場レベルで判断することになるかと思った。

4 不変の活用 ―安定動作を構築する―
4.1 再代入
4.2 可変がもたらす意図せぬ影響
4.3 不変と可変の取り扱い方針

副作用の問題について書かれている。副作用を不要に入れ込むと予期しない動作を招くことがあるため、できるだけ避けようというのは同意。

「どんなときに可変にして良いか」という節にて、大量データの高速処理や画像処理、リソース制約の厳しい組込みソフトウェアなどでは可変が必要なこともあるという注意書きはあった。

5 低凝集 ―バラバラになったモノたち―
5.1 staticメソッドの誤用
5.2 初期化ロジックの分散
5.3 共通処理クラス（Common・Util）
5.4 結果を返すために引数を使わないこと
Column C#のoutキーワード
5.5 多すぎる引数
5.6 メソッドチェイン

この本では、低凝集は大きなテーマとして扱われていると思う。staticなクラスメソッドは低凝集に繋がるため注意であったり、クラスインスタンスの初期化ロジックが分散してしまう場合は、ファクトリーメソッドパターン（デザインパターン）を使いましょうといったことが書かれている。

これは意外とやりがちで重要だなと思ったのは、共通処理クラスのCommonやUtil。これは確かに低凝集の元になるというのはなるほどと思った。様々なお互い関連のない処理が置かれがちになるため、結局よくわからなくなって使われなくなったりするのはあるあるかなと思った。ログ出力やエラー検出、デバッグ、例外処理、キャッシュ、同期処理、分散処理などの横断的な関心事は共通処理としてまとめても良いだろうとの記載。

引数が多いメソッドも低凝集の元というのはその通りと思う。

intやfloatなどのプリミティブ型ばかり使わず、クラス化して値オブジェクトとするのを勧めている。規模によってプリミティブ型でも良いと思うし、カプセル化した方が安全と判断する場合は値オブジェクトにすれば良いのかなと思う。

メソッドチェーンはデメテルの法則（最小知識の原則：知らない人に話しかけるな）に反するため、よくないとしている。一律禁止にすべきものでもないと思うが、他人が見てそのメソッドチェーンで確かにわかるのか、そうしたことを意識して使う必要はあると思う。

6 条件分岐 ―迷宮化した分岐処理を解きほぐす技法―
6.1 条件分岐のネストによる可読性低下
6.2 switch文の重複
Column クソコード動画「switch文」
6.3 条件分岐の重複とネスト
6.4 型チェックで分岐しないこと
6.5 interfaceの使いこなしが中級者への第一歩

early returnとガード節の説明。これをやればかなりネストは削減できるため、積極的に使ってほしいと思う。昔自分もearly returnを知らなかった時は、書かれているようなNGコードを書いていたこともあった。ネストを削減できるだけでなく、条件とロジックが切り離されて、読みやすく変更もしやすくなるため、非常に良いテクニックと思う。early returnを使うと、elseは不要になるケースも多いため削除できる。

switch文の重複は、ほっておくとどんどんひどいことになっていくことを経験しているため、書かれている内容はそうだよなと思いながら読むことができた。ストラテジーパターンでswitchを消せるという紹介も書かれている。デザインパターンの本ではないため仕方ないかもしれないが、状態遷移におけるステートパターンの紹介もあってもよかったかなと思う。

ポリシーパターンも出てくるが、これはストラテジーパターンと同じなのでは？ストラテジーパターンとの関連について何も書かれていないため、わかりにくかった。

「分岐を書きそうになったら、まずinterface設計！」というのは、オブジェクト指向初心者が覚えやすくて良いと思う。

7 コレクション ―ネストを解消する構造化技法―
7.1 わざわざ自前でコレクション処理を実装してしまう
Column 車輪の再発明
7.2 ループ処理中の条件分岐ネスト
7.3 低凝集なコレクション処理

early continueやearly breakの紹介。
ファーストクラスコレクションによって、コレクション（リスト等）をカプセル化して、副作用が生じないようにインスタンス生成してreturnすることの紹介。

8 密結合 ―絡まって解きほぐせない構造―
8.1 密結合と責務
Column クソコード動画「共通化の罠」
8.2 密結合の各種事例と対処方法
Column クソコード動画「継承」

単一責任の原則（単一責務の原則）の説明と、DRY原則の誤用の説明。
なんでもかんでも共通化して良いわけではなく、責務が別であれば共通化しなくて良い。むしろ共通化してしまうと、こちらのクラスは変更したいが、もう１つの方は変更したくないといったことが生じたときに面倒となる。変更する理由が異なる（概念が異なる）場合はDRYにすべきではない、ということはちゃんと理解すべき。

継承は基本的に推奨しないという方針には賛成。
継承より委譲。スーパークラスは共通処理の置き場ではない。

アクセス修飾子について。なんでもpublicにすることがなぜダメか、package privateまたはprivateを推奨している。パッケージ外に公開するものだけpublicにする。

privateメソッドが多いクラスは、単一責務ではなく、多くの責務を持っている可能性があるというのはなるほどと思った。

9 設計の健全性をそこなうさまざまな悪魔たち
9.1 デッドコード
9.2 YAGNI原則
9.3 マジックナンバー
9.4 文字列型執着
9.5 グローバル変数
9.6 null問題
9.7 例外の握り潰し
9.8 設計秩序を破壊するメタプログラミング
9.9 技術駆動パッケージング
9.10 サンプルコードのコピペ
9.11 銀の弾丸

null安全の考え方も書かれているのは良いと思った。nullは使わず、nullに相当するオブジェクトを使うという説明はあるが、null objectパターンという用語は書かれていなかったため、用語の紹介はあってもよかったと思う。

技術駆動でフォルダ名を作る人が多いが、ビジネス概念でフォルダわけした方が良いのでは？ということだった。そうかもしれないし、ただその場合はアーキテクチャがどうなっているかを把握するのが簡単ではなくなるかもしれないなと思った。例えばMVCやMVVMなのかがビジネス概念でフォルダわけされているとわからないと思う。

サンプルコードのコピペは、設計上良くないなどが書かれているが、せめてライセンス上問題となりうるため、製品コードに使う場合はそのままコピペはせず、理解して自分で考えてコードを書くようにする、コピペした場合はライセンスを必ず確認してそのライセンスに応じた対応をするという内容は書かれていてもよかったかなと思う。

10 名前設計 ―あるべき構造を見破る名前―
10.1 悪魔を呼び寄せる名前
10.2 名前を設計する―目的駆動名前設計
10.3 設計時の注意すべきリスク
10.4 意図がわからない名前
Column 技術駆動命名を用いる分野もある
10.5 構造を大きく歪ませてしまう名前
Column クソコード動画「Managerクラス」
10.6 名前的に居場所が不自然なメソッド
10.7 名前の省略

名前の付け方によって設計が変わったりもするため、名前は重要。関心事で分離する。DDDにおけるドメイン分析的な説明で、会話で登場した名前がコードに無い場合は要注意といったことが書かれていた。

「このフラグが立っているときのUserは要注意会員」「この行のpriceは新品価格で、次の行のpriceは中古価格」といったように形容詞で違いを表現している場合は、それぞれをクラス化するチャンスというのはなるほどと思った。

驚き最小の原則の紹介。名前と処理内容がかけ離れたものにならないようにする必要がある。

名前が重要なのは、上記でプログラマが知るべき９７のことの引用でも記載したが、この章でもっと詳しく解説があった。

11 コメント ―保守と変更の正確性を高める書き方―
11.1 退化コメント
11.2 コメントで命名をごまかす
11.3 意図や仕様変更時の注意点を読み手に伝えること
11.4 コメントのルール　まとめ
11.5 ドキュメントコメント

コメントには処理の説明ではなく、意図を書く。
そうしないと、処理を変更した際にコメントが修正されずにコードと乖離してしまう場合がある。
コメント書いているから変数名や関数名が適当になっていたりする場合があるが、基本的にはコメントではなく名前の方に気を配るべき。

12 メソッド（関数） ―良きクラスには良きメソッドあり―
12.1 必ず自身のクラスのインスタンス変数を使うこと
12.2 不変をベースに予期せぬ動作を防ぐ関数にすること
12.3 尋ねるな，命じろ
Column クソコード動画「カプセル化」
12.4 コマンド・クエリ分離
12.5 引数
12.6 戻り値
Column メソッドの名前設計
Column staticメソッドの扱いに注意

目次とこれまでの説明通り。

13 モデリング ―クラス設計の土台―
13.1 邪悪な構造に陥りがちなUserクラス
13.2 モデリングの考え方とあるべき構造
13.3 良くないモデルの問題点と解決方法
Column クソコード動画「Userクラス」
13.4 機能性を左右するモデリング

特にコメントなし。

14 リファクタリング ―既存コードを成長に導く技―
14.1 リファクタリングの流れ
14.2 ユニットテストでリファクタリングのミスを防ぐ
14.3 あやふやな仕様を理解するための分析方法
14.4 IDEのリファクタリング機能
14.5 リファクタリングで注意すべきこと
Column Railsアプリのリファクタリング

これを読んだら、「テスト駆動開発」や「リファクタリング 第２版」を読むと良いと思う。

15 設計の意義と設計への向き合い方
15.1 本書はなんの設計について書いたものなのか
15.2 設計しないと開発生産性が低下する
15.3 ソフトウェアとエンジニアの成長性
15.4 課題を解決する
15.5 コードの良し悪しを判断する指標
Column クラスを分割すると読みにくくなる？
15.6 コード分析をサポートする各種ツール
Column シンタックスハイライトを品質可視化に利用する
15.7 設計対象と費用対効果
15.8 時間を操る超能力者になろう

技術的負債を溜めないようにしよう。変更容易性を確保しよう。
費用対効果を意識して、なんでもかんでも修正するのではなく、変更が多い部分やコアな部分を集中的にリファクタリングした方が良い。

16 設計を妨げる開発プロセスとの戦い
16.1 コミュニケーション
16.2 設計
16.3 実装
16.4 レビュー
16.5 チームの設計力を高める

ちゃんとチームでコミュニケーションを取ろう。コーディング規約を決めよう。
割れ窓理論やボーイスカウトの規則といった達人プログラマーに書かれている内容の紹介。
コードレビューで重要なのは、敬意と礼儀。攻撃的なコメントは百害あって一利なし。

紹介されているChromiumのコードレビューの決まりとは別だが、Googleが公開しているコードレビュの基準について。
https://fujiharuka.github.io/google-eng-practices-ja/ja/review/reviewer/looking-for.html

要約

コードレビューをする際には、次のことを確認してください。

  • コードがうまく設計されている
  • 機能性がコードのユーザーにとって適切である
  • UI の変更がある場合、よく考えられていて見た目も適切である
  • 並行処理がある場合、安全に行われている
  • コードが必要以上に複雑でない
  • 開発者は将来必要になるかもしれないものではなく、現在必要だとわかっているものを実装している
  • コードには適切なユニットテストがある
  • テストがうまく設計されている
  • 開発者はあらゆるものに明確な名前を使った
  • コメントは明確で有意義なもので、「何」ではなく「なぜ」を説明している
  • コードは適切にドキュメント化されている（一般的には g3doc で）
  • コードはスタイルガイドに準拠している

レビューを依頼されたコードを一行ずつレビューすること、コンテキストを確認すること、コードの健康状態を改善しているかを見極めること、開発者が良いことをしたらそれを褒めることを忘れずに。

17 設計技術の理解の深め方
17.1 さらにステップアップするための設計技術書紹介
Column バグ退治RPG『バグハンター2 REBOOT』
17.2 設計スキルを高める学び方
Column C#と長き旅，そして設計への道

特にコメントなし。

偉そうな内容も書いてしまったが、自分も勉強中の身なので、自分への戒めのつもりでもある。こうした基礎をおろそかにせず、しっかりとした設計・実装ができるようになりたい。まだまだ大量の本を読む等する必要がある。

Discussion