Full repository review #4
Conversation
Features/board
- Added missing texture with invalid sprites - Removed unused Inception-Engine-Logo(s) - Added placeholder chip
-- Documented 'Chip.h' -- Renamed 'Chip' class > 'Gobblet' -- Left comment on empty destructor
-- Made magic numbers/strings into constant vars -- Added some docs
-- Added a new logo, deleted irrelevant images
Features/sprite class
| @@ -0,0 +1,2 @@ | |||
| # Inception-Engine | |||
-- Added documentation to entity class
-- Added documentation in for the board class
| public: | ||
| /// <summary> | ||
| /// Board constructor | ||
| /// </summary> |
There was a problem hiding this comment.
So unless you're using some sort of documentation generator, I'm not sure what benefits using this markup will bring you. Even IDEs like visual studio can function from a single comment:
// Board ConstructorIn a big project where this is actually useful and necessary, consider adding the 'why' and 'how'. For constructors like this, your comment is fine since there's not much more to be said (begging the question if the comment is necessary in the first place), but for more complex constructors then these comments are useful for obvious reasons.
| const int BOARD_SIZE = 2; | ||
| const int BOARD_WIDTH = 180; | ||
| const int BOARD_HEIGHT = 140; | ||
| const int BOARD_SCALE = 124; |
There was a problem hiding this comment.
There's not much benefit for private constants to be in the header file, it just inflates things. The question you should be asking is where these variables are used and what benefits they bring. Constants are great but since they're private and only the implementation can access them, it might be in your interest in putting them in your cpp file instead.
Take a look at this answer on SO for further discussion. Very nitpicky, but when you get lots of constants that don't benefit readers of your header file it becomes noticeable that they don't belong!
| /// <summary> | ||
| /// Update function | ||
| /// </summary> | ||
| virtual void update() {}; |
There was a problem hiding this comment.
Not in line with your naming conventions
| sf::Texture boardTexture; | ||
| sf::Sprite boardSprite; | ||
|
|
||
| void Draw(sf::RenderWindow& window); |
There was a problem hiding this comment.
Didn't expect your Draw function to be non-const --- In SFML, drawing to the screen shouldn't change your class so your function here (and all other cosmetic functions) can be const. The thing that's being mutated is window, not your Board.
| sf::Texture boardTexture; | ||
| sf::Sprite boardSprite; | ||
|
|
||
| void Draw(sf::RenderWindow& window); |
There was a problem hiding this comment.
Just because this is private doesn't mean it doesn't need a comment --- how does this differ from DrawBoard and more importantly how does this differ from Entity::Draw?
It's confusing that you have a function of the same name but with a different set of params, yet you're not overriding virtual methods. Either make it an overrideing function or make it a separate one!
| #pragma endregion Window_Properties | ||
|
|
||
| #pragma region Default_File_Routes | ||
| std::string asset_file_route = "assets/"; |
There was a problem hiding this comment.
Lots of underscores, not your normal style!
| @@ -0,0 +1,50 @@ | |||
| #ifndef CONFIG_H | |||
There was a problem hiding this comment.
You normally use #pragma once
| /// <summary> | ||
| /// The windows title | ||
| /// </summary> | ||
| extern std::string window_title; |
There was a problem hiding this comment.
You don't really need a .cpp here, you could just put it all in your header file since there's no implementation. It makes it a hassle to look up where these values are!
| #include <string> | ||
| #include <vector> | ||
|
|
||
| namespace config |
There was a problem hiding this comment.
None of these are constant, so they could get messed up at runtime. It is usually nice to have a Config struct or something to give to your game to manage or something.
That said I love namespaces so the solution is somewhere between the two!
| // Create a graphical text to display | ||
| sf::Font font; | ||
| if (!font.loadFromFile(config::default_font_file_route)) | ||
| return EXIT_FAILURE; |
There was a problem hiding this comment.
This is usually frowned upon (having no curly braces). It's just so easy to make mistakes if you add more lines or remove any etc. Subjective, but I have yet to see anyone who prefers this.
Full repo review at the request of @AmyE123