My operator= and copy constructor are not working correctly in my class

Question

Main:

#include <iostream>
#include <cstdlib>
#include "avl_tree.h"


using namespace std;


int main()
{
  AVLTree<int> av1;
  int testarray [10] = { 16, 2, 77, 40, 54 , 1 , 100, 39, 73, 35 };
  AVLTree<int> av3;

  for( unsigned int i = 0; i < 10; i++ )
  {
    av1.insert( testarray[i] );
  }

  AVLTree<int> av2 = av1; //test copy constructor
  av3 = av1;  //test operator=
  av2.printTree();
  av1.printTree();
  av3.printTree();

  exit( 0 );
}

Header:

#ifndef AVL
#define AVL

#include <iostream>
using namespace std;
/**
 * An AVL tree class adapted from Weiss.
 * Does NOT allow duplicate elements.
 */
template <typename Comparable>
class AVLTree
{
 public:
  AVLTree( ) : root ( )
  {
  //nothing goes in the main constructor
  }

  AVLTree( const AVLTree & rhs ) : root ()
  {
      copyNodes( rhs.root , root );
  }

  ~AVLTree( )
  {
      makeEmpty( root );
      delete root;
  }

  const AVLTree & operator=( const AVLTree & rhs )
  {

      makeEmpty( root );
      copyNodes( rhs.root , root );
  }

  void printTree( ) const
  {
    printTree( root, 0 );
  }

  void makeEmpty( )
  {
      makeEmpty( root );
  }

  void insert( const Comparable & x )
  {
      insert( x , root );
  }
  // void remove( const Comparable & x );

 private:

  struct AVLNode
  {
    Comparable element;
    AVLNode   *left;
    AVLNode   *right;
    int       height;

  AVLNode( const Comparable & element,
       AVLNode *left,
       AVLNode *right,
       int height = 0 )
  : element( element ), left( left ), right( right ), height( height ) { }
  }; // end of AVLNode

  AVLNode * root;

  void insert( const Comparable & x, AVLNode * & t )
  {

    if( t == NULL )
    {
      //cout << "tnull" <<endl;
      t = new AVLNode( x, NULL, NULL );
    }
    else if( x < t->element )
    {
    //cout << "c1" <<endl;
      insert( x, t->left );
      if( height( t->left ) - height( t->right ) == 2 )
        if( x < t->left->element )
          rotateWithLeftChild( t );
        else
          doubleWithLeftChild( t );
    }
    else if( t->element < x )
    {
     // cout << "c2 " << t->element << " " << x <<endl;
      insert( x, t->right );
      if( height( t->right ) - height( t->left ) == 2 )
        if( t->right->element < x )
          rotateWithRightChild( t );
        else
          doubleWithRightChild( t );
    }
    //cout << "end" << endl;
    // else duplicate; do nothing
    t->height = max( height( t->left ), height( t->right ) ) + 1;
  }

  void makeEmpty( AVLNode * & t )
  {
    if ( t != NULL )
    {
        makeEmpty ( t -> left ) ;
        makeEmpty ( t -> right ) ;
    }
    delete t;
    t = NULL;
  }

  void copyNodes( AVLNode * t , AVLNode * r )
  {
      if ( t != NULL )
      {
          copyNodes( t->left , r );
          copyNodes( t->right, r );
          insert(t->element, r );
          cout << t->element << r->element << endl; //these always print as the same 
      }
  }
#endif

I'm afraid my copy constructor and operator= are not working properly as they do not result in av2 or av3 as being copies of av1. I know that the copyNodes() is working properly because the cout on line 122 reflects that t->element and r->element are the same. Why do lines 22 and 24 of the test program produce no output?

Any help on this would be much appreciated.

note: printTree() is omitted because I know for sure that it is not the problem and it's a large function.

other note: I have walked through code step by step and examined several other copy constructor/operator= functions for other classes. When I traced through step by step, I result in it working, however it does not when I actually compile it.

Answer 1

You might be able to fix your code by making the second argument to copy_nodes a reference. In your code when you call insert from within copy_nodes you are not passing in a reference to the root node of your tree but instead a reference to the r parameter of copy_node.

But I think there's a much easier (and more efficient, no rebalancing needed) way to do it. Rewrite copy_nodes as a static method which returns the copied nodes.

static AVLNode * copyNodes( AVLNode * t)
{
    if ( t != NULL )
    {
        AVLNode* left = copyNodes( t->left );
        AVLNode* right = copyNodes( t->right );
        return new AVLNode(t->element, left, right, t->height);
    }
    else
    {
        return NULL;
    }
}

You can then use this method in your copy constructor like this

AVLTree( const AVLTree & rhs )
{
    root = copyNodes( rhs.root );
}

similarly for the assignment operator.