Iterator reborn #53

Merged
merged 7 commits into from May 17, 2012

Projects

None yet

4 participants

@stof

This is a refactoring of my previous iterator implementation, as well as a new filter iterator and some documentation.
As explained in one of the commit message, the IteratorAggregate implementation of the MenuItem is back to the native ArrayIterator as we have lots of code using non-recursive iteration during the rendering of the menu so keeping the native iterator may offer better performances.

@weaverryan if you have some time to proof-read the doc, it would be great

stof added some commits May 8, 2012
@stof stof Refactored the recursive iterator
The recursive iterator is not returned anymore by the IteratorAggregate
implementation in the MenuItem to keep the native ArrayIterator whenever
possible (many places are iterating on the item without needing the
recursion)
The ItemIterator now wraps the iterator of the item to provide the
recursion.
cc5a570
@stof stof Renamed the ItemIterator to RecursiveItemIterator 19156b5
@stof stof Added a DisplayedItemFilterIterator 96eefb2
@stof stof Added a test as an example of iteration on all items of the tree a332597
@stof stof Added a tests for filtering the direct children of an item only
This made me realize that the FilterIterator will only accept an Iterator,
not an IteratorAggregate, forcing us to call ->getIterator() or to use
an IteratorIterator.
539310f
@stof stof Added some doc for the iterators 92317a4
@merk merk commented on an outdated diff May 8, 2012
doc/04-Iterators.md
+
+```
+ A
+ / \
+ / \
+ B C
+ / | \ / \
+D E F G H
+| |
+I J
+```
+
+Iterating recursively
+---------------------
+
+The `Knp\Menu\Iterator\RecursiveItemIterator` allows to iterate recursively
@merk
merk May 8, 2012

Grammar: allows you to iterate

@merk

Looks good. My only other comment is it would be good if the docs explained why you would want to use the CHILD_FIRST iterator, most people will only want the CHILD_LAST?

@stof

@merk there is no CHILD_LAST. It is SELF_FIRST. and it depends of what you want to do. When getting current items, you may prefer getting first the child element if both a element and its parent are marked as current (if you only want to access the first one to build a breadcrumb for instance)

@merk

Sorry, i meant SELF_FIRST. I just thought it might be a good idea documentation wise to provide a real world example for each iterator (even if you point to existing renderer code that uses the iterator when possible), makes it easier to understand.

Otherwise 👍

@weaverryan weaverryan commented on an outdated diff May 9, 2012
doc/04-Iterators.md
@@ -0,0 +1,172 @@
+Iterating over Menus
+====================
+
+The simpliest way to iterate over your item object is to iterate over its
@weaverryan weaverryan commented on an outdated diff May 9, 2012
doc/04-Iterators.md
@@ -0,0 +1,172 @@
+Iterating over Menus
+====================
+
+The simpliest way to iterate over your item object is to iterate over its
+direct children, thanks to the `IteratorAggregate` implementation. But the
+PHP iterators allows much more power to deal with our tree structure.
@weaverryan
weaverryan May 9, 2012

allow much more powerful options to deal...

@weaverryan weaverryan commented on an outdated diff May 9, 2012
doc/04-Iterators.md
+// iterate recursively on the iterator
+$iterator = new \RecursiveIteratorIterator($itemIterator, \RecursiveIteratorIterator::SELF_FIRST);
+
+foreach ($iterator as $item) {
+ echo $item->getName() . " ";
+}
+```
+
+The output will be:
+
+```
+B D I E F C G J H
+```
+
+Changing the second argument to `\RecursiveIteratorIterator::CHILD_FIRST`
+allows to visit children before their parent and will produce the following
@weaverryan weaverryan and 1 other commented on an outdated diff May 9, 2012
doc/04-Iterators.md
+allows to visit children before their parent and will produce the following
+order:
+
+```
+I D E F B J G H C
+```
+
+>**NOTE**
+>If you iterate over the `RecursiveItemIterator` without wrapping it in the
+>`\RecursiveIteratorIterator`, it will simply give you the direct children
+>like when using the iterator of the item.
+
+The final iterator does not contain the root item, as we started the iteration
+on its children. Fortunately, the `RecursiveItemIterator` does not expect
+getting an item but any iterator over menu items. This allows adding the
+root in the final iterator by changing the inner iterator:
@weaverryan
weaverryan May 9, 2012

The last 2 sentences here aren't quite clear - the english just a little off I think, but it's also a bit on the technical side - hard to read.

@stof
stof May 11, 2012

could you check the reworked version ?

@weaverryan weaverryan commented on an outdated diff May 9, 2012
doc/04-Iterators.md
+ new \ArrayIterator(array($menu))
+ ),
+ \RecursiveIteratorIterator::SELF_FIRST
+);
+
+$iterator = new \Knp\Menu\Iterator\CurrentItemFilterIterator($treeIterator);
+
+foreach ($iterator as $item) {
+ echo $item->getName() . " ";
+}
+```
+
+Filtering only displayed items
+------------------------------
+
+The `Knp\Menu\Iterator\DisplayedItemFilterIterator` allows to filter items
@weaverryan
weaverryan May 9, 2012

allows you to filter...

@weaverryan weaverryan commented on an outdated diff May 9, 2012
doc/04-Iterators.md
+ \RecursiveIteratorIterator::SELF_FIRST
+);
+
+$iterator = new \Knp\Menu\Iterator\CurrentItemFilterIterator($treeIterator);
+
+foreach ($iterator as $item) {
+ echo $item->getName() . " ";
+}
+```
+
+Filtering only displayed items
+------------------------------
+
+The `Knp\Menu\Iterator\DisplayedItemFilterIterator` allows to filter items
+to keep only displayed ones. As hiding an item also hides its children, this
+filters is a recursive filter iterator and is applied on the recursive iterator,
@weaverryan
weaverryan May 9, 2012

filter is a...

@weaverryan
KNP Labs member

Wow, I think this feature is like a textbook on iterators :). Having the clear examples is a must, nice work!

@pminnieur

👍

@stof stof merged commit 9b4b988 into master May 17, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment