code-smell-detector

This skill identifies anti-patterns that violate amplihack's development philosophy and provides constructive, specific fixes. It ensures code maintains ruthless simplicity, modular design, and zero-BS implementations.

• Code review: Identify violations before merging
• Refactoring: Find opportunities to simplify and improve code quality
• New module creation: Catch issues early in development
• Philosophy compliance: Ensure code aligns with amplihack principles
• Learning: Understand why patterns are problematic and how to fix them
• Mentoring: Educate team members on philosophy-aligned code patterns

Amplihack Development Philosophy focuses on:

• Ruthless Simplicity: Every abstraction must justify its existence
• Modular Design (Bricks & Studs): Self-contained modules with clear connection points
• Zero-BS Implementation: No stubs, no placeholders, only working code
• Single Responsibility: Each module/function has ONE clear job

1. Over-Abstraction

What It Is: Unnecessary layers of abstraction, generic base classes, or interfaces that don't provide clear value.

Why It's Bad: Violates "ruthless simplicity" - adds complexity without proportional benefit. Makes code harder to understand and maintain.

Red Flags:

• Abstract base classes with only one implementation
• Generic helper classes that do very little
• Deep inheritance hierarchies (3+ levels)
• Interfaces for single implementations
• Over-parameterized functions

Example - SMELL:

```python

# BAD: Over-abstracted

class DataProcessor(ABC):

@abstractmethod

def process(self, data):

pass

class SimpleDataProcessor(DataProcessor):

def process(self, data):

return data * 2

```

Example - FIXED:

```python

# GOOD: Direct implementation

def process_data(data):

"""Process data by doubling it."""

return data * 2

```

Detection Checklist:

• [ ] Abstract classes with only 1-2 concrete implementations
• [ ] Generic utility classes that don't encapsulate state
• [ ] Type hierarchies deeper than 2 levels
• [ ] Mixins solving single problems

Fix Strategy:

1. Identify what the abstraction solves
2. Check if you really need multiple implementations now
3. Delete the abstraction - use direct implementation
4. If multiple implementations needed later, refactor then
5. Principle: Avoid future-proofing

---

2. Complex Inheritance

What It Is: Deep inheritance chains, multiple inheritance, or convoluted class hierarchies that obscure code flow.

Why It's Bad: Makes code hard to follow, creates tight coupling, violates simplicity principle. Who does what becomes unclear.

Red Flags:

• 3+ levels of inheritance (GrandparentClass -> ParentClass -> ChildClass)
• Multiple inheritance from non-interface classes
• Inheritance used for code reuse instead of composition
• Overriding multiple levels of methods
• "Mixin" classes for cross-cutting concerns

Example - SMELL:

```python

# BAD: Complex inheritance

class Entity:

def save(self): pass

def load(self): pass

class TimestampedEntity(Entity):

def add_timestamp(self): pass

class AuditableEntity(TimestampedEntity):

def audit_log(self): pass

class User(AuditableEntity):

def authenticate(self): pass

```

Example - FIXED:

```python

# GOOD: Composition over inheritance

class User:

def __init__(self, storage, timestamp_service, audit_log):

self.storage = storage

self.timestamps = timestamp_service

self.audit = audit_log

def save(self):

self.storage.save(self)

self.timestamps.record()

self.audit.log("saved user")

```

Detection Checklist:

• [ ] Inheritance depth > 2 levels
• [ ] Multiple inheritance from concrete classes
• [ ] Methods overridden at multiple inheritance levels
• [ ] Inheritance hierarchy with no code reuse

Fix Strategy:

1. Use composition instead of inheritance
2. Pass services as constructor arguments
3. Each class handles its own responsibility
4. Easier to test, understand, and modify

---

3. Large Functions (>50 Lines)

What It Is: Functions that do too many things and are difficult to understand, test, and modify.

Why It's Bad: Violates single responsibility, makes testing harder, increases bug surface area, reduces code reusability.

Red Flags:

• Functions with >50 lines of code
• Multiple indentation levels (3+ nested if/for)
• Functions with 5+ parameters
• Functions that need scrolling to see all of them
• Complex logic that's hard to name

Example - SMELL:

```python

# BAD: Large function doing multiple things

def process_user_data(user_dict, validate=True, save=True, notify=True, log=True):

if validate:

if not user_dict.get('email'):

raise ValueError("Email required")

if not '@' in user_dict['email']:

raise ValueError("Invalid email")

user = User(

name=user_dict['name'],

email=user_dict['email'],

phone=user_dict['phone']

)

if save:

db.save(user)

if notify:

email_service.send(user.email, "Welcome!")

if log:

logger.info(f"User {user.name} created")

# ... 30+ more lines of mixed concerns

return user

```

Example - FIXED:

```python

# GOOD: Separated concerns

def validate_user_data(user_dict):

"""Validate user data structure."""

if not user_dict.get('email'):

raise ValueError("Email required")

if '@' not in user_dict['email']:

raise ValueError("Invalid email")

def create_user(user_dict):

"""Create user object from data."""

return User(

name=user_dict['name'],

email=user_dict['email'],

phone=user_dict['phone']

)

def process_user_data(user_dict):

"""Orchestrate user creation workflow."""

validate_user_data(user_dict)

user = create_user(user_dict)

db.save(user)

email_service.send(user.email, "Welcome!")

logger.info(f"User {user.name} created")

return user

```

Detection Checklist:

• [ ] Function body >50 lines
• [ ] 3+ levels of nesting
• [ ] Multiple unrelated operations
• [ ] Hard to name succinctly
• [ ] 5+ parameters

Fix Strategy:

1. Extract helper functions for each concern
2. Give each function a clear, single purpose
3. Compose small functions into larger workflows
4. Each function should fit on one screen
5. Easy to name = usually doing one thing

---

4. Tight Coupling

What It Is: Modules/classes directly depend on concrete implementations instead of abstractions, making them hard to test and modify.

Why It's Bad: Changes in one module break others. Hard to test in isolation. Violates modularity principle.

Red Flags:

• Direct instantiation of classes inside functions (db = Database())
• Deep attribute access (obj.service.repository.data)
• Hardcoded class names in conditionals
• Module imports everything from another module
• Circular dependencies between modules

Example - SMELL:

```python

# BAD: Tight coupling

class UserService:

def create_user(self, name, email):

db = Database() # Hardcoded dependency

user = db.save_user(name, email)

email_service = EmailService() # Hardcoded dependency

email_service.send(email, "Welcome!")

return user

def get_user(self, user_id):

db = Database()

return db.find_user(user_id)

```

Example - FIXED:

```python

# GOOD: Loose coupling via dependency injection

class UserService:

def __init__(self, db, email_service):

self.db = db

self.email = email_service

def create_user(self, name, email):

user = self.db.save_user(name, email)

self.email.send(email, "Welcome!")

return user

def get_user(self, user_id):

return self.db.find_user(user_id)

# Usage:

user_service = UserService(db=PostgresDB(), email_service=SMTPService())

```

Detection Checklist:

• [ ] Class instantiation inside methods (Service())
• [ ] Deep attribute chaining (3+ dots)
• [ ] Hardcoded class references
• [ ] Circular imports or dependencies
• [ ] Module can't be tested without other modules

Fix Strategy:

1. Accept dependencies as constructor parameters
2. Use dependency injection
3. Create test doubles (mocks) easily
4. Swap implementations without changing code
5. Each module is independently testable

---

5. Missing `__all__` Exports (Python)

What It Is: Python modules that don't explicitly define their public interface via __all__.

Why It's Bad: Unclear what's public vs internal. Users import private implementation details. Violates the "stud" concept - unclear connection points.

Red Flags:

• No __all__ in __init__.py
• Modules expose internal functions/classes
• Users uncertain what to import
• Private names (_function) still accessible
• Documentation doesn't match exports

Example - SMELL:

```python

# BAD: No __all__ - unclear public interface

# module/__init__.py

from .core import process_data, _internal_helper

from .utils import validate_input, LOG_LEVEL

# What should users import? All of it? Only some?

```

Example - FIXED:

```python

# GOOD: Clear public interface via __all__

# module/__init__.py

from .core import process_data

from .utils import validate_input

__all__ = ['process_data', 'validate_input']

# Users know exactly what's public and what to use

```

Detection Checklist:

• [ ] Missing __all__ in __init__.py
• [ ] Internal functions (prefixed with _) exposed
• [ ] Unclear what's "public API"
• [ ] All imports at module level

Fix Strategy:

1. Add __all__ to every __init__.py
2. List ONLY the public functions/classes
3. Prefix internal implementation with _
4. Update documentation to match __all__
5. Clear = users know exactly what to use

---

Step 1: Scan Code Structure

1. Review file organization and module boundaries
2. Identify inheritance hierarchies
3. Scan for large functions (count lines)
4. Note __all__ presence/absence
5. Check for tight coupling patterns

Step 2: Analyze Each Smell

For each potential issue:

1. Confirm it violates philosophy
2. Measure severity (critical/major/minor)
3. Find specific line numbers
4. Note impact on system

Step 3: Generate Fixes

For each smell found:

1. Provide clear explanation of WHY it's bad
2. Show BEFORE code
3. Show AFTER code with detailed comments
4. Explain philosophy principle violated
5. Give concrete refactoring steps

Step 4: Create Report

1. List all smells found
2. Prioritize by severity/impact
3. Include specific examples
4. Provide actionable fixes
5. Reference philosophy docs

---

Rule 1: Abstract Base Classes

Check: class X(ABC) with exactly 1 concrete implementation

```python

# BAD pattern detection

• Count implementations of abstract class
• If count <= 2 and not used as interface: FLAG

```

Fix: Remove abstraction, use direct implementation

Rule 2: Inheritance Depth

Check: Class hierarchy depth

```python

# BAD pattern detection

• Follow inheritance chain: class -> parent -> grandparent...
• If depth > 2: FLAG

```

Fix: Use composition instead

Rule 3: Function Line Count

Check: All function bodies

```python

# BAD pattern detection

• Count lines in function (excluding docstring)
• If > 50 lines: FLAG
• If > 3 nesting levels: FLAG

```

Fix: Extract helper functions

Rule 4: Dependency Instantiation

Check: Class instantiation inside methods/functions

```python

# BAD pattern detection

• Search for "= ServiceName()" inside methods
• If found: FLAG

```

Fix: Pass as constructor argument

Rule 5: Missing **all**

Check: Python modules

```python

# BAD pattern detection

• Look for __all__ definition
• If missing: FLAG
• If __all__ incomplete: FLAG

```

Fix: Define explicit __all__

---

Smell: "Utility Class" Holder

```python

# BAD

class StringUtils:

@staticmethod

def clean(s):

return s.strip().lower()

```

Fix: Use direct function

```python

# GOOD

def clean_string(s):

return s.strip().lower()

```

---

Smell: "Manager" Class

```python

# BAD

class UserManager:

def create(self): pass

def update(self): pass

def delete(self): pass

def validate(self): pass

def email(self): pass

```

Fix: Split into focused services

```python

# GOOD

class UserService:

def __init__(self, db, email):

self.db = db

self.email = email

def create(self): pass

def update(self): pass

def delete(self): pass

def validate_user(user): pass

```

---

Smell: God Function

```python

# BAD - 200 line function doing everything

def process_order(order_data, validate, save, notify, etc...):

# 200 lines mixing validation, transformation, DB, email, logging

```

Fix: Compose small functions

```python

# GOOD

def process_order(order_data):

validate_order(order_data)

order = create_order(order_data)

save_order(order)

notify_customer(order)

log_creation(order)

```

---

Smell: Brittle Inheritance

```python

# BAD

class Base:

def work(self): pass

class Middle(Base):

def work(self):

return super().work()

class Derived(Middle):

def work(self):

return super().work() # Which work()?

```

Fix: Use clear, testable composition

```python

# GOOD

class Worker:

def __init__(self, validator, transformer):

self.validator = validator

self.transformer = transformer

def work(self, data):

self.validator.check(data)

return self.transformer.apply(data)

```

---

Smell: Hidden Dependencies

```python

# BAD

def fetch_data(user_id):

db = Database() # Where's this coming from?

return db.query(f"SELECT * FROM users WHERE id={user_id}")

```

Fix: Inject dependencies explicitly

```python

# GOOD

def fetch_data(user_id, db):

return db.query(f"SELECT * FROM users WHERE id={user_id}")

# Or in a class:

class UserRepository:

def __init__(self, db):

self.db = db

def fetch(self, user_id):

return self.db.query(f"SELECT * FROM users WHERE id={user_id}")

```

---

Example 1: Review New Module

```

User: Review this new authentication module for code smells.

Claude:

1. Scans all Python files in module
2. Checks for each smell type
3. Finds:

- Abstract base class with 1 implementation

- Large 120-line authenticate() function

- Missing __all__ in __init__.py

1. Provides specific fixes with before/after code
2. Explains philosophy violations

```

Example 2: Identify Tight Coupling

```

User: Find tight coupling in this user service.

Claude:

1. Traces all dependencies
2. Finds hardcoded Database() instantiation
3. Finds direct EmailService() creation
4. Shows dependency injection fix
5. Includes test example showing why it matters

```

Example 3: Simplify Inheritance

```

User: This class hierarchy is too complex.

Claude:

1. Maps inheritance tree (finds 4 levels)
2. Shows each level doing what
3. Suggests composition approach
4. Provides before/after refactoring
5. Explains how it aligns with brick philosophy

```

---

Philosophy Compliance

• [ ] No unnecessary abstractions
• [ ] Single responsibility per class/function
• [ ] Clear public interface (__all__)
• [ ] Dependencies injected, not hidden
• [ ] Inheritance depth <= 2 levels
• [ ] Functions < 50 lines
• [ ] No dead code or stubs

Code Quality

• [ ] Each function has one clear job
• [ ] Easy to understand at a glance
• [ ] Easy to test in isolation
• [ ] Easy to modify without breaking others
• [ ] Clear naming reflects responsibility

Modularity

• [ ] Modules are independently testable
• [ ] Clear connection points ("studs")
• [ ] Loose coupling between modules
• [ ] Explicit dependencies

---

A code review using this skill should:

• [ ] Identify all violations of philosophy
• [ ] Provide specific line numbers
• [ ] Show before/after examples
• [ ] Explain WHY each is a problem
• [ ] Suggest concrete fixes
• [ ] Include test strategies
• [ ] Reference philosophy docs
• [ ] Prioritize by severity
• [ ] Be constructive and educational
• [ ] Help writer improve future code

---

When to Use This Skill:

• During code review (before merge)
• In pull request comments
• Before creating new modules
• When refactoring legacy code
• To educate team members
• In design review meetings

Works Well With:

• Code review process
• Module spec generation
• Refactoring workflows
• Architecture discussions
• Mentoring and learning

---

• Philosophy: ~/.amplihack/.claude/context/PHILOSOPHY.md
• Patterns: ~/.amplihack/.claude/context/PATTERNS.md
• Brick Philosophy: See "Modular Architecture for AI" in PHILOSOPHY.md
• Zero-BS: See "Zero-BS Implementations" in PHILOSOPHY.md

---

This skill helps maintain code quality by:

1. Catching issues before they become technical debt
2. Educating developers on philosophy
3. Keeping code simple and maintainable
4. Preventing tightly-coupled systems
5. Making code easier to understand and modify

Use it constructively - the goal is learning and improvement, not criticism.