Rendering the periodic table in C++ using SFML

Question

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();
 }
}

Answer 1

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.

• Brandon: 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.