enes9103/my-claude-agents-skills/code-review-excellence

❌ Bad: "This is wrong."
✅ Good: "This could cause a race condition when multiple users
access simultaneously. Consider using a mutex here."
 
❌ Bad: "Why didn't you use X pattern?"
✅ Good: "Have you considered the Repository pattern? It would
make this easier to test. Here's an example: [link]"
 
❌ Bad: "Rename this variable."
✅ Good: "[nit] Consider `userCount` instead of `uc` for
clarity. Not blocking if you prefer to keep it."

Before diving into code, understand:
 
1.Read PR description and linked issue
2.Check PR size (>400 lines? Ask to split)
3.Review CI/CD status (tests passing?)
4.Understand the business requirement
5.Note any relevant architectural decisions

1.**Architecture & Design**
   -Does the solution fit the problem?
   -Are there simpler approaches?
   -Is it consistent with existing patterns?
   -Will it scale?
 
2.**File Organization**
   -Are new files in the right places?
   -Is code grouped logically?
   -Are there duplicate files?
 
3.**Testing Strategy**
   -Are there tests?
   -Do tests cover edge cases?
   -Are tests readable?

For each file:
 
1.**Logic & Correctness**
   -Edge cases handled?
   -Off-by-one errors?
   -Null/undefined checks?
   -Race conditions?
 
2.**Security**
   -Input validation?
   -SQL injection risks?
   -XSS vulnerabilities?
   -Sensitive data exposure?
 
3.**Performance**
   -N+1 queries?
   -Unnecessary loops?
   -Memory leaks?
   -Blocking operations?
 
4.**Maintainability**
   -Clear variable names?
   -Functions doing one thing?
   -Complex code commented?
   -Magic numbers extracted?

1.Summarize key concerns
2.Highlight what you liked
3.Make clear decision:
   -✅ Approve
   -💬 Comment (minor suggestions)
   -🔄 Request Changes (must address)
4.Offer to pair if complex

## 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

❌ "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?"

## 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?"

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."

# 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 = []

// 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]);
    return <div>{user.name}</div>;
}

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?"

// ❌ 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?

## 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?

Traditional: Praise + Criticism + Praise (feels fake)
 
Better: Context + Specific Issue + Helpful Solution
 
Example:
"I noticed the payment processing logic is inline in the
controller. This makes it harder to test and reuse.
 
[Specific Issue]
The calculateTotal() function mixes tax calculation,
discount logic, and database queries, making it difficult
to unit test and reason about.
 
[Helpful Solution]
Could we extract this into a PaymentService class? That
would make it testable and reusable. I can pair with you
on this if helpful."

When author disagrees with your feedback:
 
1.**Seek to Understand**
   "Help me understand your approach. What led you to
   choose this pattern?"
 
2.**Acknowledge Valid Points**
   "That's a good point about X. I hadn't considered that."
 
3.**Provide Data**
   "I'm concerned about performance. Can we add a benchmark
   to validate the approach?"
 
4.**Escalate if Needed**
   "Let's get [architect/senior dev] to weigh in on this."
 
5.**Know When to Let Go**
   If it's working and not a critical issue, approve it.
   Perfection is the enemy of progress.