Which of these is "better"?

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();  

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) {

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


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! ;)

Posted by Casper Bang on August 24, 2008 at 06:16 AM PDT #

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?

Posted by Casper Bang on August 24, 2008 at 06:23 AM PDT #

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)) {

Posted by Daniel Sheppard on August 24, 2008 at 06:29 AM PDT #

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) {

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) {

Posted by SideshowAlex on August 24, 2008 at 06:53 AM PDT #

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.

Posted by Kent on August 24, 2008 at 12:32 PM PDT #

The latter one.

In fact, I'd prefer this compact one:
for (MessageInterface mi : Lookups.forPath("MessageProviders").lookupAll(MessageInterface.class))

Posted by Vincent Cantin on August 24, 2008 at 04:13 PM PDT #

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)) {

EventQueue.invokeLater(new Runnable(){
public void run(){

Posted by Emilian Bold on August 24, 2008 at 10:43 PM PDT #

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

Posted by Geertjan on August 24, 2008 at 11:35 PM PDT #

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.

Posted by Jesse Glick on August 25, 2008 at 12:56 AM PDT #

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();

Rest is up to personal likes and dislikes.

Posted by Farrukh Ijaz on August 25, 2008 at 09:19 AM PDT #

Post a Comment:
  • HTML Syntax: NOT allowed

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.


« April 2014