2018
Apr
16




Code Review 是一件很重要的事,很多 Bugs 都可以经由 Code Review 而发现,学会怎么 Review 别人的程式更是一门学问, 每个专案大小与未来性不同,不能永远都使用同一套标准,小专案或是暂时的 Script ,不需要看得太细,只能程式能动就好,而目标是跑 5 年以上的大专案,就得多花点眼力来检查,除了 review 程式逻辑之外,其它还有很多地方要特别留意,以下是我多年来 code review 的经验。

  • Beta / Production 环境:一般软体开发都会有区分多个环境,最少有 Beta(staging) , production 两个环境 ,工程师很容易只针对其中一个环境来开发测试, Commit 的程式也只测过 Beta,并没有想到 production 的部分 ,所以 review 时要看看新增的程式是否有处理不同环境的设定,最常见的就是加了一个 Beta 测试用的设定值,而少加了 production 所需的设定。
  • 是否有 Require library, Import lib path,有些工程师写程式都不测试的,程式逻辑写完了,却没有载入对应的 library ,一执行就直接挂点,建议要求所有 RD 写 unit test or Integration test 来避免这种问题,若是公司内没有这些规定,那么 review 时就多看两眼。
  • Package dependency:如果有升级 Library / Package 要记得改版号,工程师很常自已的环境测试完,就忘了改 pom.xml / package.json 等版号。
  • Reuse function: 想想看是否有其它工程师曾经实作过相同的程式逻辑,尽可能重复使用同一段程式,不要写一模一样的 code。
  • 极端值: 程式是否能够处理各式各样奇怪的数值,例如负数,0 ,big number ,null,空字串,undefined,large array。
  • 可读性:保持程式简单易懂,如果程式逻辑著实复杂得夸张,可以把复杂而且不常更动的部分打包藏进 Library 里,让外部程式执行流程干净好懂 (Facade pattern)。
  • 程式是否会影响到其它的 service ,input / output 改变或是底层 library 变更,有可能会产生其它服务 Bugs 。
  • 程式一致性:不要创造新的变数名称,保持原来程式的命名,但是如果原程式命名有很大的问题,那就把有问题的变数全部改掉,不要只改你有动到的地方。
  • 把自已当成一个测试工程师,测试一下程式逻辑是否有 Bug。
  • 想一想是否有机会提升效能,虽然程式不一定有效能问题,但是多研究效能方面的知识,可以提升自已的能力,以应付未来流量更大的系统。
  • 是否有安全性漏洞,例如 SQL Injection, XSS, File Injection, Buffer overflow。
  • 前置作业: 新程式是否需要搭配修改 DB Schema,或是必需先修改 production settings,是否需要多增加一个开关设定上线日,时间到才自动启用新功能,如果没有事先设定好这些,程式 release 当天很可能就立马倒站,以下几种状况都会造成系统出错。
    • 修改 Code SQL 语法,但是 DB Schema 没有改。
    • 程式需搭配 DB table index,如果没有先建好 index 会造成 Slow Query 。
    • Production 需增加新的设定,而设定与程式逻辑分别在不同的 Repositories,甚至是安装在不同的机器,无法同时 Release 。
  • Release:程式是否能够 release ,有些功能还在开发当中,不能公开给外部的 User 使用,如果 merge 这段程式,是否会造成提前 release 。
  • Rollback 机制:如果新版程式在线上运作一段时间后,出现问题要如何 rollback。
  • Release 流程:一般线上机器不会只有一台,一般来说至少有 3 台以上,在 Release 的过程中,会一台一台慢慢安装,造成线上有一部分机器是旧的程式,一部分机器已经安装新的程式,新机器建立的资料格式,旧版的程式无法正确处理,而造成系统 crash。
  • 资料格式改变:移除即有栏位,很可能会造成系统错误。
    • Cache 内容变更:memcached, redis, http cache, browser cache, session cache 以上 cache 都会存在系统一段时间,资料格式改变时,并不会把即有的 cache 一并更新,这时线上会同时存在新旧两种 cache,需确定程式读取 cache 时,不会因格式改变而出错,另外在测试的过程中就有可能因为新版的 cache 资料造成线上环境出错,很可能在 review 的当下就已经有新版的 cache 格式出现在线上环境。
    • 回传格式改变,前端是否有相对应的变更,Release 流程是否要调整。
    • 是否需要 On the fly data migration
  • Merge 顺序:如果同时有多个 Pull Request,每个 PR 相依性是否有正确设定,底层 Library 优先 Merge 。

良好的 Code Review ,可以让系统更加稳定,不会因为突发性的 Bug 而被 Call 上来紧急修复。


目前回應 Comments(1 comments)

  • Aha 2018/08/30

    I don't agree with you.

    Reply

    Admin

    Don't agree all or part of these?

回應 (Leave a comment)