Implement exercise binary-search-tree #773

rodrigocam · 2017-10-08T22:59:20Z

Resolved #740

ilya-khadykin · 2017-10-09T07:44:19Z

You've created two PRs #773 and #768
Could you please close one to avoid confusion

devkabiir · 2017-10-10T04:39:09Z

exercises/binary-search-tree/example.py

+                        inserted = True
+                        return inserted
+
+    def search(self, searched_number):


I recommend changing searched_number to either number or search_number, searched is in past tense which means you've already searched for it.

Ok, changed!

devkabiir · 2017-10-10T04:41:37Z

exercises/binary-search-tree/example.py

+                        return inserted
+
+    def search(self, searched_number):
+        cur_node = self.root


Also add a check for if(self.root is None): return false which does an early exit if the tree hasn't been constructed.

Ok, checked!

devkabiir · 2017-10-10T04:49:01Z

exercises/binary-search-tree/example.py

+
+    def print_postorder(self, node):
+        if(node is not None):
+            self.print_preorder(node.left_node)


Post order should be

self.print_postorder(node.right_node) # Right comes first self.print_postorder(node.left_node) print(node.value)

Also you're calling print_preorder() .

devkabiir · 2017-10-10T04:50:44Z

exercises/binary-search-tree/example.py

+        found = False
+        while not found:
+            if(cur_node is None):
+                raise ValueError("Element not found")


You won't need to raise an exception if you do an early return false

…e tree root is none

cmccandless · 2017-10-23T18:09:38Z

config.json

@@ -888,9 +888,22 @@
      "uuid": "6b7f7b1f-c388-4f4c-b924-84535cc5e1a0",
      "slug": "hexadecimal",
      "deprecated": true
+    },


It would probably be best to move the config.json entry for this exercise before all deprecated exercises.

N-Parsons

Looks good overall, but there are a couple of issues that need to be resolved.

N-Parsons · 2017-11-01T09:14:39Z

exercises/binary-search-tree/README.md

@@ -0,0 +1,52 @@
+Insert and search for numbers in a binary tree.


This needs the title of the exercise as a heading: # Binary Search Tree

N-Parsons · 2017-11-01T09:18:11Z

exercises/binary-search-tree/binary_search_tree.py

+
+class BinarySearchTree(object):
+    def __init__(self):
+        pass


Can you also include skeletons for the add and search methods here?

N-Parsons · 2017-11-01T09:20:29Z

exercises/binary-search-tree/binary_search_tree.py

@@ -0,0 +1,8 @@
+class TreeNode(object):
+    def __init__(self, value):
+        pass


Can you add self.value = value here? I feel like if we assume that something exists in the tests, then we should make it clear in the solution template.

N-Parsons · 2017-11-01T09:26:38Z

config.json

@@ -1169,6 +1183,5 @@
    }
  ],
  "foregone": [
-


Can you add this blank line back in?

I looks like you've got some unnecessary white-space here now (see below) - can you please remove it?

N-Parsons · 2017-11-01T09:29:35Z

exercises/binary-search-tree/README.md

+       /     \
+      2       6
+     / \     / \
+    1   3   5   7


There should also be some other stuff that goes here. Check another exercise and copy it over, or regenerate this README with the configlet.

cmccandless · 2017-11-01T19:51:49Z

exercises/binary-search-tree/binary_search_tree_test.py

+    bst = BinarySearchTree()
+
+    def test_add_integer_number1(self):
+        self.assertTrue(self.bst.add(7))


We use assertIs(actual, True) and assertIs(actual, False) rather than assertTrue(actual) or assertFalse(actual) (#419).

N-Parsons · 2017-11-02T12:32:12Z

exercises/binary-search-tree/example.py

+                    else:
+                        cur_node.right_node = TreeNode(value)
+                        inserted = True
+                        return inserted


Looking at this again, I just noticed that this method is returning True/False to indicate success/failure. Under what condition is failure expected to occur? From what I can see, this code will never return False, and would actually just enter an infinite loop if value == cur_node.value. I don't think that we should use booleans in this way - exceptions are a better method for handling errors in Python, but I don't think that any exceptions should be expected to occur here.

I think it would be worth adding a BinarySearchTree.list method to this exercise that returns a sorted list of values in the tree. A collections.deque would be most efficient for this, but I don't think that the tests should enforce its use, so the tests should probably convert the actual output to a list (eg. assertEqual(list(self.bst.list()), [1, 2, 3])).

Doing this would allow us to check that values are placed in the right order - each test for adding a number should contain a second assert for the current list of stored values. We should also test duplicate values, because the README specifies a way for these to be handled.

From the README:

All data in the left subtree is less than or equal to the current node's data, and all data in the right subtree is greater than the current node's data.

@N-Parsons Returning True from an add or insert method is a common practice for container classes. I agree that it doesn't make the most sense, but that is likely why @rodrigocam did this.

@cmccandless is right, I thought this could be good to give more safety. I made the list method that @N-Parsons asked and change the tests to use this new method. I fix the infinite loop condition that you point but I keep the bool return, if it was a problem I can remove (:

Given that there's no way that False can be returned, and there is no test for this method returning False, I think it's unnecessary and likely to cause some confusion/frustration. I think it's fine for this example code to return True/False, but I don't think that it should be tested for.

Done! I take out the return of the method.

cmccandless

@rodrigocam Travis-CI build failed due to flake8 violations:

$ flake8
./exercises/binary-search-tree/binary_search_tree_test.py:15:46: E231 missing whitespace after ','
./exercises/binary-search-tree/binary_search_tree_test.py:15:48: E231 missing whitespace after ','
./exercises/binary-search-tree/binary_search_tree_test.py:15:50: E231 missing whitespace after ','
./exercises/binary-search-tree/binary_search_tree_test.py:15:52: E231 missing whitespace after ','
./exercises/binary-search-tree/binary_search_tree_test.py:24:48: E231 missing whitespace after ','
./exercises/binary-search-tree/binary_search_tree_test.py:24:52: E231 missing whitespace after ','
./exercises/binary-search-tree/binary_search_tree_test.py:24:56: E231 missing whitespace after ','
./exercises/binary-search-tree/binary_search_tree_test.py:24:60: E231 missing whitespace after ','
./exercises/binary-search-tree/binary_search_tree_test.py:32:46: E231 missing whitespace after ','
./exercises/binary-search-tree/binary_search_tree_test.py:32:50: E231 missing whitespace after ','
./exercises/binary-search-tree/binary_search_tree_test.py:32:54: E231 missing whitespace after ','
./exercises/binary-search-tree/binary_search_tree_test.py:40:46: E231 missing whitespace after ','
./exercises/binary-search-tree/binary_search_tree_test.py:40:48: E231 missing whitespace after ','
./exercises/binary-search-tree/binary_search_tree_test.py:40:52: E231 missing whitespace after ','
./exercises/binary-search-tree/binary_search_tree_test.py:48:1: W293 blank line contains whitespace
./exercises/binary-search-tree/example.py:3:1: E302 expected 2 blank lines, found 1
./exercises/binary-search-tree/example.py:82:30: W292 no newline at end of file
The command "flake8" failed and exited with 1 during .

N-Parsons · 2017-11-05T22:30:30Z

exercises/binary-search-tree/binary_search_tree_test.py

+        bst.add(1)
+        bst.add(7.5)
+        self.assertIs(bst.search(6), False)
+        self.assertIs(bst.search(8.8), False)


I don't like that search will sometimes return a bool and sometimes return a TreeNode. I think that this should either be TreeNode/None, or True/False - the former is perhaps more generally useful, since nodes are truthy and None is falsey, and returning the actual node could be useful in some instances.

Done! I change to return None when the node is not found.

N-Parsons · 2017-11-12T19:50:56Z

exercises/binary-search-tree/binary_search_tree.py

+    def add(self, value):
+        pass
+
+    def search(self, search_number):


I think this should use value rather than search_number as an argument, since we put values in and then search for them, and search_ is probably redundant.

@rodrigocam, it looks like you've updated this in example.py but not here.

N-Parsons · 2017-11-12T19:55:57Z

exercises/binary-search-tree/.cache/v/cache/lastfailed

+  "binary_search_tree_test.py::BinarySearchTreeTests::test_search_valid_number1": true,
+  "binary_search_tree_test.py::BinarySearchTreeTests::test_search_valid_number2": true,
+  "binary_search_tree_test.py::BinarySearchTreeTests::test_search_valid_number3": true
+}


You need to remove the .cache directory.

N-Parsons · 2017-11-12T19:57:31Z

Hi @rodrigocam, sorry for taking a while to get back to this - I've had a really busy couple of weeks!

There are a couple of minor issues remaining that need to be dealt with, and then this can be merged.

N-Parsons · 2017-11-13T13:33:42Z

config.json

@@ -1185,6 +1199,6 @@
    }
  ],
  "foregone": [
-
+    


@rodrigocam, the extra spaces here also need to be removed. There should be a blank line, but there should be nothing in it.

N-Parsons · 2017-11-13T13:34:56Z

@cmccandless, can you review this again? There are a couple of minor changes pending and then I think this will be ready to merge.

cmccandless · 2017-11-13T15:19:55Z

exercises/binary-search-tree/example.py

+            elements.append(node.value)
+            self.trav_inorder(node.right_node, elements)
+
+    def print_inorder(self, node):


We should probably remove all print* methods, as they are not used by the tests.

N-Parsons · 2017-11-13T19:38:42Z

Looks good. Thanks @rodrigocam!

rodrigocam added 9 commits October 8, 2017 01:37

Implement exercise binart-search-tree

cae11fe

Add TreeNode class and binary search tree add method

d8256db

Add preorder, postorder and inorder traversal

5b65169

Add search method

06b303f

Add some tests

b39e8d8

Add more testes, examle file and a readme

a5350dc

Fix flake8 diretives

5604b08

Add a new entry to config.json

1013ec8

Change new key place

5883681

This was referenced Oct 9, 2017

[WIP] Implement exercise binary-search-tree #768

Closed

binary-search-tree: implement exercise #740

Closed

ilya-khadykin added the please review label Oct 9, 2017

devkabiir suggested changes Oct 10, 2017

View reviewed changes

Change search paramater name and add early if condition to exit if th…

ab2fa3e

…e tree root is none

ilya-khadykin added the maintainer action required label Oct 19, 2017

ilya-khadykin mentioned this pull request Oct 19, 2017

binary-search-tree: Implement exercise binary-search-tree #952

Closed

N-Parsons mentioned this pull request Oct 20, 2017

Inorder program #963

Closed

cmccandless suggested changes Oct 23, 2017

View reviewed changes

devkabiir approved these changes Oct 26, 2017

View reviewed changes

rodrigocam and others added 2 commits October 27, 2017 13:32

Change exercise position in config.json

1425ffc

Merge branch 'master' into master

cb4e3bf

cmccandless approved these changes Oct 27, 2017

View reviewed changes

cmccandless added 3 commits October 27, 2017 11:38

Merge branch 'master' into master

fed194a

Merge branch 'master' into master

9990ec2

fix config.json syntax error

7192861

cmccandless requested a review from ilya-khadykin October 27, 2017 19:22

N-Parsons suggested changes Nov 1, 2017

View reviewed changes

cmccandless suggested changes Nov 1, 2017

View reviewed changes

N-Parsons reviewed Nov 2, 2017

View reviewed changes

rodrigocam and others added 7 commits November 2, 2017 22:48

Add title at readme

62c5933

Add necessary info at readme

49f6701

Add skeletons for add and search methods

10627f2

Change assert used by tests

f4d558f

Add list method

5663d14

Change tests to include the new list method

56f971b

Merge branch 'master' into master

fa1de56

cmccandless suggested changes Nov 3, 2017

View reviewed changes

cmccandless mentioned this pull request Nov 4, 2017

Create binary search tree #1094

Closed

N-Parsons reviewed Nov 5, 2017

View reviewed changes

rodrigocam added 3 commits November 8, 2017 23:13

Fix flake8 guidelines

805557a

Change search and add method

3b916a5

Merge branch 'master' of https://github.com/rodrigocam/python

1332fdc

N-Parsons reviewed Nov 12, 2017

View reviewed changes

N-Parsons self-assigned this Nov 12, 2017

Change variable name in search method and delete .cache directory

cd94b1f

N-Parsons reviewed Nov 13, 2017

View reviewed changes

rodrigocam added 2 commits November 13, 2017 11:48

Remove spaces on blank line

18c399f

Change parameter of search method

a01cb5a

cmccandless suggested changes Nov 13, 2017

View reviewed changes

Remove print methods

c054b29

cmccandless approved these changes Nov 13, 2017

View reviewed changes

N-Parsons approved these changes Nov 13, 2017

View reviewed changes

Merge branch 'master' into master

d43a2ba

N-Parsons merged commit f236ea1 into exercism:master Nov 13, 2017

cmccandless mentioned this pull request Feb 15, 2018

[WIP] Full-scale test update #1262

Closed

77 tasks

		@@ -0,0 +1,52 @@
		Insert and search for numbers in a binary tree.

+                     /     \
+6
+                   / \     / \
+3   5   7

Implement exercise binary-search-tree #773

Implement exercise binary-search-tree #773

Conversation

rodrigocam commented Oct 8, 2017

ilya-khadykin commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Parsons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Parsons Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Parsons Nov 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmccandless left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Parsons commented Nov 12, 2017

Choose a reason for hiding this comment

N-Parsons commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Parsons commented Nov 13, 2017

N-Parsons Nov 2, 2017 •

edited

Loading

N-Parsons Nov 5, 2017 •

edited

Loading

cmccandless left a comment •

edited

Loading