Tetris clone
Hey guys. Programmer-hobbyist here. I made a Tetris clone in C++/SFML in my spare time. Here's a link to GitHub in case someone's interested in the code.
https://github.com/Ashmor00/Tetris
I also made a Snake clone a few days ago.
https://github.com/Ashmor00/Snake
Feel free to offer your thoughts. Feedback is highly appreciated.
https://github.com/Ashmor00/Tetris
I also made a Snake clone a few days ago.
https://github.com/Ashmor00/Snake
Feel free to offer your thoughts. Feedback is highly appreciated.
Last edited on
Can't build it right just because my current environment, but it looks pretty clean.
Just some minor notes...
Snake:
First: To conform to C++, an array size must be a compile-time constant. Your ArSize should be declared const.
I think some variable names could be improved.
• It isn't clear at first that num refers to the current number of segments in the snake.
• isExisting doesn't refer to the snake like "isVertical" does, so this is inconsistent.
I would rather rename it to "foodExists" or "foodExisting".
• Likewise, ArSize could be "MaxNumSegments" or something like that, to further differentiate it from the current size of the snake.
"my_bool == true" is seen as redundant.
You can just do
For checking if a bool is false, you can do
Also, it appears that you have no check to monitor what happens when the number of snake segments goes above 100, if I'm reading the code correctly. It looks like you'll be going outside the bounds of the array you have set. If you wanted to avoid this issue and have it be more dynamic, you could have an std::vector that grows.
Tetris:
In some loops, you declare multiple dummy variables like "i1, i2", "j1", "j2". Since these variables seem to be updated together each time (e.g. both being incremented by 1 each time), you could simplify the code to only have 1 i/j per loop, and use a simple offset.
Inside your fall function... you set found to true and then break out of a for loop. But then, it does not appear that you reference this variable before setting it back to false at the end of this function. Something about this is probably redundant or not necessary.
Just some minor notes...
Snake:
First: To conform to C++, an array size must be a compile-time constant. Your ArSize should be declared const.
I think some variable names could be improved.
• It isn't clear at first that num refers to the current number of segments in the snake.
• isExisting doesn't refer to the snake like "isVertical" does, so this is inconsistent.
I would rather rename it to "foodExists" or "foodExisting".
• Likewise, ArSize could be "MaxNumSegments" or something like that, to further differentiate it from the current size of the snake.
"my_bool == true" is seen as redundant.
You can just do
if (my_bool) { ... }For checking if a bool is false, you can do
if (!my_bool) { ... } (! means not).Also, it appears that you have no check to monitor what happens when the number of snake segments goes above 100, if I'm reading the code correctly. It looks like you'll be going outside the bounds of the array you have set. If you wanted to avoid this issue and have it be more dynamic, you could have an std::vector that grows.
Tetris:
In some loops, you declare multiple dummy variables like "i1, i2", "j1", "j2". Since these variables seem to be updated together each time (e.g. both being incremented by 1 each time), you could simplify the code to only have 1 i/j per loop, and use a simple offset.
Inside your fall function... you set found to true and then break out of a for loop. But then, it does not appear that you reference this variable before setting it back to false at the end of this function. Something about this is probably redundant or not necessary.
Last edited on
@Ganado,
Thank you very much for your feedback!
@jonnin,
Thanks!
Yeah, I was thinking about that too :)
Thank you very much for your feedback!
@jonnin,
Thanks!
| if you wanted to cool it up, add the egaint extensions :) |
Yeah, I was thinking about that too :)
Well done. As far as feedback goes... in terms of code quality, on both projects, there's definitely big room for improvement. I'm probably going to be ninja'd on this, but hey, here's some reading (and I'm sorry if this seems eviscerating, I don't mean to sound like that).
You use global variables a lot. For small projects like yours, it's not the biggest problem. However, in general, they're frowned upon. There are several reasons why, but one big reason that is that they make your code much harder to reuse. Say you wanted to make your snake or tetris games have head-to-head multiplayer. You now need to duplicate a lot of those globals to make it happen, as well as change several of your functions. That's a lot of duplicated lines of code.
What you can do instead is write a class containing all of the (currently global) variables that you need for your game, and the functions that modify them. Then create an object of that class. If you wanted to make your game multiplayer, you'd create one such object for each game. Creating an object is one line of code, by the way.
Speaking of classes, you use 2D C-style arrays a lot. That's something that you could also make much more convenient for yourself: make a 2D array class with common functions in it, so that you don't have to write as many nested for loops.
Speaking of arrays, you're better off using std::array (if you know the size of the array before you run the program and it never changes) or std::vector (if you don't) than raw arrays. Using those requires some basic knowledge of how to use templates, but please trust me when I say that they're really easy to use. Among many, many other reasons, they keep track of your array's size on their own, and you can use range-based for loops with them: https://www.cprogramming.com/c++11/c++11-ranged-for-loop.html
Your variables are named inconsistently. I've seen variables named
* LikeThis
* likeThis
* likethis
* Like_This
It'd be great if you could pick one capitalization scheme and stick with for your variables. Convention seems to be likeThis or like_this.
You have some simple code duplication in places, such as https://github.com/Ashmor00/Tetris/blob/master/main.cpp#L270 and the code for the A and D controls. You should be able to clean those up without too much trouble. That said, nice use of a rotation matrix to do your rotating instead of hard-coding each block's rotated shape.
You also have a couple of problems in your snake game that you seem to know about in your tetris game (e.g. ArrSize needs to be constant, dir really should be initialized and shouldn't be an int), so I won't go over them in detail.
There are other code quality problems, but I don't want to get too anal. All of the above being said, you're actually off to a pretty good start. I mean it. Keep at it, alright?
-Albatross
You use global variables a lot. For small projects like yours, it's not the biggest problem. However, in general, they're frowned upon. There are several reasons why, but one big reason that is that they make your code much harder to reuse. Say you wanted to make your snake or tetris games have head-to-head multiplayer. You now need to duplicate a lot of those globals to make it happen, as well as change several of your functions. That's a lot of duplicated lines of code.
What you can do instead is write a class containing all of the (currently global) variables that you need for your game, and the functions that modify them. Then create an object of that class. If you wanted to make your game multiplayer, you'd create one such object for each game. Creating an object is one line of code, by the way.
Speaking of classes, you use 2D C-style arrays a lot. That's something that you could also make much more convenient for yourself: make a 2D array class with common functions in it, so that you don't have to write as many nested for loops.
Speaking of arrays, you're better off using std::array (if you know the size of the array before you run the program and it never changes) or std::vector (if you don't) than raw arrays. Using those requires some basic knowledge of how to use templates, but please trust me when I say that they're really easy to use. Among many, many other reasons, they keep track of your array's size on their own, and you can use range-based for loops with them: https://www.cprogramming.com/c++11/c++11-ranged-for-loop.html
Your variables are named inconsistently. I've seen variables named
* LikeThis
* likeThis
* likethis
* Like_This
It'd be great if you could pick one capitalization scheme and stick with for your variables. Convention seems to be likeThis or like_this.
You have some simple code duplication in places, such as https://github.com/Ashmor00/Tetris/blob/master/main.cpp#L270 and the code for the A and D controls. You should be able to clean those up without too much trouble. That said, nice use of a rotation matrix to do your rotating instead of hard-coding each block's rotated shape.
You also have a couple of problems in your snake game that you seem to know about in your tetris game (e.g. ArrSize needs to be constant, dir really should be initialized and shouldn't be an int), so I won't go over them in detail.
There are other code quality problems, but I don't want to get too anal. All of the above being said, you're actually off to a pretty good start. I mean it. Keep at it, alright?
-Albatross
@TheDaemoness,
Thank you very much for the feedback! This is very helpful!
I will. Computer programming is my hobby at the moment, but I hope it will become my profession at some point.
Thank you very much for the feedback! This is very helpful!
| I mean it. Keep at it, alright? |
I will. Computer programming is my hobby at the moment, but I hope it will become my profession at some point.
Topic archived. No new replies allowed.