V2EX = way to explore
V2EX 是一个关于分享和探索的地方
现在注册
已注册用户请  登录
Wsdba
V2EX  ›  Java

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

  •  
  •   Wsdba · 2021-12-09 14:54:02 +08:00 · 18508 次点击
    这是一个创建于 840 天前的主题,其中的信息可能已经有所发展或是发生改变。

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

    159 条回复    2022-01-01 21:47:28 +08:00
    1  2  
    fml87
        101
    fml87  
       2021-12-10 04:50:12 +08:00
    楼里居然有相当部分的人认为这种是好代码,这种代码习惯一两周就能堆出大几千行的屎山
    xuanbg
        102
    xuanbg  
       2021-12-10 06:36:14 +08:00   ❤️ 1
    if (member1 == null || member1.equals(member2)){
    return false;
    }

    changePartyPosition(...);
    xuanbg
        103
    xuanbg  
       2021-12-10 06:40:07 +08:00
    @DreamingCTW 你可能看错了,数据验证逻辑应该是:member1 不能为空,且 member1 不能与 member2 相同。
    ryd994
        104
    ryd994  
       2021-12-10 07:45:41 +08:00 via Android
    1 应该使用 &&
    2 可以使用 fail fast pattern ,也可以使用&&

    怎么说呢,可读性不好,但是实际性能没啥区别,毕竟编译器啥都能给你优化掉
    snw
        105
    snw  
       2021-12-10 08:23:20 +08:00 via Android
    @xuanbg
    如果 number1==null 且 number2!=null ,原代码是会执行 ChangePartyPosition 的,和你的代码不同。
    hackk
        106
    hackk  
       2021-12-10 08:28:35 +08:00
    请问只有我看不到 LZ 发的代码吗?
    xuanbg
        107
    xuanbg  
       2021-12-10 08:32:05 +08:00
    @snw 嗯嗯,这样可以简化为:
    return Object.equals(member1, member2) ? false : changePartyPosition(...);
    snw
        108
    snw  
       2021-12-10 08:33:57 +08:00 via Android
    如果业务逻辑预计一年以上不会变,那么可以尝试优化。如果预计一年以内会变化,那么楼主图中这种代码其实挺好的。

    你看上面回帖的代码,至少已经有 2 位写 bug 了🐶
    xuanbg
        109
    xuanbg  
       2021-12-10 08:34:40 +08:00
    不过换我来写代码的话,判断 member1 是否和 member2 相同这个逻辑肯定是写在 changePartyPosition 这个方法里面的。
    justest123
        110
    justest123  
       2021-12-10 08:37:17 +08:00
    这个帖子从昨天看到今天,有个有趣的现象,有贴自己“精简”代码的,好像不是被人说逻辑写错了,就是被从其他方面挑出“问题”
    逻辑易懂跟代码精简貌似不是那么好兼得(狗头(逃
    justfindu
        111
    justfindu  
       2021-12-10 08:40:07 +08:00
    为什么会觉得一定要把代码合并缩进, 缩写 if 才是高端写法, 这样写法后来接手人不是能够一眼就看出逻辑吗. 这样写法整体会浪费资源吗?
    lskjdfgl
        112
    lskjdfgl  
       2021-12-10 08:41:49 +08:00
    @hackk 尝试换个梯子?
    xuanbg
        113
    xuanbg  
       2021-12-10 08:43:14 +08:00
    @justest123 不是,恰恰是因为楼主贴的代码表达的逻辑晦涩难明,才造成大家的理解错误。经过严密推导,逻辑其实简单到令人发指:就是判断 member1 是否和 member2 相同,相同就返回 false ,不同就执行 changePartyPosition 方法。为啥要写这么复杂,我猜是因为需要预防空抛出指针异常而已。
    chenshun00
        114
    chenshun00  
       2021-12-10 09:12:46 +08:00   ❤️ 1
    难道没有人想到把这几个参数聚合成一个对象,然后这些判断放到对象里边去操作么。就算后边要加参数,要改变对象行为不是很 easy
    BlueCropland
        115
    BlueCropland  
       2021-12-10 09:18:47 +08:00
    哈哈,仿佛看了以前的自己, 堆 shi 山
    BlueCropland
        116
    BlueCropland  
       2021-12-10 09:18:53 +08:00
    6666
    pengjl
        117
    pengjl  
       2021-12-10 09:32:48 +08:00
    看到了以前的自己
    steptodream
        118
    steptodream  
       2021-12-10 09:33:46 +08:00
    这个问题是普遍存在的,本质就是很难在别人眼里不是傻 x ,最典型场景开车
    1109599636
        119
    1109599636  
       2021-12-10 09:49:04 +08:00
    接手重构项目的时候碰见这种代码难道不开心吗, 逻辑顺着代码就能看懂, 要是把判断逻辑合并到一行光看懂什么意思就得花好长时间,重构的时候万一没考虑全了还容易出 bug 。。。
    hzw94
        120
    hzw94  
       2021-12-10 10:07:17 +08:00
    不写注释通通捶死!

    拿到无效数据(null)就应该直接 return 结束代码,简介明了。不该 if..else 判空再执行。
    chihiro2014
        121
    chihiro2014  
       2021-12-10 10:13:15 +08:00
    这代码写得好,让人有工作量
    Cloutain
        122
    Cloutain  
       2021-12-10 10:16:22 +08:00
    可读性还行 ,反正编译器会合并条件
    chenyu0532
        123
    chenyu0532  
       2021-12-10 10:16:36 +08:00
    没有注释,没有 log
    除了这两个我不觉得有什么不妥。
    可改进,但是个人感觉意义不大
    接手的人不用费脑子一看就能明白
    hejw19970413
        124
    hejw19970413  
       2021-12-10 10:18:01 +08:00
    第一点 : 两个函数高度相似能不能合并成一个
    第二点 : 可以先行判断的前提条件可以提到前面部分
    第三点 : 可以直接返回的判断是不是可以不用嵌套

    我感觉简单改可以按这个改 。
    sky101001
        125
    sky101001  
       2021-12-10 10:18:43 +08:00
    又不是看不懂
    要求项目代码的每一行都经得起推敲才是强人所难,屎山是在所难免的,这种算不上好代码,但好歹一眼就能看懂,没必要特地拎出来说
    hakr
        126
    hakr  
       2021-12-10 10:20:27 +08:00
    @starsky007 #19 是的, 我也喜欢这样写, 先逆向判断, 先处理逻辑少的条件
    SS0001
        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;
    }
    shawnsh
        128
    shawnsh  
       2021-12-10 10:31:51 +08:00 via Android
    如果没有事情干,可以换种写法
    ganbuliao
        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;
    hakr
        130
    hakr  
       2021-12-10 10:58:36 +08:00
    yl666
        131
    yl666  
       2021-12-10 11:11:54 +08:00
    这种代码风格是把自己需要用到的场景写出来了,剩下的全是一个场景,有些考虑不到的场景就很容易走到 else 里面,造成 bug ,我更喜欢把所有场景列出来的方式来写
    anonydmer
        132
    anonydmer  
       2021-12-10 11:12:19 +08:00
    其实楼主第二章图没贴全,导致大家优化不彻底
    hakr
        133
    hakr  
       2021-12-10 11:17:17 +08:00
    leokino
        134
    leokino  
       2021-12-10 13:03:16 +08:00
    @liuzhaowei55 你没理解我的意思,绝大部分情况下这种条件判断粗略看过就可以了,本身就已经非常清楚了,类似这样的代码过多,说明你要多阅读数倍的代码量,才能理解整体代码的意思,实际上大大降低了可读性。
    Quarter
        135
    Quarter  
       2021-12-10 13:08:29 +08:00 via Android
    我觉得挺好的啊,条理清晰的,方便阅读
    crayygy
        136
    crayygy  
       2021-12-10 13:08:59 +08:00
    append 一个代码版本的? 图片似乎丢失了(还是我这里 load 不出来...
    comoyi
        137
    comoyi  
       2021-12-10 13:13:58 +08:00
    结构清晰,按最细粒度划分分支,对未来的变更留了空间,之后迭代只要加 else 代码不需要改 if 条件代码
    aguesuka
        138
    aguesuka  
       2021-12-10 13:45:29 +08:00
    @xuanbg 可以直接用 Objects.equals, 这个函数可以代替 StringUtils.equals
    tqrj
        139
    tqrj  
       2021-12-10 14:21:34 +08:00
    下面居然有评论觉得这样写挺好的 /。。。。+
    Morii
        140
    Morii  
       2021-12-10 14:50:29 +08:00
    咱先不说条件判断

    这个 member 1 、member2 大家都能忍受吗?
    fkdog
        141
    fkdog  
       2021-12-10 15:01:38 +08:00
    认为这种写法好的显然是不写单元测试的那种。三个嵌套 if 你就能搞出 2^3=8 种分支,你有精力去写这么多的 test-case 吗?

    能写出这样代码的一般都是逻辑很混乱的那种,不会去整理思考分支结构的前因后果,然后 debug 的时候发现空指针或者报错,然后顺势往里插一个 if 来解决问题。。
    ytmsdy
        142
    ytmsdy  
       2021-12-10 15:05:16 +08:00
    降低预期!降低预期!降低预期!
    现在 360 行,行行转码农,进来的人水平参差不齐!
    只要是能跑,没 bug ,看得懂的代码,都行。别给我出 bug ,别让我半夜起来修生产环境都行!
    否正这种代码看多了会脑溢血,你不接受能怎么办捏?帮他改?教他写?那花的不还是你自己的时间么。
    有这时间,多刷几部剧不好么。
    chenshun00
        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;
    }
    }
    ```
    tenserG
        144
    tenserG  
       2021-12-10 16:49:32 +08:00
    @darkcode 图床问题,国内能访问到,梯子不能
    litchinn
        145
    litchinn  
       2021-12-10 16:58:54 +08:00
    先说我的观点:尽管代码有优化空间,比如判断条件可以合并,但是其实并不严重,至少这么多楼下来大家看懂这个代码的占多数不是吗?
    其次就是楼上说时间久了变成屎山,其实任何代码时间久了不重构都会变成屎山,拿这个举例,如果参数 member 再多几个,判断条件的组合就会爆炸(排列组合 Cmn ),这种时候使用只要还是这样 if else 写再怎么合并条件、提前返回还是看不懂的,这种情况我能想到的解决办法就是使用 **规则引擎** 这种模式去做,将每个判断条件的处理变成一个 handler 。
    cominbril
        146
    cominbril  
       2021-12-10 17:56:02 +08:00   ❤️ 1
    @starsky007 正解,看评论怎么感觉大家都有点不入行啊
    lolizeppelin
        147
    lolizeppelin  
       2021-12-10 18:11:33 +08:00
    逻辑确实有问题
    这里逻辑确实可以简化
    mb1 != mb2 && mb1!=null return true
    return false
    siweipancc
        148
    siweipancc  
       2021-12-10 22:27:13 +08:00 via iPhone
    学完重构与代码简洁之道,写出来的代码同事看不懂。(你这代码这么多个 return ,else if 也不嵌套?)年后准备跑路
    statumer
        149
    statumer  
       2021-12-10 22:44:15 +08:00 via iPhone
    个人觉得大多数情况,大家眼中只有好深莫测的优美代码和浅显易懂的烂代码。只要能避免 bad case 就不是烂代码。
    snw
        150
    snw  
       2021-12-11 00:57:51 +08:00 via Android
    @lolizeppelin
    写 bug 了吧🐶

    String mb1 = new String("A");
    String mb2 = new String("A");
    按你的代码会 return true
    akira
        151
    akira  
       2021-12-11 01:37:09 +08:00
    代码不够优雅是水平问题
    各种特例有考虑到说明已经用心了
    Lonenso
        152
    Lonenso  
       2021-12-11 12:20:01 +08:00 via iPhone
    封装一个 member 类,把 org 和 name 作为属性,然后实现 swap
    whi147
        153
    whi147  
       2021-12-11 14:29:37 +08:00 via iPhone
    提早 return 就行了
    gyh1996
        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);
    ```
    bzl132
        155
    bzl132  
       2021-12-11 21:26:57 +08:00
    写代码的应该是新手,总体感觉有以下两个问题:
    1.方法的设计就有问题,为什么要拆成两个方法,明明就是干一个事情的,入参还都一样
    2.一般方法应该就两个部分,一是什么情况下什么也不做直接返回,二是具体要干什么,这些都没考虑清楚,直接根据业务条件来写,后面等业务变化后一定是一坨屎
    tohuer00
        156
    tohuer00  
       2021-12-11 22:03:54 +08:00
    第一个乍一看不觉得有问题,仔细看发现只是为了 equals 避免空指针写了这么一大堆 if ,改成 Objects.equals 就好。

    如果是业务条件需要的类似判断场景,那么像图一这么写两层 if 嵌套其实比楼上一些人的全写到一个 if 括号里更容易阅读。
    bzl132
        157
    bzl132  
       2021-12-11 22:56:30 +08:00
    @tohuer00 合并条件是没问题的,写法上 在 || 换行就能方便阅读了
    wangshanshan
        158
    wangshanshan  
       2021-12-17 19:06:08 +08:00
    第二个 感觉 最好不要多层 if 嵌套,这个还好层数不多 不是很复杂。 建议可以反着写 if(a==null) if(b==null)
    season8
        159
    season8  
       2022-01-01 21:47:28 +08:00
    @hackk 同看不到
    1  2  
    关于   ·   帮助文档   ·   博客   ·   API   ·   FAQ   ·   我们的愿景   ·   实用小工具   ·   3323 人在线   最高记录 6543   ·     Select Language
    创意工作者们的社区
    World is powered by solitude
    VERSION: 3.9.8.5 · 31ms · UTC 11:26 · PVG 19:26 · LAX 04:26 · JFK 07:26
    Developed with CodeLauncher
    ♥ Do have faith in what you're doing.