code review的思考和实践

YoungJeff
·发布于 8 个月前·
56 人看过

前言

关于为什么要做code review和它的好处就不过多赘述

就一个问题:你是否碰到维护团队项目,无从下手的情况,需求可能只需要改动很少一块代码,但是看着代码你还是踌躇了,受限于开发周期和改动后的风险不可控你又不敢轻易重构,但是你的职业素养又不想任由代码朝着不可维护的方向发展。
在你不知道抓掉了多少头发后,本着代码只要能跑就不要动的原则和需求方的督促,你选择了妥协,很长时间后,接手项目的小伙伴不得不重构项目,开启新一轮的循环,如此往复。

开发周期紧凑,人员流动大,每个人的水平和编码风格又不同等等因素,都会造成以上结果,至此,code review的重要性不言而喻

目前存在的问题

我们小组在此之前没有过code review,但是公司其他小组有进行,这就让我能够借鉴他们的流程。

目前我们的开发流程是:长期存在的分支只有master,大家从master切出自己的开发分支,开发完成后合并到test分支供测试环境使用,然后测试通过,把自己的开发分支合并回master分支打tag上线。
另外,由于我们目前没有开发环境,所以测试环境也充当了开发环境的角色,必然会频繁提交代码。

基于以上,现有的code review就是向master发起合并请求的时候进行;
在我看来,在最终要上线了合入master分支才进行review是滞后的,试想一下,你某个大需求,改动了很多代码,commit了很多次,又要着急上线了,谁有耐心帮你review呢,又或者你大半夜修了个bug,要上线,谁来帮你review,这些因素都会造成review成为摆设。

我的思考

一)应该在哪阶段进行

所以,code review的时间应该提前

我认为,在合入主分支之前,且功能可用稳定的状态进行比较合理,又考虑到我们的测试环境会频繁提交调试代码,所以就需要在master和test之间单独有一个分支用来作为review分支,所以我想到了gitflow

二)如何配合开发流程

Gitflow是基于git的一个流程规范,(这里只简单介绍下,不了解的小伙伴请自行搜索)。
gitflow常用分支有master,develop,feature,release,hotfix;
其中master和我们熟知的是同样的用途,包含要发布生产的稳定代码,不能直接在上面修改;
Develop作为主开发分支,包含要下一次发布测试环境的代码;
而featrun才是我们的开发分支,开发完成后合并到develop(通过gitflow finish命令会自动合并),然后从develop切出release分支用于测试,测试完成后,通过finish命令,自动合并到develop和master

所以,gitflow中长期存在的分支有两个分别是master和develop,develop分支刚好就是我要用的,但是要稍作流程上的变动;

Gitflow中开发功能稳定后才会往develop合并,然后再把变更后的develop合到release分支。
而我们团队又会频繁往测试环境提交调试代码。
所以,我们继续采用团队现有的流程:开发分支直接往测试分支(test)合并发布测试环境,在此过程中,自我评估是稳定的功能代码(非调试代码且不会再有大的改动)后再往develop分支合并进行review,develop的单一职责就是review,且和master一样长期存在。

并且,因为master分支并不是所有人都有权限,而develop则不受权限影响,任何人都是可完成review后并且审核通过

三)review的频率和节奏

  • 在日常维护修改上,应最少每天提交一次review

  • 在新功能开发任务或者改动比较大的需求上,应该拆分需求多次commit,且每次commit改动范围要尽量少且具有关联性,并且commit信息要尽量清楚描述改动内容和范围,参考社区规范(比如:https://zhuanlan.zhihu.com/p/90281637

  • 如果遇到紧急情况,比如半夜修改了一个bug其他人没在线,则也正常往develop发起pr,事后进行复盘

四)review需要看什么

本着能用工具解决的,就不要人工操作,像代码风格和基本的质量问题,则可以通过prettier和eslint这类工具来保证

包括不限于:

  • 减少无用代码(console.log,注释且确定用不着的,大量重复代码等),重复代码是否过多(维护代码很大一部份痛苦来源于此),该封装就封装

  • 对于重要且复杂的业务逻辑,或者采用了一些奇淫技巧,是否有清晰的注释

  • 避免重复造轮子,像一些工具函数,可以用别人的,就别再自己写了,自己写的不一定有别人的健壮(比如,lodash库),另外对于别人封装的公用方法,如果加一个参数就可以实现自己的需求,就不要自己在写一份;

  • 是否彻底解决了某个bug,是否有漏掉的场景

  • 避免硬编码,用了ts就要写清楚类型,没用ts就多用常量和注释,提高代码可读性

  • review的目的是为了提升大家的编码质量,和减少维护成本,所以不要陷入无谓的扯皮,审核者对于需要改进的代码要给出优化建议,当事人如果觉得不妥也可以一起讨论,总之最终结果是大家集思广益写出更好的代码

五)checklist-具体代码指南

应该由大家共同维护这个checklist,比如踩过的坑哪些代码实现更优雅等
比如:

  • 数组中的哪些方法性能更好
  • 什么习惯容易内存泄漏
  • 存在前后关系的promise,使用Promise的静态方法
  • 函数功能职责要尽量单一,对于一些公用方法,粒度可以尽量小

等等

cheklist应该是随着review进行而不断完善

总结

完整流程采用git flow+gitlab的Pull Request模式来进行

1.先基于master切出一个develop分支作为长久存在的review分支

2.基于master分支切出自己的开发分支(feature)和测试分支(test)

3.featrue分支开发完成后往test合并发布测试环境,待功能稳定后,再往root的develop发送合并请求,合并请求小组内成员都可以review,但是为了避免3个和尚没水喝的情况,每个人都对应一个兜底的审核者;(合并请求可以通过接入企业微信机器人实现通知)

4.因为每次develop分支是从master分支切出来的,所以为了保证同步,每次上线功能分支往master合并的时候,也要一并把master往develop合并;

5.如遇紧急情况,无法review,可以记录一个todo(比如,在tapd上建一个任务),方便事后追踪

gitlab webHooks接入企业微信机器人

  1. 企业微信添加一个企业微信机器人,拿到web hook地址

  2. 搭建一个node服务,通过webHooks调用该服务的接口,发送告警消息到企业机器人。
    为什么需要node服务?因为gitlab 调用 webhooks 发送的数据报文格式和企业微信机器人要求的数据报文格式不一致。
    此时就需要搭建一个 Node 服务,将 Sentry 的数据报文做下转换,并按照企业微信机器人的数据报文格式进行发送。

代码示例:

const Koa = require('koa')
const Router = require('koa-router')
const axios = require('axios')
const dayjs = require('dayjs')
const bodyParser = require('koa-bodyparser')

const mobileMap = {
  '张三': '1888888881',
  '李四': '1888888882',
  '王二': '1888888883',
}

const app = new Koa()
const router = new Router()


// 响应
function response(data = '', msg = 'OK') {
  return JSON.stringify({ code: msg === 'OK' ? 200 : 0, msg, data })
}

function createFontTag(content, color = 'warning') {
  return `<font color="${color}">${content}</font>`
}
function createReviewInfo(reviews,mobileMap=[]) {
  if (!reviews.length) return ''

  const mobiles = reviews.map(name => `<@${mobileMap[name] || name}>`).join(' ')

  return ` 请${mobiles}查看并审核代码`
}
router.post('/hook/merge', async (ctx) => {
  const { repository, user, object_attributes } = ctx.request.body
  const { target_branch, title, url,action, } = object_attributes

  if (action !== 'open') {
    ctx.body = response('完成合并不发送')

    return
  }
  // 监听对应的分支
  if (target_branch !== 'develop') {
    ctx.body = response('不是监听对应的分支')
    return
  }
  const reviews = Object.keys(mobileMap).filter(item => item !== user.name)
  const localeString = dayjs().format('YYYY-MM-DD HH:mm:ss')
  const content = `${createFontTag(repository.name)} 代码合并请求
    用户 ${createFontTag(user.name)}${createFontTag(localeString)}${createFontTag(target_branch, 'info')} 分支发起
    标题为 ${createFontTag(title)} 的代码合并请求
    [查看合并代码](${url})${createReviewInfo(reviews,mobileMap)}`
  sendMessage(content)
  ctx.body = response()
})
function sendMessage(content) {
  const options = {
    url: `www.baidu.com`, // 企业微信webhook地址
    method: 'post',
    data: {
      msgtype: 'markdown',
      markdown: { content },
    }
  }
  axios.request(options)
}

app.use(bodyParser())

app.use(router.routes())

app.listen(9100, () => {
  console.log('Server is listening on port 9100')
})

  1. 把node接口填到gitlab中项目中的web hooks中
    注:因为我们需求是发送合并请求的时候,通知机器人,所以trigger勾选merge Request events;
    其他项目根据自己需求勾选。

1.webp

测试结果如图,每次合并请求都会@除提交人以外的小组人员

2.webp


以上

代码规范
$ cd ..