improve bitter code: 对付魔鬼数字

备注: 示例中的代码并不是真实代码的完全拷贝

道高一尺,魔高一丈

魔鬼数字在项目中大量出现,是代码可读性维护性变差的重要推手。 为了控制事态恶化,项目中加入findbugs,checkstyle等静态检查工具,试图让人自觉修复魔鬼数字这类头疼的问题。

最近在review N项目的代码时,发现有新童鞋对修复checkstyle的问题热情高涨,有些修改的确是有益的, 但是对魔鬼数字的修改就显得不是正路了。大家先看看:

private static final int INT_2 = 2;
private Static final int INT_80 = 80;
private Static final int INT_1000 = 1000;
private Static final int INT_60000 = 60000;

这种情况以前也出现过,那个时候刚推广checkstyle,结果有人定义了NumberUtil类,里边就有ONE, TWO…等变量, 一直延伸到好几十。这让我非常纠结,劝说了很多次,结果这个类最终还是扎下根来了。我对此深恶痛绝。

今天再次看到这种方式,真是道高一尺魔高一丈呀。我又要发牢骚了。

认清工具的本质

这种做法完全就是为了对付checkstyle,而不是对付那对问题代码。工具本来是想提醒程序员,避免写出难以维护的代码。 结果我们更聪明,把代码藏起来,眼不见为净。这样其实是把数字和变量名直接关联起来了, 你能想象一个叫INT_2的变量值确是1么? 如果在不同含义的逻辑里边使用这样一个魔鬼变量, 一旦值发生变化,就会顺带把不应该修改的地方也改动了。

如果我们真的是为了提高自己的编码水平,提高项目的代码质量,就应该认真的对待这些魔鬼数字。 对于魔鬼数字,应该有一个良好的命名,对于经常出现的,还应该找个合适的地方加以管理。接下来以上面的例子继续分析。

解决魔鬼数字

要解决魔鬼数字,首先是认清魔鬼数字代表的意义,给它起一个合适的名字。

查看源代码,可以发现,做了下面的代码调整:

  1. 2是某个值在列表中的位置索引,换成XXX_IDX
  2. 80是http的默认端口,换成DEFAULT_HTTP_PROT
  3. 1000是用来把秒换成毫秒的,可以用MILLISECONDS_PER_SECOND
  4. 60000是默认超时时间来的,换成DEFAULT_TIMEOUT

同样,经常会看到魔鬼字符串,”0”,”1”这样的值出现代码里边,也应该 把给他们起一个好的名字,例如作为执行结果,有SUCCESS、FAILED等。

更进一步

需要考虑的还有常量定义的地方,另外考虑使用方法封装来处理魔鬼数字的逻辑。 这样可以隐藏一些细节,把作用域限制在很小的地方。 例如下面经常出现的魔鬼字符串,就可以考虑把整个判断逻辑封装到user类里边去, 而不只是简单的定义出ACTIVE这样一个常量。

// 原有的代码
if("1".equals(user.getStatus())){
    // ...
}

// 改造后,"1"作为user的常量使用于isActive方法
if(user.isActive()){
    // ...
}

不过话说回来,解决魔鬼数字、字符串之类等’小枝小节’,花费的精力不小。 但不要忽视这些细节,这样可以让自己不断提高对代码的认识,也便于其他人维护你的代码。

对付魔鬼数字,还是不要走捷径的好!

"improve bitter code"系列文章:

improve bitter code: 拘泥于单出口方法

备注: 示例中的代码并不是真实代码的完全拷贝

究竟是哪个日期

前阵子代码评审的时候发现一段代码,逻辑是用于查找一个最大日期,代码逻辑大约是这样的: 如果没有相关的日期记录,则返回当前日期,否则,当日期值为空时,是业务限制异常,并且配置的 相关日期小于当前日期的话,还是应该选择当前日期。代码如下:

public String getPrivMaxDate(){
    String currDate = DateUtil.parse(new Date());
    String maxDate = currDate;
    
    List<String> privMaxDates = ...; //load data from database
    if(Objects.isNotEmpty(privMaxDates)){
        maxDate = privMaxDates.get(0);
        if(StringUtils.isEmpty(maxDate)){
            throw new BusinessException("...");
        }
        else if(maxDate.compareTo(currDate) < 0)
            maxDate = currDate;
        }
    }
    return maxDate;
}

大家应该被我的描述搞晕,我也觉得说得很乱。从代码看,maxDate有好几处地方可以修改,有时候是当前日期,有时候是配置的日期,的确比较乱。 接下来我们尝试对代码进行一些简单的修改,看看效果。

移动代码,快速逃离

接下来,我们要做一些小的调整:

  1. 反转条件,让小逻辑先行,如Objects.isNotEmpty(privMaxDates)的判断
  2. 避免修改变量,让代码简单化,如直接使用currDate
  3. throw本身属于返回值的一种,所以在它之后的代码可以简化,例如代码中的else if
public String getPrivMaxDate(){
    String currDate = DateUtil.parse(new Date());
    
    List<String> privMaxDates = ...; //load data from database
    if(Objects.isEmpty(privMaxDates)){
        return currDate;
    }

    String maxDate = privMaxDates.get(0);
    if(StringUtils.isEmpty(maxDate)){
        throw new BusinessException("...");
    }

    if(maxDate.compareTo(currDate) < 0)
        return currDate;
    }
    return maxDate;
}

经过改造,由于maxDate只赋值一次,代码变得好理解了。

灵活的结构调整

借助eclipse提供的重构功能,可以对代码进行调整。对于我们常见的if语句,可以ctrl+1就有很多神奇的提示。 例如if反转,添加else,切换成三元表达式等手法。

#####对于if语句里边有很大块的代码,可以考虑使用反转,让另外一个分支先处理。 例如上面的例子。另外,对有些只有if没有else的代码块,在操作手法上可以先用ctrl+1添加else,再进行反转操作。

#####对于很小的if-else语句,可以考虑转换成三元表达式。如下面代码示例

String forward = null;
if(success){
    forward = "success";
}
else{
    forward = "fail";
}

// or this way
String forward = success ? "success" : "fail";

#####对于存在多次赋值的情况,如果发现已经是最终的返回值,在调整时可以使用return,理清逻辑。 例如上面的例子。同时,经过分解也便于对复杂的代码进行封装抽取更小的方法。

后话:单赋值还有其他一些好处,例如便于调试定位, 在eralng中所有的变量都是单赋值的,没接触过是很难想象是怎样的一种场景。有兴趣可以去了解一下。

"improve bitter code"系列文章:

improve bitter code: '不可避免'的重复

备注: 示例中的代码并不是真实代码的完全拷贝

一段分批处理的逻辑

周六做code diff的时候发现B项目一段颇长的处理逻辑(40行左右)。处理流程是这样的, 从页面上取到一批数据之后,用这批数据封装参数进行后台调用(远程调用)。为了避免数据调用超时, 对这批数据进行分批多次调用。代码如下所示(现实的代码比这个要复杂些,并且没有使用subList方法):

List batchdatas = ...;
int batchsize = 10; // load from parameter

int datasize = batchdatas.size();
if(datasize > batchsize){
    int batch = datasize / batchsize;
    for(int i=0;i<batch;i++){
        List inparams = new ArrayList();
        List batchdata = batchdatas.subList(batchsize*i, batchsize*(i+1));
        inparams.add(new CEntityList(batchdata));
        RemoteCall.commonInvoke(operator, inparams, "CheckCmd");
    }

    int left = datasize % batchsize;
    if(left != 0){
        List inparams = new ArrayList();
        List batchdata = batchdatas.subList(batchsize*batch, datasize);
        inparams.add(new CEntityList(batchdata));
        RemoteCall.commonInvoke(operator, inparams, "CheckCmd");
    }
}
else {
    List inparams = new ArrayList();
    inparams.add(new CEntityList(batchdatas));
    RemoteCall.commonInvoke(operator, inparams, "CheckCmd");
}

从代码的实现看,思路还是比较清晰的。如果不足一次,就一次提交。否则计算出一共要分多少次, 然后逐次处理提交,最后还要判断是否还有剩余的,如果有就再处理一次。

代码显现出来的问题也比较明显,就是远程调用的逻辑出现了重复。

算法小调整,避免重复

有没什么办法可以避免重复? 或许有些童鞋第一反应是给这几句代码抽取成小方法。 不过这里有更好的办法,首先细想一下就会发现不足一次的判断(datasize > batchsize)不是必要的, 如果计算批次而是计算每次的起始点和结束点,上面两个分支也可以合并一下。调整后代码如下:

List batchdatas = ...;
int batchsize = 10; // load from parameter

int datasize = batchdatas.size();
for(int startidx=0;startidx<datasize;startidx+=batchsize){
    int endidx = (startidx+batchsize) > datasize ? datasize : (startidx+batchsize);

    List inparams = new ArrayList();
    List batchdata = batchdatas.subList(startidx, endidx);
    inparams.add(new CEntityList(batchdata));
    RemoteCall.commonInvoke(operator, inparams, "CheckCmd");
}

其中计算endidx使用了一个三元表达式,使用三元表达式用来替代一些简单的if-else语句是个实用的小技巧。 代码量缩小为原来的三分之一,代码少了,维护量也轻松了。

类似这样的代码也并不少见,例如计算总页数的分页逻辑有下面的写法

// 常用做法
int totalpage = totalsize / pagesize;
if(totalsize % pagesize != 0){
    totalpage++;
}

// 另外一种写法
int totalpage = (totalsize + pagesize - 1) / pagesize;

基本功很重要

java是一门比较古板的语言,大多数情况下,写出来的代码也是大同小异的。 同时,java相关框架又特别的多,很容易拣了芝麻丢了西瓜。 以来面试的童鞋为例,连基本算法的时间复杂度都没弄清楚的人不在少数,所以 在项目代码中,经常看到化简为繁的代码,现在也很习惯了。

"improve bitter code"系列文章:

improve bitter code: 看不懂的正则表达式

备注: 示例中的代码并不是真实代码的完全拷贝

偶然的发现

今天好奇浏览了一下N项目的代码变更历史,发现有人提交了一段关于校验文件格式的代码。 其中包括一段校验日期格式的java代码。代码是这样的:

String validDateStr = // read from file lines
String regex = "(([0-9]{3}[1-9]|[0-9]{2}[1-9][0-9]{1}|[0-9]{1}[1-9][0-9]{2}|[1-9][0-9]{3})(((0[13578]|1[02])(0[1-9]|[12][0-9]|3[01]))|((0[469]|11)(0[1-9]|[12][0-9]|30))|(02(0[1-9]|[1][0-9]|2[0-8]))))|((([0-9]{2})(0[48]|[2468][048]|[13579][26])|((0[48]|[2468][048]|[3579][26])00))0229)";
if(validDateStr.matches(regex)){
    // do something
}

看到这个正则表达式,我立马纠结了,这个正则表达式不知是什么意思。 虽然前几天写代码的同事来问过怎么写校验日期的正则,我当时比较忙,叫他找找有没有现成的。 这次看到这个正则,还是被雷了一把。

于是我问了一下,原来这个正则是校验日期格式,不过加了闰年的判断,所以变得相当复杂。 我晚上还去搜了一下,大概是出自这里的吧!不同的是文中判断的是YYYY-MM-DD的格式,而同事的代码 是判断YYYYMMDD的格式,显得更为难懂。

保持代码的可读性、可维护性

对于这种拿来的复杂代码,的确很cool,不过即使今天你看懂了,别人不一定看得懂,也难保过些日子自己也看不懂了。

所以通常需要一些保持代码可读性、可维护性的手段:

  1. 加多几段注释,或者把来源url标注一下,就像有人喜欢标注那个需求一样。
  2. 把正则弄成常量,并把验证方法封装起来,只需调用method就可以了。
  3. 选择另外一种比较清晰的实现方式, another way, 或许有惊喜。

应该说,这几种情况都应该考虑一下,例如对于上面的例子来说,要使用这么复杂的正则,加上一些简单的注释 是相当有必要的,至少要说明你是想验证什么样的格式。更进一步,封装到方法里边去,例如

public static boolean isStrictYYYYMMDD(String datestr){
    return datestr.matches(STRICT_YYYYMMDD_REGEX);
}

不过这里有个缺点,只能校验一种日期格式,因为日期格式不像邮箱地址,它的形式多样,这样处理能得到的收益并不是很高。 如果我们可以传递校验日期的格式就更好了。

换个实现方式

换个思路,如果不使用正则表达式会怎样,例如SimpleDateFormat就提供了严格验证的格式,示例代码如下:

public static boolean isStrictYYYYMMDD(String datestr){
    SimpleDateFormat format = new SimpleDateFormat("yyyyMMdd");  
    //设置为严格验证模式
    format.setLenient(false);
    try{
        format.parse(str);
        return true;
    } catch (ParseException e) {
        // ignore exeeption
    }
    return false;
}

如果没有设置为严格验证模式的话,20090230这种日期就会变成20090302。

相对于上面正则的方式来说,代码是多了几行,但是因为格式可以变,灵活性有所提高,代码也容易理解了。 另外一方面,由于SimpleDateFormat非线程安全,必须每次都定义一个,在多次处理的情况下显得有些多余。 当然有个折中的方法就是由客户端代码构造format作为传输传递,这样做还有个好处就是,验证日期格式的方法完全就是通用的。

例如,我们可以提供下面的api和调用方式:

//client
SimpleDateFormat format = // 由客户端代码构建format
boolean isDate = DateUtil.isDateFormat(datestr, format);

//DateUtil api
boolean isDateFormat(String datestr, SimpleDateFormat format);
boolean isDateFormat(String datestr, String formatstr);//单次调用
boolean isStrictDateFormat(String datestr, String formatstr);//单次使用,用于严格处理

在不改变接口的情况下,最初的代码可以调整成以下形式

public static boolean isStrictYYYYMMDD(String datestr){
    return isStrictDateFormat(datestr, DateUtil.YYYYMMDD);
}

总结

  1. 隐藏某些复杂的细节是必要的,提供的接口要simple, clear。
  2. 封装有助于焦距局部代码,即使要更换实现方式,也更加easy。
  3. 可以提供通用可定制接口和常用特殊化接口,方便client调用。
  4. commons-langjoda-time开源库提供了非常成熟的解决方案。

"improve bitter code"系列文章:

improve bitter code: 更友好的链式写法

备注: 示例中的代码并不是真实代码的完全拷贝

无意出现的链式代码

还是前几天的事情,群里边有同事提到一个账单相关的需求,其中涉及到组装打印用数据。 有个新同事在Service里边写了一段这样的代码:

public XXService fillItem(String content, String tag) {
    // some codes
    this.printitems.add(new Item(tag, content));
    return this;
}

有同学不是特别理解为什么要这么写,甚至发出return this是什么对象的疑问。

这段代码原来是这么写的:

public void fillItem(String content, String tag, List printitems) {
    // some codes
    printitems.add(new Item(tag, content));
}

有同学奇怪,为什么再提交一次printitems就会重新加一遍,认为第一段代码的问题出现在return this上面。 这样要说明一下,我们使用的是struts1,所以如果Service实例作为action的实例变量,那也是只有一个对象来的。 所以每次刷新一次重复加一遍就没什么奇怪的了。

回头来看那段新写的代码,出发点是好的。毕竟采用这种方式可以使用链式写法(如下所示),代码有时候会变得很sexy。

XXService.fillItem("a", "A").fillItem("b", "B");

这段代码的问题不在于是否采用链式写法,而是对这种单例,变量生命周期了解不足造成的。 链式写法在代码中很少会使用到,但在设计api,也是可以考虑的。设计的好,可以有效提高代码的编写效率和api的友好型。 例如,我就对java collection api里边的add方法深恶痛绝,就是不能连续add,要add多少个就要写多少行,还真的挺烦的。

采用链式写法的代码

在很多有名的开源框架中,链式写法也不是很少见,下面举几个例子:

很常见的有jquery

$("#name").attr("readonly", true).val("test");

还有mock框架mockito

when(mockedList.get(0)).thenReturn("first");
when(mockedList.get(1)).thenThrow(new RuntimeException());

再看看rails的写法

Post.where('id > 10').limit(20).order('id desc').only(:order, :where)
Client.limit(5).offset(30)

我的看法

那么,如果你想采用链式写法,有什么地方需要注意呢?

  1. 先写一下客户端代码,看调用方式合理么,符合sexy api么?
  2. 采用链式操作的interface相关性比较强,经常一起出现。
  3. 参数parameter一般较少,因为参数多了,代码很容易变得模糊不清。
  4. 链式操作要么容错强(像jquery),要么就得直接抛出异常,对返回值不关心。

"improve bitter code"系列文章: