📖 Code Review Guidelines¶

Effective code reviews improve code quality, share knowledge, and build team collaboration. Follow these guidelines to make reviews constructive and efficient.

🎯 How to Review Code¶

1. Be Constructive¶

Goal: Help improve the code, not criticize the author.

❌ BAD:

This is wrong.

✅ GOOD:

Consider using a std::vector instead of a raw array here because:
1. Automatic memory management
2. Bounds checking with .at()
3. Size tracking built-in

2. Focus on Code, Not Person¶

Goal: Keep feedback objective and professional.

❌ BAD:

You don't understand how RAII works.

✅ GOOD:

This code could be improved by using RAII to ensure resources
are cleaned up automatically. Here's an example: [link to docs]

3. Ask Questions¶

Goal: Understand intent before suggesting changes.

❌ BAD:

Change this to use a factory pattern.

✅ GOOD:

Could we simplify this construction logic? What do you think
about using a factory pattern to reduce duplication? Here's
how it might look: [code example]

4. Distinguish Must vs Nice-to-Have¶

Goal: Clarify priority to help authors focus on critical issues.

Use clear labels:

🚫 Blocking: Must be fixed before merge
⚠️ Non-blocking: Should be fixed but not a blocker
💡 Suggestion: Optional improvement

Examples:

🚫 BLOCKING: This will crash in production when input exceeds buffer size.
Fix: Add bounds checking before memcpy.

⚠️ NON-BLOCKING: Consider renaming `process()` to `processAudioBuffer()`
for clarity.

💡 SUGGESTION: You might want to look at std::span (C++20) for safer
buffer handling in the future.

5. Explain the Why¶

Goal: Help authors learn and make better decisions.

❌ BAD:

Use const here.

✅ GOOD:

Mark this parameter `const&` to:
1. Prevent accidental modification
2. Document intent (read-only)
3. Enable compiler optimizations
4. Allow binding to temporaries

6. Provide Examples¶

Goal: Make suggestions concrete and actionable.

❌ BAD:

This could be more efficient.

✅ GOOD:

This could be more efficient. Instead of:

    for (size_t i = 0; i < vec.size(); ++i) {
        vec[i] *= gain;
    }

Consider SIMD vectorization:

    simd_multiply(vec.data(), gain, vec.size());

This should be ~4x faster for float processing.
Benchmark: [link]

7. Praise Good Work¶

Goal: Reinforce positive patterns and boost morale.

✅ EXAMPLES:

✨ Excellent use of RAII here for cleanup!

👍 Great test coverage on the edge cases.

💯 Love how this simplifies the API surface.

🎯 Perfect choice of data structure for this use case.

📋 Approval Standards¶

✅ Approve When:¶

Functionality is correct
Code does what it claims
Tests prove it works
No obvious bugs
Tests are adequate
Critical paths covered
Edge cases tested
Tests pass reliably
No critical issues
No security vulnerabilities
No data corruption risks
No production crashes
Minor issues can be addressed in follow-up
Style nitpicks
Optional refactoring
Nice-to-have improvements

⚠️ Request Changes When:¶

Bugs are present
Logic errors
Race conditions
Memory leaks
Buffer overflows
Tests are missing or insufficient
No tests for new functionality
Critical paths untested
Edge cases ignored
Security issues exist
Input validation missing
Injection vulnerabilities
Secrets exposed
Unsafe dependencies
Architecture is violated
Layer boundaries broken
Wrong dependencies
Tech debt added without justification

💬 Comment Without Verdict When:¶

Minor suggestions only
Style preferences
Optional refactoring
Alternative approaches
Questions for learning
Clarifying design decisions
Understanding rationale
Exploring alternatives
Discussion topics
Future improvements
Related issues
Broader implications

⏰ Response Time Expectations¶

Timely reviews keep development flowing. Aim for these targets:

╔════════════════════════════════════════════════════════╗
║ PR Size     │ First Response │ Final Approval         ║
╠═════════════╪════════════════╪════════════════════════╣
║ Trivial     │ 4 hours        │ Same day               ║
║ (<50 lines) │                │                        ║
╠═════════════╪════════════════╪════════════════════════╣
║ Small       │ 24 hours       │ 1-2 days               ║
║ (50-200)    │                │                        ║
╠═════════════╪════════════════╪════════════════════════╣
║ Medium      │ 24 hours       │ 2-3 days               ║
║ (200-500)   │                │                        ║
╠═════════════╪════════════════╪════════════════════════╣
║ Large       │ 24 hours       │ 3-5 days               ║
║ (500-1000)  │                │ (consider splitting)   ║
╠═════════════╪════════════════╪════════════════════════╣
║ Huge        │ N/A            │ MUST BE SPLIT          ║
║ (>1000)     │                │ into smaller PRs       ║
╚════════════════════════════════════════════════════════╝

Special Cases: - 🔥 Hotfix: Review within 1 hour - 🚀 Time-sensitive: Communicate urgency, adjust priorities - 🏖️ Low priority: Can wait for next review cycle

🎨 Review Tone Best Practices¶

Language Matters¶

Collaborative Language: - "We could..." instead of "You should..." - "What if we..." instead of "Why didn't you..." - "Consider..." instead of "Do this..." - "I wonder if..." instead of "This is wrong..."

Examples:

❌ Directive	✅ Collaborative
"Fix this bug"	"I think there's a bug here because... Could you verify?"
"You forgot error handling"	"We might want to add error handling here for..."
"This is inefficient"	"I wonder if we could optimize this by..."
"Don't do it this way"	"What do you think about an alternative approach like..."

📝 Review Comment Templates¶

Requesting Clarification¶

❓ **Question:** Could you explain the rationale behind using X instead of Y here?
I want to make sure I understand the tradeoffs.

Suggesting Improvement¶

💡 **Suggestion:** Consider extracting this logic into a helper function:

[code example]

Benefits:
- Reusable in [other location]
- Easier to test in isolation
- Clearer intent with named function

Thoughts?

Identifying Bug¶

🚫 **BLOCKING - Bug:** This will fail when `size == 0`:

Line 42: `buffer[size - 1] = value;`

Fix: Add bounds check:
```cpp
if (size > 0) {
    buffer[size - 1] = value;
}

### Praising Good Code

```markdown
✨ **Excellent:** Love the use of std::optional here! It makes the
"not found" case explicit and type-safe. Great choice.

Performance Concern¶

⚠️ **Performance:** This nested loop is O(n²) which could be slow
for large datasets.

Consider using a hash map for O(n) lookup:

[code example]

Benchmark showing the difference: [link]

Worth optimizing now, or acceptable for current use case?

🔄 Review Workflow¶

1. Initial Review¶

Understand the context
Read PR description
Check linked issues
Understand the goal
High-level check
Architecture fit
Approach soundness
Test strategy
Detailed review
Line-by-line code
Edge cases
Error handling
Provide feedback
Group related comments
Prioritize issues
Be clear on blocking items

2. Author Response¶

Address all comments
Ask for clarification if needed
Mark resolved discussions
Request re-review

3. Follow-up Review¶

Focus on changes since last review
Verify fixes implemented
Check for new issues
Approve or iterate

4. Merge¶

Ensure CI passes
Verify all approvals
Squash commits if needed
Merge with clear message

🚀 Advanced Review Techniques¶

Look for Common Patterns¶

Resource Leaks: Files, memory, locks not released
Race Conditions: Shared state without synchronization
Error Swallowing: Exceptions caught but not handled
Null Derefs: Pointers used without null checks
Buffer Overflows: Array access without bounds check
Integer Overflow: Math operations without overflow check

Use Tools¶

Static Analysis: clang-tidy, cppcheck
Sanitizers: ASan, TSan, UBSan, MSan
Profilers: perf, Instruments, VTune
Coverage: gcov, lcov
Benchmarks: Google Benchmark, Catch2

Review in Chunks¶

For large PRs: 1. Architecture and approach (first pass) 2. Core logic and algorithms (second pass) 3. Tests and edge cases (third pass) 4. Style and documentation (fourth pass)

📊 Measuring Review Effectiveness¶

Track these metrics to improve:

Time to merge: PR created → merged
Review cycles: Number of request-changes rounds
Bug escape rate: Bugs found in production that passed review
Review coverage: % of code reviewed by 2+ people
Response time: PR created → first review

Goals: - Time to merge: < 3 days (for medium PRs) - Review cycles: ≤ 2 rounds - Bug escape rate: < 1% - Review coverage: > 95% - Response time: < 24 hours

Review Checklist - Comprehensive review checklist
PR Template - Pull request template
Style Guide - Coding style standards
Testing Standards - Testing requirements

💡 Remember¶

The goal of code review is not to find every tiny issue, but to: 1. Prevent bugs from reaching production 2. Share knowledge across the team 3. Maintain code quality and consistency 4. Mentor and learn from each other 5. Build a collaborative culture

Happy Reviewing! 🎉

📖 Code Review Guidelines¶

🎯 How to Review Code¶

1. Be Constructive¶

2. Focus on Code, Not Person¶

3. Ask Questions¶

4. Distinguish Must vs Nice-to-Have¶

5. Explain the Why¶

6. Provide Examples¶

7. Praise Good Work¶

📋 Approval Standards¶

✅ Approve When:¶

⚠️ Request Changes When:¶

💬 Comment Without Verdict When:¶

⏰ Response Time Expectations¶

🎨 Review Tone Best Practices¶

Language Matters¶

📝 Review Comment Templates¶

Requesting Clarification¶

Suggesting Improvement¶

Identifying Bug¶

Performance Concern¶

🔄 Review Workflow¶

1. Initial Review¶

2. Author Response¶

3. Follow-up Review¶

4. Merge¶

🚀 Advanced Review Techniques¶

Look for Common Patterns¶

Use Tools¶

Review in Chunks¶

📊 Measuring Review Effectiveness¶

🔗 Related Resources¶

💡 Remember¶