智汇 IT
城启新篇

代码评审里没人说透的那些真问题

上周跟一个做了8年后端的朋友喝酒,他骂了半小时公司的代码评审。说每次评审跟走流程似的,大家对着提交记录挑错别字,连变量名多打了个下划线都要提,真到了逻辑有坑的地方,全装瞎。

评审的本质从来不是挑错

很多人对代码评审的误解,从把它当“代码批改作业”开始。说白了,学校里改作业是给老师交差,职场里的代码评审,是你要把自己写的东西,交到几个以后要给你擦屁股的人手里,让他们确认这东西以后不会半夜把人从床上薅起来。
代码评审会议上多人对着投影的代码逐行讨论的实拍
代码评审会议上多人对着投影的代码逐行讨论的实拍
麻烦在于,现在太多团队把这个事搞成了KPI。比如有的团队要求每个提交必须有3个以上评审人点头,不然合不进主干。结果呢?大家为了凑人头,随便拉两个不相关的同事点同意,连代码都没打开过。我见过最离谱的,一个写前端的同事,被拉去评审后端的数据库存储过程,他连那个存储过程用的什么方言都不知道,扫了两行就点了approve——后来那个存储过程上线3天,把用户的订单状态全搞混了,赔了小几十万的优惠券。 真正有价值的评审,从来不是找那些一眼能看到的小问题。变量名好不好,缩进对不对,这些东西交给lint工具去管就行,人要干的是工具干不了的事。比如这段循环会不会在极端数据下死循环,这个异常捕获会不会把本来要抛出去的业务错误吞掉,这个接口的参数校验是不是漏了权限校验的环节,这些才是评审该盯的东西。

评审里最容易被忽略的成本

很多团队算不清评审的账。比如一个3人小组,花1个小时开评审会,看起来好像没什么成本——但这1小时,是三个本来可以写代码、测bug、改需求的人的时间。要是这1小时全用来挑缩进、挑变量名,那纯纯是浪费。
IDE里代码评审插件的行内批注界面截图
IDE里代码评审插件的行内批注界面截图
我之前待过的一个团队,踩过这个坑。那时候我们要求所有提交必须走线上评审,每次评审人要写至少3条意见才能通过。结果大家为了凑够3条,本来没意见也要硬找。比如有人会在评审意见里写“这里的注释可以再写得详细一点”,“这个变量名可以改成更有意义的”——这些意见改了也没什么用,不改也出不了事,但提交的人要花半小时改完,再发回去重新评审,一来一回,大半天就没了。后来我们改了规则,lint能查的问题一律不准在评审里提,评审意见只写会影响功能、可维护性、安全性的问题,整个评审的平均时长直接砍了三分之二。 不过话说回来,也不能走另一个极端。有的团队嫌评审麻烦,干脆全跳过,谁写的代码谁自己上线。那更糟,我见过一个刚毕业的小孩,写了个批量删除用户数据的脚本,没加where条件,直接在生产库跑了,删了10万条用户数据,花了两天两夜才从备份里找回来。这事没那么玄,就是本来评审的时候只要有人扫一眼那个脚本,就能发现问题,结果没人看,全是代价。

好的评审从来不是对立关系

很多人把评审搞成了“找茬大会”。提交代码的人觉得评审人是在针对自己,评审人觉得提交的人写的什么垃圾东西,两边对着呛。上次我参加一个评审会,一个老员工对着新员工写的代码拍桌子,说“你这写的什么玩意,我看一眼就觉得恶心”。新员工脸涨得通红,会后直接提了离职。 说白了,大家都是来干活的,不是来比谁写的代码更牛的。你评审的时候提意见,是说“这段逻辑这里有个坑,咱们一起改了,以后少点麻烦”,不是说“你写的代码真垃圾”。我见过最舒服的评审,是提交代码的人自己会在提交说明里写清楚“这里我用了个临时方案,因为时间紧,后续要重构”,评审的人看到了,就会针对性地问“那这个临时方案会不会影响XX模块?要不要加个注释标出来?”,两个人三两句就把问题说清了,既不浪费时间,也不会伤和气。 还有个事,很多人没注意到:代码评审其实是团队里成本最低的技术传承方式。老员工看新员工的代码,能知道他哪里写得不对,新员工看老员工的提交,能知道团队里常用的技术方案是什么。要是每次评审都只挑错别字,那谁也学不到东西。我之前带过的一个小孩,刚进公司的时候写的代码全是硬编码,后来跟着看了几十次老员工的评审,自己就知道要把常量抽出来,要加异常处理,比给他讲两小时技术文档有用多了。 上周那个骂评审的朋友,后来跟我说他们团队改了规则。现在lint能查的问题不准提,评审意见只说真问题,每次评审之前,提交的人要先花10分钟把自己的代码逻辑过一遍,有坑的地方自己先标出来。现在他说,每次评审再也不用对着那些破事扯皮了,真出问题的概率也降了不少。
免责声明:市场有风险,选择需谨慎!此文仅供参考,不作买卖依据。如有侵权请联系删除。
文章名称:代码评审里没人说透的那些真问题
文章链接:https://www.itcdj.com/it/348.html