Question about engine design.

Hey fellow programmers, for a while I've been working on this Blackjack project and it seems that no matter how close to completion I come I always see design flaws in my code. Right now I have my classes setup so that the Player class owns the draw function, the betting function, and a function for splitting a hand. The deck class has a method for shuffling the deck. Basically I feel like I have a bunch of functions that might be a little out of place. Not to mention how my gamestate classes look, they're a pile of jumbled code that look like this:

Code:

---

void Play_State::handle_events(State_Manager * state_manager, Card_Game * game)
{
        using namespace std;
        while(!(cin >> m_result))
        {
                cin.clear();
                cin.ignore();
        }

        switch(m_result)
        {       
        case 1: // draw a card
                game->m_players[1].draw(game->m_players[1].m_active_hand, *(game->m_deck), 1);
                if (game->m_players[1].m_player_hands[game->m_players[1].m_active_hand].value() > 21)
                {
                        game->winner() = false;
                        state_manager->change_state( Deal_State::instance(), game );
                }
                break;
        case 2: // stay hand
                // while dealer has hand value less than 18
                while(game->m_players[0].m_player_hands[0].value() < 18)
                {
                        // draw cards for the dealer
                        game->m_players[0].draw(game->m_players[0].m_active_hand, *(game->m_deck), 1);
                }
                // if the dealer goes over 21
                if (game->m_players[0].m_player_hands[0].value() > 21)
                {
                        // player wins
                        game->winner() = true;
                        state_manager->change_state( Deal_State::instance(), game );
                }
                // if player's hand value is greater than the dealer's
                else if(game->m_players[0].m_player_hands[0].value() < game->m_players[1].m_player_hands[game->m_players[1].m_active_hand].value()) 
                {
                        // player wins
                        game->winner() = true;
                        state_manager->change_state( Deal_State::instance(), game );
                }
                else
                {
                        // player loses
                        game->winner() = false;
                        state_manager->change_state( Deal_State::instance(), game );
                }
                break;
        case 3: // double down?
                if (game->m_players[1].m_player_hands[game->m_players[1].m_active_hand].m_card_list.size() == 2)
                {
                        game->m_players[1].bet(game->m_players[1].m_bets[game->m_players[1].m_active_hand]);
                        game->m_players[1].draw(game->m_players[0].m_active_hand, *(game->m_deck), 1);
                        if(game->m_players[0].m_player_hands[0].value() < game->m_players[1].m_player_hands[1].value()) 
                        {
                                // player wins
                                game->winner() = true;
                                state_manager->change_state( Deal_State::instance(), game );
                        }
                        else
                        {
                                // player loses
                                game->winner() = false;
                                state_manager->change_state( Deal_State::instance(), game );
                        }
                }
                else { break; }
        case 4:// split hand
                break;
        case 5: // fold
                // player loses
                game->winner() = false;
                state_manager->change_state( Deal_State::instance(), game );
                break;
        default:// invalid entry
                cout << "Please enter 1 or 2.\n";
                break;
        }
}

---

Essentially I see alot of this code could be conformed into functions in the actual game engine (Card_Game class). I could change up my draw function to require a parameter of player, hand, and deck, and move it to the engine. I think things would be alot neater and maybe a little more logically and object oriented design wise.

Take a look at my print function for the play state, its oogly:

Code:

---

void Play_State::print(State_Manager * state_manager, Card_Game * game)
{
        using namespace std;
        cout << game->m_players[0].show_hand(0);
       
        for( unsigned int loop = 0; loop < game->m_players[1].m_player_hands.size(); loop++)
        {
                cout << game->m_players[1].show_hand(loop);
        }
        if (game->m_betting_toggle == true)
        {
                cout << "The current jackpot is " << game->m_players[1].m_bets[game->m_players[1].m_active_hand] << " dollars!\n";
        }
        cout << "\nWhat would you like to do?\n";
        cout << "1. Hit me\n";
        cout << "2. Stay\n";
        if (game->m_players[1].m_player_hands[game->m_players[1].m_active_hand].m_card_list.size() == 2)
        {
                cout << "3. Double down\n";
        }
        cout << "4. Fold\n";
}

---

Should another system entirely handle output data? Like game->output->display_hands.

Any input would be greatly appreciated!

BTW, this is how I want to pass game logic to the entire system:

Code:

---

// Main.cpp : Defines the entry point for the console application.
//
#include "stdafx.h"
#include <time.h>
#include "Game.h"
#include "State_Manager.h"
#include "Menu_State.h"

int _tmain(int argc, _TCHAR* argv[])
{
        srand ( unsigned(time(NULL)) );
        Card_Game * game = new Card_Game();
        game->init();

        State_Manager state_manager;
        state_manager.change_state( Menu_State::instance(), game );

        while ( state_manager.running() )
        {
                state_manager.update(game);
                state_manager.print(game);
                state_manager.handle_events(game);
        }

        game->cleanup();
    return 0;
}

---

As you see I create a pointer to the game logic, and pass it into the state manager so that all the gamestates can use the game logic. I'm thinking that the Card_Game class should hold the functions to draw cards and shuffle the deck and whatnot, and not the player/deck class. I think this because they are methods universal to all card games. I'm thinking that the deck class and player class are more so data objects than classes with actual methods.

I want to be able to go game->draw(player id, hand id, deck id, num cards);
This action would be performed in the Handle_Events functions.
Since the players, hands, and decks are all held in the Card_Game class it would look something like this:

game->draw(game->player_id, game->hand_id, game->deck_id, game->num_cards);

I think I should check if the hand is over 21 in the Update function, it could be a constant check at the end of every event (in the play_game state). I suppose I could move all my state altering checks in the update function.

The main idea of all this fancy state management and passing game logic to the state manager is exactly that, so I can instead pass logic for a holdem game instead of a blackjack game! Talk about modularity!

I'll show some more source code while I'm at it, I'd like to discuss alot of things about my work in progress.

Here is the deck:

Code:

---

// This header file declares the deck class and all of its functions,
// as well as the card structure.
#ifndef DECK_H
#define DECK_H
#include "Card.h"
#include <vector>

class Deck
{
public:       
        Deck();
        void shuffle_deck();
        std::vector<Card> m_cards;

};
#endif

// Deck source code
#include "stdafx.h"
#include "Deck.h"
#include <cassert>
#include <stdio.h>
#include <stdlib.h>
#include <algorithm>

Deck::Deck()
{
        unsigned int x = 0;
        unsigned int y = 0;
        unsigned int z = 0;
                for (y = 0; y < 4;  y++)
                {
                       
                        for (x = 0; x < 13; x++)
                        {
                                Card c(y,x);
                                m_cards.push_back(c);
                        }
                }
                assert(m_cards.size() == 52);
}


void Deck::shuffle_deck()
{
        random_shuffle(m_cards.begin(), m_cards.end());
}

---

The deck class is pretty self explanatory, I'm thinking the deck should not be shuffling itself but that should be a method of the engine itself, This would make the deck a pure data object (which in reality it is).

And here is the player class:

Code:

---

#ifndef PLAYER_H
#define PLAYER_H
#include "Deck.h"
#include "Hand.h"
#include <string>

class Hand;

class Player
{
public:
        Player(int p_player_id);
        void new_hand();
        void draw(int hand_index, Deck * deck, int number);
        void split_hand(Hand * hand_to_split, Hand * new_hand);
        void bet(int bet);
        void clear_hand();
        std::string & show_hand(int hand_index);

        int m_player_id;
        int m_cash;
        int m_active_hand;

        std::vector<int> m_bets;
        std::vector<Hand> m_player_hands;

};
#endif

//Player source code
#include "stdafx.h"
#include "Player.h"
#include "Hand.h"
#include <cassert>
Player::Player(int p_player_id)
{
        m_player_id = p_player_id;
        m_cash = 100;
        m_active_hand = 0;
}

void Player::new_hand()
{
        Hand new_hand;
        m_player_hands.push_back(new_hand);
}

void Player::draw(int hand_index, Deck * deck, int number)
{
        for(int loop = 0; loop < number; loop++)
        {       
                /* place card in player's hand */
                m_player_hands.at(hand_index).m_card_list.push_back(deck->m_cards.back());

                /* hide all dealer cards except for the first one */
                if (m_player_id == 0 && loop != 0)
                {
                        m_player_hands[0].m_card_list[loop].flip_card(false);
                }
                /* remove card from deck */
                deck->m_cards.pop_back();
        }
}

void Player::split_hand(Hand * hand_to_split, Hand * new_hand)
{
        new_hand->m_card_list.push_back(hand_to_split->m_card_list.back());
        hand_to_split->m_card_list.pop_back();
}

void Player::clear_hand()
{
        for(unsigned int loop = 0; loop < m_player_hands.size(); loop++ )
        {
                m_player_hands[loop].m_card_list.clear();
        }
}

std::string & Player::show_hand(int hand_index)
{
        m_player_hands[hand_index].m_hand_string.erase();
        if (m_player_id == 0)
        {
                m_player_hands[hand_index].m_hand_string.append("Dealer's Cards\n");
                m_player_hands[hand_index].m_hand_string.append("--------------\n");
        }
        else if (m_player_id != 0)
        {
                m_player_hands[hand_index].m_hand_string.append("Player's Cards\n");
                m_player_hands[hand_index].m_hand_string.append("--------------\n");

        }

        for(unsigned int loop = 0; loop < m_player_hands[m_active_hand].m_card_list.size(); loop++)
        {
                m_player_hands[hand_index].m_hand_string.append(m_player_hands[hand_index].m_card_list[loop].card_name());
                m_player_hands[hand_index].m_hand_string.append("        ");
        }
        m_player_hands[hand_index].m_hand_string.append("\n");
        m_player_hands[hand_index].m_hand_string.append("--------------\n\n");
        return m_player_hands[hand_index].m_hand_string;
}

void Player::bet(int bet)
{
        m_cash -= bet;
        m_bets.push_back(bet);
}

---

The player class is a bit more iffy. As you see I have it containing a vector of hands that the player owns, Should that even be there? I'm thinking that maybe I should manage my hands seperately and just keep the player as a pure data object as well. I'm thinking I should move the draw function into the game engine, as well as the split hand fucntion, as well as pretty much all the functions in the player class.

The game engine could then just keep track of the players as well as their correpsonding hands (it seems that if I kept the hand in the player class that things could be a little more automated, but perhaps less flexible and modular.

Really I have an endless supply of design questions about my card game engine that i pumped out in the last 3 months.

this piece of code will generate a randomized deck only once (not random)

Code:

---

random_shuffle(m_cards.begin(), m_cards.end());

---

you should first use the srand function :)

Code:

---

srand((unsigned int)time(0))
random_shuffle(m_cards.begin, m_card.end);

---

also, i have a note about this :

Code:

---

Hand new_hand;
        m_player_hands.push_back(new_hand);

---

i don't advise adding a copy of an object, specially if its size is really big, to a vector... it's an expensive assignment. Rather, you should have a vector of pointers of Hand like this :

Code:

---

std::vector<Hand*> m_vpHand;
Hand*                      m_pHand;
//..........

void Player::AddHand
{
        m_pHand = new Hand(/*constructor*/);
        m_vpHand.push_back(m_pHand);
}

---

as you see, everytime you call this function, you allocate memory for a new hand. You dont have to worry about any memory leaks since all the pointers are saved in the vector every call, that is, they are not lost :).
For a more robust application, if the number of hands is limited, for example: the maximum limit for hands is 20, what you wanna do is (preferably in the constructor)

Code:

---

m_vpHand.reserve(20);

---

so the vector allocates 10 spaces for the pointers instead of resizing everytime you add a pointer, and also because vectors capacity doubles everytime you add something to it after exceeding the current capacity, so there would be an unneeded allocated memory :) <--------- performance obsession :S

Code:

---

...
assert(m_cards.size() == 52);
}
...

---

Remember to be extremely careful when using assert(). This line of code does nothing in release mode. So if you are using this as your only check of the size of the deck that is a very dangerous thing.

Nope nope bubbah, I used that assert to clear up a bug regarding the deck size. I couldn't figure out my loops in the beginning lol :d

I would not be so concerned with using dynamic memory as I see no benefit by using it in your current design. The objects you will be copying are so small as to be unimportant performance wise and since this is a text based blackjack game I highly doubt performance is even an issue.

I would make the deck know how to 'shuffle' itself as I feel this is an operation that the deck should be responsible for. The game object should be more concerned with how the game flows and the logic involved in it. Delegating the shuffle responsibility to the game object and not the deck seems appears to me to be a misstep.

However it is your project and this is just my two cents.

I've moved a few functions into the game class, I decided to use my old resource manager so I don't have to manage pointers.

Code:

---

#ifndef GAME_H
#define GAME_H
#include <vector>
#include "Player.h"
#include "Hand.h"
#include "Resource_Manager.h"

class Card_Game
{
public:
        void init();
        void cleanup();

        Card_Game();

        int calculate_winnings();

        void initialize_players(std::string player_id);
        // Give player id a new hand
        void give_player_hand(std::string player_id)
        {
                Hand * p_hand = new Hand();
                m_rHands.Request_Resource(player_id).px->push_back(p_hand);
        };

        // Draw how_many cards to player id's hand_id from deck_id
        void draw_card(std::string player_id, int hand_id, int deck_id, int how_many)
        {
                for(int loop = 0; loop < how_many; loop++)
                {       
                        m_rHands.Request_Resource(player_id).px->at(hand_id)->m_card_list.push_back(m_vDeck[deck_id]->m_cards.back());
                        m_vDeck[deck_id]->m_cards.pop_back();
                }
        };
        // Split player's active hand
        void split_hand(std::string player_id, int hand_id)
        {
                give_player_hand(player_id);
                m_rHands.Request_Resource(player_id).px->back()->m_card_list.push_back(m_rHands.Request_Resource(player_id).px->at(hand_id)->m_card_list.back());
                m_rHands.Request_Resource(player_id).px->at(hand_id)->m_card_list.pop_back();
        }



        bool & winner();    // function to find the winner       
        int  & minimum_bet(); // function to get the minimum bet


private:

        Resource_Manager< Player > m_rPlayers;
        Resource_Manager< std::vector<Hand*> > m_rHands;
        std::vector< Deck* > m_vDeck;
        int  m_minimum_bet;
        bool m_winner;       
        bool m_load_new_players;
        bool m_betting_toggle;
};
#endif

---

Those functions will not stay public, I'm going to create a pure virtual turn function that contains turn logic, which I will pass a player ID to work.

One advice about pointers: smart pointers or a memory manager.

Does this count as smart pointers/memory managers?

Code:

---

#ifndef RESOURCE_MANAGER_H
#define RESOURCE_MANAGER_H

#include <vector>
#include <map>
#include <string>
#include <boost\shared_ptr.hpp>
#include <boost\weak_ptr.hpp>

template< typename T_ >
class Resource_Manager
{ 

public:

        typedef T_ value_type; // std library convention

        typedef boost::shared_ptr<T_> Resource_Ptr;
        typedef boost::weak_ptr<T_> Resource_Observer;
        typedef std::map< std::string, Resource_Ptr > Resource_Map;
        Resource_Manager<T_>() {};
        ~Resource_Manager<T_>() {};

        Resource_Observer Request_Resource(const std::string & name)
        {
                Resource_Map::iterator  it = mResources.find(name);

                if (it == mResources.end())
                {
                        Resource_Ptr Raw_Resource(new T_);
                        mResources.insert(std::make_pair(name, Raw_Resource));
                        Resource_Observer Resource(Raw_Resource);
                        return Resource;
                }
                else
                {
                        return Resource_Observer(it->second);
                }
        }

        void Request_Resource_Removal(const std::string & name)
        {
                Resource_Map::iterator it = mResources.find(name);

                if (it != mResources.end())
                {
                        mResources.erase(it);
                }
        }

private:
        Resource_Map  mResources;
};

#endif

---

I mean its pretty bare minimum but so far it's working quite nicely. I can tie strings to resources, woot.

Well, I haven't used boost, but it looks like it's right!
You don't have to worry about deleting them pointers anymore if you use a shared pointer, with reference count.