实践中的重构01-05

目录

实践中的重构01_小方法的细调
实践中的重构02_代码的视觉效果
实践中的重构03_批处理方法默认约定
实践中的重构04_了解每一行代码 装箱的布尔值
实践中的重构05_简洁的代码


实践中的重构01_小方法的细调

重构的概念已经为广大的程序员所熟悉。但是还是有很多细节可以注意。

    public static String getHiddenEmail(String email, int prefix, int suffix) {
        // 仅对包含@的email账户进行处理
        if (StringUtil.isBlank(email) || !StringUtil.contains(email, "@")) {
            return email;
        }

        int length = email.length();

        if (length < DEFAULT_TOTAL) {
            return email;
        }
        // @所在位置
        int splitPos = StringUtil.lastIndexOf(email, '@');
        // @前的字符段
        String preEmail = StringUtil.substring(email, 0, splitPos);
        if (StringUtil.isBlank(preEmail)) {
            return email;
        }
        // @后的字符段，包含@
        String latEmail = StringUtil.substring(email, splitPos, length);
        if (StringUtil.isBlank(latEmail)) {
            return email;
        }
        if (preEmail.length() > 17)
            preEmail = StringUtil.abbreviate(preEmail, prefix);

        if (latEmail.length() > 13)
            latEmail = StringUtil.abbreviate(latEmail, suffix);

        return preEmail.concat(latEmail);
    }

以上的code在我看来，有以下几个缺点。
1 方法名和实际的方法体不匹配。
2 魔数
3 if (StringUtil.isBlank(latEmail)) 永远返回false，因为当程序执行到这里，latEmail至少包括一个字符@。
4 方法中的代码的内聚性不够。主要是对preEmail和latEmail的处理分开了。
5 拼写错误，应该为lastEmail而误拼为latEmail

清洗代码后如下

private static String getAbbreviatedEmail(String email, int prefix, int suffix) {

        if (StringUtil.isBlank(email)) {
            return email;
        }

        if (email.length() <= DEFAULT_TOTAL) {
            return email;
        }

        // @所在位置
        int splitPos = StringUtil.lastIndexOf(email, '@');
        if (splitPos == -1 || splitPos == 0) {
            return email;
        }

        // @前的字符段
        String preEmail = StringUtil.substring(email, 0, splitPos);
        if (preEmail.length() > DEFAULT_PREFIX_LENGTH) {
            preEmail = StringUtil.abbreviate(preEmail, prefix);
        }

        // @后的字符段，包含@
        String lastEmail = StringUtil.substring(email, splitPos, email.length());
        if (lastEmail.length() > DEFAULT_SUFFIX_LENGTH) {
            lastEmail = StringUtil.abbreviate(lastEmail, suffix);
        }

        return preEmail+lastEmail;
    }

其实我个人最喜欢的风格是简单的方法的guard condition不用大括号{}，这样代码变为

private static String getAbbreviatedEmail(String email, int prefix, int suffix) {

        if (StringUtil.isBlank(email)) 
            return email;

        if (email.length() <= DEFAULT_TOTAL) 
            return email;

        // @所在位置
        int splitPos = StringUtil.lastIndexOf(email, '@');
        if (splitPos == -1 || splitPos == 0) 
            return email;

        // @前的字符段
        String preEmail = StringUtil.substring(email, 0, splitPos);
        if (preEmail.length() > DEFAULT_PREFIX_LENGTH) {
            preEmail = StringUtil.abbreviate(preEmail, prefix);
        }

        // @后的字符段，包含@
        String lastEmail = StringUtil.substring(email, splitPos, email.length());
        if (lastEmail.length() > DEFAULT_SUFFIX_LENGTH) {
            lastEmail = StringUtil.abbreviate(lastEmail, suffix);
        }

        return preEmail+lastEmail;
    }

实践中的重构02_代码的视觉效果

相信程序员都会承认读代码的时间比写代码的时间长。那么有没有写代码的时候有没有什么可以帮助其他程序员甚至自己快速读程序的手段呢。

试看一例，当然这里给的日志很简单，实际中对于重要操作，日志是要打很多东西的。

try{
  //一个操作，有可能抛出异常
  db.operate();
}catch(Exception e){
  log.warn(e);
}

log.info("msg");

从视觉上看，不是很清晰。


try{
  //一个操作，有可能抛出异常
  db.operate();
  log.info("msg");
}catch(Exception e){
  log.warn(e);
}

ok，不用思考了，一眼瞟过去，就知道一个log是成功日志，一个是失败日志。


实践中的重构03_批处理方法默认约定

最近看代码的时候，发现了一个奇怪的现象。关于调用批处理接口的问题。

如调用一个查询用户信息的接口为
UserInfo getUserInfo(String id)

则对应的批处理接口为
List<UserInfo> getUserInfoByIds(List<String> ids)

很多地方对该返回值进行了校验，即用for循环比对返回的UserInfo进行比对，担心返回的列表的长度和传入参数的长度不同，担心返回的列表的顺序和传入参数的顺序不同。

我觉得这样大可不必。调用批处理接口，应该是符合common sense的。
即可以返回一个null,可以返回一个empty list,其他情况都是返回一个大小和传入参数个数相等且顺序一致的列表。

如果有特殊情况，应该在方法的接口定义中特别声明，这样调用方的code会比较清晰好读，也符合一般人的直觉。让调用方做校验，这样的想法如果没有很强大的理由，还是不要的好，因为遵守默认约定，有可能服务方的代码会稍微复杂一点，但是考虑到多处调用方的代码的简洁和易读，这点代价完全是值得的。


实践中的重构04_了解每一行代码 装箱的布尔值

最近看到代码中有这样的code.

Boolean isNeedProxy = (Boolean)threadLocalMap.get(ip);
return ( isNeedProxy ==  Boolean.TRUE ) ? true : false;

我的猜想是编程的人为了防止isNeedProxy为null，所以有了这段代码。

这里有个问题。

如果存储的值是new出来的Boolean，那么这里的逻辑就是错的。

@Test
	public void testBoolean() {
		Boolean b = new Boolean("true");
		Assert.assertFalse(b == Boolean.TRUE);
		Assert.assertTrue(b);
	}

这样写就好了

return isNeedProxy==null?false:isNeedProxy;

实践中的重构05_简洁的代码

if (a|| b|| c) {
   return true;
}
return false;

可以改为

return a|| b|| c;

评论

15 楼 zhang_xzhi_xjtu 2010-11-16  

kazy 写道

最近看代码的时候，发现了一个奇怪的现象。关于调用批处理接口的问题。

如调用一个查询用户信息的接口为
UserInfo getUserInfo(String id)

则对应的批处理接口为
List<UserInfo> getUserInfoByIds(List<String> ids)

---------------------------------------
我觉得这个批处理接口应该这么定义：
List<UserInfo> getUserInfos(List<UserInfo> searchCondition)

恩，这个也是一个方式。
但是就实现而言，如果定义一个searchCondition，dba会抓狂的。
你想想我用很多不同的searchCondition去抓数据，db会怎样？
当然，在某些场景下，这样的定义是挺好的。

14 楼 kazy 2010-11-16  

最近看代码的时候，发现了一个奇怪的现象。关于调用批处理接口的问题。

如调用一个查询用户信息的接口为
UserInfo getUserInfo(String id)

则对应的批处理接口为
List<UserInfo> getUserInfoByIds(List<String> ids)

---------------------------------------
我觉得这个批处理接口应该这么定义：
List<UserInfo> getUserInfos(List<UserInfo> searchCondition)

13 楼 zhang_xzhi_xjtu 2010-11-15  

tuti 写道

zhang_xzhi_xjtu 写道

private static String getAbbreviatedEmail(String email, int prefix, int suffix) {

        if (StringUtil.isBlank(email)) 
            return email;

        if (email.length() <= DEFAULT_TOTAL) 
            return email;

        // @所在位置
        int splitPos = StringUtil.lastIndexOf(email, '@');
        if (splitPos == -1 || splitPos == 0) 
            return email;

        // @前的字符段
        String preEmail = StringUtil.substring(email, 0, splitPos);
        if (preEmail.length() > DEFAULT_PREFIX_LENGTH) {
            preEmail = StringUtil.abbreviate(preEmail, prefix);
        }

        // @后的字符段，包含@
        String lastEmail = StringUtil.substring(email, splitPos, email.length());
        if (lastEmail.length() > DEFAULT_SUFFIX_LENGTH) {
            lastEmail = StringUtil.abbreviate(latEmail, suffix);
        }

        return preEmail.concat(latEmail);
    }

对这段代码提点建议：
（1）方法名还是有些含混 private static String getAbbreviatedEmail(String email, int prefix, int suffix)
     看不出是处理EmailAddress还是Email本身的内容。几个参数的名字也很不好理解。
Email在系统里面是特指emailAddress的，这个是不说自明的。
参数的名字是有些问题，但是考虑到是个private方法，这样比较短一点。

（2）DEFAULT_TOTAL  是什么意思，从名字上看不出任何业务含义，看逻辑好像是个Email Address最小合法长度的检查，但却用<=，而不是用<. 这个常量命名有些问题。
这个地方本身就应该是<=。

（3）方法体中，注释太多，说明变量命名的可读性要继续提高。
这段不加注释，相信不会影响阅读，公司的编码规范，有操作都要有注释的，其实我很想把方法中的注释都杀掉。

（4）prefix 和suffix 为什么是参数传入，而不是常量定义。DEFAULT_PREFIX_LENGTH， DEFAULT_SUFFIX_LENGTH的含义还不太明确。 prefix 和 DEFAULT_PREFIX_LENGTH 2个数是什么关系？ 是否会出现 prefix< DEFAULT_PREFIX_LENGTH的情况。

prefix< DEFAULT_PREFIX_LENGTH是一定成立的。prefix是对email缩写的时候，明文保留的长度。这里的上下文太多，没有介绍清楚，我的错。
 

12 楼 zhang_xzhi_xjtu 2010-11-15  

luckaway 写道

引用

我觉得这样大可不必。调用批处理接口，应该是符合common sense的。
即可以返回一个null,可以返回一个empty list,其他情况都是返回一个大小和传入参数个数相等且顺序一致的列表。

返回一个null不好，应该返回EmptyList，Null Object模式。如果该方法被很多地方调用，那每个地方岂不是都要判断返回的是否null。如果传入的参数ids是null，要么直接抛出IllegalArgumentException，要么直接返回EmptyList。
如果该方法在Dao上，可以不做参数检查，因为dao都是给内部的Service调用的，可以保证不传null。


大小是否一致，还是得看实际情况吧，通常情况下不会所有的id都能有对应的数据。


顺序没必要一致，万一顺序有错误，很难排查。如果真的需要，可以返回List<Map>,Map的key是id，value是id对应的数据

传null和EmptyList是各有利弊的，不能一概而论。
传EmptyList也不算是NullObject吧，算得话也是很弱的模式的应用。
没有上面是必要的，只是合适和不合适。作为客户段，传一个List，返回一个List处理起来比较自然。
当然，用Map也是一个方式。
排错的问题，我一向认为，你调用的方法，如果你不信任就不要调用好了。

11 楼 zhang_xzhi_xjtu 2010-11-15  

piao_bo_yi 写道

第三段代码的例子明显是原来就是错误的，不能算重构。而且如果你是写客户代码，你怎么能要求服务代码更改实现？？你也只能写一些判断处理了。

第四段代码为什么不直接写
return ( isNeedProxy == Boolean.TRUE);

这个我觉得你没有理解这段逻辑。
这段code本身是有问题的。

	@Test
	public void testBoolean() {
		Boolean b = new Boolean("true");
		Assert.assertFalse(b == Boolean.TRUE);
		Assert.assertTrue(b);
	}

这个是可以通过测试的。这个是重构发现问题的一个例子。

10 楼 zhang_xzhi_xjtu 2010-11-15  

piao_bo_yi 写道

然后再重构一下:

private static String getAbbreviatedEmail(String email, int prefix, int suffix) {    
        if(isInvalidEmail(email))  
             return email;  
     
        return preAbb(email,prefix) + "@" + lastAbb(email,suffix);
    } 

把concat换成+号（别用效率批判我，你就3个对象），把"@"从lastEmail中提取出来，因为感觉上更像是独立的。虽然你上面写注释了，但感觉不符合逻辑的地方就算有注释，也不是直观明了。

用+号我是完全赞同的。

但是如果再切割的化我是有保留的。
如果有多个地方用,则抽出去，但是整个工程只有这个地方使用，2个短的简单的相邻的重复还是可以接受的。

9 楼 zhang_xzhi_xjtu 2010-11-15  

分离的北极熊 写道

LZ的这段代码，貌似在几个月前见过，当时好像也是重构……

对 以前是分开的，现在的想法是5个短篇就会集中成一个长的。

8 楼 liaofeng_xiao 2010-11-15  

piao_bo_yi 写道

然后再重构一下:

private static String getAbbreviatedEmail(String email, int prefix, int suffix) {    
        if(isInvalidEmail(email))  
             return email;  
     
        return preAbb(email,prefix) + "@" + lastAbb(email,suffix);
    } 

把concat换成+号（别用效率批判我，你就3个对象），把"@"从lastEmail中提取出来，因为感觉上更像是独立的。虽然你上面写注释了，但感觉不符合逻辑的地方就算有注释，也不是直观明了。

看你的回复才舒服
没感觉楼主是在重构的说，或许火候还不够。

7 楼 luckaway 2010-11-15  

引用

我觉得这样大可不必。调用批处理接口，应该是符合common sense的。
即可以返回一个null,可以返回一个empty list,其他情况都是返回一个大小和传入参数个数相等且顺序一致的列表。

返回一个null不好，应该返回EmptyList，Null Object模式。如果该方法被很多地方调用，那每个地方岂不是都要判断返回的是否null。如果传入的参数ids是null，要么直接抛出IllegalArgumentException，要么直接返回EmptyList。
如果该方法在Dao上，可以不做参数检查，因为dao都是给内部的Service调用的，可以保证不传null。


大小是否一致，还是得看实际情况吧，通常情况下不会所有的id都能有对应的数据。


顺序没必要一致，万一顺序有错误，很难排查。如果真的需要，可以返回List<Map>,Map的key是id，value是id对应的数据

6 楼 tuti 2010-11-15  

zhang_xzhi_xjtu 写道

private static String getAbbreviatedEmail(String email, int prefix, int suffix) {

        if (StringUtil.isBlank(email)) 
            return email;

        if (email.length() <= DEFAULT_TOTAL) 
            return email;

        // @所在位置
        int splitPos = StringUtil.lastIndexOf(email, '@');
        if (splitPos == -1 || splitPos == 0) 
            return email;

        // @前的字符段
        String preEmail = StringUtil.substring(email, 0, splitPos);
        if (preEmail.length() > DEFAULT_PREFIX_LENGTH) {
            preEmail = StringUtil.abbreviate(preEmail, prefix);
        }

        // @后的字符段，包含@
        String lastEmail = StringUtil.substring(email, splitPos, email.length());
        if (lastEmail.length() > DEFAULT_SUFFIX_LENGTH) {
            lastEmail = StringUtil.abbreviate(latEmail, suffix);
        }

        return preEmail.concat(latEmail);
    }

对这段代码提点建议：
（1）方法名还是有些含混 private static String getAbbreviatedEmail(String email, int prefix, int suffix)
     看不出是处理EmailAddress还是Email本身的内容。几个参数的名字也很不好理解。

（2）DEFAULT_TOTAL  是什么意思，从名字上看不出任何业务含义，看逻辑好像是个Email Address最小合法长度的检查，但却用<=，而不是用<. 这个常量命名有些问题。

（3）方法体中，注释太多，说明变量命名的可读性要继续提高。

（4）prefix 和suffix 为什么是参数传入，而不是常量定义。DEFAULT_PREFIX_LENGTH， DEFAULT_SUFFIX_LENGTH的含义还不太明确。 prefix 和 DEFAULT_PREFIX_LENGTH 2个数是什么关系？ 是否会出现 prefix< DEFAULT_PREFIX_LENGTH的情况。


 

5 楼 piao_bo_yi 2010-11-15  

第三段代码的例子明显是原来就是错误的，不能算重构。而且如果你是写客户代码，你怎么能要求服务代码更改实现？？你也只能写一些判断处理了。

第四段代码为什么不直接写
return ( isNeedProxy == Boolean.TRUE);

4 楼 piao_bo_yi 2010-11-15  

然后再重构一下:

private static String getAbbreviatedEmail(String email, int prefix, int suffix) {    
        if(isInvalidEmail(email))  
             return email;  
     
        return preAbb(email,prefix) + "@" + lastAbb(email,suffix);
    } 

把concat换成+号（别用效率批判我，你就3个对象），把"@"从lastEmail中提取出来，因为感觉上更像是独立的。虽然你上面写注释了，但感觉不符合逻辑的地方就算有注释，也不是直观明了。

3 楼 piao_bo_yi 2010-11-15  

验证代码提取出来比较好。

private static String getAbbreviatedEmail(String email, int prefix, int suffix) {  
  
        if(isInvalidEmail(email))
             return email;

        // @前的字符段  
        String preEmail = StringUtil.substring(email, 0, splitPos);  
        if (preEmail.length() > DEFAULT_PREFIX_LENGTH) {  
            preEmail = StringUtil.abbreviate(preEmail, prefix);  
        }  
  
        // @后的字符段，包含@  
        String lastEmail = StringUtil.substring(email, splitPos, email.length());  
        if (lastEmail.length() > DEFAULT_SUFFIX_LENGTH) {  
            lastEmail = StringUtil.abbreviate(latEmail, suffix);  
        }  
  
        return preEmail.concat(latEmail);  
    }  

2 楼 linux1689 2010-11-15  

重构后的代码的确有了改善，要是能再分析得更细致一些就好了。

当然，希望楼主放出更加复杂的例子。

1 楼 分离的北极熊 2010-11-14  

LZ的这段代码，貌似在几个月前见过，当时好像也是重构……