#34 Patch to add a removeReaction to reactionSet

Needs_Review
closed
None
5
2012-10-28
2009-03-09
Stefan Kuhn
No

This adds a method public void removeReaction(IReaction relevantReaction); to IReactionSet, ReactionSet and has a test for it. It is for 1.2 branch

Discussion

  • Stefan Kuhn

    Stefan Kuhn - 2009-03-09
     
    Attachments
  • Egon Willighagen

    Rajarshi, can you please review this patch? (I'm doing so now...)

     
  • Egon Willighagen

    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.

     
  • Rajarshi Guha

    Rajarshi Guha - 2009-03-10

    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:

    • Are multiple instance of the same Reaction allowed?
    • If so, removeReaction removes the first instance. Or should it remove all?
     
  • Stefan Kuhn

    Stefan Kuhn - 2009-03-10

    new version

     
    Attachments
  • Stefan Kuhn

    Stefan Kuhn - 2009-03-10

    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?

     
  • Rajarshi Guha

    Rajarshi Guha - 2009-03-10

    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?

     
  • Rajarshi Guha

    Rajarshi Guha - 2009-03-13

    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

     
  • Rajarshi Guha

    Rajarshi Guha - 2009-03-13

    Actually, in reference to my last comment, it seems that getRelevantReaction should be renamed to getRelevantReactionByID to be consistent with containsByID in AtomContainerSetManipulator

     
  • Egon Willighagen

    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'...

      • Removes all instance of a reaction from this reactionSet.
    • instanceS

    • either use {@link IReactionSet} or normal english like "reaction set".
     
  • Stefan Kuhn

    Stefan Kuhn - 2009-03-13

    Using the IDs has some reason in JCP. I will rename the methods to be more consistent.

     
  • Stefan Kuhn

    Stefan Kuhn - 2009-03-18

    New patch

     
    Attachments
  • Stefan Kuhn

    Stefan Kuhn - 2009-03-18

    Ok, I added a new patch which hopefully keeps the naming more consistent.

     
  • Rajarshi Guha

    Rajarshi Guha - 2009-03-18

    In testGetReactionByReactionID_IReactionSet_String can you add an assert testing that you get back null for a non-existant ID. Otherwise looks good

     
  • Stefan Kuhn

    Stefan Kuhn - 2009-03-18

    another new version of the patch

     
    Attachments
  • Stefan Kuhn

    Stefan Kuhn - 2009-03-18

    Done so.

     
  • Rajarshi Guha

    Rajarshi Guha - 2009-03-18

    I think it's ready to go then

     

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks