Matrix struct with random values allocation and deallocation

I've written this code in 2013 or 2014, I think. I've rediscovered it today in one of my folders, made some changes and now I'm left (and my 2013/2014 me as well) wondering how good it looks for other C programmers.

It's a simple dummy program that reads N and M from stdin, then creates a matrix with those values as rows and columns with dynamic memory allocation, prints them, and then frees the memory. A rollback is made if malloc fails.

```
#include
#include
#include
#include

#define ERROR -1
#define SUCCESS 0

char welcome[] = "\tThis program \
gets the size of a matrix from stdin, messes around \
with it and then frees it.\n";

struct Matrix
{
int N;
int M;
int **values;
};

/**
* allocates memory for the matrix
*/
int allocateMatrix(struct Matrix *matrix, int n, int m)
{
printf("Allocating memory for matrix... (%d x %d)\n", n, m);
int i, j;

/ allocate rows /
matrix->values = malloc(n sizeof(int));

if (detectError(matrix->values)) {
return ERROR;
}

/ allocate columns /
for(i = 0; i N; i++)
{
matrix->values[i] = malloc(m * sizeof(int));
/ handles errors /
if (detectError(matrix->values[i])) {
/ frees already allocated memory /
j = i-1;
for(; j >= 0; j--)
{
free(matrix->values[i]);
}
free(matrix->values);
return ERROR;
}
}
printf("%lu bytes of memory allocated! \n", (n m sizeof(int)));
}

/**
* gets the properties of the matrix
*/
int getMatrixProps(struct Matrix *matrix)
{
int n, m;
printf("\nWhat will be the size of our matrix? (N x M)\n");
scanf("%d %d", &n, &m);
matrix->N = n;
matrix->M = m;
return allocateMatrix(&(*matrix), n, m);
}

/**
* frees the memory requested
*/
void freeMatrixMem(struct Matrix *myMatrix)
{
int i;
for(i = 0; i N; i++)
{
free(myMatrix->values[i]);
}
f

Improve your error reporting:

Your detectError function is not very useful. The name is also not good. It doesn't "detect errors", but rather it detects an out-of-memory condition if called after malloc.

int detectError(void *ptr) {
     if (ptr == NULL)
    {
            printf("operation failed\n");
            printf("%s\n", strerror(errno));
            return ERROR;
    }
    return SUCCESS;
}

If malloc fails, there is only one possible reason, it can't allocate any more memory. On a POSIX implementation if that happens malloc will set errno to ENOMEM. So by calling strerror you are just inventing a very complicated way of printing "out-of-memory" to stdout. Your code would be simpler and more idiomatic if instead of calling detectError you would just check the return of malloc for NULL:

matrix->values = malloc(n * sizeof(int*));
if (matrix->values == NULL) {
    fprintf(stderr, "Out-of-memory");
    return ERROR;
}

Also note that on some implementations malloc might not set errno at all or set it to EAGAIN, which would print an unclear or wrong message if using errno with strerror.

Still looking at the error reporting, it is more conventional to print error messages to stderr, so that users can filter normal program output from errors more easily.

Use booleans whenever possible instead of error codes:

#define ERROR -1
#define SUCCESS 0

I would not define those constants and instead use bool. If there are only two possible outcomes, success or failure, then just returning true or false is simpler and avoids confusion. It is very common for libraries to mix the use of integers as booleans and as error codes, which complicates things and leads to error if you misinterpret a return value. Keep it simple.

Awkward pointer manipulation:

From getMatrixProps, which already takes matrix by pointer:

return allocateMatrix(&(*matrix), n, m);

That dereference followed by taking the address is nonsensical. Pass it directly as is.

Allocate a flat array:

Lastly, you could simplify a lot if you'd allocate the whole matrix as a sequential memory block. Instead of having to call a malloc per column, you can allocate a flat array of columns.

Since we are dealing with a 2D matrix, in the following example I'll call
the columns width and rows height, using x and y as the respective indexes.

typedef struct {
    int width;
    int height;
    int * values;
} Matrix;

bool allocMatrix(Matrix * mat, int width, int height)
{
    // The total number of elements in the matrix is
    // the number of columns times the number of rows
    //
    mat->values = malloc(width * height * sizeof(int));
    if (mat->values == NULL) {
        fprintf(stderr, "Out-of-memory");
        return false;
    }

    mat->width  = width;
    mat->height = height;
    return true;
}

//
// To access a given 2D index, now that the matrix is flat, you'll need to 
// calculate the proper offsets with the formula: 'x + y * width'
//
// To simplify element access, I suggest defining a couple helper functions:
//

int getMatrixElement(const Matrix * mat, int x, int y)
{
    return mat->values[x + (y * mat->width)];
}

void setMatrixElement(Matrix * mat, int x, int y, int value)
{
    mat->values[x + (y * mat->width)] = value;
}

StackExchange Code Review Q#96537, answer score: 7