X

Geertjan's Blog

  • August 24, 2008

Which of these is "better"?

Geertjan Wielenga
Product Manager
Today I was studying this piece of code:

Lookup lkp = Lookups.forPath("MessageProviders");  
Collection coll = (Collection) lkp.lookupAll(MessageInterface.class);
for (Iterator it = coll.iterator(); it.hasNext();) {
MessageInterface messageInterface = (MessageInterface) it.next();
jTextArea1.append(messageInterface.getMessage());
}

I figured out that I was able to rewrite it to this:

Lookup lkp = Lookups.forPath("MessageProviders");
Collection<MessageInterface> coll = (Collection<MessageInterface>) lkp.lookupAll(MessageInterface.class);
for (MessageInterface it : coll) {
jTextArea1.append(it.getMessage());
}

Which of the two, in your opinion (this is inevitably going to be a subjective question of taste) is "better"? And why?

Join the discussion

Comments ( 10 )
  • Casper Bang Sunday, August 24, 2008

    The latter, it takes less space and reads easier. Furthermore, Joshua Bloch has this to say:

    - It is less likely you write an error

    - Easier to compose and nest iterators

    - Allow the JVM to perform boundary optimization

    You make great examples of the NetBeans API, but changing from the old iterator syntax to the new is usually the first thing I do after I grabbed them! ;)


  • Casper Bang Sunday, August 24, 2008

    Oh just noticed, your first version isn't even parameterized. So there are 2 issues here, parameterized vs. manual casting, foreach vs. manual iteration.

    Is there a reason for not doing not using the generic Lookup of NetBeans 6?


  • Daniel Sheppard Sunday, August 24, 2008

    lookupAll is a generic method and the type is inferred from the argument. As a result the cast is not required and it may well be giving you a warning. Moreover, unless I need the collection elsewhere I'd probably just put the lookup in the for loop (which hides the fact that Collection is returned).

    Lookup lkp = Lookups.forPath("MessageProviders");

    for (MessageInterface it : lkp.lookupAll(MessageInterface.class)) {

    jTextArea1.append(it.getMessage());

    }


  • SideshowAlex Sunday, August 24, 2008

    Another approach would be to use the Collections.checkedCollection method:

    Lookup lkp = Lookups.forPath("MessageProviders");

    Collection<MessageInterface> coll =

    Collections.checkedCollection(lkp.lookupAll(MessageInterface.class), MessageInterface.class);

    for (MessageInterface it : coll) {

    jTextArea1.append(it.getMessage());

    }

    This little change allows you to add a method that will allow you hide the ugliness of having to specifying the MessageInterface.class:

    private static <T> Collection<T> lookup(Lookup lkp, Class<T> cls)

    {

    return Collections.checkedCollection(lkp.lookup(cls), cls);

    }

    Lookup lkp = Lookups.forPath("MessageProviders");

    for (MessageInterface it : lookup(lkp, MessageInterface.class) {

    jTextArea1.append(it.getMessage());

    }


  • Kent Sunday, August 24, 2008

    The latter one.

    Your intention here is to iterate over all the items in the collection, yet the introduction of an iterator is probably what I might consider noise in the code.


  • Vincent Cantin Sunday, August 24, 2008

    The latter one.

    In fact, I'd prefer this compact one:

    for (MessageInterface mi : Lookups.forPath("MessageProviders").lookupAll(MessageInterface.class))

    jTextArea1.append(mi.getMessage());


  • Emilian Bold Monday, August 25, 2008

    If you are asking just about the enhanced for, then the second version is ok, though I would make it more compact like Vincent.

    What I don't like about this code is that is appending text to a (probably) Swing component. This means that your code probably runs in the event queue and all the calls to getMessage() happen there. A possible bug might happen here if getMessage takes a lot of time (for example a MessageInterface to a socket or some slow remote server) -- you would block the whole interface repaint. I'd rather build the string in another thread (via RequestProcessor) and then set the text area at the end:

    final StringBuilder sb=new StringBuilder();

    for (MessageInterface it : Lookups.forPath("MessageProviders").lookupAll(MessageInterface.class)) {

    sb.append(it.getMessage());

    }

    EventQueue.invokeLater(new Runnable(){

    public void run(){

    jTextArea1.append(sb.toString());

    }

    });


  • Geertjan Monday, August 25, 2008

    Thanks all for the helpful comments -- my samples from now onwards will be better as a result. :-) The article that started me thinking about this code snippet is now available at NetBeans Zone, by Toni Epple: http://netbeans.dzone.com/news/netbeans-extension-points


  • Jesse Glick Monday, August 25, 2008

    You do not need casts, nor Collections.checkedCollection. The return type is Collection<? extends MessageInterface>, not Collection<MessageInterface>, but as noted previously you do not need to pay attention to this if you just use the result directly in a for-loop, as Daniel and Vincent showed.


  • Farrukh Ijaz Monday, August 25, 2008

    If not pointer is required, the second approach is always better because it's simple and short. If I were you, for the first approach I would use the following because of readability:

    Lookup lkp = Lookups.forPath("MessageProviders");

    Collection coll = (Collection) lkp.lookupAll(MessageInterface.class);

    Iterator<MessageInterface> it = coll.iterator();

    while(it.hasNext()) {

    MessageInterface messageInterface = it.next();

    jTextArea1.append(messageInterface.getMessage());

    }

    Rest is up to personal likes and dislikes.


Please enter your name.Please provide a valid email address.Please enter a comment.CAPTCHA challenge response provided was incorrect. Please try again.