#778: Binary Search Tree Iterator #779

mitchellirvin · 2018-08-05T01:15:33Z

In a nutshell: Implemented a BSTIterator that iterates in-order, given a root node. README.md in /binarysearchtree discusses the trivial solution, and an optimal solution (the one implemented in this PR) in depth.

Included comprehensive unit and integration tests. Refactored file structure to be friendly to future contributors with iterators of other data structures. Added JUnitPlatform to enable running test suite across all iterator implementations. Added README to /binarysearchtree to document what it does and how it works.

This is my first PR against an open source project, would love feedback!

… integration tests. Refactored file structure to be friendly to future contributors with iterators of more data structures. Added JUnitPlatform to enable running test suite across all iterator implementations. Added README to /binarysearchtree to document what it does and how it works.

npathai · 2018-08-14T06:20:11Z

@mitchellirvin Thanks for the PR 👍 I have started the review. Will complete it by tomorrow.

IAmPramod · 2018-08-15T04:28:40Z

@mitchellirvin this looks great 👍

npathai

You have my initial review. Let me know once you have made required changes or have any questions.

npathai · 2018-08-15T13:07:26Z

iterator/src/main/java/com/iluwatar/iterator/binarysearchtree/BstIterator.java

+ * expect to retrieve TreeNodes according to the Integer's natural ordering (1, 2, 3...)
+ * @param <T> This Iterator has been implemented with generic typing to allow traversal of TreeNodes of various types
+ */
+public class BstIterator<T> implements Iterator {


Iterator is used as raw type here, change it to Iterator<TreeNode<T>>

npathai · 2018-08-15T13:09:10Z

iterator/src/main/java/com/iluwatar/iterator/binarysearchtree/BstIterator.java

+import java.util.ArrayDeque;
+
+/**
+ * This BstIterator iterates IN order. For example, given a BST with Integer values,


Suggestion: The writeup of javadoc should be changed to something like

An in-order implementation of BST iterator.

npathai · 2018-08-15T13:13:35Z

iterator/src/main/java/com/iluwatar/iterator/binarysearchtree/BstIterator.java

+/**
+ * This BstIterator iterates IN order. For example, given a BST with Integer values,
+ * expect to retrieve TreeNodes according to the Integer's natural ordering (1, 2, 3...)
+ * @param <T> This Iterator has been implemented with generic typing to allow traversal of TreeNodes of various types


Suggestion:

This Iterator has been implemented with generic typing to allow traversal of TreeNodes of various types

I guess the statement should be "it allows TreeNodes with different value types." Or something on that line.

npathai · 2018-08-15T13:16:30Z

iterator/src/main/java/com/iluwatar/iterator/binarysearchtree/BstIterator.java

+  @Override
+  public TreeNode<T> next() throws IllegalStateException {
+    if (pathStack.isEmpty()) {
+      throw new IllegalStateException();


Change the exception to NoSuchElementException

npathai · 2018-08-15T13:17:31Z

iterator/src/main/java/com/iluwatar/iterator/binarysearchtree/TreeNode.java

+   */
+  public TreeNode(T val) {
+    this.val = val;
+    this.left = null;


No need to explicitly initialize class properties to null.

npathai · 2018-08-15T13:42:55Z

iterator/src/test/java/com/iluwatar/iterator/binarysearchtree/BstIteratorTest.java

+  @BeforeAll
+  void createTrees() {
+    nonEmptyRoot = new TreeNode(Integer.valueOf(5));
+    nonEmptyRoot.setLeft(new TreeNode(3));


There are lot of raw type warnings wherever new object of TreeNode is being created, it is because of raw types. TreeNode is generic but it is being used in raw manner. So change creation of TreeNode to new TreeNode<>(val)

npathai · 2018-08-15T13:49:53Z

iterator/src/test/java/com/iluwatar/iterator/binarysearchtree/BstIteratorTest.java

+  void nextAndHasNextAcrossEntirePopulatedTree() {
+    BstIterator iter = new BstIterator(nonEmptyRoot);
+    assertTrue(iter.hasNext(), "Iterator hasNext() should be true.");
+    assertEquals(iter.next().getVal(), 1, "First Node is 1.");


At all places where assertEquals is being used the order of arguments is incorrect. assertEquals(expected, actual) should be the order of arguments

npathai · 2018-08-15T13:52:15Z

iterator/src/test/java/com/iluwatar/iterator/binarysearchtree/TreeNodeTest.java

+public class TreeNodeTest<T> {
+
+  @Test
+  void treeNodeGetters() {


TreeNodeTest should not be part of unit testing. The setters and getters are being tested indirectly by BSTIteratorTest already.
Testing beans is not a good practice. Yes there are exceptions to this, if beans have validation logic then it makes sense.

Appreciate the feedback! Wasn't sure if I needed this to ensure code coverage or not.

npathai · 2018-08-15T13:56:23Z

iterator/src/main/java/com/iluwatar/iterator/binarysearchtree/README.md

+This means that given a binary search tree like the following, the iterator would 
+return values in order: 1, 3, 4, 6, 7, 8, 10, 13, 14. 
+
+![BST](../../../../../../../etc/bst.png "Binary Search Tree")  


Can we reduce the image size a bit? Its quiet big

npathai · 2018-08-15T14:01:42Z

iterator/src/main/java/com/iluwatar/iterator/list/App.java

 * access the container's elements. The Iterator pattern decouples algorithms from containers.
 * <p>
- * In this example the Iterator ({@link ItemIterator}) adds abstraction layer on top of a collection
+ * In this example the Iterator ({@link Iterator}) adds abstraction layer on top of a collection


While investigating this change I found out that the current TreasureChestIterator implementation is incorrect. It is making assumptions about the underlying datastructure being ArrayList. And if it was changed to LinkedList, then iterator would perform really badly, which violates the whole purpose of iterator. So will have to investigate how to rectify that.

npathai · 2018-08-15T14:06:30Z

@iluwatar There are a few things that I needed your help with.

How do we represent flavour implementations of same pattern, like in this cast list and bst
I guess current implementation of TresureChestIterator is incorrect, because it is making assumption about underlying type to be ArrayList and if it was changed to LinkedList, then iterator will perform badly, which violates the purpose of Iterator pattern. Will need to rectify that. What do you think? I will work on that and raise PR if you agree.

npathai · 2018-08-15T14:09:21Z

iterator/src/test/java/com/iluwatar/iterator/AppTest.java

- *
+ * Runs a test suite containing all tests within the com.iluwatar.iterator package
 */
+@RunWith(JUnitPlatform.class)


How does adding JUnitPlatform dependency help? I haven't switched to using Junit 5 yet, hence the question.

From the limited experience I have with it, I've found it's a great tool to define and run suites of tests instead of individual test classes one at a time. If you want to run all the tests for the iterator package you can run the AppTest class instead of manually running each *Test class.

I am still unsure about this change. I read about JUnitPlatform and I understand that it makes running all test cases simple. But AppTest wasn't meant for that. It tests App class and it's not being run currently.
Here is what we can do, we can revert this change and raise an issue to discuss about having AllTests class in every pattern. Till then I am afraid you will have to revert this change. Let me know what are your thoughts.

mitchellirvin · 2018-08-15T18:25:17Z

@npathai committed the requested changes. left the explicit null assignments in the TreeNode constructor because of "Clean Code"'s preference for the explicit over the implicit.

mitchellirvin

requested changes made and committed, build passing

npathai · 2018-08-17T07:53:03Z

iterator/src/main/java/com/iluwatar/iterator/bst/BstIterator.java

+  /**
+   *
+   * @return TreeNode next. The next element according to our in-order traversal of the given BST
+   * @throws IllegalStateException if this iterator does not have a next element


Javadoc needs to be updated accordingly.

npathai · 2018-08-17T07:58:34Z

iterator/src/test/java/com/iluwatar/iterator/AppTest.java

- *
+ * Runs a test suite containing all tests within the com.iluwatar.iterator package
 */
+@RunWith(JUnitPlatform.class)


I am still unsure about this change. I read about JUnitPlatform and I understand that it makes running all test cases simple. But AppTest wasn't meant for that. It tests App class and it's not being run currently.
Here is what we can do, we can revert this change and raise an issue to discuss about having AllTests class in every pattern. Till then I am afraid you will have to revert this change. Let me know what are your thoughts.

mitchellirvin · 2018-08-17T12:50:26Z

@npathai I agree with removing AppTest and JUnitPlatform after further research. I've committed the changes (along with the updated javadoc).

npathai · 2018-08-19T05:03:35Z

@mitchellirvin Looks good.
Few points:

we will need to keep AppTest class for now. Its a pattern that is followed in other patterns, so we should keep it for now. We can surely discuss about these points in a new issue related to JunitPlatform.
Also add BST iterator sample in App.java. This class is client code of pattern and serves the purpose of showcasing how pattern should be used. I hope that makes sense.

mitchellirvin · 2018-08-22T16:04:53Z

@npathai Roger that! Should have them in within the next several days.

…e each implementation of the Iterator interface. Removed the redundant ItemIterator interface. Added insert() method to TreeNode class to allow for more elegant construction of BSTs.

mitchellirvin · 2018-08-25T22:55:15Z

@npathai Sorry for the delay, ended up doing more refactoring than originally planned. Should be good for a final review. Let me know what you think of the changes? Tried hard to make the code clean and readable.

npathai

A quick little change needed. Otherwise it's 👍

npathai · 2018-08-28T07:29:33Z

iterator/src/main/java/com/iluwatar/iterator/list/TreasureChest.java

  }

-  ItemIterator iterator(ItemType itemType) {
+  public TreasureChestItemIterator iterator(ItemType itemType) {


Return type should be changed to Iterator

npathai · 2018-08-28T07:32:06Z

iterator/src/test/java/com/iluwatar/iterator/list/TreasureChestTest.java

  public void testIterator(Item expectedItem) {
    final TreasureChest chest = new TreasureChest();
-    final ItemIterator iterator = chest.iterator(expectedItem.getType());
+    final TreasureChestItemIterator iterator = chest.iterator(expectedItem.getType());


Same change here. You being the client of TreasureChest don't care if the concrete implementation of Iterator is TreasureChestItemIterator or something else. You just need an Iterator. This allows for easier refactoring as you can freely change name of the class and client code will not be affected. I hope that makes sense.

Ah, crap. Good catch!

… Iterator

mitchellirvin · 2018-08-28T14:34:50Z

@npathai should be good now. Thanks for all your help in this review!

npathai

I don't know how I missed these changes before. Caught my eye when I saw unchecked cast to Item in test case. Apologies.

npathai · 2018-08-29T10:22:44Z

iterator/src/main/java/com/iluwatar/iterator/App.java

+  private static void demonstrateTreasureChestIteratorForType(ItemType itemType) {
+    LOGGER.info("------------------------");
+    LOGGER.info("Item Iterator for ItemType " + itemType + ": ");
+    Iterator itemIterator = TREASURE_CHEST.iterator(itemType);


Raw type: Iterator<Item> should be returned from TREASURE_CHEST.iterator()

npathai · 2018-08-29T10:25:14Z

iterator/src/main/java/com/iluwatar/iterator/list/TreasureChest.java

  }

-  ItemIterator iterator(ItemType itemType) {
+  public Iterator iterator(ItemType itemType) {


return type should not be raw, it should be Iterator<Item>. Previously this was not needed because there was a dedicated interface called ItemIterator which only worked for Items

npathai · 2018-08-29T10:25:43Z

iterator/src/main/java/com/iluwatar/iterator/list/TreasureChestItemIterator.java

 *
 */
-public class TreasureChestItemIterator implements ItemIterator {
+public class TreasureChestItemIterator implements Iterator {


Raw type: it should be implements Iterator<Item>

npathai · 2018-08-29T10:26:09Z

iterator/src/test/java/com/iluwatar/iterator/list/TreasureChestTest.java

  public void testIterator(Item expectedItem) {
    final TreasureChest chest = new TreasureChest();
-    final ItemIterator iterator = chest.iterator(expectedItem.getType());
+    final Iterator iterator = chest.iterator(expectedItem.getType());


Raw type: Iterator<Item>

npathai · 2018-08-29T10:26:59Z

iterator/src/test/java/com/iluwatar/iterator/list/TreasureChestTest.java


    while (iterator.hasNext()) {
-      final Item item = iterator.next();
+      final Item item = (Item) iterator.next();


No need for cast now, it was needed because of raw types. It will be removed now.

npathai · 2018-08-30T07:16:36Z

@mitchellirvin It's done. Thanks 👍

mitchellirvin force-pushed the bst-iterator branch from 5726874 to d32cb41 Compare August 5, 2018 01:49

mitchellirvin force-pushed the bst-iterator branch from d32cb41 to 3e0cfa5 Compare August 5, 2018 02:00

mitchellirvin changed the title ~~#778 Implemented BSTIterator.~~ #778: Binary Search Tree Iterator Aug 5, 2018

mitchellirvin mentioned this pull request Aug 7, 2018

Implement Iterator for Binary Search Tree #778

Closed

npathai self-requested a review August 8, 2018 18:30

npathai added the status: under review label Aug 8, 2018

npathai suggested changes Aug 15, 2018

View reviewed changes

npathai reviewed Aug 15, 2018

View reviewed changes

mitchellirvin commented Aug 15, 2018

View reviewed changes

npathai suggested changes Aug 17, 2018

View reviewed changes

mitchellirvin force-pushed the bst-iterator branch from 759cb8f to 7610980 Compare August 17, 2018 11:35

mitchellirvin force-pushed the bst-iterator branch from 7610980 to 3971dfb Compare August 21, 2018 18:37

mitchellirvin force-pushed the bst-iterator branch from 3971dfb to 363ea63 Compare August 25, 2018 22:37

Refactored App.java to remove duplicate code and elegantly demonstrat…

1c2ddfa

…e each implementation of the Iterator interface. Removed the redundant ItemIterator interface. Added insert() method to TreeNode class to allow for more elegant construction of BSTs.

mitchellirvin force-pushed the bst-iterator branch from 363ea63 to 1c2ddfa Compare August 25, 2018 22:49

npathai suggested changes Aug 28, 2018

View reviewed changes

modified return type of TreasureChestItemIterator's constructor to be…

e6f84f2

… Iterator

npathai suggested changes Aug 29, 2018

View reviewed changes

correcting raw types for the item Iterator.

8458e42

npathai approved these changes Aug 30, 2018

View reviewed changes

npathai merged commit 038befe into iluwatar:master Aug 30, 2018

mitchellirvin deleted the bst-iterator branch August 30, 2018 11:38

Uh oh!

#778: Binary Search Tree Iterator #779

#778: Binary Search Tree Iterator #779

Uh oh!

Conversation

mitchellirvin commented Aug 5, 2018

Uh oh!

npathai commented Aug 14, 2018

Uh oh!

IAmPramod commented Aug 15, 2018

Uh oh!

npathai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

npathai commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchellirvin commented Aug 15, 2018

Uh oh!

mitchellirvin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchellirvin commented Aug 17, 2018

Uh oh!

npathai commented Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mitchellirvin commented Aug 22, 2018

Uh oh!

mitchellirvin commented Aug 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

npathai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchellirvin commented Aug 28, 2018

Uh oh!

npathai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

npathai commented Aug 19, 2018 •

edited

Loading

mitchellirvin commented Aug 25, 2018 •

edited

Loading