はじめに

社内で「Good Code, Bad Code」の輪読会を開いたので、気づいたことを残しておきたいと思います。

輪読会をはじめた動機

自分が携わっているサービスのコードレビューをしていると、もっとチーム内に共通の認識や価値観を持ったほうが、コードの品質もあがり、レビュー内容も伝えやすくなるのではないかと思いました。
メンバーと話したところ、コードの可読性を学ぶ入門書として有名な「リーダブルコード」を読んだ人は多かったので、次のステップとしてクラス間の構造やテストにまで踏み込んだ「Good Code, Bad Code」を読んでみることにしました。

輪読会の進め方

1回30分をとり、週1ペースで開催しました。
各回で基本的に1つの Chapter でしたが、後半は前半で得た共通認識が参加者内にあったのでスムーズに読み進められ、2つの Chapter を一気に進められることができました。

事前に参加者には、感想用の Google ドキュメントに気づいたことやサービスに活かしたいことを記載してもらったので、輪読会と称しながら、実際は各メンバーの発表と議論に使いました。

Part1 理論編

Chapter 1 コードの品質

正しく動くって何だろう？

「正しく動くこと」を達成するためには、何が正しいか（仕様）を把握している必要がある
仕様をプログラム実装前に詰めるのも大事だし、実装中に気づきを得ながら漏れてる仕様を詰めていくのも大事
「正しく動作し続けること」はもっと大事
- テストを書くことで、機能拡張したりリファクタリングしやすくする
- コード外まで考慮するなら、パフォーマンスが悪化していることに気付けないといけないので監視も大事
参考記事: 「要件の変更に対応しやすいこと」コード品質はビジネスに影響を与える

低品質なコードに含まれる欠陥は、高品質なコードの15倍
低品質なコードの平均開発時間は、高品質なコードの2倍
低品質なコードの最大開発時間は、高品質なコードの9倍

車輪の再発明をしないために

新しくジョインしたチームで働くとき、どんな共通部品があるか分からないのでやってしまいがち
ジョイン初期はペアプロを多めにしたり、チャットの wol 部屋使って都度、チームからヒントを得る

Chapter 2 抽象化レイヤー

大きく複雑な問題は、小さな問題に切り分け続けることで対処する

小さな問題の切り分け方を適切な関数、クラス、インターフェイス、パッケージのように広げられるかが腕の見せどころな気がする
関心の分離
高凝集と疎結合

コード品質の柱がそのままクラスの分割単位すべきかの基準になる

読みやすさ
モジュール性
再利用性と汎用化
テスタビリティ

コードレビューを出す前に自分で批判的に読む

書いた直後は理解してるから、3ヶ月後、全くコードを知らない人が読んだら理解できるか？というシーンを想定する

現実世界の問題を扱うので、レイヤーの厚さを決める唯一のルールを見つけるのは困難

だから、コードの品質の柱の観点から自分で判断する必要がある
通常、レイヤーは薄すぎるより厚すぎるほうがデメリットが多いので薄い方に寄せる

Chapter 3 コードでの契約

当初に意図した設計や使用方法が、その後の保守・改良で壊されることがある

これを防ぐためには、2種類の方法しかない
1. 壊れたらコードをコンパイルできなくする
2. テストが失敗するようにする
コンパイラ言語でない場合、壊れたことを防ぐためにはテストはマストですね

どうやって他の人は自分のコードの使い方を理解するか

名前に注目する: 適切な名前をつける
データの型に注目する: 型で使い方を縛る
ドキュメントを読む: 読まなかったり、保守されないことも多い
- 言語ごとの xDoc を有効に使う
- 外部にドキュメントがあるなら @see でドキュメントのパスを書くと良さそう
直接尋ねる: 書いた人も覚えていない、時間も取られる
コードを読む: もっとも時間がかかる。複雑になるにつれ機能拡張が困難になる

Chapter 4 エラー

早い失敗と目立つ失敗を心がけたい

エラー箇所を特定しやすくするために、エラー箇所にできるだけ近いところでエラーを通知させる
異常値が DB に入り込む前にエラーで返さないと調査やリカバリの時間が増える
明らかにエラーと分かるようにする。エラーモニタリングも大事

どうやってエラーを通知するか

明示的と暗黙的
- 明示的: 検査例外や Result 型を使った戻り値など
- 暗黙的: try-catch や Promise の catch など
基本的に、呼び出し元にエラーを認識できる明示的に寄せたい。try-catch するにしてもカスタムエラークラス作る
呼び出し元にエラー判定とエラー内容を同時に認識させられる Result 型は良いやり方

Part 2 実践編

Chapter 5 コードを読みやすくする

名前を分かりやすく: 英語に厳密すぎなくていいと思ってるけど、意識していること

他の人が何も知らない状態からコードを読んだら理解できるだろうか？
単数形 or 複数形
〜data, 〜information は意味がないなら使わない
- 形容詞を使ってみるとより具体的になる（ NG: userData, OK: activeUser）
get or set（set なのに返り値あるなら、get にする）
短すぎて意味不明なら長くても良い
- 長すぎて冗長なら、肥大化してる可能性が大きい

コードを小さな関数に分割

public 関数のトップレベルでは、全体の処理の流れが分かるように意識してます
無名関数が大きくなって内容を理解できなくなりそうなら名前付き関数にする

Chapter 6 想定外の事態をなくす

マジックバリューを返り値に使わない
null は未入力・未確定を表す
- ダミー値として -1 や 0 を使わないこと（年齢や金額などを扱う場合不具合の原因になる）
null オブジェクトパターン: 配列の戻り値のときは null より空配列の方が呼び出し元が扱いやすい場合もある
関数名で副作用を表す
- React の dangerouslySetInnerHTML の命名は、XSS埋め込む危険性を伝えられているので秀逸だと思う
関数の引数を直接変更してたらNG。参照渡しもきな臭い
デフォルトケースに注意する
- 列挙型に新しい定義を追加すると switch 文（ if 文）でデフォルトケースに入り不具合になることがある
- 想定外の値なら例外エラーなげて早く目立って失敗したほうが、結果的に悪影響が小さくなる

Chapter 7 誤用しにくいコードを書く

すべてを不変にすることを検討する

解決策のビルダーパターンとコピーオンライトパターン
- どちらも同じインスタンスを使い回すのではなく、新しくインスタンスを生成している
- コピーオンライトパターンのほうが手軽な印象なので、まずはこちらを検討したいと思った

汎用的なデータ型を避ける

緯度・経度例は分かりやすかった（[number, number] 型だとどっちか分からない）
悪いパラダイムは広がって収集つかなくなる
TypeScript のブランド型を試したい
時間の扱い
- timeout: 3 だとミリ秒、秒、分など分からないので変数には単位も表現したい

Chapter 8 コードをモジュール化する

ハードコーディングされた依存関係の問題

DB やセッション層への直接アクセス
- テスタブルにクラスを切り出せなくなる

継承の問題点

子クラスで不要な親クラスの関数を公開してしまう
親クラスの変更が子クラスの機能を壊す
子クラス間で混在する機能が出てきた時に、重複コードを生み出す
解決案: コンポジション
- 継承ではなく機能クラスをDIで保有する

クラスは自分自身に関心を持つべき

他のクラスの内容を知りすぎていると変更しづらくなる
戻り値や例外から実装の詳細が漏れることがある
- レイヤー間で閉じている情報は汎用化してあげる必要がありそう
- ただ、API や DB がごっそり変わることも考慮して実装するのは、理想ではあるけどメンバー間の共通認識をとらないと難しい
記事「技術的負債を抱えたレガシーコード。変なメソッド名と入り組んだロジック、リファクタリングするならどちらが先？」を思い出した
- 「命名的問題」が読みやすさに直結するけど、「構造的問題」を改善したほうがコードを理解しやすく（変更しやすく）するものと解釈しています

Chapter 9 コードを再利用、汎用化しやすくする

安直に共通化・早まった最適化をすると逆に汎用性の低いものを作る

ミノ駆動さんの「共通化の罠」を思い出した（面白い）

同じようなものに見えても突き詰めると別のことに関心をもっていることがある
関数の引数にオブジェクトを渡すのか、必要なものだけ渡すのかはトレードオフ
- オブジェクトを渡す:
  - 引数が増えた時にまとまりが分かりやすいが、使わないプロパティまで渡すことがある
  - Param用の型をつくることで解決するのがいいかと思う
- 値を渡す:
  - シンプルになるが、引数が増えた時に呼び出しもとの考慮が増える
  - 引数を変更するさいのリファクタリングも不具合を誘発しやすい

Part 3 ユニットテスト編

Chapter 10 ユニットテストの原則

テストケースはアレンジ、アクト、アサーションで構成される

アレンジ: セットアップや後片付け
アクト: 関数呼び出し
アサーション: 評価

モック・スタブ・フェイク

モック: テスト対象のコードの依存関係で副作用が発生する場合をシミュレートするのに役立つ
スタブ: 戻り値を提供する依存関係をシミュレートするのに役立つ
フェイク: クラス（インターフェース）の代替実装。モックやスタブより初期実装コストはかかるが、変更に強い

モック派、古典派

モック派: テスト対象のコードと相互作用をテストする傾向
- コードが壊れていてもテストがパスする危険性がある
古典派: テスト対象のコードがどう実行され、最終結果が何かを重視

Chapter 11 ユニットテストの実践

テストケースは目的ごとに分けて、不要に1つにまとめない

分割することで、失敗したときに、クラス・関数のどんな振る舞いが失敗したか分かる
テストコードがそのまま仕様書になってたら嬉しい
セットアップ時に構成を共有するのは両刃の剣
- 個々のテストの設定が簡単になるけど、他のテストケースの副作用を考慮する必要がある（beforeAll から beforeEach に変更したり、イミュータブルにテストデータを生成することで回避）

public 関数のテストをする

private 関数をテストしたくなる誘惑があるが、詳細に囚われないこと
外から見たクラスの振る舞いを保証したい
リファクタリングで詳細実装は壊れるが public 関数は壊れない
private 関数のリファクタリングでテスト壊れてほしくない
- private 関数のテストを書くと、保守のための保守コードが増える

テストが書きづらくなったらクラスの責務が大きくなりすぎていないか疑う

レイヤーを分けてクラスを薄く保つとテストも書きやすい

さいごに

輪読会をやってみて良かったこと、悪かったこともありました。

悪かったこと
- 一人で読むより時間がかかることでした
  - 今回は3ヶ月くらいかかりましたが、その間に興味が他に移ることも...
良かったこと
- コードレビューでより具体的な言語化ができた
  - 今までなんとなく肌感覚で良いと思っていたことを、しっかり説明できるようになりました
- チームとして成長できたこと
  - 輪読会の気づきをメンバーに還元したり、改善案を取り入れられました
- 一人だと途中で積読になってたかもだけど、完読できた

一人じゃできないことも、輪読会で得られたことは多かったです！ハピネス！