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

1.代码逻辑不清晰

• origin

            CommerceItem mergeItem = null;
            List items = getNgpCartModifierFormHandler().getOrder().getCommerceItemsByCatalogRefId(baseCommItem.getCatalogRefId());
            if (items != null && items.size() > 1) {
                Iterator key = items.iterator();
                while (key.hasNext()) {
                    NGPCommerceItemImpl item = (NGPCommerceItemImpl) key.next();
                    if (!item.getRepositoryItem().getRepositoryId().equals(baseCommItem.getRepositoryItem().getRepositoryId()) && item.getWarrantyItem() == null
                        && !getQuoteTools().isQuoteItem(baseCommItem) && !getQuoteTools().isQuoteItem(item)) {
                        mergeItem = item;
                    }
                }
            }

上述代码存在的问题：
①对常用API不够熟悉，“items != null && items.size() > 1”只是对集合item进行至少有一个元素的判断，可用“CollectionUtils.size(items)>1”代替，使代码更精炼可读。
②“item.iterator()“可用forEach代替，forEach反编译后的字节码就用使用迭代器，使用foreach在源码上看上去更精炼。
③变量名字不规范，”item.iterator()“的返回值是一个迭代器类型对象，应用”itor"或"iterator”，而“keys”代表一个键的集合，只有map类型有这个概念，这里用“keys”命名容易让人产生误解。
④if判断条件太长，让人不知道具体要判断什么东西
⑤代码表达错误的意图
while循环中mergeItem 最终的值是最后一次满足if条件的循环中所赋的值，以前的遍历赋值的结果会被最后的遍历给覆盖掉，之前的遍历中的if条件判断是没有意义的，浪费系统资源。如果写代码的程序员真实的意图就是这样，他应该从后向前遍历，满足if条件则退出循环。而后面了解相关业务后才知道，理论上这个item集合中最多只有一个元素满足if条件，而原代码的写法明显没有表现出这种意图，容易让人产生误解，也浪费了系统资源。

• corrected

        CommerceItem mergeItem = null;
            List items = getNgpCartModifierFormHandler().getOrder().getCommerceItemsByCatalogRefId(baseCommItem.getCatalogRefId());
            if (CollectionUtils.size(items)>1) {
                for (Object item : items) {
                    if (item instanceof NGPCommerceItemImpl) {
                        NGPCommerceItemImpl itemImpl = (NGPCommerceItemImpl) item;
                        boolean isMerged = !itemImpl.getRepositoryItem().getRepositoryId().equals(baseCommItem.getRepositoryItem().getRepositoryId())
                            && itemImpl.getWarrantyItem() == null
                            && !getQuoteTools().isQuoteItem(baseCommItem) 
                            && !getQuoteTools().isQuoteItem(itemImpl);
                        if (isMerged) {
                            mergeItem = itemImpl;
                            break;
                        }
                    }

                }
            }

2.变量名表达的含义南辕北辙

谁能看出来下面的代码要干啥，代码片段“Long.parseLong(currentId) != commerceItem.getQuantity()”在将商品唯一标识id与商品数量比较是否相等，这两个完全不在一个维度的概念可以进度比较吗，它们的比较有什么意义。
后来通过与同事交流，才知道代码片段“pRequest.getParameter(commerceItem.getId())“背后的含义，前端将商品id作为参数名、商品最新数量为参数值的方式传递到后端，那么此时其返回值叫作”currentId“就非常不全时宜了，应该叫做"modifiedQuantity"或"modifiedQuantityForTextFormat”。

• origin:

    for (CommerceItem commerceItem : commerceItems) {
            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);
                        //...省略相关代码
                    }
                }
            }
        }

• corrected

        for (CommerceItem commerceItem : commerceItems) {
            String modifiedQuantityForTextFormat = pRequest.getParameter(commerceItem.getId());
            if (StringUtils.isNotBlank(modifiedQuantityForTextFormat)) {
                if (commerceItem instanceof GCNGPLessonsCommerceItemImpl) {
                    if (Long.parseLong(modifiedQuantityForTextFormat) != commerceItem.getQuantity()) {
                        vlogError("preSetOrderByCommerceId() Lessons quantity can not be changed to:{0}", currentId);
                    }
                }
            }
        }

3.非空或非null条件顺序错误

origin:

    private Map<RepositoryItem, RepositoryItem> priceDropAlerts = new HashMap <RepositoryItem,RepositoryItem>();
    public void createAlertEmails() {
        //...省略
        if(!priceDropAlerts.isEmpty() && null != priceDropAlerts){
            addAndSendAlertMessage(priceDropAlerts);
            //...省略
        }
        //...省略
    }

上面的if条件表达式明显不正确，若priceDropAlerts为null，if条件表达式必然抛出空指针异常。实际上priceDropAlerts是实例变量，在属性定义时就已经被初始化了，那么表达式“null != priceDropAlerts”是否可以省略掉？回答是，不可以的，因为在各个方法体的某个代码片段中可能将priceDropAlerts的引用赋为null了。原始的代码就这样写的，且程序一直都运行正常，可以推测priceDropAlerts在初始化后，它的引用一直没有被修改，但为了避免程序员的误读、使代码一目了然，我还是决定重构这个if条件表达式。

corrected:

    private Map<RepositoryItem, RepositoryItem> priceDropAlerts = new HashMap<RepositoryItem, RepositoryItem>();

    public void createAlertEmails() {
        //...省略
        if ( priceDropAlerts != null  && !priceDropAlerts.isEmpty()) {
            addAndSendAlertMessage(priceDropAlerts);
            //...省略
        }
        //...省略
    }

4.代码冗长

示例1：

origin:

List<PaymentGroup> pgList = order.getPaymentGroups();
Iterator<PaymentGroup> it = pgList.listIterator();
while (it.hasNext()) {
    PaymentGroup paymentGroup = it.next();
    if (!(paymentGroup instanceof NGPPaypalPaymentGroup)) {
        continue;
    } else {
        NGPPaypalPaymentGroup payPalPayment = (NGPPaypalPaymentGroup) paymentGroup;
        if (payPalPayment.getSessionToken() == null || payPalPayment.getSessionToken().equalsIgnoreCase("")) {
            vlogDebug("removeInvalidePaypalPaymentGroup:should remove paypal payment group");
            //...省略
        }
    }
}

原始代码存在的问题：
①使用迭代器遍历集合。如果不需要添加或删除元素，只是读取元素，使用for-each代码更整洁漂亮
②乱用continue ,continue会改变代码的执行入口，另外还用了取反运算符!，取反运算符!会使代码不容易理解。
③复制粘贴代码、未认真理解业务，空字符串的比较不需要大小写转换.
④未使用已有类库的API，自己重复造轮子，StringUtils工具类完全能满足日常的字符串操作。
⑤未理解&&的精髓，&&可用于条件合并(spring框架源码大量使用这种写法)。

corrected:

List<PaymentGroup> pgList = order.getPaymentGroups();
for (PaymentGroup pg : pgList) {
    if (paymentGroup instanceof NGPPaypalPaymentGroup &&
            StringUtils.isEmpty(((NGPPaypalPaymentGroup) payPalPayment).getSessionToken())) {
        vlogDebug("removeInvalidePaypalPaymentGroup:should remove paypal payment group");
        //...省略
    }
}

示例2：

origin:

    protected boolean indicatedProfilePropertyChanged() {
        for(String property : profileChangeCheckingProperty){
            Object newValue = getValue().get(property);
            Object oldValue = getProfile().getPropertyValue(property);
            if(oldValue == null && newValue == null){
                continue;
            } else if (oldValue != null && newValue != null && oldValue.equals(newValue)){
                continue;
            } else {
                return true;
            }
        }
        return false;
    }

原代码存在的问题：

①for-each循环中只有一个if-else块，没有其他代码块，continue关键字没有任何意义。
②这里根本不需要if-else，只用一个if语句就行了。原来的if-else主要用来判断变量是否空，但从JDK1.7开始引入了Objects类，它可以减少空指针判断的模板代码。

corrected:

    protected boolean indicatedProfilePropertyChanged() {
        for (String property : profileChangeCheckingProperty) {
            Object newValue = getValue().get(property);
            Object oldValue = getProfile().getPropertyValue(property);
            if (!Objects.equals(oldValue, newValue)) {
                return true;
            }
        }
        return false;
    }