原文链接 https://medium.com/palantir/code-review-best-practices-19e02780015f

为什么要做 code review ？

1. 作为 commit 的发起者，我们希望有人能看到我们的工作，希望自己所做的改进能够被别人知晓。同时也希望知道自己对相关问题的处理是否正确，是否有更好或者更合适的方式处理这个问题，我们希望 reviewer 能够给予我们一些建议，或者能让我们看到我们认识不到的问题。或者我们在代码中运用一些新学到的技巧，或者我们做了一些特别棒的抽象，希望其他维护者能够在后面的开发工作中使用到。总而言之，我们希望别人看到，因此我们发起 code review
2. 不同项目可能会由不同的人来维护，可能会有不同的风格、规范，作为一个提交者，我们可能不清楚这些，需要有个人来做检查，不要让我们的代码成了巨型建筑里的一片坏瓦
3. 你的代码可能能跑通，功能完全 ok，但是逻辑不够清晰，如果别人一看就觉得很懵逼，那不是好代码，那是维护噩梦。所以 code review 的前提是，你的代码是写给人看的，逻辑不够清晰简单的代码不应该被 accept
4. 人无完人，经验再丰富的开发者，也难免在写代码的时候引入一些错误，甚至可能是拼写错误，code review 可以很大程度上先把这些问题拦住
5. 很多公司对安全很重视，code review 可以防住一些错误代码引入的安全问题

所有人的代码都应该被 review，不应该因为工程师的级别就会有不同的对待。当然也应该看你的修改都做了什么，人都是活的，如果只是一点点小的修改，我觉得就没必要浪费别人的时间帮你做 review 了。至于什么样的修改没有必要，可能不同的团队、不同的项目会有不一样的标准吧，这个要自己去衡量了。

何时进行 review ?

一般 code review 应该先经过自动化的 lint、test 等 CI 流程之后，才进行 review，如果连标准的流程都跑不过，又何必浪费时间做 review 呢？

作为 commit 的提交者，也要为 reviewer 考虑，毕竟人家是花上自己的时间和精力帮你做 review。

为此，我们应当把每个提交切割的尽量小，reviewer 不需要花太多时间就能审阅完。如果是个大的 feature，我们可以先对每个小 commit 提交 review，但是先不合到主分支，而是合到 feature 分支，待完成开发之后，所有代码也都 review 过了，再合到主分支。

但是这样的做法也经常会有问题，因为 feature 分支的存在，我们就要注意它跟主分支的差异情况，时间越久，两个分支的差异一定是越大的。我们最近新功能的开发中，尝试了另外一种方式 —— 特性开关。就是新的 feature 代码也合到主分支，但是我们可以通过一个开关来隔离这些新的逻辑，在开关打开之前，不应该有人能访问到他们。我觉得这样做挺好的，避免了一直维护主分支和 feature 分支的关系，维护关系本身也蛮好精力的。

在 review 之前把代码准备好

正如前面所说，我们应当为 reviewer 着想，为他们节省时间，你的代码每次都一大坨，很难 review，慢慢就没人愿意帮你 review 了。

1. commit message 应当精简，能直接表达出你做了什么。如果一句话不够，可以在下面的段落中详细的阐述。如果你的提交本身花了很长时间才完成，你最好把他分割成多个提交分别 review，因为读可能也要花差不多的时间
2. 自己的代码应该自己对其质量进行保证

3. 重构的代码不应该改变原来的行为，要改变原来的行为，不如新写

   怎样做 code review

   code review 是开发工作中的一环，你的 review 可能会阻碍别人的工作，所以要尽可能及时的完成 review。如果你没办法做到，请告知相关的人，以及相应的时间节点，好让对方做好准备。

你觉得不合适的地方一定要提出来，并且把原因解释清楚，这些信息都会应该留在 review 工具上，也方便后面复盘的时候能够有据可循。

作为一个 reviewer，你有责任维护代码的质量。

在 review 时，你应当关注以下几点：

1. 首先你要搞清楚提交者的真实目的是什么，他的代码是否达到了这个目的。要不断的对这些代码提出疑问：是否有更好的办法达到目的？这个代码是否容易理解？是否会产生歧义？另一个不明白其中缘由的人，能否一眼看懂代码在做什么？
2. 如果你来解决这个问题，你会怎么做？
3. 这些代码能否抽象出来供以后多个地方使用？
4. 像对手一样思考，想法设法找到提交者的漏洞
5. 是否有现成的代码能够实现一样的效果，提交者是不是重复造轮子了
6. 这些修改符合规范吗
7. 这个提交会不会引入特殊的依赖？
8. 代码中是否有 TODO ，这些 TODO 应该对应到具体的 issue 或者 jira 上，并给定一个解决期限，否则这些 TODO 可能会一直留存下去。
9. test 也要看，一方面看测试用例是否全面。如果代码没有相应的 test 是一定不能合到主分支的。test 也是为了项目以后能被很好地维护
10. 这个提交是否会影响兼容性，尤其是在多方依赖的情况下，一定要考虑清楚
11. 这个提交是否需要继承测试？比如代码中依赖了外部系统，这个时候最好要再做一些集成测试
12. 文档是否更新了？很多人只更新代码，不更新文档，导致文档越来越陈旧，各个依赖方怨声载道

痕迹
没有过去，就没法认定现在的自己