一个程序员怎么能做出这样的事情?

我正好看到了下面的一段代码:

        public void Execute()

        {

            ArrayList empIds = PayrollDatabase.GetAllEmployeeIds();

            foreach (int empId in empIds)

            {

                Employee employee = PayrollDatabase.GetEmployee(empId);

                if (employee.IsPayDate(payDate))

                {

                    DateTime startDate = employee.GetPayPeriodStartDate(payDate);

                    Paycheck pc = new Paycheck(startDate, payDate);

                    paychecks[empId] = pc;

                    employee.Payday(pc);

                }

            }

        }

这段代码有点老,是用.NET 2.0之前版本写的。可是,并不是里面ArrayList的用法让我苦恼。首先他从数据库里取出所有员工的Id。然后他遍历这个Id集合,从数据库这取出每个员工的信息。每当我看到这样的代码,我都想踹写这个程序的人一脚。

如果你还不明白这样的写法有什么问题,请这样想想:你第一次把select语句发送给数据库查询员工Id,查询出5条员工记录。然后你需要向数据库请求另外5条查询语句,分别查出这几个员工的信息。这还好,6次查询并不是一个多大的事情,不是吗?可是你为什么不能把所有需要的数据一次性的全部查询出来呢(这样只有一次开销大的查询)!想象一下,如果你要计算的是100个员工的工资呢,而不是5个?如果是1000个员工的呢?

让我不可理解的是,这样的代码天天都会产生。难道这些人真的不在乎、或真的不知道这样的代码有多糟吗?如果他们真的不知道,那真是很悲哀。如果他们不在意,那更糟糕,因为如果一个程序员明知这样写有问题还是要这样写,很显然,他不认为他的工作有价值,他不关心他的程序,他的团队,他的公司,以及他的客户。

如果你奇怪我是从哪里找到这段代码的…是在Robert C(敏捷软件开发理论的创始人)那里。他的《敏捷软件开发:原则、模式与实践(C#版)》这边书里。是的,是 Robert C。Martin,也就是Uncle Bob。我也许不该批评面向对象领域里如此著名的人物,可是,说真的,Bob,你脑袋进水了吗?你的整本书的目的都是在教育人如何写出优质的代码,里面可以找到大量很有价值的教导。但把这样的代码当作例子实在是不可宽恕。

[英文原文:Why On Earth Would A Developer Do This? ]
分享这篇文章:

72 Responses to 一个程序员怎么能做出这样的事情?

  1. says:

    在我看来这并不是不可饶恕,很多人都会这样使用,因为如果数据样本极大,一次返回所有符合条件的员工信息可能比返回所有员工ID后分别查询占用的内存更多(因为 支付期往往相同,这意味着将返回所有员工的信息),即使他更快也不可取。
    倒是,在foreach 循环里反复新建Employee 、DateTime 、Paycheck 我不理解,这样子似乎只是在浪费?

    foreach (int empId in empIds)
    {
    Employee employee = PayrollDatabase.GetEmployee(empId);
    if (employee.IsPayDate(payDate))
    {
    DateTime startDate = employee.GetPayPeriodStartDate(payDate);
    Paycheck pc = new Paycheck(startDate, payDate);
    paychecks[empId] = pc;
    employee.Payday(pc);
    }
    }

    • says:

      同意,一次返回所有符合条件的员工信息在某几个客户那里已经爆掉了。。

      • littledoo says:

        错!就算是占用内存过多也不应用如此的方式来优化。
        你可以选择分页来TOP 50,100。
        因为客户也不可能一次查看如此多条数据。

        我不知道你们说可以原谅的人自己测试过没有,1K次数据库访问要多少CPU时间。这不是10M,100M,甚至500M内存可以换取的容忍度。

  2. liruqi says:

    paychecks 变量哪儿来的? 如果是类成员变量不需要任何修饰么?

    Besides, most db drivers support cursor, why not use it?

  3. 六子liu says:

    我倒是没那么大反应,也许基于其他因素的考虑,也可能是本文的作者断章取义。也许是Uncle Bob故意设的这样一个局,然后去解开它。哈哈。晚上回家查查这段,确认下。

  4. Timothy says:

    我也很想踹他一脚

  5. rexfire says:

    奇怪吗,CMMI5都这样干,等用户骂了再改,不都是这样吗?我是中国程序员~

  6. flytwokites says:

    看了一下,是楼主鸡动了,这段代码一点问题也没有,是我也可能会写成这样。

    • Sweet says:

      明明一个SQL语句就可以搞定了,干嘛非的这样做,我承认这样代码的可读性和可维护性更高,但是往往要在这两者间取平衡啊。ORM把程序变傻了是不。

      • 六子liu says:

        额,ORM只是为了提高开发效率,一定程度上解决面向对象和关系型数据库间的鸿沟,是否能把程序员变成傻子要靠程序员自己。
        另外,楼上,文中的写法在有些情况下可能是个解决办法,但是绝对不是推荐的方式。

      • flytwokites says:

        你太嫩了,写个几年代码再回来看你说的话吧。

      • flytwokites says:

        1. 你再怎么拼SQL写不出比这更精炼重用度更好更面向对象的代码。
        2. 我不知道这段代码用在什么地方,看样子是发员工工资的,而世界上有几个公司能让这段代码产生性能问题?
        3. 等你到Uncle Bob的境界上,你会理解代码可维护性比无关的性能更重要。

      • 六子liu says:

        额,是挺嫩的,那你为什么不说出来缘由?那我信服。

      • Sweet says:

        @flytwokites
        兄弟,你再多写几年代码,就不会发出这样的言论了,面向对象、复用固然是好,但是过度设计,过度的使用,那就是得不偿失了,“世界上有几个公司能让这段代码产生性能问题”,就表明了代码存在隐患,而且你也说到,不知道用在什么地方,那我可以理解为需求不明确,在需求不明确的情况下,你是否需要考虑解决这个问题?假如我做的是一个数据管理公司,为N家大型企业提供员工管理服务,那么这个时候,性能的需求就出现了。而且本身通过很简单的方式可以解决,但是为了所谓的“面向对象”,却选择了牺牲性能,牺牲资源。再者,我看到有程序员写了一个按照时间段删除数据的函数,也是这样先读取,再根据ID一条一条删除,但是仅仅一个Delete的语句就可以简单而快速的做到。同时通过一定的设计和扩展,同样可以做到很好的扩展性和可维护性,这就是看你自己有没有这能力了。保持简单合适的设计才是最好的。

      • yy says:

        所以出现了hibernate

    • 六子liu says:

      这里面有个误会,我可是从来没赞成过拼sql的,我觉得文中纠结的地方在于为何将所有id取出后,再用id去对象。
      当然你说的可维护性、可读性灰常重要,我也认同。

      • flytwokites says:

        尽量重用已有的东西。
        因为用id取对象这个逻辑是已经写好的通用方法,可以用在很多地方,比如有个表单让你输个id然后你就可以用到这个方法了。

    • bhhzd says:

      你太嫩了,回去做几个大项目再来说这话

  7. 徐风子 says:

    一楼在说啥呢????

    这种代码是挺恶心的,不过工作中经常遇到,估计是没有写PayrollDatabase.GetAllEmployees() 方法, 发现有PayrollDatabase.GetAllEmployeeIds 就临时拿来凑和了吧。

  8. Jerry Chou says:

    你怎么知道存放EmpId的数据库和存放EmpInfo的数据库就是同一个数据库?这压根和开发技术无关,而与业务应用有关。

    开发时最怕的就是猜,比猜更可怕的是瞎猜。

  9. 郗晓勇 says:

    最近正在开发一个系统,有同样的问题~~

  10. CA says:

    1、C#版的代码是鲍勃大叔的儿子在原先Java的代码上给改写的,并不是鲍勃大叔自己手写的C#代码;
    2、如果写代码完全跟随SQL,那是“事务脚本”模式;如果按照“领域对象”等模式,必然会牺牲一些SQL方面的逻辑。

  11. 泥菩萨 says:

    个人认为原文作者断章取义的可能性大点。程序是全局把握的事情。

  12. haitao says:

    所以
    1、程序的执行效率是可以差几倍到几万倍的
    2、程序员的心情很重要,懒或消极就是这样的结果了

  13. Levi says:

    你博客用什么插件来给每个comment的用户弄个头像?

  14. wheat says:

    这个问题光看一小段代码不看上下文肯定得不出什么结论。要从全局看问题,才能确定怎么做是最合适的。

    另外这篇文章是3年前的,作者最新一篇文章写的关于clean code的文章也值得一看
    http://davybrion.com/blog/2011/07/clean-code-versus-great-code/

  15. georgexsh says:

    op 没有提到一件事情是,这几个getter方法背后的cache情况
    如果cache的粒度是cache一份所有id的列表,单独cache每个employee对象数据,那这样的写法就是正确的了

  16. lxu4net says:

    如果有一个PayrollDatabase里有一个缓存层的话,拿着代码也没啥大问题。
    还是一个“早期优化”的问题。可以先这样写,更可读。如果有性能问题,再改进好了。

  17. kevin.c says:

    从片面来看确实有问题,不过在一个有时间要求的大型项目里这种事经常有。
    你可能是负责实现这个功能模块的程序员,但是PayrollDatabase是由另一个人维护的(数据库接口通常是由一位资深的技术专家负责),然而背催的是PayrollDatabase里没有你需要的接口,而在时间压力下你不可能等技术专家来完成这么个接口,于是上面的代码便出现了。

  18. kevin.c says:

    不要在这方面太执着了,我想这段代码所在的文章内容不是在讨论性能问题,而是在讨论其他的吧。
    为什么不用心去理解作者想表述的思想,而要在这些东西上“刨根儿问底”?

  19. Jimmy Liu says:

    能跑不就行了吗?如果对性能有要求再根据要求去修改不就好了吗,那么较真儿干嘛⋯⋯程序员都是给自己找麻烦的人⋯⋯打压别人顺道抬高自己

  20. 忧郁 says:

    叫我写我也不会写成这个样子,我一定会先把所有的employee 对象取出来。因为每次都去访问数据库是在是太浪费时间了。

  21. 如果有cache操作呢?我们只是把ID取出,不需要在去查库就好,这个是后期可以优化的地方,如果你一条就搞定,后期怎么个优化法?

    • baibaichen says:

      en, 是这意思,如果有缓存,并不是每次调用PayrollDatabase.GetAllEmployeeIds() 和 PayrollDatabase.GetEmployee(empId)都会触发数据库操作。

    • CzBiX says:

      为什么你认为写代码时就完全不管效率,
      为了后期能有优化而前期代码就凑和写?
      优化是随时随地的进行的吧

      • yy says:

        同样一段代码在不同环境下是会有完全不同的评价的
        如同上面代码 有些情况下 会觉得很渣 有些情况下 会觉得很OK

        就拿上面的例子来说 如果数据量是一千万条 一次全部查出来的话 直接虚拟机就都挂掉的

        但是如果连接数据库有很好的机制且有代价很低的话 一千万次无非是速度慢一些
        但每次查询完了只是增加很小的内存来存储需要的个别数据 那情况就会好很多

      • yy says:

        抱歉 我常用java 随口说了虚拟机 不过意思都是一样的

      • yy says:

        当然 针对千万级的数据 有更好的方案 我这里只是举个例子

  22. 1 says:

    我只能猜测employee.IsPayDate(payDate)里面有很多玄机,以至于作者考量后认为全部都取出来的成本要低于拼SQL以及维护的方式.

    另外,ArrayList empIds = PayrollDatabase.GetAllEmployeeIds();
    如果返回的不是ArrayList,而是某种与数据库指针以及缓存关联的List,类似于Iterable那样的,也许也凑活着解释的通/.

    不过,我觉得就算是employee.IsPayDate(payDate)实现复杂,但是也可以考虑挑出一个简单的逻辑在SQL层面进行一下初步的筛选…一下取出所有数据,听听就挺可怕的.

  23. bigdog says:

    在我看来并非完全不可取,得根据情况而定,对于一些高负载的网站,为了提高缓存命中率,那只能这样取,如果是根据条件一条语句全部查出来就没法缓存了。这样可能会牺牲第一次查询的性能,但后期带来的效果是显而易见的,我所知道的大网站中经常有这样做的。

  24. zephyro says:

    直接返回所有的EmployeeInfo?如果数据库大会直接卡死吧。
    起码在做网游的时候,像这种全局更新的操作,慢点没关系,卡死就问题大了。
    不过至于公司员工发工资嘛,恐怕倒真没这么夸张。
    关于循环,更多时候看到的是hashmap使用entrySet()遍历而不是keyset之类的讨论。

  25. ZX says:

    没有搞清这段代码的运行环境,就没法得出有说服力的结论。这段代码如果要处理1000个符合条件员工的信息,确实可能要访问1001次数据库(先不考虑缓存),但是我们不知道一个员工信息的数据量有多大,如果一个员工信息的数据量很大,而这个程序恰巧又要在内存很小的客户端pc机上(甚至某种嵌入式的专用机器上)运行呢?那只好用时间换空间,用数据库资源换客户端资源

  26. giles says:

    应该把if (employee.IsPayDate(payDate))转成SQL的where IsPayDate==now 一次性查出结果集。一般来说,这种给员工发工资的程序,数据量不会很大的。如果数据量太大,可以用分页的方法来多次查。

  27. WAM says:

    写程序,不单只已完成一个功能为目的,还要让程序读出来的数据不产生冗余,最主要的是可控性和可扩展性,万一又加了一个数据库,只用到里面的ID,这样其余的数据的浪费了,造成不必要的负担。”船到桥头自然直”

  28. leizisdu says:

    谢谢分享:D

  29. camelwoo says:

    不管大家如何争论,目前为止,我还没有遇到过需要这样写的场景。估计是我做过的项目太少了吧。基于我目前的认知水平,我以后还是不会这么写代码的。出现这样的代码,比较合理的解释就像前面朋友们说的:利用已有代码,并且因为数据量小,不会产生性能问题。如果再多说点,很可能是赶工期造成的。

  30. 花の葬 says:

    具体场景具体分析。
    .net2.0之前的话就是.net1.1及.net1.0,没有泛型。基本用ArrayList居多。
    另外一些实体类内的方法都可能是带有很多的业务逻辑的。
    如果真有性能问题。可以具体问题具体优化。

  31. 小文 says:

    有点断章取义的感觉,同时,这个不思考也是很多人容易犯错。

  32. 低级程序员 says:

    首先说,我没认真读过这本书,只知道是重点讲解敏捷开发模式,兼具讲讲设计模式的书。而不是本重点讲解应用实例的书,如果非当应用书看,那就太纠结。我假设一种情况,请作者及看客评判,之所以你们认为这个代码愚蠢都是建立在可以通过SQL语句解决的问题的前提下,如果存储这些数据的只是个文本文件呢?你还觉得这段代码愚蠢吗?

  33. 魏静娴 says:

    楼主有没有想过,如果每个员工有一张1MB的照片保存在数据库里,并且公司有10000名员工,恐怕被踹的人就是你了。

  34. 吕小强 says:

    看了上面的文章和一下拉的评论,感觉都有理,其实我觉得还是是具体情况而定,不一样样应用环境采用不一样的方法,这样比较合情合理……

  35. 寻觅 says:

    当进行单表查询时,文中的代码自然不是最合适的。但如果是联表查询,尤其是极其复杂的报表查询时(以前做过的后台系统里面有碰到过),文中的代码所写的方法就是最适合的了。
    并不是所有的代码都能使用拼sql的方式的,而书中只是一个示例代码而已,没有涉及真实生产环境,仅此而已

  36. says:

    代码优化指南:
    第一条:不要优化
    第二条:针对老鸟,还是不要优化.
    第三条:参考第一条.
    过测试用例吧,测试用例和压力测试没有问题,就不用管.毕竟这个代码不一定是Hotpot, 如果不是 Hotpot, 那么为了自己所谓的好看而修改代码是不明智的.
    而且这个还指不定什么平台呢,也许是个移动设备来发工资什么的也都说不定,所以,没有测试没有发言权.但在这里看这段代码,没有任何问题

  37. lixin says:

    要看场景。如果在这后面有个以ID为键值的二级缓存,那么就一点问题都没有。

  38. 昊云 says:

    看情况而定吧。有时在数据嵌套绑定会这样写

  39. ksj_23 says:

    可能OO大师忙于研究OO而无暇学习他们认为很低级的SQL语言吧。

  40. lixin says:

    hibernate开启二级缓存的时候,都这样做,有啥好JJYY的

  41. qmigh says:

    这种写法也许错,也许没错,要看具体场合。
    1、如果PayrollDatabase.GetEmployee()是直接从数据库读取,那么这种写法是不可取的;
    2、如果是从内存读取,或者PayrollDatabase.GetEmployee里面有做内存缓存处理,那么就没问题。

  42. GGG says:

    这段代码从哪可以看出是每次都从数据库查询的?那个操纵数据库的对象,可以是任何实现方法,可以一次从数据库里面取出来,也可以每次都连接数据库,甚至可能它连接的只是一个基于http协议的远程的接口,这个对象只是个代理,如果和远程的数据库服务器连接的不是那么可靠呢,这样一次取1000条还合适吗?。。。反过来想,如果这些情况都是未知的,你怎么写这个接口,也许一次取一条合适,也许一次取1000条合适,那么是不是像书里这种方式写这样的接口才是最合适的,这个是基本的面向对象的设计方法吧

  43. 子非鱼 says:

    从代码来看,id和员工信息应该分布在两张表里,如果用的mysql4.xx版本的话,是不支持子查询的,也只能先查id,再根据id查了。

  44. Bon says:

    很明显,文章提倡的做法是只一次性的开销都保存在array中,这样之后即可通过ID索引值,效率是O(1)。 上面那些讨论是否太过偏移主题?

  45. cy 对这篇文章的反应是垃圾
  46. cy says:

    楼主,好好学学吧,太菜了,每个对象成员不是直接从数据库查出来的,还是需要计算的,楼主脑子被驴踢了

  47. fuyg says:

    这是教学用的例子,何必当真!

  48. Lee 对这篇文章的反应是标题党

发表评论

邮箱地址不会被公开。 必填项已用*标注

此站点使用Akismet来减少垃圾评论。了解我们如何处理您的评论数据