Sign in to follow this  
Xperience

Rating Code(Pong)

Recommended Posts

Xperience    1793

Hello,

I've created my first game(with graphics). It's a simple pong, but it isn't as normal pong(you can use multiple balls).

I've read this article:

 

http://www.gamedev.net/page/resources/_/technical/game-programming/your-first-step-to-game-development-starts-here-r2976

 

and author said, that we "beginners" can post code here for review.

What I do bad?, what can I do better? What terrible practic I use?

 

Thanks a lot for all the advice.

 

Here is a visual studio project:

Share this post


Link to post
Share on other sites
Xperience    1793

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.

 

 


 

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.

 

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

 

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

 

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

What I forget clean up in BackGround?

 

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

 

Do you mean that I should use there vector?

 

Ball *balls = new Ball[Bnum];

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.

 

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...

 

 

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 smile.png

Share this post


Link to post
Share on other sites
rip-off    10976

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.
No problem! The idea isn't that you change everything, or even that you change anything, but just things to consider going forward. If you are still want to work on that project, I would recommend fixing the correctness issues, rather than the stylistic notes.
 

If I create my own dynamic memory allocation, I should use it or I should use Stack?
I wouldn't recommend rolling your own memory allocation. If necessary, there are plenty of existing allocators you could drop in. However, the stack is easiest and fastest.
 
Instead of:
Foo *foo = new Foo(/* ... */);
// Use *foo
delete foo;
 
Simply allocate the object directly:
Foo foo(/* ... */);
// Use foo
This is easier, faster and exception safe. Manual memory management tends to leak when an exception or early return occurs, unless you are particularly careful.
 

That is a great idea, but in some function(when I render text) I must free Surface every frame.
If the data is being allocated every frame, it probably doesn't need to be a member variable. It can be a local variable.
 
In the specific case of text, you can actually avoid allocating every frame when the text changes infrequently (almost all in-game text changes infrequently relative to the game logic):
// "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;
};
I've omitted the usual shared_ptr<Font>, which would just complect the (untested) example.
 

What I forget clean up in BackGround?
Nothing. That isn't what I meant - I meant that your BackGround class is quite similar to how that Surface class might be implemented.
 

Do you mean that I should use there vector?
Yes, that would be one place you should use vector, or some container at the very least.
 

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..
Don't seek. Just slurp it all in during startup:
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;
}
 
The above code assumes a simple whitespace delimited format. The file could look like this:
1 2.3
2 3.4
3 4.5
4 5.6
A more robust approach might be to read each line using std::getline(), and then re-parse it using std::stringstream.
 

I completely change GameRun and Saves class and I split long functions into smaller helper functions. I hope that it is better.
A quick skim, looks great!
 

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

Second game will better 
I'm sure it will.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this