Code Review实践
2021-12-21 21:38:06 6 举报AI智能生成
代码审查实践
作者其他创作
大纲/内容
Code Review 主要是在软件开发的过程中，对源代码进行同级评审，其目的是找出并修正软件开发过程中出现的错误，保证软件质量，提高开发者自身水平
代码审查通常是指代码合到主分发布前，审查者（Reviewer）查看代码并给出改进意见
被审查人（Author）根据审查者的意见进行修改，符合要求和合并到主分支的过程
代码审查是保证项目质量和促进团队技术交流和提高技术水平的重要手段
代码审查是什么？
如果没有代码审查，代码中隐藏的 Bug，隐藏性能问题，隐藏的设计缺陷将直接发布到线上，非常影响项目的质量
如果没有代码审查，开发人员很难主动发现自己代码中存在的问题，很难快速进阶
为什么会有代码审查（CodeRewiew）
可以发现代码中潜在的安全风险
可以发现功能和性能、可读性、可拓展性、可维护性等方面的诸多问题
还可以帮助团队成员发现自己编码的问题，提高其技术水平
还能够促进团队成员之间了解彼此业务，增强团队成员间的技术交流
而一场好的代码审查
提高项目的质量
提高整个团队的技术水平
代码审查的必要性
代码审查简介
很多团队并不重视代码审查，认为代码审查可有可无
有些团队对代码审查理解过于片面，认为代码审查只不过是看看变量、函数命名，看看函数的行数等简单的带代码风格问题
重视程度不够
很多团队代码审查时间节点比较随意，有时候到了代码提测阶段才安排进行代码审查
代码审查过晚，即使审查人员能够出很多有用的建议有时候也来不及优化，导致代码审查的效果就会大打折扣，代码审查也就更流于形式
代码审查过晚可能导致优化之后引入新的 Bug 严重影响测试
时间节点不明确
提测后进行代码审查，对代码进行优化从而引入新 Bug，会让测试跳脚
很多代码审查全凭参与者的兴趣和经验，可以说参与者的技术水平直接影响代码审查的效果
评审人员很可能不了解相关业务背景，导致代码审查的效果大打折扣
评审人员容易按照自己的编码风格来评价被审查者的代码
评审人员可能会带着批评的质疑别人的能力的心态评审代码。评审人员可能敷衍了事
评审人员水平和喜好各有不同
有少部分开发者，可能听不进去别人的意见
大多数开发者，不能够在评审前进行静态代码扫描、自我代码审查等来提前发现问题
很多开发人员，在代码审查时，代码讲解毫无重点和逻辑
被评审人员的自觉性各有不同
代码审查的质量无法保证
如果没有文档留存，就无法很好地跟踪提的意见是否都落实掉了
无法让团队内其他成员共享代码审查的经验，同一个错误团队成员可能反复犯错，代码审查并没有很好的发挥其应有的作用
现在很多人组织代码审查发现问题口头沟通，虽然有些人记录到自己的临时笔记里，有些人通过 TODO 记录到代码中，很少有正式的文档留存
代码审查没有记录留存
有些项目进度比较赶，就本能地忽视单元测试和代码审查
进度矛盾
实际工作中由于开发工期很紧，很多团队对代码审查重视的程度也不够，导致代码审查草草了事，流于形式，甚至直接忽略
要求全员都要参加，很多时候并不现实
比如“推荐全员参加代码审查”
诚然理想状态下完成小部分代码就审查，及时修改，可以避免最后因时间紧张来不及修改，因要改进的太多打击自信心等情况
但是实际落地时由于大家工作都比较忙，每次只审查一小部分不太现实
比如“每次只审查少量代码”
的确如果能够通过规范的命名，优雅的编码风格，巧妙的设计让代码更清晰，可以不需要注释
然而，“理想很丰满，现实很骨感”，开发者的技术水平参差不齐，开发者的技术追求也不同，代码编写时有些令人困惑的设计、一些已经废弃的函数等需要添加必要的注释
比如“最好的注释就是没有注释”
很多代码审查的文章给的建议并不适合你的团队，希望大家可以批判性吸收
适合的才是最好的
代码审查的现状
建议在开发完成和提测前阶段，就要进行代码审查
很多修改或者优化很容易引入新的 Bug，会影响测试进度，造成不必要的回归工作量
如果再晚，即使代码审查阶段给出了很多不错的建议，如果改动稍微大一些，由于时间紧张，很容易就废弃掉了
如果在提测后进行代码审查
时间
参与项目开发的其他人员
熟悉项目的老员工
其他经验丰富的同事、技术主管或者架构师
如果是多个后端开发人员参与一个大型项目开发，代码审查时可以相互审查，互通有无
熟悉项目的老员工对项目的架构，项目的坑更了解，更容易发现业务上的隐患
技术水平高的同事或者架构师阅历更强，更容易从技术层面发现问题，给出靠谱的建议
总结
人员
https://www.gerritcodereview.com/
Gerrit
https://sider.review/
Sider
https://www.atlassian.com/software/crucible
Crucible
https://www.jetbrains.com/upsource/
Upsource
通常用 Gitlab 或者直接 IDEA 里将开发分支和 master 进行对比，投屏讲解
在实际落地时
工具
1、约好相关人员，确定好时间，拉代码审查会议
2、会上将代码审查的建议记录到代码 TODO 中或者临时文档中
3、会议结束以后形成代码审查文档，方便跟踪进度和经验共享
审查准备步骤
使用阿里规约检查工具、 FindBugs 静态代码检查工具等先把如空指针、没用到的变量等常见问题先解决掉
准备好需求文档、技术方案文档的链接，以便在正式代码审查前对业务背景和技术方案进行介绍，让参与审查的人员了解相关背景
准备代码核心入口，最起码找到自己认为最需要代码审查的部分（可能存在性能问题，但是没有好的方案）
自己按照代码审查的核心要点，对自己的代码进行初步自我代码审查
提前将代码审查文档（此时只有时间、地点、人员、项目分支、变更列表等基本信息）和需求文档、技术方案文档等相关信息，发给参与代码审查的人，这样可以提前对相关背景有了解
代码审查人员可以根据技术方案文档和项目分支和代码变更列表，提前看下代码，显而易见的问题可以提前做笔记，当代码审查会议当场提问
代码审查前的准备
推荐在审查前简单介绍需求，然后介绍技术方案，让审查者对相关背景有一个很好的认识，然后再进行代码讲解
每个公司代码审查的流程差异可能很大
需求文档简介
技术方案简介
被审查人员分别讲解核心接口代码
开发人员对自己的代码进行讲解，审查人员给出 CR 意见
形成代码审查文档
修改后重新审查
审查流程
代码审查的意见可以在对应代码处写下“TODO:: CR xxxx”，评审结束后再整理到文档中
详细记录代码审查的前期准备内容，和代码审查过程提出的宝贵建议，跟踪修改进度
代码审查的一般流程
很多人在代码审查完毕，根据修改建议修改完代码万事大吉
如果每次审查完，还是依然如故，那么代码审查还有啥用呢？
其实这还不够，大家一定要重视代码审查，对审查的意见要引起足够的重视，下次编码不再出现才能达到预期效果
代码审查后
步骤
《代码整洁之道》
《重构-改善既有代码的逻辑》
《阿里巴巴 Java 开发手册》
代码审查要审查要审查的点非常多，可以重点参考
实践中，很多人会将代码审查的重点放在可读性上，虽然可读性很重要，但是对项目演化影响最大的还是代码设计
设计是代码审查的重点
新代码是否符合整体架构设计
是否遵循 SOLID 、KISS（Keep It Simple and Stupid）、领域驱动设计和其他设计原则
使用了哪些设计模式，使用的是否恰当?
代码风格是否和团队规约保持一致？
是否存在过度设计？
代码设计需要考虑的问题有很多
设计
代码可读性很重要，因为优雅的代码是优秀程序猿的标志之一
如果可读性不高，读懂代码需要花的时间更长，改出 Bug 的概率更大，后续维护成本更高
1. 名称（属性、变量、参数、函数、类名）是否真实反映出其意图？
代码太长，如果中间某个子功能有问题需要排查或者去改造，通常需要将前面的代码都看懂，非常痛苦
public void someFunction(){// 第 1 个子功能 30 行// 第 2 个子功能 60 行// 第 3 个子功能 40 行// 第 4 个子功能 70 行// 第 5 个子功能 55 行}
可以把每个子功能独立为一个子函数，排查问题或者改造某个子功能更容易
有些规定不超过 80 行，有些可能规定不超过 90 行。只是一个“推荐值”，不要教条，其目的还是让代码变得更清晰易读，具体行数只要团队达成共识即可
2. 函数是否过长？（推荐 80 行以内）
public void someFunction(){// 第 1 个子功能 30 行 层级 1// 第 2 个子功能 60 行 层级 2// 第 3 个子功能 40 行 层级 1// 第 4 个子功能 70 行 层级 2// 第 5 个子功能 55 行 层级 1}
可以将代码行数较多的函数根据子功能提取到子函数中。但是如果函数的层级不同，则应该将相同层级放在同一个函数中
比如可以将第 1、3、5 子功能提取到独立的子函数，放在 someFunction 函数中调用，然后将第 2、4 个子功能提取到独立的子函数中在第 1、3 或 5 子函数调用
3. 子函数是否在同一抽象层级？
1、如编写工具类时，可以参考
org.apache.commons.lang3.StringUtils#defaultString(java.lang.String)
的注释，将常见的参数和结果加到注释中，对调用者非常友好：
2、如在编写前端接口或者封装二方或三方接口调用时，可以将相关枚举引用、相关接口文档链接，对接说明文档链接等放到注释中方便后面快速理解上下文
/** * xxx 功能 * 对接文档 {@link <a href="https://gitbook.cn/"/>} */public Result someFunction(Object param) { // ...}
4. 注释是否清晰？可能让其他开发人员产生困惑的代码是否有文档解释或者注释说明？
基本要求
可读性
if((( price > 0 && price < 50) && length >500)|| type ==3){`// ...}
有些判断条件特别复杂，如：
boolean priceSuitable = price > 0 && price < 50;boolean itemSuitable = Suitable && length >500;boolean someType = SomeTypeEnum.A == SomeTypeEnum.get(type);if(itemSuitable|| someType){`// ...}
可以把具有联系更紧密并且可以组成完整语义的语句提取成一个布尔变量；可以通过枚举让逻辑更清晰易懂，进行改造：
1. 判断条件是否过于复杂？
类内部的代码重复，可以通过提取子函数实现复用
类之间的代码重复，可以通过提取到公共类中实现复用
服务之间的代码重复，可以通过功能下沉或提供公共服务实现复用
2. 是否有大量重复代码？
单测是否覆盖完全？
是否有没考虑到的用例？
3. 是否有单测？
错误描述是否准确？
4. 异常处理是否正确？
是否打印的日志太多？
日志级别是否合理？
日志的级别对不对？
日志占位符的数量和参数数量是否对应？
是否超大对象的日志打印？
5. 是否缺乏必要的日志？
可维护性
比如判断某个用户 ID 是否在 userIdList 集合中，要避免下面时间复杂度为 O(n) 的写法：
List<Long> userIdList = xxx;if(userIdList.contains(someUserId)){ //... }
应该使用 Set 进行快速去重，时间复杂度为 O(1)
在实际工作中就遇到采用 List 去重，数据量大时，尤其循环中采用这种方式处理，造成接口频繁超时
for example
比如在循环中调用某个接口，但是该接口的参数和循环并不相关，就可以提取到循环外面
是否存在不必要的网络调用？
比如一个函数中需要查询 50 个用户信息：
List<Long> userIdList = xxxx;for(Long userId : userIdList){ User user = uicService.queryUserInfo(userId); // ...}
可以尽量让下游提供批量接口，如果有则直接调用批量接口；如果没有则可以考虑并发调用伪代码如下：
是否存在耗时的同步操作？是否存在循环中网络调用？
1、是否使用线程池、缓存等方式，采用空间换时间、问题转换等方式提高性能？
2、表的索引是否合理？
3、数据量大时，是否选用了合适的数据结构和算法，是否选用了合理的处理策略？
几点思考
性能需求
安全性非常重要，如果不重视，很容易造成资损，给公司带来一些无法挽回的损失
是否存在线程安全问题？是否存在数据一致性问题？
敏感操作是否有操作日志？
比如下载某个私密文件接口，需要用户 ID 或者文件 ID 两个参数：
代码中一定要验证该用户拥有该文件下载权限再执行下载的逻辑
举例说明
是否存在越权问题？
是否正常关流？
是否存在 SQL 注入风险？
是否容易暴力攻击？
数据是否需要加密？
设计是否有可能引起资损？
敏感信息是否日志脱敏？
安全性
扣款金额如果传入负数怎么办？
当前用户是否有权限执行扣款操作？
1. 是否进行了参数合法性校验（防御性编程）？
尤其在和第三方对接时，一定要考虑各种失败场景，根据实际情况考虑各种兜底方案。
有些接口调用失败需要考虑重试
设计幂等方案避免上游发送的消息重复处理
对于流量较大的业务评估是否需要做限流、熔断、降级处理等
2. 是否考虑各种失败场景（面向失败编程）？
健壮性
要点
邀请团队骨干或者了解业务的同学参与代码审查往往效果会更好
一定要留意代码审查的时机，代码审查太晚来不及修改
团队应该制定或者选择一份代码规范，代码审查时按照该规范执行，避免因标准不一致而扯皮
对事不对人：代码审查时，要营造一个讨论和解决问题的氛围，而不是批斗大会
如果项目开发时间非常紧张，在代码审查时可以选择核心的几个接口优先审查
注意事项
为了让代码审查发挥更大的价值，每次代码审查完成后建议一定要形成代码审查文档，通过代码审查文档来记录代码审查时发现的各种问题，跟踪修复情况
代码审查之后可以考虑把代码审查文档到项目组的群里让大家隐式监督代码审查情况，学习当前项目中发现的问题，学习代码审查人员给出的好的修复建议。这样才能更大限度地发挥代码审查的价值
经验共享
代码审查不是万能的，很多应该在需求分析、技术方案设计等阶段处理掉的问题不要留在代码审查阶段
问题暴露地越晚，修复的代价越大
如果代码和需求有偏差，就应该在需求评审会议上解决
如果技术方案有问题，应该在技术方案评审阶段解决
如果代码审查流于形式，风险和错误将流转到后续环节中， 进而影响项目的质量
代码审核和其它软件开发环节的关系
代码审查重头戏
它是 Visual Studio 的一个扩展，支持 Visual Studio 2019、2017、2015、2013、2012 和 2010
Review Assistant 可以帮助创建审查请求并能在不离开 IDE 的情况下对请求做出响应
它将“代码审查板（Code Review Board）”窗口添加到 IDE 中，该窗口可用于管理用户所有可用的审查
Review Assistant 支持在代码中讨论、支持电子邮件通知、支持替换 Visual Studio 代码审查功能，它的特性还包括灵活的代码审查、丰富的集成功能、带有缺陷修复的迭代审查等
超链接
1. Review Assistant
这是一个基于 SaaS（Software-as-a-Service，软件即服务）的软件平台，它可以帮助软件开发团队在部署代码到生产环境之前，更快地识别出代码中更多的漏洞
可以减少发现和修复漏洞的成本与时间，可以识别数据泄露的潜在风险，并能帮助软件公司达到合规性和法规要求
Reshift 可以与 GitHub 和 Bitbucket 集成，可以跟踪每个开发人员功能分支的漏洞，它还支持智能筛选，通过标记问题来减少超时误报
使用 Reshift，可以通过拉取请求（pull-request）这个工作流为团队的处理流程提供安全性，并可以避免切换到其他面板
Reshift 的特性还有，在合并到主干之前了解关键的漏洞，如果引入了新漏洞，则关闭构建
2. Reshift
这是一个开源的轻量级工具，它是基于“Git 版本控制系统”来进行构建的
在所有用户都是受信提交者的项目环境中，该工具非常有用，因为该工具允许用户检查项目中所做的总体变更
阻止用户直接推送到 Git 库
许开发者在源代码中查找错误
允可以帮助开发者创建新变更或更新现有的变更
支持在开发者模式和 Git 库之间进行转换
Gerrit 的主要特性
3. Gerrit
这是一个开源的在线源码审查 Web 应用程序
此代码审查工具可以帮助开发者在数据库中记录问题、注释和决策。它也可以用于代码检查（Code Inspections）
Codestriker 支持传统的文档审查，它可以与 Bugzilla、ClearCase、CVS 等集成
4. Codestriker
它还包括了基于 Web 的轻量级代码审查、规划、测试、Bug 发现等功能
这是一个开源的源码扫描程序
提交前（Pre-Commit）的代码审查
支持编写有用的注释和备注信息
Phabricator 的特性
它还可以帮助每个部门构建独立的任务表单以及定制任务管理
5. Phabricator
使用该工具，开发者可以了解整个项目的代码质量、最近提交的内容以及问题最多的文件，并针对每次提交（commit）和拉取（pull）的请求进行跟踪及问题修复
可以概述代码库
提供分析数据
与开发过程无缝集成
简化代码审查流程并改进可操作的报告
CodeFactor.io 的主要特性
6. CodeFactor.io
这是一个代码审查工具，它可以安排审查、共享内容并查看代码审查变更，还支持持续集成部署
Helix Swarm 可以帮助开发者监控进度、自动化设计过程并提高项目的发布质量，它也可以通过将多个变更分支附加到提交前（pre-commit）审查中来同时审查独立的组件，
通过将 Helix Core 与 Ping Identity、Okta 或其他工具集成，可以确保代码是安全的
Helix Swarm 允许开发者按照优先级进行筛选代码
7. Helix Swarm
它使用了二进制代码 / 字节码，并能保证 100% 的测试覆盖率
这是一个代码审查和静态分析工具，它是基于 SaaS 模型构建的
即使源码不可用，也可以通过一致的流程和策略测试桌面、Web 或任何大小的桌面应用程序
开发者不需要手动及自动配置就可以测试多个应用程序
通过在 SDLC 中简化和集成测试来自动化不同的工作流、通过持续审查过程来提高代码的生产效率等
使用 Veracode的好处
8. Veracode
Code Review工具
一场好的代码审查，可以帮助个人发现问题，可以增加团队成员之间的了解，提高整个团队的技术水平
但是不要太过依赖代码审查，很多问题需要在需求分析、技术方案设计的阶段处理掉，而不是在代码审查阶段取暴露出来，因为缺陷暴露的越晚，修改的成本越大
希望大家能够养成良好的习惯，通过规范的代码审查流程，借助代码审查文档实现经验共享，快速提高自己的技术水平
代码审查的质量也影响后续环节的质量
Code Review实践
0 条评论
回复 删除
下一页
