2016-12-28 744 views
4

我目前正在尝试重构我的代码后,我读到的实现优先于扩展。目前,我正在尝试创建一个将对象添加到场景的功能。为了更好地确定每个对象是什么,有多种解释,如用于更新,渲染列表等Java“instanceof”添加多个列表

private List<Updatable> updatables; 
private List<Removable> removables; 
private List<Renderable> renderables; 
private List<Collidable> collidables; 

我想打一个函数到我的Scene类,像这样:

public void add(Object o) { 
    if(o instanceof Updatable) 
     updatables.add((Updatable) o); 
    if(o instanceof Removable) 
     removables.add((Removable) o); 
    if(o instanceof Renderable) 
     renderables.add((Renderable) o); 
    if(o instanceof Collidable) 
     collidables.add((Collidable) o); 
} 

同样,我会创建一个类似的删除功能。我的问题是,如果这是不好的编码习惯,因为我听说过instanceof的缺陷,其次,如果是这样,是否有办法绕过/重构我的代码以使添加实例变得简单?我更喜欢不必为每个列表添加一个实例,并且每次都定义它属于哪个实例。

+2

很多类似的问题。 [例如](https://www.google.com/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8#q=site:stackoverflow.com+java+instanceof+code+smell) –

回答

1

我会用Map

static Map<String, List> maps = new HashMap<>(); 
static { 
    maps.put(Updatable.class.getName(), new ArrayList<Updatable>()); 
    maps.put(Removable.class.getName(), new ArrayList<Removable>()); 
    maps.put(Renderable.class.getName(), new ArrayList<Renderable>()); 
    maps.put(Collidable.class.getName(), new ArrayList<Collidable>()); 
} 
public static void add(Object obj){ 
    maps.get(obj.getClass().getName()).add(obj); 
} 
+0

是可能使用class'Class'而不是class'String',然后执行:maps.put(Updatable.class,new ArrayList ());?我想我会采用这种方式。 –

+1

@MichaelChoi你不应该接受一个答案,甚至没有尝试过。这会抛出NPE,因为你有这些接口的实现类,所以'maps.get()'将返回null。它也不能解决一个类实现多个接口的问题,根据你对我的答案的评论。 – EJP

+0

@EJP,OP没有提及'Updateable'是类或接口,这个代码将根据用例进行更新,并且在使用'maps.get'时需要在一些负面情况下做更多的事情。这只是一个想法,建议避免'if'' else'来检查''的实例' – Jerry06

2

做工厂模式类似只需使用重载:add(Updateable u), add(Removable r),

+1

这似乎是更自然的Java方法。 – Sudicode

+0

啊,这很有趣。我不知道这会调用到每个接口。这意味着如果我有一个同时实现可更新和可移动的对象,那么这两个函数将被调用?如果这是首先被调用的情况。 –

+0

更新:这种方式不适用于我不幸的。当一个对象同时是Updatable和Removable时,然后尝试添加该对象将导致“模糊的方法调用”。这可以通过投射解决,所以我会做scene.add((Updatable)obj); scene.add((Removable)obj);但这似乎不是一个理想的解决方案。 –

1

当您在评论中提到,你的使用情况有点特殊。 Updatable,Removable,RenderableCollidable都是接口。你有类实现一个或多个这些接口。最终,你想要你的add方法添加一个对象到它实现的接口的每个列表。 (纠正我,如果任何这是错误的。)

如果超载会过于冗长,您可以用反射做到这一点,就像这样:

private List<Updatable> updatables; 
private List<Removable> removables; 
private List<Renderable> renderables; 
private List<Collidable> collidables; 

@SuppressWarnings("unchecked") 
public void add(Object o) { 
    try { 
     for (Class c : o.getClass().getInterfaces()) { 
      // Changes "Updatable" to "updatables", "Removable" to "removables", etc. 
      String listName = c.getSimpleName().toLowerCase() + "s"; 

      // Adds o to the list named by listName. 
      ((List) getClass().getDeclaredField(listName).get(this)).add(o); 
     } 
    } catch (IllegalAccessException e) { 
     // TODO Handle it 
    } catch (NoSuchFieldException e) { 
     // TODO Handle it 
    } 
} 

但是,如果你要使用这种方法,请注意以下注意事项:

  1. 反射本质上很容易出现运行时错误。
  2. 如果层次结构更复杂,则可能会遇到问题。例如,如果您有Foo implements Updatable, RemovableBar extends Foo,则在Bar上调用getClass().getInterfaces()将返回一个空数组。如果这听起来像您的使用案例,您可能需要使用Apache Commons Lang中的ClassUtils.getAllInterfaces
+0

,这是因为酒吧被认为没有实施任何东西,即使Foo是?还是只是“.getInterfaces()”函数仅返回子类的接口? –

+1

这是因为'getInterfaces()'只会选择由implements直接引用的接口。它不会像遍历Apache的'ClassUtils.getAllInterfaces(Class)'那样遍历类层次结构。然而,如果你冗余地声明Bar类为'Bar extends Foo implements Updatable,Removable',那么'getInterfaces()'会选择这些。 – Sudicode

0

我会想到的是,你有一个单独的add方法为每个邮件列表,像这样:

public void addUpdatable(Updatable updatable) { 
    updatables.add(updatable); 
} 

public void addRemovable(Removable removable) { 
    removables.add(removable); 
} 

// and so on 

这样做的原因是,这是单独的列表,并同时从关注实施方面的观点是为了使API更加优雅和光滑,这些努力会从用户的角度来影响清晰度。

例如,添加Updatable(也就是Removable)时发生的误解(在EJP的回答评论中)表明缺乏清晰度。尽管您对类型和状态都使用了类似的术语,这使得它看起来多余,但您仍应该清楚该项目添加到哪个列表(或州的一部分)。

无论如何,它看起来像你在更大范围内的问题挣扎。我认为,投入更多时间来理解更大的问题是个好主意。

+0

你能帮我理解这个问题吗?那么这是目前设计的问题吗?我正在尝试制作一个游戏。对于每个游戏循环,我只希望某些对象执行某些功能,即更新,渲染,删除。这是一个不好的解决方案?如果是的话,你有另一种选择。看起来你似乎认为每个列表都应该是分开的,但是像Player这样的类将如何可更新,可渲染和可移除? –

0

从@ Jerry06,为我添加一个实例到多个不同类型列表的最好方法是使用一个魔术列表getter。

private Map<Class, List> maps = new HashMap<>(); 

'maps'用于查找您要查找的列表。

private <T> List<T> get(Class<T> c) { 
    return maps.get(c); 
} 

而不是使用maps.get,我只是使用get(SomeClass.class)。上面的函数会自动将其转换为具有正确元素类型的列表。然后,我能够做的:的

get(Updatable.class).stream().filter(Updatable::isAwake).forEach(Updatable::update); 

代替:

updatables.stream().filter(Updatable::isAwake).forEach(Updatable::update); 

我主要是想这样的结构,使所有的名单可以很好地包裹起来,其次所以我可以说地图。添加(SomeNewClass.class,新的ArrayList()),然后随时调用它。