c++ runtime error with random_shuffle of vector in a class

Question

I am trying to code a game of Blackjack. I have been self-learning C++ in my spare time, and this is my first time posting on any website about programming.

I have been looking up the answers to my problems as I go along, and have learned quite a lot.. but this problem has completely baffled me. I fear that I am approaching the task completely wrong, and hopefully you can help me with this.

I have a Card class, and a Deck class that holds a vector of 52 Cards. The vector is a private member of Deck class, and I fear this is my problem?

When I add the random_shuffle line to my code, it compiles fine but then the console window crashes (Windows 7 x64, code::blocks, c++). I can't work out what I am doing wrong. I call the vector random access iterators begin() and end()...

deck.h

#ifndef DECK_H
#define DECK_H

#include <vector>

using namespace std;

/** Card Class */
class Card
{
public:
/** Constructor prototypes */
//Card(); //default constructor
Card(int s, int r) : suit(s), rank(r) {} 

/** GET function prototypes */
int getRank(); // returns card number as int
string getSuit(); // returns the suit in a string

private:
int rank;
int suit;
} ;

/** Deck class */
class Deck
{
public:
Deck();
vector <Card> get_deck() { return deck; };

private:
vector<Card> deck;
};

#endif // DECK_H

deck.cpp

#include <iostream>
#include <string>
#include <vector>
#include "deck.h"

using namespace std;

/** Deck ctor to initialise deck */
Deck::Deck()
{
for(int suit = 0; suit < 4; suit++)
{
    for(int rank = 0; rank < 13; rank++)
    {
        deck.push_back(Card(suit,rank));
    }
}

}

/** Functions to GET rank and suit */
// Function to get rank as int
int Card::getRank()
{
return rank;
}

// Function to get suit as string
string Card::getSuit()
{
switch(suit)
{
    case 0:
    return "Diamonds";

    case 1:
    return "Hearts";

    case 2:
    return "Clubs";

    case 3:
    return "Spades";

    default:
    return "Error";
}
}

main.cpp

#include <iostream>
#include <algorithm>
#include <ctime> // time()
#include <string>
#include <vector>

#include "deck.h"

using namespace std;

int main()
{

Deck mydeck;

random_shuffle( mydeck.get_deck().begin(), mydeck.get_deck().end() );

// Loop to iterate through deck of cards
for(int i = 0; i<52; i++)
{
    cout << mydeck.get_deck()[i].getRank() << " of " << mydeck.get_deck()[i].getSuit() << endl;
}

// Display size of deck
//cout << endl << "The size of deck is: " << mydeck.get_deck().size() << endl;


return 0;
}

Any help or words of wisdom will be much appreciated, and I hope I formatted everything right...

Many Thanks

Dan

Answer 1

This accessor method:

vector <Card> get_deck() { return deck; };

Returns a copy of the vector of cards. So when you call it twice, you get two different copies, and the begin() of the first copy doesn't match up with the end() of the second copy, so it crashes.

To fix it, you should return the array by reference so that a copy is not made:

vector <Card>& get_deck() { return deck; }  // no semicolon needed here
//           ^
//           |
//    this is a reference

However, this allows the caller to modify the internal array, which is generally a bad idea. To avoid this, you should return it by const reference:

const vector <Card>& get_deck() { return deck; }

But if you do that, then std::random_shuffle can't modify the array. So to fix that, the ideal solution would be to add a class method to the Deck class which calls random_shuffle on itself.

Answer 2

Try returning vector<Card>& from get_deck(). In the posted code you are making two separate copies and returning those.

When random_shuffle tries to do its job it therefore has iterators pointing into two different vectors.

As @Will points out in comments to another answer, you would be better off preserving encapsulation by implementing a method void Deck::shuffle() which calls random_shuffle on the member deck and not expose deck at all.