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

Binary search tree on JS #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Binary search tree on JS #12

wants to merge 1 commit into from

Conversation

SvetlanaReutskaya
Copy link

No description provided.

@GYFK GYFK requested review from aqrln and belochub June 6, 2017 21:51
Copy link

@aqrln aqrln left a comment

Choose a reason for hiding this comment

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

Mostly looks good, awesome work. I left a few comments though.

if (this.leftChild !== null) {
this.leftChild.output();
}
console.dir(this.data);
Copy link

Choose a reason for hiding this comment

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

I think it should be console.log() instead of console.dir(). This works too, yeah, but console.log() would be more idiomatic.

current = current.leftChild;
}
return current;
} else return null;
Copy link

Choose a reason for hiding this comment

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

I'd find it more readable to use braces in the else clause when you use them in the if clause generally. In this specific case you don't need else at all though.

Copy link

Choose a reason for hiding this comment

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

It may also be worth to invert the condition and return null early to reduce the level of indentation of the following block of code.

current = current.rightChild;
}
return current;
} else return null;
Copy link

Choose a reason for hiding this comment

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

Ditto.

BinarySearchTree.prototype.insert = function(data) {
if (this.root !== null) {
this.root.insert(data);
} else this.root = new Node(null, data);
Copy link

Choose a reason for hiding this comment

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

Please use braces here.

console.log('Search node 13: ' + tree.search(13) + ': ' + tree.search(13).data);
console.log('Search node 11: ' + tree.search(11));

//tree.delete(15);
Copy link

Choose a reason for hiding this comment

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

Is it intentional that these lines are commented out?

while (current.leftChild !== null) {
current = current.leftChild;
}
return current;
Copy link

Choose a reason for hiding this comment

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

I believe returning current.data makes more sense both here and in BinarySearchTree.prototype.max(). Node instances are an implementation detail, let's encapsulate them away.

@belochub belochub removed their request for review December 6, 2018 09:48
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