关于代码Review

在一个项目组里面，经常会互相review同事的代码，review的过程中相信会产生不少矛盾，这篇文章记录一下我对代码review的一些看法。首先是编码规范的问题，拿PHP来说。比如命名规范，indent之类的。这些都应该在项目编码开始之前大家立好规矩。是采用驼峰命名法还是用下划线分割单词，indent�是用tab还是用空格，大括号是换行:

function foo() {
}

还是不换行：

function foo() 
{
}

这些最基础格式规范一定要在最开始写在一个wiki文档上，项目组所有成员都需要熟记这个规范。这种规范，两上个技术骨干定一下就好了，定下来就按它执行。这个东西其实只要统一就好，无需纠结，孰好孰坏也并不影响项目质量。

接下来我说一下我对关于一些实现逻辑的写法review的个人理解，比较经典的是关于 if...else...�的用法的讨论。如果一个函数中用if...else来做分支处理，代码如下：

if($flag = 1)
{
    .....
    $result = 'A';
}else if($flag = 2) {
    .....
    .....
    $result = 'B';
}else {
    .....
    .....
    $result = 'C';
}

.....
.....
return $result;

这种情况，如果if...else语句下面的处理跟逻辑跟结果A或者B没有什么关系的话 ，可以改写成如下代码，使得代码逻辑更加清晰

if($flag = 1)
{
    .....
    return 'A';
}
if($flag = 2) {
    .....
    .....
    return 'B';
}
.....
.....

.....
.....
return 'C';

简单一句话就是，适当的去掉if...else而换成if return结构，很多时候可以提高程序的可读性，但也不能认准这个死道理，比如下面这段（形式A）代码

    function foo($flag) {
        if($flag) {
            $var = 'A';
        }else {
            $var = 'B';
        }

        ....
    }

我的同事review的时候建议改成如下（形式B）：

    function foo($flag) {
        $var = 'B';
        if($flag) {
            $var = 'A';
        }
        ....
    }

这其实并没有意义，首先这并没有增加程序的可读性，其次形式A的代码其实更符合人类的思维，它表述出$var变量非A既B的逻辑，而下面的代码表述的是$var变量的默认值是A，当flag为true的时候它被赋值B，给人一种它可能还会有其他情况被赋值为C、D ... 之类的值的预期。

所以在review代码的时候不能教条（简单的认为只要去掉else代码就会变得更简单易读），review需要关注的点应该是是否有逻辑漏洞，潜在BUG，非常小可能性出现的异常没有处理。而对于代码的表述方式，不应该过多关注。所以反之若是有人写了形式B的代码，也没有必要在review的时候，让他改成形式A。如果review的这么仔细，不但不会提高代码质量，反而会大大减慢项目的开发进度，增加沟通成本。

所以，review这件事，要有度！