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
#Named Types OneOne 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++!
#Use C++!
You'reYou'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
#Magic Numbers
InIn 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.
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.
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.
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.