Code Review，你可以做得更好 -

Talk Is Cheap,Show Me The Code，Code Review是阻止代码堆成屎山的胡杨林，我们都是护林员。

前言

codereview的重要性
codereview为什么很多公司、团队都不愿意做。
如何codereview

Code Review的重要性

这块不多赘述了，代码质量提升、知识共享、早期发现问题、代码一致性等等等等，都是极为重要的，很多资料都有如谈谈codereview 。
但是很多事情我们知道重要，但是却又不做，这些原因是什么呢？

为什么不愿意做CR，又该如何解决呢？

    我想说下我自己的经历，14年开始进入腾讯的时候是做Android客户端的，项目成员较少，当时要和另一款竞品做时间上的厮杀，抢占市场。
    leader也非技术出身，我记得那时候代码仓库都是SVN，每次新开一条分支，代码合入工作量都很大。也没有工蜂，所以根本就没有CR的概念。  
    后面历经了一些变故，到了17年，我调到另外一个部门，成为了后端开发，负责一个新的平台的研发，新leader研发出身比较重视技术，此时腾讯慢慢的也重视代码管理倡导内部开源，工蜂平台也内部使用，甚至还专门培训了git的使用流程以及PR相关介绍，也规定每次提交，对，是每次提交都必须要经过reviewer的approve。后面甚至在kpi中都有一条就是给同事进行codereview。  
    上一家公司ONES，一个做企业流程管理软件的公司，做SAAS的，也非常重视自身的代码提交流程。负责的迭代提测，有一项是必须要codereview通过了才可以，否则测试人员有权拒绝测试。

我分享的这些，大概有如下原因不愿意做codereview：

1.人员不足，业务逼的太紧，以结果为导向

首当其冲的就是这个理由，项目进度非常紧张，团队需要尽快交付代码->提测->上线，因此没有足够的时间来执行codereview，导致代码审查被减少，甚至忽视。
解决方案：

项目这么紧张，这么多的需求，你是否想过，这些需求都是合理的吗？我在ONES的时候，需求需要遵循剃刀原理，“如无必要，勿增实体”，这些需求影响范围多大？边际成本多大？需求是否合理？所以产品经理在进行需求产出的时候，需要一定量的数据作为支撑，不是拍脑袋进行决定，压缩codereview的时间来追赶项目进度，只会导致测试和线上的bug增加。
积极和管理层进行沟通，如果没有代码审查机制，bug率会升高，且一些代码问题测试验证不出来容易导致线上问题，如空指针等等。强调review是为了代码质量，而不是批评个人。
鼓励开发程序员参与，经常性的秀代码，也是提高自身影响力的手段
最低可以接受在周会中，安排一个小时集中进行review，如果一周连这一个小时都抽取不出来的话，这个团队莫不是007?

恰恰是人员不足，才需要去组内成员交叉去了解业务，万一离职了也好顶上呀，哈哈。

2.团队文化问题

技术氛围大部分跟team leader有关，如果leader是技术或者测试出身，可能codereview的环境是必须要有的，如果是产品出身可能不会那么的重视技术，觉得codereview就是浪费开发时间，估计大概率是不会有codereview环节了，不过可以慢慢的向上灌输理论，卷起来：

解释codereview的好处，且明确目标，定义清晰的codereview准则，确保有方向性。
拉出bug率，并且和有codereview的团队进行对比，相信有这个步骤的团队的bug率更高。
与leader一起制定代码规范、代码提交流程、codereview指南、定时总结优化，说服leader并给出一定的激励机制。
以下是网图(一个团队建立codereview前后发现问题的变化，足见建立codereview的重要性)

3.工具以及流程问题

这个问题好解决，网上一大堆，很多工具都可以帮助团队更轻松进行review、记录结果、跟踪问题，如：

GitHub 和 GitLab：这些代码托管平台提供内置的代码审查功能，允许团队成员在合并请求（Pull Request）中进行审查和讨论。
腾讯工蜂：首先，每一个发送到版本库中的提交，均能针对代码行发起评论，可以用来指出对代码的疑问和不足。其次，在合并请求发起后，可在合并请求上附加评审流程，使分支的管理者能够控制必须经过评审的提交才能合入到分支中去。
Bitbucket: 与GitHub和GitLab类似，Bitbucket也提供内置的代码审查功能，适用于Git和Mercurial存储库。
至于流程，那就更简单了，不过需要符合你们团队的常规流程，无非也就是：

图片来源：How to Do Code Reviews Like a Human
code_1

4.缺乏培训和指导

这就涉及到如何进行codereview的环节了，我下一章节会讲到。但codereview绝不是为了挑战对方，需要非暴力沟通，指出哪里不好的时候，最好是有理有据。
code_2

5.由谁来进行codereview？

那么问题来了，谁是reviewer呢？

技术leader or 技术骨干：他们对于各个模块的业务，应当是较为熟悉，而且技术能力也是毋庸置疑的，但他们的时间往往来说是比较宝贵的。
Owner机制：每个人负责一个或多个模块(服务)，当改动的代码涉及到相关人时，提给模块Owner进行review。
团队互相reviewer：技术leader分配开发任务的时候，需要指定相关的reviewer。开需求评审、技术评审的时候，相关reviewer需要到场参会，最好在技术评审的时候可以给出一定的建议。
团队review，约在某个午后(如周五下午)，整个团队针对性的进行review。
以下是网图

code_2

如何进行codereview

首先得记住，我们都是人，而且你review的代码的作者，也是一个人。在你写建议或者发现问题的时候，保持友善的态度。

计入开发工作量

很多团队不做codereview的原因，可能就是觉得这就是在耽搁开发时间，或者是reviewer随便看看直接点击通过，这些都是糊弄的做法。所以codereview应当要计入开发的工作量，且在个人周报上写明本周review的内容。在上家公司，需要你进行review的时候，会给你提一个任务单，你可以在这个任务单上面登记工时。

建立起review文档，整理出来最佳实践以及坏味道的实现。

技术leader至少应该读过《重构》、《代码整洁之道》、《设计模式》等等，里面我还推荐一个Effective系列，这些书籍里面就有很多坏味道实现以及最佳实践的案列，根据团队的风格，可以从中摘选一些，作为团队code的规范和准则，经常维护，甚至可以在周会上面增加项内容，就是本周的codereview的最佳实践和坏味道的实现的讨论，维护文档。当然要是结合起来，如：在很多书籍中，包括effective go中就指明了创建切片的方式如：

i := make([]int, 10, 100)
users :=make([]User,100)

这种你说不对吗？但是我们在实践中，是有一定隐患的，因为通常会是使用append的方式去添加，如果没有指定位数的话，是往后添加的，往往会造成在遍历的时候，前面都是零值。Uber的go编码指南提出，此时的最佳方式应该是，对于零值结构，使用var来进行初始化。哪怕是知道了切片的长度，也建议使用var来进行初始化，因为你根本不知道后面这块代码会不会动。

var users []User
var i []int

还有一些如不必要的else，如：

    if a {
        return 100
    } else {
        return 200
    }

此时else就完全没意义，且代码也会显得臃肿，直接改为：(我记得effective go中也有说到这个案例)

    if a {
        return 100
    } 
    return 200

当然，如上的这个例子，如果需要靠人来review的话，那效率就太低了，完全可以依靠工具，下面会讲到。

分阶段进行提交，reviewer需要了解相关业务

根据Cisco代码评审研究证明，reviewer的review量要低于一次200-400行代码，如果超过这个量，搜索缺陷的能力就会降低，以这个速度，您可以找到 70-90% 的缺陷。换句话说，如果存在 10 个缺陷，那么您可以找到其中的 7 到 9 个。且每小时需要review的代码量，需要低于500行。

千万不能在临近提测或者提测之后提交PR，那只会浪费更多更多职能同事的时间。

去除低效率的review，利用工具

reviewer应该把精力放在代码性能优化等地方，而不是一些基础的lint检查，如注释前是否要加上空格，方法名和包名一致等等鸡毛蒜皮的小事，这些都是可以通过工具，开发人员在发起review的时候就应该解决掉。

codereview不应该成为保证代码风格和编码标准的手段。
强烈建议使用工具，进行review，这样，reviewer可以更加针对于架构、流程进行针对性的review。

代码质量管家-使用golangci-lint提高Go项目质量

review礼节

如果你有更好的建议，请提出来。需要指出来为什么不好，如果那样是否会更好。大家都是同事，不要在codereview中说：“这写的是什么垃圾代码”，而应该表达说：“这个函数名字有点含糊，如果换成XXX应该更加符合上下文”，“这块代码使用频繁，建议抽出去封装公用的方法”等等。

不要在review中讨论业务，而应该是根据业务是review代码，如果对需求有问题，应该在需求沟通时就应该提出来，上面也有提到，reviewer应当熟悉本次review的业务逻辑。

不要吝啬你的赞美

codereview 本意是改善代码质量，增强团队和成员之间的沟通，并非是挑战。你在给人家进行review的时候，当看到比较新颖的思路，一些奇淫技巧，请给ta点赞，给ta鼓励。切身体会，现在我都还记得，在ONES的时候，我写了一段接收数据转换处理错误的代码，当时只是感觉这样设计会好点，但是得到了reviewer的点赞说这段处理很好。虽然只是简单的一句话，但人都是这样，喜欢被认可。所以建议每隔review，至少给出一条肯定的评价。

func BindJSON(c *gin.Context, obj interface{}) (err error) {
    defer func() {
        if p := recover(); p != nil {
            switch p := p.(type) {
            case error:
                err = p
            default:
                err = fmt.Errorf("%s", p)
            }
            renderBindJSONError(c, err)
        }
    }()
    if err = c.BindJSON(obj); err != nil {
        renderBindJSONError(c, err)
        return
    }

    return
}

Code Review，你可以做得更好

前言