[GREENFOOT-71] World.getObject(class) spec and implementation can be improved
In World, we have:
public synchronized List getObjects(Class cls)
With comment:
* Get all the objects in the world.<br>
*
* If iterating through these objects, you should synchronize on this world
* to avoid ConcurrentModificationException.
* <p>
*
* The objects are returned in their paint order. The first object in the
* List is the one painted first. The last object is the one painted on top
* of all other objects.
* <p>
The implementation:
public synchronized List getObjects(Class cls) { return Collections.unmodifiableList(collisionChecker.getObjects(cls)); }
Firstly, the requirement to synchronize on the world when iterating through the list seems to be an undesirable requirement (force users to use synchronization). It's unnecessary anyway when a) using the IBSP collision checker, which is now the default and b) when the cls parameter is non-null, regardless of collision checker (because that implies the returned list must be a new list).
Secondly, the statement that objects are returning in their paint order isn't true (for the IBSP collision checker. This was an oversight...).
Thirdly, there's no need to involve the collision checker in the implementation of this method at all. All that's needed is to iterate over the list of objects in the world and filter according to the cls parameter. I propose doing this, and always returning a new list regardless of whether cls is null (thereby removing the requirement for synchronization).
Davin
----
My opinion is that it all sounds good.
For the second issue, I don't think we should promise anything about the order in which objects are returned. Maybe we should add a comment at the top of Actor and World that users should not rely on the order of lists of objects returned from the collision methods, unless the javadoc for the methods explicitly says so?
Poul
----
I agree with some points.
\- requiring synchronisation is unfortunate, and should be removed if we can do it without losing too much else \- maintaining paint order is not terribly important \- that constraint could be removed
The decision to return the original list seems to be a performance improvement detail. Copying the whole list could be expensive. Especially when it is used for checking whether any objects remain. If you want to stop when all objects are gone, you would now write
bq. if (getWorld().getObjects(null).size() == 0) { Greenfoot.stopSimulation(); }
This is quite a common idiom, since we have no numberOfObjects() in the world.
(Admittedly, this is more often used, I think, with a specific class of objects:
bq. if (getWorld().getObjects(!ZargonianButterNuts.class).size() == 0) { Greenfoot.stopSimulation(); }
In that case, we make a copy anyway... and the whole argument does not count.)
Alternatively, we could have returned an interator rather than a list -- then we would never need to make a copy of he list. Just implement an iterator that skips some objects... Hmmm, might have been better.
How about we always copy the list, and add a method to check for object count?
Michael
Issue metadata
- Issue type: Bug
- Priority: Low
- Fix versions: 1.3.0