技术分享 | CodeReview主要Review什么?
源宝导读:Code Review, 意即代码审查,是指一种有意识和系统的召集其他程序员来检查彼此的代码是否有错误的地方. 在敏捷团队中推行CodeReview, 可以帮助团队快速成长.本文将分享在"天际-建模平台"如何推行&实践CodeReview。
一、为什么要 Code Review?
要不要Code Review,需要结合当前的环境和形势来决定。如果项目组开发任务极其紧张, 此时再进行Code Review可能会收到不利的效果,因势利导,求同存异是Code Review的核心所在。
Code Review, 需要和项目组成员沟通和配合,同时也在检验各成员的技术水平。
需要注意的是,人都有惰性,每个人都有自己的一套行为准则和规范,要想把他们的行为或规范统一起来,很容易使他们产生抗拒心理。
在Code Review之前,和项目成员沟通好,有一个共同的愿景,并辅助以相应的培训,把部分人员的短板补齐。会减轻项目成员的一些不适感,也会使得项目运行更加顺畅。
同时, 也要注意,不要事事求完美主义,一步求成。"慢即是快"。
二、 Code Review的前提条件
Code Review本身属于"事后"工作,可以起到查漏被缺的作用。
在推行Code Review时,需要先提前准备以下几个工作,以便团队能够更快更好的接受和实施。
建立规范, 包含:
编码规范,如变量命名,文件命名规范等。
设计原则,如单一职责原则,最少知识原则等。
分支管理策略。
完善的文档,方便查阅。文档内容最好能共建,千万不可出现"一言堂";
制定Code Review流程&目标,以及实施周期。
例如, 根据当前团队的实际情况,将Code Review实施分为3个阶段。
第一阶段,重点关注,恰到好处的函数注释,"硬编码"问题,常见变量命名规则等,预期实施周期为1~3个月。
第二阶段,重点关注,代码耦合性,单一职责、最少知识原则, 潜在隐患,性能问题等,预期实施周期为3~6个月。
第三阶段,重点关注,模块实现方案,设计模式,最佳实践,代码重构等。
在实施过程中,如果遇到比较大的阻力或困难,推行Code Review所得到的收益较低时,可以考虑适当"休息"一段时间。
三、常见的Code Review项
3.1 git 提交规范
git commit提交规范, 如:
不标注信息
不及时commit
提交时标注的信息不规范等
都是严重阻扰review的因素之一,除了常规的描述信息外,还可以按类型等备注:
feat: 新特性
fix: 修改问题
refactor: 代码重构
docs: 文档修改
chore: 其他修改
test: 测试用例修改
style: 代码格式修改等等
当然,也要以利用一些工具,来协助我们完成基本的约束和检查。
3.2 风格
3.2.1 可读性
衡量可读性,有很好的实践标准,即Code Review时能否非常容易的理解代码逻辑,如果不能,那意味着代码的可读性要进行改进。
3.2.2 命名
命名对可读性非常重要,我个人倾向于函数名/类名长一点都没关系,但必须能清晰的表明函数/类的作用。
英语用词尽量准确, 哪怕需要借助翻译工具,也是值得的。但有一点需要注意,如果使用的单词很冷门,都没人认识, 那就不要使用。
3.2.3 函数体长度/类长度
函数体太长,不好阅读,一般建议不要超过50行。
类太长, 如超过1000行,那可能就要看下是否违反了"单一职责"原则。
3.2.4 参数个数
不要太多,一般不要超过5个, 超过5个,建议使用对象。
3.2.5 注释
恰到好处的注释,能够帮助我们理解函数/类的作用。
3.3 架构/设计
3.3.1 单一职责原则
这是经常被违背的原则,也是最难运用好的原则。
一个类只做一类相关的事情。
一个函数/方法, 最好只做一件事情。
3.3.2 行为是否统一
什么是行为统一? 例如:
错误处理是否统一
错误提示是否统一
弹出框是否统一
......
同一逻辑/行为,有没有执行同样的代码路径, 低质量的代码一个特征是,同一逻辑/行为,在不同的地方或不同的方式触发时,没有执行同样的代码路径(产生出不同的结果),或者是各处copy一份实现,导致非常难以维护.。
3.3.3 代码污染
代码有没有对其他模块强耦合。
3.3.4 重复代码
主要看有没有把公用组件,可复用的代码、函数抽取出来。
3.3.5 开放-封闭原则
简单理解是,看代码好不好扩展。
3.3.6 健壮性
核心数据有没有强制校验?
对业务有没有考虑完整, 逻辑是否健壮。
有没有潜在的bug?
有没有内存泄露?有没有循环依赖?
......
3.3.7 错误处理
有没有很好的Error Handling? 如网络出错,,IO出错等。
3.3.8 面向接口编程/面向对象接口编程
主要看有没有进行合适的抽象,把一些行为抽象为接口。
3.3.9 效率/性能
客户端程序对频繁的消息和较大数据等耗时操作是否处理得当。
关键算法的时间复杂度是多少? 有没有潜在的性能瓶颈?
3.3.10 代码重构
新的改动是打补丁,让代码质量继续恶化,还是对代码质量提升有帮助?
四、Code Review如何实施?
结合当前建模小组团队的实际情况:
新人较多,多对业务不对熟悉。
前期团队人员少,但有CodeReview的文化。
已有相关的编码规范的知识沉淀,但因之前迭代任务重/人力资源紧张等因素, 导致未长期推行下来。
针对以上情况,经过团队内部协商研讨,输出相应的解决机制:
"结对编程",每个Story尽量安排两位同学一起完成(1位主责人),主要有两个目的:
老带新,在工作实践中多沟通交流,帮助新员工尽快熟悉业务,融入团队。
相互审查代码,提高代码质量,同时促进新老员工沟通交流,思想上多碰撞。
两级代码审查机制
第一级,由"结对编程"小组的两位同学交叉审查代码,过滤掉一些简单的代码问题。
第二级,由技术leader进行最终审查,确保代码审查保质保量完成。
定期分享收录的典型问题
代码审查过程中,发现的一些典型问题,及时收录并做好分析。
定期开展"坏代码"分享会。
主要实施分三个阶段进行:
第一阶段, 恰到好处的函数注释, "硬编码"问题,常见变量命名规则等,预期实施周期为1~3个月。
第二阶段,代码耦合性, 单一职责、最少知识原则, 潜在隐患,性能问题等, 预期实施周期为3~6个月。
第三阶段,模块实现方案,设计模式,最佳实践,代码重构等。
从目前实施过程的体会, 以及结合同学们的一些经验来看,在Code Review时建议:
单次查看代码不多于500行,人的精力有限,一次审查太多的代码,收益可能不理想。
单次审查建议不要超过30分钟。
祖传代码怎么办?
只查看git diff中, 带有"+"的代码,历史代码可以暂时忽略不看,在不断的迭代中,迟早会遇到需要修改"祖传"代码的。
这也是一种"妥协"的办法,让代码审查者和开发者之间有一个平衡。
对于"开发者"的心理会产生一定的影响,他/她可能需要提前做好准备,万一哪天需要修改"祖传"代码呢?
在下一节中,收录了实施过程中发现的一些典型的问题代码,大家可以一起"品尝"下。
五、 低质量代码的常见特征
5.1 案例1
违反"单一职责"原则。
同一逻辑/行为, 通过不同的方式触发时, 不会执行同样的代码路径。
缺少注释。
函数/类命名乱, 词不达意, 或"挂羊头卖狗肉"。
export function setDefaultImg(res) {
let imgUrl = getImagePrefixUrl()
let previewImgSrc = require('@/asserts/images/icon.png')
if(res.imgId) {previewImgSrc = `${imgUrl}${res.imgId}.${res.type}?t=${+newDate()}`}
return previewImgSrc
}
上例函数,其实是想对传入的参数进行判断,如果值无效,取默认图片,否则返回拼接地址好的img src相对地址。从函数的实现来看, 有如下几个问题:
函数名setDefaultImg并不合适,无任何对数据的更新/修改,最终目的是想拿到img的拼接地址(或者默认图片)。
形参`res`是一个对象, 实际使用到的属性只有两个基本类型,遵循最少知识原则,应修改形参。
`let imgUrl`,其实是拼接url的前缀,取名如 `imgPrefixUrl`会比较合适。
let 使用不当,如 `let imgUrl`,因涉及不到变量再赋值,使用`const是比较合适的`。
`res.imgId`,即异常情况的处理,少了对`res.type`的处理。
变量命名,如`previewImgSrc`,取此名也不太合适,该src使用的目的并不确定,建议使用有比较通用涵义的名字。
可考虑重构为:
export function getImgSrc(imgId, imgType) { if(typeof imgId !== 'string' || typeof imgType !== 'string') { return require('@/asserts/images/icon.png') } return `${getImagePrefixUrl()}${imgId}.${imgType}?t=${+new Date()}`}
5.2 案例2
参数传递自由度过大,易引发后续不可控的风险。
handleSelect(){this.$emit('select', this.$props)
}
上例代码中,传递给`$emit`的数据是`this.$props`,会有以下问题:
存在高耦合关系( 数据结构 耦合)。
接收参数,建议尽量使用:基础数据类型,如:字符串, 数字,布尔值等,非必要情况下, 不使用复杂数据类型,如: Array, Object等。
可考虑重构为:
handleSelect(){ this.$emit('select', this.$props.userId)}
5.3 案例3
违反"单一职责"原则,一个函数处理多个任务,可能会在调函数时产生“异外的感觉”。
形参设计过度(形参无须解构,后续若有需要添加新的参数时再添加,也可考虑使用options参数等来实现扩展)。
handleRowDelete({ row }) { this.$refs.grid.remove(row) let open = false for (let i = 0, l = this.$refs.grid.innerData.length; i < l; i++) { let item = this.$refs.grid.innerData[i] if (item.isQueryField) { open = true break } } if (!open) { //文本行 this.$refs.grid.innerData[0].isQueryField = true this.handleChange({ name: 'isQueryField', row: { $index: 0 }, data: this.$refs.grid.innerData }) } }
从上面的代码中,我们可考虑从以下几个角度着手重构, 以增强可读性。
在入口函数处理,调用功能函数(通过函数名来区分功能),明示调用逻辑。
减少代码量,增强可读性,能用ES6 方法解决的就用它解决,尽量不造不必要的轮子,比如使用Array.findIndex来解决查找数组是否存在某一项的问题。
优化业务逻辑,删除不必要的代码,增强代码的可维护性。
函数重新命名,务必做到“见名知意”。
添加必要的注释(遵循jsdocs注释规范)。
可考虑重构为:
mounted(){const {row} = this.getSelectedRow();this.deleteRow(row);this.resetGrid();},/*** @description: 删除指定的表格行数据* @param {Object} row, 表格行数据* @return*/deleteRow(row) {this.$refs.grid.remove(row)}/*** @description: 重置表格**/resetGrid(){if(this.$refs.grid.innerData.length && this.$refs.grid.innerData.findIndex(item => item.isQueryField) < -1){this.$refs.grid.innerData[0].isQueryField = truethis.handleChange({name: 'isQueryField',row: { $index: 0 },data: this.$refs.grid.innerData})}}
六、总结
敏捷团队由"创建-振荡-成熟-规范"的过程中, 不同的阶段建议采取不同的方式开展CodeReview, 需要考虑团队的整体情况以及当前公司的业务的紧急程度, 优先解决首要问题,再接着解决其他次要问题。
通过几个月的渐进 & 持续的推行Code Review后,团队成员是可以达到以下效果的:
促进团队成员的相互交流, 加速团队朝"成熟-规范"阶段前进。
知识分享, 促进团队成员进行反思和总结。
代码质量和可维护性, 可读性等的提高。
查漏补缺, 发现一些潜在的问题点等。
最佳实践, 能够更好更快的完成任务的方法。
------ END ------
作者简介
张同学: 开发工程师,目前负责天际-建模平台产品的研发工作。
也许您还想看:
技术分享|Java SDK动态数据源和上下文机制
技术分享|NodeJS分布式链路追踪实现
技术分享 | Java SDK 元数据驱动的事件通信架构
技术分享|Hangfire深度实践
技术分享 | APT结合JavaPoet生成模板化Java源代码文件
技术分享 | 玩转高效UI自动化测试
更多明源云·天际开放平台场景案例与开发小知识,可以关注明源云天际开发者社区公众号:
【建模】文档服务提供高保真打印模式
明源云·天际硬核技术认可:获华为鲲鹏技术认证书
天际·开发者社区“重装发布”!
技术分享 | CodeReview主要Review什么?相关推荐
- ClickHouse技术分享第二弹(英文讲义)
前言 以下是今天为公司小伙伴们做的ClickHouse技术分享的讲义.由于PPT太难做了,索性直接用Markdown来写,搭配Chrome上的Markdown Preview Plus插件来渲染,效果 ...
- 团队管理那点破事!OKR绩效、核心人才、面试、技术分享、研发流程....
今天来聊聊团队管理,可能你现在还是一线开发,没有带团队,感觉这个话题与你无关,其实不然. 程序员的职业生涯曲折,技术更新迭代快,走技术深度,走架构师路线,势必要付出常人的时间和精力.而管理则可以更好的 ...
- 团队管理那点事,OKR绩效、核心人才、面试、技术分享、研发流程
团队管理那点事,OKR绩效.核心人才.面试.技术分享.研发流程 今天来聊聊团队管理,可能你现在还是一线开发,没有带团队,感觉这个话题与你无关,其实不然. 程序员的职业生涯曲折,技术更新迭代快,走技术深 ...
- 阿里技术分享:深度揭秘阿里数据库技术方案的10年变迁史
本文原题"阿里数据库十年变迁,那些你不知道的二三事",来自阿里巴巴官方技术公号的分享. 1.引言 第十个双11即将来临之际,阿里技术推出<十年牧码记>系列,邀请参与历年 ...
- 阿里集团口碑将于南京举行大型技术分享及专场招聘会
福利来了~~ 福利来了~~ 福利来了~~ 重要的事情说三遍~~~~~~ 口碑是阿里巴巴集团与蚂蚁金融服务集团深度整合双方优势资源,联手打造的一家互联网本地生活服务平台,于2015年6月23日正式成立 ...
- Inplayable技术分享
Inplayable技术分享 运维 设计模式 Web 安全 工具 语言 python 运维 <aws lambda 通过codebuild上线踩坑指南之 lambda 进程被占用 status ...
- iOS-FXDanmaku弹幕库介绍、相关技术分享
前言 去年, 2016年, 一大波直播平台在移动端涌出, 直播慢慢步入了人们的视角. 网上如今能够看到各式各样的直播, 如秀场直播.游戏直播.体育直播.娱乐直播等等. 在各种类型的直播中, 弹幕在PC ...
- UI培训技术分享:设计大神都在用的10种技法!
越来越多的人开始学习UI设计,想要进群到UI设计这个行业,本期小编为大家介绍的UI培训教程就是关于设计师会经常用到的一些技巧,帮助大家后期的工作中的应用. UI培训技术分享:设计大神都在用的10种技法 ...
- UI设计培训技术分享:搞定萌萌哒可爱图标
UI设计要学到的东西有很多,那么关于图标设计就是其中的一种,很多企业比较忠于萌萌哒的可爱图标,那么如何搞定萌萌哒可爱图标呢?来看看下面UI设计培训技术分享教程. UI设计培训技术分享:搞定萌萌哒可爱图 ...
最新文章
- 数据分析师在岗3年小结!
- 配置oracle服务器以从外部机器访问oracle
- Programming with gtkmm 3
- 华为ipd产品开发流程_亲历华为IPD变革是怎样一种体验|附完整版培训教材
- P7276-送给好友的礼物【dp】
- c++经典编程题_【经典C语言知识】C/C++编程难点总结
- 04.React事件 方法、 React定义方法的几种方式 获取数据 改变数据 执行方法传值...
- 练习图200例图纸讲解_【宅家数学课23】经典微课6:苏教版六年级下册比例尺典型例题选讲及练习(含答案)...
- 获取spring里的bean
- 数字图像处理(三)——频域滤波
- SecureCRT 安装与配置大全
- 适用电商BANNER的超酷炫抽象系几何时尚流行系列,PSD炫彩流体海报模板。
- 谷歌修复已遭利用的 Chrome 0day
- SQL的简单增、删、改
- 【ISL-2】什么是统计学习
- 【转载】.NET系统学习----Assembly
- 【分享】哪个OS X版本支持哪个Xcode的版本?
- The Beatles Strawberry Fields Forever 歌词翻译
- cp:略过目录:”文件名“
- 使用代理爬去微信公众号_Python3WebSpider/9.5-使用代理爬取微信公众号文章.md at master · Lainton/Python3WebSpider · GitHub...
热门文章
- linux的内核有多小,Linux 内核有小bug?
- b样条曲面绘制 opengl_CAD制图软件中如何利用EXCEL输入坐标绘制曲线?
- MySQL 隐式转换 字符串和整型说明
- P2587 [ZJOI2008]泡泡堂 神仙贪心
- 2018.12.08 codeforces 946D. Timetable(背包)
- 【Java】BigDecimal
- 将数据库设置为运行在限制模式下
- 设计模式建议学习顺序
- java input回车,用java怎样编写加减乘除,从键盘输入,例如:1+2按回车得到
- Kinect开发笔记之一Kinect详细介绍