Re: [Cdk-devel] atom removal error

[Kai H. <Kai.Hartmann@Uni-Koeln.De>]

Hi Todd, hi all.

Mar...@ep... schrieb:
> Kai Hartmann <Kai.Hartmann@Uni-Koeln.De> wrote on 10/02/2006 11:28:18
> AM:
> 
>> Is this the current subversion version? If so, please check whether
> this
>> method also fails:
>>
>> org.openscience.cdk.tools.manipulator.AtomContainerManipulator.
>> removeHydrogens(IAtomContainer)
>>
>> If yes, please send a bug report with one sample molecule attached and
>> write a test that fails in AtomContainerManipulatorTest.
>>
> 
> Yes I am using the current SVN version.
> I have added testAddRemoveHydrogens() method to the
> AtomContainerManipulatorTest.
> In the new test it still says the hydrogen counts are non zero after
> removing the hydrogens.
> 
> There is something weird going on in the AtomContainerManipulator.
> removeHydrogens method.
> Why does it have this code:
> neighb.setHydrogenCount(neighb.getHydrogenCount() + 1);
> Shouldnt all the hydrogen counts be zero?

The hydrogen count field is the implicit representation of hydrogen
atoms. This part of the code seems to increment the hydrogen count while
removing the explicit hydrogen atoms.

If all explicit hydrogens are removed and only the hydrogen count is
!=0, this is intended behaviour. Maybe the method name could be better,
but the javadoc tries to explain it.

> One more thing - why was the return class for getConnectedAtoms method
> for the AtomContainer class made into a list- it seems like it makes it
> a lot harder to use- for example to look at the symbol for a connected
> atom one could previously just say ca[0].getSymbol()  - now you have to
> write ((IAtom)ca.get(0)).getSymbol().
> It seems to me that if you only have a few elements it's easier to have
> an array than use a list and an iterator.
> Was there some reason that a list is better?

The previous code was creating a List internally, too. It was just
converted to an array. Three reasons for the transition:
- The conversion creates overhead. It can still be done by the user if
necessary (method in corresponding manipulator class)
- We wanted to provide fewer methods to access the elements. The return
types were made consistent (still some work left).
- We wanted to make the API as stable as possible approaching the 1.0
release, so the transition to List has been done before the transition
to java 1.5. With java 1.5, the code in your example will reduce to
ca.get(0).getSymbol().

Kai