patterncppMinor
Rendering the periodic table in C++ using SFML
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
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
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
Element.cpp constructor and destructor
Client.cpp draw function
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:
uses 3 types with namespaces. If you did this:
Then your new type becomes:
Or even better, use the new
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
Magic Numbers
In the constructor of
Then use that in the constructor.
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::vectorOr 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 meansThen 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 meansContext
StackExchange Code Review Q#155713, answer score: 3
Revisions (0)
No revisions yet.