avoiding using instanceof operator
I've heard that using the instanceof operator isn't such a great code technique, and since I'm in the design phase, I feel like I should be able to avoid this. Right now, part of my design requires use of the instanceof operator as well as casting.
Let's say you have a common notifier interface and two abstract classes which extend from this interface:
com.example.Notifier Interface:
publicvoid doNotify(Notification notif, List notifiables);
publicboolean canSend(Notification notif, Notifiable person);
com.example.SerialNotifier (abstract)
publicvoid doNotify(Notification notif, List people)
{
for (Iterator itList = people.iterator(); itList.hasNext();)
{
Notifiable person = (Notifiable)itList.next();
doNotify(person, notif);
}
}
publicabstract doNotify(Notification notif, Notifiable person);
com.example.MulticastNotifier (abstract)
publicabstractvoid doNotify(Notification notif, List notifiables);
MulticastNotifier purposefully doesn't define a way to send a message to a single person, so that developers don't implement a non-multicast solution for a multicast protocol.
Here's the part that seems to require an "instanceof" clause. Now let's say you have a list of Notifiers. The list of notifiers is mixed, containing both Serial and Multicast Notifiers. Before you send out the notification, you need to filter the list (because for example, someone doesn't want to be notified at all):
for (Iterator itNotifiers = notifiers.iterator(); itNotifiers.hasNext();)
{
Notifier notifier= (Notifier)itNotifiers.next();
List buildPeopleList =new ArrayList();
for (Iterator itPeople = people.iterator(); itPeople.hasNext())
{
Notifiable person= (Notifiable)itPeople.next();
if (notifier.canSend(person, notification))
{
if (notifierinstanceof MulticastNotifier)
{
buildPeopleList.add(person);
}else
{
SerialNotifier serialNotif = (SerialNotifier)notifier;
serialNotif.doNotify(notification, person);
}
}
}
if (notifierinstanceof MulticastNotifier)
{
notifier.doNotify(notification, buildPeopleList)
}
}
Is this bad design? Another option is to just iterate through the list essentially twice, but this is of course, less efficient for the serial notifier case:
for (Iterator itNotifiers = notifiers.iterator(); itNotifiers.hasNext();)
{
Notifier notifier= (Notifier)itNotifiers.next();
List buildPeopleList =new ArrayList();
for (Iterator itPeople = people.iterator(); itPeople.hasNext())
{
Notifiable person= (Notifiable)itPeople.next();
if (notifier.canSend(person, notification))
{
buildPeopleList.add(person);
}
}
notifier.doNotify(notification, buildPeopleList);
}
Any suggestions?
[4406 byte] By [
nootcha] at [2007-10-2 21:47:39]
Unless I'm missing something, your interface, abstract classes, code snippet, and explanation are somewhat out of wack with each other.
But I'll answer what I think you might be asking.
If you're doing instanceof to avoid calling method A() on some class, becuse that class doesn't define method A(), then it suggests a problem with your hierarchy. You're treating those two classes as the same when you put them into the list, but as not the same when you take them out.
I'd suggest either having two lists, since these classes appear to be two different types, or else, if you want to treat them as the same type, declare the single-recipient method in the interface. In the multicast class, implement that method to do nothing--maybe log a message saying it's doing nothing because it's multicast--and declare it final.
1. I would make the serial notifier more like the multicast notifier. I would use add() method on both notifiers. Then when you call notify, the multicast notifier would notify all at once, while the serial notifier would iterate over a list notifying one at a time.
2. I would make serial notifier more like multicast notifier. Then when you call nofify on serial notifier, it iterates over a list, creating a new notifier per list item and sending the notification that way.
Either way the anser is to make the seril notifier more like the multicast one. I have dealt with this issue myself. Adding methods to an interface that some of its implementors really can't support. I think this is called the composite pattern or someting and its used a lot in graphics. It DOES make things quite a bit easier in the long run. But it is not perfectly the pure OO that one may have in mind. But it achieves the objective of making your life easier, even if using a theoretically frowned upon technique. Besides, its in the GOF book...
jverd and _dnoyeB had good solutions for your specific situation.I have another idea for you, however, that can sometimes be used to avoid instanceof operation in general.
what has been done at the company I work for is to have a single field in each object that identifies itself what class it is. There is an enumeration that defines all the possible class types, and each class returns the appropriate item from the enumeration. Since members of an enumeration are essentially singletons, you can use the == operator to compare them.
Sort of like this:
if (notifier.getNotifierType == Notifier.MulticastNotifier) {
// whatever you need to do here.
}
I don't know if this truly is more efficient than the instanceof check (certainly it carries more overhead, space-wise). The senior architect in my department seems to believe it is better, however.
- Adam
You are making a common mistake. You design is half-OO. Yu need to go the rest of the distance.
interface Notifier
{
public void doNotify(Notification notif, List notifiables);
}
abstract class SerialNotifier
{
public void doNotify(Notification notif, List people)
{
for (Iterator itList = people.iterator(); itList.hasNext();)
{
Notifiable person = (Notifiable) itList.next();
if (canSend(person, notification)) doNotify(person, notif);
}
}
public abstract doNotify(Notification notif, Notifiable person);
public abstract boolean canSend(Notification notif, Notifiable person);
}
abstract class MulticastNotifier
{
public abstract void doNotify(Notification notif, List notifiables);
public boolean canSend(Notification notif, Notifiable person);
}
// ....
for (Iterator itNotifiers = notifiers.iterator(); itNotifiers.hasNext();)
{
Notifier notifier= (Notifier)itNotifiers.next();
notifier.doNotify(notification, people);
}
These are some good responses. Thanks everyone.
Dubwai, your solution is perfectly fine, but it will actually result in multiple iterations through the list of people. For simplification I removed a piece of the design that allows Notifiers to be grouped together by a parent notifier (hence the list of notifiers) and filtered before they are passed down to the children. Therefore, the parent actually requires a loop through each person, before the children do this. This isn't necessary if we know for example that everyone is implementing serial notifications: we can iterate through the people and hand each person down to the child notifier ... but now we're back to the original problem if one of the children notifiers is a MulticastNotifier.
jverd you've essentially nailed the heart of the problem, which is my desire to treat to differing types as one, which is going to be tough. Having 2 lists might be doable without adding all that much complexity.
_dnoyeB proposes an interesting solution, but it has one side-effect to my design which is to require instantiation of notifiers for every notification (since they now hold state). That isn't something I had planned on and I would have to weigh the seriousness of my design flaw against this new added complexity to see if that's really worth it.
Ultimately, this MulticastNotifier is more of an exceptional case than the norm, so I'm also glad that guitar man made his suggestion. It's not the most elegant solution, but an option, and given my requirements, it may well be "good enough".
> what has been done at the company I work for is to
> have a single field in each object that identifies
> itself what class it is. There is an enumeration
> that defines all the possible class types, and each
> class returns the appropriate item from the
> enumeration. Since members of an enumeration are
> essentially singletons, you can use the == operator
> to compare them.
>
> Sort of like this:
>
> if (notifier.getNotifierType ==
> Notifier.MulticastNotifier) {
>// whatever you need to do here.
>
>
How is this different than instanceof? Or, more specifically, than this?if (notifier.getClass() == MulticastNotifier.class())
You're still doing an if check for type. The semantics of instanceof and your check are slightly different, but getClass() is the same.
As for efficiency, your way and getClass should be the same. Instanceof might be slightly slower, but it most likely won't be noticeable.
> These are some good responses. Thanks everyone.
>
> Dubwai, your solution is perfectly fine, but it will
> actually result in multiple iterations through the
> list of people.
I'm not sure what you mean. I moved a loop from your code to a different class. AFAICT, it's a one-for-one. My solution has the same number of iterations as yours.
> I'm not sure what you mean. I moved a loop from your
> code to a different class. AFAICT, it's a
> one-for-one. My solution has the same number of
> iterations as yours.
Yeah, your code is actually missing a loop, because, as I explained earlier, the design supports filtering at both a parent notifier and a child notifier. To filter, it needs to loop through the list of people
So, your code actually will look like: (forgive me, doing code in a textarea is kinda a pain)
public void ParentNotifier implements Notifier
{
// member var for simplicity
protected Filter myFilter = new ExampleFilter();
protected List notifiers = // list of notifiers
public void doNotify(Notification n, List people)
{
List intermediateList= new ArrayList();
for (Iterator itPeople = people.iterator(); itPeople.hasNext();)
{
Notifiable person = (Notifiable); itPeople.next()
if (myFilter.canSend(n, person) // parent's filter
{
intermediateList.add(person);
}
}
for (Iterator itNotifiers = notifiers.iterator(); itNotifiers.hasNext();)
{
Notifier notif = (Notifier)itNotifiers.next();
notif.doNotify(n, intermediateList); // 2nd loop is in here where additional filtering takes place
}
}
If you assume a serial notifier, you don't need this additional iteration:
public void ParentNotifier implements Notifier
{
// member var for simplicity
protected Filter myFilter = new ExampleFilter();
protected List notifiers = // list of notifiers
public void doNotify(Notification n, List people)
{
for (Iterator itPeople = people.iterator(); itPeople.hasNext();)
{
Notifiable person = (Notifiable); itPeople.next()
if (myFilter.canSend(n, person) // parent's filter
{
for (Iterator itNotifiers = notifiers.iterator(); itNotifiers.hasNext();)
{
Notifier notif = (Notifier)itNotifiers.next();
// PROBLEM HERE: assumes serial notifier
notif.doNotify(n, person); // child will do own filtering
}
}
}
}
Edit: Corrected logic error in code
> > I'm not sure what you mean. I moved a loop from
> your
> > code to a different class. AFAICT, it's a
> > one-for-one. My solution has the same number of
> > iterations as yours.
>
> Yeah, your code is actually missing a loop, because,
> as I explained earlier, the design supports filtering
> at both a parent notifier and a child notifier. To
> filter, it needs to loop through the list of people
Before I offer any solutions, why do you feel this is worth worrying about? I'm sure you have heard the quote 'premature optimization is the root of all evil'. Is it really worth crapping up your design to avoid an extra loop?
> Before I offer any solutions, why do you feel this is
> worth worrying about? I'm sure you have heard the
> quote 'premature optimization is the root of all
> evil'. Is it really worth crapping up your design to
> avoid an extra loop?
Root of all evil? I've heard the expression, didn't know it went that far :)
It's a fair point. Just figured if i could avoid it, why not avoid it. But you're essentially right.
> How is this different than instanceof? Or, more
> specifically, than this?if (notifier.getClass()
> == MulticastNotifier.class())
> You're still doing an if check for type. The
> semantics of instanceof and your check are slightly
> different, but getClass() is the same.
>
> As for efficiency, your way and getClass should be
> the same. Instanceof might be slightly slower, but it
> most likely won't be noticeable.
you might very well be right, Jeff. I haven't really looked into it. I'm not sure, though, how efficeint getClass() is. getClass() of object is native, so I'm not sure how it works? Does object cache it's Class field, or does it have to re-evaluate it each time it is called? with my method (well actually, the method used on the application I'm working on), you could cache the NotifierType, making lookup very quick.
I don't know whether it would be noticeably faster than instanceof (however, the senior architect where I work beleives it is), but I doubt it could be any worse, anyway. In my opinion, however, if this is NOT faster than using instanceof, then the call to instanceof shouldn't really be a major performance concern anyway. That's my take on the situation.
- Adam
I would be surprised if there's any significant performance difference in the vast majority of cases. If I decided that I needed to test the type with an if, I'd use instanceof if any subclass was acceptble, and I'd use getClass if I needed the specific class. I really can't see any advantage to adding that field.
> > Before I offer any solutions, why do you feel this
> is
> > worth worrying about? I'm sure you have heard the
> > quote 'premature optimization is the root of all
> > evil'. Is it really worth crapping up your design
> to
> > avoid an extra loop?
>
> Root of all evil? I've heard the expression, didn't
> know it went that far :)
I'm open to the possibility that it's a bit of an exaggeration.
> It's a fair point. Just figured if i could avoid it,
> why not avoid it. But you're essentially right.
Good deal. I will offer this because it's possible it might even be a better solution for you:
Define a class:
class FilteredIterator implements Iterator
{
private final Filter filter;
private final Iterator wrapped;
private Object next;
public FilteredIterator(final Filter filter, final Iterator toWrap)
{
this.filter = filter;
this.wrapped = toWrap;
}
boolean hasNext()
{
if (next != null) return true;
while (wrapped.hasNext()) {
Object temp = wrapped.next();
if (!filter(temp) {
next = temp;
return true;
}
}
return false;
}
Object next() {
if (!hasNext()) // KABLOOEY
temp = next;
next = null;
return temp;
}
public void remove() {
// probably unsupported op exception
}
}
Then at each successive level you can add a wrapper with the next filter. Probably not worth the effort but I believe it solves your problem.
> > Sort of like this:
> >
> > if (notifier.getNotifierType ==
> > Notifier.MulticastNotifier) {
> >// whatever you need to do here.
> >
> >
>
> How is this different than instanceof? Or, more
> specifically, than this?if (notifier.getClass()
> == MulticastNotifier.class())
> You're still doing an if check for type. The
> semantics of instanceof and your check are slightly
> different, but getClass() is the same.
>
> As for efficiency, your way and getClass should be
> the same. Instanceof might be slightly slower, but it
> most likely won't be noticeable.
This is different because it is not tied to a class file. You can have several different implementations that fall under the same type with this style. With instanceof you can not. Well with instanceof you are obligated to implement an interface to become a 'type.' The other way you are not.
In any event, I do it with the type check as opposed to instanceof but I have yet to find a case where I have created a type but not implemented the standard interface...
> This is different because it is not tied to a class
> file. You can have several different implementations
> that fall under the same type with this style. With
> instanceof you can not. Well with instanceof you are
> obligated to implement an interface to become a
> 'type.' The other way you are not.
Are you saying you have class A and class B, where neither is a superclass of the other, both having the same value for that type field?
If so, then, yeah it's different from the class or instanceof test. That sounds like a very weird situation. I can't imagine where I'd want the same behavior in two unrelated classes based on some field value, where those classes don't implement a common interface
> If so, then, yeah it's different from the class or
> instanceof test. That sounds like a very weird
> situation. I can't imagine where I'd want the same
> behavior in two unrelated classes based on some field
> value, where those classes don't implement a common
> interface
The DOM API does this: http://java.sun.com/j2se/1.5.0/docs/api/org/w3c/dom/Node.html#field_detail
Note that I am not making any assesment on whether this is 'good' (no flames please.)
> Are you saying you have class A and class B, where
> neither is a superclass of the other, both having the
> same value for that type field?
>
> If so, then, yeah it's different from the class or
> instanceof test. That sounds like a very weird
> situation. I can't imagine where I'd want the same
> behavior in two unrelated classes based on some field
> value, where those classes don't implement a common
> interface
Yes. I recall one time where I was using this. When doing some work on ORM and having multiple classloaders, instanceof fails. So it was important to create another way to compare object types.
Kind of like SerialVersionID in that it wont change on recompile on diff classloaders as long as you use equals() and it resolves into primitive comparison.
