Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

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.

Source Link
user1118321
  • 11.9k
  • 1
  • 20
  • 46

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.

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /