那些在腾讯的大佬，都如何做 Code Review？

作者：cheaterlin，腾讯 PCG 后台开发工程师

前言

作为公司代码委员会 golang 分会的理事，我 review 了很多代码，看了很多别人的 review 评论。发现不少同学 code review 与写出好代码的水平有待提高。在这里，想分享一下我的一些理念和思路。

为什么技术人员包括 leader 都要做 code review

谚语曰: ‘Talk Is Cheap, Show Me The Code’。知易行难，知行合一难。嘴里要讲出来总是轻松，把别人讲过的话记住，组织一下语言，再讲出来，很容易。绝知此事要躬行。设计理念你可能道听途说了一些，以为自己掌握了，但是你会做么？有能力去思考、改进自己当前的实践方式和实践中的代码细节么？不客气地说，很多人仅仅是知道并且认同了某个设计理念，进而产生了一种虚假的安心感—自己的技术并不差。但是，他根本没有去实践这些设计理念，甚至根本实践不了这些设计理念，从结果来说，他懂不懂这些道理/理念，有什么差别？变成了自欺欺人。

代码，是设计理念落地的地方，是技术的呈现和根本。同学们可以在 review 过程中做到落地沟通，不再是空对空的讨论，可以在实际问题中产生思考的碰撞，互相学习，大家都掌握团队里积累出来最好的实践方式！当然，如果 leader 没时间写代码，仅仅是 review 代码，指出其他同学某些实践方式不好，要给出好的实践的意见，即使没亲手写代码，也是对最佳实践要有很多思考。

为什么要在 review 中思考和总结最佳实践

我这里先给一个我自己的总结：所谓架构师，就是掌握大量设计理念和原则、落地到各种语言及附带工具链（生态）下的实践方法、垂直行业模型理解，定制系统模型设计和工程实践规范细则。进而控制 30+万行代码项目的开发便利性、可维护性、可测试性、运营质量。

厉害的技术人，主要可以分为下面几个方向：

• 奇技淫巧

掌握很多技巧，以及发现技巧一系列思路，比如很多编程大赛，比的就是这个。但是，这个对工程，用处好像并不是很大。

• 领域奠基

比如约翰*卡马克，他创造出了现代计算机图形高效渲染的方法论。不论如果没有他，后面会不会有人发明，他就是第一个发明了。1999 年，卡马克登上了美国时代杂志评选出来的科技领域 50 大影响力人物榜单，并且名列第 10 位。但是，类似的殿堂级位置，没有几个，不够大家分，没我们的事儿。

• 理论研究

八十年代李开复博士坚持采用隐含马尔可夫模型的框架，成功地开发了世界上第一个大词汇量连续语音识别系统 Sphinx。我辈工程师，好像擅长这个的很少。

• 产品成功

小龙哥是标杆。

• 最佳实践

这个是大家都可以做到，按照上面架构师的定义。在这条路上走得好，就能为任何公司组建技术团队，组织建设高质量的系统。

从上面的讨论中，可以看出，我们普通工程师的进化之路，就是不断打磨最佳实践方法论、落地细节。

代码变坏的根源

在讨论什么代码是好代码之前，我们先讨论什么是不好的。计算机是人造的学科，我们自己制造了很多问题，进而去思考解法。

重复的代码

// 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。或者，如果某个错误的理解，普遍存在，我们就得找到所有这些重复的片段，都修改一遍。

当你要读一个数据，发现两个地方有，不知道该选择哪个。当你要实现一个功能，发现两个 rpc 接口、两个函数能做到，你不知道选哪一个。你有面临过这样的’人生难题’么？其实怎么选并不重要了，你写的这个代码已经在走向 shit 的道路上迈出了坚实的一步。

但是，A little copying is better than a little dependency。这里提一嘴，不展开。

这里，我必须额外说一句。大家使用 trpc。感觉自己被鼓励’每个服务搞一个 git’。那，你这个服务里访问 db 的代码，rpc 的代码，各种可以复用的代码，是用的大家都复用的 git 下的代码么？每次都重复写一遍，db 字段细节改了，每个使用过 db 的 server 对应的 git 都改一遍？这个通用 git 已经写好的接口应该不知道哪些 git 下的代码因为自己不向前兼容的修改而永远放弃了向前不兼容的修改？

早期有效的决策不再有效

很多时候，我们第一版代码写出来，是没有太大的问题的。比如，下面这个代码

// 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 尺寸。
2. 代码有问题后，再新加代码的同学，是改还是不改前人写好的代码呢？出 bug 谁来背？这是一个灵魂拷问。

过早的优化

这个大家听了很多了，这里不赘述。

对合理性没有苛求

‘两种写法都 ok，你随便挑一种吧’，‘我这样也没什么吧’，这是我经常听到的话。

// 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