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

SFML Ping Pong improved (V0.3)

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

Problem

Some of you may have seen my previous post asking if my code was efficient and easy to read.

I have made some more improvements to it by adding sounds, visual count down and checking if the window has focus. Before I carry on with my program I would like to know if this is efficient and easy to read.

Here is the zip file containing all the resources.

```
#include
#include
#include "ResourcePath.hpp"
#include
#include

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

//Version
std::string ver = "V0.3";
//Create Text and font variables
sf::Text text;
sf::Font font;

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

//Make this class Drawable
class Paddel : public sf::Drawable
{
// for member data perfered to be started with m prefix
float mSpeed;
sf::Vector2f mBorder;
sf::Vector2f mPosition;
sf::Vector2u mScreenSize;
sf::RectangleShape mShape;
public:

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

//Which player
switch (player)
{
case Players::PlayerOne:
//If player one set position to border
mPosition = mBorder;
break;
default:
//Else set positionX to (screenWidth - paddelWidth) - borderWidth
mPosition.x = (mScreenSize.x - size.x) - mBorder.x;
//positionY = borderY
mPosition.y = mBorder.y;
}
//Set size and position of drawable shape
mShape.setSize(size);
mShape.setPosition(mPosition);
}

void moveUp()
{
//Get closer to 0 (Top of screen)
mPosition.y -= mSpeed;
//If gone too far add speed
if (mPosition.y (mScreenSize.y - mBorder.y) - mShape.getSize().y)
mPosition.y -=

Solution

few thing will come in handy for your game at this point

Resource Management

resources are heavyweight multimedia items, such as images, music themes,
or fonts. "Heavyweight" refers to the fact that those objects occupy a lot of memory,
and that operations on them, especially copying, perform slowly. This affects the
way we use them in your application, as we try to restrict slow operations on them to
a minimum. Therefore, we have decided which resources are required by the application, the next step is to investigate how long and by whom they are used. This allows us to decide how
the resources are stored in the application, as well as who is responsible of loading
and releasing them.

  • We want to load the resource in advance, for example, at the time the game


starts or the player begins a new level. In contrast to loading on demand
(as soon as a resource is needed), this approach has the advantage that possible
loading times occur in the beginning and not during a game. Therefore, the
game itself remains fluent and is not interrupted because of resources

  • When resources are likely to not be needed anymore, we can release them


and free the memory. This is usually the case at the end of a level or
when the application is quit. We do not want to release resources too early
if we risk reloading them shortly after. For example, we do not release the
explosion sound buffer as soon as the sound effect is over, because the next
explosion may follow a few seconds later.

Our goal is to encapsulate the just mentioned functionality into a class that relieves
us from managing resources again and again. For resource management, the C++
idiom Resource Acquisition Is Initialization (RAII) comes in handy. like following approach

ResourceHolder.h

#ifndef RESOURCEHOLDER_H
#define RESOURCEHOLDER_H

#include 
#include 
#include 
#include 
#include 

template 
class ResourceHolder
{
public:
    void load(Identifier id, const std::string& filename);

    Resource& get(Identifier id);
    const Resource& get(Identifier id) const;

private:
    void insertResource(Identifier id, std::unique_ptr resource);

    std::map> mResourceMap;
};

#include "ResourceHolder.inl"
#endif // RESOURCEHOLDER_H


ResourceHolder.inl

template 
void ResourceHolder::load(Identifier id, const std::string& filename)
{
    // Create and load resource
    // it seems codeX doesn't support std::make_unique
    std::unique_ptr resource(new Resource());
    if (!resource->loadFromFile(filename))
        throw std::runtime_error("ResourceHolder::load - Failed to load " + filename);

    // If loading successful, insert resource to map
    insertResource(id, std::move(resource));
}

template 
Resource& ResourceHolder::get(Identifier id)
{
    auto found = mResourceMap.find(id);
    assert(found != mResourceMap.end());

    return *found->second;
}

template 
const Resource& ResourceHolder::get(Identifier id) const
{
    auto found = mResourceMap.find(id);
    assert(found != mResourceMap.end());

    return *found->second;
}

template 
void ResourceHolder::insertResource(Identifier id, std::unique_ptr resource)
{
    // Insert and check success
    auto inserted = mResourceMap.insert(std::make_pair(id, std::move(resource)));
    assert(inserted.second);
}


  • There must be a possibility to get a reference to a certain resource after it has


been loaded—using a resource identifier. This identifier (ID) could be the file
path as a std::string. This has some disadvantages: all classes that use a
resource must hardcode the path, so if it changes, a lot of code needs to be
refactored. Strings are also quite error-prone regarding typographic or case
errors. An alternative to strings are enums, where each enumerator denotes
an ID. Since an enum has a predefined set of possible states, we get some
compile-time safety, and we can handle the paths in a central place.
The best approach is going to be like following script:

ResourceIdentifiers.h

#ifndef RESOURCEIDENTIFIERS_H
#define RESOURCEIDENTIFIERS_H

// Forward declaration of SFML classes
namespace sf
{
    class SoundBuffer;
}

enum class Sounds
{
    Countdown,
    Start
};

// Forward declaration and a few type definitions
template 
class ResourceHolder;

using SoundHolder = ResourceHolder;

#endif // RESOURCEIDENTIFIERS_H


Remove compiler warning

few warnings alert due to convert from float to unsigned int, this could be solved easily by creating nums for screen size, it seems you don't like enum class, so here is an alternative approach

namespace Screen
{
    enum Size
    {
        Width = 800,
        Height = 600
    };
}


final code

```
#include
#include
//#include "ResourcePath.hpp"
#include "ResourceHolder.h"
#include "ResourceIdentifiers.h"
#include
#include

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

//Version
std::string ver = "V0.3";
//Create Text and font variables
sf::Text text;
sf::Font font;

// for eliminating magic numbers
enum class Players
{
PlayerOne

Code Snippets

#ifndef RESOURCEHOLDER_H
#define RESOURCEHOLDER_H

#include <map>
#include <string>
#include <memory>
#include <stdexcept>
#include <cassert>


template <typename Resource, typename Identifier>
class ResourceHolder
{
public:
    void load(Identifier id, const std::string& filename);

    Resource& get(Identifier id);
    const Resource& get(Identifier id) const;

private:
    void insertResource(Identifier id, std::unique_ptr<Resource> resource);

    std::map<Identifier, std::unique_ptr<Resource>> mResourceMap;
};

#include "ResourceHolder.inl"
#endif // RESOURCEHOLDER_H
template <typename Resource, typename Identifier>
void ResourceHolder<Resource, Identifier>::load(Identifier id, const std::string& filename)
{
    // Create and load resource
    // it seems codeX doesn't support std::make_unique
    std::unique_ptr<Resource> resource(new Resource());
    if (!resource->loadFromFile(filename))
        throw std::runtime_error("ResourceHolder::load - Failed to load " + filename);

    // If loading successful, insert resource to map
    insertResource(id, std::move(resource));
}

template <typename Resource, typename Identifier>
Resource& ResourceHolder<Resource, Identifier>::get(Identifier id)
{
    auto found = mResourceMap.find(id);
    assert(found != mResourceMap.end());

    return *found->second;
}

template <typename Resource, typename Identifier>
const Resource& ResourceHolder<Resource, Identifier>::get(Identifier id) const
{
    auto found = mResourceMap.find(id);
    assert(found != mResourceMap.end());

    return *found->second;
}

template <typename Resource, typename Identifier>
void ResourceHolder<Resource, Identifier>::insertResource(Identifier id, std::unique_ptr<Resource> resource)
{
    // Insert and check success
    auto inserted = mResourceMap.insert(std::make_pair(id, std::move(resource)));
    assert(inserted.second);
}
#ifndef RESOURCEIDENTIFIERS_H
#define RESOURCEIDENTIFIERS_H


// Forward declaration of SFML classes
namespace sf
{
    class SoundBuffer;
}

enum class Sounds
{
    Countdown,
    Start
};

// Forward declaration and a few type definitions
template <typename Resource, typename Identifier>
class ResourceHolder;

using SoundHolder = ResourceHolder<sf::SoundBuffer, Sounds>;

#endif // RESOURCEIDENTIFIERS_H
namespace Screen
{
    enum Size
    {
        Width = 800,
        Height = 600
    };
}
#include <SFML/Graphics.hpp>
#include <SFML/Audio.hpp>
//#include "ResourcePath.hpp"
#include "ResourceHolder.h"
#include "ResourceIdentifiers.h"
#include <cstdint>
#include <iostream>

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

//Version
std::string ver = "V0.3";
//Create Text and font variables
sf::Text text;
sf::Font font;

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


namespace Screen
{
    enum Size
    {
        Width = 800,
        Height = 600
    };
}


//Make this class Drawable
class Paddel : public sf::Drawable
{
    // for member data perfered to be started with m prefix
    float mSpeed;
    sf::Vector2f mBorder;
    sf::Vector2f mPosition;
    sf::Vector2u mScreenSize;
    sf::RectangleShape mShape;
public:

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

        //Which player
        switch (player)
        {
        case Players::PlayerOne:
            //If player one set position to border
            mPosition = mBorder;
            break;
        default:
            //Else set positionX to (screenWidth - paddelWidth) - borderWidth
            mPosition.x = (mScreenSize.x - size.x) - mBorder.x;
            //positionY = borderY
            mPosition.y = mBorder.y;
        }
        //Set size and position of drawable shape
        mShape.setSize(size);
        mShape.setPosition(mPosition);
    }

    void moveUp()
    {
        //Get closer to 0 (Top of screen)
        mPosition.y -= mSpeed;
        //If gone too far add speed
        if (mPosition.y < mBorder.y)
            mPosition.y += mSpeed;
        mShape.setPosition(mPosition);
    }

    //Do the same opposite way round
    void moveDown()
    {
        mPosition.y += mSpeed;
        if (mPosition.y >(mScreenSize.y - mBorder.y) - mShape.getSize().y)
            mPosition.y -= mSpeed;
        mShape.setPosition(mPosition);
    }

    //Override sf::Drawable function
    void draw(sf::RenderTarget& target, sf::RenderStates) const override
    {
        // sf::RectangleShape has its own defualt RenderStates
        target.draw(mShape); // <-- NOTE : shorter version 
    }
};

int main()
{
    //Initilise screen size and Window
    sf::RenderWindow Window(sf::VideoMode(Screen::Width, Screen::Height), "Ping Pong " + ver, sf::Style::Titlebar | sf::Style::Close);

    //Create Text
    bool enableText = true;
    bool countdown = false;
    // Try to load font resources
    try
    {
        font.loadFromFile(/*resourcePath() +*/ "Pacifico.ttf");
    }
    catch (std::runtime_error& e)
    {
        cout << "Exception: " << e.what() << std::endl;
        enableText = false;
    }

    text.setFont(font);
    text.setCharacterSize(50);
    text.setColor(sf::Color::White);

    //Limit frame rate and enab

Context

StackExchange Code Review Q#81837, answer score: 5

Revisions (0)

No revisions yet.