这是一个创建于 1504 天前的主题，其中的信息可能已经有所发展或是发生改变。
我呆过几家公司，都是千人以上的规模。但是 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 。
哎，难啊。
第 1 条附言 · 2020-11-15 14:21:46 +08:00
感谢各位的回复，有些建议我会认真看的，后面在项目周会的时候，我也对经理提一些建议，不能把 code review 做成形式。
 |
1
wuzhouhui 2020-11-14 18:13:25 +08:00
我们的问题的积极性不高, 因为评审代码会耽误自己写代码的时间, 自己代码写不完, 领导就会责怪. 而在安排开发时间的时候, 是不会考虑到评审的耗时. 另外, 有些人水平太差, 改了几次都有问题, 然后就不想看了. 另外就是, 公司定的评审工具效率实在太低了. 所以现在, 通常是领导和对相关代码比较熟悉的员工会负责评审代码. 而水平差一点的, 我也不想让他们看, 他们提的意见实在没价值.
|
|
2
wuzhouhui 2020-11-14 18:16:24 +08:00
还有一个, 那就是公司的编码风格太讨厌了, 非常的啰嗦, 冗余, git 日志只写一句话甚至不写. 我比较喜欢内核那样简洁清晰的编码风格, 和详尽的提交日志.
|
 |
3
stupil 2020-11-14 18:18:23 +08:00  1
有人搞一个代码流程图， 模块怎么划分，接口怎么调用，类怎么嵌套， 一目了然。
当然这样搞的话，想喷的人更方便了。 |
 |
4
Solace202 2020-11-14 18:19:52 +08:00
那么有没有哪位有比较好的 code review 的最佳实践呢？话说我四年多了两家公司都没 review 过。。。二线城市
|
 |
5
stoneabc 2020-11-14 18:27:18 +08:00 via Android
语法错误和代码规范不应该在门禁就卡回去么。。
|
 |
6
fewok 2020-11-14 19:18:52 +08:00 9
我觉得吧，代码质量不是靠 code review 来控制的，应该先搞方案设计，把关调用流程（正向、逆向），尤其是异常处理，有没有把场景考虑全，某些场景该不该实现，能不能换个方式实现，有没有错误处理等等，尤其是状态机的流转和控制，谁驱动谁，能不能驱动等。
至于语法错误这些，你司搞个单元测试，静态代码扫描不就可以了（语法都错误了，说明这都没运行起来过） |
 |
7
southwolf 2020-11-14 19:19:23 +08:00
语法错误? 这些不应该直接被 lint/CI 卡掉么?
|
 |
8
lxk11153 2020-11-14 19:20:26 +08:00
233 就是这样的 [doge]
|
 |
9
Cbdy 2020-11-14 19:21:45 +08:00 via Android
层级太多了吧，code review 可能更适合管理扁平的工作组织吧，比如志愿者合作开发开源软件
|
 |
10
dustinth 2020-11-14 19:25:58 +08:00
语法错误和代码规范都可以通过工具自动拦住; 还有更多的比如是否方法过长, 嵌套层次太多这样 common pratice 也可以通过工具实现; 真正到了业务逻辑层面, 如果不是有配套的 pair programming 实践, code review 一个标准基本不太现实;
是否使用严格的 code review, 取决于代码改动的频率和次数: 业务变化快速频繁的领域或者代码价值不太高但是又不可或缺的领域, code review 可以适当放松限制; 对于成熟的业务价值大的领域, 可以采用严格的 code review, 这样才能带来更高的收益. |
 |
11
huifer 2020-11-14 19:37:33 +08:00
sonarqube 各个语言制定规则, 或者每个项目指定一个规则. 先过静态检查吧. 对于注释, 方法名称这些得需要人力成本, 或者公司内部存在一个词汇表. 针对一个项目或者整个企业内的, 通过统一词汇表来提高可读性. 对于方法名称, 类名称 同样适用.
|
 |
12
VDimos 2020-11-14 19:55:18 +08:00 via Android 1
语法错误怎么还能到 CR 这一步
|
 |
13
impl 2020-11-14 21:01:02 +08:00 via Android 6
|
 |
15
laminux29 2020-11-14 21:16:49 +08:00 1
Code Review 的本质是让大佬带萌新。这里有几个问题：
1.让大佬带萌新，浪费了大佬的时间，对整个公司价值来说，值得吗？而且大佬会认为自己被招来干这事，属于自己的岗位职责吗？ 2.萌新缺乏基础理论，大佬教萌新，萌新能听懂吗？教学效率高吗？ 3.要提高萌新水平，还不如让萌新带薪脱产去学校学习。 |
 |
16
illusionist 2020-11-14 21:20:30 +08:00 via iPhone
代码 review 正常来讲，对多人合作开发项目是必要的，但是很多时候会出现有+2 权限的人看不懂代码，看懂代码的人没有加+2 权限。而且很容易走入为了 review 而 review, 流于形式。没啥好的解决方案，只能说管理决策层要头脑清醒，知道为啥 review，很多时候 review 是一种权利运动，而不是真正为了提高软件质量。
|
 |
17
GreyYang 2020-11-14 21:24:17 +08:00
LGTM!
|
 |
18
DoctorCat 2020-11-14 21:27:35 +08:00
常态。社会倦怠问题解决不来。我觉得 CodeReview 的测重点应该放在师徒传授阶段和别人侵犯（动）相关责任人的代码时，彼此有个技术交流，尽力降低技术风险和债务。
|
 |
19
zion03 2020-11-14 22:22:05 +08:00 via Android
|
 |
20
lecher 2020-11-14 22:33:23 +08:00 10
带着业务理解 review，我司要求 code review 的目的是为了避免单点，在代码实现的思路上让团队达成一致，至少相关业务开发人员达成一致，review 之后意味着这个模块出了问题，我也可以负责修。这样出了任何问题，review 过的人都可以直接修改代码，而不是推脱这个模块不是我写的，我不需要为此负责。
所以在我司，code review 没有把重点放在语法之类的实现细节上，重点是在代码有没有体现合理的设计思路，代码可读性上。 而且 code review 已经是最后一步了，功夫应该在 code review 之外。 概要设计文档，细节设计文档，都需要有配套的评审，将技术债务尽可能减少。设计文档的评审通常是老带新，老手写概要设计框住业务边界，新手写详细设计明确实现细节，由评审来保证相关人员对业务需求的拆解理解达到一致。 此外就是配套的 CI lint test 等支持，规范代码风格，保证功能满足需求。 实际 review 的时候，会快速看看 test 的覆盖率快速确认对外暴露的调用约定是否合理，边界样例是否覆盖业务需求的要求。其次是看代码结构，可读性如何，再次才是语法细节，有没有不合理的写法，对于不合理的写法通常只有新人的前几个 PR 高标准评审，甚至可能有一个新人三四个老人评审的情况。 这么带几轮下来，大部分团队成员的代码风格和业务拆解都接近一致，基本上出问题都可以让一个人一杆子捅到底层实现去定位和修复。如果发现是代码分层和架构满足不了新增需求可能还有重构的情况。 享受到的收益是： test 是以业务边界为要求写的，不用担心重构的影响范围过大。 底层模块业务稳定，容易交接，新人上手阅读代码和改代码的成本低。 这么做的问题是，功能迭代速度太慢，一个业务需求从设计到实现拖的周期会很长。还会有不少时间放在重构偿还技术债上。 这种模式只适合核心业务，对于开发周期急迫的任务并不太适合。 |
 |
21
feather12315 2020-11-14 22:37:56 +08:00 via Android
@lecher #20 新手代码都搞不懂，咋写实现细节？
|
 |
22
wobuhuicode 2020-11-14 23:36:00 +08:00
新入职的公司也是这样…… review 和 没 review 一个样，都是为了现实上级有干活的发个 已阅。
上线之后依旧一堆 bug |
 |
23
zpxshl 2020-11-14 23:58:08 +08:00 via Android
说说我司情况
1 cr 不需要领导＋ 2，事实上一般也不会找领导 cr 2 改动到特定模块强制需要模块负责人 cr 3 代码不符合语法规范的代码提交时自动拦住 总体还行，毕竟模块负责人也不希望你的改动影响到他的模块。至于自己模块写的代码找人 cr，主要看对方是否负责，负责的会慢慢看仔细看，同时我也会在旁边讲解。 不负责的直接就＋ 2 了。 呆几个月你也知道谁靠谱谁不靠谱了。 当然了，cr 者其实是无偿帮你，用爱发电了，所以找人 cr 时态度好点，大的改动尽量分批 cr，多点注释，画个流程图就更好了。一来就上千行业务代码的提交，反正我是没心情看得。。。 |
 |
24
0Shaka 2020-11-15 00:31:58 +08:00
比较好的实践就是平时只找组里一两个人 review, 高效快速还没有那么多 b 事.
|
 |
25
ga6840 2020-11-15 00:42:02 +08:00 1
个人认为公司里的代码也应该有 maintainer，而且他和他维护的代码肯定是利益相关。你的 PR 肯定应该是这些人 review 。无关的或者平级的再或者高出几层的领导来看的话最多好心看看。如果组织没有什么奖励热心的制度，那么八成也就没有人有意愿。
|
 |
26
linux40 2020-11-15 00:56:13 +08:00 via Android
聘几个搞程序分析、语义分析的吧。
|
|
27
lecher 2020-11-15 02:35:27 +08:00
@feather12315 特别新的新手挑几个写好细节设计的文档让他写实现，尽可能调新增业务或者特别简单的 bugfix
|
 |
28
MrGba2z 2020-11-15 04:09:19 +08:00 1
CR 这件事情我觉得从效率提升上来说的优先级是:
1. 好的 infrastructure: 我们公司代码的自动检查不仅仅负责语法, 规范, 还会自动优化 条件表达式 (比如一堆 XOR 如果可能的话他会改成更短更合理的形式) 以及 best practice 和 deprecated 的自动修复. 最后会跑配置好的测试. 这些全通过之后才会发送 CR 请求, reviewers 都会收到推送提示 (macOS 的话就是系统通知, 也有 Chrome 插件会持久监视) 2. Reviewer 人员的选择, 每个人 review 代码的严格程度是不一样, 如果你想快一点, 就选已知的比较松的人, 如果想提高或者对自己写的代码没信心, 就找 review 严格的人. 3. 组里 review 代码的风格. 我们 TL 之前介绍过他的风格, 不一定适合所有人但是我觉得挺不错, 主要的几点是: 1. 如果代码达到了目的, LGTM. 2. 可读性 > 语言规范 3. 小 CR > 大 CR 但是如果 大 CR 已经写好了, 除非自己看不懂, 否则尽量不要要求对方拆分. 除了这个之外, 我们组基本上收到 CR 会在 5~30 分钟内完成 Review. (没人要求这么做, 只是大家都这样, unblock 别人也是间接提升整个组的效率) 4. 就是自己写的代码了, CR 描述是否能提供足够的背景? CR 是否太大而不方便 Review? (比如我看到个 2000 行的 CR, 本能的会想 这 TM 得找个 30 分钟的时间段慢慢看, 现在没空 马上要吃饭了, 然后就.....) 当然话说回来,很多事情不是自己能改变的. 如果你是 TL 倒是可以尝试改变. 如果有机会的话, 去大公司去哪怕实习一个月, 也能看到别人的这套流程有多好. |
 |
29
DamienS 2020-11-15 04:36:26 +08:00
这些问题不难解决吧
积极性: 1. 代码 review 加入到季度员工 kpi 2. review kpi 包含质量考核 规范： 1. 规范 UI PR 要截图，规范日志要写啥 2. 规定一些谁可以接，几个人接了就可以（一般一个人接了就行吧，不然影响效率，而且写 c++的 review java 代码这种就很扯淡，经理接也很扯淡） 3. 规定所有的评论下个版本就要全改完，不能改一半，别人又指出有的还没改 简化工程师负担： 1. 每个 PR 自动化监测一些基本错误，比如用 linter，或者端对端测试，没过的回去改，直接不到 review 那一步 2. 建立一些 review group 。不用一个一个加或者减 |
 |
30
laike9m 2020-11-15 04:39:59 +08:00 via Android
这 review 就离谱。还是多学学正规的大公司是怎么做 review 的吧
|
 |
32
meetubyaccident 2020-11-15 05:01:04 +08:00
代码没注释，随便看看就行，TM 费时间还要猜你为啥你这么写，很累人的，自己的活不用干吗？出错自己改。
|
 |
33
yaaaaaak 2020-11-15 09:02:22 +08:00 via iPhone
语法问题 ci 都过不去吧
|
 |
34
leavic 2020-11-15 09:40:35 +08:00
都一样，做硬件十几年，早就知道了一个基本定律：所有人都不靠谱，不要指望别人帮你检查出任何错误。
|
 |
35
hpeng 2020-11-15 10:16:51 +08:00
那不是。那些都叫代码合并工程师。
|
 |
36
b00tyhunt3r 2020-11-15 10:24:39 +08:00
@wuzhouhui
老哥 想问一下详细的提交日志具体是指啥，git commit 绑定到编辑器吗 |
 |
37
hantsy 2020-11-15 11:40:59 +08:00 2
@kpppp 只能说你公司的实施问题吧。
可以看看这两位的观点。 @lecher https://www.v2ex.com/t/725245#r_9776838 @huifer https://www.v2ex.com/t/725245#r_9776373 在编码前，对于一起讨论需求的理解，最终确定代码要完成哪些功能，在开发之前也要归纳出来，可以在 PR 或者 Issue 形成一个 Checklist，例如一个表单注册。 1 。 如果信息填写不完整，Validation 失败，提示错误 2 。 信息完整，提交后，检测 Email，手机号是否存在，如果存在，反馈错误信息 3 。 如果一切检测通过，保存用户信息，（发送 Welcome 邮件，初始一些用户配置等） 首先，对于现代软件开发，特别云环境，一开始就需要一个自动化的 Pipeline 配置，检测分支代码情况，一目了然。 https://github.com/hantsy/spring-reactive-jwt-sample/pull/59 1，Commentlint 等， 2，检测语法（比如老式的静态检测 Checkstyle, findbugs 等如果你喜欢可以配置下，甚至可以加入到 Git Hooks 里面。不过基于云的 SonarCloud 检测更详细，我不再用本地静态分析工具） 3，运行测试（单元测试，功能 /集成测试）<--------必不可少， 如果不写测试，CR 只能是自欺欺人。 4, 生成测试报告，包含代码质量报告（ SonarCloud 的规则是 Social 更新的，可以支持最新的语法，规范等。公司如果大方，可以购买 Code Climate， 借助人工智能分析代码，可以修正弱智代码），可以看到 Bugs，Code Smell，技术债务等 那 CR 的时候，我们要清理所有在这个 PR 产生的技术债务，修正 Bad Code Smell 等，而且要检测功能集成测试是否包含了所有的路径，是否有漏掉什么步骤等。 CR 几乎不需要 [人工方式] 关心一些枝节细节，什么语法，命名等，这些工具检测（在 CI ）都可以完成。更多的目的大家一些讨论和分享，真正做到 Pair programming 和 Brainstorming 。 |
 |
38
andj4cn 2020-11-15 11:50:23 +08:00
我觉得跟开发团队层级管理有关。开发团队应尽可能扁平化，我一般代码只被 2 个人 review，最多了。其他人想 review 也可以
|
 |
39
ClericPy 2020-11-15 11:54:24 +08:00
哈哈哈 其实挺羡慕有正经 code review 的公司的, 现在天天赶进度文档都跟不上, 别说 review 了
PS: 听说有一种设计就是让不懂这门语言的人也能看懂你代码的, 类似于注释言简意赅地做到类似小黄鸭的程度 |
 |
40
charlie21 2020-11-15 13:22:40 +08:00
哈哈 “业务 review” 比 “code review” 更重要。光 review code 没啥意思
|
 |
42
loveyou1 2020-11-15 13:41:16 +08:00
review 毛线，prettier + 各种语言 lint + 编辑器插件，把最基本的东西定死，就是最好的办法，一大堆人没搞这个业务的来 review 这个代码，逻辑不都不清楚，还 review，开源的项目 review 也多数是通过跑自动化脚本 review 的，一般都不会去看的，每个人思路不一样，处理方式也不一样，如果水平差别大一些，可以改一下东西，如果水平差不多，人工 review 有毛线用，还不如分享分享实现思路。
|
 |
43
firefox12 2020-11-15 13:59:02 +08:00
cr 的关键是业务太快，没时间搞，一个需求可能没上线就已经改了。
真正好的是 java c++ 用什么版本的工具帮你做 cr 前的校验， 那些规则，这些更有落地的必要。 |
 |
44
yuekcc 2020-11-15 17:30:17 +08:00
|
 |
45
crclz 2020-11-15 19:44:03 +08:00
这里有一篇文章，里面包含了 Thoughtworks 公司做 code review 的部分流程。
https://insights.thoughtworks.cn/code-review/ 我摘抄一段： “ 主持人说：“我们知道，如果代码编写得好，那么不是作者的人就能在没有作者帮助的情况下读懂。我希望一位不是这段代码作者的志愿者，来为大家解释一下这段代码是做什么的。”一位非作者的志愿者上来逐行解释代码，并回答大家的疑问。 主持人等代码解释完后，问大家：“这段代码大家还有看不懂的地方吗？”如果有问题，包括作者在内的参会者都可以回答问题，但大家都不提谁是作者。 大家都看懂代码后，主持人问：“大家说说这段代码有没有好的编写模式咱们可以继续发扬？…… ” 楼主说看不懂代码，那肯定是出了严重的问题（在编写代码的阶段）。 |
 |
46
asanelder 2020-11-15 20:06:08 +08:00
@fewok #6 同意，俺认为 review 不是 review 设计相关，设计，规范之类的应该之前就做好了，review 只是看 check in 的代码是否符合系统的设计和规范，防止坏味道的东西进来。
关键问题是，很多项目设计和规范都没来的及制定，就开始写代码了，然后为了 review 而 review，简直是自期期人罢了。 还有，现在的产品和项目经理还是要懂开发的流程的，不是只写业务代码才算是有产出，那些为此支撑的东西，也是要认真考虑，加入到进度时间开发中的。但俺观察的现象是，似乎他们只认为只要开发业务代码就行了，预估时间中根本没有为其它相关的留出时间。 你说你搞了 CI/CD, 搞了设计，自动化了一些流程，制定开发分支规范等等和业务不直接相关的事，在他们看来。 “就是没任何产出” |
 |
48
Zchary 2020-11-16 00:40:56 +08:00 via iPhone
我没猜错的话，你说的应该是菊厂。
|
 |
50
ruzztok 2020-11-16 09:53:37 +08:00
我公司代码异常处理非常差。。生产环境日志文件每秒都在打印错误信息😂
|
 |
51
qinyusen 2020-11-16 10:24:08 +08:00
https://www.v2ex.com/t/719560#reply24
11 楼，和 23 楼， 虽然题目是提高自己，但是实际上，code review 的时候，附上设计文档的内部 wiki 链接之类的。。。是一个很友好的做法， 很容易让大佬挑刺。 最后看看设计文档没问题， 实现大约 1：1 就可以过了。 |
 |
52
kuro1 2020-11-16 11:17:30 +08:00
LGTM
|
 |
54
ksssdh123 2020-11-16 16:46:23 +08:00
哥们，你说得太棒了~
但现在往往最可怕的是什么你知道吗?方法论大家都知道，但往往现实就是陷入死循环，一边喊着要快速上线，一边又要抓评审、抓文档，老工程师又不愿带萌新，然后很多流程都是敷衍了事，除了编码工作，但这种环境下，这种产物也就够演示.... 跟楼主所说的一样，流程变成形式，很多都是走马观花 |
Select Language