deno_std icon indicating copy to clipboard operation
deno_std copied to clipboard

feat: add callback functionality for binary search tree

Open dquixote-jr opened this issue 6 months ago • 7 comments

I need the functionality to find the k'th node in the tree, which isn't possible with current setup This augmentation allows people to define whatever functions that they want, in this particular case, it allows me to store the subtree size in a parameter st_size and which I can use to find the k'th node in order.

First javascript project PR so this is gonna have some issues, happy to solve them.

dquixote-jr avatar Jun 20 '25 22:06 dquixote-jr

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Jun 20 '25 22:06 CLAassistant

I need the functionality to find the k'th node in the tree, which isn't possible with current setup

This change doesn't seem a good idea for adding that capability to this class. Currently BinarySearchNode class is private. This makes it a part of public API. That doesn't seem an ideal change. Can we achieve this without exposing node class public?

For small k's, it seems we can use lnrValues (or rnlValues) iterator for finding k'th node. What do your tree size and k value typically look like?

kt3k avatar Jun 23 '25 03:06 kt3k

For small k's, it seems we can use lnrValues (or rnlValues) iterator for finding k'th node. What do your tree size and k value typically look like?

The tree can be large, like ~5K. And k can be as large as O(n), so iterating over values with lnr/rnl isn't really a good idea.

This change doesn't seem a good idea for adding that capability to this class. Currently BinarySearchNode class is private.

I don't think it's the worst thing to make the BinarySearchNode a public class, as we have already made it a "binary" tree, so having left/right/parent elements is expected of it. A different thing to do would be to expose the BinarySearchNode class, but not give access directly to member objects but rather have setChild/getChild or set/get Left/Right/Parent kind of functions.
I'm open to any other ideas too, my main use case is to allow people to travel over the tree via nodes somehow via left/right nodes. I don't need them to be able to access the specific internal attributes of the class, just left/right children.

dquixote-jr avatar Jun 23 '25 04:06 dquixote-jr

Codecov Report

Attention: Patch coverage is 91.66667% with 6 lines in your changes missing coverage. Please review.

Project coverage is 93.70%. Comparing base (2258bf2) to head (a2fd74c).

Files with missing lines Patch % Lines
data_structures/binary_search_tree.ts 91.17% 2 Missing and 1 partial :warning:
data_structures/red_black_tree.ts 66.66% 2 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6730      +/-   ##
==========================================
- Coverage   94.65%   93.70%   -0.95%     
==========================================
  Files         576      559      -17     
  Lines       47065    46838     -227     
  Branches     6610     6472     -138     
==========================================
- Hits        44549    43890     -659     
- Misses       2473     2902     +429     
- Partials       43       46       +3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jun 23 '25 04:06 codecov[bot]

I don't think it's the worst thing to make the BinarySearchNode a public class, as we have already made it a "binary" tree, so having left/right/parent elements is expected of it. A different thing to do would be to expose the BinarySearchNode class, but not give access directly to member objects but rather have setChild/getChild or set/get Left/Right/Parent kind of functions.

Then maybe it's better to define a new interface which only has left, right, and parent (and maybe value) fields, and use it as input type for callback?

Also run deno task lint and see the output. We need to fix them to merge this to main

kt3k avatar Jun 23 '25 04:06 kt3k

Then maybe it's better to define a new interface which only has left, right, and parent (and maybe value) fields, and use it as input type for callback?

That sounds like a good idea. I've added a new class BSTNode and refactored the setup to use that.

Also run deno task lint and see the output. We need to fix them to merge this to main

Thank you for this. I've got it passing locally. import type gave me a headspin but I got it working in the end :)

dquixote-jr avatar Jun 23 '25 05:06 dquixote-jr

Any holdups here?

dquixote-jr avatar Aug 09 '25 16:08 dquixote-jr