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

added splaytree #32

Merged
merged 9 commits into from
May 30, 2019
Merged

added splaytree #32

merged 9 commits into from
May 30, 2019

Conversation

ashiqursuperfly
Copy link
Contributor

-Java Implementation Of SplayTree ( https://en.wikipedia.org/wiki/Splay_tree#Operations )

@williamfiset
Copy link
Owner

williamfiset commented May 24, 2019

Hi Ashiqur, thanks for the PR!

Unfortunately, I don't have time to review it in detail rn, but I should be able to early next week after memorial day holiday.

I have some high level things you can address in the meantime:

  1. If you implement the PrintableNode interface you should be able to spare yourself the trouble of writing the display tree logic :) See the interface here [1] and how it's used here [2]

  2. A splay tree is a form of search tree so this code should be moved to either the binarysearchtree or the balancedtree folder. The binarysearchtree folder seems more appropriate, wdyt?

  3. Thanks for adding tests! Can you add randomized test(s) that combines insert, delete and findMax vs say the builtin PriorityQueue impl?

[1] https://github.com/williamfiset/data-structures/blob/master/com/williamfiset/datastructures/utils/TreePrinter.java
[2] https://github.com/williamfiset/data-structures/blob/master/com/williamfiset/datastructures/balancedtree/AVLTreeRecursive.java

Added Randmized tests for SplayTree
Moved SplayTree to BST package
Updated build.gradle to support JUnit 5 🔨
Added Randomized Tests for SplayTree
Made Changes to build.gradle to support JUnit 5 🔨
@ashiqursuperfly
Copy link
Contributor Author

Hi William, thanks for your review.
-Sorry i didnt check you already had a decent way to print a Tree earlier. I've used your PrintableNode Interface to print the tree in my latest commit.
-And yes i also do think its more convenient to keep the splaytree inside the bst folder.Although, since its a self balancing tree as well i think it can be kept inside the balancedtree too,so its your call :3
-I've added some randomized tests for the splaytree operations.
-About preparing the comparison tests with the Java Collections PriorityQueue, i wrote one comparison test named splaytree1() and priorityQ1() . These tests combine insertion,deletion and searching. But,im not sure how to combine findMax().Since, priorityQueue with a custom comparator to act like a MaxHeap would always be able to findMax in O(1) but all splay tree operations would require Amortized O(logn). So, findMax on splayTree would be slower . However all the other operations(insert,delete,search) seem to be faster with the splayTree by about 1-2ms according to the test results.
image
image

Note : im using JUnit 5 for the tests, so i had to add a few dependencies on the build.gradle file. I had an issue about this with the Travis CI in the previous two commits but i think its resolved right now.Do give it a look if u can.

@williamfiset
Copy link
Owner

Thanks for the initial changes! Once you address my comments I think this PR will be ready to merge. It's already in pretty good shape.





Copy link
Owner

Choose a reason for hiding this comment

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

Yes sg. Please sync and remove the diff would be great.

@williamfiset
Copy link
Owner

Thanks for the changes! Don't forget to sync to head and resolve the merge conflict, but otherwise looks pretty good.

@ashiqursuperfly
Copy link
Contributor Author

I think I've made the changes you requested. Take a look at the CI check fail,its probably because the link provided in the readme for splaytree doesnt exist yet.Since i provided the link as it would look like in your repo. What should i do? Should i change the link to my repo ?
Link i provided : (https://github.com/williamfiset/data-structures/blob/master/com/williamfiset/datastructures/binarysearchtree/SplayTree.java
)

@williamfiset williamfiset merged commit 329f22d into williamfiset:master May 30, 2019
@williamfiset
Copy link
Owner

I checked the CI tests and only having the failing link is fine. Merged the PR into master, 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