thermo-nuclear-code-quality-review

熱核レベルのコード品質レビュー

実装品質、保守性、抽象化の品質、コードベースの健全性に焦点を当てた、非常に厳格なレビューを行う際にこのスキルを使用します。

何よりも、このスキルはレビュー担当者に、コード構造の改善に意欲的に取り組むよう促す必要があります。局所的なクリーンアップの機会を指摘するだけではいけません。「コード柔術」とも言える改善策、つまり動作を維持しながら実装を大幅に簡素化し、コンパクトで直接的かつ洗練されたものにする再構成を積極的に探してください。

中核となるプロンプト

次の方針を出発点としてください。

現在のブランチの変更について、コード品質を徹底的に監査してください。 動作に影響を与えずにコード品質を大きく向上させるため、変更の構成や実装方法を見直してください。 抽象化とモジュール性を向上させ、スパゲッティコードを減らし、簡潔さと読みやすさを高めてください。 意欲的に取り組んでください。コードベースの一部を再構成することで実装を改善する明確な方法があれば、実行してください。 極めて徹底的かつ厳密に進めてください。慎重に確認してから変更してください。

譲れない追加基準

上記の基本プロンプトに加え、以下の明示的なレビュー規則を適用してください。

0. 構造の簡素化を積極的に目指してください。

   • 「もう少しすっきりできるかもしれない」というところで止めないでください。
   • 変更を再構成し、分岐、ヘルパー、モード、条件分岐、レイヤー全体をなくせる機会を探してください。
   • 振り返ったときに、必然だったと感じられる解決策を優先してください。
   • 既存のアーキテクチャをより効果的に活用し、変更を大幅に簡素化して、より洗練されたものにする「コード柔術」のような方法があることを前提にしてください。
   • 複雑さを組み替えるのではなく削除できる道が見えたら、その方向を強く推してください。

1. 非常に強い理由がない限り、PR によって 1,000 行未満のファイルが 1,000 行を超えることを許容しないでください。

   • これは原則として、コード品質上の重大な懸念事項として扱ってください。
   • ファイルが 1,000 行を超えて肥大化するのではなく、ヘルパー、サブコンポーネント、モジュール、局所的な抽象化を切り出すことを優先してください。
   • 差分によってこのしきい値を超える場合は、まずコードを分割すべきではないか、明確に問いかけてください。
   • 構造上やむを得ない理由があり、分割後のファイルも明確に整理されている場合に限り、例外を認めてください。

2. 既存コードに無秩序なスパゲッティ状の複雑さを加えないでください。

   • 新たな場当たり的な条件分岐、散在する特殊ケース、無関係な処理フローに挿入された単発の分岐を厳しく警戒してください。
   • 「あちこちに奇妙な if 文が追加される」変更は、単なるスタイル上の指摘ではなく、設計上の問題として扱ってください。
   • 既存の処理フローを絡ませるのではなく、専用の抽象化、ヘルパー、ステートマシン、ポリシーオブジェクト、または別モジュールにロジックを移すことを優先してください。
   • 技術的には動作していても、周辺コードの理解を難しくする変更は指摘してください。

3. 動作するコードを受け入れるだけでなく、設計の改善を重視してください。

   • 動作を変えずに構造を大きくすっきりさせられるなら、より整理された実装を強く推してください。
   • コードベースをより複雑にしたままの「動けばよい」実装を安易に承認しないでください。
   • 同じ複雑さを別の場所に移すだけのリファクタリングより、要素そのものを取り除く簡素化を強く優先してください。

4. ハック的、または魔法のようなコードより、直接的で地味でも保守しやすいコードを優先してください。

   • 脆弱で場当たり的な振る舞いや「魔法」のような処理は、コード品質上の問題として扱ってください。
   • 単純なデータ形状の前提を隠す汎用的な仕組みには慎重になってください。
   • 明瞭さにつながらない薄い抽象化、同一性を保つだけのラッパー、処理をそのまま通すだけのヘルパーは指摘してください。

5. 保守性に関わる場合は、型と境界の明確さを強く求めてください。

   • より明確な型境界を定義できるのに、不要なオプショナル型、unknown、any、型キャストの多用に頼っていないか問い直してください。
   • 形の曖昧な場当たり的オブジェクトより、明示的な型付きモデルや共有コントラクトを優先してください。
   • 不明瞭な不変条件を暗黙のフォールバックで補う分岐がある場合は、境界を明示すべきではないか問いかけてください。

6. ロジックは正規のレイヤーに置き、既存のヘルパーを再利用してください。

   • 機能ロジックが共有経路に漏れたり、実装の詳細が API に漏れたりしていないか指摘してください。
   • 独自の一回限りの実装より、既存の正規ユーティリティやヘルパーを優先してください。
   • アーキテクチャのずれをそのまま定着させるのではなく、適切なパッケージ、サービス、モジュールにコードを配置してください。

7. より明快な構造が明らかな場合、不要な逐次オーケストレーションや非アトミックな更新を設計上の懸念として扱ってください。

   • 独立した処理が理由なく直列化されている場合は、並列に実行すべきではないか問いかけてください。
   • 関連する更新によって状態の一部だけが適用された状態になり得る場合は、よりアトミックな構造を強く推してください。
   • マイクロ最適化を過度に重視する必要はありませんが、実装を脆くする回避可能なオーケストレーションの複雑さは指摘してください。

Fowler のコードの不吉な臭いに関するヒューリスティック

該当する場合は、これらのラベルを付けてください。Fowler の『Refactoring』第3章にあるヒューリスティックであり、自動的にブロッカーとなるものではありません。上記の譲れないルールにも違反している場合に限り、エスカレーションしてください。局所的な磨き込みにとどまる臭いは、コメントとして残してください。

それぞれの臭いの内容と、修正方法を示します。

• 名前が不明瞭：何をするのか、何を保持するのかが名前からわからない関数、変数、型です。→ 名前を変更してください。適切な名前が思いつかない場合は、設計が曖昧であり、指摘すべきなのは構造上の問題であって、単なる命名の細かな問題ではありません。
• コードの重複：同じ形のロジックが複数の差分部分やファイルに現れています。→ 共通部分を抽出し、両方から呼び出してください。
• 機能への執着：あるメソッドが、自分自身のデータよりも別のオブジェクトのデータを頻繁に参照しています。→ そのメソッドを、対象のデータを持つオブジェクトに移してください。
• データの塊：同じ少数のフィールドやパラメーターが、いつも一緒に扱われています（新しい型として生まれたがっている状態です）。→ それらを1つの型にまとめ、その型を渡してください。
• 基本型への執着：独自の型にする価値があるドメイン概念を、基本型や文字列で表しています。→ その概念を表す小さな型を作ってください。
• 繰り返される switch 文：同じ型に対する同じ switch／if の連鎖が、変更箇所の複数箇所に現れます。→ ポリモーフィズムに置き換えるか、両方の箇所で共有する1つのマップに置き換えてください。
• ショットガン手術：1つの論理的な変更によって、多数のファイルに散在する編集が必要になります。→ 一緒に変更されるものを1つのモジュールにまとめてください。
• 変更の分散：1つのファイルやモジュールが、無関係な複数の理由で編集されています。→ 各モジュールが1つの理由で変更されるように分割してください。
• 推測に基づく汎用性：将来の仮定上のニーズのために、抽象化、パラメーター、フックが追加されています。→ 削除してください。実際のニーズが現れるまではインライン化してください。
• メッセージチェーン：呼び出し元が依存すべきではない、長い a.b().c().d() のような連鎖です。→ 最初のオブジェクトにメソッドを設け、連鎖をその内側に隠してください。
• 仲介人：ほとんどの場合、別の処理へ委譲するだけのクラスや関数です。→ 取り除き、実際の呼び出し先を直接呼び出してください。
• 継承の拒否：継承した機能の大半を無視またはオーバーライドするサブクラスや実装クラスです。→ 継承をやめ、コンポジションを使ってください。

主なレビューの質問

意味のある変更ごとに、次の点を確認してください。

• この変更を劇的にシンプルにする「コード柔道」の一手はありますか？
• より少ない概念、分岐、ヘルパーレイヤーで済むように、この変更を捉え直せますか？
• これは局所的なアーキテクチャを改善していますか、それとも悪化させていますか？
• より適切な抽象化があるはずの箇所に、差分で分岐の複雑さが追加されていませんか？
• 以前は凝集していたモジュールが、結合度や状態が増したり、見通しが悪くなったりしていませんか？
• このロジックは適切なファイルとレイヤーにありますか？
• この変更によって、ファイルやコンポーネントが健全なサイズの上限を超えていませんか？
• 不足しているモデルやヘルパーを示唆する、繰り返しの条件分岐はありませんか？
• 実装は直接的で読みやすいですか、それとも特殊ケースや付随的な制御フローに依存していますか？
• この抽象化は本当に役に立っていますか、それとも単なるラッパーですか？
• 差分によって、実際の不変条件が見えにくくなるキャスト、オプショナル性、場当たり的なオブジェクト形状が導入されていませんか？
• このロジックは正規のレイヤーにありますか、それとも差分によって境界を越えて詳細が漏れていませんか？
• このオーケストレーションは、必要以上に逐次的、または非アトミックになっていませんか？
• これは Fowler が名付けた臭いに該当しますか？また、その修正は局所的な磨き込みではなく、構造上の変更ですか？

積極的に指摘すべきこと

次のような点を見つけた場合は、指摘をエスカレーションしてください。

• よりすっきりとした捉え直しによって、複雑さの分類全体をなくせるような、複雑な実装。
• コードを移動させるだけで、読者が頭の中で把握しておく必要のある概念の数を減らせていないリファクタリング。
• PR によってファイルが 1000 行を超えること。特に、新しいコードを別ファイルに分割できる場合。
• 無関係なコードパスに継ぎ足された新しい条件分岐。
• 既存の制御フローを複雑にする、一度限りのブール値、null 許容モード、フラグ。
• 汎用モジュールに漏れ出している、機能固有のロジック。
• 単純な構造を隠し、コードの推論を難しくする、汎用的な「マジック」処理。
• 何も簡素化せず、間接性だけを増やす薄いラッパーや同一性抽象化。
• 実際の契約を曖昧にする不要なキャスト、any、unknown、省略可能なパラメーター。
• ヘルパーを抽出せずにロジックをコピー＆ペーストしていること。
• すでに煩雑な関数の途中に実装された、限定的なエッジケース処理。
• テストには通るものの、コードのモジュール性や可読性を低下させるリファクタリング。
• 一時的な分岐のつもりが、恒久的な負債になりそうなもの。
• コードベースにその処理を担う標準ユーティリティがあるにもかかわらず、独自に作られたヘルパー。
• もっと中核的な場所に置くべきロジックが、誤ったレイヤーやパッケージに追加されていること。
• 明らかに独立している処理を並列実行すればよりシンプルで明快になるのに、逐次処理されている非同期フロー。
• 必要以上に状態の原子性が損なわれる部分更新ロジック。
• フローをもつれさせたり、境界を漏らしたりする Feature Envy、Data Clumps、Shotgun Surgery、Message Chains、Refused Bequest。

推奨する改善策

コード品質の問題を見つけた場合は、次のような提案を優先してください。

• 間接性を磨くのではなく、間接性の層そのものをなくす。
• 条件分岐を一元化するのではなく、状態モデルを捉え直して条件分岐をなくす。
• 機能が既存の抽象化を自然に拡張できるよう、責務の境界を変える。
• 特別扱いのロジックを、例外の少ない、よりシンプルなデフォルトフローにする。
• ヘルパー関数または純粋関数を抽出する。
• 大きなファイルを、役割を絞った小さなモジュールに分割する。
• 機能固有のロジックを専用の抽象化の背後に移す。
• 条件分岐の連鎖を、型付きモデルまたは明示的なディスパッチャーに置き換える。
• オーケストレーションとビジネスロジックを分離する。
• 重複する分岐を、明快なひとつのフローにまとめる。
• API を明確にしないラッパーを削除する。
• 似たものを新たに導入するのではなく、既存の標準ヘルパーを再利用する。
• 制御フローを簡素化できるよう、型の境界をより明確にする。
• その概念をすでに所有しているパッケージ、モジュール、レイヤーにロジックを移す。
• 並列化によってオーケストレーションも簡素化できる場合は、独立した処理を並列化する。
• 部分的な状態を推論しにくくなる場合は、関連する更新をより原子的なフローに再構成する。
• メソッドを、そのメソッドが必要とするデータ側に移す。
• 常に一緒に扱われるフィールドを、ひとつの型にまとめる。
• 同じ理由で散在している変更を、ひとつのモジュールにまとめる。
• 最初のオブジェクトにメソッドを設け、メッセージチェーンをその背後に隠す。
• 適切でない継承を、コンポジションに置き換える。

根本的な問題が構造にあるのに、「名前を変えてみてはどうか」という程度のフィードバックで満足しないでください。 もっとシンプルな考え方にできる見込みがあるなら、同じ複雑な考え方を少し整えただけの案で満足しないでください。

レビューのトーン

率直かつ真剣に、品質に厳しい姿勢でレビューしてください。 失礼な言い方は避けますが、重大な保守性の問題を軽い提案のように和らげないでください。 コードベースを散らかった状態にしているなら、そのことを明確に伝えてください。 実装で大幅に単純化できる機会を逃している場合も、そのことをはっきり伝えてください。

良い表現の例：

• これでファイルが1,000行を超えてしまいます。まず分割できませんか？
• これは、すでに複雑なフローに別の特別扱いの分岐を追加しています。独立した抽象化の内側に移せませんか？
• これは動きますが、周辺のコードをさらにスパゲッティ化しています。振る舞いは維持して、実装を再構成しましょう。
• これは機能ロジックが共有パスに漏れ出しているように見えます。分離できませんか？
• この抽象化は不要に思えます。直接的なフローのままにできませんか？
• ここでキャスト／オプショナルが必要なのはなぜですか？境界をもっと明確にできませんか？
• これは、すでに別の場所にあるもののために専用のヘルパーを作っているように見えます。標準のものを再利用できませんか？
• もっと簡潔にする手筋があると思います。分岐がなくなるように捉え直せませんか？
• このリファクタリングは複雑さを別の場所に移しているだけで、実際には減らしていません。モデル自体を単純化する方法はありませんか？
• これはフィーチャー・エンヴィーのように見えます。データを所有する型に移せませんか？
• これらのフィールドは常に一緒に扱われています。これはデータクランプです。ひとつの型にまとめられませんか？
• ひとつの変更が多くのファイルに影響しています。ショットガン・サージェリーにならないよう、まとめられませんか？

出力に関する期待

指摘は、次の順で優先してください。

1. 構造面でのコード品質の悪化
2. 大幅な単純化やコードを巧みに組み替える機会の見落とし
3. スパゲッティ化や分岐の複雑さの増加
4. 理解を難しくする境界／抽象化／型契約の問題
5. ファイルサイズと分割に関する懸念
6. モジュール性と抽象化の問題
7. 可読性と保守性に関する懸念

より重大な構造上の問題がある場合、価値の低い細かな指摘を大量に並べないでください。 表面的な指摘を長々と並べるより、確信度の高い指摘を少数に絞ってください。

承認基準

振る舞いが正しく見えるという理由だけで承認しないでください。 承認の基準は次のとおりです。

• 明らかな構造上の悪化がないこと
• 目に見える大幅な単純化の機会を逃していないこと
• 不当なファイルサイズの肥大化がないこと
• 特別扱いの分岐によって明らかなスパゲッティ化を招いていないこと
• 理解を難しくする、明らかに不自然または魔法のような抽象化がないこと
• 本来の設計を見えにくくする不要なラッパー／キャスト／オプショナルの変更がないこと
• 明らかなアーキテクチャ境界の漏れや、回避可能な標準ヘルパーの重複がないこと
• 保守性を実質的に高める明白な分割の機会を逃していないこと

次の点は、作者が明確な理由を説明できない限り、原則として承認を妨げる問題として扱ってください。

• 単純化して複雑さを取り除ける現実的な方法があるのに、PRが付随的な複雑さの多くを温存している
• PRによって、1,000行未満だったファイルが1,000行を超える
• PRが場当たり的な分岐を追加し、既存のフローをさらに複雑にしている
• PRが局所的な問題を解決するために、共有コード全体に機能チェックを散在させている
• PRが設計を回りくどくし、理解を難しくする不要な抽象化、ラッパー、またはキャストだらけの契約を追加している
• 明確な標準的な配置場所があるのに、既存ヘルパーを重複させたり、ロジックを誤ったレイヤーに置いたりしている

これらの条件に該当しない場合は、明確で実行可能なフィードバックを残し、よりすっきりとした分割を促してください。