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

Author Topic: I made a basic retro snake game and would like some advice on my code please  (Read 690 times)

0 Members and 1 Guest are viewing this topic.

KrySoar

  • Newbie
  • *
  • Posts: 3
    • View Profile
I'm going back into programming after several years, so I decided to make a simple snake game to begin, could you give me some advice on my way of programming please ?

The code is here : https://github.com/KrySoar/SnakeGame

eXpl0it3r

  • SFML Team
  • Hero Member
  • *****
  • Posts: 10279
    • View Profile
    • development blog
    • Email
Welcome back to the land of programming! :)

Here are some comments:
  • Using operator overload for the Snake is kind of neat, but also quite misleading and not very readable - you have to know what snake++ really does. Instead you might just want to implement a method with a name, that makes the functionality clear.
  • You're mixing tabs and spaces, might want to decide for one :D
  • You call new BodyPart() but never call delete. I recommend to not use manual memory management at all. In the situation of the body parts, you could just go with a std::vector<BodyPart> and don't put the head into the vector
  • You could derive the Snake from sf::Drawable and implement the draw method, so you can write window.draw(snake) instead
  • You should use the <random> header instead of rand(), then again for such a small thing it might be okay
  • One potential "improvement" is to pass a direction vector instead of letters for the move direction. Maybe you can declare some constants for Up, Down, Left, Right
Official FAQ: https://www.sfml-dev.org/faq.php
Official Discord Server: https://discord.gg/nr4X7Fh
——————————————————————
Dev Blog: https://dev.my-gate.net/

KrySoar

  • Newbie
  • *
  • Posts: 3
    • View Profile
Welcome back to the land of programming! :)

Here are some comments:
  • Using operator overload for the Snake is kind of neat, but also quite misleading and not very readable - you have to know what snake++ really does. Instead you might just want to implement a method with a name, that makes the functionality clear.
  • You're mixing tabs and spaces, might want to decide for one :D
  • You call new BodyPart() but never call delete. I recommend to not use manual memory management at all. In the situation of the body parts, you could just go with a std::vector<BodyPart> and don't put the head into the vector
  • You could derive the Snake from sf::Drawable and implement the draw method, so you can write window.draw(snake) instead
  • You should use the <random> header instead of rand(), then again for such a small thing it might be okay
  • One potential "improvement" is to pass a direction vector instead of letters for the move direction. Maybe you can declare some constants for Up, Down, Left, Right
Thank you so much for you review,
-I changed the operator override to a function named grow(),
- and for the spaces mixed with tabs I think it's only on some lines (I'm new to vim so maybe I did something wrong).
-For the vector<BodyPart*> I thought that I needed to do that to later modify the bodypart but I changed it to vector<BodyPart> and it works.
-Yes for the random I know that this is deprecated but I wanted something simple and not very very random
-and for the direction I made an enum in constvars.hpp.
-For the next project I will derive some classes from sf::Drawable or sf::RectangleShape (I didn't think of that but this is so much better you are right) thank you for the time you took for me !

eXpl0it3r

  • SFML Team
  • Hero Member
  • *****
  • Posts: 10279
    • View Profile
    • development blog
    • Email
Cool that you directly applied my feedback! :)

Three more things I just saw after checking the code again
  • for(BodyPart bp : snakeBody) this should be a BodyPart& otherwise, you're constantly creating a copy, but instead you want a reference
  • Instead of snakeBody.push_back(snakeHead); you could snakeBody.emplace_back(snakeHead);. It does the same thing basically, but it uses the move constructor to move the snakeHead into the vector.
  • If you don't use sf::Drawable, then I recommend to change the draw signature to take a reference to the window, instead of a raw pointer: draw(sf::RenderWindow& win)
Official FAQ: https://www.sfml-dev.org/faq.php
Official Discord Server: https://discord.gg/nr4X7Fh
——————————————————————
Dev Blog: https://dev.my-gate.net/

KrySoar

  • Newbie
  • *
  • Posts: 3
    • View Profile
Cool that you directly applied my feedback! :)

Three more things I just saw after checking the code again
  • for(BodyPart bp : snakeBody) this should be a BodyPart& otherwise, you're constantly creating a copy, but instead you want a reference
  • Instead of snakeBody.push_back(snakeHead); you could snakeBody.emplace_back(snakeHead);. It does the same thing basically, but it uses the move constructor to move the snakeHead into the vector.
  • If you don't use sf::Drawable, then I recommend to change the draw signature to take a reference to the window, instead of a raw pointer: draw(sf::RenderWindow& win)

Thank you !

 

anything