[CODE REVIEW REQUEST] Pong clone with C++ and SFML

Started by
3 comments, last by Zakwayda 4 years, 7 months ago

Hello,
I'm following this article suggestions in order to improve my basic gamedev knowledge. 

I've completed the first game in the list: Pong, using C++ and SFML. The thread suggests to ask for code review after finishing a project, so this is the game repo: 
https://github.com/MaizeDaniele/Pong

 

Let me know what I should have done differently.

Thanks in advance,
MaizeDaniele
 

Advertisement
25 minutes ago, fleabay said:


Based on the code, you either have some experience as a programmer or you followed a tutorial. Which one?

 

I've used Unity for a while, developed some basic 2D mobile games. 
I'm studying some patterns from:
http://gameprogrammingpatterns.com/
for example I tried to use the command pattern for input, and the game event pattern. 

I wanted to improve my C++ knowledge. I read a basic book on C++ and some introductory tutorial on the language (ex TheChernoProject C++), but I didn't followed a tutorial specific on pong with sfml. 

By "first game" I meant the first on the list.

8 minutes ago, fleabay said:

I didn't mean to sound like I was calling you out. :D

Oh sorry, I didn't want you to think that ?
I just wanted some suggestions about the c++ project, because I've never built a game without an engine (ex. Unity) before.  

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 :)

This topic is closed to new replies.

Advertisement