1
x66 2021-01-04 10:03:59 +08:00 2
我司有这个流程,每次提交代码都会有两位同事和一位 team leader review 。
但是很多时候也会沦为一种形式,因为同事写的业务你并不一定都清楚,也只能帮忙看看语法有没有明显的问题。 有些同事一次改动的代码过多,很难 review, 更多的时候你自己思路来了正在 coding,同事来找你 review,很难放下自己手里的工作去仔细看他的代码 |
2
ferock 2021-01-04 10:14:24 +08:00 1
楼上+1
参考鹅厂,有 code review,svn 递交必须审批,回顾问题落实到责任制。 那可能沦为形式化会弱一点。 制度和人之间,关键作用的还是人。 |
3
Mirage09 2021-01-04 10:17:36 +08:00 via iPhone
看情况吧,小的 CR 走个形式,大的 CR 还是比较严肃的
|
4
NexTooo 2021-01-04 10:17:37 +08:00 6
因为安排了 code review 的任务,也不会给你 review 的时间……
|
5
duduaba 2021-01-04 10:19:18 +08:00
code review 最大的阻塞就是很大的 commit,如果各个功能改动细分 commit,那 review 就很容易了。
|
6
jzmws 2021-01-04 10:26:45 +08:00
小公司根本没办法做到 CR ,有时候看看别人的代码最多看看代码是否符合规范,注释啥子是否描述正确,其他的真的不好再弄
|
7
jdhao 2021-01-04 10:29:07 +08:00
能跑就行。
|
8
gggxxxx 2021-01-04 10:30:24 +08:00 2
code review 只在 beta 时期有用。软件大部分都完工了,这个时候任何的代码修改,大家一起开会 review 看看,看看会不会因为个人修改代码考虑太少造成其他部分额外的风险。
我一直很不认同互相 review 代码风格啊,具体实现啊。毫无意义的事情。一方面,别人看你代码可能根本看不懂,每个人负责的模块和业务都不一样,草草看看也不实际测试,然后提交一个已阅标记........给自己一种感觉仿佛好像自己是美帝互联网大厂一员..... 另一方面代码风格其实根本不重要,只要能正确运行的代码就是好代码。我曾见过真正美帝互联网巨头公司的员工代码,每个人都是风格各异性格突出,有些人就喜欢 tab 空 2 个 space 呢..... |
9
xuboying 2021-01-04 10:37:39 +08:00
@gggxxxx #8 代码风格不是项目一开始就定好了么,机器检查,项目风格都定不下来不是技术负责人压不住小弟么。要么就是没有负责人。。。
|
10
chiu 2021-01-04 10:37:44 +08:00 1
merge 代码的流程就有 review (自己 review/verify -> 机器人 review+机器人 build+机器人 test -> leader/owner review), 以上整个流程通过后, change 才会自动 merge 到对应 branch 上
|
11
saulshao 2021-01-04 10:40:33 +08:00 3
代码 review 的核心目标其实是尝试让其他人理解作者的代码。
挑刺永远都比做事容易,如果各位理解这点,就会明白 code review 其实是非常重要的,应该要切实执行。 |
12
gggxxxx 2021-01-04 10:51:01 +08:00
@xuboying 为啥老是开口就是权力话语这套说辞呢....压不住小弟....喷了。
一个团队里的成员有自己的个性其实是好事,在不重要的事情上做各种约束其实意义不大。想想看,团队里每个人都穿格子衬衫开发效率就会提高么? |
14
Lemeng 2021-01-04 10:55:18 +08:00
没有绝对的,重视程度有大不同
|
15
ericcode 2021-01-04 10:55:25 +08:00
来这家公司 3 年了,一直坚持做 review,很有用,但是真的很费时间
|
16
b1ackjack 2021-01-04 10:56:43 +08:00
之前小公司 sonar 和人工 cr 都有,来了这二线啥都没了
|
17
hantsy 2021-01-04 10:58:45 +08:00
@chiu 这个流程差不多可以。
我现在只要参与的项目 100%需要 CR 。主要创建的 PR 在进行。 进行 Code Review 之前,必须保证(这些都是由 CI 完成): 1,所有代码通过测试 2,测试 Coverage 报告(指标认可, 测试不到位,添加测试) 3,代码质量检测工具报告达标(如果引入新的 Bad Smell 代码等,需要重构清除)(现在智能化的检测工具,借助大数据,通过海量分析,可以检测很多代码质量的问题,及预测运行时产生的动态问题,不仅仅是命名规范等的问题) 这之后才是人工 Code Review,加入一些修正意见 4,Code Review,进行重构 还是重申我的观点,不写测试,做 CR,CI 都会成为形式主义。 |
18
tkl 2021-01-04 11:07:40 +08:00
1L + 10L
sonarqube |
19
channingcheng 2021-01-04 11:19:59 +08:00
@saulshao 说的太对了, 遇到一个总挑别人的人,什么都要按照他的想法来实现,是时候真不想搭理这人
|
20
channingcheng 2021-01-04 11:21:36 +08:00
@hantsy 有个问题,cr 后重构,那测试不就要重新测了?确认改那么多东西没有问题吗?
|
21
timedivision 2021-01-04 11:25:41 +08:00 1
@gggxxxx 代码风格不定好,拉代码合并的时候,你 2 个 space,他一个 tab,你咋合并?难道自己不用格式化代码的吗
|
22
Orenoid 2021-01-04 11:33:54 +08:00
我司是有 CR 的,小型创业公司。
虽然谈不上形式化,但这个流程目前基本上已经变成,以 “应该按我的想法来实现” 为目的了。反正我现在也懒得争论了,对方怎么说我就怎么改……说到底还是话语权的问题,总该有个人让步。 |
23
taowen 2021-01-04 11:45:31 +08:00
code review 只是一种给反馈的手段而已。如果能尽早提出改进意见,没必要等到 code review 的时候来做。cr 时候提一堆意见去返工,浪费也挺大的。
|
24
weer0026 2021-01-04 11:47:41 +08:00
小公司表示只有在出现性能问题才会去 code review,新员工刚来也会看看,感觉大部分小公司都这样。
|
25
IsaacYoung 2021-01-04 11:47:47 +08:00
走形式
|
26
mazyi 2021-01-04 11:59:33 +08:00 via iPhone
垃圾人做辣鸡事,都是垃圾业务怎么来厉害的技术呢?
|
27
Tonni 2021-01-04 12:10:39 +08:00
|
28
saulshao 2021-01-04 12:20:21 +08:00
@channingcheng
19# 我想表达的愿意其实是写代码的都不喜欢被人 Review,而参与 code review 的人都应该会喜欢。 所以逻辑上应该所有开发都参与 code review,这其实不是看有多少正面效果,而是增强互相之间的理解和沟通。 |
29
hantsy 2021-01-04 12:26:43 +08:00
@channingcheng 你从来没用过 CI 吗???
|
30
hantsy 2021-01-04 12:28:16 +08:00 1
@saulshao 这一点没错。更多应该 Peer to Peer Code Review,建立有效的沟通机制,而不是什么上级 Review 下面的人的代码。
|
31
Cstone 2021-01-04 12:42:56 +08:00
以前有,几个开发大家约个会议室一起认真评审代码,后来来了个领导,说减少工作时间开大会私下指定人 review 就行,代码评审也就成了走过场,再到现在没人 review 了
|
32
leega0 2021-01-04 12:50:56 +08:00
小公司就如你所说,开发周期根本没有 review 的时间给你,基本匆匆上线,各种改 bug 再测,循环往复。
|
33
dayeye2006199 2021-01-04 12:51:16 +08:00 4
我们公司只有三个人,也有 code review,有 CICD,有测试覆盖需求。这是一个企业文化的问题,对这点我们不妥协。
|
34
lights 2021-01-04 13:00:25 +08:00
刚毕业在传统 IT 行业的时候,没有 CR
后来跳槽做了两年多的互联网,配合 gitlab 做 CR 感觉很棒,偶尔能学到新姿势,也有利于增强代码维护性 再后来转行做游戏,虽然都是用 SVN,但第一家公司有做简陋的 CR,就是看一遍再用 QQ 或者口头和你沟通 现在依旧做游戏,没有 CR,虽然公司小,但毕竟有不同的人做同一个模块,偶尔会觉得比较蛋疼 |
35
hantsy 2021-01-04 13:16:14 +08:00
@dayeye2006199 这点没错,不用 CI,不写测试,所有 CR 都会是形式。
那种开会形式讨论,什么盯着纯代码提意见的,我也经历过,没任何实质性的作用。 具体 CR 实施一定要有的放矢,那么在 CI 中运行生成的测试报告,质量报告就是那个“的”,连一个最基本的讨论<<基点>>都没有的话,只能说大家公司都是在走过场,说白了,你们根本就不是在 CR 。 |
36
channingcheng 2021-01-04 14:22:07 +08:00
@saulshao 可惜我们这边都是只有业务 owner 才 code review,大头兵没有资格 code review
|
37
channingcheng 2021-01-04 14:37:57 +08:00
@hantsy CI 和重构是两个问题呀
|
38
hantsy 2021-01-04 14:38:10 +08:00
@channingcheng 你们那不是 CR 。只是普通的抽查工作情况(恰好拿代码挑刺说事),跟以前学校检查宿舍卫生一样,只有学工处干的事。
|
39
hantsy 2021-01-04 14:43:39 +08:00
@channingcheng 不知道怎么跟你解释你的问题。。。
|
40
Perry 2021-01-04 14:44:59 +08:00 via iPhone
Unit Testing, Code Review, Integration Testing, Accessibility Testing, Stress Testing, Chaos Testing, Load Testing 等等这些不通过估计进不了大公司的 Production 。
|
41
llllboy 2021-01-04 15:09:53 +08:00
就是个摆设
|
42
DiverRD 2021-01-04 15:26:47 +08:00
我一个版本改了 70 多个文件 组长看了 直接放弃 review
|
44
USAA 2021-01-04 17:19:33 +08:00
code review ? 这玩意不应该自己来弄吗
|
45
flowerains 2021-01-04 17:30:38 +08:00
有时间才能互相 code review
比如我们公司如果是压着点做需求,能按时按量把需求作完上线就不错了 |
46
hantsy 2021-01-04 17:55:04 +08:00
@DiverRD 说明你们项目管理问题非常大。一个 PR 一般只包含一个 Feature,不应该太多,正常一般几个文件吧。如果是 Bug,可能可能几行代码,几个字符。
|
47
hantsy 2021-01-04 17:57:04 +08:00
|
49
onec 2021-01-04 18:52:02 +08:00
纯纯的摆设
|
50
dfzj 2021-01-04 20:08:54 +08:00
在中型公司,项目分三个等级,只有 P1 等级才会 code review 。也就是是被定义为核心基础依赖的项目。
一般的业务项目,功能测试过了就发布了。 |
51
irytu 2021-01-04 20:30:14 +08:00
我们公司 code review 很多都喜欢鸡蛋挑骨头 要说意义么 几乎可以忽略不计
|
53
pangleon 2021-01-04 22:06:50 +08:00
楼上很多人没理解一点,CODE REVIEW 真正受益的是说人,他在给你介绍自己代码的时候也是重新梳理自己实现逻辑的流程,这个过程很能帮助他提高自己的思维甚至发现 BUG
|
54
akira 2021-01-04 22:24:16 +08:00
不做 cr 的产品 确实是能跑能上线,但是后面出问题你要花费数以倍记的时间。 单元测试也好,压力测试也好,代码评审也好,这些都是为了降低风险引入的,做了后续出问题的可能性小,不做后续出问题的可能性大。
出来混 迟早是要还的。 |
55
yexiaoxing 2021-01-04 22:26:14 +08:00 via iPhone
我们合规要求必须有人 review 才能 checkin 然后发 release 。
|
56
lagoon 2021-01-04 23:12:54 +08:00
我觉得,先是设计好,然后开发好,再然后才是测试好。
Code Review 肯定是好的,也是有用的。 但许多中小公司的情况是,设计没办法好,开发没办法好,然后领导指望抓测试,指望 Code Review,就能稳住质量。 本末倒置,只是安慰剂。 能不能设计好,取决于领导。能不能开发好,取决于领导。 领导不从自己身上找原因,寄希望于给开发人员找茬来稳住质量。 因此,大部分公司,不会 Code Review,或者只能进行走形式的 Code Review 。 |
57
hugo54 2021-01-04 23:34:48 +08:00
我所在的产品线的后端,都是 QA 来 CR 。 凡是非自主测试的代码,保准给你一行行 review 。
|
58
vagranth 2021-01-05 00:54:37 +08:00 via Android
我司不光有 review,而且还要求写测试 case 。
我曾经有一个大 patch 被要求拆分成 5 个 patch 慢慢 review 完的经历,要不是那个功能架构做的还可以,拆都拆不开。 |
59
msg7086 2021-01-05 01:23:34 +08:00 via Android
我们是强制 code review,除了其他开发以外,team leader 也要 review 一次,没问题了才能合并。
|
60
Leee 2021-01-05 08:32:30 +08:00 via Android
@dayeye2006199 你们在广州吗?还要人吗😂
|
61
cwliang 2021-01-05 09:21:04 +08:00
code review 很有必要,可以防止一些 anti-pattern 、性能冲击、维护性,虽然没时间看具体业务逻辑
|
62
jorneyr 2021-01-05 09:29:47 +08:00
理想很丰满,现实很骨感
|
64
leekafai 2021-01-05 10:13:00 +08:00
sr
self review |
65
runliuv 2021-01-05 10:49:38 +08:00
有个锤子
|
66
LemonK 2021-01-05 12:42:01 +08:00
@timedivision lint 和风格是两码事。比如有个前同事:他用他自己发明的标准划分为同类的变量,要求别人必须统一前后缀命名;多种语法都 ok 的逻辑,要求必须按他喜欢的那种写法;别人写 if 的地方他要求改成三目,别人写三目的地方他又要理论一下 if 更好。如果之前在这种代码孔乙己的身边工作,强调一下代码风格不重要还是有必要的。
|
67
timedivision 2021-01-05 13:51:04 +08:00
@LemonK 风格不是一个人制定的,不管怎么说,代码这种二进制的东西,实现某个功能逻辑有很多种方法,但一定是有一个最优解的,尽管说简单的 if 判断扯不上什么最优解,但对于项目本身来说,统一的风格一定是利于维护的
|
68
LemonK 2021-01-05 15:00:01 +08:00
@timedivision “写法一定有一个最优解”这句前提完全不认同。听起来就好像天下同一题目的作文只有一种标准写法一样。除了“一定不要这样写”的 Bad smell 共识之外,自由发挥的空间依然非常大。除非你只写 JAVA 。
|
69
timedivision 2021-01-05 15:57:30 +08:00
@LemonK 不要断章取义啊,我说的是实现某个功能逻辑的方法一定有一个最优解,另外你拿作文和代码相比,这两者根本没有可比性
|
70
mlbjay 2021-01-05 17:06:25 +08:00
项目组有个老同事,技术很强,但是 CR 后 总我“建议”我改一些“可有可无”的东西,我也不敢反对,改咯。
|