Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
    
possible duplicate of Is it a bad habit to (over)use reflection? See also: Replacement for instanceof Java? –  gnat Jun 5 '13 at 16:10
    
A relevant link warning against instanceof, including a common exception where instanceof is acceptable. –  Steve314 Jun 5 '13 at 16:10

4 Answers 4

up vote 9 down vote accepted

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.

share|improve this answer
    
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. –  Mike G Jun 5 '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 5 '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 5 '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 5 '13 at 20:58
2  
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 6 '13 at 5:07

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.

share|improve this answer

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.
share|improve this answer

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.

share|improve this answer
1  
How does this avoid the problem of having to check the Timeable instances, which may still not be of type Trigger? –  Frank Jun 6 '13 at 5:02

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.