HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Simple Breakout/Arkanoid clone with SFML

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simplesfmlarkanoidwithbreakoutclone

Problem

I'm doing this small Breakout clone for school, and I've looked at heaps of people's takes on Breakout and tried to combine bits and pieces that I liked. Only problem that I'm having is getting the game to end when the ball hits the bottom window edge and sounds.

You should be able to copy/paste the code into a project, only issues being with the sound and text.

```
#include
#include
#include
#include
#include
using namespace std;
using namespace sf;
int x = 5;
constexpr int windowWidth ( 800 ), windowHeight( 600 );
constexpr float ballRadius( 10.f ), ballVelocity( 6.f );
constexpr float paddleWidth( 100.f ), paddleHeight( 20.f ), paddleVelocity( 8.f );
constexpr float blockWidth( 60.f ), blockHeight( 20.f );
constexpr int countBlocksX( 11 ), countBlocksY( 6 );
constexpr int countBlocks2X(11), countBlocks2Y(3);
bool isPlaying = true;

struct Ball
{
CircleShape shape;
Vector2f velocity{ -ballVelocity, -ballVelocity };

Ball(float mX, float mY)
{
shape.setPosition(mX, mY);
shape.setRadius(ballRadius);
shape.setFillColor(Color::Yellow);
shape.setOrigin(ballRadius, ballRadius);
}

void update()
{
//Need to make the ball bounce of the window edges
shape.move(velocity);
//If it's leaving on the left edge, we set a positive horizontal value.
if (left() windowWidth)
velocity.x = -ballVelocity;
//Top
if (top() windowHeight)
velocity.y = -ballVelocity;

}

float x() { return shape.getPosition().x; }
float y() { return shape.getPosition().y; }
float left() { return x() - shape.getRadius(); }
float right() { return x() + shape.getRadius(); }
float top() { return y() - shape.getRadius(); }
float bottom() { return y() + shape.getRadius(); }
};

//Create the Rectangle shape class for the brick
struct Rectangle
{
RectangleShape shape;
float x() { return shape.getPosition().x; }
float y() { r

Solution

I don't have SFML installed right now, so I didn't test you code, but here are some suggestions on improvements you can look into:

Avoid using namespace at the global level

This is a common issue with single-file programs. When projects are small, this is not very harmful, but can break builds in larger code bases due to name collisions. A namespace is a way of allowing identical names to coexist peacefully, if you use namespace at the global scope, that advantage is lost. Namespace names are usually sort precisely to make it easy qualifying the references with the namespace prefix. sf:: and std:: are not an annoyance to type.

One declaration per line

Longer lines with multiple declarations are less clear. In a long line like the following, it takes a few extra milliseconds to realize how many constants are
being declared:

constexpr float paddleWidth( 100.f ), paddleHeight( 20.f ), paddleVelocity( 8.f );


Put each declaration in its own line. paddleVelocity is not even related to the sizes, so one less reason for that not be coupled in the same line.

Also, you're generally consistent by initializing all variables with the { } syntax, but those constants at the top are using parentheses.

Don't use mutable global data

It might be tempting to declare a few global variables when the code is small, but at scale, they are a sure recipe for bugs. Use function parameters to pass data around as much as you can. It will make dependencies explicit and easy to track down. isPlaying is actually only used inside main, so it doesn't even make sense for it to be a global.

The global integer x also seems unused. But worse, main apparently redeclares it, so that's probably an artifact from a previous version of the code. Always compile your code with the highest warning levels available. They will remind you of this kind of mistakes, like unused variables and names shadowed by legal but conflicting declarations.

Implement proper inheritance

The class Rectangle serves as base for two other classes. You could actually implement both Paddle and Brick by using composition instead, e.g. just by making them have a Rectangle member instead of being rectangles, this tends to make things simpler in the long run.

Nevertheless, when you do use inheritance, you have to decide how the base class destructor is going to be declared. There are two options:

-
If child classes can be deleted from a base class pointer (e.g. delete a Rectangle instance that is in fact a Brick), then the base class must have a public virtual destructor.

-
If you'd like to disallow deleting an instance from a base class pointer, or you don't need this feature, them you can make the base destructor protected and non-virtual.

Side note: You're actually using struct for your object types. As far as the language is concerned, there's no distinction between structs and classes, besides the default access level. However, the usual convention is to use class for polymorphic types or types with some behavior/logic and leave struct for Plain-Old-Data (POD) types.

Use const methods

Your helpers and accessors in Rectangle and Ball are not modifying member variables, so they should be const methods, to both allow them being called on a const instance of the classes and also to clearly convey that info to the readers of your code. More about it here.

Related are functions that take read-only parameters by reference, like isIntersecting. This method should not change the objects passed in, so it should take them by const reference:

bool isIntersecting(const T1& mA, const T2& mB) {
                    ^^^^^         ^^^^^


Please avoid goto

The fact that you are resorting to goto is a clear sign that main is overly complicated and needs immediate refactoring. Rethink it by breaking the distinct logic sections into separate functions and get rid of those gotos by using structured loops.

Default { } can help readability

The lack of { } in these nested loops added to that line broken at the start of the function gives the wrong idea, from a quick glance, about the structure of this piece of code. My suggestion is to add default curly braces when you have more complex statements like nested loops, even if the inner statement is a one-liner

for (int iX{ 0 }; iX < countBlocksX; ++iX)
     for (int iY{ 0 }; iY < countBlocksY; ++iY)
         bricks.emplace_back(
             (iX + 1) * (blockWidth + 3) + 22, (iY + 2) * (blockHeight + 3));


Suggested:

for (int iX{ 0 }; iX < countBlocksX; ++iX)
{
    for (int iY{ 0 }; iY < countBlocksY; ++iY)
    {
        bricks.emplace_back(
            (iX + 1) * (blockWidth  + 3) + 22, 
            (iY + 2) * (blockHeight + 3));
    }
}

Code Snippets

constexpr float paddleWidth( 100.f ), paddleHeight( 20.f ), paddleVelocity( 8.f );
bool isIntersecting(const T1& mA, const T2& mB) {
                    ^^^^^         ^^^^^
for (int iX{ 0 }; iX < countBlocksX; ++iX)
     for (int iY{ 0 }; iY < countBlocksY; ++iY)
         bricks.emplace_back(
             (iX + 1) * (blockWidth + 3) + 22, (iY + 2) * (blockHeight + 3));
for (int iX{ 0 }; iX < countBlocksX; ++iX)
{
    for (int iY{ 0 }; iY < countBlocksY; ++iY)
    {
        bricks.emplace_back(
            (iX + 1) * (blockWidth  + 3) + 22, 
            (iY + 2) * (blockHeight + 3));
    }
}

Context

StackExchange Code Review Q#110816, answer score: 4

Revisions (0)

No revisions yet.