Why Code Reviews Fail and How to Master Effective CR Practices

Glossary

CR: Code Review

CL: Change List – a self‑contained change submitted for review

LGTM: Looks Good to Me – approval comment

Why Codebases Become Hard to Maintain

Incomplete or superficial code reviews are a major cause. Common symptoms:

Only fixing immediate bugs, ignoring long‑term impact

Treating code review as non‑functional work

Prioritising short‑term gains over maintainability

Underestimating the compound benefits of good reviews

Significance of Modern Code Review

Effective, best‑practice code review reduces risk, improves maintainability, speeds development, and raises team skill – it is a craftsmanship mindset.

CR Principles

Approve a CL that improves overall code health even if it is not perfect.

Communicate based on technical facts and data, not personal preference.

Resolve conflicts without deadlock.

Leverage tools (linters, company standards, AI assistance).

Preparing to Open a CR

Create a personal checklist.

Review your own code as if you were the reviewer.

Identify potential problem areas.

Ensure the code runs and passes self‑tests.

Don’t rely on others to find issues.

Automated Pre‑Check Checklist

Unit test coverage

New unit tests added

Method length limits

Cyclomatic complexity limits

Code style compliance

Lint checks

Size monitoring

Suggested average review time: ≤10 minutes. Heavy e2e or performance checks should not block the MR flow.

Reasonable Review Scale

~200 LOC per review (max 400 LOC)

Review speed ≤500 LOC/hour

Review session ≤60 minutes

All team members participate; allocate 0.5–1 day per week for CR.

How to Split a CL

Follow Google’s small‑CL guide: https://google.github.io/eng-practices/review/developer/small-cls.html

Commit Message Guidelines

Bad Example

Fix bug

Vague – what bug? What was changed?

Good Example

Summary: [module‑X] Add new feature Y
Background: New requirement from product team (see ticket #123)
Details: Implemented Y using Z pattern, added unit tests, updated docs.

Mindset

A CR is a conversation.

Expect comments and respond promptly.

Be ready for your code to have defects.

The goal is to discover problems, not to resist feedback.

What to Review (Top‑Down Priority)

Architecture and responsibility separation

Encapsulation

Non‑functional aspects (performance, security)

Maintainability and readability

CLEAN Code Traits

Cohesive : easy to understand and locate bugs.

Loosely Coupled : minimal side effects, easier testing and reuse.

Encapsulated : manages complexity, easier to modify.

Assertive : behaviour and data stay together, no hidden dependencies.

Non‑redundant : single source of truth for fixes.

Avoid Over‑Design

Code should be clear enough for others to recognise the pattern and continue maintenance.

Complexity Guidance

Prefer standard‑library capabilities.

Encapsulate details.

Simpler code means fewer bugs.

Follow the Single‑Responsibility Principle.

Apply DRY – Don’t Repeat Yourself.

Useless Function Wrapper

export const isDisabledByAdmitStatus = discountListItem => {
  if (!discountListItem?.bankInfo?.admitStatus) {
    return true;
  } else {
    return false;
  }
}

Replace with a direct boolean expression, e.g.

export const isDisabledByAdmitStatus = item => !item?.bankInfo?.admitStatus;

Prefer Standard Time Libraries

export const getQuarterStartAndEndTime = ({
  time = null,
  isTimestamp = true,
  split = '/',
  startDateTime = ' 00:00:00',
  endDateTime = ' 23:59:59'
} = {}) => {
  let date = checkDate(time) ? new Date(time) : new Date();
  let year = date.getFullYear();
  let month = date.getMonth() + 1;
  let startTime = null;
  let endTime = null;
  if (month <= 3) {
    startTime = `${year}${split}01${split}01${startDateTime}`;
    endTime = `${year}${split}03${split}31${endDateTime}`;
  } else if (month > 3 && month <= 6) {
    startTime = `${year}${split}04${split}01${startDateTime}`;
    endTime = `${year}${split}06${split}30${endDateTime}`;
  } else if (month > 6 && month <= 9) {
    startTime = `${year}${split}07${split}01${startDateTime}`;
    endTime = `${year}${split}09${split}30${endDateTime}`;
  } else {
    startTime = `${year}${split}10${split}01${startDateTime}`;
    endTime = `${year}${split}12${split}31${endDateTime}`;
  }
  startTime = isTimestamp ? getTimestamp(startTime) : startTime;
  endTime = isTimestamp ? getTimestamp(endTime) : endTime;
  return { startTime, endTime };
}

Using a library such as dayjs or moment can simplify this logic.

Security

Avoid storing sensitive data (API keys, secrets) in source code.

const WX_APP_ID = 'xxxxxxxxxx';
const WX_APP_SECRET = 'xxxxxxxxxxxxxxxxxxxxx';
const PJJ_APP_ID = 'xxxxxxxxxx';
const PJJ_APP_SECRET = 'xxxxxxxxxxxxxxxxxxxxx';

Consistency

Function names match implementations.

Comments align with code.

Naming style is uniform.

Async patterns (Promise, async/await, callbacks) are consistent.

Abstraction levels are consistent.

Do not mix import and require.

Naming Conventions

File names: kebab-case Class names: PascalCase Variables/functions: camelCase Private members: optional leading underscore, but consistent.

Import Ordering Example

Before (unordered, duplicate imports):

import { ref } from 'vue';
import Taro from '@tarojs/taro';
import gwApi from '@/api/index-gateway-js';
import api from '@/api/index-js';
import { onMounted, reactive, watch } from 'vue';
import InputRight from '../components/InputRight.vue';
import { isweapp, getParams } from '@/utils';
import { FACTORY_ADDRESS_ORIGIN } from '@/utils/const.js';
import { sgmReportCustom } from '@/utils/log';
import { genAddressStr } from '@/utils/address';

After (sorted, deduplicated):

import Taro from '@tarojs/taro';
import { onMounted, reactive, ref, watch } from 'vue';
import api from '@/api/index.js';
import gwApi from '@/api/index-gateway-js';
import { getParams, isWeapp } from '@/utils';
import { FACTORY_ADDRESS_ORIGIN } from '@/utils/const.js';
import { sgmReportCustom } from '@/utils/log';
import InputRight from '../components/InputRight.vue';

Reviewer Recommendations

Refactor duplicated code and give meaningful names.

Name each FormItem clearly and simplify ternary logic.

Complexity Assessment

Use cyclomatic and cognitive complexity metrics. A simple switch may have low cyclomatic complexity (e.g., 4) but higher cognitive complexity when nested ternaries are present.

Other Guidelines

Avoid magic strings and repeated literals.

Remove unnecessary empty lines.

Limit over‑use of large UI components (e.g., FormItem).

Common Challenges and Solutions

Code Review Shows No Issues

Organise collective review sessions to raise overall review skill.

Fear of Conflict

Resolve conflicts via leader mediation, negotiation, or third‑party evaluation.

Use diplomatic language and constructive suggestions.

Lack of Time for CR

Distinguish truly urgent work. Allocate ~20 % of development time for review. Keep milestones small (≈400 LOC per week) to avoid large CLs.

Historical Debt

Adopt a “fix the increment, clean the stock” approach: each merge should improve repository health.

Team Engineering Literacy Boost

Design patterns: master 24 patterns.

SOLID principles.

Methodologies: DevOps, XP, Scrum, Kanban, Waterfall, structured analysis/design.

Practices: TDD, OOP, functional programming, CI, pair programming.

TL;DR

Code Review Quick‑Start Handbook – keep CLs ≤400 LOC, review ≤10 minutes, enforce a checklist, write clear commit messages, follow CLEAN traits, use consistent naming and import ordering, and treat review as a collaborative conversation.

References

https://composity.com/post/too-busy-to-improve

https://commadot.com/wtf-per-minute/

https://dl.acm.org/doi/10.1145/3585004#d1e372

https://google.github.io/eng-practices/review/reviewer/standard.html

https://book.douban.com/subject/35513153/

https://zhuanlan.zhihu.com/p/549019453