implement exercise binary-search-tree #412

voila · 2019-10-02T22:19:19Z

No description provided.

voila · 2019-10-02T22:21:08Z

NobbZ · 2019-10-05T08:36:25Z

As we have canonical data available in JSON format, could you please open a PR at https://github.com/exercism/erlang-tests-generator, adding the testgenerator and then pushing the generated tests on this branch?

I totally understand if you prefer to have the manually implemented tests merged as they are, but then you have to wait up to the end of the month before I can do a proper review (of course I try to as fast as possible).

NobbZ · 2019-10-08T11:51:22Z

Hello @voila!

Do you want me to review as is or do you want to give the test-generator a try?

juhlig · 2019-10-09T09:51:02Z

exercises/binary-search-tree/test/binary_search_tree_tests.erl

@@ -0,0 +1,112 @@
+%% Based on canonical data version 1.4.0
+%% https://github.com/exercism/problem-specifications/raw/master/exercises/bob/canonical-data.json
+%% This file is automatically generated from the exercises canonical data.


If the test suite is created manually (ie, there is no test generator), this line is wrong...

It were even wrong if it were from the generator, as it claims to be generated from bobs canonical data…

NobbZ

Could you also please rebase on master or cherry pick d8c83e7 and 7210cc1 into your PR, such that the tests are run against your PR?

NobbZ · 2019-10-10T14:44:29Z

@voila you have been pretty active with your other PR.

Please show some activity here as well and answer at least the open question, as well as fixing merge conflicts and obviously wrong comment in the test file.

I'm feeling forced to close this in ~2 days otherwise.

voila · 2019-10-10T22:06:40Z

Do you want me to review as is or do you want to give the test-generator a try?

Can you review as is, please ?

NobbZ · 2019-10-11T05:34:30Z

Can you review as is, please ?

I'll do first comments through the day, though I'll be on a business trip for a few days then. Not sure if I will be able to continue review before Thursday, so you can take your time for doing the actual changes then.

juhlig · 2019-10-11T06:32:41Z

There are some tests in the suite like ..._can_sort_..., based on this set of tests in the canonical data.

In the canonical data, the property is called sortedData, but your implementation uses to_list. I think this may be misleading to students, as nothing really tells them that the result should be a sorted list. Your tests however, fix the order of the elements in the expected result.

You should either align the function name with the canonical data (which is what I would suggest/prefer), or accept any order of items (which would go against what is in the canonical data and thereby make implementing a test generator more difficult, so I don't recommend it).

@NobbZ thoughts?

NobbZ · 2019-10-11T06:48:19Z

As we have canonical data, I prefer to be as close as possible, and of course I also expect the corresponding version to be set in *.app.src and as a comment in the test suite.

I'll do a more in depth review later.

NobbZ · 2019-10-11T12:41:38Z

config.json

    }
  ]
-}
+}


Please add a newline here!

NobbZ · 2019-10-11T12:46:52Z

exercises/binary-search-tree/README.md

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


Sections "Running tests" and "Questions?" are missing, please use confliglet generate to generate the README.md.

NobbZ · 2019-10-11T12:48:15Z

exercises/binary-search-tree/src/binary_search_tree.app.src

@@ -0,0 +1,9 @@
+{application, binary_search_tree,
+  [{description, "exercism.io - binary-search-tree"},
+    {vsn, "0.0.1"},


Canonical data is at version 1.0.0.

NobbZ · 2019-10-11T12:48:28Z

exercises/binary-search-tree/src/binary_search_tree.app.src

+    {applications, [kernel,
+      stdlib]},
+    {env, []}
+  ]}.


Newline please.

NobbZ · 2019-10-11T12:48:38Z

exercises/binary-search-tree/src/binary_search_tree.erl

+insert(_BST, _Value) -> undefined.
+
+%% convert _BST to a sorted list
+to_list(_BST) -> undefined.


Newline please.

Also this should really be sorted_data/1 as in the problem specification.

NobbZ · 2019-10-11T12:57:45Z

exercises/binary-search-tree/test/binary_search_tree_tests.erl

+  T = binary_search_tree:insert(
+        binary_search_tree:insert(
+          binary_search_tree:empty(),
+        2),
+      1),


Could you please unnest function calls and use numbered variables? (see above)

NobbZ · 2019-10-11T12:57:51Z

exercises/binary-search-tree/test/binary_search_tree_tests.erl

+  T = binary_search_tree:insert(
+        binary_search_tree:insert(
+          binary_search_tree:empty(),
+        2),
+    2),


Could you please unnest function calls and use numbered variables? (see above)

NobbZ · 2019-10-11T12:57:56Z

exercises/binary-search-tree/test/binary_search_tree_tests.erl

+  T = binary_search_tree:insert(
+        binary_search_tree:insert(
+          binary_search_tree:empty(),
+        2),
+    3),


Could you please unnest function calls and use numbered variables? (see above)

NobbZ · 2019-10-11T12:58:03Z

exercises/binary-search-tree/test/binary_search_tree_tests.erl

+  T = binary_search_tree:insert(
+      binary_search_tree:insert(
+        binary_search_tree:insert(
+          binary_search_tree:insert(
+            binary_search_tree:insert(
+              binary_search_tree:insert(
+                binary_search_tree:empty(),
+              2),
+            1),
+          3),
+        6),
+      7),
+    5),


Could you please unnest function calls and use numbered variables? (see above)

NobbZ · 2019-10-11T13:03:54Z

exercises/binary-search-tree/test/binary_search_tree_tests.erl

+  ?assertMatch(4, binary_search_tree:value(T)),
+  ?assertMatch(2, binary_search_tree:value(binary_search_tree:left(T))),
+  ?assertMatch(1, binary_search_tree:value(
+    binary_search_tree:left(
+      binary_search_tree:left(T)))),
+  ?assertMatch(3, binary_search_tree:value(
+    binary_search_tree:right(
+      binary_search_tree:left(T)))),
+  ?assertMatch(6, binary_search_tree:value(binary_search_tree:right(T))),
+  ?assertMatch(5, binary_search_tree:value(
+    binary_search_tree:left(
+      binary_search_tree:right(T)))),
+  ?assertMatch(7, binary_search_tree:value(
+    binary_search_tree:right(
+      binary_search_tree:right(T)))).


I do not really like the look of the multi line matches, but aside from introducing further intermediate variables that destructure the tree again, I have no idea how to clean that up…

NobbZ

Sorry, I misclicked! Please consider all comments as request to change!

NobbZ · 2019-10-14T11:29:54Z

Looks good to me, @juhlig can you please take another look and merge, I am on mobile only and am a bit frightened that I've overlooked something.

juhlig · 2019-11-01T11:35:18Z

Sorry, I missed you mentioning my name ;) I think this one is good to go, as far as I can see.

implement exercise binary-search-tree

73ac5d1

juhlig suggested changes Oct 9, 2019

View reviewed changes

NobbZ requested changes Oct 9, 2019

View reviewed changes

implementation of binary-search-tree exercise

63c61b1

fix config.json and binary-search-tree tests

28905b8

voila force-pushed the binary-search-tree branch from 713ec86 to 28905b8 Compare October 10, 2019 22:12

voila and others added 2 commits October 11, 2019 11:14

Update config.json

432e2bd

fix config.json

5f2f4a3

NobbZ mentioned this pull request Oct 11, 2019

Adjust order of exercises #420

Closed

NobbZ approved these changes Oct 11, 2019

View reviewed changes

NobbZ requested changes Oct 11, 2019

View reviewed changes

juhlig mentioned this pull request Oct 11, 2019

Implementation of the nth-prime exercise #422

Merged

fix indentation in tests, and other minot fixes

d7e79b8

NobbZ approved these changes Oct 14, 2019

View reviewed changes

Base automatically changed from master to main January 28, 2021 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement exercise binary-search-tree #412

implement exercise binary-search-tree #412

voila commented Oct 2, 2019

voila commented Oct 2, 2019

NobbZ commented Oct 5, 2019

NobbZ commented Oct 8, 2019

juhlig Oct 9, 2019

NobbZ Oct 9, 2019

NobbZ left a comment

NobbZ commented Oct 10, 2019

voila commented Oct 10, 2019

NobbZ commented Oct 11, 2019

juhlig commented Oct 11, 2019

NobbZ commented Oct 11, 2019

NobbZ Oct 11, 2019

NobbZ Oct 11, 2019

NobbZ Oct 11, 2019

NobbZ Oct 11, 2019

NobbZ Oct 11, 2019

NobbZ Oct 11, 2019

NobbZ Oct 11, 2019

NobbZ Oct 11, 2019

NobbZ Oct 11, 2019

NobbZ Oct 11, 2019

NobbZ left a comment

NobbZ commented Oct 14, 2019

juhlig commented Nov 1, 2019

implement exercise binary-search-tree #412

Are you sure you want to change the base?

implement exercise binary-search-tree #412

Conversation

voila commented Oct 2, 2019

voila commented Oct 2, 2019

NobbZ commented Oct 5, 2019

NobbZ commented Oct 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NobbZ left a comment

Choose a reason for hiding this comment

NobbZ commented Oct 10, 2019

voila commented Oct 10, 2019

NobbZ commented Oct 11, 2019

juhlig commented Oct 11, 2019

NobbZ commented Oct 11, 2019

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

NobbZ left a comment

Choose a reason for hiding this comment

NobbZ commented Oct 14, 2019

juhlig commented Nov 1, 2019