Initial commit
This commit is contained in:
532
skills/tdd-methodology-expert/references/code-smells.md
Normal file
532
skills/tdd-methodology-expert/references/code-smells.md
Normal file
@@ -0,0 +1,532 @@
|
||||
# Code Smells That Indicate Test-After Development
|
||||
|
||||
This document catalogs code smells and anti-patterns that strongly suggest tests were written after implementation rather than following TDD methodology.
|
||||
|
||||
## Understanding Code Smells in TDD Context
|
||||
|
||||
When developers write tests after code (test-after), they tend to produce different code structures than when following TDD. This is because:
|
||||
|
||||
1. **TDD enforces small steps**: Each test drives minimal implementation
|
||||
2. **TDD encourages refactoring**: The refactor phase continuously improves structure
|
||||
3. **TDD requires testability**: Code must be designed for easy testing from the start
|
||||
4. **TDD prevents over-engineering**: Only write code needed to pass tests
|
||||
|
||||
Test-after code often shows signs of:
|
||||
- Solving problems that don't exist yet (premature optimization)
|
||||
- Complex structures built all at once (big bang implementation)
|
||||
- Difficult-to-test designs (retrofitted testability)
|
||||
- Accumulated technical debt (skipped refactoring)
|
||||
|
||||
## High-Severity Code Smells
|
||||
|
||||
### 1. Deeply Nested Conditionals
|
||||
|
||||
**Description**: Multiple levels of if/elif/else statements nested within each other.
|
||||
|
||||
**Why it indicates test-after**:
|
||||
- TDD would break this down into separate, testable functions
|
||||
- Each branch would have its own test, encouraging extraction
|
||||
- Refactor phase would identify and eliminate deep nesting
|
||||
|
||||
**Example (Bad)**:
|
||||
```python
|
||||
def process_order(order):
|
||||
if order.customer:
|
||||
if order.customer.is_active:
|
||||
if order.items:
|
||||
if order.total > 0:
|
||||
if order.payment_method:
|
||||
if order.payment_method == "credit_card":
|
||||
if order.customer.credit_limit >= order.total:
|
||||
# Process credit card payment
|
||||
return "processed"
|
||||
else:
|
||||
return "insufficient_credit"
|
||||
else:
|
||||
# Process other payment
|
||||
return "processed"
|
||||
else:
|
||||
return "no_payment_method"
|
||||
else:
|
||||
return "invalid_total"
|
||||
else:
|
||||
return "no_items"
|
||||
else:
|
||||
return "inactive_customer"
|
||||
else:
|
||||
return "no_customer"
|
||||
```
|
||||
|
||||
**TDD Alternative**:
|
||||
```python
|
||||
def process_order(order):
|
||||
_validate_order(order)
|
||||
_validate_customer(order.customer)
|
||||
_validate_payment(order)
|
||||
return _execute_payment(order)
|
||||
|
||||
def _validate_order(order):
|
||||
if not order.items:
|
||||
raise OrderValidationError("Order must have items")
|
||||
if order.total <= 0:
|
||||
raise OrderValidationError("Order total must be positive")
|
||||
|
||||
def _validate_customer(customer):
|
||||
if not customer:
|
||||
raise OrderValidationError("Order must have a customer")
|
||||
if not customer.is_active:
|
||||
raise OrderValidationError("Customer is inactive")
|
||||
|
||||
def _validate_payment(order):
|
||||
if not order.payment_method:
|
||||
raise OrderValidationError("Payment method required")
|
||||
|
||||
def _execute_payment(order):
|
||||
payment_processor = PaymentProcessorFactory.create(order.payment_method)
|
||||
return payment_processor.process(order)
|
||||
```
|
||||
|
||||
**How to detect**: Look for 3+ levels of nested if/else statements.
|
||||
|
||||
### 2. Long Methods/Functions
|
||||
|
||||
**Description**: Methods exceeding 20-30 lines of code.
|
||||
|
||||
**Why it indicates test-after**:
|
||||
- TDD naturally produces small functions (5-15 lines)
|
||||
- Each test typically drives one small piece of functionality
|
||||
- Long methods suggest big-bang implementation
|
||||
|
||||
**Example (Bad)**:
|
||||
```python
|
||||
def generate_invoice(order_id):
|
||||
# 80+ lines of mixed responsibilities:
|
||||
# - Database queries
|
||||
# - Business logic
|
||||
# - Calculations
|
||||
# - Formatting
|
||||
# - File generation
|
||||
# - Email sending
|
||||
order = db.query(Order).filter_by(id=order_id).first()
|
||||
if not order:
|
||||
return None
|
||||
|
||||
total = 0
|
||||
for item in order.items:
|
||||
if item.discount:
|
||||
price = item.price * (1 - item.discount)
|
||||
else:
|
||||
price = item.price
|
||||
total += price * item.quantity
|
||||
|
||||
tax = total * 0.1
|
||||
shipping = 10 if total < 50 else 0
|
||||
grand_total = total + tax + shipping
|
||||
|
||||
# ... 50 more lines of formatting and sending
|
||||
```
|
||||
|
||||
**TDD Alternative**:
|
||||
```python
|
||||
def generate_invoice(order_id):
|
||||
order = _fetch_order(order_id)
|
||||
invoice_data = _calculate_invoice_totals(order)
|
||||
formatted_invoice = _format_invoice(order, invoice_data)
|
||||
_send_invoice(order.customer.email, formatted_invoice)
|
||||
return formatted_invoice
|
||||
|
||||
def _fetch_order(order_id):
|
||||
order = db.query(Order).filter_by(id=order_id).first()
|
||||
if not order:
|
||||
raise OrderNotFoundError(f"Order {order_id} not found")
|
||||
return order
|
||||
|
||||
def _calculate_invoice_totals(order):
|
||||
subtotal = sum(_calculate_line_total(item) for item in order.items)
|
||||
tax = _calculate_tax(subtotal)
|
||||
shipping = _calculate_shipping(subtotal)
|
||||
return InvoiceTotals(subtotal, tax, shipping)
|
||||
|
||||
def _calculate_line_total(item):
|
||||
price = item.price * (1 - item.discount) if item.discount else item.price
|
||||
return price * item.quantity
|
||||
```
|
||||
|
||||
**How to detect**: Count lines in methods. Flag anything over 20 lines.
|
||||
|
||||
### 3. Complex Boolean Conditions
|
||||
|
||||
**Description**: Conditional expressions with multiple AND/OR operators.
|
||||
|
||||
**Why it indicates test-after**:
|
||||
- TDD encourages extracting complex conditions into named methods
|
||||
- Each condition part would have its own test
|
||||
- Refactor phase would identify complexity and extract it
|
||||
|
||||
**Example (Bad)**:
|
||||
```python
|
||||
if (user.age >= 18 and user.has_license and
|
||||
user.years_experience >= 2 and
|
||||
(user.state == "CA" or user.state == "NY") and
|
||||
not user.has_violations and user.insurance_valid):
|
||||
# Allow to rent car
|
||||
pass
|
||||
```
|
||||
|
||||
**TDD Alternative**:
|
||||
```python
|
||||
def can_rent_car(user):
|
||||
return (is_eligible_driver(user) and
|
||||
is_in_service_area(user) and
|
||||
has_clean_record(user))
|
||||
|
||||
def is_eligible_driver(user):
|
||||
return user.age >= 18 and user.has_license and user.years_experience >= 2
|
||||
|
||||
def is_in_service_area(user):
|
||||
return user.state in ["CA", "NY"]
|
||||
|
||||
def has_clean_record(user):
|
||||
return not user.has_violations and user.insurance_valid
|
||||
```
|
||||
|
||||
**How to detect**: Count AND/OR operators. Flag conditions with 3+ logical operators.
|
||||
|
||||
### 4. God Objects/Classes
|
||||
|
||||
**Description**: Classes with too many responsibilities and methods.
|
||||
|
||||
**Why it indicates test-after**:
|
||||
- TDD enforces Single Responsibility Principle through testing
|
||||
- Each test focuses on one behavior, encouraging focused classes
|
||||
- Testing god objects is painful, encouraging decomposition
|
||||
|
||||
**Example (Bad)**:
|
||||
```python
|
||||
class UserManager:
|
||||
def authenticate(self, username, password): pass
|
||||
def create_user(self, user_data): pass
|
||||
def update_user(self, user_id, data): pass
|
||||
def delete_user(self, user_id): pass
|
||||
def send_welcome_email(self, user): pass
|
||||
def send_password_reset(self, user): pass
|
||||
def validate_email(self, email): pass
|
||||
def validate_password(self, password): pass
|
||||
def log_user_activity(self, user, action): pass
|
||||
def generate_report(self, user_id): pass
|
||||
def export_user_data(self, user_id): pass
|
||||
def import_users(self, file_path): pass
|
||||
# ... 20 more methods
|
||||
```
|
||||
|
||||
**TDD Alternative**:
|
||||
```python
|
||||
class AuthenticationService:
|
||||
def authenticate(self, username, password): pass
|
||||
|
||||
class UserRepository:
|
||||
def create(self, user): pass
|
||||
def update(self, user_id, data): pass
|
||||
def delete(self, user_id): pass
|
||||
def find_by_id(self, user_id): pass
|
||||
|
||||
class EmailService:
|
||||
def send_welcome_email(self, user): pass
|
||||
def send_password_reset(self, user): pass
|
||||
|
||||
class UserValidator:
|
||||
def validate_email(self, email): pass
|
||||
def validate_password(self, password): pass
|
||||
|
||||
class UserReportGenerator:
|
||||
def generate_report(self, user_id): pass
|
||||
```
|
||||
|
||||
**How to detect**: Count methods in class. Flag classes with 10+ methods.
|
||||
|
||||
## Medium-Severity Code Smells
|
||||
|
||||
### 5. Type Checking Instead of Polymorphism
|
||||
|
||||
**Description**: Using `isinstance()`, `typeof`, or type switches instead of polymorphic design.
|
||||
|
||||
**Why it indicates test-after**:
|
||||
- TDD encourages interface-based design through mocking
|
||||
- Polymorphism emerges naturally when testing behaviors
|
||||
- Type checking makes testing harder, encouraging better design
|
||||
|
||||
**Example (Bad)**:
|
||||
```python
|
||||
def calculate_area(shape):
|
||||
if isinstance(shape, Circle):
|
||||
return 3.14159 * shape.radius ** 2
|
||||
elif isinstance(shape, Rectangle):
|
||||
return shape.width * shape.height
|
||||
elif isinstance(shape, Triangle):
|
||||
return 0.5 * shape.base * shape.height
|
||||
else:
|
||||
raise ValueError("Unknown shape type")
|
||||
```
|
||||
|
||||
**TDD Alternative**:
|
||||
```python
|
||||
class Shape(ABC):
|
||||
@abstractmethod
|
||||
def calculate_area(self):
|
||||
pass
|
||||
|
||||
class Circle(Shape):
|
||||
def __init__(self, radius):
|
||||
self.radius = radius
|
||||
|
||||
def calculate_area(self):
|
||||
return 3.14159 * self.radius ** 2
|
||||
|
||||
class Rectangle(Shape):
|
||||
def __init__(self, width, height):
|
||||
self.width = width
|
||||
self.height = height
|
||||
|
||||
def calculate_area(self):
|
||||
return self.width * self.height
|
||||
|
||||
# Usage:
|
||||
def process_shape(shape: Shape):
|
||||
return shape.calculate_area()
|
||||
```
|
||||
|
||||
**How to detect**: Search for `isinstance()`, `typeof`, or type switch patterns.
|
||||
|
||||
### 6. Duplicate Code Blocks
|
||||
|
||||
**Description**: Same or similar code repeated in multiple places.
|
||||
|
||||
**Why it indicates test-after**:
|
||||
- TDD's refactor phase explicitly targets duplication
|
||||
- Each cycle includes time to eliminate redundancy
|
||||
- Test-after often skips refactoring altogether
|
||||
|
||||
**Example (Bad)**:
|
||||
```python
|
||||
def calculate_discount_price_for_books(price, quantity):
|
||||
if quantity >= 10:
|
||||
discount = 0.2
|
||||
elif quantity >= 5:
|
||||
discount = 0.1
|
||||
else:
|
||||
discount = 0
|
||||
return price * (1 - discount)
|
||||
|
||||
def calculate_discount_price_for_electronics(price, quantity):
|
||||
if quantity >= 10:
|
||||
discount = 0.15
|
||||
elif quantity >= 5:
|
||||
discount = 0.08
|
||||
else:
|
||||
discount = 0
|
||||
return price * (1 - discount)
|
||||
```
|
||||
|
||||
**TDD Alternative**:
|
||||
```python
|
||||
def calculate_discount_price(price, quantity, discount_tiers):
|
||||
discount = _get_discount_for_quantity(quantity, discount_tiers)
|
||||
return price * (1 - discount)
|
||||
|
||||
def _get_discount_for_quantity(quantity, tiers):
|
||||
for min_qty, discount in sorted(tiers.items(), reverse=True):
|
||||
if quantity >= min_qty:
|
||||
return discount
|
||||
return 0
|
||||
|
||||
# Usage:
|
||||
BOOK_DISCOUNTS = {10: 0.2, 5: 0.1}
|
||||
ELECTRONICS_DISCOUNTS = {10: 0.15, 5: 0.08}
|
||||
|
||||
book_price = calculate_discount_price(29.99, 12, BOOK_DISCOUNTS)
|
||||
```
|
||||
|
||||
**How to detect**: Use code duplication analysis tools (>6 lines duplicated).
|
||||
|
||||
### 7. Primitive Obsession
|
||||
|
||||
**Description**: Using primitive types instead of small objects to represent concepts.
|
||||
|
||||
**Why it indicates test-after**:
|
||||
- TDD encourages creating types that make tests clearer
|
||||
- Value objects emerge naturally when expressing test intent
|
||||
- Primitives make tests verbose and unclear
|
||||
|
||||
**Example (Bad)**:
|
||||
```python
|
||||
def create_appointment(patient_id, doctor_id, date_str, time_str, duration_mins):
|
||||
# Working with primitives throughout
|
||||
date = datetime.strptime(date_str, "%Y-%m-%d")
|
||||
time = datetime.strptime(time_str, "%H:%M")
|
||||
# ... complex validation and manipulation
|
||||
```
|
||||
|
||||
**TDD Alternative**:
|
||||
```python
|
||||
@dataclass
|
||||
class AppointmentTime:
|
||||
date: datetime.date
|
||||
time: datetime.time
|
||||
duration: timedelta
|
||||
|
||||
def __post_init__(self):
|
||||
if self.duration <= timedelta(0):
|
||||
raise ValueError("Duration must be positive")
|
||||
|
||||
def end_time(self):
|
||||
start = datetime.combine(self.date, self.time)
|
||||
return start + self.duration
|
||||
|
||||
def create_appointment(patient_id, doctor_id, appointment_time: AppointmentTime):
|
||||
# Working with rich domain objects
|
||||
pass
|
||||
```
|
||||
|
||||
**How to detect**: Look for functions with many primitive parameters (4+).
|
||||
|
||||
### 8. Comments Explaining What Code Does
|
||||
|
||||
**Description**: Comments that explain the mechanics of the code rather than the "why".
|
||||
|
||||
**Why it indicates test-after**:
|
||||
- TDD produces self-documenting code through clear naming
|
||||
- Tests serve as documentation for behavior
|
||||
- Need for "what" comments suggests unclear code
|
||||
|
||||
**Example (Bad)**:
|
||||
```python
|
||||
def process(data):
|
||||
# Loop through each item in data
|
||||
for item in data:
|
||||
# Check if item value is greater than 100
|
||||
if item.value > 100:
|
||||
# Multiply value by 1.5
|
||||
item.value = item.value * 1.5
|
||||
# Check if item is active
|
||||
if item.is_active:
|
||||
# Add item to results list
|
||||
results.append(item)
|
||||
```
|
||||
|
||||
**TDD Alternative**:
|
||||
```python
|
||||
def process_high_value_active_items(items):
|
||||
return [apply_premium_pricing(item)
|
||||
for item in items
|
||||
if is_premium_eligible(item)]
|
||||
|
||||
def is_premium_eligible(item):
|
||||
return item.value > 100 and item.is_active
|
||||
|
||||
def apply_premium_pricing(item):
|
||||
item.value *= PREMIUM_MULTIPLIER
|
||||
return item
|
||||
```
|
||||
|
||||
**How to detect**: Look for comments explaining mechanics; good comments explain "why".
|
||||
|
||||
## Low-Severity Code Smells
|
||||
|
||||
### 9. Magic Numbers
|
||||
|
||||
**Description**: Unexplained numeric literals scattered throughout code.
|
||||
|
||||
**Example (Bad)**:
|
||||
```python
|
||||
def calculate_shipping(weight):
|
||||
if weight < 5:
|
||||
return 10
|
||||
elif weight < 20:
|
||||
return 25
|
||||
else:
|
||||
return 50
|
||||
```
|
||||
|
||||
**TDD Alternative**:
|
||||
```python
|
||||
LIGHT_PACKAGE_THRESHOLD = 5
|
||||
MEDIUM_PACKAGE_THRESHOLD = 20
|
||||
LIGHT_PACKAGE_RATE = 10
|
||||
MEDIUM_PACKAGE_RATE = 25
|
||||
HEAVY_PACKAGE_RATE = 50
|
||||
|
||||
def calculate_shipping(weight):
|
||||
if weight < LIGHT_PACKAGE_THRESHOLD:
|
||||
return LIGHT_PACKAGE_RATE
|
||||
elif weight < MEDIUM_PACKAGE_THRESHOLD:
|
||||
return MEDIUM_PACKAGE_RATE
|
||||
else:
|
||||
return HEAVY_PACKAGE_RATE
|
||||
```
|
||||
|
||||
### 10. Long Parameter Lists
|
||||
|
||||
**Description**: Methods accepting many parameters (4+).
|
||||
|
||||
**Example (Bad)**:
|
||||
```python
|
||||
def create_user(first_name, last_name, email, phone, address, city, state, zip, country):
|
||||
pass
|
||||
```
|
||||
|
||||
**TDD Alternative**:
|
||||
```python
|
||||
@dataclass
|
||||
class UserProfile:
|
||||
first_name: str
|
||||
last_name: str
|
||||
email: str
|
||||
phone: str
|
||||
|
||||
@dataclass
|
||||
class Address:
|
||||
street: str
|
||||
city: str
|
||||
state: str
|
||||
zip: str
|
||||
country: str
|
||||
|
||||
def create_user(profile: UserProfile, address: Address):
|
||||
pass
|
||||
```
|
||||
|
||||
## Detection Strategy
|
||||
|
||||
### Automated Checks
|
||||
|
||||
Run these checks regularly to identify test-after patterns:
|
||||
|
||||
1. **Cyclomatic Complexity**: Flag methods with complexity > 10
|
||||
2. **Method Length**: Flag methods > 20 lines
|
||||
3. **Class Size**: Flag classes with > 10 methods
|
||||
4. **Nesting Depth**: Flag code with > 3 levels of nesting
|
||||
5. **Duplication**: Flag blocks of > 6 duplicated lines
|
||||
6. **Parameter Count**: Flag methods with > 4 parameters
|
||||
|
||||
### Manual Review
|
||||
|
||||
Look for these patterns during code review:
|
||||
|
||||
1. Large commits with code and tests together
|
||||
2. Tests that test implementation rather than behavior
|
||||
3. Absence of refactoring commits
|
||||
4. Complex code without corresponding complex tests
|
||||
5. Tests that mock internal methods
|
||||
|
||||
## Refactoring from Test-After to TDD
|
||||
|
||||
If you inherit test-after code:
|
||||
|
||||
1. **Add characterization tests**: Cover existing behavior
|
||||
2. **Identify smells**: Use automated and manual detection
|
||||
3. **Extract methods**: Break down large methods
|
||||
4. **Introduce types**: Replace primitives with value objects
|
||||
5. **Apply patterns**: Use polymorphism, strategy, etc.
|
||||
6. **Write tests first for new features**: Start TDD from now
|
||||
|
||||
Remember: The goal isn't perfect code, but continuously improving code quality through TDD discipline.
|
||||
Reference in New Issue
Block a user