• 推荐
  • 评论
  • 收藏

精益求精,抑或得过且过[转]

2022-11-20    5745次浏览

程序员面临的最痛苦之事,莫过于修改旧代码;如果还有比这更痛苦的,就是修改糟糕透顶,乱得一团糟的烂代码。最近因为手底下一帮程序员都在忙,市场部正好又反馈过来一个要命的bug,一时手痒,就领下了这个任务。我们这个产品是针对教育行业的,它是在好几年前开发,然后不断完善和维护。这些阶段都是在我来到这家公司之前完成的。所以,我对于产品的代码并不熟悉。

原来的需求是假定客户设置分数段时,不同的分数段有不同的有效分,对应着也就有不同的名次。这些数据都是经过分析器分析获得,并持久化到数据库中。当我们需要生成学生报告时,再从数据库中获取,并将数据填充到iReport设置好的模板中,一个是二维表,一个是柱状和曲线图。

现在,我们发现某些学校需要给不同的分数段设置完全相同的有效分,以及相同的名次。报告打印出来,二维表没有错,曲线图却出现了“缺斤少两”的现象。例如设置五个分数段,却可能只显示了四条曲线。

阅读代码,我明白了原因。在原来的实现中,由于默认不同分数段有不同名次,因此,在获取这些分数段的值时,是将它们放入到一个Map<Subject, Map<Integer, Double>>中。这个Map是根据科目进行分类的,子Map的key值为Integer类型,其实就是分数段对应的名次,value则是设置的有效分。由于现在的名次存在重复,导致Map中的元素存在偏差。这就是五个分数段只显示四条曲线的缘由。

事实上,在我看到这样的Map时,就明白这种“超级强大”的容器,事实上往往会成为坏代码的泥沼。当我们将这样的对象作为参数,在方法之间传来传去的时候,会带来诸多问题: 
1)性能影响。这种可能比较庞大的容器对象,有可能会形成性能瓶颈; 
2)强类型。虽然这里使用了泛型,但泛型类型却使用了基本类型; 
3)封装性不够好。这样的容器对象暴露了太多的数据细节,且不利于为其定义职责行为。 
4)可读性差。看到这样的Map,你并不会在第一时间明白它到底存放了什么。 
5)可扩展性差:当这个Map作为方法的参数时,相当于这个参数没有被对象化。如果拥有这个参数的方法被公开,且广泛调用,一旦需要改变参数,牵连到的代码就会非常多。

事实正是如此。当我在分析我们的遗留代码时,发现很多地方都在重复获取这个Map对象,并且这个Map对象也在多个方法之间传递。因为这种容器对象自身的缺陷,为我的bug修复带来了很多阻碍。要解决这个Bug,就不能再将名次作为Map的key值。查看相关的数据表,事实上我们还给出了一个分数段的名称。当名次和有效分存在重复的情况下,结合分数段名称就能确定唯一值。因此,一个简单地做法就是将Map的key修改为名次加名称的组合,即将Integer修改为String。

现在,我需要决策。如果希望一劳永逸,就应该摒弃这种Map的做法。我们应该利用封装来实现这一目标。例如定义ValidScore和ValidScoreRange。后者相当于之前的Map<Integer, Double>。ValidScore则包含属性:Rank, LineName, Score。事实上,ValidScoreRange正是ValidScore的集合。我们还可以在ValidScore和ValidScoreRange中定义许多与该领域相关的行为。通过这样的封装,问题会变得简单许多。

然而,思考良久,我却放弃了这个符合OO原则的做法。原因在于,遗留代码中使用Map<Subject, Map<Integer, Double>>的类和方法有很多,特别让人烦恼的是在这些方法中,有很多还定义在某些公共类中,被系统的大多数Client所调用。例如这样的代码:

public static JFreeChart createEliteTotalChart(Grade grade, String partial,
ExamSet es, Student stu, Map subToStuTotalMap,
Map<Subject, Map<Integer, Double>> subToValidScoreMap,
List subList,long stuRank,
String[] validLineSeries,double barWidth,Color barColor) {
if (subToStuTotalMap == null || subList == null) {
return null;
}
Color[] color = {Color.RED,Color.BLUE,Color.GREEN,
Color.CYAN,Color.BLACK,Color.MAGENTA};
CategoryDataset[] dataset = EIDatasetFactory.createEliteTotalDataset(
subToStuTotalMap, subToValidScoreMap, subList,validLineSeries);

//......为清晰起见,其余代码略
}

注意,在createEliteTotalChart()方法中,调用了EIDatasetFactory的createEliteTotalDataset(),对Map<Subject, Map<Integer, Double>>对象进行了处理。EIDatasetFactory正是一个公开的公共类,createEliteTotalDataset()方法也是一个被广泛调用的方法。

好吧,我们可以利用重构来完成代码的改造。但是我现在不敢轻易修改这些方法。因为我并不知道这些方法除了生成有效分报告外,还有哪些功能会调用。我们当然可以通过寻找引用来识别到底有哪些功能会调用,事实上,我找到了好几个引用。可是我依旧需要保持足够的谨慎。究其原因,是之前的遗留代码并没有提供充分的单元测试,无法通过单元测试用例来保证这一修改的正确性。

好吧。我得承认重构本来就离不开单元测试,除非是那些直接可以利用工具就能完成的重构。问题不是我不能重构,也并非我的能力达不到;问题也不是我无法保证修改的正确性;真正的问题是我没有时间。作为公司的技术领导,我可以向CEO争取这个时间吗?不能!因为CEO必须考虑成本,他要考虑花这么多时间去做看似不能增加功能的重构是否值得。或许你会说,做这样一个小小的重构需要花太多时间吗?问题是在这个遗留系统中,这样的代码简直数不胜数。所以,在这里时间才是真正的罪恶。不过,最大的罪恶还是人,是编写代码的人。如果在一开始开发系统的时候,我们就能注意代码质量,随时重构,并遵循面向对象设计原则,也许境况会变得焕然一新。这就是所谓的“破窗理论”了。从中可见,代码质量对系统架构的影响。

面对这样一座满是破窗的大楼,我们是下定决心推倒重建,还是修修补补维持现状?这实在是一个哈姆雷特似的问题。站在技术人员的角度,我倾向于推倒重建;然而站在公司领导的角度,却不得不思考成本与时间。

因为这个遗留系统还可以苟延残喘,因为我的时间不允许停留在这个遗留系统太久,我还有新产品的架构工作要做。最终我选择了“得过且过”。我针对这一特定需求,找到了调用的方法。为了不影响其他功能,我利用Copy&Paste的方式复制了该方法,只是为它换了一个名字。当然,我还可以利用Extract Method重构来尽量避免代码的重复。然后,将Map<Subject, Map<Integer, Double>>对象修改为Map<Subject, Map<String, Double>>。因为该方法又连续调用多个其他方法,传递了Map<Subject, Map<Integer, Double>>对象,我又如此炮制地复制了这些被调用方法,最终解决了这个Bug。可是看到这么多Copy&Paste的重复代码,我不寒而栗,决定仓皇逃走。在时间这个强大敌人的面前,我悲哀地选择了投降。

原文地址:https://www.cnblogs.com/Leo_wl/p/2019179.html