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):