这样代码重用？

这是一个工作中遇到的背景比较简单的争论。

有这么一个persistent object，姑且叫它Plan吧。

有这么两个函数：

Plan getPlanByName(String userid, String planName);
Plan[] getPlans(String userid);

getPlanByName内部执行的是：

select * from Plan where userid=#userid# and plan_name=#planName#
and status=1
order by order_num

getPlan的内部执行的是：

select * from Plan where userid=#userid# and status=1

现在，我的pair认为这里面有DRY violation。因为两个select有些重复的东西。pair认为可以这样重构：

Plan getPlanByName(String userid, String planName){
  Plan[] plans = getPlans(userid);
  for(int i=0; i<plans.length; i++) {
    Plan plan = plans[i];
    if(planName.equals(plan.getPlanName())) {
      return plan;
    }
  }
  return null;
}

而我并不认为应该这样做。我的理由是：
1。原来的实现很简单直观。sql本来就是声明式语言。放着简洁的声明式不用而用复杂的命令式，有走回头路的嫌疑。
2。重构之后代码量更多，还要写更多的单元测试。
3。两个select只见的共同之处更像一种偶然的而不是概念上的重复。让getPlanByName依赖于getPlan感觉增大了耦合。
4。真要觉得select里面的东西有重复，不如创建一个view: v_plans这样这两个select就变成：

select * from v_plans where userid=#userid#
select * from v_plans where userid=#userid# and plan_name=#planName#

避免了status=1的重复。虽然还有"select *"之类的重复，但是这种重复就是语法上的，就像我们在java程序里面可能写无数次"static public void"，我们从来不认为这是一个值得改变的重复。
5。效率。
6。系统本来是工作的。即使重构后确实好一点，也不值得花这个工作量。

而pair的主要观点是:
重构后维护上简单，不用维护两个select。

后来和组里其他人沟通，发现持重构想法的不只一个人，相对来说我的观点是比较孤立的。

那么你是怎么看这个问题？

评论

40 楼 strongkill 2007-02-14  

后面的代码对数据库的成本高.每次都会改到不需要的数据.

倒不如用前面的代码.

39 楼 clamp 2007-02-13  

讲一个在我们某个项目发生过的事情
这个项目是数据密集型的，有很多的批量操作，其操作逻辑和单条操作逻辑很相似，比如批量保存/批量删除等等。
一开始的时候，有若干个熟悉sql但是不怎么熟悉java的人，把大量的逻辑放在存储过程里面，java的若干个层都是简单调用。
这个项目时间很长，做了好几期，过了两三年以后，原来的人都换的差不多了，新的一批人比较熟java但是不怎么熟sql，觉得维护起来很吃力，大家一商量，把其中业务逻辑比较复杂的几个存储过程用java重新实现了一遍。
——————————————

个人觉得在敏捷过程之下，这两种具体的实现方式没什么差别，一开始写成咋样就咋样好了。

我不赞成一有重复代码就开始重构，我认为应当是在“开始出现第三次重复代码”的时候进行重构。

38 楼 pikachu 2007-02-13  

1.代码是写给人看的。
2.除了牛人的代码，费脑子的代码不是好代码。
3.这世上没几个牛人。

37 楼 amozon 2007-02-13  

如果要重构，那样的代码多少写点注释吧。。。。

36 楼 byk 2007-02-13  

显然应该重构:
Plan getPlanByName(String userid, String planName){
Plan[] ps= getPlans(userid,planName);
if(ps!=null&& ps.length>=1) return ps[0]
else return null;
}  
Plan[] getPlans(String userid){
return getPlans(userid,null);
}
Plan[] getPlans(String userid, String planName){
String sql=select * from Plan where 1=1;
if(userid!=null) sql+=" and userid="+userid
if(planName!=null) sql+=" and plan_Name"planName

          ....//exec sql
}

35 楼 lane_cn 2007-02-12  

jianfeng008cn 写道

lane_cn 写道

无伤大雅的小问题，由他好了。
如果plan name不是数据库索引的话，在你的内存里面查和在数据表里面查实际上消耗的时间相差无几。
等他明白了软件应该怎样设计，他自然不会在这种小问题上多费一点脑筋。

“软件应该怎么设计”，你就知道，人家不知道你知道。这个问题小吗，我看还是要看什么样的系统的吧，站着说话不腰疼，大家观点不一致而已，我说你说这话明显是“程序员很普遍的臭毛病”

你说的对，要看什么样的系统。如果是100万条记录，数据库表又已经建立了这个索引，他偏不用，硬要自己去一一遍历，那个差别就大了。
对于一个100行记录的表来说，是自己遍历查询还是交给数据库去遍历查询，根本没什么影响。所以由他去好了，你跟一个程序女工争论这个有啥意思，对你现在开发的系统有好处吗。

34 楼 sinokaka 2007-02-12  

支持重构，如果只是写SQL的话，增加一个内部方法
_getPlansByConditon(String userid, String planName) {
    select * from Plan where userid=#userid#
if (planName != null) {
   and plan_name=#planName#  
}
and status=1  
order by order_num
}

如果你们用的是iBatis那种的，就考虑动态SQL的写法，不知道你们的XML文件的那种是否支持。

33 楼 LucasLee 2007-02-12  

ajoo 写道

Lucas Lee 写道

你的pair的认真精神还不错，不过我不能同意按他的想法去做。
但不管怎么样，这样深入讨论后，肯定对DRY原则的如何应用，有更深刻的了解。

我觉得你这个例子里，最好的方式是，将SQL里共同的部分拆分出来，作为变量，
如：
String mainSQL="select * from plan where status=1";
String byNameSQL=mainSQL+" and plan_name=xxx";

这样就不会有重复了，又保持了使用SQL。

sql都是存在类似ibatis的xml文件中的。

如果是这样，那的确很难。这从另一个方面说明了存放在XML里这种方式的劣势。
如果这样，干脆保持现状。无论如何，我也不觉得使用JAVA里的算法来替代SQL的findByName语句，就为了什么DRY原则，你在SQL这里是符合DRY了，你在JAVA里大大的违背了DRY了。因为SQL里很简单的事情，你非要用一个不合适宜的JAVA代码实现。
而且，这种方式，如果在方法有增加或者变化的时候(比如增加或变化为like %xxx%, between and,etc.)，也是难以扩展的，不是好方式。

BTW,符合任何原则，都要再问自己一下，这么做是否合适？不要为符合什么原则而做。

32 楼 hyf 2007-02-12  

ajoo的同事的做法，我不太喜欢。
这是一种费脑筋、不通用的做法。

31 楼 jfy3d 2007-02-12  

我的习惯是这个应用一个SQL 一个方法

可能有一点隐晦，不过的团队就是我一个人-_-!

30 楼 cat 2007-02-12  

ha 看了ajoo最近的几贴，感觉你们那里代码重用到影响别人理解代码的程度了。

不过说回来，SQL代码确实不方便重用。我一开始也觉得这类code不爽，不过现在基本想通了。一方面读这样的code方便，另一方面就算要修改也没几个连动着一起改了就完了。

29 楼 温柔一刀 2007-02-12  

没有必要重构成那样吧，原来的做法未尝不可

简单至上，这是我推崇的原则

28 楼 codeutil 2007-02-12  

这个系统该不是只给一个用户用的吧?

每次多生成100个对象,多做50次(算是循环找到一半就找到了需要的对像)循环,假设系统每天拥有10000个用户操作,每个用户平均调用这个方法10次,
那么,100*10000*10= ?
50*10000*10= ?

系统不想做大的话,代码怎么烂都可以,数据库都不用,直接一个hashmap最方便.

ajoo 写道

akun_007 写道

codeutil 写道

这样写的脑袋没烧坏吧??? 还是太有钱了一点也不用考虑性能阿?

 Plan getPlanByName(String userid, String planName){  
 Plan[] plans = getPlans(userid);  
 for(int i=0; i<plans.length; i++) {  
   Plan plan = plans[i];  
   if(planName.equals(plan.getPlanName())) {  
      return plan;  
    }  
  }  
  return null;  
 }  

严重同意啊
在做大数据量查询时，恐怕很费时的。

我很希望能拿这个作为理由。但是我知道我得到的肯定是“i don't care”。因为这个查询最多就返回一百条左右记录。

27 楼 jianfeng008cn 2007-02-12  

呵呵 没听明白就算了 

26 楼 ahau205109 2007-02-12  

jianfeng008cn 写道

ahau205109 写道

Readonly 写道

偶支持你的pair写法，偶们的代码里面也有很多是这样写的，不过不是因为啥重狗，反正一件事情做一次就好了，即便是考虑到性能问题，因为可以给getPlans这种获取大集合的方法加上cache，getPlanByName这种集合过滤的方法就可以从中受益，而不是需要加2个cache

你们牛；
那也叫重构？
死读书，读死书？
我看要是来个5个条件查询，看楼主那同事怎么写；
很明显
Plan getPlanByName(String userid, String planName);  
Plan[] getPlans(String userid);  
Plan至少是个2维数据表；加cache,我看你为整张表加cache得了；
1000个用户*10个plan = 10000 ; 你机器牛

条件再多也很好处理啊，
语气很犀利嘛 ，
“看楼主那同事怎么写”，
你自己想不明白的问题并不是就无解了，
请问hibernate的query对象是用来干什么的，
动态条件查询没做过？
我感觉是你“死读书” 了哦

呵呵 有话直说

感觉你回话牛头不对马嘴；
for for for for for

25 楼 抛出异常的爱 2007-02-12  

引用

Plan getPlanByName(String userid, String planName);
Plan[] getPlans(String userid);

getPlanByName内部执行的是：

select * from Plan where userid=#userid# and plan_name=#planName#
and status=1
order by order_num

getPlan的内部执行的是：

select * from Plan where userid=#userid# and status=1

多写一个方法

Plan[] getPlansOrige(Modle m)

里面的SQL句是:

select * from Plan where userid=#m.userid# and plan_name=#m.planName#
and status=1
order by order_num

你用到的两个方法引用这个方法得到的内容


PS:有时多易少,少弈多...

24 楼 jianfeng008cn 2007-02-12  

lane_cn 写道

无伤大雅的小问题，由他好了。
如果plan name不是数据库索引的话，在你的内存里面查和在数据表里面查实际上消耗的时间相差无几。
等他明白了软件应该怎样设计，他自然不会在这种小问题上多费一点脑筋。

“软件应该怎么设计”，你就知道，人家不知道你知道。这个问题小吗，我看还是要看什么样的系统的吧，站着说话不腰疼，大家观点不一致而已，我说你说这话明显是“程序员很普遍的臭毛病”

23 楼 jianfeng008cn 2007-02-12  

ahau205109 写道

Readonly 写道

偶支持你的pair写法，偶们的代码里面也有很多是这样写的，不过不是因为啥重狗，反正一件事情做一次就好了，即便是考虑到性能问题，因为可以给getPlans这种获取大集合的方法加上cache，getPlanByName这种集合过滤的方法就可以从中受益，而不是需要加2个cache

你们牛；
那也叫重构？
死读书，读死书？
我看要是来个5个条件查询，看楼主那同事怎么写；
很明显
Plan getPlanByName(String userid, String planName);  
Plan[] getPlans(String userid);  
Plan至少是个2维数据表；加cache,我看你为整张表加cache得了；
1000个用户*10个plan = 10000 ; 你机器牛

条件再多也很好处理啊，
语气很犀利嘛 ，
“看楼主那同事怎么写”，
你自己想不明白的问题并不是就无解了，
请问hibernate的query对象是用来干什么的，
动态条件查询没做过？
我感觉是你“死读书” 了哦

呵呵 有话直说

22 楼 lane_cn 2007-02-12  

无伤大雅的小问题，由他好了。
如果plan name不是数据库索引的话，在你的内存里面查和在数据表里面查实际上消耗的时间相差无几。
等他明白了软件应该怎样设计，他自然不会在这种小问题上多费一点脑筋。

21 楼 e3002 2007-02-12  

代码中有重复使用的地方，有坏气味，个人认为应该重构