HiveBrain v1.2.0
Get Started
← Back to all entries
snippetcppMinor

Tetris as general game example

Submitted by: @import:stackexchange-codereview··
0
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

Solution

Well, imho there are some C constructs in there, while you are trying to write C++.

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 to

void Rotate(shared_ptr segments


I 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> segments
mango::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.