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

feature: consider node data in remove method #27

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

Conversation

MartinLoeper
Copy link

@MartinLoeper MartinLoeper commented Oct 28, 2018

Some thoughts on node identity

I want to suggest a change which considers the value of node.data for the remove method.
In the following I want to explain why this change is significant for the project I'm currently working on.

Scenario

We at nesto-software use this library to model a timeline as it gives us great runtime performance.
We put events on the timeline which consist of a point-in-time which is a momentjs object and another object representing the event details.
The key of the node is the momentjs object. This fits in well because the comparator is straightforward.
The event details are set as data of the corresponding node.

Issue

There are multiple events with the same timestamp, thus I had to enable the duplicate keys feature for the avl tree. This works well.
However, if I want to remove a specific event, there is no way to specify which node to remove. I can only specify the timestamp. This results in an arbitrary event getting removed at the given timestamp.

Solution

I implemented a breadth first search in the remove method. The search is triggered once there is at least one node with the same key as the one to be removed, but a different node.data. The remove method, then searches the rest of the tree by visiting every child element and comparing its data attribute. This feature is triggered only if a second parameter is passed to the remove method. Thus, it does not affect runtime performance of existing code.

I know, there are some other methods, which do not support to specify a particular node.data to search for, e.g. the contains method.

The purpose of this pull request is to discuss the idea to integrate the missing logic which supports our use case into the official version of this library. What are your thoughts? I think that this library relies on the assumption that the identity of a particular node is represented by its key attribute. I think that we should take into account the node's data attribute as part of a node's identity. We could make this feature optional as proposed by the remove method's implementation in this PR. This would not affect the runtime performance of people which do not use the node's data attribute.

@MartinLoeper MartinLoeper mentioned this pull request Oct 29, 2018
@Ban44n
Copy link

Ban44n commented Jan 11, 2019

Very nice feature! Is there an estimation when this gets merged upstream? Thanks

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