• 15
• 15
• 11
• 9
• 10

# Rating Code(Pong)

This topic is 1802 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

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

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 on other sites
It would be easier if you just posted your code here or on pastebin.

##### Share on other sites

Sorry for that, here is link:

http://pastebin.com/hGjYWSdw

##### Share on other sites

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

##### Share on other sites

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.

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 /* ... */;
}

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