How Evil is "instanceof"?

My understanding is that "instanceof" should be avoided, in favor of using the NetBeans Lookup. However, what about this situation—we have a node for libraries, with different subnodes, depending on the instance of the object in question, which is either Book or Borrower:

Here's the code I used for the above scenario:

public class LibraryNode extends AbstractNode {

    public LibraryNode(Library library) {
        super(Children.create(new SubChildFactory(library), true));
        setDisplayName(library.getName());
    }

    private static class SubChildFactory extends ChildFactory<Object> {

        private final Library library;

        private SubChildFactory(Library library) {
            this.library = library;
        }

        @Override
        protected boolean createKeys(List list) {
            EList<Book> books = library.getBooks();
            for (Book book : books) {
                list.add(book);
            }
            EList<Borrower> borrowers = library.getBorrowers();
            for (Borrower borrower : borrowers) {
                list.add(borrower);
            }
            return true;
        }

        @Override
        protected Node createNodeForKey(Object key) {
            BeanNode childNode = null;
            try {
                childNode = new BeanNode(key);
                if (key instanceof Book) {
                    Book book = (Book) key;
                    childNode.setDisplayName(book.getTitle());
                    childNode.setIconBaseWithExtension("org/library/viewer/book.png");
                } else if (key instanceof Borrower) {
                    Borrower borrower = (Borrower) key;
                    childNode.setDisplayName(borrower.getLastName());
                    childNode.setIconBaseWithExtension("org/library/viewer/borrower.png");
                }
            } catch (IntrospectionException ex) {
                Exceptions.printStackTrace(ex);
            }
            return childNode;
        }

    }

}

Is the above evil and how should it be rewritten to use Lookup? I guess FilterNode should be used.

Update. See part 2 for a solution to the above problem!

Comments:

You could make a Labeled interface, that both Book and Borrower would need to implement. Together with some other changes, a lookup map for the pics for example with the key being the Class, you could avoid all instanceof/casts.

However, one of the biggest problems with using instanceof is that it does not match on proxy objects and remote objects and the like. For example, when using the popular persistance tool Hibernate, if you fetch a Book object from the database, Hibernate might fetch this object lazily, and make a proxy for it instead of the real object. This object will not match the "instanceof" operator. It will have the same properties and it will implement the same interfaces though.

The solution to this is to always implement to an interface if you are going to use instanceof. Make a BookInterface interface for example(or even better, make Book the interface and BookImpl the implementation class). Make Book implement this interface and change your instanceof code to "instanceof BookInterface". Your code will always work as expected, making instanceof suddenly a lot less evil.

Posted by Integrating Stuff on July 17, 2010 at 09:22 AM PDT #

Actually it's not that it is Evil. But there is a way that this can be avoided. YOu can have subclasses of BeanNode for each type of class your going to display that give back the correct values for 'displayName' and 'iconBase'. That way they all support the BeanNode Interface, but you don't need to do instanceOf anymore.

Posted by Sam Griffith on July 17, 2010 at 11:41 AM PDT #

I wanted to clarify and add to my last answer.

Your use of 'instanceOf' is not Evil per say, but using some subclasses and a Factory pattern, I think you can avoid having to check the classes using instanceOf. You'll end up with a system that can handle new node types without you having to change this code to deal with it anymore either.

The Factory can use reflection to get the correct subclass of BeanNode to create and then your setters and getters are customized to do the right thing for that type of domain object for the BeanNode subclass.

Sorry I was so unclear before...

Posted by Sam Griffith on July 17, 2010 at 12:07 PM PDT #

I'd say it's just practical in this scenario - if you'd have to jump through more hoops or write more code to avoid an instanceof check, it's probably not worth it.

Where instanceof gets evil is when you start having lots of methods that take parameters of type Object - and this is because there is nothing constraining what might be passed in, and anybody who has to maintain the code will have to go search for all the things that actually \*are\* passed to try to figure out what the code should do - and all the ones you can find doesn't mean all the ones there are.

-Tim

Posted by Tim Boudreau on July 17, 2010 at 05:01 PM PDT #

I concur with the other comments.

The 'instanceof' operator is not evil per-se. But any time I see the pattern:

Object obj = ...;
if (obj instanceof ClassA) { ...
} else if (obj instanceof ClassB) { ...
} else if (obj instanceof ClassC) { ...
}

Generally I'd say ClassA, ClassB and ClassC should be refactored to inherit from a common abstract class or interface, and the code rewritten inside the {...} in terms of the methods in that interface. Thus:

CommonInterface obj = ...;
obj.doThing(); // or whatever is going on here.

The beauty of this is that then it's easy to add a new ClassD.

So in your code becomes:

protected Node createNodeForKey(Displayable key) {
BeanNode childNode = null;
try {
childNode = new BeanNode(key);
childNode.setDisplayName(key.getDisplayName());
childNode.setIconBaseWithExtension(key.getIcon());
} catch (IntrospectionException ex) {
Exceptions.printStackTrace(ex);
}
return childNode;
}

With the appropriate changes to the Book and Borrower classes to implement the specified interface.

Posted by William Woody on July 18, 2010 at 02:35 AM PDT #

http://wiki.apidesign.org/wiki/Talk:ExtendingInterfaces

I try to design my polymorphic types as I described in the above apidesign article. However, it requires that the software engineer have total control over the type design. It won't work with "foreign" types that cannot be retrofitted for the double dispatch design pattern.

I definitely try to avoid "instanceof" as much as possible, because it is a red flag for "procedural" versus "object-oriented" code. Sometimes the "cure" is worse than the "disease" of "instanceof".

Posted by Jeffrey Smith on July 18, 2010 at 03:22 AM PDT #

The question is: Is it the right hierarchy?

Add under your library two additional nodes: Books and Borrowers. And any question about wrong use of instanceof is gone ;-)

Sometimes it's impossibe to avoid instanceof. And it's also not a good idea to replace it with a null-test on a lookup-result. But the most instanceof tests in business logics are structural problems. In core system development it is useful to test with instanceof in convenience methods.

br, josh.

Posted by Aljoscha Rittner on July 18, 2010 at 03:49 AM PDT #

Thanks all for the thoughts and contributions! Here is a solution I like, by Aljoscha Rittner:

http://blogs.sun.com/geertjan/date/20100719

Posted by Geertjan Wielenga on July 18, 2010 at 06:41 PM PDT #

You could create a Renderer:

interface Renderer
{
void setDisplayName(String name);
void setIcon(Resource location); }

with a renderer service:

interface RendererService
{
void registerRenderer(Renderer renderer,
Class<T extends AbstractNode> clazz);
void renderer(AbstractNode node);
}

You would register all the renderers in the rendererService. The service would have a set of renderers and would choose the proper renderer using the class of the AbstractNode.

Posted by Pierre Thibault on July 18, 2010 at 10:30 PM PDT #

Hi Pierre Thibault!

Nodes are presentation layers for objects. A node is not a renderer for data. A node gives only a hint for a human readable presentation. To define a better representation for e.g. a Book you can use a FilterNode.

But it's possible to add renderers for properties.

br, josh.

Posted by Aljoscha Rittner on July 18, 2010 at 11:24 PM PDT #

Pierre:

The only problem I see with the RendererService and Renderer class as you suggested would be the implementation of the registerRenderer.

"The service would have a set of renderers and would choose the proper renderer using the class of the AbstractNode."

This sounds like you're simply wrapping the ugly "if (node instanceof ClassA)... if (node instanceof ClassB)" in another class.

My thinking has always been (a) eliminate things that create problems with reuse by properly segregating concerns, and (b) use as few classes as possible while maintaining (a).

And while your Renderer/RendererService is interesting, in this case it seems to violate (a), since the problem with "if (node instanceof ClassA)..." is that it makes reuse difficult, and it violates (b) by rewriting the original function in terms of two separate classes.

Posted by William Woody on July 19, 2010 at 01:16 AM PDT #

I would use a visitor pattern or some similar

Posted by Juggler on July 22, 2010 at 06:56 AM PDT #

Post a Comment:
  • HTML Syntax: NOT allowed
About

Geertjan Wielenga (@geertjanw) is a Principal Product Manager in the Oracle Developer Tools group living & working in Amsterdam. He is a Java technology enthusiast, evangelist, trainer, speaker, and writer. He blogs here daily.

The focus of this blog is mostly on NetBeans (a development tool primarily for Java programmers), with an occasional reference to NetBeans, and sometimes diverging to topics relating to NetBeans. And then there are days when NetBeans is mentioned, just for a change.

Search

Archives
« April 2014
SunMonTueWedThuFriSat
  
12
13
14
19
21
22
23
24
25
26
27
28
29
30
   
       
Today