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:

Location: File and line number
Issue: What's wrong
Impact: Why it matters
Recommendation: How to fix
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

Fix all critical issues before merge
Address important issues or provide justification
Consider minor improvements where feasible
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.