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