8. 代码审查

代码审查是通过直接阅读代码的方式来发现代码的问题以及优化代码，通常我们也称为code review。代码审查是主程序的必修课，它可以有效规避代码风险以及建立团队的代码通识。有些项目会采用团队成员互相进行代码审查的方式来共同成长与提高。

我们来实例看一个 code review 的例子，使用白鹭引擎开发的一个拼图游戏：

我们第一步先不着急看代码内容，先看文件的规划（如下图所示）。下面是一个小游戏的代码文件，它实现的目标是一个拼图逻辑。

从名字我们可以看出，这里面好几个文件都是做配置的支持的。这个文件的规划有3个问题：

1. 一眼看不到核心实现文件。

2. 配置的支持应该放到一个单独的文件夹。

3. 配置的支持应该尝试用一个类来实现。

接着我们看内容，我们先来看一下Main文件，Main文件是入口文件（如下图所示）。

看到一个奇怪的地方，入口文件里面定义了一个逻辑变量mapId。入口文件应该干的事是一些比较重要的，在项目早期就必须提前执行的逻辑，它不应该和游戏的核心逻辑产生任何的关联，仅仅应该是在适当的时候将控制权交给游戏逻辑层。

接着往下看，到了变量命名的地方（如下图所示）：

这边的命名是很糟糕的。对于2个不同单词的分割，有些直接拼合，有些用下划线分割，有些用大写分割，没有做到统一。统一是非常重要的，就像军队的纪律一样。另外一个我们应该需要注意的是，不是object的对象类型，也就是那些基础类型，如boolean，number等我们应该赋予它一个初值。上面的mapId我们应该设置一个0，防止对一个空对象尝试误用。

接着我们看一个函数的实现(如下图所示），这个函数的实现过度复杂了。

这段代码很好读懂，就是创建了一个黑色的背景。创建一个黑色的背景从直观的理解上，应该是一个简单的操作，那么它如果需要写5行代码，那么需要怀疑是不是实现复杂了。另外一方面，有些程序会用一张黑色的背景图来拉伸实现这个效果，也可能实现过度了。我们在unity里面经常为了防止点击到窗口后面的东西，我们会加一个透明的背景来接收鼠标事件，这也可能是实现过度了。实现过度指的是使用相对消耗的方式来处理一个简单的逻辑。

接着我们再看一个函数的实现（如下图所示）。函数的名称不重要，我们关注函数的内容。

在脚本语言中，从this上面去取数据是一次的查表操作，查表操作对于脚本语言而言是存在一定耗时的，尤其当数量变多了之后。所以上面的this.blockMap应该先用一个局部变量缓存住，以减少查找的次数。另外一个this.blockMap[k]，因为它的使用次数介于1-2之间，所以它不一定需要用局部变量缓存。局部变量缓存加快的速度，但也可能造成gc的负担。本质上它是一个空间换时间的策略，而在游戏里面，CPU相对内存一般是CPU先达到瓶颈，所以我们经常会优先关注时间的实现效率。

接着我们看到一个函数的内部，进行了取配置的操作（如下图所示）。

这里，先取出一个管理器对象。然后直接访问了它的成员函数，这是非常不应该行为。第一个是这个成员不应该是公有的，而应该是私有的，它的公有性质破坏了类的封装。我们应该用提供接口的方式来替代这一种写法。这种写法最大的后果是当它出现异常的时候，我们无法通过单一的接口来排查它的修改来源。切记我们不应该直接访问m_或者$开头的私有变量，除非迫不得已。迫不得已打个比方说它是被引擎包在了内部不好，这种就不好直接修改引擎提供接口，就只能直接去访问了。

我们写很多逻辑的时候，常常可能使用过于复杂的方式去实现一个简单的逻辑，下面这个图就是这样的一个案例。

这个地方有个明显的逻辑设计问题。我们从名字上看，这个过程是在鼠标选中某个物体的，使其他物体的点击响应失效。我们应该建立一个认知，我们在处理一个物体的时候应该尽可能不要去主动操作别的同级物体，注意这边的同级2字。那么我们有一些别的方式来实现，比如说在onTouchBegin的时候把它置为同级物体的最上层，这样事件就会被它先截获，也不用去关闭其他所有物品的事件响应了。

下面一个是代码实现的效率问题，在for循环里面（见下图）。

for循环里面常见的漏掉break。虽然代码也能正常工作，但是效率不够高。

上面我们看的是一个游戏职场新入门的程序写的代码，下面我们来review一点引擎代码。以白鹭中的一段引擎代码来举例，我们来看看如何去审查它的问题。

Timer.prototype.$update = function (timeStamp) {

var deltaTime = timeStamp - this.lastTimeStamp;

if (deltaTime >= this._delay) {

this.lastCount = this.updateInterval;

}

else {

this.lastCount -= 1000;

if (this.lastCount > 0) {

return false;

}

this.lastCount += this.updateInterval;

}

this.lastTimeStamp = timeStamp;

this._currentCount++;

var complete = (this.repeatCount > 0 && this._currentCount >= this.repeatCount);

if (this.repeatCount == 0 || this._currentCount <= this.repeatCount) {

egret.TimerEvent.dispatchTimerEvent(this, egret.TimerEvent.TIMER);

}

if (complete) {

this.stop();

egret.TimerEvent.dispatchTimerEvent(this,egret.TimerEvent.TIMER_COMPLETE);

}

return false;

};

上面展示的是白鹭引擎 Timer的update函数，我们来看看里面有哪些值得写好的地方。

1. 变量使用2种命名方式 lastTimeStamp 与 currentCount。

2. this.lastCount -= 1000; 硬编码的1000，无法快速读懂含义。

3. var complete = (this.repeatCount > 0 && this._currentCount >=this.repeatCount)； 与使用 complete的地方没有挨着一起（中间隔着处理的别的事情，影响代码阅读）。

4._delay看起来是延迟相关，配合return false来实现延迟处理一个事情，实现方式读起来很困难。

5. 多次使用this.lastCount，没有进行合理的引用，也就是

var lastCount = this.lastCount;

我们的code review就暂时进行到这里了。可以看到，一份代码很容易出现各种各样的问题。这些问题有大有小，之所以我们都拿出来说的原因是这些代码经常是被复制黏贴，导致一个不够好的代码出现在各个地方。当然，最重要的是我们要培养自己的条件反射意识。比如说创建了对象就要关注销毁，遇到for循环就要找找是否有地方可以提早break。不停的培养代码意识是提高代码的有效方法。如果是主程或者高级工程师，那么可以开始看看项目中的其他成员的代码来训练code review的能力。