SOLID 設計レビュー
エージェントに対して、C# のクラスまたはモジュールを、具体的な .NET の視点を用いて SOLID 原則ごとにレビューし、「不吉な匂い」「違反している原則」「最小限のリファクタリング」を特定するよう指示します。教科書的な細かすぎる指摘ではなく、実際の結合や変更の多さを引き起こす違反を優先します。
使用すべき状況
- クラスが扱いにくくなっている — IO、ビジネスロジック、マッピング、副作用が1つのファイルに混在している場合。
- PR で新しい型のバリアントが導入され、それに対応するために既存の
switchまたはif-elseチェーンを編集する必要がある場合。 - コンストラクタが自身の依存関係を
newで生成しているか、DateTime.Nowやstaticヘルパーを直接参照している場合。 - インターフェースに、無関係な複数の呼び出し元を満たすためにメンバーが累積され、no-op 実装を強制している場合。
プロセス
- レビュー対象のクラス/モジュールを特定する。 その責務を明確に述べます。もし説明に「および」「または」が必要なら、それはすでにシグナルです。
- 各 SOLID 原則を順番に、以下の C# レンズを使用して検討します。 それぞれを独立したパスとして扱い、最初の発見で終わらないでください。
- 不吉な匂いと違反している原則を明確にします。 「
HttpClientとSqlConnectionを自身で組み立てる太ったコンストラクタ」→ DIP、「NotSupportedExceptionをスローする基底のオーバーライド」→ LSP。正確に記述してください。 - 最小限のリファクタリングと C# のメカニズムを提案します。 型、パターン、DI 登録の変更を具体的に—「原則を適用する」だけでは不十分です。「SOLID に従う」という一般的なアドバイスは発見とはみなしません。
- 実際の影響に基づいて優先順位を付けます。 EF 永続化とメール送信を混在させる神サービスは毎スプリント変更を引き起こします。12 のメンバーを持つ
IOrderServiceで、どの呼び出し元もすべてを使い切っていない場合は、真の ISP 違反です。たまたま2つの緩く関連するユースケースを満たすsealedクラスでも、変更理由が1つなら SRP 違反ではありません。害を及ぼす違反と、単に不純であるだけの違反を区別してください。
.NET / Azure チェック
SRP — 単一責任の原則
- 神サービス / 神クラス。 EF Core（
_context.SaveChangesAsync）、メール送信（_emailSender.SendAsync）、ビジネスルールの検証、DTO へのマッピングに触れる単一のクラスは、変更理由が複数あります。IOrderValidator、IOrderRepository（または専用の永続化クラスで DbContext を直接使用）、IOrderNotifierを個別の協力者として抽出します。 - ファットコントローラー。
if/検証ロジックを含み、データベースを直接呼び出し、応答ペイロードを整形する ASP.NET Core コントローラーは、3つの仕事をしています。コントローラーはオーケストレーションを行うべきです：サービスを呼び出し、結果をマッピングし、IActionResultを返します。ビジネスルールはアプリケーション/ドメインレイヤーに属します。 - 型名または責務に「および」が含まれる。
OrderValidationAndPersistenceServiceは SRP 違反を示す命名上の不吉な匂いです。クラスを「および」を使ってしか説明できないなら、分割してください。 - 変更理由が複数ある。 「メールプロバイダを変更したらこのクラスは変更が必要か？データベーススキーマを変更したら？ビジネスルールが変わったら？」と問いかけてください。複数の「はい」は、複数の責務があることを意味します。
OCP — オープン/クローズドの原則
- 型判別子による
switch/if-elseの増加。 サービスクラス内のswitch(order.Type)ブロックは、新しいOrderType列挙値ごとに再オープンが必要なため OCP 違反です。戦略に置き換えます：IOrderHandler（またはIOrderProcessor）を定義し、タイプごとに1つの実装を登録します。コーディネーターに注入されたIEnumerable<IOrderHandler>経由で解決し、handler.Handles(order.Type)でディスパッチします。 - ハードコードされた動作の拡張。 新しい割引ルール、通知チャネル、エクスポート形式を追加するために既存のクラスを編集する必要があり、新しい
IDiscountRuleやIExporterを追加するだけでは済まない場合、そのクラスは修正に対して閉じていません。2番目のケースが来る前に抽象化を導入してください。 - 型判別子アンチパターン。 処理クラス内の
if (order is PremiumOrder)やif (typeof(T) == typeof(FooOrder))は、上記のswitchと同等のコードの不吉な匂いです。ポリモーフィズムまたは戦略レジストリによってこれらを排除します。
LSP — リスコフの置換原則
throw new NotSupportedException()/NotImplementedException()をスローするオーバーライド。 基本契約を尊重できないサブタイプ（例：AddでスローされるReadOnlyCollection<T>）は LSP に違反します。オーバーライドを意味のある形で実装できない場合、階層が間違っています—コンポジションまたは個別のインターフェースを優先します。- サブタイプが事後条件を弱める。 基本クラスが
GetOrders()が決してnullを返さないことを保証している場合、nullを返すオーバーライドは、呼び出し元が依存する契約に違反します。不変条件を文書化し（XML ドキュメントまたは契約）、オーバーライドで強制します。 - サブタイプが事前条件を強化する。 基本クラスにない null チェックを追加する、または基本クラスが受け入れた有効なパラメータ範囲を拒否するオーバーライドは、基本契約に従った有効な入力を受け付けるコードを破壊します。サブタイプは基本クラスが受け入れたものすべてを受け入れなければなりません。
- 呼び出しコードでの
is/asダウンキャストの蔓延。 呼び出し元でif (handler is SpecialHandler sh) sh.SpecialMethod()が頻繁に見られる場合、抽象化が壊れていることを意味します — メソッドは基本インターフェースに属しているか、型が同じ階層を共有すべきではないかのどちらかです。頻繁なダウンキャストは LSP / 抽象化の失敗のシグナルです。
ISP — インターフェース分離の原則
- 無関係な関心事を持つファットインターフェース（10 以上のメンバー）。
CreateOrder、CancelOrder、GetInvoice、SendConfirmation、ExportToCsv、CalculateTax、ValidateAddress、GetShippingRatesを公開するIOrderServiceは、すべての実装者とテストダブルに、使用しないメソッドのスタブを強制します。ロールインターフェースに分割します：IOrderWriter、IOrderReader、IOrderNotifier、IOrderExporter。 - No-op 実装 /
NotImplementedExceptionスタブ。 モックやテスト実装が、インターフェースメンバーの半分に対してthrow new NotImplementedException()を返す場合、それらのメンバーは異なるロールインターフェースに属しています。no-op の存在は ISP 違反の直接的な証拠です。 - クライアントが決して呼び出さないメソッドをインポートしている。
PaymentServiceがIOrderServiceに依存しているが、GetOrderしか呼び出さない場合、IOrderReaderに依存すべきです。依存関係を狭めることで結合が明示的になり、テストが安価になります。
DIP — 依存性逆転の原則
- クラス内部で具象依存関係を
newしている。 サービス内部でnew HttpClient()を構築すると、隠れた依存関係が作成され、DI コンテナをバイパスし、ソケットリーク（コネクションプーリングがない）が発生し、リフレクションのトリックなしではクラスがテスタブルでなくなります。IHttpClientFactory（型付きクライアント）または名前付きクライアントをコンストラクタ経由で注入します。new SqlConnection(connectionString)も同様 —IDbConnectionFactoryを注入するか、EF Core / Dapper に登録されたDbContextを介して接続管理を任せます。 - 抽象化ではなく具象に依存している。
IOrderRepositoryではなくSqlOrderRepositoryに型指定されたコンストラクタは、クラスをストレージ技術に結合します。再利用可能または単独でテスト可能であるべきクラスは、インターフェースまたは抽象型にのみ依存しなければなりません。 DateTime.Now/ 静的状態への隠れた結合。 ビジネスメソッド内のvar now = DateTime.Nowは、クラスを非決定的にし、タイムトラベルハックなしではテスト不可能にします。ISystemClock（または .NET 8+ のTimeProvider）を導入し、注入します。Environment.MachineName、Random.Shared、その他のアンビエントな静的状態にも同様です。- 静的可変シングルトン。 サービスクラス内の
static Dictionary<string, Order> _cacheは、DI のライフタイム管理をバイパスし、並行使用に対して安全でない隠れた共有状態です。IMemoryCache/IDistributedCacheを DI 経由で注入し、適切なスコープとエビクションをサポートするものに置き換えます。
判断の注意: すべての
newが DIP 違反というわけではありません。new OrderDto(order)（単なるデータ入れ）やnew List<T>()（標準コレクション）は問題ありません。DIP が問題になるのは、構築されるオブジェクトが動作、外部 I/O、設定を持つ場合 — つまり、テストダブルや代替実装に交換する価値がある場合です。
レッドフラグ
| シグナル | なぜ重要か |
|---|---|
サービスメソッドやコンストラクタ内で new HttpClient() を構築する | IHttpClientFactory のコネクションプーリングをバイパス（負荷時のソケット枯渇）、依存関係を隠し、本番 I/O なしではクラスをテスト不可能にする。 |
コーディネータークラスでの switch(order.Type) または if (x is ConcreteType) | 新しいバリアントごとに再オープンが必要。呼び出し元が型階層を知りすぎている。DI から解決された IEnumerable<IHandler> に置き換える。 |
非抽象オーバーライドでの throw new NotImplementedException() または throw new NotSupportedException() | サブタイプが基本契約を尊重できない — LSP 違反。メソッドを正しく実装するか、階層を再構築する。 |
| 10 以上のメンバーを持つインターフェースで、呼び出し元が 2～3 しか使用しない | 巨大なテストダブルを強制。複数のロールインターフェースが1つに統合されているシグナル。 |
new AppDbContext(options) で自身の DbContext を構築するクラス | DI のライフタイム管理（Scoped vs Transient vs Singleton）をバイパスし、コネクションプーリングを防ぎ、テスト可能性を損なう。AppDbContext または IDbContextFactory<AppDbContext> を注入する。 |
ビジネスメソッド内で直接 DateTime.Now を呼び出す | テスト下で非決定的。システムクロックへの結合が論理的依存関係を隠す。TimeProvider（.NET 8+）または ISystemClock を注入する。 |
| ビジネスロジックや EF クエリを含むコントローラーアクション | コントローラーがアプリケーション/ドメインレイヤーに属する責務を引き受けている。ビジネスルールの変更がコントローラーに影響する。 |
インプロセスキャッシュとして使用される static 可変フィールド | 明示的なロックなしではスレッドセーフでない。DI スコープをバイパス。エビクションや監視ができない。IMemoryCache を使用する。 |
例
examples/solid-review/ を参照してください。