Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: Yet Another Snake Clone  (Read 2618 times)

0 Members and 1 Guest are viewing this topic.

priyeshgeete

  • Newbie
  • *
  • Posts: 4
    • View Profile
    • Email
Yet Another Snake Clone
« 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
Snake Code - 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.

FRex

  • Hero Member
  • *****
  • Posts: 1845
  • Back to C++ gamedev with SFML in May 2023
    • View Profile
    • Email
Re: Yet Another Snake Clone
« Reply #1 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.
Back to C++ gamedev with SFML in May 2023

priyeshgeete

  • Newbie
  • *
  • Posts: 4
    • View Profile
    • Email
Re: Yet Another Snake Clone
« Reply #2 on: August 21, 2017, 09:08:25 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. :)
« Last Edit: August 21, 2017, 09:15:05 pm by priyeshgeete »

FRex

  • Hero Member
  • *****
  • Posts: 1845
  • Back to C++ gamedev with SFML in May 2023
    • View Profile
    • Email
Re: Yet Another Snake Clone
« Reply #3 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
Back to C++ gamedev with SFML in May 2023

priyeshgeete

  • Newbie
  • *
  • Posts: 4
    • View Profile
    • Email
Re: Yet Another Snake Clone
« Reply #4 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
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. :)

FRex

  • Hero Member
  • *****
  • Posts: 1845
  • Back to C++ gamedev with SFML in May 2023
    • View Profile
    • Email
Re: Yet Another Snake Clone
« Reply #5 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.
Back to C++ gamedev with SFML in May 2023

priyeshgeete

  • Newbie
  • *
  • Posts: 4
    • View Profile
    • Email
Re: Yet Another Snake Clone
« Reply #6 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. :)