snippetcppMinor
Tetris as general game example
Viewed 0 times
examplegametetrisgeneral
Problem
I coded a (more or less) simple Tetris game as an example for some of the other trainees in my company. Since I am used to my coding style I would like to have some other people to look over it and maybe give me some advices to improve the readability of the code.
Some of the class names in the code (such as actor) are chosen to give some more general ideas about the parts in a video game (not that I am a pro in that area) so if I had in mind to just develop a tetris game maybe I would have chosen different names so please keep that in mind.
Yes, I am planning to split the code in header/cpp files, and yes I left out the header guards.
typedefs.h:
```
//The pixelUtility isn't included in the uploaded code since that is what i provide others as a "blackbox" library to work with, as well as the mangolib.h
//Please focus at the game code and not the naming of the mango:: or the graphic:: classes/structs, they will be explained to the people who use them pretty extensive
//The mango library is used since it implements some c++11 features I want to use and our IDE does not provide (shared_ptr and lambda for instance and later threading)
//btw. feel free to use this code as you please, as general example or if necessary (I hope it's not that bad ;) ) as negativ example, but I won't provide
//a code sample without my library, but im pretty sure it won't be much work to change the code to regular c++11 for whoever wants to use it.
#define NO_THREAD_ALLOC //disables thread_safety in my customallocator for better performance since this project is single threaded and locks are not needed
#include //header which contains graphic::Character, the GraphicController the KatCoord struct etc, includes --> mango::
namespace engine
{
typedef typename graphic::Character actor; //a simple graphic class with a position, animations and collision
typedef typename mango::shared_ptr actor_handle;
typedef typename mango::ve
Some of the class names in the code (such as actor) are chosen to give some more general ideas about the parts in a video game (not that I am a pro in that area) so if I had in mind to just develop a tetris game maybe I would have chosen different names so please keep that in mind.
Yes, I am planning to split the code in header/cpp files, and yes I left out the header guards.
typedefs.h:
```
//The pixelUtility isn't included in the uploaded code since that is what i provide others as a "blackbox" library to work with, as well as the mangolib.h
//Please focus at the game code and not the naming of the mango:: or the graphic:: classes/structs, they will be explained to the people who use them pretty extensive
//The mango library is used since it implements some c++11 features I want to use and our IDE does not provide (shared_ptr and lambda for instance and later threading)
//btw. feel free to use this code as you please, as general example or if necessary (I hope it's not that bad ;) ) as negativ example, but I won't provide
//a code sample without my library, but im pretty sure it won't be much work to change the code to regular c++11 for whoever wants to use it.
#define NO_THREAD_ALLOC //disables thread_safety in my customallocator for better performance since this project is single threaded and locks are not needed
#include //header which contains graphic::Character, the GraphicController the KatCoord struct etc, includes --> mango::
namespace engine
{
typedef typename graphic::Character actor; //a simple graphic class with a position, animations and collision
typedef typename mango::shared_ptr actor_handle;
typedef typename mango::ve
Solution
Well, imho there are some C constructs in there, while you are trying to write C++.
For instance
would be nicer like this:
But maybe it's just a coding style thing.
Then
Then for example
As you are already in the namespace
And if you define
I would put the class
About files: don't write your implementation in the header file. The header file should only contain definitions. The c-file contains the implementations. (The only exception is for template classes and -functions.)
About templates: The
Also about the
I was wondering about this function:
So ownership of the contents of
Finally, try to remove all the magic numbers from your code. For example:
contains two magic numbers. Better is to replace the line by a(n inline) function call (C++ style) or at least a define (C-style).
There are more thing, but I'll leave that for the others ;)
edit: ok one more thing:
How about using a switch statement. E.g.:
For instance
typedef typename graphic::Character actor;would be nicer like this:
using actor=graphic::Character;But maybe it's just a coding style thing.
Then
typedef typename shapes::shape shape; can be simplified to using shape::shape;.Then for example
void Rotate(mango::shared_ptr segments,As you are already in the namespace
engine, it would probably be sufficient to write mango::shared_ptr.And if you define
using mango::shared_ptr; earlier, it can even be simplified tovoid Rotate(shared_ptr segmentsI would put the class
RotationProcessor and StoneFactory into a separate files. About files: don't write your implementation in the header file. The header file should only contain definitions. The c-file contains the implementations. (The only exception is for template classes and -functions.)
About templates: The
RotateXX functions seem to have a lot in common. Is there a way you could extract the commonalities and write a single template function? Then you will have a lot less code to maintain.Also about the
RotateXX functions: currently these functions are implemented as member functions: you require an object to use them. However, they do not depend on the RotationProcessor object. So why not make them static. That way you could just call them using RotationProcessor::Rotate(...);.I was wondering about this function:
mango::shared_ptr GetSegments()
{
mango::shared_ptr tmp = m_Segments;
m_Segments.reset();
return tmp;
}So ownership of the contents of
m_Segments is transferred to the caller of GetSegments(). To me that sounds more like a unique_ptr then a shared_ptr.Finally, try to remove all the magic numbers from your code. For example:
m_PlayerActor->Move(0, -10);contains two magic numbers. Better is to replace the line by a(n inline) function call (C++ style) or at least a define (C-style).
There are more thing, but I'll leave that for the others ;)
edit: ok one more thing:
if ('q' == input)
{
TryRotationLeft();
}
[...etc.etc...]
if ('X' == input)
{
isRunning = false;
}How about using a switch statement. E.g.:
switch (input) {
case 'q': TryRotationLeft(); break;
case 'e': TryRotationRight(); break;
case 'a': TryMoveLeft(); break;
case 'd': TryMoveRight(); break;
case 's': TryMoveDown(); break;
case 'X': isRunning = false; break;
}Code Snippets
typedef typename graphic::Character actor;using actor=graphic::Character;void Rotate(shared_ptr<actor_list> segmentsmango::shared_ptr<engine::actor_list> GetSegments()
{
mango::shared_ptr<engine::actor_list> tmp = m_Segments;
m_Segments.reset();
return tmp;
}m_PlayerActor->Move(0, -10);Context
StackExchange Code Review Q#158592, answer score: 5
Revisions (0)
No revisions yet.