| title | date |
|---|---|
Code Review Best Practices |
2018-11-24T16:23:00.000Z |
原文链接 https://medium.com/palantir/code-review-best-practices-19e02780015f
- 作为 commit 的发起者，我们希望有人能看到我们的工作，希望自己所做的改进能够被别人知晓。同时也希望知道自己对相关问题的处理是否正确，是否有更好或者更合适的方式处理这个问题，我们希望 reviewer 能够给予我们一些建议，或者能让我们看到我们认识不到的问题。或者我们在代码中运用一些新学到的技巧，或者我们做了一些特别棒的抽象，希望其他维护者能够在后面的开发工作中使用到。总而言之，我们希望别人看到，因此我们发起 code review
- 不同项目可能会由不同的人来维护，可能会有不同的风格、规范，作为一个提交者，我们可能不清楚这些，需要有个人来做检查，不要让我们的代码成了巨型建筑里的一片坏瓦
- 你的代码可能能跑通，功能完全 ok，但是逻辑不够清晰，如果别人一看就觉得很懵逼，那不是好代码，那是维护噩梦。所以 code review 的前提是，你的代码是写给人看的，逻辑不够清晰简单的代码不应该被 accept
- 人无完人，经验再丰富的开发者，也难免在写代码的时候引入一些错误，甚至可能是拼写错误，code review 可以很大程度上先把这些问题拦住
- 很多公司对安全很重视，code review 可以防住一些错误代码引入的安全问题
所有人的代码都应该被 review，不应该因为工程师的级别就会有不同的对待。当然也应该看你的修改都做了什么，人都是活的，如果只是一点点小的修改，我觉得就没必要浪费别人的时间帮你做 review 了。至于什么样的修改没有必要，可能不同的团队、不同的项目会有不一样的标准吧，这个要自己去衡量了。
一般 code review 应该先经过自动化的 lint、test 等 CI 流程之后，才进行 review，如果连标准的流程都跑不过，又何必浪费时间做 review 呢？
作为 commit 的提交者，也要为 reviewer 考虑，毕竟人家是花上自己的时间和精力帮你做 review。
为此，我们应当把每个提交切割的尽量小，reviewer 不需要花太多时间就能审阅完。如果是个大的 feature，我们可以先对每个小 commit 提交 review，但是先不合到主分支，而是合到 feature 分支，待完成开发之后，所有代码也都 review 过了，再合到主分支。
但是这样的做法也经常会有问题，因为 feature 分支的存在，我们就要注意它跟主分支的差异情况，时间越久，两个分支的差异一定是越大的。我们最近新功能的开发中，尝试了另外一种方式 —— 特性开关。就是新的 feature 代码也合到主分支，但是我们可以通过一个开关来隔离这些新的逻辑，在开关打开之前，不应该有人能访问到他们。我觉得这样做挺好的，避免了一直维护主分支和 feature 分支的关系，维护关系本身也蛮好精力的。
正如前面所说，我们应当为 reviewer 着想，为他们节省时间，你的代码每次都一大坨，很难 review，慢慢就没人愿意帮你 review 了。
- commit message 应当精简，能直接表达出你做了什么。如果一句话不够，可以在下面的段落中详细的阐述。如果你的提交本身花了很长时间才完成，你最好把他分割成多个提交分别 review，因为读可能也要花差不多的时间
- 自己的代码应该自己对其质量进行保证
- 重构的代码不应该改变原来的行为，要改变原来的行为，不如新写
code review 是开发工作中的一环，你的 review 可能会阻碍别人的工作，所以要尽可能及时的完成 review。如果你没办法做到，请告知相关的人，以及相应的时间节点，好让对方做好准备。
你觉得不合适的地方一定要提出来，并且把原因解释清楚，这些信息都会应该留在 review 工具上，也方便后面复盘的时候能够有据可循。
作为一个 reviewer，你有责任维护代码的质量。
在 review 时，你应当关注以下几点：
- 首先你要搞清楚提交者的真实目的是什么，他的代码是否达到了这个目的。要不断的对这些代码提出疑问：是否有更好的办法达到目的？这个代码是否容易理解？是否会产生歧义？另一个不明白其中缘由的人，能否一眼看懂代码在做什么？
- 如果你来解决这个问题，你会怎么做？
- 这些代码能否抽象出来供以后多个地方使用？
- 像对手一样思考，想法设法找到提交者的漏洞
- 是否有现成的代码能够实现一样的效果，提交者是不是重复造轮子了
- 这些修改符合规范吗
- 这个提交会不会引入特殊的依赖？
- 代码中是否有 TODO ，这些 TODO 应该对应到具体的 issue 或者 jira 上，并给定一个解决期限，否则这些 TODO 可能会一直留存下去。
- test 也要看，一方面看测试用例是否全面。如果代码没有相应的 test 是一定不能合到主分支的。test 也是为了项目以后能被很好地维护
- 这个提交是否会影响兼容性，尤其是在多方依赖的情况下，一定要考虑清楚
- 这个提交是否需要继承测试？比如代码中依赖了外部系统，这个时候最好要再做一些集成测试
- 文档是否更新了？很多人只更新代码，不更新文档，导致文档越来越陈旧，各个依赖方怨声载道