代码审查机制
[TOC]
什么是代码审查
Code Review已经是很多公司的常规实践,初看上去好像是浪费时间,降低工作效率,其实反之,好处大家有目共睹。它能检查代码的正确性,合理性,安全性,发现隐秘的bugs,让系统更可靠的运行。它能保证代码能有两个或以上的人熟悉,促进知识共享。它能让团队成员互为备份,互相支持,不会有SPOF。它能威慑埋雷的任何想法,杜绝邪念。它能互相学习好的代码,提高编程技能。等等。
代码审查应该是集体行为而不是个人行为,它是一个过程,多个开发人员审查或筛选另一位开发者(作者)编写的代码,以确保
- 代码没有任何错误或问题。
- 符合所有质量要求和标准。
- 代码执行了预期的测试。
- 合并后,它将使代码库的运行状况保持更好。
审查标准
该代码改善了系统的整体运行状况
每个更改列表(Pull Request)都会改善系统的整体运行状况。想法是,由于进行了如此小的改进,每次合并后,软件或代码库的运行状况都会得到改善。
快速的代码审查,响应和反馈
首先,不要延迟推送(合并)更好的代码。不要指望代码是完美的。如果它的状况可以改善系统的整体运行状况,则请推送。
这里的关键是没有'完美'的代码,只有更好的代码。
好的代码是初级工程师可以理解的代码。伟大的代码可以被第一年的计算机专业的新生理解。代码要追究简单
如果不在一项重点任务的中间,那么请在代码完成后立即进行检查。但是一个工作日是响应拉取请求所需的最长时间。预计变更列表将在一天之内获得多轮的部分/完整代码审查。
在代码审查期间进行教育和启发
通过尽可能共享知识和经验,在代码审查期间提供指导。
在代码审查中要谨慎,尊重,友善和清晰
至关重要的是,在代码审阅期间,要善良,清晰,礼貌和尊重,同时也要对作者非常清楚和乐于助人。查看代码时,请确保对代码而不是开发人员做出评论。
每个 Review 至少给一条正面评价
Code Review 本意是改善代码质量,增强团队成员之间的沟通,但是我一提交代码就有人说我写的垃圾,这很打击自信心啊,也不利于团队成员和平相处。代码有问题,指出问题是必须的,要实事求是,但是有的时候也需要给队友一点鼓励,例如简单的 或者“赞一个”我都很开心了。
用工具进行基础问题的自动化检查。
用 Tab 还是空格,用两个空格还是四个空格,函数后面怎么换行等基础问题检查,可以使用 eslint 和Rubocop 等类似的工具进行,团队成员应该把更多精力放在代码规范,代码性能优化等地方。
全员参加 Code Review,并设定各部分负责人。
扩大 Code Review 参与面,参与不是说一定去审核别人的代码,可以是代码被审核,也可以是看别人审核意见,这都是学习的过程。并且每部分设定负责人,该负责人对这部分代码质量负责,负责人需要是资深工程师。全员参与 Code Review 可以让团队成员更快的成长,新人在看大佬 Review 代码的过程就能学到很多。
每个代码 PR 内容一定要少
Code Review 效果和质量与 PR 代码量成反比
在写新代码之前,先 Review 掉需要评审的代码
你让我去 Review 一周前的代码?我还得把思维和项目进度切换到一周前?大家肯定不愿意,所以要形成规定,写新代码之前先 把旧的 Review 掉,提交 PR 的时候也保证代码量小,这样 Review 起来不需要大块时间,改起来也快。不能因为 Code Review 大幅耽误项目进度,进度是全团队的事,不是某个人的事。
如果你有更好的方案,尽管提出来
在 Code Review 中经常会发现写的不好的地方,如果你有更好的方法,欢迎提出来!首先能改进这个 PR 的代码,其次能体现你的能力,团队应该定期对这种提出好的解决方案的同事进行奖励。
例子
在下面的例子中,代码块中的建议性审查注释以 //R: ...
的形式标出:
不一致的命名
class MyClass { private int countTotalPageVisits; //R: 变量命名要统一 private int uniqueUsersCount; }
不一致的方法签名
interface MyInterface { /** 当 s 无法被提取时,返回 {@link Optional#empty} */ public Optional<String> extractString(String s); /** 当 {@code s} 不能被重写时,返回 null */ //R: 应该统一返回值:都用 Optional<> public String rewriteString(String s); }
Bugs
//R: 这里执行了 numIterations+1 迭代, 是故意的吗? // 如果是的话,考虑改变 numIterations 的含义吧? for (int i = 0; i <= numIterations; ++i) { ... }
架构上的关注点架构上的关注点
otherService.call(); //R: 我觉着我们应该避免对 OtherService 的依赖。我们当面聊一下如何?
审查时机
每次代码提交,向主仓库提交Merge Request 进行代码的 Review
Code Review 工具
- Crucible:Atlassian 内部代码审查工具;
- getcodeflow:微软代码评审工具
- Gerrit:Google 开源的 git 代码审查工具;
- GitHub:程序员应该很熟悉了,上面的 "Pull Request" 在代码审查这里很好用;
- LGTM:可用于 GitHub 和 Bitbucket 的 PR 代码安全漏洞和代码质量审查辅助工具;
- Phabricator:Facebook 开源的 git/mercurial/svn 代码审查工具;
- PullRequest:GitHub pull requests 代码审查辅助工具;
- Pull Reminders:GitHub 上有 PR 需要你审核,该插件自动通过 Slack 提醒你;
- Reviewable:基于 GitHub pull requests 的代码审查辅助工具;
- Sider:GitHub 自动代码审查辅助工具;
- Upsource:JetBrain 内部部署的 git/mercurial/perforce/svn 代码审查工具。
参考
https://juejin.cn/post/6869024508796207111#heading-13
https://juejin.cn/post/6844903733797584909#heading-7
https://blog.csdn.net/weixin_33975951/article/details/90334884