Binary search tree encapsulated

[michalporeba]

As explained and approved on the forum

This is to match with both similar exercise in this and other tracks, as well as with the .net approach to doing things. Binary Search Tree is now an encapsulated structure. To solve the problem a node has to be implemented internally.

The idea is similar to what we have previously implemented in Simple Linked List exercise as discussed on the forum and described in previous PR.

Since the BST structure is a search one, I removed the IEnumerable elements and instead added a constraint on the T to be IComparable. The BinarySearchTree<T> type to implement two properties Count and Depth and two methods Add and Contains.

Two remaining questions is

1. Whether to leave the where T: IComparable in the stub or not. As this is a medium difficulty exercise I would be tempted to remove it. Users implementing the solution soon will realise that they cannot compare T and either they will know about IComparable, or will search for solutions online and find out about it.

2. Whether to add a test that explicitly follows the example from the description.

[github-actions]

Hello. Thanks for opening a PR on Exercism. We are currently in a phase of our journey where we have paused community contributions to allow us to take a breather and redesign our community model. You can learn more in this blog post. As such, all issues and PRs in this repository are being automatically closed.

That doesn't mean we're not interested in your ideas, or that if you're stuck on something we don't want to help. The best place to discuss things is with our community on the Exercism Community Forum. You can use this link to copy this into a new topic there.

---

Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

[ErikSchierboom]

The tests file has understandably changed lots. We have to decide whether we want to diverge from the original prob-specs data: https://github.com/exercism/problem-specifications/blob/main/exercises/binary-search-tree/canonical-data.json

[ErikSchierboom]

@michalporeba I sort of seem to have lost the status of this PR. Do you have any idea? I have some comments that still seem to be unresolved.

[michalporeba]

Let's see if this will work (replying by email). I'm on holidays right now with no computer and a promise to myself not to look at code. I'll have a look and respond properly when I get back in a week or so.

…

On Wed, Aug 9, 2023, 12:09 Erik Schierboom ***@***.***> wrote: @michalporeba <https://github.com/michalporeba> I sort of seem to have lost the status of this PR. Do you have any idea? I have some comments that still seem to be unresolved. — Reply to this email directly, view it on GitHub <#2131 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA5QMFIFWZXP3YIGUAXGJDXUNVX7ANCNFSM6AAAAAAX3HKQ6U> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[ErikSchierboom]

Enjoy your vacation

[michalporeba]

@ErikSchierboom, I was going over the code of the exercises again today and I have noticed something else. The comments above I made after reviewing ten practice exercises. Today I looked at the concept exercises and there is a difference.

Concept exercises use messages like this: "Please implement the RemoteControlCar.SetSponsors() method" while practice exercises use "Please implement this function".

This is a practice exercise so, for consistency, we should use "this function". But, shouldn't we have consistency between the two types of exercises? Which way we should take it?

[ErikSchierboom]

This is a practice exercise so, for consistency, we should use "this function". But, shouldn't we have consistency between the two types of exercises? Which way we should take it?

The concept exercises version is superior here, mostly for online editor users as they get a more specific error messages. So we should update the practice exercise's version.

[michalporeba]

@ErikSchierboom,I had another look at what I initially proposed, our discussion here, but also the problem specification and similar solutions in a few other object-oriented language tracks. I'll summarise what I found and my current thinking here so we can decide on the direction and complete this work.

C++
Doesn't implement methods. The constructor generates a tree from an array parameter.
The tests are done either through an iterator or by accessing the nodes (e.g. tested->left()->right())

test_sort(make_tree<uint32_t>({2, 1, 3, 6, 7, 5}), {1, 2, 3, 5, 6, 7});

auto tested = make_tree<uint32_t>({4, 2, 6, 1, 3, 5, 7});
test_leaf<uint32_t>(tested, 4, true, true);
test_leaf<uint32_t>(tested->right(), 6, true, true);
test_leaf<uint32_t>(tested->left()->left(), 1, false, false);

The tree is tested with numbers (canonical) and with strings.

Crystal

Ruby implements constructor for initialisation from an array, and the standard insert method. The class is generic, but the structure is not encapsulated.

tree = Node.new(4)
tree.insert(5)
tree.value.should eq(4)
right = tree.right.not_nil!
right.value.should eq(5)

Ruby

Ruby is also implemented with limited encapsulation and access to the full tree. The implementation is non-generic, but Ruby is a dynamic language. It has insert for the method name.

four = Bst.new 4
four.insert 2
assert_equal 4, four.data
assert_equal 2, four.left.data

A summary of where I am at right now:

• I still think that asking students to implement an encapsulated structure is much more in line with what C# is about, how .net does things.
• Using .net names like Add rather than insert from the specification makes the exercise more C#-y and idiomatic.
• Asking for a generic implementation does so too.
• While the above diverges from the problem specification, it increases the educational value of the exercise and Exercism in general, in my opinion.
• I would like to add string tests to demonstrate that the structure is generic as is done in the C++ track.

Testing is the challenge.

• The problem specification has tests for sorting behaviour and for values at specific places in the structure.
• I think we should diverge and maintain the test cases manually rather than through generators, but we should find a way to replicate all the standard tests - the same data and the same (or very close) functionality.
• And so, I must conclude that the current approach of testing in this PR is not the solution we need. Testing depth and length, and whether a given number is present is not the right proxy for testing the structure. I no longer like this, as it adds two properties and doesn't directly test what's needed. I was trying to make it more difficult to implement by simply putting elements into a list and sorting them.
• Instead, I think, we should have an Inspect method. It could generate the data structure representation, ask to check if a node exists, or address by path.

Ideas for the Inspect

1. We could ask for the Inspect() to generate a data structure as defined in the problem specification.
   Something like this:

var bst = new BinarySearchTree({4, 5, 3, 1});
var actual = bst.Inspect();

producing a structure like this

{ "data": 4,
   "left": {
       "data": 3,
       "left": { "data": 1 }
   },
  "right": { "data": 5" }
}

or using nested arrays

[ 4, [3, [1, null, null], null], [5, null, null]]

Or perhaps, by returning enumeration in order of depth-first traversal

[4, 3, 1, 5]

2. We could allow the Inspect(path) to take directions in the form of a string like "LRLL" where L moves left and R moves right, eventually returning the value stored at that node. This would be similar to the tests done with exposed structures while being a bit more object-oriented.

var bst = new BinarySearchTree({4, 5, 3, 1});
Assert.Equal(4, bst.Inspect(""));
Assert.Equal(1, bst.Inspect("LL"));

3. However, to make it even more object oriented, we shouldn't be asking for an internal value. We could test like so.

var bst = new BinarySearchTree({4, 5, 3, 1});
Assert.True(bst.CheckValueAtNode("", 4));
Assert.True(bst.CheckValueAtNode("LL", 1));

Please let me know you think when you have a moment.

[ErikSchierboom]

I still think that asking students to implement an encapsulated structure is much more in line with what C# is about, how .net does things.
Using .net names like Add rather than insert from the specification makes the exercise more C#-y and idiomatic.
Asking for a generic implementation does so too.
While the above diverges from the problem specification, it increases the educational value of the exercise and Exercism in general, in my opinion.
I would like to add string tests to demonstrate that the structure is generic as is done in the C++ track.

Those all sounds reasonable

However, to make it even more object oriented, we shouldn't be asking for an internal value. We could test like so.

Not sure how this is more object-oriented than the previous suggestion, as the goal of the binary search tree is to contain and expose values, not to hide them.

My personal preference would be the Inspect option, although I think it should be called Serialize or ToJson. That sounds like a really neat option to expose the structure and values whilst still allowing the student the freedom to be somewhat free on how to implement this internally. It also makes sense as a "requirement", as serialization is used a ton in the real world. What do you think about that?

[michalporeba]

However, to make it even more object oriented, we shouldn't be asking for an internal value. We could test like so.

Not sure how this is more object-oriented than the previous suggestion, as the goal of the binary search tree is to contain and expose values, not to hide them.

OOP is all bout encapsulation and message passing, not exposing internals making it possible to change implementation of a data structure. That's why we don't have access to the internals of IList<> or IDictionary<> implementations.

But I think the serialisation idea is perfect. While the implementation is internal, the data can be serialised. ToJson() is, in my experience` the more common name in the .net world. I'll have a go at implementing this.

[ErikSchierboom]

OOP is all bout encapsulation and message passing, not exposing internals making it possible to change implementation of a data structure. That's why we don't have access to the internals of IList<> or IDictionary<> implementations.

True, although a binary search tree should be accessible via left/right like methods. 🤷 I mean, the fact that it is a binary tree implies a certain structure, so IMHO it's fine to expose that, as long as the actual internal implementation is not exposed.

But I think the serialisation idea is perfect. While the implementation is internal, the data can be serialised. ToJson() is, in my experience` the more common name in the .net world. I'll have a go at implementing this.

Yeah let's do this, I like this best! Thanks for wanting to implement this.

[michalporeba]

@ErikSchierboom have a look at the current version when you have a moment.
I was able to use all the standard tests using ToJson for testing.

For ordering checks I have used a custom method GetOrderedValues() for now. I'm in two minds about implementing the IEnumerable<T>. I think this would make the exercise even more idiomatic in .net, but it would make it even more complex.

Should we do it?

[ErikSchierboom]

For ordering checks I have used a custom method GetOrderedValues() for now. I'm in two minds about implementing the IEnumerable. I think this would make the exercise even more idiomatic in .net, but it would make it even more complex.

We are doing that in two other places too (dot-dsl and simple-linked-list), so I think it's fine to have people implement IEnumerable<T> (maybe an .docs/instructions.append.md file could help).

[ErikSchierboom]

This style is more commonly used in the code base:

Suggested change

	public int Count
	=> throw new NotImplementedException("Please implement the Count property of the BinarySearchTree<T> class.");
	public int Count =>
	throw new NotImplementedException("Please implement the Count property of the BinarySearchTree<T> class.");

[ErikSchierboom]

The binary search tree is generic, but we're only using int as the type. I think we should also add some non-int type (probably strings).

[michalporeba]

Yes, I proposed it myself higher up in the comments, so definitely agree. But I wanted first to get the shape of the exercise in the right format before I will add more tests and details like that. It's definitely coming.