3 hours ago, MaizeDaniele said:
The thread suggests to ask for code review after finishing a project, so this is the game repo:
This is all just low-hanging fruit based on a quick look at some of the code (some of the things pointed out here come up pretty commonly in code review requests):
- In C++, the use of the preprocessor for constants is generally discouraged for various reasons (at or towards the top of that list would probably be that preprocessor macros are scope-unaware). C++ provides (arguably) better tools for this, such as 'const' and 'constexpr'.
- Very minor, but I'd suggest expressing the 'degree to radians' constant in terms of pi (e.g. 'pi / 180.0') rather than expressing it as a literal. Since it's dependent on a constant you've already defined, there may be no particular reason to express it directly (which, among other things, runs the risk of getting it wrong). Technically you could probably also get pi from a trig function, although that's probably less important. (Although the chances of getting these constants wrong is obviously low, the general goal here is to minimize the number of hard-coded values - especially values that have invariant relationships to one another - and therefore reduce the potential for error.)
- It's idiomatic to pass instances of non-trivial types like std::string by reference (constant reference if mutability isn't required, which I would think is usually the case), which may avoid some overhead.
- How much to use 'const' is a matter of taste and opinion, but if you want (this is my preference), there are some things you can or might be able to make constant that aren't currently (e.g. immutable function arguments, member fields, locals, etc.).
- In the 'Game' class, you're creating and deleting m_GameSceneManager manually, but it would probably be safer and more idiomatic to use e.g. std::unique_ptr for this. Among other things, this would remove the need for deleting it explicitly in the destructor and elsewhere.
- '#pragma once' is widely supported, but technically non-standard. Just something to be aware of, depending on what your portability goals are.
- Here:
if (m_GameSceneManager == nullptr)
m_GameSceneManager = ...;
You have a control structure without braces. Opinions may differ on this, but it's often recommended to always include the braces (one reason being that if you decide to add additional statements later, you won't run the risk of introducing bugs by forgetting to add the braces, and also won't have to go to the trouble of adding them after the fact at all).
- Just as a point of interest, 'return 0' in main() is the default behavior, so its inclusion is optional. I suppose some might recommend including it for clarity regardless (as you're doing).
There are probably some other things, but I didn't look at all the code, and in any case that's probably enough for one post. Overall it looks very good