Initial commit
This commit is contained in:
Zhongwei Li
@@ -0,0 +1,143 @@
|
||||
# Architecture Violations Reference
|
||||
|
||||
Clean Architecture principles: Separation of concerns, layer independence, dependency inversion.
|
||||
|
||||
## Layer Hierarchy
|
||||
|
||||
```
|
||||
┌─────────────────┐
|
||||
│ Controllers │ (HTTP handlers, routes)
|
||||
│ (routes/) │
|
||||
└────────┬────────┘
|
||||
│
|
||||
v
|
||||
┌─────────────────┐
|
||||
│ Services │ (Business logic)
|
||||
│ (services/) │
|
||||
└────────┬────────┘
|
||||
│
|
||||
v
|
||||
┌─────────────────┐
|
||||
│ Repositories │ (Data access)
|
||||
│ (repositories/) │
|
||||
└─────────────────┘
|
||||
```
|
||||
|
||||
**Rule:** Only call downward. Controller → Service → Repository.
|
||||
|
||||
## Violation Patterns
|
||||
|
||||
### 1. Layer Skipping
|
||||
|
||||
**Violation:**
|
||||
```python
|
||||
# controllers/user_controller.py
|
||||
from repositories.user_repository import UserRepository # VIOLATION!
|
||||
|
||||
def get_user(user_id):
|
||||
user_repo = UserRepository() # Skips Service layer
|
||||
return user_repo.get(user_id)
|
||||
```
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
# controllers/user_controller.py
|
||||
from services.user_service import UserService
|
||||
|
||||
def get_user(user_id):
|
||||
user_service = UserService()
|
||||
return user_service.get_user(user_id)
|
||||
|
||||
# services/user_service.py
|
||||
from repositories.user_repository import UserRepository
|
||||
|
||||
class UserService:
|
||||
def __init__(self):
|
||||
self.user_repo = UserRepository()
|
||||
|
||||
def get_user(self, user_id):
|
||||
return self.user_repo.get(user_id)
|
||||
```
|
||||
|
||||
**Severity:** HIGH
|
||||
|
||||
---
|
||||
|
||||
### 2. Circular Dependencies
|
||||
|
||||
**Violation:**
|
||||
```python
|
||||
# module_a.py
|
||||
from module_b import function_b
|
||||
|
||||
def function_a():
|
||||
return function_b()
|
||||
|
||||
# module_b.py
|
||||
from module_a import function_a # CIRCULAR!
|
||||
|
||||
def function_b():
|
||||
return function_a()
|
||||
```
|
||||
|
||||
**Detection:** Build dependency graph, check for cycles
|
||||
|
||||
**Fix:** Extract shared code to third module or use dependency injection
|
||||
|
||||
**Severity:** HIGH
|
||||
|
||||
---
|
||||
|
||||
### 3. Business Logic in Wrong Layer
|
||||
|
||||
**Violation:**
|
||||
```python
|
||||
# controllers/user_controller.py
|
||||
def create_user(data):
|
||||
# Business logic in controller! VIOLATION!
|
||||
if not data.get('email') or '@' not in data['email']:
|
||||
return {"error": "Invalid email"}
|
||||
|
||||
if len(data.get('password', '')) < 8:
|
||||
return {"error": "Password too short"}
|
||||
|
||||
user = User(**data)
|
||||
db.session.add(user)
|
||||
db.session.commit()
|
||||
return {"success": True}
|
||||
```
|
||||
|
||||
**Fix:** Move business logic to Service layer
|
||||
|
||||
---
|
||||
|
||||
## Detection
|
||||
|
||||
```python
|
||||
def detect_layer_violations(files):
|
||||
violations = []
|
||||
|
||||
for file in files:
|
||||
# Parse imports
|
||||
imports = extract_imports(file)
|
||||
|
||||
# Check layer
|
||||
if 'controllers' in file.path or 'routes' in file.path:
|
||||
# Controller layer
|
||||
for imp in imports:
|
||||
if 'repositories' in imp:
|
||||
violations.append({
|
||||
'type': 'Architecture',
|
||||
'severity': 'HIGH',
|
||||
'file': file.path,
|
||||
'description': 'Controller imports Repository (skips Service)',
|
||||
'suggestion': 'Import from services/ instead'
|
||||
})
|
||||
|
||||
return violations
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
**Version:** 1.0.0
|
||||
**Last Updated:** 2025-11-13
|
||||
231
skills/ln-341-code-quality-checker/references/dry_violations.md
Normal file
231
skills/ln-341-code-quality-checker/references/dry_violations.md
Normal file
@@ -0,0 +1,231 @@
|
||||
# DRY Violations Reference
|
||||
|
||||
DRY (Don't Repeat Yourself) principle: Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.
|
||||
|
||||
## Common Violation Patterns
|
||||
|
||||
### Pattern 1: Duplicate Function Definitions
|
||||
|
||||
**Violation:**
|
||||
```python
|
||||
# auth/service.py
|
||||
def validate_email(email):
|
||||
if not email or '@' not in email:
|
||||
raise ValueError("Invalid email")
|
||||
|
||||
# users/service.py
|
||||
def validate_email(email): # DUPLICATE!
|
||||
if not email or '@' not in email:
|
||||
raise ValueError("Invalid email")
|
||||
```
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
# utils/validators.py
|
||||
def validate_email(email):
|
||||
if not email or '@' not in email:
|
||||
raise ValueError("Invalid email")
|
||||
|
||||
# auth/service.py + users/service.py
|
||||
from utils.validators import validate_email
|
||||
```
|
||||
|
||||
**Severity:** HIGH if >2 duplicates, MEDIUM if 2 duplicates
|
||||
|
||||
---
|
||||
|
||||
### Pattern 2: Duplicate Validation Logic
|
||||
|
||||
**Violation:**
|
||||
```python
|
||||
# File 1
|
||||
if not user.email or '@' not in user.email:
|
||||
return {"error": "Invalid email"}
|
||||
|
||||
# File 2
|
||||
if not email or email.find('@') == -1: # Same logic, different syntax!
|
||||
raise ValueError("Invalid email")
|
||||
|
||||
# File 3
|
||||
email_valid = email and '@' in email # Same logic again!
|
||||
if not email_valid:
|
||||
return False
|
||||
```
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
# utils/validators.py
|
||||
def is_valid_email(email):
|
||||
return email and '@' in email
|
||||
|
||||
# All files use:
|
||||
if not is_valid_email(user.email):
|
||||
return {"error": "Invalid email"}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Pattern 3: Duplicate Error Handling
|
||||
|
||||
**Violation:**
|
||||
```python
|
||||
# File 1
|
||||
try:
|
||||
result = api_call()
|
||||
except requests.exceptions.RequestException as e:
|
||||
logger.error(f"API call failed: {e}")
|
||||
return {"error": "Service unavailable"}
|
||||
|
||||
# File 2
|
||||
try:
|
||||
data = external_api_call()
|
||||
except requests.exceptions.RequestException as err:
|
||||
logger.error(f"API call failed: {err}") # DUPLICATE!
|
||||
return {"error": "Service unavailable"}
|
||||
```
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
# utils/decorators.py
|
||||
def handle_api_errors(func):
|
||||
@wraps(func)
|
||||
def wrapper(*args, **kwargs):
|
||||
try:
|
||||
return func(*args, **kwargs)
|
||||
except requests.exceptions.RequestException as e:
|
||||
logger.error(f"API call failed: {e}")
|
||||
return {"error": "Service unavailable"}
|
||||
return wrapper
|
||||
|
||||
# Usage
|
||||
@handle_api_errors
|
||||
def api_call():
|
||||
return requests.get("https://api.example.com")
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Pattern 4: Duplicate Database Queries
|
||||
|
||||
**Violation:**
|
||||
```python
|
||||
# users/repository.py
|
||||
def get_active_users():
|
||||
return db.query(User).filter(User.active == True, User.deleted_at == None).all()
|
||||
|
||||
# admin/repository.py
|
||||
def get_active_users(): # DUPLICATE!
|
||||
return db.query(User).filter(User.active == True, User.deleted_at == None).all()
|
||||
```
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
# users/repository.py (single source of truth)
|
||||
class UserRepository:
|
||||
@staticmethod
|
||||
def get_active_users():
|
||||
return db.query(User).filter(User.active == True, User.deleted_at == None).all()
|
||||
|
||||
# admin/repository.py imports from users/
|
||||
from users.repository import UserRepository
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Detection Algorithm
|
||||
|
||||
```python
|
||||
def detect_dry_violations(files):
|
||||
violations = []
|
||||
|
||||
# 1. Extract all functions from all files
|
||||
functions = {}
|
||||
for file in files:
|
||||
ast_tree = ast.parse(file.content)
|
||||
for node in ast.walk(ast_tree):
|
||||
if isinstance(node, ast.FunctionDef):
|
||||
functions[node.name] = functions.get(node.name, [])
|
||||
functions[node.name].append({
|
||||
'file': file.path,
|
||||
'line': node.lineno,
|
||||
'body': ast.unparse(node)
|
||||
})
|
||||
|
||||
# 2. Find duplicate function names
|
||||
for func_name, occurrences in functions.items():
|
||||
if len(occurrences) > 1:
|
||||
# 3. Check if function bodies are similar (>80% similarity)
|
||||
for i, func1 in enumerate(occurrences):
|
||||
for func2 in occurrences[i+1:]:
|
||||
similarity = calculate_similarity(func1['body'], func2['body'])
|
||||
if similarity > 0.8:
|
||||
violations.append({
|
||||
'type': 'DRY',
|
||||
'severity': 'HIGH' if len(occurrences) > 2 else 'MEDIUM',
|
||||
'file': func2['file'],
|
||||
'line': func2['line'],
|
||||
'description': f"Duplicate function '{func_name}' in {len(occurrences)} places",
|
||||
'suggestion': f"Extract to shared module"
|
||||
})
|
||||
|
||||
return violations
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Similarity Calculation
|
||||
|
||||
**Token-based Jaccard similarity:**
|
||||
|
||||
```python
|
||||
def calculate_similarity(code1, code2):
|
||||
# Tokenize code (remove whitespace, comments)
|
||||
tokens1 = set(tokenize(code1))
|
||||
tokens2 = set(tokenize(code2))
|
||||
|
||||
# Jaccard index
|
||||
intersection = tokens1 & tokens2
|
||||
union = tokens1 | tokens2
|
||||
|
||||
if not union:
|
||||
return 0.0
|
||||
|
||||
return len(intersection) / len(union)
|
||||
|
||||
def tokenize(code):
|
||||
# Remove whitespace and comments
|
||||
code = re.sub(r'#.*$', '', code, flags=re.MULTILINE)
|
||||
code = re.sub(r'\s+', ' ', code)
|
||||
|
||||
# Split into tokens
|
||||
tokens = re.findall(r'\w+|[^\w\s]', code)
|
||||
|
||||
return tokens
|
||||
```
|
||||
|
||||
**Threshold:** 80% similarity = duplicate
|
||||
|
||||
---
|
||||
|
||||
## Exceptions (When DRY Not Applicable)
|
||||
|
||||
1. **Different domains:** Same logic but different business context
|
||||
2. **Temporary code:** Prototypes or one-off scripts
|
||||
3. **Test fixtures:** Test data duplication acceptable for clarity
|
||||
4. **Configuration:** Similar config blocks OK (e.g., multiple API endpoints)
|
||||
|
||||
**Example (acceptable duplication):**
|
||||
```python
|
||||
# auth/config.py
|
||||
AUTH_API_URL = "https://api.example.com/auth"
|
||||
AUTH_API_TIMEOUT = 5
|
||||
|
||||
# users/config.py
|
||||
USERS_API_URL = "https://api.example.com/users" # Similar but different domain
|
||||
USERS_API_TIMEOUT = 5
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
**Version:** 1.0.0
|
||||
**Last Updated:** 2025-11-13
|
||||
@@ -0,0 +1,75 @@
|
||||
# KISS Violations Reference
|
||||
|
||||
KISS (Keep It Simple, Stupid) principle: Most systems work best if they are kept simple rather than made complicated.
|
||||
|
||||
## Violation Types
|
||||
|
||||
### 1. High Cyclomatic Complexity
|
||||
|
||||
**Threshold:** >10 = MEDIUM severity, >15 = HIGH severity
|
||||
|
||||
**Violation Example:**
|
||||
```python
|
||||
def process_user(user, action, options=None):
|
||||
if user:
|
||||
if user.active:
|
||||
if action == 'update':
|
||||
if options and 'email' in options:
|
||||
if validate_email(options['email']):
|
||||
if user.role == 'admin' or user.id == options.get('user_id'):
|
||||
# Complexity = 7 decision points!
|
||||
return update_email(user, options['email'])
|
||||
return None
|
||||
```
|
||||
|
||||
**Fix with Early Returns:**
|
||||
```python
|
||||
def process_user(user, action, options=None):
|
||||
if not user or not user.active:
|
||||
return None
|
||||
|
||||
if action != 'update' or not options or 'email' not in options:
|
||||
return None
|
||||
|
||||
if not validate_email(options['email']):
|
||||
return None
|
||||
|
||||
if user.role != 'admin' and user.id != options.get('user_id'):
|
||||
return None
|
||||
|
||||
return update_email(user, options['email'])
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 2. Deep Nesting
|
||||
|
||||
**Threshold:** >4 levels = violation
|
||||
|
||||
**Fix:** Extract nested logic into helper functions
|
||||
|
||||
---
|
||||
|
||||
### 3. Long Functions
|
||||
|
||||
**Threshold:** >50 lines = violation
|
||||
|
||||
**Fix:** Extract logical blocks into separate functions with descriptive names
|
||||
|
||||
---
|
||||
|
||||
## Detection
|
||||
|
||||
```python
|
||||
def calculate_complexity(func_node):
|
||||
complexity = 1 # Base complexity
|
||||
for node in ast.walk(func_node):
|
||||
if isinstance(node, (ast.If, ast.For, ast.While, ast.And, ast.Or, ast.ExceptHandler)):
|
||||
complexity += 1
|
||||
return complexity
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
**Version:** 1.0.0
|
||||
**Last Updated:** 2025-11-13
|
||||
@@ -0,0 +1,72 @@
|
||||
# YAGNI Violations Reference
|
||||
|
||||
YAGNI (You Aren't Gonna Need It) principle: Don't add functionality until it's necessary.
|
||||
|
||||
## Violation Patterns
|
||||
|
||||
### 1. Unused Functions/Classes
|
||||
|
||||
**Violation:**
|
||||
```python
|
||||
def calculate_tax(amount): # Defined but never called!
|
||||
return amount * 0.2
|
||||
|
||||
def process_payment(amount):
|
||||
return charge_card(amount) # Doesn't use calculate_tax
|
||||
```
|
||||
|
||||
**Detection:** Search codebase for function calls; if function never called → YAGNI violation
|
||||
|
||||
---
|
||||
|
||||
### 2. Premature Abstraction
|
||||
|
||||
**Violation:**
|
||||
```python
|
||||
# Interface with only ONE implementation
|
||||
class UserRepositoryInterface:
|
||||
def get_user(self, id): pass
|
||||
def save_user(self, user): pass
|
||||
|
||||
class UserRepository(UserRepositoryInterface): # Only implementation!
|
||||
def get_user(self, id):
|
||||
return db.query(User).get(id)
|
||||
def save_user(self, user):
|
||||
db.session.add(user)
|
||||
db.session.commit()
|
||||
```
|
||||
|
||||
**Fix:** Remove interface until second implementation needed
|
||||
|
||||
---
|
||||
|
||||
### 3. Over-Engineering
|
||||
|
||||
**Violation:**
|
||||
```python
|
||||
# Complex pattern for simple problem
|
||||
class UserFactoryBuilder:
|
||||
def __init__(self):
|
||||
self.strategy = None
|
||||
|
||||
def set_strategy(self, strategy):
|
||||
self.strategy = strategy
|
||||
return self
|
||||
|
||||
def build(self):
|
||||
return User(strategy=self.strategy)
|
||||
|
||||
# Usage
|
||||
user = UserFactoryBuilder().set_strategy('default').build()
|
||||
```
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
# Simple solution
|
||||
user = User()
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
**Version:** 1.0.0
|
||||
**Last Updated:** 2025-11-13
|
||||
Reference in New Issue
Block a user