chore: init gsd
This commit is contained in:
@@ -0,0 +1,179 @@
|
||||
## Language-Specific Patterns
|
||||
|
||||
### Python Code Review
|
||||
|
||||
```python
|
||||
# Check for Python-specific issues
|
||||
|
||||
# ❌ Mutable default arguments
|
||||
def add_item(item, items=[]): # Bug! Shared across calls
|
||||
items.append(item)
|
||||
return items
|
||||
|
||||
# ✅ Use None as default
|
||||
def add_item(item, items=None):
|
||||
if items is None:
|
||||
items = []
|
||||
items.append(item)
|
||||
return items
|
||||
|
||||
# ❌ Catching too broad
|
||||
try:
|
||||
result = risky_operation()
|
||||
except: # Catches everything, even KeyboardInterrupt!
|
||||
pass
|
||||
|
||||
# ✅ Catch specific exceptions
|
||||
try:
|
||||
result = risky_operation()
|
||||
except ValueError as e:
|
||||
logger.error(f"Invalid value: {e}")
|
||||
raise
|
||||
|
||||
# ❌ Using mutable class attributes
|
||||
class User:
|
||||
permissions = [] # Shared across all instances!
|
||||
|
||||
# ✅ Initialize in __init__
|
||||
class User:
|
||||
def __init__(self):
|
||||
self.permissions = []
|
||||
```
|
||||
|
||||
### TypeScript/JavaScript Code Review
|
||||
|
||||
```typescript
|
||||
// Check for TypeScript-specific issues
|
||||
|
||||
// ❌ Using any defeats type safety
|
||||
function processData(data: any) { // Avoid any
|
||||
return data.value;
|
||||
}
|
||||
|
||||
// ✅ Use proper types
|
||||
interface DataPayload {
|
||||
value: string;
|
||||
}
|
||||
function processData(data: DataPayload) {
|
||||
return data.value;
|
||||
}
|
||||
|
||||
// ❌ Not handling async errors
|
||||
async function fetchUser(id: string) {
|
||||
const response = await fetch(`/api/users/${id}`);
|
||||
return response.json(); // What if network fails?
|
||||
}
|
||||
|
||||
// ✅ Handle errors properly
|
||||
async function fetchUser(id: string): Promise<User> {
|
||||
try {
|
||||
const response = await fetch(`/api/users/${id}`);
|
||||
if (!response.ok) {
|
||||
throw new Error(`HTTP ${response.status}`);
|
||||
}
|
||||
return await response.json();
|
||||
} catch (error) {
|
||||
console.error('Failed to fetch user:', error);
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
// ❌ Mutation of props
|
||||
function UserProfile({ user }: Props) {
|
||||
user.lastViewed = new Date(); // Mutating prop!
|
||||
return <div>{user.name}</div>;
|
||||
}
|
||||
|
||||
// ✅ Don't mutate props
|
||||
function UserProfile({ user, onView }: Props) {
|
||||
useEffect(() => {
|
||||
onView(user.id); // Notify parent to update
|
||||
}, [user.id, onView]);
|
||||
return <div>{user.name}</div>;
|
||||
}
|
||||
```
|
||||
|
||||
## Advanced Review Patterns
|
||||
|
||||
### Pattern 1: Architectural Review
|
||||
|
||||
```markdown
|
||||
When reviewing significant changes:
|
||||
|
||||
1. **Design Document First**
|
||||
- For large features, request design doc before code
|
||||
- Review design with team before implementation
|
||||
- Agree on approach to avoid rework
|
||||
|
||||
2. **Review in Stages**
|
||||
- First PR: Core abstractions and interfaces
|
||||
- Second PR: Implementation
|
||||
- Third PR: Integration and tests
|
||||
- Easier to review, faster to iterate
|
||||
|
||||
3. **Consider Alternatives**
|
||||
- "Have we considered using [pattern/library]?"
|
||||
- "What's the tradeoff vs. the simpler approach?"
|
||||
- "How will this evolve as requirements change?"
|
||||
```
|
||||
|
||||
### Pattern 2: Test Quality Review
|
||||
|
||||
```typescript
|
||||
// ❌ Poor test: Implementation detail testing
|
||||
test('increments counter variable', () => {
|
||||
const component = render(<Counter />);
|
||||
const button = component.getByRole('button');
|
||||
fireEvent.click(button);
|
||||
expect(component.state.counter).toBe(1); // Testing internal state
|
||||
});
|
||||
|
||||
// ✅ Good test: Behavior testing
|
||||
test('displays incremented count when clicked', () => {
|
||||
render(<Counter />);
|
||||
const button = screen.getByRole('button', { name: /increment/i });
|
||||
fireEvent.click(button);
|
||||
expect(screen.getByText('Count: 1')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
// Review questions for tests:
|
||||
// - Do tests describe behavior, not implementation?
|
||||
// - Are test names clear and descriptive?
|
||||
// - Do tests cover edge cases?
|
||||
// - Are tests independent (no shared state)?
|
||||
// - Can tests run in any order?
|
||||
```
|
||||
|
||||
### Pattern 3: Security Review
|
||||
|
||||
```markdown
|
||||
## Security Review Checklist
|
||||
|
||||
### Authentication & Authorization
|
||||
|
||||
- [ ] Is authentication required where needed?
|
||||
- [ ] Are authorization checks before every action?
|
||||
- [ ] Is JWT validation proper (signature, expiry)?
|
||||
- [ ] Are API keys/secrets properly secured?
|
||||
|
||||
### Input Validation
|
||||
|
||||
- [ ] All user inputs validated?
|
||||
- [ ] File uploads restricted (size, type)?
|
||||
- [ ] SQL queries parameterized?
|
||||
- [ ] XSS protection (escape output)?
|
||||
|
||||
### Data Protection
|
||||
|
||||
- [ ] Passwords hashed (bcrypt/argon2)?
|
||||
- [ ] Sensitive data encrypted at rest?
|
||||
- [ ] HTTPS enforced for sensitive data?
|
||||
- [ ] PII handled according to regulations?
|
||||
|
||||
### Common Vulnerabilities
|
||||
|
||||
- [ ] No eval() or similar dynamic execution?
|
||||
- [ ] No hardcoded secrets?
|
||||
- [ ] CSRF protection for state-changing operations?
|
||||
- [ ] Rate limiting on public endpoints?
|
||||
```
|
||||
@@ -0,0 +1,86 @@
|
||||
## Review Techniques
|
||||
|
||||
### Technique 1: The Checklist Method
|
||||
|
||||
```markdown
|
||||
## Security Checklist
|
||||
|
||||
- [ ] User input validated and sanitized
|
||||
- [ ] SQL queries use parameterization
|
||||
- [ ] Authentication/authorization checked
|
||||
- [ ] Secrets not hardcoded
|
||||
- [ ] Error messages don't leak info
|
||||
|
||||
## Performance Checklist
|
||||
|
||||
- [ ] No N+1 queries
|
||||
- [ ] Database queries indexed
|
||||
- [ ] Large lists paginated
|
||||
- [ ] Expensive operations cached
|
||||
- [ ] No blocking I/O in hot paths
|
||||
|
||||
## Testing Checklist
|
||||
|
||||
- [ ] Happy path tested
|
||||
- [ ] Edge cases covered
|
||||
- [ ] Error cases tested
|
||||
- [ ] Test names are descriptive
|
||||
- [ ] Tests are deterministic
|
||||
```
|
||||
|
||||
### Technique 2: The Question Approach
|
||||
|
||||
Instead of stating problems, ask questions to encourage thinking:
|
||||
|
||||
```markdown
|
||||
❌ "This will fail if the list is empty."
|
||||
✅ "What happens if `items` is an empty array?"
|
||||
|
||||
❌ "You need error handling here."
|
||||
✅ "How should this behave if the API call fails?"
|
||||
|
||||
❌ "This is inefficient."
|
||||
✅ "I see this loops through all users. Have we considered
|
||||
the performance impact with 100k users?"
|
||||
```
|
||||
|
||||
### Technique 3: Suggest, Don't Command
|
||||
|
||||
````markdown
|
||||
## Use Collaborative Language
|
||||
|
||||
❌ "You must change this to use async/await"
|
||||
✅ "Suggestion: async/await might make this more readable:
|
||||
```typescript
|
||||
async function fetchUser(id: string) {
|
||||
const user = await db.query('SELECT * FROM users WHERE id = ?', id);
|
||||
return user;
|
||||
}
|
||||
```
|
||||
What do you think?"
|
||||
|
||||
❌ "Extract this into a function"
|
||||
✅ "This logic appears in 3 places. Would it make sense to
|
||||
extract it into a shared utility function?"
|
||||
````
|
||||
|
||||
### Technique 4: Differentiate Severity
|
||||
|
||||
```markdown
|
||||
Use labels to indicate priority:
|
||||
|
||||
🔴 [blocking] - Must fix before merge
|
||||
🟡 [important] - Should fix, discuss if disagree
|
||||
🟢 [nit] - Nice to have, not blocking
|
||||
💡 [suggestion] - Alternative approach to consider
|
||||
📚 [learning] - Educational comment, no action needed
|
||||
🎉 [praise] - Good work, keep it up!
|
||||
|
||||
Example:
|
||||
"🔴 [blocking] This SQL query is vulnerable to injection.
|
||||
Please use parameterized queries."
|
||||
|
||||
"🟢 [nit] Consider renaming `data` to `userData` for clarity."
|
||||
|
||||
"🎉 [praise] Excellent test coverage! This will catch edge cases."
|
||||
```
|
||||
Reference in New Issue
Block a user