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

Two small feature requests #1

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

maxtaco
Copy link

@maxtaco maxtaco commented Jun 6, 2013

Hi, Thanks for this package. I have two small feature requests:

  1. A procedural-style in-order traversal: zoom to the minimum element of the tree, and then call next() iteratively to traverse the tree in-order. You can stop whenever you'd like (which isn't the case for the existing forEach). Also, unlike the existing forEach, the implementation is iterative and not recursive.
  2. Delete an node by its object representation, and not its value. This is especially useful if you have multiple objects with the same key. A very small refactor is required for this --- to split delete into two small functions, and also the return newNode from insert.

Test code included for these newfeatures. Thanks.

…ersal, with minimum and next(), and also, the ability to delete a node if you have the node in your hand (and not by key).
…s to return the new internal node, and we can squirrel it away as we see fit outside of the Tree class.
@maxtaco
Copy link
Author

maxtaco commented Jun 6, 2013

Ah shoot, I forgot that pushes to my branch still show up in yours. I definitely didn't mean for that last commit (the change to package.json) to get to your repo. Sorry about that.

@scttnlsn
Copy link
Owner

scttnlsn commented Jun 6, 2013

Awesome...love the .next() style iteration! I'm a little confused why delete_node is necessary though. How/why would you have multiple objects with the same key?

@maxtaco
Copy link
Author

maxtaco commented Jun 6, 2013

For example a list of events keyed by completion time, like how node
implements setTimeout. On the one hand you need to be able to insert new
events arbitrarily into the tree since the user specifies when the event is
inserted and what its completion time will be. On the other hand you need
to be able to iterate from the lowest key upward at every time step as you
handle all of the events that have completed. There could be two events
that complete at the same time.

The example code might look like:

var x = events.minimum();
var y;
while (x && x.key < Date.now()) {
   y = x.next();
   tree.delete_node(x);
   // do something with x.value
   x = y;
}

Thanks!
On Jun 5, 2013 11:19 PM, "Scott Nelson" notifications@github.com wrote:

Awesome...love the .next() style iteration! I'm a little confused why
delete_node is necessary though. How/why would you have multiple objects
with the same key?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-19022769
.

@scttnlsn
Copy link
Owner

scttnlsn commented Jun 6, 2013

As is, I don't think duplicate keys are allowed. In the use case above couldn't the values just be an array of events that complete at the same time (specified by the key)?

By default this option is off, to maintain backwards comptability.
Provide some test cases to test both modes of operation
Also, while we're at it, clean up the tests a bit, and assert balance in
a bunch of places we weren't doing so before.
@maxtaco
Copy link
Author

maxtaco commented Jun 6, 2013

Good point about insert clobbering repeated keys, I didn't see that. There's now an option to the Tree() constructor to enable one mode of operation or the other, with the default behavior as before.

If you want to store duplicate keys as an array of values, is the onus then on the user to make that happen? I.e, every insert then becomes a lookup and then an insert if the lookup fails? Doable, but it does double the number of traversals.

There's also something a little bit unsettling when it comes to depending on strict equality of keys, especially for floats that can show rounding error. For hash tables, I agree, this isn't a concern, but for red-black trees, which are useful for storing values keyed by continuous keys, it's a common case. Why bother worrying about equality if you don't need to?

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.

None yet

2 participants