3
大家帮我看看，这代码是水平。。
source link: https://www.v2ex.com/t/821118
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
113 条回复 • 2021-12-10 08:43:14 +08:00
Kasumi20 17 小时 39 分钟前
服了，不会 AND OR 吗，一行代码搞定的事情，硬是拆成七八行
DreamingCTW 17 小时 17 分钟前
第一张图的方法...我看 if 代码块里面的返回值都是一样的.....那方法体为何不这样写.....
if ((member2 == null && member1 != null) || !member2.equals(member1)) {
return changePartPositions(member1, member2, name, org, updateTime);
}
return false;
if ((member2 == null && member1 != null) || !member2.equals(member1)) {
return changePartPositions(member1, member2, name, org, updateTime);
}
return false;
gps949 16 小时 45 分钟前
还好吧？没看出有啥值得批判的问题。现代编译器 /解释器会对多级判断有自己的优化吧，不过是一个写 web 应用 crud 的，又不是开发编译器或操作系统，只要人读得感觉清晰，有必要非要炫技“人工优化”到一行跟 js 代码混淆一样吗？
arthas2234 16 小时 35 分钟前
if 判断逻辑简单越好，判断逻辑过长可以先把结果赋值给临时变量，变量名语义要清晰
包括里面的逻辑，也是越短越好，实在不能精简，就拆分成小的函数，方便阅读
善用 if-return：if(member != null) {...} => if(member == null) return;
变量名：flag ，a1 ，a2 之类的语义不清晰的就不要用了
包括里面的逻辑，也是越短越好，实在不能精简，就拆分成小的函数，方便阅读
善用 if-return：if(member != null) {...} => if(member == null) return;
变量名：flag ，a1 ，a2 之类的语义不清晰的就不要用了
CharmingCheung 16 小时 7 分钟前 
1
图一实际就是判断两个 String 是否相等然后做不同的逻辑，直接封装个 xxUtils.equals(String a, String b)方法判断两个 String 是否相等就好了。那 changePositions 里就好阅读很多了
CharmingCheung 15 小时 46 分钟前
图二整个过程好像都没有对对象的空值做逻辑分支，那直接一个 try-catch ，try 里 return true ，catch 里 return false 完事
aguesuka 15 小时 14 分钟前
语法层面还好
第一个方法, member 也许是 memberId, 第一个方法里的 name 在第二个方法里成了 positionName, 嵌套的 if 应该该改成 &&, else if 应该合并, 多个分支执行相同的代码也应该合并.
第二个方法, if 的嵌套太多了.
设计层面
dao 层查询结果是 Map
changePartyPositions 应该拆成两个函数, 一个方法不允许 null, 另一个方法只有一个 member, 同样不允许 null.
不要返回 boolean, 而是应该抛异常
另外, updateTime 是 string 类型, 而且是参数, 最坏的可能是从前端拿到的, 而且要保存到数据库, 否则有理由怀疑 changePositions 在循环体中, 同样也很糟糕
虽然不希望和他当同事, 不过这已经算 ok 的代码了, 至少看到代码我知道他想干什么.
第一个方法, member 也许是 memberId, 第一个方法里的 name 在第二个方法里成了 positionName, 嵌套的 if 应该该改成 &&, else if 应该合并, 多个分支执行相同的代码也应该合并.
第二个方法, if 的嵌套太多了.
设计层面
dao 层查询结果是 Map
changePartyPositions 应该拆成两个函数, 一个方法不允许 null, 另一个方法只有一个 member, 同样不允许 null.
不要返回 boolean, 而是应该抛异常
另外, updateTime 是 string 类型, 而且是参数, 最坏的可能是从前端拿到的, 而且要保存到数据库, 否则有理由怀疑 changePositions 在循环体中, 同样也很糟糕
虽然不希望和他当同事, 不过这已经算 ok 的代码了, 至少看到代码我知道他想干什么.
guyeu 14 小时 53 分钟前
这么写，当任何一个参数是空的时候就不发生任何事，默默地执行结束，或者返回一个保底的`null`或者`false`，部分情况可能是符合设计意图的，很多时候其实是坑。。。
weiwenhao 13 小时 57 分钟前
@JeffersonQin
flag = true
if member1 == null {
flag = false
}
if positionsInfo == null {
flag = fase
}
类似这样做个取反逻辑会更加清晰呀
flag = true
if member1 == null {
flag = false
}
if positionsInfo == null {
flag = fase
}
类似这样做个取反逻辑会更加清晰呀
JeffersonQin 13 小时 47 分钟前
@weiwenhao 我感觉图里的逻辑和你给的例子不太一样，而且嵌套 if 还有好处，可以防止深层次的 null pointer exception
比方说这两天我写 dotnet 用 opencv 的 wrapper ，判空会这么写：
if (src == null)
if (src.IsDisposed)
if (src.CvPtr == IntPtr.Zero)
src.Dispose();
诚然你也可以用 goto 的写法，不过嵌套 if 在某些情况下还是更直观的
比方说这两天我写 dotnet 用 opencv 的 wrapper ，判空会这么写：
if (src == null)
if (src.IsDisposed)
if (src.CvPtr == IntPtr.Zero)
src.Dispose();
诚然你也可以用 goto 的写法，不过嵌套 if 在某些情况下还是更直观的
wangyzj 13 小时 18 分钟前
看方法命名就感觉像是一个 void 的方法
member1 应该是 memberId
还有就是不至于写俩方法把
if 嵌套还好
不过我感觉既然 return 了，没必要 else 了
member1 应该是 memberId
还有就是不至于写俩方法把
if 嵌套还好
不过我感觉既然 return 了，没必要 else 了
Akiya 12 小时 34 分钟前
第一个代码应该只需要 2 行：
if ((member2 == null && member1 != null) || (member2 != null && !member2.equals(member1))) {
return ...
}
return false
第二个代码感觉没啥问题，毕竟每一步值都是上面获取的，如果后面没有其他逻辑的话其实可以把 flag=true 改成 return true
if ((member2 == null && member1 != null) || (member2 != null && !member2.equals(member1))) {
return ...
}
return false
第二个代码感觉没啥问题，毕竟每一步值都是上面获取的，如果后面没有其他逻辑的话其实可以把 flag=true 改成 return true
sprite82 9 小时 50 分钟前
说没问题的，平时写的比上面怕是更不堪吧？就这么几个条件合并归纳下有这么难吗，还降低心智负担，你的心智这么难以承受，还当什么程序员？第二个，数据库查询居然拿 JsonObject 作为接收对象
miv 9 小时 31 分钟前 via Android
看着太难受了，这代码。
if 太多，判断太多。
好的代码应该是简洁明了的。
多思考抽象，把代码变简洁。
这样就直观了，出 bug 也会变少。
而不是这样，那么多 if ，一个月后你还看懂嘛
if 太多，判断太多。
好的代码应该是简洁明了的。
多思考抽象，把代码变简洁。
这样就直观了，出 bug 也会变少。
而不是这样，那么多 if ，一个月后你还看懂嘛
liuzhaowei55 9 小时 15 分钟前 via Android
@sprite82 talk is cheap show your code ，自以为是的用个卫语句，坐那里扣半天联合几个逻辑判断，后来的人谁看谁骂娘
liuzhaowei55 9 小时 8 分钟前 via Android
@leokino
业务代码中这种挺常见的，我可能就是大家所不齿的那种敲代码的，用 if 把自己想要处理的场景一层层的判断下来，看起来很烂，但一眼就能看出来想要处理的数据长什么样子。
业务代码中这种挺常见的，我可能就是大家所不齿的那种敲代码的，用 if 把自己想要处理的场景一层层的判断下来，看起来很烂，但一眼就能看出来想要处理的数据长什么样子。
liuzhaowei55 7 小时 46 分钟前
@sprite82 再认真看看 18 楼代码吧，编辑器教你怎么做人。
---
有的代码可能看起来很傻，你想说 nerver do this ，那你就给出一个更好的例子出来，我是属于那种代码能跑就行的那种人，逻辑简单清晰明了，过上一年半载谁来都能看得懂就很好了，不要让人盯着一行代码反应半天。
最后想说，公司的业务代码能做到逻辑严谨就很难得了，受限于自己技术，当时的业务要求，产品的设计能力，行业现状 == 一系列原因，才成就了现在的屌样子，有能力就自己上手改，不能就争取不要做压倒骆驼的那根稻草。
---
有的代码可能看起来很傻，你想说 nerver do this ，那你就给出一个更好的例子出来，我是属于那种代码能跑就行的那种人，逻辑简单清晰明了，过上一年半载谁来都能看得懂就很好了，不要让人盯着一行代码反应半天。
最后想说，公司的业务代码能做到逻辑严谨就很难得了，受限于自己技术，当时的业务要求，产品的设计能力，行业现状 == 一系列原因，才成就了现在的屌样子，有能力就自己上手改，不能就争取不要做压倒骆驼的那根稻草。
gvliwo 7 小时 14 分钟前
我觉得还行,就是丫的没注释,不管你自己觉得如何简单,都要加注释,因为阅读的人可能会误会
代码的话:
如果从精简的角度,确实需要优化
但是团队项目更注重的是易读性,所以并不是越精简越好.
Java 主要的目标是大型分布式,易上手.超高性能不是第一要考虑的,用一部分性能换取开发的方便才是重点,Jvm 也会优化一部分代码,至于有人说数据库用 JsonObject ,那单纯看业务需要,比如说 Redis,
曾经接手了一个烂摊子,因为纯内部环境,不需要考虑安全性,直接 js 执行 sql(甲方太恶心了,一个 sql,就算加了索引,优化了 left join ,创建了中间表等一系列手段后,依然要执行半个小时以上,我真的服了)
所以我个人认为,相比于代码的好坏, SQL 的优化好坏才能体现水平
代码的话:
如果从精简的角度,确实需要优化
但是团队项目更注重的是易读性,所以并不是越精简越好.
Java 主要的目标是大型分布式,易上手.超高性能不是第一要考虑的,用一部分性能换取开发的方便才是重点,Jvm 也会优化一部分代码,至于有人说数据库用 JsonObject ,那单纯看业务需要,比如说 Redis,
曾经接手了一个烂摊子,因为纯内部环境,不需要考虑安全性,直接 js 执行 sql(甲方太恶心了,一个 sql,就算加了索引,优化了 left join ,创建了中间表等一系列手段后,依然要执行半个小时以上,我真的服了)
所以我个人认为,相比于代码的好坏, SQL 的优化好坏才能体现水平
Wh1t3zZ 7 小时 5 分钟前 1
可以看下 StringUtils.isEquals(String st1, String str2) 的实现，判断两个字符串相等并考虑到两个字符串可能为 null ，非常优雅。
return str1 == null? str2 == null : str1.equals(str2);
return str1 == null? str2 == null : str1.equals(str2);
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK