**Issue**: [Clear description of the problem]
**Location**: [File and line number]
**Severity**: [Critical/High/Medium/Low]
**Suggestion**: [Specific, actionable recommendation]
**Example**: [Code example showing the improvement]

**Issue**: SQL injection vulnerability
**Location**: `api/users.js:42`
**Severity**: Critical
**Suggestion**: Use parameterized queries instead of string concatenation

**Current code:**
```javascript
const query = `SELECT * FROM users WHERE id = '${userId}'`;

const query = 'SELECT * FROM users WHERE id = ?';
const results = await db.query(query, [userId]);

### Use the Right Tone

**❌ Don't:**
- "This code is terrible"
- "You don't understand how X works"
- "This is obviously wrong"

**✅ Do:**
- "Consider using X instead of Y because..."
- "Have you thought about the case where...?"
- "This works, but could be improved by..."

### Prioritize Issues

**Critical (Must fix before merge):**
- Security vulnerabilities
- Data corruption risks
- Breaking changes
- Test failures

**High (Should fix before merge):**
- Performance issues
- Incorrect business logic
- Poor error handling
- Missing tests for core functionality

**Medium (Nice to have):**
- Code duplication
- Minor optimization opportunities
- Inconsistent naming
- Missing documentation

**Low (Optional):**
- Code style preferences
- Minor refactoring suggestions
- Additional test cases

## Common Patterns to Review

### Pattern 1: Error Handling

**❌ Antipattern - Silent failures:**
```javascript
try {
  await processPayment(order);
} catch (error) {
  // Silently ignoring errors
}

try {
  await processPayment(order);
} catch (error) {
  logger.error('Payment processing failed', {
    orderId: order.id,
    error: error.message,
    stack: error.stack,
  });
  throw new PaymentError('Failed to process payment', { cause: error });
}

def get_user(user_id):
    # No validation - SQL injection risk
    query = f"SELECT * FROM users WHERE id = {user_id}"
    return db.execute(query)

def get_user(user_id: int) -> User:
    # Type validation and parameterized query
    if not isinstance(user_id, int) or user_id <= 0:
        raise ValueError("Invalid user ID")

    query = "SELECT * FROM users WHERE id = ?"
    result = db.execute(query, (user_id,))

    if not result:
        raise UserNotFoundError(f"User {user_id} not found")

    return User.from_row(result[0])

def process_file(filename):
    file = open(filename, 'r')
    data = file.read()
    process(data)
    # File not closed - resource leak

def process_file(filename: str) -> None:
    with open(filename, 'r') as file:
        data = file.read()
        process(data)
    # File automatically closed

function getUserEmail(user) {
  return user.profile.email.toLowerCase();
  // Crashes if user, profile, or email is null/undefined
}

function getUserEmail(user) {
  if (!user?.profile?.email) {
    throw new Error('User email not found');
  }
  return user.profile.email.toLowerCase();
}

// Or with TypeScript
function getUserEmail(user: User): string {
  const email = user.profile?.email;
  if (!email) {
    throw new Error('User email not found');
  }
  return email.toLowerCase();
}

**Security: SQL Injection Vulnerability** (Critical)

**Location**: `src/api/users.ts:45`

The current implementation concatenates user input directly into SQL queries, creating a SQL injection vulnerability.

**Current code:**
```typescript
const query = `SELECT * FROM users WHERE username = '${username}'`;

const query = 'SELECT * FROM users WHERE username = ?';
const users = await db.query(query, [username]);

### Performance Issue

for (const order of orders) {
  order.items = await db.query('SELECT * FROM order_items WHERE order_id = ?', [
    order.id,
  ]);
}

const orderIds = orders.map((o) => o.id);
const allItems = await db.query(
  'SELECT * FROM order_items WHERE order_id IN (?)',
  [orderIds]
);

// Group items by order_id
const itemsByOrder = allItems.reduce((acc, item) => {
  if (!acc[item.order_id]) acc[item.order_id] = [];
  acc[item.order_id].push(item);
  return acc;
}, {});

orders.forEach((order) => {
  order.items = itemsByOrder[order.id] || [];
});

### Code Quality Issue

function validateUser(user: User): ValidationResult {
  return {
    ...validateUsername(user.username),
    ...validateEmail(user.email),
    ...validatePassword(user.password),
    ...validateAge(user.age),
  };
}

function validateUsername(username: string): ValidationResult {
  if (!username || username.length < 3) {
    return { valid: false, error: 'Username must be at least 3 characters' };
  }
  return { valid: true };
}

## Resources

- **Code Review Best Practices**: [Google Engineering Practices](https://google.github.io/eng-practices/review/)
- **Security Guidelines**: [OWASP Top 10](https://owasp.org/www-project-top-ten/)
- **Clean Code**: Robert C. Martin's "Clean Code"
- **Code Complete**: Steve McConnell's "Code Complete 2"

## Final Review Checklist

Before approving:

- [ ] All critical and high-priority issues addressed
- [ ] Tests are passing
- [ ] No security vulnerabilities
- [ ] Performance is acceptable
- [ ] Code follows project standards
- [ ] Documentation is updated
- [ ] Breaking changes are noted
- [ ] Feedback is constructive and specific