Battle Tanks game

I created a simple 2D game that I made for myself in C++ with SDL2.0 + OpenGL + Box2D, but I'm insecure about the code and structure because I developed the game using everything I've learned on the Internet and not following some pattern. I posted the code on GitHub and I'm spreading it on some forums to get feedback about the code, but here I'll just post the parts that I consider the more important for this.

About the game: The game is a battle between two players (soldiers) that must destroy each other's tanks, but the tank is only available to attack when the tank defensor is dead (which lasts for 5 seconds). When the tank is destroyed, the tank defensor lost the game. While the game is running, there are some boosts that appear randomly on the ground. The boosts are: speed (accumulate until two), defense and life.

I would like to have it analyzed by: viability of some operations, for example, the bullets vector and the way I'm doing it, with the FPS calculator and the painter methods.

The main loop:

```
#include

#include
#include
#include

#include "game.hpp"

SDL_Window* initSDL(const std::string& title, unsigned int w, unsigned int h);
SDL_GLContext initOpenGL(SDL_Window* window);
void events(LoopHandler* lhandler);
void logics(LoopHandler* lhandler);
void render(LoopHandler* lhandler);

int main(int argc, char* argv[]){
//fps counter
unsigned int fps_c = 0;
Timer fps_ct;

const unsigned int DISERED_DELAY = 25;
unsigned int start, end, loop_t, excess = 0;

SDL_Event event;
LoopHandler* handler = nullptr;
SDL_Window* window = nullptr;
SDL_GLContext glc = nullptr;

try{
window = initSDL("Hardcore!", 500, 500);
glc= initOpenGL(window);

handler = new Game();

handler->pre();
fps_ct.start();
while (handler->active()){
start = SDL_GetTicks();

while (SDL_PollEvent(&event)){
handler->process(&event);
}

while (excess logics();
excess -= DISERED_DELAY;
}

han

General Structure:

Indentation. Be consistent. It makes the code really hard to read when this is all over the place. Also add a layer of indentation after every block opens { and remove it when the block closes }.

Memory Management

You should not be using new/delete. Most of the time automatic variables are preferred. And when you want dynamically created objects std::unique_ptr generates code that is as quick as using raw pointers (if you can measure the difference I would be highly surprised (especially since 99% of the time they compile down to the same code).

LoopHandler* handler = nullptr;

try{

    handler = new Game();
}
catch(std::exception const& e)
{}

delete handler;

This code is not excetion safe (not all exceptions need to derive from std::exception. Just make this a local variable and all the problems of management disappear.

LoopHandler handler;

try{

}
catch(std::exception const& e)
{}

Why on why do this:

/*set default bullets*/
SimpleBullet* sb = new SimpleBullet(this);
mgun->charge(sb);                           
delete sb;

There are several problems with this one small bit of code. There are no ownership semantics defined for sb. You pass sb as a pointer to charge() so I am not 100% convinced that you need to delete them (I will now need to check how that method works to make sure you are doing it correctly). But assuming you are this means internally you are making a copy of sb so why do you need to create them dynamically like this and delete them if you are going to make a copy.

Those three lines just make my ears bleed.

I would have done.

/*set default bullets*/
mgun.charge(SimpleBullet(this));  // Pass by value (or ref)
                                  // This mean if `charge()` wants the bullets he needs to copy them

OR.

/*set default bullets*/
std::unique_ptr sb(new SimpleBullet(this));
mgun.charge(std::move(sb));          // I am moving them to `charge()`
                                     // So he is taking ownership (anyway they are not coming back).
                                     // So unique_ptr will do nothing.

OR.

/*set default bullets*/
std::unique_ptr sb(new SimpleBullet(this));
mgun.charge(sb);                    // I am passing a reference to charge (you can't copy).
                                    // This means `charge()` has the option of taking ownership.
                                    // If he does then its his responsibility to delete them
                                    // If not they stay in `sb` which will automatically
                                    // delete them in an exception safe way when it goes
                                    // out of scope.

Use RAII for Initialization/Destruction control.

When ever you see code that looks like this:

void func()
 {
   DoSomeInit();

   CODE()

   SoSomeCleanUp();
 }

You are doing it wrong. This code is not exception safe. If CODE() throws then then cleanup will not be done. You should use RAII to do this work.

class MyGameEnvironment
  {
      public:
          MyGameEnvironment()
          {
               DoSomeInit();
          }
          ~MyGameEnvironment()
          {
               SoSomeCleanUp();
          }
  };

  void func()
  {
    MyGameEnvironment  game;
    CODE();
  }

I know this looks like a lot of extra work. Its not really. And the style will save your ass more than you think it will. It also makes your functions smaller and easier to read.

Global Variables are a no-no

More precisely. Global mutable state is a bad thing. It makes your code hard to test. Hard to get correct and leads to spaghetti style code. Create your objects locally in a function and pass them around as parameters.

int main()
 {
     Tank       tank1;
     Tank       tank2;
     MyGame     game(tank1, tank2);

     game.play();
 }

StackExchange Code Review Q#112144, answer score: 12