Gregg Wonderly wrote:
Peter Firmstone wrote:
Patricia Shanahan wrote:
On 7/21/2010 5:25 PM, Peter Firmstone wrote:
Instead of Task passing an Iterable<Task>, it could pass an
Enumerator<Task>, which is inherently immutable. One more comment
inline.
I assume you mean Enumeration<Task>? I prefer Iterable<Task> because
it allows for the enhanced for loop:
Yep.
public Task runAfter(Iterable<Task> candidates){
for(Task t: candidates){
if(this has to wait for t){
return t;
}
}
return null;
}
Given the number of TaskManager callers, I think there is value in
keeping their code as clean and simple as possible.
Ok, didn't think of that, still using the old for loop.
I think the subtle issue that Peter was getting at though, is that
Iterator has the "remove()" method which perhaps should not be a
published behavior, since it implies an API that is not currently in the
design of the system. I'm not that bothered by this in an internal API,
but in some cases, you have to be careful, and perhaps pass a copy, as in
new ArrayList<Task>( ... ).interator()
just to make sure that your original collection is not compromised by
other software.
Patient: "Doctor, it hurts when I support remove() in an Iterator I
don't want used to modify the underlying collection."
Doctor: "Then don't do that." :-)
The remove implementation in the Iterator in question is:
public void remove() {
throw new UnsupportedOperationException();
}
The java.util.Iterator API documentation for remove() says
"UnsupportedOperationException - if the remove operation is not
supported by this Iterator.". My implementation fully conforms to the
contract for the interface it implements, and does so in the appropriate
way for an iterator() result from an Iterable that is designated as
being read-only.
I would be troubled by the use of ArrayList that you propose.
Collections.unmodifiableList gets the claimed benefit, and is better in
a couple of ways. First, the result throws UnsupportedOperationException
for all the modifying methods, including the iterator() result's
remove() method. That seems better than letting code outside TaskManager
modify a copy, but silently ignoring the changes. Second, creating a
wrapper may take considerably less CPU time, cache misses, memory
bandwidth, etc. than copying the whole list.
I don't necessarily agree with all your opinions, but it is *fun* being
back in an environment in which people look at my code, raise issues,
and care about the fine points of programming.
Patricia
