烂代码是怎样炼成的

这两天接手一个离职员工的项目，改bug，个中不爽，难以启齿，有些代码写得实在是让人哭笑不得，不一一列举，摘一段集烂代码之大成的典型案例：

public int getMaxFreq() {
    int totalFreq = getTotalCountFreq();
    for (int i = 0; i < ListProvider.getInstance().getG122PlusFreqList().size(); i++) {
        int totalFreqs = ListProvider.getInstance().getG122PlusFreqList().get(i) * totalFreq;
        boolean beforeCondition;
        if (totalFreq == 1) {
            beforeCondition = totalFreqs > (commandService.getModel() == StimulatorConst.MODEL_G122 ? 2000 : 10000);
        } else {
            beforeCondition = totalFreqs > 2000;
        }
        boolean midCondition;
        if (totalFreqs > 2000) {
            midCondition = ((1000000.0 / (pwList.get(getMaxProgramPW()) * 2 + 40)) < totalFreqs) || isShell() || (model && getMaxProgramAmp() > 300) || (!model && getMaxProgramAmp() > 150);
        } else {
            //midCondition = (1000000.0 / (pwList.get(getMaxProgramPW()) * 10)) <= totalFreqs;
            midCondition = ((1000000.0 / (pwList.get(getMaxProgramPW()) * 2 + 40)) < totalFreqs);
        }
        boolean isAccord = !G122ParamParser.isAccord(mAmplitudeAfter, mPulseWidthAfter, totalFreqs, model) || !G122ParamParser.isAccordOtherProgram(mProgramGroupInfo, programId, totalFreqs, model);
        if (beforeCondition || midCondition || isAccord) {
            if (i - 1 < 0) {
                maxFreq = 0;
            } else {
                maxFreq = i - 1;
            }
            return freList.get(maxFreq);
        }
    }
    maxFreq = freList.size() - 1;
    return freList.get(freList.size() - 1);
}

这段代码的作用是取最大的频率值，方法名还是比较准确的，但代码中有好几处令人哭笑不得，欲言又止。

首先第1行调用方法getTotalCountFreq()取频率的设置数量（频点个数），方法的命名虽然是中式英语，比较拗口但可以理解，不奇怪，怪就怪在变量totalFreq，既然是取频点个数，为什么赋值给了一个表示频率总和的变量呢？正常情况下其实也能很快反应过来，问题是当前页面还真有个获取频率总和的方法叫getTotalFreq()，而且还写在了一起：
1
2
3
4
5
6
private int getTotalCountFreq() {
// ...
}
private int getTotalFreq() {
// ...
}
第2行ListProvider.getInstance().getG122PlusFreqList().size()什么鬼？非要写这么长的代码么？非要在第3行完整复制一遍ListProvider.getInstance().getG122PlusFreqList()么？虽说单例执行多少次性能不会有什么影响，但你能不能稍微用点心，为他人着想一下，就好比看小说，写这么啰嗦谁愿意看呢。最让人不爽的是，这里的ListProvider.getInstance().getG122PlusFreqList()早就是在初始化方法里赋值给了全局变量freList，就是代码尾部那几行的freList，这就匪夷所思了，能不能长点儿心，为啥前后不一致呀，写代码写了一半习惯就变了么？
第4行totalFreqs，我就问你和第1行的totalFreq摆在一起你晕不晕，而且这里的totalFreqs和getTotalFreq()取值还不一样。
第6-10行if-else，没什么问题，就是看着有点长，我个人比较反对用条件表达式，不容易理解，可阅读性比较差，这里的代码假如换成下面的写法可读性会好很多：
1
2
3
4
5
if (totalFreq == 1 && pulser.getModel() == StimulatorConst.MODEL_G122R) {
beforeCondition = totalFreqs > 10000;
} else {
beforeCondition = totalFreqs > 2000;
}
第12-17行也是if-else，这段问题实在太多，需求复杂可以理解，代码一定要写成这样吗？首先印象是真长，一眼望不到头。映入眼帘的(1000000.0 / (pwList.get(getMaxProgramPW()) * 2 + 40))是数学公式，这种公式一定要提取公用方法，绝对要避免在代码中重复出现，因为一但写错了太难发现了，多写一次就增加一次出错的风险，哪怕代码是复制的也要小心，万一复制的时候前面或者后面少复制一位呢？
这里的getMaxProgramPW()又是什么鬼，外面还套着个for循环呢，要循环几百次，getMaxProgramPW()是取幅度最大值，其结果不会随着for循环而改变，完全可以在for循环体外面申明局部变量计算一次取值。
后面的isShell()值得表扬，把判断是否带壳的一大坨代码封装起来了，和周围的代码格格不入，简洁明了，简直是一股清流。
后面两个getMaxProgramAmp()与getMaxProgramPW()问题一样，真是何必呢。
第15行是注掉的代码，说啥好，这段整代码要有一行真正的注释该多好。我遇到过很多这样喜欢改bug时注掉老代码的程序员，我一直在揣测他们的内心，很难以理解。这一行显然是错误代码，为什么不删除呢？留着除了还害人还有什么作用？有些程序员改别人的代码的时候尤其会干这种事，不敢删除别人的代码，怕出事；觉得别人写的代码是别人的心血，直接删除太无情。我在以前公司做分享的时候经常讲写代码要有责任心，要有主动性，要有自信心，大家相信你，让你改Bug，你有什么好担心的呢？有git在，改坏了也能找回来，怕啥！

第18行是一段让人看了深感惋惜的代码，比较长，仔细一看还挺清晰，就调了两个封装的方法isAccord应该就是“取决于”的意思，取决于什么呢？点进去看看：

public static boolean isAccord(int amp, int pw, int totalFreqs, boolean model) {
    // ...
    return result;
}

/**
 * 其它program是否满足条件约束一
 * @return
 */
public static boolean isAccordOtherProgram(ProgramGroupInfo programGroupInfo, int programId, int totalFreqs, boolean model) {
    for (int i = 0; i < 4; i++) {
        if (i == programId) {
            continue;
        }
        PxParam pxParam = programGroupInfo.pxParams[i];
        if (!programGroupInfo.programGroupFlag.isBitValue1(8 + i) || !programGroupInfo.programUseFlag.isBitValue1(i)) {
            continue;
        }
        if (model == programGroupInfo.programGroupFlag.isBitValue1(5)) {
            if (!isAccord(pxParam.amplitude.intValue(), pxParam.pulseWidth.intValue(), totalFreqs, model)) {
                return false;
            }
        } else {
            if (!isAccord(0, pxParam.pulseWidth.intValue(), totalFreqs, model)) {
                return false;
            }
        }
    }
    return true;
}

这两个方法都挺复杂，当然也有很多问题，不细说。就说最痛心疾首的问题，第前一个方法是判断当前的program是否符合约束，而第二个方法遍历了4个program，然后把当前的programId给continue过滤掉了，专门判断其他的3个是否符合约束，真是绝望啊，既然都写了遍历，干嘛不直接判断所有的program呢？第二个方法里判断约束的时候也是复用的第一个isAccord(..)方法，说明作者知道当前program和其他的3个program逻辑是一样的，写这种代码是为的哪般呀？
也许有人会说，这里不能合并，因为isAccordOtherProgram()方法判断其他program时有if-else处理，这就又是另一个大坑了，这里if的条件model == programGroupInfo.programGroupFlag.isBitValue1(5)其实是恒成立的，因为传入参数model的的取值就是programGroupInfo.programGroupFlag.isBitValue1(5)，也就是说这里的else永远不会被执行。实际上从业务意义上也能看出来的，因为对于这4个program其model状态都是一致的，需求就是如此。说明作者在写这段代码时，思维比较混乱，抛开技术不说，首先业务需求肯定没有理解清楚。

后面19-26行终于把三个条件判断好了，大功告成，不容易啊，这里代码比较简单，没什么好说的。惟一的建议是，maxFreq是全局变量，非常不建议在for循环里对全局变量赋值，会有各种安全问题，如果这里非要赋全局变量，可以先在方法里申明一个局部变量暂存for循环产生的值，循环结束后把这个局部变量赋给全局变量。
最后的29行，明明28行已经赋值了，为啥不直接写return freList.get(maxFreq);呢，唉，当然上面那么复杂的代码都看下来了，这点代码也就无关痛痒了。

我早就想写这篇文章，一直没好意思下手，人家都离职了，都是朋友，还说啥呢，又不可能回来改bug。写这篇文章的目的不是吐槽（虽然最大的作用是吐槽），有则改之，无则加勉。有本书叫《代码整洁之道》，书中对代码整洁的苛求，我的代码是远远达不到的，上面的代码还有很多优化改进的空间。不要刻意强求，做为程序员心里要有分寸。写代码不难，编译器很强大，写出能让编译器看得懂的程序太容易了，写出让人能看得懂的程序才是真的难。
就上面的代码而言，写成这样其实我能猜到当时的情景，上面代码里的beforeCondition、midCondition、isAccord三个判断不是一下子就有的，是随着项目推进一步步提出来的，于是代码也一步步堆起来了，堆成了一坨。尤其是后在面的isAccord和isAccordOtherProgram，我估计当时就是因为没有考虑清楚究竟要怎么判断，一开始以为只判断当前program就可以了，写完了过几天，领导又过来说：唉呀，单判断当前的还不够啊，还要判断其他的3个，于是作者就不假思索地堆了点代码上来，弄了个isAccordOtherProgram。不管怎么样，代码确实写得不好，这是很中肯的，需求做得不好，设计做得不，这可以说是别人的责任，但这不是写烂代码的理由，自己写的代码自己者不去阅读，别人更不愿意去读了，光这样的代码，一模一样的在项目里至少复制了4处，哪有什么牵一发动全身，哪有那么难改的代码，不还是坏习惯导致的么。