Why Code Reviews Matter: Benefits, Pitfalls, and Practical Guidelines

References

"How Big Tech Companies Play Code Review" – Liang Songhua, JD Senior Engineer

"Learning How Facebook Really Leverages Code Review for Efficiency" – Ge Jun, Former Facebook Internal Tools Team Tech Lead

"Which Code Review Method Fits My Team" – Ge Jun, Former Facebook Internal Tools Team Tech Lead

"Talking About Code Review" – Xiong Yi (Four Fire), Oracle Chief Software Engineer

"Six Common Problems in Code Review" – Songhua Pi Danme, InfoQ

"Code Review: Hopes and Sorrows" – Hu Feng, JD Chengdu Research Institute Technical Expert

Why Code Review?

Consequences of Not Reviewing

Code quality reflects product quality; unstable products with frequent bugs damage customer satisfaction and reputation. Poor code also inflates future maintenance costs, especially when shortcuts are taken and personnel changes occur, leading to endless technical debt that can range from localized anomalies to complete rewrites.

From a physics perspective, entropy shows that without external force, systems tend toward disorder, making standards and reviews invaluable. From a software design view, long‑term evolution demands clean code; otherwise, accumulated rot cannot support growth.

Benefits of Review

Code review is a low‑investment, high‑output activity that yields personal skill growth and team knowledge sharing. Its six main benefits are:

Early bug and design issue detection – the earlier a problem is found, the cheaper it is to fix.

Improved individual engineering ability – peer feedback naturally raises code quality.

Team knowledge sharing – reviewed code becomes team code, and discussion records serve as reference documentation.

Targeted quality improvement – specialized reviews (security, performance, UI) can be conducted by experts.

Unified coding style – often automated by tools.

Social function – knowing a colleague will review your code changes your coding mindset.

Challenges and Controversies

Most disputes do not deny the benefits of code review; they focus on the reasons or excuses for skipping it.

1) Time‑consuming effort

Tight deadlines and overtime make developers reluctant to spend time on reviews, especially when the perceived effort is limited to formatting, comments, or naming.

2) Perceived impact on team building

Conflicts can arise when developers disagree during reviews.

3) Subjective preferences

Reviewers may impose personal style preferences on others.

4) Unbalanced team composition

Too many inexperienced members and too few senior engineers can make cross‑review ineffective.

5) Resistance to review Some confident developers feel a review is a personal attack. 6) Formalism and poor effectiveness When reviews become a checkbox exercise, they lose real value. Understanding Review Formats Manual inspection is called a review, while automated checks are called code analysis or static checking. Common formats include: Machine check Manual review Pure online review Offline screen‑sharing review (recommended) Offline screen‑sharing reviews, though traditional, often provide higher ROI than purely online processes. Review Targets The following types of changes should be reviewed; larger changes usually need multiple reviewers, while minor tweaks may only need a peer check. Review Participants Anyone closely related to the business flow should attend, such as: Developer's mentor Colleague responsible for the next step in the workflow Involving these roles ensures timely information sync and prevents hidden dependencies. What to Focus On During Review Common mistake: reviewers impose personal opinions. Goal: maximize team benefit. Focus points: Obvious logical errors Compliance with coding standards Readability and maintainability Adherence to basic design patterns Should not over‑focus on: Whether the code runs Whether it meets business requirements Coverage of business scenarios These aspects are primarily the responsibility of the code author and testers. Review Process Efficient developers need a predictable schedule. The six‑step process is: Agree on specification documents to avoid subjective bias. Set a review schedule (e.g., every Monday) based on logical change size and release timeline. Provide supplementary material: background, code scope, impact range. Conduct the code review; decide if a second review is needed based on severity. Complete essential elements (monitoring, comments, etc.); reject if any are missing. Refine coding‑standard agreements and periodically revisit past reviews. Key Operations Improve commit atomicity: each commit should represent a coherent, small piece of functionality rather than half a feature or a week's worth of work. Improve commit message quality: avoid vague messages like "BUG", "FIX", or "UPDATE". Follow a three‑part format—title, description, and test situation. <code>publi Article GetArticleById(long id) { return null; } publi MaterialPO GetMaterialById(long id) { return null; } publi ArticleVO GetArticleByIdWithoutStatus(long id) { return null; }</code> Example of improving commit messages and validation: <code>//错误示范 Boolean SaveError(List<ShareDetail> list) { //只判断非空，无法保证detail非空 if(list.IsEmpty()) { return false; } //无法保证list中的id一致 Int id = list.get(0).getId(); //保存数据 return Save(id, list) } //正确示范 Boolean SaveRight(Int id, List<ShareDetail> list) { //深度判断非空情况 if(id == null || CollectionUtil.hasNull(list)) { return false; } //id从参数中获取，避免二义性 return Save(id, list) }</code> Variable‑scope example: <code>public static void main(Sstring[] args){ ShareDetail shareDetail1 = new ShareDetail(); //错误示例：对象在多个方法间传递 varErrorStep1(shareDetail1); //正常示例 ShareDetail shareDetail2 = varRight(); } public static varErrorStep1(ShareDetail shareDetail){ //其他复杂操作 shareDetail.setId(111); varErrorStep2(shareDetail); } public static void varErrorStep2(ShareDetail shareDetail){ //其他复制操作 shareDetail.setName("hello"); } public static ShareDetail varRight(){ ShareDetail shareDetail = new ShareDetail(); shareDetail.setId(111); shareDetail.setName("hello"); }</code> Stage‑result example: <code>//错误示范 public void SetpError(){ Boolean flag = false; List<Integer> list = new ArrayList<>(); for(Integer detail : list){ if(detail > 0){ flag = true; } } if(flag){ return; } } //正确示范 public void SetpRight(){ //步骤一：校验参数 List<Integer> list = new ArrayList<>(); for(Integer detail : list){ if(detail > 0){ return; //直接中断，去掉多余标记 } } //步骤二： //步骤三： }</code> Logging example: <code>//错误示范 public Boolean logError(Integer id, List<ShareDetail> list){ if(id == null || CollectionUtil.hasNull(list)){ return false; } logger.info("logError run"); try{ return shareDao.save(id, list); }catch(Exception e){ //不记录异常上下文 logger.error("save error"); //只记录有限上下文 logger.error("save error e:{0}", e.getMessage()); } return false; } //正确示范 public Boolean logError(Integer id, List<ShareDetail> list){ if(id == null || CollectionUtil.hasNull(list)){ return false; } //info级别在非线上环境很容易 logger.info("logError run"); try{ return shareDao.save(id, list); }catch(Exception e){ //充分记录错误上下文 logger.error("save error"); //规避日志导致的内存OOM logger.error("save error id:{},e:{},list:{},id,list==null?list:JSONObject.toJSON(list),e); } return false; }</code> Conclusion To make a team truly embrace code review, the practice must be seen as indispensable to daily development. Quality reviews require clear formats, defined objects, appropriate participants, focused attention, a solid process, and awareness of common pitfalls. When done right, reviews benefit the whole team and, if possible, should also involve testers to broaden coverage. Source: https://www.cnblogs.com/jackyfei/p/13299877.html