代码审查解决方案

最近在推进公司的代码审查. 经过几天的研究之后,整理出一些问题与解决方案.

当前面临的问题

• 没有一个好的工具方便代码审查
• 没有一个好的代码标准代码审查
• 没有一个流程强制执行代码审查
• 没有一个固定职责的人来代码审查
• 代码审查之后的修改跟踪问题
• 代码审查者与代码修复者鼓励机制
• 跨团队资源调配的时候,容易造成过多的代码

解决方案

加入Merge Request工作流

在Gitlab上做代码审查,我们需要在我们现有的git工作流上加入Merge Request.

工作方式

Merge Request可以和功能分支工作流、Gitflow工作流或Forking工作流一起使用。过程是这样的：

• 开发者在本地仓库中新建一个专门的分支开发功能。
• 开发者push分支修改到公开的Git仓库中。
• 开发者通过Git发起一个Merge Request。
• 团队的其它成员代码审查，讨论并修改。
• 项目维护者合并功能到官方仓库中并关闭Merge Request。

结合到我们现有工作流当中

我们现在代码管理的工作流叫: Gitflow工作流

Gitflow工作流和功能分支工作流类似，但围绕项目发布定义一个严格的分支模型。

在Gitflow工作流中使用Merge Request让开发者在发布分支或是维护分支上工作时，可以有个方便的地方对关于发布分支或是维护分支的问题进行交流。

Gitflow工作流中Merge Request的使用过程：当一个功能、发布或是热修复分支需要Review时，开发者简单发起一个Merge Request，团队的其它成员会通过Bitbucket收到通知。

新功能一般合并到develop分支，而发布和热修复则要同时合并到develop分支和master分支上。Merge Request可能用做所有合并的正式管理。

审查流程

开始=>start: 代码作者通过git 克隆代码到本地
结束=>end: 结束代码审查
修改代码=>operation: 修改代码并且提交
pr=>operation: 发起 Merge Request
通知=>operation: gitlab 通知相关审核人员
审核=>operation: 审核人员开始审核代码
关闭=>operation: 关闭Merge Request
合并=>operation: 合并代码
代码审核中=>condition: 是否过审

开始->修改代码->pr->通知->审核->代码审核中
关闭->合并->结束
代码审核中(yes)->关闭
代码审核中(no)->修改代码

结合GitLab做代码审查

项目角色介绍

角色权限

我们可以看到Master跟 Owner才有权利把代码合并到受保护的分支上,对于Master角色的分配需要谨慎.

锁定受保护分支

• bugfix_*
• develop_*
• release_*
• master

多人 Review

• 提交 Merge Request 后，被指派人可通过 @你想要通知单人 邀请一个或2. 多个额外的 Reviewer （它们会收到邮件通知）
• 被邀请的 Reviewer 看过代码后， 可回复 顶 或+1表示通过，反之给出修改建议。
• 升级GitLab 版本,在最新版本中,GitLab已经支持群组审查,当前版本还是单人审查.

代码审查效率问题

为了避免我们在提交代码之后,一个Merge Request很多天都没有人审查, 我们应该有相应措施:

将审查任务分派给不同的人 审查者给出反馈的最迟时间 标识某个审查已经完成 指定负责人合并已经完成审查的代码

什么样的项目什么时候适合代码审查?

至少有5名开发人员 有新的小伙伴加入团队时 团队中有些成员对于当前所使用的技术栈还不太熟悉时 该版本有其他团队成员协助开发的时候 团队还在探索最佳实践的时候

代码审查清单

代码审查我们都在做什么? 我们需要审查点在哪里?

下面列出一些清单,提供大家在代码审核中作为参考点.

常规项

代码能够工作么？它有没有实现预期的功能，逻辑是否正确等。 所有的代码是否简单易懂？ 代码符合你所遵循的编程规范么？这通常包括大括号的位置，变量名和函数名，行的长度，缩进，格式和注释。 是否存在多余的或是重复的代码？ 代码是否尽可能的模块化了？ 是否有可以被替换的全局变量？ 是否有被注释掉的代码？ 循环是否设置了长度和正确的终止条件？ 是否有可以被库函数替代的代码？ 是否有可以删除的日志或调试代码？

安全

所有的数据输入是否都进行了检查（检测正确的类型，长度，格式和范围）并且进行了编码？ 在哪里使用了第三方工具，返回的错误是否被捕获？ 输出的值是否进行了检查并且编码？ 无效的参数值是否能够处理？

文档

是否有注释，并且描述了代码的意图？ 所有的函数都有注释吗？ 对非常规行为和边界情况处理是否有描述？ 第三方库的使用和函数是否有文档？ 数据结构和计量单位是否进行了解释？ 是否有未完成的代码？如果是的话，是不是应该移除，或者用合适的标记进行标记比如‘TODO’？

测试

代码是否可以测试？比如，不要添加太多的或是隐藏的依赖关系，不能够初始化对象，测试框架可以使用方法等。 是否存在测试，它们是否可以被理解？比如，至少达到你满意的代码覆盖(code coverage)。 单元测试是否真正的测试了代码是否可以完成预期的功能？ 是否检查了数组的“越界“错误？ 是否有可以被已经存在的API所替代的测试代码？

微信公众号

本文链接：