The 2048 game using C

Question

I've decided to do something about my horrible coding and have thus made a test case for review. I'm not sure how this site works exactly, there are so many rules and they don't seem at all clear, not to me at least. Regardless, I'm going to try.

Also, the real point of this is to find out if I've done something horrible wrong, not if curly brackets should be on one line or the other, if camel case is good or bad, etc.

The test case is the game 2048, the smartest, prettiest, in other words, most perfect way I could think of doing it.

libtk.h

#ifndef LIBTK_H
#define LIBTK_H

enum direction { UP, LEFT, DOWN, RIGHT };

void tkNewGame( unsigned int width, unsigned int height);
void tkExit(void);
// Return zero if game over.
unsigned int tkMove( enum direction );
// Returns array of size width*height.
// Format: top left to bottom right.
void tkGetBoard( unsigned char * );
void tkPrintBoard( void );
unsigned int tkGetScore( void );
unsigned int tkGetMoves( void );

#endif

libth.c

#include "libtk.h"
#include <stdlib.h>
#include <time.h>
#include <stdio.h>

unsigned int init = 1, score = 0, moves = 0, width = 0, height = 0, state = 15;
unsigned char **board = NULL;

void tkNewGame( unsigned int w, unsigned int h )
{
    // Check if grid dimensions be proper.
    if( w < 2 || h < 2 )
    {
        printf( "board dimension must be at least 2x2.\n" );
        return;
    }
    if( width != w || height != h )
    {
        width = w;
        height = h;
        // Seed the random number generator, once only.
        if( init && init-- ) srand( time( NULL ) );

        // Free memory if needed.
        if( board != NULL )
        {
            free( board[ 0 ] );
            free( board );
            board = NULL;
        }

        // Allocate memory
        if( ( board = malloc( 2*w*h*sizeof( char * ) ) ) != NULL &&
            ( board[0] = calloc( w*h, sizeof( char ) ) ) != NULL )
        {
            // Link tiles
            for( int i = 0; i < w*h; i++ )
            {
                board[ 2*w*h -1 -i ] = board[ i ] = board[ 0 ] +i;
            }
        }
        else
        {
            free( board );
            board = NULL;
            printf( "Insufficient memory.\n" );
            return;
        }
    }
    else
    {
        // Zero the board.
        for( int i = 0; i < w*h; i++ )
        {
            *board[ i ] = 0;
        }
    }
    // generate random tiles and reset variables.
    int r[ 2 ] = { rand()%( w*h ), rand()%( w*h ) };
    while( r[ 0 ] == r[ 1 ] ) r[ 1 ] = rand()%( w*h );
    *board[ r[ 0 ] ] = ( rand()%10 != 0 ) ? ( 1 ) : ( 2 );
    *board[ r[ 1 ] ] = ( rand()%10 != 0 ) ? ( 1 ) : ( 2 );
    score = moves = 0;
    state = 15;
}

void tkExit( void )
{
    free( board[ 0 ] );
    free( board );
    board = NULL;
}

void tkGetBoard( unsigned char *arr )
{
    // Check is memory is allocated.
    if(board != NULL)
    {
        // iterate and copy relevant data.
        for( int i = 0; i < width*height; i++)
        {
            arr[ i ] = *board[ i ];
        }
    }
}

void tkPrintBoard( void )
{
    for( int y = 0;  y < height; y++ )
    {
        printf( "|" );
        for( int x = 0; x < width; x++ )
        {
            printf( "%02x|", *board[ y*width +x ] );
        }
        printf( "\n" );
    }
}

unsigned int tkGetScore()
{
    return score;
}

unsigned int tkGetMoves()
{
    return moves;
}

int collapse( unsigned char **arr, enum direction dir )
{
    // Keep track of change.
    int change = 0;
    // Eliminate unwanted zeros and collapse rows/columns accordingly.
    switch( dir )
    {
    case LEFT:
    case RIGHT:
        for( int y = 0; y < height; y++ )
        {
            for( int x = width -2; x >= 0; x-- )
            {
                if( *arr[ y*width +x ] == 0 )
                {
                    for( int i = x; i < width-1; i++ )
                    {
                        *arr[ y*width +i ] = *arr[ y*width +i +1 ];
                        *arr[ y*width +i +1 ] = 0;
                        change = 1;
                    }
                }
            }

        }
        break;
    case UP:
    case DOWN:
        for( int x = 0; x < width; x++ )
        {
            for( int y = height -2; y >= 0; y-- )
            {
                if( *arr[ y*width +x ] == 0 )
                {
                    for( int i = y; i < height -1; i++ )
                    {
                        *arr[ i*width +x ] = *arr[ i*width +x +width ];
                        *arr[ i*width +x +width ] = 0;
                        change = 1;
                    }
                }
            }

        }
    }
    return change;
}

unsigned int tkMove( enum direction dir )
{
    // Keep track of change.
    int change = 0;
    // Use rotated board if DOWN or RIGHT.
    unsigned char **arr = ( dir > 1 ) ? ( board + width*height ) :
                                        ( board );
    // Get rid of the zeros.
    change = collapse( arr, dir );
    // collapse pairs of values and increment.
    switch( dir )
    {
    case LEFT:
    case RIGHT:
        for( int y = 0; y < height; y++ )
        {
            for( int x = 0; width -1; x++ )
            {
                if( *arr[ y*width +x ] > 0 && *arr[ y*width +x ] ==
                                              *arr[ y*width +x +1 ] )
                {
                    ( *arr[ y*width +x ] )++;
                    *arr[ y*width +x +1 ] = 0;
                    change = 1;
                }
            }

        }
        break;
    case UP:
    case DOWN:
        for( int x = 0; x < width; x++ )
        {
            for( int y = 0; y < height -1; y++ )
            {
                if( *arr[ y*width +x ] > 0 && *arr[ y*width +x ] ==
                                              *arr[ ( y +1 )*width +x ] )
                {
                    ( *arr[ y*width +x ] )++;
                    *arr[ ( y +1 )*width +x ] = 0;
                    change = 1;
                }
            }
        }
    }
    // Get rid of the zeros.
    change = collapse( arr, dir );
    // Generate reasonable fair random tile, if change is true.
    if( change )
    {
        // Reset state for continued play.
        state = 15;
        // Count the number of empty tiles.
        int count = 0;
        for( int i = 0; i < width*height; i++ )
        {
            if( *arr[ i ] == 0 )
            {
                count++;
            }
        }
        // Generate a random number within that range.
        count = rand()%count;
        // Find the the position and generate a random tile.
        for( int i = 0; i < width*height; i++ )
        {
            if( *arr[ i ] == 0 && --count == 0 )
            {
                *arr[ i ] = ( rand()%10 != 0 ) ? ( 1 ) : ( 2 );
                break;
            }
        }
    }
    else
    {
        // Eliminate the direction in question from the state mask.
        state ^= 1 << ( unsigned int ) dir;
    }
    // Should return zero if game is over.
    return state;
}

Assuming that this isn't immediately thrown away with the trash, I thank whomever it may concern for both advice and time spend giving it.

In case you don't know of the game 2048, it can be explored here. Other than that, the most interesting thing about the code is that I in effect have two arrays of type char *, both linking to the same data, but in reverse order (180 degree rotation), which in turn have made it possible to reduce the code quite a bit. The fact that I've also made the board of variable size, rectangular but not necessarily square, has complicated matters somewhat in terms of memory management, but everything should work as intended.

Answer 1

Here, have some tips:

1. Put the game state into its own struct. Currently, your users can only play one game at a time. While this might be fine for your current use case, what happens if somebody says to himself: "Hey, I have a great idea! Let's build a server where people can play 2048!" and decides to use your code? They will not get very far because your current state consists of globals. Just put them together in a struct tkGameState (or whatever name you find fitting) and have your functions take a pointer to this struct as an argument. This is a very common pattern.
2. It might be nice to let your users know when something fails. Currently, tkNewGame returns void, but if w or h are less than 2, you just print an error message and return, doing no initialization whatsoever in the process. This is extremely dangerous as there is no effective way to check for the caller whether the function actually succeeded and whether they can carry on working with your functions. Furthermore, calling I/O functionality from functions whose purpose is something completely different violates the single responsibility principle (as the name suggests, the single responsibility principle states that each component of a system should only fulfill a single responsibility, which, in the case of tkNewGame, is to initialize the game state, not do error reporting). The most common pattern here is to have your function return a return code that indicates whether everything is fine or something happened and the initialization wasn't carried through properly.
3. if( init && init-- ) srand( time( NULL ) ); people usually don't expect some random function to just go ahead and call srand. This is something that should be left to anybody who uses your code (for small programs, calling srand in main is appropriate). However, when you make this change, you should also document somewhere that this function uses rand and that the generator must have been seeded before.
4. int is not a fitting type for an iteration variable in most cases (such as all of your loops). You know that the iterator can never be less than 0, and width, height happen to be unsigned int, so you're also running the risk of overflow. Use unsigned here (and when not sure, use size_t).
5. board[ 2*w*h -1 -i ] = board[ i ] = board[ 0 ] +i; split lines such as this one up. The general rule of thumb should be one "action" per line, and two assignments are seldom considered the same action. The same is true for loops: while( r[ 0 ] == r[ 1 ] ) r[ 1 ] = rand()%( w*h ); is hard to parse because most people would expect the loop body to be on its own line (and ideally surrounded by curly brackets).
6. state = 15; what does 15 mean here? 15 apples? 15 cabbages? 15 metres? The point is: Don't use magic numbers. Give names to your constants. Since in this special case you seem to care about the bits only, use hexadecimal literals and a comment explaining its meaning at the very least.
7. *board[ r[ 0 ] ] = ( rand()%10 != 0 ) ? ( 1 ) : ( 2 ); Why did you but parentheses around 1 and 2? Sometimes, paranoia can prove to be useful for security and avoiding evil bugs, but putting brackets around number literals is too much.
8. tkExit is a bad name for a function that does cleanup. By analogy to the exit function from the standard library, I would expect this function to terminate my program somehow, not just do a little cleanup. My preferred name would have been tkDestroy maybe, or tkCleanup, or...

---

These tips are just that - tips. There is a lot more to be said about your code, I'm sure, but I will leave that to someone with a little more time on their hands.

The biggest improvement you can do to your current code is, to my mind at least, putting those globals in their own struct. This will put a lot of people more at ease using your code and make your whole program more idiomatic.