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\$
  • \$\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 '15 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 '15 at 3:25
  • \$\begingroup\$ @jliv902 .. i agree \$\endgroup\$ – MORTAL Feb 17 '15 at 3:44
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\$
  • \$\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 '15 at 10:24
  • \$\begingroup\$ @aPyDeveloper .. sure,,, i edited my answer. \$\endgroup\$ – MORTAL Feb 17 '15 at 11:01
  • \$\begingroup\$ I find you use of enums disturbing. \$\endgroup\$ – Jonas Schäfer Feb 17 '15 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\$ – Jonas Schäfer Feb 17 '15 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 '15 at 18:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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