浏览 2205 次
锁定老帖子 主题:实践中的重构10_平铺直叙的代码
精华帖 (0) :: 良好帖 (0) :: 新手帖 (0) :: 隐藏帖 (0)
|
|
---|---|
作者 | 正文 |
发表时间:2010-12-13
最后修改:2011-06-06
作为程序员,在代码级别,也可以通过种种努力减少这个差异。 先看下面一段代码,需求和场景是这样的。 传入一个整数,指定一次查询返回用户数的上限,结果返回查询到的用户列表。有3个不同的数据源可以用来查询用户信息,这3个数据源在查询的时候是有优先级的。即先查询第1个数据源,如果得到的用户数少于上限,则查询第2个数据源,如果还是少于上限,则查询第3个数据源。 public List<User> findUserList_0(int limitSize) { List<User> userList; int selectSize = 0; userList = findUserListFromSource1(limitSize); if (null != userList) { if (userList.size() < limitSize) { selectSize = limitSize - userList.size(); List<User> userList2 = findUserListFromSource2(selectSize); if (null != userList2 && !userList2.isEmpty()) { userList.addAll(userList2); if (userList.size() < limitSize) { selectSize = limitSize - userList.size(); List<User> userList3 = findUserListFromSource3(selectSize); if (null != userList3 && !userList3.isEmpty()) { userList.addAll(userList3); } } } } } else { userList = findUserListFromSource2(limitSize); if (userList.size() < limitSize) { selectSize = limitSize - userList.size(); List<User> userList4 = findUserListFromSource3(selectSize); if (null != userList4 && !userList4.isEmpty()) { userList.addAll(userList4); } } } return userList; } 如果你耐心的看完了这段代码,我只能说2个字,佩服!当一眼扫到5层的if嵌套,我已经是半晕状态了。我承认,我没有耐心看完这段代码。 该方法的需求和场景都是比较简单的,因此应该存在简单的写法。 重写的代码如下: public List<User> findUserList_1(final int limitSize) { List<User> userList = new ArrayList<User>(); List<User> temList; int selectSize = limitSize - userList.size(); temList = findUserListFromSource1(selectSize); if (temList != null) { userList.addAll(temList); } if (userList.size() >= limitSize) { return userList; } selectSize = limitSize - userList.size(); temList = findUserListFromSource2(selectSize); if (temList != null) { userList.addAll(temList); } if (userList.size() >= limitSize) { return userList; } selectSize = limitSize - userList.size(); temList = findUserListFromSource3(selectSize); if (temList != null) { userList.addAll(temList); } return userList; } 这里首先对参数加上final修饰符,明确表明方法中不会修改该参数。 其次,去掉了if嵌套,用相似的代码结构试图更清晰直白的表明这段代码的意图。 虽然代码重构后有所进步,但是,这段代码还存在以下一些问题,可以持续改进。 1 如果可以预期该方法返回的用户列表大小的话,可以设置初始的列表大小。当然,如果应用的场景对于空间,性能的要求不高,从可读性上而言,不传入该参数比较好。 2 如果遵循方法的返回类型为列表时,结果为空时返回空列表而不是null的话,可以使代码更清晰紧凑。当然,这个需要修改底层的API接口,审查调用该API的所有方法。 public List<User> findUserList_2(final int limitSize) { List<User> userList = new ArrayList<User>(limitSize); int selectSize = limitSize - userList.size(); userList.addAll(findUserListFromSource1(selectSize)); if (userList.size() >= limitSize) { return userList; } selectSize = limitSize - userList.size(); userList.addAll(findUserListFromSource2(selectSize)); if (userList.size() >= limitSize) { return userList; } selectSize = limitSize - userList.size(); userList.addAll(findUserListFromSource3(selectSize)); return userList; } 这样看上去好多了。这里遵循的原则就是平铺直叙的用编程语言直接翻译需求,试图用接近自然语言的表达方式来实现需求,从而减少两个世界之间的差异。 还有一种方式是先用自然语言(或者伪代码)描述需求的实现,然后翻译成真正的代码。 用自然语言描述这个需求的实现如下: 1 新建一个空的用户列表。 2 计算下一次查询的上限。 3 到数据源1去查询。 4 添加结果到用户列表。 5 如果用户列表达到上限则返回。 6 计算下一次查询的上限。 7 到数据源2去查询。 8添加结果到用户列表。 9如果用户列表达到上限则返回。 10计算下一次查询的上限。 11到数据源3去查询。 12添加结果到用户列表。 13 返回。 这个实现的自然语言描述和真实代码的实现很像,程序员参照这个自然语言的实现,翻译成对应的代码实现相对比较容易一点。 当然,计算机编程是复杂的,这里的实现还是有一个潜在的问题,代码中有重复的结构。鉴于这里的实现已经比较简单了,所以没有进一步去掉这个重复的结构。但是,如果需求改变,需要查询更多的数据源,或者要查询的数据源可配置的话,代码可以做如下演进。 定义一个接口查询用户列表。 public interface UserQueryService { /** * 查询用户列表。 * * @param limitSize * 查询结果的上限数。 * @return 用户列表。 * */ public List<User> queryUserList(int limitSize); } 不同的查询有不同的实现。 class UserQueryServiceImpl_1 implements UserQueryService { @Override public List<User> queryUserList(int limitSize) { return findUserListFromSource1(limitSize); } } class UserQueryServiceImpl_2 implements UserQueryService { @Override public List<User> queryUserList(int limitSize) { return findUserListFromSource2(limitSize); } } class UserQueryServiceImpl_3 implements UserQueryService { @Override public List<User> queryUserList(int limitSize) { return findUserListFromSource3(limitSize); } } 原有功能的实现。 public List<User> findUserList_3(final int limitSize) { // 这里的userQueryServices可以从配置文件中生成,也可以用户传入。 // 这里为了演示,硬编码一下。 List<UserQueryService> userQueryServices = new ArrayList<UserQueryService>(); userQueryServices.add(new UserQueryServiceImpl_1()); userQueryServices.add(new UserQueryServiceImpl_2()); userQueryServices.add(new UserQueryServiceImpl_3()); List<User> userList = new ArrayList<User>(limitSize); // 方式1 for (UserQueryService service : userQueryServices) { int temLimitSize = limitSize - userList.size(); userList.addAll(service.queryUserList(temLimitSize)); if (userList.size() >= limitSize) { break; } } // 方式2 for (int i = 0; i < userQueryServices.size() && userList.size() < limitSize; i++) { UserQueryService service = userQueryServices.get(i); int temLimitSize = limitSize - userList.size(); userList.addAll(service.queryUserList(temLimitSize)); } return userList; } 在方法体中,方式1和方式2都是可行的。对于简单的循环,尾部的一个判断跳出看上去比一个多行的条件判断更清晰一点。 在这个例子中,经过一步一步的演进,一块比较混乱的代码,竟然重构出来一个比较简单的用户查询的框架。这正是软件开发的乐趣所在。 声明:ITeye文章版权属于作者,受法律保护。没有作者书面许可不得转载。
推荐链接
|
|
返回顶楼 | |
发表时间:2010-12-18
Line 6, 多余。
返回UserList的方法都设计的不好, 应该不需要判null。 |
|
返回顶楼 | |
发表时间:2010-12-18
tuti 写道 Line 6, 多余。
返回UserList的方法都设计的不好, 应该不需要判null。 line6是多余的,只是为了保持结构一致才这么写的,个人行为。 get底层的服务,别人未必想的那么周全,如果它的注释中,没有声明一定不返回null的话,我觉得判一下null是比较安全的做法。 |
|
返回顶楼 | |
发表时间:2010-12-18
zhang_xzhi_xjtu 写道 line6是多余的,只是为了保持结构一致才这么写的,个人行为。 既然是讲“重构”,多余的代码就需要清理掉,否则就不是“平铺直叙”。 |
|
返回顶楼 | |
发表时间:2010-12-19
tuti 写道 zhang_xzhi_xjtu 写道 line6是多余的,只是为了保持结构一致才这么写的,个人行为。 既然是讲“重构”,多余的代码就需要清理掉,否则就不是“平铺直叙”。 这个有一定的道理,但是我觉得还是有商量的余地。 int selectSize = size - userList.size(); 这个和下面的代码块的结构保持一致。从语义上保持一致。 翻译一下三个代码块对当前要查询的用户数的处理 1 为用户数上限数减去已经查找到的用户数。 2 为用户数上限数减去已经查找到的用户数。 3 为用户数上限数减去已经查找到的用户数。 而省掉这个,是建立在这个是第一次查询,已经知道已经查找到的用户数为0。 等于说, 1 为用户数上限数(第一次默认知道已经查找到的用户数为0)。 2 为用户数上限数减去已经查找到的用户数。 3 为用户数上限数减去已经查找到的用户数。 我选择第一种,是因为看上去虽然有一点冗余,但是这个冗余增强了代码含义的一致性。 |
|
返回顶楼 | |
发表时间:2010-12-19
建议楼上,找个同事来看这2种代码。
听听对方的反馈,哪一种更容易理解。 |
|
返回顶楼 | |