#9 foreach syntax support

closed
None
5
2012-10-08
2007-12-07
No

would be nice to iterate i.e. over an AtomContainerSet with the foreach syntax. Therefore the Iterable<T> interace must be implemented. See attached patch.

Discussion

  • Egon Willighagen

    Logged In: YES
    user_id=25678
    Originator: NO

    This has been discussed in the past, but postponed after CDK 1.0... could be applied to trunk/, but I guess the patch needs to be extended throughout the core interfaces, to keep things consistent?

    Thomas, so you have interest in extending this for the other interfaces (IAtom, IAtomContainer, IElectronContainer, etc) too? If so, why not open a branch?

     
  • Rajarshi Guha

    Rajarshi Guha - 2007-12-10

    Logged In: YES
    user_id=349408
    Originator: NO

    But if one 'iterates' over an IAtomContainer, what is one iterating over? Atoms or bonds?

    I think this should not be applied IAtomContainer, IMolecule

    Also for IAtom, what would one iterate over?

     
  • Egon Willighagen

    Logged In: YES
    user_id=25678
    Originator: NO

    Rajarshi, like the current .atoms() method, can't we return something that implements Iterable<IAtom>, where we now return a Iterator<IAtom> ?

     
  • Rajarshi Guha

    Rajarshi Guha - 2007-12-10

    Logged In: YES
    user_id=349408
    Originator: NO

    Aah yes, that makes sense

     
  • Thomas Holder

    Thomas Holder - 2007-12-10

    Logged In: YES
    user_id=1306887
    Originator: YES

    yes I'm interested in extending this, so I assigned to myself :) I already tinkered with an IterableIterator<T> class or interface which merges Iterator<T> and Iterable<T> interface. I'll have a closer look at it and then post another patch here or try to open a branch like Egon suggested.

     
  • Thomas Holder

    Thomas Holder - 2007-12-22

    Logged In: YES
    user_id=1306887
    Originator: YES

    Egon, I'm not sure if I understood this right: I may "hack away in that branch" as I like but not change any source files? Sounds like a contradiction to me!?

     
  • Egon Willighagen

    Logged In: YES
    user_id=25678
    Originator: NO

    Thomans, in that branch you can make any code change you like. What you should not do, is change the formatting. If you do, the differences with trunk will be mostly those formatting changes, and not the source code changes, making it difficult to judge your work.

     
  • Thomas Holder

    Thomas Holder - 2007-12-22

    Logged In: YES
    user_id=1306887
    Originator: YES

    i see, that makes sense :) however, my branch stucks at rev 9594. I committed my changes. There are basically two concepts: Classes that are iterable themselves, like AtomContainerSet or ChemSequence. And classes that have more than one way to iterate over like AtomContainer, they now return an IterableIterator<T> instead of just an Iterator<T> with their methods like atoms() or bonds().

     
  • Egon Willighagen

    Logged In: YES
    user_id=25678
    Originator: NO

    Nice and clean patch. Since the old behavior is intact, it does not require extensive changes all over the place.
    One problem: you need to make the IterableIterator LGPL, instead of GPL, because right now it cannot be used.

    If you're branch is last synchronized with trunk 9594, you can synchronize it again by doing:

    cdk speleo3/iterable/
    svn merge -r 9594:HEAD https://cdk.../cdk/trunk/cdk

    And then review the modifications and make sure things still compile.

    There also need to be additional JUnit tests, to make sure the foreach syntax iterates over the right number of atoms/etc, something like

    IMolecule mol = createSomeMolecule();
    int iteratedAtoms = 0;
    foreach (IAtom atom : mol.atoms()) {
    iteratesAtoms++;
    }
    assertEquals(realNumberOfAtoms, iteratedAtoms);

     
  • Rajarshi Guha

    Rajarshi Guha - 2007-12-22

    Logged In: YES
    user_id=349408
    Originator: NO

    I was looking at the code and I was wondering what is the rational for the new class IterableIterator<T> rather than just using Iterator<T>

     
  • Rajarshi Guha

    Rajarshi Guha - 2007-12-22

    Logged In: YES
    user_id=349408
    Originator: NO

    Another thing I saw was that in AtomContainerSet you have two methods atomContainers and iterator, the former being an alias for the latter. Personally I would avoid having multiple equivalent ways of doing a thing - it makes life easier for the user of the library to know the 'one good' way to do a thing.

    Probably a snall point, but coming from a Python background I appreciate the fact that there's usually one way to do something, as opposed to the multiple ways in Perl.

    Moreover since both methods are doing he exact same thing (and they are not polymorphic) I would say that only one of them (iterator, since that's required to satisfy the interface) be retained

     
  • Thomas Holder

    Thomas Holder - 2007-12-23

    Logged In: YES
    user_id=1306887
    Originator: YES

    this is the advantage of IterableIterator<T> over just Iterator<T> (example):

    old:
    Iterator iter = atomContainer.bonds();
    while (iter.hasNext()) {
    IBond bond = (IBond) iter.next();
    }

    new:
    for (IBond bond : atomContainer.bonds()) {
    }

    Avoiding multiple ways for the same thing is a good plan, but I don't know how to reconcile this with the plan not to break old code :( For now I marked the replaced functions as @deprecated.

     
  • Egon Willighagen

    Logged In: YES
    user_id=25678
    Originator: NO

    My 50cents...

    For IAtomContainer just iterator() won't work, because there is bonds(), atoms(), lonePairs() etc. For many other Set-like classes it would be possible.

    However, I'd like to see consistency over non-redundancy... that is, I'd rather have iterator() and chemSequences(), to see the latter syntax consistent with atoms()...

    Then again, maybe it is not that bad to allow just iterator() for cdk.interfaces.I*Set ...

     
  • Egon Willighagen

    Logged In: YES
    user_id=25678
    Originator: NO

    Patch incorporated in patch 2040231. Thanx Thomas for starting this!

     

Log in to post a comment.

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

Sign up for the SourceForge newsletter:





No, thanks