This adds a method public void removeReaction(IReaction relevantReaction); to IReactionSet, ReactionSet and has a test for it. It is for 1.2 branch
Rajarshi, can you please review this patch? (I'm doing so now...)
The testRemoveReaction_IReaction() unit test does not test if the correct reaction has been removed, and I would also suggest to assert there are two reactions before the remove call.
First sentences in the JavaDoc must end with a period. (Please run DocCheck against your code).
Did you check if one IReaction can occur multiple times in a IReactionSet (I see nothing in the docs that it cannot)? If so, please define the behavior of the method in such situations. For example, why should it not remove all occurences of the IReaction, instead of just the first?
Implementation of the method for DebugReactionSet is missing.
In testRemoveReaction_IReaction() you should check that the correct reaction is retained after removal
Also, the NNReactionSet should be updated accordingly
Also based on Egon's comment, the behavior regarding multiple instances of a reaction in a ReactionSet should be specified:
Uploaded a new version of the patch, also including new method in manipulators. Some comments:
- I can't see what I need to do in the NNReactionSet
- I added a method getRelevantReaction(IReactionSet reactionSet, IAtomContainer atomContainer) to ReactionSetManipulator, bug found that there is one with IMolecule instead of IAtomContainer. Is there a reason for this? The method would be more universal with IAtomContainer and could still be used with IMolecule. Or do I miss something?
re NNReactionSet, yes you're right I don't think there is anything to do there.
re IMolecule, good question. My preference is to go with IAtomContainer.
I still don't know the purpose of IMolecule - it extends IAtomContainer and adds nothing to it. So basically it's an empty layer of abstraction. Why do we need it? Why do we have it?
Regarding the new method in rxn set manipulator, why do you look for reactions based on the id field rather than checking equality of references? It seems that that is contrary to other cases of code where we check for presence of objects based on references
Actually, in reference to my last comment, it seems that getRelevantReaction should be renamed to getRelevantReactionByID to be consistent with containsByID in AtomContainerSetManipulator
I second rajarshi's last comment...
Additionally, you could change containsByID() to contains(IAtomContainerSet, String), so that both methods are indeed using the same pattern, or have getRelevantReactionByID take an IReaction as second parameter.
Actually, the last one should likely be called getReactionByID()... as there is no 'relevant' involved... it either has the ID or not... it's not like like the reaction with that ID seems 'relevant'...
Using the IDs has some reason in JCP. I will rename the methods to be more consistent.
Ok, I added a new patch which hopefully keeps the naming more consistent.
In testGetReactionByReactionID_IReactionSet_String can you add an assert testing that you get back null for a non-existant ID. Otherwise looks good
another new version of the patch
I think it's ready to go then
Sign up for the SourceForge newsletter:
You seem to have CSS turned off.
Please don't fill out this field.