Thread: Just wondering about safety of my classes...

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.

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?

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.

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.

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
}

Note that returning 0 in most cases means success
So maybe you should also stick to this assumption

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.