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
    // ...
}