Code Review Assistant

Code Review Assistant

Overview

Perform thorough, constructive code reviews that identify issues, suggest improvements, and ensure code quality, security, and maintainability.

Review Categories

Examine code across these dimensions:

1. 🐛 Correctness & Bugs

• Logic errors
• Edge case handling
• Null/undefined checks
• Type mismatches
• Off-by-one errors
• Race conditions

2. 🔒 Security

• Input validation
• SQL injection risks
• XSS vulnerabilities
• Authentication/authorization flaws
• Sensitive data exposure
• Insecure dependencies

3. ⚡ Performance

• Algorithm efficiency (O(n) complexity)
• Memory leaks
• Unnecessary computations
• Database query optimization
• Caching opportunities

4. 🏗️ Code Quality

• Readability and clarity
• Naming conventions
• Code duplication (DRY principle)
• Function/method length
• Complexity (cyclomatic)
• SOLID principles

5. ✅ Testing

• Test coverage
• Edge case testing
• Unit vs integration tests
• Test quality and clarity
• Mock usage appropriateness

6. 📚 Documentation

• Code comments quality
• API documentation
• Function/method docstrings
• Complex logic explanation
• README updates

Review Workflow

Step 1: Understand Context

Gather information:

• What's the purpose of this code?
• What problem does it solve?
• What's the scope of changes?
• Are there related files to consider?

For PRs:

# View PR diff
gh pr diff <PR-NUMBER>

# View PR description
gh pr view <PR-NUMBER>

# See changed files
git diff --name-only main..HEAD

Step 2: Read the Code

First pass - high level:

• Overall structure and organization
• Naming consistency
• Code patterns used
• Separation of concerns

Second pass - detailed:

• Line-by-line logic verification
• Edge cases and error handling
• Performance considerations
• Security implications

Step 3: Identify Issues

Categorize findings by severity:

🔴 Critical: Must fix before merge

• Security vulnerabilities
• Data loss risks
• System crashes
• Breaking changes

🟡 Important: Should fix before merge

• Logic bugs
• Performance issues
• Poor error handling
• Missing tests for critical paths

🔵 Minor: Nice to have

• Code style inconsistencies
• Missing comments
• Minor optimizations
• Naming improvements

💡 Suggestion: Optional improvements

• Refactoring opportunities
• Alternative approaches
• Future considerations

Step 4: Provide Feedback

Structure feedback constructively:

For each issue:

1. Location: File and line number
2. Issue: What's wrong
3. Impact: Why it matters
4. Recommendation: How to fix
5. Example: Code suggestion if helpful

Tone guidelines:

• Be specific and objective
• Focus on code, not the person
• Explain the "why" behind suggestions
• Acknowledge good practices
• Ask questions for clarification when unsure

Review Template

# Code Review: [PR Title / Code Description]

## Summary
- **Files Reviewed:** X files, Y lines changed
- **Overall Assessment:** [Approve/Request Changes/Comment]
- **Critical Issues:** N
- **Important Issues:** M
- **Minor Issues:** K

---

## 🔴 Critical Issues

### Issue 1: [Title]
**Location:** `path/to/file.py:42`

**Problem:**
[Clear description of the issue]

**Impact:**
[Why this is critical - security, data loss, crashes, etc.]

**Recommendation:**
[Specific fix needed]

**Example:**
```python
# Current (problematic)
user_input = request.GET['id']
query = f"SELECT * FROM users WHERE id = {user_input}"

# Suggested (fixed)
user_input = request.GET.get('id')
if user_input and user_input.isdigit():
    query = "SELECT * FROM users WHERE id = %s"
    cursor.execute(query, (user_input,))

🟡 Important Issues

Issue 2: [Title]

[Same structure as above]

🔵 Minor Issues

Issue 3: [Title]

[Shorter format acceptable for minor issues]

💡 Suggestions

• [Optional improvement 1]
• [Optional improvement 2]

✅ Positive Observations

• [Good practice 1]
• [Well-implemented feature]

Questions for Author

• [Clarifying question about design decision]
• [Question about intended behavior]

Recommendations

1. Fix all critical issues before merge
2. Address important issues or provide justification
3. Consider minor improvements where feasible
4. Add tests for edge cases X, Y, Z

Overall: [Approve with suggestions / Request changes / Needs discussion]

## Common Issues by Language

### Python
```python
# ❌ Mutable default argument
def append_to(element, to=[]):  # Bug: to persists across calls
    to.append(element)
    return to

# ✅ Correct
def append_to(element, to=None):
    if to is None:
        to = []
    to.append(element)
    return to

# ❌ Catching bare exceptions
try:
    risky_operation()
except:  # Too broad, masks errors
    pass

# ✅ Specific exception handling
try:
    risky_operation()
except ValueError as e:
    logger.error(f"Invalid value: {e}")
    raise

# ❌ String concatenation in loops
result = ""
for item in items:
    result += str(item)  # Creates new string each iteration

# ✅ Use join
result = "".join(str(item) for item in items)

JavaScript/TypeScript

// ❌ == instead of ===
if (value == null) {  // Loose equality
    // ...
}

// ✅ Strict equality
if (value === null || value === undefined) {
    // ...
}

// ❌ Unhandled promise rejection
fetchData().then(data => process(data));

// ✅ Error handling
fetchData()
    .then(data => process(data))
    .catch(error => console.error('Error:', error));

// ❌ Variable shadowing
const name = "Global";
function greet() {
    const name = "Local";  // Shadows outer name
    console.log(name);
}

// ✅ Distinct names
const globalName = "Global";
function greet() {
    const userName = "User";
    console.log(userName);
}

Java

// ❌ Resource leak
public void readFile(String path) {
    FileInputStream fis = new FileInputStream(path);
    // ... use fis
    // Missing close(), leaks file handle
}

// ✅ Try-with-resources
public void readFile(String path) {
    try (FileInputStream fis = new FileInputStream(path)) {
        // ... use fis
    } // Automatically closed
}

// ❌ Using == for String comparison
if (str == "value") {  // Compares references
    // ...
}

// ✅ Use equals()
if ("value".equals(str)) {  // Compares content, null-safe
    // ...
}

Security Checklist

Input Validation:

• ✅ All user inputs validated?
• ✅ Whitelist validation used?
• ✅ Input length limits enforced?
• ✅ Special characters escaped?

SQL Injection:

• ✅ Prepared statements/parameterized queries used?
• ✅ No string concatenation for SQL?
• ✅ ORM used correctly?

XSS Prevention:

• ✅ Output encoding applied?
• ✅ HTML sanitization for user content?
• ✅ Content Security Policy headers set?

Authentication/Authorization:

• ✅ Authentication required for sensitive operations?
• ✅ Authorization checks present?
• ✅ Session management secure?
• ✅ Password hashing used (not plain text)?

Data Protection:

• ✅ Sensitive data encrypted at rest?
• ✅ Sensitive data encrypted in transit (HTTPS)?
• ✅ No secrets in code/logs?
• ✅ Proper file permissions set?

Performance Review Points

Algorithm Complexity:

• Nested loops: O(n²) or worse?
• Can use more efficient algorithm?
• Unnecessary iterations?

Database:

• N+1 query problem?
• Missing indexes?
• Fetching unnecessary data?
• Could use batch operations?

Caching:

• Repeated expensive computations?
• Could cache results?
• Cache invalidation handled?

Memory:

• Large objects kept in memory?
• Memory leaks possible?
• Could stream instead of load all?

Code Quality Standards

Function/Method Size:

• ✅ Functions under 50 lines (guideline)
• ✅ Single responsibility per function
• ✅ Clear, descriptive names

Complexity:

• ✅ Cyclomatic complexity < 10 (guideline)
• ✅ Nesting depth reasonable (< 4 levels)
• ✅ Complex logic well-commented

DRY (Don't Repeat Yourself):

• ✅ No code duplication
• ✅ Common logic extracted to functions
• ✅ Constants defined once

Naming:

• ✅ Variables: noun phrases (userData, itemCount)
• ✅ Functions: verb phrases (getUserData, calculateTotal)
• ✅ Booleans: question form (isValid, hasPermission)
• ✅ Classes: PascalCase nouns (UserService, DataProcessor)

Testing Review

Coverage:

• ✅ New code has tests?
• ✅ Critical paths tested?
• ✅ Edge cases covered?

Test Quality:

• ✅ Tests are independent?
• ✅ Clear test names describing what's tested?
• ✅ Arrange-Act-Assert pattern followed?
• ✅ No test interdependencies?

Test Types:

• ✅ Unit tests for business logic?
• ✅ Integration tests for API endpoints?
• ✅ Error cases tested?

Example Reviews

Example 1: Security Issue

### 🔴 Critical: SQL Injection Vulnerability
**Location:** `api/users.py:45`

**Problem:**
User input is directly interpolated into SQL query without sanitization.

**Impact:**
Attacker could execute arbitrary SQL commands, leading to data breach or data loss.

**Code:**
```python
# Current (VULNERABLE)
def get_user(user_id):
    query = f"SELECT * FROM users WHERE id = {user_id}"
    return db.execute(query)

Recommendation: Use parameterized queries to prevent SQL injection.

Fix:

def get_user(user_id):
    query = "SELECT * FROM users WHERE id = %s"
    return db.execute(query, (user_id,))

### Example 2: Performance Issue

```markdown
### 🟡 Important: N+1 Query Problem
**Location:** `services/order_service.py:78-82`

**Problem:**
Loading users in a loop creates N+1 database queries.

**Impact:**
For 100 orders, this creates 101 queries (1 + 100), severely impacting performance.

**Code:**
```python
# Current (inefficient)
orders = Order.query.all()
for order in orders:
    order.user = User.query.get(order.user_id)  # N queries

Recommendation: Use eager loading or a single query with join.

Fix:

# Option 1: Eager loading
orders = Order.query.options(joinedload(Order.user)).all()

# Option 2: Separate query
orders = Order.query.all()
user_ids = [o.user_id for o in orders]
users = {u.id: u for u in User.query.filter(User.id.in_(user_ids)).all()}
for order in orders:
    order.user = users[order.user_id]

## Tips for Effective Reviews

**Be constructive:**
- Explain why, not just what
- Suggest solutions, don't just criticize
- Acknowledge good code

**Be specific:**
- Point to exact lines
- Provide code examples
- Quantify impact when possible

**Prioritize:**
- Fix critical issues first
- Don't nitpick minor style issues
- Focus on what matters

**Ask questions:**
- "Could you explain the reasoning behind...?"
- "Have you considered...?"
- "What happens if...?"

**Provide context:**
- Link to documentation
- Reference coding standards
- Cite security best practices

**Be timely:**
- Review promptly
- Don't block unnecessarily
- Iterate in conversations

This skill provides comprehensive code review guidance. Save it to the current path when ready to package.