工作中的重构：提高代码质量（二）
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; }
