Is it bad programming practice to check if a class referenced by its interface is an instance of another class?

Question

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?

possible duplicate of [Is it a bad habit to (over)use reflection?](http://programmers.stackexchange.com/questions/193526/is-it-a-bad-habit-to-overuse-reflection) See also: [Replacement for instanceof Java?](http://programmers.stackexchange.com/questions/184109/replacement-for-instanceof-java) — gnat, Jun 05 '13 at 16:10
A [relevant link](http://www.javapractices.com/topic/TopicAction.do?Id=31) warning against `instanceof`, including a common exception where `instanceof` is acceptable. — , Jun 05 '13 at 16:10

score 10 · Accepted Answer · answered Jun 05 '13 at 13:48

10

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.

answered Jun 05 '13 at 13:48

dwonisch

364
3
9

That seems a little odd as well, since most (2 out of 3) of the objects that implement the interface don't have a target. – sinθ Jun 05 '13 at 13:50
That is the OO way. Only thing to check is if the hasTarget() method actually makes sense in a Timable. – ftr Jun 05 '13 at 13:59
1

@ftr A better way w/could be to put that extra method in a separate interface `ITartgetable` and ask the original interface reference whether it can return a reference to ITargetable. – Marjan Venema Jun 05 '13 at 20:57
@MikeG Indeed. Asking the interface for a reference to for example `ITargetable` which would have that method (and any others to do with Targetable stuff) would be better. – Marjan Venema Jun 05 '13 at 20:58
3

I diasgree with this answer. It violates the SRP/ISP contract of `Timeable` and does not scale if one has more such cases. – Frank Jun 06 '13 at 05:07

score 9 · Answer 2 · edited Apr 12 '17 at 07:31

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 Timeable interface, as in woni's answer. This is better than using instanceof, but still violates the interface segregation principle.
Keep separate lists of Timeable and Trigger objects in the calling code, or at the very least, only keep a combined list in contexts where the Trigger functionality isn't necessary.
Keep a back reference from your target into all the Timeable objects that use it. In other words, call target.getTriggers() instead of timeableList.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.

kevin cline · Answer 3 · 2013-06-06T15:24:54.533

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.

score -1 · Answer 4 · answered Jun 05 '13 at 15:57

-1

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.

answered Jun 05 '13 at 15:57

TMN

11,313
1
21
31

1

How does this avoid the problem of having to check the `Timeable` instances, which may still not be of type `Trigger`? – Frank Jun 06 '13 at 05:02

Is it bad programming practice to check if a class referenced by its interface is an instance of another class?

4 Answers4