Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement binary-search-tree exercise #370

Merged
merged 5 commits into from
Oct 17, 2019

Conversation

voila
Copy link
Contributor

@voila voila commented Oct 4, 2019

No description provided.

Copy link
Contributor

@sshine sshine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work!

What you have provided appears mergeable without significant modification.

We have a test generator for auto-generating the unit tests, but it has had an overhaul recently that doesn't appear to be documented yet. If you would like, you can see e.g. 37d3e38 (in particular the files affected in test-generator/) for how to extend the generator for a specific test. Since many results in this exercise are trees, this would involve deriving a tree from a JSON object structure, so the work isn't trivial. This part is entirely optional and could also be made as a separate, subsequent PR.

Comment on lines +24 to +26
let rec to_list = function
| Leaf -> []
| Node(l, v, r) -> to_list(l) @ [v] @ to_list(r)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One might consider a tail-recursive variant to avoid stack overflows.

This is also fine, and is what I expect most students would submit.

@sshine sshine requested a review from marionebl October 4, 2019 07:07
Copy link
Contributor

@sshine sshine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that the canonical version for binary-search-tree is 1.0.0, not 1.3.0.

For future self-reference: Include exercism/problem-specifications#940 when squash-merging.

@sshine sshine merged commit 7cbe850 into exercism:master Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants