AVL tree #1

gabi-250 · 2017-10-24T00:59:12Z

Contains the initial implementation of the search tree.

ltratt · 2017-10-25T21:15:31Z

src/lib.rs

+    /// assert!(tree.contains(&String::from("world!")));
+    /// ```
+    pub fn insert(&mut self, value: T) {
+        if let Ok(node) = self.create_node(value) {


If create_node returns an Err, is the insert swallowed or ...?

create_node returns an Err if value can already be found in the tree. So if Err is returned, insert won't do anything.

Ah, that's a weird return type to denote a non-bad thing :) Why not have create_node just return a bool (e.g. true means "this node already existed")?

ltratt · 2017-10-25T21:16:45Z

src/lib.rs

+
+    fn create_node(&mut self, value: T) -> Result<Node<T>, usize> {
+        match self.get_node(&value) {
+            Ok(index) => {


This can just be Ok(index) => Err(index) (without the curly brackets).

ltratt · 2017-10-25T21:20:19Z

src/lib.rs

+        std::cmp::max(left_height, right_height)
+    }
+
+    fn balance_factor(&self, node: &Node<T>) -> i8 {


Why does this return a signed int?

The balance factor can be negative. In my code the balance factor is defined as height(left_child) - height(right_child). If it is negative and less than -2, the tree is 'right-heavy'. If it's positive and greater than 2, the tree is 'right-heavy'

Aha, got it. But can the balance exceed +/- 128?

It should always be in the range [-2, 2]. If at some point it becomes +/- 2, the tree is (should be) rebalanced

Understood... but how far can it exceed +/- 2 before it's rebalanced?

In an AVL tree, the heights of the two child subtrees of any node differ by at most one
(wikipedia)

The 2 and -2 cases trigger rebalancing. You can't have < -2 or > 2 because you only ever insert/delete one node at a time.

Aha, got it.

ltratt · 2017-10-25T21:20:38Z

src/lib.rs

+                                    self.nodes[right_index].right_child);
+            self.nodes[right_index].parent = self.nodes[index].parent;
+            self.nodes[index].parent = Some(right_index);
+            //self.update_heights(right_index);


Remove commented code.

ltratt · 2017-10-25T21:26:05Z

Some relatively minor comments from me. Have you run clippy over this? https://github.com/rust-lang-nursery/rust-clippy

gabi-250 · 2017-10-25T21:27:45Z

@ltratt Thank you for the review! I'll have a look at clippy

vext01 · 2017-10-25T23:35:24Z

src/lib.rs

+    fn get_node(&self, value: &T) -> Result<usize, Option<usize>> {
+        let mut current_index = self.root;
+        let mut previous_index = None;
+        if !self.nodes.is_empty() {


I'm not sure this check is needed. In the case of the tree being empty, root would be None, and thus he while loop would not be entered, right?

Good point!

vext01 · 2017-10-25T23:36:36Z

src/lib.rs

+        }
+    }
+
+    fn get_node(&self, value: &T) -> Result<usize, Option<usize>> {


Can we add comment indicating what the error state communicates?

vext01 · 2017-10-25T23:38:21Z

src/lib.rs

+    }
+
+    fn update_heights(&mut self, start_index: usize) {
+        if !self.nodes.is_empty() {


Similarly, is this needed?

It is, since I call self.root.unwrap() at 148:

self.nodes[self.root.unwrap()].height = self.compute_height(self.nodes[self.root.unwrap()].left_child, self.nodes[self.root.unwrap()].right_child);

I should probably store the root index though, instead of unwrapping it everywhere

vext01 · 2017-10-25T23:45:05Z

src/lib.rs

+    }
+
+    fn left_rotation(&mut self, index: usize) {
+        if let Some(right_index) = self.nodes[index].right_child {


What does it mean if you don't enter the body of this conditional? i.e. if right_index is None?

Perhaps comment this section of code up more?

In this case, the right child of index becomes the parent of index and the left child of right_index becomes the right child of index. However, right_index might not have any children, which is why this condition is necessary.

vext01 · 2017-10-25T23:47:29Z

src/lib.rs

+                self.left_right_rotation(index);
+            }
+        } else if balance_factor < -2 || balance_factor > 2 {
+            panic!("Invalid balance factor for {}: {}", index, balance_factor);


^ Ah, this confirms my above comment about the -2 <= height <= 2 thing. Is unreachable!() more appropriate here? Not sure.

vext01 · 2017-10-25T23:51:09Z

src/lib.rs

+
+    fn is_balanced(&self, index: usize) -> bool {
+        let balance_factor = self.balance_factor(&self.nodes[index]);
+        balance_factor >= -1 && balance_factor <= 1


Can you chain operators in Rust?

return -1 <= self.balance_factor(&self.nodes[index]) <= 1

If you can't you could do:

return abs(self.balance_factor(&self.nodes[index])) < 2

It seems to be an open issue, I'll just use abs

vext01 · 2017-10-25T23:53:48Z

src/lib.rs

+    /// assert!(tree.iter().zip(vec![1, 2, 5]).all(|(x, y)| *x == y));
+    /// ```
+    pub fn iter(&'a self) -> SearchTreeIter<'a, T> {
+        let mut start_node = None;


I feel like this initialisation code should be encapsulated inside the iterator's ::new()

It just walks down the left side of the tree to find the first node that should fall out of the iterator, right?

Hm, I didn't implement it like that because it felt a bit awkward to pass both the root node and the tree Vec to SearchTreeIter::new, but I guess it makes makes sense to do it that way

vext01 · 2017-10-25T23:59:57Z

src/lib.rs

+                if let Some(right_child) = node.right_child {
+                    self.current_node = Some(right_child);
+                    node = &self.nodes[right_child];
+                    while let Some(left_child) = node.left_child {


There seems to be a recurring pattern.

Lots of code wants to find the deepest leftmost node in a given sub-tree. Should that be a function?

vext01 · 2017-10-26T00:00:54Z

src/lib.rs

+                assert!(tree.is_balanced(node_index));
+            }
+        }
+        {


Is this extra scoping needed to silence the borrow checker?

Ah, it used to be needed with my older implementation of the test. It's no longer necessary, I'll remove it

vext01 · 2017-10-26T00:03:46Z

This looks pretty good gabi!

vext01 · 2017-10-26T00:06:29Z

All tests passing for me :)

gabi-250 · 2017-10-26T17:35:46Z

@vext01 Thank you for your review! I've addressed your comments, let me know if there's anything else I need to change

vext01 · 2017-10-26T18:37:24Z

src/lib.rs

+            // left child of `right_index` must become the new right child of
+            // `index`.
+            //
+            //       A (index)          (right_index) B


I really like these ascii art diagrams :)

Hah, thanks! Explaining the balancing algorithm would've been a lot harder without drawing

vext01 · 2017-10-26T18:38:38Z

This looks good to me.

If @ltratt is OK too, then we can squash.

ltratt · 2017-10-26T18:55:14Z

Fine with me.

gabi-250 · 2017-10-26T19:06:43Z

All done

ltratt · 2017-10-26T19:13:35Z

Is insert still returning Err? We really should fix that.

gabi-250 · 2017-10-26T19:19:59Z

@ltratt insert is not returning anything currently. create_node returns a Result, which is unnecessary (thanks for pointing it out!). I'll fix that now

ltratt · 2017-10-26T19:23:21Z

Ah, yes, you're quite right -- create_node is the problem. [Sorry, all my comments disappeared so I couldn't easily find my old ones.]

ltratt · 2017-10-26T19:34:59Z

OK, that makes me happier :) If you squash that, I can merge.

gabi-250 · 2017-10-27T12:51:53Z

@ltratt I've slightly changed the meaning of get_node: it now returns an Option which contains the index of the node that has the lowest value greater than the value argument (or None, if the tree is empty).

ltratt · 2017-10-27T22:10:51Z

This sounds sensible to me. If @vext01 is OK, then I think we're ready for the final squash!

vext01 · 2017-10-28T00:12:20Z

Go ahead.

ltratt · 2017-10-28T09:01:17Z

OK, it's time to squash!

gabi-250 · 2017-10-28T11:57:55Z

Okay, done!

gabi-250 requested a review from ltratt October 25, 2017 21:15

ltratt reviewed Oct 25, 2017

View reviewed changes

vext01 reviewed Oct 25, 2017

View reviewed changes

vext01 reviewed Oct 26, 2017

View reviewed changes

gabi-250 force-pushed the avl-tree branch from 01478b8 to 42e9b54 Compare October 26, 2017 19:04

gabi-250 force-pushed the avl-tree branch from d4650b9 to 83fe1f9 Compare October 26, 2017 19:38

Add initial AVL tree implementation.

fc73271

gabi-250 force-pushed the avl-tree branch from 1af63a6 to fc73271 Compare October 28, 2017 11:56

ltratt merged commit 770ac1c into master Oct 28, 2017

ltratt deleted the avl-tree branch October 28, 2017 12:13

AVL tree #1

AVL tree #1

Uh oh!

Conversation

gabi-250 commented Oct 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ltratt commented Oct 25, 2017

Uh oh!

gabi-250 commented Oct 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabi-250 Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vext01 commented Oct 26, 2017

Uh oh!

vext01 commented Oct 26, 2017

Uh oh!

gabi-250 commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabi-250 Oct 26, 2017 •

edited

Loading