You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The use of method chaining forces TypeScript to demand a return type of BinarySearchTree on both the left and right - which is nonsense as it is perfectly normal for there to be noBinarySearchTree on either the left or right or both - the correct type should be BinarySearchTree | undefined.
But I think the core of the problem is that a binary search tree is usually used as an internal structure - so left and right would never be publicly exposed anyway.
So as a compromise it may make sense to have an export method that exports a simplified representation of the internal structure for testing purposes (so for the purpose of this exercise we are testing the implementation - not just the publicly exposed interface).
Then the test spec could look something like this:
// https://github.com/exercism/problem-specifications/blob/master/exercises/binary-search-tree/canonical-data.jsonimportBinarySearchTreefrom'./binary-search-tree';describe('BinarySearchTree',()=>{it('should retain data',()=>{constbst=newBinarySearchTree();bst.insert(4);expect(bst.export()).toEqual([undefined,4,undefined]);});describe('insert data at proper node',()=>{it('smaller number at left node',()=>{constfour=newBinarySearchTree([4,2]);expect(four.export()).toEqual([2,4,undefined]);});it('should insert the same number to the left',()=>{constfour=newBinarySearchTree([4,4]);expect(four.export()).toEqual([4,4,undefined]);});it('should insert a greater number to the right',()=>{constfour=newBinarySearchTree([4,5]);expect(four.export()).toEqual([undefined,4,5]);});it('should deal with a complex tree',()=>{constfour=newBinarySearchTree([4,2,6,1,3,7,5]);expect(four.export()).toEqual([[1,2,3],4,[5,6,7]]);});});describe('can sort data',()=>{it('can sort single number',()=>{expect(Array.from(newBinarySearchTree([2]))).toEqual([2]);});it('can sort if second number is smaller than first',()=>{expect(Array.from(newBinarySearchTree([2,1]))).toEqual([1,2]);});it('can sort if second number is same as first"',()=>{expect(Array.from(newBinarySearchTree([2,2]))).toEqual([2,2]);});it('can sort if second number is greater than first"',()=>{expect(Array.from(newBinarySearchTree([2,3]))).toEqual([2,3]);});it('can sort complex tree',()=>{expect(Array.from(newBinarySearchTree([2,1,3,6,7,5]))).toEqual([1,2,3,5,6,7,]);});});});
I've mentioned before (on slack) that what you're doing now was one of my future goals. These exercises were ported when TS was waaaay younger and the porters were less experienced. So some of this is legacy, and some of it is just flat out wrong.
That said.
Now perhaps the test could be salvaged by using the optional chaining operator.
I think it should use the non-null assertion operator in the tests, to match the intent (e.g. returning undefined! from the left function right now would behave the same as left! in the tests.
But I think the core of the problem is that a binary search tree is usually used as an internal structure - so left and right would never be publicly exposed anyway.
Meh. It depends™. I agree that sometimes this is true, and sometimes it is not. There are plenty of libraries where left and right are exposed and meant to be public and publicly accessed.
the correct type should be BinarySearchTree | undefined.
💯 % agreed. This is the change I'd accept without comment.
The example implementation sidesteps the issue by using the non-null assertion operator:
This is just bad design. It should not have done this. I like your new implementation way better.
So as a compromise it may make sense to have an export method that exports a simplified representation of the internal structure for testing purposes (so for the purpose of this exercise we are testing the implementation - not just the publicly exposed interface)
You know how I love testing outcome and not implementation. I'm 50/50 on this. I like what you're proposing, but I want to ask a few other tracks their opinion.
If willing, could you do a PR that changes the return type, but not hide left and right for now (and change the example to be nice and correct), and then we can discuss removing left and right from the public API later?
In particular:
typescript/exercises/binary-search-tree/binary-search-tree.test.ts
Lines 48 to 53 in 1af726c
The use of method chaining forces TypeScript to demand a return type of
BinarySearchTree
on both theleft
andright
- which is nonsense as it is perfectly normal for there to be noBinarySearchTree
on either theleft
orright
or both - the correct type should beBinarySearchTree | undefined
.The example implementation sidesteps the issue by using the non-null assertion operator:
typescript/exercises/binary-search-tree/binary-search-tree.example.ts
Lines 30 to 36 in 1af726c
which in this context is inappropriate because it is entirely reasonable in this section of code for
this._left_
orthis._right
_to beundefined
.Now perhaps the test could be salvaged by using the optional chaining operator.
But I think the core of the problem is that a binary search tree is usually used as an
internal
structure - soleft
andright
would never be publicly exposed anyway.So as a compromise it may make sense to have an
export
method that exports a simplified representation of the internal structure for testing purposes (so for the purpose of this exercise we are testing the implementation - not just the publicly exposed interface).Then the test spec could look something like this:
with an example implementation like
Also note that I replaced the former
each
method with an implementation of the standardized iterable protocol on theBinarySearchTree
class.The text was updated successfully, but these errors were encountered: