有两个现有的实现,UFOEnemyShip & RocketEnemyShip,
为了介绍实现EnemyShip接口的每个新实现,如下所示,

工厂类EnemyShipFactory中需要进行更改,并添加一个额外的else if,如下所示,
public class EnemyShipFactory {
public EnemyShip makeEnemyShip(String shipType) {
EnemyShip theEnemy = null;
if(shipType.equals("U")) {
theEnemy = new UFOEnemyShip();
}else if (shipType.equals("R")) {
theEnemy = new RocketEnemyShip();
}else if (shipType.equals("B")) {
theEnemy = new BigUFOEnemyShip();
}
return theEnemy;
}
}客户代码:
public static void main(String[] args) {
EnemyShipFactory shipFactory = new EnemyShipFactory();
EnemyShip theEnemy = shipFactory.makeEnemyShip("R");
doSomething(theEnemy);
}EnemyShipFactory中的这一代码更改是否被认为是一种代码气味?如果是,我们能避免这种代码气味吗?
发布于 2017-09-24 16:16:54
人们通常关注实现中的代码气味。然而,我认为我们还需要查看抽象中的气味--通常通过说明消费/使用而不是只关注实现来更好地看到这些气味。
您还没有在上下文中显示使用情况。
@Ewan表示,这种代码是在持久性中看到的,特别是在反序列化中。在这种情况下,抽象的重点应该是将序列化和反序列化结合在一起,以便只有一段代码(正如@Ewan建议的那样,您的数据存储库)必须知道字符串和子类之间的映射。这样我们就不会在某个地方有多个分开的代码块,例如,每个代码都使用自己的字符串"U“副本。
但是,您还可能正在做其他事情,例如调用... makeShip("U");。如果是这样的话,并且您试图避免直接调用new UFO(),那么您可能会创建一个单独的工厂方法,专门用于创建UFO类型,这样您就不必在一个地方通过"U“,然后在另一个地方测试"U”。
发布于 2017-09-24 06:58:53
你可以做很多事情来让这件事变得更好:
switch语句而不是链式else-if子句shipType的字典来生成函数。shipType从字符串更改为枚举。但它们都是微小的调整,你说得对,这感觉就像一种代码气味。如果不了解更广泛的背景,就不可能给出更多的建议,但我怀疑有两件事:
发布于 2017-09-24 11:32:45
是的,对我来说,这是一种暗号味。一个大的if块映射魔术字符串到硬编码的类。
每当您看到这样的东西时,通常是因为您正在从某种形式的持久化数据生成对象,数据库或某个文件在某个地方具有托运类型:“U”、速度: 12、盔甲:6,并且您希望创建相应类的实例。
但是,这个映射应该封装在您的数据存储库中。它将数据的编码部分转换为特定的格式。你不应该看到这些工厂类在你的应用程序周围浮动。
此外,在存储库类中,您应该通过命名表或键入与类名相同的字符串来避免使用神奇的字符串。
您还可以避免使用已知类名的Map的if语句来使用数据填充正确类的新实例的函数。(基本上和我认识的工厂一样,但更整洁)
https://softwareengineering.stackexchange.com/questions/357932
复制相似问题