代码审查参考文档.docx

代码审查参考文档

代码审查（code review）是保证软件质量的一个重要环节，通过审查代码能够发现代码中可能存在的问题并给予纠正，这些问题可能包括设计上的、实现上的或者编程风格等多方面。本文档通过列举代码编写过程中的一些常见的细节问题，为代码审查环节提供参考。
Java代码
一、 对象和变量
1. 存在未被使用的变量
Eclipse会自动用下划线标出
2. 对象的重复创建
这是系统中普遍存在的问题，比如：
public class PrtGrpEndorsementBL {
private GlobalInput mGlobalInput = new GlobalInput();

private boolean getInputData(VData cInputData) {
mGlobalInput = (GlobalInput) cInputData.getObjectByObjectName(
"GlobalInput", 0);
return true;
}
}
这里mGlobalInput对象属于重复创建，因为在getInputData方法里会对它进行赋值，mGlobalInput使用的应该是从jsp页面传入的对象，所以改为private GlobalInput mGlobalInput = null;
又如：
String msg = "";
if (..) {
msg = "A";
}
else {
msg = "B";
}
这里msg同样属于重复创建，改为String msg = null;
3. 变量的作用域
Java的局部变量可以定义在函数的任何位置，有部分由c转学java的程序员习惯将变量都定义在函数的顶部，因为在c里只能那样定义。但实际上变量的作用域越短程序的内聚性就越高，耦合性也更低，程序更容易理解，因此在java里应该在使用前才定义变量。
4. 局部变量的危害
定义过多的不必要的局部变量是造成系统难以维护的原因之一，因为每增加一个局部变量我们就要先化时间去理解这个局部变量的意思，因此我们要减少局部变量的使用。用函数的返回值来替代局部变量是一种有效的办法，这就需要我们用重构的方式从大的函数中提出小的函数，用小函数的返回值来替代原有的局部变量。把大函数分解本身也可以降低程序的耦合度。
二、 常量
1. 硬编码，将代码写死
比较严重的情况是将险种代码写死在程序中，比如：
if ("21301".equals(tLCGrpPolSchema.getRiskCode())) {
...
}
这里将“21301”写死会为系统的维护和升级带来很大困难，最好是以险种定义的形式去动态获取险种代码。如果实在描述有困难，可以考虑建立一个Constants常量类，将其以常量的形式定义，这样以后维护只需改动一处即可。
业务系统中存在不少的状态标志，比如AppFlag的状态”0”为未签单，”1”为已签单，”2”为保全增人。虽然这些状态的意义日后已没有改动的可能，但直接将”0”、”1”、”2” 写在程序里会导致程序可读性差，因此我们同样可以将其定义在常量类中，并加上注释，这样我们不必每次都去翻看文档或pdm，看常量类里的注释就行了。
2. 禁用常量接口
Java的常量有一种不好的用法，就是将常量定义在Inferface中，这种方使定义的常量在使用时可以省去类名前缀，但会为以后的维护带来麻烦。因此不要试图用继承的方式去使用常量。
三、 数组
1. 数组的定义
数组的定义采用String[] str = null; 的形式，而不是String str[] = null;
2. 数组越界
数组越界是比较常见的错误，比如：
private void dealCont() {
LCContDB tLCContDB = new LCContDB();
tLCContDB.setGrpContNo(mGrpContNo);
tLCContSet = tLCContDB.query();
for (int i = 0; i < tLCContSet.size(); i++) {
LCContSchema tLCContSchema = tLCContSet.get(i);
LCPolDB tLCPolDB = new LCPolDB();
tLCPolDB.setContNo(tLCContSchema.getContNo());
LCPolSet tLCPolSet = tLCPolDB.query();
for (int j = 0; j < tLCPolSet.size(); j++) {
LCPolSchema tLCPolSchema = tLCPolSchema.get(i);
...
}
}
}
上面的程序就会产生数组越界，应该把tLCPolSchema.get(i);中的i改为j
数组越界的错误在编译时不会报错，直到程序运行时才会被发现。有一种方法可以避免此bug的产生，程序改为：
private void dealCont() {
LCContDB tLCContDB = new LCContDB();
tLCContDB.setGrpContNo(mGrpContNo);
tLCContSet = tLCContDB.query();

for (int i = 0; i < tLCContSet.size(); i++) {
dealPol(tLCContSet.get(i));
}
}

private void dealPol(LCContSchema tLCContSchema) {
LCPolDB tLCPolDB = new LCPolDB();
tLCPolDB.setContNo(tLCContSchema.getContNo());
LCPolSet tLCPolSet = tLCPolDB.query();
for (int i = 0; i < tLCPolSet.size(); i++) {
LCPolSchema tLCPolSchema = tLCPolSchema.get(i);
...
}
}
把内层循环提到一个函数中，在编译时就避免了bug的产生，而且降低了循环的层次，具有更好的可读性。

四、 静态方法
1. 静态方法的错误使用
com..lis.bq.ChangeCodeBL是保全中的一个工具类，它用两个重载了的静态方法getCodeName，它封装了CodeQueryBL类，其作用是根据code和codeType得到codeName，
但是在使用过程中却出现了如下程序：
ChangeCodeBL tChangeCode = new ChangeCodeBL();
strArr[0] = "与被保人关系原为" + tChangeCode.getCodeName("relation",
tLCBnfSchema.getRelationToInsured())；
显然，作为静态方法getCodeName不应该这样使用，应改为：
strArr[0] = "与被保人关系原为" + ChangeCodeBL.getCodeName("relation",
tLCBnfSchema.getRelationToInsured())；
之所以出现这样的错误是因为ChangeCodeBL在定义构造函数时出了问题，构造函数的类型写成了public，但这个类实际上是不允许创建对象的。所以改为private ChangeCodeBL(){}就可以从根本上避免错误的产生。
静态方法的另一个常见的应用是用来创建对象，比如工厂模式和单例模式，这时我们同样应该使用pritvate的构造函数。
五、 参数
1. 对参数进行校验
对于传入类的参数，我们需要对其合法性进行校验，比如：
private boolean getInputData(VData cInputData) {
LCGrpContSchema tLCGrpContSchema =
(LCGrpContSchema) cInputData.getObjectByObjectName(
"LCGrpContSchema", 0);
LCContDB tLCContDB = new LCContDB();
tLCContDB.setGrpContNo(mGrpContSchema.getGrpContNo());
mLCContSet = tLCContDB.query();
return true;
}
这段代码是存在风险的，因为我们并不能保证getObjectByObjectName一定会得到一个对象返回，如果"LCGrpContSchema"写错了或者jsp页面根本就没有创建LCGrpContSchema对象，那么tLCGrpContSchema就为null,执行到mGrpContSchema.getGrpContNo()时就会抛出空指针异常。
所以改为：
private boolean getInputData(VData cInputData) {
LCGrpContSchema tLCGrpContSchema =
(LCGrpContSchema) cInputData.getObjectByObjectName(
"LCGrpContSchema", 0);
if (tLCGrpContSchema == null) {
...
return false;
}
LCContDB tLCContDB = new LCContDB();
tLCContDB.setGrpContNo(mGrpContSchema.getGrpContNo());
mLCContSet = tLCContDB.query();
return true;
}
对于public的方法，我们同样需要对其参数进行校验，因为我们不能保证客户程序的调用是正确的。
2. 不对参数进行赋值操作
Java中只有int，long，double等基本类型的参数是使用值传递，而对象参数都是传的地址，因此在函数中对对象参数赋值是无效的。C语言中由于有指针可以对函数外的对象的值直接进行修改，但java不行，因此参数只作为数据的传入而不能作为数据的传出。
六、 代码简化
1. 除去多余的代码
比如：
LCContDB tLCContDB = new LCContDB();
tLCContDB.setGrpContNo(tGrpContNo);
LCContSet tLCContSet = tLCContDB.query();
if ((tLCContSet == null) || (tLCContSet.size() == 0)) {
return false;
}
这里tLCContSet == null的判断是多余的，因为tLCContDB.query()肯定会创建一个对象返回，不存在返回 null的情况，改为if (tLCContSet.size() == 0)
又如：
if (tLCPolSet.size() > 0) {
for (int i = 1; i <= tLCPolSet.size(); i++) {
tLPEdorItemSchema.setPolNo(tLCPolSet.get(i).getPolNo());
if (this.queryLPPol(tLPEdorItemSchema)) {
tLPPolSet.add(this.getSchema());
}
}
}
这里if (tLCPolSet.size() > 0)判断多余，将其除去
2. 合并重复的代码
比如：
if (getMoney > 0) {
strArr[0] = "本次申请应补交保费" +
setPrecision(Math.abs(getMoney), 2) + "元。";
tlistTable.add(strArr);
}
else {
strArr[0] = "本次申请应退还保费" +
setPrecision(Math.abs(getMoney), 2) + "元。";
tlistTable.add(strArr);
}
这里tlistTable.add(strArr);重复，将其提到if外面合并。
七、 条件逻辑控制
1. 去除控制标志
受结构化程序设计的影响，有人喜欢用boolean型的标志来控制程序的流程，比如：
private boolean checkMoney() {
boolean flag = true;
if (money > 0){
flag = true;
}
else {
flag = false;
}
return flag;
}
但逻辑标志的使用会降低程序的可读性，当逻辑复杂时修改维护会比较困难，所以改为：
private boolean checkMoney() {
if (money > 0){
return true;
}
else {
return flase;
}
}
或者：
private boolean checkMonay() {
if (money > 0){
return true;
}
return flase;
}
2. 位运算符
&和|都是位运算符，但它们也可在if语句中进行条件判断，但此时仍是执行的位运算。
因为使用&和|容易引起不必要的错误，所以禁止在条件判断中使用位运算符。

八、 异常处理
1. try{}catch{}语句的滥用
java的异常处理机制可以把错误处理程序和正常程序分开，使我们可以更容易进行复杂的错误处理。但异常处理机制容易被滥用，因为大多数的java书只是描述了异常机制的语法，而并没有说明什么情况下使用异常。异常之所以是异常是因为其不可预料性，比如我们通过io读取配置文件而文件并不存在，又比如通过jdbc读取数据库服务器却当掉了，这些都是需要我们通过异常机制来处理的情况。而下面的代码对异常的处理是错误的：
private boolean getInputData(VData cInputData) {
try {
LCGrpContSchema tLCGrpContSchema =
(LCGrpContSchema) cInputData.getObjectByObjectName(
"LCGrpContSchema", 0);
}
catch (Exception e) {
CError.buildErr(this, "接收数据失败");
return false;
}
return true;
}
因为我们只要看一下getObjectByObjectName的代码就知道这个函数如果处理错误会返回一个null而不是抛出异常，这里的try{}catch{}是没有任何意义的，而且会降低程序的性能，因为对异常的捕获会花去额外的开销。
2. 空指针异常
NullPointerException是比较常见的错误，因此我们在使用一个对象前一定要确保这个对象已经被创建过了。
3. 数据格式化异常
使用Integer.parseInt(String s)进行数据格式转化时如果数据格式错误会抛出NumberFormatException异常，这个异常继承于RuntimeException，是一个非强制捕获的异常。但从程序的健壮性考虑，如下代码我们最好能进行异常处理：
将int bnfGrade = Integer.parseInt(tLPBnfSchema.getBnfGrade());改为：

try {
int bnfGrade = Integer.parseInt(tLPBnfSchema.getBnfGrade());
}
catch (NumberFormatException e) {
...
}
因为这里我们并不能保证tLPBnfSchema.getBnfGrade()返回的一定是可以被格式化为整数的字符串。如果我们有足够的理由确保这里不会产生异常，那么异常处理也可省略，正如前面所说的，异常处理的是不可预料的错误。
4. 数组越界
数组越界异常同样容易被滥用，比如：
try {
int i = 0;
while(true) {
a[i++].f();
}
}
catch (ArrayIndexOutOfBoundsException e) {
...
}
这种用法是错误的，改为：
int k = a.length;
for (int i = 0; i < k; i++)　{
a[i].f();
}
　不要试图用异常处理去取代条件判断，我们要做的是把数组越界的条件排除掉而不是等它越界了再去处理异常。
九、 接口和抽象类
抽象和继承是面向对象的重要特征，使用接口和抽象类可以使得程序的实现细节得到封装，耦合度降低，更容易随着需求的变化而扩展。下面对一些需要注意的地方进行说明：
1. 只从抽象类继承
使用继承时要慎重，继承代表“一般化/特殊化”的关系，继承会使子类依赖于父类，如果父类的实现发生改变那么子类的实现将不得不发生改变。因此，在使用继承时一般不要从具体类继承，只有抽象类才用来继承，因为抽象类可以用抽象方法来提供扩展，而抽象类里的具体方法一定要确保是需求比较固定的部分，以后都不会有任何的改动。
2. 不覆写父类的具体方法
在子类中可以对父类的方法进行重载，也就是父类里实现了一个方法，子类用同样的函数名去重新实现这个方法。但这种做法是错误的，子类只应该去实现父类的抽象方法，而不是去实现父类已有的具体方法。如果需要这样做只能说明父类的方法不能作为具体方法而存在，将它改为抽象方法。
3. 不在接口中定义属性
这是保全中的一个bug。为了使保全算功能便于扩展，定义了EdorAppConfirm接口：
public interface EdorAppConfirm {
public CErrors mErrors = new CErrors();
public boolean submitData(VData cInputData, String cOperate);
public VData getResult();
}
然后再实现这个接口，比如：
public class GrpEdorNIAppConfirmBL implements EdorAppConfirm {
public CErrors mErrors = new CErrors();
public boolean submitData(VData cInputData, String cOperate) {
if (!dealData()) {
return false;
}
...
}
}
最后再调用这个接口：
Class tClass = Class.forName("com..lis.bq.GrpEdor" +
tEdorType + "AppConfirmBL");
EdorAppConfirm tGrpEdorAppConfirm = (EdorAppConfirm)
tClass.newInstance();
VData tVData = new VData();
tVData.add(iLPGrpEdorItemSchema);
tVData.add(mLJSGetEndorseSchema);
tVData.add(mGlobalInput);
tVData.add(tLPGrpEdorMainSet);
if (!tGrpEdorAppConfirm.submitData(tVData,
"APPCONFIRM||" + tEdorType)) {
CError.buildErr(this, "保全项目" + tEdorType +
"试算时失败！失败原因：" + tGrpEdorAppConfirm.mErrors.
getFirstError());
return false;
}
这里本意是如果算费出错return false并返回错误原因，但实际上不可能得到错误原因。
因为EdorAppConfirm和GrpEdorNIAppConfirmBL中都定义了mErrors，此时返回的是接口中的mErrors。如果把GrpEdorNIAppConfirmBL中的mErrors去掉可以解决这个bug，但是在接口中定义public的属性不是好的做法，因此可考虑把mErrors换成getErrors方法。

4. 接口和抽象类的组合使用
当我们准备使用抽象时会考虑是使用接口还使用抽象类，因为它们各有优缺点，接口和抽象类最大的区别是抽象类可以有具体实现而接口是纯抽象的。如果用抽象类那么写在抽象类中的具体实现代码的改动会导致各级子类的改动，如果使用接口避免了这个问题但又带来了另一个问题，就是各个实现接口的子类可能有重复的代码，如维护也需要维护多处地方。
因此比较好的方法是先定义一个接口，然后定义一个抽象类来实现这个接口，然后各个具体实现类继承这个抽象类，并把重复的实现代码放到抽象类中。这里要注意的是放到抽象类中的代码一定要是固定的，不会随需求改动的部分。

十、 其它需注意地方
1. clone()的使用
java的clone机制使我们可以很方便的进行使用对象的复制，只需实现Cloneable接口就行了。如：
public class MyObject implements Cloneable {
public Object clone() {
try {
return super.clone();
} catch (CloneNotSupportedException e) {
throw new InternalError();
}
}
}
但要注意的是java分为深clone和浅clone，它们区别是对象内部的成员属性在clone时是否处理为引用。如果仍然保留为引用，则称为浅clone，反之称为深clone。而java默认是浅clone，如果要实现深clone还需要对各个成员属性分别进行clone操作。

2. Javascript脚本的执行效率
IE执行脚本取object的length效率是很低的。不要在循环结构中的控制变量上直接使用“XX.length”，应避免如下程序段的写法：
var tFlag="";
for(i = 0; i <fm.QueryType.length; i++){
if(fm.QueryType[i].checked){
tFlag=fm.QueryType[i].value;
break;
}
}
应改为：
countOfQueryType = fm.QueryType.length;
for(i = 0; i <countOfQueryType; i++){
当例子中的QueryType对象数量超过15个以上时，操作用户会明显感到时间差异。
目前程序中存在比较多，且公共组件中亦存在。项目组最好派专人解决。

3. 注意数据库连接的释放
　　Connection conn = DBConntPool.getConnection();
//建立数据库连接
try{
if (conn == null || conn.isClosed())
{
conn = DBConnPool.getConnection();
}

if (conn == null)
{
// @@错误处理
CError.buildErr(this, "数据库连接失败");
return false;
}
}catch(Exception e){}

//在关闭连接的时候，同时将对象释放。
　　if( conn != null ) {
　　conn.close();
　　conn = null;
　　}
库连接与关闭的语句，最好写在同一个方法里，使程序易读性好，便于维护。
举例：
浪费资源的代码：
conn = DBConnPool.getConnection();
conn = DBConnPool.getConnection();
if (conn == null)
{
//return false;
}
tExeSQL = new ExeSQL(conn);
如果你发现程序中存在类似写法，请立即修改为：
tExeSQL = new ExeSQL();