记一次我 code review 发现的几处小问题总结
好久没写原创了，最近一直在忙。忙着开会，忙着 Code Review 等。今天，就给大家分享一下，我最近一次 Code Review 的成果，分享给大家！希望大家都能够成长！
线程池使用不当
我们的调度系统需要将一堆会员分配给相应的人员来处理，流程如以下伪代码所示：
public void dispatch() {
while (true) {
//获取所有未分配的会员
List<Member> memberList = getUnassignedMemberList();
for(Member each : memberList) {
//为每一个会员分配相应的人员处理
singleDispatch(each);
}
try {
//休眠1秒后继续分配
Thread.sleep(1000);
} catch (InterruptedException e) {
}
}
}为了提高分配的速度，我们打算采用多线程的分配方式。一开始使用的是 newCachedThreadPool。
private static ExecutorService executor = Executors.newCachedThreadPool();
public void dispatch() {
while (true) {
List<Member> memberList = getUnassignedMemberList(); //获取所有未分配的会员
for(final Member each : memberList) {
executor.submit(new Runnable() {
@Override
public void run() {
singleDispatch(each); //为每一个会员分配相应的人员
}
});
}
try {
Thread.sleep(1000); //休眠1秒后继续分配
} catch (InterruptedException e) {
}
}
}在压测时发现，load 飙升得很高，通过抓栈发现开启了很多线程。原因是：newCachedThreadPool 最大线程数为整型的最大值，每提交一个任务，如果没有线程处理，那就产生一个新的线程。当我们 for 循环提交任务时，开辟了上百个线程，应用程序马上崩溃。
既然发现了原因，我们马上将调整线程池为 newFixedThreadPool，这里我们可以设置最大线程数为 4，队列长度为整型的最大值。
private static ExecutorService executor = Executors.newFixedThreadPool(4);但是压测又发现新问题，线程池里的队列长度不断增长，而且分配不断有异常抛出（异常信息为会员已经被分配过）。
原因是：当未分配会员较多时，可能需要5秒才能分配完，然而 executor.submit 是异步操作，当休眠1秒钟后，马上又进入下一个循环，队列里又将插入重复的会员，这会导致队列长度不断增长，此外，会导致1个会员被分配后，又继续被分配，导致异常产生。
解决方法：使用 invokeAll 这一同步语句，意思是只有当提交任务都被执行完后，才执行后续语句。
public void dispatch() {
while (true) {
List<Callable<String>> tasks = new ArrayList<Callable<String>>();
//获取所有未分配的会员
List<Member> memberList = getUnassignedMemberList();
for (final Member each : memberList) {
tasks.add(new Callable<String>() {
@Override
public String call() {
singleDispatch(each);
return "ok";
}
});
}
try {
//如果8分钟还未执行完，则超时重新再来（鲁棒性保证）
executor.invokeAll(tasks, 480, TimeUnit.SECONDS);
}catch (Exception e) {
}
try {
//休眠1秒后继续分配
Thread.sleep(1000);
} catch (InterruptedException e) {
}
}
}NullPointerException
第一种情形：
if(case.getType() == Case.TYPE_SELF) {
...
}这段代码抛出 NPE 时，直觉认为 case 为 null 导致的，后来打日志发现 case 并不为 null，而 case.getType() 返回值类型为 Integer，为null。
最后发现 Case.TYPE_SELF 返回的是int类型，而 case.getType() 是个 null，null与 int 两者一比较就报 NPE。
这个问题的诡异之处我们直觉上认为 Case.TYPE_SELF 是个 Integer，所以导致排查问题花费了些时间，因此一个建议这种常量如 Case.TYPE_SELF 都改为Integer，然后对象间比较的时候使用 equals，并增加 null 判断，就可以避免出现问题。
if(null != case.getType() && case.getType().equals(Case.TYPE_SELF))第二种情形：
Integer leftNum = (null != leftNumMap && !leftNumMap.isEmpty()) ? leftNumMap.get(stat.getDepartmentId()) : 0;这个也抛 NPE，我们排查了半个多小时，百思不得其解，后来查看 class 文件发现了原因。
假设我们代码是：
Map<String, Integer> leftNumMap = new HashMap<String , Integer>();
Integer leftNum = (null != leftNumMap && !leftNumMap.isEmpty()) ? leftNumMap.get("test") : 0;反编译后的代码如下：
HashMap leftNumMap = new HashMap();
Integer leftNum = Integer.valueOf(null != leftNumMap && !leftNumMap.isEmpty()?((Integer)leftNumMap.get("test")).intValue():0);为什么会这样呢？总的来说就是三目运算符的语法规范是这样写的：If one of the second and third operands is of primitive type T, and the type of the other is the result of applying boxing conversion (§5.1.7) to T, then the type of the conditional expression is T.
简单的来说就是：当第二，第三位操作数一个为基本类型一个为对象时，其中的对象就会拆箱为基本类型进行操作。
所以，结果就是：由于使用了三目运算符，并且第二、第三位操作数一个是基本类型一个是对象。所以对对象进行拆箱操作，由于该对象为 null，所以在拆箱过程中调用 null.intValue() 的时候就报了 NPE。
解决方法很简单：
- 要么不用三目运算符，直接使用 if else，简单可靠；
- 或者将三目运算符操作数都改为对象。
Integer leftNum = (null != leftNumMap && !leftNumMap.isEmpty()) ? leftNumMap.get(stat.getDepartmentId()) : Integer.valueOf(0);Map<K, List>用错
我们代码中经常出现以下这种数据结构，比如 key 是类目，value 是个 List，list 存储着这个类目下各个子类目的统计数据等。
Map<String,List<MyClass>> myClassListMap = new HashMap<String,List<MyClass>>()我们插入该数据结构的时候往往得这样写：
void putMyObject(String key, Object value) {
List<Object> myClassList = myClassListMap.get(key);
if(myClassList == null) {
myClassList = new ArrayList<object>();
myClassListMap.put(key,myClassList);
}
myClassList.add(value);
}当我们希望检查List中的对象是否存在，或删除一个对象，那要遍历整个数据结构，需要更多的代码。
这些代码不仅给可读性带来障碍，更提高了大家出错的概率，比如某一次我们在复杂的业务逻辑中实现 putMyObject 时，漏写了句:
myClassListMap.put(key,myClassList);结果导致花费了大量时间才发现这个问题。
当然，细致的测试以及 code review 能避免出现类似问题，但是有没有一种方法既能从技术上来帮助我们避免此类问题，又能节约代码提高代码可读性？
推荐大家使用 google guava 包，里面提供了 MultiMap 数据结构，使用方式如下，非常简洁明了，也不会有机会让我们出错。
Multimap<String, String> myMultimap = ArrayListMultimap.create();
myMultimap.put("业余草", "涛哥");
myMultimap.put("业余草", "公众号");
myMultimap.put("涛哥", "业余草");
myMultimap.put("涛哥", "公众号");
// 获取key "业余草"对应的list
Collection<String> womenDressList = myMultimap.get("业余草");
// 删除key "业余草"对应List中的"涛哥"
myMultimap.remove("业余草", "涛哥");Long与Integer错误比较
因为 Long 与 Ineger 都是包装类型，是对象。 而不是普通类型 long 与i nt ， 所以它们在比较时必须都应该用 equals，或者先使用 longValue() 或 intValue() 方法来得到他们的基本类型的值然后使用 == 比较也是可以的。
先看看一个例子：
public class Test05 {
public static void main(String[] args) {
Long a = 5L;
Long b = 5L;
System.out.println("a == b ? " + (a == b));
Long c = 129L;
Long d = 129L;
System.out.println("c == d ? " + (c == d));
}
}打印的结果是：
a == b ? true
c == d ? false具体来说，通过反编译，我们可以看到，Long a = 5L 这句赋值语句使用的 Long.valueOf() 方法，它的定义如下：
public static Long valueOf(long l) {
final int offset = 128;
if (l >= -128 && l <= 127) { // will cache
return LongCache.cache[(int)l + offset];
}
return new Long(l);
}一目了然，会先判断基本类型的值如果在 -128~127 之间，就会直接从 LongCache里面取出缓存的对象返回，否则就 new 一个新的 Long 对象返回。
Ineger 和 Long 类似，大家自行去看看源码和反编译一下。
所以，上面的结果就不难理解了。因为 a 与 b 等于 5，在 -127~128 之内，所以都是直接从 LongCach e里面返回的一个 Long 对象，所以他们在使用 == 比较的时候，就是相等的（对于对象类型来说，== 比较的是两个对象的引用指向堆中的地址） ，而 c 与 d 等于 129，不在 -127~128 之间，所以他们他们是分别 new 出来的两个新的 Long 对象，使用 == 来比较自然是不相等的了。
Long 重写了 equals 方法：
public boolean equals(Object obj) {
if (obj instanceof Long) {
return value == ((Long)obj).longValue();
}
return false;
}它是先通过 longValue() 方法获取 Long 对象的基本类型 long 的值之后再做比较的。
所以对于 Integer 与 Long 的比较，一定要记得使用 equals 来比较才能确保得到我们想要的结果。