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