This may be a mess, but wondering if it is safe to use

This is a discussion on This may be a mess, but wondering if it is safe to use within the C++ Programming forums, part of the General Programming Boards category; This a singleton Class for managing GameStates. (I know it's game programming, but it is also all C++ as well) ...

  1. #1
    Registered User
    Join Date
    Nov 2005
    Posts
    673

    This may be a mess, but wondering if it is safe to use

    This a singleton Class for managing GameStates. (I know it's game programming, but it is also all C++ as well)
    This is the class. Notice: it is all inline so it may be hard to follow, and I apologize ahead of time.
    Code:
    #include "gHge.h"
    #include <map>
    
    //This has to be the Base class for every gamestate class
    class GameState
    {
    public:
        virtual bool Initiate() = 0;
        virtual bool Shutdown() = 0;
        virtual bool Update(float delta) = 0;
        virtual bool Render() = 0;
    };
    
    class gsMgr
    {
    public:
        gsMgr():curState(0){};
        ~gsMgr()
        {
            //Free all states
            std::map<size_t, GameState*>::iterator iter;
            for ( iter = GameStates.begin(); iter != GameStates.end(); ++iter )
                FreeState((*iter).first);
        };
    
        //Add a state to the stateMgr
        void CreateState(size_t index, GameState* StatePtr)
        {
            if ( GameStates.find(index) != GameStates.end() )
            {
                if ( GameStates[index] != 0 )
                {
                    hge::Ref()->System_Log("Error: GameStates[&#37;d] already exists.\n",index);
                    //Free memory from pointer if it is not needed.
                    if ( StatePtr != 0 )
                    {
                        delete StatePtr;
                        StatePtr = 0;
                    };
                    return;
                }
            }
            GameStates[index] = StatePtr;
            hge::Ref()->System_Log("Passed: GameStates[%d] added.\n",index);
        };
    
        bool InitiateState(size_t index)
        {
            if ( GameStates.find(index) != GameStates.end() )
            {
                if ( GameStates[index] != 0 )
                {
                    if ( GameStates[index]->Initiate() )
                    {
                        hge::Ref()->System_Log("Passed: Initiate GameStates[%d].\n",index);
                        return true;
                    }
                    else
                    {
                        hge::Ref()->System_Log("Error: Initiate GameStates[%d] failed.\n",index);
                        return false;
                    }
                }
                else
                {
                    hge::Ref()->System_Log("Error: Attempted to initiate <null> GameStates[%d].\n",index);
                    return false;
                }
            }
            hge::Ref()->System_Log("Error: Attempted to initiate non-existant GameStates[%d].\n",index);
            return false;
        };
    
        //Call shutdown method in GameStates[index]. Does NOT delete state.
        bool ShutdownState(size_t index)
        {
            if ( GameStates.find(index) != GameStates.end() )
            {
                if ( GameStates[index] != 0 )
                {
                    if ( GameStates[index]->Shutdown() )
                    {
                        hge::Ref()->System_Log("Passed: Shutdown GameStates[%d].\n",index);
                        return true;
                    }
                    else
                    {
                        hge::Ref()->System_Log("Error: Shutdown GameStates[%d] failed.\n",index);
                        return false;
                    }
                }
                else
                {
                    hge::Ref()->System_Log("Error: Attempted to shutdown <null> GameStates[%d].\n", index);
                    return false;
                }
            }
            hge::Ref()->System_Log("Error: Attempted to shutdown non-existant GameStates[%d].\n",index);
            return false;
        };
    
        //Shutdown the state. Delete the state, and set value to 0
        void FreeState(size_t index)
        {
            if ( GameStates.find(index) != GameStates.end() )
            {
                if ( GameStates[index] != 0 )
                {
                    //Run shutdown function
                    ShutdownState(index);
                    //Free pointer memory
                    delete GameStates[index];
                    //Set pointer value to 0
                    GameStates[index] = 0;
                    hge::Ref()->System_Log("Passed: Freed GameStates[%d].\n", index);
                    return;
                }
                else
                {
                    hge::Ref()->System_Log("Error: Attempted to free <null> GameStates[%d].\n",index);
                    return;
                }
            }
            hge::Ref()->System_Log("Error: Attempted to free non-existant GameStates[%d].\n",index);
        };
    
        int SetState(size_t index)
        {
            if ( GameStates.find(index) != GameStates.end() )
            {
                if ( GameStates[index] != 0 )
                {
                    if ( curState != index )
                    {
                        ShutdownState(curState);
                        hge::Ref()->System_Log("Passed: Switched GameState from [%d] to [%d].\n",curState,index);
                        curState = index;
                        InitiateState(curState);
                        return 0;
                    }
                    else
                    {
                        InitiateState(curState);
                        return 0;
                    }
                }
                else
                {
                    hge::Ref()->System_Log("Failed: Requested index is <null>. Finding a valid Index.\n");
                }
            }
            //Try to find a valid index
            long t_ = FindValidState_();
            if ( t_ != -1 )
            {
                hge::Ref()->System_Log("Passed: Found index[%d].\n",t_);
                ShutdownState(curState);
                curState = t_;
                InitiateState(curState);
                return 1;
            }
            else
            {
                hge::Ref()->System_Log("Failed: No valid States. Fatal Error.\n");
                return -1;
            }
        };
    
        //Update the current gamestate
        bool UpdateState(float delta)
        {
            if (GameStates[curState] != 0)
            {
                //Update
                if ( GameStates[curState]->Update(delta) )
                {
                    //Nothing bad happened
                    return true;
                }
                else
                {
                    //Error in update
                    return false;
                }
            }
            //Error invalid state. Lots of bad things can happen if this does.
            return false;
        };
    
        bool RenderState()
        {
            if (GameStates[curState] != 0 )
            {
                //Render
                if ( GameStates[curState]->Render() )
                {
                    //Nothing bad happened
                    return true;
                }
                else
                {
                    //Error in rendering
                    return false;
                }
            }
            //Error invalid state. Lots of bad things can happen if this does.
            return false;
        };
    
    
        //Singleton functions
        static gsMgr& Ref()
        {
            if (!singleton)
            {
                hge::Ref()->System_Log("Initiated Singleton gsMgr class.\n");
                singleton = new gsMgr();
            }
            return *singleton;
        };
    
        static gsMgr*& Ptr()
        {
            if (!singleton)
            {
                hge::Ref()->System_Log("Initiated Singleton gsMgr class.\n");
                singleton = new gsMgr();
            }
            return singleton;
        };
    private:
        //The single StateMgr
        static gsMgr* singleton;
    
        //The index for the current GameState
        size_t curState;
    
        //The container of all GameStates
        std::map<size_t, GameState*> GameStates;
    
        //This function is used to find a valid state if the specified is not valid.
        //This is called by the SetState() function if it fails.
        //Return index to first found valid state. Else returns -1
        long FindValidState_()
        {
            std::map<size_t, GameState*>::iterator iter;
            for ( iter = GameStates.begin(); iter != GameStates.end(); ++iter )
            {
                if ((*iter).second != NULL )
                    return (*iter).first;
            };
            return -1;
        };
    };
    
    gsMgr* gsMgr::singleton = 0;
    Well let me know what you think. Thank you again. I just want to make sure it is safe to use before I use it large scale.
    Last edited by Raigne; 03-30-2008 at 07:25 PM.

  2. #2
    Registered User
    Join Date
    Oct 2004
    Posts
    151
    (I know it's game programming, but it is also all C++ as well)
    We actually have a board for game programming.

    I didn't read the code, but the smart-alecky summary would pretty much go like:

    This a singleton Class for managing GameStates.
    "Singleton Considered Harmful etc. etc.".

    it is all inline so it may be hard to follow
    "Premature Optimization is the Root of all Evil etc. etc."

    PHP
    Misfire. ;-)
    System: Debian Sid and FreeBSD 7.0. Both with GCC 4.3.

    Useful resources:
    comp.lang.c FAQ | C++ FQA Lite

  3. #3
    Registered User
    Join Date
    Nov 2005
    Posts
    673
    I'm not new to the boards. Yes I know there is a game programming board. I did not gear this question toward game programming so that really doesn't matter. PHP was used to easily differentiate between string literals, and code.
    I am asking about the code, not my conduct, nor my style.
    I want to know if there is something I am missing. Hanging pointers that sort of thing.
    Singleton's are not harmful if used correctly.
    I see no premature optimization here. The fact of it all being inline is the compiler's decision. I personally find it easier to work with when definition an implementation are together.

  4. #4
    Super Moderator VirtualAce's Avatar
    Join Date
    Aug 2001
    Posts
    9,598
    Quote:
    This a singleton Class for managing GameStates.

    "Singleton Considered Harmful etc. etc.".


    Quote:
    it is all inline so it may be hard to follow

    "Premature Optimization is the Root of all Evil etc. etc."


    Quote:
    PHP

    Misfire. ;-)
    Not very instructive criticisms and not very accurate either.

    The debate between using inline and not using inline is a moot point IMO. The debate over Singletons also moot as well since proper use of them is perfectly valid in object oriented code.

    Overall I think it looks good, is easy to follow, and has fairly simple logic. There are a couple of things I notice right off hand:

    • Not sure why you are checking for index !=0 in SetState(). map::find will return an iterator to the item found or it returns map::end(). If you use the iterator instead of the index you can eliminate the second if block altogether. If find returns end() you know that an invalid index or invalid ID was specified.
    • The inner if block inside CreateState has a trailing semicolon after the closing brace for the block. This may cause compilation issues.
    • Your singleton uses lazy instantiation which is perfectly fine if you know the exact order of creation of your Singletons. If any object could change that order then perhaps another type of instantiation is in order. With lazy instantiation it is possible another object that uses the Singleton could create it earlier than desired. However this may or may not be a problem in your system and is just a precaution that may or may not be needed.
    • FindValidState relies on a magic return value of -1. Perhaps a typedef or a clearer return value might be better.
    • The code returns from 4 places in InitiateState. Perhaps a ret_val variable and then a return ret_val at the end would be easier to follow. Just a minor suggestion. Nothing wrong with early out returns and all of us do it but in general one entry and one exit is best.
    • Since the functions are inlined it would be better to have the inline functions appear after the class declaration. This makes the code much easier to read. The larger functions probably should not be inlined since most likely the compiler will be unable to do so. The rules for when the compiler can or cannot inline are quite complex but in general only the simplest of functions should be inlined.
    • Some of your local variables have names like t_ and other one or two letter names. Prefer to stay away from this type of naming. The more descriptive the better.
    • This is a style issue but perhaps prefixing member variables with m_ may help identify member variables. I'm not saying you have to go 100&#37; hungarian notation here but it does have a few good naming conventions. Personally, I like the m_ for class members, s_ for statics, and g_ for globals.


    There may be more issues in the actual class logic but I do not have the working code so I'm unable to test that. What I try to concentrate on are language issues and common gotchas.
    Last edited by VirtualAce; 03-30-2008 at 11:38 PM.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. thread safe in my code to manipulate List?
    By George2 in forum C# Programming
    Replies: 8
    Last Post: 05-01-2008, 07:57 AM
  2. Bjarne's exception safe sample
    By George2 in forum C++ Programming
    Replies: 13
    Last Post: 12-28-2007, 05:38 PM
  3. A Safe Dialect of C
    By viaxd in forum Tech Board
    Replies: 11
    Last Post: 11-26-2003, 11:14 AM
  4. How safe is it?
    By hermit in forum A Brief History of Cprogramming.com
    Replies: 40
    Last Post: 05-08-2002, 10:33 PM
  5. Safe Mode on FreeBsd
    By Unregistered in forum A Brief History of Cprogramming.com
    Replies: 1
    Last Post: 10-25-2001, 10:37 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21