4
\$\begingroup\$

I had lost a lot of cards of my UNO deck, and was frustrated, and that's when this idea struck my mind.
I chose C++, knowing it would be hard, just to improve my understanding of the language.
Proudly, I was able to make a basic functioning UNO game in C++!!

UNO is a multiplayer card game, and the objective is to finish your cards as soon as possible.

My implemention is very basic, no penalties, nor any house rules.
Currently, you can play with any number of bots, and the bots are not very smart, they just play the first card they see playable.

I would appreciate any improvements or suggestions on my current implementation, be it performance or readability.

Directory structure:

UNO++ - card.h
      - deck.h
      - player.h
      - cycle.h
      - game.h
      - main.cpp

card.h

#pragma once

#include <string>
#include <array>
#include <vector>
#include <map>

namespace UNO
{
    const std::array<std::string, 4> COMMON_COLORS =
        {"BLUE", "GREEN", "RED", "YELLOW"};
    const std::array<std::string, 1> SPECIAL_COLORS =
        {"BLACK"};

    const std::array<std::string, 10> NUMBERS = 
        {"0", "1", "2", "3", "4", "5", "6", "7", "8", "9"};
    const std::array<std::string, 3> ACTION_TYPES =
        {"REVERSE", "SKIP", "2 PLUS"};
    const std::array<std::string, 2> WILD_TYPES =
        {"COLOR CHANGE", "4 PLUS"};

    std::map<std::string, std::string> COLOR_MAP = {
        {"BLUE", "\033[34m"},
        {"GREEN", "\033[32m"},
        {"RED", "\033[31m"},
        {"YELLOW", "\033[93m"},
        {"BLACK", "\033[30m"},
    };
    const std::string RESET_COLOR = "\033[0m";

    class Card
    {
    public:
        std::string color;
        std::string type;

        Card(): color{"BLUE"}, type{"0"}{};
        Card(std::string color_, std::string type_):
            color{color_}, type{type_}{};

        operator std::string() const
        {
            return COLOR_MAP[color] + type + RESET_COLOR;
        }

        friend std::ostream& operator<<(std::ostream& os, const Card& card)
        {
            os << COLOR_MAP[card.color] << card.type << RESET_COLOR;
            return os;
        }
    };

    bool can_play_card(const Card& play_card, const Card& top_card)
    {
        if (
            top_card.color == play_card.color || 
            top_card.type == play_card.type || 
            play_card.color == "BLACK"
        )
        {
            return true;
        }
        return false;
    };

    std::string cards_to_str(const std::vector<Card>& cards)
    {
        int len_cards = cards.size();
        std::string str_cards = "[";

        for (int i = 0; i < len_cards; ++i)
        {
            str_cards += std::string(cards[i]);

            if (i != len_cards - 1)
                str_cards += ", ";
        }
        str_cards += "]";

        return str_cards;
    }
};

deck.h

#pragma once

#include <algorithm>
#include "card.h"

namespace UNO
{
    std::vector<Card> create_cards(
        int cards_num=2,
        int cards_action=1,
        int cards_wild=2
    )
    {
        std::vector<Card> cards;

        for (auto color : COMMON_COLORS)
        {
            for (auto num_type : NUMBERS)
            {
                for (int i = 0; i < cards_num; ++i)
                    cards.push_back(Card(color, num_type));
            }

            for (auto action_type : ACTION_TYPES)
            {
                for (int i = 0; i < cards_action; ++i)
                    cards.push_back(Card(color, action_type));
            }
        }

        for (auto wild_type : WILD_TYPES)
        {
            for (int i = 0; i < cards_wild; ++i)
                cards.push_back(Card("BLACK", wild_type));
        }

        return cards;
    }

    class Deck
    {
    public:
        std::vector<Card> cards;

        Deck(): cards{create_cards()}{};
        Deck(std::vector<Card> cards_): cards{cards_}{};

        friend std::ostream& operator<<(std::ostream& os, const Deck& deck)
        {
            os << cards_to_str(deck.cards);
            return os;
        }

        void shuffle()
        {
            std::random_shuffle(cards.begin(), cards.end());
        }

        std::vector<Card> deal_cards(int amount)
        {
            if (cards.size() < amount)
            {
                for (const Card& card : create_cards())
                    cards.push_back(card);
            }

            std::vector<Card> dealt_cards;
            for (int i = 0; i < amount; ++i)
            {
                dealt_cards.push_back(cards.back());
                cards.pop_back();
            }

            return dealt_cards;
        }
    };
}

player.h

#pragma once

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

namespace UNO
{
    class Player
    {
    public:
        std::string name;
        std::vector<Card> cards;

        Player(std::string name_, std::vector<Card> cards_):
            name{name_}, cards{cards_}{};

        friend std::ostream& operator<<(std::ostream& os, const Player& player)
        {
            os << player.name;
            return os;
        }

        int get_play_card_index(const Card& top_card) const
        {
            for (int i = 0; i < cards.size(); ++i)
            {
                if (can_play_card(cards[i], top_card))
                    return i;
            }
            return -1;
        }

        Card pop_card(int index)
        {
            Card popped_card = cards[index];
            cards.erase(cards.begin() + index);
            return popped_card;
        }

        bool is_win() const
        {
            return (cards.size() == 0);
        }
    };
}

cycle.h

#pragma once

#include <vector>

namespace UNO
{
    template <typename Type>
    class Cycle
    {
    private:
        int index = -1;
        bool reversed = false;

    public:
        std::vector<Type> items;
        Cycle(){};
        Cycle(std::vector<Type> items_): items{items_}{};

        Type* next()
        {
            index += (reversed) ? -1 : 1;
            if (index == items.size())
                index = 0;
            else if (index == -1)
                index = items.size() - 1;

            return &items[index];
        }

        void reverse()
        {
            reversed = !reversed;
        }
    };
}

game.h

#pragma once

#include <iostream>
#include <ctime>
#include <algorithm>
#include <windows.h>

#include "card.h"
#include "deck.h"
#include "player.h"
#include "cycle.h"

namespace UNO
{
    class Game
    {
    public:
        Deck cards_deck;
        std::vector<Player> players;
        Cycle<Player> player_cycle;
        Card top_card;

        Game()
        {
            int no_of_players;
            std::cout << "Enter number of players: ";
            std::cin >> no_of_players;
            if (std::cin.fail())
                std::cerr << "Input Failure";

            std::string player_name;
            std::cout << "Enter your name: ";
            std::cin >> player_name;
            if (std::cin.fail())
                std::cerr << "Input Failure";

            cards_deck.shuffle();
            players.push_back(Player(player_name + " #1", cards_deck.deal_cards(7)));

            for (int i = 0; i < no_of_players - 1; ++i)
            {
                players.push_back(
                    Player(
                        "Player #" + std::to_string(i + 2),
                        cards_deck.deal_cards(7))
                    );
            }
            top_card = cards_deck.deal_cards(1)[0];
            player_cycle.items = players;
        };

        Game(Deck cards, Cycle<Player> player_cycle_):
            cards_deck{cards}, player_cycle{player_cycle_}{};

        bool human_player(const Player& player) const
        {
            return (player.name.back() == '1');
        }

        void play_game()
        {
            print_intro();
            while (true)
            {
                Sleep(3500);
                Player* player = next_player();
                print_turn(*player);
                handle_player(player);

                if (player->is_win())
                {
                    std::cout << *player << " wins the game!!\n";
                    break;
                }
            }
        };

        void handle_player(Player* player)
        {
            if (human_player(*player))
                handle_human_player(player);
            else
                handle_ai_player(player);
        }

        void print_intro() const
        {
            std::cout << "Welcome to UNO!\n";
            std::cout << "Finish your cards as fast as possible!\n\n";
        }

        void print_turn(Player& player) const
        {
            if (human_player(player))
            {
                std::cout << "You have " << player.cards.size()
                << " cards left - " << cards_to_str(player.cards) 
                << '\n';
            }
            else
            {
                std::cout << player << " has " << player.cards.size()
                    << " cards left...\n";
            }
            std::cout << "Top card - " << top_card << "\n\n";
        }

        void handle_human_player(Player* player)
        {
            bool any_play_card = false;
            for (const Card& card : player->cards)
            {
                if (can_play_card(card, top_card))
                {
                    any_play_card = true;
                    break;
                }
            }

            char choice = 'd';
            if (any_play_card)
            {
                while (true)
                {
                    std::cout << "Play or Draw? (p/d): ";
                    std::cin >> choice;
                    if (std::cin.fail())
                        std::cerr << "Input Failure";

                    choice = tolower(choice);
                    if (choice == 'p' || choice == 'd')
                        break;
                    else
                        std::cout << "Invalid Input\n";
                }
            }

            if (choice == 'p')
            {
                int index;
                while (true)
                {    
                    std::cout << "Enter card index " << '(' << "1 - " << 
                        player->cards.size() << ')' << ": ";
                    std::cin >> index;
                    if (std::cin.fail())
                        std::cerr << "Input Failure";

                    if (index < 0 || index > player->cards.size())
                        std::cout << "Invalid index!\n";
                    else if (!can_play_card(player->cards[index - 1], top_card))
                        std::cout << "Can not play " << player->cards[index - 1] << "!\n";
                    else
                        break;
                }

                Card card = player->pop_card(index - 1);
                top_card = card;
                std::cout << *player << " plays " << card << '\n';
                handle_card_effect(&card, player);
            }
            else if (choice == 'd')
            {
                Card card = cards_deck.deal_cards(1)[0];
                player->cards.push_back(card);
                std::cout << "You got a " << card << "...\n";
                Sleep(2000);

                if (can_play_card(card, top_card))
                {
                    std::cout << "Do you want to play " << card << "? (y/n): ";
                    char choice;
                    std::cin >> choice;
                    if (std::cin.fail())
                        std::cerr << "Input Failure";
                    choice = tolower(choice);

                    if (choice == 'y')
                    {
                        top_card = card;
                        player->cards.pop_back();
                        std::cout << *player << " plays " << card << '\n';
                        handle_card_effect(&card, player);
                    }
                }
                std::cout << '\n';
            }
        }

        void handle_ai_player(Player* player)
        {
            int card_index = player->get_play_card_index(top_card);
            if (card_index != -1)
            {
                Card play_card = player->pop_card(card_index);
                std::cout << *player << " plays " << play_card << '\n';
                handle_card_effect(&play_card, player);
            }
            else
            {
                Card card = cards_deck.deal_cards(1)[0];
                std::cout << "Dealing 1 card to " << *player << "...\n";
                if (can_play_card(card, top_card))
                {
                    top_card = card;
                    Sleep(3500);
                    std::cout << *player << " plays " << card << '\n';
                    handle_card_effect(&card, player);
                }
                else
                    player->cards.push_back(card);
                std::cout << '\n';
            }
        }

        void handle_card_effect(Card* card, Player* card_player)
        {
            bool card_is_number = (std::find(
                NUMBERS.begin(), NUMBERS.end(), card->type) != NUMBERS.end());
            top_card = *card;

            if (card_is_number) {}
            else if (card->type == "REVERSE")
            {
                player_cycle.reverse();
                std::cout << "Cycle reversed...\n";
            }
            else if (card->type == "SKIP")
            {
                Player* skip_player = next_player();
                std::cout << *skip_player << " skipped...\n";
            }
            else if (card->type == "2 PLUS")
            {
                Player* player = next_player();
                std::vector<Card> deal_cards = cards_deck.deal_cards(2);
                for (int i = 0; i < 2; ++i)
                    player->cards.push_back(deal_cards[i]);

                std::cout << "Dealing 2 cards to " << *player << ", skipped...\n";              
            }
            else if (card->type == "COLOR CHANGE")
            {
                if (human_player(*card_player))
                    top_card.color = get_color_input();
                else
                    top_card.color = COMMON_COLORS[rand() % 4];
                std::cout << "Color changed to " << COLOR_MAP[top_card.color]
                    << top_card.color << RESET_COLOR << '\n';
            }
            else if (card->type == "4 PLUS")
            {
                if (human_player(*card_player))
                    top_card.color = get_color_input();
                else
                    top_card.color = COMMON_COLORS[rand() % 4];
                std::cout << "Color changed to " << COLOR_MAP[top_card.color]
                    << top_card.color << RESET_COLOR << '\n';

                Player* player = next_player();
                std::vector<Card> deal_cards = cards_deck.deal_cards(4);
                for (int i = 0; i < 4; ++i)
                    player->cards.push_back(deal_cards[i]);

                std::cout << "Dealing 4 cards to " << *player << ", skipped...\n";            
            }
            std::cout << '\n';
        }

        Player* next_player()
        {
            while (true)
            {
                Player* player = player_cycle.next();
                if (!player->is_win())
                    return player;
            }
        };

        std::string get_color_input()
        {
            std::string color;
            do
            {
                std::cout << "Enter color: ";
                std::cin >> color;
                if (std::cin.fail())
                    std::cerr << "Input Failure";
                for (int i = 0; i < color.length(); ++i)
                    color[i] = toupper(color[i]);
            } while (
                std::find(COMMON_COLORS.begin(),
                COMMON_COLORS.end(), color) == COMMON_COLORS.end()
            );

            return color;
        };
    };
}

main.cpp - A minimal example

#include <iostream>
#include <ctime>
#include "game.h"

using namespace UNO;

int main()
{
    srand(time(0));
    /* Can also handle creating players and cards deck
    Deck cards;
    std::vector<Player> players = {
        Player("Taha #1", cards.deal_cards(7)),
        Player("player #2", cards.deal_cards(7)),
        Player("player #3", cards.deal_cards(7)),
        Player("player #4", cards.deal_cards(7)),
    };
    Cycle<Player> player_cycle(players);
    Game my_game(cards, player_cycle);
    */

    // Or use the default constructor
    Game my_game;
    my_game.play_game();
    return 0;
}

Some points to know before review:

  • I have approached a object-oriented style.
  • I have create the Cycle a template class, so it could be used for other purposes as well.
  • You may see a lot of random '\n' in between blocks of code, they are just to make the console look clean.
  • I said this, but again, the bots are not very smart.
  • It took me a lot of time and errors to reach at this point, so there may be some bugs, but the ones I observed, I have fixed them.
\$\endgroup\$
1
  • \$\begingroup\$ ANSI color codes (eg \033[34m) are not guaranteed to work on every platform, and will produce garbage on ones they don't work \$\endgroup\$
    – Offtkp
    Dec 18, 2021 at 14:08

2 Answers 2

1
\$\begingroup\$

Avoid using stringly-types

class Card is storing its color and card type as std::strings. This has several issues. Apart from using a lot of memory per card, what happens if someone creates a Card with color and type set to names that are not valid UNO card colors and types? Epsecially in a larger program, mistakes are easily made, consider:

Card card1{"Red", "3"};              // wrong capitalization
Card card2{"COLOR CHANGE", "BLACK"}; // wrong order

The way to solve these problems is to create enum types, or better enum class types, for both color and card type:

enum class Color {
    BLUE, GREEN, RED, YELLOW, BLACK
};

enum class Type {
    _0, _1, _2, ..., _9,
    REVERSE, SKIP, ...
};

class Card {
public:
    Color color;
    Type type;

    Card(): color{Color::BLUE}, type{Type::_0} {}
    Card(Color color, Type type): color{color}, type{type} {}
    ...
};

And for example, can_play_card() now looks like:

bool can_play_card(const Card& play_card, const Card& top_card) {
    return top_card.color == play_card.color ||
           top_card.type == play_card.type ||
           play_card.color == Color::BLACK;
}

You still need some way to map between std::strings and Colors and Types, unfortunately enums don't help you with that.

Use Deck everywhere instead of std::vector<Card>

You are using both Decks and std::vector<Card>s in your code for sets of cards. I suggest that you try to make it possible to use Deck everywhere (except inside class Deck you still have to use a std::vector<Card> of course). To make this work, you should add member functions to Deck that make it act like a std::vector would, for example size(), begin() and end(), push_back(), erase(), and so on. A possible shortcut is to make Deck inherit from std::vector<Card>.

You could also consider moving the code from create_cards() into a constructor of Deck.

Possible out-of-bounds access in Cycle

It is possible to have Cycle::next() read out of bounds. For example, if items is empty, it will always try to return a pointer to a non-existing item. But it is also possible to construct a Cycle with multiple players, but call reverse() before the first call to next(), in which case the first call to next() will cause index will become -2.

While this situation is perhaps not possible in a game of UNO (since you always start in the forward direction, and you can't reverse until the next_player() has been called at least once), it is a good idea that a class doesn't rely too much on the behavior of its users for its correctness.

You can easily make next() behave correctly even if reverse() was called as the first thing.

As for calling next() on an empty cycle, you could argue that a caller should never do that. It might still be nice to assert(!items.empty()), so debugging the program is easier in case this does happen. You could also consider forbidding the construction of an empty Cycle, by deleting the default constructor and making the one that takes a vector of items throw if items.empty().

Don't arbitrarily sleep

Sleeping for 3.5 seconds between players quickly gets annoying. Also, Sleep() is not portable C++, if you really want to use it, use std::this_thread::sleep_for() instead. But even better might be to just wait for the user to hit Enter on the keyboard to advance to the next player; that allows the human player to control the speed as they like.

Consider using std::getline() to read input

Reading input with std::cin >> variable can be problematic, as this doesn't read a whole line, it reads a single number (if variable is an int), or a single word (if it is a std::string). Consider:

std::string player_name;
std::cout << "Enter your name: ";
std::cin >> player_name;

What if I use my first and last name? This will cause player_name to be filled with my first name, and it will probably cause Invalid Input to be printed the first time handle_human_player() is called. To ensure you read whole lines of input, use std::getline():

std::string player_name;
std::cout << "Enter your name: ";
if (!std::getline(std::cin, player_name)) {
    std::cerr << "Input failure\n";
    return;
}

When you want to read numbers, you will have to convert the string containing the line you just read to a number, for example using std::stoi():

std::string line;
std::cout << "Enter number of players: ";
if (!std::getline(std::cin, line)) {
    std::cerr << "Input failure\n";
    return;
}
int no_of_players = std::stoi(line);

But be aware that std::stoi(), just like std::cin >> number, stops parsing as soon as it hits the first character that is not a valid part of a number.

Avoid using std::random_shuffle()

std::random_shuffle() was deprecated in C++14 and removed in C++17. Avoid using it, instead use std::shuffle().

Pass large objects by (const) reference

While you do use (const) references in some places, there are others where you are passing parameters by value unnecessarily. For example, the constructor of Player() should take the name and deck of cards as const references.

Pass Player by reference instead of by pointer

In Game::handle_player(), you pass the player by pointer, but in print_turn() it is by reference. Unless there is a reason for passing it as a pointer (like nullptr being a valid pointer value, or because you have virtual classes), prefer passing objects by reference.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for such a detailed review, if I use enum classes, create_deck() will not be easy to define. That is the reason I used arrays. \$\endgroup\$ Dec 18, 2021 at 9:50
  • 1
    \$\begingroup\$ You can still make an array of an enum type: std::array<Color, 4> common_colors = {Color::BLUE, Color::GREEN, ...}. \$\endgroup\$
    – G. Sliepen
    Dec 18, 2021 at 14:13
1
\$\begingroup\$

Place definitions in .cpp files

If you place all definitions in header files you're going to take a huge hit in compilation speed down the line, because if you make any changes to the definitions inside the headers, any file that includes those headers would have to be recompiled. An exception can be cycle.h which is a template class.

Suggestion: use include guards and pragma once together. Include guards are defined in the standard, pragma once has its benefits and is supported by pretty much every modern compiler. More reading.

Understanding operator<<

operator<< should not be a member function. And it does not always need to be friend.

operator<< should be a free function outside of the class. If and only if you need private members from the right hand side class, then you can add a friend declaration (but not definition like you do) inside the class like so: friend std::ostream& operator<<(std::ostream& out, const Y& o);.

Example from cppreference:

class Y {
    int data; // private member
    // the non-member function operator<< will have access to Y's private members
    friend std::ostream& operator<<(std::ostream& out, const Y& o);
};
// friend declaration does not declare a member function
// this operator<< still needs to be defined, as a non-member
std::ostream& operator<<(std::ostream& out, const Y& y)
{
    return out << y.data; // can access private member Y::data
}

Avoid redundant copies

The other answer suggests you use enums instead of strings (which you should). But this is a flaw that I will point out, in case you would need to iterate through a string array in the future:
for (auto color : COMMON_COLORS)
See the problem in this line?

If not, imagine it in this other form:

for (int i = 0; i < COMMON_COLORS.length(); i++) {
    auto color = COMMON_COLORS[i];
}

That's right. color is an std::string when it could've been a const std::string&.

Solution: for (const auto& color : COMMON_COLORS).
You won't see a runtime speed difference for the specific program, but it is avoiding premature pessimization. There's no need to copy color, the fix is simple, so we fix it.

Fix encapsulation

Functions that are not used by other classes should be private. Furthermore , all of your classes are class which by default makes all of its members private, and you use public to make them all public instead. If you really need all public members, might as well use a struct (same as class, but all members are public). Most of the times however you would want a mix of public and private (and protected for base classes)

class Foo {
    int f; // f is private
public:
    float a; // a is public
    std::string b; // b is public
private:
    bool c; // c is private
}

Consider using Getters and Setters instead of public member variables. Good practice is that only functions should be public, variables should be private. You don't have to follow this of course, it's a suggestion.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.