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.