Wow, that is quite the game of Pong you have there!

Some things that jump out at me from skimming:

• The error handling in main() does not correctly quit the SDL libraries if one of them fails. That is, if SDL_Mixer fails, you should close SDL and SDL_TTF. One way to do this is to use std::atexit() to register these functions to be run during shutdown:

  
   
  int main(int argc, char *args[])
  {
      if (SDL_Init(SDL_INIT_EVERYTHING) == -1) {
           exit(EXIT_FAILURE);
      }
      atexit(&SDL_Quit);
   
     if (TTF_Init() == -1) {
        exit(EXIT_FAILURE);
     }
     atexit(&TTF_Quit);
   
     if (Mix_OpenAudio(22050, MIX_DEFAULT_FORMAT, 2, 4096) == -1) {
        exit(EXIT_FAILURE);
     }
     atexit(&Mix_CloseAudio);
   
      // ...
   
      return 0;
  }
  

• Overuse of dynamic allocation. If you don't need an object to outlive its scope (and it doesn't contain large, fixed size arrays), you can and should stack allocate it. One reason to do it this way is that you cannot forget to delete the object. Another is that stack allocation is considerably faster than the default allocator.

• There are quite a few classes that do not appear to clean up all the SDL resources they load. One idea is to write a set of classes to wrap SDL and manage this for you:

  
  class Surface : noncopyable { // https://encrypted.google.com/search?q=noncopyable
  public:
      Surface(const std::string &filename);
      ~Surface();
   
      void draw(SDL_Surface *target, int x, int y);
      void draw(SDL_Surface *target, int x, int y, const SD_Rect &rect);
  private:
      SDL_Surface *surface;
  };
  

  Your "BackGround" class is an example, but it is too specific.

• Speaking of the BackGround class, the m_bLoad member is redundant, you can use the condition m_BackGroundImage != null directly.

• Consider using std::vector<> rather than dynamically allocating arrays.

• Some of your functions are very long. Long methods are harder to write, read and maintain in general. For example, in Game::Run(), you could split the various game state specific logic into a helper function.

• Instead of having an enumeration of levels, and switching on that, you could store the various level parameters in a structure, and have an array/vector of these structures. Consider maybe loading this data from a file rather than hard coding it.

• You have a lot of duplicate code for handling Player vs Player and Player vs PC, even the "game over" logic is duplicated. Ideally, you'd have one piece of code that handles running the game, and use something like polymorphism to ensure that the correct player specific logic is used.

• Your gFailure handling code seems to rely on ALGER.ttf, which is one of the potential reasons you might end up in this state! An alternative would be to bubble up the exact reason for failure from the lower levels of your code, so you can tell the user exactly what is wrong. Also, most users are not going to check that the directories contain the correct data files, if re-installing the game doesn't fix it they will at most complain to you, or just abandon the game.

• Your Saves class maintains two parallel arrays. Consider instead having a single array of structures representing the same data. However, here a superior option would be infer the locked status from the complete status. A level is unlocked if the previous level has been completed.

• The Saves class does not correctly handle an I/O error in the middle of the file. It will report that the file loaded correctly, even though the file may be only partially loaded.

• The Saves class uses the hard coded value of "11", even though it allows a custom number of save slots to be created.

• There are 150 lines of dense collision code in level.cpp. I cannot understand it at all. Consider splitting it into small helper functions.

• There are lots of "magic numbers" around the code. For instance, in player.cpp there is 253, 1004 and 945 and 1685. These should be at least named constants, if not inferred from other values (such as the screen width and paddle width).

There are several places in your program where you run input and drawing loops. An alternative design is to have the main class run these loops, and delegate the actual rendering logic and reaction to events to the "active" state.

You're loading the same font all over the place (well, sometimes in different sizes). Consider loading the various sized fonts once during startup, and allowing the relevant classes access to these fonts when rendering.

There is still a lot more code to review, but it is in danger of turning into a lovely day so I'm going out. I might come back to this later on.

Hello, thanks a lot for your time and your review what I do bad.

I rewrite some part of code, but I cann't rewrite everything.

If I create my own dynamic memory allocation, I should use it or I should use Stack?

Foo *foo = new Foo(/* ... */);
// Use *foo
delete foo;

Foo foo(/* ... */);
// Use foo

That is a great idea, but in some function(when I render text) I must free Surface every frame.

// "Wrapped" font, like the Surface idea... 
class Font : noncopyable
{
public:
    // ...
 
    SDL_Surface *renderText(const std::string &text, /* ... colour ? */);
 
};
 
class RenderedText : noncopyable
{
public:
    // N.b. Font must outlive this instance
    RenderedText(Font &font)
    :
        font(&font),
        surface(nullptr)
    {
    }
 
    ~RenderedText()
    {
        // Note: safe to pass nullptr to SDL_FreeSurface
        // http://sdl.beuc.net/sdl.wiki/SDL_FreeSurface
        SDL_FreeSurface(surface);
    }
 
    void render(SDL_Surface *destination, int x, int y)
    {
        if(surface)
        {
            SDL_BlitSurface(/* ... */);
        }
    }
 
    void setMessage(std::string newMessage)
    {
        if(newMessage != currentMessage)
        {
            SDL_Surface *temp = font->renderText(newMessage);
            if(!temp)
            {
                // TODO: handle error?
            }
            std::swap(surface, temp);
            std::swap(newMessage, currentMessage);
            SDL_FreeSurface(temp);
        }
    }
 
private:
    Font *font;
    std::string currentMessage;
    SDL_Surface *surface;
};

What I forget clean up in BackGround?

Do you mean that I should use there vector?

I'm loading this data from file as you said. But I must have 10 Files with 5 values. And it is cumbersome. If I create one file with all values I don't know, how I can move in this file. I know functions seekg but I don't know how I can go on line 2, or on line 5..

struct Level
{
    float speedx;
    float speedy;
    // ...
};
 
std::istream &operator>>(std::istream &stream, Level &level)
{
    return stream >> level.speedx >> level.speedy /* ... */;
}
 
std::vector<Level> loadLevels(std::istream &stream)
{
    // Optional:
    // Early out if stream isn't good to read from initially
 
    std::vector<Level> result;
 
    while(stream >> std::ws)
    {
        Level level;
        if(stream >> level)
        {
            result.push_back(level);
        }
    }
 
    if(!stream.eof())
    {
        // Level file might be corrupt?
        // Might be worth logging.
       std::clog << "Level data corrupt after " << result.size() << " levels!\n";
    }
 
    if(result.empty())
    {
        // Maybe complain and exit if no levels have been loaded...
        // Could be caller's reponsibility
        std::cerr << "No levels?\n";
    }
    return result;
}

1 2.3
2 3.4
3 4.5
4 5.6

I completely change GameRun and Saves class and I split long functions into smaller helper functions. I hope that it is better.

I will not go change Failure handling and loading font once, because game is complete(I hope).

Second game will better