How to do code / PR reviews | DevOps, CI/CD & Tooling

Definition

A good review verifies requirement, quality, correct business logic, error handling, performance, security, tests, and standards -- catching defects early and spreading knowledge.

Requirement check -- confirming the change does exactly what the ticket asked, nothing less and nothing more.
Quality check -- meaningful names, small functions, no duplication, readability for future maintainers.
Business logic check -- tracing the logic against realistic cases including boundaries and edge cases.
Exception handling -- specific exceptions, clean rollback, fail-closed behavior, no sensitive data in error messages.
Performance check -- watching for N+1 queries, unbounded data loading, and holding connections too long.
Security check -- parameterized queries, input validation, authorization, no hardcoded secrets.
Test check -- happy path, failure cases, boundaries, and meaningful assertions.

Think of a code review as the supervisor sign-off before a large transaction is posted at National Bank of India. Before money moves, a second officer checks that the form matches what the customer asked (requirement), is filled in neatly (quality), that the math is right (business logic), that it handles a frozen or overdrawn account (exception handling), that it will not jam the queue at month-end (performance), that nobody can tamper with it (security), that there is a receipt (tests), and that it follows standard procedure (standards).

Understanding the Concepts

How do you check the requirement?

Start by reading the ticket or user story, then the PR description, then the diff. Confirm the change does exactly what was asked, nothing less and nothing more. Scope creep -- sneaking in unrelated edits -- makes reviews and rollbacks harder even if the extra code is good.

JAVA

// Ticket NBI-456: Add daily transfer limit of 10 lakh for SAV accounts
// The PR should add ONLY the limit check, not refactor the whole service

package com.bank.payments;

import java.math.BigDecimal;

public class TransferService {

    private static final BigDecimal DAILY_LIMIT =
        new BigDecimal("1000000.00");

    public void transfer(String fromAccount, String toAccount,
                         BigDecimal amount) {
        if (amount.compareTo(DAILY_LIMIT) > 0) {
            throw new DailyLimitExceededException(
                "Transfer of " + amount + " exceeds daily limit");
        }
        // ... proceed with transfer
    }
}

How do you check business logic?

Trace the logic by hand against realistic cases. In banking, money must always use BigDecimal, never double. Look hard at boundary conditions -- zero amounts, exactly-at-the-limit values, negative inputs.

JAVA

package com.bank.payments;

import java.math.BigDecimal;

public class TransferValidator {

    // WRONG -- using double for money
    public boolean isValidAmount_WRONG(double amount) {
        return amount > 0 && amount <= 1000000.00;
        // 0.1 + 0.2 == 0.30000000000000004 in double!
    }

    // CORRECT -- using BigDecimal
    public boolean isValidAmount(BigDecimal amount) {
        return amount.compareTo(BigDecimal.ZERO) > 0
            && amount.compareTo(new BigDecimal("1000000.00")) <= 0;
    }
}

TEXT

Review comment: "amount > 0" -- what about exactly zero?
Review comment: "<= 1000000" -- the spec says 'strictly less than 10 lakh'
  or 'up to and including 10 lakh'? Please clarify with the ticket.

How do you check exception handling?

Exceptions should be specific, transactions should roll back cleanly on error, and error messages must not leak sensitive data. For a bank, failing closed (rejecting) is safer than failing open (allowing).

JAVA

package com.bank.payments;

import org.springframework.transaction.annotation.Transactional;
import java.math.BigDecimal;

public class TransferService {

    // WRONG -- catches everything, swallows the error, leaks account details
    @Transactional
    public void transfer_WRONG(String from, String to, BigDecimal amount) {
        try {
            // ... transfer logic
        } catch (Exception e) {
            System.out.println("Failed for account " + from
                + " balance " + getBalance(from)); // leaks balance!
        }
    }

    // CORRECT -- specific exception, clean rollback, no sensitive data
    @Transactional(rollbackFor = Exception.class)
    public void transfer(String from, String to, BigDecimal amount) {
        try {
            // ... transfer logic
        } catch (InsufficientFundsException e) {
            throw e; // let it propagate, transaction rolls back
        } catch (Exception e) {
            throw new TransferFailedException(
                "Transfer failed for request " + from + " -> " + to, e);
            // No balance or sensitive data in the message
        }
    }

    private BigDecimal getBalance(String accountId) {
        return BigDecimal.ZERO; // placeholder
    }
}

How do you check security?

Security is critical in banking. Check for parameterized queries, input validation, authorization so a user cannot act on another account, no hardcoded secrets, and sensitive data not logged.

JAVA

package com.bank.repository;

import com.bank.model.Account;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.Query;
import org.springframework.data.repository.query.Param;

public interface AccountRepository extends JpaRepository<Account, Long> {

    // WRONG -- string concatenation, SQL injection vulnerability
    // "SELECT * FROM accounts WHERE id = '" + accountId + "'"

    // CORRECT -- parameterized query
    @Query("SELECT a FROM Account a WHERE a.accountId = :accountId")
    Account findByAccountId(@Param("accountId") String accountId);
}

TEXT

Review comment (BLOCKER): Line 15 concatenates user input into SQL.
  This is a SQL injection vulnerability. Use @Query with :param binding.

How do you check performance?

Watch for N+1 queries, loading unbounded data into memory, and holding connections or locks too long. Do not demand premature optimization, but flag anything that will not scale to production volumes.

JAVA

package com.bank.service;

import com.bank.model.Account;
import com.bank.model.Transaction;
import java.util.List;

public class ReportService {

    // WRONG -- N+1: one query per account
    public void generateReport_WRONG(List<Account> accounts) {
        for (Account acct : accounts) {
            List<Transaction> txns = txnRepo.findByAccountId(acct.getId());
            // 1000 accounts = 1001 queries!
        }
    }

    // CORRECT -- batch query
    public void generateReport(List<String> accountIds) {
        List<Transaction> txns = txnRepo.findByAccountIdIn(accountIds);
        // 1 query regardless of account count
    }
}

How do you check tests?

Confirm there are tests for the new logic covering the happy path, important failure cases, and boundaries. Assertions should be meaningful. If a bug is being fixed, there should be a test that would have caught it.

JAVA

package com.bank.payments;

import org.junit.jupiter.api.Test;
import java.math.BigDecimal;
import static org.junit.jupiter.api.Assertions.*;

class TransferValidatorTest {

    private final TransferValidator validator = new TransferValidator();

    @Test
    void validAmount_withinLimit() {
        assertTrue(validator.isValidAmount(new BigDecimal("50000.00")));
    }

    @Test
    void validAmount_exactlyAtLimit() {
        assertTrue(validator.isValidAmount(new BigDecimal("1000000.00")));
    }

    @Test
    void invalidAmount_zero() {
        assertFalse(validator.isValidAmount(BigDecimal.ZERO));
    }

    @Test
    void invalidAmount_negative() {
        assertFalse(validator.isValidAmount(new BigDecimal("-100.00")));
    }

    @Test
    void invalidAmount_exceedsLimit() {
        assertFalse(validator.isValidAmount(new BigDecimal("1000001.00")));
    }
}

TEXT

Tests run: 5, Failures: 0, Errors: 0, Skipped: 0

How do you check standards?

Check formatting, linting, commit message conventions, documentation of public APIs, and consistency with team practices.

BASH

# Automated checks in CI
mvn checkstyle:check
mvn spotbugs:check
mvn sonar:sonar

TEXT

[INFO] Checkstyle: 0 violations
[INFO] SpotBugs: 0 bugs found
[INFO] SonarQube: Quality gate PASSED (coverage 89%)

What are common mistakes in reviewing?

The most common mistakes are skimming the diff without reading the ticket first, not checking boundary conditions, approving code that concatenates user input into SQL, and being unkind in comments.

TEXT

# WRONG review comment:
"This is terrible code, rewrite it all."

# CORRECT review comment:
"Suggestion: could we extract this into a method for readability?
 I think it would make the transfer flow clearer."

# WRONG: approving without checking boundaries
# CORRECT: "What happens if amount is exactly 0? The spec says 'positive'."

What should a senior engineer know?

A senior engineer phrases comments as questions and suggestions, distinguishes blocking issues from nitpicks (using labels like [nit] and [blocker]), praises good work, and reviews promptly so they never block teammates. The review is about the code, never the person, and a healthy review culture is one of the strongest quality tools a team has.

Visual Overview

DIAGRAM

  Code Review Checklist
  ─────────────────────

  ┌──────────────┐
  │ 1. Requirement│  Does the change match the ticket?
  └──────┬───────┘
         ▼
  ┌──────────────┐
  │ 2. Quality    │  Names, size, duplication, readability
  └──────┬───────┘
         ▼
  ┌──────────────┐
  │ 3. Business   │  BigDecimal for money, boundaries,
  │    Logic      │  zero/negative/limit edge cases
  └──────┬───────┘
         ▼
  ┌──────────────┐
  │ 4. Exception  │  Specific catch, rollback, fail-closed,
  │    Handling   │  no sensitive data in errors
  └──────┬───────┘
         ▼
  ┌──────────────┐
  │ 5. Performance│  N+1 queries, unbounded data,
  │              │  connection/lock holding
  └──────┬───────┘
         ▼
  ┌──────────────┐
  │ 6. Security   │  Parameterized SQL, input validation,
  │              │  authorization, no secrets in code
  └──────┬───────┘
         ▼
  ┌──────────────┐
  │ 7. Tests      │  Happy path, failures, boundaries,
  │              │  meaningful assertions
  └──────┬───────┘
         ▼
  ┌──────────────┐
  │ 8. Standards  │  Formatting, linting, commit messages,
  │              │  documentation
  └──────────────┘

Key Takeaway

A thorough code review works through a consistent set of dimensions. Start with the requirement to confirm the change matches the ticket. Check quality and maintainability -- meaningful names, small functions, no duplication. Trace the business logic against realistic cases, especially boundary conditions, and confirm money uses BigDecimal. Check that exceptions are specific, transactions roll back cleanly, and error messages do not leak sensitive data.

Performance and security are never optional in banking. Watch for N+1 queries, unbounded data loading, and holding connections too long. Check for parameterized queries, proper authorization, and no hardcoded secrets. Confirm tests cover the happy path, failure cases, and boundaries with meaningful assertions.

The human side matters as much as the technical. Phrase comments as questions, separate blockers from nitpicks, praise good work, and review promptly so you never block the team. A healthy review culture is one of the strongest quality tools a team has.

Remember one sentence: a good review checks requirement, logic, security, and tests in that order, phrasing every comment as a helpful question rather than a criticism.

---