2.3: Code Review Excellence

Code Reviews for Safety-Critical Systems

Why Code Reviews Matter

Industry Data:

• Code reviews catch 60–90% of defects before testing (IBM and Microsoft studies)
• Defect cost: Review (€50–200) vs. field (€10,000–100,000)
• Review effectiveness: 80–90% defect detection with trained reviewers

ASPICE Requirement:

• SUP.2: "Conduct reviews of work products" (code reviews are mandatory for CL2+)
• SWE.3 BP6: "Verify software units" (includes code review)

Safety Standards:

• ISO 26262 Part 6: "Semi-formal verification methods" (code reviews for ASIL-B/C/D)
• IEC 62304: "Software unit verification" (reviews required for Class B/C)

Code Review Process

1. Before Review (Author's Responsibility)

Author Checklist (before requesting review):

## Pre-Review Checklist

### Code Quality
- [ ] All functions have Doxygen headers?
- [ ] All public functions have @implements tags (traceability)?
- [ ] No magic numbers (all constants named)?
- [ ] Function length ≤50 lines (ideally 5–15)?
- [ ] Variable names are self-documenting?

### Testing
- [ ] All new code has unit tests?
- [ ] Test coverage ≥90% (check with gcov)?
- [ ] All tests pass locally?

### Standards Compliance
- [ ] MISRA C:2012 violations checked (cppcheck, PC-lint)?
- [ ] No compiler warnings (-Wall -Wextra)?
- [ ] Code formatted (clang-format)?

### Traceability
- [ ] All functions link to requirements (SWE-XXX)?
- [ ] Traceability matrix updated?

### Documentation
- [ ] README updated (if API changed)?
- [ ] Architecture Decision Record (ADR) written (if design decision made)?

Self-Review (author reviews own code before requesting peer review):

1. Read your own code line-by-line as if reviewing someone else's.
2. Check for obvious issues (typos, missing error handling, unclear names).
3. Run static analysis (cppcheck, PC-lint).

Benefit: Catches 30–40% of defects before peer review, saving reviewer time.

2. During Review (Reviewer's Responsibility)

Review Checklist (SUP.2 compliant):

A. Correctness

- [ ] Does code implement requirements correctly?
- [ ] Are all edge cases handled (boundary, error, invalid input)?
- [ ] Are return codes checked (no ignored errors)?
- [ ] Is error handling defensive (fail-safe behavior)?

Example Issue:

/* [FAIL] Bad: Ignores CAN read error */
CAN_ReadMessage(0x200, buffer);
float distance = buffer[0];  /* What if CAN timed out? */

/* [PASS] Good: Checks error, handles failure */
if (CAN_ReadMessage(0x200, buffer) != CAN_OK) {
    Log_Error(ERROR_CAN_TIMEOUT, 0x200);
    return -1;  /* Fail-safe */
}

B. Readability

- [ ] Are function/variable names clear (no cryptic abbreviations)?
- [ ] Are functions small (5–15 lines, single responsibility)?
- [ ] Are comments necessary (explain WHY, not WHAT)?
- [ ] Is code formatted consistently (indentation, braces)?

Example Feedback:

/* Reviewer comment: Function name unclear */
void Process(int x) { /* Process what? */
    /* ... */
}

/* Author fix: Renamed to clarify purpose */
void ValidateSensorData(int sensor_id) {
    /* ... */
}

C. Testability

- [ ] Can this function be unit tested (no global state, I/O dependencies)?
- [ ] Are unit tests included for new code?
- [ ] Do tests cover nominal, boundary, and error cases?
- [ ] Is test coverage ≥90%?

Example Issue:

/* [FAIL] Bad: Hard to test (depends on global CAN hardware) */
float GetDistance(void) {
    return CAN_ReadRadarDistance();  /* Requires real CAN hardware */
}

/* [PASS] Good: Testable (dependency injection) */
float GetDistance(CAN_Interface* can) {
    return can->ReadRadarDistance();  /* Can mock CAN interface */
}

D. MISRA C:2012 Compliance

- [ ] No MISRA violations (check cppcheck report)?
- [ ] If violations exist, are they documented and justified?
- [ ] Are all violations reviewed by safety engineer (for ASIL-B+)?

Example MISRA Violation:

/* cppcheck warning: MISRA 10.4 (incompatible essential types) */
uint16_t x = 10;
int16_t y = -5;
int16_t z = x + y;  /* [FAIL] Unsigned + signed */

/* Fix: Explicit cast */
int16_t z = (int16_t)x + y;  /* [PASS] Explicit, safe conversion */

E. Traceability

- [ ] Do all functions have @implements tags linking to requirements?
- [ ] Do all tests have @verifies tags linking to requirements?
- [ ] Is traceability matrix updated (req → code → test)?

Example:

/**
 * @brief Calculate safe following distance
 * @implements [SWE-045-11] Safe Following Distance
 * ↑ Reviewer checks: Is [SWE-045-11] a valid requirement in DOORS?
 */
float ACC_CalculateSafeDistance(float speed_kmh) { /* ... */ }

F. Safety Requirements (ASIL-B+)

- [ ] Are safety requirements clearly marked (@safety_class ASIL-B)?
- [ ] Is fail-safe behavior defined for all error cases?
- [ ] Are assertions used for preconditions (defensive programming)?
- [ ] Is there evidence of defensive programming (input validation, range checks)?

3. Review Comments (How to Give Feedback)

Good Review Comments (constructive, specific):

[PASS] "Function name `Process()` is unclear. Suggest renaming to `ValidateSensorData()` to clarify purpose."

[PASS] "Line 45: Missing error handling for CAN_ReadMessage(). Add check for CAN_OK and return error code if timeout."

[PASS] "Lines 67-85: Function too long (19 lines). Consider extracting calculation into separate function `CalculateFusedDistance()`."

[PASS] "Line 102: Magic number `150`. Define as constant `MAX_SPEED_KMH` with comment explaining rationale (ISO 26262 requirement)."

Bad Review Comments (vague, critical tone):

[FAIL] "This is wrong." (What is wrong? How to fix?)

[FAIL] "You should know better." (Unhelpful, demotivating)

[FAIL] "I would have done it differently." (Not actionable feedback)

[FAIL] "This code is a mess." (Too general, no specific issues)

Tone Guidelines:

• Be kind: Assume positive intent (author did their best)
• Be specific: Point to exact line, explain issue, suggest fix
• Be educational: Explain why (not just what to change)
• Be collaborative: "We" (not "you") - "We should add error handling here"

4. After Review (Author Responds)

Author Response Options:

1. Accept: "Good catch, fixed in commit abc123"
2. Clarify: "This is intentional because... (explain rationale)"
3. Discuss: "I considered that, but here's why I chose this approach..." (start discussion)

Example Response:

Reviewer: "Line 45: Missing error handling for CAN_ReadMessage()"

Author: "Good catch! Added error handling in commit abc123:
  - Returns -1 on CAN timeout
  - Logs error for diagnostics
  - Updated unit test to verify error path
Thanks!"

Review Metrics (ASPICE SUP.2)

Track Review Effectiveness

Metrics to Collect:

Example Report:

## Code Review Report: PR#142 (ACC Speed Control)

**Summary**:
- Lines changed: 234 LOC
- Review time: 90 minutes
- Review rate: 156 LOC/hour [PASS] (target: 100-200)
- Defects found: 12
- Defect density: 51 defects/KLOC [WARN] (target: 5-10, high for safety-critical)

**Defects by Category**:
- MISRA violations: 5 (Rule 10.4, 12.1, 14.4)
- Missing error handling: 3
- Magic numbers: 2
- Unclear naming: 2

**Action**: Author to fix all defects, re-review required

**Re-Review**: After fixes, 0 defects found [PASS] Approved for merge

Review Best Practices

1. Keep Reviews Small

Guideline: Review ≤300 LOC per session (optimal: 150–200 LOC)

Rationale:

• Review effectiveness drops after 60–90 minutes
• At 100–200 LOC/hour, reviewing 300 LOC takes 90–180 minutes
• Large reviews miss defects (cognitive overload)

If PR is Large (>400 LOC):

• Break into smaller PRs (one feature per PR)
• Review in multiple sessions (take breaks)

2. Use Checklists

Checklist Ensures Consistency

Example Checklist (customize for your project):

## Code Review Checklist (ACC ECU)

### Correctness (10 min)
- [ ] Implements requirements correctly?
- [ ] All edge cases handled?
- [ ] Error handling defensive?

### Readability (10 min)
- [ ] Function/variable names clear?
- [ ] Functions small (5–15 lines)?
- [ ] Comments explain WHY (not WHAT)?

### Testability (10 min)
- [ ] Unit tests included?
- [ ] Coverage ≥90%?
- [ ] Tests cover nominal, boundary, error cases?

### MISRA Compliance (10 min)
- [ ] cppcheck report clean?
- [ ] Violations documented?

### Traceability (10 min)
- [ ] @implements tags present?
- [ ] Traceability matrix updated?

### Safety (10 min, ASIL-B only)
- [ ] @safety_class annotation?
- [ ] Fail-safe behavior defined?
- [ ] Defensive programming (input validation)?

3. Review in Context

Don't Just Review Diff (Changed Lines) - Review Entire Function

Example:

/* Diff (changed lines) shows only this change: */
- if (speed > 150) {
+ if (speed_kmh > MAX_SPEED_KMH) {

/* But reviewer should read entire function to understand context: */
void ACC_SetSpeed(int speed_kmh) {
    const int MAX_SPEED_KMH = 150;  /* ISO 26262 requirement */

    if (speed_kmh > MAX_SPEED_KMH) {  /* ← This line changed */
        speed_kmh = MAX_SPEED_KMH;
    }

    /* Missing: Input validation for negative speed! */
    /* Reviewer catches this by reading full function, not just diff */
    g_set_speed = speed_kmh;
}

4. Automate What You Can

Automated Checks (run before human review):

• Formatting: clang-format (enforce consistent style)
• Static Analysis: cppcheck, PC-lint (catch MISRA violations)
• Tests: CI/CD runs tests automatically (verify all pass)
• Coverage: gcov (verify coverage ≥90%)

Benefit: Reviewer focuses on logic, design, safety (not formatting, MISRA)

CI/CD Pipeline (GitLab CI example):

stages:
  - format-check
  - static-analysis
  - test
  - review

format-check:
  stage: format-check
  script:
    - clang-format --dry-run --Werror src/*.c  # Fails if not formatted

static-analysis:
  stage: static-analysis
  script:
    - cppcheck --enable=all --error-exitcode=1 src/*.c
    - pc-lint std.lnt src/*.c  # MISRA C:2012 check

test:
  stage: test
  script:
    - mkdir build && cd build
    - cmake .. && make
    - ./acc_tests  # Run unit tests
    - gcov ../src/*.c  # Generate coverage report
    - lcov --summary coverage.info | grep "lines......: 9[0-9]%"  # Verify ≥90%

review:
  stage: review
  script:
    - echo "All automated checks passed. Ready for human review."
  when: manual  # Manual approval (human code review)

Common Review Anti-Patterns

Anti-Pattern 1: Rubber Stamp Review

Problem: Reviewer approves without reading code ("Looks good to me")

Solution:

• Require checklist completion
• Track review time (should be 60–90 min for 150–200 LOC)
• Measure defects escaped to testing (if too high, reviews not effective)

Anti-Pattern 2: Nitpicking

Problem: Reviewer focuses on style issues (spaces, brace position) instead of logic

Solution:

• Automate style checks (clang-format)
• Focus review on correctness, safety, design

Example:

[FAIL] "Line 45: Use spaces instead of tabs" (waste of reviewer time, automate this)

[PASS] "Line 45: Missing error handling for CAN timeout. Add check and fail-safe behavior." (real defect)

Anti-Pattern 3: Review After Implementation Complete

Problem: Large PR (1000+ LOC) at end of sprint, too late to make major changes

Solution:

• Review early and often (incremental PRs)
• Review design before implementation (ADR review)
• Pair programming (continuous review)

Pair Programming as Continuous Review

Alternative to Traditional Code Review

Pair Programming: Two engineers, one keyboard

• Driver: Types code
• Navigator: Reviews code real-time, thinks ahead

Benefits:

• Defects caught immediately (no separate review phase)
• Knowledge sharing (junior learns from senior)
• Higher code quality (two brains better than one)

When to Use:

• Complex features (Kalman filter, PID tuning)
• Safety-critical code (ASIL-B braking logic)
• Onboarding (new engineer pairs with experienced)

When NOT to Use:

• Simple tasks (formatting, documentation)
• Time-critical (pair programming 15–25% slower initially)

Summary

Code Review Best Practices:

1. Before Review: Self-review, run static analysis, ensure tests pass (author's responsibility)
2. During Review: Use checklist (correctness, readability, testability, MISRA, traceability, safety)
3. Review Comments: Be kind, specific, educational (constructive feedback)
4. After Review: Author fixes issues, responds to comments

ASPICE SUP.2 Compliance: Reviews are mandatory, track metrics (coverage, defect density, review rate)

Effectiveness: Reviews catch 60–90% of defects (much cheaper than finding in testing or field)

Automation: Automate formatting, static analysis, tests (reviewer focuses on logic, design, safety)

Next: Continuous Integration Mastery (34.04) — Automating builds, tests, and checks in CI/CD pipelines