Code Review 对于 MR 来说,不仅是一个工作流程,还是团队成员为了达到共同目标而一起努力的过程。
来自 GitLab Inc. 团队的一句话做了很好的总结:
go through a code review process to ensure the code is effective, understandable, maintainable, and secure.
Effective
“有效的”要求 MR 是正确的,能解决问题的,并且也需要在性能上过关。 这是 MR 的底线,达不到这一点的 MR,Reviewer 是一定不可以通过的。
Understandable
“可理解的”是在“有效的”基础上的要求。我们希望刚进入团队的新人也可以很容易读懂过往的代码。 Reviewer 需要换位思考,假设自己不了解背景的情况下,是否也能理解 MR 的改动。
Maintainable
“可维护的”的代码最能提现贡献者的编码水平。 这里隐含了一些要求,代码的组织方式是否符合约定,方法的颗粒度如何,是否可重用,代码是否容易扩展,是否容易测试,异常处理是否合适等等。 这需要 Reviewer 发挥自己的经验和智慧,引导贡献者向着更好的方向改进。
Secure
“安全的”要求贡献者保持良好的编码习惯,对可能暴露风险的场景添加测试。 Reviewer 需要了解常见的漏洞和攻击手段,为贡献者做好最后一道把关。
创建 MR 有非常多种方式,我们并没有要求统一从议题或者从分支创建。好的 MR 一般有以下特点:
尽可能使用英语
因为我们经常需要把 JiHu 的需求或者问题跟 Upstream 讨论,使用英语会大大提高跟 Upstream 的沟通效率。
清晰的 MR 标题
标题应该简明扼要地概括出 MR 的改动。我们希望从标题中读出,这是一个新增功能、Bug 修复还是功能重构。
请务必以大写字母开头,字符数控制在 3 到 72 之间,更多规则请注意 danger-review
的提示。
在准备阶段使用 Draft:
标记,准备完成后去除该标记再添加 Reviewer。
详尽的 MR 描述
请使用 MR 模板,并根据模板填写描述的每个章节。
好的功能 MR 应该包含以下描述信息:
好的 Bug Fix MR 应该包含以下描述信息:
恰当的标记信息
及时分配 Milestone 对追溯代码非常有好处,能方便地推断最早引入该 MR 的 release 是什么。
添加充分的 label
, 对复盘分析非常有用。
比如通过 fix:pipeline
可以过滤出所有修复 Pipeline 的 MR,然后对它们进行分类聚类,指导我们制定的下一步优化方案。
还有一些标记可以辅助调试,比如 pipeline:run-in-ruby3
和 pipeline:run-all-jest
。
好的提交是有组织的。
每个提交都有明确的分工,能达到分别 review 的程度。多个提交之间能表达出修改的渐进和依赖关系。
单个 MR 的提交数目不宜过多,建议不超过 10 个,强制要求不超过 20 个。
越大的 MR 越难 Review,Review 的周期会变长,引入 Bug 的可能性会加大。 单个 MR 的修改文件数不宜过多,建议不超过 10 个。对于超过 20 个文件变更的 MR,需要确认这样做的合理性和必要性。
创建 MR 时勾选 squash
选项,尽可能保证主分支提交的干净整洁。
提交信息编写指南 列举了一组提交信息规则,主要包含以下几点:
如果 MR 对用户的使用有影响,无论是新增去除或者修改功能,都应该书写 Changelog
,
详见 更新日志条目 。
我们有一篇文章专门介绍 JiHu Code Review 流程, 这里要讲的是流程之外还有哪些做法能让 MR 更好。
讨论式的留言
对于 Reviewer 来说,非常有把握的建议可以使用祈使句式,对于不了解或有疑问的修改,可以使用疑问句请求贡献者解答。 在 Code Review 中,"不求甚解" 是个贬义词,"是什么" 和 "为什么" 的提问能帮我们更好地梳理逻辑。
Reviewer 的提问会创建出一个 Thread,如果贡献者同意 Reviewer 的修改建议,需要在提交修改后 Thread 中标记本次修改的内容。 对于没有争议的小问题,贡献者可以自行解决,其他情况一般由 Reviewer 确认后解决。
如果贡献者不同意 Reviewer 的修改建议,需要明确当前方案的优势并展开讨论,直到跟 Reviewer 达成共识。 如果当前方案跟 Reviewer 的修改建议并没有明显的优劣,同时双方也未达成共识, 那么可以再邀请一位熟悉该逻辑的 Reviewer 帮忙裁决。
若始终难以抉择,那么选择跟现有代码风格更相近的方案。
主动自我留言
一般来说,贡献者比 Reviewer 更了解具体的逻辑和需求。 在开始正式 Review 之前,推荐贡献者先进行一轮自我 Review,并对可能产生疑问的方法添加留言。 这样 Reviewer 就相当于在阅读一份包含更多批注的代码,会加速 Review 的整体进程。
方法名应该简洁、清晰明了,能够准确描述方法的功能和用途。 一个好的方法名能够提高代码的可读性和可维护性,也能够帮助其他开发者更好地理解你的代码。
名词,使用专有名词,减少通用词汇产生的误解。
在 Controller 或 Helper 中,通常需要定义某种动作和过程,可以使用动宾短语,比如:
before_action :check_password_expiration, if: :html_request?
如果该过程会产生副作用,包括修改对象的数据,抛异常等,都应该使用叹号方法 !
命名, 比如:
before_action :authenticate_user!, only: [:new, :create]
在 Model 中需要面向对象的高内聚的命名,一般以名词、形容词来呈现。
比如:
def preferred_language
end
def is_admin?
end
得益于 Ruby 的灵活性,我们可以把对象内的变量和函数都视为对象的属性,以方法的形式来包装,既阅读通顺又灵活易改,比如:
def access_level
end
def access_level=(new_level)
end
Ruby on Rails 项目是典型的 MVC 结构, 又极力推崇约定优于配置的 Rails 信条, 在代码组织方面有比较明确的业界共识。
Ruby 这样的解释型语言,更有必要遵循约定的代码组织方式, 这样让人在不执行 Ruby 解释器的情况下,也可以方便并准确地找到方法的定义。
模型是对象的定义和描述。
模型不一定有对应的数据库表,能抽象成对象的都应该定义为模型。
视图负责展示。
模板文件中存放样式,不应该包含复杂逻辑,如果需要逻辑判断,则应该提取到 Helper 中。
控制器是内外沟通的桥梁。
它负责参数的安全过滤和流程的衔接,不应该包含复杂逻辑。
复杂逻辑应该在模型中实现,若果逻辑跨越多个模型,则可以创建更高抽象的模型。
多个控制器的公共部分,可以通过 concern 来重用。
好的代码让单元测试的编写更加容易,如果方法有合适的颗粒度,测试会显得更加有层次,方便控制变量,也更加容易读懂。 所以可以通过单元测试来反向评判逻辑代码的好坏。
RSpec 以可读性好表达能力强著称,我们也希望充分发挥它的优势。
具体来说,用 context
描述测试的上下文,应该包含正例和反例,用 it
描述具体的测试结果,它应该是一句通顺的自然语言(特别注意英语语法的时态)。
核心的功能很可能会与其他的功能有交叉,使用 shared_examples
能帮助重用测试用例。