Using vectors to shuffle cards

[Leedah8]

I'm being asked to load a deck of cards in a vector from 0-51 by creating a vector function for it. I then have to shuffle the deck using the specific random_shuffle algorithm and also declare the vectors inside of main. I'm having a hard time with proving the order that the numbers were in (ascending) and then showing that it was correctly shuffled.

Here is what I have so far:

#include <iostream>
#include <vector>
#include <algorithm>

using namespace std;

//Create Function prototypes for card loading and shuffling deck of cards
void unwrap(vector<int> &);
void shuffle(vector<int> &);

int main() {

    //Declare vector for the number of cards
    vector <int> deck(52);

    //call functions for unwrap and shuffle
    unwrap(deck);
    shuffle(deck);


    return 0;
}

//Function definitions that load cards and randomly shuffles them
void unwrap(vector<int> &deck)
{
    //Load vector with ints from 0 to 51
    for (int i = 0; i <= 51; i++)
    {
        deck.push_back(i);
        cout << "The size of the deck is now " << deck.size() << endl;
    }

}

void shuffle(vector<int> &deck)
{
    random_shuffle(deck.begin(), deck.end());
}

[Computergeek01]

First, delete the "(52)" on Line 14. That's not what you want to do.

Second, you never actually display what is in your vector. Add another for loop after you call shuffle() to display the contents of your vector. The std::vecotor::at() member function will help with that: http://en.cppreference.com/w/cpp/container/vector/at

[Leedah8]

Thanks! This is what I have so far. Still working on the loop for shuffling.

#include <iostream>
#include <vector>
#include <algorithm>

using namespace std;

//Create Function prototypes for card loading and shuffling deck of cards
void unwrap(vector<int> &);
void shuffle(vector<int> &);

int main() {

    //Declare vector for the number of cards
    vector <int> deck;

    //call functions for unwrap and shuffle
    unwrap(deck);
    shuffle(deck);

    return 0;
}

//Function definitions that load cards and randomly shuffles them
void unwrap(vector<int> &deck)
{
    //Load vector with ints from 0 to 51
    cout << "Deck Size " << endl;
    for (int i = 0; i <= 51; i++)
    {
        deck.push_back(i);
        cout << deck.size() << endl;
    }
}

// Randomize the cards in the deck
void shuffle(vector<int> &deck)
{
    random_shuffle(deck.begin(), deck.end());

}

[Computergeek01]

You're shuffle function works fine, you're just displaying the cards BEFORE you call shuffle(). Create another for loop after your call to shuffle() on Line 18 to display the shuffled vector. Numbers that never change and might be useful in other parts of the program are acceptable candidates for global const int's. It doesn't make any impact on your code here, but it's something to keep in mind.

[icy1]

yeah, basically make a completely separate display() or show() function that you call before and after your shuffling

[E0p9LyTq]

Line 14: you are declaring the size of your vector, no need to push_back values when assigning values to the elements. That extends the size of an already sized vector.

A vector keeps track of its size, why not use that?

void unwrap(std::vector<int> &deck)
{
   //Load vector with ints from 0 to 51
   for (int i = 0; i < deck.size(); i++)
   {
      deck[i] = i;
   }
}

You are not displaying the deck, either before or after being shuffled. Add a display function:

void display(const std::vector<int>&);
.
.
.
void display(const std::vector<int>& deck)
{
   for (auto loop = deck.cbegin(); loop != deck.cend(); loop++)
   {
      std::cout << *loop << ' ';
   }
   std::cout << '\n';
}

You want the vector's reference to be const, it should not be allowed to be changed.

The cbegin (constant begin) and cend (constant end) iterators require C++11, hopefully your compiler supports it.

Your main could look like this now:

int main()
{
   //Declare vector for the number of cards
   std::vector<int> deck(52);

   // unwrap the deck (fill it with values
   unwrap(deck);

   // display the original, unsorted order
   std::cout << "The deck before shuffling:\n";
   display(deck);

   // randomly shuffle the deck
   shuffle(deck);

   // display the shuffled deck
   std::cout << "\nThe deck after being shuffled:\n";
   display(deck);
}

The deck before shuffling:
0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51

The deck after being shuffled:
41 6 18 44 26 0 7 32 37 48 2 20 24 39 47 19 25 33 22 38 4 1 31 10 45 30 40 49 28 29 34 35 50 46 12 17 14 11 15 36 43 23 13 9 42 27 16 3 8 51 5 21

Two things to note, for the future:

1. std::random_shuffle uses the C-library rand function for sorting the container. You forgot to seed rand by using srand. Not seeding will give you the exact same "random" numbers every time your program runs, giving your shuffled deck of cards the exact same order.

Not a good shuffle IMO.

2. std::random_shuffle was deprecated in C++14 and removed from C++17. C++11 introduced std::shuffle. It uses a C++ random engine, so you have to create and randomize one.

Modifying your code to use std::shuffle. Add a couple of

#include <random> // for default_random_engine
#include <chrono> // for system_clock 

Change your shuffle function:

void shuffle(std::vector<int> &deck)
{
   // create a C++ random engine, seeded with the system clock
   std::default_random_engine rng(static_cast<unsigned> (std::chrono::system_clock::now().time_since_epoch().count()));
   
   // shuffle the deck
   std::shuffle(deck.begin(), deck.end(), rng);
}

I know that looks very scary and intimidating. First time seeing some new way to do C++ code can be. :)

You wrote some solid C++ code. With a few simple, easily correctable mistakes.

[Leedah8]

Thank you guys!

@FurryGuy my instructor, unfortunately has not had us use the whole "std::" way of doing things, so you are definitely right about it being scary to me. lol

[E0p9LyTq]

@Leedah8,

IMO your instructor is not doing you any favors by the use of using namespace std;. It is considered by many professionals (I am not one) as not good practice, a bad habit, and breaking bad habits can be hard.

https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice

I find it hard NOT to type std:: as part of C++ library keywords, like std::cout. :)

[Leedah8]

I would agree with you. From what I've been seeing, this is what most programmers are using. I think over the summer I'll make it a habit.

So this is what I have so far. I'm confused as to how to do the calculations in my printCards definition:

#include <iostream>
#include <vector>
#include <algorithm>

using namespace std;

//Create Function prototypes for card loading and shuffling deck of cards
void unwrap(vector<int> &);
void shuffle(vector<int> &);
void printCards(const vector<int> &);

int main() {

    //Declare vector for the number of cards
    vector <int> deck;

    cout << "The deck before shuffling " << endl;
    unwrap(deck);
    printCards(deck);

    cout << "The deck after shuffling " << endl;
    shuffle(deck);
    printCards(deck);



    return 0;
}

//Function definitions that load cards and randomly shuffles them
void unwrap(vector<int> &deck)
{
    //Load vector with ints from 0 to 51
    for (int i = 0; i < 52; i++)
    {
        deck.push_back(i);
        cout << i << endl;
    }
}

// Randomize the cards in the deck
void shuffle(vector<int> &deck)
{
    random_shuffle(deck.begin(), deck.end());

}
void printCards(const vector<int> &deck)
{
   
}

[E0p9LyTq]

@Leedah8,

I was saying this below was scary, not the use of std::

// create a C++ random engine, seeded with the system clock
std::default_random_engine rng(static_cast<unsigned> (std::chrono::system_clock::now().time_since_epoch().count()));

I'm confused as to how to do the calculations in my printCards definition

First, remove/comment out line 37. Then add to your headers <string>.

void printCards(const vector<int>& deck)
{
   std::vector<std::string> suit = { "S", "H", "C", "D" };
   std::vector<std::string> face = { "A", "K", "Q", "J", "10", "9", "8", "7", "6", "5", "4", "3", "2" };

   for (unsigned int loop = 0; loop < deck.size(); loop++)
   {
      unsigned int card_suit = deck[loop] / 13;
      unsigned int card_face = deck[loop] % 13;

      std::cout << face[card_face] << suit[card_suit] << '\t';

   }
   std::cout << '\n';
}

The deck before shuffling
AS      KS      QS      JS      10S     9S      8S      7S      6S      5S
4S      3S      2S      AH      KH      QH      JH      10H     9H      8H
7H      6H      5H      4H      3H      2H      AC      KC      QC      JC
10C     9C      8C      7C      6C      5C      4C      3C      2C      AD
KD      QD      JD      10D     9D      8D      7D      6D      5D      4D
3D      2D
The deck after shuffling
5C      QS      KD      6H      5D      2C      KS      KH      9D      9H
QH      JS      5H      3H      3D      AD      JD      QD      4H      6D
10C     4S      2S      QC      5S      3S      10S     8D      AC      JH
9C      10H     8C      3C      8H      KC      7H      10D     6S      6C
2D      AH      4C      JC      4D      2H      8S      7D      9S      7C
AS      7S

If you are going to use random_shuffle you have to call ONCE srand. If you don't you get the exact same "random" shuffling each time you run your program.

Call srand once, preferably at the start of main(). srand(time(0));
Using the C library rand/srand functions you should include <cstdlib> and <ctime> headers.

Using random_shuffle you are using rand, it is the third parameter in the function by default.

[Leedah8]

Thank you kindly!

Would you say this is good for displaying the numbers in ascending order and then showing their random order? That's really all I need this code to do for now.

#include <iostream>
#include <vector>
#include <algorithm>


using namespace std;

//Create Function prototypes for card loading and shuffling deck of cards
void unwrap(vector<int> &);
void shuffle(vector<int> &);
void printCards(const vector<int> &);

int main() {

    //Declare vector for the number of cards
    vector <int> deck;

    cout << "The deck before shuffling: " << endl;
    unwrap(deck);


    cout << "The deck after shuffling: " << endl;
    shuffle(deck);
    printCards(deck);



    return 0;
}

//Function definitions that load cards and randomly shuffles them
void unwrap(vector<int> &deck)
{
    //Load vector with ints from 0 to 51
    for (int i = 0; i <= 51; i++)
    {
        deck.push_back(i);
        cout << i << endl;
    }
}

// Randomize the cards in the deck
void shuffle(vector<int> &deck)
{
    random_shuffle(deck.begin(), deck.end());

}
void printCards(const vector<int> &deck)
{
    for(int j=0; j <=51; j++)
        cout << deck[j] << endl;
}

[icy1]

On using namespace std; -- for demonstration purposes, especially when everything is contained in one file, this is absolutely great. Typically you'll only see this in the test driver file, aka the one with main(). You almost never want this line in a header file, because you can't anticipate who might be including it.

On the bright side, including the standard namespace isn't as scary as you might think, because the classes within begin with lowercase. As per C++ stylistic guides, you're supposed to name your own classes (and functions, tbh) with uppercase, so the chances of your implementation conflicting with something in the standard namespace is that much lower.

In general, we try to limit the scope of what we use as much as possible, so if you only need to use std::string, std::vector, std::cout you could simply do:

using std::string;
using std::vector;
using std::cout;

A few tiny nitpicks about what you have there in latest ;D
1. You have couts in unwrap() function. Yes, it is more efficient to output the integers while you're generating them, but you have a printCards() function whose sole purpose is to output the deck. Following principles of each function doing its own thing and not trying to do too many things at once, you should remove line 38. Instead, unwrap() and printCards() to show the "before" state.
2. printCards() shouldn't have the 51 in it. Your deck is of some size and printCards() doesn't know or care what that size is -- utilize the built-in vector method size():

void printCards(const vector<int>& deck)
{
    // Prefer 'i', universally short for 'index', instead of 'j'.  
    // You often see 'j' only when there are nested loops happening.  
    // Likely you want a space instead of newline, esp if 52 things are being printed
    for(int i=0; i<deck.size(); ++i) 
        cout << deck[i] << " ";
    cout << endl;  // One newline after the loop
}

or if you've tasted some scripting langs and don't have an explicit need for the indices, you may prefer this syntax:

for (auto& card : deck)
    cout << card << " ";