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

Author Topic: critique my snake implementation? (relative beginner, source included)  (Read 3768 times)

0 Members and 1 Guest are viewing this topic.

klefmung

  • Newbie
  • *
  • Posts: 12
    • View Profile
backstory:
Only been coding a total of 3-4 months. I've done and am doing things a lot more complex than this, but because of the part of the learning curve I'm on, I feel like I always have code I'm ashamed of somewhere in there. Basically, what I've done so far is make a console based pokemon battle simulator, which turned into an SFML based pokemon fan game. (with stick figures. I'm a bad artist) That'll get posted when I'm done overhauling the battle system. (which was  one of the first things I did in C++. It's hard to expand with more move support, and is my n00biest of code)

Why this?
I wanted to put together a couple hours worth of work showing how my general coding practices and SFML use look right now, since nobody has ever looked at or critiqued my code before. I didn't "cheat" by looking at other versions of snake code, so if I missed anything obvious, feel free to yell at me. I tried to (maybe over)comment it with what I was thinking when I wrote certain pieces of code.

Major critiques welcome!
Minor nitpicks welcome!
"Oh crap, I got distracted playing snake, here's my high score!" welcome!

Thank you so much for your critiques and comments.

oh whoops! Better post the link!

Game and source included.

https://drive.google.com/open?id=1wINgyIggyIDnOwC1l7TEoBPu8UpjzCvo

or just the source from github:
https://github.com/letsdothis64/Snake

edit:
added github link
« Last Edit: January 08, 2018, 08:49:08 pm by klefmung »

ratzlaff

  • Newbie
  • *
  • Posts: 33
    • View Profile
Re: critique my snake implementation? (relative beginner, source included)
« Reply #1 on: January 08, 2018, 08:27:25 pm »
You should set this up in github so that I can look at it from my web browser.

klefmung

  • Newbie
  • *
  • Posts: 12
    • View Profile
Re: critique my snake implementation? (relative beginner, source included)
« Reply #2 on: January 08, 2018, 08:47:45 pm »
You should set this up in github so that I can look at it from my web browser.
https://github.com/letsdothis64/Snake
My first time using git or github. =P I took "learn how to use git" as a critique. Hopefully that works.

eXpl0it3r

  • SFML Team
  • Hero Member
  • *****
  • Posts: 10801
    • View Profile
    • development blog
    • Email
Re: critique my snake implementation? (relative beginner, source included)
« Reply #3 on: January 08, 2018, 09:50:36 pm »
I took "learn how to use git" as a critique. Hopefully that works.
Learning Git is a very useful skill to learn and in many places nearly mandatory or expected as a given.
Plus it can be quite a bit of fun, as it gives you the flexibility to experiment and revert any time you want.

Another useful tool to learn is CMake. If you know how to use it, you can easily compile a huge list of libraries and applications out there. If you learn to write your own CMake scripts, thousands of people can easily compile your application for any compiler they desire. ;)

Now for the code

I personally prefer classes to start with a capital letter, as it makes it easier to differentiate between functions and variables (see SFML's source code).

While a bit tedious to write, I tend to write getters and setters for member variables instead of making them public. I do tend to not use set/get as SFML does, but instead just use function overload and let them same function name, but one takes a value and the other returns a value. For example
void speed(int speed) { m_speed = speed; }
int speed() { return m_speed; }
How you do it is really up to your preferences, having public member variables however isn't considered a good practice, unless it's a struct that just holds data and other functions manipulate it.

You don't need to define a protected section if you have nothing in there.

Constructors have initialization lists, you should use them and not initialize the member variables inside the constructor. Do this instead for example:
snake::snake(sf::Vector2i startPos, int startSpeed, int maxSpeed)
: failure(false)
, speed(startSpeed)
, mSpeed(maxSpeed)
, currentPiece(0)
, pieces(1)
{
    pieces[0].pos = startPos;
}
Whether you use this formatting or a different one is up to you, important is that you understand what the initialization list does, so read up on it. :)

speed vs mSpeed, make sure you use descriptive names. When I first read the header it was not clear at all what mSpeed should be. Don't shy away from longer variable names. If it's tedious to type, then try some auto-complete extension for your IDE (which usually come with auto-complete by default anyways). For example currentSpeed and maxSpeed. I personally prefer the term "velocity" instead of "speed" as it feels a bit more precise/scientific.

Use white spaces around your operators! It will make your code a lot more readable to most devs out there.
while(loopvar < pieces.size())
// ...
tempVec *= mult;
Some even like to add a space after if/while/for:
while (loopvar < pieces.size())

Temporaries can sometimes make your code more readable, but sometimes they really aren't needed:
    sf::Vector2f dim(30, 30);
    rect.setSize(dim);
Would be better as:
    rect.setSize(sf::Vector2f(30, 30));
And if you use C++11 and newer, you can make it even shorter with:
    rect.setSize({30, 30});

Since C++11 (and earlier) rand() is basically considered harmful. I suggest to watch the linked video and look into possibilities the <random> header gives you.
Granted for your game it's not really an issue, it's however still good to learn the new one now, rather than later.

You shouldn't add "Obj" to a variable name, instead as suggested at the start use a capitalized first letter for the class to make it clear that Food is the class and food is probably an instance.

Mixing snake's speed and the framerate is not the best idea, instead you may want to make the movement independent of the framerate, by taking the delta time (time a frame takes) and multiply your speed by that. If you want to go crazy with the timing, check out the so often linked Fix Your Timestep.

For the direction variable you could also use an enum, that way you don't have to rely on strings yet you can still see in the code what it means.

As by the C++ standard return 0; is not needed for the main-function, but it also doesn't hurt to keep it, your choice! ;)


I bet there's more to dissect, but for now this should be enough. Hope some points will make you dig even deeper into C++ and congrats on writing this and asking for advice. Many don't get something to finished-like state nor are they open to critique which is simply required if you want to learn and make progress. Keep it and you'll be able to whip out larger code pieces in no time.
Official FAQ: https://www.sfml-dev.org/faq.php
Official Discord Server: https://discord.gg/nr4X7Fh
——————————————————————
Dev Blog: https://duerrenberger.dev/blog/

klefmung

  • Newbie
  • *
  • Posts: 12
    • View Profile
Re: critique my snake implementation? (relative beginner, source included)
« Reply #4 on: January 08, 2018, 10:38:39 pm »
Thanks! I figured there were some bugs hidden in my basic practice. I'll definitely apply all of that to my big project. And I think you've inspired me to do another mini project for code critiques that shows some other stuff I may be unaware I'm not doing the best way.
« Last Edit: January 09, 2018, 06:32:46 am by Laurent »

MorleyDev

  • Full Member
  • ***
  • Posts: 219
  • "It is not enough for code to work."
    • View Profile
    • http://www.morleydev.co.uk/
Re: critique my snake implementation? (relative beginner, source included)
« Reply #5 on: January 10, 2018, 03:28:27 pm »
On the topic of getters/setters, I'd also say to remember: You may not need a setter.

In fact, I'd argue that most of the time you don't need a setter, since that means some basic detail of the object and be reached into and fiddled with, which is a break in the encapsulation and simply surrounding it in a function doesn't actually change that fact. Plus it just sounds dirty ^^

When you find yourself needing to have one object reach inside another and change some fundamental property, ask yourself...why. If it needs to change the speed...well, does it? Or is it, in fact, accelerating the speed. If it needs to change the position, does it? Or is it in fact that you want to move the object?

A moveBy that takes the change in position, and an accelerateBy that takes a change in speed preserves encapsulation a lot more than getter/setters that give the object the reach around. An an update function that does the moving and acceleration could perhaps be said to hide it even better, in classical OOP at least.

(Note: I'm sticking to talking about OOP self-mutating objects here, there's a whole world of other approaches I could go into, and you don't want me to get started on traditional OOP vs functional and/or data-driven since we don't have all day xD).
« Last Edit: January 10, 2018, 03:35:19 pm by MorleyDev »
UnitTest11 - A unit testing library in C++ written to take advantage of C++11.

All code is guilty until proven innocent, unworthy until tested, and pointless without singular and well-defined purpose.

klefmung

  • Newbie
  • *
  • Posts: 12
    • View Profile
Re: critique my snake implementation? (relative beginner, source included)
« Reply #6 on: January 10, 2018, 07:24:23 pm »
That's interesting. I think I can definitely point to the example of my code in snake::update() where I search through the food object's pellets manually, where maybe that could be a function called intersect in food that takes a vector as an argument and returns a bool, then handles the deleting of that pellet and generating another one internally.

klefmung

  • Newbie
  • *
  • Posts: 12
    • View Profile
Re: critique my snake implementation? (relative beginner, source included)
« Reply #7 on: January 11, 2018, 12:22:28 am »
I edited the one on github taking all the critique, if anyone feels like checking it out.
Thanks for all the help!