Sunday August 24, 2008
Sunday August 24, 2008
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();
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?
Aug 24 2008, 12:55:09 PM PDT Permalink
10 Comments
Trackback URL: http://blogs.sun.com/geertjan/entry/which_of_these_is_better
Comments:
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 01:16 PM 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 01:23 PM 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)) {
jTextArea1.append(it.getMessage());
}
Posted by Daniel Sheppard on August 24, 2008 at 01:29 PM 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) {
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());
}
Posted by SideshowAlex on August 24, 2008 at 01:53 PM 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.
The latter one.
In fact, I'd prefer this compact one:
for (MessageInterface mi : Lookups.forPath("MessageProviders").lookupAll(MessageInterface.class))
jTextArea1.append(mi.getMessage());
Posted by Vincent Cantin on August 24, 2008 at 11: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)) {
sb.append(it.getMessage());
}
EventQueue.invokeLater(new Runnable(){
public void run(){
jTextArea1.append(sb.toString());
}
});
Posted by Emilian Bold on August 25, 2008 at 05:43 AM 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 25, 2008 at 06:35 AM 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 07: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();
jTextArea1.append(messageInterface.getMessage());
}
Rest is up to personal likes and dislikes.
Posted by Farrukh Ijaz on August 25, 2008 at 04:19 PM PDT #
Post a Comment: