Tetris code for pieces and map

Question

I'm making a simple Tetris clone and would like to know how's my code for the map and pieces. I'm using x, y, x_before, y_before, t0, t1 to allow smooth movement.

colors.h

#ifndef TETRIS_COLORS_H
#define TETRIS_COLORS_H
typedef enum {
 DARK_CYAN, DARK_RED, DARK_BROWN, DARK_MAGENTA, 
 DARK_GRAY, DARK_GREEN, DARK_BLUE, WALL, EMPTY
} Color;
#endif

pieces.h

#ifndef TETRIS_PIECES_H
#define TETRIS_PIECES_H
#include "colors.h"
#include "map.h"
#include "definitions.h"
#define PIECE_COUNT 7
#define PIECE_ROWS 4
#define PIECE_COLUMNS 4
#define PIECE_POINTS (PIECE_ROWS * PIECE_COLUMNS)
#define PIECE_BLOCKS_SIZE 4
typedef struct {
 int n;
 int matrix[4][16];
 Color color;
 int x;
 int y;
 int x_before;
 int y_before;
 unsigned int t0;
 unsigned int t1;
} Piece;
void piece_rotate(Piece *piece);
void piece_rotate_backwards(Piece *piece);
void piece_random(Piece *dest);
void piece_new(Piece *piece);
void piece_draw(Map *map, Piece *piece);
int piece_valid_position(Map *map, Piece *piece);
#endif

pieces.c

#include "pieces.h"
#include <stdlib.h>
#include <string.h>
Piece i = {0, 
 0, 0, 0, 0,
 1, 1, 1, 1,
 0, 0, 0, 0,
 0, 0, 0, 0,
 0, 0, 1, 0,
 0, 0, 1, 0,
 0, 0, 1, 0,
 0, 0, 1, 0,
 0, 0, 0, 0,
 0, 0, 0, 0,
 1, 1, 1, 1,
 0, 0, 0, 0,
 0, 1, 0, 0,
 0, 1, 0, 0,
 0, 1, 0, 0,
 0, 1, 0, 0
, DARK_CYAN, 0, 0, 0, 0, 0, 0};
Piece j = {0,
 1, 0, 0, 0,
 1, 1, 1, 0,
 0, 0, 0, 0,
 0, 0, 0, 0,
 0, 1, 1, 0,
 0, 1, 0, 0,
 0, 1, 0, 0,
 0, 0, 0, 0,
 0, 0, 0, 0,
 1, 1, 1, 0,
 0, 0, 1, 0,
 0, 0, 0, 0,
 0, 1, 0, 0,
 0, 1, 0, 0,
 1, 1, 0, 0,
 0, 0, 0, 0
, DARK_RED, 0, 0, 0, 0, 0, 0};
Piece l = {0,
 0, 0, 1, 0,
 1, 1, 1, 0,
 0, 0, 0, 0,
 0, 0, 0, 0,
 0, 1, 0, 0,
 0, 1, 0, 0,
 0, 1, 1, 0,
 0, 0, 0, 0,
 0, 0, 0, 0,
 1, 1, 1, 0,
 1, 0, 0, 0,
 0, 0, 0, 0,
 1, 1, 0, 0,
 0, 1, 0, 0,
 0, 1, 0, 0,
 0, 0, 0, 0
, DARK_BROWN, 0, 0, 0, 0, 0, 0};
Piece o = {0,
 0, 1, 1, 0,
 0, 1, 1, 0,
 0, 0, 0, 0,
 0, 0, 0, 0,
 0, 1, 1, 0,
 0, 1, 1, 0,
 0, 0, 0, 0,
 0, 0, 0, 0,
 0, 1, 1, 0,
 0, 1, 1, 0,
 0, 0, 0, 0,
 0, 0, 0, 0,
 0, 1, 1, 0,
 0, 1, 1, 0,
 0, 0, 0, 0,
 0, 0, 0, 0
, DARK_MAGENTA, 0, 0, 0, 0, 0, 0};
Piece s = {0,
 0, 1, 1, 0,
 1, 1, 0, 0,
 0, 0, 0, 0,
 0, 0, 0, 0,
 0, 1, 0, 0,
 0, 1, 1, 0,
 0, 0, 1, 0,
 0, 0, 0, 0,
 0, 0, 0, 0,
 0, 1, 1, 0,
 1, 1, 0, 0,
 0, 0, 0, 0,
 1, 0, 0, 0,
 1, 1, 0, 0,
 0, 1, 0, 0,
 0, 0, 0, 0
, DARK_GRAY, 0, 0, 0, 0, 0, 0};
Piece t = {0,
 0, 1, 0, 0,
 1, 1, 1, 0,
 0, 0, 0, 0,
 0, 0, 0, 0,
 0, 1, 0, 0,
 0, 1, 1, 0,
 0, 1, 0, 0,
 0, 0, 0, 0,
 0, 0, 0, 0,
 1, 1, 1, 0,
 0, 1, 0, 0,
 0, 0, 0, 0,
 0, 1, 0, 0,
 1, 1, 0, 0,
 0, 1, 0, 0,
 0, 0, 0, 0
, DARK_GREEN, 0, 0, 0, 0, 0, 0};
Piece z = {0,
 1, 1, 0, 0,
 0, 1, 1, 0,
 0, 0, 0, 0,
 0, 0, 0, 0,
 0, 0, 1, 0,
 0, 1, 1, 0,
 0, 1, 0, 0,
 0, 0, 0, 0,
 0, 0, 0, 0,
 1, 1, 0, 0,
 0, 1, 1, 0,
 0, 0, 0, 0,
 0, 1, 0, 0,
 1, 1, 0, 0,
 1, 0, 0, 0,
 0, 0, 0, 0
, DARK_BLUE, 0, 0, 0, 0, 0, 0};
Piece *piece_list[PIECE_COUNT] = {&i, &j, &l, &o, &s, &t, &z};
void piece_rotate(Piece *piece)
{
 piece->n = ++piece->n % 4;
}
void piece_rotate_backwards(Piece *piece)
{
 piece->n = (--piece->n > -1) ? piece->n : 3;
}
void piece_random(Piece *dest)
{
 memcpy(dest, piece_list[rand() % PIECE_COUNT], sizeof(Piece)-sizeof(int) * 4);
}
void piece_new(Piece *piece)
{
 piece_random(piece);
 piece->x_before = piece->x = 1 + (GAME_COLUMNS - 2 - PIECE_BLOCKS_SIZE) / 2;
 piece->y_before = piece->y = 1;
 piece->t0 = piece->t1 = 0;
}
void piece_draw(Map *map, Piece *piece)
{
 for(int i = 0; i < PIECE_BLOCKS_SIZE * PIECE_BLOCKS_SIZE; ++i)
 if(piece->matrix[piece->n][i])
 map_draw_block( map, 
 piece->y + i / PIECE_BLOCKS_SIZE, 
 piece->x + i % PIECE_BLOCKS_SIZE, 
 piece->color );
}
int piece_valid_position(Map *map, Piece *piece)
{
 for(int i = 0; i < PIECE_BLOCKS_SIZE * PIECE_BLOCKS_SIZE; ++i)
 if(piece->matrix[piece->n][i])
 if(map_block_at( map, 
 piece->y + i / PIECE_BLOCKS_SIZE, 
 piece->x + i % PIECE_BLOCKS_SIZE )
 != EMPTY)
 return 0;
 return 1;
}

map.h

#ifndef TETRIS_MAP_H
#define TETRIS_MAP_H
#include "colors.h"
typedef struct Map {
 Color *blocks;
 int rows;
 int columns;
} Map;
Map *map_create(int rows, int columns);
void map_delete(Map *map);
void map_clear(Map *map);
void map_add_wall(Map *map);
Color map_block_at(Map *map, int row, int column);
void map_draw_block(Map *map, int row, int column, Color block);
void map_clear_row(Map *map, int row);
void move_rows_down(Map *map, int bottom_y);
#endif

map.c

#include "map.h"
#include <stdlib.h>
Map *map_create(int new_rows, int new_columns)
{
 Map *map = malloc(sizeof(Map) + sizeof(Color) * new_rows * new_columns);
 if(map == NULL)
 return NULL;
 map->blocks = (Color *)(map + 1);
 map->rows = new_rows;
 map->columns = new_columns;
 return map;
}
void map_delete(Map *map)
{
 free(map);
}
void map_clear(Map *map)
{
 for(int i = 0; i < map->rows * map->columns; ++i)
 map->blocks[i] = EMPTY;
}
void map_add_wall(Map *map)
{
 //Top
 for(int i = 0; i < map->columns; ++i)
 map->blocks[i] = WALL;
 //Bottom
 for(int i = 0; i < map->columns; ++i)
 map->blocks[(map->rows -1) * map->columns + i] = WALL;
 //Left and right
 for(int i = 1; i < map->rows; ++i){
 map->blocks[i * map->columns - 1] = WALL;
 map->blocks[i * map->columns] = WALL;
 }
}
Color map_block_at(Map *map, int row, int column)
{
 return map->blocks[row * map->columns + column];
}
void map_draw_block(Map *map, int row, int column, Color block)
{
 map->blocks[row * map->columns + column] = block;
}
void map_clear_row(Map *map, int row)
{
 for(int x = 1; x < map->columns - 1; ++x)
 map->blocks[row * map->columns + x] = EMPTY;
}
void move_rows_down(Map *map, int bottom_y)
{
 for(int y = bottom_y; y > 1; --y)
 for(int x = 1; x < map->columns - 1; ++x)
 map_draw_block(map, y, x, map_block_at(map, y - 1, x));
 map_clear_row(map, 1);
}

Answer 1

Overall this looks very nice. It's easy to read, and is very refined. A few notes:

• You can remove the Map from the first line of your structure.

  typedef struct
  {
   Color *blocks;
   int rows;
   int columns;
  } Map;
  

• Your function map_delete() only performs one action, freeing the memory of map. Unless you plan to expand on the map structure where you will have to free more members within the structure itself, I would get rid of it and just call free(map). If you have compiler optimizations on, I guarantee that your compiler is already doing that for you anyways, so it should slightly decrease compilation time.

• You can simpilfy your NULL tests.

  if(!map)
  

• This is more of a style issue, but since this recently led to the Apple goto security flaw, I'm going to cover it. I don't think you are writing your brace-free single statement loops and test conditionals correctly. This is completely up to you to decide since this is a style issue, but I prefer to do it like this:

  if(!map) return NULL;
  

  Other's may be more strict and tell you to use braces, but it's up to you.

• You need more comments. There are some lines in your code that I have trouble following. And if I have trouble following it, chances are that you will have trouble following it as well when you re-visit this project in a few months. Save yourself (and possibly others) the trouble and document your code.