SOLIDレビュー
著者: Guerth Castro (github.com/GuerthCastro)。MITライセンスのもとで提供。
レビュープロセス
コードがレビューのために提供された場合:
- 言語とレイヤーを特定する（C#ドメインエンティティ、アプリケーションハンドラー、Angularサービス、Reactコンポーネントなど）。
- 5つの原則をそれぞれ独立して評価する。
- 以下のフォーマットを使用して構造化レポートを作成する。
- 違反を重大度でランク付けする: Critical（アーキテクチャを破壊） > Major（保守性を低下） > Minor（スタイルの好み）。
- CriticalまたはMajorの違反ごとに、具体的なリファクタリングバージョンを提示する。
レポートを膨らませないこと。原則が実際に満たされている場合は、PASSと表示して次に進む。
5つの原則
S: 単一責任の原則
1つのクラス、1つの変更理由。
探すべき違反:
- クラスがデータアクセスとビジネスロジックの両方を行う。
- サービスがメール送信、データ検証、データベース書き込みをすべて行う。
- コンポーネントがデータ取得、フォーマット、UI状態管理をすべて行う。
- ハンドラーが複数の無関係な操作を調整する。
- 10以上のメソッドを持ち、複数のドメインにまたがる神クラス。
違反。 OrderService が検証、永続化、通知を行っている。通知呼び出しが本来所属すべきでない部分。
public class OrderService(IOrderRepository repository, IEmailService email)
{
public async Task<bool> AddItem(long orderId, long productId)
{
Order order = await repository.GetById(orderId);
if (order.ItemCount >= order.MaxItems)
{
return false;
}
await repository.AddItem(orderId, productId);
await email.SendOrderUpdated(orderId);
return true;
}
}
修正: 通知サービスを抽出し、ハンドラーに2つの呼び出しを調整させる。
O: 開放閉鎖の原則
拡張に対して開き、修正に対して閉じていること。
探すべき違反:
- 新しい機能ごとに成長する、型文字列やenumに対する
if/else ifチェーンまたはswitchブロック。 - 新しいレポートタイプを追加するために既存のレポートサービスの編集が必要。
- 新しい支払い方法を追加するためにチェックアウトハンドラーの編集が必要。
- 抽象化が属する場所での具象依存関係。
違反。 新しいエクスポート形式ごとにこのメソッドの編集が必要。
public byte[] ExportInvoice(string format, IEnumerable<InvoiceLine> lines)
{
if (format == "pdf")
{
return GeneratePdf(lines);
}
if (format == "excel")
{
return GenerateExcel(lines);
}
throw new NotSupportedException(format);
}
修正: IInvoiceExporter 抽象化と、PdfInvoiceExporter および ExcelInvoiceExporter の実装を作成し、コンテナからキーで解決する。
L: リスコフ置換の原則
サブタイプは基本型に置き換え可能でなければならない。
探すべき違反:
- 派生クラスが継承メンバーに対して
NotImplementedExceptionをスローする。 - オーバーライドが期待される動作を変更する: サイレントなno-op、異なる戻り値の形状、新しい例外タイプ。
- インターフェースが、一部の実装で意味のある実装ができないメンバーを宣言している。
- 動作に到達するために具象型にキャストする:
if (repository is SqlOrderRepository sql)。
I: インターフェース分離の原則
クラスは使用しないメンバーの実装を強制されるべきではない。
探すべき違反:
- 15以上のメンバーを持つ巨大インターフェースで、実装では3つしか使用しない。
- 汎用的な
IRepository<T>が、すべてのエンティティにGetAll、GetById、Create、Update、Delete、Paginate、Search、BulkInsertを強制する。 - 読み取りと書き込みを混在させるサービスインターフェースで、コンシューマーが片方しか必要としない場合。
違反。 読み取り専用のレポートコンシューマーが、Create、Update、Delete を強制的に見せられる。
public interface IOrderRepository
{
Task<Order> GetById(long id);
Task<IEnumerable<Order>> GetAll();
Task Create(Order order);
Task Update(Order order);
Task Delete(long id);
}
修正: IOrderReader と IOrderWriter に分割する。両方が必要な場合はそれらを合成する。
D: 依存性逆転の原則
具象ではなく抽象に依存すること。
探すべき違反:
- ビジネスロジック内で
newを使用してサービスやリポジトリをインスタンス化する。 - サービス内の
SqlConnectionやNpgsqlConnectionへの直接依存。 - アプリケーションレイヤーがインフラストラクチャ型をインポートする。
- 可変状態を保持する静的サービスクラス。
違反。 ハンドラーが自身の具象リポジトリと接続を構築している。
public class GetOrderHandler
{
public async Task<OrderDto> Handle(long id)
{
OrderRepository repository = new(new SqlConnection("..."));
Order order = await repository.GetById(id);
return order.MapToDto();
}
}
修正: プライマリコンストラクターを通じて IOrderRepository を注入し、コンポジションルートでコンテナに登録する。
レポートフォーマット
すべてのレビューで以下の構造を生成すること:
## SOLID Review: [クラス名またはファイル名]
**言語/レイヤー:** C# Application Handler, Angular Service など
### S: 単一責任
ステータス: PASS / VIOLATION (Critical, Major, Minor)
発見: [クラスが単一責任を超えて行っていること]
修正: [具体的なアクション]
### O: 開放閉鎖
ステータス: PASS / VIOLATION
発見: ...
修正: ...
### L: リスコフ置換
ステータス: PASS / VIOLATION / N/A (継承なし)
発見: ...
### I: インターフェース分離
ステータス: PASS / VIOLATION / N/A (インターフェースなし)
発見: ...
### D: 依存性逆転
ステータス: PASS / VIOLATION
発見: ...
修正: ...
### 優先違反
1. [Critical] DIP: ハンドラーが具象リポジトリをインスタンス化
2. [Major] SRP: サービスが通知メールも送信
### リファクタリングバージョン
[完全な修正コードブロック]
クリーンアーキテクチャのレイヤー違反
SOLIDに加えて、以下の横断的な問題をフラグ付けする:
- ドメインエンティティがアプリケーションまたはインフラストラクチャ型をインポートする（依存関係方向の違反）。
- コントローラーがハンドラーに属するビジネスロジックを含む。
- リポジトリがドメインエンティティの代わりにDTOを返す。マッピングはアプリケーションに属する。
- インフラストラクチャが、アプリケーションまたはドメインで宣言されたインターフェースを実装する代わりに、外部APIを直接呼び出す。
- ハンドラーが
HttpContextに触れ、インフラストラクチャの関心事をアプリケーションレイヤーに漏洩させる。 - DI登録が誤ったアセンブリにあるため、レイヤーがコンパイルのために下位を参照する必要がある。