Expert: Ralph McArdell - 7/23/2006

Question
I need to know if my copy constructor for class Hand is correct, and what to do next in the class Deck to write correct copy constructor. Here is the program:

_________________________________________________
//Blackjack
//Plays a simple version of the casino game of
class Hand//HAND CLASS START
{
public:
   Hand();
   
   Hand(const Hand& c)// MAKE COPY CONSTRUCTOR
   {
       Copy (c);
   }
   
   Hand& Copy (const Hand& c);
   
   Hand& Hand::operator=(const Hand& rhs);
   
   virtual ~Hand();

   //adds a card to the hand
   void Add(Card* pCard);

   //clears hand of all cards
   void Clear();

   //gets hand total value, intelligently treats aces as 1 or 11
   int GetTotal() const;
protected:

   vector<Card*> m_Cards;//holds card pointers
//for memory on the heap
  
};


//COPY FUNCTION
Hand& Hand::Copy (const Hand& c)//COPY CONSTRUCTOR
{
    
   vector<Card*>::const_iterator iter = c.m_Cards.begin();
   
   for (iter = c.m_Cards.begin(); iter != c.m_Cards.end(); ++iter)
   {
       
       m_Cards.push_back(new Card);  //darn I am
   }
   
   return *this;
   
           
}
//END COPY FUNCTION
//START OVERLOAD OPERATOR
Hand& Hand::operator=(const Hand& rhs) //OPERATOR OVERLOADER
{
   if (&rhs!= this)
   {
       Clear();
       Copy(rhs);
       return *this;//returns the copy
   }

}

Hand::Hand()
{
   m_Cards.reserve(7);
}

Hand::~Hand()  //don't use the keyword virtual outside of class definition
{
   Clear();
}

void Hand::Add(Card* pCard)
{
   m_Cards.push_back(pCard);
}

void Hand::Clear()
{
   //iterate through vector, freeing all memory on the heap
   vector<Card*>::iterator iter = m_Cards.begin();
   for (iter = m_Cards.begin(); iter != m_Cards.end(); ++iter)
   {
       delete *iter;
       *iter = 0;
   }
   //clear vector of pointers
   m_Cards.clear();
}

int Hand::GetTotal() const
{
   //if no cards in hand, return 0
   if (m_Cards.empty())
       return 0;
 
   //if a first card has value of 0, then card is face down; return 0
   if (m_Cards[0]->GetValue() == 0)
       return 0;
   
   //add up card values, treat each Ace as 1
   int total = 0;
   vector<Card*>::const_iterator iter;
   for (iter = m_Cards.begin(); iter != m_Cards.end(); ++iter)
       total += (*iter)->GetValue();
                 
   //determine if hand contains an Ace
   bool containsAce = false;
   for (iter = m_Cards.begin(); iter != m_Cards.end(); ++iter)
       if ((*iter)->GetValue() == Card::ACE)
           containsAce = true;
         
   //if hand contains Ace and total is low enough, treat Ace as 11
   if (containsAce && total <= 11)
       //add only 10 since we've already added 1 for the Ace
       total += 10;   
           
   return total;
}

class Deck : public Hand
{
public:
   Deck():rePopulate(9)//default constructor
   {
   m_Cards.reserve(52);
   Populate();
   }
   
   Deck(const Deck& c):Hand(c){};//copy constructor
   
   Deck& Deck::operator=(const Deck& oRhs );
    
   virtual ~Deck();

   //create a standard deck of 52 cards
   void Populate();

   //shuffle cards
   void Shuffle();

   //deal one card to a hand
   void Deal(Hand& aHand);

   //give additional cards to a generic player
   void AdditionalCards(GenericPlayer& aGenericPlayer);
protected:
int rePopulate;
};



Deck::~Deck()
{}


//OVERLOAD OPERATOR START
Deck& Deck::operator=(const Deck& oRhs )
{
   Hand::operator=(oRhs);
}

void Deck::Populate()
{
   Clear();
   //create standard deck
   for (int s = Card::CLUBS; s <= Card::SPADES; ++s)
           for (int r = Card::ACE; r <= Card::KING; ++r)
               Add(new Card(static_cast<Card::rank>(r), static_cast<Card::suit>(s)));
}

void Deck::Shuffle()
{
   random_shuffle(m_Cards.begin(), m_Cards.end());
}

void Deck::Deal(Hand& aHand)
{
   if (!m_Cards.empty())
   {
       aHand.Add(m_Cards.back());
       m_Cards.pop_back();
       
       int stackSize = m_Cards.size();//ASKING FOR THE VECTORS SIZE
       
       if (stackSize < rePopulate)//if number of cards in stack less than 9
       {
           Populate();//repopulate the deck
       }
   }
   else
   {
       cout << "Out of cards. Unable to deal.";
   }
}

void Deck::AdditionalCards(GenericPlayer& aGenericPlayer)
{
   cout << endl;
   //continue to deal a card as long as generic player isn't busted and
   //wants another hit
   while ( !(aGenericPlayer.IsBusted()) && aGenericPlayer.IsHitting() )
   {
       Deal(aGenericPlayer);
       cout << aGenericPlayer << endl;
       
       if (aGenericPlayer.IsBusted())
           aGenericPlayer.Bust();
   }
}  

Answer
I am not best pleased to receive such question. The code you post is incomplete (classes Hand and GenericPlayer and all includes are missing), does not compile for various other reasons and is quite difficult to read.

If you are going to post code for review then please distil it down to the minimum required to demonstrate the query - this will focus the expert's attention on the problem and make sure it builds and is neatly formatted and maybe add comments to explain anything odd or that you really want us to focus on. In short make it as easy for the expert to see what you want help with. You might even find that you answer the query (at least in part) yourself while doing this.

Hand::Copy operation:
---------------------
As far as I can tell from the code you post it is not correct. You seem to create the copy of Hands with new default constructed cards instead of cards that are copies of the other Hand.

Desk copy construction and assignment:
-------------------------------------
As far as I can tell the only requirement for Deck copy and assignment is that the base Hand copy or assignment is executed. This will be performed by default anyway so I suggest you remove your explicit Deck::Deck(Deck const&) and Deck::operator=(Deck const&) operations.

Notes:
-----
You can check that the code does what you think by actually building it and creating a test harness for each class that tests each operation. Each test would have an expected result which you check for after performing the operation under test.

For example you can create a test for Hand copy construction:

   Hand h1;

   // set up cards in h1 to non-default values

   Hand h2( h1 ); // copy h1 to h2

   // check that each card in h2 matches the same card in h1

Once you have written the initial set of tests you add to them as you add to the classes. You can then re-run them anytime changes have been made to ensure that nothing is broken. If any tests fail then either the class implementation is broken or the test is broken, and you can fix as appropriate. Such tests are often called call unit tests, and running them after changes is called regression testing.

As you are creating new Cards Hand::Copy can fail due to resource exhaustion (no memory). In this case an exception would be thrown, and I think that your Hand (and Deck) objects would be left in a bad state in this situation in the case of the assignment operation. An exception thrown during copy construction will not create the object and will therefore _not_ cause the Hand destructor to be executed - thus you would 'loose' memory for all the Cards created up to the point the exception is thrown. You might like to read up on exceptions and exception safety see for example http://www.parashift.com/c++-faq-lite/exceptions.html and http://www.gotw.ca/gotw/index.htm especially articles 59..62, 65 and 66.

You are using pointers in your Card vector. Why? Why not make the Card type compatible with vector element types (if it is not already) i.e. it is copyable, assignable and destroyable by a destructor. Some operations will additionally require that Card is default constructible and comparable (operator==). None of these is too difficult to arrange.

If you do this then you can use a vector<Card> instead of vector<Card*>. You should not then require any explicit assignment or copy constructor logic for Hand as the m_Cards member will then have value semantics - meaning it will copy properly by default. Also I suspect that a lot if not all of the exception problems will go away (I would have to think a bit more on it to be sure!).

Hand::Add would then take a const reference to a Card object and you would pass it a (possibly temporary) Card thus:

void Hand::Add(Card const & card)
{
   m_Cards.push_back(card);
}

void Deck::Populate()
{
   Clear();
   //create standard deck
   for (int s = Card::CLUBS; s <= Card::SPADES; ++s)
       for (int r = Card::ACE; r <= Card::KING; ++r)
           Add( Card( static_cast<Card::rank>(r)
                    , static_cast<Card::suit>(s)
                    )
              );

}

If you do not like all this simplicity then at least use a smart pointer type rather than a raw pointer such as the Boost scoped_pointer (see http://www.boost.org/ and http://www.boost.org/libs/smart_ptr/smart_ptr.htm). Note that the unusual behaviour of std::auto_ptr means that it cannot be used with the standard containers such as std::vector.

Hope this is of use.