War card game with random play

Question

I'm trying to learn C++ with a background in python. This is my first program besides general practice. I wanted to remake the war game. I'm looking to get feedback on the C++ equivalent of its pythonicness. Some mistakes I believe I've made are possibly misusing the std functions, unnecessary type transformations, and unnecessary duplications of data, but I'm not sure where to begin.

For an explanation of the code, I didn't initialize cards themselves, I just used a 1- 52 range to signify cards. To distribute the cards, I just thought it would be fun to try and use a random binary with a bitlength of 52 and distribute the cards from that. I was also hesitant on the while(true) loop, I wanted a different way to do it but it works as intended.

#include <iostream>
#include <string>
#include <random>
#include <algorithm>
#include <set>
using namespace std;

int randNum(int min, int max) {
    random_device r;
    default_random_engine e1(r());
    uniform_int_distribution<int> uniform_dist(min, max);
    int mean = uniform_dist(e1);
    return mean;
}

string toBinary(int n) {
    string r;
    while(n != 0) {
        r = (n % 2 == 0 ? "0" : "1") + r;
        n /= 2;
    }
    return r;
}

string deckShuffleSeed() {
    string deck;
    // Decimals to make a binary w/ bit length of 26
    // /2 # of cards in deck, therefore: consolidate 2, so we can use ints
    deck = toBinary(randNum(33554432, 67108863)) +
            toBinary(randNum(33554432, 67108863));
    return deck;
}

int randSetChoice(const set<int> &s) {
    auto ItRandChoice = std::next(s.begin(), randNum(0, int(s.size())));
    return *ItRandChoice;
}

string war() {
    // hand initialization
    set<int> p1, p2;
    string deckDealingSeed = deckShuffleSeed();
    for (int i = 1; i <= 52; i++) {
        int convSeedIndex = deckDealingSeed[i] - '0';
        if (convSeedIndex == 1) {
            p2.insert(i);
        } else {
            p1.insert(i);
        }
    }
    // game loop
    while (!p1.empty() || !p2.empty()) {
        int recursiveWarCount = 0;
        set<int> cardsOnTable;
        while(true) { // recursive war loop
            if (p1.size() > 3 * recursiveWarCount && p2.size() > 3 * recursiveWarCount) {
                int p1RandChoice = randSetChoice(p1), p2RandChoice = randSetChoice(p2);
                // round loop
                if (p1RandChoice / 4 > p2RandChoice / 4) { // p1 wins
                    p1.insert(p2RandChoice);
                    p2.erase(p2RandChoice);
                    if (recursiveWarCount > 0) {
                        p1.insert(cardsOnTable.begin(), cardsOnTable.end());
                        for (int i = 1; i < recursiveWarCount * 3; i++) {
                            int randTableChoice = randSetChoice(p2);
                            p1.insert(randTableChoice);
                            p2.erase(randTableChoice);
                        }
                    }
                    break;
                } else if (p1RandChoice / 4 < p2RandChoice / 4) { // p2 wins
                    p2.insert(p1RandChoice);
                    p1.erase(p1RandChoice);
                    if (recursiveWarCount > 0) {
                        p2.insert(cardsOnTable.begin(), cardsOnTable.end());
                        for (int i = 1; i < recursiveWarCount * 3; i++) {
                            int randTableChoice = randSetChoice(p1);
                            p2.insert(randTableChoice);
                            p1.erase(randTableChoice);
                        }
                    }
                    break;
                } else {  // war
                    recursiveWarCount += 1;
                    cardsOnTable.insert(p1RandChoice);
                    cardsOnTable.insert(p2RandChoice);
                }
            } else {
                if (p1.size() > p2.size()) {
                    return "p1";
                } else {
                    return "p2";
                }
            }

        }
    }
    if (p1.empty()) {
        return "p2";
    } else if(p2.empty()) {
        return "p2";
    } else {
        return "Something is wrong!";
    }
}



int main() {
    cout << war() << endl;
    return 1;
}

Answer 1

Representing decks of cards

The way you store decks of cards and shuffle them is really very strange. There's the unnecessary conversion (even for Python) from numbers to strings and back. Using std::set for a deck is wrong, because it doesn't preserve the order of the cards in the deck. Instead, I would use a std::deque (no pun intended); it is fast to insert and remove cards from either end, and preserves their order. To create a deck of 52 cards (and lets keep them represented using ints for now), you could write:

std::deque<int> deck(52);
std::iota(deck.begin(), deck.end(), 1);

Here I've used std::iota() to initialize the value of each card in the deck, although a regular for-loop would be fine too. To shuffle the cards, use std::shuffle():

std::random_device rd;
std::default_random_engine rng(rd());
std::shuffle(deck.begin(), deck.end(), rng);

Then to split the deck evenly among the two players (as mentioned in the rules of War), just copy each half of the deck, using std::deque's constructor that takes two iterators:

auto midpoint = std::advance(deck.begin(), deck.size() / 2);
std::deque<int> p1(deck.begin(), midpoint);
std::deque<int> p2(midpoint, deck.end());

Now you can just draw a card from the deck like so:

auto card = p1.front();
p1.pop_front();

And to push the card to the back of the deck you could write:

p1.push_back(card);

Note that we didn't need any random number generator after the initial shuffling anymore.

You can also move multiple cards in one go using insert() and erase(). Instead of keeping track of recursiveWarCount and only moving cards when the war is resolved, consider moving cards from both player's decks to the deck on the table every step of the war.

Avoid repeating yourself

The code that deals with player 1 winning the war is almost identical to the one for player 2 winning the war, just with p1 and p2 reversed. Whenever you see yourself mostly duplicating the same lines of code, consider writing a function instead.

You could also consider creating a function that moves a card from one deck to another, so you can just call that instead of explicitly taking a card from one deck and adding it to another.

Consider creating new types or type aliases

Explicitly writing std::deque<int> every time you declare a deck of cards is not great; it's quite long, you have to remember the correct type, and from the type it's not clear that it represents a deck of cards. Having a better name for a deck of cards would be great. You can create an alias for that type with the using keyword:

using Card = int;
using Deck = std::deque<Card>;

And then you can use it like so:

Deck p1, p2;

Another way would be to create class Card and class Deck that represent cards and decks of cards, and have member functions that perform typical operations on it, like get_value() for a Card, and shuffle(), split(), draw() and so on for a Deck. This is a bit more work up front, but it might make implementing the actual logic of the War game much easier.

Answer 2

G. Sliepen's answer already mentioned that there is a std::shuffle function, but I wanted to point that out explicitly.

I recommend, even to experienced developers, to read through the algorithm header once a year just to remind themselves what all is in it.

As for learning the C++ idioms and best practices, I suggest watching the videos from the major C++ conferences on YouTube. CppCon had a Back to Basics track in 2020.

---

Don’t write using namespace std;.

You can, however, in a CPP file (not H file) or inside a function put individual using std::string; etc. (See SF.7.)

---

 if (p1.size() > p2.size()) {
   return "p1";
 } else {
   return "p2";
 }

You can just write: return p1.size() > p2.size() ? "p1" : "p2";
This is better not just shorter, as it states up front that you are returning something... then goes on to a level of detail that what you return this or that depending. As you wrote it, it's a general-purpose if statement and requires more cognitive overhead to figure out that both branches do similar things, and you return no matter what.

---

I don't know why you are making random choices during game play: I thought the game is played by taking your next card in order, not a random card from those you hold.

---

int p1RandChoice = randSetChoice(p1), p2RandChoice = randSetChoice(p2);

I'd like to point out that it's good style in C++ to put one declaration per statement, and not gang them up like this.

You should also make things const when you can.

---

I think your use of a set (a non-ordered collection?) makes the code more complex. But even using a deque, you can improve things by defining a function to deal out a card, as seeing the value of the next card and removing it from the collection are separate operations. (That makes sense for large complex types that you would want to avoid copying.)

// not shown:  definition for card_t
using deck = std::deque<card_t>;

card_t draw (deck& d)
{
    const auto x = d.front();
    d.pop_front();
    return x;
}