代码 if 嵌套过多，怎么优化比较好

V2EX = way to explore

V2EX 是一个关于分享和探索的地方

现在注册

已注册用户请登录

这是一个创建于 2109 天前的主题，其中的信息可能已经有所发展或是发生改变。

场景保存信息时，发现手机号重复返回 false 编辑时，手机号与自身之外的手机号重复，返回 false, 没有重复，或者编辑时号码不变返回 true

    private boolean checkMobileExists(Long shopId, CreateSupplierParam param) {
        List<Supplier> supplierList = this.get(shopId, param.getContactMobile());
        if (!CollectionUtils.isEmpty(supplierList)) {
            if (supplierList.size() == BaseConstants.INT_ONE) {
                Long existId = supplierList.get(BaseConstants.INT_ZERO).getId();
                if (Objects.equals(existId, param.getId())) {
                    return true;
                }
            }
            return false;
        }
        return true;
    }

51 条回复 • 2019-02-20 10:04:07 +08:00

2019-02-19 10:47:32 +08:00

不知道大佬怎么写的，要是我一般会写成连续的。
if() return;
if() return;
if() return;

whx20202

2019-02-19 10:50:33 +08:00

代码整洁之道原则：提前 return

if(不满足)：
return
if （不满足）：
return

如果你的多层 if else 都有代码块，那会复杂一点，但是也可以优化

yesterdaysun

2019-02-19 10:52:42 +08:00

试试 guard clause

```java
private boolean checkMobileExists(Long shopId, CreateSupplierParam param) {
List<Supplier> supplierList = this.get(shopId, param.getContactMobile());
if (CollectionUtils.isEmpty(supplierList)) {
return true;
}
if (supplierList.size() != BaseConstants.INT_ONE) {
return false;
}
Long existId = supplierList.get(BaseConstants.INT_ZERO).getId();
return Objects.equals(existId, param.getId());
}
```

LxExExl

2019-02-19 10:53:42 +08:00 via iPhone

返回 true 要么为空要么成员唯一且和输入相等

if (list is null || list.size() == 1 && list.getFirst() == input) return true

else return false

danliuwo

2019-02-19 10:57:21 +08:00

switch

wutiantong

2019-02-19 10:59:30 +08:00

return CollectionUtils.isEmpty(supplierList) || supplierList.size() == BaseConstants.INT_ONE && Objects.equals(supplierList.get(BaseConstants.INT_ZERO).getId(), param.getId());

一条语句搞定，假如你搞不清||和&&的优先级关系，就自己加个括号吧。

taaaang

2019-02-19 11:00:47 +08:00

可以看看《重构改善既有代码的设计》

BingoXuan

2019-02-19 11:03:02 +08:00

连续 if 可读性更高，更容易理解。可读性和代码行数不是成反比的，有时候提高可读性，你需要更多的代码。虽然看起来很冗余，但实际上会让阅读代码的人理解起来更加清晰。

如果 python 这种缩进严格的语言，而你同事喜欢这样嵌套写。淘宝买把游标卡尺来 review 会比较好。

felixlong

2019-02-19 11:12:27 +08:00

你这 code 有 bug 啊。supplierList 为空为什么返回的是 true?

yesterdaysun

2019-02-19 11:14:07 +08:00

BaseConstants.INT_ONE 和 BaseConstants.INT_ZERO 应该就是 int 0 和 1 吧, 需要定一个常数吗?

单单为了避免魔数也不需要做到这个程度吧

MrUser

2019-02-19 11:20:20 +08:00

exists = false;
if (!exists) {}
if (!exists) {}
if (!exists) {}
return exists;

kaedea

2019-02-19 11:20:24 +08:00 via Android

scoping function 了解一下

wleexi

2019-02-19 11:22:36 +08:00

@felixlong 额不是的。为空表示新号码，可以通过检查的

qq976739120

2019-02-19 11:23:01 +08:00

典型的箭头型代码块....如何解决楼上已经说得很好了

AngryPanda

2019-02-19 11:23:05 +08:00

尽早 return

Raymon111111

2019-02-19 11:24:51 +08:00

提早 return 是正确答案.

wleexi

2019-02-19 11:26:07 +08:00

@wutiantong 想过这种，但是考虑了可读性可能不是特别高

wleexi

2019-02-19 11:27:25 +08:00

@qq976739120 不太明白

billgreen1

2019-02-19 11:27:52 +08:00

表驱动，代码大全里面提到的

Mutoo

2019-02-19 11:30:21 +08:00

BaseConstants.INT_ONE
BaseConstants.INT_ZERO

0 和 1 本身就有自带语议了，不属于魔数，没必要搞个这么长的常量吧

fuxiuyin

2019-02-19 11:40:39 +08:00

之前在哪看到一个感觉挺不错的，记得是 windows SDK 里面的风格？
// 初始化资源
int result = 0;
while(1)
{
if(xxxx)
break;
if(xxxx)
break;
// logical
result = 1;
break;
}
// 释放资源
return result;
确保一个函数一个 return，将来该起来比较容易看，也不会到处都是 return 导致忘记释放某些资源。

wutiantong

2019-02-19 11:46:33 +08:00

@wleexi 这个写法完全符合语法而且也充分表达了逻辑含义，根本算不上什么奇技淫巧，为什么可读性就不高了呢？

6diyipi

2019-02-19 11:53:20 +08:00

```
private boolean checkMobileExists(Long shopId, CreateSupplierParam param) {
List<Supplier> supplierList = this.get(shopId, param.getContactMobile());

if (!CollectionUtils.isEmpty(supplierList) && supplierList.size() == BaseConstants.INT_ONE) { return false; }

if (supplierList.size() != BaseConstants.INT_ONE) { return false; }

Long existId = supplierList.get(BaseConstants.INT_ZERO).getId();

if (!Objects.equals(existId, param.getId())) { return false; }

return true;
}
```
这样？

6diyipi

2019-02-19 11:55:22 +08:00

写错一个条件了，发出去的居然不能编辑了。。

private boolean checkMobileExists(Long shopId, CreateSupplierParam param) {
List<Supplier> supplierList = this.get(shopId, param.getContactMobile());

if (!CollectionUtils.isEmpty(supplierList) && supplierList.size() != BaseConstants.INT_ONE) { return false; }

if (supplierList.size() != BaseConstants.INT_ONE) { return false; }

Long existId = supplierList.get(BaseConstants.INT_ZERO).getId();

if (!Objects.equals(existId, param.getId())) { return false; }

return true;
}

wleexi

2019-02-19 11:56:52 +08:00

@wutiantong 表达式太长了？

wutiantong

2019-02-19 11:57:28 +08:00

@wleexi 那就分行呗。

lhx2008

2019-02-19 11:58:16 +08:00

#3 @yesterdaysun 是我喜欢的风格。

hugedeffing

2019-02-19 12:07:08 +08:00

用工厂模式，或者继承来做啊，if 不就是多个条件嘛

Sapp

2019-02-19 12:19:11 +08:00

你这个反向 return 就行了，有啥优化难度

Kylinsun

2019-02-19 12:40:11 +08:00 via iPhone

用断言。

zzzzzzZ

2019-02-19 13:56:46 +08:00

private boolean checkMobileExists(Long shopId, CreateSupplierParam param) {
List<Supplier> supplierList = this.get(shopId, param.getContactMobile());
if(supplierList == null || supplierList.size() == BaseConstants.INT_ONE){
return false;
}
Long existId = supplierList.get(BaseConstants.INT_ZERO).getId();
if (CollectionUtils.isEmpty(supplierList) && Objects.equals(existId, param.getId())) {
return true;
}
}

分支结构直接 return，某些场合可以无视嵌套前提（像行 3 和行 4 根本不相关）
分开写是基本功，增加可读性
如果不想写逻辑运算符也可以分开写多个 if，也能增加可读性

LevineChen

2019-02-19 13:57:51 +08:00

do while + break 方案比较舒服, return 太多很混乱一个函数一个出口一个入口最好

zzzzzzZ

2019-02-19 14:05:35 +08:00

总感觉你的逻辑哪里有点不对劲，可能跟参数和变量太奇葩有关，说不上来
我算了下按照你的描述漏了一个分支在最后，但是脑袋有点晕，最后少一个 if-else 你自己看看吧

JRay

2019-02-19 14:06:26 +08:00

卫语句？

ttvast

2019-02-19 14:12:30 +08:00

抛异常

yetone

2019-02-19 14:23:14 +08:00

防御式编程

nekoneko

2019-02-19 14:33:37 +08:00

BaseConstants.INT_ZERO 简直是魔鬼。。。。

vicvinc

2019-02-19 15:08:25 +08:00

最简单的

vicvinc

2019-02-19 15:10:23 +08:00

ctl+enter 直接发送了（🤦‍♂️
补充一下：

if (条件 1)
if (条件 2)
...
if(条件 N)

可以改写成：

if (条件 1&&条件 2&&...&&条件 N)

shot

2019-02-19 15:29:15 +08:00

像这种判断条件不算太多的情况，写成 if-else 问题不大，可读性好且符合团队编码规范就行。

如果判断条件比较多（我个人习惯是 5 个及以上），可以把每个判断逻辑写成一个 lambda 函数，再循环调用它们。
```
private boolean checkMobileExists(Long shopId, CreateSupplierParam param) {
List<Supplier> supplierList = this.get(shopId, param.getContactMobile());

List<Predicate<List<Supplier>>> predicates = Arrays.asList(
CollectionUtils::isEmpty,
suppliers -> suppliers.size() == BaseConstants.INT_ONE &&
suppliers.stream().anyMatch(it -> Objects.equals(it.getId(), param.getId()))
// more predicates here
);

return predicates.stream().anyMatch(it -> it.test(supplierList));
}
```

jmk92

2019-02-19 18:06:50 +08:00

private boolean checkMobileExists(Long shopId, CreateSupplierParam param) {
List<Supplier> supplierList = this.get(shopId, param.getContactMobile());
if (CollectionUtils.isEmpty(supplierList)) return false;
if (supplierList.size() != BaseConstants.INT_ONE) return false;
Long existId = supplierList.get(BaseConstants.INT_ZERO).getId();
if (Objects.equals(existId, param.getId())==false) return false;
return true;
}

biossun

2019-02-19 18:32:55 +08:00

只是展开嵌套的话，可以写成：

private boolean checkMobileExists(Long shopId, CreateSupplierParam param) {
List<Supplier> supplierList = this.get(shopId, param.getContactMobile());

if (CollectionUtils.isEmpty(supplierList)) {
return true;
}

if (supplierList.size() == BaseConstants.INT_ONE &&
Objects.equals(supplierList.get(BaseConstants.INT_ZERO).getId(), param.getId())) {
return true;
}

return false;
}

codingKingKong

2019-02-19 18:50:41 +08:00

是不是大概这样?
```java
private boolean checkMobileExists(Long shopId, String mobile, Long id) {
List<Long> supplierList = this.get(shopId, mobile);

if (!CollectionUtils.isEmpty(supplierList))
return true;
if (supplierList.size() != 1)
return false;
Long existId = supplierList.get(0);
return Objects.equals(existId, id);

}

private List<Long> get(long shopId, String mobile){
return Collections.emptyList();
}
```