Code Review礼仪-肌肉的碰撞

3,963 阅读9分钟

一、序言: 原来程序员是这么合作的

在我工作的前四年, 经常会在网上看到Code Review这个词. 但是却从来实践过.

看着他们对于Code Review习以为常的样子, 我宛如活在了平行世界. 不知道他们在说什么, 虽然每一个字都认识, 自然也没有什么感触.

直到第五年,加入了现在的公司,才真正的知道Code Review是什么一回事. 或者说只是知道一冰山一角,毕竟到现在只有不到一年的Code Review的经验. 但是给我带来的感触还是很深的.

向开源项目提交PR, 也有Code Review? 不好意思, 在此之前,从来不看别的源码,面向百度编程罢了

这篇文章, 内容包括如下图内容。

  • 介绍什么是Code Review
  • 我们组现在的Code Review流程
  • 接着到Code Review礼仪
  • 最后加上一个问题, 小公司是否需要Code Review。
    image.png

首先,对于最后一个问题,我想来还是有一定的发言权的。 在工作的前四年换了六家公司, 去一家倒闭一家. 并不是我想要跳槽,只是普本的学历、菜狗的技术, 很多人看不起的外包我都面不过. 再加上运气着实有点问题。

所以在这些小公司协作的时候, 相当原始。 svn代码管理, 本地项目打包出dist文件夹, 压缩之后通过QQ传给后台上传。 即便到了负责APP的时候, 也是本地手动打包出.app发送到测试手机上面.。

即便后来用了git,也和svn的使用没有任何差别, 不存在什么代码审查, 仓库权限的配置. PR和 MR是啥意思都分不清楚. CI和CD是啥?

image.png
我的本科虽然是计算机科学与技术,但是除了学习,啥都干。

直到来到了现在的这家公司, 才知道,原来程序员是这么协作,具体的可以以后谈谈。

二、什么是Code Review

Code Review就是将自己的代码公开的给其他程序员审查的过程。
Reviewer和Reviewee之间主要是通过comment来进行沟通,comment可以针对每一行来进行评论。

  • 熟悉同事在编程时的思考方式,这样其余同事以后如果有需要就可以更轻松、快速的修改代码。
  • 向同事介绍修改了哪些文件,增加了什么样的功能,这样在问题出现时,可以保证至少有两个人可以帮助诊断、解决问题。

2.1 评论规范

comment也有规范,如下

1. [request] 我建议这里可以更改为函数forEach用户,这个更加的复合规范

2. [advise] 这里的判断为了应对之后的扩展,可以考虑使用策略模式

3. [question] 为什么需要多加一个变量来进行判断

如下图的实际操作情况,在对应的代码下面点击评论:

image.png

通过[]包住的标识含义还是很直接的。

  • [request] 必须要修改才能够通过
  • [advise] 不修改也可以通过
  • [question] 需要reviewee解释一下

2.2 CR是在R什么

目的是提高代码质量
提前发现bug外,还包括统一团队的代码规范,比如经常会碰到有人说你这个变量命名不对
代码风格通过配置Eslint来进行统一,命名规则,语法检查,分支规范等是需要提前团队规范来解决的。这些都不应该放在Code Review上面。
image.png
所以实际上,在CM之前是有前置条件的:

在这里之后才开始CM。CM的内容基本包括下面:

  • 业务BUG
  • 代码质量

业务BUG很直接,很多时候,写的东西不一定用在一个场合,而写的人不一定都熟悉。
代码质量包括这些内容:

  • 人工检查代码格式化的漏网之鱼
  • 是否存在重复
  • 最佳实践,if else、参数过多、ES6、设计模式(怼人的理论基础是《重构-改善既有代码》)
  • 命名可读性,能自我阐述的最好,英文用词尽量准确(命名是所有程序员的头痛之一)
  • 注释,恰到好处的注释,重要的地方及时备注,不需要的地方要删除一定的注释信息,代码既是给机器运行的,也同时是交接给人看的.
  • 代码是否健壮,是否可扩展

我们的目标是消灭下面这张图:
image.png

三、我们组的Code Review流程

我们组是每一次Merge Request都进行Code Review。

还有一种是每隔一段时间集中大家一起来进行CR。

基本的流程图如下:
image.png
简单来说,就是从test分支拉取最新的代码之后,开发完成之后需要合并到远程自己定义的分支,随后将这个分支申请合并到远程test分支上面,通过CI自动扫描之后,就出发代码审查阶段了

  • name_需求编码指的是, 远程的分支的名称 = 是以自己的名字的首字母 + 下划线 + 需求的ID。
  • test分支在经过测试之后, 才会被允许合并到releasel分支, 这个过程是封版。将该分支发布部署之后,就可以合并到master分支。当然,这个涉及到了分支策略的问题,以后再谈

我们组最多的时候十个人, 评审的人员除了申请人, 其他人都是评审员. 每一个PR只需要两个评审的通过就可以入库。当有未必解决状态的评论的时候,其他评审人一般情况是不能审批通过的。

这样的评审的机制其实已经相当宽松了 ,也是希望所有人都能够参与到评审当中,相互都能够了解到对方模块的情况。

但是现实中情况还是不太一样,由于人员之间的水平是有差距的,我们的人员搭配是两个内部的搭配7/8个外包的伙伴。虽然平时开发已经再三说明可以给任何人审查代码,不过由于整个环境的问题,实际上敢点击通过代码的依旧只有内部的两个人。形成了事实上的两人评审。

四、Code Review礼仪

Code Review的礼仪决定了你是否会被同事背后捅刀。主要是谨记一条原则:
只针对代码不针对人

4.1 总结

对评审人员的建议:

  • 看不明白的时候,可以适当的提醒对方添加注释
  • 看不明白的时候,是请教对方而不是责问对方
  • 不管是明确修改的方案还是指出问题,给出自己的理由,依据,而不是感觉
  • 评论的内容不要过于广泛

作为reviewer提出comment,目的是提升项目代码质量,而不是抨击别人,质疑别人的能力,应该保持平等友善的语气。

评论的内容广泛的问题,比如说,评论Reviewee的耦合度太高。这么的言论就很操蛋,我们需要提出的是具体的建议,可以给出具体的改进的伪代码。不然最好闭嘴。

对提审人的建议:

  • 每次提交的代码量都尽可能的小
  • 反驳一定需要给出的依据
  • 相比评审人要更加的谦虚

第一点,为了方便审阅者可以轻松了解代码中有哪些更改以及做了什么。如果 code review 的内容足够少,则可以更频繁地进行,可能每天几次,而且更易于管理。

对于第二点,评审的人花了这么多时间给你来看代码。如果提出的建议能够改善你写的代码,或者直接指出了问题,提审人都是最大的受益者。

4.2 话术的建议

badgood
你要这么做我建议这么做/这么干是否更加好
你写的代码比较差这块代码写得比较差
你这里写得太坑了重构里面提议这种情况应该提取函数

最重要的一点就是,不要吝啬你的夸奖!
当你实在找不到问题的时候,多夸夸你的同事吧。这段代码设计得真好,这段代码性能提升得真高。当然,平时做人的时候也应该如此。

五、小公司是否需要Code Review

不需要!
对于公司而言,业务第一,产品第二,技术无所属。领导只关心如何活下去。所以对于你如何使用技术并不关心,关心的是业务是否来钱,自己还有几套房可以抵押。
你们进行CR是需要时间,这是钱。
你们想要提高代码质量,需要持续重构优化,客户操作界面的时候感受得出来么?如果没有,那是浪费钱。
无关褒贬,客观事实罢了。

image.png
但是对于个人而言呢?需要,相当需要。

都不小了,要明白自己应该做什么。

如果团队的Code Review有没有推行没关系,有,自然是非常的。没有的话,你也要通过别的手段自我提升。

但凡有好的事物都应该主动去尝试,去坚持,克服外来因素影响。

一个人,也同样可以Review自己和同事代码,为的不是公司,为的是自己。

Review水平比自己高的,可以直接提升自己的编码能力,学习高水平的思路和设计。

Review水平比自己低的,可以看看那些错误都是怎么发生的,告诫自己。

Review水平和自己差不多的,可以换位思考,如果是自己来实现会怎么实现。

卷起来吧,

image.png
放错图,应该是

image.png

本文正在参加「金石计划 . 瓜分6万现金大奖」