What do you think of my SDL2 program so far?

[NewbieCPPguy]

Hi everyone,

I started learning C++ last month. I've learned enough basics to begin working with the SDL2 library. I'm starting to make a game.

My code below compiles and runs just fine on my Linux machine.

Can you help tell me if I'm writing my code right or badly?

Game.h: http://pastebin.com/nn3S0dx1
Game.cpp: http://pastebin.com/QY46ZUtp
Graphics.h: http://pastebin.com/8suq6dgC
Graphics.cpp: http://pastebin.com/cGGCaYLz
functions.cpp: http://pastebin.com/ffbs1Aii
main.cpp: http://pastebin.com/nDuFrDNX

I wish I could ask some friends, but I know no one who knows C++ well enough...

[Peter87]

Looks good, but why is the SDL_Event object a member of the Game class and not a local variable in the game loop?

You have created the changeWindowSize to be used when changing the size of the window so why are the width and height of the window public variables? I think they should be private to prevent them from being modified directly from outside the class.

[NewbieCPPguy]

Thanks, Peter.

I've been learning by emulating other code I've seen, so this code is sort of a mesh of those.

Why make the SDL_Event object a a local variable in game loop instead of a Game class member?

With window size, if I make the height and width private members, and if I make getter functions for them... should those functions return copies of the size's values? Could that mean memory leak because of violating "Don't Repeat Yourself" principle?

[Peter87]

Why make the SDL_Event object a a local variable in game loop instead of a Game class member?

The reason I think so is because the event variable is only useful while you are handling the events. It doesn't make sense to use the game's event object at any other time.

Generally I think that the scope of a variable should be restricted as much as possible so that it only exist while it is being used. If the object is costly to create and destroy each time you might want to extend the scope so that you can reuse the same object multiple times but I don't think that is anything you need to worry about in this case because SDL_Event is relatively small and have no constructor/destructor.

If you think about it SDL_Event is like a loop variable. If you had a class that contained a vector of enemies, and you looped through them using an index, would you make the index variable a member of the class? I don't think so. Instead you declare the variable inside the for loop, so that the variable will only exist in that loop.

for (std::size_t i = 0; i < enemies; ++i) {
	enemies[i].update();
}

The SDL_Event is like a loop variable, just like index i in the example above, so it makes sense to declare it as part of the loop.

for (SDL_Event ev; SDL_PollEvent(&ev) != 0;) {
	if(ev.type == SDL_QUIT)
		_isRunning= false;
}

If the loops were written like this there is no chance that we use the variables when they should not be used. If we tried, the compiler would kindly let us know that something is wrong by giving us an error.

With window size, if I make the height and width private members, and if I make getter functions for them... should those functions return copies of the size's values?

Yes. There is no benefit of returning const references to the variables because copying an integer is cheap.

Could that mean memory leak because of violating "Don't Repeat Yourself" principle?

I don't see how violating the "Don't Repeat Yourself" principle has anything to do with memory leaks. I would say no, memory leaks is not an issue here, unless you do something stupid like returning a pointer to an integer allocated with new.

And don't think of it as getters for the variables. Think of it as methods to get the current size of the window. If you implement it using member variables or using SDL_GetWindowSize is up to you.