DryZn / Bodee-and-Bopdee

Todee and Topdee but better
1 stars 0 forks source link

Review #2

Open yassha opened 3 months ago

yassha commented 3 months ago

Replace this raw pointer with smart one. https://github.com/DryZn/Bodee-and-Bopdee/blob/25f903222d6d5b49822ff49e30fa55f690810b6d/src/Main.cpp#L4

std::unique_ptr<GameLoop> game = std::make_unique<GameLoop>();

And replace this loop with a ranged base loop: https://github.com/DryZn/Bodee-and-Bopdee/blob/25f903222d6d5b49822ff49e30fa55f690810b6d/src/Map.cpp#L7

for (auto& obstacle : _obstacles) {
    obstacle.update();
  }

For the header files, you should always include guards to prevent multiple inclusions, example for "displayable.h" :

#ifndef DISPLAYABLE_H
#define DISPLAYABLE_H
// Your code here
#endif // DISPLAYABLE_H

Always use const when it's possible, the "img_path" parameter can be const, it's not modified inside the function https://github.com/DryZn/Bodee-and-Bopdee/blob/25f903222d6d5b49822ff49e30fa55f690810b6d/src/Displayable.cpp#L9

SDL_Texture *loadTexture(const std::string &img_path) const;

Another place where it's good to use smart pointers: https://github.com/DryZn/Bodee-and-Bopdee/blob/25f903222d6d5b49822ff49e30fa55f690810b6d/src/Displayable.cpp#L11 https://github.com/DryZn/Bodee-and-Bopdee/blob/25f903222d6d5b49822ff49e30fa55f690810b6d/src/Displayable.cpp#L15 https://github.com/DryZn/Bodee-and-Bopdee/blob/25f903222d6d5b49822ff49e30fa55f690810b6d/src/GameLoop.cpp#L11

You should always exist your function when there is a fatal error, no need to continue here: https://github.com/DryZn/Bodee-and-Bopdee/blob/25f903222d6d5b49822ff49e30fa55f690810b6d/src/GameLoop.cpp#L9 use this after the error print:

retrun;

Here you can just set it to true directly: https://github.com/DryZn/Bodee-and-Bopdee/blob/25f903222d6d5b49822ff49e30fa55f690810b6d/src/GameLoop.cpp#L37

close = true;

You can use a default parameter here to avoid duplicating the constructor https://github.com/DryZn/Bodee-and-Bopdee/blob/25f903222d6d5b49822ff49e30fa55f690810b6d/src/GameObject.cpp#L3 You can just add this in the declaration of the CTOR :

GameObject::GameObject(Config *config, const char *img_path = nullptr, int w, int h,
                       int x, int y, double hbX1, double hbY1, double hbX2,
                       double hbY2);
DryZn commented 2 months ago

@yassha thanks a lot ;)

DryZn commented 1 month ago

@yassha "For the header files, you should always include guards to prevent multiple inclusions" Isn't it already made by the "pragma once" instruction I put at the top of each header?

yassha commented 1 month ago

@yassha "For the header files, you should always include guards to prevent multiple inclusions" Isn't it already made by the "pragma once" instruction I put at the top of each header?

True, the #pragma once is the modern way in simple way of doing that. It is good to use it in a complexe software. But the include guards are the old way and the safety option there, because you re sure that is compatible with all compiler, and for a small project I prefer using it.