这是一个创建于 1081 天前的主题，其中的信息可能已经有所发展或是发生改变。
刚接手的一个项目，发现这个人很喜欢这样写。
 |
101
fml87 2021-12-10 04:50:12 +08:00
楼里居然有相当部分的人认为这种是好代码，这种代码习惯一两周就能堆出大几千行的屎山
|
 |
102
xuanbg 2021-12-10 06:36:14 +08:00  1
if (member1 == null || member1.equals(member2)){
return false; } changePartyPosition(...); |
|
103
xuanbg 2021-12-10 06:40:07 +08:00
@DreamingCTW 你可能看错了，数据验证逻辑应该是：member1 不能为空，且 member1 不能与 member2 相同。
|
 |
104
ryd994 2021-12-10 07:45:41 +08:00 via Android
1 应该使用 &&
2 可以使用 fail fast pattern ，也可以使用&& 怎么说呢，可读性不好，但是实际性能没啥区别，毕竟编译器啥都能给你优化掉 |
 |
105
snw 2021-12-10 08:23:20 +08:00 via Android
@xuanbg
如果 number1==null 且 number2!=null ，原代码是会执行 ChangePartyPosition 的，和你的代码不同。 |
 |
106
hackk 2021-12-10 08:28:35 +08:00
请问只有我看不到 LZ 发的代码吗？
|
|
107
xuanbg 2021-12-10 08:32:05 +08:00
@snw 嗯嗯，这样可以简化为：
return Object.equals(member1, member2) ? false : changePartyPosition(...); |
|
108
snw 2021-12-10 08:33:57 +08:00 via Android
如果业务逻辑预计一年以上不会变，那么可以尝试优化。如果预计一年以内会变化，那么楼主图中这种代码其实挺好的。
你看上面回帖的代码，至少已经有 2 位写 bug 了🐶 |
|
109
xuanbg 2021-12-10 08:34:40 +08:00
不过换我来写代码的话，判断 member1 是否和 member2 相同这个逻辑肯定是写在 changePartyPosition 这个方法里面的。
|
 |
110
justest123 2021-12-10 08:37:17 +08:00
这个帖子从昨天看到今天，有个有趣的现象，有贴自己“精简”代码的，好像不是被人说逻辑写错了，就是被从其他方面挑出“问题”
逻辑易懂跟代码精简貌似不是那么好兼得（狗头（逃 |
 |
111
justfindu 2021-12-10 08:40:07 +08:00
为什么会觉得一定要把代码合并缩进, 缩写 if 才是高端写法, 这样写法后来接手人不是能够一眼就看出逻辑吗. 这样写法整体会浪费资源吗?
|
|
113
xuanbg 2021-12-10 08:43:14 +08:00
@justest123 不是，恰恰是因为楼主贴的代码表达的逻辑晦涩难明，才造成大家的理解错误。经过严密推导，逻辑其实简单到令人发指：就是判断 member1 是否和 member2 相同，相同就返回 false ，不同就执行 changePartyPosition 方法。为啥要写这么复杂，我猜是因为需要预防空抛出指针异常而已。
|
 |
114
chenshun00 2021-12-10 09:12:46 +08:00 1
难道没有人想到把这几个参数聚合成一个对象，然后这些判断放到对象里边去操作么。就算后边要加参数，要改变对象行为不是很 easy
|
 |
115
BlueCropland 2021-12-10 09:18:47 +08:00
哈哈，仿佛看了以前的自己， 堆 shi 山
|
|
116
BlueCropland 2021-12-10 09:18:53 +08:00
6666
|
 |
117
pengjl 2021-12-10 09:32:48 +08:00
看到了以前的自己
|
 |
118
steptodream 2021-12-10 09:33:46 +08:00
这个问题是普遍存在的，本质就是很难在别人眼里不是傻 x ，最典型场景开车
|
 |
119
1109599636 2021-12-10 09:49:04 +08:00
接手重构项目的时候碰见这种代码难道不开心吗， 逻辑顺着代码就能看懂， 要是把判断逻辑合并到一行光看懂什么意思就得花好长时间，重构的时候万一没考虑全了还容易出 bug 。。。
|
 |
120
hzw94 2021-12-10 10:07:17 +08:00
不写注释通通捶死！
拿到无效数据(null)就应该直接 return 结束代码，简介明了。不该 if..else 判空再执行。 |
 |
121
chihiro2014 2021-12-10 10:13:15 +08:00
这代码写得好，让人有工作量
|
 |
122
Cloutain 2021-12-10 10:16:22 +08:00
可读性还行 ，反正编译器会合并条件
|
 |
123
chenyu0532 2021-12-10 10:16:36 +08:00
没有注释，没有 log
除了这两个我不觉得有什么不妥。 可改进，但是个人感觉意义不大 接手的人不用费脑子一看就能明白 |
 |
124
hejw19970413 2021-12-10 10:18:01 +08:00
第一点 ： 两个函数高度相似能不能合并成一个
第二点 ： 可以先行判断的前提条件可以提到前面部分 第三点 ： 可以直接返回的判断是不是可以不用嵌套 我感觉简单改可以按这个改 。 |
 |
125
sky101001 2021-12-10 10:18:43 +08:00
又不是看不懂
要求项目代码的每一行都经得起推敲才是强人所难，屎山是在所难免的，这种算不上好代码，但好歹一眼就能看懂，没必要特地拎出来说 |
 |
126
hakr 2021-12-10 10:20:27 +08:00
@starsky007 #19 是的, 我也喜欢这样写, 先逆向判断, 先处理逻辑少的条件
|
 |
127
SS0001 2021-12-10 10:22:47 +08:00
也不用过度考虑简化，像楼上说的，简化也要看值不值嘛。
非要说优化的话 图 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; } |
 |
128
shawnsh 2021-12-10 10:31:51 +08:00 via Android
如果没有事情干，可以换种写法
|
 |
129
ganbuliao 2021-12-10 10:32:11 +08:00
//个人喜欢的逻辑 觉得更符合阅读习惯
if (member2 == null && member1 != null) { return changepartypositlons(); } if (member2 != null && !member2.equals(member1)) { return changepartypositlons(); } return false; |
|
130
hakr 2021-12-10 10:58:36 +08:00
|
 |
131
yl666 2021-12-10 11:11:54 +08:00
这种代码风格是把自己需要用到的场景写出来了，剩下的全是一个场景，有些考虑不到的场景就很容易走到 else 里面，造成 bug ，我更喜欢把所有场景列出来的方式来写
|
 |
132
anonydmer 2021-12-10 11:12:19 +08:00
其实楼主第二章图没贴全，导致大家优化不彻底
|
|
133
hakr 2021-12-10 11:17:17 +08:00
|
 |
134
leokino 2021-12-10 13:03:16 +08:00
@liuzhaowei55 你没理解我的意思，绝大部分情况下这种条件判断粗略看过就可以了，本身就已经非常清楚了，类似这样的代码过多，说明你要多阅读数倍的代码量，才能理解整体代码的意思，实际上大大降低了可读性。
|
 |
135
Quarter 2021-12-10 13:08:29 +08:00 via Android
我觉得挺好的啊，条理清晰的，方便阅读
|
 |
136
crayygy 2021-12-10 13:08:59 +08:00
append 一个代码版本的？ 图片似乎丢失了(还是我这里 load 不出来...
|
 |
137
comoyi 2021-12-10 13:13:58 +08:00
结构清晰，按最细粒度划分分支，对未来的变更留了空间，之后迭代只要加 else 代码不需要改 if 条件代码
|
 |
139
tqrj 2021-12-10 14:21:34 +08:00
下面居然有评论觉得这样写挺好的 /。。。。+
|
 |
140
28Sv0ngQfIE7Yloe 2021-12-10 14:50:29 +08:00
咱先不说条件判断
这个 member 1 、member2 大家都能忍受吗？ |
 |
141
fkdog 2021-12-10 15:01:38 +08:00
认为这种写法好的显然是不写单元测试的那种。三个嵌套 if 你就能搞出 2^3=8 种分支，你有精力去写这么多的 test-case 吗？
能写出这样代码的一般都是逻辑很混乱的那种，不会去整理思考分支结构的前因后果，然后 debug 的时候发现空指针或者报错，然后顺势往里插一个 if 来解决问题。。 |
 |
142
ytmsdy 2021-12-10 15:05:16 +08:00
降低预期！降低预期！降低预期！
现在 360 行，行行转码农，进来的人水平参差不齐！ 只要是能跑，没 bug ，看得懂的代码，都行。别给我出 bug ，别让我半夜起来修生产环境都行！ 否正这种代码看多了会脑溢血，你不接受能怎么办捏？帮他改？教他写？那花的不还是你自己的时间么。 有这时间，多刷几部剧不好么。 |
|
143
chenshun00 2021-12-10 16:07:23 +08:00
```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; } } ``` |
 |
145
litchinn 2021-12-10 16:58:54 +08:00
先说我的观点：尽管代码有优化空间，比如判断条件可以合并，但是其实并不严重，至少这么多楼下来大家看懂这个代码的占多数不是吗？
其次就是楼上说时间久了变成屎山，其实任何代码时间久了不重构都会变成屎山，拿这个举例，如果参数 member 再多几个，判断条件的组合就会爆炸（排列组合 Cmn ），这种时候使用只要还是这样 if else 写再怎么合并条件、提前返回还是看不懂的，这种情况我能想到的解决办法就是使用 **规则引擎** 这种模式去做，将每个判断条件的处理变成一个 handler 。 |
 |
146
cominbril 2021-12-10 17:56:02 +08:00 1
@starsky007 正解，看评论怎么感觉大家都有点不入行啊
|
 |
147
lolizeppelin 2021-12-10 18:11:33 +08:00
逻辑确实有问题
这里逻辑确实可以简化 mb1 != mb2 && mb1!=null return true return false |
 |
148
siweipancc 2021-12-10 22:27:13 +08:00 via iPhone
学完重构与代码简洁之道，写出来的代码同事看不懂。（你这代码这么多个 return ，else if 也不嵌套？）年后准备跑路
|
 |
149
statumer 2021-12-10 22:44:15 +08:00 via iPhone
个人觉得大多数情况，大家眼中只有好深莫测的优美代码和浅显易懂的烂代码。只要能避免 bad case 就不是烂代码。
|
|
150
snw 2021-12-11 00:57:51 +08:00 via Android
|
 |
151
akira 2021-12-11 01:37:09 +08:00
代码不够优雅是水平问题
各种特例有考虑到说明已经用心了 |
 |
152
Lonenso 2021-12-11 12:20:01 +08:00 via iPhone
封装一个 member 类，把 org 和 name 作为属性，然后实现 swap
|
 |
153
whi147 2021-12-11 14:29:37 +08:00 via iPhone
提早 return 就行了
|
 |
154
gyh1996 2021-12-11 18:36:48 +08:00
```java
if (member1 == null ? member2 == null : !member1.equals(member2)) { return false; } return changePartyPositions(member1, member2, name, org, updateTime); ``` |
 |
155
bzl132 2021-12-11 21:26:57 +08:00
写代码的应该是新手，总体感觉有以下两个问题：
1.方法的设计就有问题，为什么要拆成两个方法，明明就是干一个事情的，入参还都一样 2.一般方法应该就两个部分，一是什么情况下什么也不做直接返回，二是具体要干什么，这些都没考虑清楚，直接根据业务条件来写，后面等业务变化后一定是一坨屎 |
 |
156
tohuer00 2021-12-11 22:03:54 +08:00
第一个乍一看不觉得有问题，仔细看发现只是为了 equals 避免空指针写了这么一大堆 if ，改成 Objects.equals 就好。
如果是业务条件需要的类似判断场景，那么像图一这么写两层 if 嵌套其实比楼上一些人的全写到一个 if 括号里更容易阅读。 |
 |
158
wangshanshan 2021-12-17 19:06:08 +08:00
第二个 感觉 最好不要多层 if 嵌套，这个还好层数不多 不是很复杂。 建议可以反着写 if(a==null) if(b==null)
|
Select Language