我发现 我公司的 review code 这个目的已经变质了。

2020-11-14 17:52:29 +08:00
 kpppp

我呆过几家公司,都是千人以上的规模。但是 review code 给我的统一感觉就是 [官僚] 。

明显的语法错误项目组 leader 也看不出来,只是看一眼,这些有+2 权限的人,大多数都没有做过你的项目,只是级别高。在入库这一关,卡死你,每次都要发消息,告知对方,请帮我 review !

比如我们最近的一个政策: 领导说:为了提高我们代码质量,所以我们要一起 review 。就是让写 c 或 c++的人和写 java 或 kotlin 的人互相 review code 。
实行了 2 天了,普通工程师说的最多的几句话:
1.看不懂,不知道你写的啥。
2.效率好低啊,没啥鸟用。

高级与专家工程师说的最多的几句话:
1.你这个代码有点冗余啊
2.这个没有问题吧
3.ok ,我马上+1

首席与经理说的话:
1.ok
2.已经+2

于是我直接把整个项目组的人都加入了 review,然后引起了 leader 的不满,说我太自我了.于是我又一个一个的 remove reviewer 。

哎,难啊。

10999 次点击
所在节点    程序员
54 条回复
wuzhouhui
2020-11-14 18:13:25 +08:00
我们的问题的积极性不高, 因为评审代码会耽误自己写代码的时间, 自己代码写不完, 领导就会责怪. 而在安排开发时间的时候, 是不会考虑到评审的耗时. 另外, 有些人水平太差, 改了几次都有问题, 然后就不想看了. 另外就是, 公司定的评审工具效率实在太低了. 所以现在, 通常是领导和对相关代码比较熟悉的员工会负责评审代码. 而水平差一点的, 我也不想让他们看, 他们提的意见实在没价值.
wuzhouhui
2020-11-14 18:16:24 +08:00
还有一个, 那就是公司的编码风格太讨厌了, 非常的啰嗦, 冗余, git 日志只写一句话甚至不写. 我比较喜欢内核那样简洁清晰的编码风格, 和详尽的提交日志.
stupil
2020-11-14 18:18:23 +08:00
有人搞一个代码流程图, 模块怎么划分,接口怎么调用,类怎么嵌套, 一目了然。

当然这样搞的话,想喷的人更方便了。
Solace202
2020-11-14 18:19:52 +08:00
那么有没有哪位有比较好的 code review 的最佳实践呢?话说我四年多了两家公司都没 review 过。。。二线城市
stoneabc
2020-11-14 18:27:18 +08:00
语法错误和代码规范不应该在门禁就卡回去么。。
fewok
2020-11-14 19:18:52 +08:00
我觉得吧,代码质量不是靠 code review 来控制的,应该先搞方案设计,把关调用流程(正向、逆向),尤其是异常处理,有没有把场景考虑全,某些场景该不该实现,能不能换个方式实现,有没有错误处理等等,尤其是状态机的流转和控制,谁驱动谁,能不能驱动等。
至于语法错误这些,你司搞个单元测试,静态代码扫描不就可以了(语法都错误了,说明这都没运行起来过)
southwolf
2020-11-14 19:19:23 +08:00
语法错误? 这些不应该直接被 lint/CI 卡掉么?
lxk11153
2020-11-14 19:20:26 +08:00
233 就是这样的 [doge]
Cbdy
2020-11-14 19:21:45 +08:00
层级太多了吧,code review 可能更适合管理扁平的工作组织吧,比如志愿者合作开发开源软件
dustinth
2020-11-14 19:25:58 +08:00
语法错误和代码规范都可以通过工具自动拦住; 还有更多的比如是否方法过长, 嵌套层次太多这样 common pratice 也可以通过工具实现; 真正到了业务逻辑层面, 如果不是有配套的 pair programming 实践, code review 一个标准基本不太现实;

是否使用严格的 code review, 取决于代码改动的频率和次数: 业务变化快速频繁的领域或者代码价值不太高但是又不可或缺的领域, code review 可以适当放松限制; 对于成熟的业务价值大的领域, 可以采用严格的 code review, 这样才能带来更高的收益.
huifer
2020-11-14 19:37:33 +08:00
sonarqube 各个语言制定规则, 或者每个项目指定一个规则. 先过静态检查吧. 对于注释, 方法名称这些得需要人力成本, 或者公司内部存在一个词汇表. 针对一个项目或者整个企业内的, 通过统一词汇表来提高可读性. 对于方法名称, 类名称 同样适用.
VDimos
2020-11-14 19:55:18 +08:00
语法错误怎么还能到 CR 这一步
impl
2020-11-14 21:01:02 +08:00
<amp-youtube data-videoid="rR4n-0KYeKQ" layout="responsive" width="480" height="270"></amp-youtube>
cdlixucd
2020-11-14 21:12:29 +08:00
@impl lol
laminux29
2020-11-14 21:16:49 +08:00
Code Review 的本质是让大佬带萌新。这里有几个问题:

1.让大佬带萌新,浪费了大佬的时间,对整个公司价值来说,值得吗?而且大佬会认为自己被招来干这事,属于自己的岗位职责吗?

2.萌新缺乏基础理论,大佬教萌新,萌新能听懂吗?教学效率高吗?

3.要提高萌新水平,还不如让萌新带薪脱产去学校学习。
illusionist
2020-11-14 21:20:30 +08:00
代码 review 正常来讲,对多人合作开发项目是必要的,但是很多时候会出现有+2 权限的人看不懂代码,看懂代码的人没有加+2 权限。而且很容易走入为了 review 而 review, 流于形式。没啥好的解决方案,只能说管理决策层要头脑清醒,知道为啥 review,很多时候 review 是一种权利运动,而不是真正为了提高软件质量。
GreyYang
2020-11-14 21:24:17 +08:00
LGTM!
DoctorCat
2020-11-14 21:27:35 +08:00
常态。社会倦怠问题解决不来。我觉得 CodeReview 的测重点应该放在师徒传授阶段和别人侵犯(动)相关责任人的代码时,彼此有个技术交流,尽力降低技术风险和债务。
zion03
2020-11-14 22:22:05 +08:00
lecher
2020-11-14 22:33:23 +08:00
带着业务理解 review,我司要求 code review 的目的是为了避免单点,在代码实现的思路上让团队达成一致,至少相关业务开发人员达成一致,review 之后意味着这个模块出了问题,我也可以负责修。这样出了任何问题,review 过的人都可以直接修改代码,而不是推脱这个模块不是我写的,我不需要为此负责。
所以在我司,code review 没有把重点放在语法之类的实现细节上,重点是在代码有没有体现合理的设计思路,代码可读性上。
而且 code review 已经是最后一步了,功夫应该在 code review 之外。

概要设计文档,细节设计文档,都需要有配套的评审,将技术债务尽可能减少。设计文档的评审通常是老带新,老手写概要设计框住业务边界,新手写详细设计明确实现细节,由评审来保证相关人员对业务需求的拆解理解达到一致。

此外就是配套的 CI lint test 等支持,规范代码风格,保证功能满足需求。

实际 review 的时候,会快速看看 test 的覆盖率快速确认对外暴露的调用约定是否合理,边界样例是否覆盖业务需求的要求。其次是看代码结构,可读性如何,再次才是语法细节,有没有不合理的写法,对于不合理的写法通常只有新人的前几个 PR 高标准评审,甚至可能有一个新人三四个老人评审的情况。

这么带几轮下来,大部分团队成员的代码风格和业务拆解都接近一致,基本上出问题都可以让一个人一杆子捅到底层实现去定位和修复。如果发现是代码分层和架构满足不了新增需求可能还有重构的情况。

享受到的收益是:
test 是以业务边界为要求写的,不用担心重构的影响范围过大。
底层模块业务稳定,容易交接,新人上手阅读代码和改代码的成本低。

这么做的问题是,功能迭代速度太慢,一个业务需求从设计到实现拖的周期会很长。还会有不少时间放在重构偿还技术债上。
这种模式只适合核心业务,对于开发周期急迫的任务并不太适合。

这是一个专为移动设备优化的页面(即为了让你能够在 Google 搜索结果里秒开这个页面),如果你希望参与 V2EX 社区的讨论,你可以继续到 V2EX 上打开本讨论主题的完整版本。

https://www.v2ex.com/t/725245

V2EX 是创意工作者们的社区,是一个分享自己正在做的有趣事物、交流想法,可以遇见新朋友甚至新机会的地方。

V2EX is a community of developers, designers and creative people.

© 2021 V2EX