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

Rendering the periodic table in C++ using SFML

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

Problem

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> drawList;
}

std::vector>& getDrawList()
{
    return drawList;
}


Element.cpp constructor and destructor

#include "Element.hpp"

#include 

Element::Element()
{
    drawable = std::shared_ptr( 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(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 obj : getDrawList()) {
            window.draw(*obj);
        }

        window.display();
    }
}

Solution

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>


uses 3 types with namespaces. If you did this:

typedef std::shared_ptr sharedDrawable;


Then your new type becomes:

std::vector


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

using sharedDrawableVector = std::vector>;


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 . What does it mean? What is it? Why ? 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.

Code Snippets

std::vector<std::shared_ptr<sf::Drawable>>
typedef std::shared_ptr<sf::Drawable> sharedDrawable;
std::vector<sharedDrawable>
using sharedDrawableVector = std::vector<std::shared_ptr<sf::Drawable>>;
const sf::Vector2f kRectOriginVector {49, 49}; // Or whatever it means

Context

StackExchange Code Review Q#155713, answer score: 3

Revisions (0)

No revisions yet.