今天我们来聊聊 code review，解构一下作为一个业务开发者怎样 review 代码才是最有效的。相信大家都或多或少帮同事 review 代码，这个度的把握其实是门学问，需要我们深入思考。这一点并不容易。

哪里出了问题

在目前的环境下，我们通常迭代一个需求的流程是这样：

需求评审 => 技术方案设计 => 开发 => 自测 => 联调 => 测试 => 上线

code review 通常是在【自测】节点结束后，【测试】接入前这个阶段进行的，甚至有些时候会跟【测试】并行（这样很容易造成漏测，毕竟 QA 验证的代码很可能又被改动掉）。

到了这个阶段，坦率讲代码不可能大变了。因为需求开发已经完成，测试还等着验证，只差这临门一脚就要合码上线。这个时候如果你提出来一大堆问题，会有什么后果呢？

• 如果只是代码风格或者很简单的逻辑处理，ok，没问题，可以改。

• 如果是很严重的设计问题，定义问题，这个阶段 code review 即便指出来，也会造成很大问题，因为开发者需要消化你的意见，思考怎么改，再调整结构，很可能是比较大的修改，对整体项目的工期都会产生影响。

而一旦遇到第二种 case，常见的后续是两种：

1. 虽然提出来，但不强制修改，只要没太大问题也就给 approve 了，设计上的坑以后再填。毕竟 reviewer 上下文不如 coder 多，有些事情只能靠 coder 个人的实力和素养来兜底，即使你要求改，coder 本人如果水平不在一个层次，一方面不容易理解，另一方面很难做到想要的效果；

2. 强制修改，否则不 approve 代码。这个时候就会造成 coder 要理解 reviewer 的意图，重新拆解，两个人对齐理想的设计，重新开发，工期延后。

两种说句实在话，都挺无奈。但没办法，这就是深入业务逻辑 review 出来问题时大概率会发生的情况。

问题来了，这件事情大家都知道，所以很多工程师为了避免【冲突】，避免因为 review 而去 delay 工期，有时候也是图省事，选择了简单的 review 路径：代码风格。

这里是不是多了个空行，那里注释是不是多了个空格，这里打日志可以加点信息，那里 return 可以简化一下。。。

事实上如果你想，Golang 提供了一整个 code review comments 让你来找代码风格问题。

但这样对么？

你不能说错，因为代码风格也很重要，但 reviewer 的心智如果都放到这些事情上，对于真正重要的，上线后难以再修改的逻辑，就势必会减少关注。这才是真正要命的地方。

code review 金字塔

近期看到有位博主梳理出来了一个 code review 金字塔，还蛮有意思的，这里跟大家分享一下:

• 越往塔尖走，日后修改的成本越小；越往塔基走，修改的成本越大。

• tests 和 code style 尽量交给自动化流程，不要花过多精力，将 code review 的时间尽量集中在 api, implemetation, doc 上，因为这些一旦第一版上了后续很难改，变动成本高。

我们在 code review 的时候尤其要注意这一点，坦率的讲，拿到一份代码，我们总是能有无数角度指出问题，但 reviewer 的精力有限，一定要关注重点！

什么是重点？就是代码一旦上了线，以后就不好再改的部分！

测试，以后可以再补（其实不太好，但目前的环境下，大家要求普遍降低），不影响功能即可。

代码风格，下次可以再调，本来想统一就不是一件容易的事。

但是，但是！

你的接口一旦定了，以后想改就不容易了，要连带着多端一起改。甚至如果是 open api 的话，外部很多人依赖，你怎么推动它们一起切换？

你的文档一旦定了，不管内部有重构，还是别的什么原因，再想推别人看新的文档，也不容易了。因为对外的文档反复更新是大忌，代表着质量的不稳定。

你的实现底层框架如果定了，也不好改了。因为这就是你的业务架构，主板没定义好，以后就只能在屎山上堆屎。什么？你说可以重构？不好意思，这几乎意味着完全重新开发，由此带来的巨大人力成本，以及功能稳定风险，值得么？

所以，code review 一定要把重心放在【以后不好再变】的事情上，而不是【代码风格】这种随时都能改的东西。这并不是说它不重要，而是要用好，用活各种 linter 工具，让自动化来代替你检查问题，这样 coder 在写代码的时候也可以时时验证，早点修复，而不是把一切都堆在 code review 环节。

review 的关注点

ok，现在我们明确了，review 时重点是在【不容易变更】的事情上。事实上接口定义和文档也应该更多地在【技术评审】阶段就确定，这样可以有更多的输入。

每个 reviewer 在看代码时都要问自己这三个问题：

1. 这段代码的目的是什么？

2. 如果我来写，会是什么样？

3. 二者的差异在哪儿，怎样是更好的方案。

带着这种心态，下面提供 8 个比较关键的思考点，这里大家仅供参考。对这些问题思考全面之后，review 就很简单了。

1. 如何支撑 feature

这一点建议每个 reviewer 都好好想想。很多时候代码一上来，一大堆细节，我们就忘了为了什么而出发。review 最关键的地方是要拎出来：

1. 这次 feature 需要什么能力；

2. 这份代码如何来提供这种能力，本质是什么。

尤其是第二点，一定要做到用一句话就能概括，非常简洁就能说出来。

是加了缓存？把某个计算结果存储下来？还是新增了某个 RPC 调用？还是用了某个机制？

明白了这一点再去 review 才能心中有数。

2. 主板能否扩展

在目前需求高速迭代的背景下，需求基本不会是上了就不再改。所以【主板】的设计一定要经过反复思考，review。一个纯粹面向过程的，不可扩展的逻辑，即便写的再优雅，从长期看也是毒瘤。

思考一下，现有的代码的变更空间有多大，如果有需求要对这里迭代，是否会影响一大片都要重写？

是否目前的实现非常特化，以后来个变更就要多一堆 if else ？以后的修改成本有多大？

3. 定义是否合理

新增的类型，接口，结构体等定义是否足够严谨，是否在此前的定义中是否已经出现过。是否做到了 Single Source Of Truth。如无必要，误增新概念。

是否有冗余字段？是否有不应该属于这一级关心的概念出现？是否有更好的定义方式？

是否对原有定义，引入了新的语义，新的功能概念，是否会影响别的模块？

4. 命名是否准确

合理的命名可以让 reviewer 快速 get 到代码的用意，也方便后续的维护。如果发现命名困难，很有可能设计上已经出了问题，不妨问自己：我取这个名字的目的是什么？有助于从 Problem-Solving 状态切换到 Big Picture。

那怎么样算是命名准确呢？

1. 言行一致

不要出现命名和实际干的事情不搭边的情况，或者范围明明很小，但是命名非常宏大，抽象。看到名称就应该能够判断出来要干什么事。

1. 注意语法

动词，形容词，单复数，一定要准确，否则很容易让人走向误区。一个可能让人误解的注释/定义，还不如不存在。

1. 不怕长

长很多时候不是问题，不准确才是问题，如果的确发现定义过长，很大概率是你的封装没做好，业务没拆分好。我们的重点不是【长】或【短】，而是【精准】。

如果一个地方要描述清楚场景的确需要较长的描述，then be it.

显式的代码更容易理解，即使有 Bug 也容易发现，隐式的代码可能是一些很聪明的代码，可以正常 work，但可能给其他人带来阅读困难，出 bug 时也更不容易排查。

5. 异常状况是否处理

是否存在强弱依赖，怎样处理的？会不会明明是某个弱依赖，但是轻易地碰到 err != nil 就直接接口报错了？

如果参数有误，或数据有误，会造成什么后果，有没有对应的机制来处理，是否会给以后埋坑？

边缘场景是否也能支持，对应什么表现？

6. 性能是否过关

抛开用户量，请求量谈性能是空谈。一定要结合现状和未来可能的增长，思考现有方案，对于长期来说是否够用。无论是内存，CPU 还是某个存储组件。必要的计算是一定需要的。

可以不用特别细，但至少要做到可支撑未来一定范围内业务的增长。

7. 排查修复能否做到

重要日志是否打印，打点上报是否完整。

• 如果出现了问题，我们能否通过监控，报警感知到；

• 即便无法感知到，遇到用户反馈我们最终了解到了，这套代码能够支持我们快速判断发生了什么；

• 能否快速恢复功能；

• 对于异常，出问题的 case，是否需要修数据，怎样弥补用户体验？

8. 安全隐私是否可靠

这一点需要结合具体业务来看，但一定要有意识，不该上报的是否上报，不该打印的是否打印，关键数据是否脱敏。这些一旦没做好，随后势必要花大量精力来弥补。

总结

其实开篇提到的问题大家肯定或多或少都会遇到，坦率的讲在 code review 阶段暴露问题，势必会影响整体进度。所以最好的方式一定是，把设计的思路，接口定义，尽可能在【技术方案 review】的环节就暴露出来，及时调整。到 code review 阶段只是细节的 check。

把你的精力更多集中在业务，而不是代码风格上，对 8 个关键问题思考全面，才能提出来最有帮助的 review 意见。当然，实际 review 过程中远不止这 8 个需要考虑，比如单测的有效性也是一个很重要的地方，但需要看团队规范和要求。大家灵活把握。