Matrix struct with random values allocation and deallocation

Question

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 <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
#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 < matrix->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 < myMatrix->N; i++)
 {
 free(myMatrix->values[i]);
 }
 free(myMatrix->values);
}
/**
* populates and prints the matrix
*/
void dummyHandleMatrix(struct Matrix *myMatrix)
{
 srand(time(NULL)); /* seeds random function */
 int n, m;
 printf("Populating matrix with random data... ");
 for(n = 0; n < myMatrix->N; n++)
 {
 for(m = 0; m < myMatrix->M; m++)
 {
 int value = rand() % 50;
 printf("%d ", value);
 myMatrix->values[n][m] = value;
 }
 }
 printf("\n");
 printf("Matrix populated. Values are:\n[");
 for(n = 0; n < myMatrix->N; n++)
 {
 for(m = 0; m < myMatrix->M; m++)
 {
 printf("%d, ", myMatrix->values[n][m]);
 }
 printf("\n");
 }
 printf("]\n");
}
int detectError(void *ptr) {
 if (ptr == NULL)
 {
 printf("operation failed\n");
 printf("%s\n", strerror(errno));
 return ERROR;
 }
 return SUCCESS;
}
int main(int argc, char *argv[])
{
 printf("%s", welcome);
 struct Matrix myMatrix;
 /* get size of the matrix and allocate memory for it */
 if (getMatrixProps(&myMatrix) == ERROR) {
 return ERROR;
 }
 /* populates it with random data and print to the screen */
 dummyHandleMatrix(&myMatrix);
 /* frees allocated memory */
 printf("Freeing matrix memory!\n");
 freeMatrixMem(&myMatrix);
 printf("Memory freed.\n");
}

• jacwah: Always compile your code with warnings turned on! As you can see in @JS1's answer, it can catch a lot of bugs for you. There are different ways to do this in different compilers, but a simple google search will find the way for the one you use.

Answer 1

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;
}

Answer 2

Warnings

I compiled your code and immediately got a bunch of warnings:

1. detectError() called without a prototype
2. %lu used with an unsigned int argument instead of unsigned long
3. time() called without a prototype
4. allocateMatrix() returns without returning an int

Miscellaneous things

1. It would be nice to typedef on struct Matrix to just Matrix so you don't have to use struct everywhere.

2. This code:

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

   could just be:

   return allocateMatrix(matrix, n, m);
   

3. In freeMatrixMem(), I would suggest putting this at the end of the function:

   myMatrix->values = NULL;
   

   This will help avoid problems where you reuse a stale pointer.

Answer 3

• I'm not sure if you need those macros. It's already known for C that 0 indicates success and that -1 is used for errors. As such, they wouldn't be considered magic numbers.

• welcome[] should be const as it's not meant to be modified.

• You can use puts() instead of printf() for unformatted output. It's not significant, but I've mentioned it in case you were unaware.

• You could just do if (!ptr) instead of if (ptr == NULL).

• There's some inconsistent indentation in detectError().

• glampert: The -1/0 error codes are somewhat common in a few libraries (posix comes to mind), but it is also very common for people to return ints with 1 for OK and 0 for error, using it like a boolean. My suggestion would be instead: use bool where there can only be two possible outcomes. An integer and a set of error constants otherwise. In the OP, a boolean from <stdbool.h> would be much better, IMHO.
• Jamal: @glampert: That's another good point. Based on the for loop usage, I assumed this was for an older C standard without access to <stdbool.h> (I think that's true). The omitted return 0 in main() confuses me further, though.
• jacwah: I think it's perfectly fine to use macros for return values. There's nothing wrong comparing to NULL either - this is personal style and either way is just as good imo.

Answer 4

In addition to what everyone else said, you could use calloc() instead of malloc() to allocate and clear the memory for the various pointers. Then, instead of having 2 places in your code where you might free memory, you can have just one. Something like this:

void freeMatrixMem(struct Matrix *myMatrix)
{
 if (myMatrix != NULL)
 {
 int i;
 for(i = 0; i < myMatrix->N; i++)
 {
 if (myMatrix->values[i] != NULL)
 {
 free(myMatrix->values[i]);
 }
 }
 free(myMatrix->values);
 }
}
int allocateMatrix(struct Matrix *matrix, int n, int m)
{
 int result = SUCCESS;
 printf("Allocating memory for matrix... (%d x %d)\n", n, m);
 int i, j; 
 /* allocate rows */
 matrix->values = calloc(n, sizeof(int*));
 result = detectError(matrix->values);
 /* allocate columns */
 for(i = 0; (i < matrix->N) && (result == SUCCESS); i++)
 {
 matrix->values[i] = calloc(m, sizeof(int));
 /* handles errors */
 result = detectError(matrix->values[i]);
 }
 if (result == SUCCESS)
 {
 printf("%lu bytes of memory allocated! \n", (n * m * sizeof(int)));
 }
 else
 {
 freeMatrix(matrix);
 }
 return result;
}

I'd also recommend moving the call to allocateMatrix() out of getMatrixProps(). Getting the properties of the matrix from the user should be separate from actually allocating it.

Edit: I forgot to actually free the matrix when there's an error. I've updated the code to include that. (I was tired when I wrote this last night!)