I have a class (Timer) with an array list of Timable objects. Timeable is an interface. There is some specific functionality that I need for the Trigger class (implements Timable), which has a reference called target. A lot of methods need to search through the Timer array for Trigger objects with a certain target.
What I did was a function like this:
public Timable[] findObjectsWithTarget(Cue target) {
ArrayList<Timable> result = new ArrayList<Timable>();
for (Timable timed : fireable) { //fireable is the array (actually a HashSet) of Timed objects
if (timed instanceof Trigger && ((Trigger) timed).getTarget() == target)
result.add(timed);
}
return (Timable[]) result.toArray();
}
This feels like a bad practice. The Timer class is now dependent on the Trigger class (sort of) and are no longer generalized.
So, my two questions:
1) Is it actually a bad practice? Why or why not?
2) What’s a better way if it is a bad practice?
2
Yes this is indeed a bad practice. You are using an interface to abstract your code from the implementation. When you work with the interface and access the implementation (via casting) you are violating the interface definition.
To solve this kind of problem extend your interface by another method, that you would access from within your code.
public Timable[] findObjectsWithTarget(Cue target) {
ArrayList<Timable> result = new ArrayList<Timable>();
for (Timable timed : fireable) { //fireable is the array (actually a HashSet) of Timed objects
if (timed.hasTarget(target))
result.add(timed);
}
return (Timable[]) result.toArray();
}
You have to implement that method also in Timerclass but you can return false per default.
5
Yes, it’s a bad practice, because you’re adding functionality to the Timeable interface that you’re not specifically declaring, namely the possibility of also being a Trigger. There are a few different ways to do it better:
- Explicitly add the functionality to the
Timeableinterface, as in woni’s answer. This is better than usinginstanceof, but still violates the interface segregation principle. - Keep separate lists of
TimeableandTriggerobjects in the calling code, or at the very least, only keep a combined list in contexts where theTriggerfunctionality isn’t necessary. - Keep a back reference from your
targetinto all theTimeableobjects that use it. In other words, calltarget.getTriggers()instead oftimeableList.findObjectsWithTarget(target). Often when a method feels awkward, it’s because you’ve put it into the wrong class. The fact that “a lot of methods” need to do this search means this approach would also have significant time efficiency benefits.
1) Is it actually a bad practice?
Yes
Why or why not?
Because of the backward coupling between Timer and Trigger. By design, Timer does not know anything beyond Timeable.
2) What’s a better way if it is a bad practice?
Your requirement isn’t exactly clear. Is the problem to find all triggers for a given target attached to a specific timer, or to find all triggers for a target attached to any timer? Either way, I have a couple of ideas:
-
Move “findObjectsWithTarget” out of Timer and into Trigger. That breaks the coupling between Timer and Trigger, but may require adding a method to Timer returning all the attached “Timeable” instances.
-
Record the target -> trigger associations in the Target class.
I’d implement something like Target.searchTriggersForTarget(List<Timeable>, Cue) : List<Timeable>. Target is already dependent on Timeable, so no new coupling there, and you now have your Target-specific logic in the right place. Just pass in your list of Timeables from Timer and return the result. This will also facilitate using the same logic in other classes that hold lists of Timeables, which sounds useful since you say it’s used in many contexts.
1