3
\$\begingroup\$

I have created this as an example. It uses SFML as its core dependency along with JSON for modern C++ to load the elements from a file.

Basically what happens is when an object that contains an instance of sf::Drawable is created, its constructor will be responsible for adding it to the draw list. Its destructor is responsible for removing it from the list.

I am interested in if this is a good approach to dealing with the objects in my program that will need to be rendered or if I should be extending sf::Drawable class directly.

I am also interested in the overall code quality of the entire project. I am aware of the assignment and copy constructors that do nothing. I am relatively new to C++ (1 semester).

Drawables.cpp

#include "Drawables.hpp"

namespace {
    std::vector<std::shared_ptr<sf::Drawable>> drawList;
}

std::vector<std::shared_ptr<sf::Drawable>>& getDrawList()
{
    return drawList;
}

Element.cpp constructor and destructor

#include "Element.hpp"

#include <exception>

Element::Element()
{
    drawable = std::shared_ptr<sf::RectangleShape>( new sf::RectangleShape(sf::Vector2f{49, 49}) );
    drawable->setFillColor(sf::Color::Green);
    getDrawList().push_back(drawable);
}

Element::~Element()
{
    // Remove it's drawable from the drawList
    for(int i = getDrawList().size() - 1; i >= 0; --i) {
        if(dynamic_cast<sf::RectangleShape*>(getDrawList()[i].get()) == drawable.get())
        {
            getDrawList().erase(getDrawList().begin() + i);
        }
    }
}

Client.cpp draw function

void Client::draw()
{
    while(window.isOpen()) {
        window.clear();
        for(std::shared_ptr<sf::Drawable> obj : getDrawList()) {
            window.draw(*obj);
        }

        window.display();
    }
}
\$\endgroup\$
3
\$\begingroup\$

Looking at the code you've posted here, it's pretty straightforward and I'm able to understand what it does. That's great! I have a few suggestions on how to improve it.

Named Types

One common complaint about the standard template library and templates in general in C++ is the ridiculously long type names that result from using them. I see that in your code here. It's easily solved by creating new named types that represent the common types you use. For example:

std::vector<std::shared_ptr<sf::Drawable>>

uses 3 types with namespaces. If you did this:

typedef std::shared_ptr<sf::Drawable> sharedDrawable;

Then your new type becomes:

std::vector<sharedDrawable>

Or even better, use the new using directive in C++11:

using sharedDrawableVector = std::vector<std::shared_ptr<sf::Drawable>>;

Or something along those lines.

Use C++!

You're writing in C++, but you're using older methods of iterating through containers. In the destructor of Element you're going through the list manually when you could use a std::find() method instead.

Magic Numbers

In the constructor of Element you create a vector with coordinates <49, 49>. What does it mean? What is it? Why <49, 49>? You should create a constant with a name that describes what this represents:

const sf::Vector2f kRectOriginVector {49, 49}; // Or whatever it means

Then use that in the constructor.

\$\endgroup\$
  • \$\begingroup\$ I will implement the std::find() approach, and I really like the typedef suggestion. I agree that 49 (the width/height of a "periodic element card") should be stored as a constant along with some other variables in my mock-up. \$\endgroup\$ – Brandon Feb 20 '17 at 16:34

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.