什么才是Code Review的正确姿势？

NOTE: PR (pull request, a set of changes someone tries to push to a GitHub repository. Once a PR is sent, interested parties (peer engineers) can review the set of changes, discuss potential modifications, and even push follow-up commits if necessary.

在 stamp 前，通常是 github page 上可以给出各种 comments，page 是共享的，所以谁都能看到。有些 comments 是询问，那么代码作者直接回复或解释就行。有些 comments 是指出代码的问题，这样代码作者可能会依此改动；也可能不同意，那就回复 comments，有的时候关于一个实现细节，讨论的 comments thread 可以多达十几条。这样对于大家达成共识很有帮助。

以前遇到有朋友说：

“看到代码写的太烂，觉得来回扯皮效率不高，干脆扑上去自己写。”

曾经我也有过类似的想法。不过最近遇到的一些事让我慢慢地意识到这种想法很要不得。

首先就是从对方的角度来说。对方写不好，可能是对业务不熟悉，可能是对语言不熟悉，也可能是对公司代码的大架构不熟悉。如果你是帮他/她 “写” 而不是耐心指出哪里有问题，那么下一次，他/她可能还是不知道。不仅无益于别的人成长，有的时候甚至会让别人有挫败感。

再然后就是这种方式的可扩展性很差。即 使 code review 会花掉哪怕十倍于你自己写的时间和精力，但它会让人明白代码应该怎么写，从长远来看，这其实是在一定程度上 “复制” 你的生产力。你不能什么都自己写。尤其是你慢慢开始带项目、带新人。每天 review 五个人的代码，和写五个人的代码，你觉得长期而言哪个更合算？

此外，如果说写代码是一个学习过程，怎么做一个好的代码审核人更是一个学习和成长的过程。自己绕过一个坑不难，难的是看到别人那么走，远远地你就能告诉他/她那里有个坑。而他/她在经你指出多次后，下一次他/她也会帮着指出别人的类似的问题。

这 一点最近感触尤为深刻。前一阵组里咔咔咔接二连三来了好多新人，老大说，你少写点代码，多做点 review。于是那几天我几乎工作的一半时间都用来看代码，写 comments。可是最近就会发现，他/她们相互之间大部分时候已经可以很好的相互 review 了，于是我又腾出好一些时间来做别的工作。

Code review 做多了，发现其实也是有一定套路的，试着归纳如下：

• 代码格式方面。很 多公司都有 coding style guideline。大家的约定俗成，避免公司的代码风格不一致，也避免一些不不要的为了 “要不要把闭括号另起一行” 而无谓地争论，除非是不小心，通常大家都不会弄错。但是新员工往往会在这方面还不太熟悉。这一类问题也比较容易指出。
• 代码可读性方面。这 包括一个函数不要太长，太长就 break down。所有的变量名尽量能够说明它的用意和类型。比如 hosting_address_hash，一看就知道是房东地址，而且是个 hash 类型。不要有嵌套太多层的条件语句或者循环语句。不要有一个太长的 boolean 判断语句。如果一个函数，别人需要看你的长篇注释才能明白，那这个函数就一定有重构的空间。另外，如果不可避免有一些注释，则一定要保证注释准确且与代码完全一致。
• 帮代码作者想想他/她有没有漏掉任何 corner case。很多时候这是业务逻辑相关的，尤其需要比较老的工程师帮助指出需要处理的所有情况。
• Error handling。这 是最常见也是代码审核最容易帮别人看出的问题。举个例子，下面一段简单到不能再简单的代码就至少有三个潜在的问题：params 里面需要 validate 是不是有 user_id 和 new_name 这两个 key；能不能找到这个 user_id 对应的 user；save 的时候会不会有 DB level 的 exception，应该怎么处理。

• 测试例和防坑。测试例不用说了，每段代码都应该有测试例。但是对于一些你能预见如果别人改动代码会引起可能问题的情况，一定要额外的加测试例防止这种事情的发生。这一点没有例子参考也不太好说。怎么写好测试例，本身就值得写一篇文章了。
• 小架构。什么意思呢，就是一个文件或者类内部的代码组织。比如如果有重复的代码段，就应该提取出来公用。不要在代码里随意设常数，所有的常数都应该文件顶部统一定义。哪些应该是 private，等等
• 大架构。这个就更广了。包括文件组织，函数是不是应该抽象到 lib 或者 helper 文件里；是不是应该使用继承类；是不是和整个代码库的风格一致，等等。

当然，视具体情况可能还有很多别的需要在 code review 中注意的。但是如果按照这个 checklist 过一遍，一些明显的问题就可以避免个八九不离十了。

另外，代码 review 和写代码是我们每个工程师都应该致力的两个方面，缺一不可。可能在工作中的某个阶段或者某个项目里，你会花更多的时间在某一面，但长久看来，两个技能缺一不可。

代 码审核是工作，不要抱有情绪化。我曾经干过一件事，一个外组同事的代码，别人已经 stamp 了，可是我觉得有问题，于是把 stamp revert 掉了，在 comments 里写了为什么有问题，和建议的改法。当时心里还有点觉得好像怪得罪人的。可是后来发现同事反而很客气地接受并道谢了。心里也是很感激有这样的工作环境。

另外，经常会遇到有些 review 的 comments 里出现不同意见，其实是两个人在编程习惯和约定俗成上没有共识。如 果在不违背公司 style guideline 的情况下，没必要一定让对方和你有一样的习惯。如果你真的觉得这样更好，通常我也只会写 “I personally would prefer A over B, but no strong opinion.” 或者 “How about change it to A, since … However, this is not a merge blocker. ”

人 都有不周到的时候。多几双眼睛一起帮你看一遍，就可以很大程度地减少代码里的错误。另外，相互 review 的过程中还能从彼此那里学到很多编程的小技巧和好习惯。细想来，很多 Java 和 Ruby 的特别好用的 feature，我都是在 code review 的过程中学到的。

参考阅读：《写代码的四个境界》