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 上来紧急修复。