谈谈 code review?

2020-07-29 09:57:45 +08:00
 sockpuppet9527

最近就遇到个烦人的,改来改去,就写个模块的接口,反反复复改,想到喊我改到哪。 比如一个函数:

int xxxx_init(CTX * ctx,A *a){
	if (xxx){
    	return rc;
    }
    xxxx // 实际逻辑代码段
    return rc;
}

一般来说,项目风格就是这样的,先检查 ctx 啥的,然后如果实际逻辑,最后返回调用状态。 这个方法能给我提 3 个 comments 。

  1. it is simple memcpy... do we really need all the checks below?
  2. i guess this function should return,remove A * a 。
  3. is it documented on API level? (实际逻辑代码段上一顿 bb)

看到这个我都不想回也不再改了,想问你调用 CTX 是有状态的,你调用函数前不需要 check ?其二本身方法逻辑就是大页来分配 A,名字也叫 xxx_init,我还返回个毛球啊。

搞得真想跑路。

6712 次点击
所在节点    程序员
35 条回复
otakustay
2020-07-29 11:53:56 +08:00
风格一致不应该是你靠人去 Review 的
Sasasu
2020-07-29 12:05:48 +08:00
明显指针作为入参比在函数内部 new 一个作为返回更好。

把对象生命周期控制交给调用者有更好的性能和更灵活的使用方式
Leigg
2020-07-29 12:26:41 +08:00
@gadsavesme [大家一起] 实际上是很难的,只有非常扁平化的团队。
netnr
2020-07-29 12:37:54 +08:00
安排不对口的人做专业的事
bsg1992
2020-07-29 13:04:26 +08:00
cr 是一件非常吃力不讨好的事情。首先 review 的人必须要懂你负责的业务,如果不懂业务背景单纯的只看代码的 review 是没有任何意义的,反而会出现负面效果。
代码风格和结构可以通过静态检查来约束。
团队人员多起来后 cr 是一个非常难以维持的一件事情。
我觉得 cr 最佳实践 符合小团队开发模式
iyaozhen
2020-07-29 13:33:14 +08:00
所以这就是 CR 推不起来的原因

CR 有这有那的问题,并不是 CR 本身有问题,还是做的不够好的原因。
1. CR 需要一个高 level 的来最终决定、平级可以给 review 意见
2. 大量代码量 CR 可以组织会议的方式进行
3. 不已评论数为唯一 KPI (可以作为大盘参考)

但这个事情其实需要团队内部文化认同,比如 CR 执行初期我们就遇到过有个同学 2 周硬是没有合入代码,但经过一段时间磨合就好额。
最后珍惜还有 CR 的公司吧
wangyzj
2020-07-29 13:39:24 +08:00
如网友想象的那么闲的公司 code review 估计也就是看格式,命名,和一些基础的样式的东西,深层次的东西不会有人有那么多时间去看
只有网友想象不到的闲的公司才会把逻辑,算法啥的看到底,当然效率肯定也是低下的
大部分 code review 估计都没有,测试过了就拉倒
tinkgoose
2020-07-29 13:47:14 +08:00
kop1989
2020-07-29 13:51:22 +08:00
code review 没错,但是 code review 的成本是极其高昂的。
如果是严谨的 code review,那么基本上审核者就要在脑中把程序编一遍才行。也就是说代码审核和编码其实就差一个“把代码打出来”的工序。

所以基本上国内 code review 都成了“格式审核”。
tinkgoose
2020-07-29 13:53:28 +08:00
@wangyzj 手滑了囧,不过一般来说格式、命名简单的我这边都是依靠工具而不是人来约束,因为确实没那么闲。

然后不了解这块业务的人去 review 的话,基本只能保证不出大问题,让项目更规整一些。所以这对 reviewer 的要去蛮高的,真的更依赖于 reviewer 的自觉。按经验我被 review 到的时候,都是一些不痛不痒的问题,很少能指出很实质性的问题,往往都是出了问题或者有同事来修改这一块的时候才发现的。

review 还是蛮有意义的,也不需要一直 review,一般磨合一段时间,就不需要过分地 review 了。
wangyzj
2020-07-29 14:21:18 +08:00
@tinkgoose #30 工具,lint 之类的做限制肯定是能解决一部分问题
核心问题还得看人
至于人能做到什么程度真的就按照你说的,得看个人水平和自觉性
所以很可能就会是你遇到的那种不痛不痒的问题

review 意义不用多说,不过自然最根本的也得看公司的业务发展等多个因素了
shenqi
2020-07-29 16:30:44 +08:00
不要把 code review 当成是挑别人毛病的事情,而是把 code review 当成自己学习成长以及促进别人成长的事情。
wangritian
2020-07-29 17:01:54 +08:00
code review 和做架构一样吧,需要根据实际情况柔性处理
jackindata
2020-07-29 17:46:53 +08:00
这个 reviewer 需要接受培训。
我觉得《代码大全》已经把 code review 讲得很清楚了。
leekafai
2020-07-30 10:47:49 +08:00
保证代码质量这个就很虚了,你的运行性能跟业务场景是挂钩的,大部分的“保证代码质量”可能就是人肉 linter,因为 viewer 不一定熟知你的业务场景跟需求,除非他有参与到你的项目中,哪怕做个技术观众也好过啥都不知道。
代码是写给人看的,code review 现在很多也是卡在了人肉 linter 这个层面,像业务逻辑这种,有时候越纠结越歪 B,到最后连需求是啥都混乱了。
最好的还是写测试,测试过了就是过了,因为测试就是可以跟着业务走的。fuzzer 跑几遍没什么问题还能咋地。
如果还有要紧地拓展性要考虑,那事实上 code review 也不能完全覆盖掉,否则哪有这么多老系统要重构呢。

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

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

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

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

© 2021 V2EX