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

Text-based Tetris game - follow-up

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
textgamefollowbasedtetris

Problem

Previous question:

Text-based Tetris game

I don't have a Linux terminal to be sure whether or not I have implemented the Linux version correctly, but hopefully it's OK.

Summary of improvements:

  • More OOP



  • Improved the names of the classes and their functions and I hope they're OK now



  • More portable, isolated platform-specific code



  • Eliminated "magic numbers"



I would like to know if there is anything I should take care of before processing the code.

```
#include
#include
#include
#include

#ifdef __linux__
/***
kbhit() and getch() for Linux/UNIX
Chris Giese http://my.execpc.com/~geezer
Release date: ?
This code is public domain (no copyright).
You can do whatever you want with it.
/
#include / struct timeval, select() /
/ ICANON, ECHO, TCSANOW, struct termios /
#include / tcgetattr(), tcsetattr() /
#include / atexit(), exit() /
#include / read() /
#include / printf() /

static struct termios g_old_kbd_mode;
/***
***/
static void cooked(void)
{
tcsetattr(0, TCSANOW, &g_old_kbd_mode);
}
/***
***/
static void raw(void)
{
static char init;
/**/
struct termios new_kbd_mode;

if (init)
return;
/ put keyboard (stdin, actually) in raw, unbuffered mode /
tcgetattr(0, &g_old_kbd_mode);
memcpy(&new_kbd_mode, &g_old_kbd_mode, sizeof(struct termios));
new_kbd_mode.c_lflag &= ~(ICANON | ECHO);
new_kbd_mode.c_cc[VTIME] = 0;
new_kbd_mode.c_cc[VMIN] = 1;
tcsetattr(0, TCSANOW, &new_kbd_mode);
/ when we exit, go back to normal, "cooked" mode /

Solution

Just a few quick comments on things I saw when scanning through:

struct Block : public NonCopyable


By defining this as a struct, you make members public (unless overridden). As a general rule, it is preferred for members to be private as a default. If you are marking a member as public then you should have a specific reason for that member. That pushes you to write code that does not rely on the members being public. You do not seem to be publicly using your public members, so you might as well make this a class. Note: this is not to say that there is never a reason to use a struct or public member, just that I see no sign of such a reason here.

block.resize(4, std::vector(4, 0));
    field.resize(22, std::vector(13, 0));
    stage.resize(22, std::vector(13, 0));
int x = 4;
Random getRandom{ 0, 6 };
for (size_t i = 0; i <= board.size() - 2; ++i)
    for (size_t j = 0; j <= board.rowSize() - 2; ++j)
        if ((j == 0) || (j == 11) || (i == 20))
            board.getDim(i, j) = stage.getDim(i, j) = 9;
x = 4;
        COORD coord = { 20, 20 };


There still seem to be a lot of magic numbers in the code. Some of them are repeated.

Random getRandom{ 0, 6 };


could be something like

Random getRandom{ 0, blocklist.size() };


I don't understand when the following triggers:

if (board.getDim(i, j + block.size()) > 1)
        {
            gameOver = true;


It's probably in the code somewhere, but I don't feel like tracking it down now. A comment of why we are comparing to 1 would be helpful.

Code Snippets

struct Block : public NonCopyable
block.resize(4, std::vector<int>(4, 0));
    field.resize(22, std::vector<int>(13, 0));
    stage.resize(22, std::vector<int>(13, 0));
int x = 4;
Random getRandom{ 0, 6 };
for (size_t i = 0; i <= board.size() - 2; ++i)
    for (size_t j = 0; j <= board.rowSize() - 2; ++j)
        if ((j == 0) || (j == 11) || (i == 20))
            board.getDim(i, j) = stage.getDim(i, j) = 9;
x = 4;
        COORD coord = { 20, 20 };
Random getRandom{ 0, 6 };
Random getRandom{ 0, blocklist.size() };
if (board.getDim(i, j + block.size()) > 1)
        {
            gameOver = true;

Context

StackExchange Code Review Q#74357, answer score: 3

Revisions (0)

No revisions yet.