Just wondering about safety of my classes...

This is a discussion on Just wondering about safety of my classes... within the Game Programming forums, part of the General Programming Boards category; I have a few classes, which are all pretty short, but are made to do one thing and only one ...

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

    Just wondering about safety of my classes...

    I have a few classes, which are all pretty short, but are made to do one thing and only one thing.

    First off is my graphics class header then impl.
    Code:
    class Graphics
    {
    public:
       Graphics(int windowWidth = 800, int windowHeight = 600, bool fullscreen = false,
                   const char* windowTitle = "",
                   int bgR = 0, int bgG = 0, int bgB = 0);
       ~Graphics();
    
       bool CreateSurfaceFromFile(SDL_Surface*& dst, const char* filename, int tran_r = -1, int tran_g = -1, int tran_b = -1);
       void CloseSurface(SDL_Surface* src);
    
       void DrawSurface(SDL_Surface* src, SDL_Rect* clip = NULL, int x_pos = 0, int y_pos = 0);
    
       void BeginScene() { SDL_FillRect(screen, &screen->clip_rect, SDL_MapRGB(screen->format, r, g, b)); }
       void RenderScene(){ SDL_Flip(screen); }
    
    private:
        SDL_Surface* screen;
    
        int r;
        int g;
        int b;
    };
    Code:
    Graphics::Graphics(int windowWidth, int windowHeight, bool fullscreen, const char *windowTitle, int bgR, int bgG, int bgB)
    {
        if ( SDL_Init(SDL_INIT_TIMER|SDL_INIT_VIDEO) < 0 )
            return;
        
    
        screen = NULL;
        if (fullscreen == true)
        {
            screen = SDL_SetVideoMode(windowWidth,windowHeight, 32, SDL_FULLSCREEN|SDL_HWSURFACE|SDL_DOUBLEBUF|SDL_ANYFORMAT);
        }
        else
        {
            screen = SDL_SetVideoMode(windowWidth, windowHeight, 32,SDL_ANYFORMAT|SDL_HWSURFACE|SDL_DOUBLEBUF);
        }
        SDL_WM_SetCaption(windowTitle,NULL);
        r = bgR;
        g = bgG;
        b = bgB;
    }
    
    Graphics::~Graphics()
    {
        SDL_FreeSurface(screen);
        SDL_Quit();
        screen = NULL;
    }
    
    bool Graphics::CreateSurfaceFromFile(SDL_Surface*& dst, const char *filename, int tran_r, int tran_g, int tran_b)
    {
        //if file name is not empty then attempt to load the surface.
        if (filename!=NULL)
        {
            SDL_Surface* d = NULL;
            d = IMG_Load(filename);
            if ( d != NULL )
                dst = SDL_DisplayFormat(d);
            SDL_FreeSurface(d);
        }
        else
        {
            return false;
        }
    
        if (dst==NULL)
            return false;
    
        if ( tran_r < 0 || tran_g < 0 || tran_b < 0 )
            return true;
        else
        {
            SDL_SetColorKey(dst, SDL_SRCCOLORKEY|SDL_RLEACCEL, SDL_MapRGB(dst->format, tran_r, tran_g, tran_b));
            return true;
        }
        return false;
    }
    
    void Graphics::CloseSurface(SDL_Surface *src)
    {
        SDL_FreeSurface(src);
        src = NULL;
    }
    
    void Graphics::DrawSurface(SDL_Surface* src, SDL_Rect* clip, int x_pos, int y_pos)
    {
        SDL_Rect offset;
        offset.x = x_pos;
        offset.y = y_pos;
    
        SDL_BlitSurface(src,clip,screen,&offset);
    }
    Second is the input class...
    Code:
    class Input
    {
    // Methods
    public:
       Input();
       ~Input();
    
       void readInput();
    
       bool* getInput();
    
       bool get(const int& key) { return m_keysHeld[key]; };
    
       // Check this each frame
       bool windowClosed();
    
    // Data
    private:
       SDL_Event m_event;
       bool m_keysHeld[324];
       bool m_windowClosed;
    };
    Code:
    Input::Input()
    {
       m_windowClosed = false;
    
       // Make sure all the keys are set to false.
       for (int i = 0; i < 323; i++)
       {
          m_keysHeld[i] = false;
       }
    }
    
    Input::~Input()
    {
    }
    
    void Input::readInput()
    {
       while (SDL_PollEvent(&m_event))
       {
          if (m_event.type == SDL_QUIT)
          {
             m_windowClosed = true;
          }
    
          if (m_event.type == SDL_KEYDOWN)
          {
             m_keysHeld[m_event.key.keysym.sym] = true;
          }
    
          if (m_event.type == SDL_KEYUP)
          {
             m_keysHeld[m_event.key.keysym.sym] = false;
          }
       }
    }
    
    bool* Input::getInput()
    {
       return m_keysHeld;
    }
    
    bool Input::windowClosed()
    {
       return m_windowClosed;
    }
    And the only other that is questionable to me is my Surface class
    Code:
    class Surface
    {
    public:
        Surface(const char* FILE):file(FILE) {};
        ~Surface() { SDL_FreeSurface(surface); };
    
        SDL_Surface*& Get() { return surface; }
        const char* GetFile() { return file.c_str(); }
    
    private:
        std::string file;
    
        SDL_Surface* surface;
    };
    They all look good, but i figure someone with more experience will find a flaw, or even a recommendation for better design or functionality. Thanks in advance for any insight.

  2. #2
    Super Moderator VirtualAce's Avatar
    Join Date
    Aug 2001
    Posts
    9,596
    Obvious items:

    • Your classes pollute the global namespace and do not reside in their own namespaces
    • No copy constructor defined or hidden for any of your classes - is this by design?
    • No assignment operator defined or hidden for any of your classes - is this by design?
    • Graphics constructor - what happens if SDL_SetVideoMode() fails? screen could be invalid and yet this is never checked and no exception thrown. Should SDL_SetVideoMode() be done inside the constructor or inside of a separate init() function?
    • Graphics constructor - returning from a constructor? Does this even compile?
    • Graphics constructor - windowTitle is used without checking if it is valid
    • Graphics::CloseSurface() receives a pointer from an external source and does not check the validity of it prior to using it.
    • Graphics::CreateSurfaceFromFile() - hard to tell what true and false return values actually mean.
    • Graphics::DrawSurface() - two pointers from external sources are used without checking for validity
    • Input::getInput() - looks suspect and dangerous
    • Input::get() - uses constant reference when not necessary. If you are passing by value then the value cannot be modified except inside of the function. Since you are not using the value later in the function, making it const does nothing. If you did not want the value changed prior to a function call within the function this would be ok. It would also be ok if you wanted to force a copy of the variable to be made in order to change it. I see no benefit in making it a const reference.
    • Input::m_keysheld array - uses some magic value of 324
    • Inconsistent variable naming scheme used in classes. Graphics does not use the form m_<var> but Input and Surface do.
    • Surface - surface pointer is used but never assigned. Surface pointer is used in destructor but not checked for validity. If no other object can touch this pointer, thus you own it, this should be fine. If at any point you pass this to another object you should check this for validity. If your application is multi-threaded you will want to check for validity.
    • If the application is multi-threaded these classes are not thread safe.


    Code:
    if ( tran_r < 0 || tran_g < 0 || tran_b < 0 )
            return true;
        else
        {
            SDL_SetColorKey(dst, SDL_SRCCOLORKEY|SDL_RLEACCEL, SDL_MapRGB(dst->format, tran_r, tran_g, tran_b));
            return true;
        }
    Both of these return and your other blocks above it return. When is the return false ever going to be used that is at the bottom of the function? Also you could rewrite the above code as:

    Code:
    if ( tran_r >= 0 && tran_g >= 0 && tran_b >= 0 )
    {
            SDL_SetColorKey(dst, SDL_SRCCOLORKEY|SDL_RLEACCEL, SDL_MapRGB(dst->format, tran_r, tran_g, tran_b));
            return true;
    }
    You could ensure that tran_r, tran_g and tran_b are never less than zero by declaring them as unsigned int. Furthermore if you did this you could remove the above if statement altogether since you would be guaranteed they would always be >= 0.

    That's not an exhaustive list by any means.
    Last edited by VirtualAce; 02-27-2008 at 10:10 PM.

  3. #3
    Registered User
    Join Date
    Nov 2005
    Posts
    673
    Thank you bubba. I am going to try and fix those problems then i will repost, and see if it is improved.
    Also the array of 324 has reason. SDL has 323 different input codes, so any real key press will be in bounds.

    The return statement in the constructor does compile. Is it not suppose to? (Visual C++ 2008)
    One last question, how do i make the classes thread safe?
    Last edited by Raigne; 02-27-2008 at 10:20 PM.

  4. #4
    Super Moderator VirtualAce's Avatar
    Join Date
    Aug 2001
    Posts
    9,596
    You are welcome. This is the normal stuff I would find in a code review and I'm not saying all of it has to be or should be changed. In the end it's your decision I'm just pointing out areas that might be of concern.

    Instead of using some value like 323 it may be better to #define it. Many do not like the pre-processor but I feel this is an appropriate use of it. Just using some value like 323 it is not immediately clear from the code what this number represents.

    In order to make your classes thread safe you would have to put guards around access to your pointers. It is possible that two threads could attempt to access your pointers at the same time. If they attempt a write via the pointer this could cause a crash. Or one could attempt a write via the pointer while the other was reading via the pointer. You need to ensure that only one thread has access to the pointer at any time. This would be a very rare condition since the time frame that this could occur in is very small in your current code.

    Another area of concern is the key input array. If two threads access at the same time this could cause an issue. It probably would not die since this is a read operation on the array but what if one thread called get() and another called readInput? Or worse yet what if one thread called getInput() while another was attempting to readInput?

    Also you may want to change the name of getInput() to something like getInputArray() to make it more clear what is really taking place. You could change get() to several functions such as:

    keyDown()
    keyUp()
    keyPressed()

    These function names make it crystal clear as to what is happening in the function. get() is rather vague.

    The fastest and easiest way to make your code safer for threads under win32 is using critical sections. They are used like this:

    Code:
    class MyObject
    {
       public:
          ...
          ...
          ~MyObject();
          void Init();
          void Foo();
       private:
          CRITICAL_SECTION m_CriticalSection;
    
          std::vector<SomeObject> m_ObjectVector;
    };
    
    MyObject::~MyObject()
    {
       DeleteCriticalSection(&m_CriticalSection);
    }
    
    void MyObject::Init()
    {
       InitializeCriticalSection(&m_CriticalSection);
    }
    
    void MyObject::Foo()
    {
          EnterCriticalSection(&m_CriticalSection);
    
          //Do something with vector
          std::vector<SomeObject>::iterator iter = m_ObjectVector.begin();
    
          while (iter != m_ObjectVector.end())
          {
              //Do something with vector
              ... 
              ++iter;
          }
    
          LeaveCriticalSection(&m_CriticalSection);
    }
    Some gotchas:
    • You must call InitializeCriticalSection prior to using it
    • If you call EnterCriticalSection, you must call LeaveCriticalSection. If not the next thread to access the function will hold at EnterCriticalSection since the previous thread still owns the section.
    • Failure to call DeleteCriticalSection on a CRITICAL_SECTION object that has been initialized results in a memory leak
    • Calling DeleteCriticalSection on an invalid CRITICAL_SECTION will throw an exception
    • Entering twice (IE: calling EnterCriticalSection twice in a row) is an error. IE: critical sections do not 'nest'


    According to MS EnterCriticalSection() normally takes 10 CPU instructions to enter. If there is a race condition then a WaitForSingleObject() is called which takes about 100 CPU instructions. Normally it will only take 10 CPU instructions. For more information you can consult Thread Synchronization Objects in the Windows SDK.
    Last edited by VirtualAce; 02-27-2008 at 10:40 PM.

  5. #5
    Registered User
    Join Date
    Nov 2005
    Posts
    673
    Should I wrap all of the classes in one single namespace?
    like
    Code:
    namespace varia
    {
       class Graphics;
       class Input;
       class Surface;
       //and so on?
    }
    or am i misunderstanding?

    I changed the Input::get() function to Input::keyPressed()

    I am also wondering what should i do to check for the validity of the pointers?
    just a simple
    Code:
    if ( <ptr> == NULL )
    Is there any reason why the return shouldn't be in the constructor?

    I know this sounds really beginnerish, but honestly i am so anyhow. Thank you for all the help, and sorry for being a burden.
    Last edited by Raigne; 02-27-2008 at 10:44 PM.

  6. #6
    Super Moderator VirtualAce's Avatar
    Join Date
    Aug 2001
    Posts
    9,596
    Yes you can put them in whatever namespace you want. I normally try to separate them into namespaces that reflect what they do. I would not clump input with graphics since they are two fundamentally different objects. What this does is simply give you another level of protection against name clashes.

    And yes your pointer check is correct. If you own the pointer in that you create it, use it, and no one else can ever use it, you can probably remove the check. But if anything else can access it, they can also delete it or modify it and you then need to check it's validity prior to using it. Not doing this can cause an instant crash. I have to alter my game engine code to check for this same thing so don't feel bad.

    Some of this may lose it's importance if you are the only one that will ever work on your code. But in a group or team setting many of these items are quite important. And I'm by no means a threading expert. I'm just relaying to you what has plagued me in the past.

  7. #7
    Registered User
    Join Date
    Nov 2005
    Posts
    673
    I thank you for that. I am 18 years old and I am currently attending community college for my core classes. I start my programming classes next semester so any insight as to proper technique and or coding practices will surely help me in the long run. I thank you very much bubba. I am going to work on my code for awhile, and see if I can't get the kinks worked out.

    Should i make header file just for special return values (like error returns)

    I was thinking something like

    ErrorValues.h
    Code:
    namespace varia
    {
      static const int SDL_INIT_ERROR = 0;
      static const int SDL_VIDEO_ERROR = 1;
      //and so on
    }
    Last edited by Raigne; 02-27-2008 at 11:05 PM. Reason: typo

  8. #8
    CSharpener vart's Avatar
    Join Date
    Oct 2006
    Location
    Rishon LeZion, Israel
    Posts
    6,484
    Note that returning 0 in most cases means success
    So maybe you should also stick to this assumption
    The first 90% of a project takes 90% of the time,
    the last 10% takes the other 90% of the time.

  9. #9
    Registered User
    Join Date
    Nov 2005
    Posts
    673
    I know that was just a example

  10. #10
    Super Moderator VirtualAce's Avatar
    Join Date
    Aug 2001
    Posts
    9,596
    Using an enum instead of those statics is also an option. And if you are going to use an enum for error values you may want to place the enum inside of the class using it. I asked another developer about why he did this and he told me placing an enum inside of a class implies ownership of it.

  11. #11
    Registered User
    Join Date
    Nov 2005
    Posts
    673
    is this better design for the Graphics init?
    Code:
    int varia::Graphics::Init(unsigned int windowWidth, unsigned int windowHeight, bool fullscreen,
    						  const char* windowTitle, unsigned int bgR, unsigned int bgG, unsigned int bgB)
    {
    	if ( SDL_Init(SDL_INIT_VIDEO|SDL_INIT_TIMER) < 0 )
    		return SDL_INIT_ERROR;
    
    
    	if (fullscreen)
    	{
    		m_screen = SDL_SetVideoMode(windowWidth,windowHeight,32,SDL_ANYFORMAT|SDL_FULLSCREEN|SDL_ASYNCBLIT|SDL_HWSURFACE|SDL_DOUBLEBUF);
    		if ( m_screen == NULL )
    			return SDL_SETVIDEO_ERROR;
    	}
    	else
    	{
    		m_screen = SDL_SetVideoMode(windowWidth,windowHeight,32,SDL_ANYFORMAT|SDL_ASYNCBLIT|SDL_HWSURFACE|SDL_DOUBLEBUF);
    		if ( m_screen == NULL )
    			return SDL_SETVIDEO_ERROR;
    	}
    	//
    	if ( windowTitle != NULL )
    		SDL_WM_SetCaption(windowTitle,NULL);
    	else
    		SDL_WM_SetCaption("",NULL);
    
    	//-------------//
    	m_bgR = bgR;
    	m_bgG = bgG;
    	m_bgB = bgB;
    	return FUNCTION_SUCCESS;
    }

  12. #12
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,652
    Code:
    		if ( m_screen == NULL )
    			return SDL_SETVIDEO_ERROR;
    is in both branches, so you should extract out of the if statement and else block.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  13. #13
    Registered User
    Join Date
    Nov 2005
    Posts
    673
    I am very grateful bubba. Just with those relatively small changes my code has been at least 90&#37; less error prone. I made most of my functions return int so i could see where and what was going wrong with them, and it has made a drastic improvement. Thank you. One last question is it better to have each object clean up itself or have a resource_manager template class do it?

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Defining multiple classes in the same header file
    By Stonehambey in forum C++ Programming
    Replies: 2
    Last Post: 08-14-2008, 10:36 AM
  2. Questions on Classes
    By Weng in forum C++ Programming
    Replies: 2
    Last Post: 11-18-2003, 05:49 AM
  3. Prime Number Generator... Help !?!!
    By Halo in forum C++ Programming
    Replies: 9
    Last Post: 10-20-2003, 07:26 PM
  4. include question
    By Wanted420 in forum C++ Programming
    Replies: 8
    Last Post: 10-17-2003, 03:49 AM
  5. Sharing a variable between classes of different .CPP files
    By divingcrab in forum C++ Programming
    Replies: 5
    Last Post: 07-07-2002, 02:57 PM

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