2
\$\begingroup\$

About a month ago I made a Tetris game for Windows. After a helpful review, I improved the code. I want to hear again an opinion about the implementation, how readable the code is, new mistakes or anything that will improve the quality of the code.

Action.h

#pragma once

#ifndef ACTIONS_H
#define ACTIONS_H

/*
    Possible actions that a player can do
*/

class Action
{
public:
    static constexpr char moveLEFT{ 'a' };
    static constexpr char moveRIGHT{ 'd' };
    static constexpr char moveDOWN{ 's' };
    static constexpr char rotateLEFT{ 'q' };
    static constexpr char rotateRIGHT{ 'e' };
};

#endif // !ACTIONS_H

Coordinates.cpp

#include "Coordinates.h"

Coordinates::Coordinates(int x, int y)
{
    this->x = x;
    this->y = y;
}

Coordinates& Coordinates::operator=(const Coordinates &coord)
{
    if (this != &coord)
    {
        this->x = coord.x;
        this->y = coord.y;
    }
    return *this;
}

int Coordinates::getX()
{
    return x;
}

int Coordinates::getY()
{
    return y;
}

void Coordinates::setX(const int &x)
{
    this->x = x;
}

void Coordinates::setY(const int &y)
{
    this->y = y;
}

void Coordinates::moveCoordinatesInADirection(char direction)
{
    switch (direction)
    {
    case Action::moveLEFT:
        y--;
        break;
    case Action::moveRIGHT:
        y++;
        break;
    case Action::moveDOWN:
        x++;
        break;
    default:
        break;
    }
}

void Coordinates::Draw()
{
    MoveTo(x + Drawable::startPositionX, y + Drawable::startPositionY);
    cout << form;
}

void Coordinates::DeleteDraw()
{
    MoveTo(x + Drawable::startPositionX, y + Drawable::startPositionY);
    cout << " ";
}

Coordinates.h

#pragma once

#ifndef COORDINATES_H
#define COORDINATES_H

#include "Actions.h"
#include "Drawable.h"
#include <iostream>

using namespace std;

class Coordinates : public Drawable
{
private:
    int x;
    int y;
public:
    static constexpr char form{ '*' };

public:
    Coordinates(int x = 0, int y = 0);
    Coordinates& operator =(const Coordinates &coord);
    int getX();
    int getY();
    void setX(const int &x);
    void setY(const int &y);
    // Methods using a coordinate
    void moveCoordinatesInADirection(char direction);
    void Draw() override;
    void DeleteDraw() override;
};

#endif // !Coordinates_H

Difficulty.cpp

#include "Difficulty.h"

int Difficulty::increaseSpeedAfterXTilesPlayed = 20;
int Difficulty::speedOfTiles = 600;

void Difficulty::setDifficulty(char numberOfDifficulty)
{
    switch (numberOfDifficulty)
    {
    //Easy
    case '1':
        increaseSpeedAfterXTilesPlayed = 20;
        speedOfTiles = 600;
        break;
    //Normal
    case '2':
        increaseSpeedAfterXTilesPlayed = 15;
        speedOfTiles = 400;
        break;
    //Hard
    case '3':
        increaseSpeedAfterXTilesPlayed = 10;
        speedOfTiles = 200;
        break;
    //Impossible
    case '4':
        increaseSpeedAfterXTilesPlayed = 5;
        speedOfTiles = 100;
        break;
    }
}

void Difficulty::increaseSpeedafterXTiles(int& counterNumberOfTilesPlayed)
{
    if ((counterNumberOfTilesPlayed == Difficulty::increaseSpeedAfterXTilesPlayed)
        && (Difficulty::speedOfTiles > 20))
    {

        Difficulty::speedOfTiles = Difficulty::speedOfTiles - 20;
        counterNumberOfTilesPlayed = 0;
    }
}

Difficulty.h

#pragma once

#ifndef DIFFICULTY_H
#define DEFFICULTY_H

class Difficulty
{
public:
    static int increaseSpeedAfterXTilesPlayed;
    static int speedOfTiles;

public:
    static void setDifficulty(char numberOfDifficulty);
    static void increaseSpeedafterXTiles(int& counterNumberOfTilesPlayed);
};
#endif // !DIFFICULTY_H

Drawable.cpp

#include "Drawable.h"

int Drawable::getstartPositionX()
{
    return startPositionX;
}

void Drawable::hideCursor()
{
    CONSOLE_CURSOR_INFO info = { 100,FALSE };
    SetConsoleCursorInfo(GetStdHandle(STD_OUTPUT_HANDLE), &info);
}

void Drawable::MoveTo(const int &x, const int &y)
{
    COORD coord = { startPositionY + y,startPositionX + x };
    SetConsoleCursorPosition(GetStdHandle(STD_OUTPUT_HANDLE), coord);
}

Drawable.h

#pragma once

#ifndef DRAWABLE_H
#define DRAWABLE_H

#include <windows.h>

class Drawable
{
protected:
    //The position where the table game will be displayed in console
    static constexpr int startPositionX{ 10 };
    static constexpr int startPositionY{ 25 };

public:
    static int getstartPositionX();
    static void hideCursor();
    static void MoveTo(const int &x,const int &y);
    virtual void Draw() = 0;
    virtual void DeleteDraw() = 0;
};

#endif // !DRAWABLE_H

Source.cpp

#include "Table.h"

int main()
{
    Table a;

    try {
        a.startGame();
    }
    catch (...) 
    {
        // In case player loses
        Drawable::MoveTo(Drawable::getstartPositionX() + Table::numberOfLines + 1, 0);
        cout << "\n" << "Good job, you made " << a.score * 1000 << " points.\n";
    }
}

Table.cpp

#include "Table.h"

Table::Table()
{
    // When you start the game the table is empty and the score is 0
    score = 0;

    for (int currentLine = 0; currentLine < numberOfLines; currentLine++)
    {
        for (int currentColumn = 0; currentColumn < numberOfColumns; currentColumn++)
        {
            table[currentLine][currentColumn] = 0;
        }
    }
}

void Table::informationAboutGame()
{
    // General informations about the game and setting the difficulty the player wants to play on

    cout << "\n\n\n\t This is a tetris game.The controls for the game are:\n";
    cout << "\n\t a - move the tile left";
    cout << "\n\t d - move the tile right";
    cout << "\n\t s - move the tile down";
    cout << "\n\t e - rotate the tile right";
    cout << "\n\t q - rotate the tile left";
    cout << "\n\n\t The game has 3 difficulties: ";
    cout << "\n\t 1. Easy";
    cout << "\n\t 2. Normal";
    cout << "\n\t 3. Hard";
    cout << "\n\t 4. Impossible";
    cout << "\n\n\t Introduce the number of the difficulty you want to play on and good luck: ";

    char numberOfDifficulty = _getch();

    while ((numberOfDifficulty != '1') && (numberOfDifficulty != '2') && 
        (numberOfDifficulty != '3') && (numberOfDifficulty!='4'))
    {
        cout << "\n\tInsert a number between 1-4: ";
        numberOfDifficulty = _getch();
    }

    Difficulty::setDifficulty(numberOfDifficulty);
}

void Table::checkingAndDeletingCompletedLines()
{
    // We parse the table and check if there is any line with only 1 on it, and than we delete the line
    int check = 1;

    for (int currentLine = 0; currentLine < numberOfLines; currentLine++)
    {
        check = 1;

        for (int currentColumn = 0; currentColumn < numberOfColumns; currentColumn++)
        {
            if (table[currentLine][currentColumn] == 0)
            {
                check = 0;
                break;
            }
        }

        if (check)
        {
            deleteCompletedLineFromTable(currentLine);
            score++;
        }
    }
}

void Table::deleteCompletedLineFromTable(const int& line)
{
    // Deleting the line which is completed
    // We need to actualize the table by replacing every line (starting from the completed line until the second line) with the previous lines
    // Also we need to draw the actualized lines in the console
    for (int currentLine = line; currentLine > 0; currentLine--)
    {
        for (int currentColumn = 0; currentColumn < numberOfColumns; currentColumn++)
        {
            Drawable::MoveTo(currentLine + Drawable::startPositionX, currentColumn + Drawable::startPositionY);
            if (table[currentLine - 1][currentColumn] == 0)
            {
                cout << " ";
            }
            else {
                cout << Coordinates::form;
            }

            table[currentLine][currentColumn] = table[currentLine - 1][currentColumn];
        }
    }

    for (int currentColumn = 0; currentColumn < numberOfColumns; currentColumn++)
    {
        Drawable::MoveTo(0 + Drawable::startPositionX, currentColumn + Drawable::startPositionY);
        cout << " ";
        table[0][currentColumn] = 0;
    }
}

void Table::moveTileDownAutomatically()
{
    //Moving the actual tile down every and checking if the player wants to make a move(right, left, down) or rotate(right, left) the tile
    actualTile.Draw();

    int counterTime = 0;

    do {
        counterTime = 0;

        while (counterTime <= Difficulty::speedOfTiles)
        {
            if (_kbhit())             // if the player presses a key on keyboard
            {
                possibleMoves(counterTime);
            }

            Sleep(1);
            counterTime = counterTime + 1;
        }

        if (checkIfCanMoveInADirection(Action::moveDOWN))
        {
            actualTile.DeleteDraw();
            moveTileInADirection(Action::moveDOWN);
            actualTile.Draw();
        }
        else 
        {
            break;
        }
    } while (true);
}

void Table::moveTileInADirection(char direction)
{
    // To move the tile in a direction we need to :
    // - delete the previous tile from the game table
    // - move the tile to the new coordinates
    // - actualizate the game table
    for (int i = 0; i < 4; i++)
    {
        table[actualTile.getcoordX(i)][actualTile.getcoordY(i)] = 0;
    }

    actualTile.moveTileInADirection(direction);

    for (int i = 0; i < 4; i++)
    {
        table[actualTile.getcoordX(i)][actualTile.getcoordY(i)] = 1;
    }
}

void Table::possibleMoves(int &counterTime)
{
    //Possible moves that can be effectuated on a tile (move and rotate)
    char direction = _getch();

    if (checkIfCanMoveInADirection(direction))
    {
        actualTile.DeleteDraw();                  // delete old tile
        moveTileInADirection(direction);          // move the tile in the direction the player wanted
        actualTile.Draw();                        // draw the new tile
        if (direction == Action::moveDOWN)
        {
            // If we move the tile down we reset the counter until the tile moves again down by itself
            counterTime = 1;
        }
    }
    // check if the player wanted to rotate the tile (right, left)
    if ((direction == Action::rotateRIGHT) || (direction == Action::rotateLEFT))
    {
        actualTile.DeleteDraw();
        rotateTileInADirection(direction);
        actualTile.Draw();
    }
}

void Table::positioningTileInTableAfterRotation()
{
    // This method is used to check and correct a tile if it goes out of boundaries of the game table after a rotation
    int index = 0;
    int checkOutOfBoundaries = 0;

    while (index < 4)
    {
        if (actualTile.getcoordY(index) < 0)
        {
            // passed left boundary of the game table
            for (int j = 0; j < 4; j++)
            {
                actualTile.setcoordY(j, actualTile.getcoordY(j) + 1);
            }
            checkOutOfBoundaries = 1;
        }

        if (actualTile.getcoordY(index) > numberOfColumns - 1)
        {
            // passed right boundary of the game table
            for (int j = 0; j < 4; j++)
            {
                actualTile.setcoordY(j, actualTile.getcoordY(j) - 1);
            }
            checkOutOfBoundaries = 1;
        }

        if (actualTile.getcoordX(index) < 0)
        {
            // passed top boundary of the game table and there are cases where the player loses
            for (int j = 0; j < 4; j++)
            {
                actualTile.setcoordX(j, actualTile.getcoordX(j) + 1);
            }

            for (int j = 0; j < 4; j++)
            {
                if ((actualTile.getcoordX(j) > 0) && (table[actualTile.getcoordX(j)][actualTile.getcoordY(j)] == 1))
                {
                    throw 0;
                }
            }
            checkOutOfBoundaries = 1;
        }

        if ((actualTile.getcoordX(index) > numberOfLines - 1) ||
            (table[actualTile.getcoordX(index)][actualTile.getcoordY(index)] == 1))
        {
            // passed the down boundary or reached a possition that is occupied
            for (int j = 0; j < 4; j++)
            {
                actualTile.setcoordX(j, actualTile.getcoordX(j) - 1);
            }
            checkOutOfBoundaries = 1;
        }

        if (checkOutOfBoundaries == 1)
        {
            index = 0;
            checkOutOfBoundaries = 0;
        }
        else 
        {
            index++;
        }
    }
}

void Table::rotateTileInADirection(char direction)
{
    // To rotate the tile in a direction we need to :
    // - delete the previous tile from the game table
    // - move the tile to the new coordinates and adjust it so it doesnt pass the boundaries of the game table
    // - actualizate the game table
    for (int i = 0; i < 4; i++)
    {
        table[actualTile.getcoordX(i)][actualTile.getcoordY(i)] = 0;
    }

    actualTile.rotateTileInADirection(direction);
    positioningTileInTableAfterRotation();

    for (int i = 0; i < 4; i++)
    {
        table[actualTile.getcoordX(i)][actualTile.getcoordY(i)] = 1;
    }
}

void Table::startGame()
{
    Drawable::hideCursor();
    informationAboutGame();

    DeleteDraw();
    Draw();

    int counterNumberOfTilesPlayed = 0;

    // This while will end when the player will lose
    while (true)
    {
        checkingAndDeletingCompletedLines();

        actualTile = Tiles::generateRandomTile();

        if (checkIfPlayerLost() == false)
        {
            moveTileDownAutomatically();

            counterNumberOfTilesPlayed++;
            Difficulty::increaseSpeedafterXTiles(counterNumberOfTilesPlayed);
        }
        else 
        {
            Drawable::MoveTo(Drawable::startPositionX + numberOfLines + 1, 0);
            cout << "\n" << "Good job, you made " << score * 1000 << " points.\n";
            break;
        }
    }
}

void Table::Draw()
{
    // Method used to draw the table 
    for (int index = -1; index <= numberOfLines; index++)
    {
        MoveTo(Drawable::startPositionX + index, Drawable::startPositionY - 1);
        cout << char(219);
        MoveTo(Drawable::startPositionX + index, Drawable::startPositionY + numberOfColumns);
        cout << char(219);
    }

    for (int index = -1; index <= numberOfColumns; index++)
    {
        Drawable::MoveTo(Drawable::startPositionX - 1, Drawable::startPositionY + index);
        cout << char(219);
        Drawable::MoveTo(Drawable::startPositionX + numberOfLines, Drawable::startPositionY + index);
        cout << char(219);
    }
}


void Table::DeleteDraw()
{
    // Method used to delete the table
    system("cls");
}

bool Table::belongsToActualTile(const int& x, const int& y)
{
    //Checking if a piece(point/coordinate) of a tile belonds to the actual tile
    for (int currentCoordinate = 0; currentCoordinate < 4; currentCoordinate++)
    {
        if ((actualTile.getcoordX(currentCoordinate) == x) && (actualTile.getcoordY(currentCoordinate) == y))
        {
            return false;
        }
    }

    return true;
}

bool Table::checkIfCanMoveInADirection(char direction)
{
    for (int i = 0; i < 4; i++)
    {
        switch (direction)
        {
            // Check if the player can move left
        case Action::moveLEFT:
            if ((actualTile.getcoordY(i) - 1 < 0) ||
                ((belongsToActualTile(actualTile.getcoordX(i), actualTile.getcoordY(i) - 1)) &&
                (table[actualTile.getcoordX(i)][actualTile.getcoordY(i) - 1] == 1)))
            {
                return false;
            }
            break;
            // Check if the player can move right
        case Action::moveRIGHT:
            if ((actualTile.getcoordY(i) + 1 > numberOfColumns - 1) ||
                ((belongsToActualTile(actualTile.getcoordX(i), actualTile.getcoordY(i) + 1)) &&
                (table[actualTile.getcoordX(i)][actualTile.getcoordY(i) + 1] == 1)))
            {
                return false;
            }
            break;
            // Check if the player can move down
        case Action::moveDOWN:
            if ((actualTile.getcoordX(i) + 1 > numberOfLines - 1) ||
                ((belongsToActualTile(actualTile.getcoordX(i) + 1, actualTile.getcoordY(i))) &&
                (table[actualTile.getcoordX(i) + 1][actualTile.getcoordY(i)] == 1)))
            {
                return false;
            }
            break;
        default:
            break;
        }
    }
    return true;
}

bool Table::checkIfPlayerLost()
{
    for (int currentCoordinate = 0; currentCoordinate < 4; currentCoordinate++)
    {
        if (table[actualTile.getcoordX(currentCoordinate)][actualTile.getcoordY(currentCoordinate)] == 1)
        {
            return true;
        }
    }

    return false;
}

Table.h

#pragma once

#ifndef TABLE_H
#define TABLE_H

#include "Difficulty.h"
#include "Tile.h"
#include "Tiles.h"
#include <conio.h>
#include <random>

class Table : public Drawable           // class that represents the game table 
{
public:
    static constexpr int  numberOfColumns{ 11 };
    static constexpr int numberOfLines{ 21 };
    long score;
private:
    int table[numberOfLines][numberOfColumns];            // the game table= a matrix with 0 if there is nothing draw in that point and 1 if there is something draw 
    Tile actualTile;         // the tile that moves in the game table(the actual tile)

public:
    Table();
    void informationAboutGame();
    void checkingAndDeletingCompletedLines();
    void deleteCompletedLineFromTable(const int& line);        // after a line from the table is completated, it will be deleted from the game table and the score will rise
    void moveTileDownAutomatically();
    void moveTileInADirection(char direction);
    void possibleMoves(int& counterTime);         // possible moves of a player (right, left, down)
    void positioningTileInTableAfterRotation();
    void rotateTileInADirection(char direction);
    void startGame();
    void Draw();
    void DeleteDraw();
    bool belongsToActualTile(const int& x, const int& y);
    bool checkIfCanMoveInADirection(char direction);
    bool checkIfPlayerLost();
};

#endif // !TABLE_H

Tile.cpp

#include "Tile.h"
#include "Table.h"

Tile::Tile()
{
    for (int index = 0; index < 4; index++)
    {
        coordTile[index].setX(0);
        coordTile[index].setY(0);
    }

    centerOfTile = -1;
}

Tile& Tile::operator=(const Tile &tile)
{
    if (this != &tile)
    {
        for (int i = 0; i < 4; i++)
        {
            this->coordTile[i] = tile.coordTile[i];
        }
        this->centerOfTile = tile.centerOfTile;
    }

    return *this;
}

/*
    A tile is in tiles.in is saved like this:
        0 0 0 2
        0 2 2 2
        0 0 0 0
        0 0 0 0
    A tile stores has a array of 4 coordinates(Coordinates) and a center(int)
    In the array we will save the 4 coordinates ((0,3) (1,1) (1,2) (1,3)) that don't have the value 0 in matrix, and in the centerOfTile the center of the figure
*/

void Tile::initializationOfTile(ifstream& input)
{
    int counter = 0;
    int checkValue = 0;

    for (int x = 0; x < 4; x++)
    {
        for (int y = 0; y < 4; y++)
        {
            input >> checkValue;

            if (checkValue != 0)
            {
                coordTile[counter].setX(x);
                coordTile[counter++].setY(Table::numberOfColumns / 2 + 2 - y);  // Setting the coordinate for Y in the middle of the table

                if ((x == 1) && (y == 2))
                {
                    centerOfTile = counter - 1;
                }
            }
        }
    }
}

int Tile::getcoordX(const int &position)
{
    return coordTile[position].getX();
}

int Tile::getcoordY(const int &position)
{
    return coordTile[position].getY();
}

int Tile::getcenterOfTile(const int &position)
{
    return centerOfTile;
}

void Tile::setcoordX(const int &position, const int &x)
{
    coordTile[position].setX(x);
}

void Tile::setcoordY(const int &position, const int &y)
{
    coordTile[position].setY(y);
}

void Tile::setcenterOfTile(const int &centerOfTile)
{
    this->centerOfTile = centerOfTile;
}

void Tile::moveTileInADirection(char direction)
{
    for (int currentCoordinate = 0; currentCoordinate < 4; currentCoordinate++)
    {
        coordTile[currentCoordinate].moveCoordinatesInADirection(direction);
    }
}

void Tile::rotateTileInADirection(char direction)
{
    int dir = 0;

    switch (direction)
    {
    case Action::rotateRIGHT:           // to rotate the tile to the right we need +90* check formula down
        dir = +90;
        break;
    case Action::rotateLEFT:           // to rotate the tile to the left we need -90* check formula down
        dir = -90;
        break;
    default:
        return;
    }

    // If the tile can be rotated
    if (centerOfTile != -1)
    {
        double centerOfTileX = coordTile[centerOfTile].getX();
        double centerOfTileY = coordTile[centerOfTile].getY();

        double tileX = 0;
        double tileY = 0;

        // Rotate every piece(point/coordinate) from the tile with 90*(to right) or -90*(to left) depends on dir
        for (int currentCoordinate = 0; currentCoordinate < 4; currentCoordinate++)
        {
            tileX = (double) coordTile[currentCoordinate].getX();
            tileY = (double) coordTile[currentCoordinate].getY();

            coordTile[currentCoordinate].setX((int)round((tileX - centerOfTileX) * cos((3.14 * dir) / 180) +
                (tileY - centerOfTileY) * sin((3.14 * dir) / 180) + centerOfTileX));
            coordTile[currentCoordinate].setY((int)round((centerOfTileX - tileX) * sin((3.14 * dir) / 180) +
                (tileY - centerOfTileY) * cos((3.14 * dir) / 180) + centerOfTileY));
        }
    }
}

void Tile::Draw()
{
    for (int currentCoordinate = 0; currentCoordinate < 4; currentCoordinate++)
    {
        coordTile[currentCoordinate].Draw();             // Drawing the tile by drawing every piece (point/coordinate) of it
    }
}

void Tile::DeleteDraw()
{
    for (int currentCoordinate = 0; currentCoordinate < 4; currentCoordinate++)
    {
        coordTile[currentCoordinate].DeleteDraw();      // Deleting the tile by deleting every piece (point/coordinate) of it
    }
}

Tile.h

#pragma once

#ifndef TILE_H
#define TILE_H

#include "Coordinates.h"
#include "Drawable.h"
#include <fstream>

// Class that represents a tile and all its methods
class Tile : public Drawable
{
private:
    // Every tile is composed of 4 coordinates and a center
    Coordinates coordTile[4];
    int centerOfTile;

public:
    Tile();
    Tile& operator=(const Tile &tile);
    void initializationOfTile(ifstream& input);
    // Getter and setter
    int getcenterOfTile(const int &position);
    int getcoordX(const int &position);
    int getcoordY(const int &position);
    void setcenterOfTile(const int &centerOfTile);
    void setcoordX(const int &position, const int &x);
    void setcoordY(const int &position, const int &y);
    // Methods using a tile
    void moveTileInADirection(char direction);          // Moves the tile in a specific direction (right, left, down)
    void rotateTileInADirection(char direction);       // Rotates the tile in a specific direction (right, left)
    void Draw() override;                             // Overrides function Draw() from Drawable() and is used to draw the tile in the game table
    void DeleteDraw() override;                      // Overrides function DeleteDraw() from Drawable() and is used to delete the tile from the game table
};

#endif // !TILE_H

Tiles.cpp

#include "Tiles.h"


int Tiles::numberOfTiles = initializationOfNumberOfTiles();
Tile* Tiles::figures = initializationOfFigures();

int Tiles::initializationOfNumberOfTiles()
{
    int numberOfTiles = 0;

    ifstream input("tiles.in");
    input >> numberOfTiles;
    input.close();

    return numberOfTiles;
}

Tile* Tiles::initializationOfFigures()
{
    Tile* figures;
    int numberOfTiles = 0;

    ifstream input("tiles.in");
    input >> numberOfTiles;
    figures = new Tile[numberOfTiles];

    for (int currentTile = 0; currentTile < numberOfTiles; currentTile++)
    {
        figures[currentTile].initializationOfTile(input);
    }

    //The center of a line respectively a square is different than the other figures
    figures[0].setcenterOfTile(2);
    figures[3].setcenterOfTile(-1);

    input.close();

    return figures;
}

Tile Tiles::generateRandomTile()
{
    Tile randomTile;
    int randomNumber = 0;

    random_device random;
    uniform_int_distribution<int>dist(0, numberOfTiles - 1);

    randomNumber = dist(random);
    randomTile = figures[randomNumber];

    return randomTile;
}

Tiles::~Tiles()
{
    delete[] figures;
}

Tiles.h

#pragma once

#ifndef TILES_H
#define TILES_H

#include "Tile.h"
#include <fstream>
#include <random>

class Tiles                 // Contains the number of tiles and the tiles possible
{
private:
    static int numberOfTiles;
    static Tile* figures;

private:
    static int initializationOfNumberOfTiles();
    static Tile* initializationOfFigures();
    ~Tiles();

public:
    static Tile generateRandomTile();
};

#endif // !TILES_H

tiles.in

7

2 2 2 2 
0 0 0 0
0 0 0 0
0 0 0 0

0 2 0 0
0 2 2 2
0 0 0 0
0 0 0 0

0 0 0 2
0 2 2 2
0 0 0 0
0 0 0 0

0 2 2 0
0 2 2 0
0 0 0 0
0 0 0 0

0 0 2 2
0 2 2 0
0 0 0 0
0 0 0 0

0 0 2 0
0 2 2 2
0 0 0 0
0 0 0 0

0 2 2 0
0 0 2 2
0 0 0 0
0 0 0 0

The code is also available: Github Tetris game

Edit: I will respond to both commentaries in this edit, thanks you both for the time spend to respond.
So, I managed to move almost all includes in the .cpp files, rearranged the methods and members in classes (pub, prot, priv), modified const& int in int, used const wherever I could, deleted the copy constructor and other destructors that I didn't need, gave better names to the variables, used namespace instead of some classes that had only static methods, made better comments. I didn't modify the prefix for members, because it seemed harder to read the code, and I tried to use namespace on every class but than I modified back without namespace because don't know how to get rid of x::x::A(); and it makes the lines so long and hard to read, tried to don't use namespace std; anywhere, I used it only in one file .cpp out of 15files. Replaced every array with a std::vector, managed to get rid of the try catch block. In Difficulty.cpp I had int& counterNumberOfTilesPlayed; because I change this value in the function. Besides that I had before on every function const type&.
I will get rid of the namespace std; totally and try to find a way to use namespace tomorrow I think. Besides that I did everyother observations. I don't know if I should add the code again in this thread, I updated the github and all the changes can be seen in the link above.
Edit 2: @Sandro4912 managed to do the last advice you gave me that I didn't do last night and wrapped all functions and classes into a namespace, stopped using namespace std;

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Unfortunately your question is off-topic as of now, as the code to be reviewed must be present in the question. Code behind links is considered non-reviewable. Please add the code you want reviewed in your question. Thanks! \$\endgroup\$
    – Edward
    Commented Jul 9, 2019 at 21:10
  • \$\begingroup\$ I have 15 different files (.h and .cpp) because I separated declaration and implementation and put one class per file. Should I copy all the code from all the 15 files in the question ? \$\endgroup\$ Commented Jul 9, 2019 at 21:15
  • \$\begingroup\$ You could do that or just the most important classes and their interfaces. The more code you supply, generally the more comprehensive and useful the reviews. \$\endgroup\$
    – Edward
    Commented Jul 9, 2019 at 21:31
  • \$\begingroup\$ I understand I will edit the question tomorrow morning with the code and everything. \$\endgroup\$ Commented Jul 9, 2019 at 23:00

2 Answers 2

2
\$\begingroup\$

Action.h

  1. The Action class only contains public static data members. While it's not illegal, a class might not be the best way to go about it. Consider using struct, which is the same as a class but has publicas the default access specifier. Or even better, don't use a class or struct and instead, wrap it inside a namespace, which seems perfect for this kind of thing.
namespace Action
{
  static constexpr char moveLEFT{ 'a' };
  static constexpr char moveRIGHT{ 'd' };
  static constexpr char moveDOWN{ 's' };
  static constexpr char rotateLEFT{ 'q' };
  static constexpr char rotateRIGHT{ 'e' };    
} 

You can then use it the same way as you're doing now. Action::moveLeft et cetera.

Coordinate.cpp

  1. Use initializer list instead of setting the members in the body of the constructor. Only it doesn't really matter in this case, it's still a good idea to do so. See here why.
Coordinates::Coordinates(int x_, int y_): x(x_), y(y_) { /* empty */ }
  1. Make your getters const. In fact, you should mark all methods that are not modifying your data members const.
int Coordinates::getX() const
{
    return x;
}

int Coordinates::getY() const
{
  return y;
}
  1. You don't need to do const int& x in the setX() function. The cost of copying an int is negligible. void setX(int x) is fine. We pass by reference when we want to change the value of the argument void changeValueOfX(int& x) or when a structure is large enough the copying it incurs a non-negligible penalty void doStuffWith2DVector(const std::vector<std::vector<int>>& vec).

Coordinate.h

  1. Since all your constructor is doing is setting the value, you could just put the initializer list in this header instead of putting it in the cpp file.

Coordinates(int x_ = 0, int y_ = 0): x(x_), y(y_) {}

  1. using namespace std; is frowned upon, and using it inside a header is a big no-no. Remember that all the code in the header file is literally copy-pasted whenever you do #include "Coordinates.h. That means every file where you include this file will contain the line using namespace std; and can lead to some nasty bugs if you're not careful.

  2. Don't include headers when you don't need them. The header is not using iostream. Instead, include the file when in the Coordinates.cpp file, when you actually do need to use it.

Difficulty.cpp

  1. Again, no need to do int& counterNumberOfTilesPlayed. Just do int counterNumberOfTilesPlayed. Also, your argument list is inconsistent. In the earlier file you did const int& x and now you're doing int& counterNumberOfTilesPlayed, when you're not changing the value of the argument in either of those.

Difficulty.h

  1. Spelling error in the include guard.

  2. Again, all your class is containing is public static members and member functions. Wrap them inside a namespace rather than a class. C++ contains OOP features, but we don't have to use it all the time.

Drawable.cpp

  1. Mark methods as const.

  2. Don't need to const int& x, et cetera.

Drawable.h

  1. You don't need windows.h in this file. Move it over to the implementation.

Source.cpp

  1. I haven't actually looked at the implementation of your code, but there is surely a better way to realize when a player loses than a try catch block. Rethink your logic. A try catch block to decide whether a player loses seems, to be honest, ugly.

Table.cpp

  1. Use member initializer list in the constructor to sets values of your data members.
Table::Table(): score(0)
{
    ...

}

Table.h

  1. Move the non-required headers into the implementation.

General

  1. A lot of your classes contain only or mostly static data. You should reevaluate whether a class is the best choice.

  2. Your naming of methods and members is too verbose. While variables should always be named to describe their purpose, too long a variable name can be cumbersome, for the person writing the code and the person who will read it.

A few examples:

  • moveCoordinatesInADirection(char direction) can be renamed moveCoordinates(char direction). In both cases, the name describes what the method is doing, but the latter is shorter and more concise.

  • counterNumberOfTilesPlayed can be renamed numberOfTilesPlayed or numTilesPlayed or even tilesPlayed.

  • informationAboutGame can be renamed gameInfo.

  • checkingAndDeletingCompletedLines can be renamed deleteCompletedLines

  • moveTileDownAutomatically can be renamed moveTileDown.

Any many more..

  1. Comments: I firmly believe code should be self-documenting unless a method or variable or statement requires explicit instructions or programmer's intention.
void moveTileInADirection(char direction);          // Moves the tile in a specific direction (right, left, down)

adds nothing to the source code. One could figure it out from the method prototype.

  1. Consider using std::vector or std::array instead of C-style arrays.
\$\endgroup\$
1
  • \$\begingroup\$ Thanks you for your answer @Rish, really helpful, I posted a response in the question. I think to do all the suggestions you posted. \$\endgroup\$ Commented Jul 14, 2019 at 21:12
2
\$\begingroup\$

No time to read through all the code for now. So just some random observations.

In Coordinates.h:

Include as less as poissible in headers: You include iostream but it is only needed in Coordinates.cpp. Generally you should include as less as possible in header files. If you include iostream in the header every class which includes Coordinates.h also includes iostream even if it is not necessary. This leads to longer compilation times. Same for Actions.h

Follow the order public protected private in classes: You want to read the most important parts of a class at first. Normally this is the functions the user can use, not implementation specific details like private members.

Pass built in types by value: It is cheaper to copy an int than to pass it by const reference. So in youre setX method you should just pass it as int not const int&

Use const when possible: getX() does not modify its object so declare it const. You should always use const for functions who don't modify the object.

Don't declare a Copy Constructor on trivial classes: Youre class only has int data members. Therefore youre compiler will happily generate the copy constructor for you. Theres absolutely no need to declare it. Besides if you declare a copy constructor you should also declare the other classes of the rule of five: https://en.cppreference.com/w/cpp/language/rule_of_three. In the case of youre class its not necessary.

Be consistent with naming: I would not use capital letters for functions. So Draw() and DeleteDraw() should be draw() and deleteDraw().

Don't use namespace std: Especially not in a header file. You force youre user to import all the namespace std if they ever include youre header file. It really is very bad style to use namespace std. https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice

Use a namespace: You should wrap all youre functions and classes in its own namespace to prevent nameclashes. Otherwise if you write a programm with other libraries theres the danger nameclases happen. https://stackoverflow.com/questions/4211827/why-and-how-should-i-use-namespaces-in-c

Use a prefix for members:: This is a bit controversial but i would use a prefix m for member variables so they are clearly visible as member variables. It also elimnates the need for this-> in youe coordinates.cpp

Avoid unessary comments:: The comment in youre class Methods using a coordinate is just a bloat its pretty obvious that these methods do sth with Coordinates because they are in the Coordinates class.

Putting everything together we get this:

Coordinate.h

#ifndef COORDINATES_H
#define COORDINATES_H

#include "Drawable.h"

namespace tetris {

    class Coordinates : public Drawable
    {
    public:
        Coordinates(int x = 0, int y = 0);

        int getX() const;
        int getY() const;

        void setX(int x);
        void setY(int y);

        // Methods using a coordinate
        void moveCoordinatesInADirection(char direction);
        void draw() override;
        void deleteDraw() override;

    private:
        static constexpr char form{ '*' };

        int mX;
        int mY;
    };
}

#endif 

Coordinates.cpp

#include "Coordinates.h"

#include "Actions.h"

#include <iostream>

namespace tetris {

    Coordinates::Coordinates(int x, int y)
        :mX{x}, mY{y}
    {
    }

    int Coordinates::getX() const
    {
        return mX;
    }

    int Coordinates::getY() const
    {
        return mY;
    }

    void Coordinates::setX(int x)
    {
        mX = x;
    }

    void Coordinates::setY(int y)
    {
        mY = y;
    }

    void Coordinates::moveCoordinatesInADirection(char direction)
    {
        switch (direction)
        {
        case Action::moveLEFT:
            mY--;
            break;
        case Action::moveRIGHT:
            mY++;
            break;
        case Action::moveDOWN:
            mX++;
            break;
        default:
            break;
        }
    }

    void Coordinates::draw()
    {
        MoveTo(mX + Drawable::startPositionX, mY + Drawable::startPositionY);
        std::cout << form;
    }

    void Coordinates::deleteDraw()
    {
        MoveTo(mX + Drawable::startPositionX, mY + Drawable::startPositionY);
        std::cout << " ";
    }
}

I will probaly add more to this answer later when I have time.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thanks again @Sandro4912 for taking your free time to help me improve even more the quality of the code and learn. I posted an response to your answer in the question. \$\endgroup\$ Commented Jul 14, 2019 at 21:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.