大家帮我看看，这代码是水平。。

[Wsdba]

刚接手的一个项目，发现这个人很喜欢这样写。

[fml87]

楼里居然有相当部分的人认为这种是好代码，这种代码习惯一两周就能堆出大几千行的屎山

[xuanbg]

if (member1 == null || member1.equals(member2)){
return false;
}

changePartyPosition(...);

[xuanbg]

@DreamingCTW 你可能看错了，数据验证逻辑应该是：member1 不能为空，且 member1 不能与 member2 相同。

[ryd994]

1 应该使用 &&
2 可以使用 fail fast pattern ，也可以使用&&

怎么说呢，可读性不好，但是实际性能没啥区别，毕竟编译器啥都能给你优化掉

[snw]

@xuanbg
如果 number1==null 且 number2!=null ，原代码是会执行 ChangePartyPosition 的，和你的代码不同。

[hackk]

请问只有我看不到 LZ 发的代码吗？

[xuanbg]

@snw 嗯嗯，这样可以简化为：
return Object.equals(member1, member2) ? false : changePartyPosition(...);

[snw]

如果业务逻辑预计一年以上不会变，那么可以尝试优化。如果预计一年以内会变化，那么楼主图中这种代码其实挺好的。

你看上面回帖的代码，至少已经有 2 位写 bug 了🐶

[xuanbg]

不过换我来写代码的话，判断 member1 是否和 member2 相同这个逻辑肯定是写在 changePartyPosition 这个方法里面的。

[justest123]

这个帖子从昨天看到今天，有个有趣的现象，有贴自己“精简”代码的，好像不是被人说逻辑写错了，就是被从其他方面挑出“问题”
逻辑易懂跟代码精简貌似不是那么好兼得（狗头（逃

[justfindu]

为什么会觉得一定要把代码合并缩进, 缩写 if 才是高端写法, 这样写法后来接手人不是能够一眼就看出逻辑吗. 这样写法整体会浪费资源吗?

[lskjdfgl]

@hackk 尝试换个梯子?

[xuanbg]

@justest123 不是，恰恰是因为楼主贴的代码表达的逻辑晦涩难明，才造成大家的理解错误。经过严密推导，逻辑其实简单到令人发指：就是判断 member1 是否和 member2 相同，相同就返回 false ，不同就执行 changePartyPosition 方法。为啥要写这么复杂，我猜是因为需要预防空抛出指针异常而已。

[chenshun00]

难道没有人想到把这几个参数聚合成一个对象，然后这些判断放到对象里边去操作么。就算后边要加参数，要改变对象行为不是很 easy

[BlueCropland]

哈哈，仿佛看了以前的自己， 堆 shi 山

[BlueCropland]

6666

[pengjl]

看到了以前的自己

[steptodream]

这个问题是普遍存在的，本质就是很难在别人眼里不是傻 x ，最典型场景开车

[1109599636]

接手重构项目的时候碰见这种代码难道不开心吗， 逻辑顺着代码就能看懂， 要是把判断逻辑合并到一行光看懂什么意思就得花好长时间，重构的时候万一没考虑全了还容易出 bug 。。。

[hzw94]

不写注释通通捶死！

拿到无效数据(null)就应该直接 return 结束代码，简介明了。不该 if..else 判空再执行。

[chihiro2014]

这代码写得好，让人有工作量

[Cloutain]

可读性还行 ，反正编译器会合并条件

[chenyu0532]

没有注释，没有 log
除了这两个我不觉得有什么不妥。
可改进，但是个人感觉意义不大
接手的人不用费脑子一看就能明白

[hejw19970413]

第一点 ： 两个函数高度相似能不能合并成一个
第二点 ： 可以先行判断的前提条件可以提到前面部分
第三点 ： 可以直接返回的判断是不是可以不用嵌套

我感觉简单改可以按这个改 。

[sky101001]

又不是看不懂
要求项目代码的每一行都经得起推敲才是强人所难，屎山是在所难免的，这种算不上好代码，但好歹一眼就能看懂，没必要特地拎出来说

[hakr]

@starsky007 #19 是的, 我也喜欢这样写, 先逆向判断, 先处理逻辑少的条件

[SS0001]

也不用过度考虑简化，像楼上说的，简化也要看值不值嘛。
非要说优化的话 图 1 已经很多朋友说了，图二的话我个人认为改用 try catch 也可以。
private boolean changePartyPositions(String member1, String member2, String position...) {
boolean flag = false;
try {
JSONObject member1Json = partyMemberDao.getInfoById(member1);
JSONObject memberPositionsInfo = memberPositionsDao.getInfo(member1, p...);
flag = memberPositionsDao.delete(member1, positionName, org, updateT...)
} catch (NullPointerException e) {
// do nothing.
}
return flag;
}

[shawnsh]

如果没有事情干，可以换种写法

[ganbuliao]

//个人喜欢的逻辑 觉得更符合阅读习惯
if (member2 == null && member1 != null) {
return changepartypositlons();
}
if (member2 != null && !member2.equals(member1)) {
return changepartypositlons();
}
return false;

[hakr]

https://s1.ax1x.com/2021/12/10/o5VVN4.png

[yl666]

这种代码风格是把自己需要用到的场景写出来了，剩下的全是一个场景，有些考虑不到的场景就很容易走到 else 里面，造成 bug ，我更喜欢把所有场景列出来的方式来写

[anonydmer]

其实楼主第二章图没贴全，导致大家优化不彻底

[hakr]

https://s1.ax1x.com/2021/12/10/o5uC0P.png
第二次优化

[leokino]

@liuzhaowei55 你没理解我的意思，绝大部分情况下这种条件判断粗略看过就可以了，本身就已经非常清楚了，类似这样的代码过多，说明你要多阅读数倍的代码量，才能理解整体代码的意思，实际上大大降低了可读性。

[Quarter]

我觉得挺好的啊，条理清晰的，方便阅读

[crayygy]

append 一个代码版本的？ 图片似乎丢失了(还是我这里 load 不出来...

[comoyi]

结构清晰，按最细粒度划分分支，对未来的变更留了空间，之后迭代只要加 else 代码不需要改 if 条件代码

[aguesuka]

@xuanbg 可以直接用 Objects.equals, 这个函数可以代替 StringUtils.equals

[tqrj]

下面居然有评论觉得这样写挺好的 /。。。。+

[28Sv0ngQfIE7Yloe]

咱先不说条件判断

这个 member 1 、member2 大家都能忍受吗？

[fkdog]

认为这种写法好的显然是不写单元测试的那种。三个嵌套 if 你就能搞出 2^3=8 种分支，你有精力去写这么多的 test-case 吗？

能写出这样代码的一般都是逻辑很混乱的那种，不会去整理思考分支结构的前因后果，然后 debug 的时候发现空指针或者报错，然后顺势往里插一个 if 来解决问题。。

[ytmsdy]

降低预期！降低预期！降低预期！
现在 360 行，行行转码农，进来的人水平参差不齐！
只要是能跑，没 bug ，看得懂的代码，都行。别给我出 bug ，别让我半夜起来修生产环境都行！
否正这种代码看多了会脑溢血，你不接受能怎么办捏？帮他改？教他写？那花的不还是你自己的时间么。
有这时间，多刷几部剧不好么。

[chenshun00]

```java
public class CulContext {

public String member1;
public String member2;
public String name;
public String org;
public String updateTime;

public boolean canChangePartyPositions() {
if (changeCondition()) {
return true;
}
return !Objects.equals(member1, member2);
}

private boolean changeCondition() {
return member2 == null && member1 != null;
}
}
```
===
```java
public class BizService {

public boolean changePositions(CulContext culContext) {
if (culContext.canChangePartyPositions()) {
return changePartyPosition(culContext);
}
return false;
}

private boolean changePartyPosition(CulContext culContext) {
//提前返回降低代码复杂度
//biz
return false;
}
}
```

[tenserG]

@darkcode 图床问题,国内能访问到,梯子不能

[litchinn]

先说我的观点：尽管代码有优化空间，比如判断条件可以合并，但是其实并不严重，至少这么多楼下来大家看懂这个代码的占多数不是吗？
其次就是楼上说时间久了变成屎山，其实任何代码时间久了不重构都会变成屎山，拿这个举例，如果参数 member 再多几个，判断条件的组合就会爆炸（排列组合 Cmn ），这种时候使用只要还是这样 if else 写再怎么合并条件、提前返回还是看不懂的，这种情况我能想到的解决办法就是使用 **规则引擎** 这种模式去做，将每个判断条件的处理变成一个 handler 。

[cominbril]

@starsky007 正解，看评论怎么感觉大家都有点不入行啊

[lolizeppelin]

逻辑确实有问题
这里逻辑确实可以简化
mb1 != mb2 && mb1!=null return true
return false

[siweipancc]

学完重构与代码简洁之道，写出来的代码同事看不懂。（你这代码这么多个 return ，else if 也不嵌套？）年后准备跑路

[statumer]

个人觉得大多数情况，大家眼中只有好深莫测的优美代码和浅显易懂的烂代码。只要能避免 bad case 就不是烂代码。

[snw]

@lolizeppelin
写 bug 了吧🐶

String mb1 = new String("A");
String mb2 = new String("A");
按你的代码会 return true

[akira]

代码不够优雅是水平问题
各种特例有考虑到说明已经用心了

[Lonenso]

封装一个 member 类，把 org 和 name 作为属性，然后实现 swap

[whi147]

提早 return 就行了

[gyh1996]

```java
if (member1 == null ? member2 == null : !member1.equals(member2)) {
return false;
}
return changePartyPositions(member1, member2, name, org, updateTime);
```

[bzl132]

写代码的应该是新手，总体感觉有以下两个问题：
1.方法的设计就有问题，为什么要拆成两个方法，明明就是干一个事情的，入参还都一样
2.一般方法应该就两个部分，一是什么情况下什么也不做直接返回，二是具体要干什么，这些都没考虑清楚，直接根据业务条件来写，后面等业务变化后一定是一坨屎

[tohuer00]

第一个乍一看不觉得有问题，仔细看发现只是为了 equals 避免空指针写了这么一大堆 if ，改成 Objects.equals 就好。

如果是业务条件需要的类似判断场景，那么像图一这么写两层 if 嵌套其实比楼上一些人的全写到一个 if 括号里更容易阅读。

[bzl132]

@tohuer00 合并条件是没问题的，写法上 在 || 换行就能方便阅读了

[wangshanshan]

第二个 感觉 最好不要多层 if 嵌套，这个还好层数不多 不是很复杂。 建议可以反着写 if(a==null) if(b==null)

[season8]

@hackk 同看不到