SOLID Principles Code Review

SOLID Principles Code Review

Overview

This skill evaluates Python code against the five SOLID principles of object-oriented design. SOLID principles guide the creation of maintainable, flexible, and testable code.

Scope: This skill focuses exclusively on SOLID principle violations and design patterns, not general code quality, performance, or formatting issues.

The Five Principles:

• Single Responsibility Principle (SRP): One class, one responsibility
• Open/Closed Principle (OCP): Open for extension, closed for modification
• Liskov Substitution Principle (LSP): Subclasses must be substitutable for parent classes
• Interface Segregation Principle (ISP): Many small interfaces > one large interface
• Dependency Inversion Principle (DIP): Depend on abstractions, not concrete implementations

When to Use

• Reviewing new features or refactoring code
• Evaluating code maintainability and testability
• Identifying design issues that make code hard to change
• Before merging significant architectural changes
• When code is becoming difficult to test or extend

Process

Focus only on SOLID principle violations.

Step 1: Single Responsibility Principle (SRP)

Look for:

• Classes with multiple reasons to change
• Methods that do more than one thing
• Mixed concerns (e.g., business logic + database access)
• Boolean flags that change method behavior
• Long classes (>200 lines often indicates multiple responsibilities)

Red Flags:

# BAD: Multiple responsibilities
class Order:
    def calculate_total(self): pass  # Calculation
    def save_to_database(self): pass  # Persistence
    def send_email(self): pass        # Notification

Good Pattern:

# GOOD: Single responsibilities
class Order:
    pass  # Just data

class OrderCalculator:
    def calculate_total(self, order): pass

class OrderRepository:
    def save(self, order): pass

class OrderNotifier:
    def send_confirmation(self, order): pass

Step 2: Open/Closed Principle (OCP)

Look for:

• if-elif chains checking types or categories
• Type checking with isinstance() or type()
• Methods that must be modified to add new functionality
• Hard-coded lists of types

Red Flags:

# BAD: Must modify for new shapes
def calculate_area(shape):
    if shape.type == "circle":
        return 3.14 * shape.radius ** 2
    elif shape.type == "rectangle":
        return shape.width * shape.height
    # Adding triangle requires modifying this function

Good Pattern:

# GOOD: Extend via inheritance or protocol
from abc import ABC, abstractmethod

class Shape(ABC):
    @abstractmethod
    def calculate_area(self): pass

class Circle(Shape):
    def calculate_area(self):
        return 3.14 * self.radius ** 2

class Rectangle(Shape):
    def calculate_area(self):
        return self.width * self.height

Step 3: Liskov Substitution Principle (LSP)

Look for:

• Subclasses throwing exceptions that parent doesn't throw
• Empty method implementations or NotImplementedError
• Subclasses requiring more restrictive inputs than parent
• Type checking before calling methods (indicates substitution doesn't work)
• Subclasses that weaken postconditions or strengthen preconditions

Red Flags:

# BAD: Square can't substitute for Rectangle
class Rectangle:
    def set_width(self, width):
        self._width = width
    def set_height(self, height):
        self._height = height

class Square(Rectangle):
    def set_width(self, width):
        self._width = width
        self._height = width  # Unexpected side effect!

Good Pattern:

# GOOD: Proper abstraction hierarchy
from abc import ABC, abstractmethod

class Shape(ABC):
    @abstractmethod
    def get_area(self): pass

class Rectangle(Shape):
    def get_area(self):
        return self._width * self._height

class Square(Shape):  # Not a Rectangle!
    def get_area(self):
        return self._side ** 2

Step 4: Interface Segregation Principle (ISP)

Look for:

• Large abstract classes with many methods (>5-7 methods)
• Implementations with empty methods or NotImplementedError
• Classes implementing interfaces but only using a subset
• Comments like "not applicable" or "not supported"

Red Flags:

# BAD: Fat interface forces all implementations
from abc import ABC, abstractmethod

class MultiFunctionDevice(ABC):
    @abstractmethod
    def print_document(self, doc): pass
    @abstractmethod
    def scan_document(self): pass
    @abstractmethod
    def fax_document(self, doc): pass

class SimplePrinter(MultiFunctionDevice):
    def print_document(self, doc):
        print(doc)
    def scan_document(self):
        raise NotImplementedError  # Forced to implement!
    def fax_document(self, doc):
        raise NotImplementedError  # Forced to implement!

Good Pattern:

# GOOD: Small, focused interfaces
from abc import ABC, abstractmethod

class Printer(ABC):
    @abstractmethod
    def print_document(self, doc): pass

class Scanner(ABC):
    @abstractmethod
    def scan_document(self): pass

class SimplePrinter(Printer):
    def print_document(self, doc):
        print(doc)

class MultiFunctionPrinter(Printer, Scanner):
    def print_document(self, doc):
        print(doc)
    def scan_document(self):
        return "scanned"

Step 5: Dependency Inversion Principle (DIP)

Look for:

• Direct imports and instantiation of concrete classes in business logic
• Hard-coded dependencies created inside __init__
• Difficult-to-test code requiring real databases/files/network
• High-level modules depending on low-level implementation details

Red Flags:

# BAD: Direct dependency on concrete implementation
import sqlite3

class UserService:
    def __init__(self):
        # Hard-coded dependency
        self.db = sqlite3.connect('users.db')

    def register_user(self, name, email):
        # Business logic coupled to SQLite
        self.db.execute("INSERT INTO users VALUES (?, ?)", (name, email))

Good Pattern:

# GOOD: Depend on abstraction via injection
from abc import ABC, abstractmethod

class UserRepository(ABC):
    @abstractmethod
    def save(self, user_data): pass

class SQLiteUserRepository(UserRepository):
    def __init__(self, db_path):
        self.db = sqlite3.connect(db_path)
    def save(self, user_data):
        self.db.execute("INSERT INTO users VALUES (?, ?)",
                       (user_data['name'], user_data['email']))

class UserService:
    def __init__(self, repository: UserRepository):
        self.repository = repository  # Injected dependency

    def register_user(self, name, email):
        self.repository.save({'name': name, 'email': email})

Step 6: Python-Specific Considerations

Prefer:

• Protocol over ABC for structural subtyping (when runtime enforcement not needed)
• Type hints to make dependencies explicit
• Dataclasses for data structures (supports SRP)
• Decorators for cross-cutting concerns (supports OCP)
• Constructor injection for dependency injection (simplest DIP approach)

Example with Protocol:

from typing import Protocol

class Repository(Protocol):
    def save(self, data: dict) -> None: ...
    def find(self, id: str) -> dict | None: ...

# No inheritance needed - duck typing with type hints
class InMemoryRepository:
    def save(self, data: dict) -> None: pass
    def find(self, id: str) -> dict | None: pass

Output Format

For each SOLID violation found:

File: [file path] Lines: [line range, e.g., "45-52" or "78"] Principle: [SRP|OCP|LSP|ISP|DIP] Issue: [brief description of the violation] Impact: [why this matters - testability, maintainability, flexibility] Suggestion: [specific refactoring approach or pattern to apply] Example: [code snippet showing the improved design, if helpful]

Decision Rules

When to Flag Issues

Always flag:

• NotImplementedError in production code (ISP/LSP violation)
• Multiple isinstance() checks for behavior switching (OCP violation)
• Can't write unit tests without real database/network (DIP violation)
• Classes over 500 lines (likely SRP violation)
• Subclasses throwing exceptions parent doesn't throw (LSP violation)

Flag if significant:

• Classes with 5+ distinct responsibilities (SRP)
• Abstract classes with 7+ methods (ISP)
• Long parameter lists (might indicate missing abstractions - DIP)
• Type-based conditionals (OCP)

Don't flag:

• Simple scripts (<100 lines total) - SOLID overkill for simple code
• Prototypes or proof-of-concept code
• Business rule conditionals (e.g., if total > 100:) - these are logic, not type switching
• One-time migration scripts
• Proper use of Python idioms (duck typing, EAFP style)

Balancing Pragmatism

SOLID principles are guidelines, not absolute rules. Consider:

• Code lifecycle: Is this code actively maintained or rarely changed?
• Team context: Will abstractions help or confuse the team?
• Actual pain points: Is the code currently causing problems?
• Project size: Small projects need less abstraction than large systems

Suggest refactoring when:

• Code is hard to test
• Code changes frequently break unrelated features
• Adding new features requires modifying existing code
• Team struggles to understand or modify the code

Defer refactoring when:

• Code works and rarely changes
• Refactoring is purely academic
• Risk of bugs outweighs benefits
• Code is scheduled for deprecation

Anti-patterns

• ❌ Don't: Flag every conditional as an OCP violation

  • ✅ Do: Only flag type-based conditionals that require modification for new types

• ❌ Don't: Suggest SOLID refactoring for simple scripts

  • ✅ Do: Consider code context and lifecycle before suggesting abstractions

• ❌ Don't: Recommend ABC when Protocol would work better

  • ✅ Do: Prefer Protocol for flexibility unless runtime enforcement needed

• ❌ Don't: Focus only on violations without explaining impact

  • ✅ Do: Explain why the violation matters (testability, maintainability, etc.)

• ❌ Don't: Suggest over-engineering with excessive abstraction layers

  • ✅ Do: Recommend the simplest solution that solves the actual problem

Examples

Example 1: SRP Violation

Context: Order class doing too much

Code:

class Order:
    def calculate_total(self):
        return sum(item.price for item in self.items)

    def save_to_database(self, db):
        db.execute("INSERT INTO orders VALUES (?)", (self.total,))

    def send_confirmation_email(self, email_service):
        email_service.send("Order confirmed", self.customer_email)

Review Output:

---
**File**: order.py
**Lines**: 1-8
**Principle**: SRP
**Issue**: Order class has three responsibilities: calculation, persistence, and notification
**Impact**: Changes to email format require modifying Order class; difficult to test calculation logic without mocking database and email
**Suggestion**: Extract into separate classes: OrderCalculator, OrderRepository, OrderNotificationService
**Example**:
class Order:
    pass  # Data only

class OrderCalculator:
    def calculate_total(self, order):
        return sum(item.price for item in order.items)

class OrderRepository:
    def save(self, order, db):
        db.execute("INSERT INTO orders VALUES (?)", (order.total,))

class OrderNotifier:
    def send_confirmation(self, order, email_service):
        email_service.send("Order confirmed", order.customer_email)
---

Example 2: DIP Violation

Context: Service depends on concrete implementation

Code:

import sqlite3

class UserService:
    def __init__(self):
        self.db = sqlite3.connect('users.db')

    def create_user(self, name, email):
        self.db.execute("INSERT INTO users VALUES (?, ?)", (name, email))

Review Output:

---
**File**: user_service.py
**Lines**: 3-8
**Principle**: DIP
**Issue**: UserService directly depends on SQLite implementation; cannot test without real database
**Impact**: Impossible to write unit tests with mocks; cannot switch to different database without modifying UserService
**Suggestion**: Introduce repository abstraction and inject via constructor
**Example**:
from typing import Protocol

class UserRepository(Protocol):
    def save(self, name: str, email: str) -> None: ...

class SQLiteUserRepository:
    def __init__(self, db_path: str):
        self.db = sqlite3.connect(db_path)
    def save(self, name: str, email: str) -> None:
        self.db.execute("INSERT INTO users VALUES (?, ?)", (name, email))

class UserService:
    def __init__(self, repository: UserRepository):
        self.repository = repository
    def create_user(self, name: str, email: str) -> None:
        self.repository.save(name, email)

# Easy to test with mock
mock_repo = Mock(spec=UserRepository)
service = UserService(mock_repo)
---

Testing This Skill (TDD Validation)

Test cases are located in skills/collaboration/solid-principles-review/examples/:

1. example-1-srp-violation: Order class with multiple responsibilities

   • Expected: Flag calculation + persistence + notification in one class

2. example-2-ocp-violation: Type-based conditionals requiring modification

   • Expected: Flag if-elif chain checking shape types

3. example-3-dip-violation: Hard-coded database dependency

   • Expected: Flag direct SQLite instantiation in business logic

4. example-4-complex-good: Well-designed code following SOLID

   • Expected: Find zero violations or only minor improvements

Run tests by launching subagents with:

Apply skills/collaboration/solid-principles-review/SKILL.md to review [test file path]