Code Review Guidelines

Overview

Code reviews are essential for maintaining code quality, sharing knowledge, and ensuring best practices are followed. This document outlines the guidelines and best practices for conducting effective code reviews.

---

Goals of Code Review

Primary Goals

1. Ensure Code Quality - Verify code meets established standards
2. Catch Bugs Early - Identify potential issues before production
3. Knowledge Sharing - Spread understanding across the team
4. Maintain Consistency - Ensure codebase follows conventions
5. Improve Design - Suggest better architectural approaches
6. Security Review - Identify security vulnerabilities

---

Review Checklist

Functionality

• [ ] Code solves the intended problem
• [ ] Edge cases are handled
• [ ] Error scenarios are covered
• [ ] No obvious bugs or logical errors
• [ ] Changes match the requirements/ticket
• [ ] Breaking changes are documented

Code Quality

• [ ] Code is readable and understandable
• [ ] Functions/methods are single-purpose and focused
• [ ] No unnecessary complexity
• [ ] No code duplication (DRY principle)
• [ ] Proper error handling and logging
• [ ] Type hints are used consistently

Standards Compliance

• [ ] Follows PSR-12 coding standards
• [ ] Naming conventions are followed
• [ ] File and class structure is correct
• [ ] Comments are clear and necessary
• [ ] No commented-out code
• [ ] No debug statements or console logs

Security

• [ ] No SQL injection vulnerabilities
• [ ] Input is properly validated
• [ ] Output is properly escaped
• [ ] Authentication and authorization are correct
• [ ] No sensitive data in logs
• [ ] Secrets are not hardcoded
• [ ] Mass assignment protection is in place

Performance

• [ ] No N+1 query problems
• [ ] Database queries are optimized
• [ ] Proper indexing is used
• [ ] Long-running tasks are queued
• [ ] Caching is implemented where appropriate
• [ ] No memory leaks

Testing

• [ ] Tests are included for new features
• [ ] Tests cover edge cases
• [ ] Tests are meaningful (not just for coverage)
• [ ] All tests pass
• [ ] No flaky tests introduced

Database

• [ ] Migrations are reversible
• [ ] Foreign keys are properly defined
• [ ] Indexes are added where needed
• [ ] No data loss migrations
• [ ] Seeders are updated if needed

Documentation

• [ ] README is updated if needed
• [ ] API documentation is updated
• [ ] Complex logic is commented
• [ ] Breaking changes are documented
• [ ] Migration notes are included

---

Review Process

Before Submitting for Review

For the Author

# 1. Run code formatting
./vendor/bin/pint

# 2. Run static analysis
./vendor/bin/phpstan analyse

# 3. Run tests
php artisan test

# 4. Check for unused imports/variables
# 5. Review your own changes first
# 6. Write a clear PR description

Pull Request Description Template:

## Description
Brief description of what this PR does

## Type of Change
- [ ] Bug fix
- [ ] New feature
- [ ] Breaking change
- [ ] Documentation update

## Related Issue
Fixes #123

## Changes Made
- Added user authentication
- Updated database schema
- Fixed N+1 query in posts

## Testing
- [ ] Unit tests added/updated
- [ ] Feature tests added/updated
- [ ] Manual testing completed

## Screenshots (if applicable)
[Add screenshots if UI changes]

## Checklist
- [ ] Code follows style guidelines
- [ ] Self-review completed
- [ ] Comments added for complex code
- [ ] Documentation updated
- [ ] No new warnings generated
- [ ] Tests pass locally

During Review

For the Reviewer

1. Read the PR Description First

• Understand the context and goals
• Check related tickets/issues

2. Review in Order

1. Migrations (if any)
2. Models
3. Services/Repositories
4. Controllers
5. Routes
6. Tests
7. Configuration changes

3. Use the Checklist

• Go through each section systematically
• Don't rush

4. Test Locally (when needed)

# Pull the branch
git checkout feature/branch-name

# Install dependencies
composer install

# Run migrations
php artisan migrate:fresh --seed

# Run tests
php artisan test

# Test manually
php artisan serve

---

Review Comments

Be Constructive and Respectful

Good Examples

✅ Consider extracting this logic into a separate method for better readability:
[Code suggestion]

✅ This could cause an N+1 query issue. Try using eager loading:
$users = User::with('posts')->get();

✅ Great solution! One small suggestion: we could use a enum here for better type safety.

✅ This works, but have you considered using a repository pattern here?
It would make testing easier.

✅ Nice catch on the edge case! Could we add a test for this scenario?

Bad Examples

❌ This code is terrible.

❌ Why didn't you use a repository?

❌ Obviously this is wrong.

❌ Everyone knows this is not how you do it.

❌ This is the worst implementation I've seen.

Types of Comments

1. Critical - Must be fixed

🚨 **Critical**: This creates a SQL injection vulnerability.
Must use parameter binding.

2. Important - Should be fixed

⚠️ **Important**: This will cause an N+1 query problem with large datasets.

3. Suggestion - Nice to have

💡 **Suggestion**: Consider using a cached scope here for better performance.

4. Question - Seeking clarification

❓ **Question**: Why did we choose this approach over using a repository?

5. Praise - Positive feedback

✨ **Nice**: Great use of early returns for better readability!

---

Common Issues to Look For

1. Security Issues

// ❌ Bad - SQL Injection
DB::select("SELECT * FROM users WHERE email = '{$email}'");

// ✅ Good
User::where('email', $email)->first();

// ❌ Bad - Mass Assignment Vulnerability
User::create($request->all());

// ✅ Good
User::create($request->only(['name', 'email', 'password']));

// ❌ Bad - Exposing Sensitive Data
return response()->json($user);  // Includes password hash

// ✅ Good
return new UserResource($user);  // Excludes sensitive fields

2. Performance Issues

// ❌ Bad - N+1 Query
$users = User::all();
foreach ($users as $user) {
    echo $user->posts->count();  // Query on each iteration
}

// ✅ Good
$users = User::withCount('posts')->get();
foreach ($users as $user) {
    echo $user->posts_count;
}

// ❌ Bad - Loading unnecessary data
$users = User::all();  // Loads all columns

// ✅ Good
$users = User::select(['id', 'name', 'email'])->get();

3. Code Quality Issues

// ❌ Bad - Doing too much in controller
public function store(Request $request)
{
    $validated = $request->validate([...]);
    $user = new User();
    $user->name = $validated['name'];
    $user->password = Hash::make($validated['password']);
    $user->save();
    Mail::to($user)->send(new WelcomeEmail());
    Log::info('User created');
    return response()->json($user);
}

// ✅ Good - Thin controller
public function store(StoreUserRequest $request)
{
    $user = $this->userService->create($request->validated());
    return new UserResource($user);
}

// ❌ Bad - Magic numbers
if ($attempt->count > 5) {
    // Lock account
}

// ✅ Good
const MAX_LOGIN_ATTEMPTS = 5;

if ($attempt->count > self::MAX_LOGIN_ATTEMPTS) {
    // Lock account
}

4. Error Handling Issues

// ❌ Bad - Silent failure
try {
    $this->process();
} catch (\Exception $e) {
    // Empty catch
}

// ✅ Good
try {
    $this->process();
} catch (\Exception $e) {
    Log::error('Process failed', ['error' => $e->getMessage()]);
    throw new ProcessingException('Failed to process', [], $e);
}

// ❌ Bad - Generic exception
throw new \Exception('User not found');

// ✅ Good
throw new UserNotFoundException($userId);

5. Testing Issues

// ❌ Bad - Testing implementation instead of behavior
public function test_method_calls_repository()
{
    $repository = $this->mock(UserRepository::class);
    $repository->shouldReceive('find')->once();
    // ...
}

// ✅ Good - Testing behavior
public function test_returns_user_when_found()
{
    $user = User::factory()->create();

    $response = $this->getJson("/api/users/{$user->id}");

    $response->assertStatus(200)
        ->assertJson(['data' => ['id' => $user->id]]);
}

// ❌ Bad - No assertions
public function test_creates_user()
{
    $this->postJson('/api/users', $data);
    // No assertions
}

// ✅ Good
public function test_creates_user()
{
    $response = $this->postJson('/api/users', $data);

    $response->assertStatus(201);
    $this->assertDatabaseHas('users', ['email' => $data['email']]);
}

---

Responding to Reviews

For the Author

Be Open to Feedback

✅ Good responses:
- "Great catch! I'll fix that."
- "Good point. I hadn't considered that scenario."
- "Thanks for the suggestion. I'll implement that."
- "I chose this approach because X. What do you think?"

❌ Bad responses:
- "This works fine, no need to change."
- "That's not my problem."
- "You're wrong."
- "Just approve it."

Ask for Clarification

✅ "Could you clarify what you mean by...?"
✅ "Can you provide an example of how you'd implement this?"
✅ "I'm not familiar with that pattern. Could you explain more?"

Explain Your Reasoning

✅ "I used this approach because it handles edge case X better."
✅ "I considered that, but chose this for performance reasons."
✅ "The requirements specifically asked for this behavior."

Mark Resolved Comments

• Fix the issue
• Comment with what you changed
• Mark as resolved

---

Review Approval Guidelines

When to Approve

• All critical issues are resolved
• Code meets quality standards
• Tests pass and provide good coverage
• No security vulnerabilities
• Performance is acceptable
• Documentation is adequate

When to Request Changes

• Critical bugs exist
• Security vulnerabilities present
• Code doesn't meet standards
• Tests are missing or failing
• Breaking changes not documented
• Performance issues not addressed

When to Comment (No Approval)

• Minor suggestions that aren't blocking
• Questions for clarification
• General feedback on approach

---

Review Time Guidelines

Response Time

• Small PRs (< 100 lines): Within 4 hours
• Medium PRs (100-500 lines): Within 1 day
• Large PRs (> 500 lines): Within 2 days

Review Duration

• Small PRs: 10-15 minutes
• Medium PRs: 30-60 minutes
• Large PRs: 1-2 hours

Note: Split large PRs into smaller, reviewable chunks when possible.

---

Pull Request Size Guidelines

Ideal PR Size

• Lines changed: 100-300
• Files changed: 3-10
• Focus: Single feature or bug fix

When PR is Too Large

📝 Suggestion: This PR changes 1,500 lines across 30 files.
Consider splitting into:
1. Database migrations + models
2. Service layer implementation
3. API controllers + routes
4. Tests

This will make reviews more thorough and faster.

---

Code Review Tools

GitHub/GitLab Features

# Suggest specific changes
```suggestion
public function getUserById(int $id): ?User
{
    return User::find($id);
}

Request review from specific person

@username Could you review the security aspects?

Link to related PRs/issues

Related to #123 Depends on #456

### Automated Checks

Enable these in CI/CD:

```yaml
# .github/workflows/ci.yml
- name: Run Pint
  run: ./vendor/bin/pint --test

- name: Run PHPStan
  run: ./vendor/bin/phpstan analyse

- name: Run Tests
  run: php artisan test

- name: Check Coverage
  run: php artisan test --coverage --min=80

---

Best Practices

For Authors

1. Keep PRs Small - Easier to review thoroughly
2. Write Clear Descriptions - Explain the "why"
3. Self-Review First - Catch obvious issues
4. Add Tests - Demonstrate functionality
5. Respond Promptly - Don't block the review process
6. Be Receptive - View feedback as learning

For Reviewers

1. Be Timely - Don't leave PRs waiting
2. Be Thorough - Check all aspects
3. Be Constructive - Suggest solutions
4. Ask Questions - Understand the reasoning
5. Praise Good Code - Positive reinforcement
6. Test Locally - For complex changes

---

Review Priority Matrix

Urgency	Complexity	Priority	Review Time
Critical Bug	Simple	Highest	< 2 hours
Critical Bug	Complex	Highest	< 4 hours
Feature	Simple	Medium	< 1 day
Feature	Complex	Medium	< 2 days
Refactor	Simple	Low	< 2 days
Refactor	Complex	Low	< 3 days
Documentation	Any	Low	< 3 days

---

Common Review Mistakes to Avoid

For Reviewers

1. ❌ Nitpicking formatting - Let automated tools handle this
2. ❌ Being vague - "This could be better" → How?
3. ❌ Not testing - Review code AND test functionality
4. ❌ Approving too quickly - Take time to review properly
5. ❌ Being confrontational - Be respectful
6. ❌ Ignoring tests - Tests are as important as code
7. ❌ Requesting changes without explanation - Always explain why

For Authors

1. ❌ Submitting without self-review - Review your own code first
2. ❌ No description - Always explain what and why
3. ❌ Too large PRs - Split into smaller chunks
4. ❌ No tests - Include tests with code
5. ❌ Defensive responses - Be open to feedback
6. ❌ Force pushing - Destroys review history
7. ❌ Rushing approval - Give reviewers time

---

Example Review Comments

Security

🚨 **Security Issue**: This endpoint doesn't check user permissions.

Current:
```php
public function destroy(Post $post)
{
    $post->delete();
}

Suggested:

public function destroy(Post $post)
{
    $this->authorize('delete', $post);
    $post->delete();
}

### Performance

```markdown
⚠️ **Performance**: This will create an N+1 query problem.

Current code loads posts for each user in a loop.
Consider eager loading:

```php
$users = User::with('posts')->get();

This will reduce database queries from N+1 to 2.

### Code Quality

```markdown
💡 **Suggestion**: This method is doing too much. Consider using a service:

```php
// UserService.php
public function register(array $data): User
{
    DB::transaction(function () use ($data) {
        $user = User::create($data);
        $user->profile()->create($data['profile']);
        event(new UserRegistered($user));
        return $user;
    });
}

// Controller
public function store(StoreUserRequest $request)
{
    $user = $this->userService->register($request->validated());
    return new UserResource($user);
}

This separates concerns and makes the code more testable.

---

## Related Documentation

- [Coding Standards](coding-standards.md)
- [Best Practices](best-practices.md)
- [Naming Conventions](naming-conventions.md)
- [Error Handling Standards](../standards/error-handling.md)

---

## Quick Reference

### Review Checklist (Short Version)

```markdown
- [ ] Functionality works as intended
- [ ] Code is readable and maintainable
- [ ] Follows coding standards
- [ ] No security vulnerabilities
- [ ] Performance is acceptable
- [ ] Tests are included and passing
- [ ] Documentation is updated

Comment Prefixes

• 🚨 Critical - Must fix
• ⚠️ Important - Should fix
• 💡 Suggestion - Nice to have
• ❓ Question - Needs clarification
• ✨ Praise - Good work!

Response Time SLA

• Small PR: 4 hours
• Medium PR: 1 day
• Large PR: 2 days