Skip to content

Conversation

bmulvihill
Copy link
Contributor

@bmulvihill bmulvihill commented Dec 20, 2016

Adds binary-search-tree exercise

It currently tests the insert, search, traverse(each), and delete methods.

Most implementations I have seen in exercism only test insert and traverse, but I think if you are going to implement a Binary Search Tree I feel you should implement the common operations.

I also added tests for additional Crystal features that could be leveraged i.e. making the BST an Enumerable and Iterable (which makes it far more flexible, and allows the programmer to dive a little deeper into the language). I put comments above them, but maybe I could make it more obvious that they are not required tests. What do you think @mhelmetag?

Copy link
Contributor

@mhelmetag mhelmetag left a comment

Choose a reason for hiding this comment

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

I also added tests for additional Crystal features that could be leveraged i.e. making the BST an Enumerable and Iterable (which makes it far more flexible, and allows the programmer to dive a little deeper into the language).

I think this idea is awesome. Tiered challenges are always great (at least in my eyes).

I put comments above them, but maybe I could make it more obvious that they are not required tests.

IIRC there's a specific structure for files that pertain to extra info for an exercise. You could probably put an explanation in that (I'll look around for that).

Otherwise this looks pretty awesome. The only thing that comes to mind is well... keeping in mind that there may be a common json file (in the future; I know it currently doesn't exist) for this to generate off of and it would be cool to keep thing more easily generated... but... at the same time that's not super high priority now.

@bmulvihill
Copy link
Contributor Author

@mhelmetag Cool, I will move the language specific tests into their own describe block to make it more obvious they are not directly related to implementing a BST.

As far as the JSON file is concerned - I think we could easily revisit that after this goes in.

@mhelmetag
Copy link
Contributor

Cool, I will move the language specific tests into their own describe block to make it more obvious they are not directly related to implementing a BST.

Awesome! That'll work.

As far as the JSON file is concerned - I think we could easily revisit that after this goes in.

Yep! 👍

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