由一段代码发表一点想法

今天在公司发现了一段很怪异的代码，为此还跟公司员工争执了一下，但由于自身是新员工，我只有无奈的屈服了，心里确实不爽，在这里发表一下自己的看法。先看一下代码，代码已经我已经简化了，只有两个类，一个是action层，另一个是Service层，具体如下：
Action：

package com.yf.test;

import java.util.List;

public class Action {
	public void excute(){
		List list=null;
		list=Service.doSomething();
		if(list.isEmpty()){
			/*
			 *此处省略其他操作
			 */
		}
	}
}

Service

package com.yf.test;

import java.util.ArrayList;
import java.util.List;

public class Service {
	public static List doSomething() {
		List list = new ArrayList();
		if(1==2){
			/*
			 * 此处省略对list的其他操作
			 *  list=.....
			 */			
		}
		return list;
	}
}

当我看到if(list.isEmpty()){时，发现潜在产生空指针异常的可能，然后就给同事说了一下，这里应该先判断List是否为null，然后他让我看Service层，说Service不会返回null，我看了一下，确实不会返回为null，但是关于这段代码，我觉得写的实在太烂，为什么烂，主要有一下几个原因：
1、在Service层，每次调用doSomething方法时，都实例化一个List，虚拟机都会在堆中为这个list开辟内存，这无疑实在浪费内存和虚拟机的，而且这个list只有在if条件成立时，才需要，如果if不成立，虚拟机还得在方法调用结束后，回收这块内存，这难道不是没事找事吗？？
2、在Action层，action不对返回的list做非null判断，这也是一种很恶心的做法，首先，根据面向对象的封装性，Service层中的实现对Action而言，应该是不可见的，Action层应该对其返回值的可能情况做判断，即list!=null必须在Action做，如果后续Service层单独抽出，以API提供Jar包的形式，即我们无法知道里面的具体细节，这时，Action层还得做非空判断。因此，本人觉得，这段代码应该做如下重构
Action：

package com.yf.test;

import java.util.List;

public class Action {
	public void excute(){
		List list=null;
		list=Service.doSomething();
		if(list!=null&&list.isEmpty()){
			/*
			 *此处省略其他操作
			 */
		}
	}
}

Service

package com.yf.test;

import java.util.ArrayList;
import java.util.List;

public class Service {
	public static List doSomething() {
		List list=null;
		if(1==2){
			list= new ArrayList();
			/*
			 * 此处省略对list的其他操作
			 *  list=.....
			 */			
		}
		return list;
	}
}

或许并不是每个人都认同我这种做法，不过我个人觉得这样比较合理，软件设计的时候要讲究层次，各层应该干得事情，就应该在所在层做好，而不是有其下层来保证，这种强依赖下层保证是一种很恶心的做法，如果后续下层代码逻辑变更，还得去上层看看对其的影响，这就很无耻了！！！

评论

38 楼 hlylove 2012-10-15  

syx278250658 写道

final List<String> someList = Service.doSomething();    
if(CollectionUtils.isEmpty(someList)){    
   .  
   .  
   . 

不管service如何，action这样判断是应该的

我也是这样的

37 楼 iceblooded 2012-10-15  

看下面的评论 学习了

36 楼 gwei 2012-10-15  

拜读了，个人认为像这种返回结果集对象的服务方法最好不要返回null，返回null无意义。至于什么内存开销太较真了吧，这算什么，在远程的情况下说不定都不在一个环境中。程序健壮性应该得到保证，Action还是应该判断NP吧，至于方式还是按项目组规范。拙见拙见。

35 楼 beiyeren 2012-10-15  

骨之灵魂 写道

flashing 写道

sqtds 写道

opnmzxcvb 写道

kidneyball 写道

1. 容器类型的底层方法应该尽量返回空集，避免返回null。从而避免顶层方法做大量的null判断。具体可参考《Effective Java》。

2. 一个以容器为返回类型的方法，返回空集通常是不常见的异常分支，为一个不常见的分支考虑类创建和垃圾回收层面上的性能问题而导致调用者做大量的额外NP判断并不值得。这类优化应该在有profiling数据支持的情况下再做。

3. “具体细节不可见”的问题，写点Javadoc就能解决。调用者需要参考接口文档知道返回值的具体情况，是很正常的事情。后续维护时要保证接口协议不变，也是很正常的事情。

4. 某些团队都会形成共识，只要是返回容器类型的方法就不返回null。只要大家都遵守这个协议，不会出问题而且可以免去大量无谓的NP判断。博主作为一个新人，应该先了解团队中的开发规范，而不是和现有规则较劲。

说的对，  

看看effective java

yeah，我也想说 effective java

看看effective java

分析的很到位

34 楼 congjl2002 2012-10-15  

michael8335 写道

young_suse 写道

楼主的担心也很合理，每次在方法开头用List list = new ArrayList();确实可能造成不必要的开销。
但从代码规范的角度来讲，声明为集合（List或数组）返回类型的函数，通常不要返回null，宁可返回size为0的空集合。
要达到既减少内存开销，又规范代码，应该使用List list = Collections.emptyList()来取代List list=null;并且按照kidneyball说的，用javadoc来说明，当条件不满足时，返回size为0的List，而不会返回null。

由于我编码时间不长，所以估计是我自身理解确实有问题，不过通过这篇文章，获得了一些知识，看来大家的意见还是倾向于不返回null，那就按照大家的来吧，反正我只是就这段代码发表一下自己的看法，谢谢大家的意见，后期会买一本《Effective Java》，在设计方面继续积累

你们员工可以坚持不实用Null说明他们水平不错，你有机会多向他们学习，不要总想去质疑他们的代码，毕竟在编程里有这样一个规定“习惯大于配置”，编程时间短，有很多行规你还不清楚，因此要虚心，但也不是就全盘接受，你有这个分析问题的想法很好，只是要注意方法

33 楼 lianglaiyang 2012-10-15  

forcer521 写道

jorneyR 写道

在职场中，不要轻易去表现出质疑别人的东西，就算那东西再差，自己不赞同，也尽量不要表现出来，人心难测，除非你有决策权的时候。

很有经验啊，呵呵。
我们程序员很多时候就是缺乏这种思维能力。
很多时候，做的对并不能得到大家的认可的。

那是自己一厢情愿的认为自己对，其实最终可能会发现自己错的离谱，是那么可笑。。。

32 楼 骨之灵魂 2012-10-15  

flashing 写道

sqtds 写道

opnmzxcvb 写道

kidneyball 写道

1. 容器类型的底层方法应该尽量返回空集，避免返回null。从而避免顶层方法做大量的null判断。具体可参考《Effective Java》。

2. 一个以容器为返回类型的方法，返回空集通常是不常见的异常分支，为一个不常见的分支考虑类创建和垃圾回收层面上的性能问题而导致调用者做大量的额外NP判断并不值得。这类优化应该在有profiling数据支持的情况下再做。

3. “具体细节不可见”的问题，写点Javadoc就能解决。调用者需要参考接口文档知道返回值的具体情况，是很正常的事情。后续维护时要保证接口协议不变，也是很正常的事情。

4. 某些团队都会形成共识，只要是返回容器类型的方法就不返回null。只要大家都遵守这个协议，不会出问题而且可以免去大量无谓的NP判断。博主作为一个新人，应该先了解团队中的开发规范，而不是和现有规则较劲。

说的对，  

看看effective java

yeah，我也想说 effective java

看看effective java

31 楼 lianglaiyang 2012-10-15  

参考大部分开源框架API，返回集合时都不会返回null这一种情况！当今什么时代了，如果一般应用程序的话那一点内存根本可以忽略不计

30 楼 fwmlove 2012-10-15  

我怎么觉得Service层的重构跟没重构一样，你就算NUll不NULL也好，这个List在Service层里面又不是成员变量，只要是同个线程内，你每次调用这个方法，原来的那个就会可以被回收啦。不知道我理解有没有问题。

29 楼 vvggsky 2012-10-15  

防御式编程 未尝不可

28 楼 zuoshu 2012-10-15  

skzr.org 写道

尽量不返回null——说实在的就算返回了null，你又能做什么了。
——你只能得到一大堆的if (xxx == null) ...，如果直接返回empty集合，代码更加简洁易懂

绝大多数情况（99%）empty集合更加合适。
过早的优化——万恶之源啊。。。

既然意识到只能保证99%，为何不优化到100%，程序的健壮性何在！

27 楼 kyfxbl 2012-10-15  

我只能说楼主乐于思考的精神还是很好的，应该保持

26 楼 skzr.org 2012-10-15  

尽量不返回null——说实在的就算返回了null，你又能做什么了。
——你只能得到一大堆的if (xxx == null) ...，如果直接返回empty集合，代码更加简洁易懂

绝大多数情况（99%）empty集合更加合适。
过早的优化——万恶之源啊。。。

25 楼 syx278250658 2012-10-15  

final List<String> someList = Service.doSomething();    
if(CollectionUtils.isEmpty(someList)){    
   .  
   .  
   . 

不管service如何，action这样判断是应该的

24 楼 jinnianshilongnian 2012-10-15  

rensanning 写道

jinnianshilongnian 写道

当然还有一种叫做契约式编程：

在service返回时 加：
Assert.assertNotNull(list);  来强制约束返回值不为null 

契约的缺点就是靠程序员意识，写不写很难控制

spring对契约式编程的支持：
http://jinnianshilongnian.iteye.com/blog/1495594

产品代码中一般很少见JDK Assertions,多见于调试或者测试。运行时默认是关闭状态，即使开启，它所抛出的AssertionError异常扩展自Error而不是Exception，会扰乱系统的异常处理。普遍的框架中对采用IllegalArgumentException，比如Spring的org.springframework.util.Assert

不是java自带的assert断言  而是自己扩展的一套 正如你说的如Spring的

23 楼 rensanning 2012-10-15  

jinnianshilongnian 写道

当然还有一种叫做契约式编程：

在service返回时 加：
Assert.assertNotNull(list);  来强制约束返回值不为null 

契约的缺点就是靠程序员意识，写不写很难控制

spring对契约式编程的支持：
http://jinnianshilongnian.iteye.com/blog/1495594

产品代码中一般很少见JDK Assertions,多见于调试或者测试。运行时默认是关闭状态，即使开启，它所抛出的AssertionError异常扩展自Error而不是Exception，会扰乱系统的异常处理。普遍的框架中对采用IllegalArgumentException，比如Spring的org.springframework.util.Assert

22 楼 jinnianshilongnian 2012-10-15  

当然还有一种叫做契约式编程：

在service返回时 加：
Assert.assertNotNull(list);  来强制约束返回值不为null 

契约的缺点就是靠程序员意识，写不写很难控制

spring对契约式编程的支持：
http://jinnianshilongnian.iteye.com/blog/1495594

21 楼 jinnianshilongnian 2012-10-15  

1、在Service层，每次调用doSomething方法时，都实例化一个List，虚拟机都会在堆中为这个list开辟内存，这无疑实在浪费内存和虚拟机的，而且这个list只有在if条件成立时，才需要，如果if不成立，虚拟机还得在方法调用结束后，回收这块内存，这难道不是没事找事吗？？

如果服务层 调用 持久层框架（如hibernate） 即使是空 也是返回一个集合的；
                 自己写业务  如果为null  可以返回Collections.EMPTY_LIST(单例 且不可变的) 
建议：不要过早优化 先保证代码可读和可维护



2、在Action层，action不对返回的list做非null判断，这也是一种很恶心的做法，首先，根据面向对象的封装性，Service层中的实现对Action而言，应该是不可见的，Action层应该对其返回值的可能情况做判断，即list!=null必须在Action做，如果后续Service层单独抽出，以API提供Jar包的形式，即我们无法知道里面的具体细节，这时，Action层还得做非空判断。因此，本人觉得，这段代码应该做如下重构

  可以通过如ListUtils.isEmpty(list) 来判断  然后把判断null 和 空的逻辑封装起来；
  另外：如StringUtils等这种工具应该是自己工具箱必备的；

  建议：掌握常用工具箱工具，减少做重复的事情。

20 楼 lazy_ 2012-10-15  

package com.yf.test;

import java.util.ArrayList;
import java.util.List;

public class Service {
	public static List doSomething() {
		List list=null;
		if(xxx){
			list= new ArrayList();
			/*
			 * 此处省略对list的其他操作
			 *  list=.....
			 */			
		}
                if(null == list){
                      return new ArrayList(0);
                }
		return list;
	}
}

19 楼 lazy_ 2012-10-15  

楼主的问题我也在烦恼。从实践的角度看，NULL好容易引起错误。也许就是这个原因，freemarker没有NULL。

1 我编码的时候，尽量不返回NULL。可以return一个new ArrayList(0)或者new HashMap(0)，或者"",这就不用担心所谓内存消耗的问题。
2 我用别人写的类，或者好久之前的类，保守起见我肯定会做NULL判断。

引用

容器类型的底层方法应该尽量返回空集，避免返回null。从而避免顶层方法做大量的null判断。具体可参考《Effective Java》。

我没看过Effective Java，但是这句我非常赞成啊，能少写代码就少写代码，简单是王道。其实封装一下
class Util{
  //not null
  public static boolean NN(list){
       if(null != list && !list.empty() ){
              return true;
       }
       return false;
  }
}
也能达到目的。当然这样虽然较少了业务代码，但是会导致公用代码变多了。