Skip to main content

Code Review Best Practices

Code review is a collaborative process to ensure code quality, catch bugs early, share knowledge, and maintain consistency across the codebase.

Overview​

Effective code review catches bugs before production, spreads knowledge across the team, and maintains high code quality standards. In banking applications, thorough code review is essential for security, compliance, and reliability.

Code Review Purpose

Code review is NOT about finding fault with the author. It's about collaboratively ensuring high-quality, maintainable code that meets business requirements and technical standards.

Core Principles​

  • Constructive: Provide helpful feedback, not criticism
  • Timely: Review within 4 hours of request
  • Thorough: Check logic, tests, security, performance
  • Collaborative: Discuss and learn together
  • Automated Where Possible: Let pipelines handle formatting, linting, test execution

Automated vs Manual Review​

Automated (Pipeline Enforced)​

These should NEVER be reviewed manually. If pipeline passes, assume these are correct:

  • Formatting: Spotless (Java), Prettier (JavaScript/TypeScript)
  • Linting: ESLint, SonarQube quality gates
  • Tests: All unit, integration, contract tests passing
  • Coverage: >85% code coverage, >80% mutation coverage
  • Security: SAST scans, dependency vulnerability checks
  • Build: Successful compilation and packaging
Let Automation Work

If a reviewer comments on formatting or linting, the pipeline configuration needs fixing, not the code. Never waste review time on things machines can check.

Manual Review Focus​

Reviewers should focus on:

  1. Logic and Correctness

    • Does the code do what it's supposed to?
    • Are edge cases handled?
    • Is error handling appropriate?

  2. Design and Architecture

    • Does this fit our architecture?
    • Are patterns used correctly?
    • Is this the right solution approach?

  3. Security and Compliance

    • No sensitive data logged or exposed
    • Input validation present
    • Authorization checks correct
    • Audit logging for financial transactions

  4. Performance

    • No obvious performance issues
    • Database queries optimized
    • Caching used appropriately

  5. Maintainability

    • Code is readable and understandable
    • Comments explain "why", not "what"
    • Complex logic is well-documented

  6. Testing

    • Test coverage is meaningful (not just high percentage)
    • Edge cases tested
    • Integration tests for critical paths

Review Requirements​

Approval Requirements (GitLab)​

Feature Merge Requests (to develop or feature branches):

  • Approvals required: 1
  • Eligible approvers: Any team member (except author)
  • Bypass: Not allowed

Release Merge Requests (to main or release/*):

  • Approvals required: 2
  • Eligible approvers: Tech leads, senior developers
  • Code owner approval: Required for shared modules
  • Security review: Required if security-related changes

Hotfix Merge Requests:

  • Approvals required: 2 (expedited timeline)
  • Eligible approvers: Tech leads on-call
  • Post-merge review: Required within 24 hours

Protected Branch Settings​

Configure in Settings > Repository > Protected Branches:

main:
  - Allowed to merge: Maintainers
  - Allowed to push: No one
  - Code owner approval required: Yes
  - Approvals required: 2

develop:
  - Allowed to merge: Developers + Maintainers
  - Allowed to push: No one
  - Approvals required: 1

release/*:
  - Allowed to merge: Maintainers
  - Allowed to push: No one
  - Approvals required: 2

For Code Authors​

Before Requesting Review​

Self-Review Checklist:

  • I've read through all my changes in GitLab diff view
  • All automated checks are passing (green pipeline)
  • MR description is complete with Jira link, problem, solution, evidence
  • Tests added for new functionality
  • No obvious bugs or issues
  • Complex logic has comments explaining "why"
  • No sensitive data logged (passwords, tokens, PII)
  • Database migrations tested (if applicable)
  • OpenAPI spec updated (if API changed)
  • Reviewers tagged (1-2 specific people, not entire team)

How to Request Review:

@john @sarah - Please review when you have a chance

**Estimated review time**: 30 minutes
**Priority**: Normal
**Areas of focus**:
- Payment retry logic in PaymentService.java (lines 145-203)
- Error handling approach
- Test coverage for edge cases

**Questions for reviewers**:
1. Is the retry configuration too aggressive?
2. Should we add circuit breaker here?

During Review​

Responding to Feedback:

 Good Response:
> **Reviewer**: Consider using Optional.ofNullable() here to handle nulls
"Good catch! Changed to Optional.ofNullable() in commit abc123"

 Good Response (Disagreement):
> **Reviewer**: This should use a singleton pattern
"I considered that, but chose instance-per-request for thread safety.
Happy to discuss if you feel strongly. See this article: [link]"

 Good Response (Follow-up):
> **Reviewer**: Add validation for negative amounts
"Great idea! Created follow-up task JIRA-5678 since this MR is already large.
Will address in next sprint."

 Bad Response:
> **Reviewer**: Consider using Optional.ofNullable() here
"Done" (no explanation, hard to verify)

 Bad Response:
> **Reviewer**: This should use a singleton pattern
"No" (dismissive, no explanation)

GitLab Thread Resolution:

  • Author resolves: When you've addressed the feedback
  • Explain what you did: Don't just click "Resolve"
  • Reviewer re-reviews: Reviewer should verify resolution
  • All threads must be resolved: Enforced before merge

Timely Responses:

  • Target: Respond within 4-8 hours during work hours
  • Acknowledgment: If you need time, acknowledge and give ETA
  • Re-request review: After addressing all feedback

After Approval​

Pre-Merge Checklist:

  • All review threads resolved
  • All required approvals received
  • Pipeline still passing (re-run if stale)
  • No merge conflicts with target branch
  • Squash commits if multiple (clean history)

Squash Commit Message:

[JIRA-1234] feat: Add payment retry mechanism

Implements exponential backoff retry logic for failed payments.
- Retries up to 3 times with exponential backoff
- Circuit breaker opens after 5 consecutive failures
- Comprehensive error handling and logging

Co-authored-by: Sarah Reviewer <[email protected]>

For Code Reviewers​

Review Types​

1. Task (Blocking)

Use for issues that MUST be fixed before merge:

**Task**: This will cause a NullPointerException in production

The `payment.getRecipient()` call on line 145 can be null if the payment
is scheduled. Add a null check or use Optional.

```java
// Suggested fix:
String recipient = payment.getRecipient()
    .orElse("Unknown");

**2. Comment (Discussion)** 

Use for questions, alternative approaches, or suggestions:

```markdown
**Comment**: Have you considered using a circuit breaker here?

The retry logic looks good, but for external API calls, a circuit breaker
pattern might prevent cascading failures. Resilience4j has good support.

Not blocking for this MR, but worth discussing for future work.

3. Nitpick (Non-blocking)

Use for minor improvements that don't block merge:

**Nitpick**: Variable name could be more descriptive

`tmp` on line 67 could be renamed to `formattedAmount` for clarity.
Feel free to address in a follow-up if you prefer.

4. Praise

Don't forget to acknowledge good work:

**Praise**: Excellent error handling!

The comprehensive error messages with context will make debugging much easier.
Great attention to detail.

Using GitLab Threading​

Create Thread:

  1. Click line number in diff
  2. Add comment with type prefix
  3. Mark as Start a thread
  4. For blocking issues, mark as Priority or use /label ~blocker

Thread Actions:

/label ~blocker           # Mark thread as blocking
/label ~suggestion        # Mark as suggestion
/assign @author          # Assign to author
/due tomorrow            # Set due date for resolution

Resolve Thread:

  • Author addresses feedback
  • Author adds comment explaining resolution
  • Reviewer verifies and resolves thread
  • All threads MUST be resolved before merge

Review Process​

Review Checklist​

Logic & Correctness:

  • Code does what PR description claims
  • Edge cases handled appropriately
  • Error handling comprehensive
  • No obvious bugs or logic errors
  • Null safety (Java: Optional, TypeScript: proper type guards)

Architecture & Design:

  • Fits existing architecture patterns
  • No unnecessary complexity
  • Proper layering (controller → service → repository)
  • Dependencies injected correctly (constructor injection)
  • No God classes or methods (single responsibility)

Security (Critical for Sensitive Systems):

  • No sensitive data logged (passwords, tokens, credentials, PII)
  • Input validation present and comprehensive
  • SQL injection prevented (use parameterized queries)
  • Authorization checks in place
  • Audit logging for financial transactions
  • No hardcoded secrets or credentials

Performance:

  • No N+1 query problems
  • Database queries optimized (indexes exist)
  • Appropriate caching used
  • No obvious memory leaks
  • API response times acceptable

Testing:

  • Unit tests for business logic
  • Integration tests for critical paths
  • Tests are meaningful (not just coverage hunting)
  • Edge cases tested
  • Error scenarios tested
  • Mutation tests passing (>80% Java, >75% TypeScript)

Code Quality:

  • Variable/method names are descriptive
  • Comments explain "why", not "what"
  • No dead or commented-out code
  • No TODO comments (create Jira tickets instead)
  • Consistent with codebase style
  • Code is readable and maintainable

Documentation:

  • README updated if needed
  • API documentation updated (OpenAPI spec)
  • Complex logic documented
  • Architecture decisions recorded (ADR if significant)

How to Comment Productively​

Be Specific:

 Bad: "This is wrong"
 Good: "This will throw NPE if payment.getRecipient() is null (line 145)"

 Bad: "Performance issue"
 Good: "This query will cause N+1 problem. Consider using JOIN FETCH
         or separate query with IN clause. See docs/database.md#n+1"

 Bad: "Not secure"
 Good: "Sensitive credential is logged on line 67, violating security policy.
         Use maskSensitiveData() utility before logging."

Suggest Solutions:

GOOD:  Provide code suggestion:
**Task**: Use Optional to handle potential null

```diff
return paymentRepository.findById(id)
    .orElseThrow(() -> new PaymentNotFoundException(id));

GOOD: Link to examples: Comment: Consider using the builder pattern here

See PaymentTestBuilder.java for an example of how we typically implement builders in this codebase.


**Ask Questions**:

```markdown
GOOD:  "Why did you choose approach X over Y? I'm curious about the trade-offs."

GOOD:  "Have you considered using a circuit breaker here? Not blocking,
   but worth discussing for resilience."

GOOD:  "Can you explain the logic on lines 145-160? I'm having trouble
   following the intent."

Frame Feedback Constructively:

 "This is a terrible way to do it"
 "I think there might be a simpler approach. What do you think about...?"

BAD:  "You didn't handle errors"
 "What should happen if the external API call fails? I don't see
   error handling for that case."

BAD:  "This violates SOLID principles"
GOOD:  "This class has multiple responsibilities. Consider splitting into
   PaymentValidator and PaymentProcessor for better testability."

Review Timing​

Target Response Times:

MR TypeFirst ReviewApproval
Hotfix< 1 hour< 2 hours
Small (<200 lines)< 4 hours< 8 hours
Medium (200-500 lines)< 8 hours< 1 day
Large (500+ lines)< 1 day< 2 days

If You Can't Review Promptly:

Thanks for the review request! I'm heads-down on JIRA-9999 today.
Can review tomorrow morning, or @alice might be able to review sooner?

GitLab Code Review Features​

Suggesting Changes​

Use GitLab's suggestion feature for simple changes:

The variable name could be more descriptive:

```diff
BigDecimal totalAmount = calculateTotal(items);
```

Author can accept suggestion with one click (creates commit).

Batch Comments​

Save up comments and publish all at once:

  1. Click "Start a review" instead of "Add comment now"
  2. Add all your comments
  3. Click "Finish review" when done
  4. Approve/Request Changes

Benefits:

  • Author gets one notification, not 20
  • You can review holistically before commenting
  • Faster review process

Review Checklist​

Add checklist to review comment:

## Review Checklist

- [x] Logic is correct
- [x] Tests are comprehensive
- [x] Security considerations addressed
- [ ] Performance could be improved (see comment on line 145)
- [x] Documentation updated

Overall: Approve with minor suggestion

Assigning Tasks​

Create GitLab tasks from review comments:

**Task**: Add input validation

/todo Add validation for negative amounts
/assign @author
/due tomorrow

Common Anti-Patterns​

Bike-shedding​

Problem: Debating trivial details while missing big issues

BAD:  50 comments about variable names
GOOD:  5 comments about critical logic bugs

Solution: Use /label ~nitpick for trivial items, focus on substance

Rubber Stamping​

Problem: Approving without actually reviewing

BAD:  "LGTM!" (3 seconds after MR created, 500 lines changed)

Solution: Take time to actually understand the changes

Design Debates in MR​

Problem: Major architectural discussions in MR comments

Solution: If discussion exceeds 3 back-and-forths, schedule a call

This is getting complex. Let's discuss synchronously.
Creating Zoom link: [link]
Will document decision in MR after call.

Late Reviews​

Problem: Reviews that take days

Solution: Block calendar time for reviews, treat as priority

Personal Attacks​

Problem: Feedback that attacks the person, not the code

 "You obviously don't know how to write Java"
BAD:  "This is lazy coding"

GOOD:  "This approach might cause issues. Consider..."
GOOD:  "I think there's a simpler way to achieve this..."

Review Metrics​

Track these in GitLab Analytics:

Health Metrics:

  • Time to first review: Target <4 hours
  • Time to merge: Target <2 days
  • Review cycles: Target <3 iterations
  • Approval rate: Should be >80%

Red Flags:

  • MRs open >7 days: Blocked or stale

  • 5 review cycles: Unclear requirements

  • <50% approval rate from reviewer: Misalignment
  • Review comments only on formatting/linting: Pipeline issues

Further Reading​

Summary​

Key Takeaways:

  1. Automated Checks: Let pipelines handle formatting, linting, tests
  2. Manual Focus: Review logic, design, security, performance, maintainability
  3. Approval Requirements: 1 approval for features, 2 for releases
  4. Productive Comments: Be specific, suggest solutions, ask questions
  5. Comment Types: Task (blocking), Comment (discussion), Nitpick (non-blocking), Praise
  6. Thread Resolution: All threads must be resolved before merge
  7. Timely Reviews: Target <4 hours for first review
  8. GitLab Features: Use suggestions, batch comments, task creation
  9. Constructive Feedback: Frame feedback positively, focus on code not person
  10. Security Critical: Always check for logged sensitive data in business-critical code