SFML community forums

General => SFML projects => Topic started by: priyeshgeete on August 20, 2017, 06:09:59 pm

Title: Yet Another Snake Clone
Post by: priyeshgeete on August 20, 2017, 06:09:59 pm

Hello all. I have just started learning sfml and I'm c++beginner: pointers give me headaches kind of beginner.

To practice I started making Snake, like many other beginners, and just finished it.

Snake Game - https://drive.google.com/file/d/0B_XEonRcHGvwNnhXTVFHMEJGSEE/view?usp=sharing (https://drive.google.com/file/d/0B_XEonRcHGvwNnhXTVFHMEJGSEE/view?usp=sharing)
Snake Code - https://github.com/gitPriyesh/Snake (https://github.com/gitPriyesh/Snake)

I am requesting you all to go through the code and guide me what could be better and how.
Also, please play the game and see if its working alright.

I found practicing through making small games to be really a good learning experience.

Title: Re: Yet Another Snake Clone
Post by: FRex on August 21, 2017, 07:55:57 pm

You seem to have forgotten stdafx.h in the repo, it's also not a very usual/portable feature (I know it's in VS, not sure about it on GCC/Clang, I think it's present but I don't know how it works) so it's best avoided IMO.

At a glance the code looks okay but it's a bit messy C++, I'll try to take a better look at it later.

Title: Re: Yet Another Snake Clone
Post by: priyeshgeete on August 21, 2017, 09:08:25 pm

Quote from: FRex on August 21, 2017, 07:55:57 pm

You seem to have forgotten stdafx.h in the repo, it's also not a very usual/portable feature (I know it's in VS, not sure about it on GCC/Clang, I think it's present but I don't know how it works) so it's best avoided IMO.

I purposefully submitted only those files which were necessary for reviewing the code, I didn't think if anyone would need to compile the code. Should stdafx.h be avoided in my projects? What about this other header file "targetver.h"which VS includes by default?

Quote

At a glance the code looks okay but it's a bit messy C++, I'll try to take a better look at it later.

Any guidance is awaited and will be appreciated. Thanks. :)

Title: Re: Yet Another Snake Clone
Post by: FRex on August 22, 2017, 04:27:43 pm

Here it is, it's a lot but don't take it personally and you did great for your first C++ game. ;)

Overall:
- stdafx.h I mentioned before
- you really like shorts and uint8 for some reason, just use ints and unsigned (ints) and char and unsigned char as appropriate but don't optimize for size/efficiency like that, there is no point
- names in all caps and starting with _ shouldn't be used (but many projects do it to not clutter auto complete with them, yes), see: https://stackoverflow.com/a/228797
- some magic numbers are present, ie. 735 and 375 in Food.cpp
- it's not an error but it became a convention to put public members first and private ones second (or third if there are protected ones) in classes in C++
- it's also not an error but many people use hpp for C++ headers to signify they aren't C headers, if you ever mix C and C++ or happen to use a library that has binding/APIs in both (i.e. like SFML does) then you will appreciate the difference
- you could use sf::RenderTaraget, not sf::RenderWindow in draw functions, or even make your classes inherit from sf::Drawable

Snake.cpp:
- it's not an error but it's customary and nice towards the reader to name the file with main function main.cpp (or main.c in C)
- you don't need to restart clock right after it's created
- you might want to look into c++ <random> header instead of using C randomness, both are as easy for int but c++ is easier for floats and more featureful and correct for ints
- you don't need to comment //creating player before calling player.create(), it's self evident from the function what's going on
- you don't need braces around case values, case sf::Event::Closed: will do
- your key handling is wrong, you should use evnt.key.code to see which key got pressed in sf::Event::KeyPressed, not call sf::Keyboard::isKeyPressed which are for realtime keyboard polling, since your loop is quick the result is the same but that's wrong way to do it according to how SFML is made
- the above would also allow you to use switch to pick level
- you might actually reconsider using numeric keypad and use normal numeric keys (Num1, etc.) since many older laptops don't have it (mine does, but still), such a key->level number function is also a good candidate to put in its own function since its independent from other code, etc.
- in general you might want to put that stuff into its own class and call methods to draw, update, etc. since such a long main function is just hard to follow and there is no downside to using a class or a few functions
- you can compare sf::Vectors of same kind with just ==, no need to compare x and y separately and comparping floats for equality might be a bad idea, snake's logic is very tight and fully doable/designed with just ints so it might be better that way and just using floats to interface with SFML shapes and such

Food.h and Food.cpp:
- you might make spawn take snake by reference if it's never null
- the float constant could be a global const in the cpp file, it's not needed in the class itself, especially since it's private
-

Block.h and Block.cpp:
- just that magic constant 14.f

Player.h and Player.cpp:
- block and snake don't have m_ in front of them but you use that convention for all other normal class members across the game
- you use windows line endings usually (which is so-so, but default in VS still...) but here you mix windows and linux style line endings in the file, not an error but it shows up in some editors when there is an inconsistency
- block is esentially a useless member, you could create or emplace a block in the function intself, no need for having a member you never use or modify and only push back into the vector
- setFillColor is weird, why the & and then -> ? why not just

snake[i].setFillColor

or even better a ranged for from C++11?
- vector has a 'back' member that you can use to access the last element, no need for smt[smt.size() - 1] and they are both exactly the same in terms of safety (that is - they don't check if vector isn't empty)
- in drawTo you use a for loop but without a & so you copy each block, then render it, then throw it away
- a few magic numbers again, 195, 735, 375
- deletePlayer and createPlayer are a bad interface, just clear the snake on each player creation, there is no downside to that and it's cleaner, if you need to create player twice without deletion in between then you can add a bool argument dontdelete that is false by default or use a separate function, the only purpose deletePlayer serves right now is to be forgotten and cause an unusual bug

Title: Re: Yet Another Snake Clone
Post by: priyeshgeete on August 23, 2017, 04:54:29 pm

FRex, Thanks for your detailed feedback. I might not have grasped all the points at once but they are all appreciated and didn't get unnoticed.

Quote

names in all caps and starting with _ shouldn't be used (but many projects do it to not clutter auto complete with them, yes), see: https://stackoverflow.com/a/228797 (https://stackoverflow.com/a/228797)

I am trying to understand everything in that post but I think I get the point. In my case, my header guards should not have started with an underscore. Did I get that right?

Quote

many people use hpp for C++ headers to signify they aren't C headers, if you ever mix C and C++ or happen to use a library that has binding/APIs in both (i.e. like SFML does) then you will appreciate the difference

Alright, its .hpp from now on.

Quote

you could use sf::RenderTaraget, not sf::RenderWindow in draw functions

So, it can be like

Code: [Select]

drawableObject.drawTo(sf::RenderTarget &window); Can you tell me the difference it will make?

Quote

some magic numbers are present, ie. 735 and 375 in Food.cpp

Noted: Hard coding should be avoided.

Quote

it's customary and nice towards the reader to name the file with main function main.cpp

Makes sense.

Quote

you might want to look into c++ <random> header instead of using C randomness

Well, I tried using that but it gave me same number pattern when the game restarts. May be I should have researched more to find a way to put current time as a seed in  c++11 random generator.

Quote

your key handling is wrong, you should use evnt.key.code to see which key got pressed in sf::Event::KeyPressed

Well, thanks for pointing this out. I didn't know this. event.key.code for polling event and sf::Keyboard::isKeyPressed for real time key capturing.

Quote

you might actually reconsider using numeric keypad and use normal numeric keys (Num1, etc.) since many older laptops don't have it

Hahaha.. damn my short-sightedness. :D

Food.cpp

Quote

you might make spawn take snake by reference if it's never null

Err. I am confused.

Block.cpp

Quote

setFillColor is weird, why the & and then -> ?

I needed to do that in order to change the color of existing blocks. I also used the following code initially, but it didn't work.

Code: [Select]

snake[i].setFillColor

Quote

vector has a 'back' member that you can use to access the last element

I should have gone through the std::vector documentation properly. Well, I learned something new. Thanks

Quote

in drawTo you use a for loop but without a & so you copy each block, then render it, then throw it away

That is why code reviewing is necessary. I'll be cautious of this in future practices.

Quote

deletePlayer and createPlayer are a bad interface, just clear the snake on each player creation

Are you suggesting I should have handled creation and clearing of vector through Player's constructor? If yes, then in my case I would have to create two player objects in snake.cpp: once at the start of the game before the game loop and then once after the game is over if player chose to retry the game. I can definitely see the problem of structuring in my snake.cpp now. May be, I should have added pressing space to start the game for the first time as well. Good point.

Again thanking you for taking time to review the code, your suggestions are immensely helpful. :)

Title: Re: Yet Another Snake Clone
Post by: FRex on August 23, 2017, 09:30:42 pm

No, but you could both clear the vector and add Blocks all in createPlayer call.
The way it is now if you forget to call deletePlayer then you'll have two snakes in one or something.
And if you forget to call createPlayer after calling deletePlayer then you will be accessing elements that don't exist in addBlock, setDirection, etc. because you assume the vector of Blocks isn't empty.

As for references, they are a C++ thing, it's (like a) pointer that can't be null nor changed (the object that it refers to/points at can be changed of course, unless it's a const reference) and behaves like a value:

void somefunc(SomeClass& someobj)
{
someobj.somefunc(); //not -> !
someobj[0].smt = 10; //no need for (*someobj) before [0]
}
 

SFML uses them all over the place too and to implement many operators, comparator in std::sort, etc. you have to (or at least should) use them.

Title: Re: Yet Another Snake Clone
Post by: priyeshgeete on August 24, 2017, 11:56:51 am

Alright, FRex. I cannot thank you enough for your valuable feedback. I'll try to keep all these things in mind. :)