1
\$\begingroup\$

I started writing a card game in C++ yesterday, just as something to do to familiarize myself with the language. When I showed it to a friend of mine in the compsci field he said I should share it here for advice on how to improve.

The program's basic functionality right now is it can build a deck of cards, shuffle it, access information about those cards' stats and such, draw cards into a hand, play cards from the hand into play, and discard from either the hand or play into the discard pile.

I'd love any advice on how I can improve my code. Please assume I know nothing about C++ when explaining something, as this is my second program ever in the language (my first was a hello world lol). The only other language I know is python.

The main deck manager file:

#include <iostream>
#include <array>
#include <algorithm>
#include <random>
#include <chrono>
#include "cards.h"
using namespace std;

class Deck{
    public:
        array<string, 10> draw_pile;
        array<string, 10> discard_pile = {"Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty"};
        map<string, map<string, string>> card_tags;
        map<string, map<string, int>> card_stats;
        map<string, map<string, bool>> card_abilities;
        Deck(string deck_type){
            if (deck_type == "base") {
                draw_pile[0] = "minion";
                draw_pile[1] = "minion";
                draw_pile[2] = "minion";
                draw_pile[3] = "minion";
                draw_pile[4] = "minion";
                draw_pile[5] = "minion";
                draw_pile[6] = "minion";
                draw_pile[7] = "super_minion";
                draw_pile[8] = "super_minion";
                draw_pile[9] = "super_minion";
                base_cards(&card_tags, &card_stats, &card_abilities);
                unsigned seed = std::chrono::system_clock::now().time_since_epoch().count();
                shuffle (draw_pile.begin(), draw_pile.end(), default_random_engine(seed));
            } else {cout << "unrecognized deck";};
        };
        void shuffle_draw_pile() {
            unsigned seed = std::chrono::system_clock::now().time_since_epoch().count();
            shuffle (draw_pile.begin(), draw_pile.end(), default_random_engine(seed));
        }

};

class Hand{
    public:
        int owner_id;
        array<string, 10> hand_cards = {"Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty"};
        array<string, 10> in_play_creatures = {"Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty"};
        array<string, 10> in_play_lands = {"Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty", "Empty"};
        Hand(int owner) {
            owner_id = owner;
        };
        void sort_hand() {
            int i = 1;
            int first_empty_idx = 0;
            while (i < hand_cards.size()) {
                if (all_of(hand_cards.begin(), hand_cards.end(), [](string card) {return card == "Empty";})) {
                    i += (2 * hand_cards.size());
                    //cout << "hand empty ";
                } else if (all_of(hand_cards.begin(), hand_cards.end(), [](string card) {return card != "Empty";})) {
                    i += (2 * hand_cards.size());
                    //cout << "hand full ";
                } else if (hand_cards[i-1] != "Empty" && hand_cards[i] != "Empty") {
                    //cout << i << " all full, proceed\n";
                    i++;
                } else if (hand_cards[i-1] != "Empty" && hand_cards[i] == "Empty") {
                    //cout << i << " found the first empty, proceeding\n";
                    first_empty_idx = i;
                    i++;
                } else if (hand_cards[i-1] == "Empty" && hand_cards[i] == "Empty") {
                    //cout << i << " all empty proceed\n";
                    i++;
                } else if (hand_cards[i-1] == "Empty" && hand_cards[i] != "Empty") {
                    //cout << i << " found one out of order, reordering\n";
                    hand_cards[first_empty_idx] = hand_cards[i];
                    hand_cards[i] = "Empty";
                    i = first_empty_idx + 1;
                } else {
                    cout << "idk what happened the sorter is fucked";
                };
            };
        };
        void sort_in_play_creatures() {
            int i = 1;
            int first_empty_idx = 0;
            while (i < in_play_creatures.size()) {
                if (all_of(in_play_creatures.begin(), in_play_creatures.end(), [](string card) {return card == "Empty";})) {
                    i += (2 * in_play_creatures.size());
                    //cout << "hand empty ";
                } else if (all_of(in_play_creatures.begin(), in_play_creatures.end(), [](string card) {return card != "Empty";})) {
                    i += (2 * in_play_creatures.size());
                    //cout << "hand full ";
                } else if (in_play_creatures[i-1] != "Empty" && in_play_creatures[i] != "Empty") {
                    //cout << i << " all full, proceed\n";
                    i++;
                } else if (in_play_creatures[i-1] != "Empty" && in_play_creatures[i] == "Empty") {
                    //cout << i << " found the first empty, proceeding\n";
                    first_empty_idx = i;
                    i++;
                } else if (in_play_creatures[i-1] == "Empty" && in_play_creatures[i] == "Empty") {
                    //cout << i << " all empty proceed\n";
                    i++;
                } else if (in_play_creatures[i-1] == "Empty" && in_play_creatures[i] != "Empty") {
                    //cout << i << " found one out of order, reordering\n";
                    in_play_creatures[first_empty_idx] = in_play_creatures[i];
                    in_play_creatures[i] = "Empty";
                    i = first_empty_idx + 1;
                } else {
                    cout << "idk what happened the sorter is fucked";
                };
            };
        };
        void draw(Deck* source_deck_pointer) {
            int i = 0;
            int j = 0;
            while (i < source_deck_pointer->draw_pile.size()) {
                if (source_deck_pointer->draw_pile[i] != "Empty") {
                    while (j < hand_cards.size()) {
                        if (hand_cards[j] == "Empty") {
                            hand_cards[j] = source_deck_pointer->draw_pile[i];
                            source_deck_pointer->draw_pile[i] = "Empty";
                            i += (2 * source_deck_pointer->draw_pile.size());
                            j += (2 * hand_cards.size());
                        } else {j++;};
                    };
                } else {i++;};
            };
            if (i == source_deck_pointer->draw_pile.size()) {
                cout << "deck empty!";
            };
        };
        void draw_starting_hand(Deck* source_deck_pointer) {
            for (int k = 0; k < 5; k++) {
                draw(source_deck_pointer);
            };
        };
        void messy_discard(Deck* source_deck_pointer, int card_idx) {
            //this discards but does not sort the hand, can be used if you want to mass discard and only sort once at the end
            int i = (source_deck_pointer->discard_pile.size() - 1);
            while (i > 0) {
                if (source_deck_pointer->discard_pile[i] == "Empty") {
                    source_deck_pointer->discard_pile[i] = hand_cards[card_idx];
                    hand_cards[card_idx] = "Empty";
                    i -= (2 * source_deck_pointer->discard_pile.size());
                    i = -1;
                } else {i--;};
            };
        };
        void discard(Deck* source_deck_pointer, int card_idx) {
            int i = (source_deck_pointer->discard_pile.size() - 1);
            while (i > 0) {
                if (source_deck_pointer->discard_pile[i] == "Empty") {
                    source_deck_pointer->discard_pile[i] = hand_cards[card_idx];
                    hand_cards[card_idx] = "Empty";
                    i -= (2 * source_deck_pointer->discard_pile.size());
                    i = -1;
                } else {i--;};
            };
            sort_hand();
        };
        void play_card(map<string, map<string, string>> cardTags, int card_idx) {
            if (cardTags[hand_cards[card_idx]]["Type"] == "Creature") {
                int i = 0;
                while (i < in_play_creatures.size()) {
                    if (in_play_creatures[i] == "Empty") {
                        in_play_creatures[i] = hand_cards[card_idx];
                        hand_cards[card_idx] = "Empty";
                        sort_hand();
                        i += (2 * in_play_creatures.size());
                    } else {i++;};
                };
            } else if (cardTags[hand_cards[card_idx]]["Type"] == "Land") {
                int i = 0;
                while (i < in_play_lands.size()) {
                    if (in_play_lands[i] == "Empty") {
                        in_play_lands[i] = hand_cards[card_idx];
                        hand_cards[card_idx] = "Empty";
                        sort_hand();
                        i += (2 * in_play_lands.size());
                    } else {i++;};
                };
            }
            
        };
        void messy_remove_from_play(Deck* source_deck_pointer, int card_idx) {
            //this removes from play but does not sort the play field, can be used if you want to mass discard and only sort once at the end
            int i = (source_deck_pointer->discard_pile.size() - 1);
            while (i > 0) {
                if (source_deck_pointer->discard_pile[i] == "Empty") {
                    source_deck_pointer->discard_pile[i] = in_play_creatures[card_idx];
                    in_play_creatures[card_idx] = "Empty";
                    i -= (2 * source_deck_pointer->discard_pile.size());
                    i = -1;
                } else {i--;};
            };
        };
        void remove_from_play(Deck* source_deck_pointer, int card_idx) {
            int i = (source_deck_pointer->discard_pile.size() - 1);
            while (i > 0) {
                if (source_deck_pointer->discard_pile[i] == "Empty") {
                    source_deck_pointer->discard_pile[i] = in_play_creatures[card_idx];
                    in_play_creatures[card_idx] = "Empty";
                    i -= (2 * source_deck_pointer->discard_pile.size());
                    i = -1;
                } else {i--;};
            };
            sort_in_play_creatures();
        };
};

int main(){
    Deck deck("base");
    cout << "currrent top card: " << deck.card_tags[deck.draw_pile[0]]["DisplayName"];
    cout << "\nIts ap and hp: " << deck.card_stats[deck.draw_pile[0]]["ap"] << " " << deck.card_stats[deck.draw_pile[0]]["hp"] << "\nshuffling\n";
    deck.shuffle_draw_pile();
    cout << "current top card: " << deck.card_tags[deck.draw_pile[3]]["DisplayName"];
    cout << "\nIts ap and hp: " << deck.card_stats[deck.draw_pile[0]]["ap"] << " " << deck.card_stats[deck.draw_pile[0]]["hp"] << "\n \n";
    cout << "drawing starting hand" << "\n";
    Hand hand(1);
    hand.draw_starting_hand(&deck);
    cout << "discarding twice" << "\n";
    hand.discard(&deck, 3);
    hand.discard(&deck, 0);
    cout << "playing a card" << "\n";
    hand.play_card(deck.card_tags, 2);
    cout << "\nfinal hand and deck state\nPlay: ";
    for (int p = 0; p < hand.in_play_creatures.size(); p++) {
        cout << hand.in_play_creatures[p] << " ";
    };
    cout << "\nHand: ";
    for (int i = 0; i < hand.hand_cards.size(); i++) {
        cout << hand.hand_cards[i] << " ";
    };
    cout << "\nDeck: ";
    for (int j =0; j < deck.draw_pile.size(); j++) {
        cout << deck.draw_pile[j] << " ";
    };
    cout << "\nDiscard: ";
    for (int k =0; k < deck.discard_pile.size(); k++) {
        cout << deck.discard_pile[k] << " ";
    };
};

The cards header file:

#ifndef Cards
#define Cards
#include <map>
#include <iostream>
using namespace std;

class Played_card{
    public:
        int owner_id;
        string name;
        string display_name;
        int charge_rating;
        int hp;
        int ap;
        bool swiftness;
        Played_card(int owner, map<string, string> tags, map<string, int> stats, map<string, bool> abilities) {
            owner_id = owner;
            name = tags["Name"];
            display_name = tags["DisplayName"];
            charge_rating = stats["ChargeRating"];
            hp = stats["hp"];
            ap = stats["ap"];
        };
};

void base_cards(map<string, map<string, string>>* tags_loc, map<string, map<string, int>>* stats_loc, map<string, map<string, bool>>* abilities_loc) {
    //Initializing maps
    map<string, map<string, string>> base_card_tags;
    map<string, map<string, int>> base_card_stats;
    map<string, map<string, bool>> base_card_abilities;

    //Imputing card info
    base_card_tags["minion"] = {{"Name", "minion"}, {"DisplayName", "Minion"}, {"Type", "Creature"}, {"Domain", "Minion"}, {"Affiliation", "Minion Hoard"}, {"Element", ""}};
    base_card_stats["minion"] = {{"ChargeRating", 1}, {"ap", 1}, {"hp", 1}};
    base_card_abilities["minion"] = {{"Swiftness", false}};

    base_card_tags["super_minion"] = {{"Name", "super_minion"}, {"DisplayName", "Super-Minion"}, {"Type", "Creature"}, {"Domain", "Minion"}, {"Affiliation", "Minion Hoard"}, {"Element", ""}};
    base_card_stats["super_minion"] = {{"ChargeRating", 3}, {"ap", 3}, {"hp", 3}};
    base_card_abilities["super_minion"] = {{"Swiftness", false}};

    //Anchoring maps to proper locations
    *tags_loc = base_card_tags;
    *stats_loc = base_card_stats;
    *abilities_loc = base_card_abilities;
};
#endif
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

The obvious problem that jumps out at me is this:

using namespace std;

It's especially harmful when used in a header file, because this affects the global namespace of every translation unit which includes that header. The harm occurs when we think we are using a function we declared (e.g. begin()) and get a surprise when the compiler finds a better match from the included std namespace.

Never include a whole namespace in a header file like that. I recommend removing the using from your implementation file too, for the same reason. Instead, either write the names in full (the std:: prefix is intentionally very short), or bring in just the names you need, into the smallest reasonable scope (e.g. at function level), not at file scope.

\$\endgroup\$
2
  • \$\begingroup\$ Oh thanks that makes a lot of sense, I'll do that :) \$\endgroup\$
    – Morgan
    Aug 22, 2022 at 17:19
  • \$\begingroup\$ Further explanation can be found on Stack Overflow. \$\endgroup\$ Aug 22, 2022 at 17:22
0
\$\begingroup\$

Minor stylistic things

  • adopt a class naming style of Card, Deck,... ie upper cased words,
  • adopt a nameing convention for member fields of a class. I cannot tell by looking at 'foo' if its a member or not. This will save a lot of pain later. Common ones are to prepend m_ or just _ or append _

My overall feedback is that yuo are using maps of string to int, boo, string when what you are really doing is creating types with fields.

Also dont use magic strings (Empty, Creature, ... etc), use enums

enum class CardType {Creature, Hero}; //hero ?? other types

struct CardDefinition{ string _name; string _displayName; CardType _type; string _domain; int _chargeRating; int _hitPoints; ..... }

then do

 map<string,CardDefinition> cardefs;
 
 CardDefinition minion;
 minion.CardType = CardType::Creature;
 minion._name = "minion";
 minion._displayName = "minion";
 minion._hitPoints = 1;
 //etc
  
 cardefs["minion"] = minion;
  
 
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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