How to do Code Review correctly - Yiming Chen

[utterances-bot]

How to do Code Review correctly - Yiming Chen

My current take on How to do code review correctly

https://yiming.dev/blog/2017/10/30/how-to-do-code-review-correctly/

[forrestchang]

业务代码做 Code Review 太困难了：

1. 你不熟悉别人的需求，所以 Code Review 仅仅停留在表层；
2. 一个业务需求可能涉及到几百上千行的改动，Reviewer 一一 review 的成本很大；
3. 很多业务代码用完就仍，基本不会写测试（会有专门的 QA 做集成测试），review 的效果不大。

非业务的代码还是值得做 Code Review 的。

[dsdshcym]

@forrestchang

我完全认同你说的 review 业务代码的三个难点。但也有不同的看法：

1. 不熟悉别人的需求，所以 Code Review 仅仅停留在表层

• 如果是同一团队内部 review 的话，大家对需求都会比较熟悉，这样的情况就会比较少出现。
• 如果是不同团队、不同项目 review 的话，其实我觉得效果更好。因为 code review 的目标之一就是增加代码的可读性。而高可读性的代码就意味着一个对项目陌生的人来看也能很容易看懂。所以让不熟悉项目的人来 review 在这方面还是有优势的。
  1. 我个人在实践时并没觉得需要理解别的项目的需求增加了 code review 的难度。 1) 只要 author 有链接到相关的 issue，或者写了适当的描述，review 就能顺利的进行。（也许是因为我们的项目都比较简单。） 2) 如果 author 的介绍没有做到位，我理解需求的过程也是帮他梳理的过程。往往在和 author 一起梳理了需求之后，我们就能发现一些可以改进的点。
  2. 即使停留在表层，改进一些命名，检查风格，或者提供一些不同的代码组织思路、API 使用，都算值得了。

2. 一个业务需求可能涉及到几百上千行的改动，Reviewer 一一 review 的成本很大；

但我觉得这是 author 的问题。reviewer 应该帮助 author 去做 commit 拆分，在下一次做类似改动的时候能让 commit/PR 变小。

甚至这可能是项目管理的问题。一个涉及到上千行改动的大功能，如果能事先拆分成几个只涉及到百行左右的小功能，分阶段开发、部署，尽快有可交付的成果，对整个项目开发也是有益的。

3. 很多业务代码用完就仍，基本不会写测试（会有专门的 QA 做集成测试），review 的效果不大。

如果确定是用完就扔，那只要能满足需求，代码写得再烂都行。但现实是，一开始觉得用完就扔的代码往往会被复用很多次，甚至一直使用。我们很难去界定什么样的业务代码是用完就扔的。

至于 review 的效果，我觉得只要 author 和 reviewer 能从中学到一些东西，下次能把「用完就扔」的代码写得更好，完成得更快，这次 review 就很值了。

另外，我觉得「集成测试」也应该由对应功能的开发者来负责。不过这个话题就扯远了，也跟公司、团队结构有关。