Thread: 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.

(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. ;-)

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% 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.