prefer node deletion over data swapping

lib/red-black-tree.rb

@@ -145,21 +145,38 @@ def delete! node
     original_node = node
 
     if node.children_are_valid?
+      is_root = is_root? node
+
       successor = node.left
       successor = successor.left until successor.left.leaf?
-      node.swap_data_with! successor
+      node.swap_colour_with! successor
+      node.swap_position_with! successor
+      node.swap_position_with! LeafNode.new
+
+      @root = successor if is_root
+
+      original_node.validate_free!
 
-      return delete! successor
+      decrement_size!
+      update_left_most_node!
+
+      return self
     elsif node.single_child_is_valid?
       is_root = is_root? node
 
       valid_child = node.children.find(&:valid?)
-      node.swap_data_with! valid_child
-      node.black!
+      valid_child.black!
+      node.swap_position_with! valid_child
+      node.swap_position_with! LeafNode.new
+
+      @root = valid_child if is_root
+
+      original_node.validate_free!
 
-      @root = node if is_root
+      decrement_size!
+      update_left_most_node!
 
-      return delete! valid_child
+      return self
     elsif node.children_are_leaves?
       if is_root? node
         @root = nil

lib/red_black_tree/node.rb

@@ -125,12 +125,6 @@ def validate_free!
         raise StructuralError, "Node is still chained to #{anchors.join(", ")}"
       end
 
-      def swap_data_with! other_node
-        temp_data = @data
-        @data = other_node.data
-        other_node.data = temp_data
-      end
-
       def swap_colour_with! other_node
         temp_colour = @colour
 
@@ -148,27 +142,47 @@ def swap_colour_with! other_node
       def swap_position_with! other_node
         self_position = position
         other_position = other_node.position
+        opp_other_position = opposite_direction other_position if other_position
 
         if other_node.parent.object_id == self.object_id
           self[other_position] = other_node[other_position]
           other_node[other_position] = self
 
+          self[other_position].parent = self
           other_node.parent = @parent
+
+          @parent[self_position] = other_node if self_position
           @parent = other_node
-        elsif other_node.object_id == @parent.object_id
-          other_node[self_position] = self[self_position]
-          self[self_position] = other_node
 
-          temp_parent = other_node.parent
-          other_node.parent = self
-          @parent = temp_parent
+          temp_node = self[opp_other_position]
+          self[opp_other_position] = other_node[opp_other_position]
+          other_node[opp_other_position] = temp_node
+
+          self[opp_other_position].parent = self
+          other_node[opp_other_position].parent = other_node
+        elsif other_node.object_id == @parent.object_id
+          other_node.swap_position_with! self
         else
           other_node.parent[other_position] = self if other_node.parent
           @parent[self_position] = other_node if @parent
 
-          temp_parent = other_node.parent
+          temp_node = other_node.parent
           other_node.parent = @parent
-          @parent = temp_parent
+          @parent = temp_node
+
+          temp_node = other_node.left
+          other_node.left = @left
+          @left = temp_node
+
+          @left.parent = self if @left
+          other_node.left.parent = other_node if other_node.left
+
+          temp_node = other_node.right
+          other_node.right = @right
+          @right = temp_node
+
+          @right.parent = self if @right
+          other_node.right.parent = other_node if other_node.right
         end
       end
     end

test/test_red_black_tree.rb

@@ -333,7 +333,7 @@ def test_delete_root_node_with_valid_children
       assert node_15.red?
       assert node_15.left.leaf?
       assert node_15.right.leaf?
-      assert_equal root_node_10, tree.left_most_node
+      assert_equal root_node_5, tree.left_most_node
     end
 
     def test_delete_non_root_node_with_valid_children