Hi Rajarshi,

I agree - I am actually fine with it not extending ChemObject (and
hence loosing notifications). It's basically a container anyway

On thinking about this, the refactoring you note would be useful in
providing a more meaningful API. But unfrotunately, it does'nt let us
reduce the API (which is what I'm trying to work out)

It would work by doing an inversion, having the class extending a List/Set
and then having a ChemObject as field. You could simply delegate all
calls to the internal ChemObject field.

Another thing I noticed was that the real functionality of
IAtomContainerSet is essentially only used by the IReaction derived
classes (for the multiplier field). Other classes that use this
actually use just as a List<IAtomContainer> - I'm for converting those
usages to List<>, but it's a lot of manual work and if the interface
became a Set, we'd to manually assess whether classes using this
assumed it was a List.

For these cases it may be even better to use Collection<IAtomContainer>. Then 
they would accept anything List, Set, Queue. It would be a lot of work but I'm happy
to help out with this. 

If this is mainly used by Reaction, then I have an idea for some
improvements. In our group we mainly deal with biochemical/metabolic reactions
which also have a cell compartment for each metabolite (required for transport). Due to this
requirement we have our own Reaction which uses CDK AtomContainer's. 

The model we have is that a Reaction contains Lists of Participant objects for
the left/right sides. Each participant defines a coefficient, molecule and 
compartment (if needed). This class actually still implements the IReaction 
interface so it would integrate quite nicely. I'll have a play on branch next
week so demonstrate some advantages. 

What it would mean that instead of doing this. 
 addProduct(IMolecule product, Double coefficient) 
you call
 addProduct(new Participant(product, coefficient));

the existing implementation would simply wrap this allowing
existing classes that use Reaction to function as expected.


On 15 Dec 2011, at 23:54, Rajarshi Guha wrote:

Hi John, thanks for your comments. Some more below

On Thu, Dec 15, 2011 at 5:59 PM, John May <john.wilkinsonmay@gmail.com> wrote:
First post here, but here's my two pennies....

I think that current implementation of IAtomContainerSet as a Set rather then a
List is more sensible. In my mine the 'set' implies no duplicates, if this were a 'list'
you would need to allow duplicates (and nulls). This might be intuitive, for example,
in reactions where participants have integer coefficients but imagine if you had a
coefficient of 0.5? You'd have to add half an object :-D.

That is what the multiplier field is meant to support - but I agree,
together with the multiplier, this class really is more of a Set than
a List

Anyways, regarding refactoring, it would be nice have it extending List or Set rather
then use this as an attribute. You could simply add the counters for molecules
on addition and removal of AtomConatainer's and implement the listeners.

I agree - I am actually fine with it not extending ChemObject (and
hence loosing notifications). It's basically a container anyway

On thinking about this, the refactoring you note would be useful in
providing a more meaningful API. But unfrotunately, it does'nt let us
reduce the API (which is what I'm trying to work out)

Another thing I noticed was that the real functionality of
IAtomContainerSet is essentially only used by the IReaction derived
classes (for the multiplier field). Other classes that use this
actually use just as a List<IAtomContainer> - I'm for converting those
usages to List<>, but it's a lot of manual work and if the interface
became a Set, we'd to manually assess whether classes using this
assumed it was a List.
--
Rajarshi Guha
NIH Chemical Genomics Center