工程中Java Code Review发现的问题汇总

概述

最近对团队内近期开发的一些Java web工程进行了Code Review，这些Code主要是需要在多个工程中复用的基础组件，Java代码为主。审核中发现了一些编码问题（暂时不考虑设计模式、架构层面的），这里进行一下汇总总结。

问题列表

注释

普通的程序员最痛恨接手或使用没有文档的代码，而程序员一般又不喜欢些文档，代码注释是文档的一种，在Code Review中发现工具扫描时注释率很高，但是真正进去去看，注释率极低。

1. 接口一定要有详细的注释，public的类至少要有类注释，类注释中需要写明作者、修改日期。而下面那种空注释，一般是Eclipse自动生成的：

   /**
   *
   *
   */
   public interface WaterMarker {

   ......

   }

   建议Eclipse中配置Types的代码Templates为：
   

   /**

   • ${tags}
     
   • @author ${user} ${date} ${time}
   • @version 1.0
     */
2. 对于Java bean，Getter/Setter中如果没有注释，那么就需要给Bean的成员变量加上注释，特别是如果有些变量取值范围是有一定限制的话。当然在表示限制的时候需要能准确表达，比如数值是开区间还是闭区间，“(0,1)”和“[0,1]” 表达的意思就有区别。
3. 按照本人经验，一般应用系统代码不被其他工程代码调用的话，15%-30%的注释率是正常的，如果是工具形式的代码会被其他工程的代码直接调用，那么注释率要在20%-50%是合理的。这里的注释率统计中不包含自动生成的代码。当然不同人心里可以有不同的衡量标准。

Java Bean规范

Java Bean有自己的规范，一般代码中出现大量Getters/Setters，就算作者不认为自己写的是Java bean，为了防止调用者误解，也得符合Java bean规范，除非从命名上让人一眼看出来这个不是Java bean。下面代码就是一个getters/setters的反例：

public class TextWaterMark extends AbstractWaterMark {
    private String fontColor = "black";

    /**
     * @return the fontColor
     */
    public Color getFontColor() {
        if (this.fontColor.equals("white")) {
            return Color.white;
        }
        ......
        return Color.black;
    }

    /**
     * @param fontColor
     *            the fontColor to set
     */
    public void setFontColor(String fontColor) {
        this.fontColor = fontColor;
    }
}

上面代码中fontColor是String，而getFontColor返回值却是Color，不符合Java bean的规范。并且，相对于java.awt.Color的表现能力来说，这里的fontColor只能表现少数的十几种颜色，削弱了表现能力，如果是应用系统，可以认为是业务上那么些颜色已经足够，而作为基础类库是不应该的。

资源关闭

1. 对于IO流（所有IO流实现了Closeable，Closeable实现了AutoCloseable），用完是需要关掉的，关闭流写在finally块里是为了在任何情况下都能执行到关闭语句。对于反模式的设计，则是把流new出来，然后在方法/类之间传递，在某一次使用后在别的地方关闭。这种设计也许写代码的人知道需要关闭，在刚写的时候知道的确关闭了。可是过了一段时间或者其他人以第三方库的形式调用，就不一定能那么清楚了。

   合理的设计对于new出来的需要关闭的对象，在哪个方法里new出AutoCloseable的关闭实例，就在哪个方法里关闭。对于类中必须存在需要关闭的变量，则需要让类实现AutoCloseable接口，这样至少IDE的自动静态代码校验时就能查出问题。下面这种设计很难让人知道这里竟然有一个流是需要调用者关闭的，且这个关闭语句不在finally块中：
   

   if (waterMark instanceof ImageStreamWaterMark) {

   ImageStreamWaterMark iswm = (ImageStreamWaterMark) waterMark;
   ......
   iswm.getStream().close();

   }

2. IO流在代码中关闭需要置于finally中，其他地方均不靠谱。而最好的置于finally的方式是使用try-with-resources语法。

   传统的finally写法：
   

   InputStream is = new FileInputStream("");
   OutputStream os = new FileOutputStream("");
   try {

   is = new FileInputStream("");
   os = new FileOutputStream("");
   IOUtils.copy(is, os);

   } finally {

   //这里每一个close都需要用单独的try块包围，以免无法关闭，还得做null值校验，否则需要考虑cache NullPointerException
   if (is != null) {
       try {
           is.close();
       } catch (IOException e) {
           //deal exception
       }
   }
   if (os != null) {
       try {
           os.close();
       } catch (IOException e) {
           //deal exception
       }
   }

   }

   使用try-with-resources语法的写法
   

   try (InputStream is = new FileInputStream("");

   OutputStream os = new FileOutputStream("");) {
   IOUtils.copy(is, os);

   }

异常处理

异常处理的原则是：能处理就处理，不能处理就抛出，抛出带着原信息，切勿一点不处理。

1. 不能为了外层少写处理异常的代码，就抛出RuntimeException

   **RuntimeException是运行时异常，表示代码本身存在Bug，是程序员的错**，比如数组越界代表使用数组时没有检查边界。那么随便接到一个异常就抛出去，其实相当于说“我这里不可能有错，有错就是别人的错”。如下面的代码，可能出现IOException，出现后直接往上抛就行了，没必要在这里做出保证说除非程序员写错，否则我这里不可能出现IO异常。
   

   try {

   OutputStream outputStream = new FileOutputStream(outFileName);
   mark(fileType, inputStream, outputStream, waterMarks);

   } catch (Exception e) {

   throw new RuntimeException(e);

   }

2. 异常不能一点也不处理，至少得要记录日志，如果记录日志必要都没有，那最后的底线是要有一条注释说明这里不处理是人为这样的，是经过考虑的。

   } catch (Exception e) {

   //这里要有日志，至少也要有注释说明是故意留空的。

   }

3. 同一系统内，底层抛给外层的异常无需记录日志。抛出去了，处理的责任在外层，记录日志的责任也在外层，没有必要层层抛出的过程中层层打印Full stacktrace，这样日志重复太多，不利于日志审计（人肉和机器审计都一样）。

命名的一致性

对于枚举类，应该是一个单数，在代码Review中发现同一个模块同一个开发者写的枚举类，单复数形式均有出现。这里再次唠叨，类名、成员变量用名词，方法名用动词，get/set符合Javabean规范。

硬编码（Hard Code）

硬编码是众所周知该避免的。限于工期、配合、规划、架构等等原因，经常会违背。比如：

String appKey = "******";
String appSecret = "******";
String tokenUrl = "https://******.com/******";
String refreshTokenUrl = "https://******.com/******";
String suggestionUrl = "https://******.com/******";

这些信息应该配置在配置项中，通过属性文件带入程序。

字符集的实例化

Charset cs = Charset.forName("UTF-8")是一个比较古董的写法，这个方法虽然执行快，但好歹也要执行一堆代码，还存在着一个Hard Code的UTF-8。这种使用场景可以考虑两种解决方案：

1. 使用JDK7提供的StandardCharsets.UTF_8，是static final的对象。
2. 在系统中的常量类里保存一个全系统通用的Charset不可变静态对象。

可扩展的业务形态与不可扩展的枚举类

枚举类中把所有可能的值都进行了穷举，如果需要增加一个类型，需要修改枚举类的代码并重新编译。我们期望的是基础类库与业务尽量少的关联，业务系统在新需求进来后尽可能用配置或增加类的方式解决而不用修改和编译已有代码。

public enum Module {
    POT(1),
    PRD_DOC(2),
    SOLUTION(3);
}
public enum SubModule {
    POT_ODPS(),
    POT_ADS();
}

上面的代码含义是一个解决方案中心系统中，里面有三种类型的主模块，分别是PoT、产品文档、解决方案，PD明确表示只有这三个类别，所以开发人员写了这样的代码。看上去没有问题，但是从产品角度，需求不会一成不变，这三个类型只是产品当前形态，将来增加类型必然会修改代码（现在其实系统中已经从3类变成了4类）。从架构设计角度，这个类和业务相关，却被放到了基础公共类库中，让基础类库与业务产生了强耦合。这一类需求应该放在业务代码的工程中，并设计为在数据库中配置，在重构中把hard code的模块类型和模块名干掉。

重复的类

com.company.authority.sso.async.http.HttpProxy与com.company.authority.service.util.HttpProxy包名不同，类的方法签名完全一致，内部实现一个采用了HttpClient3.X，另一个采用了HttpClient4.X的API，需要合并。

Lazy load的单例需要注意运行效率

public static LoginAdapter<?> getLoginAdapter() {
    synchronized (LoginAdapterFactory.class) {
        if (adapter == null) {
            ......
        }
        return adapter;
    }
}

在这里，多线程状态下执行getLoginAdapter()方法，其实是串行的，因为有一个synchronized。Lazy load的单例另一种写法是双重判断，即：

public static LoginAdapter<?> getLoginAdapter() {
    if(adapter==null){
        synchronized (LoginAdapterFactory.class) {
            if (adapter == null) {
                ......
            }
        }
    }
    return adapter;
}

上面的代码在adapter初始化成功后，允许多线程并发执行，效率高。但是网上很多文章说JVM规范并不保证这样执行一定正确，其实这是针对早起Java而言的，现在的JVM早已能够保证这种代码写的单例能够正确执行。切不可人云亦云，也不可长期不更新已有的知识。

重复造轮子

这是一段生成指定长度随机字母、数字的方法，槽点比较多。

public static String getRandomString(int length) {
    SecureRandom ran = new SecureRandom();
    String rt = "";
    String all = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
    int rint = 0;
    for (int i = 0; i < length; i++) {
        rint = ran.nextInt();
        if (rint < 0) {
            rint = -rint;
        }
        rint = rint % all.length();
        rt += all.substring(rint, rint + 1);
    }
    return rt;
}

1. 能用StringBuilder的地方用String

   字符串相加消耗的资源比较多，应该用`StringBuilder`。这里的`rt += all.substring(rint, rint + 1);`经过JDK编译后，虽然优化为`rt = (new StringBuilder(String.valueOf(rt))).append(all.substring(rint, rint + 1)).toString();`，可避免不了`StringBuilder`对象多次创建，并和`String`对象之间不停转换的问题。
   

2. 能用char[]地方用了String

   `all.substring(rint, rint + 1);`是字符串操作，相比char[]操作更耗费资源，若吧`all`变量toCharArray()一下，效率会高很多。
   

3. 能用nextInt(int)地方用了nextInt()

   `SecureRandom`是`Random`的子类，具有`nextInt(int)`方法，如果用了这个方法，5行代码直接变为1行就行了。
   

其实上面的槽点皆来自于重复造轮子。在Java世界里，第三方工具类库非常多，有的已经被广泛使用，经过无数的项目检验，优化了一版又一版，比如Apache commons-lang。在commons-lang中有一个方法，实现了上面那段代码的功能，那就是：org.apache.commons.lang3.RandomStringUtils.randomAlphanumeric(int)。一个应用程序员应该需要对第三方类库的合理选择和使用，提高生产效率。

包名的命名与类的放置

Review的基础类库将来可能接不同的SSO，其中一个是阿里的BUC。含有BUC的BucLoginAdapter类直接放在了***.sso.adapter包中，合理的方式是放在***.sso.adapter.buc包下。

null值参数的检测

基础类的方法可能被任何应用调用，方法中传进来的参数可能千奇百怪，其中就包含传入null。如下面代码片段：

public static JSONObject sendPost(String url, Map<String, String> hashmap) {
    HttpPost hp = new HttpPost(url);
    List<NameValuePair> params = new ArrayList<NameValuePair>();
    Iterator<Map.Entry<String, String>> iter = hashmap.entrySet().iterator();

一旦url或hashmap传入的是null，就会抛出NullPointerException，NullPointerException是RuntimeException，代表是程序的Bug。在设计程序特别是基础类时，需要尽可能做到null-safe，如果做不到那就需要在注释中明确说明，并在方法内部加上assert断言。。

尽可能小的作用域

根据OCP原理，对扩展而开放，对修改而关闭。在编程中不愿扩展的地方尽可能的不要去开放，一旦开放被别人调用就不受自己的控制，那时候就不是自己想改就能改的了。这一点做过Open API的同学应该感触特别深。

public class AuthorityInterceptor extends HandlerInterceptorAdapter {
    @Autowired
    PermissionCheckService permissionCheckService;
    @Autowired
    AuthorityResourceMapper authorityResourceMapper;

上面的代码中，成员变量作用域规定为friendly，意味着同一个包的代码均可调用，实际上可以定位private就行。因为friendly作用域无关键字，无法判断是因为开发人员漏写还是设计的就是这个作用域，因此所有friendly作用域的均需要在注释中说明。

后记

严谨的代码可以带来更少的Bug，给调用者、接手的人更好的体验，也是一个程序员技术的外在表现。一个优秀的程序员必然对程序运行效率、程序开发效率、上下游衔接配合、代码容错性有着较高的自我要求。每次Code Review都是一次好的学习机会，让自己向优秀程序员更加接近。