Simple Tetris game

Question

I made this simple Tetris game where I only have 2 blocks, the square and the line. Also the game ends after 20 blocks are created and the player wins. I was wondering how I can make it better overall.

Here's the code:

#include <stdio.h>
#include <windows.h>
#include <string.h>
#include <time.h>
#define RIGA 21
#define COLONNA 10
#define DESTRA 77
#define SINISTRA 75
#define GIU 80
#define SU 72
void GotoXY(int x, int y)
{
 COORD CursorPos = {x, y};
 HANDLE hConsole = GetStdHandle(STD_OUTPUT_HANDLE);
 SetConsoleCursorPosition(hConsole, CursorPos);
}
// FUNCTION PER RALLENTARE I MOVIMENTI
void ritardo()
{
 long double i;
 for(i=0;i<=(30000000);i++);
}
int i, j; // CONTATORI
int sposta, random; // VARIABILI BOOLEANE
int punti = 0; // VARIABILE PER ASSEGNAZIONE DEI PUNTI
int contatore_pezzi = 1; // VARIABILE PER CONTARE IL NUMERO DI OGGETTI CREATI
char griglia[21][10]; // DICHIARAZIONE ARRAY DI CHAR
char pezzo[2][2] = {{'X', 'X'}, // DICHIARAZIONE CUBO
 {'X', 'X'}};
char linea[3][1] = {{'X'},
 {'X'},
 {'X'}}; // DICHIARAZIONE LINEA
char linea_or[1][3] = { {'X'},{'X'},{'X'}};
void caricaArray() // INIZIALIZZA L'ARRAY VUOTO CON CICLI FOR INNESTATI
{
 for (i = 0; i < RIGA; i++)
 {
 for (j = 0; j < COLONNA; j++)
 {
 griglia[i][j] = ' ';
 }
 }
}
// PROTOTIPI FUCTION
void movimenti();
void assegnaPezzo();
void eliminaRiga();
void stampaBordi();
int main()
{
 caricaArray();
 movimenti();
 printf("\n\n");
 return 0;
}
void visualizzaArray() // VISUALIZZA L'ARRAY ALLO STATO ATTUALE
{
 printf("\n");
 for (i = 0; i < RIGA; i++)
 {
 printf(" "); //Spazio per visualizzare tutti i punti all'interno del bordo
 for (j = 0; j < COLONNA; j++)
 {
 printf("%c ", griglia[i][j]);
 }
 printf("\n");
 }
 stampaBordi();
}
void movimenti() // GESTISCE TUTTI I POSSIBILI MOVIMENTI DELLA FORMA
{
 do {
 eliminaRiga(); // Ad ogni ciclo verifica la presenza di righe piene
 int r = 0;
 int c = 4;
 srand(time(NULL)); // Random
 random = rand() % 2;
 assegnaPezzo();
 visualizzaArray();
 do {
 GotoXY(r,c); // Inizia a scorrere verso il basso
 r++;
 system("cls");
 if (random == 0) { // Se l'oggetto creato e' il cubo
 griglia[r-1][c] = ' '; // Libera le posizioni precedenti
 griglia[r-1][c+1] = ' ';
 griglia[r][c] = pezzo[0][0]; // Riassegna l'oggetto nella nuova posizione
 griglia[r][c+1] = pezzo[0][1];
 griglia[r+1][c] = pezzo[1][0];
 griglia[r+1][c+1] = pezzo[1][1];
 }
 if (random == 1) { // Se l'oggetto creato e' la linea
 if ( !kbhit() ) // Se non si premono tasti
 {
 griglia[r-1][c] = ' '; // Libera le posizioni precedenti
 griglia[r][c] = linea[0][0]; // Riassegna l'oggetto nella nuova posizione
 griglia[r+1][c] = linea [1][0];
 griglia[r+2][c] = linea [2][0];
 }
 }
 visualizzaArray();
 ritardo();
 if ( kbhit() ) // Se durante lo scroll verticale si preme un tasto
 {
 sposta = getch(); // Prende in input un tasto e lo assegna alla variabile sposta
 if (sposta==SU) // E se e' la freccia SU
 {
 griglia[r-1][c] = ' ';
 griglia[r][c-1] = linea_or[0][0];
 griglia[r][c] = linea_or[1][0];
 griglia[r][c+1] = linea_or[2][0];
 }
 if (sposta == DESTRA ) // E se e' la freccia DESTRA
 {
 c++; // L'oggetto si sposta a destra
 system("cls");
 if (random == 0) {
 if (c+1>COLONNA-1) { break; } // Se supera il bordo destro
 griglia[r][c-1] = ' '; // Libera le posizioni precedenti
 griglia[r+1][c-1] = ' ';
 }
 else if (random == 1) {
 griglia[r-2][c-1] = ' '; // Libera le posizioni precedenti
 griglia[r-1][c-1] = ' ';
 griglia[r][c-1] = ' ';
 griglia[r+1][c-1] = ' ';
 griglia[r+2][c-1] = ' ';
 }
 visualizzaArray(); //visualizza il tutto
 }
 if (sposta == SINISTRA ) // E se e' la freccia SINISTRA
 {
 c--; // L'oggetto si sposta a sinistra
 if (c<0) { break; } // Se supera il bordo sinistro
 system("cls");
 if (random == 0) {
 griglia[r][c+2] = ' '; // Libera le posizioni precedenti
 griglia[r+1][c+2] = ' ';
 }
 else if (random == 1) {
 griglia[r-1][c+2] = ' '; // Libera le posizioni precedenti
 griglia[r-1][c+1] = ' ';
 griglia[r][c+1] = ' ';
 griglia[r+1][c+1] = ' ';
 griglia[r+2][c+1] = ' ';
 }
 visualizzaArray();
 }
 }
 if (random == 0) {
 if (griglia[r+2][c] == 'X' || griglia[r+2][c+1] == 'X') // Se il pezzo tocca un altro pezzo inferiormente
 {
 if (griglia[0][c] == 'X' || griglia[1][c] == 'X') // Se lo spazio superiore e' troppo piccolo
 {
 system("cls");
 printf("HAI PERSO!\nPremi ESC per uscire.");
 break;
 }
 contatore_pezzi++;
 return movimenti(); //Allora attiva la ricorsivita'
 }
 }
 if (random == 1) {
 if (griglia[r+3][c] == 'X') {
 if (griglia[0][c] == 'X' || griglia[1][c] == 'X')
 {
 system("cls");
 printf("HAI PERSO!\nPremi ESC per uscire.");
 break;
 }
 contatore_pezzi++;
 return movimenti();
 }
 }
 if (random == 0) {
 if (r+1==20) { // Se tocca il borso inferiore crea un altro oggetto
 contatore_pezzi++;
 return movimenti(); }
 }
 else if (random == 1) {
 if (r+2==20) { // Se tocca il borso inferiore crea un altro oggetto
 contatore_pezzi++;
 return movimenti(); }
 }
 } while ( sposta != 27 );
 } while ( getch() != 27 );
}
void stampaBordi() // STAMPA LA CORNICE DEL CAMPO DI GIOCO
{
 for(i=1;i<=19;i++)
 {
 GotoXY(i,22); putch('-');
 GotoXY(i,0); putch('-');
 }
 printf(" PUNTI:%d - PEZZO No %d", punti, contatore_pezzi);
 for (j=1;j<22; j++) {
 GotoXY(0,j); putch('|');
 GotoXY(20,j); putch('|');
 }
}
void assegnaPezzo()
{
 int i1 = 0;
 int j1 = 4;
 if (random==0) {
 griglia[i1][j1] = pezzo[0][0]; // Assegna il pezzo nella posizione iniziale
 griglia[i1][j1+1] = pezzo[0][1];
 griglia[i1+1][j1] = pezzo[1][0];
 griglia[i1+1][j1+1] = pezzo[1][1];
 }
 if (random==1) {
 griglia[i1][j1] = linea[0][0];
 griglia[i1+1][j1] = linea [1][0];
 griglia[i1+2][j1] = linea [2][0];
 }
}
void eliminaRiga() // ELIMINA UNA O PIU' RIGHE, SE PIENE
{
 int k;
 int cont_riga;
 int colonna;
 char aus[COLONNA];
 for (cont_riga=0; cont_riga<=20; cont_riga++)
 {
 if (griglia[cont_riga][0] == 'X' && griglia[cont_riga][1] == 'X' && griglia[cont_riga][2] == 'X' && griglia[cont_riga][3] == 'X' && griglia[cont_riga][4] == 'X' && griglia[cont_riga][5] == 'X' && griglia[cont_riga][6] == 'X' && griglia[cont_riga][7] == 'X' && griglia[20][8] == 'X' && griglia[20][9] == 'X')
 {
 for (colonna=0; colonna<10; colonna++) {
 aus[colonna] = griglia[cont_riga-1][colonna];
 }
 for (colonna=0; colonna<10; colonna++) {
 griglia[cont_riga][colonna] = aus[colonna];
 }
 for (colonna=0; colonna<10; colonna++) {
 griglia[cont_riga-1][colonna] = '.';
 }
 punti++;
 for (k=2; k<11; k++) {
 if (griglia[cont_riga-k][0] == 'X' || griglia[cont_riga-k][1] == 'X' || griglia[cont_riga-k][2] == 'X' || griglia[cont_riga-k][3] == 'X' || griglia[cont_riga-k][4] == 'X' || griglia[cont_riga-k][5] == 'X' || griglia[cont_riga-k][6] == 'X' || griglia[cont_riga-k][7] == 'X' || griglia[cont_riga-k][8] == 'X' || griglia[cont_riga-k][9] == 'X' )
 {
 for (colonna=0; colonna<10; colonna++) {
 aus[colonna] = griglia[cont_riga-k][colonna];
 }
 for (colonna=0; colonna<10; colonna++) {
 griglia[cont_riga-k+1][colonna] = aus[colonna];
 }
 for (colonna=0; colonna<10; colonna++) {
 griglia[cont_riga-k][colonna] = '.';
 }
 }
 }
 system("cls");
 visualizzaArray();
 }
 }
}

Answer 1

• Always write source code and comments in English (I'm not a native English speaker either). You will sooner or later need to share code with other people around the world, like you just did here and now. Therefore it needs to be in English.

  More importantly still, learning two technical programming terms for everything when you only need to learn one is madness. The worst mistake I did when studying engineering back at university was to read books translated to my own language, rather than English. This meant that I had to learn twice the amount of technical terms: the real, correct term in English, and the hogwash term questionably translated to my own language. The latter is completely useless knowledge - in the real world outside school, English terms are used.

• As pointed out in another review, the ritardo busy-delay should have been replaced by Windows API Sleep in this case. In more advanced Windows programming with threads and events, you wouldn't use Sleep either but WaitForSingleObject etc. Multi-threading is a somewhat advanced topic, but one you'll eventually need to pick up. Then this program would use one thread for the graphics, one for input and maybe a third for calculations. Rather than using old MS DOS style kbhit - DOS didn't have threads.

• If you were to implement a busy-delay loop, the loop iterator i would need to be declared volatile. Otherwise the compiler is just going to optimize away the whole loop.

• Never use floating point numbers for loop iterators. It makes the code slower and the comparison expression might unexpectedly fail because of floating point inaccuracy.

• Always try to declare the loop iterator inside the for loop: for(int i=0; ....

• You need to get rid of all global variables. Plenty of info about why they are bad to be found on the net.

• Never use obsolete style empty parenthesis functions in C: void caricaArray(). This means that the function takes any parameter and this style might get removed from C at any point, since it is formally obsolete. Correct form is void caricaArray(void).

  (Not to be confused with C++ where empty parenthesis is fine and equivalent to (void).)

• As noted in another review, you need to decide a coding style and stick with it. Brace placement, indention etc. As it stands, the code is barely readable.

• srand(); should only be called once at the beginning of the program, you call it inside a loop.

• user712092: It is true that reducing scope where a variable lives makes it easier to understand code, but some variables are global by nature. Having goal to get rid of all globals is nonsense.
• Lundin: @user712092 As usual, it depends on what you mean with "global". Internal linkage file scope variables are fine. External linkage spaghetti across multiple files is not fine.
• TheRubberDuck: @user712092 I believe that, especially as a program grows large, the likelihood of a variable (rather than just a constant) being truly global vanishes. Drawing clear lines around what data is needed where is good for code quality, and the same goes for loosening coupling by programming to abstractions (like parameters rather than closed-over globals). Of course, you can always go too far and it's a situational question to answer.

Answer 2

Some suggestions in no particular order:

1. Organize your header files.

   I like to group related things together. In this case, "standard" versus "OS specific". I also default to sorting things alphabetically/asciibetically if there is no reason to do otherwise.

2. Group related things: functions with functions, variables with variables. Don't mix functions and global variables randomly.

3. Don't make variables global unless you must. Your i and j should be local variables.

4. Tetris lines have 4 blocks, not 3.

5. Use library functions where possible. Your ritardo() seems like a bad replacement for POSIX sleep() or Windows Sleep().

6. Use memset to initialize your griglia in caricaArray:

   memset(griglia, SPACE, sizeof(griglia));
   

7. Standard C requires the use of void to prototype a function taking no arguments. (C++ allows empty parens to mean no arguments.) So a line like:

   void movimenti();
   

   isn't really a prototype, but a declaration.

8. Turn on every possible warning. Your compiler has some settings, somewhere, that will enable "All warnings". It then probably has settings that will enable "Uncommon warnings" or "Extra warnings" or something. It might even have a setting for "Enable incredibly pedantic, ridiculous warnings". Turn them on. All of them.

9. The definition of main() should either be the very first, or the very last, function. If you favor "read my programs" then main is first, and functions will be appended to the end as they are used (main calls A, A calls B, etc.) If you favor "avoid forward declarations" then you try to define every function before it gets called. So lowest-level functions go at the top of the file, then higher, and finally main() at the end. If you favor "everything should be in alphabetical order" then you're a psychopath, but at least you know where things should be.

   This is a matter of style, so pick one and stay with it.

10. Don't mix terminal modes. If you're going to position the cursor and draw characters, then write your own print functions (to manage a scrolling window, if nothing else). Also, don't call cls to clear the screen. Just write your own function.

11. Write more functions

    You have some very large functions. There are lots of guidelines about what makes a good size for a function, but most of them concentrate on keeping functions visible on a single screen. (Some suggest very low numbers. You'd be surprised what you can do if you try to use a really small number!)

    As a suggestion, if you are doing the "same thing" in different places, that should be a function.

    If you are doing the same thing in different ways -- for example, drawing a square versus drawing a line -- then you probably need to sit and think about how you could treat them the same way. Perhaps you need a data structure to tell you what the sizes are? Or maybe you could use a pointer and a single integer to tell you what was being pointed to? An enum type?

12. Cache the results of kbhit(). That function checks whether a key has been hit. Which means that each time you call it, you're doing another check. In a loop, it's probably easier to call it once at the top of the loop, remember the result, and wait until the next pass through the loop to call it another time.

13. Pick a programming style and stay with it until the end of the program! I see three indentation styles in your program (all right next to each other!):

    if (sposta==SU) // E se e' la freccia SU
     {
     griglia[r-1][c] = ' ';
     griglia[r][c-1] = linea_or[0][0];
     griglia[r][c] = linea_or[1][0];
     griglia[r][c+1] = linea_or[2][0];
     }
    

    This is the "GNU Style." Don't use it unless you personally owe Richard Stallman money.

    if (sposta == DESTRA ) // E se e' la freccia DESTRA
    {
    

    This is the "Allman Style".

     c++; // L'oggetto si sposta a destra
     system("cls");
     if (random == 0) {
    

    This is the "One True Brace Style" or the "K & R Style" depending on how you treat functions.

     if (c+1>COLONNA-1) { break; } // Se supera il bordo destro
    

    This is the "Mandatory Braces variant," a.k.a. the "Banner Plane" style.

    So just pick one of GNU, Allman, K&R, or BannerPlane. My mistake, four styles.

14. Use named #defines or enum constants everywhere. You're off to a good start, but then you get bogged down with values like 'X' and 20. What's a 20?

There's probably more to say, but that seems like enough for now.

• user712092: 11] More functions is not always better. It creates so much more indirection.

Answer 3

It looks like pretty good procedural code overall.

Scope

int i, j; // CONTATORI

These indices can be local to functions. Globals are fine, but not in this case.

Also sposta is probably always local to movimenti() function.

Magic numbers

void stampaBordi() // STAMPA LA CORNICE DEL CAMPO DI GIOCO
{
 for(i=1;i<=19;i++)
 {
 GotoXY(i,22); putch('-');
 GotoXY(i,0); putch('-');

• I don't know what these numbers mean (19, 22), and You probably won't few days/months/years later
• what's i is columnIndex or numberOfIterations, or something like else?

Using structures

char griglia[21][10]; // DICHIARAZIONE ARRAY DI CHAR
...
void stampaBordi() // STAMPA LA CORNICE DEL CAMPO DI GIOCO
{
 for(i=1;i<=19;i++)
 {
 GotoXY(i,22); putch('-');
 GotoXY(i,0); putch('-');

These are all related to board. It is usually good idea to pull these variables together because it is easier to think about when related values are toghether (understanding / readability reason).

struct Grid {
 char cells[21][10];
 int rows;
 int columns;
};
Grid grid = {
 rows = 21;
 columns = 10;
}
...
void stampaBordi(Grid * grid) // STAMPA LA CORNICE DEL CAMPO DI GIOCO
{
 for(int i=1;i<= grid->rows - 2;i++)
 {
 GotoXY(i, grid->rows + 1 ); putch('-');
 GotoXY(i, 0 ); putch('-');

I am pretty sure that some of these constants should also be members of Grid structure :) ...

#define RIGA 21
#define COLONNA 10
#define DESTRA 77
#define SINISTRA 75
#define GIU 80
#define SU 72

Compressing code

Also in this case ...

if (sposta == DESTRA ) // E se e' la freccia DESTRA
 {
 c++; // L'oggetto si sposta a destra
 system("cls");
 if (random == 0) {
 if (c+1>COLONNA-1) { break; } // Se supera il bordo destro
 griglia[r][c-1] = ' '; // Libera le posizioni precedenti
 griglia[r+1][c-1] = ' ';
 }
 else if (random == 1) {
 griglia[r-2][c-1] = ' '; // Libera le posizioni precedenti
 griglia[r-1][c-1] = ' ';
 griglia[r][c-1] = ' ';
 griglia[r+1][c-1] = ' ';
 griglia[r+2][c-1] = ' ';
 }
 visualizzaArray(); //visualizza il tutto
 }

Pull out related information into structures to make it easier make new stuff and read.

// code like this repeats on multiple places
// and it seems like You are working with an array of offsets
// and You are doing single operation to them
 griglia[r-2][c-1] = ' '; // Libera le posizioni precedenti
 griglia[r-1][c-1] = ' ';
 griglia[r][c-1] = ' ';
 griglia[r+1][c-1] = ' ';
 griglia[r+2][c-1] = ' ';
// this can be compressed like this
struct Offset { int x; int y; }
Offset offsets = [{-2,-1},{-1,-1},{0,-1},{1,-1},{2,-1}];
int offsetCount = 5;
for(auto i = 0; i < offsetCount; i++) {
 auto offset = offsets[i];
 griglia[r + offset.x][c + offset.y] = ' ';
}
// and it can be pulled into function
clearOffsets( Offsets * offsets, int offsetCount )
// Also these offsets seem to be always asociated with a tetromino
// each tetromino has it's own offsets, so
struct Offset { int x; int y; }
struct Tetromino { Offset cellOffsets[]; int offsetCount; ... }
Tetromino l_shape = { cellOffsets = [{-2,-1},{-1,-1},{0,-1},{1,-1},{2,-1}], 5}
// now You can make functions like this
clearTetromino(l_shape);
rotateTetrominoLeft(l_shape);
// also You can reuse function to rotate another shape or shapes ...
rotateTetrominoLeft(box_shape); 
rotateTetrominoRight(shapes[shapeIndex]);

For more on code compression see excellent https://caseymuratori.com/blog_0015 .

Formatting

 else if (random == 1) {
 if (r+2==20) { // Se tocca il borso inferiore crea un altro oggetto

This is hard for me to quickly scan, because I am used to K&R brace style and Allman brace style (most people use them).

Comments

English would be nice in comments :)

I have this heuristic: I avoid comments that explain what a variable or function means, I make better (longer, more descriptive) name for functions/variables instead. I comment stuff which is non-obvious ...

This one is ok

 if (r+2==20) { // Se tocca il borso inferiore crea un altro oggetto

but, again, maybe removing comment by introducing variable can clear things up (and You can re-use variable at multiple places)

auto touchesLongerEdge = r + 2 === 20;