代码中的坏味道

最近InfoQ上连载了郑烨 写的《代码之丑》系列文章 ，好评不断，其实早在InfoQ开始连载前，我就在他的博客 上看到过了，当时就觉得这个系列写得很实在，应该让大家都知道什么样的代码是有问题的。

 

说起遗留代码，大家脑子里就会反映出代码质量差、难以理解等不好的印象，其实这些代码也是大家写出来的，它们也有还是新代码的时候，也许这段“遗留代码”就是两个月前你自己写的。

最近负责做新员工转正前的代码抽查工作，按照公司的简版代码规范对代码质量做评估，看了不少代码，大多数都能符合规范，但我却不能说这是好代码，因为当中充斥着不少坏味道。下面就列举了5个问题，与大家分享一下：

1、if的陷阱

这应该是个老生常谈的问题了，当if后只有一句语句时，可以不加大括号，其危害相信大家都知道。像FindBugs 这样的工具也会对此类代码做出提示（这次貌似FindBugs失灵了，估计是规则的问题）；此外，清晰的代码缩进，也可以避免由于没有大括号而造成问题。

 

if (flag)
  doSomethingA();
doSomethingC();

 

希望有人要加doSomethingB()的时候能先想清楚怎么加。

2、OO的封装性

 

public class ClassXXX {
  private Abc abc;
  private Def def;
  Hij1Impl hij1;
  private Klm klm;

  ...
}

 

这是一个Spring的Bean，刚看到这段代码时，当中那个hij1是不是出于什么特殊的原因而没有声明为private（要么大家都没，要么就都有，唯独它没有，就有点问题了），但询问了开发的同学后，他也说不出什么原因。

 

OO的三大基本特征之一就是封装，其他两点是继承与多态，如无必要，还是不要打破封装性，把对象的内部特征暴露在外。

 

3、依赖于接口而非实现

 

同样是上面的代码，这位同学还是很好地遵循了代码命名规范，于是一眼就看到了那个Impl，说明这是一个实现类，而非接口。我们提倡“面向接口编程”，这行代码的上下几行都是声明的接口，而唯独这行注入实现，我再一次询问了写这行代码的同学，这次有理由了：

这个Hij接口有两个实现类，分别是Hij1Impl和Hij2Impl，分别对应两种不同的功能，这个类里需要注入前者。

 

我听完的第一反应就是他没有把Spring Bean的注入搞清楚，这里还是应该声明为Hij接口，然后：

• 通过<property>配置 ，直接显式地注入所依赖的Bean。
• 使用Spring的Autowire机制 ，并且明确使用byName，而非byType；此处要注意两个Bean的id。

4、消失的异常堆栈

 

当遇到异常时，有两种处理方法：通过try-catch直接处理，或者向外抛出异常。总之最终要以一种合理地方式解决异常，同时还必须要打印错误日志，虽然这不会影响程序的运行，但对问题排查大有帮助。

 

在一个SpringMVC的Controller中有这样一段代码：

 

@RequestMapping
public Stirng processXxx() {
  try {
    doSomething();
  } catch(Exception e) {
    return "redirect:" + systemExceptionView;
  }
  ...
}

 

乍一看，发生异常后显示一个系统异常页面，处理异常了，里面的内容可能是“啊呀，出错了”，或者“系统发生异常，请稍后再试”等等），但这些内容对排查问题没有任何帮助。究竟是什么原因导致系统异常的？

 

应该用Logger以合理 的日志级别记录下错误堆栈，切记是日志工具（Commons Logging、Apache Log4J、Slf4J等），而不是向System.err输出，或者用e.printStackTrace()；而且应该如下形式，不是“消息”+e：

 

try {
 doSomething();
} catch (Exception e) {
  logger.warn("错误消息描述", e);
}

 

 

5、过多的嵌套与庞大的类

 

try {
  if (conditionA) {
     doSomething1();
  } else if (conditionB) {
     if (xxx==null) {
       doSomething2();
     } else {
        doSomething3();
        if (conditionC) {
          for (String str : strList) {
            List<Item> itemList = getItems(str);
            if (conditionD) {
              for (Item item : itemList) {
                doSomething4();
              }
            }
          }
        }
     }
  }
} catch (Exception e) {
  cleanUp();
}

 

是不是看晕了，这里只是一个演示，真实的代码更恐怖，外面还有两层if，这两层if又是在一个else里，这还只是一个方法，就有300多行，整个类有1300多行，里面有140多个if，就是一个不折不扣的大泥潭，应该对它进行重构。我相信有人看到过更大的方法，更大的类，但作为一个月前写的新代码，这个坑未免太大了一点。

 

当时代码是做了CodeReview的，也指出了这个类过于庞大，需要重构，可是鉴于页面复杂，项目开发后期重构对项目存在风险，建议项目后另外重构。这个类成功地实现了功能，可是这代码却很难读懂，如果在编写时不能重构它，那等到它变成“遗留代码”时，重构的成本就会翻上几倍，真希望自己能在它编写的时候就看到这段代码。

 

魔鬼往往隐藏于细节之中，代码中的坏味道也藏于细节之中，迟早它会跳出来给你当头一棒。《细节决定成败》中有这么一句话：“一个企业家要有明确的经营理念和对细节无限的爱”。对于开发人员，这句话也同样适用，尤其是“对细节无限的爱”。

 

（注：此处均非正式代码，仅做演示）