Mastering Code Review: Proven Practices to Boost Code Quality and Team Collaboration

As part of an excellent engineering culture, Code Review (CR) is continuously practiced, though each team adjusts its strictness to fit its context.

Why Do Code Review

Early defect detection – logical errors, business misunderstandings, and performance risks are caught during CR.

Improve code quality – robustness, design rationality, and elegance are enhanced.

Unify standards and style – consistent coding conventions improve readability for future maintainers.

Prevent architecture decay – everyone participates in preserving architectural health.

Knowledge sharing – each review is a learning opportunity for both author and reviewer.

Team consensus – repeated discussions build shared understanding of design principles.

Industry Practices

Company A

Code must be approved by at least two reviewers before check‑in.

Review principle : if a change improves overall code quality, it can be merged even if not perfect.

Review guidelines : prioritize objective technical factors over personal preference; follow the coding style guide; adhere to basic design principles.

What reviewers should look at : design, functionality, complexity, tests, naming, comments, code style, documentation.

Company B

Dedicated CR phase: e.g., 2 days of review for a 5‑day development sprint.

Quantitative metrics: reviews per KLOC, non‑trivial issues per KLOC.

Company C

Code Owner mechanism – every change must be approved by the owner.

“Show me the code” principle for all engineers.

Transparent review participation – any team member can review any change.

Performance is tied to CR output in evaluations.

How to Conduct CR

As a Submitter

Start the review early; keep changes small.

Keep the title concise (<400 lines of code per review is ideal) and provide a clear description.

Use IDE tools (e.g., IDEA plugin) to enforce style rules.

As a Reviewer

Complete the review quickly; avoid chasing perfection.

Ensure comments are actionable and resolved.

Avoid rhetorical questions; focus on facts.

Common Pitfalls

Lack of buy‑in – reviewers’ attitude affects team acceptance.

Large changes – big diffs reduce defect discovery rate and slow reviews.

Insufficient reviewer time – allocate time in advance; aim for <500 lines/hour.

Reviewer lacks domain knowledge – authors should write clear titles/descriptions; reviewers may need to read the PRD.

Unaddressed review suggestions – ensure follow‑up reviews for modified code.

Typical Code Issues Discovered in CR

DRY (Don’t Repeat Yourself)

Duplicated code appears in multiple places; refactor using Extract Method, Pull Up Method, or Extract Class. Example of bad code:

private BillVO convertBillDTO2BillVO(BillDTO billDTO) {
    if (billDTO == null) {
        return null;
    }
    BillVO billVO = new BillVO();
    Money cost = billDTO.getCost();
    if (cost != null && cost.getAmount() != null) {
        billVO.setCostDisplayText(String.format("%s %s", cost.getCurrency(), cost.getAmount()));
    }
    // similar blocks for sale and grossProfit
    return billVO;
}

Refactored version:

private static final String MONEY_DISPLAY_TEXT_PATTERN = "%s %s";

private BillVO convertBillDTO2BillVO(BillDTO billDTO) {
    if (billDTO == null) {
        return null;
    }
    BillVO billVO = new BillVO();
    billVO.setCostDisplayText(buildMoneyDisplayText(billDTO.getCost()));
    billVO.setSaleDisplayText(buildMoneyDisplayText(billDTO.getSale()));
    billVO.setGrossProfitDisplayText(buildMoneyDisplayText(billDTO.getGrossProfit()));
    return billVO;
}

private String buildMoneyDisplayText(Money money) {
    if (money == null || money.getAmount() == null) {
        return StringUtils.EMPTY;
    }
    return String.format(MONEY_DISPLAY_TEXT_PATTERN, money.getCurrency(), money.getAmount().toPlainString());
}

Primitive Obsession

Replace raw primitive fields with small domain objects (e.g., Money, Range, Email) to improve semantics and reuse.

Distributed Lock Misuse

Incorrect lock handling leads to data inconsistency. Bad example:

boolean lockSuccess = lockService.tryLock(LockBizType.ORDER, orderId);
if (!lockSuccess) {
    // TODO: retry or throw
    return;
}
// do something
} finally {
    lockService.unlock(LockBizType.ORDER, orderId);
}

Better approach – encapsulate lock‑acquire/release:

Boolean processSuccess = lockService.executeWithLock(LockBizType.ORDER, orderId, () -> doProcess(orderId));

Also ensure lock type matches lock key; mismatched keys render the lock ineffective.

Pagination Issues

Missing pagination or overly large page sizes cause severe DB performance problems. Bad example (no pagination):

private List<OrderDTO> queryOrderList(Long customerId) {
    // ... fetch all rows ...
}

Correct example with proper page size checks:

private Page<OrderDTO> queryOrderList(OrderPageQuery query) {
    Preconditions.checkNotNull(query, "查询条件不能为空");
    Preconditions.checkArgument(query.getPageSize() <= MAX_PAGE_SIZE, "分页size不能大于" + MAX_PAGE_SIZE);
    long cnt = orderMapper.count(query);
    if (cnt == 0) {
        return PageQueryUtil.buildPageData(query, null, cnt);
    }
    List<OrderDO> orderDOList = orderMapper.list(query);
    List<OrderDTO> orderDTOList = orderConverter.doList2dtoList(orderDOList);
    return PageQueryUtil.buildPageData(query, orderDTOList, cnt);
}

Conclusion

Code Review is a powerful practice that surfaces defects early, spreads knowledge, enforces standards, and keeps architecture healthy. By following the guidelines and avoiding common pitfalls illustrated above, teams can continuously refine their codebase toward higher quality and maintainability.

Author: Fang Jicheng (Runfu) – Source: Alibaba Middleware