detect-code-smells

# Code Smells Detector

## Overview

This skill analyzes PHP codebases for code smells (symptoms of deeper problems) and generates detailed reports with severity levels and refactoring recommendations.

## Code Smells Catalog

| Smell | Description | Detection | Severity |
|-------|-------------|-----------|----------|
| God Class | Class doing too much | >500 LOC, >15 methods | CRITICAL |
| Feature Envy | Method uses another class more | Foreign calls > own calls | WARNING |
| Data Clumps | Same fields appear together | 3+ repeated params/fields | WARNING |
| Long Parameter List | Method with many params | >4 parameters | WARNING |
| Long Method | Method doing too much | >50 LOC | WARNING |
| Primitive Obsession | Primitives instead of objects | string $email, int $money | INFO |
| Message Chains | Long getter chains | ->get()->get()->get() | WARNING |
| Inappropriate Intimacy | Classes knowing too much | Direct field access | WARNING |

## Detection Patterns

### God Class Detection

```bash
# Large classes (>500 lines)
Grep: "^class " --glob "**/*.php"
# Then check file line counts

# Many public methods (>15)
Grep: "public function " --glob "**/*.php"
# Count per file

# Many dependencies (>8)
Grep: "__construct" --glob "**/*.php" -A 20
# Count constructor parameters

# Problematic names
Grep: "class.*Manager|class.*Handler|class.*Helper|class.*Util|class.*Processor" --glob "**/*.php"
```

**Indicators:**
- Class > 500 lines → CRITICAL
- Class > 15 public methods → CRITICAL
- Class > 8 constructor dependencies → WARNING
- Class name contains Manager, Handler, Helper, Util → INFO

### Feature Envy Detection

```bash
# Methods using other class data excessively
Grep: "\$this->[a-z]+->get[A-Z]" --glob "**/*.php"

# Multiple calls to same foreign object
Grep: "\$[a-z]+->.*\$[a-z]+->" --glob "**/*.php"

# Getters called more than own methods
Grep: "function [a-z]+\(" --glob "**/*.php" -A 30
# Analyze method bodies for foreign vs own calls
```

**Indicators:**
- Method calls other object's methods > own methods → WARNING
- Multiple chained calls to foreign object → INFO
- Method only transforms data from another class → WARNING

### Data Clumps Detection

```bash
# Repeated parameter groups in constructors
Grep: "__construct\(" --glob "**/*.php" -A 10
# Look for patterns: (string $x, string $y, string $z) appearing multiple times

# Repeated parameter groups in methods
Grep: "function [a-z]+\(" --glob "**/*.php"
# Look for same 3+ parameter combinations

# Multiple classes with same field groups
Grep: "(private|readonly) (string|int|float)" --glob "**/*.php"
# Detect repeated field patterns
```

**Common Data Clumps:**
- `$street, $city, $zipCode, $country` → Address Value Object
- `$startDate, $endDate` → DateRange Value Object
- `$amount, $currency` → Money Value Object
- `$firstName, $lastName, $email` → Contact/Person Value Object

### Long Parameter List Detection

```bash
# Methods with many parameters
Grep: "function [a-z]+\(" --glob "**/*.php"
# Count parameters (comma-separated)

# Constructors with many parameters
Grep: "__construct\(" --glob "**/*.php" -A 15
# Count parameters
```

**Thresholds:**
- 4+ parameters → INFO
- 6+ parameters → WARNING
- 8+ parameters → CRITICAL

### Long Method Detection

```bash
# Find method definitions and count lines until closing brace
Grep: "function [a-z]+\(" --glob "**/*.php" -A 60
# Analyze method length

# Nested control structures (indicator of complexity)
Grep: "if\s*\(.*\{.*if\s*\(" --glob "**/*.php" --multiline
```

**Thresholds:**
- 30+ lines → INFO
- 50+ lines → WARNING
- 100+ lines → CRITICAL

### Primitive Obsession Detection

```bash
# String parameters that should be Value Objects
Grep: "string \$email|string \$phone|string \$url|string \$currency|string \$country" --glob "**/*.php"

# Integer amounts
Grep: "int \$amount|int \$price|int \$total|int \$money|int \$cents" --glob "**/*.php"

# Float for money
Grep: "float \$amount|float \$price|float \$money" --glob "**/*.php"

# String status/type
Grep: "string \$status|string \$type|string \$state" --glob "**/*.php"

# Magic strings
Grep: "=== 'pending'|=== 'active'|=== 'completed'|=== 'draft'" --glob "**/*.php"
```

**Should be Value Objects:**
- Email addresses → Email
- Phone numbers → PhoneNumber
- URLs → Url or Uri
- Money amounts → Money (with currency)
- Dates/periods → DateRange, Period
- Identifiers → UserId, OrderId, etc.
- Status/Type → Enum

### Message Chains Detection

```bash
# Long getter chains
Grep: "->get[A-Z][a-z]+\(\)->get[A-Z][a-z]+\(\)" --glob "**/*.php"

# Triple or more chains
Grep: "->.*->.*->" --glob "**/*.php"

# Law of Demeter violations
Grep: "\$this->[a-z]+->get[A-Z].*->get[A-Z]" --glob "**/*.php"
```

**Indicators:**
- 2 chained getters → INFO
- 3+ chained getters → WARNING
- Chains in loops → CRITICAL

### Inappropriate Intimacy Detection

```bash
# Direct public property access
Grep: "\$[a-z]+->(?!get|set|is|has|can)[a-z]+" --glob "**/*.php"

# Friend classes accessing private state (via reflection)
Grep: "ReflectionClass|ReflectionProperty|setAccessible" --glob "**/*.php"

# Classes knowing internal structure
Grep: "->getInternalState|->getRawData|->getFields" --glob "**/*.php"
```

## Report Format

```markdown
# Code Smells Analysis Report

## Summary

| Smell | Critical | Warning | Info |
|-------|----------|---------|------|
| God Class | X | X | - |
| Feature Envy | - | X | X |
| Data Clumps | - | X | - |
| Long Parameter List | X | X | X |
| Long Method | - | X | X |
| Primitive Obsession | - | X | X |
| Message Chains | - | X | X |
| Inappropriate Intimacy | - | X | - |

**Total Issues:** X critical, X warnings, X info

## Critical Issues

### SMELL-001: God Class
- **File:** `src/Service/OrderManager.php`
- **Lines:** 847
- **Public Methods:** 23
- **Dependencies:** 12
- **Issue:** Class has too many responsibilities
- **Refactoring:**
  - Extract `OrderValidator` (validation logic)
  - Extract `OrderNotifier` (notification logic)
  - Extract `OrderPriceCalculator` (pricing logic)
- **Skills:** `create-use-case`, `create-domain-service`

### SMELL-002: Long Parameter List
- **File:** `src/Domain/Order/Order.php:45`
- **Method:** `createOrder()`
- **Parameters:** 9
- **Issue:** Too many parameters, hard to maintain
- **Refactoring:** Introduce Parameter Object
- **Skills:** `create-dto`, `create-builder`

## Warning Issues

### SMELL-003: Data Clump
- **Files:**
  - `src/Domain/User/User.php:15` — $street, $city, $zipCode
  - `src/Domain/Company/Company.php:23` — $street, $city, $zipCode
  - `src/Application/DTO/CreateOrderDTO.php:8` — $street, $city, $zipCode
- **Issue:** Address fields repeated across 3 classes
- **Refactoring:** Extract Address Value Object
- **Skills:** `create-value-object`

### SMELL-004: Feature Envy
- **File:** `src/Service/ReportGenerator.php:89`
- **Method:** `generateUserReport()`
- **Issue:** Method makes 15 calls to User object, only 2 to own class
- **Refactoring:** Move method to User or create UserReportBuilder
- **Skills:** `create-domain-service`

### SMELL-005: Primitive Obsession
- **File:** `src/Domain/User/User.php:12`
- **Field:** `private string $email`
- **Issue:** Email should be Value Object for validation
- **Refactoring:** Create Email Value Object
- **Skills:** `create-value-object`

### SMELL-006: Message Chain
- **File:** `src/Application/Handler/CreateOrderHandler.php:34`
- **Code:** `$user->getCompany()->getAddress()->getCountry()`
- **Issue:** Law of Demeter violation, tight coupling
- **Refactoring:** Add shortcut method or delegate

## Info Issues

### SMELL-007: Long Method
- **File:** `src/Infrastructure/Repository/OrderRepository.php:78`
- **Method:** `findByComplexCriteria()`
- **Lines:** 45
- **Issue:** Method approaching complexity threshold
- **Refactoring:** Extract query builder or specification

## Refactoring Priority

1. **Immediate:** God Classes blocking testing
2. **High:** Data Clumps causing duplication
3. **Medium:** Long Parameter Lists
4. **Low:** Message Chains, minor smells
```

## Remediation Skills

| Smell | Recommended Skill | Approach |
|-------|-------------------|----------|
| God Class | `create-use-case`, `create-domain-service` | Extract focused classes |
| Feature Envy | `create-domain-service` | Move method to data owner |
| Data Clumps | `create-value-object` | Extract Value Object |
| Long Parameter List | `create-dto`, `create-builder` | Introduce Parameter Object |
| Long Method | `create-use-case` | Extract methods |
| Primitive Obsession | `create-value-object` | Replace with Value Object |
| Message Chains | (refactoring) | Hide delegate, extract method |
| Inappropriate Intimacy | (refactoring) | Move method, extract class |

## Quick Analysis Commands

```bash
# Full smell detection
echo "=== God Classes ===" && \
find . -name "*.php" -path "*/src/*" -exec wc -l {} \; | awk '$1 > 400' && \
echo "=== Long Parameter Lists ===" && \
grep -rn "function [a-z]*(" --include="*.php" src/ | grep -E "(\$[a-z]+,\s*){5,}" && \
echo "=== Primitive Obsession ===" && \
grep -rn "string \$email\|string \$phone\|int \$amount\|float \$price" --include="*.php" src/ && \
echo "=== Message Chains ===" && \
grep -rn "->get[A-Z].*->get[A-Z].*->get[A-Z]" --include="*.php" src/ && \
echo "=== Magic Strings ===" && \
grep -rn "=== '[a-z]*'\|== '[a-z]*'" --include="*.php" src/
```

## Integration with Other Skills

This skill works alongside:
- `analyze-solid-violations` — SOLID violations overlap with some smells
- `structural-auditor` — architectural context for smells
- `ddd-auditor` — domain model quality assessment

## References

Based on Martin Fowler's "Refactoring" catalog:
- https://refactoring.guru/refactoring/smells
- "Refactoring: Improving the Design of Existing Code" (Fowler)

## When This Is Acceptable

- **DTOs** — Data classes are NOT a code smell; they serve a clear data transfer purpose
- **Configuration classes** — Classes with many constants/properties for configuration
- **Builder pattern** — Method chaining in builders creates apparent "Feature Envy" but is by design

### False Positive Indicators
- Class has `DTO`, `Request`, `Response`, `Config` in its name
- Class implements a Builder pattern with fluent API
- "Long parameter list" is actually a constructor with proper dependency injection