前言
Code Review是写出好代码的必备条件,但是有许多开发者盲目的去Code Review反而会降低开发效率。今天就跟大家分享一下Code Review的理念和思路。
一.为什么程序员需要做好Code Review
谚语曰: “Talk Is Cheap, Show Me The Code”。知易行难,知行合一难。嘴里要讲出来总是轻松,把别人讲过的话记住,组织一下语言,再讲出来,很容易。设计理念你可能道听途说了一些,以为自己掌握了,但是你会做么?有能力去思考、改进自己当前的实践方式和实践中的代码细节么?不客气地说,很多人仅仅是知道并且认同了某个设计理念,进而产生了一种虚假的安心感——自己的技术并不差。但是他根本没有去实践这些设计理念,甚至根本实践不了这些设计理念,从结果来说,他懂不懂这些道理/理念,有什么差别?变成了自欺欺人。// 堆代码 duidaima.com // BatchGetQQTinyWithAdmin 获取QQ uin的tinyID, 需要主uin的tiny和登录态 // friendUins 可以是空列表, 只要admin uin的tiny func BatchGetQQTinyWithAdmin(ctx context.Context, adminUin uint64, friendUin []uint64) ( adminTiny uint64, sig []byte, frdTiny map[uint64]uint64, err error) { var friendAccountList []*basedef.AccountInfo for _, v := range friendUin { friendAccountList = append(friendAccountList, &basedef.AccountInfo{ AccountType: proto.String(def.StrQQU), Userid: proto.String(fmt.Sprint(v)), }) } req := &cmd0xb91.ReqBody{ Appid: proto.Uint32(model.DocAppID), CheckMethod: proto.String(CheckQQ), AdminAccount: &basedef.AccountInfo{ AccountType: proto.String(def.StrQQU), Userid: proto.String(fmt.Sprint(adminUin)), }, FriendAccountList: friendAccountList, }因为最开始协议设计得不好,第一个使用接口的人,没有类似上面这个函数的代码,自己实现了一个嵌入逻辑代码的填写请求结构结构体的代码,在最开始效果还可以。但当有第二、第三个人干了类似的事情,我们将无法重构这个协议,只能麻烦地向前兼容。并且每个同学,都要理解一遍上面这个协议怎么填,理解有问题,就触发 bug。或者如果某个错误的理解普遍存在,我们就得找到所有这些重复的片段,去修改一遍。
// Update 增量更新 func (s *FilePrivilegeStore) Update(key def.PrivilegeKey, clear, isMerge bool, subtract []*access.AccessInfo, increment []*access.AccessInfo, policy *uint32, adv *access.AdvPolicy, shareKey string, importQQGroupID uint64) error { // 获取之前的数据 info, err := s.Get(key) if err != nil { return err } incOnlyModify := update(info, &key, clear, subtract, increment, policy, adv, shareKey, importQQGroupID) stat := statAndUpdateAccessInfo(info) if !incOnlyModify { if stat.groupNumber > model.FilePrivilegeGroupMax { return errors.Errorf(errors.PrivilegeGroupLimit, "group num %d larger than limit %d", stat.groupNumber, model.FilePrivilegeGroupMax) } } if !isMerge { if key.DomainID == uint64(access.SPECIAL_FOLDER_DOMAIN_ID) && len(info.AccessInfos) > model.FilePrivilegeMaxFolderNum { return errors.Errorf(errors.PrivilegeFolderLimit, "folder owner num %d larger than limit %d", len(info.AccessInfos), model.FilePrivilegeMaxFolderNum) } if len(info.AccessInfos) > model.FilePrivilegeMaxNum { return errors.Errorf(errors.PrivilegeUserLimit, "file owner num %d larger than limit %d", len(info.AccessInfos), model.FilePrivilegeMaxNum) } } pbDataSt := infoToData(info, &key) var updateBuf []byte if updateBuf, err = proto.Marshal(pbDataSt); err != nil { return errors.Wrapf(err, errors.MarshalPBError, "FilePrivilegeStore.Update Marshal data error, key[%v]", key) } if err = s.setCKV(generateKey(&key), updateBuf); err != nil { return errors.Wrapf(err, errors.Code(err), "FilePrivilegeStore.Update setCKV error, key[%v]", key) } return nil }
现在看这个代码挺好的,长度没超过80行,逻辑比较清晰。但是当 isMerge 这里判断逻辑,如果加入更多的逻辑,把局部行数撑到50行以上,这个函数味道就坏了。
由此出现两个问题:
1).函数内代码不在一个逻辑层次上,阅读代码,本来在阅读着顶层逻辑,突然就掉入了长达50行的 isMerge 的逻辑处理细节,还没看完,读者已经忘了前面的代码讲了什么,需要来回看,挑战自己大脑的 cache 尺寸。// Get 获取IP func (i *IPGetter) Get(cardName string) string { i.l.RLock() ip, found := i.m[cardName] i.l.RUnlock() if found { return ip } i.l.Lock() var err error ip, err = getNetIP(cardName) if err == nil { i.m[cardName] = ip } i.l.Unlock() return ip }i.l.Unlock()可以放在当前的位置,也可以放在 i.l.Lock()下面,做成 defer。两种在最初构造的时候好像都行,这个时候很多同学态度就变得不坚决。实际上,这里必须是 defer 的。
i.l.Lock() defer i.l.Unlock() var err error ip, err = getNetIP(cardName) if err != nil { return "127.0.0.1" } i.m[cardName] = ip return ip这样的修改,是极有可能发生的,它还是要变成 defer,那为什么不一开始就是 defer,进入最合理的状态?不一开始就进入最合理的状态,在后续协作中,其他同学很可能犯错!
template<class _PKG_TYPE> class CSuperAction : public CSuperActionBase { public: typedef _PKG_TYPE pkg_type; typedef CSuperAction<pkg_type> this_type; ... }这是 sspp 的代码。CSuperAction 和 CSuperActionBase,一会儿 super,一会儿又 base,Super 和 SuperBase 是在怎样的两个抽象层次上,不通读代码,没人能读明白。我想确认任何细节,都要把多个层次的代码都通读了,有什么封装性可言?
三.必须形而上的思考
同学们常常听演讲、公开课,就喜欢听一些细枝末节的“干货”。这没有问题。但是你干了几年活,学习了多少干货知识点?构建起自己的技术思考“面”,进入立体的“工程思维”,把技术细节和系统要满足的需求在思考上连接起来了么?当听到一个需求的时候,你能思考到自己的 code package 该怎么组织,函数该怎么组织了么?把工程实践中遇到的问题,从问题类型和解法类型两个角度去归类,总结出一些有限适用的原则,就从点到了面。把诸多总结出的原则,组合应用到自己的项目代码中,就是把多个面结合起来构建了一套立体的最佳实践的方案。当你这套方案能适应 30w+行代码的项目,超过 30 人的项目,你就入门架构师了!
当你这个项目是多端、多语言,代码量超过 300w 行,参与人数超过 300 人,代码质量依然很高,代码依然在高效地自我迭代,每天消除掉过时的代码,填充高质量的替换旧代码和新生的代码。恭喜你,你已经是一个很高级的架构师了!再进一步,你对某个业务模型有独到或者全面的理解,构建了一套行业第一的解决方案,结合刚才高质量实现的能力,实现了这么一个项目,恭喜你,你已经成为专家工程师了!
那么,我们要从头开始积累思考和总结吗?不,有一本书叫做《 Unix 编程艺术》,我在不同的时期分别读了 3 遍,文中的很多原则,正好就能作为 code review 时大家判定代码质量的准绳。但在那之前,我得讲一下另外一个很重要的话题,模型设计。
上来就是干,能实现上面提到的三个看似简单的需求么?想一想,亚马逊、腾讯云们折腾了多少年,最后才找到了容器+Kubernetes 的大杀器。这里需要谷歌多少年在 BORG 系统上的实践,提出了优秀的服务编排领域模型。权限领域,有 RBAC、DAC、MAC 等等模型,到了业务,又会有细节的不同。如 Domain Driven Design 说的,没有良好的领域思考和模型抽象,逻辑复杂度就是 n^2 指数级的,你得写多少 if else,得思考多少可能的 if 路径,来 cover 所有的不合符预期的情况。你必须要有 Domain 思考探索、model 拆解/抽象/构建的能力。
有人问过我,要怎么有效地获得这个能力?这个问题我没能回答,就像是在问我,怎么才能获得 MIT 博士的学术能力?我无法回答。唯一回答就是,进入某个领域,就是首先去看前人的思考,站在前人的肩膀上,再用上自己的通识能力,去进一步思考。至于怎么建立好的通识思考能力,可能得去常青藤读个书吧 :),或者就在工程实践中思考和锻炼自己的这个能力!
同时,基于 model 设计的代码,能更好地适应产品经理不断变更的需求。比如说,一个 calendar(日历)应用,随便想想,不要太简单!以“userid_date”为 key 记录一个用户的每日安排不就完成了么?只往前走一步,设计了一个任务,上限分发给 100w 个人,创建这么一个任务,是往 100w 个人下面添加一条记录?你得改掉之前的设计,换 db。再往前走一步,要拉出某个用户和某个人一起要参与的所有事务,是把两个人的所有任务来做 join?好像还行。如果是和 100 个人一起参与的所有任务呢?100 个人的任务来 join?不现实了吧。
好,你引入一个群组 id,那么,你最开始的“userid_date”为 key 的设计,是不是又要修改和做数据迁移了?经常来一个需求,你就得把系统推翻重来,或者根本就只能拒绝用户的需求,这样的战斗力,还好意思叫自己工程师?你一开始就应该思考自己面对的业务领域,思考自己的日历应用可能的模型边界,把可能要做的能力都拿进来思考,构建一个 model,设计一套通用的 store 层接口,基于通用接口的逻辑代码。当产品不断发展,就是不停往模型里填内容,而不是推翻重来。思考模型边界,构建模型细节,就是两个很重要的能力,也是绝大多数产品经理不具备的能力。你面对产品经理时,就听取他们出于对用户体验负责思考出的需求点,到你自己这里,用一个完整的模型去涵盖这些零碎的点。
if !needContinue { doA() return } else { doB() return } if !needContinue { doA() return } doB() return下面这个就是 early return,把两端代码从逻辑上解耦了。