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
+returninserted
+
+defsearch(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

+returninserted
+
+defsearch(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

+
+defprint_postorder(self, node):
+if(nodeisnotNone):
+self.print_preorder(node.left_node)


Post order should be
self.print_postorder(node.right_node) # Right comes firstself.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
+whilenotfound:
+if(cur_nodeisNone):
+raiseValueError("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

 "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

+
+classBinarySearchTree(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 @@
+classTreeNode(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

 }
 ],
 "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()
+
+deftest_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
+returninserted


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

+defadd(self, value):
+pass
+
+defsearch(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

 ],
 "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)
+
+defprint_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

rodrigocamand 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

rodrigocamand 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' ofhttps://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:masterNov 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

Uh oh!

Implement exercise binary-search-tree#773

Implement exercise binary-search-tree #773

Uh oh!

Conversation

rodrigocam commented Oct 8, 2017

Uh oh!

ilya-khadykin commented Oct 9, 2017

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!

N-Parsons 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

N-ParsonsNov 2, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

N-ParsonsNov 5, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccandless left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

N-ParsonsNov 2, 2017•
edited
Loading

N-ParsonsNov 5, 2017•
edited
Loading

cmccandless left a comment •
edited
Loading