   Is using a constructor with SDL correct?
  I'm revisiting a project I was trying long ago now that I'm back to playing in SDL. All I am trying to do is get a class created that handles the graphics aspects of SDL that I am learning (ie blitting images, displaying text, etc.)I am using the lazyfoo tutorials for now since I know all of his functions work :) but I am having trouble moving them into a class. Take a look below:All this is supposed to do is to is display some graphics. I have three files. The first file is where my surfaces are stored:#ifndef _SURFACES_H#define _SURFACES_H#include "SDL.h"#include <string>//SurfacesSDL_Surface *message    = NULL;SDL_Surface *background = NULL;SDL_Surface *screen= NULL;SDL_Surface *load_image(std::string filename){//Temporary surface for loaded imageSDL_Surface *loadedImage = NULL;//Optimized surface for loaded imageSDL_Surface *optimizedImage = NULL;//Load the imageloadedImage = SDL_LoadBMP(filename.c_str());//Load image if nothing went wrongif(loadedImage != NULL){//Create an optimized imageoptimizedImage = SDL_DisplayFormat(loadedImage);//Free the old imageSDL_FreeSurface(loadedImage);}return optimizedImage;}#endif //_SURFACES_HThe second file is my class:#ifndef _SDL_ENGINE_H#define _SDL_ENGINE_H#include "SDL.h"#include <string>class Graphics{private:public:Graphics();~Graphics();void apply_surface(int x, int y, SDL_Surface *source, SDL_Surface *destination);};Graphics::Graphics(){//Initialize all SDL subsystemsSDL_Init(SDL_INIT_EVERYTHING); //<-I suspect this doesnt do anything//screen = SDL_SetVideoMode(640, 480, 32, SDL_SWSURFACE);//SDL_WM_SetCaption("Testing!",NULL);}Graphics::~Graphics(){}void Graphics::apply_surface(int x, int y, SDL_Surface *source, SDL_Surface *destination){//Make a temp rectangle to hold the offsetsSDL_Rect offset;//Give the offsets to the rectangleoffset.x = x;offset.y = y;//Blit the surfaceSDL_BlitSurface(source, NULL, destination, &offset);}#endif //_SDL_ENGINE_H_And finally my main function//Headers#include "SDL.h"#include "SDL_Engine.h"#include "Surfaces.h"#include <string>int main(int argc, char* args[]){Graphics *graphics = NULL;//Set up the screenscreen = SDL_SetVideoMode(SCREEN_WIDTH, SCREEN_HEIGHT, SCREEN_BPP, SDL_SWSURFACE);//Set the window captionSDL_WM_SetCaption("Testing!",NULL);//Load the imagesmessage    = load_image("images\\message.bmp");background = load_image("images\\background.bmp");//Apply the message to the screengraphics->apply_surface(0,0,message,background);//Apply the background to the screengraphics->apply_surface(0,0,background,screen);//Update the screenif(SDL_Flip(screen) == -1)return 1;//Wait 2 secondsSDL_Delay(2000);//Free the surfacesSDL_FreeSurface(message);SDL_FreeSurface(background);//Quit SDLSDL_Quit();return 0;}Now, I know the above is a bit messy but it works. (Bear with me I'm trying to make it be neat and work)Ok, now if I want to initialize everything SDL in the constructor as was suggested to me long ago when I posted this how would I do that? I used to have an init() which worked fine, but if I change the constructor to something like this:Graphics::Graphics(){//Initialize all SDL subsystemsSDL_Init(SDL_INIT_EVERYTHING);screen = SDL_SetVideoMode(640, 480, 32, SDL_SWSURFACE);SDL_WM_SetCaption("Testing!",NULL);}Everything compiles correctly however nothing shows up. Even if I add a delay in. It's almost like I can't pass these type of parameters into the constructor. Anyone have any ideas? I just wanted the constructor to set all the window parameters.Also if anyone knows how, how would I move the following function:SDL_Surface *load_image(std::string filename){//Temporary surface for loaded imageSDL_Surface *loadedImage = NULL;//Optimized surface for loaded imageSDL_Surface *optimizedImage = NULL;//Load the imageloadedImage = SDL_LoadBMP(filename.c_str());//Load image if nothing went wrongif(loadedImage != NULL){//Create an optimized imageoptimizedImage = SDL_DisplayFormat(loadedImage);//Free the old imageSDL_FreeSurface(loadedImage);}return optimizedImage;}into a method of the class? It seems like I cannot use anything with the datatype SDL_Surface in my class. Sorry I am not terribly familiar with SDL, any help would be greatly appreciated.   
  There's nothing in principle that would prevent you from setting up the video mode in a constructor, so if it isn't working, there's probably a problem with your implementation.I only glanced over your code, but it doesn't look like you ever actually create a Graphics instance in main(). Did you copy and paste your code exactly? If so, it looks to me like no Graphics object is ever created, which means you're accessing a NULL pointer in main(). (It also means that the Graphics constructor is never called.)   ;  Doh! You're right! Not sure how I missed that. Thank you so much :)Now, I have another question. Going off of another book I have it states that if you have a class to handle graphics its better to have a pointer to that class vs copying it. So I changed my main code to the following://Headers#include "SDL.h"#include "SDL_Engine.h"#include "Surfaces.h"#include <string>//Screen attributesconst int SCREEN_WIDTH  = 640;const int SCREEN_HEIGHT = 480;const int SCREEN_BPP    = 32;Graphics graphics; //<-----Added this here to instantiate the objectint main(int argc, char* args[]){Graphics *p_graphics = &graphics; //<----made a pointer to that object//Load the imagesmessage    = load_image("images\\message.bmp");background = load_image("images\\background.bmp");//Apply the message to the screenp_graphics->apply_surface(0,0,message,background);//Apply the background to the screenp_graphics->apply_surface(0,0,background,screen);//Update the screenif(SDL_Flip(screen) == -1)return 1;//Wait 2 secondsSDL_Delay(2000);//Free the surfacesSDL_FreeSurface(message);SDL_FreeSurface(background);//Quit SDLSDL_Quit();return 0;}Is this the most efficient or correct way to do things? Sorry I know these are probably simple questions :/   ;Quote:Original post by Chrono1081Now, I have another question. Going off of another book I have it states that if you have a class to handle graphics its better to have a pointer to that class vs copying it. So I changed my main code to the following:Is this the most efficient or correct way to do things? Sorry I know these are probably simple questions :/It is more efficient to pass large objects by reference or by pointer than by value.  In your code, you aren't passing your object to any functions, so there is no difference in speed between your two snippets.  Using pointers doesn't automatically make your code more efficient.The best style is to not use pointers unless you have to and not use global variables (especially when the constructors have side effects like yours does).  Allocate your object on the stack in main and then if you need pass it to a function, do so by reference.   ;  One thing to keep in mind is that several of the SDL functions you call in the Graphics constructor can fail for several different reasons.  Since constructors need to return an object in a usable form and you can't return any error messages, your only option is to throw an exception when a constructor (or in this case, a function called within the constructor) fails.  An alternative idea is to put those functions in an init() call of the graphics object.  So you would create a graphics object, then call init() on it.  The init() function could fail and return false giving your application a means of handling when a SDL cannot initialize or a surface cannot be created.Cheers,Bob   ;Quote:Original post by Chrono1081Doh! You're right! Not sure how I missed that. Thank you so much :)Now, I have another question. Going off of another book I have it states that if you have a class to handle graphics its better to have a pointer to that class vs copying it. Is this the most efficient or correct way to do things? Sorry I know these are probably simple questions :/In your original code you already had a pointer to it, all you had to do was add a call to new.  Note what the advice you have been given says don't copy the graphics object use a pointer.  So rather than getting in to cargo cult programming let us look at what is being suggested. Don't copy the graphics object, fairly good advise the graphics object will more than likely be a heavy object(though in your case it doesn't seem to be) with a fairly simple lifetime(during the whole program.) The second bit is use a pointer.  This I find more contentious as there is no reason to use a pointer.  We could use a reference and get the same don't make a copy behavior with better life time semantics(not null already enforced rather than you having to check it everywhere.)  Though enforcing the use of ref is a little harder (no type error if you forget the &.)  Third you may want to add a check to your start up so that if there is an accidental copy of the graphics object that SDL is only inited once.  Infact your graphics class appears to be used as a namespace and not as a class outside of the sdl init part. If that is the case I would just have an instance declared in main and forget about the pointer thing because it would be a pointer to nothing.   ;Quote: Simian Man: It is more efficient to pass large objects by reference or by pointer than by value. In your code, you aren't passing your object to any functions, so there is no difference in speed between your two snippets. Using pointers doesn't automatically make your code more efficient.The best style is to not use pointers unless you have to and not use global variables (especially when the constructors have side effects like yours does). Allocate your object on the stack in main and then if you need pass it to a function, do so by reference. Thank you for the advice. I removed the globals and the pointer. (I did not remove the global surfaces yet though. I will once I organize the rest of my class) I really didn't understand the suggestion on the pointer in the book I am reading but figured the author knew more then I did :) I also don't foresee me needing to pass my object to a function. The whole purpose of my class is to get rid of the spaghetti I am accumulating learning SDL :)Quote: Scourage: One thing to keep in mind is that several of the SDL functions you call in the Graphics constructor can fail for several different reasons. Since constructors need to return an object in a usable form and you can't return any error messages, your only option is to throw an exception when a constructor (or in this case, a function called within the constructor) fails.An alternative idea is to put those functions in an init() call of the graphics object. So you would create a graphics object, then call init() on it. The init() function could fail and return false giving your application a means of handling when a SDL cannot initialize or a surface cannot be created.I thought about this and wondered if it wouldn't be better to create an init() that the constructor calls. Is this the suggested route? (I already changed my code so I can error check everything). It makes more sense to me to error check the screen and other things and return a -1 should something fail.Quote:Stonemetal: In your original code you already had a pointer to it, all you had to do was add a call to new.I forgot all about the new operator. (I've never had the need that I am aware of to use pointers even though I covered them many times in books and in classes)Thank you guys for all of the suggestions :) It gives me a lot to work with. I had quite a long break from programming to focus on math and more theory stuff and now I am getting back into the programming swing.   ;Quote: I thought about this and wondered if it wouldn't be better to create an init() that the constructor calls. Is this the suggested route? (I already changed my code so I can error check everything). It makes more sense to me to error check the screen and other things and return a -1 should something fail.While that's totally possible and acceptable to have your init() function get called by your constructor, you still have the possibility of the constructor failing.  How you handle it is your call.  To me, if the constructor fails, I throw an exception.  But at the same time, my constructors do the bare minimum to create an object that I can work on (usually just memory allocation and default values for variables) and I usually have an init() call outside the constructor that I call after creating the object.  If the init() call fails, I just handle it in my code rather than propagating exceptions.A -1 as a return value is common, but I think that there are several other options.  You could have an enumerated type representing the possible error messages or status (easier to read in a debugger).  Instead of an enum, you could use "const int error_nomouse=1".  I would shy away from "#define error_nomouse 1" type error messages since they're not readable in a debugger.  I usually have my function print out error messages to my log file and return a boolean.  Cheers, Bob;  Thank you guys for the great suggestions :)   