#Design

###Single Responsibility Principle (SRP)

#Correctness

###Resource handling

###Uninitialized value

#Implementation

Design

Single Responsibility Principle (SRP)

Correctness

Resource handling

Uninitialized value

Implementation

• All members are uninitialized upon construction.
• Game::Stop doesn't check if the member variables/SDL systems are actually initialized or if m_Running is true. This might cause lots of issues, especially when the member variables are still in an uninitialized state!
• The instructions in Game::Start might be more appropriate in Games constructor (as they initialize/acquire resources). Similarly, most instructions in Game::Stop would be more appropriate in the destructor.
• I'm personally not a fan of the tail calls in Game::Start and Game::GameLoop. I'd rather extract the sequence of calls to Game::Start, Game::GameLoop and Game::Stop into a Game::Run member function.
• Game::Stop should not be public in its current state. While the game is running, the original caller of Game::Start is still executing that call and unable to call Game::Stop so only internal calls to Game::Stop could be made anyways. The only other possible caller of Game::Stop would be another thread - but Game, especially Game::Stop, is not thread safe and might release resources still used by another thread!
• I'm not a fan of the Timer usage in Game::GameLoop. Not only is it impossible to use the timer pause/unpause functionality this way (as likely intended to pause the game), different systems very likely require different pause states (e.g. the GUI should still respond, even if the game is paused). Also, the repeated calls to Timer::Start seem like a hacky workaround that is throwing away useful information; it would be just as easy to cache the result of the last call to Timer::GetTicks in a local variable.
• Variable naming: Most variables are more or less given descriptive names. But then, in Game::Render, all local variables have very general names. textSurface doesn't contain any generic text, it contains the FPS counter (and similar for textTexture and textBounds). colour might be better described by what it represents (white) or, even better, what it's intended usage is: text_color (system_text_color?) or maybe foreground_color. font might be borderline acceptable, but even there a more descriptive name like system_font or similar would help to better communicate its intended usage.
• If you have to use console output for error reporting, prefer std::cerr over std::cout. Even though they might look the same for a human observer on the console window, they can easily be distiguished from normal console output by other programs.
• There are a lot of "magic numbers" (constants without obvious meaning). While I can rather confidently estimate that "assets/fonts/caliban.ttf" is the path to a font file, I just have to guess what the default background color is: "fully opaque black" (RGBA)? "bright transparent green" (ARGB)? Or maybe something completely different?

• All members are uninitialized upon construction.
• Game::Stop doesn't check if the member variables/SDL systems are actually initialized or if m_Running is true. This might cause lots of issues, especially when the member variables are still in an uninitialized state!
• The instructions in Game::Start might be more appropriate in Games constructor (as they initialize/acquire resources). Similarly, most instructions in Game::Stop would be more appropriate in the destructor.
• I'm personally not a fan of the tail calls in Game::Start and Game::GameLoop. I'd rather extract the sequence of calls to Game::Start, Game::GameLoop and Game::Stop into a Game::Run member function.
• Game::Stop should not be public in its current state. While the game is running, the original caller of Game::Start is still executing that call and unable to call Game::Stop so only internal calls to Game::Stop could be made anyways. The only other possible caller of Game::Stop would be another thread - but Game, especially Game::Stop, is not thread safe and might release resources still used by another thread!
• I'm not a fan of the Timer usage in Game::GameLoop. Not only is it impossible to use the timer pause/unpause functionality this way (as likely intended to pause the game), different systems very likely require different pause states (e.g. the GUI should still respond, even if the game is paused). Also, the repeated calls to Timer::Start seem like a hacky workaround that is throwing away useful information; it would be just as easy to cache the result of the last call to Timer::GetTicks in a local variable.
• Variable naming: Most variables are more or less given descriptive names. But then, in Game::Render, all local variables have very general names. textSurface doesn't contain any generic text, it contains the FPS counter (and similar for textTexture and textBounds). colour might be better described by what it represents (white) or, even better, what it's intended usage is: text_color (system_text_color?) or maybe foreground_color. font might be borderline acceptable, but even there a more descriptive name like system_font or similar would help to better communicate its intended usage.
• If you have to use console output for error reporting, prefer std::cerr over std::cout. Even though they might look the same for a human observer on the console window, they can easily be distiguished from normal console output by other programs.
• There are a lot of "magic numbers" (constants without obvious meaning). While I can rather confidently estimate that "assets/fonts/caliban.ttf" is the path to a font file, I just have to guess what the default background color is: "fully opaque black" (RGBA)? "bright transparent blue" (ARGB)? Or maybe something completely different?

Example class layout:

• All members are uninitialized upon construction.
• Game::Stop doesn't check if the member variables/SDL systems are actually initialized or if m_Running is true. This might cause lots of issues, especially when the member variables are still in an uninitialized state!
• The instructions in Game::Start might be more appropriate in Games constructor (as they initialize/acquire resources). Similarly, most instructions in Game::Stop would be more appropriate in the destructor.
• I'm personally not a fan of the tail calls in Game::Start and Game::GameLoop. I'd rather extract the sequence of calls to Game::Start, Game::GameLoop and Game::Stop into a Game::Run member function.
• Game::Stop should not be public in its current state. While the game is running, the original caller of Game::Start is still executing that call and unable to call Game::Stop so only internal calls to Game::Stop could be made anyways. The only other possible caller of Game::Stop would be another thread - but Game, especially Game::Stop, is not thread safe and might release resources still used by another thread!
• I'm not a fan of the Timer usage in Game::GameLoop. Not only is it impossible to use the timer pause/unpause functionality this way (as likely intended to pause the game), different systems very likely require different pause states (e.g. the GUI should still respond, even if the game is paused). Also, the repeated calls to Timer::Start seem like a hacky workaround that is throwing away useful information; it would be just as easy to cache the result of the last call to Timer::GetTicks in a local variable.

Example class layouts:

• All members are uninitialized upon construction.
• Game::Stop doesn't check if the member variables/SDL systems are actually initialized or if m_Running is true. This might cause lots of issues, especially when the member variables are still in an uninitialized state!
• The instructions in Game::Start might be more appropriate in Games constructor (as they initialize/acquire resources). Similarly, most instructions in Game::Stop would be more appropriate in the destructor.
• I'm personally not a fan of the tail calls in Game::Start and Game::GameLoop. I'd rather extract the sequence of calls to Game::Start, Game::GameLoop and Game::Stop into a Game::Run member function.
• Game::Stop should not be public in its current state. While the game is running, the original caller of Game::Start is still executing that call and unable to call Game::Stop so only internal calls to Game::Stop could be made anyways. The only other possible caller of Game::Stop would be another thread - but Game, especially Game::Stop, is not thread safe and might release resources still used by another thread!
• I'm not a fan of the Timer usage in Game::GameLoop. Not only is it impossible to use the timer pause/unpause functionality this way (as likely intended to pause the game), different systems very likely require different pause states (e.g. the GUI should still respond, even if the game is paused). Also, the repeated calls to Timer::Start seem like a hacky workaround that is throwing away useful information; it would be just as easy to cache the result of the last call to Timer::GetTicks in a local variable.
• Variable naming: Most variables are more or less given descriptive names. But then, in Game::Render, all local variables have very general names. textSurface doesn't contain any generic text, it contains the FPS counter (and similar for textTexture and textBounds). colour might be better described by what it represents (white) or, even better, what it's intended usage is: text_color (system_text_color?) or maybe foreground_color. font might be borderline acceptable, but even there a more descriptive name like system_font or similar would help to better communicate its intended usage.
• If you have to use console output for error reporting, prefer std::cerr over std::cout. Even though they might look the same for a human observer on the console window, they can easily be distiguished from normal console output by other programs.
• There are a lot of "magic numbers" (constants without obvious meaning). While I can rather confidently estimate that "assets/fonts/caliban.ttf" is the path to a font file, I just have to guess what the default background color is: "fully opaque black" (RGBA)? "bright transparent green" (ARGB)? Or maybe something completely different?