which of the two is good practice?

[Pavan Kumar]

This may be a very basic thing, but I want to hear your opinion.

Thanks for your time.

option 1

 private Iterator copyListeners(){
Iterator iter;
synchronized (listeners) {
iter = new ArrayList(listeners).iterator();
}
return iter;
}
 

Option 2

  private Iterator copyListeners() {
  synchronized (listeners) {
   return new ArrayList(listeners).iterator();
}
  }     

I looked at the byte code, and looks like monitor is released in both the cases. Which one do you prefer to write and why?

Thanks,
Cnu

[Ernest Friedman-Hill]

Given that ArrayList itself isn't synchronized, I'm assuming that "listeners" is a synchronized collection like a Vector or Collections.synchronizedList(), or this is really meaningless.

I wouldn't be surprised to find that the bytecode for both is identical. The only difference, then, is clarity. I prefer the first, since it's shorter but no more complex.

[Layne Lund]

Originally posted by Ernest Friedman-Hill:
Given that ArrayList itself isn't synchronized, I'm assuming that "listeners" is a synchronized collection like a Vector or Collections.synchronizedList(), or this is really meaningless.

I wouldn't be surprised to find that the bytecode for both is identical. The only difference, then, is clarity. I prefer the first, since it's shorter but no more complex.

Umm...the second looks shorter to me...or maybe I can't count...

[Guy Allard]

Originally posted by cnu sri:
This may be a very basic thing, but I want to hear your opinion.
..........

So what are the differences in the byte code?

G.

[Ernest Friedman-Hill]

Originally posted by Layne Lund:

Umm...the second looks shorter to me...or maybe I can't count...

Sorry. I should have just said "the shorter one."

[Ernest Friedman-Hill]

I used javap to check it out. Both jikes and javac do generate slightly different code for the two routines. The shorter one does give rise to slightly shorter bytecode. Basically, for the short one, the return value is loaded onto the stack before the lock is released; for the longer one, it's loaded afterwards. The difference is very slight.

[Ilja Preuss]

I prefer Option 2, simply because it's easier to read and understand. Option one takes me a couple of seconds more to parse, and why waste any brain cells on that?

[Marilyn de Queiroz]

I prefer option 1 for the same reason that Ilja prefers option 2.

Also, I think that returning in the middle of a method rather than at the end, particularly if there is more than one return in the method, makes it more difficult to refactor later.

[Ilja Preuss]

Originally posted by Marilyn de Queiroz:
I prefer option 1 for the same reason that Ilja prefers option 2.

That's interesting - could you please elaborate on what you find easier to parse about option 1?

Here is what happens to me:

Option 1: "OK, there is some synchronized block. After that, iter gets returned. What is iter? Ah, iter gets assigned to in the synchronized block. What's that line above. Duh, it's just the declaration of iter. Let's double check that it's really the same variable. Yes, seems to be. So let's summarize: it returns an iterator for the listeners, wrapped by a new ArrayList."

Option 2: "OK, just a synchronized block with a return in it. Seems to return an iterator of listeners, wrapped by a new ArrayList."

The difference really is just a couple of seconds. Still, I find option 1 to be unnecessarily complex. It's also more likely that it contains a bug - just let's also have a field called itera, and return that one instead...

Also, I think that returning in the middle of a method rather than at the end, particularly if there is more than one return in the method, makes it more difficult to refactor later.

That's true, though I don't see how it's true in this case.

On the other hand I find that early returns can make methods easier to *understand*, which is a prerequisite for refactoring.

Often when I have to refactor a big method, one of the very first things I do is introducing Guard Clauses. After that, it's much easier to understand which steps the method can be separated into. Refactoring the method so that, for example, it's easy to extract methods, isn't too hard, even with the guard clauses present.

Your mileage may vary, of course.

[Pavan Kumar]

Thanks very much for your replies.

I'm assuming that "listeners" is a synchronized collection like a Vector or Collections.synchronizedList()

Yes, indeed it's Collections.synchronizedList().

So what are the differences in the byte code?

I should probably have mentioned that I did not understand most of it,besides asking my question more clearly. but my concern was to find out if the monitor was released in both the cases. I was still not very assured and wanted to hear some more about the issue.

Thanks for your valueable comments and time.

it may not matter much, but here's the complete code,

 public class MessageDispatcher {
private List listeners = Collections.synchronizedList(new ArrayList());

private void registerMessageListener(EventListener listener){
synchronized (listeners) {
listeners.add(listener);
}
}
private void removeMessageListener(EventListener listener){
synchronized (listeners) {
listeners.remove(listener);
}
}
private Iterator copyListeners(){
synchronized (listeners) {
return new ArrayList(listeners).iterator();
}
}
public void fireMessageEvent(ExecutionMessageEvent event){
for (Iterator iter = copyListeners(); iter.hasNext() :wink:  {
ExecutionMessageListener listener = (ExecutionMessageListener) iter.next();
listener.executionEventReceived(event);
}
}
}
 

Cnu,