工作中的重构：提高代码质量（一）

 

1) continue/break语句过多

continue/break本身是循环的流程控制关键字，但不应该滥用，否则将导致代码可读性降低。一个循环体内尽量只出现一次continue/break语句。
origin:

        for (Object commerceItem : commerceItems) {
            if (commerceItem == null) {
                continue;
            }
            NGPCommerceItemImpl lNGPCommerceItemImpl = (NGPCommerceItemImpl) commerceItem;
            if (lNGPCommerceItemImpl.getCatalogRefId() == null) {
                continue;
            }
            if (lNGPCommerceItemImpl.getCatalogRefId().equals(addItems[0].getCatalogRefId())) {
                addedcommerceItem = lNGPCommerceItemImpl;
            }
        }

 

①此处两个continue语句相邻，两者间没有其他的代码（业务逻辑），可以将两个continue语句合并在一起。
corrected1:

  for (Object commerceItem : commerceItems) {
           NGPCommerceItemImpl commerceItemImpl = (NGPCommerceItemImpl) commerceItem;
            if (commerceItemImpl== null || commerceItemImpl.getCatalogId() == null) {
                continue;
            }
            if (commerceItemImpl.getCatalogRefId().equals(addItems[0].getCatalogRefId())) {
                addedcommerceItem = commerceItemImpl;
            }
        }

 

上面的代码还是有些问题的：②commerceItem 到底是不是NGPCommerceItemImpl类型还不一定（这是以前别人写的代码，都在生产环境运行了好几年了，当前它一定是NGPCommerceItemImpl类型），如果未来添加了新的类型NGPCommerceItemImpl2，那是时commerceItem 还恰巧就是NGPCommerceItemImpl2类型，此时就会出现类型转换异常，因此要事先做类型判断。③另外，注意循环体内的重要代码就赋值的一行代码“addedcommerceItem = commerceItemImpl”,continue/break本省是用来减少大段大段的if块代码，continue/break和goto语句类似，都改变了代码执行原有的方向，能不用就尽量不用它。可以用去反条件的if语句替代它。
corrected2:

for (Object commerceItem : commerceItems) {
     if (!(commerceItem instanceof NGPCommerceItemImpl)) {
         continue;
     }
     NGPCommerceItemImpl commerceItemImpl = (NGPCommerceItemImpl) commerceItem;
     if (commerceItemImpl != null || commerceItemImpl.getCatalogId() != null
         && commerceItemImpl.getCatalogRefId().equals(addItems[0].getCatalogRefId())) {
         addedcommerceItem = commerceItemImpl;
     }
}

 

不知道有没有看出来其他可改进的地方？④“commerceItemImpl.getCatalogId() != null”其实没有必要进行非空判断，这里的非空判断主要是对下面的equal()方法做准备，而Objects.equals()方法可以减少非空判断这一步骤。⑤另外，if的判断语句过长可读性差，可以将判断语句的结果赋值给临时变量更好。
corrected3:

        for (Object commerceItem : commerceItems) {
            if (!(commerceItem instanceof NGPCommerceItemImpl)) {
                continue;
            }
            NGPCommerceItemImpl commerceItemImpl = (NGPCommerceItemImpl) commerceItem;
            boolean isValidOfCatalogRefId = commerceItem != null &&
            Objects.equals(commerceItemImpl.getCatalogRefId(), addItems[0].getCatalogRefId());
            if (isValidOfCatalogRefId) {
                addedcommerceItem = commerceItemImpl;
            }
        }

 

2)不知所谓的代码

maxqty 被定义为局部变量，实际上却是个常量，后面的代码没有对其进行修改，这里应将其定义为静态常量。
数组details到底是啥，我将方法读完后都它不知道是什么东西，应该取一个更长具有描述性的变量名。

                int maxqty = 999;
                Object[] details = new Object[2];
                details[0] = maxqty;
                //.....省略后面的代码
                

 

3)判断条件错误

条件明显写反了，如果result 为null,if块内的代码会执行，那么代码
"result.getError(InternationalTools.RESTRICTION_ERROR_CODE).toString()"一定会出现空指针异常！
origin:

            if (result == null || result.hasErrors()) {
                String errMessageKey = result.getError(InternationalTools.RESTRICTION_ERROR_CODE).toString();
               //.......省略若干代码
                }

corrected:

            if (result != null && result.hasErrors()) {
                String errMessageKey = result.getError(InternationalTools.RESTRICTION_ERROR_CODE).toString();
               //.......省略若干代码
                }

 

4）条件合并：

origin：

            String currentId = pRequest.getParameter(commerceItem.getId());
            if (StringUtils.isNotBlank(currentId)) {
                if (commerceItem instanceof GCNGPLessonsCommerceItemImpl) {
                    if (Long.parseLong(currentId) != commerceItem.getQuantity()) {
                        vlogError("preSetOrderByCommerceId() Lessons quantity can not be changed to:{0}", currentId);
                        addFormException(new DropletException(UserMessage.format(UserMessageKeys.KEY_VALIDATION_QUANTITY_FORMAT, new Long[]{new Long(1)}, pRequest.getLocale())));
                    }
                }
            }

 

多个连续的if语句可以用“&&”或“||”减少if块的圈层数，使代码更整齐。
corrected:

            boolean isIncorrectQuantity = StringUtils.isNotBlank(currentId) && (commerceItem instanceof GCNGPLessonsCommerceItemImpl)
                && Long.parseLong(currentId) != commerceItem.getQuantity();
            if (isIncorrectQuantity) {
                vlogError("preSetOrderByCommerceId() Lessons quantity can not be changed to:{0}", currentId);
                addFormException(new DropletException(UserMessage.format(UserMessageKeys.KEY_VALIDATION_QUANTITY_FORMAT, new Long[]{new Long(1)}, pRequest.getLocale())));
            }

 

5）无用的代码

“getFormError()”为只读方法（只读方法的唯一作用是获取返回值），方法本身不会改变某些对象的行为、无副作用。“getFormError()”的返回值又只用来做条件判断，if块内只有“return”一条语句，，而if块已经到方法底部，不管其返回值如何都会结束“xxx()”方法.

public void xxx(){
        //.....省略前面的代码
        if (getFormError()) {
            return;
        }
    }