万字长文细说 Code Review 的正确姿势_大语言模型 code review(1)

作为公司代码委员会 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.gro