6
\$\begingroup\$

I am learning C++ and SFML so I want to know if I am laying my code out neatly and if my code is efficient. I know this is not a full game yet but I want to know if I am on the right tracks.

#include <SFML/Graphics.hpp>
#include "ResourcePath.hpp"

#include <iostream>

class Paddel {
    int speed = 3;
    sf::Vector2f border;
    sf::Vector2f size;
    sf::Vector2f position;
    sf::Vector2f screenSize;
public:
    sf::RectangleShape shape;

    Paddel(sf::Vector2f screenSize,int player)
    {
        this->screenSize = screenSize;
        this->size = sf::Vector2f(20,100);
        this->border = sf::Vector2f(8,6);

        if(player == 1)
            this->position = border;
        else if(player == 2) {
            position.x = (screenSize.x - size.x) - border.x;
            position.y = border.y;
        }

        this->shape.setSize(this->size);
        this->shape.setPosition(this->position);
    }

    void moveUp()
    {
        this->position.y -= this->speed;
        if(this->position.y < this->border.y)
            this->position.y += this->speed;
        this->shape.setPosition(this->position);
    }

    void moveDown()
    {
        this->position.y += this->speed;
        //100 - (600 + 5)
        if(this->position.y > (this->screenSize.y - this->border.y) - this->size.y)
            this->position.y -= this->speed;
        this->shape.setPosition(this->position);
    }
};

using std::cout;
using std::endl;


int main()
{
    sf::Vector2f screenSize(800,600);
    sf::RenderWindow Window(sf::VideoMode(screenSize.x,screenSize.y),"Ping Pong");
    Window.setFramerateLimit(60);
    Window.setVerticalSyncEnabled(true);

    Paddel playerOne(screenSize,1);
    Paddel playerTwo(screenSize,2);

    const sf::Time TimePerFrame = sf::seconds(1.f/60.f);
    sf::Clock clock;
    sf::Time timeSinceLastUpdate = sf::Time::Zero;
    sf::Time elapsedTime;

    bool play = true;

    while(Window.isOpen())
    {
        Window.clear();
        sf::Event Event;

        while(Window.pollEvent(Event))
        {
            if(Event.type == sf::Event::Closed)
                Window.close();
        }

        elapsedTime = clock.restart();
        timeSinceLastUpdate += elapsedTime;
        if(play){
            if(timeSinceLastUpdate > TimePerFrame){
                if(sf::Keyboard::isKeyPressed(sf::Keyboard::W))
                {
                    playerOne.moveUp();
                }
                if(sf::Keyboard::isKeyPressed(sf::Keyboard::S))
                {
                    playerOne.moveDown();
                }

                if(sf::Keyboard::isKeyPressed(sf::Keyboard::Up))
                {
                    playerTwo.moveUp();
                }
                if(sf::Keyboard::isKeyPressed(sf::Keyboard::Down))
                {
                    playerTwo.moveDown();
                }

                if(sf::Keyboard::isKeyPressed(sf::Keyboard::P))
                    play = false;
            }

            Window.draw(playerOne.shape);
            Window.draw(playerTwo.shape);

            Window.display();
        }
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Since I have only got up-votes does this mean that my code is efficient and that my code is displayed neatly or does this mean that someone else has to answer? \$\endgroup\$
    – iProgram
    Feb 16, 2015 at 21:48
  • 2
    \$\begingroup\$ Looking at all the other SFML questions, it seems Code Review does not currently have enough active users who use this library. Unfortunately, people who are unfamiliar with the library may shy away from providing a review. You can help build the SFML community on Code Review by continuing to ask great questions and by reviewing or even partially reviewing other questions or even your own questions. \$\endgroup\$
    – jliv902
    Feb 17, 2015 at 3:25
  • \$\begingroup\$ @jliv902 .. i agree \$\endgroup\$
    – MORTAL
    Feb 17, 2015 at 3:44

1 Answer 1

2
\$\begingroup\$

your code is fine but I would go with some thing like this, i have commented the changes

#include <SFML/Graphics.hpp>
#include <cstdint>

// for eliminating magic numbers
enum class Players
{
    PlayerOne,
    PlayerTow
};

enum class ScreenSize
{
    Width = 800,
    Height = 600
};

// SFML has tiny class for rendering purpose, it has only a one virtual fucntion 
class Paddel : public sf::Drawable
{
    // for member data perfered tobe started with m prefix 
    float mSpeed; // here is an optional you may use integer value
    sf::Vector2f mBorder;
    sf::Vector2f mPosition;
    sf::Vector2u mScreenSize;
    sf::RectangleShape mShape; // <-- now it's private
public:

    Paddel(sf::Vector2u screenSize, Players player)
        // member data perfered be initilaized as contructor's initilaized list
        : mScreenSize(screenSize)
        , mBorder(8, 6)
        , mSpeed(3.f)
    {
        sf::Vector2f size = sf::Vector2f(20, 100);


        switch (player)
        {
        case Players::PlayerOne:
            mPosition = mBorder;
            break;
        default:
            mPosition.x = (mScreenSize.x - size.x) - mBorder.x;
            mPosition.y = mBorder.y;
        }

        mShape.setSize(size);
        mShape.setPosition(mPosition);
    }

    void moveUp()
    {
        mPosition.y -= mSpeed;
        if (mPosition.y < mBorder.y)
            mPosition.y += mSpeed;
        mShape.setPosition(mPosition);
    }

    void moveDown()
    {
        mPosition.y += mSpeed;
        //100 - (600 + 5)
        if (mPosition.y >(mScreenSize.y - mBorder.y) - mShape.getSize().y)
            mPosition.y -= mSpeed;
        mShape.setPosition(mPosition);
    }

    // sf::Drawable has singe virtual fucntion for rendering, override it 
    void draw(sf::RenderTarget& target, sf::RenderStates states) const override
    {
        // sf::RectangleShape has its own defualt RenderStates
        target.draw(mShape, states);
    }
};

int main()
{
    // unfortunately strong type enumeration doesn't decay to integer value, so that we need to static cast it to integer type 
    // here we don't want to have a negative value for size, we should use unsigned int or use std::uint32_t which is declared 
    // in <cstdint> header file if you considered C++11 features.
    sf::Vector2u screenSize(static_cast<std::uint32_t>(ScreenSize::Width), static_cast<std::uint32_t>(ScreenSize::Height));
    sf::RenderWindow Window(sf::VideoMode(screenSize.x, screenSize.y), "Ping Pong");

    Window.setFramerateLimit(60);
    Window.setVerticalSyncEnabled(true);

    Paddel playerOne(screenSize, Players::PlayerOne); 
    Paddel playerTwo(screenSize, Players::PlayerTow);

    const sf::Time TimePerFrame = sf::seconds(1.f / 60.f);
    sf::Clock clock;
    sf::Time timeSinceLastUpdate = sf::Time::Zero;
    sf::Time elapsedTime;

    bool play = true;

    while (Window.isOpen())
    {
        Window.clear();
        sf::Event Event;

        while (Window.pollEvent(Event))
        {
            if (Event.type == sf::Event::Closed)
                Window.close();
        }

        elapsedTime = clock.restart();
        timeSinceLastUpdate += elapsedTime;

        if (play)
        {
            if (timeSinceLastUpdate > TimePerFrame)
            {
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::W))
                {
                    playerOne.moveUp();
                }
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::S))
                {
                    playerOne.moveDown();
                }

                if (sf::Keyboard::isKeyPressed(sf::Keyboard::Up))
                {
                    playerTwo.moveUp();
                }
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::Down))
                {
                    playerTwo.moveDown();
                }

                if (sf::Keyboard::isKeyPressed(sf::Keyboard::P))
                    play = false;
            }

            Window.draw(playerOne); // <-- we can draw Paddel object directly
            Window.draw(playerTwo);

            Window.display();
        }
    }
}
\$\endgroup\$
10
  • \$\begingroup\$ Could you explain to me what static_cast<std::uint32_t>(ScreenSize::Width) does or can't you on CR? also why have you changed speed to a float when it is always going to be a int? Thanks \$\endgroup\$
    – iProgram
    Feb 17, 2015 at 10:24
  • \$\begingroup\$ @aPyDeveloper .. sure,,, i edited my answer. \$\endgroup\$
    – MORTAL
    Feb 17, 2015 at 11:01
  • \$\begingroup\$ I find you use of enums disturbing. \$\endgroup\$ Feb 17, 2015 at 11:10
  • \$\begingroup\$ Sorry, missed my edit grace period by thinking too long about it: Why don’t you use static constants in namespaces or classes? The fact that you need an explicit cast here for something which intuitively should decay (an integer constant to integer) should set off an alarm. \$\endgroup\$ Feb 17, 2015 at 11:15
  • 1
    \$\begingroup\$ @aPyDeveloper .. it's matter of choice if you feel comfortable with using enum class thats fine or you can use regular enum instead ... btw mSpeed better tobe float for later use, to get what i mean, SFML has pong example written by auther of SFML library. here a link to how they did it - github.com/MORTAL2000/Ping-Pong/tree/master \$\endgroup\$
    – MORTAL
    Feb 17, 2015 at 18:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.