实践中的重构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;

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

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

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

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

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);  
    }  

然后再重构一下:

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中提取出来，因为感觉上更像是独立的。虽然你上面写注释了，但感觉不符合逻辑的地方就算有注释，也不是直观明了。

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

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

对这段代码提点建议：
（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的情况。


 

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


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


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

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

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