[Cdk-devel] Atom/Bond Iterators

[John M. <joh...@gm...>]

Hi All,

I'm doing some clean up/efficiency optimising on some core objects. One part of this is using java collections in the atom container to store the bonds/atoms/electron containers and makes this so much simpler as the container doesn't have to manage the array grow size etc.. One issue this has thrown up as that there are several inner classes for each iterator (one for each stored array) and these inner classes probably shouldn't be the "responsibility" of the atom container (also why it is beneficial to switch to the 1.5 Collections).

The current implementation allows you to remove components whilst iterating (https://gist.github.com/3741888) and actually calls the invokes the removeAtom/removeBond method on the container.

> IAtomContainer container = ...
> 
> Iterator<IAtom> it = container.atoms();
> while(it.hasNext()){
>    IAtom atom = it.next();
>    if("H".equals(atom.getSymbol())){
>        it.remove(); // invokes container.removeBond(IBond);
>    }
> }

Although I can see the benefit I would argue this is unexpected as people may use remove without realising it actually modifies the underlying container. I've check the main library and there are no instances of removing with an iterator on atoms/bonds/electron containers and would like to change the behaviour of this iterator.

Okay so what can we do? Well normally when you encapsulate a collection and have custom add/remove methods (as we do - needing to managed object listeners) you should return an unmodifiable collection (http://www.javapractices.com/topic/TopicAction.do?Id=173). This allows iteration but not addition/removal and effectively forces explicit use of the actual method it's invoking "removeAtom" or "removeBond". If you want to have a list you can modify the unmodifiable collections also make you create a new collection to do this. The alternative is to actually create a new collection for each iterator and thus not allow modification to the underlying container.

tl;dr
Atom container iterators have a remove() method which removes atoms/bonds from the container. This may be confusing. It could be beneficial to inhibit removal except except from the AtomContainer. Thoughts?

J